From fbf120f8090c6eb472048121fdcb3253241613f8 Mon Sep 17 00:00:00 2001 From: liuxiaocs7 Date: Wed, 26 Aug 2026 16:31:04 +0800 Subject: [PATCH 1/5] fix(desktop): cache external-session catalog per selection so switching sources shows loaded rows instantly Switching import source (or toggling the archived filter / search) blanked the list and flashed the full-width 'reading external conversations' spinner on every switch, including returning to a source already loaded, because loadCatalog unconditionally reset the catalog and loading flag with no per-selection caching. Cache the last loaded CatalogState per (adapterId, includeArchived, search): a cache hit renders instantly and refreshes in the background instead of blanking; misses keep the spinner. The poll now observes rather than claims the request generation so it can never strand an in-flight load's spinner. Adds source-switching tests (harness now supports multiple adapters). Generated-by: Claude Code --- .../import-tasks-settings-page.test.ts | 230 +++++++++++++++++- .../settings/import-tasks-settings-page.tsx | 94 ++++++- 2 files changed, 313 insertions(+), 11 deletions(-) diff --git a/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts b/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts index 6b4cb62aeb..5e2608aac9 100644 --- a/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts +++ b/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts @@ -610,6 +610,207 @@ describe('ImportTasksSettingsPage durable import state', () => { }); }); +describe('ImportTasksSettingsPage source switching', () => { + const LOADING = /Reading external conversations/; + + it('shows the reading spinner the first time a source is opened', async () => { + let settle: ((r: CatalogResult) => void) | undefined; + const pending = new Promise((resolve) => { + settle = resolve; + }); + const harness = await renderPage({ + adapterIds: ['codex', 'claude-code'], + bySource: { + codex: [catalog(externalSession({ id: 's-codex', name: 'Codex conv' }))], + 'claude-code': [pending], + }, + }); + + assert.match(harness.container.textContent, /Codex conv/, 'codex loads on mount'); + assert.doesNotMatch(harness.container.textContent, LOADING, 'no spinner once codex is loaded'); + + const cc = segment(harness.container, 'claude-code'); + assert.ok(cc, 'claude-code segment renders'); + await act(async () => { + cc.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + + // First visit to claude-code: nothing cached, so the blank + spinner shows. + assert.match(harness.container.textContent, LOADING, 'first-time load shows the spinner'); + assert.doesNotMatch(harness.container.textContent, /Codex conv/, 'codex rows are cleared'); + + await act(async () => { + settle?.(catalog(externalSession({ id: 's-cc', name: 'CC conv' }))); + await Promise.resolve(); + await Promise.resolve(); + }); + + assert.match(harness.container.textContent, /CC conv/, 'claude-code rows arrive'); + assert.doesNotMatch(harness.container.textContent, LOADING, 'spinner clears when loaded'); + + await act(async () => harness.root.unmount()); + }); + + it('shows a previously-loaded source instantly with no spinner, then refreshes in place', async () => { + let settleRevisit: ((r: CatalogResult) => void) | undefined; + const revisitRefresh = new Promise((resolve) => { + settleRevisit = resolve; + }); + const harness = await renderPage({ + adapterIds: ['codex', 'claude-code'], + bySource: { + // [initial load, background refresh on revisit] + codex: [catalog(externalSession({ id: 's-codex', name: 'Codex conv' })), revisitRefresh], + 'claude-code': [catalog(externalSession({ id: 's-cc', name: 'CC conv' }))], + }, + }); + + assert.match(harness.container.textContent, /Codex conv/); + + await act(async () => { + segment(harness.container, 'claude-code')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /CC conv/, 'claude-code loaded'); + + // Revisit codex: cached rows appear immediately with no blanking spinner + // (the background refresh is still pending here). + await act(async () => { + segment(harness.container, 'codex')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /Codex conv/, 'cached codex rows shown instantly'); + assert.doesNotMatch(harness.container.textContent, LOADING, 'no spinner on revisit'); + + await act(async () => { + settleRevisit?.(catalog(externalSession({ id: 's-codex', name: 'Codex conv refreshed' }))); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /Codex conv refreshed/, 'background refresh lands'); + assert.doesNotMatch(harness.container.textContent, LOADING, 'still no spinner after refresh'); + + await act(async () => harness.root.unmount()); + }); + + it('does not let a stale background refresh overwrite a newer source selection', async () => { + let settleStaleCodexRefresh: ((r: CatalogResult) => void) | undefined; + const staleCodexRefresh = new Promise((resolve) => { + settleStaleCodexRefresh = resolve; + }); + const harness = await renderPage({ + adapterIds: ['codex', 'claude-code'], + bySource: { + codex: [catalog(externalSession({ id: 's-codex', name: 'Codex conv' })), staleCodexRefresh], + 'claude-code': [ + catalog(externalSession({ id: 's-cc', name: 'CC conv' })), + catalog(externalSession({ id: 's-cc', name: 'CC conv' })), + ], + }, + }); + + await act(async () => { + segment(harness.container, 'claude-code')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /CC conv/); + + // Revisit codex (cache hit → background refresh left pending)... + await act(async () => { + segment(harness.container, 'codex')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + // ...then switch straight back to claude-code before that refresh resolves. + await act(async () => { + segment(harness.container, 'claude-code')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /CC conv/, 'claude-code is the current source'); + + await act(async () => { + settleStaleCodexRefresh?.( + catalog(externalSession({ id: 's-codex', name: 'Stale codex conv' })), + ); + await Promise.resolve(); + await Promise.resolve(); + }); + + assert.match(harness.container.textContent, /CC conv/, 'claude-code rows remain'); + assert.doesNotMatch( + harness.container.textContent, + /Stale codex conv/, + 'the superseded codex refresh never lands under claude-code', + ); + + await act(async () => harness.root.unmount()); + }); + + it('drops an in-flight import poll after switching source, with no stuck spinner', async (context) => { + context.mock.timers.enable({ apis: ['setTimeout'] }); + let settleStalePoll: ((r: CatalogResult) => void) | undefined; + const stalePoll = new Promise((resolve) => { + settleStalePoll = resolve; + }); + const importing = externalSession({ + id: 's-codex', + name: 'Codex conv', + importState: { importedCount: 0, importedSessionIds: [], isImporting: true }, + }); + const harness = await renderPage({ + adapterIds: ['codex', 'claude-code'], + bySource: { + // [initial load with an import in flight, background poll read left pending] + codex: [{ sessions: [importing], nextCursor: null }, stalePoll], + 'claude-code': [catalog(externalSession({ id: 's-cc', name: 'CC conv' }))], + }, + }); + assert.match(harness.container.textContent, /Codex conv/); + + // The importing row schedules a poll; fire it so refreshLoadedCatalog is in + // flight against the pending read. + await act(async () => { + context.mock.timers.runAll(); + await Promise.resolve(); + await Promise.resolve(); + }); + + // Switch to claude-code while the codex poll is still in flight. + await act(async () => { + segment(harness.container, 'claude-code')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /CC conv/, 'claude-code loaded'); + assert.doesNotMatch(harness.container.textContent, LOADING, 'no stuck reading spinner'); + + // The stale codex poll resolves last — it must not overwrite claude-code. + await act(async () => { + settleStalePoll?.({ + sessions: [externalSession({ id: 's-codex', name: 'Codex conv refreshed' })], + nextCursor: null, + }); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /CC conv/, 'still showing claude-code'); + assert.doesNotMatch( + harness.container.textContent, + /Codex conv refreshed/, + 'stale poll result is dropped', + ); + assert.doesNotMatch(harness.container.textContent, LOADING, 'still no spinner'); + + await act(async () => harness.root.unmount()); + }); +}); + function externalSession( overrides: Partial = {}, ): DesktopExternalSessionCatalogItem { @@ -626,6 +827,10 @@ function externalSession( async function renderPage(options: { catalog?: CatalogResult; catalogs?: Array>; + // Multi-source tests: `listSources` reports these, and `list` draws per-source + // queues from `bySource` (keyed by adapterId) instead of the flat `catalogs`. + adapterIds?: string[]; + bySource?: Record>>; importResult?: | { ok: false; reason: 'commit_outcome_unknown' } | Promise<{ ok: false; reason: 'commit_outcome_unknown' }>; @@ -671,16 +876,29 @@ async function renderPage(options: { host?: DesktopRuntimeHostRef; }> = []; const catalogs = options.catalogs ?? [options.catalog ?? { sessions: [], nextCursor: null }]; + const sourceCounts: Record = {}; (window as unknown as { maka: unknown }).maka = { externalSessions: { listSources: async (host?: DesktopRuntimeHostRef) => { hostCalls.push({ operation: 'listSources', host }); - return { adapterIds: ['codex'] }; + return { adapterIds: options.adapterIds ?? ['codex'] }; }, - list: async (input: { includeArchived?: boolean }, host?: DesktopRuntimeHostRef) => { + list: async ( + input: { includeArchived?: boolean; adapterId: string }, + host?: DesktopRuntimeHostRef, + ) => { hostCalls.push({ operation: 'list', host }); listInputs.push({ includeArchived: input.includeArchived === true }); - const result = catalogs[Math.min(listCalls++, catalogs.length - 1)]; + listCalls++; + if (options.bySource) { + const queue = options.bySource[input.adapterId] ?? [{ sessions: [], nextCursor: null }]; + const index = Math.min(sourceCounts[input.adapterId] ?? 0, queue.length - 1); + sourceCounts[input.adapterId] = (sourceCounts[input.adapterId] ?? 0) + 1; + const perSource = queue[index]; + if (perSource instanceof Error) throw perSource; + return perSource; + } + const result = catalogs[Math.min(listCalls - 1, catalogs.length - 1)]; if (result instanceof Error) throw result; return result; }, @@ -729,3 +947,9 @@ function buttonWithText(container: HTMLElement, text: string): HTMLButtonElement (button) => button.textContent === text, ); } + +function segment(container: HTMLElement, value: string): HTMLButtonElement | undefined { + return Array.from( + container.querySelectorAll('button[role="radio"]'), + ).find((button) => button.getAttribute('data-value') === value); +} diff --git a/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx b/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx index c154954ef2..69e7e46ebb 100644 --- a/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx +++ b/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx @@ -48,6 +48,36 @@ type CatalogState = { const EMPTY_CATALOG: CatalogState = { sessions: [], nextCursor: null }; const EXTERNAL_SESSION_IMPORT_POLL_MS = 1_000; +/** + * A loaded catalog is cached per (source, archived filter, search) so switching + * back to a source or filter already viewed shows its rows instantly instead of + * blanking to the "reading external conversations" spinner on every switch. The + * key is the same tuple `catalogSelectionRef` tracks, so a cache hit and a + * selection match always agree on what "this catalog" is. + */ +function catalogSelectionKey(adapterId: string, includeArchived: boolean, search: string): string { + return `${adapterId} ${includeArchived ? '1' : '0'} ${search}`; +} + +// A few dozen selections is plenty to make back-and-forth switching instant +// without letting a long search session grow the cache without bound. Re-insert +// on write so the oldest untouched selection is the one evicted. +const CATALOG_CACHE_LIMIT = 24; + +function writeCatalogCache( + cache: Map, + key: string, + value: CatalogState, +): void { + cache.delete(key); + cache.set(key, value); + while (cache.size > CATALOG_CACHE_LIMIT) { + const oldest = cache.keys().next().value; + if (oldest === undefined) break; + cache.delete(oldest); + } +} + type CatalogWindow = CatalogState & { targetSource: DesktopExternalSessionCatalogItem | undefined; }; @@ -199,6 +229,16 @@ export function ImportTasksSettingsPage(props: { // source's rows under the new source's label. const requestGeneration = useRef(0); const recoveryGeneration = useRef(0); + // Last loaded catalog per selection key, so revisiting a source or filter is + // instant. Read/written only inside the async loaders; never rendered + // directly (the `catalog` state is what renders). + const catalogCacheRef = useRef(new Map()); + // Mirrors the committed `catalog` so a page append can extend the visible + // window without threading it through a stale render closure. + const catalogStateRef = useRef(catalog); + useEffect(() => { + catalogStateRef.current = catalog; + }, [catalog]); const catalogSelectionRef = useRef({ adapterId, includeArchived, search, generation: 0 }); if ( catalogSelectionRef.current.adapterId !== adapterId || @@ -231,6 +271,10 @@ export function ImportTasksSettingsPage(props: { setImportError(null); setAdapterIds([]); setAdapterId(null); + // A different host (or locale-driven remount) is a different catalog space; + // the cache keys carry neither, so drop everything rather than serve a + // previous host's rows. + catalogCacheRef.current.clear(); setCatalog(EMPTY_CATALOG); try { const result = await window.maka.externalSessions.listSources(host); @@ -252,8 +296,15 @@ export function ImportTasksSettingsPage(props: { async (sourceId: string, cursor?: string) => { const generation = ++requestGeneration.current; const append = cursor !== undefined; - if (append) setLoadingMore(true); - else { + const key = catalogSelectionKey(sourceId, includeArchived, search); + if (append) { + setLoadingMore(true); + } else if (catalogCacheRef.current.has(key)) { + // Already loaded this selection once. Show it immediately and refresh in + // the background instead of blanking to the spinner on every switch. + setCatalog(catalogCacheRef.current.get(key)!); + setImportRecovery(null); + } else { setCatalogLoading(true); setCatalog(EMPTY_CATALOG); setImportRecovery(null); @@ -268,10 +319,14 @@ export function ImportTasksSettingsPage(props: { ...(cursor === undefined ? {} : { cursor }), }, host); if (generation !== requestGeneration.current) return; - setCatalog((current) => ({ - sessions: append ? [...current.sessions, ...result.sessions] : result.sessions, + const next: CatalogState = { + sessions: append + ? [...catalogStateRef.current.sessions, ...result.sessions] + : result.sessions, nextCursor: result.nextCursor, - })); + }; + writeCatalogCache(catalogCacheRef.current, key, next); + setCatalog(next); } catch (error) { if (generation !== requestGeneration.current) return; setCatalogError(localizedShellErrorMessage(error, copy.loadFailedFallback, locale)); @@ -287,7 +342,10 @@ export function ImportTasksSettingsPage(props: { const refreshLoadedCatalog = useCallback( async (sourceId: string, loadedItemCount: number) => { - const generation = ++requestGeneration.current; + // Observe, don't preempt: the poll captures the current generation rather + // than claiming a new one, so it defers to any in-flight authoritative + // load/recovery instead of retiring that load's own catalogLoading reset. + const generation = requestGeneration.current; try { const result = await readCatalogWindow({ adapterId: sourceId, @@ -297,7 +355,18 @@ export function ImportTasksSettingsPage(props: { host, isCurrent: () => mountedRef.current && generation === requestGeneration.current, }); - if (result !== undefined) setCatalog(result); + if (result !== undefined) { + const refreshed: CatalogState = { + sessions: result.sessions, + nextCursor: result.nextCursor, + }; + writeCatalogCache( + catalogCacheRef.current, + catalogSelectionKey(sourceId, includeArchived, search), + refreshed, + ); + setCatalog(refreshed); + } } catch { // Catalog polling is best-effort. Keep the last authoritative page // visible and retry rather than replacing it with a transient error. @@ -384,10 +453,19 @@ export function ImportTasksSettingsPage(props: { // Recovery is the newest authoritative read for this exact catalog // selection. Retire an older poll/load-more response before publishing // it so that response cannot put pre-import state back on screen. + const recoveredState: CatalogState = { + sessions: result.sessions, + nextCursor: result.nextCursor, + }; + writeCatalogCache( + catalogCacheRef.current, + catalogSelectionKey(attempt.adapterId, attempt.includeArchived, attempt.text), + recoveredState, + ); requestGeneration.current += 1; setCatalogLoading(false); setLoadingMore(false); - setCatalog(result); + setCatalog(recoveredState); } setUncertainImports((current) => current.filter( From ffbdd5c859b0834464377c1d5d8c6722c5f5ea19 Mon Sep 17 00:00:00 2001 From: liuxiaocs7 Date: Sun, 30 Aug 2026 01:52:00 +0800 Subject: [PATCH 2/5] fix(desktop): clear stale catalog loading state on a cached-selection hit Restoring a cached catalog only reset the rows and the recovery banner, so a search or pagination request still in flight from the previous selection kept its loading flag. That older generation can never reach its own `finally` reset once a newer request supersedes it, and this hit's background refresh may not have landed yet, so the full-page spinner or a disabled Load More could strand over otherwise-complete cached rows indefinitely. Clear both `catalogLoading` and `loadingMore` when publishing a non-append cache hit so the instant, no-spinner contract holds regardless of a pending prior request or a slow background refresh. Adds two source-switching tests: a pending search returning to a cached term (reachable via the search box, the only control not disabled during a load) and a pending Load More when switching to a cached source. Both fail without the change (19/21) and pass with it (21/21). Addresses the P2 review comment on #3905. Generated-by: Claude Code --- .../import-tasks-settings-page.test.ts | 138 ++++++++++++++++++ .../settings/import-tasks-settings-page.tsx | 8 + 2 files changed, 146 insertions(+) diff --git a/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts b/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts index 5e2608aac9..ce109b9f80 100644 --- a/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts +++ b/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts @@ -809,6 +809,126 @@ describe('ImportTasksSettingsPage source switching', () => { await act(async () => harness.root.unmount()); }); + + it('clears the reading spinner when a pending search returns to a cached term', async (context) => { + context.mock.timers.enable({ apis: ['setTimeout'] }); + // The uncached search never resolves, so its spinner generation stays in + // flight; the return-to-'' refresh never resolves either, so only the + // cache-hit path — not a completed refresh — can retire the spinner. + const pendingSearch = new Promise(() => {}); + const refreshPending = new Promise(() => {}); + const harness = await renderPage({ + // [initial '' load, uncached 'zzz' search, background refresh on return to ''] + catalogs: [ + catalog(externalSession({ id: 's-codex', name: 'Codex conv' })), + pendingSearch, + refreshPending, + ], + }); + assert.match(harness.container.textContent, /Codex conv/, 'initial load shows rows'); + + // Type an uncached term (the source and archived controls disable during a + // load, but the search box does not, so this is the reachable way to leave a + // request pending). It blanks to the spinner and never resolves. + await act(async () => { + setSearchInput(harness.container, 'zzz'); + await Promise.resolve(); + }); + // Fire the 250ms debounce only after the effect above has registered it. + await act(async () => { + context.mock.timers.runAll(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, LOADING, 'uncached search shows the spinner'); + + // Return the search to the already-loaded empty term. The cached rows must + // come back with no spinner even though the older 'zzz' load is still + // pending and this hit's own refresh has not landed — the cache hit has to + // clear the stranded loading state itself. + await act(async () => { + setSearchInput(harness.container, ''); + await Promise.resolve(); + }); + await act(async () => { + context.mock.timers.runAll(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /Codex conv/, 'cached rows shown instantly'); + assert.doesNotMatch( + harness.container.textContent, + LOADING, + 'the stranded search spinner is cleared on the cache hit', + ); + + await act(async () => harness.root.unmount()); + }); + + it('clears a pending Load More lock when switching back to a cached source', async () => { + // Both revisit refreshes and the Load More append are left pending, so the + // only thing that can release the Load More lock is the cache-hit reset. + const codexRefreshPending = new Promise(() => {}); + const codexLoadMorePending = new Promise(() => {}); + const claudeCodeRefreshPending = new Promise(() => {}); + const harness = await renderPage({ + adapterIds: ['codex', 'claude-code'], + bySource: { + // [initial load (paged), revisit refresh, Load More append] + codex: [ + { sessions: [externalSession({ id: 's-codex', name: 'Codex conv' })], nextCursor: 'c1' }, + codexRefreshPending, + codexLoadMorePending, + ], + // [initial load (paged), revisit refresh] + 'claude-code': [ + { sessions: [externalSession({ id: 's-cc', name: 'CC conv' })], nextCursor: 'cc1' }, + claudeCodeRefreshPending, + ], + }, + }); + + // Load claude-code so it is cached with its own paged Load More. + await act(async () => { + segment(harness.container, 'claude-code')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /CC conv/, 'claude-code loaded'); + + // Revisit codex (cache hit; background refresh left pending), then start a + // Load More whose append never resolves so `loadingMore` stays set. + await act(async () => { + segment(harness.container, 'codex')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + const codexLoadMore = buttonWithText(harness.container, 'Load more'); + assert.ok(codexLoadMore, 'codex Load More renders'); + await act(async () => { + codexLoadMore.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + const busyLoadMore = Array.from( + harness.container.querySelectorAll('button'), + ).find((button) => button.textContent?.includes('Loading…')); + assert.ok(busyLoadMore, 'Load More shows the pending label while the append is in flight'); + + // Switch back to the cached claude-code before that append resolves. Its + // Load More must not inherit the stranded lock from codex's pending append. + await act(async () => { + segment(harness.container, 'claude-code')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /CC conv/, 'cached claude-code rows shown'); + const cachedLoadMore = buttonWithText(harness.container, 'Load more'); + assert.ok(cachedLoadMore, "claude-code's Load More is released, not stuck on 'Loading…'"); + assert.equal(cachedLoadMore.hasAttribute('disabled'), false, 'Load More is enabled again'); + + await act(async () => harness.root.unmount()); + }); }); function externalSession( @@ -948,6 +1068,24 @@ function buttonWithText(container: HTMLElement, text: string): HTMLButtonElement ); } +// Drives the search TextInput the way goal-dialog.test does: set the value and +// invoke the React onChange the renderer wired to it, so `searchDraft` updates +// without a real input event. The caller fires the debounce timer afterward. +function setSearchInput(container: HTMLElement, value: string): void { + const input = Array.from(container.querySelectorAll('input')).find( + (element) => element.type !== 'checkbox' && element.type !== 'radio', + ); + assert.ok(input, 'search input renders'); + input.value = value; + const propsKey = Object.keys(input).find((key) => key.startsWith('__reactProps$')); + assert.ok(propsKey, 'missing React props on the search input'); + const props = (input as unknown as Record)[propsKey] as { + onChange?: (event: { target: HTMLInputElement; defaultPrevented: boolean }) => void; + }; + assert.ok(props.onChange, 'missing search change handler'); + props.onChange({ target: input, defaultPrevented: false }); +} + function segment(container: HTMLElement, value: string): HTMLButtonElement | undefined { return Array.from( container.querySelectorAll('button[role="radio"]'), diff --git a/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx b/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx index 69e7e46ebb..3a8261e4b0 100644 --- a/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx +++ b/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx @@ -303,6 +303,14 @@ export function ImportTasksSettingsPage(props: { // Already loaded this selection once. Show it immediately and refresh in // the background instead of blanking to the spinner on every switch. setCatalog(catalogCacheRef.current.get(key)!); + // Clear any spinner/Load More lock left by a superseded request. A + // still-pending search or pagination load from the previous selection + // will never reach its own `finally` reset (its generation is now + // stale), so restoring cached rows without this would strand the + // full-page spinner or a disabled Load More over an otherwise complete + // view until this hit's background refresh happens to land. + setCatalogLoading(false); + setLoadingMore(false); setImportRecovery(null); } else { setCatalogLoading(true); From 729f9c6891d26f71612c0ded70f89277a970c4bb Mon Sep 17 00:00:00 2001 From: liuxiaocs7 Date: Sun, 30 Aug 2026 02:32:56 +0800 Subject: [PATCH 3/5] fix(desktop): re-read the whole loaded window when refreshing a cached selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Revisiting a cached selection restored its full CatalogState — which can be several pages deep after Load More — but the background refresh that followed requested only the first page (no cursor) and then replaced the cache and the view with that single page. So a multi-page source, revisited, flashed all its pages and then snapped back to page one once the refresh landed, silently dropping everything the user had paged in. Refresh a cache hit through the existing `readCatalogWindow`, seeded with the cached `sessions.length`, so it re-reads the entire loaded window instead of just page one. The uncached first-page load and the Load More append paths are unchanged. Adds a source-switching test: a two-page source, revisited, keeps both pages after the refresh (re-reading every page). It fails without the change (21/22) and passes with it (22/22). Addresses the P2 pagination-shrink review comment on #3905. Generated-by: Claude Code --- .../import-tasks-settings-page.test.ts | 64 +++++++++++++++++++ .../settings/import-tasks-settings-page.tsx | 59 +++++++++++------ 2 files changed, 105 insertions(+), 18 deletions(-) diff --git a/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts b/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts index ce109b9f80..2645b3153b 100644 --- a/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts +++ b/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts @@ -929,6 +929,70 @@ describe('ImportTasksSettingsPage source switching', () => { await act(async () => harness.root.unmount()); }); + + it('keeps every loaded page when a revisited multi-page source refreshes', async () => { + const harness = await renderPage({ + adapterIds: ['codex', 'claude-code'], + bySource: { + // [page 1, page 2 via Load More, refresh page 1, refresh page 2] + codex: [ + { + sessions: [externalSession({ id: 's-codex-1', name: 'Codex page one' })], + nextCursor: 'codex-cursor-1', + }, + { sessions: [externalSession({ id: 's-codex-2', name: 'Codex page two' })], nextCursor: null }, + { + sessions: [externalSession({ id: 's-codex-1', name: 'Codex page one' })], + nextCursor: 'codex-cursor-1', + }, + { sessions: [externalSession({ id: 's-codex-2', name: 'Codex page two' })], nextCursor: null }, + ], + 'claude-code': [catalog(externalSession({ id: 's-cc', name: 'CC conv' }))], + }, + }); + + // Page in the second page of codex via Load More. + const loadMore = buttonWithText(harness.container, 'Load more'); + assert.ok(loadMore, 'codex has a second page to load'); + await act(async () => { + loadMore.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /Codex page one/); + assert.match(harness.container.textContent, /Codex page two/, 'both pages are loaded'); + + // Switch away to claude-code, then back to codex. + await act(async () => { + segment(harness.container, 'claude-code')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /CC conv/); + + await act(async () => { + segment(harness.container, 'codex')!.click(); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + + // The cache hit shows both pages instantly; the background refresh must + // re-read the *whole* loaded window rather than shrink the list back to the + // first page. + assert.match(harness.container.textContent, /Codex page one/, 'first page kept'); + assert.match( + harness.container.textContent, + /Codex page two/, + 'the second page survives the background refresh', + ); + // codex page 1 + Load More + refresh page 1 + refresh page 2, plus the one + // claude-code load = 5. A first-page-only refresh would stop at 4. + assert.equal(harness.listCalls(), 5, 'the revisit refresh re-read every loaded page'); + + await act(async () => harness.root.unmount()); + }); }); function externalSession( diff --git a/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx b/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx index 3a8261e4b0..ef31f38cef 100644 --- a/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx +++ b/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx @@ -297,12 +297,13 @@ export function ImportTasksSettingsPage(props: { const generation = ++requestGeneration.current; const append = cursor !== undefined; const key = catalogSelectionKey(sourceId, includeArchived, search); + const cached = append ? undefined : catalogCacheRef.current.get(key); if (append) { setLoadingMore(true); - } else if (catalogCacheRef.current.has(key)) { + } else if (cached !== undefined) { // Already loaded this selection once. Show it immediately and refresh in // the background instead of blanking to the spinner on every switch. - setCatalog(catalogCacheRef.current.get(key)!); + setCatalog(cached); // Clear any spinner/Load More lock left by a superseded request. A // still-pending search or pagination load from the previous selection // will never reach its own `finally` reset (its generation is now @@ -320,21 +321,43 @@ export function ImportTasksSettingsPage(props: { setCatalogError(null); setImportError(null); try { - const result = await window.maka.externalSessions.list({ - adapterId: sourceId, - includeArchived, - ...(search ? { text: search } : {}), - ...(cursor === undefined ? {} : { cursor }), - }, host); - if (generation !== requestGeneration.current) return; - const next: CatalogState = { - sessions: append - ? [...catalogStateRef.current.sessions, ...result.sessions] - : result.sessions, - nextCursor: result.nextCursor, - }; - writeCatalogCache(catalogCacheRef.current, key, next); - setCatalog(next); + if (cached !== undefined) { + // Refresh a revisited selection by re-reading the *whole* loaded + // window, not just page one: the cache can be several pages deep from + // Load More, and a bare first-page read here would overwrite it and + // silently drop every page the user already paged in. + const refreshed = await readCatalogWindow({ + adapterId: sourceId, + includeArchived, + text: search, + minimumItemCount: cached.sessions.length, + host, + isCurrent: () => mountedRef.current && generation === requestGeneration.current, + }); + if (refreshed === undefined) return; + const next: CatalogState = { + sessions: refreshed.sessions, + nextCursor: refreshed.nextCursor, + }; + writeCatalogCache(catalogCacheRef.current, key, next); + setCatalog(next); + } else { + const result = await window.maka.externalSessions.list({ + adapterId: sourceId, + includeArchived, + ...(search ? { text: search } : {}), + ...(cursor === undefined ? {} : { cursor }), + }, host); + if (generation !== requestGeneration.current) return; + const next: CatalogState = { + sessions: append + ? [...catalogStateRef.current.sessions, ...result.sessions] + : result.sessions, + nextCursor: result.nextCursor, + }; + writeCatalogCache(catalogCacheRef.current, key, next); + setCatalog(next); + } } catch (error) { if (generation !== requestGeneration.current) return; setCatalogError(localizedShellErrorMessage(error, copy.loadFailedFallback, locale)); @@ -345,7 +368,7 @@ export function ImportTasksSettingsPage(props: { } } }, - [copy.loadFailedFallback, host, includeArchived, locale, search], + [copy.loadFailedFallback, host, includeArchived, locale, mountedRef, search], ); const refreshLoadedCatalog = useCallback( From 9a2d0d1568db58b70877b7b3150875d27c7f2af8 Mon Sep 17 00:00:00 2001 From: Xiao Liu Date: Sun, 30 Aug 2026 04:39:55 +0800 Subject: [PATCH 4/5] fix(desktop): stop two concurrent refreshes racing and cache recovered imports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review-driven follow-ups to the per-selection catalog cache: - Revisiting a selection whose cached rows still show an import in flight started a background readCatalogWindow *and* let the 1s import poll fire. Both ran under the same request generation, so a pre-import page read started by the revisit could land after a newer poll result and snap "Imported once" back to "Importing…". The revisit now defers to the poll — the single refresher for an importing selection — and issues no second read. - A successful unknown-outcome recovery only wrote the recovered rows back to the cache when its selection was still current. If the user had switched source or filter, the original selection kept its stale pre-import cache and, on return, showed an "Import" button instead of "Import again" until a later refresh — inviting a duplicate import. Recovery now always refreshes that selection's cache; only setCatalog and the generation bump stay gated on the selection still being current. Adds two source-switching tests (revisiting an importing source issues no second read; a recovered import is reflected in its cache after switching away). Both fail without their fix (23/24) and pass with it (24/24). Addresses the two P2 review comments on #3905. Generated-by: Claude Code --- .../import-tasks-settings-page.test.ts | 117 ++++++++++++++++++ .../settings/import-tasks-settings-page.tsx | 33 +++-- 2 files changed, 141 insertions(+), 9 deletions(-) diff --git a/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts b/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts index 2645b3153b..bbc6636415 100644 --- a/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts +++ b/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts @@ -993,6 +993,123 @@ describe('ImportTasksSettingsPage source switching', () => { await act(async () => harness.root.unmount()); }); + + it('does not start a second catalog read when revisiting a still-importing source', async (context) => { + context.mock.timers.enable({ apis: ['setTimeout'] }); + const importing = externalSession({ + id: 's-codex', + name: 'Codex conv', + importState: { importedCount: 0, importedSessionIds: [], isImporting: true }, + }); + const harness = await renderPage({ + adapterIds: ['codex', 'claude-code'], + bySource: { + codex: [{ sessions: [importing], nextCursor: null }], + 'claude-code': [catalog(externalSession({ id: 's-cc', name: 'CC conv' }))], + }, + }); + assert.match(harness.container.textContent, /Codex conv/); + assert.equal(harness.listCalls(), 1, 'codex loaded once on mount'); + + // Load claude-code (now cached), then return to the still-importing codex. + await act(async () => { + segment(harness.container, 'claude-code')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.equal(harness.listCalls(), 2, 'claude-code loaded'); + + await act(async () => { + segment(harness.container, 'codex')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + // The cache hit shows the importing rows again, but must NOT kick off its own + // background readCatalogWindow: the 1s import poll is the single refresher for + // an importing selection, and a second concurrent read shares the same request + // generation and can land a stale pre-import page on top of a newer poll + // result (the timer is deliberately left un-fired here). + assert.match(harness.container.textContent, /Codex conv/, 'cached codex rows shown'); + assert.equal( + harness.listCalls(), + 2, + 'revisiting an importing source starts no second catalog read', + ); + + await act(async () => harness.root.unmount()); + }); + + it('updates the cache for a recovered import even after switching away', async () => { + let settleImport: ((r: { ok: false; reason: 'commit_outcome_unknown' }) => void) | undefined; + const importResult = new Promise<{ ok: false; reason: 'commit_outcome_unknown' }>((resolve) => { + settleImport = resolve; + }); + const codexRecovered = externalSession({ + id: 's-codex', + name: 'Codex conv', + importState: { importedCount: 1, importedSessionIds: ['codex-task'], isImporting: false }, + }); + // The revisit's background refresh never resolves, so the returned view is the + // cache alone — proving the cache itself holds the recovered state. + const codexRevisitPending = new Promise(() => {}); + const harness = await renderPage({ + adapterIds: ['codex', 'claude-code'], + bySource: { + // [mount, recovery readCatalogWindow, revisit background refresh] + codex: [ + { sessions: [externalSession({ id: 's-codex', name: 'Codex conv' })], nextCursor: null }, + { sessions: [codexRecovered], nextCursor: null }, + codexRevisitPending, + ], + 'claude-code': [catalog(externalSession({ id: 's-cc', name: 'CC conv' }))], + }, + importResult, + }); + + // Start an import on codex, then switch to claude-code before the (unknown) + // outcome resolves. + const importButton = harness.container.querySelector( + 'button[aria-label="Import Codex conv"]', + ); + assert.ok(importButton); + await act(async () => importButton.click()); + + await act(async () => { + segment(harness.container, 'claude-code')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /CC conv/); + + // The import comes back unknown; recovery confirms it landed while codex is not + // the current selection. + await act(async () => { + settleImport?.({ ok: false, reason: 'commit_outcome_unknown' }); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /The imported task is available now/); + assert.match(harness.container.textContent, /CC conv/, 'the current view is untouched by recovery'); + + // Returning to codex must show the recovered "imported" state straight from + // the cache — not the stale pre-import row that would invite a duplicate + // import — even though the background refresh has not landed. + await act(async () => { + segment(harness.container, 'codex')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /Codex conv/); + assert.match( + harness.container.textContent, + /Imported once/, + 'the cache reflects the recovered import on return', + ); + + await act(async () => harness.root.unmount()); + }); }); function externalSession( diff --git a/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx b/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx index ef31f38cef..b1a8781bfc 100644 --- a/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx +++ b/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx @@ -322,6 +322,15 @@ export function ImportTasksSettingsPage(props: { setImportError(null); try { if (cached !== undefined) { + // While a revisited selection still shows an import in flight, the 1s + // import poll owns refreshing it. Starting our own readCatalogWindow + // here would run a second read under the *same* request generation as + // that poll, and a pre-import page read started here can land after a + // newer poll result and snap "Imported once" back to "Importing…". + // Leave the poll as the single refresher and keep the cached rows up. + if (cached.sessions.some((session) => session.importState.isImporting)) { + return; + } // Refresh a revisited selection by re-reading the *whole* loaded // window, not just page one: the cache can be several pages deep from // Load More, and a bare first-page read here would overwrite it and @@ -475,6 +484,21 @@ export function ImportTasksSettingsPage(props: { throw new Error('External Session source disappeared during import recovery'); } + const recoveredState: CatalogState = { + sessions: result.sessions, + nextCursor: result.nextCursor, + }; + // Always refresh the cache for the selection this attempt came from, even + // if the user has since switched source or filter. Recovery has confirmed + // the import landed; leaving the pre-import rows in the cache would greet + // the user with an "Import" button (not "Import again") when they return, + // inviting a duplicate import until a later background refresh corrects it. + writeCatalogCache( + catalogCacheRef.current, + catalogSelectionKey(attempt.adapterId, attempt.includeArchived, attempt.text), + recoveredState, + ); + const currentSelection = catalogSelectionRef.current; if ( currentSelection.adapterId === attempt.adapterId && @@ -484,15 +508,6 @@ export function ImportTasksSettingsPage(props: { // Recovery is the newest authoritative read for this exact catalog // selection. Retire an older poll/load-more response before publishing // it so that response cannot put pre-import state back on screen. - const recoveredState: CatalogState = { - sessions: result.sessions, - nextCursor: result.nextCursor, - }; - writeCatalogCache( - catalogCacheRef.current, - catalogSelectionKey(attempt.adapterId, attempt.includeArchived, attempt.text), - recoveredState, - ); requestGeneration.current += 1; setCatalogLoading(false); setLoadingMore(false); From abdc7038e84510f01f5ba772c1320f19ebd0aa4c Mon Sep 17 00:00:00 2001 From: Xiao Liu Date: Sun, 30 Aug 2026 05:24:34 +0800 Subject: [PATCH 5/5] fix(desktop): publish a recovered import when its selection is revisited MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recovery only pushed the recovered rows to the screen when the current catalog selection still had the exact generation captured at import time. Navigating away and back to the same source/filter/search (A→B→A) lands on the same selection with a newer generation, so the check refused to publish: the cache was refreshed but the visible rows stayed pre-import, and once activeImport cleared the row showed a clickable "Import" even though the success banner was up — persisting until a slow background refresh caught up. Match the current selection by its actual tuple (adapterId + includeArchived + search) instead of the import-time generation, so recovery publishes to the view — and bumps requestGeneration to retire any in-flight revisit/poll read — whenever the user is looking at that selection. The now-unused catalogSelectionGeneration field is dropped from ImportAttempt. Adds an A→B→A-then-recovery test; it fails on the generation check (24/25) and passes on the tuple match (25/25). Addresses the follow-up P2 review comment on #3905. Generated-by: Claude Code --- .../import-tasks-settings-page.test.ts | 69 +++++++++++++++++++ .../settings/import-tasks-settings-page.tsx | 14 ++-- 2 files changed, 78 insertions(+), 5 deletions(-) diff --git a/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts b/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts index bbc6636415..b69c9bf3bb 100644 --- a/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts +++ b/apps/desktop/src/main/__tests__/import-tasks-settings-page.test.ts @@ -1110,6 +1110,75 @@ describe('ImportTasksSettingsPage source switching', () => { await act(async () => harness.root.unmount()); }); + + it('publishes a recovered import to the current view after leaving and returning to its source', async () => { + let settleImport: ((r: { ok: false; reason: 'commit_outcome_unknown' }) => void) | undefined; + const importResult = new Promise<{ ok: false; reason: 'commit_outcome_unknown' }>((resolve) => { + settleImport = resolve; + }); + const codexRecovered = externalSession({ + id: 's-codex', + name: 'Codex conv', + importState: { importedCount: 1, importedSessionIds: ['codex-task'], isImporting: false }, + }); + // The revisit's own background refresh never resolves, so recovery is the only + // thing that can update the screen — proving recovery publishes rather than + // leaving the view to wait on a slow refresh. + const codexRevisitPending = new Promise(() => {}); + const harness = await renderPage({ + adapterIds: ['codex', 'claude-code'], + bySource: { + // [mount, revisit background refresh (pending), recovery readCatalogWindow] + codex: [ + { sessions: [externalSession({ id: 's-codex', name: 'Codex conv' })], nextCursor: null }, + codexRevisitPending, + { sessions: [codexRecovered], nextCursor: null }, + ], + 'claude-code': [catalog(externalSession({ id: 's-cc', name: 'CC conv' }))], + }, + importResult, + }); + + const importButton = harness.container.querySelector( + 'button[aria-label="Import Codex conv"]', + ); + assert.ok(importButton); + await act(async () => importButton.click()); + + // Switch to claude-code, then back to codex — all before the import resolves. + await act(async () => { + segment(harness.container, 'claude-code')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + await act(async () => { + segment(harness.container, 'codex')!.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /Codex conv/, 'back on codex, pre-import rows shown'); + assert.doesNotMatch(harness.container.textContent, /Imported once/, 'not recovered yet'); + + // Recovery lands. codex is the current selection again, but at a *newer* + // generation than when the import started, so a generation check would refuse + // to publish. Matching the selection tuple, recovery must still reach the + // screen — not just the cache — even though the revisit refresh is pending. + await act(async () => { + settleImport?.({ ok: false, reason: 'commit_outcome_unknown' }); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.match(harness.container.textContent, /The imported task is available now/); + assert.match( + harness.container.textContent, + /Imported once/, + 'recovery publishes to the returned-to view, not only the cache', + ); + + await act(async () => harness.root.unmount()); + }); }); function externalSession( diff --git a/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx b/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx index b1a8781bfc..bd7740f55d 100644 --- a/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx +++ b/apps/desktop/src/renderer/settings/import-tasks-settings-page.tsx @@ -144,7 +144,6 @@ type ImportAttempt = { importedCountBefore: number; latestImportedSessionIdBefore: string | undefined; loadedCatalogItemCountBefore: number; - catalogSelectionGeneration: number; }; type ImportRecovery = @@ -499,15 +498,21 @@ export function ImportTasksSettingsPage(props: { recoveredState, ); + // Publish to screen whenever the user is currently viewing the same + // selection tuple this attempt came from. Match the tuple, not the + // generation captured at import time: navigating A→B→A lands back on the + // same selection with a *newer* generation, and a generation check would + // refuse to publish there — leaving a clickable "Import" on a row that + // already imported until a slow background refresh happened to catch up. const currentSelection = catalogSelectionRef.current; if ( currentSelection.adapterId === attempt.adapterId && currentSelection.includeArchived === attempt.includeArchived && - currentSelection.generation === attempt.catalogSelectionGeneration + currentSelection.search === attempt.text ) { // Recovery is the newest authoritative read for this exact catalog - // selection. Retire an older poll/load-more response before publishing - // it so that response cannot put pre-import state back on screen. + // selection. Retire an older poll/load-more/revisit response before + // publishing it so that response cannot put pre-import state back up. requestGeneration.current += 1; setCatalogLoading(false); setLoadingMore(false); @@ -565,7 +570,6 @@ export function ImportTasksSettingsPage(props: { importedCountBefore: session.importState.importedCount, latestImportedSessionIdBefore: session.importState.importedSessionIds[0], loadedCatalogItemCountBefore: catalog.sessions.length, - catalogSelectionGeneration: catalogSelectionRef.current.generation, }; setActiveImport(attempt); setImportError(null);