feat: unified decision control across annotate and code review - #1458
Merged
Conversation
buildDecisionSpec maps { app, gate, count, hasFeedback,
approvalNotesSupported } onto one primary plus ordered menu items —
labels, subtitles and confirm strings verbatim from the approved
prototype (DESIGN_final-proposal.html). approvalNotesSupported gates
every approve-carrying item; the discard item is count-gated so it can
never offer to discard zero annotations; frozen copy is marked inline.
Pure (no React, no DOM) so the full state matrix runs in the plain
bun test lane.
…e-popover hook The joined split control: incumbent primary segment (never opens the popover, never fades while it is open — the popover holds only alternates, so the primary keeps its meaning), caret popover with role=menu rows and roving arrow-key focus, in-place composer morph (Esc ladder consumes on exactly two rungs, drafts kept; Mod+Enter submits trimmed; plain Enter is a newline; empty note refocuses instead of graying), and the single discard confirm through ConfirmDialog. useDismissablePopover is the shared pointerdown-outside + Escape effect with the framed-surface strategy: window blur to an iframe dismisses, since iframe clicks never reach the parent document. ActionMenuItem gains additive role/className props and ExitButton an additive appearance='ghost' form; defaults are byte-identical for existing consumers. DOM tests registered in the CI seam-contract step so they cannot silently skip.
One scope at the shortcuts root (both apps mount the identical control) documenting only the two chords the control implements: Mod+Enter submits the note, Escape steps back keeping it. Enter is deliberately not documented as submit — it is a newline. Registry test guards that the scope composes into both adopting settings registries without id or token collisions ahead of the PR2/PR3 registration.
Internal (non-host-surface) decision-control primitives only: DecisionControl, decisionSpec, useDismissablePopover join the unsupported list — no supported import is added. Core pin stays exact at 0.25.1; smoke:package verified the 0.38.0 tarball.
Review F1: the hook's document-level Escape dismissed the popover but let the event travel on to the apps' window-level Escape ladders — from PR2 one Esc would both close the menu AND drop an HTML-annotate surface to Interact. The hook now skips defaultPrevented events and, when it acts, calls preventDefault + stopPropagation (document bubbles before window). An outside-focus Escape while the composer is open closes the whole popover — an outside-dismissal gesture like an outside click; the draft is kept — commented at the dismiss call site. New DOM test pins all three: open+outside-focus consumes, composer case closes-all keeping the draft, closed control consumes nothing (verified failing without the hook fix). Review F3: pin the frozen 'Request changes…' label (it is in the spec's frozen set but was never asserted) and downgrade the unfrozen sentence pins (composer action labels, approve-note labels, discard sentences) to data assertions — flow verb, live count, singular form, composer distinctness — per the Testing Rules.
…lback F2 (maintainer default, option A, pending final confirmation): the count:0 + hasFeedback:true cell now offers approve-with-notes on capable approval flows with zero-form copy (no annotation-count language); discard-and-finish stays omitted (nothing to discard). F6: when the live spec changes while the composer is open and the active item id leaves the spec, the control morphs back to the menu (draft kept); an emptied menu closes. Implemented in the control so PR2/PR3 cannot diverge.
The annotate header's Exit/Send Feedback/Approve cluster becomes ghost-X Close + DecisionControl driven by buildDecisionSpec. One submitPrimaryDecision() serves the header primary, the global Mod+Enter handler (via a render-assigned ref), and the compact primary row. The zero-state keyboard-only silent submit becomes the visible Done primary and keeps posting the byte-identical legacy /api/feedback body; the gate empty primary stays Approve on /api/approve. Note transports reimplement the held #1436 mechanism: the note becomes a GLOBAL_COMMENT at submit time with a one-render deferred submit whose route is captured at menu-choice time. Non-gated "Done with a note…" carries the approval-framing sentence (buildCompleteAnnotateFeedback approvalFraming); discard flows post the legacy zero payload with empty annotation arrays on their flow's endpoint. Deletions per spec §5.3-§5.5: getAnnotateApprovalPolicy + its confirm dialog and suppression refs, exitWarningAction's approve half, requestAnnotateApprove, handleHeaderAnnotateApprove/Feedback, the annotate half of the shared Approve cluster and its header props. Compact/touch rows are generated from the same spec (visible positive action in every state); composer items open DecisionNoteDialog, the discard item the shared ConfirmDialog. Header predicate consolidation: the flip is hasFeedbackToSend, so agent-terminal-delivered feedback no longer leaves a stale Send Feedback primary. decision-control scope registered in the annotate settings shortcut registry; the submitAnnotations entry now describes the adaptive primary.
… Esc-ladder coverage - decisionHandlers.test.ts (pure, spec §8C): every id buildDecisionSpec can emit resolves to a transport route; note/discard routes follow the gate matrix; compact row ids stay collision-free. - App.decisionControl.test.tsx (DOM, spec §8D/E16, adapted from the held App.submitNote.test.tsx harness): Done posts the byte-identical legacy /api/feedback body, Mod+Enter equals the visible primary, the two note items do not collapse into one payload, gate approves on /api/approve, the discard confirm drops annotations, composer Escape keeps the note, compact touch has a working positive row at zero, and the closed popover never consumes the HTML Esc ladder's Escape. - Registered the DOM file in the CI seam-contract step so it cannot silently skip.
…-spec confirms, armed retry M1 (ruling: honest label, keep transport): additive feedbackDelivered on DecisionSpecInput; the agent-terminal delivered empty state keeps the Done primary and its full-payload /api/feedback transport but stops claiming 'no feedback' in the tooltip and note-item subtitle (free prose, not frozen). App passes isCurrentFeedbackDeliveredToAgent; decisionSpec fact-guard pins the delivered-state copy. L1: dead gate prop removed from AppHeader (decl, destructure, App call site, both test prop objects). L2: confirm/composer surfaces resolve from the LIVE spec — DecisionControl stores only the confirm item id and steps back to the menu when the item leaves the spec; the App's compact composer/confirm dialogs do the same. F6 DOM test extended with a confirm-open spec-flip case. L3: a failed submit keeps the captured note decision armed — handleAnnotateFeedback/Approve report success, pendingDecisionSubmit is cleared only on success (one auto-dispatch per arming, in-flight ref guards double submit), and the next primary invocation replays the captured route/framing. Pinned: note-commit -> POST 500 -> primary retry posts approvalFraming intact on the captured route. Trivial guard: a pending whose note id left annotations state disarms. L4 (ruling: delete): the F6 empty-items close branch and its test are gone — buildDecisionSpec guarantees at least one item. L5: discard confirm copy neutralized (counts can include other tools' findings; the non-gate message notes direct edits still ride). Nit: dropped the review-registry absence assertion in shortcuts.test.ts.
Agent-mode review replaces AgentReviewActions (Close/SendFeedback flip + dimmed Approve + hover tooltip) with the ghost-X Close + DecisionControl driven by buildDecisionSpec: Approve at zero, Send Feedback . n otherwise, with Request changes... / Send with a note... and the explicit 'Approve, discard n annotations...' confirm behind the caret. One submitPrimaryDecision() serves the header primary, the global Mod+Enter handler, and the compact primary row; compact rows are generated from the same spec so a visible positive decision exists in every state, with DecisionNoteDialog / ConfirmDialog resolving from the LIVE spec by item id. Transport routing is pure in reviewDecision.ts and single-endpoint: every decision POSTs /api/feedback with approved as the only fork. A note commits a scope:'general' CodeAnnotation (filePath ''/0/0 sentinels, crypto-random id, no PR-context stamp, not history-recorded) and submits one render later; a failed POST keeps the armed note decision for a primary retry (cleared only on success). Approve-carrying items stay capability-gated on reviewApprovalNotesSupported, hardcoded false until PR5 ships the server advert + consumer delivery (spec 6.4) - no server field is invented here. Deletions per spec 5.1/5.2: AgentReviewActions.tsx outright, the 'Annotations Won't Be Sent' approve confirm + its suppression references, and handleSendFeedback's send-at-zero guard (unreachable once no send is offered at zero, and it would swallow a request-changes submission). The exit warning and the copy-path 'No Annotations' dialog survive. The decision-control shortcut scope joins the review settings registry and the submit entry now describes the adaptive primary.
…overage - reviewDecision.test.ts (pure lane): every id the spec can emit resolves to a route, routes fork only on approved, compact row ids stay unique - the runtime stand-in for the missing app typecheck (spec 8C). - review-note-payload.test.ts, ported as-is from the held branch: the scope:'general' note survives waitForDecision unmodified on BOTH runtimes and archives as a feedback decision counting the note. - exportFeedback.reviewNote.test.ts, ported as-is: the note renders under '## General', co-exists with placed annotations, and is real feedback. - App.decisionControl.test.tsx (DOM lane): mounts the real review App (workerPool's Vite-only ?worker&inline import stubbed per the AllFilesCodeView.lifecycle idiom) and asserts through the posted body: empty-state Approve posts the legacy LGTM shape, Send Feedback posts the live annotations, Request changes... delivers the sentinel general note in the array and the export, discard posts empty arrays only after its confirm, Mod+Enter equals the visible primary, approve-carrying items are absent while the advert is off, and compact touch offers a positive row at zero that posts (E16-review). Registered in the CI DOM lane so it cannot silently skip. - shortcuts.test.ts now asserts the decision-control scope is registered in BOTH adopting registries (the PR2-era compose-cleanly probe became real).
…ed-note failure coverage, loud PR5-gated refusal HIGH-1: one Mod+Enter over the open discard confirm posted TWO contradictory decisions — the app's window-level Mod+Enter effect fired submitPrimaryDecision (approved:false send) while ConfirmDialog's own window-level handler fired onConfirm -> handleApprove (approved:true LGTM) from the same event; stopPropagation cannot stop same-target listeners. Fixed with PR2's idiom: the review Mod+Enter effect bails on the data-plannotator-confirm-dialog sentinel, which covers both the DecisionControl-internal confirm and the compact confirm. Pinned by a DOM repro test asserting exactly ONE POST (the confirm's LGTM body); revert-verified — with the guard neutralized the test fails with two recorded POSTs. MEDIUM-1: added the armed-note failure-path DOM test (500-then-200 double): a failed note POST keeps the scope:'general' note in state with the primary flipped to Send Feedback, the next primary click retries the same note-carrying body, and success clears the armed decision so a subsequent Mod+Enter dispatches nothing. MEDIUM-2: the approve-with-notes refusal now console.errors naming PR5, and the route object carries implemented:false. The advert constant moved to reviewDecision.ts (REVIEW_APPROVAL_NOTES_SUPPORTED) so the new pure contract test can pin 'the advert never emits an id whose route is an unimplemented refusal' — flipping the advert in PR5 without wiring delivery fails the test, and deleting the constant without updating it breaks at import. Docs: the review-header section now records F8 (Approve primary icon-only below lg where ApproveButton showed OK; title carries the accessible name), the two-click discard replacing the dimmed one-click Approve, the Mod+Enter confirm-ownership rule, and the accepted compact-composer draft asymmetry. Also trimmed the pure routing test's per-id echo assertions to the load-bearing equalities.
Maintainer ruling on spec open question 2 (rides PR4): normalizeAnnotation's copied-field allowlist gains scope?: string, so a review-level general comment archives distinguishably from a line comment in index.jsonl. Additive only — the JSONL line shape is a cross-tool contract (fields are added, never repurposed) and readers must keep ignoring unknown fields; an absent scope stays absent (the pre-scope line default), never defaulted-in. apps/pi-extension/generated/ is gitignored and rebuilt by vendor.sh, so the shared edit reaches the Pi runtime at build time; the dual-runtime review-note-payload archive test now asserts the archived general-note record carries scope on BOTH runtimes, and the pure shared-lane test pins the normalizer directly (cannot silently skip).
…comment producer (PR4)
Spec §3.3: scope:'general' annotations already render, badge, and export;
the sole producer was Call Flow. This adds the human one — the community's
"integrated global comment field" ask.
- ReviewSidebar gains optional onAddGeneralComment. The SAME button renders
in the General section header (which now renders whenever the callback is
present, even with zero general comments) AND in the all-empty state — the
state the affordance is most useful in. The composer is the shared
DecisionNoteField in a small anchored popover (the third consumer the
branches' extraction tripwire named); the draft survives a dismissal, an
empty commit refocuses the field (the decision-composer contract).
- Both human producers now share one shape factory,
createGeneralReviewComment in reviewDecision.ts: scope:'general',
sentinel filePath ''/0/0, review-note-${randomUUID()} id, and NO PR
context — an unstamped annotation passes every PR scope predicate
(utils/annotationScope.ts), so the comment survives an in-place PR switch.
- Unlike the header composer's one-submit note, the sidebar comment goes
through addCodeAnnotationsWithHistory: undoable, draft-persisted, and
deletable via the sidebar's existing delete. Creating one raises
totalAnnotationCount, which flips the header control to Send Feedback · n
— the control is state-driven by construction.
No server change (spec §6.1): the comment rides the existing /api/feedback
annotations array and the export's ## General section on both runtimes.
…PR-switch survival Spec §7 PR4 hunt list + §8 test 17: - ReviewSidebar.generalComment.test.tsx (DOM, registered in test.yml's DOM lane so it cannot silently skip): reachable at totalCount === 0; the General section header offers the button with only line comments; a commit delivers the trimmed text exactly once (button and Mod+Enter); an empty note never commits; Escape dismisses but keeps the draft. - App.decisionControl.test.tsx: end-to-end flip proof — a sidebar general comment flips the header primary from Approve to Send Feedback · 1 and rides the posted /api/feedback body with the sentinel shape and no prUrl. Guards the wiring a component test cannot see (prop dropped from the sidebar mount, handler stamping PR context). - reviewDecision.test.ts (pure lane): createGeneralReviewComment shape — trimmed scope:'general' with sentinels, randomUUID ids that cannot collide in one millisecond (spec §9), whitespace-only returns null, and the PR-switch survival pin: no prUrl/diffScope, passes annotationMatchesPrScope for any PR + diff scope.
The portable viewer's Tailwind scan covers packages/review-editor, so the sidebar composer's new utility classes changed the built viewer CSS hash; check:manifest fails on a stale pin.
…t, in-flight guard - M1: clamp the general-comment popover to the sidebar panel width (inline maxWidth = panelWidth - 32) so it never clips inside OverlayScrollArea's overflow-x hidden at the 200px end of the persisted 200-600px range; the viewport-relative class remains only as the overlay-presentation guard. - L2: lift the composer's open + draft state into ReviewSidebar, shared by both placements, so an SSE placement flip (empty state -> General header) or a tab switch keeps a half-typed draft; sidebar collapse discards it. - L1: guard handleAddGeneralComment against submitted/in-flight decisions so a commit during a decision POST cannot vanish from wire and archive. - I1: AGENTS.md — platform-mode presence is deliberate (rides the pr-action body via scope:'general'), and honest draft-lifetime wording. - NIT: the empty-commit test also asserts the refocus contract; new flip test drives the draft survival through the parent. Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9
…age composer (PR5) Both runtimes gain an `approvalNotesSupported` option on the review server (default false) echoed on every diff payload — /api/diff, /api/diff/switch, /api/pr-diff-scope, /api/pr-switch — so the advert survives a diff switch (spec §6.4). An old caller that never passes the option advertises false and the client keeps rendering no approve-carrying items. packages/shared/prompts.ts gains composeReviewApprovedMessage (vendored to Pi with the rest of prompts.ts): the one composer every review decision consumer emits approvals through — the approved prompt, then the approve-time feedback when the decision carries any. It filters the legacy 'LGTM - no changes requested.' placeholder so a new consumer reading an old built client's approval never appends filler the reviewer did not write. Dual-runtime tests: advert default false / true-and-survives-switch on both servers; approve-time feedback reaches waitForDecision byte-identical and archives as approved-with-notes; a bare approval (post-placeholder shape) archives as lgtm with no sidecar — the decision the placeholder made unreachable (spec §6.2 fact 1). Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9
…carding consumers (PR5) The four waitForDecision consumers that threw result.feedback away on the approved branch (spec §6.3) now emit composeReviewApprovedMessage — the approved prompt, then the note when one rides the decision: 1. Claude Code CLI `plannotator review` (apps/hook/server/index.ts); the amp/droid plugins relay its stdout and inherit the delivery. 2. OpenCode native (apps/opencode-plugin/commands.ts) — also fixes the delivery gate: it rode on the LGTM placeholder making feedback truthy, so with the placeholder gone a bare approval would have been silently dropped; the gate is now `feedback || approved`. 3. OpenCode CLI bridge (buildReviewPromptFromBridgeOutcome) — the CLI's JSON record always carried the feedback; the bridge stops discarding it. 4. Pi (apps/pi-extension/index.ts) via the vendored prompts module. Each consumer's startReviewServer call now passes the matching advert: supportsReviewApprovalNotes(origin) for the hook CLI (new seam in apps/hook/server/review-output.ts — every origin shares the one stdout relay today), Boolean(sessionId) for OpenCode native (no session, no delivery — the annotate precedent), unconditional true for Pi and the standalone dev server (which already emitted feedback on approve). Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9
… placeholder removed (PR5)
Client half of spec §6.4, extending PR3's tripwire exactly as its comment
instructed:
- REVIEW_APPROVAL_NOTES_SUPPORTED is deleted. The spec input is now the
server advert, read off /api/diff (and re-read from any diff payload that
carries it) through readApprovalNotesAdvert — absent reads false, so an old
server renders no approve-carrying items and a new server against an old
client changes nothing.
- The approve-with-notes route stops being a marked refusal: it forks on
withAnnotations ('Approve with notes' ships the live annotations plus their
export; 'Approve with a note…' ships the composer note alone) and lands on
handleApprove via the new pure buildReviewApprovalBody.
- handleApprove drops the 'LGTM - no changes requested.' placeholder: a bare
approval sends feedback '' — consumers now print approve-time feedback, and
the empty body is what makes the archive's lgtm decision reachable and
stops bare approvals writing sidecars (spec §6.2 fact 1).
- reviewDecision.test.ts: the PR3 tripwire becomes the delivery assertion
(under a true advert every approve-carrying item's payload carries the
content, never the placeholder) plus the absent-advert-is-false pin.
- App.decisionControl.test.tsx: bare-approval assertions move to the empty
body, the advert-off test pins the old-server payload shape, and a new
advert-on case proves 'Approve with notes' posts the live annotations.
Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9
…e, PR-payload advert, note fold
Applies the PR5 stage-review rulings:
M0: approve-time feedback is no longer appended raw after the approved
prompt ("no changes requested" beside a change-request-shaped export read as
a contradiction). composeReviewApprovedMessage now resolves the new
DEFAULT_REVIEW_APPROVED_WITH_NOTES_PROMPT (configurable as
prompts.review.approvedWithNotes; field added to the config review section),
which frames the notes as non-blocking guidance and says not to revise or
reopen. Signature is now (runtime, feedback, config) so one function fixes
all four consumers; re-vendored to Pi; prompts.test.ts pins the framing,
the config override, the byte-identical bare approval, and the legacy
placeholder filter; cli-bridge.test.ts asserts the bridge routes through
the composer.
M1: fail-closed approval-notes handshake for the OpenCode CLI bridge. The
plugin declares supportsApprovalNotes: true on the opencode-review stdin
JSON; the binary adverts approvalNotesSupported for opencode ONLY when the
declaration is present, so a new binary + old plugin (advert in the binary,
delivery in the independently-versioned plugin) renders no approve-carrying
items instead of silently dropping the reviewer's note.
supportsReviewApprovalNotes stays the seam; documented at both ends beside
the existing version-skew reasoning. Pinned end to end by
apps/hook/server/opencode-review-advert.test.ts, which spawns the real
entrypoint and reads /api/diff: stdin without the declaration serves false,
with it true.
m1: applyPRResponse re-applies the advert from the PR-family payloads
(pr-diff-scope, pr-switch, load-full-diff), so the client stays in lockstep
with whatever diff payload it last applied — the "whole diff family" comment
is now literally true.
m2: buildReviewApprovalBody folds a note in ahead of the export when
annotations also ride, so a future combined item cannot lose data; pinned
with a pure assertion.
i1: AGENTS.md corrected — the standalone dev server emits raw decision JSON
with unfiltered feedback and does not route through the composer; the
consumer list, framing, and handshake are now described accurately.
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
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
The mute classes added to the shared ui components changed the viewer CSS hash; regenerate so CI's staleness check passes. Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9
… a11y, item mute guard - 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
…UUID global-comment id (PR7)
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.
…0.38.0 handoff accuracy (PR7) 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).
…low accuracy (PR7 review) 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.
…-pr7 # Conflicts: # packages/core/guide-viewer-manifest.ts
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.
…; purge em dashes from decision copy 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).
This was referenced Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
One adaptive decision control for every review surface
Implements the approved unified-header design (DESIGN_decision-control.md, DESIGN_final-proposal.html): a single joined split control per surface, with approve and send-feedback never rendered side by side anywhere. Built as a seven-stage stack; every stage got an independent adversarial review, and every finding was fixed in-stage with a pinning regression test before the next stage branched. 34 commits, 65 files, +6383/-552.
What changes for the user
Intentional behavior changes (release-notes material)
Engineering shape
Known follow-ups (tracked)
Supersedes #1452 (its commits are the first stage of this stack).