diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a2616c76cc..debef591f1 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -130,6 +130,7 @@ jobs: packages/review-editor/hooks/useReviewSearch.test.tsx packages/review-editor/hooks/useTokenHover.test.tsx packages/review-editor/components/TokenHoverCard.test.tsx + packages/review-editor/components/TokenHoverAnnouncementDialog.test.tsx packages/review-editor/utils/stitchTokenIdentifier.test.ts packages/review-editor/hooks/useAutoViewed.test.tsx packages/review-editor/hooks/useCallFlowAnalysis.test.tsx diff --git a/AGENTS.md b/AGENTS.md index 1ea91e2019..db37464a75 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -305,7 +305,7 @@ The default code-review diff is **`since-base`** — a composite of `merge-base( **Staging display invariant:** `useGitAdd`'s `stagedFiles` is the EFFECTIVE staged set (sections-sidecar snapshot + session stage/unstage overrides) and is the only source any surface may render staging state from. The sidecar entry's `staged` flag is a snapshot — ORing it back in makes files unstaged mid-session render as staged (and inverts the next toggle). -`since-base` is only offered when the base ref actually resolves — on a repo whose trunk isn't discoverable (`trunk`, no `origin/HEAD`) `getGitContext` omits it and the default falls through to `uncommitted`, so committed branch work is never silently hidden. The since-base patch/sections/fingerprint/file-content paths all degrade to `HEAD` together when merge-base fails for a resolvable-but-unrelated base. First-run shows `ReviewSetupDialog` (replaces the removed `DiffTypeSetupDialog`), which initializes an unseen reviewer's panel to Tree once while preserving the resolved diff default, and is reopenable from the review header menu. The one-time dialog chain is guide intro → look-and-feel → review setup → Edit Mode; none of the dialogs stack. Analysis layers no longer add a startup dialog: Semantic Changes retains its enabled default, while Call Flow remains disabled until the user explicitly enables it in Settings, which is also consent for its managed runtime installation. +`since-base` is only offered when the base ref actually resolves — on a repo whose trunk isn't discoverable (`trunk`, no `origin/HEAD`) `getGitContext` omits it and the default falls through to `uncommitted`, so committed branch work is never silently hidden. The since-base patch/sections/fingerprint/file-content paths all degrade to `HEAD` together when merge-base fails for a resolvable-but-unrelated base. First-run shows `ReviewSetupDialog` (replaces the removed `DiffTypeSetupDialog`), which initializes an unseen reviewer's panel to Tree once while preserving the resolved diff default, and is reopenable from the review header menu. The one-time dialog chain is guide intro → look-and-feel → review setup → Edit Mode → token hover cards; none of the dialogs stack. The token hover announcement is last and additionally skips a session where hover cards cannot run at all (no live workspace), WITHOUT consuming its cookie, and never shows to a reviewer whose trigger is already non-default (which after the boolean-to-trigger migration is exactly the early adopter who turned cards off). Analysis layers no longer add a startup dialog: Semantic Changes retains its enabled default, while Call Flow remains disabled until the user explicitly enables it in Settings, which is also consent for its managed runtime installation. ### GitButler review invariants @@ -553,7 +553,7 @@ During normal plan review, an Archive sidebar tab provides the same browsing via | `/api/guide/:jobId/output` | GET | Fetch a failed guide job's captured raw output for manual repair (404 if none captured) | | `/api/guide/:jobId/submit` | POST | Manually submit corrected guide JSON for a failed job (body: `{ payload }`) | | `/api/code-nav/resolve` | POST | Search for symbol definitions and references via ripgrep (body: `{ symbol, filePath, line, charStart, side, language? }`) | -| `/api/code-nav/hover` | POST | Token hover card resolution: the same body and the same guards as `/resolve`, over the same ripgrep search, returning one enriched definition (kind, approximate signature, heuristic doc comment, short preview), an optional runner-up candidate, and a five-reference sample. `backend: 'unavailable'` (rg missing) is a normal 200 and the client renders nothing. `source` is always `'search'` in Tier 0; the nullable `symbolKind` / `signature` / `doc` fields are what a later syntax- or index-backed tier would fill. | +| `/api/code-nav/hover` | POST | Token hover card resolution: the same body and the same guards as `/resolve`, over the same ripgrep search, returning one enriched definition (kind, approximate signature, heuristic doc comment, short preview), an optional runner-up candidate, and a five-reference sample. `backend: 'unavailable'` (rg missing) is a normal 200 and the client renders nothing. `source` is always `'search'` in Tier 0; the nullable `symbolKind` / `signature` / `doc` fields are what a later syntax- or index-backed tier would fill. Client-side the card is governed by two cookie-only settings (`packages/ui/config/settings.ts`, types in `@plannotator/core/token-hover`): `tokenHoverTrigger` (`hover` default / `modifier`, meaning the platform's primary modifier held, Cmd on macOS and Ctrl elsewhere via `isModKeyHeld` / `modEventKey` in `packages/ui/utils/platform.ts` / `off`, which withholds the handler props so the diff views wire no listeners at all) and `tokenHoverDelay` (150 / 300 default / 700 ms, the dwell before any request exists; 300 matches the VS Code hover delay). `tokenHoverTrigger` REPLACED the original `tokenHoverCards` boolean and re-reads its cookie as a migration on every load until the user touches the setting (`false` becomes `off`), never writing it back: a migrating read returns a value, so the registry's default-seeding write never fires, and resolution is pure and identical every time. Cmd rather than Alt because Alt is widely bound to push-to-talk dictation (an Alt gate would open cards while the user speaks) and because Cmd+hover is already VS Code's "tell me about this symbol" gesture; the navigable-target underline and the card under one held key is that composite gesture, not a collision, since `handleCodeNavRequest` dismisses the hover surface on every References invocation. Only the modifier ALONE arms: any other key while it is held (Cmd+C) disarms and closes, so a copy with the pointer parked on a token cannot pop a card. Both key branches read `composedPath()[0]`, not `event.target`, because a window-level listener sees the shadow HOST and Pierre's edit-session editor is a contenteditable inside that root; typing suppresses ARMING only, never disarming, and keyup carries no typing guard at all. The `pn-token-nav` affordance is the other half of the held-key gesture: the diff views paint it from the pointer enter event, and the hook additionally reports arm/disarm through `onModifierGate(armed, tokenElement)` so a key pressed over a parked pointer (and the release after it) paints and unpaints it too. `pn-token-hover` (underline plus a `cursor: pointer !important` that beats Pierre's I-beam) is painted by BOTH diff views, gated on a hover handler actually being wired, so `off` and the portable viewer get no affordance. Click behavior is untouched by the trigger setting, and the #1461 Alt+click References alias is unrelated. The stored value stays `modifier`: the setting names the shape of the gate, not which key fills it. | | `/api/code-nav/file` | GET | Read file from working tree for code-nav preview (`?path=`) | ### Annotate Server (`packages/server/annotate.ts`) diff --git a/DESIGN_hover-settings.md b/DESIGN_hover-settings.md new file mode 100644 index 0000000000..d45d72344f --- /dev/null +++ b/DESIGN_hover-settings.md @@ -0,0 +1,494 @@ +# Hover cards: trigger customizability and first-run announcement + +Design for the follow-up to #1461 (token hover cards, Tier 0). Written before +implementation; the implementation follows this document. + +## 1. The problem + +#1461 shipped hover cards on for everyone, behind one boolean +(`tokenHoverCards`, cookie-only, default `true`). Resting the pointer on a +symbol for 350ms opened a card. That was a good default and a bad only-option: +a reviewer who reads with the pointer parked in the text gets cards they did +not ask for, and their only recourse today is a binary off switch that also +takes away a feature they might want on demand. + +The ask is trigger customizability plus a one-time announcement, and above all +a **rational** settings list: the fewest controls that cover real needs. + +## 2. Prior art: how VS Code models this + +VS Code's hover family, and what each one is actually for: + +| Setting | Type | Default | What it buys | +|---|---|---|---| +| `editor.hover.enabled` | boolean | `true` | Kill switch. | +| `editor.hover.delay` | number (ms) | `300` | Dwell before the hover appears. | +| `editor.hover.sticky` | boolean | `true` | Whether the pointer can travel onto the hover without it closing. | +| `editor.hover.above` | boolean | `true` | Placement preference. | +| `editor.gotoLocation.multiple*` | enum x5 | `peek` | What Go-to-Definition does when there are several results. | +| `editor.multiCursorModifier` | enum (`alt` \| `ctrlCmd`) | `alt` | Which modifier means "extra cursor", which in turn decides which one means "go to definition on click". | + +Three lessons worth taking: + +1. **The modifier is a single global choice, not a per-gesture one.** VS Code + does not ship a "modifier for definition preview" and a separate "modifier + for go to definition". `multiCursorModifier` picks one, and the other + gesture takes what is left. The user never gets to build a conflicting pair. +2. **The trigger and the delay are separate axes and both are real.** Delay is + the single most-tuned hover setting in VS Code, because "I want hovers but + they are too eager" is a distinct complaint from "I do not want hovers". +3. **`gotoLocation.*` is the cautionary tale.** Five enums describing what a + click does when the answer is ambiguous. Almost nobody touches them, they + are hard to reason about together, and they exist because click behavior was + made configurable. We should not import that. + +VS Code's Cmd/Ctrl+hover definition peek is a fourth relevant behavior, but it +is not a setting there: it is a fixed consequence of `multiCursorModifier`. It +is also why our modifier choice below is not free. + +## 3. Our constraints (what is already true, and must stay true) + +- **The dwell IS the gate.** At the time of writing that was a 350ms + `DWELL_MS` constant in + `packages/review-editor/hooks/useTokenHover.ts:10` (§6 replaces it with the + configurable `delayMs`, defaulting to 300). Nothing is requested + before it elapses; sweeping a diff costs zero ripgrep processes. Any new + trigger must gate at least as early as this, never later. +- **Cmd/Ctrl+click opens the References panel.** Shipped contract + (`AllFilesCodeView.handleTokenClick`, `DiffViewer.handleTokenClick`). Not + negotiable, not configurable. +- **Cmd/Ctrl+hover already has a meaning:** it paints `pn-token-nav` on the + token, the "this is a navigable target" affordance. The modifier is taken. +- **Alt+click is a quiet alias** into the same References panel, added by + #1461 and deliberately unadvertised. +- **A plain click is not free.** In `DiffViewer` an unmodified token click + falls through to `toolbarHostRef.current?.handleTokenClick`, the + selection/annotation toolbar host. Plain click already means something. +- **Comment-only surfaces and read-only views never get the card.** The + portable guides.show viewer passes neither hover handler; `AllFilesCodeView` + and `DiffViewer` treat the props as absent by default. Untouched by this work. +- **The whole feature also rides `canUseLiveWorkspaceActions`.** No live + workspace (GitButler stack views, and friends) means no cards regardless of + any setting. +- **Off must mean off at the source.** Today `tokenHoverEnabled === false` + means the handler props are never passed, so there are no listeners, no + requests and no card in the React tree. That property is the reason the + feature is cheap when unwanted, and it survives this change verbatim. + +## 4. The model + +**Naming.** The user-facing name is **"Hover cards"** everywhere a reader meets +it: the announcement title, the Settings heading, and the descriptions. Every +internal identifier keeps the `tokenHover*` spelling it shipped with — +`tokenHoverTrigger`, `tokenHoverDelay`, the cookie keys, the hook, the +component and file names, and the `data-token-hover-*` test ids — so the rename +carries no migration churn. "Token" was accurate and unhelpful: it describes +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. + +**Two settings. Both cookie-only. Nothing else.** + +### 4.1 `tokenHoverTrigger` — how cards open + +| Value | Label | Meaning | +|---|---|---| +| `hover` (default) | On hover | Rest the pointer on a symbol; the card opens after the dwell. Today's behavior. | +| `modifier` | While holding Cmd (Ctrl on Windows and Linux) | Cards only while the primary modifier is held. With the key up, nothing is armed, nothing is requested, nothing opens. | +| `off` | Off | No handlers, no listeners, no requests, no card. | + +This is one control, not a boolean plus an enum. A separate on/off toggle +alongside a mode select creates an unreachable state (`enabled: false, +mode: 'modifier'`) that has to be reasoned about at every read site, and it +puts two controls where the user is answering one question. `off` is a value +of the question, not a switch above it. + +**Why Cmd (Ctrl) and not Alt.** Ratified by the maintainer; this replaces an +earlier Alt proposal. + +1. **Alt is spoken for, in a way that fires constantly.** Push-to-talk + dictation is very commonly bound to a held Alt. An Alt-gated card would open + every time the user starts speaking, with the pointer wherever they left it, + which is the exact failure this setting exists to prevent. +2. **Cmd+hover is already the gesture.** It is what VS Code does for "tell me + about this symbol": the navigable-target underline plus a definition peek, + under one held key. Gating on Cmd rides that muscle memory instead of + inventing a competing one. +3. **Alt-hold is taken inside Plannotator too.** The plan editor already binds + a held Alt to the temporary input-method switch + (`useInputMethodSwitch`, `inputMethod.shortcuts.ts`). One held key meaning + two different things across two surfaces is a worse story than reusing the + modifier that already means "tell me about this". + +**The old anti-Cmd argument is moot.** It was that Cmd+hover already paints +`pn-token-nav`, so a card on the same gesture would collide with the "this is +clickable" affordance and precede every deliberate Cmd+click. Two things +settled it: + +- The underline and the card appearing together under a held Cmd is not a + collision, it **is** the composite gesture, the same one VS Code ships. The + views paint `pn-token-nav` from the pointer ENTER event, which fires for + neither transition this mode turns on (a key going down over a parked + pointer, and the release after it), so the hook reports both through an + `onModifierGate(armed, tokenElement)` option and the App paints them. Both + halves of the gesture therefore arrive and leave together, on every disarm + route: chord, release, blur, and a trigger change with the key still held. + The callback rather than the hook touching the class directly, because the + diff views are compiled into the portable viewer and their prop signatures + must not move. +- `handleCodeNavRequest` now dismisses the hover surface on **every** + References invocation (§6), so a Cmd+click cleanly supersedes an open card + or a pending dwell instead of stacking with it. + +**Why the modifier is not itself a setting.** Offering "Cmd or Alt" is a third +control that buys one keystroke of taste and doubles what every doc sentence +has to describe. VS Code makes exactly one modifier choice for the same reason. + +**The stored value stays `modifier`.** The setting names the shape of the gate, +not which key fills it, so the ruling changed labels and one key check and +migrated nothing. + +### 4.2 `tokenHoverDelay` — how long the dwell is + +| Value | Label | +|---|---| +| `150` | Fast | +| `300` (default) | Default | +| `700` | Relaxed | + +(Respaced from an earlier 200/350/700 by the maintainer ruling recorded in +§6: 300 is VS Code's `editor.hover.delay`, and dropping Fast to 150 keeps it +perceptibly distinct from a 300 default.) + +**Why this earns a control.** "Hover cards annoy me" is not one complaint, it +is three, with three different remedies: + +- *"I never want these"* → `off`. +- *"I want them when I ask, not when I read"* → `modifier`. +- *"I want them, they are just too eager"* → a longer dwell. + +The third is not served by either of the first two, and it is the complaint +that a fixed constant produces most often, because dwell tolerance is a +personal reading-speed property. It is also the one VS Code found worth +shipping (`editor.hover.delay`) after having both an enable flag and a +modifier-gated peek. + +**Why three fixed steps and not a slider or a number field.** A slider invites +tuning a value whose perceptible granularity is roughly 150ms; nobody can tell +340 from 360, and offering the precision implies it matters. Three named steps +say what the axis is for and stop. + +**Why the delay applies in both trigger modes.** It would be defensible to +argue that holding the modifier is already an expression of intent, so +`modifier` mode +should dwell less (or not at all). Rejected: a zero dwell in modifier mode +fires one ripgrep per token swept while the key is down, and a *separate* +implicit dwell for modifier mode is a hidden coupling that cannot be explained +in the Settings row. One constant, one control, one sentence: this is how long +the pointer rests before a card is requested. + +The delay control is disabled (visible, inert) when the trigger is `off`, so +the pair reads as one idea with an inapplicable half rather than as two +unrelated rows. + +### 4.3 Rejected controls, and why + +| Rejected | Argument | +|---|---| +| **Click to open the card** | See §5. It has no free gesture. | +| **A click-mode that overrides the References panel default** | Directly asked about in the brief. It replaces a shipped, documented contract (Cmd+click = References) with a per-browser preference, which means the answer to "what does Cmd+click do in Plannotator" stops being knowable. It also doubles the trigger matrix (3 hover modes x 2 click modes) for a destination the card already reaches: every location line on the card routes into the References panel. There is no user need it serves that the card does not already serve one click later. This is our `gotoLocation.*`. | +| **A separate on/off boolean beside the mode** | Creates an unreachable state, adds a control, answers no new question. `off` is a mode. | +| **A modifier chooser (Cmd vs Alt)** | One keystroke of taste, at the price of doubling every sentence that describes the gate. Alt is also the dictation key (§4.1). | +| **`sticky` / leave-grace toggle** | The 250ms leave grace exists so the card's own reference links are reachable. A user who turns it off cannot click the card. It is a correctness constant, not a taste. | +| **Placement (above/below)** | The card already flips when the viewport would clip it. Nothing left to choose. | +| **Per-language or per-file-size gating** | Speculative. The render threshold (definition, or two or more references) already suppresses the thin answers this would target. | +| **A dwell for *closing*** | Same category as `sticky`. | + +## 5. The click-to-open interaction matrix (why it does not earn its place) + +The brief asks for proof, so here is the gesture budget on a token in a diff +pane. Each row is a gesture; the column is what already owns it. + +| Gesture | Owner today | Free for a card? | +|---|---|---| +| Pointer down + drag | Text selection, and line-range selection in the diff gutter chain | No. A card-opening click has to be distinguished from the start of a drag, which means deferring to `mouseup` and comparing coordinates, and any threshold you pick is wrong for someone. | +| Plain click (no drag) | `toolbarHostRef.handleTokenClick`: the annotation/selection toolbar host | No. Taking it means either annotating stops working from a token click, or the card and the toolbar both appear and fight for the same anchor rect. | +| Cmd/Ctrl+click | References panel (shipped contract) | No, by rule. | +| Alt+click | References panel (quiet alias, #1461) | Only by breaking the alias. Unrelated to the gate and deliberately left alone. | +| Shift+click | Range selection extension in the surrounding text | No. | +| Double click | Word selection (native) | No. | +| Middle click | Paste on X11, new tab semantics elsewhere | Not usable. | + +Every gesture on a token is spoken for. A click-to-open mode can only be built +by evicting a sitting tenant, and the two tenants worth evicting (plain click, +Alt+click) are the annotation toolbar and the References alias. Meanwhile +`modifier` mode already delivers the thing a click mode is wanted for, which is +"cards only when I ask", using a gesture that conflicts with nothing because a +held key is not a click. + +**Conclusion: no click mode.** Click behavior is untouched by this work. + +## 6. Trigger mechanics + +The gate is in the hook, not in the diff views. `useTokenHover` gains an +options argument, `{ mode, delayMs }`; `AllFilesCodeView` and `DiffViewer` +keep their prop signatures byte-identical, which matters because both are +compiled into the portable guides.show viewer. + +`off` is still enforced one level up, in `App.tsx`, by not passing the handlers +at all. The hook never sees `off` in practice; the mode it sees is `hover` or +`modifier`. That keeps the zero-footprint property of #1461 exactly as shipped. + +**Which key.** `isModKeyHeld` / `modEventKey` in `packages/ui/utils/platform.ts`: +`metaKey` and `Meta` on macOS, `ctrlKey` and `Control` elsewhere. One helper +pair, shared with the labels (`modKeyWord`), so the key the code checks and the +key the copy names can never disagree. + +**Gate before the dwell.** In `modifier` mode `onTokenHoverEnter` checks the +tracked held state and returns before arming any timer, setting any state, or +touching the cache. Holding nothing costs one boolean read. + +**Modifier pressed while already hovering.** The pointer does not re-enter a +token when a key goes down, so a gate that only reads the enter event would +make the common gesture ("what is this? *holds Cmd*") do nothing. The hook +remembers the last token entered (request + element, one ref, written on every +enter including gated ones) and runs the normal enter path when the key goes +down. Leave clears it. + +**Only the key ALONE arms.** The gate's modifier is also the editing modifier, +so any other key going down while it is held (Cmd+C, Cmd+V, Cmd+S) disarms and +takes an open card with it. Without this, a copy performed with the pointer +parked over the diff pops a card mid-copy. This is cheap because the branch +already exists: the keydown handler compares `event.key` to `modEventKey`. + +**Modifier released.** Release behaves the way pointer-leave behaves: it starts +the same 250ms grace, so the card can still be reached. One exception: if the +pointer is already inside the card when the key is released, the release is +ignored, because otherwise the card vanishes out from under someone reading it. +`onCardEnter`/`onCardLeave` already bracket that state; one ref records it. + +**Typing owns the key.** A keydown whose target is an input, textarea, select +or contenteditable never arms, because the pointer is often parked over the +diff while a comment is being written and cards would pop mid-sentence. This is +belt and braces with the chord rule above. + +**Window blur.** On macOS the app switcher is the same key this gate arms on, +so Cmd+Tab is now the COMMON way to leave with the key held. On `blur` the held +flag clears and the card closes. + +**Listener cost.** The two key listeners and the blur listener exist only while +the mode is `modifier`. In `hover` mode the pipeline is byte-for-byte what +#1461 shipped, plus a delay constant read from settings. + +**Delay.** `DWELL_MS` becomes the hook's `delayMs` option, defaulting to 300 (matching VS Code, maintainer ruling; tiers respaced 150/300/700 to keep Fast perceptibly distinct) so +every existing call site and test is unchanged. The leave grace, the cache +size, the scroll cancel and the render threshold are untouched. + +## 7. Migration + +`tokenHoverCards` (boolean, `plannotator-token-hover-cards`) is **replaced**, +not kept alongside. Keeping both would mean two sources for one question. + +`tokenHoverTrigger.fromCookie()` reads `plannotator-token-hover-trigger` first. +When that key is absent it reads the legacy boolean: + +- `'false'` (an early adopter who turned cards off) → `'off'`. +- `'true'` → `'hover'`. +- absent or unrecognized → `undefined`, so the registry default (`'hover'`) + applies. + +**The mechanism is re-resolution, not one-time seeding.** `configStore`'s +default-seeding write only fires when `fromCookie()` returns `undefined`, and a +migrating read returns `'off'`, so nothing is written on load: the legacy key is +re-read and re-resolved on every page load until the user actually touches the +setting, at which point `toCookie` writes `plannotator-token-hover-trigger` and +that branch wins from then on. Resolution is pure and identical every time, so +the repeated read costs one cookie lookup and can never drift. + +Seeding it eagerly (a write inside `fromCookie`) was considered and rejected: +it would put a storage write inside a getter that the registry calls during +`ensureLoaded`, for no behavioral gain over re-resolving. + +The legacy value is read, never written, and never deleted: a stale cookie is +inert, and deleting it would make a downgrade to the #1461 build silently +re-enable cards for someone who had turned them off. + +Consequence worth stating plainly: **an early adopter who toggled cards off +stays off, and is never shown the announcement** (§8 gates on the resolved +trigger still being the default). + +## 8. The announcement dialog + +### 8.1 Where it sits in the chain + +The review app's one-time chain is: guide intro → look-and-feel → review setup +→ Edit Mode. The new dialog goes **last**, after Edit Mode, so no existing +gate function changes. + +Following `editModeAnnouncement.ts` exactly: + +- A cookie gate, `plannotator-token-hover-announcement-seen`, holding a version + string so a future revision can re-show it. Cookie-backed because Plannotator + runs on random localhost ports. +- A pure `tokenHoverAnnouncementCanShow(state)` predicate, unit-testable, that + returns false while `isLoading` or while any earlier chain dialog is visible. + None of the chain dialogs ever stack. +- The pending flag is **latched at mount**, so choosing a trigger inside the + dialog does not unmount the dialog mid-click. + +Two extra conditions on top of the Edit Mode shape: + +- **Skipped when the feature cannot run**: `canUseLiveWorkspaceActions` false + means no cards are possible in this session, so announcing them is noise. It + is skipped *without consuming the cookie*, mirroring how the guide intro + skips on an empty diff, so the next real session still shows it. +- **Skipped when the user already has a non-default trigger**, which after §7 + is exactly the early adopter who turned cards off. Telling someone about a + feature they already declined is the worst version of this dialog. That skip + *does* consume the cookie: they have expressed the preference the dialog + exists to collect. + +### 8.2 Shape + +The repo's big-format announcement shell, matching `EditModeAnnouncementDialog` +and `LookAndFeelAnnouncementDialog`: `max-w-5xl`, a header carrying the badge, +title and description, a `grid-cols-[1.1fr_1fr]` body that collapses to one +column under 820px, and a footer with the Settings pointer and one action. +Portal to `document.body`, `role="dialog"`, `aria-modal`, focus trap and Escape +wired the same way, Escape and the Done button both meaning "accept what the +radio currently says". + +**The left column is a TRY-IT**, in the slot where the Edit Mode dialog plays +its recording: a three-line strip of diff whose `withRetry` token is genuinely +hoverable, with a prompt line inviting it. Resting the pointer there opens the +**real** `TokenHoverCard` through the **real** `useTokenHover`, so the reviewer +feels the actual dwell, the leave grace and the modifier gate before +committing to a setting. + +Nothing is redrawn. A forked copy of the card's markup, or a second copy of the +dwell logic, would drift from the shipped surface the first time either changed +— which is precisely what an example is supposed to prevent. Three thin seams +carry it instead: + +| Seam | Why | +|---|---| +| `useTokenHover({ resolve })` | The only fixture. The default posts to `/api/code-nav/hover`; the try-it returns a hardcoded `CodeNavHoverResponse`, because a demo must not search the reviewer's repository for a symbol they never asked about. | +| `TokenHoverCard({ layerClassName })` | The card portals to `` like every instance, so its host has to be able to put it above the modal it is demonstrated inside (`z-[110]` over the dialog's `z-[100]`). | +| `TokenHoverCard({ inert })` | The try-it has no References panel behind it, so its location buttons lead nowhere and leave the tab order. | + +The hovered token wears the same underline treatment the diff pane paints, from +one definition (`tokenHoverStyles.ts`): the shadow-DOM form is serialized into +Pierre's stylesheet as `.pn-token-hover`, the dialog applies the same +declarations as a style object. + +**The try-it reads the LIVE setting**, not a prop: flipping the radio to the +hold-modifier option makes the demo behave that way immediately (hold Cmd over +the demo token and the demo card opens), and "Off" makes it do nothing and +closes any open card. That is the honest preview of each choice. + +Because it is interactive it is **labeled rather than hidden**: the region +carries `role="group"` and an `aria-label`, the visible prompt line carries the +meaning, and only the mock code lines stay `aria-hidden` (read aloud they are a +wall of invented identifiers). + +The right column carries the decision: the three options as a **radio group +that applies immediately** on selection (`configStore.set('tokenHoverTrigger', +value)`), following the WAI-ARIA radiogroup pattern (roving tabindex, arrows +move and wrap), plus the note about click behavior. Dismiss is therefore always +"accept current choice", with no separate confirm, and Escape can never lose a +selection the user made. + +### 8.3 Copy (verbatim, no em dashes) + +Badge: `New` + +Title: `Hover cards` + +Description: +> Rest the pointer on a symbol in a diff and Plannotator shows you where it is +> defined, an approximate signature, its doc comment, and a sample of its +> references. Every location on the card opens in the References panel. + +Choice group label: `Show cards` + +Options: + +- `On hover` / `Rest the pointer on a symbol and the card appears. This is the default.` +- `While holding {Cmd|Ctrl}` / `Cards stay out of the way until you hold {Cmd|Ctrl}. Nothing is searched while the key is up.` +- `Off` / `No cards, no listeners, no searches.` + +Note under the group: +> {Cmd|Ctrl}+click on a symbol still opens the References panel, whichever option +> you pick. Hover cards need ripgrep and a local checkout, and nothing appears when +> the search comes back empty. + +(The modifier word is platform-resolved through `modKeyWord`: `Cmd` on macOS, +`Ctrl` elsewhere.) + +Footer left: +> Change this anytime in Settings, in the Editor tab. + +Footer button: `Done` + +### 8.4 Settings UI + +`ReviewDisplayTab` in `packages/ui/components/Settings.tsx` (the tab is labeled +**Editor** in review mode, which is what the dialog footer points at). The existing "Token hover cards" `ToggleSwitch` is +replaced in place by: + +- Heading `Hover cards` with the same one-line explanation the toggle carried. +- A `SegmentedControl` for the trigger: `On hover` / `Hold {Cmd|Ctrl}` / `Off`. +- A `SegmentedControl` for the delay: `Fast` / `Default` / `Relaxed`, disabled + when the trigger is `Off`. + +Two rows, one section, same place in the tab. + +## 9. What is deliberately not changing + +- Cmd/Ctrl+click and Alt+click both still open the References panel. The + Alt+click alias from #1461 is unrelated to the gate and is left alone. +- The 250ms leave grace, the 30-entry LRU, the snapshot flush, the scroll and + wheel cancel, the render threshold, and the silent-failure rule. +- Comment-only surfaces, read-only views, and the portable guides.show viewer: + they pass no hover handlers and gain no props. This is now what gates the + `pn-token-hover` treatment as well: the class carries a `cursor: pointer` + that only wins over Pierre's I-beam with `!important`, so it is painted only + where a hover handler is actually wired. It is also painted in + `AllFilesCodeView` now, not just `DiffViewer` — that view is the default + review surface, and leaving it out meant the underline the announcement's + try-it demonstrates was absent from the surface most reviewers use. +- `canUseLiveWorkspaceActions` remains an independent hard gate above the + setting. +- The server (`POST /api/code-nav/hover`) in either runtime. + +## 10. Test plan (critical path only) + +Per the repo's testing rules, each of these names a failure it catches. + +1. **Modifier gating arms nothing** (`useTokenHover.test.tsx`): in `modifier` + mode, an enter with Alt up spawns no request no matter how long time + advances. Catches a regression where the gate moves after the dwell, which + would restore per-token ripgrep on an idle sweep. +2. **Modifier pressed while hovering opens the card** (same suite): the + gesture the mode exists for. Catches a gate that only reads the enter event. +3. **Modifier released starts the close** (same suite), and does not when the + pointer is in the card. Catches a card that cannot be read or one that + cannot be dismissed. +3b. **A chord opens nothing and closes an open card**: Cmd+C with the pointer + parked on a token must not pop a card mid-copy. +4. **Delay feeds the dwell** (same suite): a 700 delay spawns nothing at 300. + Catches the constant being left hardcoded. +5. **Legacy cookie migration** (new `tokenHoverSetting.test.ts`, pure lane): + `plannotator-token-hover-cards=false` resolves to `off`; `true` and absent + resolve to `hover`; the new key wins over the legacy one; round-trip through + `toCookie`/`fromCookie`; unrecognized values fall through to the default. + Catches silently re-enabling a feature a user turned off. +6. **Announcement once-ness and chain gate** (new + `tokenHoverAnnouncement.test.ts`, pure lane): needed until marked seen, + version bump re-shows, and the predicate is false while loading or while any + earlier chain dialog is visible. Same shape as `editModeAnnouncement.test.ts`. +7. **The dialog's radio applies the setting** (DOM lane): selecting an option + writes the trigger. Catches a dialog whose choice is decorative. + +New DOM-gated files are registered in `.github/workflows/test.yml`'s +`--isolate` lane beside the existing hover tests. diff --git a/packages/core/guide-viewer-manifest.ts b/packages/core/guide-viewer-manifest.ts index ba5e257e08..a02c7750ee 100644 --- a/packages/core/guide-viewer-manifest.ts +++ b/packages/core/guide-viewer-manifest.ts @@ -5,10 +5,10 @@ import type { GuideViewerAssets } from "./guide-format"; export const GUIDE_VIEWER_MANIFEST: Omit = { - js: "viewer.DpysBozt.js", - css: "viewer.C2JutkwF.css", - jsIntegrity: "sha384-jYwXTMOLsFhrgmcIWkKkd8nt0Z1d9UvIx3UD/qmvlQ2j+ImpAksADyjgO/hAJU+i", - cssIntegrity: "sha384-VSwARSMv4pnUClW4PvJM2o9oMfwKSYRePc5WxS2RzqByWWyGq/yQD6BlNlEMGx+u", + js: "viewer.C8JgNbZu.js", + css: "viewer.DPA8Hcib.css", + jsIntegrity: "sha384-dqRkZ2N5WNwLIuwa+1kdrvswkOVHpkBxrrJ5mTU4YrUWzO/8fRS5MRTlDGV9t7eB", + cssIntegrity: "sha384-Bs19ETlOYAovT/ItKvQ9S93jOGOSYjsC6QQk4+oTMunGv7TJ0kxWApsP7hF2fHGE", langs: { "astro": "chunks/astro.BykyiR6i.js", "c": "chunks/c.BIGW1oBm.js", diff --git a/packages/core/package.json b/packages/core/package.json index efc0da7911..b6249c89b2 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -26,6 +26,7 @@ "./open-in-apps": "./open-in-apps.ts", "./project": "./project.ts", "./source-save": "./source-save.ts", + "./token-hover": "./token-hover.ts", "./config-types": "./config-types.ts", "./storage-types": "./storage-types.ts", "./workspace-status-types": "./workspace-status-types.ts", diff --git a/packages/core/token-hover.ts b/packages/core/token-hover.ts new file mode 100644 index 0000000000..e0bd9c4de5 --- /dev/null +++ b/packages/core/token-hover.ts @@ -0,0 +1,88 @@ +/** + * Token hover cards: how the card is triggered, and how long the pointer has + * to rest before one is requested. + * + * Two settings, deliberately. "Hover cards annoy me" is three complaints with + * three remedies: never want them (`off`), want them on request rather than + * while reading (`modifier`), and want them just less eager (a longer dwell). + * A modifier mode does not serve the third, and a delay does not serve the + * second. Everything else about the card (the leave grace, the render + * threshold, the cache) is a correctness constant, not a taste. + */ + +/** + * How a hover card opens. + * + * - `hover`: rest the pointer on a symbol. The shipped default. + * - `modifier`: only while the platform's primary modifier is held, Cmd on + * macOS and Ctrl on Windows and Linux. With the key up nothing is armed and + * nothing reaches the wire. + * - `off`: the handlers are never wired, so there are no listeners, no + * requests and no card in the tree. + * + * Cmd rather than Alt for two reasons. Alt is widely bound to push-to-talk + * dictation, so an Alt-held gate would open cards while the user is speaking. + * And Cmd+hover is already VS Code's "tell me about this symbol" gesture, so + * this rides muscle memory instead of competing with it: the navigable-target + * underline and the card appearing together under one held key is the + * composite gesture, not a collision. Cmd+click still supersedes the card, + * because every References invocation dismisses the hover surface first. + * + * The stored value stays `modifier`, deliberately: the setting names the shape + * of the gate, not which key fills it, so nothing had to migrate. + */ +export type TokenHoverTrigger = "hover" | "modifier" | "off"; + +/** Every trigger, in the order the Settings control presents them. */ +export const TOKEN_HOVER_TRIGGERS = [ + "hover", + "modifier", + "off", +] as const satisfies readonly TokenHoverTrigger[]; + +export function isTokenHoverTrigger(value: unknown): value is TokenHoverTrigger { + return value === "hover" || value === "modifier" || value === "off"; +} + +/** + * Dwell before any request exists, in milliseconds. Three fixed steps rather + * than a slider: the perceptible granularity here is around 150ms, so offering + * finer precision would imply a difference nobody can feel. + */ +export type TokenHoverDelay = 150 | 300 | 700; + +/** The dwell #1461 shipped, and still the default. */ +export const DEFAULT_TOKEN_HOVER_DELAY_MS = 300 satisfies TokenHoverDelay; + +/** Every delay, fastest first, in the order the Settings control presents them. */ +export const TOKEN_HOVER_DELAYS = [ + 150, + 300, + 700, +] as const satisfies readonly TokenHoverDelay[]; + +export function isTokenHoverDelay(value: unknown): value is TokenHoverDelay { + return value === 150 || value === 300 || value === 700; +} + +/** + * Read a stored trigger, migrating the pre-#1461-follow-up boolean. + * + * `tokenHoverCards` was one on/off switch. Its `false` is a real preference + * ("I turned these off") and must survive, so the legacy value is READ here + * and never written back: the new key is seeded on first settings access, and + * the legacy cookie is left in place so a downgrade does not silently + * re-enable cards for someone who had turned them off. + * + * Returns undefined when neither key says anything, which lets the settings + * registry apply its own default. + */ +export function resolveStoredTokenHoverTrigger( + stored: string | null | undefined, + legacyEnabled: string | null | undefined, +): TokenHoverTrigger | undefined { + if (isTokenHoverTrigger(stored)) return stored; + if (legacyEnabled === "false") return "off"; + if (legacyEnabled === "true") return "hover"; + return undefined; +} diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index 5d509e5de2..d0f7f93d5a 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -162,6 +162,13 @@ import { markEditModeAnnouncementSeen, needsEditModeAnnouncement, } from './utils/editModeAnnouncement'; +import { TokenHoverAnnouncementDialog } from './components/TokenHoverAnnouncementDialog'; +import { + markTokenHoverAnnouncementSeen, + resolveTokenHoverAnnouncementPending, + shouldConsumeTokenHoverAnnouncement, + tokenHoverAnnouncementCanShow, +} from './utils/tokenHoverAnnouncement'; import { ExternalLineAnnotationComposer } from './components/ExternalLineAnnotationComposer'; import { DestinationSpotlight } from './components/DestinationSpotlight'; import { needsDestinationSpotlight, markDestinationSpotlightSeen } from './utils/destinationSpotlight'; @@ -439,7 +446,8 @@ const ReviewApp: React.FC = () => { const diffTabSize = useConfigValue('diffTabSize'); const reviewShowViewedControls = useConfigValue('reviewShowViewedControls'); const reviewShowStageControls = useConfigValue('reviewShowStageControls'); - const tokenHoverCardsEnabled = useConfigValue('tokenHoverCards'); + const tokenHoverTrigger = useConfigValue('tokenHoverTrigger'); + const tokenHoverDelay = useConfigValue('tokenHoverDelay'); // EXPERIMENTAL: edit code in place to author suggestions (default OFF). const editSuggestionsEnabled = useConfigValue('editSuggestions'); const semanticDiffEnabled = useConfigValue('semanticDiffEnabled'); @@ -1060,6 +1068,23 @@ const ReviewApp: React.FC = () => { enableEditSuggestionsFromAnnouncement(); setEditModeIntroPending(false); }, []); + // One-time token hover card announcement. LAST in the dialog chain (guide + // intro → look-and-feel → review setup → edit mode → token hover) — the + // chain dialogs never stack. Latched at mount so choosing a trigger inside + // the dialog does not unmount it mid-click; a user who already has a + // non-default trigger never sees it (resolveTokenHoverAnnouncementPending). + const [tokenHoverIntroPending, setTokenHoverIntroPending] = useState( + resolveTokenHoverAnnouncementPending, + ); + const dismissTokenHoverIntro = useCallback(() => { + markTokenHoverAnnouncementSeen(); + setTokenHoverIntroPending(false); + }, []); + // Retiring the announcement for a reviewer who already chose a trigger is a + // WRITE, so it belongs here rather than in the state initializer above. + useEffect(() => { + if (shouldConsumeTokenHoverAnnouncement()) markTokenHoverAnnouncementSeen(); + }, []); const aiChat = useAIChat({ patch: diffData?.rawPatch ?? '', diffType, @@ -1086,9 +1111,51 @@ const ReviewApp: React.FC = () => { } = aiChat; const codeNav = useCodeNav(); - const tokenHover = useTokenHover(snapshotId); + // The other half of the held-modifier gesture. The diff views paint + // `pn-token-nav` from the pointer ENTER event, which covers "hold the key, + // then move onto a symbol" but neither half of the gesture modifier mode + // exists for: a key going down over a parked pointer, and the release after + // it, fire no pointer event at all. The hook reports those two transitions + // and this paints them, so the affordance and the card arrive and leave + // together — which is the composite gesture the mode is modelled on. + // + // It lives here rather than in the views because both are compiled into the + // portable guides.show viewer and their prop signatures must not move. + const navAffordanceRef = useRef(null); + const handleModifierGate = useCallback((armed: boolean, tokenElement: HTMLElement | null) => { + const previous = navAffordanceRef.current; + // The pointer can drift to a neighbour while the key is held: that token + // was painted by its own enter event, and this one was left painted by an + // arm. Both come off. + if (previous && previous !== tokenElement) previous.classList.remove('pn-token-nav'); + navAffordanceRef.current = null; + if (!tokenElement) return; + if (!armed) { + tokenElement.classList.remove('pn-token-nav'); + return; + } + tokenElement.classList.add('pn-token-nav'); + navAffordanceRef.current = tokenElement; + }, []); + // `off` never reaches the hook: it is enforced below by withholding the + // handler props entirely, so the diff views wire no listeners at all. + const tokenHover = useTokenHover(snapshotId, { + mode: tokenHoverTrigger === 'modifier' ? 'modifier' : 'hover', + delayMs: tokenHoverDelay, + onModifierGate: handleModifierGate, + }); + + const closeTokenHover = tokenHover.close; const handleCodeNavRequest = useCallback((request: CodeNavRequest) => { + // Opening References is a deliberate action; a hover is an idle gesture, + // and the two must never be on screen together. This covers EVERY route + // in: Cmd+click, Ctrl+click, the Alt+click alias, and the card's own + // location links. It also settles the overlap #1461 shipped with, where a + // Cmd+click landed on a token whose hover card was open or mid-dwell and + // both surfaces appeared. close() cancels the pending dwell too, so a + // click during the dwell never resolves into a card behind the panel. + closeTokenHover(); if (!gitContext && !agentCwd) { toast('Code navigation requires a local checkout', { description: 'Re-run with --local for PR reviews', @@ -1118,7 +1185,7 @@ const ReviewApp: React.FC = () => { initialHeight: 250, }); } - }, [codeNav.resolve, dockApi, isAllFilesActive, isCallFlowActive, isSemanticDiffActive, gitContext, agentCwd]); + }, [closeTokenHover, codeNav.resolve, dockApi, isAllFilesActive, isCallFlowActive, isSemanticDiffActive, gitContext, agentCwd]); // Check AI capabilities only after /api/diff confirms AI is enabled. useEffect(() => { @@ -1415,9 +1482,39 @@ const ReviewApp: React.FC = () => { // Token hover cards ride the same gate as Cmd+click code navigation, plus // their own setting. Off means no handler props reach the diff views, so // there are no listeners, no requests and no card in the tree. - const tokenHoverEnabled = canUseLiveWorkspaceActions && tokenHoverCardsEnabled; + const tokenHoverEnabled = canUseLiveWorkspaceActions && tokenHoverTrigger !== 'off'; + // Announcing a feature this session cannot run is noise, so a stack/branch + // view skips it — and skips it WITHOUT consuming the cookie, so the next + // ordinary review still shows it (same rule the guide intro uses for an + // empty diff). `off` is not part of the availability test: a user who + // reaches the dialog has, by construction, never chosen a trigger. + // + // Eligibility is LATCHED once the initial load clears: + // canUseLiveWorkspaceActions changes on mid-session diff switches, and a + // stack→ordinary switch must not pop the announcement over work in + // progress, nor an ordinary→stack switch yank an open one away mid-read. + // + // In an effect rather than in the render body: a latch written during render + // is a side effect React may discard (a concurrent render that never + // commits would still have stamped the ref). The commit ordering is safe + // because the gate below independently requires !isLoading, so the render + // that first clears the flag shows no dialog and the effect has latched + // before the next one. + const [tokenHoverAvailable, setTokenHoverAvailable] = useState(null); + useEffect(() => { + if (isLoading) return; + setTokenHoverAvailable((current) => (current === null ? canUseLiveWorkspaceActions : current)); + }, [isLoading, canUseLiveWorkspaceActions]); + const tokenHoverIntroVisible = tokenHoverAnnouncementCanShow({ + announcementPending: tokenHoverIntroPending, + isLoading, + featureAvailable: tokenHoverAvailable === true, + guideIntroVisible, + lookAndFeelVisible: showLookAndFeel, + reviewSetupVisible: showReviewSetup, + editModeVisible: editModeIntroVisible, + }); const hoveredTokenSymbol = tokenHover.hover?.request.symbol; - const closeTokenHover = tokenHover.close; const startTokenHover = tokenHover.onTokenHoverEnter; // Stitching lives here, not in the diff views: rebuilding a fragmented // identifier is app-only work, and both views are compiled into the portable @@ -1432,9 +1529,15 @@ const ReviewApp: React.FC = () => { useEffect(() => { if (canUseLiveWorkspaceActions) return; codeNav.clear(); - closeTokenHover(); dockApi?.getPanel(REVIEW_CODE_NAV_PANEL_ID)?.api.close(); - }, [canUseLiveWorkspaceActions, codeNav.clear, closeTokenHover, dockApi]); + }, [canUseLiveWorkspaceActions, codeNav.clear, dockApi]); + // Turning cards off (or losing the workspace gate) with one already open + // would otherwise leave it standing over the diff with nothing left to + // close it: the handler props are gone, so no leave event can arrive. + useEffect(() => { + if (tokenHoverEnabled) return; + closeTokenHover(); + }, [tokenHoverEnabled, closeTokenHover]); const handleTokenHoverSelectLocation = useCallback( (location: { filePath: string; line: number; column: number }) => { closeTokenHover(); @@ -2379,7 +2482,7 @@ const ReviewApp: React.FC = () => { // (not lost) behind the guide takeover or a first-run dialog — the file // still marks and the next auto-view retries the toast. if (!needsAutoViewedNotice()) return; - if (guideOpen || guideIntroVisible || showLookAndFeel || showReviewSetup || editModeIntroVisible) return; + if (guideOpen || guideIntroVisible || showLookAndFeel || showReviewSetup || editModeIntroVisible || tokenHoverIntroVisible) return; markAutoViewedNoticeSeen(); toast('Files are marked viewed as you scroll', { description: "Scroll past a file or move on to the next and it's checked off. Turn this off in Settings → Git, or from the gear above the file list.", @@ -2399,7 +2502,7 @@ const ReviewApp: React.FC = () => { }, }, }); - }, [guideOpen, guideIntroVisible, showLookAndFeel, showReviewSetup, editModeIntroVisible]); + }, [guideOpen, guideIntroVisible, showLookAndFeel, showReviewSetup, editModeIntroVisible, tokenHoverIntroVisible]); const { handleReadingFileChange: handleAutoViewReadingFile, handleFileScrolledPast } = useAutoViewed({ enabled: autoViewedEnabled, // Rule 4 — only the review target. The guide takeover CSS-hides the dock @@ -4053,7 +4156,7 @@ const ReviewApp: React.FC = () => { if (event.defaultPrevented || isNativeHistoryOwner(event)) return false; if (submitted || isSendingFeedback || isApproving || isExiting || isPlatformActioning || isLoadingDiff) return false; if (guideOpen || openSettingsMenu || showDestinationMenu || platformCommentDialog || showExportModal || showWorktreeDialog || showNoAnnotationsDialog || showExitWarning) return false; - if (showLookAndFeel || showGuideIntro || showReviewSetup || editModeIntroVisible || tourDialogJobId) return false; + if (showLookAndFeel || showGuideIntro || showReviewSetup || editModeIntroVisible || tokenHoverIntroVisible || tourDialogJobId) return false; return !hasActiveHistoryOverlay(document); }, [ guideOpen, @@ -4063,6 +4166,7 @@ const ReviewApp: React.FC = () => { isPlatformActioning, isSendingFeedback, editModeIntroVisible, + tokenHoverIntroVisible, openSettingsMenu, platformCommentDialog, showDestinationMenu, @@ -5416,12 +5520,20 @@ const ReviewApp: React.FC = () => { /> )} + {/* One-time token hover card announcement. LAST in the dialog chain + (guide intro → look-and-feel → review setup → edit mode → token + hover) — tokenHoverAnnouncementCanShow gates on every earlier + dialog, so the chain dialogs never stack. */} + {tokenHoverIntroVisible && ( + + )} + {/* One-time PR feedback-destination spotlight. Strictly AFTER the first-run dialog chain (guide intro → look-and-feel → review - setup → edit mode): it only mounts once none of the four is + setup → edit mode → token hover): it only mounts once none of the five is showing, so it never stacks with them. PR mode only — the switcher it points at doesn't render otherwise. */} - {showDestSpotlight && !isCompactTouchLayout && !!prMetadata && !isLoading && !showLookAndFeel && !guideIntroVisible && !showReviewSetup && !editModeIntroVisible && ( + {showDestSpotlight && !isCompactTouchLayout && !!prMetadata && !isLoading && !showLookAndFeel && !guideIntroVisible && !showReviewSetup && !editModeIntroVisible && !tokenHoverIntroVisible && ( 200 -> 0" jump heuristic. @@ -1995,6 +1996,13 @@ export const AllFilesCodeView: React.FC = ({ props.tokenElement.classList.add('pn-token-nav'); } if (!onTokenHoverEnter || item.type !== 'diff') return; + // The hovered-token treatment (underline + pointer cursor), the same one + // DiffViewer paints and the one the announcement dialog's try-it shows. + // This view is the DEFAULT review surface, so leaving it out meant the + // affordance the demo advertises was absent from the surface most + // reviewers actually use. Gated on the hover handler, so a read-only or + // portable host (which passes none) still paints nothing. + props.tokenElement.classList.add('pn-token-hover'); // File identity comes from the owning item, exactly as handleTokenClick // resolves it — never from an active-file side channel. const filePath = itemIdToFilePath.get(item.id); @@ -2005,6 +2013,7 @@ export const AllFilesCodeView: React.FC = ({ const handleTokenLeave = useStableCallback((props: DiffTokenEventBaseProps) => { props.tokenElement.classList.remove('pn-token-nav'); + props.tokenElement.classList.remove('pn-token-hover'); onTokenHoverLeave?.(); }); diff --git a/packages/review-editor/components/DiffViewer.tsx b/packages/review-editor/components/DiffViewer.tsx index 0c4df51314..ab3eef6889 100644 --- a/packages/review-editor/components/DiffViewer.tsx +++ b/packages/review-editor/components/DiffViewer.tsx @@ -731,7 +731,11 @@ export const DiffViewer: React.FC = ({ }, [filePath, onCodeNavRequest]); const handleTokenEnter = useCallback((props: DiffTokenEventBaseProps, event: PointerEvent) => { - props.tokenElement.classList.add('pn-token-hover'); + // Only where a card can actually open. The class carries a pointer cursor + // (now !important, so it genuinely wins over Pierre's I-beam), and painting + // it unconditionally promised clickability on every token even with hover + // cards switched off. + if (onTokenHoverEnter) props.tokenElement.classList.add('pn-token-hover'); if ((event.metaKey || event.ctrlKey) && onCodeNavRequest) { props.tokenElement.classList.add('pn-token-nav'); } diff --git a/packages/review-editor/components/TokenHoverAnnouncementDialog.test.tsx b/packages/review-editor/components/TokenHoverAnnouncementDialog.test.tsx new file mode 100644 index 0000000000..e98190dd32 --- /dev/null +++ b/packages/review-editor/components/TokenHoverAnnouncementDialog.test.tsx @@ -0,0 +1,252 @@ +/** + * DOM-gated tests (DOM_TESTS=1) for the one-time token hover card + * announcement. Registered in .github/workflows/test.yml's "Run UI + * seam-contract + DOM tests" step. + * + * The failure to catch is a decorative dialog: three options that look like a + * choice but write nothing, so a reviewer who picks "Off" still gets cards. + * Dismissal must therefore be "accept what is selected", with no separate + * confirm step that could drop the selection. + */ +import { afterEach, beforeEach, describe, expect, jest, test } from 'bun:test'; +import React from 'react'; +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { configStore } from '@plannotator/ui/config'; +import { resetStorageBackend, setStorageBackend } from '@plannotator/ui/utils/storage'; +import { isMac, modEventKey } from '@plannotator/ui/utils/platform'; +import { DEFAULT_TOKEN_HOVER_DELAY_MS } from '@plannotator/shared/token-hover'; +import { EXAMPLE_HOVER, TokenHoverAnnouncementDialog } from './TokenHoverAnnouncementDialog'; + +const hasDom = typeof document !== 'undefined'; + +let host: HTMLDivElement | null = null; +let root: Root | null = null; +let stored: Map; + +async function mount(node: React.ReactElement): Promise { + host = document.createElement('div'); + document.body.appendChild(host); + root = createRoot(host); + await act(async () => { + root!.render(node); + }); +} + +function option(value: string): HTMLButtonElement { + const el = document.querySelector(`[data-token-hover-trigger-option="${value}"]`); + if (!el) throw new Error(`no trigger option "${value}" in the dialog`); + return el; +} + +async function click(el: HTMLElement): Promise { + await act(async () => { + el.dispatchEvent(new MouseEvent('click', { bubbles: true })); + }); +} + +beforeEach(() => { + if (!hasDom) return; + // Seed the trigger explicitly rather than relying on the registry default. + // configStore is a process-global singleton and loadFromBackend keeps its + // in-memory value when the new backend has nothing to say, so an empty map + // would inherit whatever the previous test (or test FILE) last set. + stored = new Map([['plannotator-token-hover-trigger', 'hover']]); + setStorageBackend({ + getItem: key => stored.get(key) ?? null, + setItem: (key, value) => { stored.set(key, value); }, + removeItem: key => { stored.delete(key); }, + }); + configStore.loadFromBackend(); +}); + +afterEach(async () => { + if (!hasDom) return; + if (root) { + const current = root; + await act(async () => current.unmount()); + root = null; + } + host?.remove(); + host = null; + resetStorageBackend(); + // Same reason as tokenHoverAnnouncement.test.ts: the store is a process + // global that this file re-resolved against a fake backend, so hand it back + // to the real one rather than leaving it holding this file's values. + configStore.loadFromBackend(); +}); + +describe.skipIf(!hasDom)('TokenHoverAnnouncementDialog', () => { + test('renders nothing at all when closed', async () => { + await mount( {}} />); + expect(document.querySelector('[data-token-hover-announcement-dialog]')).toBeNull(); + }); + + test('the try-it is labeled, and its mock code stays out of the reading order', async () => { + // It is interactive now, so hiding the whole block would make it + // unreachable and unexplained. The region carries the label; the mock code + // inside is decorative (read aloud it is a wall of invented identifiers) + // and the visible prompt line carries the meaning instead. + await mount( {}} />); + const region = document.querySelector('[data-token-hover-example]')!; + expect(region.getAttribute('aria-hidden')).toBeNull(); + expect(region.getAttribute('role')).toBe('group'); + expect(region.getAttribute('aria-label')).toBeTruthy(); + expect( + document.querySelector('[data-token-hover-example-code]')!.getAttribute('aria-hidden'), + ).toBe('true'); + // The prompt names the token the reviewer is being asked to hover. + const prompt = document.querySelector('[data-token-hover-tryit-prompt]')!; + expect(prompt.textContent).toContain('withRetry'); + }); + + test('hovering the try-it token opens the real card from the fixture', async () => { + // This is the whole point of the seam: real dwell, real card component, + // fixture answer. If someone reimplements the demo with its own timing or + // its own markup, the card either stops matching the product or stops + // behaving like it. + jest.useFakeTimers(); + try { + await mount( {}} />); + const token = document.querySelector('[data-token-hover-example-token]')!; + + await act(async () => { + token.dispatchEvent(new MouseEvent('pointerover', { bubbles: true })); + }); + // Nothing before the dwell elapses. + expect(document.querySelector('[data-token-hover-card]')).toBeNull(); + + // The try-it reads the live `tokenHoverDelay`, which the seeded backend + // says nothing about, so the dwell here is the registry default. + await act(async () => { jest.advanceTimersByTime(DEFAULT_TOKEN_HOVER_DELAY_MS); }); + await act(async () => { await Promise.resolve(); await Promise.resolve(); }); + + // `data-token-hover-card` is the real component's own marker. + const card = document.querySelector('[data-token-hover-card]'); + expect(card).not.toBeNull(); + expect(card!.textContent).toContain(EXAMPLE_HOVER.symbol); + expect(card!.textContent).toContain(EXAMPLE_HOVER.definition!.filePath); + // No References panel behind the demo, so its locations are neither + // tabbable nor advertised as clickable, and the card is out of the + // reading order (it portals to , outside the aria-modal dialog). + for (const b of card!.querySelectorAll('button')) { + expect(b.tabIndex).toBe(-1); + expect(b.className).toContain('cursor-default'); + } + expect(card!.getAttribute('aria-hidden')).toBe('true'); + // The card portals to like every other instance, so the dialog it + // is demonstrated INSIDE would cover it without an explicit layer. This + // is the seam's whole reason for existing: a buried demo card is exactly + // the failure a live try-it is supposed to make impossible. + expect(card!.className).toContain('z-[110]'); + } finally { + jest.useRealTimers(); + } + }); + + test('the try-it obeys the trigger the radio currently says', async () => { + // Choosing the hold-modifier option has to be feelable BEFORE committing + // to it, which is why the try-it reads the live setting, not a prop. + jest.useFakeTimers(); + try { + await mount( {}} />); + await click(option('modifier')); + + const token = document.querySelector('[data-token-hover-example-token]')!; + await act(async () => { + token.dispatchEvent(new MouseEvent('pointerover', { bubbles: true })); + }); + await act(async () => { jest.advanceTimersByTime(2000); }); + await act(async () => { await Promise.resolve(); }); + expect(document.querySelector('[data-token-hover-card]')).toBeNull(); + + // Modifier down, and the same rest opens it. + await act(async () => { + window.dispatchEvent(new KeyboardEvent('keydown', { + key: modEventKey, + metaKey: isMac, + ctrlKey: !isMac, + })); + }); + await act(async () => { jest.advanceTimersByTime(DEFAULT_TOKEN_HOVER_DELAY_MS); }); + await act(async () => { await Promise.resolve(); await Promise.resolve(); }); + expect(document.querySelector('[data-token-hover-card]')).not.toBeNull(); + + // And Off takes an open card away with it. + await click(option('off')); + expect(document.querySelector('[data-token-hover-card]')).toBeNull(); + } finally { + jest.useRealTimers(); + } + }); + + test('the radio applies the trigger immediately, with no confirm step', async () => { + await mount( {}} />); + expect(configStore.get('tokenHoverTrigger')).toBe('hover'); + + await click(option('modifier')); + expect(configStore.get('tokenHoverTrigger')).toBe('modifier'); + expect(option('modifier').getAttribute('aria-checked')).toBe('true'); + expect(option('hover').getAttribute('aria-checked')).toBe('false'); + + await click(option('off')); + expect(configStore.get('tokenHoverTrigger')).toBe('off'); + }); + + test('dismissing keeps the chosen trigger', async () => { + let dismissed = 0; + await mount( { dismissed += 1; }} />); + + await click(option('off')); + const done = Array.from(document.querySelectorAll('button')).find(b => b.textContent === 'Done'); + await click(done!); + + expect(dismissed).toBe(1); + expect(configStore.get('tokenHoverTrigger')).toBe('off'); + }); + + test('arrows move selection within the group, which is one Tab stop', async () => { + // WAI-ARIA radiogroup. Without roving tabindex a keyboard user Tabs + // through three options to reach Done; without the arrow handler they + // cannot change the selection with the keyboard at all. + await mount( {}} />); + expect(option('hover').tabIndex).toBe(0); + expect(option('modifier').tabIndex).toBe(-1); + + await act(async () => { + option('hover').dispatchEvent( + new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true }), + ); + }); + expect(configStore.get('tokenHoverTrigger')).toBe('modifier'); + expect(option('modifier').tabIndex).toBe(0); + expect(option('hover').tabIndex).toBe(-1); + + // Wraps backwards off the first option. + await act(async () => { + option('modifier').dispatchEvent( + new KeyboardEvent('keydown', { key: 'ArrowUp', bubbles: true }), + ); + }); + expect(configStore.get('tokenHoverTrigger')).toBe('hover'); + await act(async () => { + option('hover').dispatchEvent( + new KeyboardEvent('keydown', { key: 'ArrowUp', bubbles: true }), + ); + }); + expect(configStore.get('tokenHoverTrigger')).toBe('off'); + }); + + test('Escape dismisses and also keeps the chosen trigger', async () => { + let dismissed = 0; + await mount( { dismissed += 1; }} />); + + await click(option('modifier')); + await act(async () => { + document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })); + }); + + expect(dismissed).toBe(1); + expect(configStore.get('tokenHoverTrigger')).toBe('modifier'); + }); +}); diff --git a/packages/review-editor/components/TokenHoverAnnouncementDialog.tsx b/packages/review-editor/components/TokenHoverAnnouncementDialog.tsx new file mode 100644 index 0000000000..296df5828e --- /dev/null +++ b/packages/review-editor/components/TokenHoverAnnouncementDialog.tsx @@ -0,0 +1,411 @@ +import React, { useCallback, useEffect, useRef } from 'react'; +import { createPortal } from 'react-dom'; +import { configStore, useConfigValue } from '@plannotator/ui/config'; +import { modKeyWord } from '@plannotator/ui/utils/platform'; +import type { CodeNavHoverResponse, CodeNavRequest } from '@plannotator/shared/code-nav'; +import { TokenHoverCard } from './TokenHoverCard'; +import { useTokenHover } from '../hooks/useTokenHover'; +import { TOKEN_HOVER_UNDERLINE_STYLE } from './tokenHoverStyles'; +import { + TOKEN_HOVER_TRIGGERS, + type TokenHoverTrigger, +} from '@plannotator/shared/token-hover'; + +/** + * One-time hover card announcement, in the same big-format shell as + * EditModeAnnouncementDialog and LookAndFeelAnnouncementDialog: a worked + * example on the left, the decision on the right. + * + * LAST in the first-run dialog chain (guide intro, look-and-feel, review + * setup, edit mode, then this). The App gates rendering through + * tokenHoverAnnouncementCanShow so the chain dialogs never stack. + * + * The example is a TRY-IT: the token in the strip is genuinely hoverable and + * opens the REAL card through the REAL hook, governed by whichever trigger and + * delay the radio currently says. Only the answer is a fixture, so the demo + * never searches the reviewer's repository. Nothing here is a redraw, so it + * cannot drift from the surface it illustrates. + * + * The radio group writes the real setting on selection, so Done and Escape + * both mean "accept what is selected" and a choice can never be lost by + * dismissing. + */ + +interface TokenHoverAnnouncementDialogProps { + readonly isOpen: boolean; + /** Mark seen and close. Also wired to Escape. */ + readonly onDismiss: () => void; +} + +const TRIGGER_COPY: Record = { + hover: { + label: 'On hover', + description: 'Rest the pointer on a symbol and the card appears. This is the default.', + }, + modifier: { + label: `While holding ${modKeyWord}`, + description: `Cards stay out of the way until you hold ${modKeyWord}. Nothing is searched while the key is up.`, + }, + off: { + label: 'Off', + description: 'No cards, no listeners, no searches.', + }, +}; + +/** + * The fixture the example card is rendered from. A realistic + * CodeNavHoverResponse, nothing more: the card itself decides what to show. + * + * Exported so the DOM test can assert the rendered card against the SAME + * fixture it was built from, which is what pins "the example is the real + * card" rather than a copy of its markup. + */ +/** The symbol the try-it demonstrates. Named in the prompt line too. */ +const EXAMPLE_SYMBOL = 'withRetry'; + +export const EXAMPLE_HOVER: CodeNavHoverResponse = { + backend: 'search', + source: 'search', + symbol: EXAMPLE_SYMBOL, + definition: { + filePath: 'src/lib/retry.ts', + line: 24, + column: 22, + confidence: 'likely', + symbolKind: 'function', + signature: 'export async function withRetry(fn, attempts = 3) {', + signatureApproximate: true, + doc: 'Runs fn, retrying with exponential backoff until attempts runs out.', + preview: null, + otherCandidateCount: 0, + }, + alternateDefinition: null, + references: [ + { filePath: 'src/checkout/order.ts', line: 42, column: 24 }, + { filePath: 'src/billing/settle.ts', line: 88, column: 11 }, + ], + referenceCount: 7, + capped: false, + stats: { elapsedMs: 18 }, +}; + +/** + * The try-it's request, shaped exactly like one the diff pane would build. + * Only the resolver is a fixture; everything downstream is the real pipeline. + */ +const EXAMPLE_REQUEST: CodeNavRequest = { + symbol: EXAMPLE_SYMBOL, + filePath: 'src/checkout/order.ts', + line: 42, + charStart: 24, + side: 'new', + language: 'typescript', +}; + +/** + * The illustration is a TRY-IT, not an exhibit: the token below is genuinely + * hoverable and opens the REAL card through the REAL hook, so the reviewer + * feels the actual dwell, the leave grace and the modifier gate before + * committing to a setting. The only thing that is not real is where the answer comes + * from, and that is deliberate: a demo must not search the reviewer's + * repository for a symbol they never asked about. + * + * The trigger and delay come from the LIVE setting, so flipping the radio to + * the hold-modifier option makes the try-it behave that way immediately, and "Off" + * makes it do nothing, which is the honest preview of that choice. + */ +function HoverCardTryIt({ + mode, + delayMs, + enabled, +}: { + mode: 'hover' | 'modifier'; + delayMs: number; + enabled: boolean; +}) { + const tokenRef = useRef(null); + + // No network, no LRU sharing with the app: this hook instance is its own. + const resolve = useCallback(async () => EXAMPLE_HOVER, []); + const tokenHover = useTokenHover('token-hover-announcement', { mode, delayMs, resolve }); + + const { onTokenHoverEnter, onTokenHoverLeave, close } = tokenHover; + + // Turning the try-it off (the Off option) must take any open card with it; + // no leave event arrives when the handlers simply stop being wired. + useEffect(() => { + if (!enabled) close(); + }, [enabled, close]); + + return ( +
+

+ Try it: rest your pointer on {EXAMPLE_SYMBOL} below. +

+ + + + {enabled && tokenHover.hover && ( + {}} + inert + // Above the modal this is demonstrated inside; the app's own default + // layer sits under it. + layerClassName="fixed z-[110]" + /> + )} +
+ ); +} + +export function TokenHoverAnnouncementDialog({ + isOpen, + onDismiss, +}: TokenHoverAnnouncementDialogProps) { + const trigger = useConfigValue('tokenHoverTrigger'); + const delayMs = useConfigValue('tokenHoverDelay'); + const optionRefs = useRef<(HTMLButtonElement | null)[]>([]); + const primaryActionRef = useRef(null); + const previousFocusRef = useRef(null); + const onDismissRef = useRef(onDismiss); + + useEffect(() => { + onDismissRef.current = onDismiss; + }, [onDismiss]); + + useEffect(() => { + if (!isOpen) return; + + previousFocusRef.current = document.activeElement instanceof HTMLElement + ? document.activeElement + : null; + primaryActionRef.current?.focus(); + + const handleKeyDown = (event: KeyboardEvent) => { + if (event.key === 'Escape') { + event.preventDefault(); + onDismissRef.current(); + return; + } + if (event.key !== 'Tab') return; + + const dialog = document.querySelector('[data-token-hover-announcement-dialog]'); + const focusable = Array.from( + // `:not([tabindex="-1"])` has to apply to the BUTTONS too, not just the + // [tabindex] arm: the roving radio group parks two unselected options + // at -1 and neither belongs in the Tab cycle. (The try-it's card is + // not a factor either way — it portals to , so this query never + // reaches it; its own locations leave the tab order via `inert`.) + dialog?.querySelectorAll('button:not([disabled]):not([tabindex="-1"]), [href], [tabindex]:not([tabindex="-1"])') + ?? [], + ); + if (focusable.length === 0) return; + + const first = focusable[0]; + const last = focusable[focusable.length - 1]; + if (event.shiftKey && document.activeElement === first) { + event.preventDefault(); + last.focus(); + } else if (!event.shiftKey && document.activeElement === last) { + event.preventDefault(); + first.focus(); + } + }; + + document.addEventListener('keydown', handleKeyDown); + return () => { + document.removeEventListener('keydown', handleKeyDown); + previousFocusRef.current?.focus(); + previousFocusRef.current = null; + }; + }, [isOpen]); + + /** + * Arrow keys move selection within the group, per the WAI-ARIA radiogroup + * pattern: both axes are accepted because the group is a vertical list of + * horizontally-laid-out rows, and both wrap. Selection follows focus, which + * is what makes the group a single Tab stop that still reaches every option. + */ + const handleOptionKeyDown = (event: React.KeyboardEvent, index: number) => { + const forward = event.key === 'ArrowDown' || event.key === 'ArrowRight'; + const backward = event.key === 'ArrowUp' || event.key === 'ArrowLeft'; + if (!forward && !backward) return; + event.preventDefault(); + const count = TOKEN_HOVER_TRIGGERS.length; + const next = (index + (forward ? 1 : -1) + count) % count; + configStore.set('tokenHoverTrigger', TOKEN_HOVER_TRIGGERS[next]); + optionRefs.current[next]?.focus(); + }; + + if (!isOpen) return null; + + return createPortal( +
+
+
+ + New + +

+ Hover cards +

+

+ Rest the pointer on a symbol in a diff and Plannotator shows you where it is + defined, an approximate signature, its doc comment, and a sample of its + references. Every location on the card opens in the References panel. +

+
+ +
+
+ +
+ +
+
+
+ Show cards +
+ {TOKEN_HOVER_TRIGGERS.map((value, index) => { + const selected = trigger === value; + return ( + + ); + })} +
+ +

+ {modKeyWord}+click on a symbol still opens the References panel, whichever option + you pick. Hover cards need ripgrep and a local checkout, and nothing appears when + the search comes back empty. +

+
+
+ +
+

+ Change this anytime in Settings, in the Editor tab. +

+ +
+
+
, + document.body, + ); +} diff --git a/packages/review-editor/components/TokenHoverCard.tsx b/packages/review-editor/components/TokenHoverCard.tsx index af38d6101a..4f44740c92 100644 --- a/packages/review-editor/components/TokenHoverCard.tsx +++ b/packages/review-editor/components/TokenHoverCard.tsx @@ -9,16 +9,43 @@ const EDGE_MARGIN = 12; const RIGHT_MARGIN = 30; const ANCHOR_GAP = 8; +/** + * The card's own box. Split from the positioning layer so a host can restack + * the card without restating what it looks like. + */ +const CARD_SURFACE_CLASS = + 'w-[400px] max-w-[calc(100vw-2rem)] rounded-xl border border-border bg-popover text-popover-foreground shadow-2xl'; + +export type TokenHoverLocation = { + filePath: string; + line: number; + column: number; +}; + export interface TokenHoverCardProps { hover: TokenHoverState; onPointerEnter: () => void; onPointerLeave: () => void; /** Opens the References panel for the hovered symbol, anchored at a location. */ - onSelectLocation: (location: { - filePath: string; - line: number; - column: number; - }) => void; + onSelectLocation: (location: TokenHoverLocation) => void; + /** + * Positioning layer. Defaults to the app's own `fixed z-[60]`. + * + * The announcement dialog's try-it needs the card ABOVE the modal it is + * demonstrated inside, and the card portals to like every other + * instance, so the stacking context is the one thing its host has to be able + * to say. + */ + layerClassName?: string; + /** + * The card is on display rather than in service: its locations lead nowhere, + * because the host has no References panel behind it (the announcement + * dialog's try-it). The locations leave the tab order, stop looking + * clickable, and the card leaves the accessibility tree — it portals to + * , outside the aria-modal dialog it belongs to, and its meaning is + * carried by that dialog's own labelled region instead. + */ + inert?: boolean; } function locationLabel(filePath: string, line: number): string { @@ -26,67 +53,40 @@ function locationLabel(filePath: string, line: number): string { } /** - * The Tier 0 hover card: what the search actually found, and nothing it did - * not. It never renders rank vocabulary, a backend name or a latency readout — - * uncertainty is expressed by SHOWING the alternative location, not by - * describing the algorithm. + * The card's contents, with no opinion about where the card sits. + * + * Split out from the positioning shell only. There is ONE card component and + * the announcement dialog renders it, fixture answer and all — nothing here is + * redrawn anywhere, because a forked copy of this markup would drift from the + * shipped card the first time either changed. + * + * `interactive: false` is the only difference a display-only host needs: the + * location buttons leave the tab order and stop advertising a click the host + * will swallow. Every class, every section, and their order are otherwise + * shared verbatim. */ -export function TokenHoverCard({ - hover, - onPointerEnter, - onPointerLeave, +function TokenHoverCardContent({ + data, onSelectLocation, -}: TokenHoverCardProps) { - const { data, rect } = hover; - const cardRef = useRef(null); - const [placement, setPlacement] = useState< - { for: TokenHoverState; left: number; top: number } | null - >(null); - - // Below the token, flipped above when the viewport bottom would clip it, - // clamped horizontally. Measured in a layout effect so the flip decision uses - // the real height and nothing is painted at the pre-measurement position. - useLayoutEffect(() => { - const height = cardRef.current?.offsetHeight ?? 0; - const left = Math.max( - EDGE_MARGIN, - Math.min(rect.left, window.innerWidth - CARD_WIDTH - RIGHT_MARGIN), - ); - let top = rect.bottom + ANCHOR_GAP; - if (top + height > window.innerHeight - EDGE_MARGIN) { - // Flipping a card taller than the space above the token would put its - // head off the top of the viewport, where the name and signature are - // unreachable. Clamp instead: the foot may overlap the token. - top = Math.max(EDGE_MARGIN, rect.top - height - ANCHOR_GAP); - } - setPlacement({ for: hover, left, top }); - }, [hover, rect]); - - const placed = placement?.for === hover ? placement : null; - + interactive = true, +}: { + data: TokenHoverState['data']; + onSelectLocation: (location: TokenHoverLocation) => void; + interactive?: boolean; +}) { const { definition, alternateDefinition, references, referenceCount, capped } = data; const shownCount = capped ? `${referenceCount}+` : `${referenceCount}`; const remaining = referenceCount - references.length; + const inertProps = interactive ? {} : { tabIndex: -1, 'aria-disabled': true as const }; + // Out of the tab order, and out of the affordance: the card's own footer + // says "Click a location to jump", so a location that still underlines on + // hover and shows a hand cursor is advertising something that does nothing. + // Appended to each button's own classes, never replacing them, so the + // display-only card stays the same card. + const locationClass = interactive ? '' : ' cursor-default hover:no-underline opacity-80'; - return createPortal( -
+ return ( + <>
{data.symbol} {definition?.symbolKind && ( @@ -118,7 +118,8 @@ export function TokenHoverCard({ Defined at
+ + ); +} + +/** + * The Tier 0 hover card: what the search actually found, and nothing it did + * not. It never renders rank vocabulary, a backend name or a latency readout — + * uncertainty is expressed by SHOWING the alternative location, not by + * describing the algorithm. + */ +export function TokenHoverCard({ + hover, + onPointerEnter, + onPointerLeave, + onSelectLocation, + layerClassName = 'fixed z-[60]', + inert = false, +}: TokenHoverCardProps) { + const { data, rect } = hover; + const cardRef = useRef(null); + const [placement, setPlacement] = useState< + { for: TokenHoverState; left: number; top: number } | null + >(null); + + // Below the token, flipped above when the viewport bottom would clip it, + // clamped horizontally. Measured in a layout effect so the flip decision uses + // the real height and nothing is painted at the pre-measurement position. + useLayoutEffect(() => { + const height = cardRef.current?.offsetHeight ?? 0; + const left = Math.max( + EDGE_MARGIN, + Math.min(rect.left, window.innerWidth - CARD_WIDTH - RIGHT_MARGIN), + ); + let top = rect.bottom + ANCHOR_GAP; + if (top + height > window.innerHeight - EDGE_MARGIN) { + // Flipping a card taller than the space above the token would put its + // head off the top of the viewport, where the name and signature are + // unreachable. Clamp instead: the foot may overlap the token. + top = Math.max(EDGE_MARGIN, rect.top - height - ANCHOR_GAP); + } + setPlacement({ for: hover, left, top }); + }, [hover, rect]); + + const placed = placement?.for === hover ? placement : null; + + return createPortal( +
, which puts it OUTSIDE the + // aria-modal dialog it is being shown inside — where a screen reader + // would reach it as a stray tooltip with no context. Its meaning is + // carried by that dialog's own labelled region instead. Safe to hide + // because `inert` has already taken every control out of the tab order. + aria-hidden={inert || undefined} + onPointerEnter={onPointerEnter} + onPointerLeave={onPointerLeave} + className={`${layerClassName} ${CARD_SURFACE_CLASS}`} + style={{ + left: placed?.left ?? rect.left, + top: placed?.top ?? rect.bottom + ANCHOR_GAP, + visibility: placed ? 'visible' : 'hidden', + }} + > +
, document.body, ); diff --git a/packages/review-editor/components/tokenHoverStyles.ts b/packages/review-editor/components/tokenHoverStyles.ts new file mode 100644 index 0000000000..7def06e7be --- /dev/null +++ b/packages/review-editor/components/tokenHoverStyles.ts @@ -0,0 +1,46 @@ +import type { CSSProperties } from 'react'; + +/** + * The hovered-token treatment, for the two places it has to exist. + * + * The diff pane renders inside Pierre's shadow root, so its copy has to be + * serialized into that stylesheet as `.pn-token-hover`; the announcement + * dialog's example lives in the ordinary document and applies a style object. + * Everything that decides how the treatment LOOKS is single-sourced in + * `UNDERLINE` below and read by both forms, so the example cannot drift away + * from the surface it is illustrating. + * + * Exactly two things are per-form, and neither is a look: + * - the decoration COLOR. Both mean the theme primary, but `var(--primary)` + * does not cross into Pierre's shadow root, so that form takes the already + * resolved value as an argument instead. + * - `!important` on the cursor, which the shadow form appends at its own call + * site. Pierre's `[data-*]` selectors outrank a bare class in there and the + * I-beam would otherwise win; in the ordinary document a style object + * already wins, and forcing it would be noise. + */ +const UNDERLINE = { + textDecorationLine: 'underline', + textDecorationThickness: '1.5px', + textUnderlineOffset: '2px', + cursor: 'pointer', +} as const; + +/** + * The shadow-DOM form. `primaryColor` is the resolved theme primary; see the + * note above on why it is passed rather than referenced. + */ +export function tokenHoverUnderlineCss(primaryColor: string): string { + return ` + text-decoration: ${UNDERLINE.textDecorationLine}; + text-decoration-color: ${primaryColor}; + text-decoration-thickness: ${UNDERLINE.textDecorationThickness}; + text-underline-offset: ${UNDERLINE.textUnderlineOffset}; + cursor: ${UNDERLINE.cursor} !important;`; +} + +/** The main-document form, for the announcement dialog's example. */ +export const TOKEN_HOVER_UNDERLINE_STYLE: CSSProperties = { + ...UNDERLINE, + textDecorationColor: 'var(--primary)', +}; diff --git a/packages/review-editor/hooks/usePierreTheme.ts b/packages/review-editor/hooks/usePierreTheme.ts index b0c1affc4d..a1e225c527 100644 --- a/packages/review-editor/hooks/usePierreTheme.ts +++ b/packages/review-editor/hooks/usePierreTheme.ts @@ -10,6 +10,7 @@ import { useConfigValue } from '@plannotator/ui/config'; * the import path the review editor has always used. */ import { resolveSyntaxTheme } from '@plannotator/ui/utils/syntaxTheme'; +import { tokenHoverUnderlineCss } from '../components/tokenHoverStyles'; export { SHIKI_THEME_MAP, resolveSyntaxTheme } from '@plannotator/ui/utils/syntaxTheme'; export interface PierreTheme { @@ -274,16 +275,14 @@ export function usePierreTheme(options?: { [data-diff-type='split'][data-overflow='scroll'] > [data-code][data-additions] [data-content] { min-width: 0 !important; } - .pn-token-hover { - text-decoration: underline; - text-decoration-color: ${primary || 'oklch(0.70 0.20 280)'}; - text-decoration-thickness: 1.5px; - text-underline-offset: 2px; - cursor: pointer; + .pn-token-hover {${tokenHoverUnderlineCss(primary || 'oklch(0.70 0.20 280)')} } .pn-token-nav { text-decoration-thickness: 2px; - cursor: pointer; + /* Pierre's [data-*] selectors outrank a bare class inside the + shadow root, so the pointer needs the same !important the + neighboring overrides use, or the I-beam wins. */ + cursor: pointer !important; opacity: 0.85; } diff --git a/packages/review-editor/hooks/useTokenHover.test.tsx b/packages/review-editor/hooks/useTokenHover.test.tsx index 882f71eeda..6bddb09189 100644 --- a/packages/review-editor/hooks/useTokenHover.test.tsx +++ b/packages/review-editor/hooks/useTokenHover.test.tsx @@ -15,7 +15,12 @@ import React from 'react'; import { act } from 'react'; import { createRoot, type Root } from 'react-dom/client'; import type { CodeNavHoverResponse, CodeNavRequest } from '@plannotator/shared/code-nav'; -import { useTokenHover, type UseTokenHoverResult } from './useTokenHover'; +import { isMac, modEventKey } from '@plannotator/ui/utils/platform'; +import { + useTokenHover, + type UseTokenHoverOptions, + type UseTokenHoverResult, +} from './useTokenHover'; const hasDom = typeof document !== 'undefined'; @@ -66,20 +71,88 @@ let host: HTMLDivElement | null = null; let root: Root | null = null; let latest: UseTokenHoverResult | null = null; -function Harness({ snapshotId = 'snapshot-1' }: { snapshotId?: string }) { - latest = useTokenHover(snapshotId); +function Harness({ + snapshotId = 'snapshot-1', + options, +}: { snapshotId?: string; options?: UseTokenHoverOptions }) { + latest = useTokenHover(snapshotId, options); return null; } -async function mount(snapshotId?: string): Promise { +async function mount(snapshotId?: string, options?: UseTokenHoverOptions): Promise { host = document.createElement('div'); document.body.appendChild(host); root = createRoot(host); await act(async () => { - root!.render(); + root!.render(); }); } +/** + * The gate's modifier going down/up on the window, the way the browser reports + * it. Platform-derived so the suite exercises the same key the product does on + * whatever machine it runs: Cmd on macOS, Ctrl on the Linux CI runner. + */ +const held = { metaKey: isMac, ctrlKey: !isMac }; +const released = { metaKey: false, ctrlKey: false }; + +async function modDown(): Promise { + await act(async () => { + window.dispatchEvent(new KeyboardEvent('keydown', { key: modEventKey, ...held })); + }); +} + +async function modUp(): Promise { + await act(async () => { + window.dispatchEvent(new KeyboardEvent('keyup', { key: modEventKey, ...released })); + }); +} + +/** A chord (copy) while the modifier is held: a command, not a hover intent. */ +async function modChord(key: string, target: EventTarget = window): Promise { + await act(async () => { + target.dispatchEvent(new KeyboardEvent('keydown', { key, ...held, bubbles: true })); + }); +} + +/** The modifier ALONE, pressed from inside some element rather than at the window. */ +async function modDownFrom(target: EventTarget): Promise { + await act(async () => { + target.dispatchEvent( + new KeyboardEvent('keydown', { key: modEventKey, ...held, bubbles: true, composed: true }), + ); + }); +} + +/** + * The modifier alone, as the WINDOW sees it when it was pressed inside a + * contenteditable in a shadow root (Pierre's edit session). + * + * The retargeting is modelled explicitly rather than produced by dispatching + * from inside a real shadow root, because happy-dom does not retarget: + * `event.target` there stays the inner node, so a dispatched event would let a + * `event.target` guard pass and the test would assert nothing. What the + * browser actually delivers is `target === host` with `composedPath()[0] === + * the editable`, and that is exactly what this builds. + */ +async function modDownFromShadowEditor(): Promise { + const host = document.createElement('div'); + document.body.appendChild(host); + const editable = document.createElement('div'); + editable.setAttribute('contenteditable', 'true'); + host.attachShadow({ mode: 'open' }).appendChild(editable); + + const event = new KeyboardEvent('keydown', { key: modEventKey, ...held }); + // Retargeted, the way a window-level listener receives it. + Object.defineProperty(event, 'target', { value: host }); + Object.defineProperty(event, 'composedPath', { + value: () => [editable, host, document.body, document.documentElement, document, window], + }); + + await act(async () => { window.dispatchEvent(event); }); + host.remove(); +} + function token(): HTMLElement { const el = document.createElement('span'); document.body.appendChild(el); @@ -134,7 +207,7 @@ describe.skipIf(!hasDom)('useTokenHover', () => { await mount(); await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); - await act(async () => { jest.advanceTimersByTime(349); }); + await act(async () => { jest.advanceTimersByTime(299); }); expect(pending).toHaveLength(0); await act(async () => { jest.advanceTimersByTime(2); }); @@ -155,7 +228,7 @@ describe.skipIf(!hasDom)('useTokenHover', () => { test('a new symbol supersedes and aborts the in-flight request', async () => { await mount(); await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); expect(pending).toHaveLength(1); await act(async () => { @@ -163,7 +236,7 @@ describe.skipIf(!hasDom)('useTokenHover', () => { }); expect(pending[0].signal?.aborted).toBe(true); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); expect(pending).toHaveLength(2); expect(pending[1].request.symbol).toBe('withRetry'); }); @@ -171,7 +244,7 @@ describe.skipIf(!hasDom)('useTokenHover', () => { test('the card survives the leave grace and entering it cancels the close', async () => { await mount(); await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); await settle(0, hoverResponse()); expect(latest!.hover).not.toBeNull(); @@ -193,13 +266,13 @@ describe.skipIf(!hasDom)('useTokenHover', () => { test('a re-hover inside the cache spawns no second request', async () => { await mount(); await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); await settle(0, hoverResponse()); await act(async () => latest!.onTokenHoverLeave()); await act(async () => { jest.advanceTimersByTime(250); }); await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); expect(pending).toHaveLength(1); expect(latest!.hover).not.toBeNull(); @@ -208,7 +281,7 @@ describe.skipIf(!hasDom)('useTokenHover', () => { test('an unavailable backend renders nothing', async () => { await mount(); await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); await settle(0, hoverResponse({ backend: 'unavailable', definition: null, @@ -222,7 +295,7 @@ describe.skipIf(!hasDom)('useTokenHover', () => { test('an answer with no definition and one reference renders nothing', async () => { await mount(); await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); await settle(0, hoverResponse({ definition: null, references: [{ filePath: 'src/other.js', line: 4, column: 2, snippet: 'charge(1)' }], @@ -240,14 +313,14 @@ describe.skipIf(!hasDom)('useTokenHover', () => { await mount(); const tokenX = token(); await act(async () => latest!.onTokenHoverEnter(REQUEST, tokenX)); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); await settle(0, hoverResponse()); expect(latest!.hover!.data.symbol).toBe('charge'); const neighbour = { ...REQUEST, symbol: 'withRetry' }; await act(async () => latest!.onTokenHoverLeave()); await act(async () => latest!.onTokenHoverEnter(neighbour, token())); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); expect(pending).toHaveLength(2); // Back onto the still-open card's token, then the neighbour answers. @@ -267,13 +340,13 @@ describe.skipIf(!hasDom)('useTokenHover', () => { await mount(); const tokenX = token(); await act(async () => latest!.onTokenHoverEnter(REQUEST, tokenX)); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); expect(pending).toHaveLength(1); await act(async () => latest!.onTokenHoverLeave()); await act(async () => { jest.advanceTimersByTime(100); }); await act(async () => latest!.onTokenHoverEnter(REQUEST, tokenX)); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); expect(pending).toHaveLength(1); expect(pending[0].signal?.aborted).toBe(false); @@ -286,7 +359,7 @@ describe.skipIf(!hasDom)('useTokenHover', () => { test('a below-threshold answer for another token closes the stale card', async () => { await mount(); await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); await settle(0, hoverResponse()); expect(latest!.hover).not.toBeNull(); @@ -294,7 +367,7 @@ describe.skipIf(!hasDom)('useTokenHover', () => { await act(async () => { latest!.onTokenHoverEnter({ ...REQUEST, symbol: 'gateway' }, token()); }); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); await settle(1, hoverResponse({ symbol: 'gateway', definition: null, @@ -313,7 +386,7 @@ describe.skipIf(!hasDom)('useTokenHover', () => { await mount(); const recycled = token(); await act(async () => latest!.onTokenHoverEnter(REQUEST, recycled)); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); recycled.remove(); await settle(0, hoverResponse()); @@ -323,7 +396,7 @@ describe.skipIf(!hasDom)('useTokenHover', () => { test('scrolling inside the card leaves it open; scrolling the pane closes it', async () => { await mount(); await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); await settle(0, hoverResponse()); // The signature block is a horizontal scroller: reading a long signature @@ -351,7 +424,7 @@ describe.skipIf(!hasDom)('useTokenHover', () => { // cached answer must never be served across a refresh. await mount('snapshot-1'); await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); await settle(0, hoverResponse()); expect(pending).toHaveLength(1); @@ -361,14 +434,14 @@ describe.skipIf(!hasDom)('useTokenHover', () => { expect(latest!.hover).toBeNull(); await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); expect(pending).toHaveLength(2); }); test('unmounting abandons the pending request', async () => { await mount(); await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); expect(pending).toHaveLength(1); const current = root!; @@ -381,7 +454,7 @@ describe.skipIf(!hasDom)('useTokenHover', () => { test('scrolling closes the card and abandons the pending request', async () => { await mount(); await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); - await act(async () => { jest.advanceTimersByTime(350); }); + await act(async () => { jest.advanceTimersByTime(300); }); expect(pending).toHaveLength(1); await act(async () => { @@ -391,3 +464,359 @@ describe.skipIf(!hasDom)('useTokenHover', () => { expect(latest!.hover).toBeNull(); }); }); + +/** + * The trigger mode's whole promise is "with the key up this costs nothing". + * The failure to catch is a gate that lands AFTER the dwell timer arms, which + * would silently restore one ripgrep per token on an idle sweep and make the + * mode a cosmetic filter over the same traffic. + */ +describe.skipIf(!hasDom)('useTokenHover trigger mode', () => { + test('with the key up, no dwell is ever armed', async () => { + await mount('snapshot-1', { mode: 'modifier' }); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + + await act(async () => { jest.advanceTimersByTime(10_000); }); + expect(pending).toHaveLength(0); + expect(latest!.hover).toBeNull(); + }); + + test('holding the key first, then hovering, opens the card', async () => { + await mount('snapshot-1', { mode: 'modifier' }); + await modDown(); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + + await act(async () => { jest.advanceTimersByTime(300); }); + expect(pending).toHaveLength(1); + await settle(0, hoverResponse()); + expect(latest!.hover?.request.symbol).toBe('charge'); + }); + + test('pressing the key while already parked on a token opens the card', async () => { + // The commonest gesture in this mode. A key press fires no pointer event, + // so a gate that only reads the enter event would make it do nothing. + await mount('snapshot-1', { mode: 'modifier' }); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + expect(pending).toHaveLength(0); + + await modDown(); + await act(async () => { jest.advanceTimersByTime(300); }); + + expect(pending).toHaveLength(1); + }); + + test('a token left before the key goes down is not resurrected by it', async () => { + await mount('snapshot-1', { mode: 'modifier' }); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + await act(async () => latest!.onTokenHoverLeave()); + + await modDown(); + await act(async () => { jest.advanceTimersByTime(300); }); + + expect(pending).toHaveLength(0); + }); + + test('releasing the key closes the card the way leaving does', async () => { + await mount('snapshot-1', { mode: 'modifier' }); + await modDown(); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + await act(async () => { jest.advanceTimersByTime(300); }); + await settle(0, hoverResponse()); + expect(latest!.hover).not.toBeNull(); + + await modUp(); + // The same leave grace, so the card's own reference links stay reachable. + expect(latest!.hover).not.toBeNull(); + await act(async () => { jest.advanceTimersByTime(250); }); + expect(latest!.hover).toBeNull(); + }); + + test('releasing the key while reading the card does not yank it away', async () => { + await mount('snapshot-1', { mode: 'modifier' }); + await modDown(); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + await act(async () => { jest.advanceTimersByTime(300); }); + await settle(0, hoverResponse()); + await act(async () => latest!.onCardEnter()); + + await modUp(); + await act(async () => { jest.advanceTimersByTime(1000); }); + + expect(latest!.hover).not.toBeNull(); + }); + + test('the modifier pressed while typing in a comment box arms nothing', async () => { + // Typing owns the key. The pointer is routinely parked over the diff while + // a comment is being written, so a bare Cmd there — the first half of + // Cmd+A, Cmd+V, or a Cmd the reviewer simply leaned on — must not open a + // card mid-sentence. This is the ONLY path isTypingTarget covers: a chord + // is caught by the disarm branch whether or not the guard exists, so a + // chord-shaped test would pass with the guard deleted. + await mount('snapshot-1', { mode: 'modifier' }); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + + const textarea = document.createElement('textarea'); + document.body.appendChild(textarea); + await modDownFrom(textarea); + await act(async () => { jest.advanceTimersByTime(1000); }); + textarea.remove(); + + expect(pending).toHaveLength(0); + expect(latest!.hover).toBeNull(); + }); + + test('the modifier pressed inside a shadow-root editor arms nothing either', async () => { + // A window-level listener sees event.target RETARGETED to the shadow host, + // so a guard reading event.target sees a plain
and lets the gate arm + // while the reviewer is typing in Pierre's edit-session editor. Reading + // composedPath()[0] is what the three sibling guards in this package do, + // and it is what this test pins. + await mount('snapshot-1', { mode: 'modifier' }); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + + await modDownFromShadowEditor(); + await act(async () => { jest.advanceTimersByTime(1000); }); + + expect(pending).toHaveLength(0); + expect(latest!.hover).toBeNull(); + }); + + test('a chord while typing still disarms an open card', async () => { + // The asymmetry that matters: typing may suppress ARMING, never + // DISARMING. Focus can reach a composer between the two (a card is open, + // the reviewer clicks into the comment box and pastes), and a typing + // guard placed ahead of the chord branch would leave that card standing + // over the diff with the gate still armed behind it. + await mount('snapshot-1', { mode: 'modifier' }); + await modDown(); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + await act(async () => { jest.advanceTimersByTime(300); }); + await settle(0, hoverResponse()); + expect(latest!.hover).not.toBeNull(); + + const textarea = document.createElement('textarea'); + document.body.appendChild(textarea); + await modChord('v', textarea); + await act(async () => { jest.advanceTimersByTime(250); }); + textarea.remove(); + + expect(latest!.hover).toBeNull(); + }); + + test('a copy chord with the pointer parked on a token opens nothing', async () => { + // The modifier that gates this feature is also the editing modifier, and + // the pointer is wherever it was left. Only the key ALONE is a request to + // be told about a symbol; Cmd+C is a command, and must not pop a card + // over the diff mid-copy. + await mount('snapshot-1', { mode: 'modifier' }); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + + await modDown(); + await modChord('c'); + await act(async () => { jest.advanceTimersByTime(5000); }); + + expect(pending).toHaveLength(0); + expect(latest!.hover).toBeNull(); + }); + + test('a chord while a card is open takes the card with it', async () => { + await mount('snapshot-1', { mode: 'modifier' }); + await modDown(); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + await act(async () => { jest.advanceTimersByTime(300); }); + await settle(0, hoverResponse()); + expect(latest!.hover).not.toBeNull(); + + await modChord('c'); + await act(async () => { jest.advanceTimersByTime(250); }); + + expect(latest!.hover).toBeNull(); + }); + + test('a card closed while the pointer was inside it does not deafen the next release', async () => { + // The reviewer's exact sequence. onCardEnter sets "pointer is in the card" + // and only onCardLeave used to clear it — but a scroll-close unmounts the + // card UNDER the pointer, so no leave ever arrives. The flag stayed true + // for the rest of the session and every later modifier release was + // ignored as "they are reading the card", leaving cards stuck open. + await mount('snapshot-1', { mode: 'modifier' }); + await modDown(); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + await act(async () => { jest.advanceTimersByTime(300); }); + await settle(0, hoverResponse()); + await act(async () => latest!.onCardEnter()); + + // Closed by a pane scroll, with the pointer still inside it. + await act(async () => { document.dispatchEvent(new Event('scroll')); }); + expect(latest!.hover).toBeNull(); + + // A fresh card on another token, then a release. + const second = { ...REQUEST, symbol: 'refund' }; + await act(async () => latest!.onTokenHoverEnter(second, token())); + await act(async () => { jest.advanceTimersByTime(300); }); + await settle(1, hoverResponse({ symbol: 'refund' })); + expect(latest!.hover).not.toBeNull(); + + await modUp(); + await act(async () => { jest.advanceTimersByTime(250); }); + + expect(latest!.hover).toBeNull(); + }); + + test('window blur clears the held state', async () => { + // Cmd+Tab (Alt+Tab elsewhere): the browser reports no keyup, so without + // the blur reset the key reads as held forever and the mode silently + // becomes plain hover. + await mount('snapshot-1', { mode: 'modifier' }); + await modDown(); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + await act(async () => { jest.advanceTimersByTime(300); }); + await settle(0, hoverResponse()); + expect(latest!.hover).not.toBeNull(); + + await act(async () => { window.dispatchEvent(new Event('blur')); }); + expect(latest!.hover).toBeNull(); + + // Held is clear, so hovering again arms nothing until the key goes down + // again: `pending` is still the ONE request from before the blur. + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + await act(async () => { jest.advanceTimersByTime(10_000); }); + expect(pending).toHaveLength(1); + }); + + test('the gate paints the navigable-target affordance on the parked token', async () => { + // The composite gesture: under one held key the card and the "this is a + // navigable target" underline arrive together and leave together. The diff + // views paint that class from the pointer ENTER event, which fires for + // neither transition here, so before this the mode's own primary gesture + // opened a card on a token wearing no affordance, and the release closed + // the card while leaving the affordance painted until the pointer left. + // + // The painter is the three lines App wires to this callback; what is under + // test is that the hook reports both transitions with the right element. + const paint = (armed: boolean, el: HTMLElement | null) => { + if (el) el.classList.toggle('pn-token-nav', armed); + }; + await mount('snapshot-1', { mode: 'modifier', onModifierGate: paint }); + + const parked = token(); + await act(async () => latest!.onTokenHoverEnter(REQUEST, parked)); + expect(parked.classList.contains('pn-token-nav')).toBe(false); + + await modDown(); + expect(parked.classList.contains('pn-token-nav')).toBe(true); + + await modUp(); + expect(parked.classList.contains('pn-token-nav')).toBe(false); + }); + + test('a chord and a blur take the affordance with them too', async () => { + // Every disarm route, not just the release: a card that is dismissed by + // Cmd+C or by Cmd+Tab must not leave the token underlined behind it. + const paint = (armed: boolean, el: HTMLElement | null) => { + if (el) el.classList.toggle('pn-token-nav', armed); + }; + await mount('snapshot-1', { mode: 'modifier', onModifierGate: paint }); + + const parked = token(); + await act(async () => latest!.onTokenHoverEnter(REQUEST, parked)); + + await modDown(); + await modChord('c'); + expect(parked.classList.contains('pn-token-nav')).toBe(false); + + await modDown(); + expect(parked.classList.contains('pn-token-nav')).toBe(true); + await act(async () => { window.dispatchEvent(new Event('blur')); }); + expect(parked.classList.contains('pn-token-nav')).toBe(false); + }); + + test('hover mode ignores the key entirely', async () => { + await mount('snapshot-1', { mode: 'hover' }); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + await act(async () => { jest.advanceTimersByTime(300); }); + await settle(0, hoverResponse()); + expect(latest!.hover).not.toBeNull(); + + await modUp(); + await act(async () => { jest.advanceTimersByTime(1000); }); + + // No key listeners exist in this mode, so a stray keyup cannot close it. + expect(latest!.hover).not.toBeNull(); + }); +}); + +/** + * Opening the References panel must never leave a hover card on screen. The + * App funnels EVERY route in (Cmd+click, Ctrl+click, the Alt+click alias, the + * card's own location links) through handleCodeNavRequest, which calls close() + * first. These two tests own the mechanism that handoff depends on; the + * "References handoff wiring" suite below pins that the App still calls it. + * + * Both cases are real: a modified click on a token the pointer has been + * resting on is the overlap #1461 shipped with, and in modifier mode it is the + * NORMAL way to click, because the key that opened the card is already held + * when the reviewer decides they want the whole panel. + */ +describe.skipIf(!hasDom)('useTokenHover References handoff', () => { + test('an open card is gone the moment References is invoked', async () => { + await mount('snapshot-1', { mode: 'modifier' }); + await modDown(); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + await act(async () => { jest.advanceTimersByTime(300); }); + await settle(0, hoverResponse()); + expect(latest!.hover).not.toBeNull(); + + // What handleCodeNavRequest does before resolving the symbol. + await act(async () => latest!.close()); + + expect(latest!.hover).toBeNull(); + }); + + test('a click during the dwell cancels the card instead of letting it land later', async () => { + // Without this the request completes behind the panel and opens a card + // over it, seconds after a click that meant "navigate, not explain". + await mount('snapshot-1', { mode: 'modifier' }); + await modDown(); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + await act(async () => { jest.advanceTimersByTime(200); }); + + await act(async () => latest!.close()); + + await act(async () => { jest.advanceTimersByTime(10_000); }); + expect(pending).toHaveLength(0); + expect(latest!.hover).toBeNull(); + }); + + test('a click after the request launched abandons the in-flight answer', async () => { + await mount('snapshot-1', { mode: 'modifier' }); + await modDown(); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + await act(async () => { jest.advanceTimersByTime(300); }); + expect(pending).toHaveLength(1); + + await act(async () => latest!.close()); + expect(pending[0].signal?.aborted).toBe(true); + + await act(async () => { jest.advanceTimersByTime(10_000); }); + expect(latest!.hover).toBeNull(); + }); +}); + +describe.skipIf(!hasDom)('useTokenHover delay', () => { + test('the configured delay is the dwell, not the shipped constant', async () => { + await mount('snapshot-1', { delayMs: 700 }); + await act(async () => latest!.onTokenHoverEnter(REQUEST, token())); + + // The two advances SUM to the configured dwell, so both halves have to + // hold: 350 is past DEFAULT_TOKEN_HOVER_DELAY_MS (300), so a hook that + // ignored `delayMs` would already have fired here; and 350+350 reaches + // exactly 700, so one that never fires is caught below. Changing either + // number in isolation silently guts the test — they are a pair. + await act(async () => { jest.advanceTimersByTime(350); }); + expect(pending).toHaveLength(0); + + await act(async () => { jest.advanceTimersByTime(350); }); + expect(pending).toHaveLength(1); + }); +}); diff --git a/packages/review-editor/hooks/useTokenHover.ts b/packages/review-editor/hooks/useTokenHover.ts index 75df1287d8..40e286ec57 100644 --- a/packages/review-editor/hooks/useTokenHover.ts +++ b/packages/review-editor/hooks/useTokenHover.ts @@ -3,15 +3,79 @@ import type { CodeNavHoverResponse, CodeNavRequest, } from '@plannotator/shared/code-nav'; +import { DEFAULT_TOKEN_HOVER_DELAY_MS } from '@plannotator/shared/token-hover'; +import { isModKeyHeld, modEventKey } from '@plannotator/ui/utils/platform'; export type { CodeNavHoverResponse }; -/** Dwell before any request exists. Sweeping a diff costs zero rg processes. */ -const DWELL_MS = 350; /** Grace on leave, so the card's own links are reachable. */ const LEAVE_GRACE_MS = 250; const MAX_CACHE_ENTRIES = 30; +/** + * How this hook opens cards. + * + * `off` never reaches here: the App withholds the handler props entirely, so + * the diff views wire no listeners at all. What is left is the shipped hover + * behavior and the modifier-held gate, which is Cmd on macOS and Ctrl + * elsewhere. + */ +export type TokenHoverMode = 'hover' | 'modifier'; + +/** + * How an answer is obtained for a request. The default posts to + * `/api/code-nav/hover`. + * + * The seam exists so the one-time announcement's try-it can drive the REAL + * dwell, supersession, leave-grace and modifier mechanics from a hardcoded + * fixture instead of reimplementing them: a second copy of this timing logic + * would drift, and a demo that hits the server would search the reviewer's + * repository for a symbol they never asked about. + */ +export type TokenHoverResolver = ( + request: CodeNavRequest, + signal: AbortSignal, +) => Promise; + +const fetchResolver: TokenHoverResolver = async (request, signal) => { + const res = await fetch('/api/code-nav/hover', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(request), + signal, + }); + if (!res.ok) return null; + return (await res.json()) as CodeNavHoverResponse; +}; + +export interface UseTokenHoverOptions { + /** Default `hover`, which is byte-for-byte the shipped behavior. */ + mode?: TokenHoverMode; + /** Dwell before any request exists. Sweeping a diff costs zero rg processes. */ + delayMs?: number; + /** Default: POST /api/code-nav/hover. */ + resolve?: TokenHoverResolver; + /** + * The gate armed or disarmed, with the token the pointer is parked on (null + * when it is not on one). + * + * The card is only half of the held-modifier gesture: the other half is the + * `pn-token-nav` affordance the diff views paint on the token. The views + * paint it from the pointer ENTER event, which is enough for "hold the key, + * then move onto a symbol" but fires for neither half of the gesture this + * mode exists for — a key going down over a parked pointer, and the release + * after it, produce no pointer event at all. Without this the canonical + * gesture opened a card on a token wearing no affordance, and a release + * closed the card while leaving the affordance painted until the pointer + * eventually left. + * + * A callback rather than the hook touching the class itself: 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. + */ + onModifierGate?: (armed: boolean, tokenElement: HTMLElement | null) => void; +} + /** * Marks the card's own subtree. The card carries this attribute and the scroll * cancel reads it, so the two stay in step through one name. @@ -39,6 +103,37 @@ export interface UseTokenHoverResult { close: () => void; } +/** + * Where a key event actually came from. + * + * composedPath()[0] pierces shadow DOM: window-level e.target retargets to the + * shadow HOST (e.g. Pierre's ), which would hide a typeable + * element living inside a shadow root from the guard below — and the review + * pane's edit-session contenteditable is exactly that. The three other + * window-level typing guards in this package (AllFilesCodeView, FileTree, + * SectionsPanel) all read the origin this way; this one is not an exception. + */ +function eventOrigin(event: KeyboardEvent): EventTarget | null { + return event.composedPath?.()[0] ?? event.target; +} + +/** + * The primary modifier is also the editing modifier (Cmd+C, Cmd+V, Cmd+A), so + * arming on it while the reviewer is writing a comment would pop cards over + * the diff mid-sentence whenever the pointer happens to be parked there. + * Typing owns the key; only the pointer's own gesture arms. + */ +function isTypingTarget(target: EventTarget | null): boolean { + if (!(target instanceof HTMLElement)) return false; + const tag = target.tagName; + return ( + tag === 'INPUT' || + tag === 'TEXTAREA' || + tag === 'SELECT' || + target.isContentEditable + ); +} + function cacheKey(request: CodeNavRequest): string { return `${request.symbol}|${request.filePath}|${request.side}`; } @@ -62,13 +157,44 @@ function meetsRenderThreshold(data: CodeNavHoverResponse): boolean { * * @param snapshotId the active diff snapshot; a change flushes the cache so a * refreshed diff can never serve positions from the diff before it. + * @param options the user's trigger mode and dwell. Omitted means the shipped + * hover-at-default-delay behavior. */ -export function useTokenHover(snapshotId?: string): UseTokenHoverResult { +export function useTokenHover( + snapshotId?: string, + options: UseTokenHoverOptions = {}, +): UseTokenHoverResult { + const { + mode = 'hover', + delayMs = DEFAULT_TOKEN_HOVER_DELAY_MS, + resolve = fetchResolver, + onModifierGate, + } = options; + // Read through a ref so an inline resolver literal cannot re-arm every + // callback in this hook on each render of its host. + const resolveRef = useRef(resolve); + resolveRef.current = resolve; + // Same reason, and additionally so the key-listener effect below does not + // re-register on every render of its host. + const modifierGateRef = useRef(onModifierGate); + modifierGateRef.current = onModifierGate; const [hover, setHover] = useState(null); // Drives the scroll listener: a document-level listener exists only while // something is actually pending or open. const [engaged, setEngaged] = useState(false); + /** The gate's modifier held right now. Tracked only while the mode needs it. */ + const modifierHeldRef = useRef(false); + /** + * The last token the pointer entered, kept even when the gate rejected it. + * A key going down fires no pointer event, so without this the commonest + * gesture in modifier mode — park on a symbol, THEN hold the modifier — would do + * nothing at all. + */ + const lastEnterRef = useRef<{ request: CodeNavRequest; element: HTMLElement } | null>(null); + /** Pointer is inside the card, so a key release must not close it. */ + const pointerInCardRef = useRef(false); + const dwellTimerRef = useRef | null>(null); const graceTimerRef = useRef | null>(null); const abortRef = useRef(null); @@ -101,6 +227,13 @@ export function useTokenHover(snapshotId?: string): UseTokenHoverResult { abortInFlight(); activeKeyRef.current = null; openKeyRef.current = null; + lastEnterRef.current = null; + // The card is gone, so the pointer is not in it any more — and no pointer + // event will say so, because the card unmounted underneath the pointer + // rather than being left. Without this reset the flag stays true for the + // rest of the session and every later modifier release is ignored as + // "they are reading the card". + pointerInCardRef.current = false; setHover(null); setEngaged(false); }, [abortInFlight, clearDwell, clearGrace]); @@ -152,7 +285,7 @@ export function useTokenHover(snapshotId?: string): UseTokenHoverResult { [close], ); - const onTokenHoverEnter = useCallback( + const beginHover = useCallback( (request: CodeNavRequest, tokenElement: HTMLElement) => { clearGrace(); const key = cacheKey(request); @@ -205,14 +338,8 @@ export function useTokenHover(snapshotId?: string): UseTokenHoverResult { inFlightKeyRef.current = key; void (async () => { try { - const res = await fetch('/api/code-nav/hover', { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(request), - signal: controller.signal, - }); - if (!res.ok) return; - const data = (await res.json()) as CodeNavHoverResponse; + const data = await resolveRef.current(request, controller.signal); + if (data == null) return; if (controller.signal.aborted || activeKeyRef.current !== key) return; writeCache(key, data); open(key, request, data, activeElementRef.current ?? tokenElement); @@ -225,9 +352,22 @@ export function useTokenHover(snapshotId?: string): UseTokenHoverResult { } } })(); - }, DWELL_MS); + }, delayMs); + }, + [abortInFlight, clearDwell, clearGrace, delayMs, open, readCache, writeCache], + ); + + const onTokenHoverEnter = useCallback( + (request: CodeNavRequest, tokenElement: HTMLElement) => { + // Recorded before the gate, so a later key press has a token to act on. + lastEnterRef.current = { request, element: tokenElement }; + // The gate sits HERE, ahead of the dwell timer, the cache read and every + // piece of state: in modifier mode with the key up, a hover costs one + // boolean read and nothing else. + if (mode === 'modifier' && !modifierHeldRef.current) return; + beginHover(request, tokenElement); }, - [abortInFlight, clearDwell, clearGrace, open, readCache, writeCache], + [beginHover, mode], ); const startGrace = useCallback(() => { @@ -241,12 +381,109 @@ export function useTokenHover(snapshotId?: string): UseTokenHoverResult { const onTokenHoverLeave = useCallback(() => { // The dwell dies with the pointer; the in-flight fetch does not, so a // re-entry inside the grace window still lands its own answer. + lastEnterRef.current = null; clearDwell(); startGrace(); }, [clearDwell, startGrace]); - const onCardEnter = useCallback(() => clearGrace(), [clearGrace]); - const onCardLeave = useCallback(() => startGrace(), [startGrace]); + const onCardEnter = useCallback(() => { + pointerInCardRef.current = true; + clearGrace(); + }, [clearGrace]); + const onCardLeave = useCallback(() => { + pointerInCardRef.current = false; + startGrace(); + }, [startGrace]); + + // Modifier mode only. Three listeners, and only while the mode needs them: + // in hover mode this effect never runs, so the pipeline is exactly what + // #1461 shipped. + useEffect(() => { + if (mode !== 'modifier') { + modifierHeldRef.current = false; + return; + } + // Always reports the token the pointer is on RIGHT NOW, so a disarm + // unpaints the token the pointer drifted onto while the key was held, not + // only the one the arm painted. + const notifyGate = (armed: boolean) => { + modifierGateRef.current?.(armed, lastEnterRef.current?.element ?? null); + }; + const onKeyDown = (event: KeyboardEvent) => { + if (!isModKeyHeld(event)) return; + // Only the modifier ITSELF arms. Any other key while it is held is a + // command (Cmd+C, Cmd+V, Cmd+S), not a request to be told about the + // symbol the pointer happens to be resting on, so a chord disarms and + // takes any open card with it rather than popping one mid-copy. + // + // Ahead of the typing guard on purpose: DISARMING is never something + // focus should be able to suppress. Focus can land in a comment box + // between the arm and the chord (a card is open, the reviewer clicks + // into the composer and hits Cmd+V), and a guard that returned first + // would leave that card standing over the diff with the gate still + // armed behind it. + if (event.key !== modEventKey) { + if (!modifierHeldRef.current) return; + modifierHeldRef.current = false; + clearDwell(); + notifyGate(false); + if (!pointerInCardRef.current) startGrace(); + return; + } + // Typing owns the key, so only the ARM path is suppressed. Read through + // composedPath: the review pane's editor is a contenteditable inside a + // shadow root, and a bare event.target reads as its host element. + if (isTypingTarget(eventOrigin(event))) return; + if (modifierHeldRef.current) return; + modifierHeldRef.current = true; + // Pressing the key does not re-enter the token under the pointer, so + // this is what makes "park on a symbol, then hold Cmd" work — for the + // affordance as well as the card. + const pending = lastEnterRef.current; + notifyGate(true); + if (pending) beginHover(pending.request, pending.element); + }; + // No typing guard here, deliberately: a release always disarms. If the + // gate never armed the branch below is already a no-op, and if it DID arm + // (focus was elsewhere) the release has to be honored wherever focus has + // since travelled, or the card outlives the key that opened it. + const onKeyUp = (event: KeyboardEvent) => { + if (isModKeyHeld(event) || !modifierHeldRef.current) return; + modifierHeldRef.current = false; + // Ahead of the reading-the-card early return below: the key IS up, so + // the "this is a navigable target" affordance is over either way. Only + // the card's dismissal is deferred, never the affordance's. + notifyGate(false); + // Releasing behaves like leaving: the same grace, so the card's own + // reference links stay reachable. Unless the pointer is already inside + // the card, in which case a release would yank it out from under + // someone reading it. + if (pointerInCardRef.current) return; + clearDwell(); + startGrace(); + }; + // Cmd+Tab (Alt+Tab elsewhere) leaves the key state stale-held forever + // otherwise, and on macOS it is now the COMMON way to leave: the app + // switcher is the same key this gate arms on. + const onBlur = () => { + if (modifierHeldRef.current) notifyGate(false); + modifierHeldRef.current = false; + close(); + }; + window.addEventListener('keydown', onKeyDown); + window.addEventListener('keyup', onKeyUp); + window.addEventListener('blur', onBlur); + return () => { + window.removeEventListener('keydown', onKeyDown); + window.removeEventListener('keyup', onKeyUp); + window.removeEventListener('blur', onBlur); + // Leaving modifier mode (the reviewer picked another trigger, or the app + // unmounted) with the key still down would strand the affordance on the + // token, with no listener left to take it off. + if (modifierHeldRef.current) notifyGate(false); + modifierHeldRef.current = false; + }; + }, [beginHover, clearDwell, close, mode, startGrace]); // The anchor rect is stale the moment the PANE moves, so scrolling closes the // card and abandons the pending fetch. Capture phase, because the scroll diff --git a/packages/review-editor/utils/codeNavHoverHandoff.test.ts b/packages/review-editor/utils/codeNavHoverHandoff.test.ts new file mode 100644 index 0000000000..6727c44eeb --- /dev/null +++ b/packages/review-editor/utils/codeNavHoverHandoff.test.ts @@ -0,0 +1,104 @@ +/** + * References-panel / hover-card exclusivity, pinned at source level (the + * `iframeIsolation` and `live-proxy` suites' precedent, because the coupling + * lives inside a 5000-line component whose only entry point is a full App + * mount with a stubbed diff, worker pool, and SSE transport). + * + * The invariant: `handleCodeNavRequest` is the ONE funnel every route into the + * References panel passes through — Cmd+click and Ctrl+click, the Alt+click + * alias, and the card's own location links — so closing the hover surface + * there covers all of them at once. The regression this catches is the line + * being dropped in a refactor, which reads as harmless and silently restores + * the #1461 double-surface: a card standing over the panel, or a dwell that + * was already in flight resolving into one seconds after the click. + * + * The behavior close() itself provides (open card gone, pending dwell + * cancelled, in-flight request aborted) is covered behaviorally by the + * "References handoff" suite in useTokenHover.test.tsx. + */ +import { describe, expect, test } from 'bun:test'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; + +const APP = join(import.meta.dir, '..', 'App.tsx'); + +describe('References panel closes the hover surface', () => { + test('handleCodeNavRequest closes the token hover before anything else', () => { + const source = readFileSync(APP, 'utf8'); + + const start = source.indexOf('const handleCodeNavRequest = useCallback('); + expect(start).toBeGreaterThan(0); + const body = source.slice(start, source.indexOf('\n }, [', start)); + + // First statement, ahead of the no-checkout early return: a click that + // cannot resolve must still dismiss the card it was made on top of. + expect(body).toContain('closeTokenHover();'); + expect(body.indexOf('closeTokenHover();')).toBeLessThan(body.indexOf('if (!gitContext')); + expect(body.indexOf('closeTokenHover();')).toBeLessThan(body.indexOf('codeNav.resolve(')); + }); + + test('every diff-view route into References goes through that one handler', () => { + const source = readFileSync(APP, 'utf8'); + // If a second producer ever calls codeNav.resolve directly it bypasses the + // dismissal, so the funnel is part of the invariant. + const direct = source.match(/codeNav\.resolve\(/g) ?? []; + expect(direct).toHaveLength(1); + expect(source).toContain('onCodeNavRequest: canUseLiveWorkspaceActions ? handleCodeNavRequest : undefined'); + }); + + test('the Alt+click alias shares the meta/ctrl branch in both diff views', () => { + // Alt+click is the quiet alias INTO References; the trigger's own key is + // Cmd/Ctrl, so the two are separate gestures onto one destination. Both + // still have to dismiss an open card, which is what the funnel above + // guarantees for either of them. + for (const view of ['components/DiffViewer.tsx', 'components/AllFilesCodeView.tsx']) { + const source = readFileSync(join(import.meta.dir, '..', view), 'utf8'); + expect(source).toMatch(/event\.metaKey \|\| event\.ctrlKey \|\| event\.altKey/); + } + }); +}); + +/** + * The other half of the held-modifier gesture, pinned the same way and for the + * same reason: the coupling lives in App.tsx, whose only entry point is a full + * mount. The behavior itself (both transitions reported, with the token the + * pointer is parked on) is covered in useTokenHover.test.tsx; what can rot + * here is the App forgetting to wire the callback, or wiring it to something + * that does not paint, which reads as harmless and silently returns the mode + * to opening cards on tokens wearing no affordance. + */ +describe('the modifier gate paints the navigable-target affordance', () => { + test('App passes onModifierGate and the handler toggles pn-token-nav', () => { + const source = readFileSync(APP, 'utf8'); + + expect(source).toContain('onModifierGate: handleModifierGate'); + + const start = source.indexOf('const handleModifierGate = useCallback('); + expect(start).toBeGreaterThan(0); + const body = source.slice(start, source.indexOf('\n }, [', start)); + expect(body).toContain("classList.add('pn-token-nav')"); + expect(body).toContain("classList.remove('pn-token-nav')"); + }); + + test('both diff views gate the hovered-token class on a hover handler', () => { + // pn-token-hover carries a `cursor: pointer !important`, so painting it + // unconditionally promises clickability on every token in a session where + // hover cards are switched off — and paints it in the portable viewer, + // which wires no hover handler at all. + for (const view of ['components/DiffViewer.tsx', 'components/AllFilesCodeView.tsx']) { + const source = readFileSync(join(import.meta.dir, '..', view), 'utf8'); + const start = source.indexOf('const handleTokenEnter ='); + expect(start).toBeGreaterThan(0); + const body = source.slice(start, source.indexOf('const handleTokenLeave =', start)); + + const paint = body.indexOf("classList.add('pn-token-hover')"); + const guard = body.indexOf('onTokenHoverEnter'); + expect(paint).toBeGreaterThan(0); + // The handler reads the hover prop BEFORE painting: either arm is a + // guard (`if (onTokenHoverEnter)` / an early return on its absence), and + // both leave the class unreachable without one. + expect(guard).toBeGreaterThan(-1); + expect(guard).toBeLessThan(paint); + } + }); +}); diff --git a/packages/review-editor/utils/tokenHoverAnnouncement.test.ts b/packages/review-editor/utils/tokenHoverAnnouncement.test.ts new file mode 100644 index 0000000000..e234855855 --- /dev/null +++ b/packages/review-editor/utils/tokenHoverAnnouncement.test.ts @@ -0,0 +1,135 @@ +import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; +import { resetStorageBackend, setStorageBackend } from '@plannotator/ui/utils/storage'; +import { configStore } from '@plannotator/ui/config'; +import { + markTokenHoverAnnouncementSeen, + needsTokenHoverAnnouncement, + resolveTokenHoverAnnouncementPending, + shouldConsumeTokenHoverAnnouncement, + tokenHoverAnnouncementCanShow, + type TokenHoverAnnouncementGateState, +} from './tokenHoverAnnouncement'; + +const SEEN_KEY = 'plannotator-token-hover-announcement-seen'; +const TRIGGER_KEY = 'plannotator-token-hover-trigger'; +const LEGACY_KEY = 'plannotator-token-hover-cards'; +let stored: Map; + +function installBackend(seed: Record = {}): void { + // configStore is a process-global singleton whose loadFromBackend keeps its + // in-memory value when the new backend is silent, so an unseeded map would + // inherit another test file's last write. Seed the trigger to make that + // deterministic — but ONLY when the case says nothing about either trigger + // key, so the migration case still exercises a genuinely absent new key. + // Resolving 'hover' from a cookie and from the registry default are the same + // thing to every caller here; the absent-cookie resolution itself is covered + // in tokenHoverSetting.test.ts. + const speaksForTrigger = TRIGGER_KEY in seed || LEGACY_KEY in seed; + stored = new Map(Object.entries(speaksForTrigger ? seed : { [TRIGGER_KEY]: 'hover', ...seed })); + setStorageBackend({ + getItem: key => stored.get(key) ?? null, + setItem: (key, value) => { stored.set(key, value); }, + removeItem: key => { stored.delete(key); }, + }); + // The store is a process-global singleton that may already be resolved from + // another suite's backend; re-hydrate so the trigger it reports is this + // test's, not a leftover. + configStore.loadFromBackend(); +} + +describe('token hover announcement persistence', () => { + beforeEach(() => { installBackend(); }); + afterEach(() => { + resetStorageBackend(); + // configStore is a process-global singleton and every installBackend() + // above re-resolved EVERY setting from a two-entry fake map. Bun runs the + // whole pure lane in one process, so leaving it resolved against a backend + // that no longer exists hands the next test file this file's defaults. + // Re-hydrate from the real backend now that it is restored. + configStore.loadFromBackend(); + }); + + test('is needed until the current announcement is marked seen', () => { + expect(needsTokenHoverAnnouncement()).toBe(true); + + markTokenHoverAnnouncementSeen(); + + expect(needsTokenHoverAnnouncement()).toBe(false); + }); + + test('shows again when a stored announcement version is stale', () => { + stored.set(SEEN_KEY, '0'); + expect(needsTokenHoverAnnouncement()).toBe(true); + }); + + test('pends for a reviewer who has never chosen a trigger', () => { + expect(resolveTokenHoverAnnouncementPending()).toBe(true); + // Latching must not consume the cookie: the dialog has not been shown yet. + expect(needsTokenHoverAnnouncement()).toBe(true); + }); + + test('never shows to someone who already chose a trigger', () => { + installBackend({ [TRIGGER_KEY]: 'modifier' }); + + expect(resolveTokenHoverAnnouncementPending()).toBe(false); + // Retiring the announcement is a cookie WRITE, so the pure latch must not + // do it. A React state initializer can run more than once, and a getter + // that writes storage is the kind of side effect that goes unnoticed + // until StrictMode double-invokes it. + expect(needsTokenHoverAnnouncement()).toBe(true); + + // The App's effect is what consumes it. + expect(shouldConsumeTokenHoverAnnouncement()).toBe(true); + markTokenHoverAnnouncementSeen(); + expect(shouldConsumeTokenHoverAnnouncement()).toBe(false); + }); + + test('never shows to the early adopter who turned cards off with the old boolean', () => { + // The migration resolves that legacy false to `off`, which reads as a + // choice already made — announcing a feature they declined is the worst + // version of this dialog. + installBackend({ [LEGACY_KEY]: 'false' }); + + expect(resolveTokenHoverAnnouncementPending()).toBe(false); + expect(shouldConsumeTokenHoverAnnouncement()).toBe(true); + }); + + test('a reviewer who has not chosen is never retired behind their back', () => { + expect(shouldConsumeTokenHoverAnnouncement()).toBe(false); + }); +}); + +describe('tokenHoverAnnouncementCanShow (never-stack chain gate)', () => { + const openState: TokenHoverAnnouncementGateState = { + announcementPending: true, + isLoading: false, + featureAvailable: true, + guideIntroVisible: false, + lookAndFeelVisible: false, + reviewSetupVisible: false, + editModeVisible: false, + }; + + test('shows when pending and no other chain dialog is open', () => { + expect(tokenHoverAnnouncementCanShow(openState)).toBe(true); + }); + + test('never shows once the announcement is no longer pending', () => { + expect(tokenHoverAnnouncementCanShow({ ...openState, announcementPending: false })).toBe(false); + }); + + test('never renders while the initial diff is still loading', () => { + expect(tokenHoverAnnouncementCanShow({ ...openState, isLoading: true })).toBe(false); + }); + + test('never renders in a session where hover cards cannot run at all', () => { + expect(tokenHoverAnnouncementCanShow({ ...openState, featureAvailable: false })).toBe(false); + }); + + test('never renders while any earlier chain dialog is open', () => { + expect(tokenHoverAnnouncementCanShow({ ...openState, guideIntroVisible: true })).toBe(false); + expect(tokenHoverAnnouncementCanShow({ ...openState, lookAndFeelVisible: true })).toBe(false); + expect(tokenHoverAnnouncementCanShow({ ...openState, reviewSetupVisible: true })).toBe(false); + expect(tokenHoverAnnouncementCanShow({ ...openState, editModeVisible: true })).toBe(false); + }); +}); diff --git a/packages/review-editor/utils/tokenHoverAnnouncement.ts b/packages/review-editor/utils/tokenHoverAnnouncement.ts new file mode 100644 index 0000000000..ffa95f0938 --- /dev/null +++ b/packages/review-editor/utils/tokenHoverAnnouncement.ts @@ -0,0 +1,90 @@ +import { storage } from '@plannotator/ui/utils/storage'; +import { configStore } from '@plannotator/ui/config'; + +/** + * One-time gate for the token hover card announcement dialog. Cookie-backed + * like the other announcement gates, so the dismissal survives Plannotator's + * random localhost ports. + */ +const STORAGE_KEY = 'plannotator-token-hover-announcement-seen'; +// Bump to re-show the announcement after a meaningful revision. +const CURRENT_VERSION = '1'; + +export function needsTokenHoverAnnouncement(): boolean { + return storage.getItem(STORAGE_KEY) !== CURRENT_VERSION; +} + +export function markTokenHoverAnnouncementSeen(): void { + storage.setItem(STORAGE_KEY, CURRENT_VERSION); +} + +/** + * Latch the pending flag at mount. PURE, like its `needsEditModeAnnouncement() + * && !configStore.get('editSuggestions')` sibling: a React state initializer + * can run more than once (StrictMode, a re-render before the store commits), + * so the cookie write that goes with this decision lives in an effect + * (`shouldConsumeTokenHoverAnnouncement` below), never here. + * + * A non-default trigger means the user has already answered the only question + * this dialog asks. After the boolean-to-trigger migration that is exactly the + * early adopter who turned cards off with the old switch, and telling someone + * about a feature they already declined is the worst version of this dialog. + */ +export function resolveTokenHoverAnnouncementPending(): boolean { + return needsTokenHoverAnnouncement() && configStore.get('tokenHoverTrigger') === 'hover'; +} + +/** + * True when the announcement should be retired without ever being shown: the + * reviewer already expressed the preference it exists to collect, so it has no + * reason to come back. The App runs this once in an effect and marks it seen. + * + * Distinct from a session that merely CANNOT run hover cards (a GitButler + * stack view), which skips the dialog while deliberately keeping the cookie. + */ +export function shouldConsumeTokenHoverAnnouncement(): boolean { + return needsTokenHoverAnnouncement() && configStore.get('tokenHoverTrigger') !== 'hover'; +} + +export interface TokenHoverAnnouncementGateState { + /** Latched at mount by resolveTokenHoverAnnouncementPending. */ + announcementPending: boolean; + /** The app is still fetching its initial diff. */ + isLoading: boolean; + /** + * Hover cards are possible in this session at all. A GitButler stack or + * branch view has no live workspace, so there is nothing to announce; the + * cookie is deliberately NOT consumed for that (see the dialog wiring), so + * the next ordinary review still shows it. + */ + featureAvailable: boolean; + /** Guided-review intro dialog is visible (first in the chain). */ + guideIntroVisible: boolean; + /** Look-and-feel announcement is pending (second in the chain). */ + lookAndFeelVisible: boolean; + /** Review setup chooser is open (third in the chain). */ + reviewSetupVisible: boolean; + /** Edit Mode announcement is visible (fourth in the chain). */ + editModeVisible: boolean; +} + +/** + * Chain gate for the announcement dialog. It is LAST in the first-run dialog + * chain (guide intro, look-and-feel, review setup, edit mode, then this) and + * must never stack with any of them. Waiting for isLoading to clear matters: + * showReviewSetup only latches during the initial diff load, so rendering + * earlier could flash this dialog under a chain that is about to open. + */ +export function tokenHoverAnnouncementCanShow( + state: TokenHoverAnnouncementGateState, +): boolean { + return ( + state.announcementPending && + state.featureAvailable && + !state.isLoading && + !state.guideIntroVisible && + !state.lookAndFeelVisible && + !state.reviewSetupVisible && + !state.editModeVisible + ); +} diff --git a/packages/shared/package.json b/packages/shared/package.json index be7d0c1c71..f9d05f5b2b 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -61,6 +61,7 @@ "./annotate-target": "./annotate-target.ts", "./at-reference": "./at-reference.ts", "./code-nav": "./code-nav.ts", + "./token-hover": "./token-hover.ts", "./goal-setup": "./goal-setup.ts", "./semantic-diff": "./semantic-diff.ts", "./semantic-diff-types": "./semantic-diff-types.ts", diff --git a/packages/shared/token-hover.ts b/packages/shared/token-hover.ts new file mode 100644 index 0000000000..b8a19642ad --- /dev/null +++ b/packages/shared/token-hover.ts @@ -0,0 +1 @@ +export * from '@plannotator/core/token-hover'; diff --git a/packages/ui/components/Settings.tsx b/packages/ui/components/Settings.tsx index 4af5e91948..f58a305bfd 100644 --- a/packages/ui/components/Settings.tsx +++ b/packages/ui/components/Settings.tsx @@ -3,6 +3,7 @@ import { createPortal } from 'react-dom'; import type { AnnotateAgentTerminalSide } from '@plannotator/core/agent-terminal'; import type { Origin } from '@plannotator/core/agents'; import type { DiffLineBgIntensity } from '@plannotator/core/config-types'; +import type { TokenHoverDelay } from '@plannotator/core/token-hover'; import { configStore, useConfigValue, setReviewPanelView, setReviewDefaultDiffType, setReviewAutoViewed } from '../config'; import { setWebMcpToolsEnabled, useWebMcpToolsEnabled } from '../webmcp/preference'; import { loadDiffFont } from '../utils/diffFonts'; @@ -64,7 +65,7 @@ import { useAgents } from '../hooks/useAgents'; import { KeyboardShortcuts } from './KeyboardShortcuts'; import { type QuickLabel, getQuickLabels, saveQuickLabels, resetQuickLabels, DEFAULT_QUICK_LABELS, getLabelColors, LABEL_COLOR_MAP } from '../utils/quickLabels'; import { ThemeTab } from './ThemeTab'; -import { isMac, modKey, altKey } from '../utils/platform'; +import { isMac, modKey, modKeyWord, altKey } from '../utils/platform'; import { getAIProviderSettings, resolveAIProviderSelection } from '../utils/aiProvider'; import { AISettingsTab } from './AISettingsTab'; import { HooksTab } from './settings/HooksTab'; @@ -133,6 +134,23 @@ export const DIFF_STYLE_OPTIONS = [ { value: 'split' as const, label: 'Split' }, { value: 'unified' as const, label: 'Unified' }, ]; +/** + * The modifier gate is the platform's primary modifier: Cmd on macOS, Ctrl on + * Windows and Linux. Labeled through modKeyWord so the control names the key + * the reader actually has. + */ +export const TOKEN_HOVER_TRIGGER_OPTIONS = [ + { value: 'hover' as const, label: 'On hover' }, + { value: 'modifier' as const, label: `Hold ${modKeyWord}` }, + { value: 'off' as const, label: 'Off' }, +]; +/** SegmentedControl keys on strings, so the ms values ride as their digits. */ +export type TokenHoverDelayOption = '150' | '300' | '700'; +export const TOKEN_HOVER_DELAY_OPTIONS = [ + { value: '150' as const, label: 'Fast' }, + { value: '300' as const, label: 'Default' }, + { value: '700' as const, label: 'Relaxed' }, +]; export const OVERFLOW_OPTIONS = [ { value: 'scroll' as const, label: 'Scroll' }, { value: 'wrap' as const, label: 'Wrap' }, @@ -171,22 +189,27 @@ const AGENT_TERMINAL_SIDE_OPTIONS: { value: AnnotateAgentTerminalSide; label: st { value: 'hidden', label: 'Hidden' }, ]; -function SegmentedControl({ options, value, onChange }: { +function SegmentedControl({ options, value, onChange, disabled = false }: { options: { value: T; label: string }[]; value: T; onChange: (v: T) => void; + /** Visible but inert, for a control whose axis does not apply right now. */ + disabled?: boolean; }) { return ( -
+
{options.map((opt) => ( @@ -471,7 +494,8 @@ const ReviewDisplayTab: React.FC<{ isCompactTouchLayout?: boolean }> = ({ isComp const diffExpandUnchanged = useConfigValue('diffExpandUnchanged'); const diffFontFamily = useConfigValue('diffFontFamily'); const diffFontSize = useConfigValue('diffFontSize'); - const tokenHoverCards = useConfigValue('tokenHoverCards'); + const tokenHoverTrigger = useConfigValue('tokenHoverTrigger'); + const tokenHoverDelay = useConfigValue('tokenHoverDelay'); // Load font for the preview swatch useEffect(() => { @@ -495,14 +519,37 @@ const ReviewDisplayTab: React.FC<{ isCompactTouchLayout?: boolean }> = ({ isComp
- {/* Token hover cards */} + {/* Hover cards (internally tokenHover*; the label is what changed, not + the ids). One trigger select rather than a toggle plus a mode: `Off` + is a value of the same question, so there is no unreachable + enabled-but-off state to reason about. The delay stays a separate + axis because "too eager" is a complaint neither the hold-modifier option + nor Off answers. */}
- configStore.set('tokenHoverCards', v)} - label="Token hover cards" - description="Rest the pointer on a symbol in a diff to see where it is defined and who references it. Needs ripgrep and a local checkout; nothing appears when the search comes back empty." +
+
Hover cards
+
+ Rest the pointer on a symbol in a diff to see where it is defined and who + references it. Needs ripgrep and a local checkout; nothing appears when the + search comes back empty. {modKeyWord}+click still opens the References panel either way. +
+
+ configStore.set('tokenHoverTrigger', v)} /> +
+
+ How long the pointer rests before a card is requested +
+ configStore.set('tokenHoverDelay', Number(v) as TokenHoverDelay)} + disabled={tokenHoverTrigger === 'off'} + /> +
diff --git a/packages/ui/config/settings.ts b/packages/ui/config/settings.ts index 4bd69bce5e..a748ad9127 100644 --- a/packages/ui/config/settings.ts +++ b/packages/ui/config/settings.ts @@ -15,6 +15,13 @@ import { } from '@plannotator/core/agent-terminal'; import type { DiffLineBgIntensity } from '@plannotator/core/config-types'; import { isFaviconStyle, type FaviconStyle } from '@plannotator/core/favicon'; +import { + DEFAULT_TOKEN_HOVER_DELAY_MS, + isTokenHoverDelay, + resolveStoredTokenHoverTrigger, + type TokenHoverDelay, + type TokenHoverTrigger, +} from '@plannotator/core/token-hover'; import { storage } from '../utils/storage'; import { generateIdentity } from '../utils/generateIdentity'; import { @@ -279,15 +286,38 @@ export const SETTINGS = { // Hovering a token in a code-review diff opens a card with what the search // backend knows about that symbol. Cookie-only like the other review-chrome // preferences: it is presentational, per-browser, and changes no review - // semantics — off simply means no listeners, no requests and no card. - tokenHoverCards: { - defaultValue: true as boolean, + // semantics — `off` simply means no listeners, no requests and no card. + // + // This one select REPLACED the original `tokenHoverCards` boolean rather + // than sitting beside it: a toggle plus a mode has an unreachable state + // (disabled + modifier) and asks one question with two controls. The legacy + // cookie is still read — on every load until the user touches this setting, + // since a migrating read returns a value and so never triggers the + // registry's default-seeding write — so an early adopter who turned cards + // off stays off. Resolution is pure and identical every time; see + // resolveStoredTokenHoverTrigger. + tokenHoverTrigger: { + defaultValue: 'hover' as TokenHoverTrigger, + fromCookie: () => resolveStoredTokenHoverTrigger( + storage.getItem('plannotator-token-hover-trigger'), + storage.getItem('plannotator-token-hover-cards'), + ), + toCookie: (value: TokenHoverTrigger) => + storage.setItem('plannotator-token-hover-trigger', value), + serverKey: undefined, fromServer: undefined, toServer: undefined, + }, + + // How long the pointer rests on a symbol before a card is requested. Three + // fixed steps, not a slider: "too eager" is a real complaint that neither + // `modifier` nor `off` answers, but nobody can tell 340ms from 360ms. + tokenHoverDelay: { + defaultValue: DEFAULT_TOKEN_HOVER_DELAY_MS as TokenHoverDelay, fromCookie: () => { - const value = storage.getItem('plannotator-token-hover-cards'); - return value === 'true' ? true : value === 'false' ? false : undefined; + const parsed = Number(storage.getItem('plannotator-token-hover-delay')); + return isTokenHoverDelay(parsed) ? parsed : undefined; }, - toCookie: (value: boolean) => - storage.setItem('plannotator-token-hover-cards', String(value)), + toCookie: (value: TokenHoverDelay) => + storage.setItem('plannotator-token-hover-delay', String(value)), serverKey: undefined, fromServer: undefined, toServer: undefined, }, diff --git a/packages/ui/config/tokenHoverSetting.test.ts b/packages/ui/config/tokenHoverSetting.test.ts new file mode 100644 index 0000000000..9b47f58b6b --- /dev/null +++ b/packages/ui/config/tokenHoverSetting.test.ts @@ -0,0 +1,104 @@ +/** + * Token hover trigger/delay settings. + * + * The failure this guards is the expensive one: `tokenHoverCards` was a + * boolean, and its `false` is a real preference. A migration that drops it + * silently turns hover cards back ON for the one user who went looking for the + * switch. The rest pins the registry contract the Settings UI and the App both + * read through (unrecognized values fall back to the default rather than + * reaching the hook). + */ +import { afterEach, describe, expect, test } from 'bun:test'; +import { resetStorageBackend, setStorageBackend } from '../utils/storage'; +import { SETTINGS } from './settings'; + +const TRIGGER_KEY = 'plannotator-token-hover-trigger'; +const DELAY_KEY = 'plannotator-token-hover-delay'; +const LEGACY_KEY = 'plannotator-token-hover-cards'; + +function installBackend(seed: Record = {}): Map { + const values = new Map(Object.entries(seed)); + setStorageBackend({ + getItem: (key) => values.get(key) ?? null, + setItem: (key, value) => { values.set(key, value); }, + removeItem: (key) => { values.delete(key); }, + }); + return values; +} + +afterEach(() => { + resetStorageBackend(); +}); + +describe('tokenHoverTrigger', () => { + test('defaults to on-hover when nothing is stored', () => { + installBackend(); + expect(SETTINGS.tokenHoverTrigger.defaultValue).toBe('hover'); + expect(SETTINGS.tokenHoverTrigger.fromCookie()).toBeUndefined(); + }); + + test('round-trips every trigger and rejects anything else', () => { + const values = installBackend(); + + for (const trigger of ['hover', 'modifier', 'off'] as const) { + SETTINGS.tokenHoverTrigger.toCookie(trigger); + expect(values.get(TRIGGER_KEY)).toBe(trigger); + expect(SETTINGS.tokenHoverTrigger.fromCookie()).toBe(trigger); + } + + values.set(TRIGGER_KEY, 'sometimes'); + expect(SETTINGS.tokenHoverTrigger.fromCookie()).toBeUndefined(); + }); + + test('an early adopter who turned cards off with the old boolean stays off', () => { + installBackend({ [LEGACY_KEY]: 'false' }); + expect(SETTINGS.tokenHoverTrigger.fromCookie()).toBe('off'); + }); + + test('the old boolean true migrates to the on-hover behavior it meant', () => { + installBackend({ [LEGACY_KEY]: 'true' }); + expect(SETTINGS.tokenHoverTrigger.fromCookie()).toBe('hover'); + }); + + test('a stored trigger wins over the legacy boolean', () => { + installBackend({ [TRIGGER_KEY]: 'modifier', [LEGACY_KEY]: 'false' }); + expect(SETTINGS.tokenHoverTrigger.fromCookie()).toBe('modifier'); + }); + + test('the legacy cookie is read, never written', () => { + const values = installBackend({ [LEGACY_KEY]: 'false' }); + SETTINGS.tokenHoverTrigger.fromCookie(); + SETTINGS.tokenHoverTrigger.toCookie('off'); + // Left in place deliberately: deleting it would let a downgrade to the + // pre-migration build re-enable cards for someone who turned them off. + expect(values.get(LEGACY_KEY)).toBe('false'); + expect(values.get(TRIGGER_KEY)).toBe('off'); + }); +}); + +describe('tokenHoverDelay', () => { + test('defaults to the dwell the feature shipped with', () => { + installBackend(); + expect(SETTINGS.tokenHoverDelay.defaultValue).toBe(300); + expect(SETTINGS.tokenHoverDelay.fromCookie()).toBeUndefined(); + }); + + test('round-trips the three steps as numbers and rejects anything else', () => { + const values = installBackend(); + + for (const delay of [150, 300, 700] as const) { + SETTINGS.tokenHoverDelay.toCookie(delay); + expect(values.get(DELAY_KEY)).toBe(String(delay)); + expect(SETTINGS.tokenHoverDelay.fromCookie()).toBe(delay); + } + + // An off-step number is as invalid as a word: the hook takes this value as + // a raw setTimeout delay, so a hand-edited cookie must not reach it. + values.set(DELAY_KEY, '5000'); + expect(SETTINGS.tokenHoverDelay.fromCookie()).toBeUndefined(); + values.set(DELAY_KEY, 'slow'); + expect(SETTINGS.tokenHoverDelay.fromCookie()).toBeUndefined(); + values.set(DELAY_KEY, ''); + expect(SETTINGS.tokenHoverDelay.fromCookie()).toBeUndefined(); + }); +}); diff --git a/packages/ui/utils/platform.ts b/packages/ui/utils/platform.ts index f343437c9a..6a2333f391 100644 --- a/packages/ui/utils/platform.ts +++ b/packages/ui/utils/platform.ts @@ -8,4 +8,20 @@ export const isMac = typeof navigator !== 'undefined' && /Mac|iPhone|iPad/.test( export const modKey = isMac ? '⌘' : 'Ctrl'; export const altKey = isMac ? '⌥' : 'Alt'; export const submitHint = isMac ? '⌘↵' : 'Ctrl+Enter'; +/** + * The primary modifier spelled for PROSE ("Cmd+click"), where `modKey`'s ⌘ + * glyph reads as an artifact rather than a word. Use this in sentences; + * use `modKey` in key hints and shortcut chips. + */ +export const modKeyWord = isMac ? 'Cmd' : 'Ctrl'; +/** + * The primary modifier's own `KeyboardEvent.key` name, for features that arm + * on the modifier being HELD rather than on a chord. Pair with `isModKeyHeld`: + * this identifies the key itself, that reads the held flag off any event. + */ +export const modEventKey = isMac ? 'Meta' : 'Control'; +/** Whether the primary modifier is down for this event, on this platform. */ +export function isModKeyHeld(event: { metaKey: boolean; ctrlKey: boolean }): boolean { + return isMac ? event.metaKey : event.ctrlKey; +} export const isWindows = typeof navigator !== 'undefined' && /^Win/.test(navigator.platform);