Skip to content

feat: make approval mode discoverable from the approval bar - #5942

Merged
bolichen97 merged 1 commit into
mainfrom
feat/approval-mode-discoverability
Aug 26, 2026
Merged

feat: make approval mode discoverable from the approval bar#5942
bolichen97 merged 1 commit into
mainfrom
feat/approval-mode-discoverability

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

New users who hit the chat approval bar only see Allow once / Trust / Reject. Nothing tells them that approval prompting itself is adjustable (the Normal / Reads / Trust / YOLO picker in the composer footer), so repeated prompts read as unavoidable friction — the picker is effectively undiscoverable at the exact moment its existence matters.

Why it matters

Approval fatigue is a first-session churn driver: a user who does not find the mode picker either rubber-stamps every prompt (annoyance) or gives up on agentic work. Users who DO know the picker exists get no cost; discoverability only has to fire once.

What changed (motivation → approach → change)

Goal: teach where the real control lives, without re-adding an on-card trust decision (the chat approval endpoint is one-shot; an on-card Trust tier was deliberately removed in #5433/#5434, so a fake affordance is off the table).

  • A1 — hint row on the approval bar (ChatInput.tsx): a muted "Tired of confirming every step?" + accent link "Adjust approval mode↗" under the action row. Withheld for unattended sources (cron/heartbeat/taskrunner — the picker governs this slot, not the job), in the ghost/collapsed composer state (the picker is unmounted there, so the link would be a dead end), while the B2 callout is up (one pointer at a time), and permanently once the user has found the picker (via the link, or by changing the mode).
  • A2 — the link opens the real picker (ApprovalModePicker.tsx gains an openSignal prop): the footer picker opens with a 2s spotlight ring on its trigger, so the user learns the control's home rather than a one-off popup.
  • B2 — threshold nudge: after 3 manual one-shot approvals in one slot while the mode is still normal, a one-time role="dialog" callout anchors above the picker ("Want fewer approval prompts?") with "See options" (opens the picker) and "Got it". Either action, or picking a mode, dismisses it forever (localStorage). Counting is in-memory per dashboard session by design — stale history should not fire it.

The callout is a Radix Popover/PopoverAnchor/PopoverContent (portalled, collisionPadding={8}) — the same collision machinery the picker's own menu already uses — because the composer control row is a horizontally scrollable container that clips absolute children, and an earlier hand-rolled fixed-position draft overflowed 390px viewports. Keyboard path: focus capture is deliberately suppressed while an editable element has focus (the callout mounts from an async approval completion, so the user may be mid-word in the composer), Escape dismisses, and dismissal returns focus to the picker trigger. The trigger wears the spotlight ring while the callout is up, anchoring the "this is where" copy visually. ANY menu open — trigger click included — or either button dismisses the callout for good (and permanent dismissal also retires the A1 hint: the callout has delivered the lesson). Escape or clicking anywhere outside hides it for the sitting only — reflexive gestures must not spend the one-time teaching moment unseen (verified end-to-end: the capture harness clicks outside, asserts the callout hides, then re-fires on the next approval). One localStorage flag carries both retirements (hint + callout).

New i18n keys (approval_hint_question, approval_hint_adjust, nudge_title, nudge_body, nudge_dismiss, nudge_view_options) across all 13 catalogs with {{productName}} interpolation; en-XA regenerated. The nudge body says "Choosing "Trust" here…" to disambiguate from the approval bar's own per-command Trust ▾ button.

A pre-push review round (GPT + Opus mirrors + a new-user usability reviewer) drove seven fixes now in this diff: the narrow-viewport clamp, no-op re-pick not retiring the hint, dialog autoFocus + Escape, an owned nudge_dismiss key instead of borrowing markdownPanel.got_it, the "here" copy anchor, hint retirement on link click, and hint suppression while the callout is up.

Tests

website/src/test/ApprovalModePicker.nudge.test.tsx and website/src/test/ChatInput.approvalNudge.test.tsx (28 tests total), mutation-verified:

  • A1: hint renders with the pending approval; withheld for unattended sources / after adjustment; clicking it opens the picker menu and writes the retirement flag; suppressed while the callout is up.
  • A2: openSignal bump opens the menu + spotlight ring; ring clears on timeout; a mount-time signal does not replay on slot-switch remount.
  • B2: fires on the 3rd manual approval (not the 2nd), not when the mode is no longer normal, not once dismissed; "See options" / "Got it" / Escape / ANY menu open (trigger click included, so a closed menu cannot resurrect the callout) each dismiss; focus is NOT stolen while an editable element has focus; the trigger wears the spotlight ring while the callout is up.
  • Narrow viewports: the capture harness asserts the callout's bounding box stays fully inside a 390px viewport (Radix collision handling), frame 04-nudge-mobile-clamp.
  • Retirement: merely opening the picker writes the flag (discovery achieved even when the user deliberately stays on Normal).

Manual verification

Self-checking capture harness (website/capture/approval-mode-discover.* + website/scripts/capture-approval-mode-discover.mjs): mounts the real ChatInput with a seeded pending approval, drives the REAL hint click and three REAL "Allow once" clicks (API answered by route interception), asserts each state before writing a frame. 7/7 frames pass, including a 390px bounding-box containment check for the clamp.

Screenshots / video

A1 hint row on the approval bar (dark)

A2 clicking the hint opens the spotlighted picker (dark)

B2 one-time callout after three manual approvals (dark)

Light theme + 390px narrow-viewport clamp

A1 hint (light)

A2 spotlight (light)

B2 callout (light)

B2 callout fully contained on a 390px viewport

Related Issues

no linked issue: product improvement chosen from design mockups by the maintainer in-session; no tracking issue was filed.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — N/A, no doc-owned behavior changed
  • No secrets, credentials, or internal references in the diff

@CrysisDeu
CrysisDeu requested a review from a team August 25, 2026 20:43
@CrysisDeu
CrysisDeu requested a review from a team as a code owner August 25, 2026 20:43
@CrysisDeu
CrysisDeu requested a review from Zedmor August 25, 2026 20:43
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Pre-push review round (local GPT + Opus lane mirrors + a new-user usability reviewer) on pre-fix head 2b503fa57 — all blocking findings fixed in 918b20e (see PR body). Declined findings, each with rationale:

  • rebutted (Opus advisory, localStorage read per render in ChatInput): the read is gated on a pending approval existing (!!pendingApproval &&), so it does not run on the ordinary typing path; when it does run it is a sub-microsecond same-process key lookup. Caching it in a ref keyed on approvalId would add state for no measured cost — disproportional to this PR's scope.
  • accepted-and-deferred (usability, compact trigger is an unlabeled icon after the spotlight fades): real but optional polish; un-compacting the trigger while spotlighted touches the mobile control-row layout and deserves its own change if wanted.
  • accepted-and-deferred (usability, Trust consequence wording could name 'run commands and change files'): the picker's own per-mode descriptions carry the consequence and the YOLO confirm gate is unchanged; tightening nudge copy further is a wording call for the maintainer.

@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 25, 2026
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 715a4a8d6a38f615898111200155e08c74eceead and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 715a4a8

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 715a4a8d6a38f615898111200155e08c74eceead: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 715a4a8d6a38f615898111200155e08c74eceead — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A real first-session friction point, solved with layered, self-retiring UI that adds no backend surface and is fully reversible (two components + one localStorage flag).

The hint/nudge/spotlight trio could have been noise, but the mutual suppression (one pointer at a time), the single shared retirement flag, and the forever-vs-sitting dismissal split are all deliberate, documented in-code, and pinned by tests — the intricate part of this design is exactly the part with the strongest coverage. Capture harness and screenshots follow the repo's established convention. No contract, schema, or one-way-door changes.

[DESIGN-REVIEWED] 715a4a8

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 715a4a8d6a38f615898111200155e08c74eceead — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have everything I need. The capture harness and committed screenshots follow a documented repo convention (2,393 files under temp-screenshots/, 262 sibling capture scripts, a cleanup workflow); the i18n fan-out is mandated by the CI-gated 12-language invariant; AutoNudgePopover/NudgeCard are a different domain (auto-nudge loops), so B2 duplicates nothing pre-existing. The findings that survive are below.

First-Principles-Verdict: CONCERNS

The B2 threshold nudge is a second pointer at the same control the A1 hint already points at, on an unsupported requirement — no report, no issue, "design mockups… in-session".

What this change ships

Intent: teach new users, at the moment of approval friction, that prompting is adjustable via the footer mode picker. ADDITION.

  1. "Tired of confirming every step? Adjust approval mode" row under the approval bar — justified
  2. That link opens the real footer picker with a 2s spotlight ring — justified
  3. One-time dialog callout after 3 manual approvals in a slot — inherited provenance, overlaps item 1
  4. New permanent localStorage flag mc-approval-mode-adjusted retiring hint + callout — justified, matches existing mc-* flags
  5. Merely opening the picker (any route) now writes that flag — declared, justified
  6. Escape/outside-click hides the callout per-sitting; buttons/menu-open dismiss forever — declared; fallback branch has zero consumers
  7. Picker trigger wears the spotlight ring while the callout is up — declared
  8. Six i18n keys × 13 catalogs — mandated by the CI-gated i18n invariant
  9. Capture harness + committed screenshots — documented temp-screenshots/ convention

Watch

Item 3's zero option: during all three counted approvals the A1 hint ("Tired of confirming every step?") is already on the approval bar — the description itself has A1 fire on the first approval and retire only on discovery, so B2 escalates a pointer the user has now seen three times. The only incremental harm is banner-blindness, and the description concedes there is no fundamental behind it: "no linked issue: product improvement chosen from design mockups by the maintainer in-session." B2 carries most of the PR's permanent surface — the Popover callout, dual dismiss/hide semantics, APPROVAL_NUDGE_THRESHOLD, the per-slot counter, and 4 of the 6 i18n keys × 13 catalogs.

Subtractions

  • Drop the ?? onNudgeDismiss fallback in ApprovalModePicker's onEscapeKeyDown/onInteractOutside: both shipped mounts pass onNudgeHide (2 call sites, ChatInput.tsx:3281/3302; 0 consumers of the fallback — only the test "Escape falls back…" pins it). Make onNudgeHide the single handler.
  • Defer B2 (the callout, nudge/onNudgeDismiss/onNudgeHide props, threshold counter, its 4 i18n keys): ship A1+A2 first and let a reported failure of the always-visible hint justify the escalation tier.

[FIRST-PRINCIPLES-REVIEWED] 715a4a8

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 715a4a8d6a38f615898111200155e08c74eceead — this comment is updated in place on each push.

Review details

I've verified the core logic paths myself: activeSlot === slotId (ChatInput.tsx:848) makes the tally key and the approvalNudgeActive comparison consistent; handleApprovalAction's deps now include approvalMode, so the normal-gate uses no stale value; {{productName}} is an established i18next default variable used throughout the catalog, so nudge_body interpolates; openSignal's lastSignal ref seeds from the current value on mount, so a slot-switch remount replays nothing; all 12 shipped locales plus the pseudo-locale received both key groups. Nothing survives to the 80+ bar.

No findings.

[OPUS-REVIEWED] 715a4a8

Verdict parsed from the review's SHA-scoped output markers for commit 715a4a8d6a38f615898111200155e08c74eceead.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 715a4a8d6a38f615898111200155e08c74eceead: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of 715a4a8d6a38f615898111200155e08c74eceead — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: CONCERNS

Solid teaching flow — but outside-click only hides the nudge for the sitting, so the callout re-summons itself on every subsequent approval.

Watch

  • Re-fire loop after outside-click dismissal. onInteractOutside={() => (onNudgeHide ?? onNudgeDismiss)?.()} hides for the sitting, and the tally (n >= APPROVAL_NUDGE_THRESHOLD with no per-sitting fire cap) re-arms on the very next manual approval — so a user who dismisses the unsolicited callout with the most common gesture (clicking anywhere else) gets re-interrupted on every approval until they find the buttons or open the menu. Frequency: every approval in the sitting for outside-click dismissers × friction × sitting-scoped — notable, not blocking. Smallest fix: cap the nudge to one re-show per sitting (or treat a second outside-click as "Got it").

Suggestions

  • Screenshot 03/04: the callout fully covers the assistant's in-progress message bubble; nudging sideOffset/align so it clears the last transcript line would keep the teaching moment from hiding what the user is reading.

[UX-REVIEWED] 715a4a8

@CrysisDeu
CrysisDeu force-pushed the feat/approval-mode-discoverability branch from 918b20e to 384b208 Compare August 25, 2026 21:38
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

First Principles CONCERNS on 918b20e, per finding:

  • fixed (Subtraction 1 — hand-rolled callout positioning duplicates ui/popover.tsx): adopted in 384b208. createPortal, the measure-on-resize/scroll effect, clampNudgeLeft, nudgeArrowLeft and their geometry tests are deleted; the callout is now a Radix Popover/PopoverAnchor/PopoverContent (portalled, collisionPadding={8}), the same machinery this component already trusts for its menu. The 390px capture assertion (04-nudge-mobile-clamp) now pins Radix's collision handling instead of hand math.
  • rebutted (Subtraction 2 — defer B2 until A1 is shown insufficient): B2 ships by explicit maintainer product decision — the A1+A2+B2 combination was chosen from rendered design mockups in-session before implementation, which is this repo's decision gate for user-visible features. The two mechanisms serve different moments: A1 is ambient (visible only while an approval card is up, muted, ignorable); B2 is the one-time intervention at the measured friction point for exactly the user who has not acted on A1 three times over. The threshold of 3 is a product judgment call, stated as such in the code comment, not presented as derived data. Cost after Subtraction 1 is one counter ref, one flag, and one Radix popover.

B2's audience is asserted, not derived … B2's delta is most of the diff
The clipping rationale for the hand-rolled portal … is exactly what a Radix portal solves

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

UX CONCERNS on 918b20e, per finding:

  • fixed (hint never retires for users who deliberately stay Normal): in 384b208 the retirement flag is written on ANY picker open (onOpenChange(true)), so opening the menu and confirming Normal retires the hint permanently. Pinned by the 'merely OPENING the picker retires the approval-bar hint' test.
  • fixed (nudge autoFocus can steal mid-typing keystrokes): the Radix onOpenAutoFocus handler now prevents focus capture whenever an editable element (textarea/input/contentEditable) has focus. Pinned by the 'does not steal focus from an editable element' test.
  • fixed (focus dropped on dismissal): onCloseAutoFocus returns focus to the picker trigger, except when the dismissal itself opened the menu (which manages its own focus).
  • fixed (ArrowUpRight promises external navigation): icon removed from the hint link; the label alone reads as an in-place action.

The hint never retires for users who deliberately stay in Normal
Nudge autoFocus fires from an async completion
Focus is dropped, not returned, on nudge dismissal

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 25, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/approval-mode-discoverability branch from 384b208 to 5d9e5be Compare August 25, 2026 22:24
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Opus advisory on 384b208:

  • fixed (trigger-open only hides the callout; closing the menu resurrects it): in 5d9e5be onOpenChange(true) now retires an active nudge (if (nudge) onNudgeDismiss?.()), so every open route — trigger click, hint link, See options, keyboard — permanently dismisses it. Pinned by the 'opening the menu via the trigger dismisses an active nudge (no resurrection on close)' test, mutation-verified.

opening the picker via the trigger button … hides the callout through <Popover open={!!nudge && !open}> but never calls onNudgeDismiss

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

UX CONCERNS on 384b208, per finding:

  • fixed (deictic copy with no visible anchor): in 5d9e5be the picker trigger wears the spotlight ring (ring-2 ring-accent/60) for the whole time the callout is up, so "This is where" points at a visibly marked control. Pinned by the 'trigger wears the spotlight ring while the callout is up' test.
  • fixed (nudge survives the user's own answer): any menu open now dismisses the callout via onOpenChange(true) — same fix as the Opus finding, same pin test.
  • fixed (stale description): the PR body's positioning and Tests paragraphs are rewritten to describe the shipped Radix implementation (collision handling, suppressed focus capture, focus return, any-open dismissal); the phantom clampNudgeLeft/nudgeArrowLeft/geometry-test claims are gone.

Deictic copy with no visible anchor
Nudge survives the user's own answer
Stale description

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

First Principles CONCERNS on 384b208, per finding:

  • fixed (description-vs-diff drift): PR body rewritten in place before the 5d9e5be push — the positioning paragraph now describes the Radix popover actually shipped, the Tests section lists the real coverage (25 tests), and the phantom geometry helpers/tests are gone.
  • fixed (Subtraction — drop the zero-consumer viaView boolean): adopted in 5d9e5be; onNudgeDismiss is now a no-arg callback.
  • rebutted (B2 duplicates A1 / provenance unsupported): standing ruling from the prior round holds — B2 ships by explicit maintainer product decision (mockup selection is this repo's decision gate), and the two mechanisms serve different moments (ambient pointer vs one-time intervention at measured friction). Marginal cost after this round is one counter ref, one flag, one Radix popover.

Description-vs-diff drift … Behavior still matches; the record of what shipped does not.
Drop the viaView boolean from onNudgeDismiss

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Design CONCERNS on 384b208:

  • fixed (phantom description of the callout's positioning and its tests): the PR body was rewritten in place before the 5d9e5be push — positioning paragraph now records the Radix implementation (the design this lane itself calls the better one), the focus behavior is described as shipped (suppressed capture while typing + focus return), and the test list matches the diff (25 tests).

a maintainer auditing the claimed mutation-verified geometry coverage finds it missing

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 25, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/approval-mode-discoverability branch from 5d9e5be to b917390 Compare August 25, 2026 22:57
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

UX CONCERNS on 5d9e5be, per finding:

  • fixed (hint never retires by acknowledgment): in b917390 permanent nudge dismissal (Got it / any menu open) also writes APPROVAL_MODE_ADJUSTED_LS_KEY — the callout delivered the lesson, so the A1 hint retires with it. Pinned by the extended '"Got it" dismisses and writes the permanent flag' test asserting both flags.
  • fixed (Suggestion — Escape should not spend the one-time callout unseen): Escape now routes to a session-scoped hide (onNudgeHide, clears the slot state without writing the forever flag), so the callout may re-fire on a later approval in this sitting; permanent dismissal stays with the two buttons and menu opens. Pinned by 'Escape routes to the session-scoped hide, not the permanent dismissal' + a fallback-semantics test.

a user who read the callout … and clicked "Got it" … sees "Tired of confirming every step?" reappear on every approval indefinitely
a reflexive Escape aimed at typing kills the one-time teaching moment unseen

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

First Principles CONCERNS on 5d9e5be:

  • rebutted (A1 and B2 serve one population — third statement of the round-1/round-2 Subtraction): standing maintainer ruling, recorded twice on this PR (dispositions at heads 918b20e and 384b208): the A1+A2+B2 combination is an explicit product decision made from rendered mockups before implementation, which is this repo's decision gate for user-visible features. The population overlap is by design — B2 is the escalation tier for exactly the user the ambient hint did not convert after three approvals; 'reads a dialog but not a muted link' is a recognized attention-tier distinction in first-run UX, not an incoherence. No new evidence or mechanism is presented in this round's restatement, so the prior ruling stands; relitigating a settled product decision per round is not actionable by this PR. This round additionally hardened B2's cost side: permanent dismissal now retires the A1 hint too, so the two mechanisms converge to zero residual chrome after one interaction.

A1 and B2 serve one population … B2 carries most of the diff's permanent complexity

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running labels Aug 25, 2026
@github-actions github-actions Bot added the readiness: passed Eligible automated validation passed for the current revision label Aug 25, 2026
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 25, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/approval-mode-discoverability branch from b917390 to 715a4a8 Compare August 25, 2026 23:31
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 25, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

UX CONCERNS on b917390, per finding:

  • fixed (no outside-dismiss path — callout occludes streaming content): in 715a4a8 onInteractOutside routes to the session-scoped hide, exactly the suggested semantics: outside click hides for the sitting (like Escape), so the one-time moment is not spent unseen and the callout can re-fire on a later approval this session. jsdom cannot drive Radix's DismissableLayer, so the pin is end-to-end: the capture harness (03b-outside-hide-refire, dark+light) clicks outside, asserts the callout hides, injects the next approval, approves, and asserts the callout re-fires.
  • rebutted (Suggestion — impression-cap the hint): the hint is one muted line rendered only while an approval card is up, and it now retires on ANY picker open or permanent callout dismissal — every path that demonstrates the user has seen the control. An impression counter would add persisted state to suppress a surface for a user who has never engaged with any of its exits; that trade is not clearly positive and is severable if the maintainer wants it (happy to file on request).

clicking anywhere else (composer, transcript) leaves it up
retire it after a handful of impressions

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

First Principles CONCERNS on b917390, per finding:

  • fixed (Subtraction — drop APPROVAL_NUDGE_DISMISSED_LS_KEY): adopted in 715a4a8, exactly as grepped: since permanent dismissal writes the adjusted/discovery flag, the second flag was only ever written beside it and its read sat in a conjunction the adjusted check already decided. The nudge now gates on the adjusted flag alone; constant, write, and read deleted; tests updated to the one-flag contract.
  • rebutted (founding harm is a hypothesis / B2 is a louder retry of A1 — fourth statement of the settled Subtraction): standing maintainer ruling, recorded at heads 918b20e, 384b208, and 5d9e5be. The escalation-tier design is intentional; the provenance (maintainer mockup selection, no filed issue) is disclosed in the PR body rather than hidden. No new mechanism is presented; the ruling stands.

Drop APPROVAL_NUDGE_DISMISSED_LS_KEY … 1 write … 1 read … adjusted check already decides
B2 is a louder retry of A1

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

UX on 715a4a8 (Suggestion only, no Watch items — converged):

  • rebutted (nudge sideOffset/align to clear the last transcript line): with outside-click session-hide shipped this round, any occlusion is one click deep and never spends the one-time callout; the callout must overlay SOMETHING in a bottom-anchored composer layout, and pushing it higher trades transcript occlusion for composer occlusion. Offset tuning is severable polish that costs a full CI round per attempt; deferring to maintainer taste rather than iterating advisory pixels.

the callout fully covers the assistant's in-progress message bubble

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

First Principles on 715a4a8, per finding:

  • rebutted (Subtraction — drop the ?? onNudgeDismiss fallback): the fallback is deliberate optional-prop semantics, not dead code: onNudgeHide is optional, and an unwired mount's Escape/outside-click must still DO something — falling back to permanent dismissal is strictly safer than a dead gesture on a modal-like surface. Both shipped mounts wire the hide handler, so the fallback is the API's defined default for future mounts (app-sdk embeds), pinned by its own test precisely so the contract cannot rot silently. Removing it saves 6 characters and deletes a defined behavior; disproportional.
  • rebutted (defer B2 — fifth statement): standing maintainer ruling recorded at heads 918b20e, 384b208, 5d9e5be, and b917390. No new mechanism this round; banner-blindness escalation is precisely the designed purpose of the tier. The ruling stands; per this repo's review conventions a settled product decision is not relitigated per round.

Drop the ?? onNudgeDismiss fallback … 0 consumers of the fallback
ship A1+A2 first and let a reported failure … justify the escalation tier

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 25, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Review-ready at head 715a4a8: 62/62 checks green (0 pending, 0 failing), MERGEABLE, all 5 AI review lanes settled — GPT / Opus / Design PASS; First Principles and UX advisory CONCERNS each answered with per-lane, per-finding dispositions (latest: this head's two comments above). Every raised concern across 5 review rounds has a recorded disposition: 15 findings fixed with pinning tests, the rest rebutted or deferred with rationale. Awaiting human review and merge.

@bolichen97
bolichen97 enabled auto-merge (squash) August 26, 2026 04:43
@bolichen97
bolichen97 merged commit 9c6df4a into main Aug 26, 2026
66 checks passed
@bolichen97
bolichen97 deleted the feat/approval-mode-discoverability branch August 26, 2026 05:42
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 26, 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.

3 participants