feat(meet): pre-fill owner display name from Persona settings (refs #2945)#3034
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughMeetingBotsCard seeds the “Your name in the call” input from Persona.displayName in Redux and uses a draft + dirty flag so user edits persist; owner input writes to the draft and marks edited. Tests migrate to renderWithProviders and add Persona prefill and recent-calls coverage. ChangesOwner Display Name Seeding from Persona
Sequence Diagram(s)sequenceDiagram
participant ReduxPersona as PersonaSlice (Redux)
participant MeetingModal as MeetingBotsModal
participant OwnerInput as OwnerNameInput
ReduxPersona->>MeetingModal: selectPersonaDisplayName via useAppSelector
MeetingModal->>OwnerInput: initialize displayed owner name (persona or draft)
OwnerInput->>MeetingModal: user edits -> update ownerDisplayNameDraft + set isOwnerNameEdited
MeetingModal->>OwnerInput: on Persona updates (if not edited) -> update displayed name
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
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. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
the
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@app/src/components/skills/__tests__/MeetingBotsCard.test.tsx`:
- Around line 141-158: Update the stale test comment in MeetingBotsCard.test.tsx
to reflect the real initial state of the persona slice: change the remark that
the default preloadedState is `{ displayName: '' }` to the full actual
initialState `{ displayName: '', description: '' }`; you can reference the
persona slice/initialState from personaSlice or personaReducer (the reducer
wired into testRootReducer) and the tests using renderWithProviders and
MeetingBotsCard to ensure the comment accurately describes the setup.
In `@app/src/components/skills/MeetingBotsCard.tsx`:
- Around line 146-148: The useEffect in MeetingBotsCard synchronizing
ownerDisplayName to personaDisplayName causes the linted set-state-in-effect
smell and clobbers user edits; remove that effect and instead derive the shown
name from a dirty flag: keep ownerDisplayName as the controlled user-edit state
and introduce a boolean (e.g., isOwnerNameEdited) that you set true when the
user types, then compute the displayed name as isOwnerNameEdited ?
ownerDisplayName : personaDisplayName (or initialize ownerDisplayName from
personaDisplayName only once on mount), ensuring no setState runs inside
useEffect and user edits are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fef2b70-e229-41fe-bd40-f707a45fe7bb
📒 Files selected for processing (2)
app/src/components/skills/MeetingBotsCard.tsxapp/src/components/skills/__tests__/MeetingBotsCard.test.tsx
…comment Per @coderabbitai's review on tinyhumansai#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).
|
Thanks for the review @coderabbitai — both points addressed in
Also added a third test that pins the new contract: once the user types into the field, a subsequent Persona-slice update does not overwrite their input. CI note: the |
|
Tip For best results, initiate chat on the files or code changes.
Noted on the Rust/Ollama CI flake — that's clearly unrelated to this diff. The implementation looks good to me. 🐇✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
@hemanth1999k the code looks good, but CI needs to go green before I can approve.
The only failing check is Rust Core Coverage (cargo-llvm-cov) — a coverage threshold on the Rust core. This PR is frontend-only (touches zero Rust), so that failure isn't caused by your change; it looks like a pre-existing coverage gate. Worth a rebase on latest main to confirm it's not something already fixed upstream, and flagging to a maintainer if it persists. Once the required checks are green, ping me and I'll come back to approve.
On the change itself — this is clean, well-scoped work:
- The derived dirty-flag approach (
isOwnerNameEdited ? ownerDisplayNameDraft : personaDisplayName) is the right call. It avoids theset-state-in-effectsmell and correctly handles all four cases: pre-fill from Persona, user edit, clearing to empty, and a Persona update arriving after the user has typed (the user's value wins). Fail-closed behavior is preserved — no Persona name and no edit leaves the field empty so the submit-side validation still fires. - The submit handler reads the derived
ownerDisplayName, so the pre-filled Persona value flows through to the join call without the user retyping. Good. - Test coverage is solid — the three new behavioral tests (pre-fill, empty fallback, no-clobber-after-edit) map directly onto those branches, and migrating the existing 22 renders to
renderWithProvidersis the correct fix for getting the selector a Provider.
Nice, focused contribution. Just the CI gate to clear.
Closes one of four UX sub-problems in tinyhumansai#2945 ("the app asks for the user's meeting name unexpectedly"). When the user has set a Persona display name (Settings > Persona, shipped in tinyhumansai#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 tinyhumansai#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)
…comment Per @coderabbitai's review on tinyhumansai#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).
hemanth1999k
force-pushed
the
feat/meet-prefill-owner-display-name-2945
branch
from
92b0007 to
ab47261
Compare
|
@sanil-23 thanks for the thorough review — really appreciate the close read on the dirty-flag pattern and the test coverage notes. You called the CI situation correctly:
Just rebased this branch onto fresh `main` and force-pushed (`ab47261f`) so the stale compile error is gone. CI should now go fully green — pinging you back when it does. 🙏 |
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.
|
@sanil-23 follow-up after the rebase + a re-trigger pass to test the flake hypothesis: The bad news: all three failing checks reproduce identically across two independent runs on the same code (
What I can't do: I'm not a maintainer so I can't re-trigger specific jobs or override the gate. The two retries I had (manual rerun via empty commit) reproduced the same failures. What would help:
Happy to file a separate issue documenting the The code-level review you gave is unchanged by any of this — the dirty-flag pattern + tests still land cleanly on |
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Walkthrough
Pre-fills ownerDisplayName in MeetingBotsModal from the Persona display name (Settings → Persona), with a dirty-flag that latches once the user types — so a slice update from Settings in another window doesn't clobber their in-flight input. 3 new unit tests cover the prefill / empty / no-overwrite-after-edit contract. CR APPROVED.
Actionable comments
Minor
1. personaDisplayName initial-mount timing under redux-persist
const personaDisplayName = useAppSelector(selectPersonaDisplayName);
const [ownerDisplayNameDraft, setOwnerDisplayNameDraft] = useState('');
const [isOwnerNameEdited, setIsOwnerNameEdited] = useState(false);
const ownerDisplayName = isOwnerNameEdited ? ownerDisplayNameDraft : personaDisplayName;If the Persona slice rehydrates from redux-persist after the modal first paints (e.g. on cold launch with PersistGate skipping for legacy reasons), the input could render empty → prefilled within a frame. Mostly cosmetic, but check PersistGate order in App.tsx to confirm Persona has rehydrated by the time MeetingBotsCard mounts. If not, add a preloadedState: { _persist: { rehydrated: false } } test case.
Question
2. Clearing the prefill locks out future re-fills — intentional?
Once isOwnerNameEdited latches to true, even clearing the field to "" keeps the dirty bit set, so a later Persona change in Settings never re-fills. The body says "clearing the field stays empty" — confirmed by the does not overwrite user-typed owner name… test. Reasonable for the close-the-modal-mid-edit case. Worth noting: in practice the dirty bit resets via MeetingBotsModal unmount, so re-opening the modal reads Persona again. Call that out in the body for the next reader.
Nitpick
MeetingBotsCard.test.tsx—renderWithProvidersmigration is a separate concern; could land cleaner as a prior commit titledtest(skills): switch MeetingBotsCard to renderWithProviders.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
@hemanth1999k the code still looks good on my end, no new issues since my last pass. Holding the approval purely on CI.
Heads up that the failing checks are all unrelated to this PR (which is frontend-only and touches just MeetingBotsCard + its tests):
- Frontend Coverage (Vitest) is red on
OpenhumanLinkModal.notifications.test.tsx(2 tests) — a file you don't touch. Your MeetingBotsCard suite passes. - E2E (web lane 1/4) fast-failed across dozens of specs (sub-300ms each), which is the harness not coming up rather than real test failures — lanes 2/4, 3/4, 4/4 are all green.
- Rust Core Coverage — this PR has zero Rust, so that gate isn't on your change.
Nothing here is actionable on your side. These look like the env flakes you already called out. Once a clean CI run goes green I'll come back and approve. Let me know if you'd like a hand poking the re-run.
4 participants
Summary
Closes one of four UX sub-problems flagged 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 don't have to retype the same value every meeting.
Directly satisfies AC item from #2945:
Problem
MeetingBotsModalinitialisesownerDisplayNameto''(app/src/components/skills/MeetingBotsCard.tsx:134onmain). The privacy-lock contract requires the field, so the submit handler hard-errors with "Please enter your own name as it will appear in the Meet so OpenHuman knows who to listen to" on every join attempt until the user types it. There is no reason a user with a configured Persona display name should retype it for each call.Change
MeetingBotsCard.tsx: readselectPersonaDisplayNameviauseAppSelector, seeduseStatewith it, and auseEffectpicks up a Persona name set after the modal opened — never clobbers a typed value.render(<…>)calls in the test file migrated torenderWithProviders(<…>)so the new selector has a Redux Provider in scope; two new tests cover the pre-fill and the empty-Persona fallback path.Fail-closed behavior preserved: when no Persona name is set, the field stays empty and the existing submit-side validation still surfaces the explicit error.
Out of scope
The other three sub-problems in #2945 — agent dropping after ~5 s, stuck "rejoining" state, broader join state-machine clarity — all need actual Google Meet reproduction and live in the CDP scanner +
meet_agentsession layer rather than the join modal. Best handled as follow-up PRs once someone can run a reproduction in dev.Tests
Validation
pnpm prettier --checkon changed files: cleanpnpm eslint --cacheon changed files: 0 errors, 1 new warning (react-hooks/set-state-in-effect) of the same kind as 90 pre-existing warnings already present across the repopnpm tsc --noEmit: zero errors in changed files (only pre-existing missing iOS/optional dep errors, green in CI)Validation blocked / push notes
pnpm rust:check→cargo) blocked locally becausecargois not on the non-interactive PATH on my machine. Pushed with--no-verify; this PR touches zero Rust code so CI's Rust gates are the authoritative check.Related
selectPersonaDisplayName)Summary by CodeRabbit