Repository navigation
Keep menus above message hover actions - #1300
Conversation
Extend the existing overlay priority owner across menus and pickers, preserve the target message highlight, and restore keyboard focus after dismissal. Co-authored-by: goose <goose@users.noreply.github.com> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Close activity before its guarded host focus handoff. Align reaction journeys with shared overlay priority and distinguish legal scroll clamping from a repeated message reveal. Co-authored-by: goose <goose@users.noreply.github.com> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Accept both closed and visibility-hidden toolbar states in the activity journey, reusing the existing floating-toolbar assertion. Product behavior and timing limits are unchanged. Co-authored-by: goose <goose@users.noreply.github.com> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Use the existing dialog final-focus guard with the lifecycle trigger and stable Settings fallback, so late confirmation closure cannot return to an older menu control. Cover fast unarchive during a held real exit and preserve deliberately moved focus. Co-authored-by: goose <goose@users.noreply.github.com> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Use runtime test discovery so parameterized test locations match Vitest run filters. Exercise real listing, selection and execution without repeating unrelated tests. Co-authored-by: goose <goose@users.noreply.github.com> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated source review via Wes's account.
No actionable introduced defects found. Source assessment: 9/10.
Reviewed head 002e97a65e72394404196e22cc2a73e0a426668c against base 6166034b30a7d3561cc9e8982fcb4d4fb8aab381. Coverage includes shared overlay classification, toolbar anchor/highlight preservation, preview suppression, emoji Escape restoration, activity retirement/error dismissal, lifecycle-dialog return focus, and the runtime Vitest discovery correction. The implementation reuses the shared overlay and guarded focus owners. The public description, five commit messages and full changed-file surface were inspected; the description has no attached images. No new disclosure finding identified.
Independent coverage: Rocket (systems/performance/lifecycle and tooling), Mantis (correctness/security/concurrency), Groot (product/accessibility/flows), integrated and source-checked by Star Lord.
Hosted timing review: the head and exact-base unit artifacts show 10,277 versus 10,267 passing cases (9 skipped in both). Summed assertion time increased from 934.26 s to 1,111.85 s, including slower unchanged files; separate hosted runs do not establish a change-caused performance regression. The new selector integration case passed in 4.43 s. Browser-wide timing comparison remains unverified.
Validation limits: source-only; no PR code, tests, builds or app launches executed by this review. One exact-head GitHub checks snapshot showed 38 successful checks and Windows validation skipped. Hosted results are evidence, not independent runtime verification. Actual focus timing, pointer/hit testing, screen-reader behavior, themes/responsive/enlarged-text rendering, native behavior and human acceptance were not exercised. This COMMENT review is not approval or merge authorization.
What this does
Keeps open menus and pickers clickable by making message hover actions and background hover previews yield to them—including the profile menu, composer pickers, and message actions. The target message stays highlighted while its own menu or reaction picker is open, even though its toolbar is hidden. Escape returns keyboard focus to the original control.
Why it matters
Moving toward a menu should not summon a message toolbar above it and steal the click. People should also retain a clear visual cue identifying the message they are acting on.
How it works
Extends the existing shared modal-layer protection to interactive overlays and removes the message-row-specific exception. Both message toolbars and hover previews use that owner; there is no new popup registry or per-menu suppression. Existing popup state and accessible trigger links distinguish active popups from inline lists, notifications, and supplemental previews.
Toolbars retain their anchor geometry while hidden, without painting, intercepting clicks, or entering keyboard navigation. Visibility releases before return focus; external emoji triggers use the existing guarded focus-restoration path. Ordinary composer Escape behavior is unchanged. No relay, persistence, native-host, or token changes.
Verification
f58732dd1: TypeScript and 26 focused owner tests pass; all 36 journeys across the complete floating-toolbar, Buzz-link, and user-status files pass in Chromium and WebKit with zero retries. This covers pointer/keyboard behavior, focus restoration, nested menus, responsive widths, both themes, and enlarged text.f58732dd1: TypeScript/full unit suite, design-system type/static checks, and destination policy; Rust correctly skipped because no Rust inputs changed. Pre-commit filename, icon, formatting/lint, and machine secret-scan hooks passed. The documented hook-wiring probe passed 41 cases (three optional isolated-lhm cases skipped). The stale worktree-local hook override was removed so the managed global hooks and current project jobs both run; no tracked tooling changes or hook bypasses.BUZZ_TEST_WORKERS=2, preserving the full test selection, original deadlines, assertions, and zero retries.emoji.spec.mjsfails at initial picker opening (line 401) identically on the clean unchanged base6166034b3in both engines. It is not claimed as passing or fixed.Coordination
#1298 independently changes reaction-hover activation in the same toolbar owner; the second change to land must preserve both behaviors. #1273 also touches the floating-toolbar browser file. Based on fetched
mainat6166034b3.CI follow-up
The first hosted run on
f58732dd1had 11 failures across six scenarios. This correction keeps the shared overlay rule and repairs the affected integration rather than reopening background toolbars.Evidence: all 10 activity/reaction engine failures reproduced locally before correction. The controlled scroll probe reproduced the failure in WebKit; its corrected case passed twice per engine. Independent architecture/runtime and test-honesty review found no blocker. The focused activity tests also hold the handoff to prove that deliberately moved focus wins; browser retirement covers reduced motion. Existing browser cases were extended (0 added / 0 removed in this follow-up), without retries, deadline changes, or disabled animations to mask failures.
Correction
dbd70617d: all 108 cases across the seven complete affected browser files pass in Chromium and WebKit (zero retries), along with TypeScript and 38 focused owner tests. Mandatory commit and push hooks passed, including the full unit suite with the documented two-worker limit, design guards, secret scanning and destination policy. All temporary fixture browsers and servers stopped; the existing desktop development app was left running. Hosted verification for this correction is pending. This PR remains draft pending human acceptance of the additional focus repair.Repeated-run assertion correction
On
dbd70617d, all main CI jobs passed, but the advisory WebKit repeated-run job found two failures in the activity journey'svisibility: hiddenassertion. The retained trace shows the targeted toolbar had already closed, with no native popover-open marker; its normal closed styling makes it transparent and non-interactive. The test rejected a valid suppression path rather than exposing a visible toolbar.Test-only correction
f9a592ddfshares the existing floating-toolbar paint-and-hit-testing assertion with the activity journey. Both callers require no painted toolbar and no pointer interception, while retaining the popup's own clickability check. No production code, retry policy, timeout, or tolerance changed; browser cases added/removed: 0/0.Both complete affected files passed ten consecutive repetitions in each engine: 240/240 checks, zero retries. Independent read-only review confirmed the trace and assertion correction. Mandatory commit/push hooks passed; production code is byte-identical to
dbd70617d. Hosted CI for this new head is pending.Latest hosted status
Run 39001504637 on
f9a592ddfpassed all overlay-related journeys and every repeated-test shard. The remaining required failure is WebKitchannel-archive-delete-pane.spec.mjs:350: after unarchiving, focus did not return to Close Channel settings tab. The other 89 cases in that shard passed.The same archive case passed five local WebKit repetitions on clean base
6166034b3and five on this head. Its test, channel lifecycle dialog, sidebar focus handoff, and pane owners are unchanged by this PR; the captured failing screen has no messages or message hover toolbar. Source inspection found an existing focus-handoff ordering risk, but the failure has not been reproduced deterministically or established as a proven flake. A bounded search of eight other recent failed CI runs identified two failing WebKit-1 logs; neither had this signature. The later controlled investigation below reproduced and repaired this focus failure; no blind CI rerun was used.Unarchive focus correction
The remaining hosted WebKit failure (
channel-archive-delete-pane.spec.mjs, unarchive completion) is now reproduced deterministically in both engines by holding the real confirmation exit animation while the channel returns. The unchanged code loses focus from Close Channel settings tab in that ordering.A targeted trace showed two restoration owners disagreeing: the sidebar successfully focuses the stable Settings close control, then the dialog's default focus history chooses the older Channel actions button. The correction passes the lifecycle operation's already-captured trigger and stable fallback into the existing guarded Dialog
finalFocusAPI. The target is resolved when focus returns, so a replaced Unarchive button falls back correctly. Cancellation remains sidebar-owned, pending/submission behavior is preserved, and deliberate user focus moves still win. No archive writes, permissions, navigation policy, timers, or new focus state were added.The existing browser journey now holds and releases the actual exit with an explicit barrier and
finallycleanup; its original focus assertion is unchanged. The existing moved-focus unit matrix now also covers unarchive. No browser cases added or removed, no timeouts/retries/tolerances changed, and all temporary diagnostic logging was removed.The first local push gate on this repair timed out in unchanged
store-system-discovery.test.tswhile machine load reached 55 on 16 logical CPUs (10,276 other tests passed). The complete discovery file then passed on clean base and this head with its original five-second deadline; the formerly timed-out case took 2.24s and 2.42s respectively. The push gate was rerun in full with the repository's supportedBUZZ_TEST_WORKERS=1; test selection, deadlines, assertions, and retries were unchanged.Repair
2e9161024: TypeScript and all 92 focused lifecycle/sidebar tests pass. Both complete related browser files pass in Chromium and WebKit (22 cases), and the controlled archive round-trip passes ten repetitions per engine (20 runs, zero retries). Independent architecture and test-honesty review found no blocker. Mandatory commit and push hooks passed, including the full unit suite with the documented worker-limit setting at one worker, design guards, secret scanning and destination policy. Temporary fixture browsers/servers are closed; the existing desktop app was left running. Hosted CI on this correction is pending.Repeated Vitest discovery correction
On
2e9161024, every main CI job and every repeated browser shard passed. The remaining advisory Vitest lane failed before running one parameterized group: static discovery listedChannelSidebar.test.tsx:775, but runtime line filtering recognizes line 774. The same error repeated in all ten iterations; it was not a product failure or random flake.Correction
002e97a65adds Vitest's--staticParse=falseto the existing listing command, so discovery and execution share runtime call-site locations. There is no new AST location-mapping layer, dependency change, retry, deadline adjustment, or test exclusion. The executable integration regression lists real.each/.forcases, selects their changed bodies, executes them, and proves an unrelated failing neighbor remains unselected. Removing the runtime-collection flag makes the regression fail.All ten selector integration cases passed (~6s), and the exact PR selection completed ten iterations successfully with all 13 selected cases running each time, including archive/unarchive/delete. Runtime collection costs more than static parsing but the measured local selector completed well within the five-minute selection job's budget. Independent review found no correctness blocker. Mandatory commit/push hooks passed; product source is unchanged from
2e9161024. Hosted CI for this head is pending.Final hosted verification
CI run 39010228964 passed on
002e97a65, including both JavaScript lanes, Rust/tool integration, browser measurements, all 18 Chromium/WebKit functional shards, macOS WebKit recording, all repeated-run shards (Vitest and both browsers), required CI, security checks, and DCO. Windows validation was not selected for this change.Final sweep found no unresolved review threads or outstanding comments. Required human/code-owner review is still missing (
REVIEW_REQUIRED); this is not approval to merge. Human confirmation of the later focus corrections remains separate from automated browser evidence. No merge was performed.