mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
Michael Ramos
91d05511b8
* 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
d04cfce5 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 b5538350 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 eebab519, 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.