From 02125deee838b2018f5f6f2e3219a8048cce2b19 Mon Sep 17 00:00:00 2001 From: HuanLinOTO Date: Wed, 26 Aug 2026 22:46:56 +0800 Subject: [PATCH] fix: address Copilot PR review (4 comments) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. getSessionStates() returns defensive copy (new Map) — callers can't mutate the live bySession map at runtime 2. closePinnedInHome passes home cwd to ptyClose — the host resolves the PTY in the correct workspace container (same scope the WS open used); previously only sessionId was passed 3. setTabPin walk uses findIndex + conditional slice instead of map() — only the leaf/path containing the target tab gets a new array; idempotent no-ops return the same tab reference (=== holds) 4. setTabPin guards non-terminal tabs (tab.type !== 'terminal' early return in apply); sanitizePersistedTab strips pin from non-terminal tabs on hydration — keeps the terminal-only invariant defensive --- src/client/Sidebar.tsx | 7 ++++++- src/client/state.ts | 36 ++++++++++++++++++++++++++++++------ 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/src/client/Sidebar.tsx b/src/client/Sidebar.tsx index 8cd7838..8897571 100644 --- a/src/client/Sidebar.tsx +++ b/src/client/Sidebar.tsx @@ -1266,6 +1266,11 @@ export function Sidebar(props: { ctx: Context; store: SidebarStore }) { if (pinnedVirtualTabs.length === 0) return actions const closePinnedInHome = (virtualId: string): void => { const { homeSessionId, tabId: originalId } = parsePinnedVirtualId(virtualId) + // The home cwd lives in the virtual tab's meta (snapshotted at pin + // time) — pass it to ptyClose so the host resolves the PTY in the + // correct workspace container (same scope the WS open used). + const vtab = pinnedVirtualTabs.find(t => t.id === virtualId) + const homeCwd = vtab !== undefined ? getPinnedHomeScope(vtab)?.cwd : undefined store.reduceFor(homeSessionId, s => { const leaf = leafWithTab(s.splits, originalId) ?? leafWithTab(s.bottomSplits, originalId) if (leaf !== undefined) return closeTab(s, leaf.id, originalId) @@ -1275,7 +1280,7 @@ export function Sidebar(props: { ctx: Context; store: SidebarStore }) { if (isAgentTabId(originalId)) { void api.agentPtyClose(agentUuidOf(originalId)).catch(() => { /* already released */ }) } else { - void api.ptyClose({ sessionId: homeSessionId }, originalId).catch(() => { /* already released */ }) + void api.ptyClose({ sessionId: homeSessionId, ...(homeCwd !== undefined ? { cwd: homeCwd } : {}) }, originalId).catch(() => { /* already released */ }) } if (activePinnedTabId === virtualId) setActivePinnedTabId(null) setPinnedRevision(v => v + 1) diff --git a/src/client/state.ts b/src/client/state.ts index 5b610b6..5ecde61 100644 --- a/src/client/state.ts +++ b/src/client/state.ts @@ -569,6 +569,9 @@ export function setTabPin( ): SidebarState { let changed = false const apply = (tab: SidebarTab): SidebarTab => { + // Pin is terminal-only (design YAGNI): a defensive guard keeps the + // invariant even if a caller accidentally targets a non-terminal tab. + if (tab.type !== 'terminal') return tab // Idempotent: setting the same pin (deep-equal on scope + homeCwd) is a // no-op so re-clicking the menu item never churns the state. if (pin === null) { @@ -586,15 +589,34 @@ export function setTabPin( } const walk = (node: SplitNode): SplitNode => { if (node.kind === 'leaf') { - const tabs = node.tabs.map(tab => (tab.id === tabId ? apply(tab) : tab)) - return tabs === node.tabs ? node : { ...node, tabs } + // Find the target tab without rebuilding the whole array: only clone + // when the tab is actually here and apply changed it (idempotent + // no-ops return the same tab reference, so === holds). + const idx = node.tabs.findIndex(tab => tab.id === tabId) + if (idx < 0) return node + const oldTab = node.tabs[idx]! + const newTab = apply(oldTab) + if (newTab === oldTab) return node + const tabs = node.tabs.slice() + tabs[idx] = newTab + return { ...node, tabs } } const children = node.children.map(walk) - return children === node.children ? node : { ...node, children } + // Only rebuild if at least one child actually changed reference. + if (children.every((child, i) => child === node.children[i])) return node + return { ...node, children } } const splits = walk(state.splits) const bottomSplits = walk(state.bottomSplits) - const floats = state.floats.map(float => (float.tab.id === tabId ? { ...float, tab: apply(float.tab) } : float)) + const floatIdx = state.floats.findIndex(f => f.tab.id === tabId) + const floats = floatIdx < 0 ? state.floats : (() => { + const oldFloat = state.floats[floatIdx]! + const newTab = apply(oldFloat.tab) + if (newTab === oldFloat.tab) return state.floats + const next = state.floats.slice() + next[floatIdx] = { ...oldFloat, tab: newTab } + return next + })() return changed ? { ...state, splits, bottomSplits, floats } : state } @@ -1341,8 +1363,10 @@ function sanitizePersistedTab(tab: unknown): SidebarTab | 'diff' | undefined { // shape so a hand-edited / corrupted pin never crashes the rail's // resolver: an unknown scope or a non-string homeCwd drops the pin // silently (the tab survives, just unpinned — the legacy behavior). + // Pin is terminal-only: a non-terminal tab carrying a persisted pin + // (e.g. from a hand-edited state) has it stripped here. const pin = (candidate as Record).pin - if (pin !== null && typeof pin === 'object' && !Array.isArray(pin)) { + if (pin !== null && typeof pin === 'object' && !Array.isArray(pin) && result.type === 'terminal') { const pinRecord = pin as Record if (pinRecord.scope === 'workspace' || pinRecord.scope === 'global') { const homeCwd = pinRecord.homeCwd @@ -1525,7 +1549,7 @@ export class SidebarStore { * visible until first load — accepted as YAGNI by the design). */ getSessionStates(): ReadonlyMap { - return this.bySession + return new Map(this.bySession) } /** Apply a pure reducer (returns the next state). */