Repository navigation
fix(notifications): dismiss the attention ring on interaction, not on refocus - #160
Conversation
… refocus The blue attention ring stayed lit while the user typed into the very pane it was pointing at. The only way to clear it was to click a different pane and click back. PaneWrapper's clear-on-focus effect reads `hasUnread` but lists only `[isFocused]` as its dependency, so it runs on the focus TRANSITION and nothing else. A notification that arrives while the pane is already focused flips `hasUnread` true without touching `isFocused`, the effect never re-runs, and the ring sits there. Clicking away and back is the user manufacturing that missing transition by hand -- which is exactly why the workaround works. Adding `hasUnread` to that dependency array is the one-word fix, but it trades one problem for another: the effect would then run on the same render the notification arrives, so a pane you are already looking at clears its ring before ever painting it. No glow, and the 950ms `justFired` flash cut off mid-animation. So that swaps a ring that will not leave for a ring you can never see. So the ring is now dismissed by INTERACTION, which is what "I dealt with it" actually means: a keydown or a mousedown inside the pane, via listeners attached only while there is something to dismiss. Capture phase is load-bearing for keydown. xterm's hidden helper textarea is a descendant of the pane element and consumes the event, so a bubble-phase listener would see clicks but never a keystroke -- leaving the reported case (typing) broken while looking fixed. Bare modifiers do not dismiss (attention-dismiss.ts). A lone Shift or Control is the first half of a chord, and several of those chords are the user LEAVING the pane -- Ctrl+Alt+arrow switches panes -- so clearing on the modifier's own keydown would drop the ring a beat before the user's attention arrives. The chord's terminating key still dismisses, because that was typed here. The focus effect stays as it is. It is not redundant: switching panes by keyboard lands no click and no keystroke inside the pane it moves to. Marking every surface in the pane read is unchanged from the existing focus behaviour, and is deliberate -- the ring's own visibility is per-pane, so clearing only the active tab would leave it lit after the user had plainly interacted, reproducing the same complaint. Tests: 885 pass (was 881). 4 added, covering the dismissal rule. The listener wiring itself is not unit-tested -- this repo has no renderer test harness (vitest runs `environment: 'node'`, no jsdom) and adding one is out of scope here; the capture-phase behaviour needs a manual check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Merged in 1.0.0. The part worth calling out is the fix you didn't take. Adding Two details I'd have got wrong on my own: Capture phase for keydown. xterm's hidden helper textarea is a descendant of the pane element and consumes the event, so a bubble-phase listener sees clicks and never a keystroke. That is the exact shape of bug that passes a click-based check while the reported behaviour — typing — stays broken. Your coverage note says the same thing, which is why I'm not treating the missing listener test as a gap worth blocking on: a click-only test would pass even with the capture flag wrong. Bare modifiers don't dismiss. This is the one I would not have thought of until a user complained. Keeping the focus effect alongside the interaction listener is also correct and I'd have been tempted to delete it as redundant. Switching panes by keyboard lands no click and no keystroke inside the destination. On the jsdom harness: not needed for this, and I'd rather not add one as a passenger on a bug fix. If you want to do it as its own PR I'll take it, but the pure-function split you used here (same shape as Thanks for splitting this out of #159 rather than folding it in. Two independent things, two reviews, and I could take them in either order. |
Small one. The blue attention ring stays lit while you type into the very pane it's pointing at — the way to clear it is to click a different pane and click back.
Unrelated to #151 (this is the notification ring, not the sidebar status). I found it while testing that work and split it out, since it shares no files.
Cause
PaneWrapper's clear-on-focus effect readshasUnreadbut lists only[isFocused]as its dependency, so it runs on the focus transition:A notification that arrives while the pane is already focused flips
hasUnreadtrue without touchingisFocused, so the effect doesn't re-run and the ring stays. Clicking away and back is the user manufacturing that missing transition by hand — which is why the workaround works.Why I didn't take the one-line version
Adding
hasUnreadto that dependency array is the obvious fix, but it trades one problem for another: the effect would then run on the same render the notification arrives, so a pane you're already looking at clears its ring before ever painting it. No glow, and the 950msjustFiredflash cut off mid-animation. That swaps a ring that won't leave for a ring you can never see.Fix
The ring is dismissed by interaction, which is closer to what "I dealt with it" means: a keydown or mousedown inside the pane, via listeners attached only while there's something to dismiss.
Two details in there are load-bearing:
Capture phase for keydown. xterm's hidden helper textarea is a descendant of the pane element and consumes the event, so a bubble-phase listener would see clicks but never a keystroke — which would leave the reported case (typing) broken while looking fixed.
Bare modifiers don't dismiss (
attention-dismiss.ts). A lone Shift or Control is the first half of a chord, and several of those chords are the user leaving the pane — Ctrl+Alt+arrow switches panes — so clearing on the modifier's own keydown would drop the ring just before the user's attention arrives. The chord's terminating key still dismisses, because that was typed here.The rule lives in its own module because it's the part worth testing and it's invisible in a type check.
What stays the same
The focus effect is kept as-is — it isn't redundant, since switching panes by keyboard lands no click and no keystroke inside the pane it moves to.
Marking every surface in the pane read is unchanged from the existing focus behaviour, and deliberate: the ring's visibility is per-pane, so clearing only the active tab would leave it lit after the user had plainly interacted.
Verification
npx vitest run→ 885 passed, 78 files (881 in 77 onmaster). 4 tests added, covering the dismissal rule.npm run build:mainclean.npm run lint→ 17 errors / 15 warnings, identical to unmodifiedmaster. All pre-existing.masterat 0.50.0 (8122b69); nopackage.jsonchanges.Coverage gap I'd rather flag than have you find: the 4 new tests cover the dismissal rule (
keyDismissesAttention, a pure function — which is why it's in its own module), but not the listener wiring. There's no renderer test harness here — vitest runsenvironment: 'node'with no jsdom — and introducing one seemed out of scope for a bug fix.So the capture-phase behaviour is verified by hand rather than by CI: on Windows 11, a pane ringed while already focused clears on the next keystroke. That's the case that matters, because a click-only check would pass even if the capture-phase detail were wrong — xterm's helper textarea would still let the mousedown through — while the reported behaviour stayed broken.
If you'd like a jsdom harness added so this can be pinned properly, happy to do that as its own PR rather than fold it in here.