From 5bdaec12aeb21b66b0b7fa93fbb0eb0c3d7495c2 Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Mon, 11 May 2026 13:54:05 -0700 Subject: [PATCH] fix(smart-sort): correct liveness check + freshness gate from review - smart-sort.ts cold-start: switch from `tab.ptyId` (wake-hint) to `tabHasLivePty(ptyIdsByTabId, tab.id)` so slept tabs after restart don't skip the persisted-sortOrder fallback. - smart-attention.ts: only suppress the title-heuristic fallback for panes whose hook entry is fresh, so stale hooks no longer hide a live `permission` title behind a dead entry. - WorktreeList.tsx: drop dead `prCache` subscription for smart mode (the new comparator doesn't read it) and refresh the now-stale cold-start comment. - smart-sort.test.ts: pin the slept-tab cold-start path; correct a misleading comment about a not-actually-dropped heuristic. - docs: align telemetry property names with the shipped schema and drop stale `WorktreeJumpPalette.tsx` line-number references. Co-authored-by: Orca --- docs/smart-worktree-order-redesign.md | 10 ++++----- .../src/components/sidebar/WorktreeList.tsx | 21 ++++++++++--------- .../src/components/sidebar/smart-attention.ts | 7 +++++++ .../src/components/sidebar/smart-sort.test.ts | 18 ++++++++++++++-- .../src/components/sidebar/smart-sort.ts | 6 +++++- 5 files changed, 44 insertions(+), 18 deletions(-) diff --git a/docs/smart-worktree-order-redesign.md b/docs/smart-worktree-order-redesign.md index 78171ca6c86..dea6402506f 100644 --- a/docs/smart-worktree-order-redesign.md +++ b/docs/smart-worktree-order-redesign.md @@ -154,7 +154,7 @@ Primary: - `src/renderer/src/components/sidebar/smart-attention.test.ts` — **new file**: helper-level tests - `src/renderer/src/components/sidebar/WorktreeList.tsx` — thread `runtimePaneTitlesByTabId` and `ptyIdsByTabId` into the sort path so the title-heuristic fallback (Edge case 9) can fire for hookless panes. - `src/renderer/src/components/sidebar/visible-worktrees.ts` — same: callers of `sortWorktreesSmart`/`buildWorktreeComparator` must thread the new state. -- `src/renderer/src/components/WorktreeJumpPalette.tsx` — thread `agentStatusByPaneKey`, `runtimePaneTitlesByTabId`, and `ptyIdsByTabId` at lines 227 and 239. +- `src/renderer/src/components/WorktreeJumpPalette.tsx` — thread `agentStatusByPaneKey`, `runtimePaneTitlesByTabId`, and `ptyIdsByTabId` into the two `sortWorktreesSmart(...)` calls (the typed-query and browser-tabs paths). No changes: - `src/shared/agent-status-types.ts` — already provides `AgentStatusEntry`, `AgentStateHistoryEntry.startedAt`, `AgentStateHistoryEntry.interrupted`, `AGENT_STATUS_STALE_AFTER_MS`, `AGENT_STATE_HISTORY_MAX`. @@ -324,7 +324,7 @@ Callers today (verify before each edit): - `src/renderer/src/components/sidebar/visible-worktrees.ts:179` — already threads `state.agentStatusByPaneKey`. **Must add** `state.runtimePaneTitlesByTabId` and `state.ptyIdsByTabId`. - `src/renderer/src/components/sidebar/visible-worktrees.ts:188` — same: already threads agent status, **must add** the two title-fallback inputs. - `src/renderer/src/components/sidebar/WorktreeList.tsx:652` — uses `buildWorktreeComparator`; verify the parent state read picks up the two new fields and threads them through. -- `src/renderer/src/components/WorktreeJumpPalette.tsx:227` and `:239` — currently `sortWorktreesSmart(visibleWorktrees, tabsByWorktree, repoMap, prCache)` (4 args). **Must change** to pass `agentStatusByPaneKey`, `runtimePaneTitlesByTabId`, and `ptyIdsByTabId` from the store. +- The two `sortWorktreesSmart(...)` calls in `src/renderer/src/components/WorktreeJumpPalette.tsx` (the typed-query and browser-tabs paths) — updated to pass `agentStatusByPaneKey`, `runtimePaneTitlesByTabId`, and `ptyIdsByTabId` from the store. Make all three new parameters **non-optional** in the new `sortWorktreesSmart` signature (and on `buildWorktreeComparator`'s smart branch). Why: a forgotten caller fails type-check rather than silently regressing the palette to "all Class 4" or quietly losing the hookless-fallback path. @@ -355,7 +355,7 @@ This section is the implementation handoff. Follow in order; each step lands a c - Keep the existing cold-start branch (`!hasAnyLivePty`) using persisted `sortOrder` as today — see Open Question #3 (Migration) and Edge case 8. **Step 5 — Update callers.** -- Update `WorktreeJumpPalette.tsx:227` and `:239` to pass `state.agentStatusByPaneKey`, `state.runtimePaneTitlesByTabId`, and `state.ptyIdsByTabId`. +- Update both `sortWorktreesSmart(...)` calls in `WorktreeJumpPalette.tsx` (the typed-query and browser-tabs paths) to pass `state.agentStatusByPaneKey`, `state.runtimePaneTitlesByTabId`, and `state.ptyIdsByTabId`. - Update `WorktreeList.tsx` and `visible-worktrees.ts` to thread the same two title-fallback fields. - Run `pnpm typecheck` to confirm no other callers slipped through. @@ -397,9 +397,9 @@ This section is the implementation handoff. Follow in order; each step lands a c Three PostHog events ship with this change to validate the redesign post-launch (Orca project ID 406068): -- **`smart_sort_class_distribution`** — fired from `WorktreeList` after each completed `sortedIds` recomputation, throttled to once per ~30s. Stops firing when `sortBy !== 'smart'` (timer ownership lives with the component, so a sort-selector switch cancels it cleanly). Properties: `class1Count`, `class2Count`, `class3Count`, `class4Count`, `totalWorktrees`. Lets us see whether real users actually have Class 1/2/3 populated or whether everyone sits in Class 4 (signal that hook coverage is too low and the redesign isn't doing work). +- **`smart_sort_class_distribution`** — fired from `WorktreeList` after each completed `sortedIds` recomputation, throttled to once per ~30s. Stops firing when `sortBy !== 'smart'` (timer ownership lives with the component, so a sort-selector switch cancels it cleanly). Properties: `class1`, `class2`, `class3`, `class4`, `totalWorktrees`. Lets us see whether real users actually have Class 1/2/3 populated or whether everyone sits in Class 4 (signal that hook coverage is too low and the redesign isn't doing work). - **`smart_sort_class_1_promotion`** — fired from `WorktreeList` after each `sortedIds` recomputation, suppressing repeat-fires by comparing against a `prevClassByWorktreeId` map keyed by worktree id. Only fires on transitions INTO Class 1 (not on Class 1 → Class 1 noise from transient stale-window flickers). Properties: `cause` ∈ `{ 'blocked', 'waiting', 'title-heuristic' }`. Distinguishes hook-driven promotions from the title-heuristic fallback path so we can tell whether Edge case 9 is carrying weight or whether the heuristic is rarely needed. -- **`smart_to_recent_switch`** — fired when a user switches the sort selector from `smart` to `recent` *after* the redesign ships. A spike post-launch is the cleanest regression signal we have ("users are voting with their feet"). Pair with the inverse `recent_to_smart_switch` to read net flow. +- **`smart_to_recent_switch`** — fired when a user switches the sort selector from `smart` to `recent` *after* the redesign ships. A spike post-launch is the cleanest regression signal we have ("users are voting with their feet"). A future inverse event (`recent_to_smart_switch`) would pair with this for net-flow analysis; not shipped in v1 because the abandonment direction is the cleaner regression signal on its own. Keep payloads small; no per-worktree IDs or paths in event properties. diff --git a/src/renderer/src/components/sidebar/WorktreeList.tsx b/src/renderer/src/components/sidebar/WorktreeList.tsx index 7682d8fdb62..9c81f29bd50 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.tsx +++ b/src/renderer/src/components/sidebar/WorktreeList.tsx @@ -563,10 +563,10 @@ const WorktreeList = React.memo(function WorktreeList() { const cardProps = useAppStore((s) => s.worktreeCardProperties) - // PR cache is needed for PR-status grouping, smart sorting, and when the - // PR card property is visible. + // PR cache is needed for PR-status grouping and when the PR card property + // is visible. const prCache = useAppStore((s) => - groupBy === 'pr-status' || sortBy === 'smart' || cardProps.includes('pr') ? s.prCache : null + groupBy === 'pr-status' || cardProps.includes('pr') ? s.prCache : null ) const sortEpoch = useAppStore((s) => s.sortEpoch) @@ -645,13 +645,14 @@ const WorktreeList = React.memo(function WorktreeList() { (worktree) => !worktree.isArchived ) - // Why cold-start detection: the smart score is dominated by ephemeral - // signals (running jobs +60, live terminals +12, needs attention +35) - // that vanish after restart. Recomputing the smart score on cold start - // produces a shuffled ordering because those signals are gone while - // persistent ones (unread, linked PR) survive — changing relative ranks. - // Instead, restore the pre-shutdown order from the persisted sortOrder - // snapshot, and switch to the live smart score once PTYs start spawning. + // Why cold-start detection: smart-class resolution depends on the + // agent-status snapshot (agentStatusByPaneKey) hydrating from the hook + // server, which lands asynchronously after launch. Running the warm + // comparator before that arrives would collapse every worktree to Class 4 + // and shuffle the sidebar against the comparator's tiebreakers. Restore + // the pre-shutdown order from the persisted sortOrder snapshot until any + // live PTY appears, then switch to the live class layer. See Edge case 8 + // in docs/smart-worktree-order-redesign.md. if (sortBy === 'smart' && !sessionHasHadPty.current) { // Why: `tabHasLivePty` (over `ptyIdsByTabId`) is the source of truth for // liveness — slept terminals retain `tab.ptyId` as a wake hint, so reading diff --git a/src/renderer/src/components/sidebar/smart-attention.ts b/src/renderer/src/components/sidebar/smart-attention.ts index 98e3d83847f..516ced3a16d 100644 --- a/src/renderer/src/components/sidebar/smart-attention.ts +++ b/src/renderer/src/components/sidebar/smart-attention.ts @@ -268,6 +268,13 @@ export function buildAttentionByWorktree( if (hookEntries) { for (const entry of hookEntries) { panes.push({ kind: 'hook', entry }) + // Why: only fresh hook entries should suppress the title-heuristic + // fallback for their pane. A stale hook is filtered out by + // resolveAttention; if we marked its pane as "hook-covered" we'd hide + // the live title behind a dead entry and drop the worktree to Class 4. + if (!isExplicitAgentStatusFresh(entry, now, AGENT_STATUS_STALE_AFTER_MS)) { + continue + } const paneId = paneIdFromPaneKey(entry.paneKey) if (paneId !== null) { hookPaneIds.add(paneId) diff --git a/src/renderer/src/components/sidebar/smart-sort.test.ts b/src/renderer/src/components/sidebar/smart-sort.test.ts index e5b0781aad1..6c4f9a0144b 100644 --- a/src/renderer/src/components/sidebar/smart-sort.test.ts +++ b/src/renderer/src/components/sidebar/smart-sort.test.ts @@ -451,6 +451,20 @@ describe('sortWorktreesSmart — cold start fallback', () => { expect(sorted.map((w) => w.id)).toEqual(['b', 'a']) }) + it('treats slept tabs (tab.ptyId without live entry) as cold start', () => { + // Why: tab.ptyId is the wake-hint sessionId preserved under sleep — not a + // liveness signal. With slept tabs but no live PTYs, sortWorktreesSmart + // must fall back to persisted sortOrder. + const a = makeWorktree({ id: 'a', sortOrder: 1, displayName: 'a' }) + const b = makeWorktree({ id: 'b', sortOrder: 2, displayName: 'b' }) + const tabsByWorktree = { + [a.id]: [makeTab({ id: 'ta', worktreeId: a.id, ptyId: 'wake-hint' })] + } + // ptyIdsByTabId is empty — slept tab has wake-hint ptyId but no live entry. + const sorted = sortWorktreesSmart([a, b], tabsByWorktree, repoMap, {}, {}, {}) + expect(sorted.map((w) => w.id)).toEqual(['b', 'a']) + }) + it('uses the smart comparator once a PTY is alive', () => { const blocked = makeWorktree({ id: 'blocked', displayName: 'Blocked', sortOrder: 0 }) const done = makeWorktree({ id: 'done', displayName: 'Done', sortOrder: 100 }) @@ -487,8 +501,8 @@ describe('sortWorktreesSmart — cold start fallback', () => { describe('sortWorktreesSmart — palette caller regression', () => { // Why: WorktreeJumpPalette routes typed queries through sortWorktreesSmart. - // This test pins that the palette path uses the class layer (not the - // dropped terminal-title heuristic) when threading agentStatusByPaneKey. + // This test pins that the palette path uses the class layer (not just the + // recent-activity fallback) when threading agentStatusByPaneKey. it('palette ranks blocked above working when both flow through sortWorktreesSmart', () => { const blocked = makeWorktree({ id: 'blocked', displayName: 'A-Blocked' }) const working = makeWorktree({ id: 'working', displayName: 'B-Working' }) diff --git a/src/renderer/src/components/sidebar/smart-sort.ts b/src/renderer/src/components/sidebar/smart-sort.ts index 64bbc1717b2..3d539c2f3b9 100644 --- a/src/renderer/src/components/sidebar/smart-sort.ts +++ b/src/renderer/src/components/sidebar/smart-sort.ts @@ -1,5 +1,6 @@ import type { Worktree, Repo, TerminalTab } from '../../../../shared/types' import type { AgentStatusEntry } from '../../../../shared/agent-status-types' +import { tabHasLivePty } from '@/lib/tab-has-live-pty' import { IDLE, buildAttentionByWorktree, type WorktreeAttention } from './smart-attention' type SortBy = 'name' | 'smart' | 'recent' | 'repo' @@ -122,9 +123,12 @@ export function sortWorktreesSmart( runtimePaneTitlesByTabId: Record>, ptyIdsByTabId: Record ): Worktree[] { + // Why: `tabHasLivePty` (over `ptyIdsByTabId`) is the source of truth for + // liveness — slept terminals retain `tab.ptyId` as a wake hint, so reading + // it directly would falsely keep cold-start ordering off after restart. const hasAnyLivePty = Object.values(tabsByWorktree) .flat() - .some((t) => t.ptyId) + .some((tab) => tabHasLivePty(ptyIdsByTabId, tab.id)) if (!hasAnyLivePty) { // Cold start: use persisted sortOrder snapshot until the agent-status