Skip to content

Commit c305c91

Browse files
committed
perf(hooks): use primitive fingerprint for ensure-draft-when-empty effect
Audit pass on the same class of bug fixed in b0b41a6 found a second hook subscribing to the entire `appState` and using it as an effect dep: `useEnsureDraftWhenEmpty` had `[appState, ...]` in its dep array. Lower severity than the worktree-include case because the inner Tauri call (`agentChatCreatePane`) is guarded by an `inFlightSpawnRef`, so the effect didn't actually fan out into duplicate IPC calls. But the effect BODY still re-ran on every backend tick (many per second under the 5 s git poll + agent streaming), walking workspace surface trees and doing pane-tree checks for nothing. Replaced the `appState` subscription with a primitive string fingerprint built from the four fields the effect actually depends on: `${active_workspace_id}|${hasPane}|${project_root}` The selector returns a primitive string so React/Zustand bail on identity equality. The effect body now only runs when one of those three fields actually changes — i.e. when the user activates a different workspace, or panes are added/removed on the active surface, or `project_root` flips. Inside the effect, the live `appState` is read via `useAppStore.getState()` at fire time so we can still walk surfaces to pick the Home-draft-vs-pane-spawn branch. Added a regression test (`use-ensure-draft-when-empty.test.tsx`) that simulates a backend tick changing only `git_branch` on the active workspace (the most frequent backend mutation under the 5 s git poll). The hook must NOT fire `agentChatCreatePane` a second time across the rerender. Now 17 tests in this file (up from 16), 1686/1686 frontend tests pass. Tests verified: - npx tsc --noEmit: clean - npx vitest run: 1686/1686 pass
1 parent b0b41a6 commit c305c91

2 files changed

Lines changed: 106 additions & 3 deletions

File tree

src/hooks/use-ensure-draft-when-empty.test.tsx

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,68 @@ describe("useEnsureDraftWhenEmpty", () => {
348348
expect(agentChatCreatePane).toHaveBeenCalledTimes(1);
349349
});
350350

351+
it("does NOT re-spawn when only an irrelevant workspace field changes (perf — fingerprint guard)", () => {
352+
// Regression guard for the audit-pass fix that switched this hook
353+
// from a `[appState]` dep array to a primitive-fingerprint
354+
// selector. Without the fingerprint, every backend
355+
// `app-state-changed` tick (agent tokens, git polls, hook events)
356+
// re-ran the effect body — walking surface trees and doing
357+
// workspace lookups for nothing. The `inFlightSpawnRef` guard
358+
// prevented duplicate IPC, but the wasted cycles still added up.
359+
//
360+
// This test simulates a backend tick that changes `git_branch` on
361+
// the active workspace (a frequent mutation under the 5 s git
362+
// poll). The effect's fingerprint is built from
363+
// `(active_workspace_id, hasPane, project_root)` — `git_branch`
364+
// is intentionally NOT in it — so the effect must NOT fire a
365+
// second time across the rerender.
366+
enableAgentChatFlag = true;
367+
enableLazyFlag = true;
368+
flagsLoaded = true;
369+
homeDirSnapshot = "/home/user";
370+
appStateSnapshot = {
371+
active_workspace_id: "ws-foo",
372+
workspaces: [
373+
{
374+
workspace_id: "ws-foo",
375+
active_surface_id: "surf-empty",
376+
surfaces: [emptySplitSurface],
377+
project_root: "/projects/foo",
378+
cwd: "/projects/foo",
379+
git_branch: "main",
380+
},
381+
],
382+
};
383+
const { rerender } = renderHook(() => useEnsureDraftWhenEmpty());
384+
expect(agentChatCreatePane).toHaveBeenCalledTimes(1);
385+
386+
// Backend tick: same fingerprint fields, different git_branch +
387+
// a different workspaces array reference (Rust rebuilds the
388+
// snapshot on every emit). The hook's effect MUST NOT fire
389+
// again — fingerprint is reference-equal.
390+
appStateSnapshot = {
391+
active_workspace_id: "ws-foo",
392+
workspaces: [
393+
{
394+
workspace_id: "ws-foo",
395+
active_surface_id: "surf-empty",
396+
surfaces: [emptySplitSurface],
397+
project_root: "/projects/foo",
398+
cwd: "/projects/foo",
399+
git_branch: "feature-x",
400+
},
401+
],
402+
};
403+
rerender();
404+
405+
// Either branch of the proof works:
406+
// - in-flight ref is set, so even if the effect DID fire it
407+
// would short-circuit; OR
408+
// - fingerprint is unchanged, so the effect didn't re-fire at all.
409+
// The contract we care about is: zero extra Tauri calls.
410+
expect(agentChatCreatePane).toHaveBeenCalledTimes(1);
411+
});
412+
351413
it("falls back to workspace.cwd when project_root is missing (ad-hoc workspaces)", () => {
352414
enableAgentChatFlag = true;
353415
enableLazyFlag = true;

src/hooks/use-ensure-draft-when-empty.ts

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,34 @@ import { useFeatureFlags } from "@/stores/feature-flags";
55
import { hasAnyPane } from "@/lib/pane-tree";
66
import { agentChatCreatePane } from "@/tauri/commands";
77

8+
/**
9+
* Primitive-summary selector for the bits of `appState` this hook
10+
* actually depends on. Returning a string fingerprint means the effect
11+
* below only re-runs when one of the four relevant fields changes —
12+
* not on every backend `app-state-changed` tick (agent tokens, git
13+
* polls, hook events). Without this, the effect body would walk the
14+
* surface tree on every tick under heavy load.
15+
*/
16+
function selectEmptyWorkspaceFingerprint(
17+
s: { appState: ReturnType<typeof useAppStore.getState>["appState"] },
18+
): string {
19+
const app = s.appState;
20+
if (!app) return "no-app-state";
21+
const wsId = app.active_workspace_id;
22+
if (!wsId) return "no-active-ws";
23+
const ws = app.workspaces.find((w) => w.workspace_id === wsId);
24+
if (!ws) return `ws-missing:${wsId}`;
25+
// Whether the active surface has any panes — this is the only
26+
// structural fact the effect cares about.
27+
const activeSurface = ws.surfaces.find(
28+
(sf) => sf.surface_id === ws.active_surface_id,
29+
);
30+
const hasPane = activeSurface ? hasAnyPane(activeSurface.root) : false;
31+
// Project_root vs cwd drives the Home-draft-vs-pane branch.
32+
const projectRoot = ws.project_root ?? ws.cwd;
33+
return `${wsId}|${hasPane ? "1" : "0"}|${projectRoot}`;
34+
}
35+
836
/**
937
* Ensure something is showing for the active workspace whenever the
1038
* user has nothing else to look at.
@@ -28,7 +56,13 @@ import { agentChatCreatePane } from "@/tauri/commands";
2856
* effect fires while the Tauri call is pending don't double-spawn.
2957
*/
3058
export function useEnsureDraftWhenEmpty() {
31-
const appState = useAppStore((s) => s.appState);
59+
// Subscribe to a primitive string fingerprint of the four fields
60+
// this effect actually depends on, NOT the whole appState. Without
61+
// this, the effect body re-ran on every backend `app-state-changed`
62+
// tick (many per second under load) walking surface trees and doing
63+
// workspace lookups for nothing. The audit pass found this pattern;
64+
// see `selectEmptyWorkspaceFingerprint` above.
65+
const fingerprint = useAppStore(selectEmptyWorkspaceFingerprint);
3266
const homeDir = useHomeDir();
3367
const enableAgentChat = useFeatureFlags((s) => s.enableAgentChat);
3468
const enableLazyWorkspaceCreation = useFeatureFlags(
@@ -44,10 +78,17 @@ export function useEnsureDraftWhenEmpty() {
4478
const inFlightSpawnRef = useRef<string | null>(null);
4579

4680
useEffect(() => {
47-
if (!appState || !flagsLoaded) return;
81+
if (!flagsLoaded) return;
4882
if (!enableAgentChat || !enableLazyWorkspaceCreation) return;
4983
if (activeDraftId) return;
5084

85+
// Re-read the live snapshot at effect-fire time. The fingerprint
86+
// dep above ensures we only get here when the relevant slice
87+
// changed, but we still need the full workspace object to read
88+
// surfaces / surface ids.
89+
const appState = useAppStore.getState().appState;
90+
if (!appState) return;
91+
5192
const activeWs = appState.workspaces.find(
5293
(w) => w.workspace_id === appState.active_workspace_id,
5394
);
@@ -86,7 +127,7 @@ export function useEnsureDraftWhenEmpty() {
86127
const draft = store.getOrCreateHomeDraft();
87128
store.setActiveDraft(draft.draftId);
88129
}, [
89-
appState,
130+
fingerprint,
90131
homeDir,
91132
enableAgentChat,
92133
enableLazyWorkspaceCreation,

0 commit comments

Comments
 (0)