Skip to content

feat(ui): decision-control primitives (PR1, nothing mounted) - #1452

Closed
backnotprop wants to merge 6 commits into
mainfrom
feat/decision-control-pr1
Closed

feat(ui): decision-control primitives (PR1, nothing mounted)#1452
backnotprop wants to merge 6 commits into
mainfrom
feat/decision-control-pr1

Conversation

@backnotprop

Copy link
Copy Markdown
Owner

PR1: Decision-control primitives (nothing mounted)

First PR of the unified decision-control project (DESIGN_decision-control.md §7 "PR1").
Adds the shared primitives to packages/ui; no app mounts them yet. Zero changes to
packages/editor, packages/review-editor, or any server.

What ships

  • packages/ui/utils/decisionSpec.ts: pure buildDecisionSpec({ app, gate, count, hasFeedback, approvalNotesSupported }) returning { primary, items[] }. Labels,
    subtitles and confirm strings are verbatim from the approved prototype
    (DESIGN_final-proposal.html, authoritative over the older branch/mock labels).
    approvalNotesSupported gates every approve-carrying item (spec §2.2, single
    mechanism). Frozen copy (Done, Approve, Send Feedback, Request changes…,
    Approve with notes, Discard & finish, Discard & approve) is marked inline.
  • packages/ui/components/DecisionControl.tsx: the joined split control
    (DecisionControl + DecisionNoteField + DecisionNoteDialog). The incumbent primary
    segment never opens the popover and deliberately never fades while it is open (audit
    finding 3 reversed, rationale in a code comment). The caret popover renders
    role="menu" rows over ActionMenuItem with component-implemented roving focus
    (Up/Down wrap, Home/End), an in-place composer morph (same element, children swap),
    the three-rung Esc ladder (composer to menu keeping the draft, menu to close focusing
    the caret, else propagate; consumed on exactly the two acting rungs), Mod+Enter submit
    with Enter-is-newline, empty-note no-op-and-refocus (the action is never
    disabled-gray), one ConfirmDialog path for discard-and-finish with cancel returning
    to the open menu, and busy closing everything.
  • packages/ui/hooks/useDismissablePopover.ts: shared pointerdown-outside plus Escape
    dismissal with the framed-surface strategy (dismissOnIframeFocus: window blur, then a
    next-task activeElement iframe check), spec §2.4. Any Escape it acts on is consumed
    (preventDefault + stopPropagation at document, which bubbles before the apps'
    window-level ladders) and defaultPrevented events are skipped, so one Escape is never
    both "close the menu" and a rung of a host ladder. An outside-focus Escape while the
    composer is open closes the whole popover (it is an outside-dismissal gesture, like an
    outside click); the draft is kept either way. Commented at the dismiss call site and
    pinned by a DOM test.
  • packages/ui/shortcuts/decisionControl.shortcuts.ts: scope decision-control at the
    shortcuts root (spec §4: both apps, identical semantics), documenting only the two
    chords the control implements: Mod+Enter submit-note and Escape step-back. Enter is
    deliberately not documented as submit anywhere.
  • Additive props with defaults byte-identical for existing callers: ActionMenuItem
    role?: 'menuitem' + className?, and ExitButton appearance?: 'pill' | 'ghost'
    (ghost keeps its aria-label; the pinned default-label test still passes).
  • Tests: decisionSpec.test.ts (pure lane, full state matrix and invariant sweeps),
    DecisionControl.test.tsx (DOM lane, registered in the CI
    Run UI seam-contract + DOM tests file list per spec §8B so it cannot silently skip),
    and a shortcuts.test.ts guard that the new scope composes into both adopting
    registries ahead of PR2/PR3.
  • @plannotator/ui 0.37.0 → 0.38.0 (spec §2.1 cost note and §9, "batch the bump into
    PR1"); smoke:package verified the tarball. One-line version prose updates in
    README/HANDOFF.

Support-surface stance (one asymmetry worth naming)

The component, hook and util files are deliberately not host-supported surface. Like
ActionMenu/ConfirmDialog, they are absent from the README supported-import list and
tsconfig.strict-consumer.json, and the only HANDOFF change is one bullet in the
"Unsupported" list (spec §2.1). One asymmetry: decisionControlShortcuts is exported
from the blessed @plannotator/ui/shortcuts barrel, because spec §4 places every scope
there (the registry entries feed the in-app help modal and the generated marketing
shortcuts page). The scope is documentation data, not a backend seam; no
configurePlannotatorUI seam is added anywhere.

Self-flagged decisions, with review rulings

  1. className? on ActionMenuItem. Spec §2.3 names one additive prop (role) but
    also demands "Destructive rows get the existing destructive text token", and
    ActionMenuItem had no tone/styling vector at all. Ruling: justified. A dedicated
    tone?: prop was considered and set aside as the larger API for the same one class
    string; className (default undefined) keeps existing consumers byte-identical.
  2. No platform arm on DecisionSpecInput. The task brief asked for "platform state
    in the type model"; spec §2.2's input type has no such field and §3.4 maps platform
    (PR) mode onto the same output model in PR6 (composer-less items, Post Comments
    primary, all expressible today). Ruling: spec wins; the output types already carry
    the platform shape, and the input grows an arm only if PR6 needs one.
  3. Count-gating of discard-and-finish / approve-with-notes. See "Open item"
    below; left as implemented pending a maintainer ruling.
  4. Version bump in PR1. Spec §2.1 ("the PR that lands this bumps
    packages/ui/package.json") and §9 ("batch the bump into PR1 and PR7"). Minor per
    the 0.36.0/0.37.0 precedent: new files land in the published tarball even though they
    are not supported surface. bun.lock deliberately untouched (bump-commit precedent
    496a1fd1).

Open item awaiting maintainer ruling (F2)

The count: 0 + hasFeedback: true cell (annotate sessions whose feedback is direct
edits, saved-file changes, or attachments, with no annotations): the spec's matrix keys
on "feedback (n)" and never addresses it. As implemented, the primary is Send Feedback
with no count pill, Send with a note… renders with a countless subtitle, and both
discard-and-finish ("discard 0 annotations…") and approve-with-notes ("your 0
annotations ride along") are omitted. If the maintainer wants an approve or discard path
for direct-edit-only sessions, that is a one-conditional change plus copy for the zero
form.

Handoff notes for PR2/PR3

  • F6 (PR2/PR3 must handle): if the live spec prop changes while the composer is
    open and the active item id disappears from the new spec (e.g. an annotation delete
    flips feedback to empty), the popover renders an empty shell until Escape. The control
    keeps state by item id on purpose (drafts survive spec updates); the adopting apps
    should close or morph the popover gracefully when the active item leaves the spec, or
    PR2 adds that effect to the control itself.
  • F8 (PR3 should flag): Done/Approve primaries follow the FeedbackButton
    responsive pattern (icon-only below the label breakpoint) where the old
    ApproveButton showed a compact OK label. The tooltip (title) carries the
    accessible name. Narrow-desktop reviewers will see a visual change; mobileLabel
    ("Send feedback" etc.) is in the spec model for the compact rows PR2/PR3 generate.
  • The decision-control scope must be added to annotateSettingsShortcutRegistry and
    reviewSettingsShortcutRegistry in the adopting PRs (spec §4); the shortcuts.test.ts
    guard already proves the composition is collision-free.

AI-assisted (Claude) under maintainer direction.

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.
@backnotprop

Copy link
Copy Markdown
Owner Author

Superseded by #1458, which carries this branch's commits as the first stage of the integrated stack.

@backnotprop backnotprop closed this Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant