From ca86a4ab2ce4ff31783668fbc4078f83d660f402 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 1 Aug 2026 06:04:41 -0700 Subject: [PATCH] perf(crash): scan worktree tabs by key, and soften the verdict rationale Round 6 corrected my own round-5 comment. I had written that `true` is the repair-loop signature and `false` covers a deliberate background activation. The repair effect can emit `false` too: its closure holds the worktree from its render while the guard runs against live state, so a worktree switch landing in between reattributes the tab. The verdict hints at the caller; it does not prove it, and neither value should be discarded. Comment-only. Also take the free scan win the perf review measured: Object.entries allocates a pair array per worktree on a path that runs per tab activation. Own keys are safe to index by, so Object.keys plus an indexed read is behaviour-identical (16.1us -> 5.0us at 170 worktrees x 10 tabs). --- ...ash-reporting-renderer-breadcrumbs.test.ts | 4 ++-- src/main/ipc/crash-reporting.ts | 8 +++---- .../store/slices/active-tab-owner-worktree.ts | 21 ++++++++++++------- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts b/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts index 4acc2cb3e69..22aa81fe7b4 100644 --- a/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts +++ b/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts @@ -319,8 +319,8 @@ describe('renderer breadcrumb IPC routing', () => { } }) - // Why flag-scoped: coalescing keeps only the newest payload, and the two - // verdicts come from different call paths, so either would erase the other. + // Why flag-scoped: coalescing keeps only the newest payload, and the verdict + // flips under a persisting duplicate, so one would erase the other. it('keeps a non-converging duplicate-tab-owner notice out of the converging one', () => { emitRendererBreadcrumb({ name: 'terminal_tab_id_owned_by_multiple_worktrees', diff --git a/src/main/ipc/crash-reporting.ts b/src/main/ipc/crash-reporting.ts index f60cdf7b055..d32c80ed942 100644 --- a/src/main/ipc/crash-reporting.ts +++ b/src/main/ipc/crash-reporting.ts @@ -367,10 +367,10 @@ function rendererBreadcrumbCoalesceKey( // Why coalesce at all: the renderer guard is once-per-tab-id, not // once-per-session, so one stale worktree map duplicates every tab id at once // and emits a crumb per tab. Why keyed on the flag rather than name alone: the - // two verdicts come from different call paths — `true` from the active-terminal - // repair effect (the React #185 signature), `false` also from a deliberate - // background activation — and coalescing keeps only the newest payload, so - // either would erase the other. Two keys still bound the storm. + // verdict flips under a persisting duplicate (it tracks whether the tab still + // resolves to the active worktree), and coalescing keeps only the newest + // payload, so one verdict would erase the other. Two keys still bound the + // storm. if (name === DUPLICATE_TAB_OWNER_BREADCRUMB) { return `${name}:${String(data?.resolvedToActiveWorktree ?? '')}` } diff --git a/src/renderer/src/store/slices/active-tab-owner-worktree.ts b/src/renderer/src/store/slices/active-tab-owner-worktree.ts index e2d04b8af1d..9e7843bc9b3 100644 --- a/src/renderer/src/store/slices/active-tab-owner-worktree.ts +++ b/src/renderer/src/store/slices/active-tab-owner-worktree.ts @@ -33,7 +33,10 @@ export function resolveActiveTabOwnerWorktreeId( // truthiness guard below and silently fall back to the first match — the very // misattribution this function exists to remove. let activeOwnerId: string | null = null - for (const [worktreeId, tabs] of Object.entries(tabsByWorktree)) { + // Why keys and not entries: entries allocates a pair array per worktree on a path + // that runs per tab activation. Own keys stay safe to index by. + for (const worktreeId of Object.keys(tabsByWorktree)) { + const tabs = tabsByWorktree[worktreeId] if (!tabs.some((tab) => tab.id === tabId)) { continue } @@ -48,12 +51,16 @@ export function resolveActiveTabOwnerWorktreeId( // Why breadcrumb: no production origin for a duplicated tab id is known yet, // so a crash bundle carrying this is what proves or kills the hypothesis. - // Reading it: `ownerCount > 1` is the load-bearing datum. `true` is the - // repair-loop signature (that effect only ever activates a tab from the active - // worktree's own list), while `false` also covers a deliberate background - // activation such as jump-to-agent. Why the verdict is in the guard key: the - // active worktree changes under a persisting duplicate, and coalescing keeps - // only the newest payload, so either verdict would otherwise erase the other. + // Reading it: `ownerCount > 1` is the load-bearing datum; the verdict only + // hints at the caller. A sustained repair loop shows up as `true`, since that + // effect picks from the active worktree's own list — but it does not prove the + // caller, and the repair effect can emit `false` too: its closure holds the + // worktree from its render while this runs against live state, so a worktree + // switch landing in between (an earlier-flushed effect, or IPC before the + // passive flush) reattributes the tab. So `false` covers that race as well as + // a deliberate background activation such as jump-to-agent — discard neither. + // Why the verdict is in the guard key: it flips under a persisting duplicate, + // and coalescing keeps only the newest payload, so one would erase the other. // Still at most two crumbs per tab id. const resolvedToActiveWorktree = activeOwnerId !== null const verdictKey = `${tabId}:${resolvedToActiveWorktree}`