fix: address review findings (#305)

This commit is contained in:
Jinjing
2026-04-05 10:58:02 -07:00
committed by GitHub
parent 0cf0bbaab4
commit d8536b89ff
5 changed files with 270 additions and 71 deletions
@@ -7,7 +7,7 @@ import { Button } from '@/components/ui/button'
import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip'
import { cn } from '@/lib/utils'
import type { Worktree, Repo } from '../../../../shared/types'
import { buildWorktreeComparator } from './smart-sort'
import { buildWorktreeComparator, hasRecentPRSignal, type RecentSortOverride } from './smart-sort'
import { branchName, type Row, buildRows, getGroupKeyForWorktree } from './worktree-list-groups'
const WorktreeList = React.memo(function WorktreeList() {
@@ -15,6 +15,7 @@ const WorktreeList = React.memo(function WorktreeList() {
const worktreesByRepo = useAppStore((s) => s.worktreesByRepo)
const repos = useAppStore((s) => s.repos)
const activeWorktreeId = useAppStore((s) => s.activeWorktreeId)
const activeWorktreeSelectionNonce = useAppStore((s) => s.activeWorktreeSelectionNonce)
const setActiveWorktree = useAppStore((s) => s.setActiveWorktree)
const searchQuery = useAppStore((s) => s.searchQuery)
const groupBy = useAppStore((s) => s.groupBy)
@@ -45,8 +46,8 @@ const WorktreeList = React.memo(function WorktreeList() {
return m
}, [repos])
// Flatten, filter, sort
const worktrees = useMemo(() => {
// Flatten and filter the current live worktree data first.
const visibleWorktrees = useMemo(() => {
let all: Worktree[] = Object.values(worktreesByRepo).flat()
// Filter archived
@@ -77,20 +78,66 @@ const WorktreeList = React.memo(function WorktreeList() {
})
}
// Sort
all.sort(buildWorktreeComparator(sortBy, tabsByWorktree, repoMap, prCache))
return all
}, [
worktreesByRepo,
filterRepoIds,
searchQuery,
showActiveOnly,
sortBy,
repoMap,
tabsByWorktree,
prCache
])
}, [worktreesByRepo, filterRepoIds, searchQuery, showActiveOnly, repoMap, tabsByWorktree])
const latestRecentSortInputsRef = useRef<Record<string, RecentSortOverride>>({})
const frozenActiveRecentSortRef = useRef<{
selectionNonce: number
override: RecentSortOverride | null
}>({ selectionNonce: -1, override: null })
// Why: snapshot the active worktree's sort inputs at the moment the user
// selects it, so that subsequent background updates (e.g. clearing isUnread)
// don't cause the active card to jump in the list. The ref mutation is
// idempotent for a given nonce value, which makes it safe even if React
// replays the render under concurrent mode.
if (sortBy === 'recent') {
if (activeWorktreeSelectionNonce !== frozenActiveRecentSortRef.current.selectionNonce) {
frozenActiveRecentSortRef.current = {
selectionNonce: activeWorktreeSelectionNonce,
override: activeWorktreeId
? (latestRecentSortInputsRef.current[activeWorktreeId] ?? null)
: null
}
}
}
// Why: grab the stable primitive/ref values instead of creating a new object
// literal every render, which would bust the useMemo cache and cause the
// worktree list to re-sort on every single React render loop.
const activeOverride =
sortBy === 'recent' && activeWorktreeId ? frozenActiveRecentSortRef.current.override : null
const worktrees = useMemo(() => {
// Only construct the record literal inside the memo so it doesn't
// break memoization as a new object reference each render.
const overrides =
activeOverride && activeWorktreeId ? { [activeWorktreeId]: activeOverride } : null
const sorted = [...visibleWorktrees]
sorted.sort(
buildWorktreeComparator(sortBy, tabsByWorktree, repoMap, prCache, Date.now(), overrides)
)
return sorted
}, [visibleWorktrees, sortBy, tabsByWorktree, repoMap, prCache, activeWorktreeId, activeOverride])
// Why: memoize the per-worktree recent-sort inputs so we only iterate
// visible worktrees and call hasRecentPRSignal when the underlying data
// actually changes, rather than on every render.
const nextRecentSortInputs = useMemo(() => {
const inputs: Record<string, RecentSortOverride> = {}
if (sortBy === 'recent') {
for (const wt of visibleWorktrees) {
inputs[wt.id] = {
worktree: wt,
tabs: tabsByWorktree?.[wt.id] ?? [],
hasRecentPRSignal: hasRecentPRSignal(wt, repoMap, prCache)
}
}
}
return inputs
}, [visibleWorktrees, tabsByWorktree, repoMap, prCache, sortBy])
latestRecentSortInputsRef.current = nextRecentSortInputs
// Collapsed group state
const [collapsedGroups, setCollapsedGroups] = useState<Set<string>>(new Set())
@@ -1,6 +1,7 @@
/* eslint-disable max-lines */
import { afterEach, describe, expect, it, vi } from 'vitest'
import type { Repo, TerminalTab, Worktree } from '../../../../shared/types'
import { buildWorktreeComparator, computeSmartScore } from './smart-sort'
import { buildWorktreeComparator, computeSmartScore, type RecentSortOverride } from './smart-sort'
const NOW = new Date('2026-03-27T12:00:00.000Z').getTime()
@@ -275,4 +276,82 @@ describe('buildWorktreeComparator', () => {
expect(worktrees.map((worktree) => worktree.id)).toEqual(['cold-cache', 'plain'])
})
it('can freeze the active worktree recent signals without blocking background reordering', () => {
const activeBeforeClick = makeWorktree({
id: 'active',
displayName: 'Active',
isUnread: true,
lastActivityAt: NOW - 30_000
})
const activeAfterClick = { ...activeBeforeClick, isUnread: false }
const background = makeWorktree({
id: 'background',
displayName: 'Background',
lastActivityAt: NOW - 60_000
})
const worktrees = [background, activeAfterClick]
const tabsByWorktree = {
[background.id]: [makeTab({ worktreeId: background.id, title: 'Claude Code - working' })]
}
const recentSortOverrides: Record<string, RecentSortOverride> = {
[activeAfterClick.id]: {
worktree: activeBeforeClick,
tabs: [],
hasRecentPRSignal: false
}
}
worktrees.sort(
buildWorktreeComparator('recent', tabsByWorktree, repoMap, null, NOW, recentSortOverrides)
)
expect(worktrees.map((worktree) => worktree.id)).toEqual(['background', 'active'])
})
it('can keep the active worktree in place while its unread badge is cleared on selection', () => {
const activeBeforeClick = makeWorktree({
id: 'active',
displayName: 'Active',
isUnread: true,
lastActivityAt: NOW - 30_000
})
const activeAfterClick = { ...activeBeforeClick, isUnread: false }
const background = makeWorktree({
id: 'background',
displayName: 'Background',
lastActivityAt: NOW - 2 * 60_000
})
const worktrees = [background, activeAfterClick]
const recentSortOverrides: Record<string, RecentSortOverride> = {
[activeAfterClick.id]: {
worktree: activeBeforeClick,
tabs: [],
hasRecentPRSignal: false
}
}
worktrees.sort(buildWorktreeComparator('recent', null, repoMap, null, NOW, recentSortOverrides))
expect(worktrees.map((worktree) => worktree.id)).toEqual(['active', 'background'])
})
it('keeps a more recent worktree ahead even without an override', () => {
const activeAfterClick = makeWorktree({
id: 'active',
displayName: 'Active',
isUnread: false,
lastActivityAt: NOW - 30_000
})
const background = makeWorktree({
id: 'background',
displayName: 'Background',
lastActivityAt: NOW - 2 * 60_000
})
const worktrees = [background, activeAfterClick]
worktrees.sort(buildWorktreeComparator('recent', null, repoMap, null, NOW))
expect(worktrees.map((worktree) => worktree.id)).toEqual(['active', 'background'])
})
})
+106 -53
View File
@@ -4,12 +4,17 @@ import type { Worktree, Repo, TerminalTab } from '../../../../shared/types'
type SortBy = 'name' | 'recent' | 'repo'
type PRCacheEntry = { data: object | null; fetchedAt: number }
export type RecentSortOverride = {
worktree: Worktree
tabs: TerminalTab[]
hasRecentPRSignal: boolean
}
function branchDisplayName(branch: string): string {
return branch.replace(/^refs\/heads\//, '')
}
function hasRecentPRSignal(
export function hasRecentPRSignal(
worktree: Worktree,
repoMap: Map<string, Repo>,
prCache: Record<string, PRCacheEntry> | null
@@ -29,6 +34,67 @@ function hasRecentPRSignal(
return worktree.linkedPR !== null
}
function computeSmartScoreFromSignals(
worktree: Worktree,
tabs: TerminalTab[],
hasRecentPR: boolean,
now: number
): number {
const liveTabs = tabs.filter((t) => t.ptyId)
let score = 0
const isRunning = liveTabs.some((t) => detectAgentStatusFromTitle(t.title) === 'working')
if (isRunning) {
score += 60
}
const needsAttention = liveTabs.some((t) => detectAgentStatusFromTitle(t.title) === 'permission')
if (needsAttention) {
score += 35
}
if (worktree.isUnread) {
score += 18
}
if (liveTabs.length > 0) {
score += 12
}
if (hasRecentPR) {
score += 10
}
if (worktree.linkedIssue !== null) {
score += 6
}
const activityAge = now - (worktree.lastActivityAt || 0)
if (worktree.lastActivityAt > 0) {
const ONE_DAY = 24 * 60 * 60 * 1000
score += 24 * Math.max(0, 1 - activityAge / ONE_DAY)
}
return score
}
function getRecentSortCandidate(
worktree: Worktree,
tabsByWorktree: Record<string, TerminalTab[]> | null,
repoMap: Map<string, Repo>,
prCache: Record<string, PRCacheEntry> | null,
recentSortOverrides: Record<string, RecentSortOverride> | null
): RecentSortOverride {
return (
recentSortOverrides?.[worktree.id] ?? {
worktree,
tabs: tabsByWorktree?.[worktree.id] ?? [],
hasRecentPRSignal: hasRecentPRSignal(worktree, repoMap, prCache)
}
)
}
/**
* Build a comparator for sorting worktrees based on the current sort mode.
*/
@@ -37,18 +103,43 @@ export function buildWorktreeComparator(
tabsByWorktree: Record<string, TerminalTab[]> | null,
repoMap: Map<string, Repo>,
prCache: Record<string, PRCacheEntry> | null,
now: number = Date.now()
now: number = Date.now(),
recentSortOverrides: Record<string, RecentSortOverride> | null = null
): (a: Worktree, b: Worktree) => number {
return (a, b) => {
switch (sortBy) {
case 'name':
return a.displayName.localeCompare(b.displayName)
case 'recent': {
const recentA = getRecentSortCandidate(
a,
tabsByWorktree,
repoMap,
prCache,
recentSortOverrides
)
const recentB = getRecentSortCandidate(
b,
tabsByWorktree,
repoMap,
prCache,
recentSortOverrides
)
// Recent means meaningful recent work, not selection time.
return (
computeSmartScore(b, tabsByWorktree, repoMap, prCache, now) -
computeSmartScore(a, tabsByWorktree, repoMap, prCache, now) ||
b.lastActivityAt - a.lastActivityAt ||
computeSmartScoreFromSignals(
recentB.worktree,
recentB.tabs,
recentB.hasRecentPRSignal,
now
) -
computeSmartScoreFromSignals(
recentA.worktree,
recentA.tabs,
recentA.hasRecentPRSignal,
now
) ||
recentB.worktree.lastActivityAt - recentA.worktree.lastActivityAt ||
a.displayName.localeCompare(b.displayName)
)
}
@@ -84,52 +175,14 @@ export function computeSmartScore(
prCache: Record<string, PRCacheEntry> | null,
now: number = Date.now()
): number {
const tabs = tabsByWorktree?.[worktree.id] ?? []
const liveTabs = tabs.filter((t) => t.ptyId)
let score = 0
// Running: any live PTY with an AI agent actively working
const isRunning = liveTabs.some((t) => detectAgentStatusFromTitle(t.title) === 'working')
if (isRunning) {
score += 60
}
// Needs attention: permission prompt in a live agent terminal
const needsAttention = liveTabs.some((t) => detectAgentStatusFromTitle(t.title) === 'permission')
if (needsAttention) {
score += 35
}
// Unread
if (worktree.isUnread) {
score += 18
}
// Live terminals are a strong sign of ongoing work, even if no agent title is detected.
if (liveTabs.length > 0) {
score += 12
}
// Why: branch-aware PR cache is the freshest signal, but off-screen
// worktrees may not have fetched it yet. Fall back to persisted linkedPR
// only while that branch cache entry is still cold so recent sorting stays
// stable on launch without reviving stale PRs after a cache miss resolves.
if (repoMap && hasRecentPRSignal(worktree, repoMap, prCache)) {
score += 10
}
if (worktree.linkedIssue !== null) {
score += 6
}
// Recent meaningful activity should stay relevant for the rest of the day,
// not vanish after an hour.
const activityAge = now - (worktree.lastActivityAt || 0)
if (worktree.lastActivityAt > 0) {
const ONE_DAY = 24 * 60 * 60 * 1000
score += 24 * Math.max(0, 1 - activityAge / ONE_DAY)
}
return score
return computeSmartScoreFromSignals(
worktree,
tabsByWorktree?.[worktree.id] ?? [],
// Why: branch-aware PR cache is the freshest signal, but off-screen
// worktrees may not have fetched it yet. Fall back to persisted linkedPR
// only while that branch cache entry is still cold so recent sorting stays
// stable on launch without reviving stale PRs after a cache miss resolves.
repoMap ? hasRecentPRSignal(worktree, repoMap, prCache) : worktree.linkedPR !== null,
now
)
}
@@ -14,6 +14,7 @@ export type WorktreeDeleteState = {
export type WorktreeSlice = {
worktreesByRepo: Record<string, Worktree[]>
activeWorktreeId: string | null
activeWorktreeSelectionNonce: number
deleteStateByWorktreeId: Record<string, WorktreeDeleteState>
/**
* Monotonically increasing counter that signals when the sidebar sort order
+20 -1
View File
@@ -1,3 +1,4 @@
/* eslint-disable max-lines */
import type { StateCreator } from 'zustand'
import type { AppState } from '../types'
import type { Worktree } from '../../../../shared/types'
@@ -39,6 +40,7 @@ function areWorktreesEqual(current: Worktree[] | undefined, next: Worktree[]): b
export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice> = (set, get) => ({
worktreesByRepo: {},
activeWorktreeId: null,
activeWorktreeSelectionNonce: 0,
deleteStateByWorktreeId: {},
sortEpoch: 0,
@@ -272,7 +274,16 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
let shouldClearUnread = false
set((s) => {
if (!worktreeId) {
return { activeWorktreeId: null }
return {
activeWorktreeId: null,
// Why: only bump the nonce when the selection genuinely changes.
// Re-selecting null when already null should not invalidate the
// frozen sort override, which would defeat the freeze effect.
activeWorktreeSelectionNonce:
s.activeWorktreeId === null
? s.activeWorktreeSelectionNonce
: s.activeWorktreeSelectionNonce + 1
}
}
const worktree = findWorktreeById(s.worktreesByRepo, worktreeId)
@@ -300,6 +311,14 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
return {
activeWorktreeId: worktreeId,
// Why: only bump the nonce when the worktreeId genuinely changes.
// Clicking the already-active worktree again would re-freeze the
// sort override with its current (post-click, isUnread:false) state,
// defeating the freeze and causing the card to drop in the list.
activeWorktreeSelectionNonce:
worktreeId === s.activeWorktreeId
? s.activeWorktreeSelectionNonce
: s.activeWorktreeSelectionNonce + 1,
activeFileId,
activeTabType,
worktreesByRepo: applyWorktreeUpdates(