Repository navigation
Keep message hover actions separate from reaction pills - #1298
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
No actionable findings; no changes requested. Source assessment: 9/10 for minimalness, ownership and correctness.
Reviewed head 3e069b13a42ac0605954326260c390b0c9c81096 against base 6166034b30a7d3561cc9e8982fcb4d4fb8aab381. The existing toolbar owner suppresses reaction-only hover while retaining keyboard focus, open-menu state, deferred mounting and cleanup. Populated, empty-plus and fallback reaction markers match their callers. Shared IconButton, PreviewCard and picker ownership is preserved; reaction delivery and retry behavior remain unchanged. The browser changes retain native hover/paint checks and explicit preview/animation barriers without adding cases or relaxing screenshot equality.
Optional observation: reaction-list changes beneath a stationary pointer can leave the cached hover decision until a subsequent pointer transition. This is a transient source-level observation, not a demonstrated material defect or requested fix.
Inspected the public description, complete six-file diff and both commit messages; no new disclosure finding and no attached images in the description. One current-head CI snapshot showed 36 successful checks and one skipped check.
Source-only review: no tests, builds, app/browser/native execution or performance measurements were run by this review team. Live reaction failure/retry and focus behavior, both themes, narrow/enlarged-text rendering, touch/hybrid input and human acceptance remain unverified. CI and author-reported browser runs do not replace those checks. This COMMENT is not approval or merge authorization.
Preserve reaction previews, toggles and the inline picker without revealing the message toolbar over their hit areas. Keep keyboard access and open-menu retention unchanged. Co-authored-by: Carl <carl@users.noreply.github.com> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Check transparent retained toolbar controls through paint, hit testing and native popover state. Compare the inline plus in the same resting state and synchronize preview exit and keyboard focus before the next interaction. Both complete browser files pass in Chromium and WebKit (30 tests). Restoring the original hover owner makes the corrected regression fail in both engines; production code is unchanged by this commit. Co-authored-by: Carl <carl@users.noreply.github.com> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
3e069b1 to
a5b6ec9
Compare
What this does
Keeps the message toolbar out of the way when hovering an existing reaction or the inline plus button. Hover still shows who reacted, clicking an emoji still toggles that reaction, and plus still opens the picker. Hovering the message itself continues to reveal its actions.
Why it matters
Inspecting a reaction should not summon another set of controls over the message. This refines the hover interaction from #1219 without replacing its animation, previews, or reaction behavior.
How it works
The existing toolbar owner excludes the reaction capsule and plus button from pointer-hover activation. Keyboard access, open-menu retention, toolbar pointer travel, touch layout, and reaction delivery remain unchanged. Empty space beside a first-reaction plus remains part of the normal message hover area. Existing shared IconButton, PreviewCard, and picker owners are reused; no CSS, tokens, or shared component APIs change.
Rebased onto main
6166034b3(saved Goose sessions). The incoming commit does not touch any of the six changed files. The full feature patch and every changed file were byte-for-byte identical before and after rebase;git range-diffreports the commit unchanged. Open #1271/#733 change image or row sizing, and #1135 changes coordination presentation; this patch does not alter those owners. #1191 is the retained, superseded split reference.Verification
f5ca7db45: all 148 focused message-row, reaction, and action-bar component tests passed. The new regression failed on the original code because reaction hover left the toolbar revealed, then passed with the repair.f5ca7db45: TypeScript and related unit tests, design-system type/static checks, and destination policy. The Rust lane correctly skipped because no Rust inputs changed. Commit formatting, icon, filename and machine secret-scan hooks passed; diff checks are clean.Browser CI correction
The initial red browser jobs repeated two test-modeling errors, not ten independent regressions. The toolbar test treated transparent, non-interactive controls retained for keyboard navigation as hidden DOM; it now verifies opacity, pointer hit testing, and native popover closure. The icon test compared the plus button's hovered border with its resting state; it now rests in the reaction capsule's padding before and after neighboring animation, preserving exact full-button image equality. Preview-exit and keyboard-focus boundaries are explicit rather than timed sleeps. No production code, assertions' tolerances, retries, or browser coverage was removed.
Correction head
3e069b13a: both complete affected browser files passed in Chromium and WebKit, 30/30 in 1.4 minutes on macOS. Temporarily restoring the original toolbar owner made the corrected reaction-hover case fail for the intended visible-toolbar regression in both engines; the owner was restored byte-for-byte afterward. The production diff is identical tof5ca7db45. Independent review accepted the test corrections and lifecycle boundaries. Mandatory commit and push hooks passed on the correction head, including TypeScript/related unit tests, design checks, the secret scan, and destination policy. Hosted CI is pending for this head.