diff --git a/docs/back-button-tasks-page.md b/docs/back-button-tasks-page.md new file mode 100644 index 000000000..370117cf5 --- /dev/null +++ b/docs/back-button-tasks-page.md @@ -0,0 +1,138 @@ +# Back/Forward support for the Tasks page + +## Goal + +Make the titlebar back/forward buttons and the `Cmd/Ctrl+Alt+←/→` shortcut +traverse Tasks visits in addition to worktree activations. Today both are +no-ops outside `activeView === 'terminal'` (see `App.tsx:616`, `App.tsx:900`). + +## Current shape + +- History lives in `src/renderer/src/store/slices/worktree-nav-history.ts`. +- Entries are `string[]` (worktree IDs), recorded from + `worktree-activation.ts:96` via `recordWorktreeVisit`. +- `goBackWorktree`/`goForwardWorktree` skip dead worktrees via + `findPrev/NextLiveWorktreeHistoryIndex`, gated by an `activator` ref and + `isNavigatingHistory` to prevent re-recording. +- UI (pre-change): titlebar buttons hidden unless + `activeView === 'terminal'`; shortcut ignored outside terminal view. + +## Minimal implementation (downscoped) + +1. **History entry type** → `string | 'tasks'`. + - `findPrev/NextLiveWorktreeHistoryIndex` must short-circuit on `'tasks'` + before calling `findWorktreeById` (which takes a worktree id, not a + view tag) — Tasks entries are unconditionally live. + - Keep `recordWorktreeVisit(worktreeId: string)` signature to avoid churn + in `terminals-hydration.test.ts`. Add a sibling + `recordViewVisit(entry: 'tasks')` that shares the dedupe/truncate/cap + logic. + - Keep existing names (`worktreeNavHistory`, `goBackWorktree`, + `canGoBackWorktreeHistory`). They're now slight misnomers since + entries may be `'tasks'`, but renaming touches ~20 call sites for no + behavioral win. Add a one-line comment at the top of the slice + stating the entry type and the intentional keeping of the name, so + future readers don't assume it's worktree-only. + +2. **Record Tasks visits** → call `recordViewVisit('tasks')` from + `openTaskPage` (`ui.ts:189`). No `isNavigatingHistory` guard needed: + back-to-Tasks routes through `setActiveView('tasks')` per step 3, which + never touches `openTaskPage`. The slice's existing adjacent-entry dedupe + covers any other re-entry. Note: `openTaskPage` is called with varying + `taskPageData` (e.g. `{ taskSource: 'github' }` vs `'linear'` from + `SidebarNav.tsx:88,102`); all collapse to a single `'tasks'` entry and + dedupe against the current entry, so toggling presets produces no extra + history entries. This is consistent with the out-of-scope "no per-entry + snapshotting" decision. + +3. **Dispatch in goBack/goForward** → based on entry kind: + - worktreeId → existing `activator(worktreeId)` path. When current entry + is `'tasks'`, no extra view handling is needed: + `activateAndRevealWorktree` at `worktree-activation.ts:82-84` already + switches `activeView` back to `'terminal'`. + - `'tasks'` → `setActiveView('tasks')` (not `openTaskPage` — avoids + mutating `previousViewBeforeTasks` and the SWR prefetch). + +4. **Fix Esc-close history desync.** `closeTaskPage` (`ui.ts:210`) currently + sets `activeView` to `previousViewBeforeTasks` without touching the + history index, so after `A → Tasks → Esc` the index still points at the + `'tasks'` entry and Back becomes a visual no-op (activator re-activates A) + while Forward re-opens Tasks. Fix: in `closeTaskPage`, if the current + history entry is `'tasks'`, move the index to the previous live entry + (same scan as `findPrevLiveWorktreeHistoryIndex`). Sub-case: if there is + no previous live entry (e.g. user's first action was open Tasks, then + Esc — history `['tasks']` at index 0), leave the index unchanged at 0. + Accept the minor cost (Back becomes a visual no-op until a real visit + records a new entry) rather than setting to -1, which would lose the + only forward target. Guard with `isNavigatingHistory` is unnecessary + here — `closeTaskPage` is never invoked from the history path. + +5. **Unhide UI on Tasks** (allowlist, not denylist — `activeView` union is + `'terminal' | 'settings' | 'tasks'`, but an allowlist won't silently + include future views): + - Replace the `activeView !== 'terminal'` early-return at `App.tsx:616` + with `activeView !== 'terminal' && activeView !== 'tasks'`. Update the + adjacent comment (`App.tsx:613-615`) — it currently claims the + shortcut is a no-op outside terminal because the buttons are hidden; + replace with: back/forward traverse worktree + Tasks visits, so the + shortcut is active whenever the button cluster is (terminal or + Tasks); still suppressed elsewhere (Settings). + - Change the titlebar cluster guard at `App.tsx:900` to + `activeView === 'terminal' || activeView === 'tasks'`. Update the + adjacent comment (`App.tsx:896-899`) — drop the "terminal view only" + framing; explain the cluster is shown wherever the history shortcut + is live, and hidden in Settings to keep that view modal-ish. + +6. **Tests**: extend `worktree-nav-history.test.ts` for mixed entries: + - `A → Tasks → B`, back lands on Tasks, back again on A. + - Tasks dedupe against current entry. + - Dead worktree between Tasks entries is skipped. + - `A → Tasks → closeTaskPage()`: index moves back to A; subsequent Back + is a no-op, Forward re-opens Tasks. + - `Tasks (only entry) → closeTaskPage()`: index stays at 0, Back is a + visual no-op. + +## Explicitly out of scope + +- **Settings in history.** Stays modal-ish; closes via + `previousViewBeforeSettings`. +- **Per-entry `taskPageData` snapshotting.** Back-to-Tasks opens Tasks with + default filters/source. Revisit if users complain. +- **Session persistence of history.** Keep in-memory only (it already is). + +## Edge cases handled + +- Worktree deletion mid-session → skipped by existing live-check; Tasks + entries always live. +- Dedupe semantics → same adjacency rule works for `'tasks'`. +- Activator failure on worktree → unchanged (`result !== false` gate). Tasks + dispatch can't fail. +- Editable-target guard at `App.tsx:600` still fires first → typing in the + Tasks search box + `Cmd+Alt+←` remains a no-op. +- `MAX_HISTORY = 50` → Tasks entries consume slots; worst-case effective + worktree depth halves to ~25. Acceptable. + +## Known residual quirks (accepted) + +1. **Prefetch loss on back-to-Tasks.** Routing through `setActiveView('tasks')` + skips the SWR prefetch at `ui.ts:201-208`; back-to-Tasks is ~300–800ms + slower than a fresh open via the sidebar. Not a regression vs today + (back-to-Tasks isn't possible at all today). + +2. **Titlebar layout shift.** Revealing the button cluster on Tasks changes + the titlebar — needs a visual check that nothing Tasks-specific collides. + +## Files touched + +- `src/renderer/src/store/slices/worktree-nav-history.ts` — entry type, + `recordViewVisit`, dispatch in `goBack/goForwardWorktree`. +- `src/renderer/src/store/slices/worktree-nav-history.test.ts` — new cases. +- `src/renderer/src/store/slices/ui.ts` — `openTaskPage` records via + `recordViewVisit`; `closeTaskPage` rewinds history index when the current + entry is `'tasks'`. +- `src/renderer/src/App.tsx` — widen the two `activeView === 'terminal'` + guards to include `'tasks'`, and update the adjacent "why" comments at + `App.tsx:613-615` and `App.tsx:896-899` to reflect the new invariant. + +Estimated diff: ~30 lines of slice logic, 1 call site in `ui.ts`, 2 guards +in `App.tsx`, plus tests. diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index b50dfea0c..5f4974b40 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -610,10 +610,10 @@ function App(): React.JSX.Element { (isMac ? e.metaKey && !e.ctrlKey : e.ctrlKey && !e.metaKey) && (e.code === 'ArrowLeft' || e.code === 'ArrowRight') ) { - // Why: hidden buttons in non-terminal views mean the shortcut must be - // a no-op there too — navigating worktree history from Settings or - // Tasks is not a meaningful action. - if (activeView !== 'terminal') { + // Why: Back/Forward traverse mixed worktree + Tasks visits, so the + // shortcut is active wherever the titlebar button cluster is (terminal + // or tasks). Still suppressed in Settings to keep that view modal-ish. + if (activeView !== 'terminal' && activeView !== 'tasks') { return } dispatchClearModifierHints() @@ -893,11 +893,10 @@ function App(): React.JSX.Element { ) : null} - {/* Why: Back/Forward navigate worktree-activation history. Only - meaningful while viewing a worktree (terminal view); hidden in - Settings/Tasks/Landing to keep the titlebar compact and the - semantics unambiguous. */} - {activeView === 'terminal' && ( + {/* Why: Back/Forward traverse mixed worktree + Tasks history, so the + cluster is shown wherever the history shortcut is live (terminal or + tasks). Hidden in Settings to keep that view modal-ish. */} + {(activeView === 'terminal' || activeView === 'tasks') && (
diff --git a/src/renderer/src/lib/worktree-activation.ts b/src/renderer/src/lib/worktree-activation.ts index 3973768ee..2eee77d4a 100644 --- a/src/renderer/src/lib/worktree-activation.ts +++ b/src/renderer/src/lib/worktree-activation.ts @@ -3,7 +3,10 @@ import { shouldAutoCreateInitialTerminal } from '@/components/terminal/initial-t import { buildSetupRunnerCommand } from './setup-runner' import { useAppStore } from '@/store' import { findWorktreeById } from '@/store/slices/worktree-helpers' -import { setWorktreeNavActivator } from '@/store/slices/worktree-nav-history' +import { + setWorktreeNavActivator, + setWorktreeNavViewActivator +} from '@/store/slices/worktree-nav-history' // Why: issue commands can originate from two sources with different shapes — // (1) a repo-level runner script generated by main (WorktreeSetupLaunch), or @@ -211,3 +214,11 @@ export function ensureWorktreeHasInitialTerminal( // @/store, which activation already imports from. Registering the activator // at module init here lets the slice call back without importing this file. setWorktreeNavActivator(activateAndRevealWorktree) + +// Why: Tasks entries in the nav history dispatch via setActiveView('tasks') +// (not openTaskPage) — see the 'tasks' branch in navigateToIndex. Going this +// route avoids mutating previousViewBeforeTasks and skips the SWR prefetch +// (an accepted residual from the design doc; see "Known residual quirks"). +setWorktreeNavViewActivator((entry) => { + useAppStore.getState().setActiveView(entry) +}) diff --git a/src/renderer/src/store/slices/ui.ts b/src/renderer/src/store/slices/ui.ts index be8aeb7b3..58d9b2ef2 100644 --- a/src/renderer/src/store/slices/ui.ts +++ b/src/renderer/src/store/slices/ui.ts @@ -1,6 +1,7 @@ /* eslint-disable max-lines */ import type { StateCreator } from 'zustand' import type { AppState } from '../types' +import { findPrevLiveWorktreeHistoryIndex } from './worktree-nav-history' import type { ChangelogData, PersistedUIState, @@ -198,6 +199,13 @@ export const createUISlice: StateCreator = (set, get) taskPageData: {}, newWorkspaceDraft: null, openTaskPage: (data = {}) => { + // Why: record a Tasks visit in the shared back/forward history so the + // titlebar Back/Forward buttons can return to Tasks. All task-source + // variants (github/linear presets) collapse to a single 'tasks' entry; + // the slice's adjacent-entry dedupe drops re-opens. No isNavigatingHistory + // guard needed — back-to-Tasks routes through setActiveView('tasks') and + // never re-enters openTaskPage. + get().recordViewVisit('tasks') set((state) => ({ activeView: 'tasks', previousViewBeforeTasks: @@ -219,10 +227,30 @@ export const createUISlice: StateCreator = (set, get) } }, closeTaskPage: () => - set((state) => ({ - activeView: state.previousViewBeforeTasks, - taskPageData: {} - })), + set((state) => { + // Why: Esc-close from Tasks must rewind the history index if we're + // currently parked on a 'tasks' entry. Without this, A → Tasks → Esc + // leaves the index at the 'tasks' entry, making Back a visual no-op + // (activator re-activates A) and Forward re-opens Tasks. If there is no + // earlier live entry (e.g. history is just ['tasks']), leave the index + // at 0 — setting it to -1 would lose the only forward target, while the + // resulting Back visual no-op self-heals as soon as a real visit records + // a new entry. closeTaskPage never runs from the history-nav path, so no + // isNavigatingHistory guard is needed. + const currentEntry = state.worktreeNavHistory[state.worktreeNavHistoryIndex] + let nextHistoryIndex = state.worktreeNavHistoryIndex + if (currentEntry === 'tasks') { + const prev = findPrevLiveWorktreeHistoryIndex(state) + if (prev !== null) { + nextHistoryIndex = prev + } + } + return { + activeView: state.previousViewBeforeTasks, + taskPageData: {}, + worktreeNavHistoryIndex: nextHistoryIndex + } + }), setNewWorkspaceDraft: (draft) => set({ newWorkspaceDraft: draft }), clearNewWorkspaceDraft: () => set({ newWorkspaceDraft: null }), openSettingsPage: () => diff --git a/src/renderer/src/store/slices/worktree-nav-history.test.ts b/src/renderer/src/store/slices/worktree-nav-history.test.ts index 8a1666aae..a24c5311c 100644 --- a/src/renderer/src/store/slices/worktree-nav-history.test.ts +++ b/src/renderer/src/store/slices/worktree-nav-history.test.ts @@ -6,7 +6,9 @@ import { canGoBackWorktreeHistory, canGoForwardWorktreeHistory, createWorktreeNavHistorySlice, - setWorktreeNavActivator + findPrevLiveWorktreeHistoryIndex, + setWorktreeNavActivator, + setWorktreeNavViewActivator } from './worktree-nav-history' type MinimalState = Pick< @@ -15,6 +17,7 @@ type MinimalState = Pick< | 'worktreeNavHistoryIndex' | 'isNavigatingHistory' | 'recordWorktreeVisit' + | 'recordViewVisit' | 'goBackWorktree' | 'goForwardWorktree' | 'worktreesByRepo' @@ -182,6 +185,104 @@ describe('worktree-nav-history slice: goBack / goForward', () => { }) }) +describe('worktree-nav-history slice: Tasks entries', () => { + afterEach(() => { + setWorktreeNavActivator(null) + setWorktreeNavViewActivator(null) + }) + + it('A -> Tasks -> B, back lands on Tasks then A', () => { + const store = createHistoryStore(['a', 'b']) + const activated: string[] = [] + const viewed: string[] = [] + setWorktreeNavActivator((id) => { + activated.push(id as string) + return { primaryTabId: null } + }) + setWorktreeNavViewActivator((v) => { + viewed.push(v) + }) + + store.getState().recordWorktreeVisit('a') + store.getState().recordViewVisit('tasks') + store.getState().recordWorktreeVisit('b') + + expect(store.getState().worktreeNavHistory).toEqual(['a', 'tasks', 'b']) + expect(store.getState().worktreeNavHistoryIndex).toBe(2) + + store.getState().goBackWorktree() + expect(viewed).toEqual(['tasks']) + expect(store.getState().worktreeNavHistoryIndex).toBe(1) + + store.getState().goBackWorktree() + expect(activated).toEqual(['a']) + expect(store.getState().worktreeNavHistoryIndex).toBe(0) + }) + + it('dedupes Tasks against the current Tasks entry', () => { + const store = createHistoryStore(['a']) + store.getState().recordWorktreeVisit('a') + store.getState().recordViewVisit('tasks') + store.getState().recordViewVisit('tasks') + store.getState().recordViewVisit('tasks') + + expect(store.getState().worktreeNavHistory).toEqual(['a', 'tasks']) + expect(store.getState().worktreeNavHistoryIndex).toBe(1) + }) + + it('skips a dead worktree between two Tasks entries', () => { + // 'b' is deleted; history is [tasks, b, tasks]. + const store = createHistoryStore([]) + const viewed: string[] = [] + setWorktreeNavViewActivator((v) => { + viewed.push(v) + }) + + store.setState({ + worktreeNavHistory: ['tasks', 'b', 'tasks'], + worktreeNavHistoryIndex: 2 + }) + + store.getState().goBackWorktree() + expect(viewed).toEqual(['tasks']) + expect(store.getState().worktreeNavHistoryIndex).toBe(0) + }) + + it('closeTaskPage-style rewind: A -> Tasks, rewind moves index to A', () => { + const store = createHistoryStore(['a']) + store.getState().recordWorktreeVisit('a') + store.getState().recordViewVisit('tasks') + expect(store.getState().worktreeNavHistoryIndex).toBe(1) + + // Simulate closeTaskPage's rewind logic. + const prev = findPrevLiveWorktreeHistoryIndex(store.getState() as AppState) + expect(prev).toBe(0) + store.setState({ worktreeNavHistoryIndex: prev ?? store.getState().worktreeNavHistoryIndex }) + + // Forward re-opens Tasks. + setWorktreeNavActivator(() => ({ primaryTabId: null })) + const viewed: string[] = [] + setWorktreeNavViewActivator((v) => { + viewed.push(v) + }) + + store.getState().goForwardWorktree() + expect(viewed).toEqual(['tasks']) + expect(store.getState().worktreeNavHistoryIndex).toBe(1) + }) + + it('closeTaskPage-style rewind with only-Tasks history leaves index at 0', () => { + const store = createHistoryStore([]) + store.getState().recordViewVisit('tasks') + expect(store.getState().worktreeNavHistoryIndex).toBe(0) + + const prev = findPrevLiveWorktreeHistoryIndex(store.getState() as AppState) + expect(prev).toBeNull() + // closeTaskPage leaves the index unchanged when there's no prior live entry. + expect(store.getState().worktreeNavHistoryIndex).toBe(0) + }) +}) + describe('worktree-nav-history selectors', () => { it('reports back availability only when a live prior entry exists', () => { const store = createHistoryStore(['c']) diff --git a/src/renderer/src/store/slices/worktree-nav-history.ts b/src/renderer/src/store/slices/worktree-nav-history.ts index d7900d9e7..9aa1479a7 100644 --- a/src/renderer/src/store/slices/worktree-nav-history.ts +++ b/src/renderer/src/store/slices/worktree-nav-history.ts @@ -8,9 +8,16 @@ import { findWorktreeById } from './worktree-helpers' // linear skip-deleted scan in goBack/goForward stays trivially cheap. const MAX_HISTORY = 50 +// Why: entries are worktree IDs OR the sentinel 'tasks' for visits to the +// Tasks page. The slice, selector, and action names retain the +// "worktree"/"WorktreeHistory" prefix for call-site stability — renaming +// across ~20 sites would churn for no behavior win. Tasks entries are +// always live (never skipped by findPrev/NextLiveWorktreeHistoryIndex). +export type WorktreeNavHistoryEntry = string | 'tasks' + export type WorktreeNavHistorySlice = { // Linear history, oldest -> newest. - worktreeNavHistory: string[] + worktreeNavHistory: WorktreeNavHistoryEntry[] // Index into worktreeNavHistory; points at the currently-active entry. // -1 means empty (no worktree ever activated this session). worktreeNavHistoryIndex: number @@ -21,26 +28,76 @@ export type WorktreeNavHistorySlice = { isNavigatingHistory: boolean recordWorktreeVisit: (worktreeId: string) => void + recordViewVisit: (entry: 'tasks') => void goBackWorktree: () => void goForwardWorktree: () => void } type ActivateFn = (worktreeId: string) => unknown +type ViewActivateFn = (entry: 'tasks') => void // Why: the slice must call activateAndRevealWorktree from goBack/goForward, but // importing it directly would create a cycle (activation imports the store). // Install the reference at module init via setWorktreeNavActivator and keep // the slice itself unaware of the activation module. let activator: ActivateFn | null = null +let viewActivator: ViewActivateFn | null = null export function setWorktreeNavActivator(fn: ActivateFn | null): void { activator = fn } +// Why: installed by App-level init so the slice can dispatch 'tasks' entries +// to setActiveView('tasks') without importing the UI slice directly (the UI +// slice already transitively depends on this module via the store creator). +export function setWorktreeNavViewActivator(fn: ViewActivateFn | null): void { + viewActivator = fn +} + +// Why: Tasks entries short-circuit as live unconditionally — findWorktreeById +// takes a worktree id and would always return undefined for the 'tasks' +// sentinel, which would incorrectly treat Tasks entries as dead. +function isLiveEntry(entry: WorktreeNavHistoryEntry, state: AppState): boolean { + if (entry === 'tasks') { + return true + } + return findWorktreeById(state.worktreesByRepo, entry) !== undefined +} + +function appendHistoryEntry( + s: { worktreeNavHistory: WorktreeNavHistoryEntry[]; worktreeNavHistoryIndex: number }, + entry: WorktreeNavHistoryEntry +): { worktreeNavHistory: WorktreeNavHistoryEntry[]; worktreeNavHistoryIndex: number } { + // Why: re-visiting the same entry must not pollute history. The de-dup + // applies only to the current entry so that A -> B -> A remains a valid + // stack (user left B, returned to A). Same rule covers Tasks re-opens + // with different taskPageData — all collapse to a single 'tasks' entry. + if (s.worktreeNavHistory[s.worktreeNavHistoryIndex] === entry) { + return s + } + + // Truncate any forward entries, then append and advance the index. + const truncated = s.worktreeNavHistory.slice(0, s.worktreeNavHistoryIndex + 1) + truncated.push(entry) + let nextIndex = s.worktreeNavHistoryIndex + 1 + + // Why: cap eviction drops the oldest entries. The index must shift left + // by the same count so it still points at the just-appended current entry. + if (truncated.length > MAX_HISTORY) { + const evict = truncated.length - MAX_HISTORY + truncated.splice(0, evict) + nextIndex = Math.max(0, nextIndex - evict) + } + + return { + worktreeNavHistory: truncated, + worktreeNavHistoryIndex: nextIndex + } +} + export function findPrevLiveWorktreeHistoryIndex(state: AppState): number | null { for (let i = state.worktreeNavHistoryIndex - 1; i >= 0; i--) { - const id = state.worktreeNavHistory[i] - if (findWorktreeById(state.worktreesByRepo, id)) { + if (isLiveEntry(state.worktreeNavHistory[i], state)) { return i } } @@ -49,8 +106,7 @@ export function findPrevLiveWorktreeHistoryIndex(state: AppState): number | null export function findNextLiveWorktreeHistoryIndex(state: AppState): number | null { for (let i = state.worktreeNavHistoryIndex + 1; i < state.worktreeNavHistory.length; i++) { - const id = state.worktreeNavHistory[i] - if (findWorktreeById(state.worktreesByRepo, id)) { + if (isLiveEntry(state.worktreeNavHistory[i], state)) { return i } } @@ -76,95 +132,90 @@ export const createWorktreeNavHistorySlice: StateCreator< isNavigatingHistory: false, recordWorktreeVisit: (worktreeId) => { - set((s) => { - // Why: re-activating the same worktree must not pollute history. The - // de-dup applies only to the current entry so that A -> B -> A remains - // a valid stack (user left B, returned to A via the sidebar). - if (s.worktreeNavHistory[s.worktreeNavHistoryIndex] === worktreeId) { - return s - } + set((s) => appendHistoryEntry(s, worktreeId)) + }, - // Truncate any forward entries, then append and advance the index. - const truncated = s.worktreeNavHistory.slice(0, s.worktreeNavHistoryIndex + 1) - truncated.push(worktreeId) - let nextIndex = s.worktreeNavHistoryIndex + 1 - - // Why: cap eviction drops the oldest entries. The index must shift left - // by the same count so it still points at the just-appended current entry. - if (truncated.length > MAX_HISTORY) { - const evict = truncated.length - MAX_HISTORY - truncated.splice(0, evict) - nextIndex = Math.max(0, nextIndex - evict) - } - - return { - worktreeNavHistory: truncated, - worktreeNavHistoryIndex: nextIndex - } - }) + recordViewVisit: (entry) => { + set((s) => appendHistoryEntry(s, entry)) }, goBackWorktree: () => { - const state = get() - if (state.worktreeNavHistoryIndex <= 0) { - return - } - const targetIndex = findPrevLiveWorktreeHistoryIndex(state) - if (targetIndex === null) { - return - } - if (!activator) { - // Why: a silent no-op here would mean the back/forward chord simply - // does nothing with no diagnostic. The activator is registered at - // module init by worktree-activation.ts, so a missing activator means - // either test setup forgot to install one or the production import - // graph regressed. - console.warn('goBackWorktree called before worktree activator was registered') - return - } - const targetId = state.worktreeNavHistory[targetIndex] - // Why: capture-and-restore (not force false) so re-entrant navigation - // (e.g. a store subscriber synchronously triggers another goBack) does - // not race on the boolean — the outer call's `finally` restores its own - // prior value rather than clobbering state set by an inner call. - const prevNavigating = get().isNavigatingHistory - set({ isNavigatingHistory: true }) - try { - // Why: activateAndRevealWorktree returns `ActivateAndRevealResult | false`; - // `false` is the only observable failure signal. Advance the index only on - // success so the slice stays consistent with what the user actually sees. - const result = activator(targetId) - if (result !== false) { - set({ worktreeNavHistoryIndex: targetIndex }) - } - } finally { - set({ isNavigatingHistory: prevNavigating }) - } + navigateToIndex(get, set, 'back') }, goForwardWorktree: () => { - const state = get() + navigateToIndex(get, set, 'forward') + } +}) + +function navigateToIndex( + get: () => AppState, + set: (partial: Partial) => void, + direction: 'back' | 'forward' +): void { + const state = get() + if (direction === 'back') { + if (state.worktreeNavHistoryIndex <= 0) { + return + } + } else { if (state.worktreeNavHistoryIndex >= state.worktreeNavHistory.length - 1) { return } - const targetIndex = findNextLiveWorktreeHistoryIndex(state) - if (targetIndex === null) { - return - } - if (!activator) { - console.warn('goForwardWorktree called before worktree activator was registered') - return - } - const targetId = state.worktreeNavHistory[targetIndex] - const prevNavigating = get().isNavigatingHistory - set({ isNavigatingHistory: true }) - try { - const result = activator(targetId) - if (result !== false) { - set({ worktreeNavHistoryIndex: targetIndex }) - } - } finally { - set({ isNavigatingHistory: prevNavigating }) - } } -}) + const targetIndex = + direction === 'back' + ? findPrevLiveWorktreeHistoryIndex(state) + : findNextLiveWorktreeHistoryIndex(state) + if (targetIndex === null) { + return + } + const targetEntry = state.worktreeNavHistory[targetIndex] + + // Why: capture-and-restore (not force false) so re-entrant navigation + // (e.g. a store subscriber synchronously triggers another goBack) does + // not race on the boolean — the outer call's `finally` restores its own + // prior value rather than clobbering state set by an inner call. + const prevNavigating = get().isNavigatingHistory + set({ isNavigatingHistory: true } as Partial) + try { + if (targetEntry === 'tasks') { + if (!viewActivator) { + // Why: a silent no-op would mean the back/forward chord lands on a + // Tasks history entry and appears broken. See setWorktreeNavActivator + // rationale above. + console.warn( + `go${direction === 'back' ? 'Back' : 'Forward'}Worktree: view activator not registered` + ) + return + } + // Why: dispatch via setActiveView (installed as viewActivator) rather + // than openTaskPage so we don't mutate previousViewBeforeTasks or fire + // the SWR prefetch on back-to-Tasks. activateAndRevealWorktree on the + // other branch already switches activeView back to 'terminal'. + viewActivator('tasks') + set({ worktreeNavHistoryIndex: targetIndex } as Partial) + } else { + if (!activator) { + // Why: a silent no-op here would mean the back/forward chord simply + // does nothing with no diagnostic. The activator is registered at + // module init by worktree-activation.ts, so a missing activator means + // either test setup forgot to install one or the production import + // graph regressed. + console.warn( + `go${direction === 'back' ? 'Back' : 'Forward'}Worktree called before worktree activator was registered` + ) + return + } + // Why: activateAndRevealWorktree returns `ActivateAndRevealResult | false`; + // `false` is the only observable failure signal. Advance the index only on + // success so the slice stays consistent with what the user actually sees. + const result = activator(targetEntry) + if (result !== false) { + set({ worktreeNavHistoryIndex: targetIndex } as Partial) + } + } + } finally { + set({ isNavigatingHistory: prevNavigating } as Partial) + } +}