Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>() },
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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>() {
let resolve!: (value: T) => void;
Expand Down Expand Up @@ -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<string, boolean>): 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<boolean>;
connections?: LlmConnection[];
Expand All @@ -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<string>();
const pendingBySession: Record<string, boolean> = {};
const permissionModePending: Record<string, boolean> = {};
const sessionModelPending: Record<string, boolean> = {};
const modelCalls: string[] = [];
const permissionCalls: string[] = [];
const thinkingCalls: string[] = [];
Expand Down Expand Up @@ -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) => {
Expand All @@ -137,8 +146,8 @@ function createHarness(options: {
modelCalls,
modelResult,
newTaskPermissionModes,
pending,
pendingBySession,
permissionModePending,
sessionModelPending,
permissionCalls,
sessionsRef,
thinkingCalls,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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 () => {
Expand All @@ -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' }]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>() },
stopPending: { claim: () => true, release: () => undefined },
removeTransientMessage: (sessionId, messageId) => removed.push({ sessionId, messageId }),
toastApi: { error() {} },
});
Expand Down
74 changes: 74 additions & 0 deletions apps/desktop/src/main/__tests__/use-stable-actions.test.ts
Original file line number Diff line number Diff line change
@@ -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');
});
});
24 changes: 5 additions & 19 deletions apps/desktop/src/renderer/app-shell-chat-actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -78,7 +79,6 @@ type ComposerImportOwner = {
};

type RefBox<T> = { current: T };
type BooleanRecordUpdater = (updater: (current: Record<string, boolean>) => Record<string, boolean>) => void;
type LiveTurnRecordUpdater = (
updater: (current: Record<string, LiveTurnProjection>) => Record<string, LiveTurnProjection>,
) => void;
Expand Down Expand Up @@ -139,29 +139,18 @@ export interface AppShellChatActions {
export function createAppShellChatActions(deps: {
uiLocale: UiLocale;
activeIdRef: RefBox<string | undefined>;
addPendingSessionAction: (
sessionId: string,
pendingRef: RefBox<Set<string>>,
setPendingBySession: BooleanRecordUpdater,
) => boolean;
captureComposerImportOwner: () => ComposerImportOwner;
checkTaskSubmissionReadiness: () => Promise<boolean>;
clearPendingSessionAction: (
sessionId: string,
pendingRef: RefBox<Set<string>>,
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<Set<string>>;
messageRetryPending: SessionPendingClaim;
refreshSessions: () => Promise<DesktopSessionSummary[]>;
activateSessionForFirstSend: (sessionId: string) => Promise<void>;
setActiveId: (sessionId: string | undefined) => void;
setMessageLoadErrorBySession: MessageLoadErrorUpdater;
setMessageRetryPendingBySession: BooleanRecordUpdater;
setMessages: MessageListUpdater;
addTransientMessage: (
sessionId: string,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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();
Expand All @@ -830,7 +816,7 @@ export function createAppShellChatActions(deps: {
}));
toastApi.error(copy.refreshFailedTitle, message, undefined, { sessionId });
} finally {
clearPendingSessionAction(sessionId, messageRetryPendingRef, setMessageRetryPendingBySession);
messageRetryPending.release(sessionId);
}
}

Expand Down
13 changes: 2 additions & 11 deletions apps/desktop/src/renderer/app-shell-effects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,7 @@ export function useAppShellBootstrapSubscriptions(options: {
handleConnectionEvent: (event: ConnectionEvent) => void;
openHelp: () => void;
openSettings: () => void;
pendingPermissionModeChangesRef: RefBox<Set<string>>;
pendingSessionModelChangesRef: RefBox<Set<string>>;
pendingTurnActionTimersRef: RefBox<Map<string, ReturnType<typeof setTimeout>>>;
pendingTurnActionsRef: RefBox<Set<string>>;
clearPendingTurnActions: () => void;
projectPickerPendingRef: RefBox<boolean>;
projectPickerRequestRef: RefBox<number>;
refreshConnections: () => Promise<void>;
Expand Down Expand Up @@ -288,13 +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.pendingPermissionModeChangesRef.current.clear();
options.pendingSessionModelChangesRef.current.clear();
options.clearPendingTurnActions();
});

useEffect(() => {
Expand Down
Loading