fix: stop smart sort from reordering after focus (#730)

This commit is contained in:
Jinwoo Hong 2026-04-16 18:17:48 -04:00 committed by GitHub
parent bd16c0cfdd
commit c8a8ef1b5c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 103 additions and 30 deletions

View file

@ -1,5 +1,6 @@
/* eslint-disable max-lines */
import { describe, it, expect, vi, beforeEach } from 'vitest'
import { buildWorktreeComparator } from '@/components/sidebar/smart-sort'
// Mock sonner (imported by repos.ts)
vi.mock('sonner', () => ({ toast: { info: vi.fn(), success: vi.fn(), error: vi.fn() } }))
@ -307,10 +308,19 @@ describe('setActiveWorktree', () => {
it('does not rewrite sortOrder when selecting a worktree', () => {
const store = createTestStore()
const worktreeId = 'repo1::/path/wt1'
const lastActivityAt = 123456
seedStore(store, {
worktreesByRepo: {
repo1: [makeWorktree({ id: worktreeId, repoId: 'repo1', sortOrder: 123, isUnread: false })]
repo1: [
makeWorktree({
id: worktreeId,
repoId: 'repo1',
sortOrder: 123,
lastActivityAt,
isUnread: false
})
]
},
refreshGitHubForWorktree: vi.fn(),
refreshGitHubForWorktreeIfStale: vi.fn()
@ -320,15 +330,80 @@ describe('setActiveWorktree', () => {
const worktree = store.getState().worktreesByRepo.repo1[0]
expect(worktree.sortOrder).toBe(123)
// Why: setActiveWorktree persists lastActivityAt for the smart sort's
// time-decay signal, but must never touch sortOrder which is managed
// by persistSortOrder.
expect(mockApi.worktrees.updateMeta).toHaveBeenCalledWith(
expect.objectContaining({
worktreeId,
updates: expect.not.objectContaining({ sortOrder: expect.anything() })
})
)
expect(worktree.lastActivityAt).toBe(lastActivityAt)
// Why: selecting a worktree should not manufacture smart-sort activity.
// Persisted ordering signals come from real background work or edits, not focus.
expect(mockApi.worktrees.updateMeta).not.toHaveBeenCalled()
})
it('clears unread on selection without manufacturing smart-sort activity', () => {
const store = createTestStore()
const worktreeId = 'repo1::/path/wt1'
const lastActivityAt = 123456
seedStore(store, {
worktreesByRepo: {
repo1: [
makeWorktree({
id: worktreeId,
repoId: 'repo1',
isUnread: true,
lastActivityAt
})
]
},
refreshGitHubForWorktree: vi.fn(),
refreshGitHubForWorktreeIfStale: vi.fn()
})
store.getState().setActiveWorktree(worktreeId)
const worktree = store.getState().worktreesByRepo.repo1[0]
expect(worktree.isUnread).toBe(false)
expect(worktree.lastActivityAt).toBe(lastActivityAt)
expect(mockApi.worktrees.updateMeta).toHaveBeenCalledWith({
worktreeId,
updates: { isUnread: false }
})
})
it('does not change smart-sort rank after selection when a background event bumps sortEpoch', () => {
const store = createTestStore()
const focusedId = 'repo1::/path/focused'
const backgroundId = 'repo1::/path/background'
const now = new Date('2026-04-16T12:00:00.000Z').getTime()
vi.spyOn(Date, 'now').mockReturnValue(now)
seedStore(store, {
worktreesByRepo: {
repo1: [
makeWorktree({
id: focusedId,
repoId: 'repo1',
displayName: 'Focused',
lastActivityAt: now - 2 * 60_000
}),
makeWorktree({
id: backgroundId,
repoId: 'repo1',
displayName: 'Background',
lastActivityAt: now - 60_000
})
]
},
refreshGitHubForWorktree: vi.fn(),
refreshGitHubForWorktreeIfStale: vi.fn()
})
store.getState().setActiveWorktree(focusedId)
store.getState().bumpWorktreeActivity(backgroundId)
const worktrees = [...store.getState().worktreesByRepo.repo1]
const repoMap = new Map(store.getState().repos.map((repo) => [repo.id, repo]))
worktrees.sort(buildWorktreeComparator('smart', {}, repoMap, null, now))
expect(worktrees.map((worktree) => worktree.id)).toEqual([backgroundId, focusedId])
})
it('falls back to the worktree browser tab when the restored editor id belongs to a different worktree', () => {

View file

@ -378,7 +378,6 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
const reconciledActiveTabId = worktreeId
? get().reconcileWorktreeTabModel(worktreeId).activeRenderableTabId
: null
const now = Date.now()
let shouldClearUnread = false
set((s) => {
if (!worktreeId) {
@ -497,15 +496,13 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
? restoredTabId
: (worktreeTabs[0]?.id ?? null)
// Why: bump lastActivityAt so the smart sort's time-decay signal
// reflects navigation recency. Do NOT bump sortEpoch — that would
// re-sort the sidebar on every click, causing the reorder-on-click
// bug (PR #209). The timestamp is persisted so the next sortEpoch
// bump (from a background event) includes this worktree's updated score.
const metaUpdates: Partial<WorktreeMeta> = { lastActivityAt: now }
if (shouldClearUnread) {
metaUpdates.isUnread = false
}
// Why: focusing a worktree is not meaningful background activity for the
// smart sort. Writing lastActivityAt here makes the next unrelated
// sortEpoch bump reshuffle cards based on what the user merely looked at,
// which is the "jump after focus" bug reported in Slack. Keep selection
// side-effects limited to unread clearing; true activity signals such as
// PTY lifecycle and explicit edits still flow through bumpWorktreeActivity.
const metaUpdates: Partial<WorktreeMeta> = shouldClearUnread ? { isUnread: false } : {}
return {
activeWorktreeId: worktreeId,
activeFileId,
@ -513,7 +510,9 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
activeTabType,
activeTabTypeByWorktree: { ...s.activeTabTypeByWorktree, [worktreeId]: activeTabType },
activeTabId,
worktreesByRepo: applyWorktreeUpdates(s.worktreesByRepo, worktreeId, metaUpdates)
...(shouldClearUnread
? { worktreesByRepo: applyWorktreeUpdates(s.worktreesByRepo, worktreeId, metaUpdates) }
: {})
}
})
@ -546,17 +545,16 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
return
}
const updates: Parameters<typeof window.api.worktrees.updateMeta>[0]['updates'] = {
lastActivityAt: now
}
if (shouldClearUnread) {
updates.isUnread = false
}
const updates: Parameters<typeof window.api.worktrees.updateMeta>[0]['updates'] = {
isUnread: false
}
void window.api.worktrees.updateMeta({ worktreeId, updates }).catch((err) => {
console.error('Failed to persist worktree activation state:', err)
void get().fetchWorktrees(getRepoIdFromWorktreeId(worktreeId))
})
void window.api.worktrees.updateMeta({ worktreeId, updates }).catch((err) => {
console.error('Failed to persist worktree activation state:', err)
void get().fetchWorktrees(getRepoIdFromWorktreeId(worktreeId))
})
}
},
allWorktrees: () => Object.values(get().worktreesByRepo).flat()