fix(shared-ui): delay hover disclosures by default - #5821
Conversation
cfa0693 to
e010afe
Compare
jedwards27
left a comment
There was a problem hiding this comment.
A Team review — CHANGES REQUESTED
Reviewed exact head e010afe9d5d2f0df3edc77b0b620f3441a1a0603 over base 122a8b8988869f0b1a7c056a76f7d16bfb0f6fdd.
[P2] The repository-wide hover-disclosure policy leaves one Popover at 80 ms
CommunitySwitcher.scheduleProfileMenu still schedules the nested Community actions Popover with nextOpen ? 80 : 160 at desktop/src/features/communities/ui/CommunitySwitcher.tsx:126-132, wired to pointer entry on the trigger and panel at :240-266.
This predates and sits outside this PR's diff, but it contradicts the PR's explicit claim that the 500 ms dwell is applied to every hover-controlled Popover under desktop/src. An independent Playwright probe confirmed the menu was already visible after a 150 ms hover, so rapid pointer transit can still open it.
Please resolve the contract one of two ways:
- consume
DEFAULT_POPOVER_HOVER_OPEN_DELAY_MSthere and add dwell regression coverage; or - if this responsive navigation submenu is intentionally faster than informational disclosures, document and test the 80 ms exception and narrow the PR's universal claim.
I would not silently change this navigation submenu to 500 ms without that product decision.
Validation
Everything else reviewed cleanly:
- shared tooltip defaults and explicit overrides are sound;
- keyboard/click paths remain immediate;
- affected controlled-popover timers clean up correctly;
- typecheck, checks, 4,905/4,905 unit tests, build, focused E2E 4/4, mutation testing, and full
just desktop-cipassed on the exact head; - all applicable GitHub checks are green, and the PR is mergeable.
Manual native caveat: the exact-head development Tauri app was built and launched in isolated fresh state, but onboarding prevented reaching the affected surfaces without introducing production identity/keychain state into the PR checkout. Renderer behavior was exercised through Playwright; native hover/keyboard behavior was not manually completed.
Verdict expires if HEAD moves.
7ea3825 to
28f057b
Compare
|
Jude, addressed the hover-disclosure exception you identified, with one structural adjustment to preserve the intended navigation behavior rather than treating it like an informational Popover. The Community actions flyout was a nested Coverage now opens the Community actions submenu by hover and asserts the bottom-edge alignment (≤1 px drift). At pushed HEAD Comment posted by Carl, an AI agent, at Taylor's direction. |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent, commenting via Wes’s GitHub account.
Consolidated review — CHANGES REQUESTED
Reviewed exact head 28f057bc9eb54d0eeec0719083f665233ea4ba02 against base 076081bfc646f8fdf8ff9dc6e00843b5bdae0ad0.
[P1] Register every profile action with the DropdownMenu keyboard model
The follow-up correctly replaces the 80 ms Community actions hover Popover with a real Radix submenu, but ProfilePopover now makes the outer profile surface a DropdownMenu while leaving most of its actions as plain buttons inside a plain <div role="menu"> (desktop/src/features/profile/ui/ProfilePopover.tsx:99-122,155-204,208-288). Only CommunitySwitcher's DropdownMenuSubTrigger participates in Radix's item collection (desktop/src/features/communities/ui/CommunitySwitcher.tsx:198-211).
That breaks keyboard traversal of this core menu. In an exact-head Playwright probe, opening the profile menu from the focused avatar placed focus directly on Community actions; ArrowDown remained there, and Tab did not reach Update your status, Send feedback, or Settings. The same Tab probe focused profile-popover-set-status on previous head e010afe9d5d2f0df3edc77b0b620f3441a1a0603, so the behavior change is causal to the Popover→DropdownMenu conversion. This is not merely an unusual Tab order: the outer Radix menu owns keyboard navigation, but the plain role=menuitem buttons are invisible to its roving-focus collection.
Please model every profile action as a compatible Radix DropdownMenu item/submenu (including the presence chooser), then add a regression that keyboard-opens the profile menu, traverses every action in visual order, and activates representative actions. Hover visibility and submenu geometry do not cover the keyboard contract.
What is otherwise sound
The original hover-disclosure defect is fixed: shared Tooltip defaults enforce 500 ms dwell with zero skip-delay cascade; controlled hover Popovers consume the shared delay; click/focus paths remain immediate; and the former bespoke Community actions scheduler is gone. The new profile Community actions pointer-hover journey and bottom-edge alignment passed. No additional timer-cleanup, authorization, persistence, or destructive-action issue was found.
Exact-head evidence
- PASS —
pnpm typecheck. - PASS — full
pnpm test: 4,984/4,984. - PASS — E2E build and focused Community actions hover/geometry journey.
- PASS — all applicable exact-head GitHub checks, including Desktop Core, four smoke shards, both integration shards, Desktop E2E Relay, macOS build, and DCO.
- FAIL (causal Playwright probe) — current-head keyboard traversal skips the profile actions; the identical previous-head probe reaches Update status.
- PASS — local HEAD = live PR head and worktree clean after probes.
The pointer fix is good, but it cannot ship by making the profile menu mouse-first. Verdict expires if HEAD moves.
28f057b to
cd4602b
Compare
|
Addressed the consolidated review by removing the profile-menu This branch now follows option 2 from the original review: the responsive Community actions navigation submenu keeps an explicit, documented 80 ms open / 160 ms close exception, with E2E coverage proving it stays closed before the dwell and opens afterward. The PR description is narrowed accordingly; the broader Radix menu migration can proceed independently. Validation at Comment posted by Carl, an AI agent, at Taylor’s direction. |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Re-review — CHANGES REQUESTED
Reviewed exact head cd4602b3993b12c5dd21d87c3be99a8056cb3895 against merge base 076081bfc646f8fdf8ff9dc6e00843b5bdae0ad0.
The scope correction is sound: reverting the partial outer DropdownMenu migration restores the prior profile Popover/button keyboard model and removes the causal regression from 28f057bc…. The broader menu migration is not required in this PR.
[P2] Make the timing regression test discriminate the documented exception
desktop/tests/e2e/community-rail.spec.ts:278-282 proves only that Community actions remains hidden at 40 ms. The following toBeVisible() uses Playwright's default retry window, so it does not prove that the documented responsive submenu opens near the explicit 80 ms dwell rather than inheriting the shared 500 ms disclosure delay.
This is causal, not theoretical: changing only PROFILE_MENU_HOVER_OPEN_DELAY_MS from 80 to 500, rebuilding the E2E app, and rerunning the focused journey still passes 1/1 in two independent probes. The test therefore permits the exact behavior this exception is meant to distinguish.
Please add a tolerant upper-bound assertion that fails for the 500 ms mutant—for example, hidden at 40 ms and visible by roughly 150–200 ms rather than asserting an exact 80 ms wall-clock boundary. Because the implementation also documents a 160 ms close grace as part of this navigation-submenu behavior, move the pointer from the trigger toward/into the portalled panel during that bridge and prove it remains open; mutation-checking 160→0 should fail that row. This is the smallest in-scope remedy: test the retained 80/160 contract, without reopening the menu migration.
Exact-head evidence
- PASS — clean worktree; local HEAD matched live PR head before verdict.
- PASS — full
just desktop-ciatcd4602b3…, including Desktop checks, 4,984/4,984 JS tests, production build, Tauri check, and full Tauri workspace tests. Existing unrelated diagnostics remain visible. - PASS — independent focused E2E runs for the profile Community actions path; combined affected smoke coverage passed 28/28.
- PASS — keyboard-opening the restored Community actions Popover with Enter completed the action path in a focused probe.
- FAIL — 80→500 mutation survives the new regression test, 1/1.
- NOT PROVEN — the 160 ms trigger-to-panel pointer bridge is not exercised by the new test.
- GitHub at verdict time: macOS build, all four smoke shards, both integration shards, relay E2E, release candidate, and DCO passed; Desktop Core remained pending. The PR was mergeable but blocked by review state.
No additional timer cleanup, keyboard, authorization, persistence, or destructive-action issue was found. Verdict expires on any head movement.
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
cd4602b to
c6c5c3e
Compare
|
Addressed Jude’s timing-test feedback at
AI-generated by Carl (Buzz agent). |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Re-review — CHANGES REQUESTED
Reviewed exact head c6c5c3ed0294b3020fa43f17fbcdf3a7449b4f37 against PR base a282e0643fe0f14ace4d9b57ead99d0635e38995.
The production change is clear: the shared 500 ms Tooltip/informational-Popover dwell is centralized, the Community actions 80/160 ms navigation-submenu exception remains explicit, immediate click/focus behavior is preserved, and the prior profile button/Popover keyboard model remains restored. I found no production lifecycle cleanup, accessibility, auth, tenancy, persistence, destructive-action, or platform-conditional regression in the changed boundary.
[P2] Replace the intrinsically racy Community actions timing regression
desktop/tests/e2e/community-rail.spec.ts:279-282 starts the production 80 ms open timer, sleeps 40 ms, then invokes Playwright's retrying toBeHidden(). Host scheduling or assertion setup can consume the remaining ~40 ms before the first observation; once the menu is visible, that negative assertion cannot recover. This is reproduced rather than hypothetical:
- one isolated exact-head series failed 3/4 runs: line 281 once, line 315 once, and line 322 once;
- a second isolated exact-head series failed 5/5 runs: line 281 four times and line 315 once.
The bridge drive does not model the documented route either. community-rail.spec.ts:305-308 moves from the trigger center to 4 px below it, while the portalled submenu is placed to the right (CommunitySwitcher.tsx:270-277). The test then waits 80 ms and runs retrying assertions before entering the menu only at line 323, leaving an unbounded portion of the 160 ms grace outside both trigger and panel. The exact-head failures show the production timer can expire before ingress. A 160→0 mutant failing this row therefore does not establish that the correct 160 ms implementation is tested reliably.
Consequence: correct code can randomly fail/retry, and green CI does not reliably prove the documented 80 ms open / 160 ms trigger-to-panel behavior. CI retries this suite twice (desktop/playwright.config.ts:5-6), so the current green result can conceal the defect.
Please instrument pointer-entry-to-aria-expanded=true timing (or use a controlled clock/event probe) and assert a tolerant bound that excludes immediate and 500 ms opening without racing a 40 ms margin. Drive from the trigger's right edge directly toward/into the right-side panel within a bounded interval, then assert sustained visibility. Require several repeated green candidate runs plus failing 80→500 and 160→0 mutants.
Exact-head evidence
- PASS — two independent source/accessibility/lifecycle reviews; production delta is clear.
- PASS —
pnpm typecheckin both review lanes. - PASS — changed-file Biome; full frontend tests 4,984/4,984 in the workflow/accessibility lane.
- PASS — focused composer Tooltip E2E 4/4, repeated across three isolated runs.
- FAIL — Community actions focused E2E: aggregate 1/9 passed, with failures across all three timing-sensitive assertions described above.
- PASS —
git diff --check; clean tree and local HEAD matched live PR head immediately before this verdict. - GitHub exact-head checks are green, including Desktop Core, all smoke/integration shards, macOS build, release candidate, and DCO.
- Native Tauri hover was not run because this shared host requires explicit GUI opt-in. Exact renderer E2E and the PR GIFs are the available visual evidence; this is residual risk, not the blocker.
Any new head invalidates this verdict.
Co-authored-by: Taylor Ho <taylorkmho@gmail.com> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
|
Addressed Jude’s timing-test feedback at
AI-generated by Carl (Buzz agent). |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Re-review — APPROVE
Reviewed: a282e0643fe0f14ace4d9b57ead99d0635e38995..2ef44d2c21e3faf14d4c96b122bbbfbaad7e5381 (exact head 2ef44d2c21e3faf14d4c96b122bbbfbaad7e5381)
Risk: medium — shared user-visible hover behavior, with this follow-up limited to repairing its regression proof.
The prior timing-test blocker is resolved. The new row timestamps pointer entry and the first aria-expanded="true" mutation in-page, enforcing a tolerant 40–300 ms window that excludes both immediate opening and the shared 500 ms informational-disclosure delay (desktop/tests/e2e/community-rail.spec.ts:279-324). It then exits from the trigger’s right edge toward the right-side portalled panel, measures a 60–140 ms bridge interval, enters the panel, and proves the menu never closed after opening (:326-358). This exercises the documented 80 ms open / 160 ms close-grace exception without the previous sleep-plus-retrying-negative race.
The production timer contract remains sound and unchanged: scheduling replaces pending work, controlled close clears it, unmount clears it, and trigger/panel entry share the same timer (desktop/src/features/communities/ui/CommunitySwitcher.tsx:117-161,262-274). Click/focus behavior, keyboard semantics, profile actions, accessibility, tenancy, persistence, auth, and native contracts are unchanged by this test-only head.
Exact-head validation
- PASS — independent clean E2E builds and two separate focused candidate cohorts, 10/10 each, one worker on isolated ports.
- PASS —
80 → 500mutant rejected at the upper-bound assertion, measured about 504 ms. - PASS —
160 → 0mutant rejected bydata-closed-after-opening="true", even when later pointer ingress reopened the panel. - PASS —
pnpm typecheck, changed-file Biome, andgit diff --check. - PASS — local clean heads and the live PR head were rechecked equal to
2ef44d2c…after restoring mutants. - PASS — applicable exact-head GitHub checks, including Desktop build/release, E2E, relay, unit, security, Rust, Windows, and DCO.
Manual/native evidence: no native Tauri GUI run under the shared-host safety rule. This head changes only renderer test code; exact renderer E2E and existing PR media cover the repaired boundary.
Residual risk: native pointer delivery was not independently re-exercised, but no production bytes changed from the previously reviewed production implementation.
No material findings remain. Any new head invalidates this approval.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Re-review — APPROVED
Reviewed exact head 2ef44d2c21e3faf14d4c96b122bbbfbaad7e5381 against PR base a282e0643fe0f14ace4d9b57ead99d0635e38995. The prior request for changes at c6c5c3ed0294b3020fa43f17fbcdf3a7449b4f37 is resolved.
The follow-up is test-only. desktop/tests/e2e/community-rail.spec.ts:279-358 now timestamps pointer entry and the first aria-expanded="true" transition in-page, enforcing a tolerant 40–300 ms opening bound that rejects both immediate opening and the shared 500 ms dwell. Its pointer path exits the trigger toward the right-side portalled panel, measures a 60–140 ms bridge outside both surfaces, and records any close-after-open transition before asserting sustained visibility and actionability. This matches the unchanged side="right" production geometry and exercises the documented 160 ms close grace without the former retrying-negative race.
Two independent exact-head reviews found no material product, accessibility, geometry, lifecycle, integration, or regression-test issue. Timer ownership remains unchanged and sound: rescheduling clears prior work, controlled close and unmount clear pending work, and trigger/panel entry share the same timer. The production boundary remains the previously reviewed one: click/focus stays immediate, ARIA state remains truthful, hover does not steal focus, informational disclosures retain the shared 500 ms default, and only this responsive navigation submenu keeps the explicit 80/160 ms exception. No persistence, auth, tenancy, network, schema, IPC, or release behavior changed in this follow-up.
Exact-head evidence
- PASS — focused Community actions journey 10/10 in each of two independent runs, one worker, after clean E2E builds; restored control also passed.
- PASS —
PROFILE_MENU_HOVER_OPEN_DELAY_MSmutant80 → 500rejected at the<300 msbound (measured 503.8/504.2 ms). - PASS —
PROFILE_MENU_HOVER_CLOSE_DELAY_MSmutant160 → 0rejected viadata-closed-after-opening="true", even when pointer ingress reopened the panel. - PASS —
pnpm typecheck, changed-file Biome, andgit diff --check; both mutation worktrees were restored clean at exact head. - PASS — GitHub exact-head checks are complete and green, including Desktop Core, all smoke/integration shards, macOS build, release candidate, security, and DCO.
Native Tauri hover was not rerun under the shared-host no-GUI safety rule. This head changes only renderer test code; exact renderer E2E plus the existing PR media are the available visual evidence. Residual risk is limited to native pointer delivery not being independently re-exercised in this round.
Any new head invalidates this approval.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Royal Court consolidated review — APPROVED
Reviewed exact head 2ef44d2c21e3faf14d4c96b122bbbfbaad7e5381 against PR base a282e0643fe0f14ace4d9b57ead99d0635e38995.
All four Court lanes are clear. The shared Tooltip wrapper establishes the intended 500 ms dwell and zero skip-delay contract, and the remaining nested providers inherit it without local timing overrides. Controlled informational Popovers consistently consume the shared 500 ms constant while preserving immediate click/focus paths, their existing close grace, and unmount cleanup.
The Community actions submenu is correctly isolated as an intentional responsive-navigation exception at 80 ms open / 160 ms close. Its repaired E2E records pointer entry and the actual aria-expanded transition, rejects both immediate and shared-500 ms opening, drives the pointer toward the right-side portalled panel, and records any close-after-open transition. That directly covers the contract without the earlier racy negative assertion.
No material product, accessibility, lifecycle, provider-boundary, timer-ownership, or regression-coverage issue remains.
Evidence
- Four independent Royal Court reviews returned clear/approve with no material findings.
- Exact-head search found no
delayDurationorskipDelayDurationoverride underdesktop/srcoutside the shared wrapper. git diff --checkpassed.- GitHub reports every applicable exact-head check successful or skipped, including Desktop Core, smoke/integration E2E, relay E2E, macOS build, unit, Rust, Windows, Security, and DCO.
- Live PR head was rechecked immediately before submission and still equals
2ef44d2c21e3faf14d4c96b122bbbfbaad7e5381; the PR is mergeable.
I did not duplicate CI-equivalent suites locally. Residual risk is limited to native Tauri pointer delivery not being independently exercised beyond renderer E2E and the supplied before/after media. Any head movement invalidates this approval.
…c-agent-commit-identity * origin/main: feat(managed-agents): close five Claude Code agent-config gaps (#4557) chore(hooks): keep mobile analysis out of pre-commit (#6236) fix(shared-ui): delay hover disclosures by default (#5821) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
* origin/main: (43 commits) perf(desktop): parallelize relay agent directory rebuild (block#6258) Refine the mobile emoji picker (block#5853) fix(desktop): exclude archived agents from nest, order regeneration (block#5905) Add font size and conversation density preferences (block#5644) fix(desktop): emit camelCase config-write payload fields (block#6062) fix(desktop): downscale large avatars for agent-share PNG body (block#6260) fix(desktop): preserve early relay auth challenges (block#3320) Polish mobile message actions (block#5873) Refine mobile pairing confirmation (block#6018) chore(scripts): add buzz-adopt-prod-agents.sh (block#6250) feat(managed-agents): close five Claude Code agent-config gaps (block#4557) chore(hooks): keep mobile analysis out of pre-commit (block#6236) fix(shared-ui): delay hover disclosures by default (block#5821) fix(desktop-chrome): preserve balanced layout when sidebar collapses (block#6000) Polish mobile timeline navigation (block#5874) chore(release): release Buzz Desktop version 0.5.17 (block#6234) fix(prompt): simplify pickup follow-through (block#6186) fix(mcp): scope todo usage (block#6216) fix(desktop): bound remote agent mention authorization (block#6224) fix: bump h2 for RUSTSEC-2026-0258 (block#6222) ... Signed-off-by: Princess Donut <3cb959c7eb65d61f634e61df318e450f18f82fa0e01849e7010b82666ead0587@buzz.block.builderlab.xyz> # Conflicts: # desktop/src/main.tsx # mobile/ios/Podfile.lock
…-in-thread * origin/main: (32 commits) Revert "fix(acp): gate relay-signed workflow messages on their attributed author" (#6311) fix(desktop): morph the drawer panel icon instead of sliding it (#6306) feat(desktop): refine repository-aware project workspaces (#6003) Fix mobile Activity thread navigation (#5850) perf(desktop): parallelize relay agent directory rebuild (#6258) Refine the mobile emoji picker (#5853) fix(desktop): exclude archived agents from nest, order regeneration (#5905) Add font size and conversation density preferences (#5644) fix(desktop): emit camelCase config-write payload fields (#6062) fix(desktop): downscale large avatars for agent-share PNG body (#6260) fix(desktop): preserve early relay auth challenges (#3320) Polish mobile message actions (#5873) Refine mobile pairing confirmation (#6018) chore(scripts): add buzz-adopt-prod-agents.sh (#6250) feat(managed-agents): close five Claude Code agent-config gaps (#4557) chore(hooks): keep mobile analysis out of pre-commit (#6236) fix(shared-ui): delay hover disclosures by default (#5821) fix(desktop-chrome): preserve balanced layout when sidebar collapses (#6000) Polish mobile timeline navigation (#5874) chore(release): release Buzz Desktop version 0.5.17 (#6234) ... Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
Category: fix
User Impact: Hover tooltips and informational popovers now wait for deliberate pointer dwell instead of appearing while users move around the app.
Problem: Tooltips and hover-controlled popovers appeared after inconsistent, often very short delays, so moving across composer and navigation controls could obstruct the next interaction.
Solution: Establish a 500 ms shared dwell default with no tooltip skip-delay cascade, apply it to informational hover-controlled Popovers, and preserve immediate click and keyboard behavior. The responsive Community actions navigation submenu retains its documented 80 ms open / 160 ms close timing.
Before / after
File changes
desktop/src/shared/ui/tooltip.tsx
Wraps the Radix provider with documented 500 ms and zero skip-delay defaults. The provider API still permits a future proven exception, but no current
desktop/srccaller overrides either timing.desktop/src/shared/ui/popover.tsx
Exports the documented shared hover-open timing for controlled popovers; ordinary click/focus Popovers remain immediate.
desktop/src/main.tsx
Uses the shared Tooltip provider defaults at the application root.
desktop/src/shared/ui/sidebar.tsx
Removes the sidebar's instant Tooltip timing override.
desktop/src/features/channels/ui/AddChannelBotTeamsSection.tsx
Removes the local short Tooltip timing override.
desktop/src/features/home/ui/InboxDetailPane.tsx
Removes the local short Tooltip timing override.
desktop/src/features/messages/ui/MessageTimeline.tsx
Removes the timeline's local short Tooltip timing override.
desktop/src/features/messages/ui/MessageTimestamp.tsx
Drops the timestamp-only provider now that its 500 ms, zero-skip behavior is shared globally.
desktop/src/features/channels/ui/BotActivityBar.tsx
Raises composer agent-activity hover dwell from 150 ms to the shared default while preserving immediate click/focus opening.
desktop/src/features/sidebar/ui/ChannelActivityPopover.tsx
Raises channel activity hover dwell from 250 ms to the shared default while preserving immediate focus opening.
desktop/src/features/profile/ui/UserProfilePopover.tsx
Reuses the shared hover timing in place of its equivalent local constant.
desktop/src/shared/ui/PubKey.tsx
Reuses the shared hover timing in place of its equivalent local constant.
desktop/src/shared/ui/markdown/InlineEmojiPopover.tsx
Raises emoji inspection hover dwell from 200 ms to the shared default while preserving immediate focus opening.
desktop/src/features/messages/ui/MessageReactions.tsx
Raises reaction inspection hover dwell from 200 ms to the shared default while preserving immediate focus and reaction clicks.
desktop/src/features/communities/ui/CommunitySwitcher.tsx
Documents the Community actions navigation submenu as an intentional timing exception: 80 ms to open responsively and 160 ms to preserve the pointer bridge into its portalled panel.
Reproduction steps
Move the pointer rapidly across each surface first, then hold it still over a labeled control. Hover-only disclosures should stay closed during transit and open after about 500 ms of deliberate dwell. Moving directly between adjacent Tooltip triggers should start a fresh 500 ms dwell rather than cascading the next Tooltip open immediately.
Verification
pnpm typecheckpnpm check(passes with three existing diagnostics outside this diff)pnpm test— 4,775 passedpnpm build