Repository navigation
feat(review): hover card trigger settings and first-run introduction - #1462
Merged
Merged
Conversation
…ement
Hover-for-everyone is a good default and a bad only-option. Two cookie-only
settings now cover the three complaints it produces, and a one-time dialog
puts the choice in front of the reviewer once.
- @plannotator/core/token-hover (shimmed to shared): TokenHoverTrigger
(hover | modifier | off), TokenHoverDelay (200 | 350 | 700), and the
migration that reads the old tokenHoverCards boolean once.
- settings.ts: tokenHoverTrigger REPLACES tokenHoverCards rather than sitting
beside it, so there is no unreachable enabled-but-off state. A legacy
`false` resolves to `off`; the legacy cookie is read, never written, so a
downgrade cannot silently re-enable cards for someone who turned them off.
- useTokenHover: an options argument, { mode, delayMs }. In modifier mode the
Alt gate sits ahead of the dwell timer, the cache read and every piece of
state, so a hover with the key up costs one boolean read. Pressing Alt while
already parked on a token opens the card (a key press fires no pointer
event, so the last enter is remembered); releasing it starts the same leave
grace, except while the pointer is inside the card; window blur clears a
stale held flag; and typing owns the key, since Alt+Backspace and Alt+arrow
are word-editing chords. The three listeners exist only in modifier mode, so
hover mode is byte-for-byte what shipped.
- The diff views are untouched: their prop signatures are unchanged, which
matters because both are compiled into the portable guide viewer. `off` is
still enforced in App by withholding the handler props entirely.
- TokenHoverAnnouncementDialog: last in the first-run chain, compact, with a
radio group that applies the trigger immediately so Done and Escape both
mean "accept what is selected". It skips a session where cards cannot run
without consuming its cookie, and never shows to a reviewer who already has
a non-default trigger.
- Settings > Review Display: the toggle becomes a trigger segment plus a delay
segment, the delay inert while the trigger is off.
Click behavior is deliberately untouched. Cmd/Ctrl+click and Alt+click both
still open the References panel; every gesture on a token is already spoken
for, and the modifier mode delivers what a click mode would be wanted for.
Alt rather than Cmd because Cmd/Ctrl+hover already paints the navigable-target
affordance, and Alt is spelled the same on every platform. Reasoning and the
rejected controls are in DESIGN_hover-settings.md.
The guides.show viewer manifest moves with this: the settings registry is in
the portable viewer's graph.
M1 stale pointerInCardRef. onCardEnter set "the pointer is inside the card" and only onCardLeave cleared it, but a scroll-close or a window blur unmounts the card UNDER the pointer, so no leave ever arrives. The flag then stayed true for the rest of the session and every later Alt release was ignored as "they are reading the card", leaving cards stuck open. close() now resets it, which covers the blur path too since onBlur closes. M2 Alt+click double surface. handleCodeNavRequest closes the hover first, so EVERY route into the References panel is covered by one line: Cmd+click, Ctrl+click, the Alt+click alias, and the card's own location links. It sits ahead of the no-checkout early return, because a click that cannot resolve must still dismiss the card it was made on top of. This also repairs the overlap #1461 shipped with, where a click on a token whose card was open or mid-dwell left both surfaces on screen; close() cancels the pending dwell and aborts an in-flight request, so a click during the dwell can no longer resolve into a card behind the panel. Alt+click is where this is routine rather than incidental: in modifier mode the alias and the trigger share the key. Minors: - The announcement's featureAvailable is LATCHED at the first post-load render, like guideIntroEligibleRef. canUseLiveWorkspaceActions changes on mid-session diff switches, so an unlatched read could pop the dialog over work in progress or yank an open one away mid-read. - resolveTokenHoverAnnouncementPending is pure again, like its editMode sibling. The cookie write that retires the announcement for an already-decided reviewer moved to shouldConsumeTokenHoverAnnouncement, called from an App effect: a state initializer can run more than once, and a getter that writes storage goes unnoticed until StrictMode double-invokes it. - The migration is documented as what it is. fromCookie RE-READS the legacy key on every load until the user touches the setting, because a migrating read returns a value and so never triggers the registry's default-seeding write. Resolution is pure and identical every time, so the repeated read costs one lookup and cannot drift; seeding inside a getter was rejected as more code for no behavioral gain. DESIGN §7, the settings comment and AGENTS.md all say this now instead of claiming a one-time seed. - SegmentedControl builds its classNames from a filtered array (no double or trailing spaces) and only puts `disabled` on the button when true. Nits: the Cmd-only copy in the dialog note and the Settings row is now platform-aware through a new modKeyWord export ('Cmd' / 'Ctrl'), which is the prose spelling modKey's glyph cannot serve; and the dialog's radio group follows the WAI-ARIA roving-tabindex pattern, so Tab enters and leaves it as one stop while arrows move the selection and wrap. Tests: M1's exact reviewer sequence (hold Alt, open, pointer into card, scroll-close, hover another token, release, assert it closes after the grace) fails without the reset; the References-handoff mechanism (open card gone, pending dwell cancelled, in-flight request aborted) is behavioral in the hook suite, and codeNavHoverHandoff.test.ts pins the App wiring at source level the way iframeIsolation does, failing without the closeTokenHover call. Also: blur clears the held state, arrows move the radio selection, and the announcement latch no longer writes. The dialog suite additionally seeds its trigger cookie: configStore is a process-global singleton whose loadFromBackend keeps the in-memory value when the new backend is silent, so an unseeded map inherited another file's last write. The new roving-focus test is what surfaced that.
Owner
Author
|
Real screenshots of the hover-settings work, captured with Playwright against the built app ( 1. First-run announcement dialog. Reached through the real dialog chain on a fresh browser profile (guide intro "Got it", look-and-feel "Continue", edit mode "Done", then this arrives naturally). Note: the review-setup dialog did not appear in the chain; see the finding below.
2. Same dialog with "While holding Alt (Option)" selected. The radio writes the setting on selection; Done just closes.
3. Settings, Editor tab, defaults. The new controls ship under Settings > Editor (not a "Review Display" tab; see finding below): the "Token hover cards" trigger segmented control (On hover / Hold Alt / Off, On hover selected) and the delay segmented control captioned "How long the pointer rests before a card is requested" (Fast / Default / Relaxed, Default selected).
4. Off selected. The delay control stays visible but its three buttons are truly
5. Hold Alt selected. The delay control is re-enabled.
Findings from driving the real build:
|
…o Hover cards The announcement was too plain for a first-use dialog. It now uses the repo's big-format shell (max-w-5xl, badge/title/description header, a 1.1fr/1fr body that collapses to one column under 820px, footer with the Settings pointer and one action), matching EditModeAnnouncementDialog and LookAndFeelAnnouncementDialog. The left column, where the Edit Mode dialog plays its recording, is a worked example: a three-line strip of diff with `charge` shown under the pointer, and below it a hover card reproducing the real card's anatomy in the order TokenHoverCard renders it (name plus kind badge, approximate signature with its matched-line marker, doc line, Defined at, a reference sample ending in the overflow line). It is JSX over the existing theme tokens rather than a bitmap, so it is crisp at any DPI, follows the active palette in both light and dark, and cannot go stale against a card whose anatomy changes. It is decorative: the whole block is aria-hidden, carries nothing focusable, and every fact it shows is also stated in the prose beside it. The right column is unchanged in behavior: the same three-option radio group, the same immediate apply, the same roving-tabindex keyboard model, and the click-behavior note (now also carrying the ripgrep requirement, which the Settings row already stated). The footer keeps the Editor-tab destination d04cfce corrected. Rename: the user-facing name is "Hover cards" everywhere a reader meets it, in the dialog title and the Settings heading. "Token" described the implementation's unit rather than the thing the reviewer points at, and in an app full of annotations "token hover card" invites reading it as a kind of comment card. Every internal identifier is untouched: tokenHoverTrigger, tokenHoverDelay, the cookie keys, the hook and component names, and the data-token-hover-* test ids all keep their spelling, so there is no migration. Tests: the behavioral assertions are unchanged (once-ness, radio applies, dismiss and Escape keep the choice, arrows move selection), plus one that the example block is aria-hidden and holds nothing focusable, because read aloud it is a wall of invented identifiers that says nothing about the choice on offer. The guides.show CSS bundle hash moves with this: Tailwind scans source for class names, and the example introduces utilities the shared stylesheet did not carry. The JS bundle is unchanged, as expected for an app-side dialog.
… card
The example was a hand-drawn impression of a hover card. A fork of that markup
drifts from the shipped card the first time either changes, which is exactly
what an example is supposed to prevent. It is now the real thing, and it is
interactive: the `withRetry` token in the strip is genuinely hoverable, and
resting on it opens the REAL TokenHoverCard through the REAL useTokenHover, so
the reviewer feels the actual dwell, leave grace and Alt gate before committing
to a setting.
Nothing is reimplemented. Three thin seams carry it:
- useTokenHover gains `resolve`, defaulting to the POST /api/code-nav/hover it
always did. The try-it returns a hardcoded CodeNavHoverResponse instead. This
is the ONLY fixture: a demo must not search the reviewer's repository for a
symbol they never asked about, and a second copy of the dwell/supersession/
grace logic would drift. The resolver is read through a ref so an inline
literal cannot re-arm the hook's callbacks every render.
- TokenHoverCard gains `layerClassName` (default `fixed z-[60]`, unchanged).
The card portals to <body> like every instance, so the one thing its host has
to be able to say is which stacking context it belongs in; the try-it puts it
at z-[110], above the z-[100] modal it is demonstrated inside.
- TokenHoverCard gains `inert`, which drops the location buttons out of the tab
order. The try-it has no References panel behind it, so they lead nowhere.
The card body was extracted to TokenHoverCardContent so the positioned wrapper
and any other host share one copy; every class, section and separator is the
same markup the live card renders, not a duplicate.
The hovered token wears the diff pane's own underline treatment, now defined
once in tokenHoverStyles.ts: usePierreTheme serializes it into Pierre's shadow
stylesheet as `.pn-token-hover`, the dialog applies the same declarations as a
style object. Two consumers, one definition.
The try-it reads the LIVE setting rather than a prop, so flipping the radio to
"While holding Alt" makes the demo behave that way immediately and "Off" makes
it do nothing and closes any open card. That is the honest preview of each
choice, which is the point of putting the control next to the demo.
Because it is interactive it is labeled rather than hidden: the region carries
role="group" and an aria-label, the visible prompt line ("Try it: rest your
pointer on withRetry") carries the meaning, and only the mock code lines stay
aria-hidden, since read aloud they are a wall of invented identifiers.
Tests: hovering the token opens the real card (asserted through the component's
own data-token-hover-card marker) with the fixture's symbol and definition path
after the dwell and not before; the try-it obeys the live trigger, so under
"Hold Alt" a rest opens nothing until Alt goes down, and choosing Off takes an
open card away; and the region is labeled while its mock code is hidden. React
synthesizes onPointerEnter from pointerover, which is what the tests dispatch.
The guides.show viewer hashes move on both halves: usePierreTheme is in the
viewer's graph and now imports the shared style module (JS), and the try-it
introduces Tailwind utilities the shared stylesheet did not carry (CSS).
Budgets are unaffected (395.7 KB gz of 400).
…o hover Two defects the browser capture caught, both invisible in the DOM tests. The prompt said "rest your pointer on withRetry" and the card that opened was titled `charge`: the fixture still described the symbol from the earlier static example. It now describes withRetry, and takes its name from EXAMPLE_SYMBOL so the prompt and the card cannot disagree again. The hovered token sat mid-line, and the card anchors at the token's left edge and is 400px wide, so it opened across the radio column the reviewer is meant to be reading. The token now leads the line, which keeps the card inside the example's own column.
Owner
Author
Hover cards: the announcement is now a try-it on the real cardRecaptured with Playwright against the built app ( The dialog, with the try-it card openThe
The try-it card and a real card, side by sideLeft: the card the try-it opens inside the dialog. Right: a real hover card opened by hovering
Same component, verified in the browser rather than by eye:
The right card shows no doc line and the left one does, because the real search found no doc comment for How it is wiredNo markup or timing is duplicated. Three thin seams:
The hovered token wears the diff pane's own underline treatment, defined once in The try-it reads the live setting, so choosing "While holding Alt" makes the demo behave that way immediately, and "Off" makes it do nothing and closes any open card. RenameUser-facing name is "Hover cards" in the dialog title and the Settings heading. Every internal identifier is unchanged ( Checks
|
Maintainer ruling. Two reasons, both recorded in DESIGN_hover-settings.md §4.1
in place of the old Alt argument:
- Alt is very commonly bound to push-to-talk dictation, so an Alt-held gate
would open cards every time the user starts speaking, with the pointer
wherever they left it. That is the exact failure this setting exists to
prevent. Alt-hold is also already taken inside Plannotator, by the plan
editor's temporary input-method switch.
- Cmd+hover is already VS Code's "tell me about this symbol" gesture: the
navigable-target underline plus a definition peek under one held key.
Gating on Cmd rides that muscle memory instead of competing with it.
The old anti-Cmd argument is moot. The underline and the card appearing
together under a held Cmd is not a collision, it IS that composite gesture; and
handleCodeNavRequest already dismisses the hover surface on every References
invocation, so Cmd+click cleanly supersedes an open card or a pending dwell
rather than stacking with it.
The key is platform-aware through two new helpers beside the existing ones in
packages/ui/utils/platform.ts: `isModKeyHeld` (metaKey on macOS, ctrlKey
elsewhere) and `modEventKey` ('Meta' / 'Control'). Labels come from the
existing modKeyWord, so the key the code checks and the key the copy names
cannot disagree. Labels updated in the dialog radio and its description, the
Settings segmented control, the design doc and AGENTS.md.
Only the modifier ALONE arms. Any other key going down while it is held
(Cmd+C, Cmd+V, Cmd+S) disarms and takes an open card with it, so a copy
performed with the pointer parked over the diff cannot pop a card mid-copy.
This was cheap: the keydown handler already had to compare event.key. The
typing-target guard stays as belt and braces.
Blur handling stays and matters more now: on macOS the app switcher is the same
key this gate arms on, so Cmd+Tab is the common way to leave with it held.
Internal ids are untouched. The stored trigger value is still `modifier`,
because the setting names the shape of the gate and not which key fills it, so
nothing migrates. The #1461 Alt+click References alias is unrelated and is left
alone.
The try-it feels the new gate for free, since it drives the same hook: choosing
the hold option and holding Cmd over the demo token opens the demo card.
Tests: the gate suites now build their events from the platform helpers, so
they exercise Cmd on macOS and Ctrl on the Linux runner rather than pinning one
platform. All behavioral assertions are unchanged. Two new ones cover the chord
rule, and both fail without it: a copy chord with the pointer parked on a token
opens nothing, and a chord while a card is open closes it.
Owner
Author
|
Updated announcement dialog after 51e85d5: the modifier gate now reads "While holding Cmd" (Ctrl elsewhere), and the footer note leads with Cmd+click.
Try-it demo with On hover selected, the live card opens on the withRetry token inside the dialog.
|
…spaced 150/300/700
The 350 -> 300 sweep in b553835 rewrote both advances in this test, but they SUM to the configured dwell rather than each naming it: 300+300 is 600, so a 700ms delay never elapsed and the suite went red at the tip. Back to 350+350 with the pairing spelled out, so the guard stays meaningful in both directions: the first advance is past the 300 default (a hook that ignored delayMs would have fired already) and the pair reaches exactly 700. Verified fail-closed by pinning the timeout back to DEFAULT_TOKEN_HOVER_DELAY_MS, which fails this test and nothing else. The dialog test's two advances become DEFAULT_TOKEN_HOVER_DELAY_MS itself: its dwell IS the registry default (the seeded backend says nothing about tokenHoverDelay), so naming the constant is both correct and immune to the next respacing.
Two defects in one branch of the modifier gate's keydown handler. The guard read event.target, which a window-level listener receives RETARGETED to the shadow host. Pierre's edit-session editor is a contenteditable inside that shadow root, so a bare Cmd pressed while editing code in place read as a plain <div>, armed the gate, and popped a card over the text being edited: verbatim the failure "typing owns the key" exists to prevent. The three other window-level typing guards in this package (AllFilesCodeView, FileTree, SectionsPanel) already read composedPath()[0] for exactly this reason and say so in comments; this one is no longer the exception. The guard also sat ahead of the chord branch, so it suppressed DISARMING as well as arming. Focus reaches a composer between the two routinely (a card is open, the reviewer clicks into the comment box and pastes), and that left the card standing over the diff with the gate still armed behind it. The chord branch now runs first and typing only gates the arm path. keyup deliberately keeps no typing guard: if the gate never armed the branch is already a no-op, and if it did arm the release has to be honored wherever focus has since travelled. Tests: the old "modifier chords in a comment box" case exercised the chord branch, not the guard, so it passed with isTypingTarget deleted. Replaced by a bare-modifier press from a textarea and one from a shadow-root editor, plus a chord-while-typing case for the asymmetry; the two pre-existing chord-disarm tests stay, so that coverage is unchanged. The shadow event is synthesized rather than dispatched from a real shadow root because happy-dom does not retarget, which would have made the test vacuous. Each of the three fails closed against its own defect: reverting composedPath fails only the shadow case, deleting the guard fails both typing cases, and moving the guard back above the chord branch fails only the asymmetry case.
The ruling that settled Cmd over Alt was that the underline and the card appearing together under one held key IS the composite gesture rather than a collision. The code did not deliver it: the diff views paint pn-token-nav from the pointer ENTER event, and neither half of the gesture this mode exists for fires one. Parking on a symbol and THEN holding the key opened a card on a token wearing no affordance, and releasing closed the card while leaving the affordance painted until the pointer eventually left. useTokenHover now reports the arm and disarm transitions through an optional onModifierGate(armed, tokenElement), always carrying the token the pointer is parked on RIGHT NOW so a disarm also unpaints a neighbour the pointer drifted onto while the key was down. Every disarm route is covered: chord, release, blur, and the effect cleanup when the reviewer switches trigger with the key still held. The release notifies ahead of the reading-the-card early return, because the key being up ends the affordance whether or not the card is being read. App owns the painting, not the hook: the diff views are compiled into the portable guides.show viewer and their prop signatures must not move, and the class is a review-app concern. Enter-time painting is untouched, so plain Cmd+hover in the default hover mode behaves exactly as before. Also gates pn-token-hover on a hover handler actually being wired, and paints it in AllFilesCodeView. That class carries a cursor: pointer that became !important in eebab51, so painting it unconditionally started promising clickability on every token in a session with hover cards switched off; and leaving it out of the all-files view meant the affordance the announcement's try-it demonstrates was missing from the default review surface. A read-only or portable host passes no hover handler and still gets neither class. Fail-closed: dropping notifyGate(true) fails both new hook tests, dropping the App wiring fails the App pin, and making the DiffViewer paint unconditional fails the gating pin.
Comments first, because several of them had become false and this repo treats them as contract. The dialog cited a StaticTokenHoverCard that no longer exists and a card that "lives inside an aria-hidden decorative block" (it does not; the try-it region is a labelled role=group), the try-it still named "the Alt gate", and two hook comments plus four test comments still described the trigger key as Alt or claimed the Alt+click alias and the trigger share a key, which stopped being true when the gate became Cmd. The focus trap's rationale claimed the example card's buttons were in its query; the card portals to <body>, so they never were. tokenHoverStyles now single-sources what it says it does. Only thickness and offset were actually shared; the decoration line and the cursor were written twice and could drift. Both forms read one object, and the two genuinely per-form bits (the resolved color, and the !important the shadow form needs to beat Pierre's I-beam) are named as such. The display-only card stops advertising clicks it swallows: its own footer says "Click a location to jump", so under `inert` the locations lose the hand cursor and hover underline as well as the tab order, and the card gets aria-hidden — it portals outside the aria-modal dialog it belongs to, where a screen reader would meet it as a stray tooltip. The announcement's availability latch moves from the render body into an effect: a ref written during render is a side effect React may discard. The gate independently requires !isLoading, so the ordering is unchanged. Both announcement test files now re-hydrate configStore in afterEach. They re-resolve a process-global singleton from a two-entry fake map, and the pure lane runs in one process, so they were handing their own defaults to whatever file ran next. The dialog test gains the two assertions that were missing: the demo card sits at z-[110] (the layerClassName seam's entire purpose is not being buried under the modal it is demonstrated inside) and its locations are non-interactive. Both fail closed against dropping the respective prop. Design doc: §4.2 still listed 200/350/700 with a 350 default, which the maintainer ruling in §6 had already replaced; §3's 350 now reads as the history it is; §6 records how the composite gesture is delivered and §9 how the hovered-token treatment is gated.
The manifest was synced before the last three commits of this branch and went stale: usePierreTheme (which serializes the token-hover CSS into Pierre's stylesheet), the token-hover default constant, and now AllFilesCodeView are all inside the viewer bundle, so the JS hash moved while the CSS hash did not — the .pn-token-hover rule is a JS template string, not a stylesheet entry, which is exactly why the drift was easy to miss by eye. check:manifest was failing at the tip. Rebuilt and synced last, so this pins the branch's final source. Budgets after: js 395.8 KB gz against a 400 KB budget, css 50.9 against 64, worker still import-free.
backnotprop
added a commit
that referenced
this pull request
Sep 3, 2026
…1462) * docs(review): design the token hover card trigger settings and announcement * feat(review): token hover card trigger settings and first-run announcement Hover-for-everyone is a good default and a bad only-option. Two cookie-only settings now cover the three complaints it produces, and a one-time dialog puts the choice in front of the reviewer once. - @plannotator/core/token-hover (shimmed to shared): TokenHoverTrigger (hover | modifier | off), TokenHoverDelay (200 | 350 | 700), and the migration that reads the old tokenHoverCards boolean once. - settings.ts: tokenHoverTrigger REPLACES tokenHoverCards rather than sitting beside it, so there is no unreachable enabled-but-off state. A legacy `false` resolves to `off`; the legacy cookie is read, never written, so a downgrade cannot silently re-enable cards for someone who turned them off. - useTokenHover: an options argument, { mode, delayMs }. In modifier mode the Alt gate sits ahead of the dwell timer, the cache read and every piece of state, so a hover with the key up costs one boolean read. Pressing Alt while already parked on a token opens the card (a key press fires no pointer event, so the last enter is remembered); releasing it starts the same leave grace, except while the pointer is inside the card; window blur clears a stale held flag; and typing owns the key, since Alt+Backspace and Alt+arrow are word-editing chords. The three listeners exist only in modifier mode, so hover mode is byte-for-byte what shipped. - The diff views are untouched: their prop signatures are unchanged, which matters because both are compiled into the portable guide viewer. `off` is still enforced in App by withholding the handler props entirely. - TokenHoverAnnouncementDialog: last in the first-run chain, compact, with a radio group that applies the trigger immediately so Done and Escape both mean "accept what is selected". It skips a session where cards cannot run without consuming its cookie, and never shows to a reviewer who already has a non-default trigger. - Settings > Review Display: the toggle becomes a trigger segment plus a delay segment, the delay inert while the trigger is off. Click behavior is deliberately untouched. Cmd/Ctrl+click and Alt+click both still open the References panel; every gesture on a token is already spoken for, and the modifier mode delivers what a click mode would be wanted for. Alt rather than Cmd because Cmd/Ctrl+hover already paints the navigable-target affordance, and Alt is spelled the same on every platform. Reasoning and the rejected controls are in DESIGN_hover-settings.md. The guides.show viewer manifest moves with this: the settings registry is in the portable viewer's graph. * fix(review): hover card review findings, M1/M2 and minors M1 stale pointerInCardRef. onCardEnter set "the pointer is inside the card" and only onCardLeave cleared it, but a scroll-close or a window blur unmounts the card UNDER the pointer, so no leave ever arrives. The flag then stayed true for the rest of the session and every later Alt release was ignored as "they are reading the card", leaving cards stuck open. close() now resets it, which covers the blur path too since onBlur closes. M2 Alt+click double surface. handleCodeNavRequest closes the hover first, so EVERY route into the References panel is covered by one line: Cmd+click, Ctrl+click, the Alt+click alias, and the card's own location links. It sits ahead of the no-checkout early return, because a click that cannot resolve must still dismiss the card it was made on top of. This also repairs the overlap #1461 shipped with, where a click on a token whose card was open or mid-dwell left both surfaces on screen; close() cancels the pending dwell and aborts an in-flight request, so a click during the dwell can no longer resolve into a card behind the panel. Alt+click is where this is routine rather than incidental: in modifier mode the alias and the trigger share the key. Minors: - The announcement's featureAvailable is LATCHED at the first post-load render, like guideIntroEligibleRef. canUseLiveWorkspaceActions changes on mid-session diff switches, so an unlatched read could pop the dialog over work in progress or yank an open one away mid-read. - resolveTokenHoverAnnouncementPending is pure again, like its editMode sibling. The cookie write that retires the announcement for an already-decided reviewer moved to shouldConsumeTokenHoverAnnouncement, called from an App effect: a state initializer can run more than once, and a getter that writes storage goes unnoticed until StrictMode double-invokes it. - The migration is documented as what it is. fromCookie RE-READS the legacy key on every load until the user touches the setting, because a migrating read returns a value and so never triggers the registry's default-seeding write. Resolution is pure and identical every time, so the repeated read costs one lookup and cannot drift; seeding inside a getter was rejected as more code for no behavioral gain. DESIGN §7, the settings comment and AGENTS.md all say this now instead of claiming a one-time seed. - SegmentedControl builds its classNames from a filtered array (no double or trailing spaces) and only puts `disabled` on the button when true. Nits: the Cmd-only copy in the dialog note and the Settings row is now platform-aware through a new modKeyWord export ('Cmd' / 'Ctrl'), which is the prose spelling modKey's glyph cannot serve; and the dialog's radio group follows the WAI-ARIA roving-tabindex pattern, so Tab enters and leaves it as one stop while arrows move the selection and wrap. Tests: M1's exact reviewer sequence (hold Alt, open, pointer into card, scroll-close, hover another token, release, assert it closes after the grace) fails without the reset; the References-handoff mechanism (open card gone, pending dwell cancelled, in-flight request aborted) is behavioral in the hook suite, and codeNavHoverHandoff.test.ts pins the App wiring at source level the way iframeIsolation does, failing without the closeTokenHover call. Also: blur clears the held state, arrows move the radio selection, and the announcement latch no longer writes. The dialog suite additionally seeds its trigger cookie: configStore is a process-global singleton whose loadFromBackend keeps the in-memory value when the new backend is silent, so an unseeded map inherited another file's last write. The new roving-focus test is what surfaced that. * fix(review): dialog footer names the real Settings tab (Editor, not Review Display) * feat(review): jumbo hover card announcement, and rename the feature to Hover cards The announcement was too plain for a first-use dialog. It now uses the repo's big-format shell (max-w-5xl, badge/title/description header, a 1.1fr/1fr body that collapses to one column under 820px, footer with the Settings pointer and one action), matching EditModeAnnouncementDialog and LookAndFeelAnnouncementDialog. The left column, where the Edit Mode dialog plays its recording, is a worked example: a three-line strip of diff with `charge` shown under the pointer, and below it a hover card reproducing the real card's anatomy in the order TokenHoverCard renders it (name plus kind badge, approximate signature with its matched-line marker, doc line, Defined at, a reference sample ending in the overflow line). It is JSX over the existing theme tokens rather than a bitmap, so it is crisp at any DPI, follows the active palette in both light and dark, and cannot go stale against a card whose anatomy changes. It is decorative: the whole block is aria-hidden, carries nothing focusable, and every fact it shows is also stated in the prose beside it. The right column is unchanged in behavior: the same three-option radio group, the same immediate apply, the same roving-tabindex keyboard model, and the click-behavior note (now also carrying the ripgrep requirement, which the Settings row already stated). The footer keeps the Editor-tab destination d04cfce corrected. Rename: the user-facing name is "Hover cards" everywhere a reader meets it, in the dialog title and the Settings heading. "Token" described the implementation's unit rather than the thing the reviewer points at, and in an app full of annotations "token hover card" invites reading it as a kind of comment card. Every internal identifier is untouched: tokenHoverTrigger, tokenHoverDelay, the cookie keys, the hook and component names, and the data-token-hover-* test ids all keep their spelling, so there is no migration. Tests: the behavioral assertions are unchanged (once-ness, radio applies, dismiss and Escape keep the choice, arrows move selection), plus one that the example block is aria-hidden and holds nothing focusable, because read aloud it is a wall of invented identifiers that says nothing about the choice on offer. The guides.show CSS bundle hash moves with this: Tailwind scans source for class names, and the example introduces utilities the shared stylesheet did not carry. The JS bundle is unchanged, as expected for an app-side dialog. * feat(review): make the announcement example a live try-it on the real card The example was a hand-drawn impression of a hover card. A fork of that markup drifts from the shipped card the first time either changes, which is exactly what an example is supposed to prevent. It is now the real thing, and it is interactive: the `withRetry` token in the strip is genuinely hoverable, and resting on it opens the REAL TokenHoverCard through the REAL useTokenHover, so the reviewer feels the actual dwell, leave grace and Alt gate before committing to a setting. Nothing is reimplemented. Three thin seams carry it: - useTokenHover gains `resolve`, defaulting to the POST /api/code-nav/hover it always did. The try-it returns a hardcoded CodeNavHoverResponse instead. This is the ONLY fixture: a demo must not search the reviewer's repository for a symbol they never asked about, and a second copy of the dwell/supersession/ grace logic would drift. The resolver is read through a ref so an inline literal cannot re-arm the hook's callbacks every render. - TokenHoverCard gains `layerClassName` (default `fixed z-[60]`, unchanged). The card portals to <body> like every instance, so the one thing its host has to be able to say is which stacking context it belongs in; the try-it puts it at z-[110], above the z-[100] modal it is demonstrated inside. - TokenHoverCard gains `inert`, which drops the location buttons out of the tab order. The try-it has no References panel behind it, so they lead nowhere. The card body was extracted to TokenHoverCardContent so the positioned wrapper and any other host share one copy; every class, section and separator is the same markup the live card renders, not a duplicate. The hovered token wears the diff pane's own underline treatment, now defined once in tokenHoverStyles.ts: usePierreTheme serializes it into Pierre's shadow stylesheet as `.pn-token-hover`, the dialog applies the same declarations as a style object. Two consumers, one definition. The try-it reads the LIVE setting rather than a prop, so flipping the radio to "While holding Alt" makes the demo behave that way immediately and "Off" makes it do nothing and closes any open card. That is the honest preview of each choice, which is the point of putting the control next to the demo. Because it is interactive it is labeled rather than hidden: the region carries role="group" and an aria-label, the visible prompt line ("Try it: rest your pointer on withRetry") carries the meaning, and only the mock code lines stay aria-hidden, since read aloud they are a wall of invented identifiers. Tests: hovering the token opens the real card (asserted through the component's own data-token-hover-card marker) with the fixture's symbol and definition path after the dwell and not before; the try-it obeys the live trigger, so under "Hold Alt" a rest opens nothing until Alt goes down, and choosing Off takes an open card away; and the region is labeled while its mock code is hidden. React synthesizes onPointerEnter from pointerover, which is what the tests dispatch. The guides.show viewer hashes move on both halves: usePierreTheme is in the viewer's graph and now imports the shared style module (JS), and the try-it introduces Tailwind utilities the shared stylesheet did not carry (CSS). Budgets are unaffected (395.7 KB gz of 400). * fix(review): the try-it fixture names the token the prompt asks you to hover Two defects the browser capture caught, both invisible in the DOM tests. The prompt said "rest your pointer on withRetry" and the card that opened was titled `charge`: the fixture still described the symbol from the earlier static example. It now describes withRetry, and takes its name from EXAMPLE_SYMBOL so the prompt and the card cannot disagree again. The hovered token sat mid-line, and the card anchors at the token's left edge and is 400px wide, so it opened across the radio column the reviewer is meant to be reading. The token now leads the line, which keeps the card inside the example's own column. * feat(review): the hover card modifier gate is Cmd (Ctrl), not Alt Maintainer ruling. Two reasons, both recorded in DESIGN_hover-settings.md §4.1 in place of the old Alt argument: - Alt is very commonly bound to push-to-talk dictation, so an Alt-held gate would open cards every time the user starts speaking, with the pointer wherever they left it. That is the exact failure this setting exists to prevent. Alt-hold is also already taken inside Plannotator, by the plan editor's temporary input-method switch. - Cmd+hover is already VS Code's "tell me about this symbol" gesture: the navigable-target underline plus a definition peek under one held key. Gating on Cmd rides that muscle memory instead of competing with it. The old anti-Cmd argument is moot. The underline and the card appearing together under a held Cmd is not a collision, it IS that composite gesture; and handleCodeNavRequest already dismisses the hover surface on every References invocation, so Cmd+click cleanly supersedes an open card or a pending dwell rather than stacking with it. The key is platform-aware through two new helpers beside the existing ones in packages/ui/utils/platform.ts: `isModKeyHeld` (metaKey on macOS, ctrlKey elsewhere) and `modEventKey` ('Meta' / 'Control'). Labels come from the existing modKeyWord, so the key the code checks and the key the copy names cannot disagree. Labels updated in the dialog radio and its description, the Settings segmented control, the design doc and AGENTS.md. Only the modifier ALONE arms. Any other key going down while it is held (Cmd+C, Cmd+V, Cmd+S) disarms and takes an open card with it, so a copy performed with the pointer parked over the diff cannot pop a card mid-copy. This was cheap: the keydown handler already had to compare event.key. The typing-target guard stays as belt and braces. Blur handling stays and matters more now: on macOS the app switcher is the same key this gate arms on, so Cmd+Tab is the common way to leave with it held. Internal ids are untouched. The stored trigger value is still `modifier`, because the setting names the shape of the gate and not which key fills it, so nothing migrates. The #1461 Alt+click References alias is unrelated and is left alone. The try-it feels the new gate for free, since it drives the same hook: choosing the hold option and holding Cmd over the demo token opens the demo card. Tests: the gate suites now build their events from the platform helpers, so they exercise Cmd on macOS and Ctrl on the Linux runner rather than pinning one platform. All behavioral assertions are unchanged. Two new ones cover the chord rule, and both fail without it: a copy chord with the pointer parked on a token opens nothing, and a chord while a card is open closes it. * fix(review): pointer cursor wins over Pierre's I-beam on hovered and Cmd-held tokens * fix(review): the intro dialog's demo token gets the pointer cursor too * feat(review): hover delay default is 300ms matching VS Code; tiers respaced 150/300/700 * test(review): restore the 700-delay test's summed advances The 350 -> 300 sweep in b553835 rewrote both advances in this test, but they SUM to the configured dwell rather than each naming it: 300+300 is 600, so a 700ms delay never elapsed and the suite went red at the tip. Back to 350+350 with the pairing spelled out, so the guard stays meaningful in both directions: the first advance is past the 300 default (a hook that ignored delayMs would have fired already) and the pair reaches exactly 700. Verified fail-closed by pinning the timeout back to DEFAULT_TOKEN_HOVER_DELAY_MS, which fails this test and nothing else. The dialog test's two advances become DEFAULT_TOKEN_HOVER_DELAY_MS itself: its dwell IS the registry default (the seeded backend says nothing about tokenHoverDelay), so naming the constant is both correct and immune to the next respacing. * fix(review): the hover gate's typing guard pierces the shadow root Two defects in one branch of the modifier gate's keydown handler. The guard read event.target, which a window-level listener receives RETARGETED to the shadow host. Pierre's edit-session editor is a contenteditable inside that shadow root, so a bare Cmd pressed while editing code in place read as a plain <div>, armed the gate, and popped a card over the text being edited: verbatim the failure "typing owns the key" exists to prevent. The three other window-level typing guards in this package (AllFilesCodeView, FileTree, SectionsPanel) already read composedPath()[0] for exactly this reason and say so in comments; this one is no longer the exception. The guard also sat ahead of the chord branch, so it suppressed DISARMING as well as arming. Focus reaches a composer between the two routinely (a card is open, the reviewer clicks into the comment box and pastes), and that left the card standing over the diff with the gate still armed behind it. The chord branch now runs first and typing only gates the arm path. keyup deliberately keeps no typing guard: if the gate never armed the branch is already a no-op, and if it did arm the release has to be honored wherever focus has since travelled. Tests: the old "modifier chords in a comment box" case exercised the chord branch, not the guard, so it passed with isTypingTarget deleted. Replaced by a bare-modifier press from a textarea and one from a shadow-root editor, plus a chord-while-typing case for the asymmetry; the two pre-existing chord-disarm tests stay, so that coverage is unchanged. The shadow event is synthesized rather than dispatched from a real shadow root because happy-dom does not retarget, which would have made the test vacuous. Each of the three fails closed against its own defect: reverting composedPath fails only the shadow case, deleting the guard fails both typing cases, and moving the guard back above the chord branch fails only the asymmetry case. * feat(review): the modifier gate paints the navigable-target affordance The ruling that settled Cmd over Alt was that the underline and the card appearing together under one held key IS the composite gesture rather than a collision. The code did not deliver it: the diff views paint pn-token-nav from the pointer ENTER event, and neither half of the gesture this mode exists for fires one. Parking on a symbol and THEN holding the key opened a card on a token wearing no affordance, and releasing closed the card while leaving the affordance painted until the pointer eventually left. useTokenHover now reports the arm and disarm transitions through an optional onModifierGate(armed, tokenElement), always carrying the token the pointer is parked on RIGHT NOW so a disarm also unpaints a neighbour the pointer drifted onto while the key was down. Every disarm route is covered: chord, release, blur, and the effect cleanup when the reviewer switches trigger with the key still held. The release notifies ahead of the reading-the-card early return, because the key being up ends the affordance whether or not the card is being read. App owns the painting, not the hook: the diff views are compiled into the portable guides.show viewer and their prop signatures must not move, and the class is a review-app concern. Enter-time painting is untouched, so plain Cmd+hover in the default hover mode behaves exactly as before. Also gates pn-token-hover on a hover handler actually being wired, and paints it in AllFilesCodeView. That class carries a cursor: pointer that became !important in eebab51, so painting it unconditionally started promising clickability on every token in a session with hover cards switched off; and leaving it out of the all-files view meant the affordance the announcement's try-it demonstrates was missing from the default review surface. A read-only or portable host passes no hover handler and still gets neither class. Fail-closed: dropping notifyGate(true) fails both new hook tests, dropping the App wiring fails the App pin, and making the DiffViewer paint unconditional fails the gating pin. * fix(review): hover card cleanups, and the design's delay tiers Comments first, because several of them had become false and this repo treats them as contract. The dialog cited a StaticTokenHoverCard that no longer exists and a card that "lives inside an aria-hidden decorative block" (it does not; the try-it region is a labelled role=group), the try-it still named "the Alt gate", and two hook comments plus four test comments still described the trigger key as Alt or claimed the Alt+click alias and the trigger share a key, which stopped being true when the gate became Cmd. The focus trap's rationale claimed the example card's buttons were in its query; the card portals to <body>, so they never were. tokenHoverStyles now single-sources what it says it does. Only thickness and offset were actually shared; the decoration line and the cursor were written twice and could drift. Both forms read one object, and the two genuinely per-form bits (the resolved color, and the !important the shadow form needs to beat Pierre's I-beam) are named as such. The display-only card stops advertising clicks it swallows: its own footer says "Click a location to jump", so under `inert` the locations lose the hand cursor and hover underline as well as the tab order, and the card gets aria-hidden — it portals outside the aria-modal dialog it belongs to, where a screen reader would meet it as a stray tooltip. The announcement's availability latch moves from the render body into an effect: a ref written during render is a side effect React may discard. The gate independently requires !isLoading, so the ordering is unchanged. Both announcement test files now re-hydrate configStore in afterEach. They re-resolve a process-global singleton from a two-entry fake map, and the pure lane runs in one process, so they were handing their own defaults to whatever file ran next. The dialog test gains the two assertions that were missing: the demo card sits at z-[110] (the layerClassName seam's entire purpose is not being buried under the modal it is demonstrated inside) and its locations are non-interactive. Both fail closed against dropping the respective prop. Design doc: §4.2 still listed 200/350/700 with a 350 default, which the maintainer ruling in §6 had already replaced; §3's 350 now reads as the history it is; §6 records how the composite gesture is delivered and §9 how the hovered-token treatment is gated. * chore(core): re-pin the guide viewer build The manifest was synced before the last three commits of this branch and went stale: usePierreTheme (which serializes the token-hover CSS into Pierre's stylesheet), the token-hover default constant, and now AllFilesCodeView are all inside the viewer bundle, so the JS hash moved while the CSS hash did not — the .pn-token-hover rule is a JS template string, not a stylesheet entry, which is exactly why the drift was easy to miss by eye. check:manifest was failing at the tip. Rebuilt and synced last, so this pins the branch's final source. Budgets after: js 395.8 KB gz against a 400 KB budget, css 50.9 against 64, worker still import-free.
1 task
backnotprop
added a commit
that referenced
this pull request
Sep 11, 2026
…0.38.1) and make the package smoke catch an unpublished core (#1510) @plannotator/ui 0.38.0 does not install into a consumer: config/settings.ts imports @plannotator/core/token-hover (#1462), but that PR added the export to core without bumping it, so 0.38.0 pins core 0.25.1, which never published the subpath. Every host reaches it through configure -> config -> settings, so typecheck and build fail on the first import. core 0.25.2 publishes ./token-hover plus the other changes core accumulated since 0.25.1 (the rotated guide-viewer-manifest pin, the config-types hover fields). ui 0.38.1 is 0.38.0 pinning core 0.25.2, and aligns @codemirror/state to ^6.7.2 beside @codemirror/view ^6.43.10 so a consumer cannot resolve two state copies. The package smoke passed on 0.38.0 because its external consumer installs core from the LOCAL tarball (pnpm override), so an export that exists in source but not on the registry is invisible to it, and its consumer entry never imported the configure/config path. It now (1) collects every @plannotator/core/<subpath> the UI source imports and asserts each is an export of the core source manifest, (2) when the pinned core version is already on the registry, asserts each is exported by THAT published core and that the core source exports match it exactly (otherwise: bump core and publish it first), printing a publish-core-first reminder when the pinned version is not published yet, and (3) compiles @plannotator/ui/configure and @plannotator/ui/config in the external consumer's vite build and tsc. With the pins set back to 0.38.0's, the smoke now fails naming token-hover. HANDOFF and README: current pair 0.38.1 on 0.25.2; do not consume 0.38.0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Trigger customizability and a first-run introduction for the token hover cards (#1461), designed doc-first in DESIGN_hover-settings.md (committed with the change).
The rational settings model, after pressure-testing: exactly two cookie-only settings. "Show cards" is one select with three values: On hover (default), While holding Alt (Option), or Off; folding Off into the select kills the unreachable enabled-false-plus-mode state a separate toggle would create. "Card delay" offers Fast 200, Default 350, Relaxed 700, because "cards annoy me" is three different complaints: never (Off), on request (Alt), or just less eager (delay). Alt was chosen over Cmd because Cmd already means "take me there" (Cmd+click opens the References panel, and Cmd hover paints the clickable affordance), while Alt spells the same on every platform.
Rejected with written arguments in the design doc: a click-to-open mode and any click-override of the References panel. Every click gesture on a token already has an owner (plain click opens the annotation toolbar, Cmd/Ctrl/Alt+click opens References, Shift extends selection), so a click mode could only evict a sitting tenant while the Alt mode already delivers cards-on-request with zero conflicts.
The one-time dialog joins the existing announcement chain as its fifth member (same gating conventions, provably never stacks with the other four) and carries the trigger choice inline as a radio that applies immediately; dismissing keeps the choice, and Settings under Review Display can change it anytime. Users who had already turned the earlier boolean setting off are migrated to Off, never see the dialog, and keep working even on a downgrade (the legacy cookie is read, never rewritten).
In modifier mode the Alt gate sits ahead of the dwell timer, the cache, and all state, so with the key up nothing is searched and no listeners exist outside that mode. Typing surfaces (inputs, textareas, CodeMirror) never arm the gate. Adversarial review drove two interaction fixes with revert-proven regression tests: a stale pointer-in-card flag that could deafen Alt releases after a scroll-close, and the Alt+click double surface: every References-panel invocation now dismisses or cancels the hover surface first, which also repairs the pre-existing hover-plus-modifier-click overlap from #1461.
Lanes: six touched suites 63 pass, full DOM lane 827 pass across 101 files, typecheck clean across all projects, full vendored suite at the pre-existing 12-failure environmental floor with name-identical failures, viewer manifest in sync and budgets green.