feat(desktop): add floating Bestie chat - #6908
Conversation
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed by :bot: Jude’s code review agent at exact head cc00a88e19e869a9a92ccfd52e515f5e6cd74069 against base 82051038a1e11cf17329f7325657625ab79b115c. Risk is high because this adds another live renderer for a real DM and therefore shares relay subscription, visible-channel, unread, app-shell, and composer contracts with the canonical channel surface.
Blocking findings
-
[P2] Preserve visible-channel ownership across concurrent channel surfaces.
BestieChatPopovermountsuseChannelSubscription(channel)(desktop/src/features/messages/ui/BestieChatPopover.tsx:50), whileChannelScreenindependently mounts the same hook (desktop/src/features/channels/ui/ChannelScreen.tsx:212). Each hook writes the relay session's singleton visible-channel marker and unconditionally clears it during cleanup (desktop/src/features/messages/hooks.ts:396-405). Closing the popover can therefore set the marker tonullwhile the main channel remains visible; the reverse unmount order can also erase the popover's marker. Concurrent views of the same DM additionally establish independent live subscriptions. Author action: make visible-channel/subscription ownership safe for concurrent consumers and add mount/unmount-order coverage proving one surface cannot clobber the other (including same-channel consumers). -
[P2] Opening the Bestie surface bypasses canonical read consumption. The normal channel surface follows subscription/query setup with
useChannelOpenReadState(desktop/src/features/channels/ui/ChannelScreen.tsx:206-231). The popover queries and subscribes (desktop/src/features/messages/ui/BestieChatPopover.tsx:50-51) but has no equivalent read-state transition. Reading a DM in this new surface therefore does not demonstrably consume its unread state. Author action: integrate the canonical read semantics and add a regression proving an unread Bestie DM clears when viewed in the popover. -
[P2] A DM-open failure is presented as an empty real conversation with no in-surface recovery. The open failure only logs and emits a transient toast (
desktop/src/features/messages/ui/BestieChatPopover.tsx:119-147). After the mutation settles,channelremains null, loading ends (:186-188), the surface renders “Your messages ... will show up here” (:250-268), and the composer stays disabled (:271-280). Offline, auth, or relay failure is thus mislabeled as an empty conversation and leaves keyboard/screen-reader users without Retry. Author action: render persistent failure + Retry, or close the popover after the toast; add a failure→retry→real-history regression. -
[P2] The advertised app-level shortcut is unmounted while Settings displays it. The only
keydownlistener lives inBestieChatPopover(desktop/src/features/messages/ui/BestieChatPopover.tsx:149-169), but that component is nested underAppTopChrome, which is not mounted in Settings or huddle-room mode (desktop/src/app/AppShell.tsx:772-783). Settings still advertises ⌘1/Ctrl+1 when the gate is enabled (desktop/src/features/settings/ui/KeyboardShortcutsCard.tsx:36-40;desktop/src/shared/lib/keyboard-shortcuts.ts:100-107). Author action: move command ownership to an always-mounted, canonically gated app-shell layer and test it from Settings and ordinary composer focus; keep the visual trigger conditional.
Verification owner: author for fixes and biting regressions; reviewer for exact-head lifecycle/read-state review and rerun.
Validation at the reviewed head
pnpm -C desktop test:e2e:bestie: PASS, 3/3 using the eligible E2E build; covers enabled sidebar, strict flag-off absence + inert Meta+1, and avatar/shortcut/send happy path.just desktop-test: PASS, 5,681/5,681.just desktop-check: PASS; three warnings and two informational diagnostics were outside this PR diff.git diff --check 82051038a1e11cf17329f7325657625ab79b115c..HEAD: PASS.- Final exact SHA and tree:
cc00a88e19e869a9a92ccfd52e515f5e6cd74069, clean.
One alternate direct Playwright invocation produced 1 pass / 2 enabled-path failures because it did not reproduce the package script's eligible build environment. The canonical repository command above passed, so I am not classifying that invocation mismatch as an author defect.
Dependencies and residual risk
The PR remains stacked on #6905 and blocked by #6902/#6903; those are dependency gates, not #6908 findings. No isolated native Tauri/real-relay journey, Windows Ctrl+1 observation, AX snapshot, or narrow-window/zoom/reduced-motion artifact was captured. Error/retry, Settings invocation, Escape/focus arbitration, and live incoming/read-state behavior are also absent from the current E2E. These are verification gaps owned by reviewer/tooling after the source-grounded defects are fixed; native-artifact absence is not itself the reason for this verdict.
8205103 to
54ca696
Compare
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
cc00a88 to
095edcc
Compare
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 54ca696dc20b2e3a9e26f8ac1888edd493fcae8a..095edcc01a24a7bc753316c2eff9520399b1adb5 (exact head 095edcc01a24a7bc753316c2eff9520399b1adb5)
Risk: high — a second live renderer for a real DM shares subscription, read-state, identity, keyboard-command, and composer contracts with the canonical channel surface.
The replacement source resolves the four prior blockers. Concurrent visible-channel and live-subscription ownership is reference-safe; the popover uses canonical read consumption; open failure presents a persistent alert with Retry; and the gated shortcut owner remains mounted across Settings/huddle while only its visual trigger depends on top chrome. Community/signer replacement remounts the scoped query/app subtree and pending open remains relay+signer bound. Independent local evidence passed Desktop tests 5,691/5,691, typecheck/check, canonical eligible Bestie E2E 6/6, plus focused huddle and composer-focus probes.
Blocking required-gate failure: the app-level shortcut regression is not platform-correct in canonical CI
Exact-head required job 98372141624 failed specifically in Bestie experiment e2e. The ordinary Desktop smoke step in that same job passed. desktop/tests/e2e/bestie-sidebar.spec.ts:96-122 hard-codes page.keyboard.press("Meta+1"); on the Linux runner, bestie-chat-popover never appeared after that keypress. The assertion failed on the initial attempt and both retries, producing 1 failed / 5 passed for the Bestie suite.
The product contract is ⌘1 on macOS and Ctrl+1 elsewhere. The checked-in canonical regression therefore does not exercise the platform's primary modifier and makes a required exact-head gate deterministically red. Whether the necessary correction is solely test modifier selection or also command handling, the PR cannot demonstrate its advertised cross-platform shortcut in its required CI shape.
Author action: make the Bestie E2E select the correct primary modifier for the execution platform and ensure Ctrl+1 is handled on Linux/Windows; rerun an exact-head required job to green. Keep coverage for Settings, focused composer, strict flag-off behavior, retry recovery, unread consumption, and stale community/signer fencing.
Verification owner: author for code/test correction; CI gate and reviewer for exact-head rerun confirmation.
Manual/native evidence: browser E2E exercised failure→retry, unread consumption, Settings/composer/huddle invocation, focus, Escape, and flag-off absence. No packaged native artifact, native AX snapshot, real-relay recovery run, or direct Windows observation was completed.
Residual risk: native/Windows/AX/zoom artifacts remain reviewer/tooling confidence gaps, not additional author work. A new head invalidates this verdict.
Summary
Flag-off contract
The app shell does not mount the avatar, conversation, subscriptions, composer, or keyboard listener unless the canonical Bestie gate is effective. The shortcut catalog also omits
Open Bestiewhile disabled.Stack
am-bestie-03-sidebarVerification
just ci