Repository navigation
Highlight the visible channel origin when a thread closes - #908
Conversation
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
One low-priority fix: restrict highlight cleanup to its own animation, so a message’s other animations cannot cut the return cue short (inline).
Star Lord automated source review via Wes’s account — head cdbf1013557499d106c48faf22cc5d6f60b3b73f, base 24ad1e8d21c5c18ed205e36fe5e645cccabc85a6. Source-only: no tests or app execution; browser/native behavior and hosted CI remain unverified.
| ) | ||
| return; | ||
| row.classList.add(styles.returnHighlight ?? ""); | ||
| row.addEventListener("animationend", clear); |
There was a problem hiding this comment.
[P3] Ignore descendant animations when retiring the return cue
animationend bubbles, but this listener treats every event from the message subtree as completion of the highlight. A live reaction-count change uses a separate 260ms animation (MessageReactions.tsx:324, Messages.module.css:261–287); if its end event arrives during the 1100ms return cue, clear() immediately removes the wash instead of allowing its own fade to finish. The reaction component also has a 260ms removal timer, so this depends on event/timer ordering rather than happening on every update.
Filter completion to the highlighted row’s own ::after animation (for example, check the event target and pseudo-element) before clearing. Keep unconditional cleanup for cancellation/unmount, and cover a descendant animation-end event followed by the actual highlight completion.
There was a problem hiding this comment.
🤖 Fixed in 64dc783. Animation completion now clears the cue only when its target is the highlighted row and its pseudo-element is ::after. Descendant reaction animations and unrelated row/::before completions are ignored; explicit cancellation, channel changes and unmount still clear unconditionally.
A real mounted-hook regression failed before the repair and passes afterward, including descendant completion followed by the actual cue completion and lifecycle cleanup. Typecheck, three focused tests, all 16 navigation-thread-history cases in Chromium/WebKit (including real pseudo-animation retirement), and push hooks pass. Independent focused review is clear. Fresh CI is running; leaving this thread open for verification.
baxen
left a comment
baxen
left a comment
There was a problem hiding this comment.
🤖 Approving on behalf of baxen. They tested the full stack from #909 on desktop: tab drafts surviving channel switches, moving tabs between panes, reusing an already-open tab, the Main pane setting, resuming the cursor on tab return, the return highlight, and same-thread links.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 No new blockers. The open P3 on animationend bubbling still applies at this head and should be fixed: check event.target === row && event.animationName before clearing. Close-time visibility gating, channel-change cleanup and reduced-motion handling look right.
Validation: source review only. The current CI run was cancelled, not failed, so this head has no completed CI signal. Rerun after the stack is rebased onto #906.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
24ad1e8 to
5e7a1ee
Compare
Paint only the confirmed visible origin with a rounded accent wash; no scroll, fetch or delayed cue. Co-authored-by: Carl <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Only the highlighted row's own after-pseudo animation retires the cue. Keep explicit cancel, channel change and unmount cleanup unconditional, with a real-hook regression. Co-authored-by: Carl <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
cdbf101 to
64dc783
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
No further changes requested for the return-cue repair. The animation-ownership finding is addressed: only the row’s own ::after completion retires the cue, while explicit cancellation, channel change and unmount still clear it; the mounted-hook regression covers those paths.
Star Lord automated source review via Wes’s account; head 64dc7834bcd62df3860694b062953584031c143a, base ebadc94c86a52940013748794f23a5f486ecc0fa. Source-only; no tests/app executed. Hosted CI was incomplete at the snapshot; fresh browser/native acceptance was not independently verified. This follow-up does not clear the separately documented stack findings or deferred pane-rewrap behavior.
3 participants
What this does
Closing an active thread briefly highlights its original message when that message is already visible in the channel. The rounded accent wash covers the message only, not the date divider, and extends into the existing side inset without changing layout.
This is slice 5, rebased onto main including merged #904–907 and #850. Escape still closes the thread tab; this change does not alter which tab becomes active or the approved return-to-channel navigation.
Why it matters
Returning from a thread should make it easy to recognize which message it came from. The cue provides orientation without scrolling the channel away from the person's reading position.
How it works
A one-shot check uses the confirmed thread root after the close commits. Hidden channels, offscreen or unloaded messages and inactive-tab closes get no cue. It does not fetch history, move focus, or queue a highlight for later. Reduced motion shows and removes a static tint rather than fading it.
Review repair and verification
64dc7834bcd62df3860694b062953584031c143a, baseebadc94c8. The return cue now retires only when the highlighted row’s own::afteranimation ends. Descendant reaction animations, row-element animations and::beforecannot cut it short. Explicit cancellation, channel change and unmount still clear unconditionally.Stack
Slice 5 of 6. Based on main containing merged Dragging #907. Next: Composer focus #909, to be rebased after this merge.