Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions packages/web-shell/client/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14079,6 +14079,7 @@ describe('App session callbacks', () => {
});
expect(sessionCatalogController.turnCompleted).toHaveBeenCalledWith(
'/tmp/project',
'session-1',
);

onSessionChange.mockClear();
Expand Down Expand Up @@ -14133,6 +14134,7 @@ describe('App session callbacks', () => {

expect(sessionCatalogController.turnCompleted).toHaveBeenCalledWith(
'/tmp/project',
'session-late',
);
expect(onSessionChange).toHaveBeenCalledWith(
expect.objectContaining({
Expand Down
2 changes: 1 addition & 1 deletion packages/web-shell/client/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7262,7 +7262,7 @@ export function App({
? new Error(`Turn error (block ${lastTurnErrorIdRef.current})`)
: undefined;
if (workspaceCwd) {
sessionCatalogController.turnCompleted(workspaceCwd);
sessionCatalogController.turnCompleted(workspaceCwd, sessionId);
if (!connectionRef.current.displayName) {
scheduleDelayedActiveSessionDisplayNameRefresh(
sessionId,
Expand Down
16 changes: 13 additions & 3 deletions packages/web-shell/client/components/ChatPane.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1154,7 +1154,10 @@ describe('ChatPane', () => {
streamingStateValue = 'idle';
rerender();

expect(catalogController.turnCompleted).toHaveBeenCalledWith('/w');
expect(catalogController.turnCompleted).toHaveBeenCalledWith(
'/w',
'sess-1',
);
});

it('does not duplicate turn completion owned by the outer session', () => {
Expand Down Expand Up @@ -1188,7 +1191,10 @@ describe('ChatPane', () => {
streamingStateValue = 'idle';
rerender();

expect(catalogController.turnCompleted).toHaveBeenCalledWith('/w');
expect(catalogController.turnCompleted).toHaveBeenCalledWith(
'/w',
'sess-late',
);
});

it('captures a pane workspace that becomes available mid-turn', () => {
Expand All @@ -1202,9 +1208,13 @@ describe('ChatPane', () => {
rerender();

expect(catalogController.turnCompleted).toHaveBeenCalledTimes(1);
expect(catalogController.turnCompleted).toHaveBeenCalledWith('/secondary');
expect(catalogController.turnCompleted).toHaveBeenCalledWith(
'/secondary',
'sess-1',
);
expect(catalogController.turnCompleted).not.toHaveBeenCalledWith(
'/primary',
'sess-1',
);
});

Expand Down
5 changes: 4 additions & 1 deletion packages/web-shell/client/components/ChatPane.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,10 @@ export function ChatPane({
catalogOwnerCwd === catalogStreamingWorkspaceCwdRef.current &&
reportCatalogTurnCompletion
) {
sessionCatalogController.turnCompleted(catalogOwnerCwd);
sessionCatalogController.turnCompleted(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In split view ChatPane and App can both report the same completion, and the second record bumps the sequence so the in-flight settle no-ops, costing a full extra poll cycle.

SplitView.tsx:488-491 sets reportCatalogTurnCompletion={sessionId !== currentSessionId || paneWorkspaceCwd !== connection.workspaceCwd}, and paneWorkspaceCwd = workspaceCwdById.get(sessionId) (SplitView.tsx:420) is undefined until the session list resolves, so the second disjunct is true even for a pane showing the current session. React runs the child effect first: ChatPane records seq 1, the wake starts a poll that snapshots {S:1}, then App's effect records seq 2, so resolveSessionActivity(cwd, S, 1) hits if (pending?.get(sessionId) !== sequence) return; and no-ops. Probe: a single record leaves snapshotSessionActivity('/work') === undefined after the wake poll, a double record leaves Map(1) { 'session-a' => 2 } — the completion waits another SESSION_LIVE_STATE_POLL_MS, and if the row is off-page it raises invalidationRequested twice, scheduling a duplicate full reconcile.


Reviewed with Claude Code (claude-opus-5, 1M context) — /code-review 9476 --comment.

catalogOwnerCwd,
connection.sessionId,
);
}
}, [
catalogOwnerCwd,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type {
DaemonSessionListPage,
} from '@qwen-code/sdk/daemon';
import type { SessionCatalogQuery } from './session-catalog-store';
import { getSessionCatalogStore } from './session-catalog-store';

interface Deferred<T> {
promise: Promise<T>;
Expand Down Expand Up @@ -52,9 +53,11 @@ vi.mock('@qwen-code/webui/daemon-react-sdk', () => ({
useWorkspace: () => mocks.workspace,
}));

const { useSessionCatalogQuery, useWebShellSessions } = await import(
'./session-catalog-hooks'
);
const {
useSessionCatalogQuery,
useSessionCatalogController,
useWebShellSessions,
} = await import('./session-catalog-hooks');

let root: Root;
let container: HTMLDivElement;
Expand Down Expand Up @@ -214,4 +217,36 @@ describe('session catalog hooks', () => {
});
expect(result).toBeUndefined();
});

it('routes turn completions by live-state ownership', () => {
const client = mocks.workspace.client as DaemonClient;
const store = getSessionCatalogStore(client);
let controller: ReturnType<typeof useSessionCatalogController> | undefined;

function ControllerProbe() {
controller = useSessionCatalogController(client);
return null;
}

act(() => root.render(<ControllerProbe />));
const record = vi.spyOn(store, 'recordSessionActivity');
const invalidate = vi.spyOn(store, 'invalidateWorkspace');
const schedule = vi.spyOn(store, 'scheduleWorkspaceRefresh');

// Without live-state ownership the legacy rescan path stays.
act(() => controller!.turnCompleted('/w', 'sess-1'));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new hooks test leaks a real 2-second timer and three unrestored spies, breaking the teardown convention its sibling suites follow.

This file has no vi.useFakeTimers(), no vi.restoreAllMocks() and no store.dispose(); afterEach (lines 80-83) only unmounts the root and removes the container. The new case's first half takes the non-live-state branch and really calls store.scheduleWorkspaceRefresh('/w'), whose internal setTimeout(…, SESSION_CATALOG_TRAILING_REFRESH_MS = 2000) is never cleared, so it lands during later cases and calls invalidateWorkspace('/w') on a store nobody disposed; the three vi.spyOn(store, …) handles are never restored. The sibling workspace-session-live-state.test.tsx afterEach does both vi.useRealTimers() and vi.restoreAllMocks(). Isolation currently survives only because each test builds a fresh client (the store is cached per client) — coincidence, not containment.


Reviewed with Claude Code (claude-opus-5, 1M context) — /code-review 9476 --comment.

expect(invalidate).toHaveBeenCalledWith('/w');
expect(schedule).toHaveBeenCalledWith('/w');
expect(store.snapshotSessionActivity('/w')).toBeUndefined();

invalidate.mockClear();
schedule.mockClear();
const releaseLiveState = store.retainWorkspaceLiveState('/w');
act(() => controller!.turnCompleted('/w', 'sess-1'));
expect(record).toHaveBeenCalledWith('/w', 'sess-1');
expect(store.snapshotSessionActivity('/w')?.get('sess-1')).toBeDefined();
expect(invalidate).not.toHaveBeenCalled();
expect(schedule).not.toHaveBeenCalled();
releaseLiveState();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -218,13 +218,13 @@ export function useSessionCatalogController(client: DaemonClient) {
store.invalidateWorkspace(workspaceCwd);
});
},
turnCompleted(workspaceCwd: string) {
turnCompleted(workspaceCwd: string, sessionId: string) {
update(() => {
if (store.isWorkspaceLiveStateEnabled(workspaceCwd)) {
// The catalog revision doesn't advance on turn completion and
// the live-state overlay carries no updatedAt — flag a
// rate-limited reconcile so activity stamps keep refreshing.
store.requestWorkspaceLiveStateInvalidation(workspaceCwd);
// The catalog revision doesn't advance on turn completion; record

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is the "follow-up consumer" the design doc describes, but the doc still states the consumer is unimplemented.

docs/design/2026-08-18-workspace-session-live-state-updated-at.md still reads "This document defines the server and TypeScript SDK contract only. Web Shell consumption is a separate implementation change." (lines 5-7), heads section 447 as "Follow-up Web Shell Consumption Contract" in the future tense, says at 519 "It intentionally excludes all Web Shell changes.", and at 640 "That metric changes only after the follow-up consumer ships." A reader cannot tell which of the seven numbered rules landed and which were relaxed — rule 2 ("records … only for a turn known to have reached the running state", which this client does not distinguish) and rule 5 ("using the server comparator", which diverges on the default path) both shipped with caveats.


Reviewed with Claude Code (claude-opus-5, 1M context) — /code-review 9476 --comment.

// the completion so the live-state loop can settle it from the
// response's updatedAt watermark instead of a full catalog scan.
store.recordSessionActivity(workspaceCwd, sessionId);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turn completions were the only steady-state catalog reader once live-state is on, so retiring the rescan freezes every row whose recency the live overlay does not carry.

With live-state enabled the store refuses all background/poll catalog work (session-catalog-store.ts:822 if (this.isWorkspaceLiveStateEnabled(entry.query.workspaceCwd)) return;) and the sidebar disables its own autoLoad/pollIntervalMs (WebShellSidebar.tsx:995, :1304); every remaining invalidateWorkspace producer is a user action (create/rename/archive/delete), and no persistent subscriber sets maxAgeMs. A persisted-only row, or a row advanced by a second qwen process on the same workspace, has no live entry so applyLiveState never touches its updatedAt, and the bridge revision does not move (bridge.ts:2512 is reached only from membership/metadata changes). Measured A/B: 30 completions over a minute produce 6 catalog rescans on base (the other row's timestamp converges to its new value) and 0 on head (it stays frozen at the pre-advance value) for as long as the sidebar is open.


Reviewed with Claude Code (claude-opus-5, 1M context) — /code-review 9476 --comment.

return;
}
store.invalidateWorkspace(workspaceCwd);
Expand Down
Loading
Loading