diff --git a/src/providers/shared/renderer/conditions/dispatchRefusal.renderer.test.tsx b/src/providers/shared/renderer/conditions/dispatchRefusal.renderer.test.tsx index 2b2e08e94..299701aa0 100644 --- a/src/providers/shared/renderer/conditions/dispatchRefusal.renderer.test.tsx +++ b/src/providers/shared/renderer/conditions/dispatchRefusal.renderer.test.tsx @@ -189,9 +189,10 @@ describe('the dispatcher reports what it used to discard (#1070)', () => { }) it('leaves the pty arm alone, because its caller already answers for it', async () => { - // `sendConditionKey` in TileLeaf reads main's boolean and shows a pane - // toast for a write it could not make. Reporting here too would double the - // message for the one arm that was never silent. + // `sendConditionKey` in TileLeaf reads main's boolean and surfaces the + // refusal itself — on the global toast since #711, the same surface this + // reporter's callers use. Reporting here too would double the message for + // the one arm that was never silent. const onRefused = vi.fn() const onPtyAction = vi.fn(async () => {}) const dispatch = makeOutletDispatch(onPtyAction, async () => ({ ok: false, reason: 'timeout' }), onRefused) diff --git a/src/renderer/src/workspace/tile-tree/TileLeaf.conditionRefusal.renderer.test.tsx b/src/renderer/src/workspace/tile-tree/TileLeaf.conditionRefusal.renderer.test.tsx index f931a0c95..234b0b8bf 100644 --- a/src/renderer/src/workspace/tile-tree/TileLeaf.conditionRefusal.renderer.test.tsx +++ b/src/renderer/src/workspace/tile-tree/TileLeaf.conditionRefusal.renderer.test.tsx @@ -5,6 +5,7 @@ import { useAppStore } from '@renderer/app-state/store' import { emptyRuntime } from '@renderer/session-runtime/state' import type { SessionRuntime } from '@renderer/session-runtime/state' import type { ConditionRefusal, ConditionRefusalReporter } from '@shared/conditions-core/dispatch' +import type { ConditionPtyAction } from '@shared/conditions-core/contract' import type { Workspace } from '@renderer/workspace/hook' import { GlobalToastProvider } from '@renderer/ui/GlobalToast' import { AgentTerminalOwnerVisibilityProvider } from '@renderer/workspace/terminal/AgentTerminalOwnership' @@ -28,15 +29,32 @@ import { TileLeaf } from './TileLeaf' // src/providers/shared/renderer/conditions/dispatchRefusal.renderer.test.tsx. let reporter: ConditionRefusalReporter | undefined +/** The pty arm TileLeaf passes as `onPtyAction`, which reaches + * `sendConditionKey`. Probed through the real prop (#1177 renamed it from + * `onSend`), and adapted back to a keystroke so the tests read as one. A probe + * that no longer matches the prop leaves `sendKey` undefined, and `mount`'s + * assertion fails loudly rather than letting every keystroke test pass + * vacuously. */ +let sendKey: ((data: string) => Promise) | undefined vi.mock('@providers/shared/renderer/conditions/ProviderConditionOutlet', () => ({ - ProviderConditionOutlet: (props: { onConditionRefused?: ConditionRefusalReporter }) => { + ProviderConditionOutlet: (props: { + onConditionRefused?: ConditionRefusalReporter + onPtyAction?: (action: ConditionPtyAction) => Promise + }) => { reporter = props.onConditionRefused + const onPtyAction = props.onPtyAction + sendKey = onPtyAction ? data => onPtyAction({ kind: 'pty', id: 'probe', label: 'probe', data }) : undefined return
}, })) vi.mock('@renderer/features/feed/ui/Feed', () => ({ Feed: () =>
})) -vi.mock('@renderer/features/sessionFeed/SessionFeedContext', () => ({ useSessionFeed: () => ({}) })) +// `sendInput` is what main's refusal comes back through; the tests that care +// set `sendInputResult` before clicking. +let sendInputResult = true +vi.mock('@renderer/features/sessionFeed/SessionFeedContext', () => ({ + useSessionFeed: () => ({ sendInput: async () => sendInputResult }), +})) vi.mock('@renderer/features/feed/ledger/useLedgerFeedItems', () => ({ useLedgerFeedItems: () => ({ items: [] }) })) vi.mock('@renderer/features/usage-limit/useUsageLimitActions', () => ({ useUsageLimitActions: () => ({}) })) vi.mock('@renderer/features/workflows/model/useSessionWorkflowViews', () => ({ @@ -54,6 +72,9 @@ const originalApi = window.api beforeEach(() => { reporter = undefined + sendKey = undefined + sendInputResult = true + paneToasts.length = 0 window.api = { ...(originalApi ?? {}), onExtensionNotification: vi.fn().mockReturnValue(() => {}), @@ -75,15 +96,19 @@ const refusal = (over: Partial = {}): ConditionRefusal => ({ ...over, }) -function mount(): void { +/** Every message TileLeaf tried to put in the PANE toast. */ +const paneToasts: Array<{ sessionId: string; message: string }> = [] + +function mount(options?: { conditions?: SessionRuntime['conditions'] }): void { const workspace = { state: { sessions: { agent: { kind: 'claude', cwd: '/trial' } } }, acknowledgeSession: vi.fn(), setDraftInput: vi.fn(), + showPaneToast: (sessionId: string, message: string) => { paneToasts.push({ sessionId, message }) }, } as unknown as Workspace const runtime: SessionRuntime = { ...emptyRuntime(), - conditions: { + conditions: options?.conditions !== undefined ? options.conditions : { provider: 'claude', ts: 1, conditions: { @@ -98,8 +123,10 @@ function mount(): void { , ) - // A reporter TileLeaf never passed would make every assertion below vacuous. + // A reporter or send TileLeaf never passed would make every assertion below + // vacuous. expect(reporter).toBeTypeOf('function') + expect(sendKey).toBeTypeOf('function') } it('tells the user in words what the refusal was, on the surface a modal cannot cover', () => { @@ -163,3 +190,70 @@ it('leaves the toast dismissable so it cannot sit over the question it is about' expect(screen.queryByText(message)).toBeNull() }) + +// --------------------------------------------------------------------------- +// The pty arm (#711 item 1) +// +// #1070 moved the STRUCTURED refusal to the global toast because every +// condition that can refuse is a modal, and the pane toast is an in-flow +// sibling with no z-index sitting under a 1100-z scrim. The keystroke arm was +// left behind, so the same click produced a readable message or an invisible +// one depending on which arm handled it — and the keystroke arm is the one a +// TRUST DIALOG uses, i.e. the case where the modal is guaranteed to be up. +// --------------------------------------------------------------------------- + +it('shows a refused keystroke where a modal cannot cover it', async () => { + mount() + sendInputResult = false + + await act(async () => { await sendKey?.('1') }) + + const toast = screen.getByText( + 'That keystroke did not reach the agent. If it stays stuck, retry the pane.', + ) + expect(toast.closest('[class*="z-[1200]"]')).not.toBeNull() + // The pane toast is the surface this message used to go to, and it is under + // the scrim. Asserting the text moved is not enough — a message sent to BOTH + // would still pass that. + expect(paneToasts).toEqual([]) +}) + +it('says a vanished prompt is gone on the same surface as every other refusal', async () => { + // The other early return of the same click. A single click must not land on + // two different surfaces depending on which check failed first. + mount({ conditions: null }) + + await act(async () => { await sendKey?.('1') }) + + expect(screen.getByText('That prompt is no longer live.')).toBeInTheDocument() + expect(paneToasts).toEqual([]) +}) + +it('holds a refused keystroke as long as the refusal it matches', async () => { + // The comment beside the fix calls the duration load-bearing — 2.5s is not + // enough to look at a trust dialog and decide. A mutation that drops both + // duration arguments passed all 1013 renderer tests (#1110 review), because + // the sibling reporter's duration test was never carried across to this arm. + vi.useFakeTimers() + mount() + sendInputResult = false + + await act(async () => { await sendKey?.('1') }) + const message = 'That keystroke did not reach the agent. If it stays stuck, retry the pane.' + expect(screen.getByText(message)).toBeInTheDocument() + + act(() => { vi.advanceTimersByTime(4_000) }) + expect(screen.getByText(message)).toBeInTheDocument() + + act(() => { vi.advanceTimersByTime(2_100) }) + expect(screen.queryByText(message)).toBeNull() +}) + +it('stays silent when the keystroke landed', async () => { + mount() + + await act(async () => { await sendKey?.('1') }) + + expect(screen.queryByText(/did not reach the agent/)).toBeNull() + expect(paneToasts).toEqual([]) +}) diff --git a/src/renderer/src/workspace/tile-tree/TileLeaf.tsx b/src/renderer/src/workspace/tile-tree/TileLeaf.tsx index 1a9e4a4de..bb50c7d8d 100644 --- a/src/renderer/src/workspace/tile-tree/TileLeaf.tsx +++ b/src/renderer/src/workspace/tile-tree/TileLeaf.tsx @@ -425,25 +425,75 @@ export function TileLeaf({ // precisely what the quarantine exists to prevent. Requiring a live // snapshot mirrors what `RemoteServer.applyPermissionReply` already does // before it writes. + // + // WHY BOTH REFUSALS GO TO THE GLOBAL TOAST (#711 item 1) + // + // This arm is reached by FOUR condition kinds, not one shape (#1110 review + // corrected an earlier version of this comment that claimed they were all + // modals): + // + // - `claude.trust-dialog` and `claude.permission-prompt` are Radix + // modals; + // - `codex.approval` and `claude.resume-prompt` are in-flow STRIPS + // (`layout: 'strip'`, plain `role="group"` divs, no scrim); + // + // and there is a fifth entry point with no condition surface at all: + // `useComposerKeybinds` routes composer ArrowUp/Down here whenever + // `hasActionCondition` is true. + // + // The modal cases are what force the move. `showPaneToast` renders + // `PaneToast`, an in-flow sibling inside the pane with no z-index; the + // dialog overlay is z-[1100] and 85% opaque, and Radix marks the rest of the + // subtree `aria-hidden`. The message was painted under the scrim, inside the + // region screen readers are told to ignore. Worse, a modal from a RETAINED + // pane (Spotlight keeps the tile tree mounted under `display: none`) portals + // to `body` while its pane toast is inside the hidden subtree — invisible + // outright. `GlobalToast` is z-[1200], and its own header records this trap. + // + // For the strip cases the pane toast was readable, and the global toast is + // the lesser fit: it is not adjacent to the strip that was clicked. That + // cost is accepted because the alternative is branching the surface on the + // condition's layout, which would put the same click on two surfaces again — + // the exact inconsistency this fixes. Every path here is focus-safe (the + // pane root takes `onMouseDown={onFocusRequest}`, strip keyboard handling is + // gated on `interactionActive`, and the composer route needs the textarea + // focused), so the toast always describes the pane the user is looking at. + // + // #1070 moved the STRUCTURED refusal here and left this arm behind, which + // was worse than either surface on its own: one click produced a readable + // message or an invisible one depending on which arm handled it, and this is + // the arm a trust dialog uses. + // + // The vanished-prompt branch moves too. It is in fact not reachable today — + // the outlet is fed `normalizedConditions` and renders nothing for a null + // snapshot, and the composer route is gated by `hasActionCondition` — so + // moving it costs nothing and keeps one click on one surface if it ever + // becomes reachable again. + // + // Duration matches the refusal reporter's: these ask the user to look at + // something and decide, which the 2.5s default does not allow. const sendConditionKey = useCallback(async (data: string) => { acknowledgeSession() if (!runtime.conditions) { - workspace.showPaneToast(sessionId, 'That prompt is no longer live.') + showToast('That prompt is no longer live.', 6000) return } const ok = await feed.sendInput(sessionId, data) if (!ok) { + // Parity with the structured reporter below: the toast tells the user, + // the console says which session, because the toast deliberately carries + // no session identity and a 4-pane grid gives it no context (#1110 + // review). + // eslint-disable-next-line no-console + console.warn(`[condition ${sessionId.slice(0, 8)}] keystroke refused`) // main returns a bare boolean for two disjoint reasons — a prompt // delivery holds the write reservation, or there is no backend at all. // Only the first is worth retrying, and we cannot tell them apart from // here; the message stays honest about that rather than promising a // retry that can never work. - workspace.showPaneToast( - sessionId, - 'That keystroke did not reach the agent. If it stays stuck, retry the pane.', - ) + showToast('That keystroke did not reach the agent. If it stays stuck, retry the pane.', 6000) } - }, [acknowledgeSession, feed, runtime.conditions, sessionId, workspace.showPaneToast]) + }, [acknowledgeSession, feed, runtime.conditions, sessionId, showToast]) const loadOlderHistory = useCallback(async () => { await workspace.loadOlderHistory(sessionId) diff --git a/src/shared/conditions-core/dispatch.ts b/src/shared/conditions-core/dispatch.ts index a666c84d7..054f9dde6 100644 --- a/src/shared/conditions-core/dispatch.ts +++ b/src/shared/conditions-core/dispatch.ts @@ -66,10 +66,11 @@ export function makeOutletDispatch( if (action.kind === 'pty') { // The pty arm is NOT reported here. Its caller owns the answer: // `onPtyAction` returns void because each surface already reports its - // own failed write (the desktop's sendConditionKey shows a pane toast - // for main's false; the phone shows the desktop's refusal of an action - // it could not match). Reporting it twice would double the message for - // the one arm that was never silent. + // own failed write (the desktop's sendConditionKey shows it on the + // global toast since #711, the same surface this reporter's callers + // use; the phone shows the desktop's refusal of an action it could not + // match). Reporting it twice would double the message for the one arm + // that was never silent. await onPtyAction(action) return }