From 2556d88eb69e46aa00250b7e3c5fc7fd785590ce Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Tue, 1 Sep 2026 23:20:55 -0700 Subject: [PATCH 1/6] feat(ui): pure decision spec for the unified header control MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/ui/utils/decisionSpec.test.ts | 190 +++++++++++++++++ packages/ui/utils/decisionSpec.ts | 274 +++++++++++++++++++++++++ 2 files changed, 464 insertions(+) create mode 100644 packages/ui/utils/decisionSpec.test.ts create mode 100644 packages/ui/utils/decisionSpec.ts diff --git a/packages/ui/utils/decisionSpec.test.ts b/packages/ui/utils/decisionSpec.test.ts new file mode 100644 index 000000000..7ae058fca --- /dev/null +++ b/packages/ui/utils/decisionSpec.test.ts @@ -0,0 +1,190 @@ +import { describe, expect, it } from 'bun:test'; +import { + buildDecisionSpec, + type DecisionSpec, + type DecisionSpecInput, +} from './decisionSpec'; + +/** Every input combination the spec can receive, for the invariant sweeps. */ +function allInputs(): DecisionSpecInput[] { + const inputs: DecisionSpecInput[] = []; + for (const app of ['annotate', 'review'] as const) + for (const gate of [false, true]) + for (const hasFeedback of [false, true]) + for (const approvalNotesSupported of [false, true]) + for (const count of [0, 1, 3]) + inputs.push({ app, gate, count, hasFeedback, approvalNotesSupported }); + return inputs; +} + +function itemIds(spec: DecisionSpec): string[] { + return spec.items.map((item) => item.id); +} + +describe('buildDecisionSpec state matrix', () => { + // Guards the model itself: each row of the spec's state table produces the + // expected primary and the expected ordered menu. + it('annotate, no feedback, no gate → Done + note/request-changes', () => { + const spec = buildDecisionSpec({ + app: 'annotate', gate: false, count: 0, hasFeedback: false, approvalNotesSupported: false, + }); + expect(spec.primary.label).toBe('Done'); // frozen copy, maintainer-approved + expect(spec.primary.tone).toBe('success'); + expect(spec.primary.icon).toBe('check'); + expect(itemIds(spec)).toEqual(['note-with-approval', 'request-changes']); + // "Done with a note…" posts /api/feedback — never capability-gated. + expect(spec.items[0].composer?.actionLabel).toBe('Done — send note'); + expect(spec.items[1].dividerBefore).toBe(true); + expect(spec.items[1].composer?.actionLabel).toBe('Send as feedback'); + }); + + it('annotate, no feedback, gate → Approve; approve-note item only with the capability', () => { + const withCap = buildDecisionSpec({ + app: 'annotate', gate: true, count: 0, hasFeedback: false, approvalNotesSupported: true, + }); + expect(withCap.primary.label).toBe('Approve'); // frozen copy, maintainer-approved + expect(withCap.primary.tone).toBe('success'); + expect(itemIds(withCap)).toEqual(['note-with-approval', 'request-changes']); + expect(withCap.items[0].label).toBe('Approve with a note…'); + + const withoutCap = buildDecisionSpec({ + app: 'annotate', gate: true, count: 0, hasFeedback: false, approvalNotesSupported: false, + }); + expect(itemIds(withoutCap)).toEqual(['request-changes']); + expect(withoutCap.items[0].dividerBefore).toBe(false); + }); + + it('annotate, feedback (n) → Send Feedback + note/(approve-with-notes)/discard', () => { + const nonGate = buildDecisionSpec({ + app: 'annotate', gate: false, count: 3, hasFeedback: true, approvalNotesSupported: true, + }); + expect(nonGate.primary.label).toBe('Send Feedback'); // frozen copy, maintainer-approved + expect(nonGate.primary.tone).toBe('primary'); + expect(nonGate.primary.icon).toBe('send'); + // No gate ⇒ no approve channel ⇒ no Approve-with-notes, capability or not. + expect(itemIds(nonGate)).toEqual(['note-with-feedback', 'discard-and-finish']); + expect(nonGate.items[1].label).toBe('Done, discard 3 annotations…'); + expect(nonGate.items[1].confirm?.confirmText).toBe('Discard & finish'); // frozen copy + + const gate = buildDecisionSpec({ + app: 'annotate', gate: true, count: 3, hasFeedback: true, approvalNotesSupported: true, + }); + expect(itemIds(gate)).toEqual(['note-with-feedback', 'approve-with-notes', 'discard-and-finish']); + expect(gate.items[1].label).toBe('Approve with notes'); // frozen copy, maintainer-approved + expect(gate.items[2].label).toBe('Approve, discard 3 annotations…'); + expect(gate.items[2].confirm?.confirmText).toBe('Discard & approve'); // frozen copy + + const gateNoCap = buildDecisionSpec({ + app: 'annotate', gate: true, count: 3, hasFeedback: true, approvalNotesSupported: false, + }); + expect(itemIds(gateNoCap)).toEqual(['note-with-feedback', 'discard-and-finish']); + }); + + it('review, no feedback → Approve; phase-1 menu is Request changes only', () => { + const phase1 = buildDecisionSpec({ + app: 'review', gate: true, count: 0, hasFeedback: false, approvalNotesSupported: false, + }); + expect(phase1.primary.label).toBe('Approve'); + expect(itemIds(phase1)).toEqual(['request-changes']); + + const phase2 = buildDecisionSpec({ + app: 'review', gate: true, count: 0, hasFeedback: false, approvalNotesSupported: true, + }); + expect(itemIds(phase2)).toEqual(['note-with-approval', 'request-changes']); + expect(phase2.items[0].label).toBe('Approve with a note…'); + }); + + it('review, feedback (n) → Send Feedback + note/(approve-with-notes)/discard', () => { + const phase2 = buildDecisionSpec({ + app: 'review', gate: true, count: 3, hasFeedback: true, approvalNotesSupported: true, + }); + expect(phase2.primary.label).toBe('Send Feedback'); + expect(phase2.primary.shortLabel).toBe('Send'); + expect(itemIds(phase2)).toEqual(['note-with-feedback', 'approve-with-notes', 'discard-and-finish']); + + const phase1 = buildDecisionSpec({ + app: 'review', gate: true, count: 3, hasFeedback: true, approvalNotesSupported: false, + }); + expect(itemIds(phase1)).toEqual(['note-with-feedback', 'discard-and-finish']); + expect(phase1.items[1].dividerBefore).toBe(true); + }); +}); + +describe('buildDecisionSpec invariants', () => { + // Guards the maintainer's hard rule: Approve/Done and Send Feedback never + // render side by side — there is exactly one primary and the menu never + // smuggles a second one in. + it('never yields two primaries, in any input combination', () => { + for (const input of allInputs()) { + const spec = buildDecisionSpec(input); + expect(spec.primary.id).toBe('primary'); + expect(itemIds(spec)).not.toContain('primary'); + // The header shows Send Feedback XOR a positive finish, never both. + const positiveLabels = ['Done', 'Approve']; + if (spec.primary.label === 'Send Feedback') { + expect(positiveLabels).not.toContain(spec.primary.label); + } else { + expect(positiveLabels).toContain(spec.primary.label); + } + } + }); + + // Guards rendering an item that silently drops content: without the + // capability advert, no approve-carrying item exists in the approval flows. + it('approvalNotesSupported: false ⇒ no approve-carrying item anywhere', () => { + for (const input of allInputs()) { + if (input.approvalNotesSupported) continue; + if (input.app === 'annotate' && !input.gate) continue; // no approve channel at all + const ids = itemIds(buildDecisionSpec(input)); + expect(ids).not.toContain('approve-with-notes'); + expect(ids).not.toContain('note-with-approval'); + } + }); + + // Guards a refactor that drops the one remaining guard dialog. + it('every discard item carries a confirm', () => { + for (const input of allInputs()) { + for (const item of buildDecisionSpec(input).items) { + if (item.id === 'discard-and-finish') { + expect(item.confirm).toBeDefined(); + expect(item.tone).toBe('destructive'); + } + } + } + }); + + // Guards a stale count in the label after an annotation is deleted. + it('interpolates the live count into the pill and the discard copy', () => { + const zero = buildDecisionSpec({ + app: 'annotate', gate: false, count: 0, hasFeedback: true, approvalNotesSupported: false, + }); + expect(zero.primary.count).toBeUndefined(); + // Nothing to discard at zero — no discard item with a lying "0 annotations". + expect(itemIds(zero)).not.toContain('discard-and-finish'); + + const three = buildDecisionSpec({ + app: 'review', gate: true, count: 3, hasFeedback: true, approvalNotesSupported: true, + }); + expect(three.primary.count).toBe(3); + const discard = three.items.find((item) => item.id === 'discard-and-finish')!; + expect(discard.label).toContain('3'); + expect(discard.confirm!.title).toContain('3'); + + const one = buildDecisionSpec({ + app: 'annotate', gate: false, count: 1, hasFeedback: true, approvalNotesSupported: false, + }); + const discardOne = one.items.find((item) => item.id === 'discard-and-finish')!; + expect(discardOne.label).toBe('Done, discard 1 annotation…'); + }); + + // Every composer item must actually be a composer and every plain item must + // not — the control branches on these fields, so an item with both (or a + // confirm item with a composer) would render an unreachable surface. + it('composer and confirm are mutually exclusive per item', () => { + for (const input of allInputs()) { + for (const item of buildDecisionSpec(input).items) { + expect(item.composer && item.confirm).toBeFalsy(); + } + } + }); +}); diff --git a/packages/ui/utils/decisionSpec.ts b/packages/ui/utils/decisionSpec.ts new file mode 100644 index 000000000..2e9c00949 --- /dev/null +++ b/packages/ui/utils/decisionSpec.ts @@ -0,0 +1,274 @@ +/** + * Pure state→spec mapping for the unified header decision control. + * + * No React, no DOM, no imports — this is what keeps the state matrix testable + * in the plain `bun test` lane and keeps "both apps and both states are data, + * not forked components" true. `DecisionControl.tsx` renders whatever this + * returns; the apps translate ids into handlers. + * + * NOT host-supported surface: like ActionMenu/ConfirmDialog, this module is + * app-shared chrome and is deliberately absent from the README supported-import + * list and the strict-consumer tsconfig. + * + * Labels, subtitles and confirm strings are the approved prototype's, verbatim + * (DESIGN_final-proposal.html `spec()`), which is authoritative over any older + * branch or mock copy. + */ + +export type DecisionActionId = + | 'primary' // the left segment + | 'note-with-approval' // "Done with a note…" / "Approve with a note…" + | 'request-changes' // "Request changes…" + | 'note-with-feedback' // "Send with a note…" + | 'approve-with-notes' // review + gate-annotate; capability-gated + | 'discard-and-finish'; // "Done/Approve, discard n annotations…" + +export type DecisionTone = 'success' | 'primary' | 'destructive'; + +export interface DecisionPrimary { + id: 'primary'; + label: string; // 'Done' | 'Approve' | 'Send Feedback' + shortLabel?: string; // 'Send' — the lg-breakpoint label + mobileLabel?: string; // compact/touch row label + title: string; // tooltip / aria description + tone: Exclude; + icon: 'check' | 'send'; + count?: number; // rendered as the inline pill; omitted when 0 +} + +export interface DecisionComposer { + title: string; // popover back-button title, e.g. 'Send with a note' + actionLabel: string; // the composer's own button, e.g. 'Send feedback with note' + tone: Exclude; + icon: 'check' | 'send'; + placeholder: string; // 'Add a note...' +} + +export interface DecisionConfirm { + title: string; + message: string; + confirmText: string; +} + +export interface DecisionMenuItem { + id: Exclude; + label: string; + subtitle: string; + tone: DecisionTone; + icon: 'check' | 'send'; + dividerBefore?: boolean; + composer?: DecisionComposer; // present ⇒ the item morphs the popover + confirm?: DecisionConfirm; // present ⇒ the item raises one confirm +} + +export interface DecisionSpec { + primary: DecisionPrimary; + items: DecisionMenuItem[]; +} + +export interface DecisionSpecInput { + app: 'annotate' | 'review'; + /** Annotate: `gate`. Review: always true — review's primary decision IS approval. */ + gate: boolean; + /** The count rendered in the pill and interpolated into labels. */ + count: number; + /** + * Whether there is anything to send. Deliberately separate from `count`: + * annotate counts direct edits / saved-file changes / attachments as + * feedback with count 0 (`hasFeedbackContent` in the annotate app). + */ + hasFeedback: boolean; + /** Does the runtime deliver feedback on approve? Gates every approve-carrying item. */ + approvalNotesSupported: boolean; +} + +export const DECISION_NOTE_PLACEHOLDER = 'Add a note...'; + +function annotationNoun(count: number): string { + return count === 1 ? 'annotation' : 'annotations'; +} + +/** + * The empty state: no feedback to send, the primary is the positive finish. + * `approvalFlow` (gate annotate, or review) makes it `Approve`; plain annotate + * gets `Done`. + */ +function buildEmptySpec(input: DecisionSpecInput, approvalFlow: boolean): DecisionSpec { + // "Done with a note…" posts /api/feedback like every other non-gated annotate + // outcome, so it is never capability-gated. "Approve with a note…" carries a + // note on the approve channel, which four runtimes still discard — it renders + // only where the advert says delivery works (never an item that silently + // drops content). + const positive: DecisionMenuItem | null = approvalFlow + ? input.approvalNotesSupported + ? { + id: 'note-with-approval', + label: 'Approve with a note…', + subtitle: 'Approve and send a short note with it', + tone: 'success', + icon: 'check', + composer: { + title: 'Approve with a note', + actionLabel: 'Approve — send note', + tone: 'success', + icon: 'check', + placeholder: DECISION_NOTE_PLACEHOLDER, + }, + } + : null + : { + id: 'note-with-approval', + label: 'Done with a note…', + subtitle: 'Finish and send a short note with the approval', + tone: 'success', + icon: 'check', + composer: { + title: 'Done with a note', + actionLabel: 'Done — send note', + tone: 'success', + icon: 'check', + placeholder: DECISION_NOTE_PLACEHOLDER, + }, + }; + + const requestChanges: DecisionMenuItem = { + id: 'request-changes', + // Frozen copy (maintainer-approved): 'Request changes…'. + label: 'Request changes…', + subtitle: 'Write overall feedback — sent as a change request', + tone: 'primary', + icon: 'send', + dividerBefore: positive !== null, + composer: { + title: 'Request changes', + actionLabel: 'Send as feedback', + tone: 'primary', + icon: 'send', + placeholder: DECISION_NOTE_PLACEHOLDER, + }, + }; + + return { + primary: approvalFlow + ? { + id: 'primary', + // Frozen copy (maintainer-approved): 'Approve'. + label: 'Approve', + title: 'Approve — no changes requested', + tone: 'success', + icon: 'check', + } + : { + id: 'primary', + // Frozen copy (maintainer-approved): 'Done'. + label: 'Done', + title: 'Finish — records that you reviewed with no feedback', + tone: 'success', + icon: 'check', + }, + items: positive ? [positive, requestChanges] : [requestChanges], + }; +} + +/** + * The feedback state: something to send, the primary is `Send Feedback`. + * Identical across apps; only the discard item's verb follows the flow. + */ +function buildFeedbackSpec(input: DecisionSpecInput, approvalFlow: boolean): DecisionSpec { + const { count } = input; + const noun = annotationNoun(count); + + const items: DecisionMenuItem[] = [ + { + id: 'note-with-feedback', + label: 'Send with a note…', + subtitle: + count > 0 + ? `Add an overall note on top of your ${count} ${noun}` + : 'Add an overall note on top of your feedback', + tone: 'primary', + icon: 'send', + composer: { + title: 'Send with a note', + actionLabel: 'Send feedback with note', + tone: 'primary', + icon: 'send', + placeholder: DECISION_NOTE_PLACEHOLDER, + }, + }, + ]; + + // The divider separates "send" alternates from "approve away" alternates — + // it sits before whichever approve-flavoured item comes first. + let dividerPending = true; + + // Capability-gated AND count-gated: with zero annotations (feedback is + // direct edits/attachments) there are no notes to ride along, so the item + // would be a no-op with a lying label. + if (approvalFlow && input.approvalNotesSupported && count > 0) { + items.push({ + id: 'approve-with-notes', + // Frozen copy (maintainer-approved): 'Approve with notes'. + label: 'Approve with notes', + subtitle: `Approve; your ${count} ${noun} ride along as non-blocking guidance`, + tone: 'success', + icon: 'check', + dividerBefore: dividerPending, + }); + dividerPending = false; + } + + // Destructive by definition — it throws the annotations away — so it always + // carries the one confirm the new model keeps. With count 0 there is nothing + // to discard and the item is omitted. + if (count > 0) { + items.push({ + id: 'discard-and-finish', + label: approvalFlow + ? `Approve, discard ${count} ${noun}…` + : `Done, discard ${count} ${noun}…`, + subtitle: 'Asks to confirm — the annotations are not sent', + tone: 'destructive', + icon: 'check', + dividerBefore: dividerPending, + confirm: approvalFlow + ? { + title: `Discard ${count} ${noun} and approve?`, + message: + 'Your annotations are change requests. Approving without them tells the agent no changes are needed.', + // Frozen copy (maintainer-approved): 'Discard & approve'. + confirmText: 'Discard & approve', + } + : { + title: `Discard ${count} ${noun} and finish?`, + message: + 'Your annotations are change requests. Finishing without them sends a "reviewed, no feedback" record.', + // Frozen copy (maintainer-approved): 'Discard & finish'. + confirmText: 'Discard & finish', + }, + }); + } + + return { + primary: { + id: 'primary', + // Frozen copy (maintainer-approved): 'Send Feedback'. + label: 'Send Feedback', + shortLabel: 'Send', + mobileLabel: 'Send feedback', + title: 'Send your feedback to the agent', + tone: 'primary', + icon: 'send', + count: count > 0 ? count : undefined, + }, + items, + }; +} + +export function buildDecisionSpec(input: DecisionSpecInput): DecisionSpec { + // Review's primary positive decision IS approval, gate flag or not. + const approvalFlow = input.app === 'review' || input.gate; + return input.hasFeedback + ? buildFeedbackSpec(input, approvalFlow) + : buildEmptySpec(input, approvalFlow); +} From 90dd3577e15c28229bafd8bdb0df7bd24620f45f Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Tue, 1 Sep 2026 23:21:03 -0700 Subject: [PATCH 2/6] feat(ui): DecisionControl split pill + note field/dialog + dismissable-popover hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/workflows/test.yml | 1 + packages/ui/components/ActionMenu.tsx | 11 +- .../ui/components/DecisionControl.test.tsx | 293 ++++++++++ packages/ui/components/DecisionControl.tsx | 542 ++++++++++++++++++ packages/ui/components/ToolbarButtons.tsx | 20 +- packages/ui/hooks/useDismissablePopover.ts | 57 ++ 6 files changed, 922 insertions(+), 2 deletions(-) create mode 100644 packages/ui/components/DecisionControl.test.tsx create mode 100644 packages/ui/components/DecisionControl.tsx create mode 100644 packages/ui/hooks/useDismissablePopover.ts diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3116632e9..451258e35 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -177,6 +177,7 @@ jobs: packages/editor/components/AppHeader.webmcpIndicator.test.tsx packages/ui/components/MathBlock.firstPaint.test.tsx packages/ui/components/DiagramBlock.lazyRetry.test.tsx + packages/ui/components/DecisionControl.test.tsx opencode-v2: name: OpenCode 2 installed package diff --git a/packages/ui/components/ActionMenu.tsx b/packages/ui/components/ActionMenu.tsx index 4993ab2b3..4f7186b19 100644 --- a/packages/ui/components/ActionMenu.tsx +++ b/packages/ui/components/ActionMenu.tsx @@ -69,6 +69,12 @@ interface ActionMenuItemProps { subtitle?: string; badge?: React.ReactNode; disabled?: boolean; + /** ARIA menu semantics for hosts that render a real `role="menu"` popover + * (DecisionControl). Default undefined so existing consumers are unchanged. */ + role?: 'menuitem'; + /** Appended to the row's classes (e.g. a tone token). Default undefined so + * existing consumers are byte-identical. */ + className?: string; } export const ActionMenuItem: React.FC = ({ @@ -78,13 +84,16 @@ export const ActionMenuItem: React.FC = ({ subtitle, badge, disabled = false, + role, + className, }) => (