From 4fec2c0d2374d85ad65c31e8c5574fcef5a4ad07 Mon Sep 17 00:00:00 2001 From: yuhi yamane Date: Wed, 13 May 2026 12:19:54 +0900 Subject: [PATCH] fix(switch): drop duplicate tab refs that accumulate across switches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Symptom: switching between Spaces accumulates phantom rows in the side panel that all point at the same chrome tabId. Closing the tab leaves the ghost rows behind; clicking them throws "No tab with id"; closing and reopening the side panel clears them (because reconcile runs on mount). The race that seeds the duplicates: switchTo's empty-Space starter path calls chrome.tabs.create, which fires chrome.tabs.onCreated → registerTab in parallel with switchTo's own updateStore. Since the SW just wrote activeSpaceByWindow = target, registerTab picks the same Space and pushes the new tabId into root.items. switchTo's updateStore then pushes again with no guard, leaving two entries — repeated for every starter creation. Defenses: - switchTo: guard the starter push with a `.some(...)` check so a concurrent registerTab can't produce a duplicate. - pruneDeadTabs: dedupe folder.items by tabId on top of the existing dead-tab cull, so any duplicates that slipped through earlier (or through a race we haven't yet identified) self-heal on the next reconcile. - reconcile: also trigger when items have duplicate live tabIds (not only when refs are dead), so the dedup pass actually runs. - activateTab handler: when chrome.tabs.update throws "No tab with id", drop the stale ref before surfacing the error so the ghost row goes away on the next refresh instead of sitting there until the user reopens the panel. Test coverage: reconcile dedupes duplicate live tab refs without any tabs being dead. Co-Authored-By: Claude Opus 4.7 --- src/background/index.ts | 15 +++++++++++++-- src/background/reconcile.test.ts | 25 +++++++++++++++++++++++++ src/background/reconcile.ts | 11 +++++++++-- src/background/space-manager.ts | 28 +++++++++++++++++++++++++--- 4 files changed, 72 insertions(+), 7 deletions(-) diff --git a/src/background/index.ts b/src/background/index.ts index de847c4..3a6e6fa 100644 --- a/src/background/index.ts +++ b/src/background/index.ts @@ -228,8 +228,19 @@ async function handleMessage(msg: Message): Promise { await dropTab(msg.tabId) return } - case 'activateTab': - return chrome.tabs.update(msg.tabId, { active: true }) + case 'activateTab': { + try { + await chrome.tabs.update(msg.tabId, { active: true }) + return + } catch (e) { + // Stale ref: the tab is gone but folder.items still points at + // its id. Drop the ref so the next refresh removes the ghost + // row, then surface a generic "missing tab" error to the panel. + console.warn('[Spaces] activateTab failed; dropping ref', msg.tabId, e) + await dropTab(msg.tabId) + throw new Error('Tab is no longer available') + } + } case 'reconcile': return reconcileIfStale() case 'pinUrl': diff --git a/src/background/reconcile.test.ts b/src/background/reconcile.test.ts index 40f6faa..af90f3a 100644 --- a/src/background/reconcile.test.ts +++ b/src/background/reconcile.test.ts @@ -63,6 +63,31 @@ describe('reconcile', () => { expect(store.folders[space.rootFolderId].items.filter(it => it.kind === 'tab' && it.tabId === t1.id!)).toEqual([]) }) + it('dedupes duplicate tab refs within a folder even when no tabs are dead', async () => { + const space = await createSpace({ name: 'S', color: 'blue', windowId: 1 }) + const t1 = await chrome.tabs.create({ windowId: 1 }) + await registerTab(t1) + + // Simulate the race that this fix targets: the same live tabId + // appears twice in root.items. (In the field this comes from + // switchTo's starter push running concurrently with + // chrome.tabs.onCreated → registerTab.) + const dirty = await loadStore() + dirty.folders[space.rootFolderId].items.push({ kind: 'tab', tabId: t1.id! }) + dirty.folders[space.rootFolderId].items.push({ kind: 'tab', tabId: t1.id! }) + await chrome.storage.local.set({ spaceStore: dirty }) + + const result = await reconcile() + // No dead tabs to report, but we still wrote the dedup-ed store. + expect(result.dropped).toBe(0) + + const store = await loadStore() + const tabRefs = store.folders[space.rootFolderId].items.filter( + (it) => it.kind === 'tab' && it.tabId === t1.id!, + ) + expect(tabRefs).toHaveLength(1) + }) + describe('reconcileIfStale', () => { it('throttles reconcile calls within 30 seconds', async () => { vi.useFakeTimers() diff --git a/src/background/reconcile.ts b/src/background/reconcile.ts index cc91c45..c0f4613 100644 --- a/src/background/reconcile.ts +++ b/src/background/reconcile.ts @@ -4,6 +4,8 @@ import { pruneDeadTabs } from './space-manager' // Drop tab refs and TabRecord entries for tabs that no longer exist. // Called at SW startup; the store can drift if tabs were closed while the // SW was suspended (MV3 events fired during suspension are not replayed). +// Also kicks in when a folder's items hold duplicate entries for the +// same tabId — pruneDeadTabs dedupes those alongside the dead-tab cull. export async function reconcile(): Promise<{ dropped: number }> { const tabs = await chrome.tabs.query({}) const liveIds = new Set( @@ -11,15 +13,20 @@ export async function reconcile(): Promise<{ dropped: number }> { ) const before = await loadStore() const droppedIds = new Set() + let hasDuplicate = false for (const id of Object.keys(before.tabs).map(Number)) { if (!liveIds.has(id)) droppedIds.add(id) } for (const f of Object.values(before.folders)) { + const seen = new Set() for (const it of f.items) { - if (it.kind === 'tab' && !liveIds.has(it.tabId)) droppedIds.add(it.tabId) + if (it.kind !== 'tab') continue + if (!liveIds.has(it.tabId)) droppedIds.add(it.tabId) + else if (seen.has(it.tabId)) hasDuplicate = true + else seen.add(it.tabId) } } - if (droppedIds.size === 0) return { dropped: 0 } + if (droppedIds.size === 0 && !hasDuplicate) return { dropped: 0 } await updateStore((s) => { pruneDeadTabs(s, liveIds) }) diff --git a/src/background/space-manager.ts b/src/background/space-manager.ts index 0e6bb94..23689d8 100644 --- a/src/background/space-manager.ts +++ b/src/background/space-manager.ts @@ -598,12 +598,24 @@ export async function switchTo(spaceId: SpaceId, windowId?: number): Promise { s.tabs[created.id!] = { tabId: created.id!, windowId: winId } const sp = s.spaces[spaceId] if (!sp) return const root = s.folders[sp.rootFolderId] - if (root) root.items.push({ kind: 'tab', tabId: created.id! }) + if (!root) return + if ( + !root.items.some( + (it) => it.kind === 'tab' && it.tabId === created.id!, + ) + ) { + root.items.push({ kind: 'tab', tabId: created.id! }) + } }) } } catch { @@ -1091,15 +1103,25 @@ export async function reattachOrphanSpaces(): Promise { }) } -// Used by reconcile to drop stale tab refs. +// Used by reconcile to drop stale tab refs. Also deduplicates tab refs +// within each folder — race-condition fallouts (e.g. a previous +// registerTab vs switchTo starter push) can leave the same tabId in +// folder.items more than once, which surfaces as ghost rows in the side +// panel that keep replicating across switches. Keeping the dedup pass +// here means every reconcile sweep (panel mount + visibility change) +// self-heals existing damage even if the original racer is long gone. export function pruneDeadTabs(s: SpaceStore, liveTabIds: Set): boolean { let changed = false for (const f of Object.values(s.folders)) { const before = f.items.length + const seenTabs = new Set() f.items = f.items.filter((it) => { if (it.kind === 'folder') return true if (it.kind === 'live') return true - return liveTabIds.has(it.tabId) + if (!liveTabIds.has(it.tabId)) return false + if (seenTabs.has(it.tabId)) return false + seenTabs.add(it.tabId) + return true }) if (f.items.length !== before) changed = true if (f.live) {