From ce0954d873bb361a486c6d9b3a60d5bddd016030 Mon Sep 17 00:00:00 2001 From: AstroHan Date: Sat, 29 Aug 2026 03:18:18 +0800 Subject: [PATCH 1/4] refactor(desktop): make the session UI store the only pending authority MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four in-flight facts each had two representations: a `Set` ref that the duplicate guard read, and a `Record` in the session UI store that the disabled mask rendered. Nothing read across the pair, and the two were kept aligned by hand at every claim, every `finally`, and two separate teardown paths — `clearOwnedSessionState` deleting from the refs, `clearSessionUiState` wiping the maps, correct only because one calls the other in that order. The ref half is redundant. State replacement in the controller is synchronous, so a claim is visible to the next `getState()` in the same task; the guard can read the map it already writes. `createPendingClaim` puts both halves behind one compare-and-set, and message retry, stop, permission mode, and session model each get a claim in place of a ref plus a setter plus a pair of AppShell helpers. Removed along the way: `addPendingSessionAction` and `clearPendingSessionAction` with their unused optional-setter parameter, three private copies of `omitSessionKey` that existed only to maintain the map half, the four `set*BySession` setters those copies fed, and the two teardown paths that had to agree. One behaviour change. `setPermissionMode` now claims before its bypass confirmation rather than after, so a second click cannot open a second dialog; the control reads as pending while the user decides, which is what is true. A cancelled confirmation releases the claim. Generated-by: Claude Code --- .../app-shell-chat-actions-fixture.ts | 5 +- ...app-shell-session-settings-actions.test.ts | 44 +++++--- .../__tests__/app-shell-stop-action.test.ts | 5 +- .../src/renderer/app-shell-chat-actions.ts | 24 +--- .../desktop/src/renderer/app-shell-effects.ts | 4 - .../app-shell-session-settings-actions.ts | 59 +++------- .../renderer/app-shell-session-ui-state.ts | 55 +++++++-- .../src/renderer/app-shell-stop-action.ts | 24 +--- apps/desktop/src/renderer/app-shell.tsx | 61 ++-------- .../src/renderer/session-workspace-actions.ts | 6 - .../use-app-shell-session-workspace.ts | 14 +-- .../src/renderer/use-turn-action-registry.ts | 106 ++++++++++++++++++ 12 files changed, 223 insertions(+), 184 deletions(-) create mode 100644 apps/desktop/src/renderer/use-turn-action-registry.ts diff --git a/apps/desktop/src/main/__tests__/app-shell-chat-actions-fixture.ts b/apps/desktop/src/main/__tests__/app-shell-chat-actions-fixture.ts index f7f2bbc22f..ad52d98382 100644 --- a/apps/desktop/src/main/__tests__/app-shell-chat-actions-fixture.ts +++ b/apps/desktop/src/main/__tests__/app-shell-chat-actions-fixture.ts @@ -96,24 +96,21 @@ export function createActionsDeps() { return { uiLocale: 'en' as const, activeIdRef, - addPendingSessionAction: () => true, captureComposerImportOwner: () => ({ sessionId: undefined, navSection: 'sessions' as const, }), checkTaskSubmissionReadiness: async () => true, - clearPendingSessionAction: () => undefined, isNewChatSendSurfaceActive: () => true, isShellSurfaceOwnerActive: () => true, markSessionReadLocally: () => undefined, - messageRetryPendingRef: { current: new Set() }, + messageRetryPending: { claim: () => true, release: () => undefined }, refreshSessions: async () => [], activateSessionForFirstSend: async (sessionId: string) => { activeIdRef.current = sessionId; }, setActiveId: () => undefined, setMessageLoadErrorBySession: () => undefined, - setMessageRetryPendingBySession: () => undefined, setMessages: () => undefined, addTransientMessage: () => undefined, updateTransientMessage: () => undefined, diff --git a/apps/desktop/src/main/__tests__/app-shell-session-settings-actions.test.ts b/apps/desktop/src/main/__tests__/app-shell-session-settings-actions.test.ts index 6889c68786..7468c17afa 100644 --- a/apps/desktop/src/main/__tests__/app-shell-session-settings-actions.test.ts +++ b/apps/desktop/src/main/__tests__/app-shell-session-settings-actions.test.ts @@ -23,6 +23,7 @@ import type { LlmConnection } from '@maka/core/llm-connections'; import type { StoredMessage } from '@maka/core/session'; import type { DesktopSessionSummary } from '../../preload/bridge-contract.js'; import { createAppShellSessionSettingsActions } from '../../renderer/app-shell-session-settings-actions.js'; +import type { SessionPendingClaim } from '../../renderer/app-shell-session-ui-state.js'; function deferred() { let resolve!: (value: T) => void; @@ -56,6 +57,20 @@ function session(id: string): DesktopSessionSummary { }; } +/** The store's claim semantics over a plain map the assertions can read. */ +function pendingClaimOver(state: Record): SessionPendingClaim { + return { + claim(key) { + if (state[key] === true) return false; + state[key] = true; + return true; + }, + release(key) { + delete state[key]; + }, + }; +} + function createHarness(options: { confirm?: () => Promise; connections?: LlmConnection[]; @@ -65,8 +80,8 @@ function createHarness(options: { const activeIdRef = { current: 'session-a' as string | undefined }; const sessions = [session('session-a'), session('session-b')]; const sessionsRef = { current: sessions }; - const pending = new Set(); - const pendingBySession: Record = {}; + const permissionModePending: Record = {}; + const sessionModelPending: Record = {}; const modelCalls: string[] = []; const permissionCalls: string[] = []; const thinkingCalls: string[] = []; @@ -107,18 +122,12 @@ function createHarness(options: { activeIdRef, connections: options.connections ?? ([{ slug: 'e2e', name: 'E2E' }] as LlmConnection[]), messages: options.messages ?? [], - pendingPermissionModeChangesRef: { current: new Set() }, - pendingSessionModelChangesRef: { current: pending }, + permissionModePending: pendingClaimOver(permissionModePending), + sessionModelPending: pendingClaimOver(sessionModelPending), refreshSessions: async () => sessions, saveComposerDefaults: () => undefined, sessionsRef, setNewTaskPermissionMode: (mode) => void newTaskPermissionModes.push(mode), - setPendingPermissionModeBySession: () => undefined, - setPendingSessionModelBySession: (update) => { - const next = update(pendingBySession); - for (const key of Object.keys(pendingBySession)) delete pendingBySession[key]; - Object.assign(pendingBySession, next); - }, toastApi: { success: (title, description) => successes.push({ title, description }), error: (title, _description, _details, target) => { @@ -137,8 +146,8 @@ function createHarness(options: { modelCalls, modelResult, newTaskPermissionModes, - pending, - pendingBySession, + permissionModePending, + sessionModelPending, permissionCalls, sessionsRef, thinkingCalls, @@ -225,7 +234,7 @@ describe('AppShell session settings actions', () => { assert.deepEqual(harness.modelCalls, ['session-a']); assert.deepEqual(harness.thinkingCalls, []); - assert.equal(harness.pendingBySession['session-a'], true); + assert.equal(harness.sessionModelPending['session-a'], true); harness.modelResult.resolve(session('session-a')); await modelChange; @@ -312,7 +321,7 @@ describe('AppShell session settings actions', () => { assert.deepEqual(harness.modelCalls, ['session-a']); assert.deepEqual(harness.thinkingCalls, ['session-b']); - assert.deepEqual(harness.pending, new Set(['session-a', 'session-b'])); + assert.deepEqual(Object.keys(harness.sessionModelPending), ['session-a', 'session-b']); harness.thinkingResult.resolve(session('session-b')); await thinkingChange; @@ -332,11 +341,11 @@ describe('AppShell session settings actions', () => { assert.deepEqual(harness.thinkingCalls, ['session-a']); assert.deepEqual(harness.modelCalls, []); - assert.equal(harness.pendingBySession['session-a'], true); + assert.equal(harness.sessionModelPending['session-a'], true); harness.thinkingResult.resolve(session('session-a')); await thinkingChange; - assert.equal(harness.pendingBySession['session-a'], undefined); + assert.equal(harness.sessionModelPending['session-a'], undefined); }); it('releases the session owner after a failed mutation so the next action can run', async () => { @@ -346,8 +355,7 @@ describe('AppShell session settings actions', () => { harness.thinkingResult.reject(new Error('fixture failure')); await thinkingChange; - assert.equal(harness.pending.has('session-a'), false); - assert.equal(harness.pendingBySession['session-a'], undefined); + assert.equal(harness.sessionModelPending['session-a'], undefined); assert.equal(harness.errors.length, 1); assert.deepEqual(harness.errorTargets, [{ sessionId: 'session-a' }]); diff --git a/apps/desktop/src/main/__tests__/app-shell-stop-action.test.ts b/apps/desktop/src/main/__tests__/app-shell-stop-action.test.ts index e86ed76f1d..a16796aa15 100644 --- a/apps/desktop/src/main/__tests__/app-shell-stop-action.test.ts +++ b/apps/desktop/src/main/__tests__/app-shell-stop-action.test.ts @@ -39,10 +39,7 @@ test('removes exactly the transient messages the Host retracts while stopping', const stop = createAppShellStopAction({ uiLocale: 'en', activeIdRef: { current: 'session-1' }, - addPendingSessionAction: () => true, - clearPendingSessionAction: () => undefined, - setStopPendingBySession: () => undefined, - stopPendingRef: { current: new Set() }, + stopPending: { claim: () => true, release: () => undefined }, removeTransientMessage: (sessionId, messageId) => removed.push({ sessionId, messageId }), toastApi: { error() {} }, }); diff --git a/apps/desktop/src/renderer/app-shell-chat-actions.ts b/apps/desktop/src/renderer/app-shell-chat-actions.ts index fedf80b37e..300d5d5e9e 100644 --- a/apps/desktop/src/renderer/app-shell-chat-actions.ts +++ b/apps/desktop/src/renderer/app-shell-chat-actions.ts @@ -51,6 +51,7 @@ import { skillInvocationDisplayText, } from './skill-invocation-feedback.js'; import type { DesktopTranscriptRangeController } from './desktop-transcript-range-store.js'; +import type { SessionPendingClaim } from './app-shell-session-ui-state.js'; import { retainedAttachmentRefs, toComposerIngestItems, @@ -78,7 +79,6 @@ type ComposerImportOwner = { }; type RefBox = { current: T }; -type BooleanRecordUpdater = (updater: (current: Record) => Record) => void; type LiveTurnRecordUpdater = ( updater: (current: Record) => Record, ) => void; @@ -139,29 +139,18 @@ export interface AppShellChatActions { export function createAppShellChatActions(deps: { uiLocale: UiLocale; activeIdRef: RefBox; - addPendingSessionAction: ( - sessionId: string, - pendingRef: RefBox>, - setPendingBySession: BooleanRecordUpdater, - ) => boolean; captureComposerImportOwner: () => ComposerImportOwner; checkTaskSubmissionReadiness: () => Promise; - clearPendingSessionAction: ( - sessionId: string, - pendingRef: RefBox>, - setPendingBySession: BooleanRecordUpdater, - ) => void; isNewChatSendSurfaceActive: (owner: ComposerImportOwner) => boolean; /** The shell's one answer to "is this owner still the surface the user is * looking at". Both halves matter — the section AND the session id — which * is why the send path asks it instead of comparing the id itself. */ isShellSurfaceOwnerActive: (owner: ComposerImportOwner) => boolean; - messageRetryPendingRef: RefBox>; + messageRetryPending: SessionPendingClaim; refreshSessions: () => Promise; activateSessionForFirstSend: (sessionId: string) => Promise; setActiveId: (sessionId: string | undefined) => void; setMessageLoadErrorBySession: MessageLoadErrorUpdater; - setMessageRetryPendingBySession: BooleanRecordUpdater; setMessages: MessageListUpdater; addTransientMessage: ( sessionId: string, @@ -207,18 +196,15 @@ export function createAppShellChatActions(deps: { const { uiLocale, activeIdRef, - addPendingSessionAction, captureComposerImportOwner, checkTaskSubmissionReadiness, - clearPendingSessionAction, isNewChatSendSurfaceActive, isShellSurfaceOwnerActive, - messageRetryPendingRef, + messageRetryPending, refreshSessions, activateSessionForFirstSend, setActiveId, setMessageLoadErrorBySession, - setMessageRetryPendingBySession, setMessages, addTransientMessage, updateTransientMessage, @@ -817,7 +803,7 @@ export function createAppShellChatActions(deps: { } } async function retryMessages(sessionId: string) { - if (!addPendingSessionAction(sessionId, messageRetryPendingRef, setMessageRetryPendingBySession)) return; + if (!messageRetryPending.claim(sessionId)) return; try { if (activeIdRef.current !== sessionId) return; await transcriptRangeRef.current?.reload(); @@ -830,7 +816,7 @@ export function createAppShellChatActions(deps: { })); toastApi.error(copy.refreshFailedTitle, message, undefined, { sessionId }); } finally { - clearPendingSessionAction(sessionId, messageRetryPendingRef, setMessageRetryPendingBySession); + messageRetryPending.release(sessionId); } } diff --git a/apps/desktop/src/renderer/app-shell-effects.ts b/apps/desktop/src/renderer/app-shell-effects.ts index 6277ff143b..24fddcc8f3 100644 --- a/apps/desktop/src/renderer/app-shell-effects.ts +++ b/apps/desktop/src/renderer/app-shell-effects.ts @@ -159,8 +159,6 @@ export function useAppShellBootstrapSubscriptions(options: { handleConnectionEvent: (event: ConnectionEvent) => void; openHelp: () => void; openSettings: () => void; - pendingPermissionModeChangesRef: RefBox>; - pendingSessionModelChangesRef: RefBox>; pendingTurnActionTimersRef: RefBox>>; pendingTurnActionsRef: RefBox>; projectPickerPendingRef: RefBox; @@ -293,8 +291,6 @@ export function useAppShellBootstrapSubscriptions(options: { } options.pendingTurnActionTimersRef.current.clear(); options.pendingTurnActionsRef.current.clear(); - options.pendingPermissionModeChangesRef.current.clear(); - options.pendingSessionModelChangesRef.current.clear(); }); useEffect(() => { diff --git a/apps/desktop/src/renderer/app-shell-session-settings-actions.ts b/apps/desktop/src/renderer/app-shell-session-settings-actions.ts index 1b01820dce..35385308ab 100644 --- a/apps/desktop/src/renderer/app-shell-session-settings-actions.ts +++ b/apps/desktop/src/renderer/app-shell-session-settings-actions.ts @@ -28,9 +28,9 @@ import type { ThinkingLevel } from '@maka/core/model-thinking'; import type { UiLocale } from '@maka/core/ui-locale'; import type { DesktopSessionSummary } from '../preload/bridge-contract.js'; import { getShellCopy, localizedShellErrorMessage } from './locales/shell-copy.js'; +import type { SessionPendingClaim } from './app-shell-session-ui-state.js'; type RefBox = { current: T }; -type BooleanRecordUpdater = (updater: (current: Record) => Record) => void; type ToastApi = { success(title: string, description?: string): void; @@ -64,8 +64,8 @@ export function createAppShellSessionSettingsActions(deps: { activeIdRef: RefBox; connections: readonly LlmConnection[]; messages: readonly StoredMessage[]; - pendingPermissionModeChangesRef: RefBox>; - pendingSessionModelChangesRef: RefBox>; + permissionModePending: SessionPendingClaim; + sessionModelPending: SessionPendingClaim; refreshSessions: () => Promise; saveComposerDefaults: (patch: { model: { llmConnectionId: string; llmConnectionSlug: string; model: string }; @@ -73,8 +73,6 @@ export function createAppShellSessionSettingsActions(deps: { sessionsRef: RefBox; /** Persists the chat default; awaited so a failure surfaces as one. */ setNewTaskPermissionMode: (mode: ChatDefaultPermissionMode) => void | Promise; - setPendingPermissionModeBySession: BooleanRecordUpdater; - setPendingSessionModelBySession: BooleanRecordUpdater; toastApi: ToastApi; }): AppShellSessionSettingsActions { const { @@ -82,25 +80,16 @@ export function createAppShellSessionSettingsActions(deps: { activeIdRef, connections, messages, - pendingPermissionModeChangesRef, - pendingSessionModelChangesRef, + permissionModePending, + sessionModelPending, refreshSessions, saveComposerDefaults, sessionsRef, setNewTaskPermissionMode, - setPendingPermissionModeBySession, - setPendingSessionModelBySession, toastApi, } = deps; const copy = getShellCopy(uiLocale).sessionSettingsActions; - function omitSessionKey(current: Record, sessionId: string): Record { - if (!(sessionId in current)) return current; - const next = { ...current }; - delete next[sessionId]; - return next; - } - function modelLabel(connectionSlug: string, model: string): string { const connection = connections.find((entry) => entry.slug === connectionSlug); const displayName = connection?.models?.find((entry) => entry.id === model)?.displayName?.trim(); @@ -121,8 +110,14 @@ export function createAppShellSessionSettingsActions(deps: { ? sessionsRef.current.find((session) => session.id === sessionId)?.permissionMode : undefined; if (currentMode === mode) return true; + // No session means the chat default, which has no row to mark pending. It + // still needs a key of its own so two rapid switches cannot both run. const pendingKey = sessionId ?? '__global_permission_mode__'; - if (pendingPermissionModeChangesRef.current.has(pendingKey)) return false; + // Claimed before the confirm rather than after it: the dialog is part of + // the change, so a second click while it is open must not open a second + // one. The cost is that the control reads as pending while the user + // decides, which is what is actually true. + if (!permissionModePending.claim(pendingKey)) return false; if ( mode === 'bypass' && !(await toastApi.confirm({ @@ -133,15 +128,10 @@ export function createAppShellSessionSettingsActions(deps: { destructive: true, })) ) { + permissionModePending.release(pendingKey); return false; } - pendingPermissionModeChangesRef.current.add(pendingKey); - if (sessionId) - setPendingPermissionModeBySession((current) => ({ - ...current, - [sessionId]: true, - })); try { let nextMode = mode; if (sessionId) { @@ -165,8 +155,7 @@ export function createAppShellSessionSettingsActions(deps: { ); return false; } finally { - pendingPermissionModeChangesRef.current.delete(pendingKey); - if (sessionId) setPendingPermissionModeBySession((current) => omitSessionKey(current, sessionId)); + permissionModePending.release(pendingKey); } } @@ -179,12 +168,7 @@ export function createAppShellSessionSettingsActions(deps: { if (!sessionId) return; const previous = sessionsRef.current.find((session) => session.id === sessionId); const lastUsedModel = latestAssistantModelId(messages); - if (pendingSessionModelChangesRef.current.has(sessionId)) return; - pendingSessionModelChangesRef.current.add(sessionId); - setPendingSessionModelBySession((current) => ({ - ...current, - [sessionId]: true, - })); + if (!sessionModelPending.claim(sessionId)) return; try { const next = await window.maka.sessions.setModel(sessionId, input); if (activeIdRef.current === sessionId) { @@ -217,8 +201,7 @@ export function createAppShellSessionSettingsActions(deps: { ); } } finally { - pendingSessionModelChangesRef.current.delete(sessionId); - setPendingSessionModelBySession((current) => omitSessionKey(current, sessionId)); + sessionModelPending.release(sessionId); } } @@ -227,12 +210,7 @@ export function createAppShellSessionSettingsActions(deps: { if (!sessionId) return; const current = sessionsRef.current.find((session) => session.id === sessionId); if (current && current.thinkingLevel === level) return; - if (pendingSessionModelChangesRef.current.has(sessionId)) return; - pendingSessionModelChangesRef.current.add(sessionId); - setPendingSessionModelBySession((currentPending) => ({ - ...currentPending, - [sessionId]: true, - })); + if (!sessionModelPending.claim(sessionId)) return; try { await window.maka.sessions.setThinkingLevel(sessionId, level); if (activeIdRef.current === sessionId) { @@ -249,8 +227,7 @@ export function createAppShellSessionSettingsActions(deps: { ); } } finally { - pendingSessionModelChangesRef.current.delete(sessionId); - setPendingSessionModelBySession((currentPending) => omitSessionKey(currentPending, sessionId)); + sessionModelPending.release(sessionId); } } diff --git a/apps/desktop/src/renderer/app-shell-session-ui-state.ts b/apps/desktop/src/renderer/app-shell-session-ui-state.ts index bd3ba23947..d7464d9537 100644 --- a/apps/desktop/src/renderer/app-shell-session-ui-state.ts +++ b/apps/desktop/src/renderer/app-shell-session-ui-state.ts @@ -46,6 +46,20 @@ export interface MessageQueueUiState { type AppShellSessionUiStateMapKey = keyof AppShellSessionUiState; +/** The maps that record nothing but "an action is in flight for this key". */ +type BooleanMapKey = + | 'messageRetryPendingBySession' + | 'stopPendingBySession' + | 'pendingPermissionModeBySession' + | 'pendingSessionModelBySession'; + +export interface SessionPendingClaim { + /** Marks `key` in flight. Returns false — a no-op — if it already was. */ + claim(key: string): boolean; + /** Gives the claim back. Safe to call for a key that never held one. */ + release(key: string): void; +} + const SESSION_UI_MAP_KEYS = [ 'messageLoadErrorBySession', 'messageRetryPendingBySession', @@ -169,14 +183,41 @@ export function createAppShellSessionUiStateController( return (updater) => updateMap(key, updater); } + /** + * The in-flight claim on one of the pending maps: `claim` marks a key and + * reports whether it won, `release` gives it back. + * + * Both read and write the same map, which is what makes this the only + * representation of "an action is in flight". Each of these four used to be a + * `Set` ref for the duplicate guard beside a map for the rendered flag, + * synchronized by hand at every add and every `finally`, and cleared by two + * separate teardown paths that stayed aligned only by ordering. Nothing + * needed the ref: state replacement is synchronous, so a claim is visible to + * the next `getState()` in the same task. + */ + function createPendingClaim(key: BooleanMapKey): SessionPendingClaim { + return { + claim(claimKey: string): boolean { + if (currentState[key][claimKey] === true) return false; + updateMap(key, (current) => ({ ...current, [claimKey]: true })); + return true; + }, + release(claimKey: string): void { + updateMap(key, (current) => omitSessionKey(current, claimKey)); + }, + }; + } + return { getState: () => currentState, subscribe, liveTurnBySessionRef, sessionEventHealthBySessionRef, setMessageLoadErrorBySession: createMapSetter('messageLoadErrorBySession'), - setMessageRetryPendingBySession: createMapSetter('messageRetryPendingBySession'), - setStopPendingBySession: createMapSetter('stopPendingBySession'), + messageRetryPending: createPendingClaim('messageRetryPendingBySession'), + stopPending: createPendingClaim('stopPendingBySession'), + permissionModePending: createPendingClaim('pendingPermissionModeBySession'), + sessionModelPending: createPendingClaim('pendingSessionModelBySession'), setLiveTurnBySession: createMapSetter('liveTurnBySession'), setShellRunUpdatesBySession: createMapSetter('shellRunUpdatesBySession'), setInteractionBySession: createMapSetter('interactionBySession'), @@ -184,8 +225,6 @@ export function createAppShellSessionUiStateController( setSessionEventHealthBySession: ((updater) => { sessionEventHealthBySessionRef.current = updater(sessionEventHealthBySessionRef.current); }) satisfies StateUpdater>, - setPendingPermissionModeBySession: createMapSetter('pendingPermissionModeBySession'), - setPendingSessionModelBySession: createMapSetter('pendingSessionModelBySession'), /** * The authority said something about `turnId` — it started, failed to * start, or ended. Drop that arm's `unconfirmed` claim so a session list @@ -239,15 +278,15 @@ export function useAppShellSessionUiState() { liveTurnBySessionRef: controller.liveTurnBySessionRef, sessionEventHealthBySessionRef: controller.sessionEventHealthBySessionRef, setMessageLoadErrorBySession: controller.setMessageLoadErrorBySession, - setMessageRetryPendingBySession: controller.setMessageRetryPendingBySession, - setStopPendingBySession: controller.setStopPendingBySession, + messageRetryPending: controller.messageRetryPending, + stopPending: controller.stopPending, + permissionModePending: controller.permissionModePending, + sessionModelPending: controller.sessionModelPending, setLiveTurnBySession: controller.setLiveTurnBySession, setShellRunUpdatesBySession: controller.setShellRunUpdatesBySession, setInteractionBySession: controller.setInteractionBySession, setMessageQueueBySession: controller.setMessageQueueBySession, setSessionEventHealthBySession: controller.setSessionEventHealthBySession, - setPendingPermissionModeBySession: controller.setPendingPermissionModeBySession, - setPendingSessionModelBySession: controller.setPendingSessionModelBySession, confirmLiveTurn: controller.confirmLiveTurn, clearSessionUiState: controller.clearSessionUiState, clearTurnTransientStateIfCurrent: controller.clearTurnTransientStateIfCurrent, diff --git a/apps/desktop/src/renderer/app-shell-stop-action.ts b/apps/desktop/src/renderer/app-shell-stop-action.ts index ea1fa70eaf..36525e8311 100644 --- a/apps/desktop/src/renderer/app-shell-stop-action.ts +++ b/apps/desktop/src/renderer/app-shell-stop-action.ts @@ -20,9 +20,9 @@ import type { UiLocale } from '@maka/core/ui-locale'; import { localizedShellErrorMessage } from './locales/shell-copy.js'; import { getDesktopConversationCopy } from './locales/conversation-copy.js'; +import type { SessionPendingClaim } from './app-shell-session-ui-state.js'; type RefBox = { current: T }; -type BooleanRecordUpdater = (updater: (current: Record) => Record) => void; type ToastApi = { error( @@ -36,35 +36,21 @@ type ToastApi = { export function createAppShellStopAction(deps: { uiLocale: UiLocale; activeIdRef: RefBox; - addPendingSessionAction: ( - sessionId: string, - pendingRef: RefBox>, - setPendingBySession: BooleanRecordUpdater, - ) => boolean; - clearPendingSessionAction: ( - sessionId: string, - pendingRef: RefBox>, - setPendingBySession: BooleanRecordUpdater, - ) => void; - setStopPendingBySession: BooleanRecordUpdater; - stopPendingRef: RefBox>; + stopPending: SessionPendingClaim; removeTransientMessage: (sessionId: string, messageId: string) => void; toastApi: ToastApi; }): () => Promise { const { uiLocale, activeIdRef, - addPendingSessionAction, - clearPendingSessionAction, - setStopPendingBySession, - stopPendingRef, + stopPending, removeTransientMessage, toastApi, } = deps; async function stop() { const sessionId = activeIdRef.current; - if (!sessionId || !addPendingSessionAction(sessionId, stopPendingRef, setStopPendingBySession)) return; + if (!sessionId || !stopPending.claim(sessionId)) return; try { const result = await window.maka.sessions.stop(sessionId, { source: 'stop_button' }); if (result?.kind === 'interrupted') { @@ -89,7 +75,7 @@ export function createAppShellStopAction(deps: { ); } } finally { - clearPendingSessionAction(sessionId, stopPendingRef, setStopPendingBySession); + stopPending.release(sessionId); } } diff --git a/apps/desktop/src/renderer/app-shell.tsx b/apps/desktop/src/renderer/app-shell.tsx index d532b01bdb..2df60e1ff4 100644 --- a/apps/desktop/src/renderer/app-shell.tsx +++ b/apps/desktop/src/renderer/app-shell.tsx @@ -353,22 +353,20 @@ function AppShellContent({ transcriptRangeRef, messageLoadPending, setMessageLoadPending, - messageRetryPendingRef, - stopPendingRef, sessionUiController, liveTurnBySessionRef, sessionEventHealthBySessionRef, setMessageLoadErrorBySession, - setMessageRetryPendingBySession, - setStopPendingBySession, + messageRetryPending, + stopPending, + permissionModePending, + sessionModelPending, setLiveTurnBySession, confirmLiveTurn, setShellRunUpdatesBySession, setInteractionBySession, setMessageQueueBySession, setSessionEventHealthBySession, - setPendingPermissionModeBySession, - setPendingSessionModelBySession, } = useAppShellSessionWorkspace(toastApi); const interactionHydrationEpochRef = useRef(new Map()); const markInteractionChanged = useCallback((sessionId: string) => { @@ -902,45 +900,16 @@ function AppShellContent({ autoClearMs: 5000, }); const pendingTurnActions = turnActionRegistry.keys; - const permissionModeChangeRegistry = useKeyedPendingRegistry(); - const sessionModelChangeRegistry = useKeyedPendingRegistry(); const pendingKeyOf = (sessionId: string, turnId: string, actionId: string) => `${sessionId}:${turnId}:${actionId}`; - function omitSessionKey(current: Record, sessionId: string): Record { - if (!(sessionId in current)) return current; - const next = { ...current }; - delete next[sessionId]; - return next; - } - - function addPendingSessionAction( - sessionId: string, - pendingRef: { current: Set }, - setPendingBySession?: (updater: (current: Record) => Record) => void, - ): boolean { - if (pendingRef.current.has(sessionId)) return false; - pendingRef.current.add(sessionId); - setPendingBySession?.((current) => ({ ...current, [sessionId]: true })); - return true; - } - - function clearPendingSessionAction( - sessionId: string, - pendingRef: { current: Set }, - setPendingBySession?: (updater: (current: Record) => Record) => void, - ): void { - if (!pendingRef.current.has(sessionId)) return; - pendingRef.current.delete(sessionId); - setPendingBySession?.((current) => omitSessionKey(current, sessionId)); - } function clearSessionRendererState(sessionId: string): void { + // `clearOwnedSessionState` ends in `clearSessionUiState`, which drops this + // session from every session-UI map — the four pending claims included. clearOwnedSessionState(sessionId); turnActionRegistry.clearForSession(sessionId); - permissionModeChangeRegistry.keysRef.current.delete(sessionId); planModeIntent.clear(sessionId); orchestrationModeIntent.clear(sessionId); - sessionModelChangeRegistry.keysRef.current.delete(sessionId); } const { @@ -952,14 +921,12 @@ function AppShellContent({ activeIdRef, connections, messages, - pendingPermissionModeChangesRef: permissionModeChangeRegistry.keysRef, - pendingSessionModelChangesRef: sessionModelChangeRegistry.keysRef, + permissionModePending, + sessionModelPending, refreshSessions, saveComposerDefaults, sessionsRef, setNewTaskPermissionMode, - setPendingPermissionModeBySession, - setPendingSessionModelBySession, toastApi, }); @@ -1753,18 +1720,15 @@ function AppShellContent({ } = useStableActions(createAppShellChatActions, { uiLocale, activeIdRef, - addPendingSessionAction, captureComposerImportOwner, checkTaskSubmissionReadiness: taskSubmissionReadyAtSend, - clearPendingSessionAction, isNewChatSendSurfaceActive, isShellSurfaceOwnerActive, - messageRetryPendingRef, + messageRetryPending, refreshSessions, activateSessionForFirstSend, setActiveId, setMessageLoadErrorBySession, - setMessageRetryPendingBySession, setMessages, addTransientMessage, updateTransientMessage, @@ -2175,10 +2139,7 @@ function AppShellContent({ const stop = createAppShellStopAction({ uiLocale, activeIdRef, - addPendingSessionAction, - clearPendingSessionAction, - setStopPendingBySession, - stopPendingRef, + stopPending, removeTransientMessage, toastApi, }); @@ -2253,8 +2214,6 @@ function AppShellContent({ handleConnectionEvent, openHelp, openSettings, - pendingPermissionModeChangesRef: permissionModeChangeRegistry.keysRef, - pendingSessionModelChangesRef: sessionModelChangeRegistry.keysRef, pendingTurnActionTimersRef: turnActionRegistry.timersRef, pendingTurnActionsRef: turnActionRegistry.keysRef, projectPickerPendingRef, diff --git a/apps/desktop/src/renderer/session-workspace-actions.ts b/apps/desktop/src/renderer/session-workspace-actions.ts index bef2637567..093ecea794 100644 --- a/apps/desktop/src/renderer/session-workspace-actions.ts +++ b/apps/desktop/src/renderer/session-workspace-actions.ts @@ -71,8 +71,6 @@ export function createSessionWorkspaceActions(deps: { transientMessagesBySessionRef: RefBox>>; transcriptRangeRef: RefBox; selectionRevisionRef: RefBox; - messageRetryPendingRef: RefBox>; - stopPendingRef: RefBox>; setActiveIdState: (next: string | undefined) => void; setMessagesState: (next: StoredMessage[]) => void; setTransientMessagesState: (next: TransientUserMessage[]) => void; @@ -85,8 +83,6 @@ export function createSessionWorkspaceActions(deps: { transientMessagesBySessionRef, transcriptRangeRef, selectionRevisionRef, - messageRetryPendingRef, - stopPendingRef, setActiveIdState, setMessagesState, setTransientMessagesState, @@ -220,8 +216,6 @@ export function createSessionWorkspaceActions(deps: { } function clearOwnedSessionState(sessionId: string): void { - messageRetryPendingRef.current.delete(sessionId); - stopPendingRef.current.delete(sessionId); transientMessagesBySessionRef.current.delete(sessionId); if (activeIdRef.current === sessionId) setTransientMessagesState([]); clearSessionUiState(sessionId); diff --git a/apps/desktop/src/renderer/use-app-shell-session-workspace.ts b/apps/desktop/src/renderer/use-app-shell-session-workspace.ts index d607d11621..6f90522772 100644 --- a/apps/desktop/src/renderer/use-app-shell-session-workspace.ts +++ b/apps/desktop/src/renderer/use-app-shell-session-workspace.ts @@ -55,8 +55,6 @@ export function useAppShellSessionWorkspace(toastApi: ToastApi) { ); const transcriptRangeRef = useRef(undefined); const [messageLoadPending, setMessageLoadPending] = useState(false); - const messageRetryPendingRef = useRef>(new Set()); - const stopPendingRef = useRef>(new Set()); const actionsRef = useRef(null); // Every dep below is a ref box, a state setter, or a method of the @@ -69,8 +67,6 @@ export function useAppShellSessionWorkspace(toastApi: ToastApi) { transientMessagesBySessionRef, transcriptRangeRef, selectionRevisionRef, - messageRetryPendingRef, - stopPendingRef, setActiveIdState, setMessagesState: setMessages, setTransientMessagesState: setTransientMessages, @@ -99,21 +95,19 @@ export function useAppShellSessionWorkspace(toastApi: ToastApi) { transcriptRangeRef, messageLoadPending, setMessageLoadPending, - messageRetryPendingRef, - stopPendingRef, sessionUiController: sessionUi.controller, liveTurnBySessionRef: sessionUi.liveTurnBySessionRef, sessionEventHealthBySessionRef: sessionUi.sessionEventHealthBySessionRef, setMessageLoadErrorBySession: sessionUi.setMessageLoadErrorBySession, - setMessageRetryPendingBySession: sessionUi.setMessageRetryPendingBySession, - setStopPendingBySession: sessionUi.setStopPendingBySession, + messageRetryPending: sessionUi.messageRetryPending, + stopPending: sessionUi.stopPending, + permissionModePending: sessionUi.permissionModePending, + sessionModelPending: sessionUi.sessionModelPending, setLiveTurnBySession: sessionUi.setLiveTurnBySession, setShellRunUpdatesBySession: sessionUi.setShellRunUpdatesBySession, setInteractionBySession: sessionUi.setInteractionBySession, setMessageQueueBySession: sessionUi.setMessageQueueBySession, setSessionEventHealthBySession: sessionUi.setSessionEventHealthBySession, - setPendingPermissionModeBySession: sessionUi.setPendingPermissionModeBySession, - setPendingSessionModelBySession: sessionUi.setPendingSessionModelBySession, confirmLiveTurn: sessionUi.confirmLiveTurn, }; } diff --git a/apps/desktop/src/renderer/use-turn-action-registry.ts b/apps/desktop/src/renderer/use-turn-action-registry.ts new file mode 100644 index 0000000000..a336323226 --- /dev/null +++ b/apps/desktop/src/renderer/use-turn-action-registry.ts @@ -0,0 +1,106 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { useRef, useState } from 'react'; + +type RefBox = { current: T }; + +/** + * How long a turn-footer button stays disabled without a confirming + * `sessions:changed` event. Long enough that the normal round trip wins the + * race, short enough that a dropped event does not disable the button forever. + */ +const AUTO_CLEAR_MS = 5000; + +/** + * The in-flight keys behind the turn-footer buttons' disabled mask. + * + * A key is `${sessionId}:${turnId}:${actionId}`, claimed on click and released + * when the Host confirms — so a second click while the first request settles is + * ignored. `keys` drives the mask through the turn presentation derivation; + * `keysRef` is the same content for synchronous reads. + * + * Deliberately concrete. This began as a generic registry with `trackState` and + * `autoClearMs` options, serving these keys plus per-session permission-mode + * and model changes. Those two moved to the session UI store, which already + * held their rendered flag, and the options had no variation left: the timers + * and the state mirror exist for the turn footer alone. + */ +export interface TurnActionRegistry { + /** Reactive snapshot of the pending keys, for the disabled mask. */ + keys: Set; + /** Stable mirror of the pending keys for synchronous reads. */ + keysRef: RefBox>; + /** + * Marks `key` pending and arms its auto-clear timer. Returns false (a no-op) + * if it was already pending, so callers can bail on a duplicate action. + */ + addKey(key: string): boolean; + /** Clears a single pending key along with its timer and snapshot entry. */ + clearKey(key: string): void; + /** Clears every pending key prefixed with `${sessionId}:` (session teardown). */ + clearForSession(sessionId: string): void; + /** Clears every key and timer (unmount, or a Runtime Host generation change). */ + clearAll(): void; +} + +export function useTurnActionRegistry(): TurnActionRegistry { + const [keys, setKeys] = useState>(() => new Set()); + const keysRef = useRef>(new Set()); + const timersRef = useRef>>(new Map()); + const controllerRef = useRef | null>(null); + + if (!controllerRef.current) { + const syncState = (): void => { + setKeys(new Set(keysRef.current)); + }; + const clearKey = (key: string): void => { + if (!keysRef.current.has(key)) return; + keysRef.current.delete(key); + const timeoutHandle = timersRef.current.get(key); + if (timeoutHandle) clearTimeout(timeoutHandle); + timersRef.current.delete(key); + syncState(); + }; + const addKey = (key: string): boolean => { + if (keysRef.current.has(key)) return false; + keysRef.current.add(key); + syncState(); + timersRef.current.set(key, setTimeout(() => clearKey(key), AUTO_CLEAR_MS)); + return true; + }; + const clearForSession = (sessionId: string): void => { + const prefix = `${sessionId}:`; + for (const key of Array.from(keysRef.current)) { + if (key.startsWith(prefix)) clearKey(key); + } + }; + const clearAll = (): void => { + for (const timeoutHandle of timersRef.current.values()) { + clearTimeout(timeoutHandle); + } + timersRef.current.clear(); + keysRef.current.clear(); + syncState(); + }; + controllerRef.current = { keysRef, addKey, clearKey, clearForSession, clearAll }; + } + + return { keys, ...controllerRef.current }; +} From 801899bc0a0cf61037c8bfc3944021f0f263e644 Mon Sep 17 00:00:00 2001 From: AstroHan Date: Sat, 29 Aug 2026 03:19:14 +0800 Subject: [PATCH 2/4] refactor(desktop): collapse the pending registry onto the turn footer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `useKeyedPendingRegistry` was generic over `trackState` and `autoClearMs` because it served three instances. Two of them moved to the session UI store, and both options had only ever had one consumer: the turn footer needs the reactive snapshot for its disabled mask and the timers so a dropped `sessions:changed` cannot disable a button forever. With no variation left, the options object, the `trackState` branch, and the seeded-but-unused `keys` on a ref-only registry are generality nothing asks for. `clearAll()` had no caller. The unmount cleanup in `app-shell-effects` walked `timersRef` and `keysRef` itself — the same work, reaching around the method that exists to do it. It now calls `clearAll()`, which is why `timersRef` no longer needs to be public. Generated-by: Claude Code --- .../desktop/src/renderer/app-shell-effects.ts | 9 +- apps/desktop/src/renderer/app-shell.tsx | 15 +-- .../renderer/use-pending-action-registry.ts | 122 ------------------ 3 files changed, 7 insertions(+), 139 deletions(-) delete mode 100644 apps/desktop/src/renderer/use-pending-action-registry.ts diff --git a/apps/desktop/src/renderer/app-shell-effects.ts b/apps/desktop/src/renderer/app-shell-effects.ts index 24fddcc8f3..fa2fd09a1f 100644 --- a/apps/desktop/src/renderer/app-shell-effects.ts +++ b/apps/desktop/src/renderer/app-shell-effects.ts @@ -159,8 +159,7 @@ export function useAppShellBootstrapSubscriptions(options: { handleConnectionEvent: (event: ConnectionEvent) => void; openHelp: () => void; openSettings: () => void; - pendingTurnActionTimersRef: RefBox>>; - pendingTurnActionsRef: RefBox>; + clearPendingTurnActions: () => void; projectPickerPendingRef: RefBox; projectPickerRequestRef: RefBox; refreshConnections: () => Promise; @@ -286,11 +285,7 @@ export function useAppShellBootstrapSubscriptions(options: { options.rendererMountedRef.current = false; options.projectPickerRequestRef.current += 1; options.projectPickerPendingRef.current = false; - for (const timeoutHandle of options.pendingTurnActionTimersRef.current.values()) { - clearTimeout(timeoutHandle); - } - options.pendingTurnActionTimersRef.current.clear(); - options.pendingTurnActionsRef.current.clear(); + options.clearPendingTurnActions(); }); useEffect(() => { diff --git a/apps/desktop/src/renderer/app-shell.tsx b/apps/desktop/src/renderer/app-shell.tsx index 2df60e1ff4..9f5aee7c73 100644 --- a/apps/desktop/src/renderer/app-shell.tsx +++ b/apps/desktop/src/renderer/app-shell.tsx @@ -201,7 +201,7 @@ import { type LiveContentSeed, } from './live-content-seed'; import { loadComposerDefaults, saveComposerDefaults } from './composer-defaults'; -import { useKeyedPendingRegistry } from './use-pending-action-registry'; +import { useTurnActionRegistry } from './use-turn-action-registry'; import { useComposerAttachments } from './use-composer-attachments'; import { useAppShellComposerQuotes } from './use-app-shell-composer-quotes'; import { useComposerMentions } from './use-composer-mentions'; @@ -892,13 +892,9 @@ function AppShellContent({ // mask. Per @kenji PR109d review: pending state prevents double-click // duplicate sibling turns by disabling the action button between // click and `sessions:changed turn-status-change` arriving. - // These de-dup registries (turn-footer actions and per-session permission-mode - // / model changes) share the same keyed-Set shape; see - // useKeyedPendingRegistry. Session-row mutations live in Session Navigation. - const turnActionRegistry = useKeyedPendingRegistry({ - trackState: true, - autoClearMs: 5000, - }); + // Session-row mutations live in Session Navigation; the per-session mode and + // model claims live in the session UI store. + const turnActionRegistry = useTurnActionRegistry(); const pendingTurnActions = turnActionRegistry.keys; const pendingKeyOf = (sessionId: string, turnId: string, actionId: string) => `${sessionId}:${turnId}:${actionId}`; @@ -2214,8 +2210,7 @@ function AppShellContent({ handleConnectionEvent, openHelp, openSettings, - pendingTurnActionTimersRef: turnActionRegistry.timersRef, - pendingTurnActionsRef: turnActionRegistry.keysRef, + clearPendingTurnActions: turnActionRegistry.clearAll, projectPickerPendingRef, projectPickerRequestRef, refreshConnections: refreshConnectionProjections, diff --git a/apps/desktop/src/renderer/use-pending-action-registry.ts b/apps/desktop/src/renderer/use-pending-action-registry.ts deleted file mode 100644 index a16fbee0ed..0000000000 --- a/apps/desktop/src/renderer/use-pending-action-registry.ts +++ /dev/null @@ -1,122 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -import { useRef, useState } from 'react'; - -type RefBox = { current: T }; - -/** - * Generic keyed-pending registry shared by AppShell's four hand-rolled - * de-dup sets (turn-footer actions, session-row actions, per-session - * permission-mode changes, per-session model changes). Each tracks a Set of - * in-flight keys so a second click while the first request is still settling - * is ignored, then clears the key when the backend confirms. - * - * The four instances differ only along two axes, both opt-in: - * - * - `trackState` — the turn-footer registry drives a React-visible disabled - * mask (the turn presentation derivation reads `keys`), so it mirrors the ref - * into state on every mutation. The three ref-only registries pass their - * `keysRef` straight to their action factories / the bootstrap cleanup - * effect and never re-render, so they skip the state mirror entirely. - * - `autoClearMs` — the turn-footer registry arms a per-key timeout so a - * button re-enables even if the confirming `sessions:changed` event is - * dropped. The others clear synchronously in their action's `finally`. - * - * `keysRef` is always a stable Set instance so the action factories that - * mutate it directly (session-row-actions, session-settings-actions) and the - * unmount cleanup (app-shell-effects) keep operating on the same object. - */ -export interface KeyedPendingRegistry { - /** - * Reactive snapshot of the pending keys. Only refreshed when the registry - * is created with `trackState: true`; otherwise stays the empty seed and - * callers read `keysRef` instead. - */ - keys: Set; - /** Stable mirror of the pending keys for synchronous reads / external mutation. */ - keysRef: RefBox>; - /** Per-key auto-clear timers. Empty unless the registry sets `autoClearMs`. */ - timersRef: RefBox>>; - /** - * Marks `key` pending. Returns false (a no-op) if it was already pending so - * callers can bail on a duplicate action. Bumps the reactive snapshot and - * arms the auto-clear timer when the registry is configured for them. - */ - addKey(key: string): boolean; - /** Clears a single pending key along with its timer and snapshot entry. */ - clearKey(key: string): void; - /** Clears every pending key prefixed with `${sessionId}:` (session teardown). */ - clearForSession(sessionId: string): void; - /** Clears the registry when its owning Runtime Host generation changes. */ - clearAll(): void; -} - -export function useKeyedPendingRegistry(options?: { - trackState?: boolean; - autoClearMs?: number; -}): KeyedPendingRegistry { - const trackState = options?.trackState ?? false; - const autoClearMs = options?.autoClearMs; - const [keys, setKeys] = useState>(() => new Set()); - const keysRef = useRef>(new Set()); - const timersRef = useRef>>(new Map()); - const controllerRef = useRef | null>(null); - - if (!controllerRef.current) { - const syncState = (): void => { - if (trackState) setKeys(new Set(keysRef.current)); - }; - const clearKey = (key: string): void => { - if (!keysRef.current.has(key)) return; - keysRef.current.delete(key); - const timeoutHandle = timersRef.current.get(key); - if (timeoutHandle) clearTimeout(timeoutHandle); - timersRef.current.delete(key); - syncState(); - }; - const addKey = (key: string): boolean => { - if (keysRef.current.has(key)) return false; - keysRef.current.add(key); - syncState(); - if (autoClearMs !== undefined) { - const timeoutHandle = setTimeout(() => clearKey(key), autoClearMs); - timersRef.current.set(key, timeoutHandle); - } - return true; - }; - const clearForSession = (sessionId: string): void => { - const prefix = `${sessionId}:`; - for (const key of Array.from(keysRef.current)) { - if (key.startsWith(prefix)) clearKey(key); - } - }; - const clearAll = (): void => { - for (const timeoutHandle of timersRef.current.values()) { - clearTimeout(timeoutHandle); - } - timersRef.current.clear(); - keysRef.current.clear(); - syncState(); - }; - controllerRef.current = { keysRef, timersRef, addKey, clearKey, clearForSession, clearAll }; - } - - return { keys, ...controllerRef.current }; -} From bd23e59276ecd286a6e8f0bc36119d8c911503bf Mon Sep 17 00:00:00 2001 From: AstroHan Date: Sat, 29 Aug 2026 03:20:58 +0800 Subject: [PATCH 3/4] refactor(desktop): reach the session UI store through its controller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The controller's surface was restated twice on the way to its consumers: `useAppShellSessionUiState` returned the controller plus a member-by-member copy of it, `useAppShellSessionWorkspace` copied that copy, and AppShell destructured the result. Adding a map to the store meant editing three lists that nothing keeps in agreement, and the copies carried no semantics of their own — `sessionUiController` was already in the same destructure. The hook now returns the controller. Call sites name it, which is longer to read and shorter to keep true. Generated-by: Claude Code --- .../renderer/app-shell-session-ui-state.ts | 34 +++------- apps/desktop/src/renderer/app-shell.tsx | 64 ++++++++----------- .../use-app-shell-session-workspace.ts | 25 ++------ 3 files changed, 42 insertions(+), 81 deletions(-) diff --git a/apps/desktop/src/renderer/app-shell-session-ui-state.ts b/apps/desktop/src/renderer/app-shell-session-ui-state.ts index d7464d9537..ae0115175e 100644 --- a/apps/desktop/src/renderer/app-shell-session-ui-state.ts +++ b/apps/desktop/src/renderer/app-shell-session-ui-state.ts @@ -263,32 +263,14 @@ export type AppShellSessionUiStateController = ReturnType(null); - - if (!controllerRef.current) { - controllerRef.current = createAppShellSessionUiStateController(); - } - - const controller = controllerRef.current; - - return { - controller, - liveTurnBySessionRef: controller.liveTurnBySessionRef, - sessionEventHealthBySessionRef: controller.sessionEventHealthBySessionRef, - setMessageLoadErrorBySession: controller.setMessageLoadErrorBySession, - messageRetryPending: controller.messageRetryPending, - stopPending: controller.stopPending, - permissionModePending: controller.permissionModePending, - sessionModelPending: controller.sessionModelPending, - setLiveTurnBySession: controller.setLiveTurnBySession, - setShellRunUpdatesBySession: controller.setShellRunUpdatesBySession, - setInteractionBySession: controller.setInteractionBySession, - setMessageQueueBySession: controller.setMessageQueueBySession, - setSessionEventHealthBySession: controller.setSessionEventHealthBySession, - confirmLiveTurn: controller.confirmLiveTurn, - clearSessionUiState: controller.clearSessionUiState, - clearTurnTransientStateIfCurrent: controller.clearTurnTransientStateIfCurrent, - }; + controllerRef.current ??= createAppShellSessionUiStateController(); + return controllerRef.current; } diff --git a/apps/desktop/src/renderer/app-shell.tsx b/apps/desktop/src/renderer/app-shell.tsx index 9f5aee7c73..98822a0e7e 100644 --- a/apps/desktop/src/renderer/app-shell.tsx +++ b/apps/desktop/src/renderer/app-shell.tsx @@ -354,19 +354,6 @@ function AppShellContent({ messageLoadPending, setMessageLoadPending, sessionUiController, - liveTurnBySessionRef, - sessionEventHealthBySessionRef, - setMessageLoadErrorBySession, - messageRetryPending, - stopPending, - permissionModePending, - sessionModelPending, - setLiveTurnBySession, - confirmLiveTurn, - setShellRunUpdatesBySession, - setInteractionBySession, - setMessageQueueBySession, - setSessionEventHealthBySession, } = useAppShellSessionWorkspace(toastApi); const interactionHydrationEpochRef = useRef(new Map()); const markInteractionChanged = useCallback((sessionId: string) => { @@ -917,8 +904,8 @@ function AppShellContent({ activeIdRef, connections, messages, - permissionModePending, - sessionModelPending, + permissionModePending: sessionUiController.permissionModePending, + sessionModelPending: sessionUiController.sessionModelPending, refreshSessions, saveComposerDefaults, sessionsRef, @@ -1201,22 +1188,22 @@ function AppShellContent({ ) { return; } - setInteractionBySession((current) => reconcileInteractions(current, activeId, requests)); + sessionUiController.setInteractionBySession((current) => reconcileInteractions(current, activeId, requests)); }) .catch(() => {}); return () => { cancelled = true; }; - }, [activeId, setInteractionBySession]); + }, [activeId, sessionUiController.setInteractionBySession]); useEffect( () => window.maka.sessions.subscribeActiveInteractions(({ sessionId, interactions }) => { markInteractionChanged(sessionId); - setInteractionBySession((current) => + sessionUiController.setInteractionBySession((current) => reconcileInteractions(current, sessionId, interactions), ); }), - [markInteractionChanged, setInteractionBySession], + [markInteractionChanged, sessionUiController.setInteractionBySession], ); const activeBoundarySurface = deriveDesktopExecutionBoundarySurface( activeId, @@ -1720,18 +1707,18 @@ function AppShellContent({ checkTaskSubmissionReadiness: taskSubmissionReadyAtSend, isNewChatSendSurfaceActive, isShellSurfaceOwnerActive, - messageRetryPending, + messageRetryPending: sessionUiController.messageRetryPending, refreshSessions, activateSessionForFirstSend, setActiveId, - setMessageLoadErrorBySession, + setMessageLoadErrorBySession: sessionUiController.setMessageLoadErrorBySession, setMessages, addTransientMessage, updateTransientMessage, removeTransientMessage, transcriptRangeRef, - setLiveTurnBySession, - setInteractionBySession, + setLiveTurnBySession: sessionUiController.setLiveTurnBySession, + setInteractionBySession: sessionUiController.setInteractionBySession, onInteractionChanged: markInteractionChanged, onExecutionBoundaryChanged: reloadActiveExecutionBoundary, showModelSetupToast, @@ -1873,7 +1860,7 @@ function AppShellContent({ metadata?.workspaceFileReferences, sessionId ? retractedWorkspaceReferencesRef.current[sessionId] : undefined, ); - const liveTurn = sessionId ? liveTurnBySessionRef.current[sessionId] : undefined; + const liveTurn = sessionId ? sessionUiController.liveTurnBySessionRef.current[sessionId] : undefined; const runningTurnIds = sessionId ? sessionsRef.current.find((session) => session.id === sessionId)?.runningTurnIds : undefined; @@ -2135,7 +2122,7 @@ function AppShellContent({ const stop = createAppShellStopAction({ uiLocale, activeIdRef, - stopPending, + stopPending: sessionUiController.stopPending, removeTransientMessage, toastApi, }); @@ -2151,12 +2138,12 @@ function AppShellContent({ } = useStableActions(createAppShellSessionEventHandlers, { uiLocale, activeIdRef, - liveTurnBySessionRef, + liveTurnBySessionRef: sessionUiController.liveTurnBySessionRef, refreshMessages, refreshSessions, - setLiveTurnBySession, - setInteractionBySession, - setMessageQueueBySession, + setLiveTurnBySession: sessionUiController.setLiveTurnBySession, + setInteractionBySession: sessionUiController.setInteractionBySession, + setMessageQueueBySession: sessionUiController.setMessageQueueBySession, projectQueuedTransientMessages, removeTransientMessage, displayBatch: sessionDisplayBatch, @@ -2204,7 +2191,7 @@ function AppShellContent({ applyE2eFixture, bootstrapSessions, clearPendingTurnActionsForSession: turnActionRegistry.clearForSession, - confirmLiveTurn, + confirmLiveTurn: sessionUiController.confirmLiveTurn, clearSessionRendererState, createSession, handleConnectionEvent, @@ -2222,7 +2209,7 @@ function AppShellContent({ rendererMountedRef, setActiveId, setMessages, - setSessionEventHealthBySession, + setSessionEventHealthBySession: sessionUiController.setSessionEventHealthBySession, toastApi, }); useAppShellPersistenceEffects({ @@ -2264,11 +2251,11 @@ function AppShellContent({ handleEvent, beginObservationSeed, completeObservationSeed, - setMessageLoadErrorBySession, + setMessageLoadErrorBySession: sessionUiController.setMessageLoadErrorBySession, setMessageLoadPending, setMessages, transcriptRangeRef, - setSessionEventHealthBySession, + setSessionEventHealthBySession: sessionUiController.setSessionEventHealthBySession, toastApi, }); let newestDurablePromptSequence: number | null = null; @@ -2327,7 +2314,7 @@ function AppShellContent({ }) .catch((error) => { if (disposed || activeIdRef.current !== target.sessionId) return; - setMessageLoadErrorBySession((current) => ({ + sessionUiController.setMessageLoadErrorBySession((current) => ({ ...current, [target.sessionId]: localizedShellErrorMessage( error, @@ -2340,7 +2327,10 @@ function AppShellContent({ disposed = true; }; }, [activeId, searchScrollTarget?.nonce]); - useShellRunUpdates({ activeId, setShellRunUpdatesBySession }); + useShellRunUpdates({ + activeId, + setShellRunUpdatesBySession: sessionUiController.setShellRunUpdatesBySession, + }); useSessionEventHealthPolling({ activeId, activeInteraction, @@ -2349,8 +2339,8 @@ function AppShellContent({ hasInFlightLiveTools, refreshMessages, refreshSessions, - sessionEventHealthBySessionRef, - setSessionEventHealthBySession, + sessionEventHealthBySessionRef: sessionUiController.sessionEventHealthBySessionRef, + setSessionEventHealthBySession: sessionUiController.setSessionEventHealthBySession, }); function captureComposerImportOwner(): ComposerImportOwner { return { diff --git a/apps/desktop/src/renderer/use-app-shell-session-workspace.ts b/apps/desktop/src/renderer/use-app-shell-session-workspace.ts index 6f90522772..f8ccc337b6 100644 --- a/apps/desktop/src/renderer/use-app-shell-session-workspace.ts +++ b/apps/desktop/src/renderer/use-app-shell-session-workspace.ts @@ -39,11 +39,11 @@ type TransientUserMessage = TransientUserMessageProjection; export function useAppShellSessionWorkspace(toastApi: ToastApi) { const [activeId, setActiveIdState] = useState(); const activeIdRef = useRef(undefined); - const sessionUi = useAppShellSessionUiState(); + const sessionUiController = useAppShellSessionUiState(); const sessionList = useAppShellSessionList(toastApi, { activeIdRef, - liveTurnBySessionRef: sessionUi.liveTurnBySessionRef, - clearTurnTransientStateIfCurrent: sessionUi.clearTurnTransientStateIfCurrent, + liveTurnBySessionRef: sessionUiController.liveTurnBySessionRef, + clearTurnTransientStateIfCurrent: sessionUiController.clearTurnTransientStateIfCurrent, }); const selectionRevisionRef = useRef(0); const bootstrapSelectionLeaseRef = useRef | null>(null); @@ -71,7 +71,7 @@ export function useAppShellSessionWorkspace(toastApi: ToastApi) { setMessagesState: setMessages, setTransientMessagesState: setTransientMessages, setMessageLoadPending, - clearSessionUiState: sessionUi.clearSessionUiState, + clearSessionUiState: sessionUiController.clearSessionUiState, }); const actions = actionsRef.current; @@ -95,19 +95,8 @@ export function useAppShellSessionWorkspace(toastApi: ToastApi) { transcriptRangeRef, messageLoadPending, setMessageLoadPending, - sessionUiController: sessionUi.controller, - liveTurnBySessionRef: sessionUi.liveTurnBySessionRef, - sessionEventHealthBySessionRef: sessionUi.sessionEventHealthBySessionRef, - setMessageLoadErrorBySession: sessionUi.setMessageLoadErrorBySession, - messageRetryPending: sessionUi.messageRetryPending, - stopPending: sessionUi.stopPending, - permissionModePending: sessionUi.permissionModePending, - sessionModelPending: sessionUi.sessionModelPending, - setLiveTurnBySession: sessionUi.setLiveTurnBySession, - setShellRunUpdatesBySession: sessionUi.setShellRunUpdatesBySession, - setInteractionBySession: sessionUi.setInteractionBySession, - setMessageQueueBySession: sessionUi.setMessageQueueBySession, - setSessionEventHealthBySession: sessionUi.setSessionEventHealthBySession, - confirmLiveTurn: sessionUi.confirmLiveTurn, + // The store's own surface, not a copy of it. Consumers reach setters and + // claims through the controller. + sessionUiController, }; } From a8d68f428d8ea70f7e6db9a1b24de0585a05ddd4 Mon Sep 17 00:00:00 2001 From: AstroHan Date: Sat, 29 Aug 2026 03:22:18 +0800 Subject: [PATCH 4/4] test(desktop): assert the stable-actions identity contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `stable-actions.ts` was split from its hook on the stated grounds that keeping it React-free made it testable from `node:test`. No such test was ever written, and `createDelegatingActions` had exactly one consumer. The claim is now honoured the other way round: the facade moves in with `useStableActions` and the contract is asserted through the hook, where React's commit semantics are part of what is being promised. The test covers all nine call sites by construction, because it constrains the mechanism rather than each factory: identities fixed across renders, and calls delegating to the latest committed closures. It fails on `return actions`. What it cannot catch is a factory that goes through neither this hook nor a once-created object — a bare function declaration in a hook body passes types and `useExhaustiveDependencies` alike. That gap is why the Session rail carries an outcome budget as well, and the comment in `session-workspace-actions.ts` that argued for one mechanism over the other now defers to the tests instead. Generated-by: Claude Code --- .../main/__tests__/use-stable-actions.test.ts | 74 +++++++++++++++++++ .../src/renderer/session-workspace-actions.ts | 16 ++-- apps/desktop/src/renderer/stable-actions.ts | 44 ----------- .../src/renderer/use-stable-actions.ts | 22 +++++- 4 files changed, 104 insertions(+), 52 deletions(-) create mode 100644 apps/desktop/src/main/__tests__/use-stable-actions.test.ts delete mode 100644 apps/desktop/src/renderer/stable-actions.ts diff --git a/apps/desktop/src/main/__tests__/use-stable-actions.test.ts b/apps/desktop/src/main/__tests__/use-stable-actions.test.ts new file mode 100644 index 0000000000..b6c708f779 --- /dev/null +++ b/apps/desktop/src/main/__tests__/use-stable-actions.test.ts @@ -0,0 +1,74 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { strict as assert } from 'node:assert'; +import { afterEach, describe, it } from 'node:test'; +import { act, createElement, useState } from 'react'; +import { cleanupFakeDom, installReactRenderer } from './fake-dom.js'; +import { useStableActions } from '../../renderer/use-stable-actions.js'; + +/** + * Action identity is a contract in this renderer, not an implementation + * detail. AppShell's nine `useStableActions` call sites hand their results to + * consumers that list them in dependency arrays and pass them down as props; + * a per-render identity there re-arms effect timers and defeats `memo` on + * every commit — measured at 20 full re-renders of a 32-row sidebar for one + * session switch (#4109). + * + * Asserting it on the mechanism covers all nine by construction. What it + * cannot cover is a factory that goes through neither this hook nor a + * once-created object; that failure is invisible to types and to + * `useExhaustiveDependencies`, which is why the Session rail also carries an + * outcome budget in `session-rail-render-contract.spec.ts`. + */ +describe('useStableActions', () => { + afterEach(cleanupFakeDom); + + it('fixes action identities while delegating to the latest committed closures', () => { + const { root } = installReactRenderer(); + const identities: Array<{ report(): number }> = []; + let bump: ((next: number) => void) | undefined; + + function Probe(): null { + const [value, setValue] = useState(0); + bump = setValue; + // A factory whose closure genuinely captures a changing dep — the case + // the facade exists for. + identities.push( + useStableActions((deps: { value: number }) => ({ report: () => deps.value }), { value }), + ); + return null; + } + + act(() => { + root.render(createElement(Probe)); + }); + act(() => bump?.(1)); + act(() => bump?.(2)); + + assert.equal(identities.length, 3); + const [first] = identities; + assert.ok(first); + for (const actions of identities) { + assert.equal(actions, first, 'the facade itself is re-created'); + assert.equal(actions.report, first.report, 'a method identity changed between renders'); + } + assert.equal(first.report(), 2, 'the facade did not delegate to the latest committed render'); + }); +}); diff --git a/apps/desktop/src/renderer/session-workspace-actions.ts b/apps/desktop/src/renderer/session-workspace-actions.ts index 093ecea794..503919e2d5 100644 --- a/apps/desktop/src/renderer/session-workspace-actions.ts +++ b/apps/desktop/src/renderer/session-workspace-actions.ts @@ -22,13 +22,15 @@ * list, and the transient (optimistic) message projection. * * Every dependency here is a ref box, a React state setter, or a method of the - * once-created session-UI controller — all fixed for the renderer's lifetime. - * The factory therefore runs ONCE (issue #1043's `useStableActions` exists for - * factories whose closures capture changing deps; these capture none, so the - * cheaper structural guarantee applies). Declaring these in the hook body - * instead handed every consumer a fresh identity per render, and - * `activateSession` alone rebuilt the Session rail's whole command chain, which - * defeated `SessionNavRow`'s `memo` on every commit. + * once-created session-UI controller — all fixed for the renderer's lifetime — + * so the factory runs ONCE and the identities follow structurally. + * + * Declaring these in the hook body instead handed every consumer a fresh + * identity per render, and `activateSession` alone rebuilt the Session rail's + * whole command chain, which defeated `SessionNavRow`'s `memo` on every commit. + * That is asserted in `session-workspace-action-identity.test.ts`; whether a + * factory earns its identity this way or through `useStableActions` is an + * implementation choice the test does not care about. */ import type { StoredMessage } from '@maka/core/session'; diff --git a/apps/desktop/src/renderer/stable-actions.ts b/apps/desktop/src/renderer/stable-actions.ts deleted file mode 100644 index 056c23bceb..0000000000 --- a/apps/desktop/src/renderer/stable-actions.ts +++ /dev/null @@ -1,44 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -/** - * Delegating-actions facade behind `useStableActions` (issue #1043). - * - * App-shell action factories run in the render body, so every render allocates - * fresh handler identities. Any consumer that lists a handler in an effect dep - * array (the streaming-settle fallback timer did) tears down and re-arms on - * every render. The facade fixes the identity without freezing the closures: - * each method delegates to the latest committed render's factory result through - * `latestRef`, so calls always see fresh deps while consumers see one stable - * identity for the component's lifetime. - * - * Factories must return a constant key shape of function values — the - * facade's key set is fixed at creation. Kept React-free so the behavior is - * testable from `node:test`. - */ -export function createDelegatingActions(latestRef: { current: A }): A { - const facade: Record unknown> = {}; - for (const key of Object.keys(latestRef.current)) { - facade[key] = (...args: unknown[]) => { - const action = Reflect.get(latestRef.current, key) as unknown as (...args: unknown[]) => unknown; - return action(...args); - }; - } - return facade as unknown as A; -} diff --git a/apps/desktop/src/renderer/use-stable-actions.ts b/apps/desktop/src/renderer/use-stable-actions.ts index 57b73d27f6..18a901c654 100644 --- a/apps/desktop/src/renderer/use-stable-actions.ts +++ b/apps/desktop/src/renderer/use-stable-actions.ts @@ -18,7 +18,24 @@ */ import { useLayoutEffect, useRef, useState } from 'react'; -import { createDelegatingActions } from './stable-actions.js'; + +/** + * The facade itself: one method per key, each forwarding to whatever + * `latestRef` holds when it is called. + * + * Factories must return a constant key shape of function values — the key set + * is fixed at creation. + */ +function createDelegatingActions(latestRef: { current: A }): A { + const facade: Record unknown> = {}; + for (const key of Object.keys(latestRef.current)) { + facade[key] = (...args: unknown[]) => { + const action = Reflect.get(latestRef.current, key) as unknown as (...args: unknown[]) => unknown; + return action(...args); + }; + } + return facade as unknown as A; +} /** * Runs an app-shell action factory in the render body but returns a stable @@ -35,6 +52,9 @@ import { createDelegatingActions } from './stable-actions.js'; * * `createAppShellStopAction` is deliberately NOT wrapped: it returns a bare * function (no object to facade) and only feeds JSX props, never effect deps. + * + * The identity guarantee is asserted in `use-stable-actions.test.ts` rather + * than argued for in review — see that file for why this is a contract. */ export function useStableActions(factory: (deps: D) => A, deps: D): A { const actions = factory(deps);