From c8a8ef1b5cee339ae85d0ed5ced3b0c6e42a6827 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Thu, 16 Apr 2026 18:17:48 -0400 Subject: [PATCH] fix: stop smart sort from reordering after focus (#730) --- .../src/store/slices/store-cascades.test.ts | 95 +++++++++++++++++-- src/renderer/src/store/slices/worktrees.ts | 38 ++++---- 2 files changed, 103 insertions(+), 30 deletions(-) diff --git a/src/renderer/src/store/slices/store-cascades.test.ts b/src/renderer/src/store/slices/store-cascades.test.ts index 363f497ec..054de6272 100644 --- a/src/renderer/src/store/slices/store-cascades.test.ts +++ b/src/renderer/src/store/slices/store-cascades.test.ts @@ -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', () => { diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index 9c2ebc97a..b4454c41e 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -378,7 +378,6 @@ export const createWorktreeSlice: StateCreator 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 ? 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 = { 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 = shouldClearUnread ? { isUnread: false } : {} return { activeWorktreeId: worktreeId, activeFileId, @@ -513,7 +510,9 @@ export const createWorktreeSlice: StateCreator 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 return } - const updates: Parameters[0]['updates'] = { - lastActivityAt: now - } if (shouldClearUnread) { - updates.isUnread = false - } + const updates: Parameters[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()