feat(chat): merge Human and Chat pages into unified conversation surface#2227
feat(chat): merge Human and Chat pages into unified conversation surface#2227graycyrus wants to merge 12 commits into
Conversation
…ace (tinyhumansai#1520) Move the mascot + voice interaction from a standalone /human route into a selectable mode within the /chat (Accounts) page. The mascot appears as a rail button alongside Agent and connected providers, eliminating tab-bar clutter and unifying thread management across voice and text modes. - Add MascotChatPane component (mascot stage + Conversations sidebar) - Add mascot entry to Accounts rail with MASCOT_ACCOUNT_ID sentinel - Migrate speakReplies from localStorage to Redux (mascotSlice) - Remove /human route (redirect to /chat) and Human tab from bottom nav - Exclude mascot mode from fullscreen in accountsFullscreen.ts - Add unit tests for MascotChatPane and speakReplies slice Closes tinyhumansai#1520
The i18n coverage CI check requires keys in en.ts to also exist in the chunked locale files. Adds translated accounts.human to all 11 chunk-3 files.
…tsFullscreen Cover the mascot selection branch in Accounts.tsx (isMascotSelected, selectMascot, MascotChatPane render) and the MASCOT_ACCOUNT_ID export + fullscreen exclusion in accountsFullscreen.ts to satisfy the ≥80% diff-cover gate.
…inyhumansai#1520) Replace the separate mascot rail button approach with a mic toggle in the Conversations composer. Clicking it switches to MicComposer (cloud STT) — the same component the old /human page used — showing a centered mascot (180px), mic button, and speak-replies toggle. The device selector is handled by the system, keeping the UI clean. - Revert Accounts.tsx and accountsFullscreen.ts to pre-tinyhumansai#1520 state - Delete MascotChatPane (no longer needed) - Add composerOverride state to toggle between text and mic-cloud mode - Center mascot as hero focal point in voice mode - Wire speakReplies from Redux into voice mode controls Closes tinyhumansai#1520
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR consolidates the /human mascot/voice route into /chat by adding mascot voice-mode state to Redux (persisted), removing the /human tab/route, integrating YellowMascot and voice controls into Conversations, refining MicComposer, adding i18n keys, updating tests, and documenting persistence/migration behavior. ChangesMascot Mode in Chat
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.claude/memory.md (1)
187-187: ⚡ Quick winMove this entry to the "Build & Tooling Gotchas" section.
The worktree native dependencies issue is a general pnpm/tooling concern, not specific to mascot functionality. It would be more discoverable and appropriately categorized in the existing "Build & Tooling Gotchas" section (line 147), alongside similar environment and build issues.
📝 Suggested reorganization
Remove from line 187 and add to the "Build & Tooling Gotchas" section:
## Build & Tooling Gotchas - **`pnpm typecheck` script was renamed** — Check `app/package.json` for the current name; as of issue `#830` work, use `pnpm workspace openhuman-app compile` for tsc checks. - **PR `#745` (command palette) merged without its deps** — `@radix-ui/react-dialog`, `cmdk`, and `@testing-library/user-event` are missing from `package.json`. Install them if tsc fails after syncing main. - **Pre-push hooks fail on upstream lint warnings** — ESLint warns on `setState` in effects and unused `eslint-disable` directives inherited from upstream. Use `--no-verify` only when the lint errors are pre-existing upstream issues, not new code. ++ **Worktree native deps issue** — pnpm worktrees may lack platform-specific native bindings (e.g. `@rolldown/binding-darwin-arm64`). Workaround: run tests from the main repo directory (shares git objects), or `rm -rf node_modules && pnpm install` in the worktree.Then remove the redundant line from the "Mascot Redux & Persistence" section:
## Mascot Redux & Persistence -- **Worktree native deps issue** — pnpm worktrees may lack platform-specific native bindings (e.g. `@rolldown/binding-darwin-arm64`). Workaround: run tests from the main repo directory (shares git objects), or `rm -rf node_modules && pnpm install` in the worktree. - **`accountsFullscreen.ts` sentinel exclusion** — Any new sentinel account ID (like `MASCOT_ACCOUNT_ID`) must be excluded from fullscreen mode in `accountsFullscreen.ts`, same as `AGENT_ACCOUNT_ID`. Otherwise the bottom tab bar hides during those views.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/memory.md at line 187, Move the "Worktree native deps issue" bullet out of the "Mascot Redux & Persistence" section and add it to the "Build & Tooling Gotchas" section: remove the existing line "- **Worktree native deps issue** — pnpm worktrees may lack platform-specific native bindings (e.g. `@rolldown/binding-darwin-arm64`). Workaround: run tests from the main repo directory (shares git objects), or `rm -rf node_modules && pnpm install` in the worktree." from where it currently appears and insert the same bullet under the "Build & Tooling Gotchas" heading so it sits with other build/tooling notes; ensure no duplicate remains in "Mascot Redux & Persistence" and keep the original formatting and wording when moving.app/src/store/__tests__/mascotSlice.test.ts (1)
149-210: ⚡ Quick winAdd parity tests for
voiceModeActivepersistence behavior.You added thorough
speakRepliestests, but the newly persistedvoiceModeActivepath still lacks default/reducer/reset/REHYDRATE coverage. Adding those cases here would close the regression gap for mode restoration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/store/__tests__/mascotSlice.test.ts` around lines 149 - 210, Add a parallel test block that mirrors the speakReplies suite but for voiceModeActive: use reducer, setVoiceModeActive, selectVoiceModeActive and resetUserScopedState to assert the default value, toggling false/true via setVoiceModeActive, resetUserScopedState resets to default, and a REHYDRATE block (use the REHYDRATE action helper) that restores persisted voiceModeActive true/false, defaults when missing, and migrates legacy localStorage keys ('human.voiceModeActive' = '0'/'1') to false/true while removing that localStorage key after migration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.claude/memory.md:
- Line 187: Move the "Worktree native deps issue" bullet out of the "Mascot
Redux & Persistence" section and add it to the "Build & Tooling Gotchas"
section: remove the existing line "- **Worktree native deps issue** — pnpm
worktrees may lack platform-specific native bindings (e.g.
`@rolldown/binding-darwin-arm64`). Workaround: run tests from the main repo
directory (shares git objects), or `rm -rf node_modules && pnpm install` in the
worktree." from where it currently appears and insert the same bullet under the
"Build & Tooling Gotchas" heading so it sits with other build/tooling notes;
ensure no duplicate remains in "Mascot Redux & Persistence" and keep the
original formatting and wording when moving.
In `@app/src/store/__tests__/mascotSlice.test.ts`:
- Around line 149-210: Add a parallel test block that mirrors the speakReplies
suite but for voiceModeActive: use reducer, setVoiceModeActive,
selectVoiceModeActive and resetUserScopedState to assert the default value,
toggling false/true via setVoiceModeActive, resetUserScopedState resets to
default, and a REHYDRATE block (use the REHYDRATE action helper) that restores
persisted voiceModeActive true/false, defaults when missing, and migrates legacy
localStorage keys ('human.voiceModeActive' = '0'/'1') to false/true while
removing that localStorage key after migration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a10947f9-a924-4a59-a21b-86c3387da12d
📒 Files selected for processing (33)
.claude/memory.mdapp/src/AppRoutes.tsxapp/src/components/BottomTabBar.tsxapp/src/features/human/MicComposer.tsxapp/src/lib/i18n/chunks/ar-1.tsapp/src/lib/i18n/chunks/ar-2.tsapp/src/lib/i18n/chunks/bn-1.tsapp/src/lib/i18n/chunks/bn-2.tsapp/src/lib/i18n/chunks/en-1.tsapp/src/lib/i18n/chunks/en-2.tsapp/src/lib/i18n/chunks/es-1.tsapp/src/lib/i18n/chunks/es-2.tsapp/src/lib/i18n/chunks/fr-1.tsapp/src/lib/i18n/chunks/fr-2.tsapp/src/lib/i18n/chunks/hi-1.tsapp/src/lib/i18n/chunks/hi-2.tsapp/src/lib/i18n/chunks/id-1.tsapp/src/lib/i18n/chunks/id-2.tsapp/src/lib/i18n/chunks/it-1.tsapp/src/lib/i18n/chunks/it-2.tsapp/src/lib/i18n/chunks/pt-1.tsapp/src/lib/i18n/chunks/pt-2.tsapp/src/lib/i18n/chunks/ru-1.tsapp/src/lib/i18n/chunks/ru-2.tsapp/src/lib/i18n/chunks/zh-CN-1.tsapp/src/lib/i18n/chunks/zh-CN-2.tsapp/src/lib/i18n/en.tsapp/src/pages/Conversations.tsxapp/src/pages/__tests__/Conversations.render.test.tsxapp/src/store/__tests__/mascotSlice.test.tsapp/src/store/index.tsapp/src/store/mascotSlice.tsapp/test/e2e/specs/navigation.spec.ts
💤 Files with no reviewable changes (2)
- app/test/e2e/specs/navigation.spec.ts
- app/src/components/BottomTabBar.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/pages/__tests__/Conversations.render.test.tsx (1)
932-985: ⚡ Quick winConsider organizing voice mode tests in a separate describe block.
The three voice mode tests are well-structured and provide good coverage. However, they're currently mixed into the "smoke render (
#1123welcome-lock removal)" suite, while the tests address#1520(voice mode integration). Following the existing pattern (e.g., line 992 has a separate describe block for worker thread tests), consider extracting these into their own suite:+describe('Conversations — voice mode toggle (`#1520`)', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockGetThreads.mockResolvedValue({ threads: [], count: 0 }); + mockGetThreadMessages.mockResolvedValue({ messages: [], count: 0 }); + }); + it('renders a voice mode button in the text composer', async () => { ... }); ... +});This improves test organization and makes it easier to locate tests by feature area.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/pages/__tests__/Conversations.render.test.tsx` around lines 932 - 985, These three tests ("renders a voice mode button in the text composer", "clicking voice mode button shows the voice controls and mascot", "clicking Switch to text returns to text composer") should be moved out of the existing smoke suite into their own describe block (e.g., describe('voice mode integration', ...)) to mirror the existing pattern; wrap the three it(...) cases together, keeping their internal calls to renderConversations, emptyThreadState, socketState, act, fireEvent, and waitFor unchanged, and update any localized beforeEach/afterEach setup if needed so the new describe block has the same test fixtures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src/pages/__tests__/Conversations.render.test.tsx`:
- Around line 932-985: These three tests ("renders a voice mode button in the
text composer", "clicking voice mode button shows the voice controls and
mascot", "clicking Switch to text returns to text composer") should be moved out
of the existing smoke suite into their own describe block (e.g., describe('voice
mode integration', ...)) to mirror the existing pattern; wrap the three it(...)
cases together, keeping their internal calls to renderConversations,
emptyThreadState, socketState, act, fireEvent, and waitFor unchanged, and update
any localized beforeEach/afterEach setup if needed so the new describe block has
the same test fixtures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 608e1250-7ca4-4262-a545-03e583327f79
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
app/src/lib/i18n/chunks/ar-2.tsapp/src/lib/i18n/chunks/bn-2.tsapp/src/lib/i18n/chunks/en-2.tsapp/src/lib/i18n/chunks/es-2.tsapp/src/lib/i18n/chunks/fr-2.tsapp/src/lib/i18n/chunks/hi-2.tsapp/src/lib/i18n/chunks/id-2.tsapp/src/lib/i18n/chunks/it-2.tsapp/src/lib/i18n/chunks/pt-2.tsapp/src/lib/i18n/chunks/ru-2.tsapp/src/lib/i18n/chunks/zh-CN-2.tsapp/src/lib/i18n/en.tsapp/src/pages/Conversations.tsxapp/src/pages/__tests__/Conversations.render.test.tsx
✅ Files skipped from review due to trivial changes (11)
- app/src/lib/i18n/chunks/es-2.ts
- app/src/lib/i18n/chunks/it-2.ts
- app/src/lib/i18n/chunks/zh-CN-2.ts
- app/src/lib/i18n/chunks/hi-2.ts
- app/src/lib/i18n/chunks/en-2.ts
- app/src/lib/i18n/chunks/bn-2.ts
- app/src/lib/i18n/chunks/id-2.ts
- app/src/lib/i18n/chunks/ru-2.ts
- app/src/lib/i18n/chunks/pt-2.ts
- app/src/lib/i18n/en.ts
- app/src/lib/i18n/chunks/fr-2.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/pages/Conversations.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/features/human/MicComposer.test.tsx (1)
368-382: ⚡ Quick winUse
waitForfrom@testing-library/reactand combine assertions to match established pattern.The test now uses
vi.waitFor(line 380), which is inconsistent with the rest of the file wherewaitForfrom@testing-library/reactis used throughout (lines 118, 120, 133, etc.). More importantly, splitting the mock verification and DOM assertion into separate steps creates a potential race condition—the component may not have finished updating state by the time line 381 executes.The test at lines 360–365 demonstrates the correct pattern: wrap both the mock check and DOM query inside a single
waitForcallback to ensure they're evaluated together.♻️ Align with the established testing pattern
render(<MicComposer disabled={false} onSubmit={vi.fn()} showDeviceSelector />); - await vi.waitFor(() => expect(enumerateDevicesMock).toHaveBeenCalled()); - expect(screen.queryByRole('combobox', { name: /microphone device/i })).not.toBeInTheDocument(); + await waitFor(() => { + expect(enumerateDevicesMock).toHaveBeenCalled(); + expect( + screen.queryByRole('combobox', { name: /microphone device/i }) + ).not.toBeInTheDocument(); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/features/human/MicComposer.test.tsx` around lines 368 - 382, The test uses vi.waitFor and splits the mock verification and DOM assertion, risking a race; replace vi.waitFor with the waitFor imported from `@testing-library/react` and wrap both checks together: inside a single waitFor call assert that enumerateDevicesMock has been called and that screen.queryByRole('combobox', { name: /microphone device/i }) is not in the document, keeping the render(<MicComposer ... />) and the enumerateDevicesMock definition as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src/features/human/MicComposer.test.tsx`:
- Around line 368-382: The test uses vi.waitFor and splits the mock verification
and DOM assertion, risking a race; replace vi.waitFor with the waitFor imported
from `@testing-library/react` and wrap both checks together: inside a single
waitFor call assert that enumerateDevicesMock has been called and that
screen.queryByRole('combobox', { name: /microphone device/i }) is not in the
document, keeping the render(<MicComposer ... />) and the enumerateDevicesMock
definition as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9346209-228a-4043-b540-49287cf3af08
📒 Files selected for processing (1)
app/src/features/human/MicComposer.test.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/pages/__tests__/Conversations.render.test.tsx (3)
1218-1224: ⚡ Quick winPrefer the simpler store-capture pattern used elsewhere in this file.
The IIFE wrapping
renderConversationsadds cognitive overhead compared to the pattern used throughout this file (e.g., lines 260–269, 430–440):let store: ReturnType<typeof buildStore> | undefined; await act(async () => { store = await renderConversations({ thread: emptyThreadState }); }); // ... later: store!.getState()Consider refactoring for consistency.
♻️ Suggested refactor
- const store = await (async () => { - let s: ReturnType<typeof buildStore>; - await act(async () => { - s = await renderConversations({ thread: emptyThreadState }); - }); - return s!; - })(); + let store: ReturnType<typeof buildStore> | undefined; + await act(async () => { + store = await renderConversations({ thread: emptyThreadState }); + }); const micBtn = screen.getByRole('button', { name: /voice mode/i }); await act(async () => { fireEvent.click(micBtn); }); - expect(store.getState().mascot.voiceModeActive).toBe(true); + expect(store!.getState().mascot.voiceModeActive).toBe(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/pages/__tests__/Conversations.render.test.tsx` around lines 1218 - 1224, Replace the IIFE used to capture the store with the simpler pattern used elsewhere: declare let store: ReturnType<typeof buildStore> | undefined; then call await act(async () => { store = await renderConversations({ thread: emptyThreadState }); }); and later use store! where needed. Target the block that currently awaits an immediately-invoked async function around renderConversations and replace it to use the top-level store variable, keeping the same use of act, renderConversations, buildStore, and emptyThreadState.
1236-1252: ⚡ Quick winSame IIFE pattern; apply the same refactor.
The IIFE pattern here should be simplified to match the existing pattern (see comment on lines 1218–1224).
♻️ Suggested refactor
- const store = await (async () => { - let s: ReturnType<typeof buildStore>; - await act(async () => { - s = await renderConversations({ - thread: emptyThreadState, - mascot: { - color: 'yellow', - speakReplies: true, - voiceModeActive: true, - voiceId: null, - voiceGender: 'female', - voiceUseLocaleDefault: false, - selectedMascotId: null, - }, - }); - }); - return s!; - })(); + let store: ReturnType<typeof buildStore> | undefined; + await act(async () => { + store = await renderConversations({ + thread: emptyThreadState, + mascot: { + color: 'yellow', + speakReplies: true, + voiceModeActive: true, + voiceId: null, + voiceGender: 'female', + voiceUseLocaleDefault: false, + selectedMascotId: null, + }, + }); + }); const backBtn = screen.getByTitle(/switch to text/i); await act(async () => { fireEvent.click(backBtn); }); - expect(store.getState().mascot.voiceModeActive).toBe(false); + expect(store!.getState().mascot.voiceModeActive).toBe(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/pages/__tests__/Conversations.render.test.tsx` around lines 1236 - 1252, Replace the unnecessary immediately-invoked async function with the same simpler pattern used earlier: declare s: ReturnType<typeof buildStore> before the act call, then call await act(async () => { s = await renderConversations({ thread: emptyThreadState, mascot: { ... } }); }); and finally return s!; update references to buildStore, renderConversations, emptyThreadState and act accordingly so the IIFE is removed and the code matches the pattern at lines ~1218–1224.
932-940: 💤 Low valueMinor: duplicate test coverage.
This test duplicates lines 1184–1191 (both assert the same "Voice mode" button renders). Consider removing this test since the dedicated voice-mode describe block already provides comprehensive coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/pages/__tests__/Conversations.render.test.tsx` around lines 932 - 940, The test "renders a voice mode button in the text composer" duplicates existing coverage (see the voice-mode describe block) and should be removed to avoid redundant tests; delete or skip the it block named 'renders a voice mode button in the text composer' that calls renderConversations({ thread: emptyThreadState, socket: socketState('connected') }) and asserts screen.getByRole('button', { name: 'Voice mode' }) so the dedicated voice-mode tests remain the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src/pages/__tests__/Conversations.render.test.tsx`:
- Around line 1218-1224: Replace the IIFE used to capture the store with the
simpler pattern used elsewhere: declare let store: ReturnType<typeof buildStore>
| undefined; then call await act(async () => { store = await
renderConversations({ thread: emptyThreadState }); }); and later use store!
where needed. Target the block that currently awaits an immediately-invoked
async function around renderConversations and replace it to use the top-level
store variable, keeping the same use of act, renderConversations, buildStore,
and emptyThreadState.
- Around line 1236-1252: Replace the unnecessary immediately-invoked async
function with the same simpler pattern used earlier: declare s:
ReturnType<typeof buildStore> before the act call, then call await act(async ()
=> { s = await renderConversations({ thread: emptyThreadState, mascot: { ... }
}); }); and finally return s!; update references to buildStore,
renderConversations, emptyThreadState and act accordingly so the IIFE is removed
and the code matches the pattern at lines ~1218–1224.
- Around line 932-940: The test "renders a voice mode button in the text
composer" duplicates existing coverage (see the voice-mode describe block) and
should be removed to avoid redundant tests; delete or skip the it block named
'renders a voice mode button in the text composer' that calls
renderConversations({ thread: emptyThreadState, socket: socketState('connected')
}) and asserts screen.getByRole('button', { name: 'Voice mode' }) so the
dedicated voice-mode tests remain the single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6a58608-49ed-43e8-a220-046dc853ffed
📒 Files selected for processing (1)
app/src/pages/__tests__/Conversations.render.test.tsx
9181530 to
28b5c2e
Compare
…ansai#1520) Change DefaultRedirect to navigate to /human instead of /home after login + onboarding, making the mascot + voice page the first thing users see. Closes tinyhumansai#1520
28b5c2e to
752c64e
Compare
|
i'd request we not do this. this would lose a lot of users. |
Summary
/humanpagevoiceModeActivein mascotSlice) so the user's last choice survives navigationspeakRepliesfromlocalStorageto Redux with one-time backward-compat migration/humanroute (redirect to/chat) and Human tab from bottom nav (7→6 tabs)Test plan
/humanredirects to/chatCloses #1520
Summary by CodeRabbit
New Features
Navigation & UI Updates
/humannow redirects to/chatLocalization
Persistence
Tests