refactor(sync): replace the session-status setState monkeypatch with an owned API
The global session-status store patched its own setState to derive active membership for callers replacing statusById — a boundary that silently trusted any caller passing both fields to keep them consistent. replaceGlobalSessionStatusById is now the one sanctioned way to swap the map from outside the event reducers; the runtime-switch reset and the tests that replaced the map directly go through it, and the patch is gone. Follow-up to #3126 review.
This commit is contained in:
@@ -15,7 +15,7 @@ import { useFilesViewTabsStore } from '@/stores/useFilesViewTabsStore';
|
|||||||
import { useTerminalStore } from '@/stores/useTerminalStore';
|
import { useTerminalStore } from '@/stores/useTerminalStore';
|
||||||
import { useSessionUIStore } from '@/sync/session-ui-store';
|
import { useSessionUIStore } from '@/sync/session-ui-store';
|
||||||
import { resetStreamingState } from '@/sync/streaming';
|
import { resetStreamingState } from '@/sync/streaming';
|
||||||
import { useGlobalSessionStatusStore } from '@/sync/global-session-status';
|
import { useGlobalSessionStatusStore, replaceGlobalSessionStatusById } from '@/sync/global-session-status';
|
||||||
import { resetSessionOrdering } from '@/sync/session-ordering';
|
import { resetSessionOrdering } from '@/sync/session-ordering';
|
||||||
import { resetSessionActivityTiming } from '@/sync/session-activity-timing';
|
import { resetSessionActivityTiming } from '@/sync/session-activity-timing';
|
||||||
import { syncDesktopSettings } from '@/lib/persistence';
|
import { syncDesktopSettings } from '@/lib/persistence';
|
||||||
@@ -57,7 +57,7 @@ export const resetAppForRuntimeEndpointChange = (detail: RuntimeEndpointChangedD
|
|||||||
// Cross-project session list (mobile sessions sheet & co) belongs to the
|
// Cross-project session list (mobile sessions sheet & co) belongs to the
|
||||||
// previous instance — drop it so stale sessions can't linger after a switch.
|
// previous instance — drop it so stale sessions can't linger after a switch.
|
||||||
useGlobalSessionsStore.getState().resetForRuntimeSwitch();
|
useGlobalSessionsStore.getState().resetForRuntimeSwitch();
|
||||||
useGlobalSessionStatusStore.setState({ statusById: new Map() });
|
replaceGlobalSessionStatusById(new Map());
|
||||||
resetSessionOrdering();
|
resetSessionOrdering();
|
||||||
// Turn timings belong to the previous instance's sessions, and the reset also
|
// Turn timings belong to the previous instance's sessions, and the reset also
|
||||||
// restarts the resume window so the switch is treated as a fresh load.
|
// restarts the resume window so the switch is treated as a fresh load.
|
||||||
|
|||||||
@@ -4,7 +4,7 @@ import type { Event } from '@opencode-ai/sdk/v2/client';
|
|||||||
import React, { act } from 'react';
|
import React, { act } from 'react';
|
||||||
import { createRoot, type Root } from 'react-dom/client';
|
import { createRoot, type Root } from 'react-dom/client';
|
||||||
import { deriveRecentSessions } from '../recent/activitySections';
|
import { deriveRecentSessions } from '../recent/activitySections';
|
||||||
import { applyGlobalSessionStatusEvent, useGlobalSessionStatusStore } from '@/sync/global-session-status';
|
import { applyGlobalSessionStatusEvent, useGlobalSessionStatusStore , replaceGlobalSessionStatusById} from '@/sync/global-session-status';
|
||||||
import {
|
import {
|
||||||
buildSidebarSessionProjection,
|
buildSidebarSessionProjection,
|
||||||
getDescendantIds,
|
getDescendantIds,
|
||||||
@@ -270,7 +270,7 @@ describe('useRecentSessionCollection', () => {
|
|||||||
};
|
};
|
||||||
|
|
||||||
try {
|
try {
|
||||||
useGlobalSessionStatusStore.setState({ statusById: new Map() });
|
replaceGlobalSessionStatusById(new Map());
|
||||||
await act(async () => root.render(React.createElement(Harness)));
|
await act(async () => root.render(React.createElement(Harness)));
|
||||||
expect(renderedIds).toEqual([]);
|
expect(renderedIds).toEqual([]);
|
||||||
|
|
||||||
@@ -296,7 +296,7 @@ describe('useRecentSessionCollection', () => {
|
|||||||
expect(timeReadCount).toBe(activeDeriveOperationCount);
|
expect(timeReadCount).toBe(activeDeriveOperationCount);
|
||||||
} finally {
|
} finally {
|
||||||
await act(async () => root.unmount());
|
await act(async () => root.unmount());
|
||||||
useGlobalSessionStatusStore.setState({ statusById: new Map() });
|
replaceGlobalSessionStatusById(new Map());
|
||||||
dom.restore();
|
dom.restore();
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|||||||
+5
-9
@@ -2,7 +2,7 @@ import { describe, expect, test } from 'bun:test';
|
|||||||
import React, { act } from 'react';
|
import React, { act } from 'react';
|
||||||
import { createRoot } from 'react-dom/client';
|
import { createRoot } from 'react-dom/client';
|
||||||
import type { Session } from '@opencode-ai/sdk/v2';
|
import type { Session } from '@opencode-ai/sdk/v2';
|
||||||
import { useGlobalSessionStatusStore } from '@/sync/global-session-status';
|
import { useGlobalSessionStatusStore , replaceGlobalSessionStatusById} from '@/sync/global-session-status';
|
||||||
import { useNotificationStore } from '@/sync/notification-store';
|
import { useNotificationStore } from '@/sync/notification-store';
|
||||||
import { useCollapsedSessionActivityState } from './collapsedActivityState';
|
import { useCollapsedSessionActivityState } from './collapsedActivityState';
|
||||||
import type { SessionNode } from '../types';
|
import type { SessionNode } from '../types';
|
||||||
@@ -15,7 +15,7 @@ describe('collapsed activity scalar selector', () => {
|
|||||||
test('does not rerender for unrelated updates and rerenders for relevant scalar changes', async () => {
|
test('does not rerender for unrelated updates and rerenders for relevant scalar changes', async () => {
|
||||||
const dom = installHookTestDom();
|
const dom = installHookTestDom();
|
||||||
const root = createRoot(dom.container);
|
const root = createRoot(dom.container);
|
||||||
useGlobalSessionStatusStore.setState({ statusById: new Map() });
|
replaceGlobalSessionStatusById(new Map());
|
||||||
useNotificationStore.setState({
|
useNotificationStore.setState({
|
||||||
list: [],
|
list: [],
|
||||||
index: { session: { unseenCount: {}, unseenHasError: {} }, project: { unseenCount: {}, unseenHasError: {} } },
|
index: { session: { unseenCount: {}, unseenHasError: {} }, project: { unseenCount: {}, unseenHasError: {} } },
|
||||||
@@ -30,9 +30,7 @@ describe('collapsed activity scalar selector', () => {
|
|||||||
try {
|
try {
|
||||||
await act(async () => root.render(React.createElement(Harness)));
|
await act(async () => root.render(React.createElement(Harness)));
|
||||||
const initialRenders = capture.renders;
|
const initialRenders = capture.renders;
|
||||||
await act(async () => useGlobalSessionStatusStore.setState({
|
await act(async () => replaceGlobalSessionStatusById(new Map([['unrelated', { status: { type: 'busy' }, directory: '/other' }]])));
|
||||||
statusById: new Map([['unrelated', { status: { type: 'busy' }, directory: '/other' }]]),
|
|
||||||
}));
|
|
||||||
await act(async () => useNotificationStore.getState().append({
|
await act(async () => useNotificationStore.getState().append({
|
||||||
type: 'turn-complete', session: 'unrelated', time: Date.now(), viewed: false,
|
type: 'turn-complete', session: 'unrelated', time: Date.now(), viewed: false,
|
||||||
}));
|
}));
|
||||||
@@ -43,14 +41,12 @@ describe('collapsed activity scalar selector', () => {
|
|||||||
}));
|
}));
|
||||||
expect(capture.state).toBe('unread');
|
expect(capture.state).toBe('unread');
|
||||||
const unreadRenders = capture.renders;
|
const unreadRenders = capture.renders;
|
||||||
await act(async () => useGlobalSessionStatusStore.setState({
|
await act(async () => replaceGlobalSessionStatusById(new Map([['relevant', { status: { type: 'busy' }, directory: '/workspace' }]])));
|
||||||
statusById: new Map([['relevant', { status: { type: 'busy' }, directory: '/workspace' }]]),
|
|
||||||
}));
|
|
||||||
expect(capture.state).toBe('active');
|
expect(capture.state).toBe('active');
|
||||||
expect(capture.renders).toBe(unreadRenders + 1);
|
expect(capture.renders).toBe(unreadRenders + 1);
|
||||||
} finally {
|
} finally {
|
||||||
await act(async () => root.unmount());
|
await act(async () => root.unmount());
|
||||||
useGlobalSessionStatusStore.setState({ statusById: new Map() });
|
replaceGlobalSessionStatusById(new Map());
|
||||||
useNotificationStore.setState({
|
useNotificationStore.setState({
|
||||||
list: [],
|
list: [],
|
||||||
index: { session: { unseenCount: {}, unseenHasError: {} }, project: { unseenCount: {}, unseenHasError: {} } },
|
index: { session: { unseenCount: {}, unseenHasError: {} }, project: { unseenCount: {}, unseenHasError: {} } },
|
||||||
|
|||||||
@@ -5,12 +5,13 @@ import {
|
|||||||
applyGlobalSessionStatusEvents,
|
applyGlobalSessionStatusEvents,
|
||||||
applyGlobalSessionStatusSnapshot,
|
applyGlobalSessionStatusSnapshot,
|
||||||
useGlobalSessionStatusStore,
|
useGlobalSessionStatusStore,
|
||||||
|
replaceGlobalSessionStatusById,
|
||||||
} from "./global-session-status"
|
} from "./global-session-status"
|
||||||
import { resetSessionOrdering, useSessionOrderingStore } from "./session-ordering"
|
import { resetSessionOrdering, useSessionOrderingStore } from "./session-ordering"
|
||||||
import { resetSessionActivityTiming, useSessionActivityTimingStore } from "./session-activity-timing"
|
import { resetSessionActivityTiming, useSessionActivityTimingStore } from "./session-activity-timing"
|
||||||
|
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
useGlobalSessionStatusStore.setState({ statusById: new Map() })
|
replaceGlobalSessionStatusById(new Map())
|
||||||
resetSessionOrdering()
|
resetSessionOrdering()
|
||||||
resetSessionActivityTiming()
|
resetSessionActivityTiming()
|
||||||
})
|
})
|
||||||
@@ -162,7 +163,7 @@ describe("global session status index", () => {
|
|||||||
properties: { sessionID: "session-a", status: { type: "busy" } },
|
properties: { sessionID: "session-a", status: { type: "busy" } },
|
||||||
} as Event)
|
} as Event)
|
||||||
|
|
||||||
useGlobalSessionStatusStore.setState({ statusById: new Map() })
|
replaceGlobalSessionStatusById(new Map())
|
||||||
|
|
||||||
expect(activeSessionIds()?.size).toBe(0)
|
expect(activeSessionIds()?.size).toBe(0)
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -40,61 +40,28 @@ const initialState: GlobalSessionStatusState = {
|
|||||||
export const useGlobalSessionStatusStore = create<GlobalSessionStatusState>(() => initialState);
|
export const useGlobalSessionStatusStore = create<GlobalSessionStatusState>(() => initialState);
|
||||||
useGlobalSessionStatusStore.subscribe(() => countSyncPerformance('globalStatusPublications'));
|
useGlobalSessionStatusStore.subscribe(() => countSyncPerformance('globalStatusPublications'));
|
||||||
|
|
||||||
// Runtime switching currently replaces statusById directly. Keep that boundary
|
/**
|
||||||
// synchronized without making normal status mutations derive membership again.
|
* Replaces the status map wholesale and derives active membership from it.
|
||||||
const storeSetState = useGlobalSessionStatusStore.setState;
|
* This is the ONE sanctioned way to swap statusById from outside the event
|
||||||
type GlobalSessionStatusStateUpdate = GlobalSessionStatusState
|
* reducers (runtime switch, tests) — previously a setState monkeypatch
|
||||||
| Partial<GlobalSessionStatusState>
|
* derived membership for arbitrary callers, which silently trusted any
|
||||||
| ((state: GlobalSessionStatusState) => GlobalSessionStatusState | Partial<GlobalSessionStatusState>);
|
* caller passing both fields to keep them consistent.
|
||||||
|
*/
|
||||||
function setSynchronizedState(
|
export const replaceGlobalSessionStatusById = (statusById: Map<string, GlobalSessionStatusEntry>): void => {
|
||||||
partial: GlobalSessionStatusStateUpdate,
|
|
||||||
replace?: false,
|
|
||||||
): void;
|
|
||||||
function setSynchronizedState(
|
|
||||||
partial: GlobalSessionStatusState | ((state: GlobalSessionStatusState) => GlobalSessionStatusState),
|
|
||||||
replace: true,
|
|
||||||
): void;
|
|
||||||
function setSynchronizedState(partial: GlobalSessionStatusStateUpdate, replace?: boolean): void {
|
|
||||||
if (partial instanceof Function) {
|
|
||||||
if (replace === true) {
|
|
||||||
// SAFETY: Zustand's `replace: true` overload only accepts a complete state or a complete-state updater.
|
|
||||||
storeSetState(partial as GlobalSessionStatusState | ((state: GlobalSessionStatusState) => GlobalSessionStatusState), true);
|
|
||||||
} else {
|
|
||||||
storeSetState(partial, replace);
|
|
||||||
}
|
|
||||||
return;
|
|
||||||
}
|
|
||||||
if (partial.statusById === undefined || partial.activeSessionIds) {
|
|
||||||
if (replace === true) {
|
|
||||||
// SAFETY: Zustand's `replace: true` overload only accepts a complete state or a complete-state updater.
|
|
||||||
storeSetState(partial as GlobalSessionStatusState, true);
|
|
||||||
} else {
|
|
||||||
storeSetState(partial, replace);
|
|
||||||
}
|
|
||||||
return;
|
|
||||||
}
|
|
||||||
|
|
||||||
const nextStatusById = partial.statusById;
|
|
||||||
const current = useGlobalSessionStatusStore.getState();
|
const current = useGlobalSessionStatusStore.getState();
|
||||||
const nextActiveSessionIds = new Set<string>();
|
const nextActiveSessionIds = new Set<string>();
|
||||||
for (const [sessionId, entry] of nextStatusById) {
|
for (const [sessionId, entry] of statusById) {
|
||||||
if (entry.status.type === 'busy' || entry.status.type === 'retry') {
|
if (entry.status.type === 'busy' || entry.status.type === 'retry') {
|
||||||
nextActiveSessionIds.add(sessionId);
|
nextActiveSessionIds.add(sessionId);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
const sameMembership = nextActiveSessionIds.size === current.activeSessionIds.size
|
const sameMembership = nextActiveSessionIds.size === current.activeSessionIds.size
|
||||||
&& [...nextActiveSessionIds].every((sessionId) => current.activeSessionIds.has(sessionId));
|
&& [...nextActiveSessionIds].every((sessionId) => current.activeSessionIds.has(sessionId));
|
||||||
const nextState = {
|
useGlobalSessionStatusStore.setState({
|
||||||
...current,
|
statusById,
|
||||||
...partial,
|
|
||||||
activeSessionIds: sameMembership ? current.activeSessionIds : nextActiveSessionIds,
|
activeSessionIds: sameMembership ? current.activeSessionIds : nextActiveSessionIds,
|
||||||
};
|
});
|
||||||
if (replace === true) storeSetState(nextState, true);
|
};
|
||||||
else storeSetState(nextState, replace);
|
|
||||||
}
|
|
||||||
|
|
||||||
useGlobalSessionStatusStore.setState = setSynchronizedState;
|
|
||||||
|
|
||||||
const normalizeStatusType = (type: string | undefined): ActiveStatusType | 'idle' => {
|
const normalizeStatusType = (type: string | undefined): ActiveStatusType | 'idle' => {
|
||||||
if (type === 'busy') return 'busy';
|
if (type === 'busy') return 'busy';
|
||||||
|
|||||||
Reference in New Issue
Block a user