From 9ad774d8fee55a7c4351facbacbb2e2ebc03d749 Mon Sep 17 00:00:00 2001 From: hemanth1999k Date: Sat, 30 May 2026 18:55:36 -0500 Subject: [PATCH 1/3] feat(meet): pre-fill owner display name from Persona settings Closes one of four UX sub-problems in #2945 ("the app asks for the user's meeting name unexpectedly"). When the user has set a Persona display name (Settings > Persona, shipped in #2558), the Meeting Bots modal's "Your name in the call" field now pre-fills from it instead of starting empty, so repeat callers do not have to retype the same value every meeting. Fail-closed behavior preserved: when no Persona name is set, the field stays empty and the existing submit-side validation still surfaces the explicit "Please enter your own name..." error. A useEffect picks up a Persona name set after the modal opened (e.g. the user edited Settings in another window) but never clobbers a value the user has typed into the field. Out of scope: the other three sub-problems in #2945 (5-second drop, rejoin-loop, broader join state-machine) need actual Meet reproduction and are deferred to follow-up PRs. Tests - New: pre-fills owner display name from the Persona slice - New: leaves the field empty when no Persona name is set - Existing 22 tests migrated from `render()` to `renderWithProviders()` so the new `useAppSelector(selectPersonaDisplayName)` call has a Redux Provider in scope. Validation (local) - pnpm debug unit MeetingBotsCard: 24/24 - pnpm debug unit personaSlice: 13/13 (sanity) - pnpm prettier --check on changed files: clean - pnpm eslint --cache on changed files: 0 errors; 1 new warning (react-hooks/set-state-in-effect) of the same kind as 90 pre-existing warnings already present in the repo - pnpm tsc --noEmit: zero errors in changed files (pre-existing missing iOS/optional dep errors only) --- app/src/components/skills/MeetingBotsCard.tsx | 16 ++++- .../skills/__tests__/MeetingBotsCard.test.tsx | 70 +++++++++++++------ 2 files changed, 62 insertions(+), 24 deletions(-) diff --git a/app/src/components/skills/MeetingBotsCard.tsx b/app/src/components/skills/MeetingBotsCard.tsx index 6b94488b21..5c02972aad 100644 --- a/app/src/components/skills/MeetingBotsCard.tsx +++ b/app/src/components/skills/MeetingBotsCard.tsx @@ -16,6 +16,8 @@ import { type MascotMeetPlatform, type MeetCallRecord, } from '../../services/meetCallService'; +import { useAppSelector } from '../../store/hooks'; +import { selectPersonaDisplayName } from '../../store/personaSlice'; type Toast = { type: 'success' | 'error' | 'info'; title: string; message?: string }; @@ -131,7 +133,19 @@ export function MeetingBotsModal({ onClose, onToast }: ModalProps) { // remote participant from issuing tool calls in the owner's // name. Empty fails closed; the submit handler will surface an // explicit error before opening the CEF window. - const [ownerDisplayName, setOwnerDisplayName] = useState(''); + // + // Seed from the user's Persona display name (Settings → Persona) + // so repeat callers don't have to retype it every meeting — the + // "name prompt" UX complaint in #2945. Stays empty when no Persona + // name is set, preserving the prior fail-closed behavior. + const personaDisplayName = useAppSelector(selectPersonaDisplayName); + const [ownerDisplayName, setOwnerDisplayName] = useState(personaDisplayName); + // Pick up a Persona name set after the modal opened (e.g. user + // edited Settings in another window), but never clobber a value + // the user has already typed here. + useEffect(() => { + setOwnerDisplayName(prev => (prev.trim() === '' ? personaDisplayName : prev)); + }, [personaDisplayName]); const [submitting, setSubmitting] = useState(false); const [error, setError] = useState(null); // Recent-calls history loaded from core when the modal opens. diff --git a/app/src/components/skills/__tests__/MeetingBotsCard.test.tsx b/app/src/components/skills/__tests__/MeetingBotsCard.test.tsx index 6ca1ae47db..168569fe06 100644 --- a/app/src/components/skills/__tests__/MeetingBotsCard.test.tsx +++ b/app/src/components/skills/__tests__/MeetingBotsCard.test.tsx @@ -1,7 +1,8 @@ -import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { cleanup, fireEvent, screen, waitFor } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import type { MeetCallRecord } from '../../../services/meetCallService'; +import { renderWithProviders } from '../../../test/test-utils'; import MeetingBotsCard, { MeetingBotsModal } from '../MeetingBotsCard'; const joinMock = vi.fn(); @@ -32,26 +33,26 @@ describe('MeetingBotsCard', () => { afterEach(() => cleanup()); it('renders the banner and hides the modal by default', () => { - render(); + renderWithProviders(); expect(screen.getByTestId('meeting-bots-banner')).toBeInTheDocument(); expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); }); it('opens the modal when the banner is clicked', () => { - render(); + renderWithProviders(); fireEvent.click(screen.getByTestId('meeting-bots-banner')); expect(screen.getByRole('dialog')).toBeInTheDocument(); }); it('closes the modal on Cancel', () => { - render(); + renderWithProviders(); fireEvent.click(screen.getByTestId('meeting-bots-banner')); fireEvent.click(screen.getByRole('button', { name: 'Cancel' })); expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); }); it('closes the modal on Escape', () => { - render(); + renderWithProviders(); fireEvent.click(screen.getByTestId('meeting-bots-banner')); fireEvent.keyDown(window, { key: 'Escape' }); expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); @@ -60,7 +61,7 @@ describe('MeetingBotsCard', () => { it('submits to joinMeetCall and fires a success toast', async () => { joinMock.mockResolvedValueOnce({ requestId: 'req-1' }); const onToast = vi.fn(); - render(); + renderWithProviders(); fireEvent.click(screen.getByTestId('meeting-bots-banner')); fireEvent.change(screen.getByLabelText(/meeting link/i), { @@ -105,7 +106,7 @@ describe('MeetingBotsCard', () => { it('surfaces a join error inline + as an error toast', async () => { joinMock.mockRejectedValueOnce(new Error('Bad URL')); const onToast = vi.fn(); - render(); + renderWithProviders(); fireEvent.click(screen.getByTestId('meeting-bots-banner')); fireEvent.change(screen.getByLabelText(/meeting link/i), { @@ -125,13 +126,36 @@ describe('MeetingBotsCard', () => { }); it('disables the submit when the active platform is coming-soon', () => { - render(); + renderWithProviders(); fireEvent.click(screen.getByTestId('meeting-bots-banner')); // Pick Zoom (coming soon) fireEvent.click(screen.getByRole('button', { name: /Zoom/ })); const submit = screen.getByRole('button', { name: /coming soon/i }); expect(submit).toBeDisabled(); }); + + // Issue #2945: users were being asked to retype "your name in the call" + // every meeting. When the Persona display name (Settings → Persona) is + // set, the owner-name field pre-fills from it so the user can submit + // without retyping. + it('pre-fills the owner display name from the Persona slice', () => { + const { store } = renderWithProviders(, { + preloadedState: { persona: { displayName: 'Hemanth', description: '' } }, + }); + fireEvent.click(screen.getByTestId('meeting-bots-banner')); + const ownerInput = screen.getByLabelText(/your name in the call/i) as HTMLInputElement; + expect(ownerInput.value).toBe('Hemanth'); + // Sanity: the slice is wired so the assertion above isn't a no-op. + expect(store.getState().persona.displayName).toBe('Hemanth'); + }); + + it('leaves the owner display name empty when no Persona name is set', () => { + // Default preloadedState — persona slice initial state is { displayName: '' }. + renderWithProviders(); + fireEvent.click(screen.getByTestId('meeting-bots-banner')); + const ownerInput = screen.getByLabelText(/your name in the call/i) as HTMLInputElement; + expect(ownerInput.value).toBe(''); + }); }); // ── RecentCallsSection / RecentCallRow tests ────────────────────────────────── @@ -160,7 +184,7 @@ describe('MeetingBotsModal — recent calls section', () => { // Never resolves during this test — simulates a slow fetch. listMock.mockReturnValue(new Promise(() => {})); - render( {}} />); + renderWithProviders( {}} />); expect(screen.getByText(/loading…/i)).toBeInTheDocument(); }); @@ -168,7 +192,7 @@ describe('MeetingBotsModal — recent calls section', () => { it('shows an empty-state message when listMeetCalls returns an empty array', async () => { listMock.mockResolvedValueOnce([]); - render( {}} />); + renderWithProviders( {}} />); await waitFor(() => { expect(screen.getByText(/no previous calls yet/i)).toBeInTheDocument(); @@ -182,7 +206,7 @@ describe('MeetingBotsModal — recent calls section', () => { ]; listMock.mockResolvedValueOnce(records); - render( {}} />); + renderWithProviders( {}} />); await waitFor(() => { expect(screen.getByText('aaa-bbbb-ccc')).toBeInTheDocument(); @@ -196,7 +220,7 @@ describe('MeetingBotsModal — recent calls section', () => { it('shows the count badge when there is at least one record', async () => { listMock.mockResolvedValueOnce([makeCallRecord()]); - render( {}} />); + renderWithProviders( {}} />); await waitFor(() => { // The "(1)" count badge next to the "Recent calls" heading. @@ -207,7 +231,7 @@ describe('MeetingBotsModal — recent calls section', () => { it('shows an error hint and an empty list when listMeetCalls rejects', async () => { listMock.mockRejectedValueOnce(new Error('Network timeout')); - render( {}} />); + renderWithProviders( {}} />); await waitFor(() => { expect(screen.getByText(/network timeout/i)).toBeInTheDocument(); @@ -221,7 +245,7 @@ describe('MeetingBotsModal — recent calls section', () => { makeCallRecord({ meet_url: 'https://meet.google.com/xyz-1234-abc' }), ]); - render( {}} />); + renderWithProviders( {}} />); await waitFor(() => { expect(screen.getByText('xyz-1234-abc')).toBeInTheDocument(); @@ -235,7 +259,7 @@ describe('MeetingBotsModal — recent calls section', () => { makeCallRecord({ spoken_seconds: 40, listened_seconds: 20 }), ]); - render( {}} />); + renderWithProviders( {}} />); await waitFor(() => { expect(screen.getByText(/60s on call/i)).toBeInTheDocument(); @@ -248,7 +272,7 @@ describe('MeetingBotsModal — recent calls section', () => { makeCallRecord({ started_at_ms: Date.now() - 5 * 60 * 1000 }), ]); - render( {}} />); + renderWithProviders( {}} />); await waitFor(() => { expect(screen.getByText(/\dm ago/)).toBeInTheDocument(); @@ -258,7 +282,7 @@ describe('MeetingBotsModal — recent calls section', () => { it('shows "—" for a zero started_at_ms timestamp', async () => { listMock.mockResolvedValueOnce([makeCallRecord({ started_at_ms: 0 })]); - render( {}} />); + renderWithProviders( {}} />); await waitFor(() => { expect(screen.getByText('—')).toBeInTheDocument(); @@ -270,7 +294,7 @@ describe('MeetingBotsModal — recent calls section', () => { it('shows singular "turn" (not "turns") when turn_count is 1', async () => { listMock.mockResolvedValueOnce([makeCallRecord({ turn_count: 1 })]); - render( {}} />); + renderWithProviders( {}} />); await waitFor(() => { expect(screen.getByText(/1 turn$/)).toBeInTheDocument(); @@ -281,7 +305,7 @@ describe('MeetingBotsModal — recent calls section', () => { it('falls back to the raw URL when it cannot be parsed', async () => { listMock.mockResolvedValueOnce([makeCallRecord({ meet_url: 'not-a-valid-url' })]); - render( {}} />); + renderWithProviders( {}} />); await waitFor(() => { expect(screen.getByText('not-a-valid-url')).toBeInTheDocument(); @@ -293,7 +317,7 @@ describe('MeetingBotsModal — recent calls section', () => { makeCallRecord({ started_at_ms: Date.now() - 3 * 60 * 60 * 1000 }), ]); - render( {}} />); + renderWithProviders( {}} />); await waitFor(() => { expect(screen.getByText(/3h ago/)).toBeInTheDocument(); @@ -305,7 +329,7 @@ describe('MeetingBotsModal — recent calls section', () => { makeCallRecord({ started_at_ms: Date.now() - 25 * 60 * 60 * 1000 }), ]); - render( {}} />); + renderWithProviders( {}} />); await waitFor(() => { expect(screen.getByText('yesterday')).toBeInTheDocument(); @@ -317,7 +341,7 @@ describe('MeetingBotsModal — recent calls section', () => { makeCallRecord({ started_at_ms: Date.now() - 3 * 24 * 60 * 60 * 1000 }), ]); - render( {}} />); + renderWithProviders( {}} />); await waitFor(() => { expect(screen.getByText(/3d ago/)).toBeInTheDocument(); @@ -329,7 +353,7 @@ describe('MeetingBotsModal — recent calls section', () => { makeCallRecord({ started_at_ms: Date.now() - 10 * 24 * 60 * 60 * 1000 }), ]); - render( {}} />); + renderWithProviders( {}} />); await waitFor(() => { // toLocaleDateString returns "Month Day" — just check it's not a relative label. From ab47261fed1ed2b39e5c8324fd32993d62255e3d Mon Sep 17 00:00:00 2001 From: hemanth1999k Date: Sat, 30 May 2026 20:09:15 -0500 Subject: [PATCH 2/3] =?UTF-8?q?refactor(meet):=20address=20CodeRabbit=20re?= =?UTF-8?q?view=20=E2=80=94=20dirty-flag=20pattern,=20test=20comment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per @coderabbitai's review on #3034: 1. Replace the useEffect(() => setOwnerDisplayName(prev => ...)) sync pattern with a derived value driven by a dirty flag (isOwnerNameEdited). Effective owner name is `isOwnerNameEdited ? draft : personaDisplayName` — Persona changes flow through until the user types, after which the field becomes fully controlled. Same user-visible behavior, but no setState inside an effect, so the react-hooks/set-state-in-effect warning my prior commit added is now gone. 2. Expand the stale test comment about persona initial state to include the `description: ''` field (matches personaSlice.ts initialState). Also adds a third test pinning the new contract: typing in the field latches the dirty flag so a subsequent Persona update does not overwrite the user's input. Tests: 25/25 (was 24/24) — pnpm debug unit MeetingBotsCard. Lint: 0 errors, my prior new warning removed (the surviving warning is pre-existing on line 172 for refreshRecentCalls, untouched). --- app/src/components/skills/MeetingBotsCard.tsx | 25 ++++++++++--------- .../skills/__tests__/MeetingBotsCard.test.tsx | 23 ++++++++++++++++- 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/app/src/components/skills/MeetingBotsCard.tsx b/app/src/components/skills/MeetingBotsCard.tsx index 5c02972aad..94f3791284 100644 --- a/app/src/components/skills/MeetingBotsCard.tsx +++ b/app/src/components/skills/MeetingBotsCard.tsx @@ -134,18 +134,16 @@ export function MeetingBotsModal({ onClose, onToast }: ModalProps) { // name. Empty fails closed; the submit handler will surface an // explicit error before opening the CEF window. // - // Seed from the user's Persona display name (Settings → Persona) - // so repeat callers don't have to retype it every meeting — the - // "name prompt" UX complaint in #2945. Stays empty when no Persona - // name is set, preserving the prior fail-closed behavior. + // Effective value = Persona display name (Settings → Persona) until + // the user types into the field — the "name prompt" UX complaint in + // #2945, so repeat callers don't retype the same value every meeting. + // Once the user edits the field, the dirty flag latches and the + // input becomes fully controlled — Persona changes no longer + // overwrite their input, and clearing the field stays empty. const personaDisplayName = useAppSelector(selectPersonaDisplayName); - const [ownerDisplayName, setOwnerDisplayName] = useState(personaDisplayName); - // Pick up a Persona name set after the modal opened (e.g. user - // edited Settings in another window), but never clobber a value - // the user has already typed here. - useEffect(() => { - setOwnerDisplayName(prev => (prev.trim() === '' ? personaDisplayName : prev)); - }, [personaDisplayName]); + const [ownerDisplayNameDraft, setOwnerDisplayNameDraft] = useState(''); + const [isOwnerNameEdited, setIsOwnerNameEdited] = useState(false); + const ownerDisplayName = isOwnerNameEdited ? ownerDisplayNameDraft : personaDisplayName; const [submitting, setSubmitting] = useState(false); const [error, setError] = useState(null); // Recent-calls history loaded from core when the modal opens. @@ -319,7 +317,10 @@ export function MeetingBotsModal({ onClose, onToast }: ModalProps) { setOwnerDisplayName(e.target.value)} + onChange={e => { + setOwnerDisplayNameDraft(e.target.value); + setIsOwnerNameEdited(true); + }} maxLength={64} placeholder="As shown in Google Meet (e.g. Nikhil Bajaj)" disabled={isComingSoon || submitting} diff --git a/app/src/components/skills/__tests__/MeetingBotsCard.test.tsx b/app/src/components/skills/__tests__/MeetingBotsCard.test.tsx index 168569fe06..31290597dd 100644 --- a/app/src/components/skills/__tests__/MeetingBotsCard.test.tsx +++ b/app/src/components/skills/__tests__/MeetingBotsCard.test.tsx @@ -150,12 +150,33 @@ describe('MeetingBotsCard', () => { }); it('leaves the owner display name empty when no Persona name is set', () => { - // Default preloadedState — persona slice initial state is { displayName: '' }. + // Default preloadedState — persona slice initial state is + // { displayName: '', description: '' } (see personaSlice.ts). renderWithProviders(); fireEvent.click(screen.getByTestId('meeting-bots-banner')); const ownerInput = screen.getByLabelText(/your name in the call/i) as HTMLInputElement; expect(ownerInput.value).toBe(''); }); + + // Dirty-flag contract: once the user has typed into the field, a + // subsequent Persona update from the slice must NOT overwrite their + // input. The pre-edit case is covered by the "pre-fills" test above. + it('does not overwrite user-typed owner name when Persona slice updates later', () => { + const { store } = renderWithProviders(, { + preloadedState: { persona: { displayName: 'Hemanth', description: '' } }, + }); + fireEvent.click(screen.getByTestId('meeting-bots-banner')); + const ownerInput = screen.getByLabelText(/your name in the call/i) as HTMLInputElement; + // Sanity: pre-fill landed. + expect(ownerInput.value).toBe('Hemanth'); + // User types over it. + fireEvent.change(ownerInput, { target: { value: 'Alice' } }); + expect(ownerInput.value).toBe('Alice'); + // Persona name changes underneath (e.g. user edits Settings in another window). + store.dispatch({ type: 'persona/setPersonaDisplayName', payload: 'Nova' }); + // User's typed value wins — does NOT flip to 'Nova'. + expect(ownerInput.value).toBe('Alice'); + }); }); // ── RecentCallsSection / RecentCallRow tests ────────────────────────────────── From 2184cda096a2ed80a114b6325fdff5f734bd0519 Mon Sep 17 00:00:00 2001 From: hemanth1999k Date: Sun, 31 May 2026 21:38:45 -0500 Subject: [PATCH 3/3] chore: re-trigger CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous run hit a known repo-wide flake in inference_*_raw_coverage_e2e tests (sibling PRs around the same time failed on different tests in the same test family — likely env-dependent Ollama / HTTP mock assumptions). This empty commit re-triggers the workflow on the same code.