mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
fix(ui): collapse the non-gate empty menu to one Send a note composer; 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 commit is contained in:
@@ -389,9 +389,11 @@ keyboard and header can never disagree. Transport routing is pure in
|
||||
`packages/editor/annotateDecision.ts`: `Done` and every note post `/api/feedback` (a note becomes
|
||||
a `GLOBAL_COMMENT` at submit time with a one-render deferred submit — zero server change), so
|
||||
`formatAnnotateOutcome` shapes and strict-gate exit codes are byte-identical to the old
|
||||
keyboard-only zero submit; only gate-mode approvals reach `/api/approve`. The non-gated
|
||||
"Done with a note…" is distinguished from "Request changes…" solely by the approval-framing
|
||||
sentence (`buildCompleteAnnotateFeedback`'s `approvalFraming`), and the only confirm left is the
|
||||
keyboard-only zero submit; only gate-mode approvals reach `/api/approve`. The non-gated empty
|
||||
menu carries a single composer, "Send a note…" (maintainer ruling: the old "Done with a note…" /
|
||||
"Request changes…" pair differed only by framing on the same transport and was collapsed into
|
||||
one item); the approval-framing sentence (`buildCompleteAnnotateFeedback`'s `approvalFraming`)
|
||||
now serves only the non-gated discard path, and the only confirm left is the
|
||||
explicit `Done/Approve, discard n annotations…` menu item (plus the pre-existing
|
||||
close-with-content warning). Compact/touch rows are generated from the same spec, so a visible
|
||||
positive decision exists in every state; composer rows open `DecisionNoteDialog`. The header flip
|
||||
|
||||
@@ -11,9 +11,8 @@
|
||||
* - The note must reach the agent AS A GLOBAL COMMENT in both the exported
|
||||
* feedback string AND the annotations array, one render after the commit —
|
||||
* a same-tick submit sends the pre-note payload and silently drops it.
|
||||
* - "Done with a note…" and "Request changes…" must NOT collapse into one
|
||||
* payload: only the approval framing distinguishes them on the one
|
||||
* feedback transport.
|
||||
* - "Send a note…" (the collapsed non-gate composer) must post plain,
|
||||
* unframed feedback: a note is a note, never a fabricated approval.
|
||||
* - Gate mode's empty primary must approve through /api/approve (that is
|
||||
* what makes a strict gate exit 0).
|
||||
* - The discard confirm must drop the annotations from the posted body, not
|
||||
@@ -280,12 +279,12 @@ describe.if(hasDom)("annotate decision control", () => {
|
||||
expect(submissions[0]!.feedback).toBe(ANNOTATE_NO_FEEDBACK_SENTENCE);
|
||||
});
|
||||
|
||||
test("Request changes… posts one GLOBAL_COMMENT change request without approval framing", async () => {
|
||||
test("Send a note… posts one GLOBAL_COMMENT without approval framing", async () => {
|
||||
setStorageBackend(memoryBackend);
|
||||
seedAnnouncementsSeen();
|
||||
await mountAnnotate();
|
||||
|
||||
await openComposer("Request changes");
|
||||
await openComposer("Send a note");
|
||||
await typeNote("tighten the intro");
|
||||
await pressNoteKey("Enter", { metaKey: true });
|
||||
|
||||
@@ -299,23 +298,20 @@ describe.if(hasDom)("annotate decision control", () => {
|
||||
expect(body.feedback!.startsWith(ANNOTATE_NO_FEEDBACK_SENTENCE)).toBe(false);
|
||||
});
|
||||
|
||||
test("Done with a note… posts the same note WITH the approval framing sentence", async () => {
|
||||
// Maintainer ruling (empty-menu collapse): the non-gate empty menu is ONE
|
||||
// composer. A resurrected "Done with a note…" row would silently split the
|
||||
// decision back into two costumes.
|
||||
test("the empty non-gate menu offers exactly one composer item", async () => {
|
||||
setStorageBackend(memoryBackend);
|
||||
seedAnnouncementsSeen();
|
||||
await mountAnnotate();
|
||||
|
||||
await openComposer("Done with a note");
|
||||
await typeNote("looks fine, watch the migration");
|
||||
await pressNoteKey("Enter", { ctrlKey: true });
|
||||
await act(async () => caretButton()!.click());
|
||||
await settle();
|
||||
|
||||
expect(submissions).toHaveLength(1);
|
||||
const body = submissions[0]!;
|
||||
expect(body.endpoint).toBe("feedback");
|
||||
const notes = globalComments(body);
|
||||
expect(notes).toHaveLength(1);
|
||||
expect(notes[0]!.text).toBe("looks fine, watch the migration");
|
||||
expect(body.feedback!.startsWith(ANNOTATE_NO_FEEDBACK_SENTENCE)).toBe(true);
|
||||
expect(body.feedback).toContain("looks fine, watch the migration");
|
||||
const items = Array.from(document.querySelectorAll<HTMLButtonElement>('[role="menuitem"]'));
|
||||
expect(items).toHaveLength(1);
|
||||
expect(menuItem("Done with a note")).toBeUndefined();
|
||||
});
|
||||
|
||||
test("the note rides alongside annotations already in the session", async () => {
|
||||
@@ -405,55 +401,55 @@ describe.if(hasDom)("annotate decision control", () => {
|
||||
seedAnnouncementsSeen();
|
||||
await mountAnnotate();
|
||||
|
||||
await openComposer("Request changes");
|
||||
await openComposer("Send a note");
|
||||
await typeNote("not ready to send");
|
||||
await pressNoteKey("Escape");
|
||||
|
||||
// Back at the menu, nothing submitted.
|
||||
expect(noteInput()).toBeNull();
|
||||
expect(menuItem("Request changes")).toBeDefined();
|
||||
expect(menuItem("Send a note")).toBeDefined();
|
||||
expect(submissions).toHaveLength(0);
|
||||
|
||||
const item = menuItem("Request changes")!;
|
||||
const item = menuItem("Send a note")!;
|
||||
await act(async () => item.click());
|
||||
await settle();
|
||||
expect(noteInput()?.value).toBe("not ready to send");
|
||||
});
|
||||
|
||||
// L3 pin: a failed POST must keep the captured decision armed — retrying
|
||||
// through the primary re-posts the SAME route with the approval framing
|
||||
// intact. Without it, the retry would re-derive from live state (the note
|
||||
// raised hasFeedbackToSend) and silently reframe "Done with a note" as a
|
||||
// bare change request.
|
||||
test("a failed note submit keeps the decision armed; retry keeps the captured framing", async () => {
|
||||
// through the primary replays THAT decision. Without it, the retry would
|
||||
// either drop the armed note or commit it a second time (two
|
||||
// GLOBAL_COMMENTs from one composer submit).
|
||||
test("a failed note submit keeps the decision armed; retry replays it without a second note", async () => {
|
||||
setStorageBackend(memoryBackend);
|
||||
seedAnnouncementsSeen();
|
||||
failFeedbackPosts = 1;
|
||||
await mountAnnotate();
|
||||
|
||||
await openComposer("Done with a note");
|
||||
await openComposer("Send a note");
|
||||
await typeNote("hold the line");
|
||||
await pressNoteKey("Enter", { metaKey: true });
|
||||
|
||||
// First attempt: captured framing, but the POST failed — no completion.
|
||||
// First attempt: captured decision posted, but the POST failed — no
|
||||
// completion overlay, the session stays reviewable.
|
||||
expect(submissions).toHaveLength(1);
|
||||
expect(submissions[0]!.endpoint).toBe("feedback");
|
||||
expect(submissions[0]!.feedback!.startsWith(ANNOTATE_NO_FEEDBACK_SENTENCE)).toBe(true);
|
||||
expect(submissions[0]!.feedback).toContain("hold the line");
|
||||
const primary = primaryButton();
|
||||
expect(primary).not.toBeNull(); // still reviewable, not submitted
|
||||
|
||||
// Retry via the primary: the armed decision replays its captured route
|
||||
// and framing rather than the bare (now Send Feedback) primary.
|
||||
// Retry via the primary: the armed decision replays with the SAME single
|
||||
// note — never a re-commit, never a dropped note.
|
||||
await act(async () => primary!.click());
|
||||
await settle();
|
||||
|
||||
expect(submissions).toHaveLength(2);
|
||||
const retry = submissions[1]!;
|
||||
expect(retry.endpoint).toBe("feedback");
|
||||
expect(retry.feedback!.startsWith(ANNOTATE_NO_FEEDBACK_SENTENCE)).toBe(true);
|
||||
expect(retry.feedback).toContain("hold the line");
|
||||
const notes = globalComments(retry);
|
||||
expect(notes).toHaveLength(1);
|
||||
expect(notes[0]!.text).toBe("hold the line");
|
||||
});
|
||||
|
||||
// The HTML pinpoint Esc ladder must still receive Escape when the popover
|
||||
|
||||
@@ -2808,7 +2808,7 @@ const App: React.FC = () => {
|
||||
/** Discard flow: every annotation source is dropped, so the builder
|
||||
* emits the legacy zero payload (plus any direct-edit sections). */
|
||||
discardAnnotations?: boolean;
|
||||
/** Positive-finish framing for the non-gated note (spec §3.1). */
|
||||
/** Positive-finish framing for the non-gated discard (spec §3.1). */
|
||||
approvalFraming?: boolean;
|
||||
},
|
||||
): string => {
|
||||
@@ -3686,7 +3686,8 @@ const App: React.FC = () => {
|
||||
/** Discard-and-finish (post-confirm): annotations dropped, the payload is
|
||||
* the legacy "reviewed, no feedback" record. */
|
||||
discardAnnotations?: boolean;
|
||||
/** "Done with a note…" — approval framing on the one feedback string. */
|
||||
/** Approval framing on the one feedback string — the non-gated discard
|
||||
* path only, since the empty-menu collapse removed the framed note. */
|
||||
approvalFraming?: boolean;
|
||||
}): Promise<boolean> => {
|
||||
setIsSubmitting(true);
|
||||
@@ -5023,8 +5024,9 @@ const App: React.FC = () => {
|
||||
if (pendingDecisionSubmit) {
|
||||
// L3: a failed note submit stays armed with its captured route/framing;
|
||||
// the next primary invocation retries THAT decision, never the bare
|
||||
// primary (which would silently reframe "Done with a note" as a change
|
||||
// request once the note raised hasFeedbackToSend).
|
||||
// primary (which would re-derive from live state — dropping a gate's
|
||||
// captured approve route, or re-committing the note — once the note
|
||||
// raised hasFeedbackToSend).
|
||||
dispatchPendingDecision(pendingDecisionSubmit);
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -16,8 +16,10 @@ import type { CompactPlanAction } from "@plannotator/ui/components/PlanHeaderMen
|
||||
export type AnnotateDecisionRoute =
|
||||
| { kind: "primary" }
|
||||
/** Commit the note as a GLOBAL_COMMENT, then submit on the next render
|
||||
* (#1436 mechanism). `route` picks the endpoint; `approvalFraming` marks
|
||||
* the non-gated positive finish ("Done with a note…"). */
|
||||
* (#1436 mechanism). `route` picks the endpoint. `approvalFraming` is kept
|
||||
* for the framing machinery (the App's discard path still frames its
|
||||
* positive finish), but since the empty-menu collapse no spec-emitted note
|
||||
* route sets it — every menu note posts plain, unframed feedback. */
|
||||
| { kind: "note"; route: "feedback" | "approve"; approvalFraming: boolean }
|
||||
/** Direct approve with the live feedback riding along (gate + capability). */
|
||||
| { kind: "approve-with-notes" }
|
||||
@@ -32,13 +34,16 @@ export function resolveAnnotateDecisionAction(
|
||||
case "primary":
|
||||
return { kind: "primary" };
|
||||
case "note-with-approval":
|
||||
// Gate: the note rides the approval body (/api/approve). Non-gate has
|
||||
// no approve channel, so "Done with a note…" posts /api/feedback with
|
||||
// the approval-framing sentence — the only place a menu choice changes
|
||||
// payload text rather than endpoint (spec §3.1 "framing").
|
||||
// Gate: the note rides the approval body (/api/approve). The non-gate
|
||||
// arm is UNREACHABLE from the spec since the empty-menu collapse
|
||||
// (maintainer ruling: the non-gate menu's one composer is
|
||||
// 'request-changes' / "Send a note…"); it stays only because the id
|
||||
// union is shared with the gate. If a stray dispatch ever lands here it
|
||||
// must behave like the collapsed item — plain feedback, and never
|
||||
// fabricated approval framing.
|
||||
return ctx.gate
|
||||
? { kind: "note", route: "approve", approvalFraming: false }
|
||||
: { kind: "note", route: "feedback", approvalFraming: true };
|
||||
: { kind: "note", route: "feedback", approvalFraming: false };
|
||||
case "request-changes":
|
||||
case "note-with-feedback":
|
||||
// The two differ only by state (empty vs feedback), never by transport.
|
||||
|
||||
@@ -231,10 +231,10 @@ describe("annotate approval submission", () => {
|
||||
expect(feedback).toContain("(line 5) ");
|
||||
});
|
||||
|
||||
// Guards the two menu items collapsing into one payload (spec §3.1 framing):
|
||||
// "Done with a note…" and "Request changes…" both post one GLOBAL_COMMENT to
|
||||
// /api/feedback, so only the framing prefix distinguishes approval from a
|
||||
// change request.
|
||||
// Guards the framing machinery the discard path still rides: a framed
|
||||
// payload prefixes the zero-state sentence, an unframed one never does, and
|
||||
// at zero content the frame is idempotent (the discard body stays
|
||||
// byte-identical to the legacy zero payload).
|
||||
test("approval framing prefixes the zero-state sentence before the note", () => {
|
||||
const note: Annotation = {
|
||||
id: "global-note-1",
|
||||
|
||||
@@ -77,12 +77,13 @@ export interface CompleteAnnotateFeedbackInput {
|
||||
savedFileChangesSection: string;
|
||||
messageEntries?: MessageAnnotationEntry[];
|
||||
/**
|
||||
* Positive-finish framing ("Done with a note…"). Non-gated annotate has no
|
||||
* approve channel — every outcome is one feedback string — so without this
|
||||
* the approval-note and change-request menu items would post byte-identical
|
||||
* bodies. This is the ONLY place a menu choice changes payload text rather
|
||||
* than endpoint: it prefixes the zero-state sentence before the note's
|
||||
* global-comment section (idempotent when the text already is the sentence).
|
||||
* Positive-finish framing. Non-gated annotate has no approve channel —
|
||||
* every outcome is one feedback string — so this prefixes the zero-state
|
||||
* sentence before the annotation sections (idempotent when the text already
|
||||
* is the sentence). Since the empty-menu collapse (the maintainer merged
|
||||
* "Done with a note…" into the single unframed "Send a note…"), the only
|
||||
* caller is the non-gated discard path, which frames its positive finish
|
||||
* over any direct edits that still ride along.
|
||||
*/
|
||||
approvalFraming?: boolean;
|
||||
}
|
||||
|
||||
@@ -50,14 +50,23 @@ describe("annotate decision handler exhaustiveness", () => {
|
||||
// Guards the endpoint matrix (spec §3.1/§6.1): Done and every note stay on
|
||||
// /api/feedback so formatAnnotateOutcome shapes and strict-gate exit codes
|
||||
// are untouched; only gate-mode approvals reach /api/approve.
|
||||
test("note and discard routes follow the gate's transport, framing only on the non-gated positive finish", () => {
|
||||
test("note and discard routes follow the gate's transport; no menu note ever carries approval framing", () => {
|
||||
const gated = { gate: true };
|
||||
const ungated = { gate: false };
|
||||
|
||||
expect(resolveAnnotateDecisionAction("note-with-approval", gated))
|
||||
.toEqual({ kind: "note", route: "approve", approvalFraming: false });
|
||||
// The non-gate arm is dead code by construction since the empty-menu
|
||||
// collapse. Assert the unreachability itself (so this pin cannot pass
|
||||
// vacuously over a route the spec quietly resurrects)…
|
||||
for (const input of annotateInputs()) {
|
||||
if (input.gate) continue;
|
||||
expect(buildDecisionSpec(input).items.map((item) => item.id))
|
||||
.not.toContain("note-with-approval");
|
||||
}
|
||||
// …and pin that even a stray dispatch cannot fabricate approval framing.
|
||||
expect(resolveAnnotateDecisionAction("note-with-approval", ungated))
|
||||
.toEqual({ kind: "note", route: "feedback", approvalFraming: true });
|
||||
.toEqual({ kind: "note", route: "feedback", approvalFraming: false });
|
||||
|
||||
for (const ctx of [gated, ungated]) {
|
||||
// The two differ only by state, never by transport or framing.
|
||||
|
||||
@@ -31,7 +31,7 @@ export const planEditorShortcuts = defineShortcutScope({
|
||||
displayOrder: 10,
|
||||
},
|
||||
submitAnnotations: {
|
||||
description: 'Done / Send feedback — whichever the header shows',
|
||||
description: 'Done / Send feedback, whichever the header shows',
|
||||
bindings: ['Mod+Enter'],
|
||||
section: 'Actions',
|
||||
hint: 'Fires the adaptive header primary: Done (or Approve in gate mode) with nothing to send, Send Feedback otherwise.',
|
||||
|
||||
@@ -20,7 +20,7 @@ export const reviewEditorShortcuts = defineShortcutScope({
|
||||
title: 'Review Editor',
|
||||
shortcuts: {
|
||||
submit: {
|
||||
description: 'Approve / Send feedback — whichever the header shows',
|
||||
description: 'Approve / Send feedback, whichever the header shows',
|
||||
bindings: ['Mod+Enter'],
|
||||
section: 'Actions',
|
||||
hint: 'Fires the adaptive header primary: Approve with no annotations, Send Feedback otherwise.',
|
||||
|
||||
@@ -101,7 +101,9 @@ describe('shortcuts', () => {
|
||||
|
||||
expect(getShortcut(planReviewSettingsShortcutRegistry, 'plan-review-editor-settings', 'submitPlan')?.description).toBe('Approve / Send feedback');
|
||||
expect(getShortcut(planReviewSettingsShortcutRegistry, 'plan-review-editor-settings', 'submitAnnotations')).toBeUndefined();
|
||||
expect(getShortcut(annotateSettingsShortcutRegistry, 'annotate-editor-settings', 'submitAnnotations')?.description).toBe('Done / Send feedback — whichever the header shows');
|
||||
// Fact-guard, not a prose pin: the annotate registry's submit action must
|
||||
// describe the adaptive Done/Send primary (distinct from plan review's).
|
||||
expect(getShortcut(annotateSettingsShortcutRegistry, 'annotate-editor-settings', 'submitAnnotations')?.description).toContain('Done / Send feedback');
|
||||
expect(getShortcut(annotateSettingsShortcutRegistry, 'annotate-editor-settings', 'submitPlan')).toBeUndefined();
|
||||
expect(getShortcut(annotateSettingsShortcutRegistry, 'annotate-sidebar', 'toggleContents')?.description).toBe('Toggle Contents sidebar');
|
||||
|
||||
|
||||
@@ -24,7 +24,7 @@ function itemIds(spec: DecisionSpec): string[] {
|
||||
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', () => {
|
||||
it('annotate, no feedback, no gate → Done + the single Send a note composer', () => {
|
||||
const spec = buildDecisionSpec({
|
||||
app: 'annotate', gate: false, count: 0, hasFeedback: false, approvalNotesSupported: false,
|
||||
});
|
||||
@@ -33,16 +33,13 @@ describe('buildDecisionSpec state matrix', () => {
|
||||
// it must never wear the success tone or check icon Approve wears.
|
||||
expect(spec.primary.tone).toBe('neutral');
|
||||
expect(spec.primary.icon).toBeUndefined();
|
||||
expect(itemIds(spec)).toEqual(['note-with-approval', 'request-changes']);
|
||||
// Frozen copy (maintainer-approved): 'Request changes…'.
|
||||
expect(spec.items[1].label).toBe('Request changes…');
|
||||
// "Done with a note…" posts /api/feedback — never capability-gated. The
|
||||
// two composers must stay DISTINCT actions (positive finish vs change
|
||||
// request) — the labels themselves are free prose.
|
||||
expect(spec.items[0].composer?.tone).toBe('neutral');
|
||||
expect(spec.items[1].dividerBefore).toBe(true);
|
||||
expect(spec.items[1].composer?.tone).toBe('primary');
|
||||
expect(spec.items[0].composer?.actionLabel).not.toBe(spec.items[1].composer?.actionLabel);
|
||||
// Maintainer ruling (empty-menu collapse): ONE composer item — the old
|
||||
// "Done with a note…" / "Request changes…" pair differed only by framing
|
||||
// on the same transport and must not come back. Label is free prose.
|
||||
expect(itemIds(spec)).toEqual(['request-changes']);
|
||||
expect(spec.items[0].composer).toBeDefined();
|
||||
expect(spec.items[0].composer?.tone).toBe('primary');
|
||||
expect(spec.items[0].dividerBefore).toBe(false);
|
||||
});
|
||||
|
||||
it('annotate, no feedback, gate → Approve; approve-note item only with the capability', () => {
|
||||
@@ -105,8 +102,6 @@ describe('buildDecisionSpec state matrix', () => {
|
||||
|
||||
expect(delivered.primary.label).toBe('Done'); // frozen copy, maintainer-approved
|
||||
expect(delivered.primary.title).not.toContain('no feedback');
|
||||
const deliveredNote = delivered.items.find((item) => item.id === 'note-with-approval')!;
|
||||
expect(deliveredNote.subtitle).not.toContain('no feedback');
|
||||
// The two states must actually differ — a regression that ignores the
|
||||
// flag would silently restore the lying tooltip.
|
||||
expect(delivered.primary.title).not.toBe(plain.primary.title);
|
||||
@@ -167,13 +162,53 @@ describe('buildDecisionSpec invariants', () => {
|
||||
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');
|
||||
}
|
||||
});
|
||||
|
||||
// Maintainer ruling (empty-menu collapse): without a gate there is no
|
||||
// approval channel, so no approve-carrying id may appear in any non-gate
|
||||
// annotate state, capability advert or not — this is also what keeps the
|
||||
// non-gate 'note-with-approval' arm in annotateDecision.ts dead code.
|
||||
it('non-gate annotate never emits an approve-carrying item', () => {
|
||||
for (const input of allInputs()) {
|
||||
if (input.app !== 'annotate' || input.gate) continue;
|
||||
const ids = itemIds(buildDecisionSpec(input));
|
||||
expect(ids).not.toContain('note-with-approval');
|
||||
expect(ids).not.toContain('approve-with-notes');
|
||||
}
|
||||
});
|
||||
|
||||
// Maintainer ruling: no user-facing decision-control string carries an em
|
||||
// dash. Sweeps every field the control renders, across both arms.
|
||||
it('no user-facing string contains an em dash', () => {
|
||||
const inputs: DecisionSpecInput[] = [
|
||||
...allInputs(),
|
||||
...([0, 1, 3] as const).flatMap((count) =>
|
||||
[false, true].map((selfAuthored): DecisionSpecInput => ({
|
||||
app: 'review', gate: true, count, hasFeedback: count > 0,
|
||||
approvalNotesSupported: false,
|
||||
platform: { label: 'GitHub', mrLabel: 'PR', selfAuthored },
|
||||
}))),
|
||||
...allInputs().map((input) => ({ ...input, feedbackDelivered: true })),
|
||||
];
|
||||
for (const input of inputs) {
|
||||
const spec = buildDecisionSpec(input);
|
||||
const strings = [
|
||||
spec.primary.label, spec.primary.shortLabel, spec.primary.mobileLabel,
|
||||
spec.primary.title,
|
||||
...spec.items.flatMap((item) => [
|
||||
item.label, item.subtitle,
|
||||
item.composer?.title, item.composer?.actionLabel, item.composer?.placeholder,
|
||||
item.confirm?.title, item.confirm?.message, item.confirm?.confirmText,
|
||||
]),
|
||||
];
|
||||
for (const value of strings) expect(value ?? '').not.toContain('—');
|
||||
}
|
||||
});
|
||||
|
||||
// Guards a refactor that drops the one remaining guard dialog.
|
||||
it('every discard item carries a confirm', () => {
|
||||
for (const input of allInputs()) {
|
||||
|
||||
@@ -10,14 +10,16 @@
|
||||
* 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.
|
||||
* Labels, subtitles and confirm strings are the approved prototype's
|
||||
* (DESIGN_final-proposal.html `spec()`), authoritative over any older branch
|
||||
* or mock copy — except where a later maintainer ruling supersedes it: the
|
||||
* non-gate empty menu carries ONE composer ("Send a note…"), and no
|
||||
* user-facing string uses an em dash.
|
||||
*/
|
||||
|
||||
export type DecisionActionId =
|
||||
| 'primary' // the left segment
|
||||
| 'note-with-approval' // "Done with a note…" / "Approve with a note…"
|
||||
| 'note-with-approval' // "Approve with a note…" (approval flows only)
|
||||
| 'request-changes' // "Request changes…"
|
||||
| 'note-with-feedback' // "Send with a note…"
|
||||
| 'approve-with-notes' // review + gate-annotate; capability-gated
|
||||
@@ -139,74 +141,73 @@ function annotationNoun(count: number): string {
|
||||
* 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',
|
||||
// "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). Non-gate annotate has
|
||||
// no approve channel and no positive-note item at all: the maintainer ruled
|
||||
// the old "Done with a note…" / "Request changes…" pair collapsed into the
|
||||
// single "Send a note…" below, because their only difference was framing on
|
||||
// the same /api/feedback transport.
|
||||
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 and send note',
|
||||
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…',
|
||||
// Maintainer ruling (post-demo): without a gate there IS no approval —
|
||||
// Done is a positive finish, not an approve — so this row must not
|
||||
// wear the approval costume (success tone + check). Neutral tone,
|
||||
// send icon, and copy that never says "approval". Free prose, NOT
|
||||
// frozen. (M1: the delivered variant names the session record.)
|
||||
subtitle: input.feedbackDelivered
|
||||
? 'Finish and send a short note with the session record'
|
||||
: 'Finish and send a short note with it',
|
||||
tone: 'neutral',
|
||||
placeholder: DECISION_NOTE_PLACEHOLDER,
|
||||
},
|
||||
}
|
||||
: null;
|
||||
|
||||
const requestChanges: DecisionMenuItem = approvalFlow
|
||||
? {
|
||||
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: 'Done with a note',
|
||||
actionLabel: 'Done — send note',
|
||||
tone: 'neutral',
|
||||
title: 'Request changes',
|
||||
actionLabel: 'Send as feedback',
|
||||
tone: 'primary',
|
||||
icon: 'send',
|
||||
placeholder: DECISION_NOTE_PLACEHOLDER,
|
||||
},
|
||||
}
|
||||
: {
|
||||
// Maintainer ruling (empty-menu collapse): the one non-gate composer.
|
||||
// Same id and route as the old change request (plain /api/feedback,
|
||||
// no approval framing) — only the copy is new. Free prose, NOT frozen.
|
||||
id: 'request-changes',
|
||||
label: 'Send a note…',
|
||||
subtitle: 'Write a note and send it as feedback',
|
||||
tone: 'primary',
|
||||
icon: 'send',
|
||||
dividerBefore: false,
|
||||
composer: {
|
||||
title: 'Send a note',
|
||||
actionLabel: 'Send as feedback',
|
||||
tone: 'primary',
|
||||
icon: 'send',
|
||||
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',
|
||||
title: 'Approve: no changes requested',
|
||||
tone: 'success',
|
||||
icon: 'check',
|
||||
}
|
||||
@@ -219,8 +220,8 @@ function buildEmptySpec(input: DecisionSpecInput, approvalFlow: boolean): Decisi
|
||||
// on stdout may never have seen the terminal delivery), so the
|
||||
// tooltip must not claim "no feedback". Free prose, NOT frozen.
|
||||
title: input.feedbackDelivered
|
||||
? 'Finish — sends the session record (feedback already shared in the terminal)'
|
||||
: 'Finish — records that you reviewed with no feedback',
|
||||
? 'Finish: sends the session record (feedback already shared in the terminal)'
|
||||
: 'Finish: records that you reviewed with no feedback',
|
||||
// Maintainer ruling (post-demo): Done without a gate is a positive
|
||||
// finish, NOT an approval — no success tone, no check icon, so it
|
||||
// can never be mistaken for the gate/review Approve.
|
||||
@@ -292,7 +293,7 @@ function buildFeedbackSpec(input: DecisionSpecInput, approvalFlow: boolean): Dec
|
||||
label: approvalFlow
|
||||
? `Approve, discard ${count} ${noun}…`
|
||||
: `Done, discard ${count} ${noun}…`,
|
||||
subtitle: 'Asks to confirm — the annotations are not sent',
|
||||
subtitle: 'Asks to confirm: the annotations are not sent',
|
||||
tone: 'destructive',
|
||||
icon: 'check',
|
||||
dividerBefore: dividerPending,
|
||||
@@ -354,7 +355,7 @@ function buildPlatformSpec(input: DecisionSpecInput, platform: DecisionPlatformI
|
||||
id: 'primary',
|
||||
// Frozen copy (maintainer-approved): 'Approve'.
|
||||
label: 'Approve',
|
||||
title: selfAuthored ? selfReason : 'Approve - no changes needed',
|
||||
title: selfAuthored ? selfReason : 'Approve: no changes needed',
|
||||
tone: 'success',
|
||||
icon: 'check',
|
||||
...(selfAuthored ? { muted: true } : {}),
|
||||
@@ -376,7 +377,7 @@ function buildPlatformSpec(input: DecisionSpecInput, platform: DecisionPlatformI
|
||||
id: 'request-changes',
|
||||
// Frozen copy (maintainer-approved): 'Request changes…'.
|
||||
label: 'Request changes…',
|
||||
subtitle: 'Overall feedback, zero line comments — via the dialog',
|
||||
subtitle: 'Overall feedback, zero line comments, via the dialog',
|
||||
tone: 'primary',
|
||||
icon: 'send',
|
||||
dividerBefore: true,
|
||||
@@ -409,7 +410,7 @@ function buildPlatformSpec(input: DecisionSpecInput, platform: DecisionPlatformI
|
||||
{
|
||||
id: 'note-with-feedback',
|
||||
label: 'Post comments, then…',
|
||||
subtitle: 'Request changes / stay neutral — chosen in the dialog',
|
||||
subtitle: 'Request changes or stay neutral, chosen in the dialog',
|
||||
tone: 'primary',
|
||||
icon: 'send',
|
||||
dividerBefore: true,
|
||||
|
||||
Reference in New Issue
Block a user