* fix(ui): remove the annotation panel's per-row Copy button
The panel's card chrome goes back to Edit and Delete. The element context
still rides every HTML and live-app pinpoint into the exported feedback;
exportAnnotationEntry stays as a pure helper for hosts and tests.
* chore(ui): bump @plannotator/ui to 0.39.0
Element context on HTML and live-app pinpoints (#1517, #1520). Core stays
0.25.2; peer ranges unchanged from 0.38.2.
The panel's card chrome goes back to Edit and Delete. The element context
still rides every HTML and live-app pinpoint into the exported feedback;
exportAnnotationEntry stays as a pure helper for hosts and tests.
On OpenCode 2 a plugin message is a pending inbox row; resume: false only
declines the immediate wake. SessionInbox.promote publishes pending steers
as a batch but queued rows one at a time, oldest first, so the queued
notice #1460 introduced was promoted alone by the feedback's own wake and
became its own model turn with the reviewer's annotations behind it.
The notice now rides steer delivery, and feedback that follows an
admitted notice rides steer too, so both enter one promotion and the
agent acts on the annotations. A rejected notice, OpenCode 1, and hosts
without session.synthetic keep the queue delivery unchanged.
A pinpointed element exported as a one-word placeholder ('[element:
Navigation]') or its flattened textContent. The bridge now captures a
bounded description at click time (tag, id, classes, ancestor path, role,
accessible name, allowlisted attributes, rendered text, an adaptive HTML
skeleton, box, landmark, heading, component hint, live route) as
elementContext; the parent re-validates and re-caps it; the export prints
a fenced skeleton plus selector/path/role/name lines under the comment;
the annotation panel gains a per-row Copy for element-bearing cards; the
feedback archive records element identity. Additive: annotations without
the field export byte-identically, share links drop it, no protocol bump.
* fix(ui): drop the annotation count pill from the header decision primary
The primary reads Send Feedback / Post Comments with no inline count; the
count stays in the caret menu's discard and note copy where it is needed.
DecisionPrimary loses its count field, tests pin the count-free label, and
the docs no longer spell the control as 'Send Feedback · n'.
* chore(guides-show): sync the viewer manifest
ui 0.38.1 named a titled alert's row with aria-label="<Type>: <title>" on
a plain div. ARIA prohibits naming a generic element: Chrome exposes it,
WebKit drops it, so VoiceOver on Safari read only the bold title and lost
the type word that 0.37.0 showed as text. The type word now rides a
visually hidden sr-only span before the title, which every engine reads.
Also: the alertIconRenderer seam was consulted even when the title line
carried an emoji (its result was discarded, so rendering was right, but
the contract says it is not called); it is now called only for a title
line with an icon comment and no emoji. Caught by the seam test, which,
like the title test, had been silently skipped: both are DOM-gated and
were missing from the CI DOM list in test.yml. Registered now.
React peer loosened back to ^19.2.3 (0.38.1 declared ^19.2.8 only because
the dependency batch moved it; nothing in the package needs a newer API).
Verified: DOM_TESTS=1 bun test --isolate on both alert test files 14/14,
tsc clean, smoke:package against the published core 0.25.2, full ui suite
unchanged.
@plannotator/ui 0.38.0 does not install into a consumer: config/settings.ts
imports @plannotator/core/token-hover (#1462), but that PR added the export
to core without bumping it, so 0.38.0 pins core 0.25.1, which never
published the subpath. Every host reaches it through configure -> config ->
settings, so typecheck and build fail on the first import.
core 0.25.2 publishes ./token-hover plus the other changes core accumulated
since 0.25.1 (the rotated guide-viewer-manifest pin, the config-types hover
fields). ui 0.38.1 is 0.38.0 pinning core 0.25.2, and aligns
@codemirror/state to ^6.7.2 beside @codemirror/view ^6.43.10 so a consumer
cannot resolve two state copies.
The package smoke passed on 0.38.0 because its external consumer installs
core from the LOCAL tarball (pnpm override), so an export that exists in
source but not on the registry is invisible to it, and its consumer entry
never imported the configure/config path. It now (1) collects every
@plannotator/core/<subpath> the UI source imports and asserts each is an
export of the core source manifest, (2) when the pinned core version is
already on the registry, asserts each is exported by THAT published core
and that the core source exports match it exactly (otherwise: bump core and
publish it first), printing a publish-core-first reminder when the pinned
version is not published yet, and (3) compiles @plannotator/ui/configure
and @plannotator/ui/config in the external consumer's vite build and tsc.
With the pins set back to 0.38.0's, the smoke now fails naming token-hover.
HANDOFF and README: current pair 0.38.1 on 0.25.2; do not consume 0.38.0.
A GitHub alert (> [!NOTE|TIP|IMPORTANT|WARNING|CAUTION]) whose first body
line is a bold-only run, optionally led by one emoji and optionally
trailed by an <!-- icon: name --> comment, followed by an empty line (or
the end of the block), now renders that line as the alert's title on the
icon row in place of the type word. An emoji takes the icon slot; the
icon comment is stripped from the rendered output and resolved through a
new configurePlannotatorUI seam, alertIconRenderer: (name) => ReactNode |
null, default null so the type's own icon renders exactly as today. The
body under a title is indented by the icon width plus the row gap so it
starts at the title's left edge. The type word stays the accessible name
(aria-label "Tip: Browser quirks"). An alert with no title line renders
byte-for-byte as before.
The grammar is pure and dependency-free in utils/alertTitle (exported via
the utils/* subpath) so a host editor can apply the same byte-level rule
it decorates with. Pinned by alertTitle.test.ts; rendering by
AlertBlock.title.test.tsx (spec examples A-E plus controls); the seam by
AlertBlock.seam.test.tsx in the *.seam.test.tsx pattern.
Deliberate deviation from the seam law for the title rendering itself:
Plannotator renders a titled alert differently after this (title on the
icon row instead of a bold paragraph). Owner ruling: a plain rendering
improvement Plannotator gets too, no flag.
Not in this change: a fenced code block inside an alert body still
renders as text. Nesting CodeBlock inside a block stamps a second
data-block-id that the annotation layer resolves against the block list,
so that needs its own design.
/api/diff/switch answered a same-cwd commit:<sha> switch (the Commits-rail
click, where the context recompute is deliberately skipped) with the
launch-frozen session gitContext. The client merges gitContext from every
switch response, so each rail click reverted the base picker and the
commit-baseline list to launch-time data: a commit made mid-session
disappeared until the next non-commit switch, and a worktree review
repointed at the main repo. The rendered diff was never affected.
Regressed in #1497, which added the clientGitContext fallback to carry
the jj diffFallback. That fallback is only reachable on the skipped-
recompute path, where 0.27.13 emitted nothing. Restore that shape in both
runtimes: emit gitContext only when a context was actually recomputed,
still attaching diffFallback to it.
Dual-runtime regression test: a recomputing switch after a mid-session
commit carries the new commit; the following commit:<sha> switch carries
no gitContext. Fails on both servers without the fix.
* feat(editor): Mod+E toggles markdown edit mode in place (#1479)
* fix(editor): scope the edit-mode exit chord and wire its tests into CI
- Early-return the dedicated Mod+E exit listener when the event target is
an INPUT/TEXTAREA outside .cm-editor, so native text fields (Ask AI box,
annotation textareas) keep their own Mod+E instead of committing and
exiting the edit session underneath them; match the key case-insensitively
so Caps Lock cannot dead-key the exit. CodeMirror's content DOM is
contenteditable, so the editor path is untouched. Also note in code that
mounting atomic-editor's selectionToolbar() would claim Mod-e and dead-key
the chord.
- New DOM test pinning the ownership split (foreign textarea keeps the
session open; the chord from inside CodeMirror still exits),
revert-verified against the guard.
- De-flake "never discards an unsaved source-backed buffer silently":
the single setTimeout(0) after a contentEditable DOM mutation raced
CodeMirror's MutationObserver, which happy-dom intermittently never
re-delivers after CM's stop()/start() observer cycling — the buffer is
now dirtied through the editor's own dispatch, with a bounded poll for
the toolstrip's Save readout. 8/8 stable under CI's exact invocation
preceded by another App suite (previously ~3/8 failures).
- Add App.editModeShortcut.test.tsx to the DOM-gated file enumeration in
.github/workflows/test.yml so the suite actually runs in CI.
---------
Co-authored-by: Michael Ramos <mdramos8@gmail.com>
* fix(pi): persist approved plan checklist progress
* test(pi): restore checklist progress from prior sessions
* feat(pi): mark plan steps during execution
* fix(pi): align mark_done instructions and harden checklist rendering
Review fixes for #1496:
- phase-tools-runtime: pin plannotator_mark_done in the executing-phase
active tool union and its release at agent_end.
- Align the plan-approved doneMsg with the executing framing: call
plannotator_mark_done after each completed step, [DONE:n] markers as
the fallback for interrupted executions.
- Harden renderCompletedChecklist: restrict the shared checklist
pattern's whitespace to [^\S\n] so a whitespace-only checkbox line
can never swallow the next line (the root cause the review named —
\s+ crossing newlines), replace the marker at the match's own capture
position instead of first-occurrence line.replace("[ ]", ...), and
mirror parseChecklist's empty-text skip so ordinals stay aligned.
Regression test: blank checkbox placeholders around real steps; the
real step's box flips, the placeholders are never touched
(revert-verified).
- Document the one-turn ordinal-desync window when an agent edits the
plan's checkboxes mid-turn, bounded by upgrade-only writes plus the
per-turn re-parse.
---------
Co-authored-by: Michael Ramos <mdramos8@gmail.com>
* fix(data-dir): resolve the data directory per call instead of at import
* fix(shared): refresh agent schema files once per process, not once ever
---------
Co-authored-by: Michael Ramos <mdramos8@gmail.com>
* fix(review): stabilize JJ line-of-work bases
* fix(review): scope switch-handler context adoption to gitbutler and jj
Maintainer review follow-ups: keep plain-git sessions serving their
launch-frozen context on reload (the recomputed worktree context still
rides the switch response transiently), truncate frozen 40-hex commit
ids in the jj-line header label, and mark the two deliberate test pins.
---------
Co-authored-by: Michael Ramos <mdramos8@gmail.com>
Adds two session-only flags to plannotator review, parsed in the shared
parser so every host inherits them together:
- --base <ref> opens the session against a caller-chosen compare target
(branch, origin/<branch>, tag, SHA, HEAD~N), probed with git rev-parse
--verify --end-of-options before the server starts so a typo'd ref is a
startup error with near-match suggestions instead of a silently
mislabelled merge-base->HEAD diff.
- --diff-type <id> opens the session in one of the nine flat git diff
modes (REVIEW_OPEN_DIFF_TYPES, pinned against GIT_DIFF_TYPES).
The flags are a seed, never a setting: nothing writes config.json or any
review cookie, and the UI stays fully mutable. Validation is pure in
packages/shared/review-open-state.ts (provider matrix errors on
jj/GitButler/P4/workspace/PR mode, promote-with-notice when the saved
default is base-irrelevant, fatal explicit contradiction).
A flagged base rides explicitBase semantics: the new initialBaseExplicit
server option (both runtimes) seeds baseExplicitlyChosen, suppressing the
startup origin/* upgrade and canonicalization, and openStatePinned rides
/api/diff so the client neither offers the first-run setup dialog (its
one-time cookie is NOT consumed) nor runs the panel-pair self-heal for a
pinned session. The since-base dropdown label now renders from the live
active base, matching the adjacent base picker.
Coverage: Bun CLI, opencode-review bridge, OpenCode embedded plugin, and
the Pi extension (re-vendored; strict validation on the slash-command
path only, programmatic callers unchanged). Skills, command stubs, help
text, and docs updated across every host surface.
parseReviewArgs now reports argument-shape problems through an always-present
errors[] field instead of letting unknown dash-prefixed tokens fall into the
ignored positional list. All four host surfaces refuse to start a session on
a parse error: the CLI and the opencode-review bridge exit 1 with the errors
on stderr, and the Pi and OpenCode plugins notify through their hosts. Plain
non-dashed words stay tolerated for slash-command hosts that forward raw user
prose. The loop is index-based so value-taking flags can consume their value
token.
Gates /api/doc reads on realpath containment: a path must resolve inside the project both as written and after symlink resolution, else 403. Consolidates the duplicated doc-read logic from both server runtimes into one shared module (packages/shared/doc-resolve.ts, vendored to Pi).
Claude-Session: https://claude.ai/code/session_019GV6EKtzh8Pf9GA2rrBLNf
Closes#1456. Adds plannotator review --json emitting one { decision, message } record from the same builder as the plaintext output, routes Amp solely on the decision field, and removes the substring classifier that could silently drop feedback mentioning phrases like 'no feedback'. Invalid or legacy CLI output produces a recoverable update notice, never a guessed decision.
Claude-Session: https://claude.ai/code/session_019GV6EKtzh8Pf9GA2rrBLNf
Closes#1455. Resolves the data directory per call in shared storage and sandboxes PLANNOTATOR_DATA_DIR for every bun test via the preload, so test runs can no longer write into a contributor's real ~/.plannotator.
Claude-Session: https://claude.ai/code/session_019GV6EKtzh8Pf9GA2rrBLNf
- Anonymize the real project paths and ticket prefix in session-log
test fixtures (slug expectations updated to match).
- Untrack scripts/convert-themes.ts: a one-shot migration script
hardcoding a path into an unrelated private project; its output in
packages/ui/themes/ is already committed.
- Delete the three unreferenced sprite_package_*/index.html preview
pages, which also shipped in the @plannotator/ui npm tarball via the
wholesale directory entries in files.
crypto.randomUUID is undefined in insecure contexts, so the three note
paths added in this window (annotate composer note, Viewer global
comment, review general note) threw and silently lost the typed text in
remote-mode http sessions. generateId already carries the fallback.
* docs(review): design the token hover card trigger settings and announcement
* feat(review): token hover card trigger settings and first-run announcement
Hover-for-everyone is a good default and a bad only-option. Two cookie-only
settings now cover the three complaints it produces, and a one-time dialog
puts the choice in front of the reviewer once.
- @plannotator/core/token-hover (shimmed to shared): TokenHoverTrigger
(hover | modifier | off), TokenHoverDelay (200 | 350 | 700), and the
migration that reads the old tokenHoverCards boolean once.
- settings.ts: tokenHoverTrigger REPLACES tokenHoverCards rather than sitting
beside it, so there is no unreachable enabled-but-off state. A legacy
`false` resolves to `off`; the legacy cookie is read, never written, so a
downgrade cannot silently re-enable cards for someone who turned them off.
- useTokenHover: an options argument, { mode, delayMs }. In modifier mode the
Alt gate sits ahead of the dwell timer, the cache read and every piece of
state, so a hover with the key up costs one boolean read. Pressing Alt while
already parked on a token opens the card (a key press fires no pointer
event, so the last enter is remembered); releasing it starts the same leave
grace, except while the pointer is inside the card; window blur clears a
stale held flag; and typing owns the key, since Alt+Backspace and Alt+arrow
are word-editing chords. The three listeners exist only in modifier mode, so
hover mode is byte-for-byte what shipped.
- The diff views are untouched: their prop signatures are unchanged, which
matters because both are compiled into the portable guide viewer. `off` is
still enforced in App by withholding the handler props entirely.
- TokenHoverAnnouncementDialog: last in the first-run chain, compact, with a
radio group that applies the trigger immediately so Done and Escape both
mean "accept what is selected". It skips a session where cards cannot run
without consuming its cookie, and never shows to a reviewer who already has
a non-default trigger.
- Settings > Review Display: the toggle becomes a trigger segment plus a delay
segment, the delay inert while the trigger is off.
Click behavior is deliberately untouched. Cmd/Ctrl+click and Alt+click both
still open the References panel; every gesture on a token is already spoken
for, and the modifier mode delivers what a click mode would be wanted for.
Alt rather than Cmd because Cmd/Ctrl+hover already paints the navigable-target
affordance, and Alt is spelled the same on every platform. Reasoning and the
rejected controls are in DESIGN_hover-settings.md.
The guides.show viewer manifest moves with this: the settings registry is in
the portable viewer's graph.
* fix(review): hover card review findings, M1/M2 and minors
M1 stale pointerInCardRef. onCardEnter set "the pointer is inside the card"
and only onCardLeave cleared it, but a scroll-close or a window blur unmounts
the card UNDER the pointer, so no leave ever arrives. The flag then stayed
true for the rest of the session and every later Alt release was ignored as
"they are reading the card", leaving cards stuck open. close() now resets it,
which covers the blur path too since onBlur closes.
M2 Alt+click double surface. handleCodeNavRequest closes the hover first, so
EVERY route into the References panel is covered by one line: Cmd+click,
Ctrl+click, the Alt+click alias, and the card's own location links. It sits
ahead of the no-checkout early return, because a click that cannot resolve
must still dismiss the card it was made on top of. This also repairs the
overlap #1461 shipped with, where a click on a token whose card was open or
mid-dwell left both surfaces on screen; close() cancels the pending dwell and
aborts an in-flight request, so a click during the dwell can no longer resolve
into a card behind the panel. Alt+click is where this is routine rather than
incidental: in modifier mode the alias and the trigger share the key.
Minors:
- The announcement's featureAvailable is LATCHED at the first post-load
render, like guideIntroEligibleRef. canUseLiveWorkspaceActions changes on
mid-session diff switches, so an unlatched read could pop the dialog over
work in progress or yank an open one away mid-read.
- resolveTokenHoverAnnouncementPending is pure again, like its editMode
sibling. The cookie write that retires the announcement for an
already-decided reviewer moved to shouldConsumeTokenHoverAnnouncement,
called from an App effect: a state initializer can run more than once, and a
getter that writes storage goes unnoticed until StrictMode double-invokes it.
- The migration is documented as what it is. fromCookie RE-READS the legacy
key on every load until the user touches the setting, because a migrating
read returns a value and so never triggers the registry's default-seeding
write. Resolution is pure and identical every time, so the repeated read
costs one lookup and cannot drift; seeding inside a getter was rejected as
more code for no behavioral gain. DESIGN §7, the settings comment and
AGENTS.md all say this now instead of claiming a one-time seed.
- SegmentedControl builds its classNames from a filtered array (no double or
trailing spaces) and only puts `disabled` on the button when true.
Nits: the Cmd-only copy in the dialog note and the Settings row is now
platform-aware through a new modKeyWord export ('Cmd' / 'Ctrl'), which is the
prose spelling modKey's glyph cannot serve; and the dialog's radio group
follows the WAI-ARIA roving-tabindex pattern, so Tab enters and leaves it as
one stop while arrows move the selection and wrap.
Tests: M1's exact reviewer sequence (hold Alt, open, pointer into card,
scroll-close, hover another token, release, assert it closes after the grace)
fails without the reset; the References-handoff mechanism (open card gone,
pending dwell cancelled, in-flight request aborted) is behavioral in the hook
suite, and codeNavHoverHandoff.test.ts pins the App wiring at source level the
way iframeIsolation does, failing without the closeTokenHover call. Also: blur
clears the held state, arrows move the radio selection, and the announcement
latch no longer writes.
The dialog suite additionally seeds its trigger cookie: configStore is a
process-global singleton whose loadFromBackend keeps the in-memory value when
the new backend is silent, so an unseeded map inherited another file's last
write. The new roving-focus test is what surfaced that.
* fix(review): dialog footer names the real Settings tab (Editor, not Review Display)
* feat(review): jumbo hover card announcement, and rename the feature to Hover cards
The announcement was too plain for a first-use dialog. It now uses the repo's
big-format shell (max-w-5xl, badge/title/description header, a 1.1fr/1fr body
that collapses to one column under 820px, footer with the Settings pointer and
one action), matching EditModeAnnouncementDialog and
LookAndFeelAnnouncementDialog.
The left column, where the Edit Mode dialog plays its recording, is a worked
example: a three-line strip of diff with `charge` shown under the pointer, and
below it a hover card reproducing the real card's anatomy in the order
TokenHoverCard renders it (name plus kind badge, approximate signature with its
matched-line marker, doc line, Defined at, a reference sample ending in the
overflow line). It is JSX over the existing theme tokens rather than a bitmap,
so it is crisp at any DPI, follows the active palette in both light and dark,
and cannot go stale against a card whose anatomy changes. It is decorative:
the whole block is aria-hidden, carries nothing focusable, and every fact it
shows is also stated in the prose beside it.
The right column is unchanged in behavior: the same three-option radio group,
the same immediate apply, the same roving-tabindex keyboard model, and the
click-behavior note (now also carrying the ripgrep requirement, which the
Settings row already stated). The footer keeps the Editor-tab destination
d04cfce5 corrected.
Rename: the user-facing name is "Hover cards" everywhere a reader meets it, in
the dialog title and the Settings heading. "Token" described the
implementation's unit rather than the thing the reviewer points at, and in an
app full of annotations "token hover card" invites reading it as a kind of
comment card. Every internal identifier is untouched: tokenHoverTrigger,
tokenHoverDelay, the cookie keys, the hook and component names, and the
data-token-hover-* test ids all keep their spelling, so there is no migration.
Tests: the behavioral assertions are unchanged (once-ness, radio applies,
dismiss and Escape keep the choice, arrows move selection), plus one that the
example block is aria-hidden and holds nothing focusable, because read aloud it
is a wall of invented identifiers that says nothing about the choice on offer.
The guides.show CSS bundle hash moves with this: Tailwind scans source for
class names, and the example introduces utilities the shared stylesheet did not
carry. The JS bundle is unchanged, as expected for an app-side dialog.
* feat(review): make the announcement example a live try-it on the real card
The example was a hand-drawn impression of a hover card. A fork of that markup
drifts from the shipped card the first time either changes, which is exactly
what an example is supposed to prevent. It is now the real thing, and it is
interactive: the `withRetry` token in the strip is genuinely hoverable, and
resting on it opens the REAL TokenHoverCard through the REAL useTokenHover, so
the reviewer feels the actual dwell, leave grace and Alt gate before committing
to a setting.
Nothing is reimplemented. Three thin seams carry it:
- useTokenHover gains `resolve`, defaulting to the POST /api/code-nav/hover it
always did. The try-it returns a hardcoded CodeNavHoverResponse instead. This
is the ONLY fixture: a demo must not search the reviewer's repository for a
symbol they never asked about, and a second copy of the dwell/supersession/
grace logic would drift. The resolver is read through a ref so an inline
literal cannot re-arm the hook's callbacks every render.
- TokenHoverCard gains `layerClassName` (default `fixed z-[60]`, unchanged).
The card portals to <body> like every instance, so the one thing its host has
to be able to say is which stacking context it belongs in; the try-it puts it
at z-[110], above the z-[100] modal it is demonstrated inside.
- TokenHoverCard gains `inert`, which drops the location buttons out of the tab
order. The try-it has no References panel behind it, so they lead nowhere.
The card body was extracted to TokenHoverCardContent so the positioned wrapper
and any other host share one copy; every class, section and separator is the
same markup the live card renders, not a duplicate.
The hovered token wears the diff pane's own underline treatment, now defined
once in tokenHoverStyles.ts: usePierreTheme serializes it into Pierre's shadow
stylesheet as `.pn-token-hover`, the dialog applies the same declarations as a
style object. Two consumers, one definition.
The try-it reads the LIVE setting rather than a prop, so flipping the radio to
"While holding Alt" makes the demo behave that way immediately and "Off" makes
it do nothing and closes any open card. That is the honest preview of each
choice, which is the point of putting the control next to the demo.
Because it is interactive it is labeled rather than hidden: the region carries
role="group" and an aria-label, the visible prompt line ("Try it: rest your
pointer on withRetry") carries the meaning, and only the mock code lines stay
aria-hidden, since read aloud they are a wall of invented identifiers.
Tests: hovering the token opens the real card (asserted through the component's
own data-token-hover-card marker) with the fixture's symbol and definition path
after the dwell and not before; the try-it obeys the live trigger, so under
"Hold Alt" a rest opens nothing until Alt goes down, and choosing Off takes an
open card away; and the region is labeled while its mock code is hidden. React
synthesizes onPointerEnter from pointerover, which is what the tests dispatch.
The guides.show viewer hashes move on both halves: usePierreTheme is in the
viewer's graph and now imports the shared style module (JS), and the try-it
introduces Tailwind utilities the shared stylesheet did not carry (CSS).
Budgets are unaffected (395.7 KB gz of 400).
* fix(review): the try-it fixture names the token the prompt asks you to hover
Two defects the browser capture caught, both invisible in the DOM tests.
The prompt said "rest your pointer on withRetry" and the card that opened was
titled `charge`: the fixture still described the symbol from the earlier static
example. It now describes withRetry, and takes its name from EXAMPLE_SYMBOL so
the prompt and the card cannot disagree again.
The hovered token sat mid-line, and the card anchors at the token's left edge
and is 400px wide, so it opened across the radio column the reviewer is meant
to be reading. The token now leads the line, which keeps the card inside the
example's own column.
* feat(review): the hover card modifier gate is Cmd (Ctrl), not Alt
Maintainer ruling. Two reasons, both recorded in DESIGN_hover-settings.md §4.1
in place of the old Alt argument:
- Alt is very commonly bound to push-to-talk dictation, so an Alt-held gate
would open cards every time the user starts speaking, with the pointer
wherever they left it. That is the exact failure this setting exists to
prevent. Alt-hold is also already taken inside Plannotator, by the plan
editor's temporary input-method switch.
- Cmd+hover is already VS Code's "tell me about this symbol" gesture: the
navigable-target underline plus a definition peek under one held key.
Gating on Cmd rides that muscle memory instead of competing with it.
The old anti-Cmd argument is moot. The underline and the card appearing
together under a held Cmd is not a collision, it IS that composite gesture; and
handleCodeNavRequest already dismisses the hover surface on every References
invocation, so Cmd+click cleanly supersedes an open card or a pending dwell
rather than stacking with it.
The key is platform-aware through two new helpers beside the existing ones in
packages/ui/utils/platform.ts: `isModKeyHeld` (metaKey on macOS, ctrlKey
elsewhere) and `modEventKey` ('Meta' / 'Control'). Labels come from the
existing modKeyWord, so the key the code checks and the key the copy names
cannot disagree. Labels updated in the dialog radio and its description, the
Settings segmented control, the design doc and AGENTS.md.
Only the modifier ALONE arms. Any other key going down while it is held
(Cmd+C, Cmd+V, Cmd+S) disarms and takes an open card with it, so a copy
performed with the pointer parked over the diff cannot pop a card mid-copy.
This was cheap: the keydown handler already had to compare event.key. The
typing-target guard stays as belt and braces.
Blur handling stays and matters more now: on macOS the app switcher is the same
key this gate arms on, so Cmd+Tab is the common way to leave with it held.
Internal ids are untouched. The stored trigger value is still `modifier`,
because the setting names the shape of the gate and not which key fills it, so
nothing migrates. The #1461 Alt+click References alias is unrelated and is left
alone.
The try-it feels the new gate for free, since it drives the same hook: choosing
the hold option and holding Cmd over the demo token opens the demo card.
Tests: the gate suites now build their events from the platform helpers, so
they exercise Cmd on macOS and Ctrl on the Linux runner rather than pinning one
platform. All behavioral assertions are unchanged. Two new ones cover the chord
rule, and both fail without it: a copy chord with the pointer parked on a token
opens nothing, and a chord while a card is open closes it.
* fix(review): pointer cursor wins over Pierre's I-beam on hovered and Cmd-held tokens
* fix(review): the intro dialog's demo token gets the pointer cursor too
* feat(review): hover delay default is 300ms matching VS Code; tiers respaced 150/300/700
* test(review): restore the 700-delay test's summed advances
The 350 -> 300 sweep in b5538350 rewrote both advances in this test, but
they SUM to the configured dwell rather than each naming it: 300+300 is
600, so a 700ms delay never elapsed and the suite went red at the tip.
Back to 350+350 with the pairing spelled out, so the guard stays
meaningful in both directions: the first advance is past the 300 default
(a hook that ignored delayMs would have fired already) and the pair
reaches exactly 700. Verified fail-closed by pinning the timeout back to
DEFAULT_TOKEN_HOVER_DELAY_MS, which fails this test and nothing else.
The dialog test's two advances become DEFAULT_TOKEN_HOVER_DELAY_MS
itself: its dwell IS the registry default (the seeded backend says
nothing about tokenHoverDelay), so naming the constant is both correct
and immune to the next respacing.
* fix(review): the hover gate's typing guard pierces the shadow root
Two defects in one branch of the modifier gate's keydown handler.
The guard read event.target, which a window-level listener receives
RETARGETED to the shadow host. Pierre's edit-session editor is a
contenteditable inside that shadow root, so a bare Cmd pressed while
editing code in place read as a plain <div>, armed the gate, and popped
a card over the text being edited: verbatim the failure "typing owns the
key" exists to prevent. The three other window-level typing guards in
this package (AllFilesCodeView, FileTree, SectionsPanel) already read
composedPath()[0] for exactly this reason and say so in comments; this
one is no longer the exception.
The guard also sat ahead of the chord branch, so it suppressed
DISARMING as well as arming. Focus reaches a composer between the two
routinely (a card is open, the reviewer clicks into the comment box and
pastes), and that left the card standing over the diff with the gate
still armed behind it. The chord branch now runs first and typing only
gates the arm path. keyup deliberately keeps no typing guard: if the
gate never armed the branch is already a no-op, and if it did arm the
release has to be honored wherever focus has since travelled.
Tests: the old "modifier chords in a comment box" case exercised the
chord branch, not the guard, so it passed with isTypingTarget deleted.
Replaced by a bare-modifier press from a textarea and one from a
shadow-root editor, plus a chord-while-typing case for the asymmetry;
the two pre-existing chord-disarm tests stay, so that coverage is
unchanged. The shadow event is synthesized rather than dispatched from a
real shadow root because happy-dom does not retarget, which would have
made the test vacuous. Each of the three fails closed against its own
defect: reverting composedPath fails only the shadow case, deleting the
guard fails both typing cases, and moving the guard back above the chord
branch fails only the asymmetry case.
* feat(review): the modifier gate paints the navigable-target affordance
The ruling that settled Cmd over Alt was that the underline and the card
appearing together under one held key IS the composite gesture rather
than a collision. The code did not deliver it: the diff views paint
pn-token-nav from the pointer ENTER event, and neither half of the
gesture this mode exists for fires one. Parking on a symbol and THEN
holding the key opened a card on a token wearing no affordance, and
releasing closed the card while leaving the affordance painted until the
pointer eventually left.
useTokenHover now reports the arm and disarm transitions through an
optional onModifierGate(armed, tokenElement), always carrying the token
the pointer is parked on RIGHT NOW so a disarm also unpaints a neighbour
the pointer drifted onto while the key was down. Every disarm route is
covered: chord, release, blur, and the effect cleanup when the reviewer
switches trigger with the key still held. The release notifies ahead of
the reading-the-card early return, because the key being up ends the
affordance whether or not the card is being read.
App owns the painting, not the hook: the diff views are compiled into
the portable guides.show viewer and their prop signatures must not move,
and the class is a review-app concern. Enter-time painting is untouched,
so plain Cmd+hover in the default hover mode behaves exactly as before.
Also gates pn-token-hover on a hover handler actually being wired, and
paints it in AllFilesCodeView. That class carries a cursor: pointer that
became !important in eebab519, so painting it unconditionally started
promising clickability on every token in a session with hover cards
switched off; and leaving it out of the all-files view meant the
affordance the announcement's try-it demonstrates was missing from the
default review surface. A read-only or portable host passes no hover
handler and still gets neither class.
Fail-closed: dropping notifyGate(true) fails both new hook tests,
dropping the App wiring fails the App pin, and making the DiffViewer
paint unconditional fails the gating pin.
* fix(review): hover card cleanups, and the design's delay tiers
Comments first, because several of them had become false and this repo
treats them as contract. The dialog cited a StaticTokenHoverCard that no
longer exists and a card that "lives inside an aria-hidden decorative
block" (it does not; the try-it region is a labelled role=group), the
try-it still named "the Alt gate", and two hook comments plus four test
comments still described the trigger key as Alt or claimed the Alt+click
alias and the trigger share a key, which stopped being true when the gate
became Cmd. The focus trap's rationale claimed the example card's buttons
were in its query; the card portals to <body>, so they never were.
tokenHoverStyles now single-sources what it says it does. Only thickness
and offset were actually shared; the decoration line and the cursor were
written twice and could drift. Both forms read one object, and the two
genuinely per-form bits (the resolved color, and the !important the
shadow form needs to beat Pierre's I-beam) are named as such.
The display-only card stops advertising clicks it swallows: its own
footer says "Click a location to jump", so under `inert` the locations
lose the hand cursor and hover underline as well as the tab order, and
the card gets aria-hidden — it portals outside the aria-modal dialog it
belongs to, where a screen reader would meet it as a stray tooltip.
The announcement's availability latch moves from the render body into an
effect: a ref written during render is a side effect React may discard.
The gate independently requires !isLoading, so the ordering is unchanged.
Both announcement test files now re-hydrate configStore in afterEach.
They re-resolve a process-global singleton from a two-entry fake map, and
the pure lane runs in one process, so they were handing their own
defaults to whatever file ran next.
The dialog test gains the two assertions that were missing: the demo
card sits at z-[110] (the layerClassName seam's entire purpose is not
being buried under the modal it is demonstrated inside) and its
locations are non-interactive. Both fail closed against dropping the
respective prop.
Design doc: §4.2 still listed 200/350/700 with a 350 default, which the
maintainer ruling in §6 had already replaced; §3's 350 now reads as the
history it is; §6 records how the composite gesture is delivered and §9
how the hovered-token treatment is gated.
* chore(core): re-pin the guide viewer build
The manifest was synced before the last three commits of this branch and
went stale: usePierreTheme (which serializes the token-hover CSS into
Pierre's stylesheet), the token-hover default constant, and now
AllFilesCodeView are all inside the viewer bundle, so the JS hash moved
while the CSS hash did not — the .pn-token-hover rule is a JS template
string, not a stylesheet entry, which is exactly why the drift was easy
to miss by eye. check:manifest was failing at the tip.
Rebuilt and synced last, so this pins the branch's final source. Budgets
after: js 395.8 KB gz against a 400 KB budget, css 50.9 against 64,
worker still import-free.
* feat(review): add POST /api/code-nav/hover in both runtimes
Tier 0 of the token hover card: the hover pipeline is the ripgrep search
/resolve already runs, plus three cheap enrichments read off the same
machinery.
- packages/shared/code-nav.ts: definition patterns carry the kind they
prove (alternations split one-per-kind, so definition-vs-reference
classification is byte-identical and classifyMatch becomes a thin
wrapper over classifyMatchDetailed); scanDocComment, buildSignature and
resolveCodeNavHover; an additive timeoutMs option on resolveCodeNav so
hover can ask for 3s while /resolve keeps its 5s; an optional readFile
member on CodeNavRuntime so /resolve callers stay unchanged.
- Both review servers gain /api/code-nav/hover behind the same guard
stack as /resolve. /resolve itself is untouched.
The doc scan is conservative by construction: per-language, blank-line
separated, capped, and null for an unknown language. Returning nothing
always beats returning garbage.
* feat(review): token hover cards in the code-review diff
Resting the pointer on a symbol opens a card with where it is defined, an
approximate signature, its doc comment if the scan found a real one, and a
sample of its references. Every location on the card routes into the same
References panel Cmd+click opens.
- utils/stitchTokenIdentifier: rebuilds one identifier from the token spans
Shiki fragmented it into, using each span's data-char column to prove
adjacency. It stops at dots (rg searches with --word-regexp, where a
dotted path matches nothing) and refuses keywords and one-character
names, which is what keeps most hovers off the wire entirely.
- hooks/useTokenHover: 350ms dwell before any request exists, one in-flight
request aborted by its successor, a 30-entry LRU flushed whenever the diff
snapshot changes, a 250ms leave grace so the card's own links are
reachable, and a scroll/wheel cancel because the anchor rect is stale the
moment the pane moves. An unavailable backend, a failure, a timeout and a
thin answer all render nothing, silently: a hover is an idle gesture and
must never nag.
- components/TokenHoverCard: portaled to body so it escapes the Dockview
overflow and stacking context; anchored below the token, flipped above
when the viewport would clip it. It shows what the search found and
nothing it did not, so uncertainty is a second location line rather than
a description of the ranking.
- Wiring: two optional props beside onCodeNavRequest in both diff views,
passed only when the existing live-workspace gate AND the new cookie-only
"Token hover cards" setting are on. Off means no listeners, no requests
and no card in the tree. Alt+click joins Cmd+click as an unadvertised
alias into the References panel; the meta/ctrl branch is unchanged.
The guides.show viewer manifest moves with this: AllFilesCodeView is in the
portable viewer's graph, so its new optional props shift the bundle hash.
* fix(review): correct token hover supersession, scroll and doc-scan defects
Review findings, each with the regression test that fails without the fix.
- An open card could be rewritten by a NEIGHBOUR's answer: drifting onto an
adjacent token launched its request, and returning to the open card took
the same-key early return without reclaiming the active key, so the
neighbour's answer still passed the landing check. The early return now
reclaims the key, kills the pending dwell, and aborts a foreign request.
- Re-entering a token inside the leave grace re-armed the dwell while that
token's own request was still in flight, spawning a second ripgrep for an
answer already on its way. The dwell now joins the in-flight request, and
the answer anchors to the span the pointer is on now.
- Scrolling INSIDE the card closed it, which made the signature block's
horizontal scroller unreadable by the gesture meant to read it. The
cancel now ignores events originating in the card's own subtree; a pane
scroll still closes it.
- A below-threshold answer for a different token left the previous token's
card standing over a symbol the reviewer had already left.
- The doc scan rendered tooling directives as documentation. Directives are
dropped from BOTH ENDS of the comment run — eslint-disable, @ts-*,
prettier-ignore, biome-ignore, istanbul ignore, noqa, type: ignore and
triple-slash references — because the commonest real position is the line
immediately above the definition, which is the trailing end of the run as
collected. Never from the middle: a directive surrounded by prose sits
inside documentation we would have to interpret to cut safely. A run that
is nothing but directives returns null, and prose that merely mentions a
directive is untouched.
Also: an answer whose token has been recycled out of the DOM opens no card
(a detached rect is 0,0 and would pin it to the viewport corner); a flipped
card is clamped to the top edge; the card is a tooltip, not a dialog; a
location click describes the CLICKED location rather than forwarding the
hover's charStart and language into another file; definition.preview stays
declared but unpopulated until a consumer exists; the overflow line regains
its leading ellipsis and now renders under the banned-vocabulary sweep.
Portable viewer: the hover prop is inverted to (props, filePath) so the two
diff views import nothing new and stitching lives in App. The read-only
guide viewer bundle no longer carries the stitcher or the request builder.
Maintainer rulings: the non-gated annotate empty state offered 'Done with a
note…' and 'Request changes…' whose only difference was the approval-framing
sentence on the same /api/feedback transport - they collapse into a single
'Send a note…' item (id 'request-changes', plain feedback, no framing). The
approvalFraming machinery stays for the non-gated discard path and gate mode;
the non-gate 'note-with-approval' route is now dead code pinned as unframed
feedback so a stray dispatch can never fabricate approval. And no user-facing
decision-control string carries an em dash any more: titles, subtitles,
composer action labels, confirm copy, and the two Mod+Enter shortcut
descriptions are rewritten with plain punctuation (agent-facing prompt
constants and frozen labels untouched).
Post-demo maintainer feedback: without a gate there is no approval, so the
non-gate Done primary and its note row drop the success tone and check icon
(gate and review Approve keep them), and the ghost close button renders the
word Close at every breakpoint instead of a bare X.
F1: with vim enabled, Escape on an open ActionMenu/ApproveDropdown/
DecisionControl was consumed by useVimDocumentFocus (registered first on
document, preventDefaults while reclaiming focus; the popover hook then
skips the defaultPrevented event) and the menu stayed open. Open popovers
now stamp data-pn-dismissable-popover and the vim hook's
BLOCKING_OVERLAY_SELECTOR treats them as blocking overlays, so vim defers.
Pinned in ActionMenu.test.tsx (verified failing without the selector entry).
F2: the plan editor's diff-exit Escape listener gains the defaultPrevented
early-return so a popover-consumed Escape no longer also exits the diff
view. Audit of the remaining document-level Escape listeners found only
component-internal ones (Settings theme preview, which already consumes;
dialog focus traps handle Tab, not Escape) — left alone.
F7: onDismiss callbacks memoized in ActionMenu/ApproveDropdown so
useDismissablePopover stops re-registering listeners every render while
open.
F8: ActionMenu.test.tsx, PlanHeaderMenu.mobile.test.tsx, and
ReviewHeaderMenu.mobile.test.tsx were DOM-gated but never registered in the
CI DOM lane; added to the test.yml file list so they actually run (the F1
pin lives in a lane-registered file).
F3-F5 (docs): test-opencode-review.sh stdout describes the decision control
(Approve at zero) instead of the removed always-on Send Feedback button;
UI-TESTING.md's platform-mode flow uses the real positional PR URL; the
AGENTS.md annotate flow diagram ends on the adaptive primary like the
review diagram.
tests/UI-TESTING.md gains the Decision Control Manual Checklist (annotate
file/gate/folder/last, HTML/live-app iframe dismissal, review agent +
platform modes, compact/touch, sidebar general comment) and fixes the
test-opencode-review.sh flow that asserted a Send Feedback button at zero
annotations. AGENTS.md: the code-review flow diagram stops claiming Approve
sends the literal LGTM (gone since the approve-with-notes delivery), and the
shortcuts-root scope list names decisionControl.shortcuts.ts beside
history.shortcuts.ts. packages/ui/HANDOFF.md's 0.38.0 line now covers the
whole stack accurately: internal primitives stay off the host-supported
surface, decisionControlShortcuts rides the blessed shortcuts barrel, and
the ToolbarButtons muted-prop removal is internal. The stack ships as one
unpublished 0.38.0 — no further version bump (spec §2.1/§9: batch the bump
into PR1 and PR7; nothing was published in between).
ActionMenu and ApproveDropdown drop their hand-rolled outside-pointerdown +
Escape effects for the shared useDismissablePopover (spec §5.7/§7 PR7). The
one deliberate behavior delta is the hook's documented contract: the Escape
that dismisses an open menu is consumed, so it no longer also runs the host
app's Escape ladder — one Escape, one rung. ApproveDropdown's listeners now
run only while open (they were no-ops when closed). FloatingQuickLabelPicker
keeps its own dismissal on purpose (deferred capture-phase registration so
the opening selection gesture cannot dismiss it; Escape shares the
digit-select listener) — the spec's third named copy, QuickLabelDropdown.tsx,
carries no dismissal effect to converge.
Viewer's global comment id moves from global-${Date.now()} to a UUID
(spec §9): two comments minted in the same millisecond would collide on a
timestamp id.
- AGENTS.md: the platform-mode sentence now describes the shipped PR6 state
(ghost-X + DecisionControl, no composer, everything through the existing
ReviewSubmissionDialog, self-approval mute preserved) instead of the
pre-PR6 three-button row.
- ToolbarButtons: delete the now-dead `muted` prop from FeedbackButton and
ApproveButton — PR6 removed its last consumer. `dimmed` stays (live plan-
mode consumer in packages/editor AppHeader); ToolbarButtons is not on the
host-supported README surface, so the removal is internal.
- DecisionControl: the muted primary's reason now renders through the shared
Tooltip (Base UI trigger wires floating-ui useFocus, so it shows on hover
AND keyboard focus-visible) plus a persistent aria-describedby description
via a hidden span; the native title is dropped for the muted case only so
the tooltip is not doubled. Frozen sentence unchanged.
- runPlatformDecisionAction: structural mute guard for the approve ITEM ids
too — a muted item's handler is inert from any surface, not only where the
DOM disables the row.
- DOM test: the empty+self menu pins initial focus on the first non-disabled
row (the roving-focus :not(:disabled) skip now has a regression pin), and
the muted-primary assertions follow the reason to aria-describedby.
Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9
The platform three-button row (ExitButton + conditional "Post Comments"
FeedbackButton + self-approval ApproveButton with its hover tooltip div) is
replaced by the same ghost-X Close + DecisionControl shape agent mode wears,
rendered from buildDecisionSpec's platform arm. Every action routes through
the pure resolvePlatformDecisionAction into openPlatformDialog — the existing
ReviewSubmissionDialog (per-target state, retry, "leave PR open" toggle) is
untouched and remains the only note field on this side.
Mod+Enter in platform mode now fires the visible primary via the same
runPlatformDecisionAction the header and compact rows call, so the muted
self-approval empty state is a keyboard no-op (previously it silently fell
back to the comment dialog); while the submission dialog is open the keydown
effect still routes Mod+Enter to the dialog's own submit before any primary
dispatch, so nothing double-fires. Compact/touch rows are generated from the
platform spec exactly like agent mode's, reusing the same row-id mappers
(unique per spec, swept in the pure lane). The destination dropdown,
double-tap Alt, and the ghost-X exit warning are untouched.
DOM tests cover the four platform states (n>0/empty x self/not): the primary
and every menu item open the dialog in the right mode with nothing posted,
the mute is a dead-end-free no-op, Mod+Enter over the open dialog submits it
exactly once, and a destination flip swaps the spec while stranding nothing.
Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9
buildDecisionSpec grows a platform input arm (§3.4): the same DecisionSpec
shape with no composer, confirm, or discard items, ever — labels per the
approved DESIGN_header-pr-mode mock. Approve-carrying rows gate only on
selfAuthored and mute rather than disappear; approvalNotesSupported is
deliberately ignored (the platform posts to the forge natively).
DecisionControl renders the mute: a muted primary dims, no-ops on click, and
keeps its native title tooltip (never `disabled`, which would eat the reason);
muted menu rows disable with the reason in their subtitle, and the roving
focus skips disabled rows so arrow keys keep working.
Pure tests pin the §3.4 hard rules at spec level: no composer/confirm on the
platform side in any state, mute-not-remove with a live row always left, and
the advert ignored.
Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9
Michael Ramos