From fbba9748907b0b87f4d46ec8ca835f1ef4a5003b Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:39 -0500 Subject: [PATCH 01/26] fix(organize): a slow cross-drive move runs to its end instead of being reported failed --- src/main/ipc/files/batchLockManager.js | 10 +- src/main/ipc/files/batchOrganizeHandler.js | 76 +++++++-- src/shared/atomicFileOperations.js | 30 +++- ...ileOperations.crossDeviceCollision.test.js | 115 ++++++++++++++ test/batchOrganizeHandler.test.js | 150 ++++++++++++++++++ 5 files changed, 359 insertions(+), 22 deletions(-) diff --git a/src/main/ipc/files/batchLockManager.js b/src/main/ipc/files/batchLockManager.js index 365d1fc..fcba52d 100644 --- a/src/main/ipc/files/batchLockManager.js +++ b/src/main/ipc/files/batchLockManager.js @@ -20,10 +20,12 @@ if (typeof createLogger !== 'function' && logger?.setContext) { /** @type {{ batchId: string, acquiredAt: number } | null} */ let batchOperationLock = null; // Budget for the rollback tail that runs AFTER a batch trips -// MAX_TOTAL_BATCH_TIME: the in-flight move must finish (FILE_COPY) and every -// completed op must be restored. Holders renew the lock per op, so this TTL -// is only the crash backstop — it must still exceed the worst-case -// legitimate hold, or a second batch could start mid-rollback. +// MAX_TOTAL_BATCH_TIME: every completed op must be restored. Holders renew +// the lock per op and on an interval while a move is in flight (a move has +// no time limit of its own; a cross-volume copy of a large file can outlast +// any fixed one), so this TTL is only the crash backstop — it must still +// exceed the worst-case hold between two renewals, or a second batch could +// start mid-rollback. FILE_COPY stays in the sum as slack. const ROLLBACK_ALLOWANCE_MS = 2 * 60 * 1000; const BATCH_LOCK_TIMEOUT = PROCESSING_LIMITS.MAX_TOTAL_BATCH_TIME + TIMEOUTS.FILE_COPY + ROLLBACK_ALLOWANCE_MS; diff --git a/src/main/ipc/files/batchOrganizeHandler.js b/src/main/ipc/files/batchOrganizeHandler.js index 045331b..881f024 100644 --- a/src/main/ipc/files/batchOrganizeHandler.js +++ b/src/main/ipc/files/batchOrganizeHandler.js @@ -89,6 +89,10 @@ const VERIFY_MAX_ATTEMPTS = RETRY?.FILE_OPERATION?.maxAttempts ?? RETRY?.MAX_ATT const VERIFY_BACKOFF_STEP_MS = RETRY?.FILE_OPERATION?.initialDelay ?? TIMEOUTS?.DELAY_TINY ?? MOVE_LOCK_BACKOFF_STEP_MS; const VERIFY_MAX_DELAY_MS = RETRY?.FILE_OPERATION?.maxDelay ?? 5000; +// How often a move still in flight renews the batch lock. A cross-volume move +// of a large file (copy plus a checksum of both sides) has no upper bound, so +// the lease is kept fresh for as long as the move runs. +const MOVE_LOCK_RENEW_INTERVAL_MS = 30 * 1000; const MAX_RESULTS_PER_CHUNK = 100; // Max results per IPC message chunk const YIELD_EVERY_N_OPS = 10; // Yield to event loop every N operations @@ -762,16 +766,27 @@ async function handleBatchOrganize(params) { // Handle file move with collision handling // TOCTOU fix: removed verifySourceFile pre-check, handle ENOENT from move directly - // Use timeout for file operations to prevent hangs - const moveWithTimeout = () => - withTimeout( - performFileMove(op, log, computeFileChecksum, abortController.signal), - TIMEOUTS.FILE_COPY, - `File move ${path.basename(op.source)}` + // The move is awaited to its end, never raced against a timer: a + // timer cannot stop a copy, so the row would say "failed" while + // the file went on to move with no undo entry or path update. + // The batch lock is renewed while it runs, and the abort signal + // is what stops a cross-volume move before it publishes. + const moveToCompletion = async () => { + const lockRenewal = setInterval( + () => renewBatchLock(batchId), + MOVE_LOCK_RENEW_INTERVAL_MS ); + lockRenewal.unref?.(); + try { + return await performFileMove(op, log, computeFileChecksum, abortController.signal); + } finally { + clearInterval(lockRenewal); + } + }; + /** @type {any} */ let moveResult; try { - moveResult = await moveWithTimeout(); + moveResult = await moveToCompletion(); } catch (firstMoveError) { let moveError = firstMoveError; // A rename ENOENT is either a missing source or a destination @@ -793,7 +808,7 @@ async function handleBatchOrganize(params) { } try { await fs.mkdir(destDir, { recursive: true }); - moveResult = await moveWithTimeout(); + moveResult = await moveToCompletion(); moveError = null; } catch (retryError) { moveError = retryError; @@ -829,8 +844,9 @@ async function handleBatchOrganize(params) { } if (moveResult && moveResult.cancelled) { - // performFileMove bailed before any rename (user abort landed - // between the gate above and the first rename attempt). + // performFileMove bailed with the source untouched: the abort + // landed between the gate above and the first rename attempt, + // or during a cross-volume copy that was then never published. recordCancelledOp(op); return; } @@ -1651,9 +1667,11 @@ async function performFileMove(op, log, checksumFn, abortSignal = null) { // runs; the no-replace publish then refuses and the walk moves on to // the next suffix, exactly like a same-device EEXIST. try { - await performCrossDeviceMove(op.source, uniqueDestination, log, checksumFn); + await performCrossDeviceMove(op.source, uniqueDestination, log, checksumFn, abortSignal); operationComplete = true; } catch (crossDeviceError) { + // Stopped by the abort before anything was published: nothing moved. + if (isMoveAbortedError(crossDeviceError, abortSignal)) return { cancelled: true }; if (!isNameTakenError(crossDeviceError)) throw crossDeviceError; advanceToNextCandidate(); } @@ -1665,12 +1683,35 @@ async function performFileMove(op, log, checksumFn, abortSignal = null) { // UUID fallback if numeric exhausted if (!operationComplete) { - uniqueDestination = await performUUIDFallback(op, baseName, ext, log, checksumFn); + try { + uniqueDestination = await performUUIDFallback( + op, + baseName, + ext, + log, + checksumFn, + abortSignal + ); + } catch (fallbackError) { + if (isMoveAbortedError(fallbackError, abortSignal)) return { cancelled: true }; + throw fallbackError; + } } return { destination: uniqueDestination }; } +/** + * Whether a cross-device move stopped because the batch's signal aborted. It + * rejects that way only with the source left in place, so nothing moved. + * @param {any} error + * @param {AbortSignal|null} [abortSignal] + * @returns {boolean} + */ +function isMoveAbortedError(error, abortSignal) { + return Boolean(abortSignal?.aborted) && error?.name === 'AbortError'; +} + /** * Whether a failed move means "that name is taken": a raw EEXIST, or the * FileSystemError(FILE_EXISTS) crossDeviceMove wraps its publish-time EEXIST in. @@ -1729,11 +1770,13 @@ async function renameWithRetry(source, destination, log) { * @param {string} destination * @param {LoggerLike} log * @param {typeof computeFileChecksum} checksumFn + * @param {AbortSignal|null} [abortSignal] - Stops the move before it publishes */ -async function performCrossDeviceMove(source, destination, log, checksumFn) { +async function performCrossDeviceMove(source, destination, log, checksumFn, abortSignal = null) { await crossDeviceMove(source, destination, { verify: true, - checksumFn + checksumFn, + signal: abortSignal }); } @@ -1744,8 +1787,9 @@ async function performCrossDeviceMove(source, destination, log, checksumFn) { * @param {string} ext * @param {LoggerLike} log * @param {typeof computeFileChecksum} checksumFn + * @param {AbortSignal|null} [abortSignal] */ -async function performUUIDFallback(op, baseName, ext, log, checksumFn) { +async function performUUIDFallback(op, baseName, ext, log, checksumFn, abortSignal = null) { log.warn(`[FILE-OPS] Falling back to UUID for ${path.basename(op.source)}`); const maxPathLength = getEffectiveMaxPathLength(); @@ -1771,7 +1815,7 @@ async function performUUIDFallback(op, baseName, ext, log, checksumFn) { if (error.code === 'EEXIST') continue; if (error.code === 'EXDEV') { try { - await performCrossDeviceMove(op.source, uniqueDestination, log, checksumFn); + await performCrossDeviceMove(op.source, uniqueDestination, log, checksumFn, abortSignal); return uniqueDestination; } catch (crossDeviceError) { if (isNameTakenError(crossDeviceError)) continue; diff --git a/src/shared/atomicFileOperations.js b/src/shared/atomicFileOperations.js index 2cc3919..b979512 100644 --- a/src/shared/atomicFileOperations.js +++ b/src/shared/atomicFileOperations.js @@ -1800,13 +1800,23 @@ AtomicFileOperations.prototype.safeWriteFile = function safeWriteFile(filePath, * @param {Object} [options={}] - Options * @param {boolean} [options.verify=true] - Verify copy by comparing file sizes * @param {Function} [options.checksumFn] - Optional checksum function for additional verification + * @param {AbortSignal|null} [options.signal] - Caller's abort signal. A copy or + * checksum already running is not interrupted, but once the signal aborts + * nothing more is published or deleted: the source stays, the staging copy + * is removed and the call rejects with an AbortError. * @returns {Promise} Resolves when move is complete * @throws {Error} If copy fails or verification fails */ async function crossDeviceMove(source, dest, options = {}) { const normalizedSource = resolveFsPath(source); const normalizedDest = resolveFsPath(dest); - const { verify = true, checksumFn } = options; + const { verify = true, checksumFn, signal = null } = options; + const throwIfMoveAborted = () => { + if (!signal?.aborted) return; + const abortError = new Error(`Move aborted: ${normalizedSource}`); + abortError.name = 'AbortError'; + throw abortError; + }; // Stage the copy under a temp name IN THE DESTINATION DIRECTORY, then // rename into place. Copying straight to the canonical destination meant a @@ -1832,8 +1842,10 @@ async function crossDeviceMove(source, dest, options = {}) { await fs.mkdir(destinationDir, { recursive: true }); try { + throwIfMoveAborted(); // Copy to the staging name — never the canonical destination. await fs.copyFile(normalizedSource, stagingPath, fsConstants.COPYFILE_EXCL); + throwIfMoveAborted(); // Verify the staged copy before it can be published under the real name. if (verify) { @@ -1869,6 +1881,7 @@ async function crossDeviceMove(source, dest, options = {}) { // this rename is atomic. renameNoReplace preserves the no-overwrite // contract the old direct COPYFILE_EXCL copy provided — an occupied // destination surfaces as EEXIST instead of clobbering the occupant. + throwIfMoveAborted(); await renameNoReplace(stagingPath, normalizedDest); } catch (stagingError) { // Nothing was ever written at the canonical destination — only the @@ -1886,6 +1899,19 @@ async function crossDeviceMove(source, dest, options = {}) { throw stagingError; } + // An abort that landed with the publish: the source is still whole, so + // withdraw the copy just published instead of finishing a move the caller + // has already given up on (and may already have reported as not done). + if (signal?.aborted) { + await fs.unlink(normalizedDest).catch((withdrawError) => { + logger.error('[ATOMIC-OPS] Failed to withdraw published copy after abort', { + destination: normalizedDest, + error: withdrawError.message + }); + }); + throwIfMoveAborted(); + } + // Delete the source file — LAST, only after the verified copy is live at // the canonical destination. try { @@ -1926,7 +1952,7 @@ async function crossDeviceMove(source, dest, options = {}) { destination: normalizedDest }); } catch (error) { - if (error.isFileSystemError) { + if (error.isFileSystemError || error?.name === 'AbortError') { throw error; } // EEXIST can only come from the publish onto the destination name, so diff --git a/test/atomicFileOperations.crossDeviceCollision.test.js b/test/atomicFileOperations.crossDeviceCollision.test.js index df0f107..79112fe 100644 --- a/test/atomicFileOperations.crossDeviceCollision.test.js +++ b/test/atomicFileOperations.crossDeviceCollision.test.js @@ -97,3 +97,118 @@ describe('cross-device move losing the name to another writer', () => { expect(await fsp.readFile(source, 'utf8')).toBe('MINE'); }); }); + +/** + * A caller that gives up on a cross-device move (cancel, strict-mode + * rollback) aborts its signal. The copy itself cannot be interrupted, but + * nothing may be published or deleted afterwards: the source stays, the + * destination name stays free and the staging copy is removed. + */ +describe('cross-device move aborted by its caller', () => { + /** @type {string} */ + let root; + /** @type {string} */ + let source; + /** @type {string} */ + let destination; + + beforeEach(async () => { + root = await fsp.mkdtemp(path.join(realOs.tmpdir(), 'ss-exdev-abort-')); + source = path.join(root, 'vol1', 'video.mp4'); + destination = path.join(root, 'vol2', 'video.mp4'); + await fsp.mkdir(path.dirname(source), { recursive: true }); + await fsp.mkdir(path.dirname(destination), { recursive: true }); + await fsp.writeFile(source, 'MINE'); + }); + + afterEach(async () => { + jest.restoreAllMocks(); + await fsp.rm(root, { recursive: true, force: true }); + }); + + async function expectNothingMoved() { + expect(await fsp.readFile(source, 'utf8')).toBe('MINE'); + expect(await fsp.readdir(path.dirname(destination))).toEqual([]); + } + + test('abort during the copy: nothing is published and the source stays', async () => { + const controller = new AbortController(); + const realCopy = fsp.copyFile.bind(fsp); + jest.spyOn(fsp, 'copyFile').mockImplementation(async (from, to, mode) => { + await realCopy(from, to, mode); + controller.abort('user-cancel-organize'); + }); + const checksumFn = jest.fn(async () => 'same'); + + const error = await crossDeviceMove(source, destination, { + checksumFn, + signal: controller.signal + }).then( + () => null, + (e) => e + ); + + expect(error).toMatchObject({ name: 'AbortError' }); + // The verification pass (two full-file hashes) is skipped too. + expect(checksumFn).not.toHaveBeenCalled(); + await expectNothingMoved(); + }); + + test('abort during verification: nothing is published and the source stays', async () => { + const controller = new AbortController(); + const checksumFn = jest.fn(async () => { + controller.abort(); + return 'same'; + }); + + const error = await crossDeviceMove(source, destination, { + checksumFn, + signal: controller.signal + }).then( + () => null, + (e) => e + ); + + expect(error).toMatchObject({ name: 'AbortError' }); + await expectNothingMoved(); + }); + + test('abort landing with the publish: the copy is withdrawn and the source stays', async () => { + const controller = new AbortController(); + const realRename = fsp.rename.bind(fsp); + jest.spyOn(fsp, 'rename').mockImplementation(async (from, to) => { + await realRename(from, to); + controller.abort(); + }); + + const error = await crossDeviceMove(source, destination, { signal: controller.signal }).then( + () => null, + (e) => e + ); + + expect(error).toMatchObject({ name: 'AbortError' }); + await expectNothingMoved(); + }); + + test('a signal that never aborts changes nothing', async () => { + await crossDeviceMove(source, destination, { signal: new AbortController().signal }); + + expect(await fsp.readFile(destination, 'utf8')).toBe('MINE'); + await expect(fsp.access(source)).rejects.toMatchObject({ code: 'ENOENT' }); + }); + + test('a signal already aborted copies nothing', async () => { + const controller = new AbortController(); + controller.abort(); + const copySpy = jest.spyOn(fsp, 'copyFile'); + + const error = await crossDeviceMove(source, destination, { signal: controller.signal }).then( + () => null, + (e) => e + ); + + expect(error).toMatchObject({ name: 'AbortError' }); + expect(copySpy).not.toHaveBeenCalled(); + await expectNothingMoved(); + }); +}); diff --git a/test/batchOrganizeHandler.test.js b/test/batchOrganizeHandler.test.js index a55e8a1..c25fb40 100644 --- a/test/batchOrganizeHandler.test.js +++ b/test/batchOrganizeHandler.test.js @@ -1413,6 +1413,156 @@ describe('Batch Organize Handler', () => { expect(batchLockManager.renewBatchLock).toHaveBeenCalledWith(result.batchId); }); + // A cross-volume move copies and checksums the whole file, which takes + // longer than any fixed limit for a large one. The handler used to race the + // move against a 30 s timer that cancelled nothing: the row came back + // failed while the copy went on to publish and delete the source. + describe('a move that outlasts 30 s', () => { + const { crossDeviceMove } = require('../src/shared/atomicFileOperations'); + const promiseUtils = require('../src/shared/promiseUtils'); + const flush = () => new Promise((resolve) => setImmediate(resolve)); + const exdev = () => Object.assign(new Error('EXDEV: cross-device link'), { code: 'EXDEV' }); + const abortError = () => Object.assign(new Error('Move aborted'), { name: 'AbortError' }); + + /** A crossDeviceMove that copies until `finishCopy()` and honors its signal like the real one. */ + function stubSlowCrossDeviceMove() { + /** @type {string[]} */ + const moved = []; + /** @type {() => void} */ + let finishCopy = () => {}; + const copying = new Promise((resolve) => { + finishCopy = () => resolve(undefined); + }); + crossDeviceMove.mockImplementation(async (source, _dest, options) => { + await copying; + if (options?.signal?.aborted) throw abortError(); + moved.push(source); + }); + return { moved, finishCopy }; + } + + async function waitForCopyToStart() { + for (let i = 0; i < 50 && crossDeviceMove.mock.calls.length === 0; i++) await flush(); + expect(crossDeviceMove).toHaveBeenCalledTimes(1); + } + + beforeEach(() => { + fs.rename.mockImplementation(async (source) => { + if (source === '/src/doc1.pdf') throw exdev(); + }); + }); + + afterEach(() => { + jest.useRealTimers(); + fs.rename.mockReset().mockResolvedValue(); + crossDeviceMove.mockReset(); + promiseUtils.withTimeout.mockReset().mockImplementation((promise) => promise); + }); + + test('is reported as moved, with its undo entry, and keeps the batch lock renewed meanwhile', async () => { + jest.useFakeTimers({ doNotFake: ['setImmediate', 'nextTick', 'queueMicrotask'] }); + // The real race, so the 30 s limit is in force wherever the handler still uses it. + promiseUtils.withTimeout.mockImplementation( + jest.requireActual('../src/shared/promiseUtils').withTimeout + ); + const svc = makeStableSvc(); + const { moved, finishCopy } = stubSlowCrossDeviceMove(); + + const pending = handleBatchOrganize({ + ...params, + getServiceIntegration: jest.fn(() => svc), + operation: { + operations: [{ source: '/src/doc1.pdf', destination: '/dest/doc1.pdf', type: 'move' }] + } + }); + await waitForCopyToStart(); + + await jest.advanceTimersByTimeAsync(95 * 1000); + await flush(); + const renewalsDuringCopy = batchLockManager.renewBatchLock.mock.calls.length; + finishCopy(); + const result = await pending; + + expect(moved).toEqual(['/src/doc1.pdf']); + expect(result.results).toEqual([ + expect.objectContaining({ + source: '/src/doc1.pdf', + destination: '/dest/doc1.pdf', + success: true + }) + ]); + expect(result.failCount).toBe(0); + expect(result.completedOperations).toBe(1); + expect(svc.undoRedo.recordAction).toHaveBeenCalledTimes(1); + expect(svc.undoRedo.recordAction.mock.calls[0][1].operations).toEqual([ + { type: 'move', originalPath: '/src/doc1.pdf', newPath: '/dest/doc1.pdf' } + ]); + expect(svc.processingState.markOrganizeOpError).not.toHaveBeenCalled(); + expect(renewalsDuringCopy).toBeGreaterThanOrEqual(3); + expect(batchLockManager.renewBatchLock).toHaveBeenCalledWith(result.batchId); + // The renewal stops with the move. + expect(jest.getTimerCount()).toBe(0); + }); + + test('strict mode: a move still copying when another file fails is stopped, not finished after the rollback', async () => { + fs.rename.mockImplementation(async (source) => { + if (source === '/src/doc1.pdf') throw exdev(); + throw Object.assign(new Error('EINVAL: invalid argument'), { code: 'EINVAL' }); + }); + const { moved, finishCopy } = stubSlowCrossDeviceMove(); + + const pending = handleBatchOrganize({ + ...params, + operation: { ...params.operation, strict: true } + }); + await waitForCopyToStart(); + for (let i = 0; i < 20; i++) await flush(); + finishCopy(); + const result = await pending; + + expect(moved).toEqual([]); + expect(crossDeviceMove.mock.calls[0][2].signal.aborted).toBe(true); + expect(result.success).toBe(false); + expect(result.results.filter((row) => row.success)).toEqual([]); + expect(result.results).toEqual( + expect.arrayContaining([ + expect.objectContaining({ source: '/src/doc2.pdf', strictModeTriggered: true }) + ]) + ); + // Nothing moved, so there is nothing to roll back or undo. + const { executeRollback } = require('../src/main/ipc/files/batchRollback'); + expect(executeRollback).not.toHaveBeenCalled(); + }); + + test('user cancel during the copy leaves the file where it was and reports it cancelled', async () => { + const controller = new AbortController(); + const svc = makeStableSvc(); + const { moved, finishCopy } = stubSlowCrossDeviceMove(); + + const pending = handleBatchOrganize({ + ...params, + abortSignal: controller.signal, + getServiceIntegration: jest.fn(() => svc), + operation: { + operations: [{ source: '/src/doc1.pdf', destination: '/dest/doc1.pdf', type: 'move' }] + } + }); + await waitForCopyToStart(); + controller.abort('user-cancel-organize'); + finishCopy(); + const result = await pending; + + expect(moved).toEqual([]); + expect(result.cancelled).toBe(true); + expect(result.cancelledCount).toBe(1); + expect(result.failCount).toBe(0); + expect(result.results).toEqual([ + expect.objectContaining({ source: '/src/doc1.pdf', success: false, cancelled: true }) + ]); + expect(svc.processingState.markOrganizeOpError).not.toHaveBeenCalled(); + }); + }); + // Overlap detection folds case wherever the default filesystem does. On // darwin (APFS/HFS+ are case-insensitive by default) two destinations that // differ only by case are the SAME file: run in parallel, both From ea650c52397560edd351d32071d96c0984697b9f Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:39 -0500 Subject: [PATCH 02/26] fix(undo): an entry whose file is gone leaves history and errors keep their code --- src/main/services/UndoRedoService.js | 162 ++++++++++++++++-- src/renderer/components/UndoRedoSystem.jsx | 11 ++ test/UndoRedoService.test.js | 187 +++++++++++++++++++++ test/undoRedo.batchRehydrateSync.test.js | 61 +++++++ 4 files changed, 407 insertions(+), 14 deletions(-) diff --git a/src/main/services/UndoRedoService.js b/src/main/services/UndoRedoService.js index 1e7f14b..032d00c 100644 --- a/src/main/services/UndoRedoService.js +++ b/src/main/services/UndoRedoService.js @@ -191,12 +191,13 @@ class UndoRedoService { ? normalizedSource.toLowerCase() === normalizedDestination.toLowerCase() : normalizedSource === normalizedDestination; if (!(await this.fileExists(normalizedSource))) { - const missingErr = /** @type {Error & { code?: string }} */ ( + const missingErr = /** @type {Error & { code?: string, path?: string }} */ ( new Error( `Cannot move: source no longer exists (moved or deleted outside the app): ${normalizedSource}` ) ); missingErr.code = 'UNDO_SOURCE_MISSING'; + missingErr.path = normalizedSource; throw missingErr; } if (!isSameFileTarget && (await this.fileExists(normalizedDestination))) { @@ -1179,8 +1180,122 @@ class UndoRedoService { logger.error('[UndoRedoService] Failed to undo action', { error: error.message }); - throw new Error(`Failed to undo action: ${error.message}`, { cause: error }); + // The batch branch above always advances; a single action that can + // never be reversed must not hold the cursor either, or every Undo + // retries it and nothing older can be undone. + if (await this._isPermanentlyIrreversible(error)) { + return this._dropIrreversibleAction(action, 'undo', error); + } + // Keep the code on the wrapper: the IPC layer forwards error.code so + // the renderer can tell the user what to do. + const wrapped = /** @type {Error & { code?: string }} */ ( + new Error(`Failed to undo action: ${error.message}`, { cause: error }) + ); + if (error?.code) wrapped.code = error.code; + throw wrapped; + } + } + + /** + * True when a missing file is gone rather than out of reach: the path + * itself reports ENOENT while its volume root still answers. An unplugged + * or offline drive fails the root probe. + * @private + * @param {any} filePath + * @returns {Promise} + */ + async _isGoneForGood(filePath) { + if (typeof filePath !== 'string' || !filePath) return false; + try { + await fs.stat(filePath); + return false; + } catch (statError) { + if (/** @type {any} */ (statError)?.code !== 'ENOENT') return false; + } + // On macOS and Linux the root is always "/", so probe the mount point of + // a removable or network volume instead: an ejected drive leaves no + // /Volumes/, and its files must not read as deleted. + const mount = filePath.match( + /^(\/Volumes\/[^/]+|\/media\/[^/]+\/[^/]+|\/run\/media\/[^/]+\/[^/]+|\/mnt\/[^/]+)\// + ); + try { + await fs.access(mount ? mount[1] : path.parse(filePath).root); + return true; + } catch { + return false; + } + } + + /** + * Whether a failed single undo/redo can never succeed on a retry: the file + * (or the delete backup) it needs no longer exists. An occupied destination + * or a changed copy is something the user can fix, and a file on a volume + * that is not reachable right now may come back; those keep their entry. + * @private + * @param {any} error + * @returns {Promise} + */ + async _isPermanentlyIrreversible(error) { + const code = error?.code; + if (code !== 'UNDO_SOURCE_MISSING' && code !== 'UNDO_BACKUP_MISSING') return false; + // A delete recorded without any backup has nothing to wait for. + if (code === 'UNDO_BACKUP_MISSING' && !error.path) return true; + return this._isGoneForGood(error.path); + } + + /** + * Remove an entry that can never be undone or redone, as the trash-only + * branch of _doUndo does, and say so: the result names the entry, the + * reason and the fact that it left history, so the failure is never silent. + * @private + * @param {any} action + * @param {'undo'|'redo'} direction + * @param {any} error + */ + async _dropIrreversibleAction(action, direction, error) { + const index = this.actions.indexOf(action); + if (index !== -1) { + this.actions.splice(index, 1); + if (index <= this.currentIndex) this.currentIndex--; + this._recalculateMemoryEstimate(); } + try { + await this._saveActionsNow(); + } catch (saveError) { + logger.warn('[UndoRedoService] Could not persist history after dropping an entry', { + actionId: action.id, + error: /** @type {any} */ (saveError)?.message + }); + } + logger.warn( + `[UndoRedoService] Entry can never be ${direction === 'undo' ? 'undone' : 'redone'}; removed from history`, + { + actionId: action.id, + type: action.type, + description: action.description, + code: error.code, + error: error.message + } + ); + this._notifyStateChanged('remove'); + const reason = + error.code === 'UNDO_BACKUP_MISSING' + ? 'the backup of the deleted file is gone' + : 'the file was moved or deleted outside the app'; + return { + success: false, + code: error.code, + removedFromHistory: true, + action: { + id: action.id, + type: action.type, + description: action.description, + timestamp: action.timestamp + }, + message: `Cannot ${direction} "${action.description}": ${reason}. The entry was removed from history.`, + error: error.message, + pathMutations: /** @type {Array<{ oldPath: string, newPath: string }>} */ ([]) + }; } /** @@ -1319,7 +1434,16 @@ class UndoRedoService { logger.error('[UndoRedoService] Failed to redo action', { error: error.message }); - throw new Error(`Failed to redo action: ${error.message}`, { cause: error }); + // Same as undo: an entry whose file is gone leaves history, and the + // wrapper keeps the code. + if (await this._isPermanentlyIrreversible(error)) { + return this._dropIrreversibleAction(action, 'redo', error); + } + const wrapped = /** @type {Error & { code?: string }} */ ( + new Error(`Failed to redo action: ${error.message}`, { cause: error }) + ); + if (error?.code) wrapped.code = error.code; + throw wrapped; } } @@ -1421,11 +1545,15 @@ class UndoRedoService { // Previous code didn't persist backup paths immediately, risking data loss on crashes // Now we check multiple backup locations and provide detailed error information if (!action.data.backupPath) { - throw new Error( - 'Cannot restore deleted file - no backup path was recorded. ' + - 'File may have been permanently deleted without backup. ' + - `Original path: ${action.data.originalPath}` + const noBackupErr = /** @type {Error & { code?: string }} */ ( + new Error( + 'Cannot restore deleted file - no backup path was recorded. ' + + 'File may have been permanently deleted without backup. ' + + `Original path: ${action.data.originalPath}` + ) ); + noBackupErr.code = 'UNDO_BACKUP_MISSING'; + throw noBackupErr; } const backupExists = await this.fileExists(action.data.backupPath); @@ -1439,13 +1567,18 @@ class UndoRedoService { timestamp: action.timestamp }); - throw new Error( - `Cannot restore deleted file - backup not found at expected location.\n` + - `Original file: ${action.data.originalPath}\n` + - `Expected backup: ${action.data.backupPath}\n` + - `This may indicate the backup was never created, was deleted, or the path is incorrect.\n` + - `Action ID: ${action.id}, Timestamp: ${action.timestamp}` + const backupErr = /** @type {Error & { code?: string, path?: string }} */ ( + new Error( + `Cannot restore deleted file - backup not found at expected location.\n` + + `Original file: ${action.data.originalPath}\n` + + `Expected backup: ${action.data.backupPath}\n` + + `This may indicate the backup was never created, was deleted, or the path is incorrect.\n` + + `Action ID: ${action.id}, Timestamp: ${action.timestamp}` + ) ); + backupErr.code = 'UNDO_BACKUP_MISSING'; + backupErr.path = action.data.backupPath; + throw backupErr; } // Restore file from backup @@ -1566,10 +1699,11 @@ class UndoRedoService { const copySrc = await this._validateActionPath(action.data.originalPath, 'copy-source'); const copyDest = await this._validateActionPath(action.data.newPath, 'copy-destination'); if (!(await this.fileExists(copySrc))) { - const missingErr = /** @type {Error & { code?: string }} */ ( + const missingErr = /** @type {Error & { code?: string, path?: string }} */ ( new Error(`Cannot redo copy: source no longer exists: ${copySrc}`) ); missingErr.code = 'UNDO_SOURCE_MISSING'; + missingErr.path = copySrc; throw missingErr; } try { diff --git a/src/renderer/components/UndoRedoSystem.jsx b/src/renderer/components/UndoRedoSystem.jsx index b6ace1a..5561e99 100644 --- a/src/renderer/components/UndoRedoSystem.jsx +++ b/src/renderer/components/UndoRedoSystem.jsx @@ -1031,6 +1031,12 @@ export function UndoRedoProvider({ children }) { showInfo(result.message || 'Nothing restored: the files are in the system Trash.'); return; } + if (result?.success === false && result.removedFromHistory) { + // Main removed an entry that can never be reversed (file or backup + // gone) and says so in its message; the next undo reaches older ones. + showError(result.message || formatUndoRedoFailure('undo', undoAction.description, result)); + return; + } assertSuccessfulUndoRedoResult(result, 'undo'); showSuccess(result?.message || `Undid: ${undoAction.description}`); } catch (error) { @@ -1079,6 +1085,11 @@ export function UndoRedoProvider({ children }) { try { const result = await action.redo(action); + if (result?.success === false && result.removedFromHistory) { + // Main removed an entry that can never be redone; see undo above. + showError(result.message || formatUndoRedoFailure('redo', action.description, result)); + return; + } assertSuccessfulUndoRedoResult(result, 'redo'); showSuccess(result?.message || `Redid: ${action.description}`); } catch (error) { diff --git a/test/UndoRedoService.test.js b/test/UndoRedoService.test.js index c621033..d334a0d 100644 --- a/test/UndoRedoService.test.js +++ b/test/UndoRedoService.test.js @@ -697,6 +697,136 @@ describe('UndoRedoService', () => { await expect(service.undo()).rejects.toThrow('Cannot undo truncated batch action data'); }); + + // A single action whose file is gone can never be reversed. It used to + // stay under the cursor, so every Undo retried it and nothing older could + // be undone. + const recordMove = async (name, { landed }) => { + const originalPath = path.join(testDir, `${name}-from.txt`); + const newPath = path.join(testDir, 'sorted', `${name}.txt`); + await fs.mkdir(path.dirname(newPath), { recursive: true }); + if (landed) await fs.writeFile(newPath, name); + await service.recordAction('FILE_MOVE', { originalPath, newPath }); + return { originalPath, newPath }; + }; + + test('a single move whose file is gone is dropped, reported, and no longer blocks older entries', async () => { + const older = await recordMove('older', { landed: true }); + const gone = await recordMove('gone', { landed: false }); + const goneId = service.actions[1].id; + const changes = []; + service.onStateChanged((change) => changes.push(change.reason)); + + const result = await service.undo(); + + expect(result).toEqual( + expect.objectContaining({ + success: false, + code: 'UNDO_SOURCE_MISSING', + removedFromHistory: true, + action: expect.objectContaining({ id: goneId, type: 'FILE_MOVE' }), + pathMutations: [] + }) + ); + // The user is told which entry failed, why, and that it is gone. + expect(result.message).toContain('gone-from.txt'); + expect(result.message).toMatch(/moved or deleted outside the app/); + expect(result.message).toMatch(/removed from history/); + expect(result.error).toContain(gone.newPath); + expect(service.actions).toHaveLength(1); + expect(service.currentIndex).toBe(0); + expect(service.canRedo()).toBe(false); + expect(changes).toContain('remove'); + const persisted = JSON.parse(await fs.readFile(service.actionsPath, 'utf8')); + expect(persisted.actions.map((a) => a.id)).not.toContain(goneId); + + // The older entry is reachable again. + const second = await service.undo(); + expect(second.success).toBe(true); + await expect(fs.readFile(older.originalPath, 'utf8')).resolves.toBe('older'); + expect(service.currentIndex).toBe(-1); + }); + + test('an occupied original location keeps the entry and the error carries its code', async () => { + const move = await recordMove('blocked', { landed: true }); + await fs.writeFile(move.originalPath, 'someone else'); + + const error = await service.undo().then( + () => null, + (e) => e + ); + + expect(error).toBeInstanceOf(Error); + expect(error.code).toBe('UNDO_DESTINATION_OCCUPIED'); + expect(error.message).toMatch(/^Failed to undo action: /); + // Fixable by the user, so the entry stays for a retry. + expect(service.actions).toHaveLength(1); + expect(service.currentIndex).toBe(0); + await expect(fs.readFile(move.originalPath, 'utf8')).resolves.toBe('someone else'); + + await fs.unlink(move.originalPath); + await expect(service.undo()).resolves.toEqual(expect.objectContaining({ success: true })); + }); + + test('a missing file on a volume that does not answer keeps the entry for a retry', async () => { + const move = await recordMove('offline', { landed: false }); + const root = path.parse(move.newPath).root; + const realAccess = fs.access; + const accessSpy = jest.spyOn(fs, 'access').mockImplementation(async (target, ...rest) => { + if (String(target) === root) { + throw Object.assign(new Error('volume offline'), { code: 'ENOENT' }); + } + return realAccess.call(fs, target, ...rest); + }); + + try { + const error = await service.undo().then( + () => null, + (e) => e + ); + expect(error).toBeInstanceOf(Error); + expect(error.code).toBe('UNDO_SOURCE_MISSING'); + expect(service.actions).toHaveLength(1); + expect(service.currentIndex).toBe(0); + } finally { + accessSpy.mockRestore(); + } + }); + + test.each([ + '/Volumes/StratoSortNoSuchVolume/sorted/a.txt', + '/media/nobody/StratoSortNoSuchVolume/a.txt', + '/run/media/nobody/StratoSortNoSuchVolume/a.txt', + '/mnt/StratoSortNoSuchVolume/a.txt' + ])('a file on an ejected volume (%s) does not read as deleted', async (filePath) => { + // "/" always answers, so the mount point is what has to be probed. + await expect(service._isGoneForGood(filePath)).resolves.toBe(false); + }); + + test('a single delete whose backup is gone is dropped with its own code', async () => { + await recordMove('older', { landed: true }); + const originalPath = path.join(testDir, 'deleted.txt'); + await service.recordAction('FILE_DELETE', { + originalPath, + backupPath: path.join(testDir, 'backups', 'deleted.txt.bak'), + createBackup: true + }); + + const result = await service.undo(); + + expect(result).toEqual( + expect.objectContaining({ + success: false, + code: 'UNDO_BACKUP_MISSING', + removedFromHistory: true + }) + ); + expect(result.message).toMatch(/backup/); + expect(result.message).toMatch(/removed from history/); + expect(service.actions).toHaveLength(1); + expect(service.actions[0].type).toBe('FILE_MOVE'); + expect(service.currentIndex).toBe(0); + }); }); describe('redo', () => { @@ -722,6 +852,63 @@ describe('UndoRedoService', () => { await expect(service.redo()).rejects.toThrow('Cannot redo truncated batch action data'); }); + + test('a single move whose file is gone is dropped from the redo side and reported', async () => { + const originalPath = path.join(testDir, 'redo-from.txt'); + const newPath = path.join(testDir, 'sorted', 'redo.txt'); + const laterFrom = path.join(testDir, 'later-from.txt'); + const laterTo = path.join(testDir, 'sorted', 'later.txt'); + await fs.mkdir(path.dirname(newPath), { recursive: true }); + await fs.writeFile(newPath, 'redo'); + await fs.writeFile(laterTo, 'later'); + await service.recordAction('FILE_MOVE', { originalPath, newPath }); + await service.recordAction('FILE_MOVE', { originalPath: laterFrom, newPath: laterTo }); + await service.undo(); + await service.undo(); + expect(service.currentIndex).toBe(-1); + // The first file is deleted outside the app before the redo. + await fs.unlink(originalPath); + + const result = await service.redo(); + + expect(result).toEqual( + expect.objectContaining({ + success: false, + code: 'UNDO_SOURCE_MISSING', + removedFromHistory: true, + pathMutations: [] + }) + ); + expect(result.message).toMatch(/^Cannot redo /); + expect(result.message).toMatch(/removed from history/); + expect(service.actions).toHaveLength(1); + expect(service.currentIndex).toBe(-1); + + // The entry behind it can be redone now. + await expect(service.redo()).resolves.toEqual(expect.objectContaining({ success: true })); + await expect(fs.readFile(laterTo, 'utf8')).resolves.toBe('later'); + }); + + test('an occupied destination keeps the entry and the error carries its code', async () => { + const originalPath = path.join(testDir, 'redo-blocked-from.txt'); + const newPath = path.join(testDir, 'sorted', 'redo-blocked.txt'); + await fs.mkdir(path.dirname(newPath), { recursive: true }); + await fs.writeFile(newPath, 'mine'); + await service.recordAction('FILE_MOVE', { originalPath, newPath }); + await service.undo(); + await fs.writeFile(newPath, 'someone else'); + + const error = await service.redo().then( + () => null, + (e) => e + ); + + expect(error).toBeInstanceOf(Error); + expect(error.code).toBe('UNDO_DESTINATION_OCCUPIED'); + expect(service.actions).toHaveLength(1); + expect(service.currentIndex).toBe(-1); + expect(service.canRedo()).toBe(true); + }); }); describe('shutdown', () => { diff --git a/test/undoRedo.batchRehydrateSync.test.js b/test/undoRedo.batchRehydrateSync.test.js index 5956fff..b7d3541 100644 --- a/test/undoRedo.batchRehydrateSync.test.js +++ b/test/undoRedo.batchRehydrateSync.test.js @@ -437,6 +437,67 @@ describe('UndoRedo batch rehydrated sync', () => { expect(mockShowSuccess).not.toHaveBeenCalledWith(expect.stringMatching(/^Undid/)); }); + test('an entry main removed because it can never be reversed shows main’s message once, as an error', async () => { + window.electronAPI.undoRedo.getState = undefined; + window.electronAPI.undoRedo.onStateChanged = undefined; + const undoMessage = + 'Cannot undo "Clean a.txt": the backup of the deleted file is gone. The entry was removed from history.'; + const redoMessage = + 'Cannot redo "File b.txt": the file was moved or deleted outside the app. The entry was removed from history.'; + const onDone = jest.fn(); + + function RemovedScenario() { + const { executeAction, undo, redo } = useUndoRedo(); + React.useEffect(() => { + let active = true; + const run = async () => { + await executeAction({ + type: 'CUSTOM_ACTION', + description: 'File b.txt', + execute: async () => ({ success: true }), + undo: async () => ({ success: true }), + redo: async () => ({ + success: false, + code: 'UNDO_SOURCE_MISSING', + removedFromHistory: true, + message: redoMessage + }) + }); + await undo(); + await redo(); + await executeAction({ + type: 'CUSTOM_ACTION', + description: 'Clean a.txt', + execute: async () => ({ success: true }), + undo: async () => ({ + success: false, + code: 'UNDO_BACKUP_MISSING', + removedFromHistory: true, + message: undoMessage + }), + redo: async () => ({ success: true }) + }); + await undo(); + if (active) onDone(); + }; + run(); + return () => { + active = false; + }; + }, [executeAction, undo, redo]); + return null; + } + + render( + + + + ); + + await waitFor(() => expect(onDone).toHaveBeenCalled()); + expect(mockShowError.mock.calls.map(([text]) => text)).toEqual([redoMessage, undoMessage]); + }); + test('defers main-process sync while a multi-step jump is in progress', async () => { const getState = jest .fn() From 5edd053842cd02522a8dc59dc2cda67006087de8 Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:40 -0500 Subject: [PATCH 03/26] fix(suggestions): feedback notes no longer write the ranking score into confidence --- .../OrganizationSuggestionServiceCore.js | 54 +++++++------ test/OrganizationSuggestionService.test.js | 80 +++++++++++++++++++ 2 files changed, 111 insertions(+), 23 deletions(-) diff --git a/src/main/services/organization/OrganizationSuggestionServiceCore.js b/src/main/services/organization/OrganizationSuggestionServiceCore.js index 6769403..b36f553 100644 --- a/src/main/services/organization/OrganizationSuggestionServiceCore.js +++ b/src/main/services/organization/OrganizationSuggestionServiceCore.js @@ -1884,13 +1884,6 @@ class OrganizationSuggestionServiceCore { const adjusted = rankedSuggestions.map((suggestion) => { const suggestionFolder = suggestion.folder || suggestion.path; - const baseScore = Number.isFinite(suggestion.weightedScore) - ? suggestion.weightedScore - : Number.isFinite(suggestion.score) - ? suggestion.score - : Number.isFinite(suggestion.confidence) - ? suggestion.confidence - : 0; let boost = 0; let penalty = 0; @@ -1924,27 +1917,42 @@ class OrganizationSuggestionServiceCore { } } - const adjustedScore = Math.max(0, Math.min(1, baseScore + boost - penalty)); - const updated = { - ...suggestion, - score: adjustedScore, - confidence: adjustedScore, - weightedScore: adjustedScore - }; + if (boost === 0 && penalty === 0) { + return suggestion; + } - if (boost > 0 || penalty > 0) { - updated.memoryAdjustment = { + // `weightedScore` is the ranking key (score x source weight, up to 1.5) and + // `confidence` is what auto-organize compares with its threshold. Adjust + // each on its own scale: writing the ranking key into confidence moved a + // 0.69 match past 0.75. The ranking key is not capped at 1, or every + // strong suggestion would tie. + const delta = boost - penalty; + const score = Number.isFinite(suggestion.score) + ? suggestion.score + : Number.isFinite(suggestion.confidence) + ? suggestion.confidence + : 0; + const confidence = Number.isFinite(suggestion.confidence) ? suggestion.confidence : score; + const rankingScore = Number.isFinite(suggestion.weightedScore) + ? suggestion.weightedScore + : score; + + return { + ...suggestion, + score: Math.max(0, Math.min(1, score + delta)), + confidence: Math.max(0, Math.min(1, confidence + delta)), + weightedScore: Math.max(0, rankingScore + delta), + memoryAdjustment: { boost, penalty - }; - } - - return updated; + } + }; }); - return adjusted.sort( - (a, b) => (b.weightedScore || b.score || 0) - (a.weightedScore || a.score || 0) - ); + /** @param {any} s */ + const rankingKey = (s) => + Number.isFinite(s.weightedScore) ? s.weightedScore : Number.isFinite(s.score) ? s.score : 0; + return adjusted.sort((a, b) => rankingKey(b) - rankingKey(a)); } async rebuildFeedbackMemoryEmbeddings() { diff --git a/test/OrganizationSuggestionService.test.js b/test/OrganizationSuggestionService.test.js index 9b63b09..dea5624 100644 --- a/test/OrganizationSuggestionService.test.js +++ b/test/OrganizationSuggestionService.test.js @@ -655,6 +655,86 @@ describe('OrganizationSuggestionService', () => { const adjusted = await service.applyFeedbackMemoryAdjustments(file, suggestions); expect(adjusted[0].folder).toBe('CAD'); }); + + test('an unrelated memory leaves confidence and score as they were', async () => { + const file = createTestFile(); + await service.addFeedbackMemory('Keep folder names short', { source: 'manual' }); + + // weightedScore is the ranking key: score x the semantic source weight (1.1). + const suggestions = [ + { folder: 'Invoices', source: 'semantic', score: 0.69, confidence: 0.69 }, + { folder: 'Projects', source: 'llm', score: 0.55, confidence: 0.55 } + ].map((s) => ({ ...s, weightedScore: s.score * (s.source === 'llm' ? 1.5 : 1.1) })); + + const adjusted = await service.applyFeedbackMemoryAdjustments(file, suggestions); + + const invoices = adjusted.find((s) => s.folder === 'Invoices'); + const projects = adjusted.find((s) => s.folder === 'Projects'); + expect(invoices.confidence).toBe(0.69); + expect(invoices.score).toBe(0.69); + expect(invoices.memoryAdjustment).toBeUndefined(); + expect(projects.confidence).toBe(0.55); + expect(projects.score).toBe(0.55); + // Ranking still follows the weighted score: llm 0.825 beats semantic 0.759. + expect(adjusted[0].folder).toBe('Projects'); + }); + + test('a matching memory raises confidence from the confidence, not from the ranking score', async () => { + const file = createTestFile(); + const memory = await service.addFeedbackMemory('Invoices go to Finance', { + source: 'manual' + }); + mockVectorDbService.queryFeedbackMemory.mockResolvedValue([ + { id: memory.id, score: 1, metadata: {}, document: memory.text } + ]); + + const suggestions = [ + { folder: 'Finance', source: 'llm', score: 0.5, confidence: 0.5, weightedScore: 0.75 }, + { folder: 'Projects', source: 'llm', score: 0.8, confidence: 0.8, weightedScore: 1.2 } + ]; + + const adjusted = await service.applyFeedbackMemoryAdjustments(file, suggestions); + + const finance = adjusted.find((s) => s.folder === 'Finance'); + expect(finance.memoryAdjustment).toEqual({ boost: 0.15, penalty: 0 }); + expect(finance.confidence).toBeCloseTo(0.65, 10); + expect(finance.score).toBeCloseTo(0.65, 10); + expect(finance.weightedScore).toBeCloseTo(0.9, 10); + // A weighted score above 1 keeps its lead; clamping it would tie the two. + expect(adjusted[0].folder).toBe('Projects'); + }); + + test('getSuggestionsForFile returns the same confidence with and without an unrelated memory', async () => { + const file = createTestFile(); + const smartFolders = createTestSmartFolders(3); + mockVectorDbService.queryFolders.mockResolvedValue([ + { + id: 'folder-1', + score: 0.69, + metadata: { path: '/docs/Invoices', folderName: 'Invoices' } + } + ]); + // mockLlama is shared by the whole file and clearAllMocks keeps + // implementations, so put back what the tests after this one rely on. + const previousGenerateText = mockLlama.generateText.getMockImplementation(); + mockLlama.generateText.mockResolvedValue({ + response: JSON.stringify({ suggestions: [] }) + }); + + const before = await service.getSuggestionsForFile(file, smartFolders); + expect(before.success).toBe(true); + expect(before.primary.folder).toBe('Invoices'); + expect(before.confidence).toBe(0.69); + + await service.addFeedbackMemory('Keep folder names short', { source: 'manual' }); + + const after = await service.getSuggestionsForFile(file, smartFolders); + expect(after.primary.folder).toBe('Invoices'); + expect(after.confidence).toBe(before.confidence); + // Below the 0.75 auto-organize default: the file stays held for review. + expect(after.confidence).toBeLessThan(0.75); + mockLlama.generateText.mockImplementation(previousGenerateText); + }); }); describe('Error Handling and Edge Cases', () => { From 88b6e320ef8748e924e579aa4ecd8bf5947e4c43 Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:40 -0500 Subject: [PATCH 04/26] fix(vectors): a file moved onto a leftover row's path keeps its own search entry --- src/main/services/OramaVectorService.js | 26 +++++- test/OramaVectorService.test.js | 112 ++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 1 deletion(-) diff --git a/src/main/services/OramaVectorService.js b/src/main/services/OramaVectorService.js index 6ddd619..b12dd0d 100644 --- a/src/main/services/OramaVectorService.js +++ b/src/main/services/OramaVectorService.js @@ -4508,6 +4508,10 @@ class OramaVectorService extends EventEmitter { await this.initialize(); + // Ids this batch still has to move away. A row at one of them belongs to a + // live file (a chain or swap inside one batch), so it is never replaced. + const pendingSourceIds = new Set(pathUpdates.map((spec) => spec?.oldId).filter(Boolean)); + let updated = 0; for (const updateSpec of pathUpdates) { try { @@ -4559,6 +4563,22 @@ class OramaVectorService extends EventEmitter { // If ID changed, insert new then delete old to prevent data loss on insert failure if (newId && newId !== oldId) { + // A row already at the destination id is a leftover: its file was + // deleted or replaced outside the app and the row waits for the + // orphan reaper. Orama's insert throws on an existing id, which left + // the moved file answered by that leftover's vector, so drop the row, + // its sidecar vector and its chunks first. + if (!pendingSourceIds.has(newId) && (await getByID(this._databases.files, newId))) { + await remove(this._databases.files, newId); + this._embeddingStore?.files?.delete(newId); + const staleChunks = await this._removeExactFileChunks(newId, 'update-file-paths'); + if (staleChunks > 0) { + this._emitSearchDataMutated('fileChunks', 'delete', { + fileId: newId, + count: staleChunks + }); + } + } await insert(this._databases.files, { ...existing, ...metaOverrides, @@ -4566,9 +4586,13 @@ class OramaVectorService extends EventEmitter { embedding: safeEmbedding, hasVector, filePath: newPath || existing.filePath, - fileName: newName || existing.fileName + fileName: newName || existing.fileName, + // The file was just seen at its new path. + isOrphaned: false, + orphanedAt: '' }); await remove(this._databases.files, oldId); + pendingSourceIds.delete(oldId); // Migrate the embedding in the sidecar store. Zero-vector placeholders are // intentionally left uncached so the sidecar-repair pass can detect and diff --git a/test/OramaVectorService.test.js b/test/OramaVectorService.test.js index 9eef149..1c1166a 100644 --- a/test/OramaVectorService.test.js +++ b/test/OramaVectorService.test.js @@ -503,6 +503,118 @@ describe('OramaVectorService', () => { expect(file.hasVector).toBe(false); expect(service._embeddingStore.files.has('novec-id')).toBe(false); }); + + // Regression: a row left behind at the destination id (a file deleted outside + // the app keeps its row, orphan-marked, for up to 30 days) made Orama's insert + // throw DOCUMENT_ALREADY_EXISTS. The re-key was swallowed and the moved file + // ended up served by the deleted file's vector, hidden by its orphan flag. + describe('destination id already holds a row', () => { + const staleVector = new Array(768).fill(0.9); + const movedVector = new Array(768).fill(0).map((_, i) => (i % 2 === 0 ? 0.5 : -0.5)); + const staleId = 'file:C:\\dest\\report.pdf'; + const movedId = 'file:C:\\inbox\\report.pdf'; + + const seed = async () => { + await service.upsertFile({ + id: staleId, + vector: staleVector, + meta: { + path: 'C:\\dest\\report.pdf', + fileName: 'report.pdf', + fileType: 'application/pdf', + suggestedName: 'deleted-file' + } + }); + await service.upsertFile({ + id: movedId, + vector: movedVector, + meta: { + path: 'C:\\inbox\\report.pdf', + fileName: 'report.pdf', + fileType: 'application/pdf', + suggestedName: 'moved-file' + } + }); + }; + + const chunksFor = (fileId, n, vector) => + Array.from({ length: n }, (_, i) => ({ + id: `chunk:${fileId}:${i}`, + vector, + meta: { fileId, chunkIndex: i, content: `passage ${i} of ${fileId}` }, + document: `passage ${i}` + })); + + test('updateFilePaths replaces a stale orphaned row at the destination id', async () => { + await seed(); + await service.markEmbeddingsOrphaned([staleId]); + + const updated = await service.updateFilePaths([ + { oldId: movedId, newId: staleId, newMeta: { path: 'C:\\dest\\report.pdf' } } + ]); + + expect(updated).toBe(1); + expect(mockLogger.warn).not.toHaveBeenCalledWith( + '[OramaVectorService] Failed to update file path:', + expect.anything() + ); + await expect(service.getFile(movedId)).resolves.toBeNull(); + + const file = await service.getFile(staleId); + expect(file.suggestedName).toBe('moved-file'); + expect(file.isOrphaned).toBe(false); + expect(file.orphanedAt).toBe(''); + expect(service._embeddingStore.files.get(staleId)).toEqual(movedVector); + expect(service._embeddingStore.files.has(movedId)).toBe(false); + + const results = await service.querySimilarFiles(movedVector, 5); + expect(results.some((r) => r.id === staleId)).toBe(true); + }); + + test('updateFilePaths clears the orphan flag the moved row carried', async () => { + await seed(); + await service.markEmbeddingsOrphaned([movedId]); + + const updated = await service.updateFilePaths([ + { oldId: movedId, newId: 'file:C:\\elsewhere\\report.pdf' } + ]); + + expect(updated).toBe(1); + const file = await service.getFile('file:C:\\elsewhere\\report.pdf'); + expect(file.isOrphaned).toBe(false); + expect(file.orphanedAt).toBe(''); + }); + + test('updateFilePaths drops the stale chunks of the replaced destination row', async () => { + await seed(); + await service.batchUpsertFileChunks(chunksFor(staleId, 3, staleVector)); + await service.batchUpsertFileChunks(chunksFor(movedId, 1, movedVector)); + + await service.updateFilePaths([{ oldId: movedId, newId: staleId }]); + // FilePathCoordinator clones the moved file's chunks after the re-key. + await service.cloneFileChunks(movedId, staleId); + + const chunks = await service.getChunksForFile(staleId); + expect(chunks.map((c) => c.text)).toEqual([`passage 0 of ${movedId}`]); + }); + + test('updateFilePaths keeps a destination row that the same batch still has to move', async () => { + await seed(); + const onwardId = 'file:C:\\archive\\report.pdf'; + + const updated = await service.updateFilePaths([ + { oldId: movedId, newId: staleId }, + { oldId: staleId, newId: onwardId } + ]); + + // The first spec fails as it always did; the row it would have replaced + // belongs to a live file and reaches its own new id intact. + expect(updated).toBe(1); + expect((await service.getFile(onwardId)).suggestedName).toBe('deleted-file'); + expect(service._embeddingStore.files.get(onwardId)).toEqual(staleVector); + expect((await service.getFile(movedId)).suggestedName).toBe('moved-file'); + }); + }); }); describe('persistence scheduling', () => { From 3e27399bf1071ece4d0d324e9e4e13df33762b71 Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:40 -0500 Subject: [PATCH 05/26] fix(organize-ui): notification focus, saved bulk category, real counts and singular wording --- .../components/organize/OrganizeProgress.jsx | 22 ++- src/renderer/phases/CompletePhase.jsx | 11 +- src/renderer/phases/OrganizePhase.jsx | 47 ++--- .../phases/organize/useFileEditing.js | 20 +- .../phases/organize/useOrganization.js | 45 +++-- .../organize/useOrganizeNotificationFocus.js | 84 ++++++--- test/components/CompletePhase.test.js | 65 +++++++ test/components/OrganizeProgress.test.js | 87 +++++++++ test/useFileEditing.test.js | 52 ++++++ test/useOrganization.test.js | 171 +++++++++++++++++- test/useOrganizeNotificationFocus.test.js | 93 +++++++++- 11 files changed, 619 insertions(+), 78 deletions(-) diff --git a/src/renderer/components/organize/OrganizeProgress.jsx b/src/renderer/components/organize/OrganizeProgress.jsx index 1563ee6..9d82a45 100644 --- a/src/renderer/components/organize/OrganizeProgress.jsx +++ b/src/renderer/components/organize/OrganizeProgress.jsx @@ -19,6 +19,7 @@ function OrganizeProgress({ isPaused = false, pausedSummary = null, onResume, + organizedCount, onDismissPaused }) { // PERF: Use memoized selector instead of inline Boolean coercion @@ -216,6 +217,11 @@ function OrganizeProgress({ // keeps the bar at 100% and the count at "N of N" while we hold the frame. const flashSnapshot = completedFlash ? lastProgressRef.current : null; const displayCurrent = flashSnapshot ? flashSnapshot.current : Number(batchProgress.current); + // The bar counts every file the run got through, including ones that could + // not be moved. Only say "Organized" for the number that actually moved. + const completedText = Number.isFinite(organizedCount) + ? `Organized ${organizedCount} ${organizedCount === 1 ? 'file' : 'files'}.` + : `Processed ${displayCurrent} ${displayCurrent === 1 ? 'file' : 'files'}.`; const displayTotal = flashSnapshot ? flashSnapshot.total : Number(batchProgress.total); const displayHasTotals = displayTotal > 0; const percentToShow = completedFlash @@ -252,9 +258,7 @@ function OrganizeProgress({ {completedFlash ? 'Done!' : 'Organizing files...'} - {completedFlash - ? `Organized ${displayCurrent} ${displayCurrent === 1 ? 'file' : 'files'}.` - : 'Do not close the app until completion'} + {completedFlash ? completedText : 'Do not close the app until completion'} @@ -358,8 +362,14 @@ function OrganizeProgress({
{preview.slice(0, 6).map((op, idx) => { + // Progress names the file by its original name; fileName here is + // the name it will have after the move. + const sourceName = + typeof op.sourcePath === 'string' && op.sourcePath + ? op.sourcePath.split(/[\\/]/).pop() + : op.fileName; const isCurrentFile = - batchProgress.currentFile && op.fileName === batchProgress.currentFile; + Boolean(batchProgress.currentFile) && sourceName === batchProgress.currentFile; const isProcessed = hasTotals && idx < batchProgress.current; return ( @@ -420,6 +430,7 @@ OrganizeProgress.propTypes = { preview: PropTypes.arrayOf( PropTypes.shape({ fileName: PropTypes.string, + sourcePath: PropTypes.string, destination: PropTypes.string }) ), @@ -433,7 +444,8 @@ OrganizeProgress.propTypes = { remaining: PropTypes.number }), onResume: PropTypes.func, - onDismissPaused: PropTypes.func + onDismissPaused: PropTypes.func, + organizedCount: PropTypes.number }; export default OrganizeProgress; diff --git a/src/renderer/phases/CompletePhase.jsx b/src/renderer/phases/CompletePhase.jsx index a6a224b..05fffce 100644 --- a/src/renderer/phases/CompletePhase.jsx +++ b/src/renderer/phases/CompletePhase.jsx @@ -99,9 +99,10 @@ function CompletePhase() { const folderCounts = new Map(); safeFiles.forEach((file) => { if (!file || typeof file !== 'object') return; - const destination = file.path || file.newLocation || file.destination || 'Organized'; - destinations.add(destination); + // A destination is a folder. Counting file paths made every file its + // own destination. const folder = pickOpenableFolder(file); + destinations.add(folder ? toCanonicalPath(folder) : file.smartFolder || 'Organized'); if (folder) { folderCounts.set(folder, (folderCounts.get(folder) || 0) + 1); } @@ -244,9 +245,9 @@ function CompletePhase() { > {totalFiles > 0 - ? `Your reviewed batch is complete. ${totalFiles} file${ - totalFiles !== 1 ? 's' : '' - } are now in place.` + ? `Your reviewed batch is complete. ${totalFiles} ${ + totalFiles === 1 ? 'file is' : 'files are' + } now in place.` : 'No files were organized in this session.'} diff --git a/src/renderer/phases/OrganizePhase.jsx b/src/renderer/phases/OrganizePhase.jsx index 080f301..df48870 100644 --- a/src/renderer/phases/OrganizePhase.jsx +++ b/src/renderer/phases/OrganizePhase.jsx @@ -145,27 +145,30 @@ function OrganizePhase() { return getFilteredFiles(filesWithAnalysis); }, [getFilteredFiles, filesWithAnalysis]); + // Shared by single-card edits and the bulk category change. + const persistFileEdit = useCallback( + (filePath, field, value) => { + if (!filePath) return; + if (field === 'category') { + // userCategory marks the choice as the user's, so it still wins + // over the engine's ranking after Organize remounts and the card + // edit state is gone. + dispatch( + updateAnalysisResult({ + path: filePath, + changes: { category: value, userCategory: value } + }) + ); + } + if (field === 'suggestedName') { + dispatch(updateAnalysisResult({ path: filePath, changes: { suggestedName: value } })); + } + }, + [dispatch] + ); + const { editingFiles, setEditingFiles, handleEditFile, getFileWithEdits } = useFileEditing({ - onEditChange: useCallback( - (filePath, field, value) => { - if (!filePath) return; - if (field === 'category') { - // userCategory marks the choice as the user's, so it still wins - // over the engine's ranking after Organize remounts and the card - // edit state is gone. - dispatch( - updateAnalysisResult({ - path: filePath, - changes: { category: value, userCategory: value } - }) - ); - } - if (field === 'suggestedName') { - dispatch(updateAnalysisResult({ path: filePath, changes: { suggestedName: value } })); - } - }, - [dispatch] - ) + onEditChange: persistFileEdit }); const findSmartFolderForCategory = useSmartFolderMatcher(safeSmartFolders); @@ -260,7 +263,8 @@ function OrganizePhase() { editingFiles, setEditingFiles, setSelectedFiles, - addNotification + addNotification, + onEditChange: persistFileEdit }); // Drop edits for files that left the queue so a filtered grid cannot write @@ -779,6 +783,7 @@ function OrganizePhase() { pausedSummary={pausedSummary} onResume={resumeOrganize} onDismissPaused={dismissPausedBatch} + organizedCount={lastOrganizeSummary?.count} />
)} diff --git a/src/renderer/phases/organize/useFileEditing.js b/src/renderer/phases/organize/useFileEditing.js index 86c4021..76f91f9 100644 --- a/src/renderer/phases/organize/useFileEditing.js +++ b/src/renderer/phases/organize/useFileEditing.js @@ -289,7 +289,7 @@ export function useFileSelection(files) { /** * Hook for bulk operations - * @param {{ selectedFiles: any[], editingFiles: Record, setEditingFiles: Function, setSelectedFiles: Function, addNotification: Function }} params + * @param {{ selectedFiles: any[], editingFiles: Record, setEditingFiles: Function, setSelectedFiles: Function, addNotification: Function, onEditChange?: Function }} params * @returns {Record} Bulk operation state and handlers */ export function useBulkOperations({ @@ -297,7 +297,8 @@ export function useBulkOperations({ editingFiles, setEditingFiles, setSelectedFiles, - addNotification + addNotification, + onEditChange }) { const [bulkEditMode, setBulkEditMode] = useState(false); const [bulkCategory, setBulkCategory] = useState(''); @@ -306,6 +307,7 @@ export function useBulkOperations({ const editingFilesRef = useRef(editingFiles); const bulkCategoryRef = useRef(bulkCategory); const addNotificationRef = useRef(addNotification); + const onEditChangeRef = useRef(onEditChange); // Keep refs in sync with latest values useEffect(() => { @@ -320,6 +322,9 @@ export function useBulkOperations({ useEffect(() => { addNotificationRef.current = addNotification; }, [addNotification]); + useEffect(() => { + onEditChangeRef.current = onEditChange; + }, [onEditChange]); // Debounced bulk category change ref const debouncedBulkCategoryChangeRef = useRef(null); @@ -340,10 +345,19 @@ export function useBulkOperations({ newEdits[i] = { ...existingEdit, category }; }); setEditingFiles((/** @type {any} */ prev) => ({ ...prev, ...newEdits })); + // Save the choice the way a single-card edit does. The card edit state + // alone is dropped when Organize remounts or the file is re-analyzed. + const persistEdit = onEditChangeRef.current; + if (typeof persistEdit === 'function') { + selected.forEach((/** @type {any} */ key) => persistEdit(key, 'category', category)); + } setBulkEditMode(false); setBulkCategory(''); setSelectedFiles(new Set()); - notify(`Applied category "${category}" to ${selected.size} files`, 'success'); + notify( + `Applied category "${category}" to ${selected.size} ${selected.size === 1 ? 'file' : 'files'}`, + 'success' + ); }, 300); return () => { diff --git a/src/renderer/phases/organize/useOrganization.js b/src/renderer/phases/organize/useOrganization.js index bafe052..63f27b2 100644 --- a/src/renderer/phases/organize/useOrganization.js +++ b/src/renderer/phases/organize/useOrganization.js @@ -32,6 +32,16 @@ function describeFirstFailure(results) { return message; } +/** + * "1 file", "3 files". + * @param {number} count + * @param {string} [noun] + * @returns {string} + */ +function countOf(count, noun = 'file') { + return `${count} ${count === 1 ? noun : `${noun}s`}`; +} + const ROLLBACK_NAMES_SHOWN = 3; /** @@ -923,7 +933,7 @@ export function useOrganization({ // Reported once, after this block. } else if (result?.partialFailure || failedCount > 0) { addNotification( - `Organized ${uiResults.length} files with ${failedCount} failures. ${describeFirstFailure(resArray)}`, + `Organized ${countOf(uiResults.length)} with ${countOf(failedCount, 'failure')}. ${describeFirstFailure(resArray)}`, 'warning', 8000 ); @@ -933,7 +943,7 @@ export function useOrganization({ error: result?.error }); } else { - addNotification(`Organized ${uiResults.length} files`, 'success', 7000); + addNotification(`Organized ${countOf(uiResults.length)}`, 'success', 7000); } // Files moved on disk but the search index / history / undo @@ -963,11 +973,20 @@ export function useOrganization({ }); } - setBatchProgress({ - current: filesToProcess.length, - total: filesToProcess.length, - currentFile: '' - }); + // Only a run that got through every file fills the bar. A + // cancelled, paused, interrupted or rolled-back run keeps the + // count it reached, so it is not shown as done. + const stoppedEarly = + rolledBackBatch || + cancelledRef.current || + Boolean(result?.cancelled || result?.interrupted || result?.paused); + if (!stoppedEarly) { + setBatchProgress({ + current: filesToProcess.length, + total: filesToProcess.length, + currentFile: '' + }); + } } else if (!rolledBackBatch) { logger.warn('[ORGANIZE] onExecute: No successful results to process'); if (resArray.length > 0) { @@ -1060,12 +1079,14 @@ export function useOrganization({ if (failCount > 0) { addNotification( - `Undo partially complete: ${successCount} files restored, ${failCount} failed.`, + `Undo partially complete: ${countOf(successCount)} restored, ${failCount} failed.`, 'warning' ); } else if (successCount > 0) { addNotification( - `Undo complete. ${successCount} files restored to original locations.`, + successCount === 1 + ? 'Undo complete. 1 file restored to its original location.' + : `Undo complete. ${successCount} files restored to original locations.`, 'info' ); } @@ -1175,11 +1196,11 @@ export function useOrganization({ if (failCount > 0) { addNotification( - `Redo partially complete: ${successCount} files re-organized, ${failCount} failed.`, + `Redo partially complete: ${countOf(successCount)} re-organized, ${failCount} failed.`, 'warning' ); } else if (successCount > 0) { - addNotification(`Redo complete. ${successCount} files re-organized.`, 'info'); + addNotification(`Redo complete. ${countOf(successCount)} re-organized.`, 'info'); } } catch (redoError) { logger.error('[ORGANIZE] onRedo callback failed:', redoError.message); @@ -1193,7 +1214,7 @@ export function useOrganization({ const result = await executeAction( createOrganizeBatchAction( - `Organize ${operations.length} files`, + `Organize ${countOf(operations.length)}`, operations, stateCallbacks, resumeInfo ? { resumeBatchId: resumeInfo.batchId } : undefined diff --git a/src/renderer/phases/organize/useOrganizeNotificationFocus.js b/src/renderer/phases/organize/useOrganizeNotificationFocus.js index 790dd10..c68e611 100644 --- a/src/renderer/phases/organize/useOrganizeNotificationFocus.js +++ b/src/renderer/phases/organize/useOrganizeNotificationFocus.js @@ -31,6 +31,21 @@ export function useOrganizeNotificationFocus({ ); const handledRequestAtRef = useRef(0); + // The lookup below outlives the render that started it. Organize replaces + // the file lists right after mount (and whenever results stream in), so + // they are read through refs: a new list must not cancel a lookup that is + // still waiting on history or Explorer. + const unprocessedFilesRef = useRef(unprocessedFiles); + const processedFilesRef = useRef(processedFiles); + const addNotificationRef = useRef(addNotification); + const revealFileRef = useRef(revealFile); + useEffect(() => { + unprocessedFilesRef.current = unprocessedFiles; + processedFilesRef.current = processedFiles; + addNotificationRef.current = addNotification; + revealFileRef.current = revealFile; + }); + useEffect(() => { if (!organizeFocusRequest?.requestedAt) return; if (handledRequestAtRef.current === organizeFocusRequest.requestedAt) return; @@ -41,27 +56,35 @@ export function useOrganizeNotificationFocus({ const finish = () => dispatch(clearOrganizeFocus()); - const unprocessedIndex = findFileIndexByPathOrName(unprocessedFiles, filePath, fileName); - if (unprocessedIndex >= 0) { - setSelectedFiles(new Set([toCanonicalPath(unprocessedFiles[unprocessedIndex].path)])); - setFocusFileIndex(unprocessedIndex); - if (openDetails) { - setViewingFileDetails(unprocessedFiles[unprocessedIndex]); + /** Focus the file if it is in the queue or the organized list. */ + const focusInLists = () => { + const unprocessed = unprocessedFilesRef.current; + const unprocessedIndex = findFileIndexByPathOrName(unprocessed, filePath, fileName); + if (unprocessedIndex >= 0) { + setSelectedFiles(new Set([toCanonicalPath(unprocessed[unprocessedIndex].path)])); + setFocusFileIndex(unprocessedIndex); + if (openDetails) { + setViewingFileDetails(unprocessed[unprocessedIndex]); + } + return true; } - finish(); - return; - } - const processedMatch = findFileByPathOrName(processedFiles, filePath, fileName); - if (processedMatch) { - setShowHistoryModal(true); - if (openDetails) { - setViewingFileDetails({ - name: fileName || processedMatch.originalName || processedMatch.name, - path: filePath || processedMatch.path || processedMatch.currentPath, - analysis: processedMatch.analysis || null - }); + const processedMatch = findFileByPathOrName(processedFilesRef.current, filePath, fileName); + if (processedMatch) { + setShowHistoryModal(true); + if (openDetails) { + setViewingFileDetails({ + name: fileName || processedMatch.originalName || processedMatch.name, + path: filePath || processedMatch.path || processedMatch.currentPath, + analysis: processedMatch.analysis || null + }); + } + return true; } + return false; + }; + + if (focusInLists()) { finish(); return; } @@ -74,6 +97,10 @@ export function useOrganizeNotificationFocus({ const history = await window.electronAPI.analysisHistory.getFileHistory(filePath); if (cancelled) return; + // On a fresh mount the organized list is still empty when the + // request arrives; it is filled by the time history answers. + if (focusInLists()) return; + const entry = Array.isArray(history) ? history[0] : history; if (entry && typeof entry === 'object') { setViewingFileDetails({ @@ -81,46 +108,47 @@ export function useOrganizeNotificationFocus({ path: filePath, analysis: entry }); - finish(); return; } } - if (filePath && typeof revealFile === 'function') { - await revealFile(filePath); + const reveal = revealFileRef.current; + if (filePath && typeof reveal === 'function') { + await reveal(filePath); if (!cancelled) { - addNotification( + addNotificationRef.current( fileName ? `Opened ${fileName} in Explorer` : 'Opened file in Explorer', 'info', 3500 ); } } else if (fileName) { - addNotification(`Could not locate ${fileName} in Organize`, 'warning', 4500); + addNotificationRef.current(`Could not locate ${fileName} in Organize`, 'warning', 4500); } } catch (error) { logger.warn('Failed to focus file from notification', { error: error?.message, filePath }); if (!cancelled) { - addNotification('Could not open the file from this notification', 'warning', 4500); + addNotificationRef.current( + 'Could not open the file from this notification', + 'warning', + 4500 + ); } } finally { if (!cancelled) finish(); } })(); + // Runs on unmount or when a newer request replaces this one. return () => { cancelled = true; }; }, [ organizeFocusRequest, - unprocessedFiles, - processedFiles, setSelectedFiles, setFocusFileIndex, setViewingFileDetails, setShowHistoryModal, - addNotification, - revealFile, dispatch ]); } diff --git a/test/components/CompletePhase.test.js b/test/components/CompletePhase.test.js index d411029..41b1fc3 100644 --- a/test/components/CompletePhase.test.js +++ b/test/components/CompletePhase.test.js @@ -176,4 +176,69 @@ describe('CompletePhase premium finish', () => { expect(mockCancelPendingPersistence).toHaveBeenCalled(); }); + + describe('counts in the summary', () => { + const { useAppSelector } = require('../../src/renderer/store/hooks'); + const { selectOrganizedCount } = require('../../src/renderer/store/selectors'); + const defaultSelector = useAppSelector.getMockImplementation(); + + const renderWithOrganized = (organizedFiles) => { + selectRedactPaths.mockReturnValue(false); + selectOrganizedCount.mockReturnValue(organizedFiles.length); + useAppSelector.mockImplementation((selector) => + selector({ + files: { organizedFiles, organizedPathIndex: [] }, + analysis: { results: [] } + }) + ); + render(); + }; + + afterEach(() => { + useAppSelector.mockImplementation(defaultSelector); + selectOrganizedCount.mockReturnValue(2); + }); + + test('counts destination folders, not one destination per file', () => { + renderWithOrganized([ + { originalName: 'a.pdf', path: 'C:\\Docs\\Finance\\a.pdf', smartFolder: 'Finance' }, + { originalName: 'b.pdf', path: 'c:/docs/finance/b.pdf', smartFolder: 'Finance' }, + { originalName: 'c.txt', path: 'C:\\Docs\\Notes\\c.txt', smartFolder: 'Notes' } + ]); + + expect(screen.getByText('3 files moved into 2 destinations.')).toBeInTheDocument(); + expect(screen.getByText('2 destinations')).toBeInTheDocument(); + }); + + test('files in one folder are one destination, with no destinations badge', () => { + renderWithOrganized([ + { originalName: 'a.pdf', path: '/dest/finance/a.pdf', smartFolder: 'finance' }, + { originalName: 'b.pdf', path: '/dest/finance/b.pdf', smartFolder: 'finance' } + ]); + + expect(screen.getByText('2 files moved into 1 destination.')).toBeInTheDocument(); + expect(screen.queryByText(/\d+ destinations$/)).not.toBeInTheDocument(); + }); + + test('a single organized file reads "1 file is now in place"', () => { + renderWithOrganized([ + { originalName: 'a.pdf', path: '/dest/finance/a.pdf', smartFolder: 'finance' } + ]); + + expect( + screen.getByText('Your reviewed batch is complete. 1 file is now in place.') + ).toBeInTheDocument(); + }); + + test('several organized files read "are now in place"', () => { + renderWithOrganized([ + { originalName: 'a.pdf', path: '/dest/finance/a.pdf', smartFolder: 'finance' }, + { originalName: 'b.pdf', path: '/dest/finance/b.pdf', smartFolder: 'finance' } + ]); + + expect( + screen.getByText('Your reviewed batch is complete. 2 files are now in place.') + ).toBeInTheDocument(); + }); + }); }); diff --git a/test/components/OrganizeProgress.test.js b/test/components/OrganizeProgress.test.js index 21596fd..aaab30b 100644 --- a/test/components/OrganizeProgress.test.js +++ b/test/components/OrganizeProgress.test.js @@ -93,6 +93,93 @@ describe('OrganizeProgress pause/resume (UX-4)', () => { expect(screen.queryByText(/done!/i)).toBeNull(); }); + describe('completion flash', () => { + const finishRun = (lastProgress, extraProps = {}) => { + const { rerender } = render( + + ); + rerender( + + ); + }; + + test('counts the files that were organized, not every file the run touched', () => { + finishRun({ current: 10, total: 10 }, { organizedCount: 7 }); + + expect(screen.getByText('Done!')).toBeTruthy(); + expect(screen.getByText('Organized 7 files.')).toBeTruthy(); + expect(screen.queryByText(/Organized 10 files/)).toBeNull(); + }); + + test('without an organized count it only claims the files were processed', () => { + finishRun({ current: 10, total: 10 }); + + expect(screen.getByText('Processed 10 files.')).toBeTruthy(); + expect(screen.queryByText(/Organized 10 files/)).toBeNull(); + }); + + test('a run that stopped short of the total shows no "Done!"', () => { + finishRun({ current: 4, total: 10 }, { organizedCount: 4 }); + + expect(screen.queryByText(/done!/i)).toBeNull(); + }); + }); + + describe('planned operations list', () => { + // The name can also appear in the "now moving" line above the list. + const rowFor = (name) => + screen + .getAllByText(name) + .map((el) => el.closest('.rounded-lg')) + .find(Boolean); + + test('highlights the row being moved when the file is renamed on the way', () => { + render( + + ); + + expect(rowFor('Invoice 2026.pdf').className).toContain('bg-stratosort-blue/10'); + expect(rowFor('Notes.txt').className).not.toContain('bg-stratosort-blue/10'); + }); + + test('does not highlight a row whose new name equals another file’s original name', () => { + render( + + ); + + expect(rowFor('report.pdf').className).not.toContain('bg-stratosort-blue/10'); + }); + }); + test('renders nothing when neither organizing nor paused', () => { const { container } = render( { }); }); + describe('useBulkOperations', () => { + const renderBulk = (overrides = {}) => { + const props = { + selectedFiles: new Set(['/a.txt', '/b.txt']), + editingFiles: { '/a.txt': { suggestedName: 'A.txt' } }, + setEditingFiles: jest.fn(), + setSelectedFiles: jest.fn(), + addNotification: jest.fn(), + ...overrides + }; + const { result } = renderHook(() => hooks.useBulkOperations(props)); + return { result, props }; + }; + + const applyCategory = (result, category) => { + act(() => { + result.current.setBulkCategory(category); + }); + act(() => { + result.current.applyBulkCategoryChange(); + }); + }; + + test('saves the bulk category as each selected file’s own choice', () => { + const onEditChange = jest.fn(); + const { result, props } = renderBulk({ onEditChange }); + + applyCategory(result, 'Finance'); + + expect(onEditChange.mock.calls).toEqual([ + ['/a.txt', 'category', 'Finance'], + ['/b.txt', 'category', 'Finance'] + ]); + const update = props.setEditingFiles.mock.calls[0][0]; + expect(update({})).toEqual({ + '/a.txt': { suggestedName: 'A.txt', category: 'Finance' }, + '/b.txt': { category: 'Finance' } + }); + }); + + test('says "1 file" when one file is selected', () => { + const { result, props } = renderBulk({ selectedFiles: new Set(['/a.txt']) }); + + applyCategory(result, 'Finance'); + + expect(props.addNotification).toHaveBeenCalledWith( + 'Applied category "Finance" to 1 file', + 'success' + ); + }); + }); + describe('useProcessedFiles', () => { test('initializes with empty processedFileIds', () => { const { result } = renderHook(() => hooks.useProcessedFiles([])); diff --git a/test/useOrganization.test.js b/test/useOrganization.test.js index 06515dd..b8cc679 100644 --- a/test/useOrganization.test.js +++ b/test/useOrganization.test.js @@ -305,7 +305,7 @@ describe('useOrganization', () => { }); expect(options.addNotification).toHaveBeenCalledWith( - expect.stringMatching(/1 failures/), + expect.stringMatching(/1 failure\./), 'warning', expect.anything() ); @@ -1034,7 +1034,7 @@ describe('useOrganization', () => { }) ); expect(options.addNotification).toHaveBeenCalledWith( - 'Undo partially complete: 1 files restored, 1 failed.', + 'Undo partially complete: 1 file restored, 1 failed.', 'warning' ); expect(notified(options, /Undo complete/)).toBe(false); @@ -1085,12 +1085,177 @@ describe('useOrganization', () => { expect.not.arrayContaining(['/inbox/b.txt']) ); expect(options.addNotification).toHaveBeenCalledWith( - 'Redo partially complete: 1 files re-organized, 1 failed.', + 'Redo partially complete: 1 file re-organized, 1 failed.', 'warning' ); }); }); + describe('singular and plural counts in the outcome messages', () => { + const messages = (options) => + options.addNotification.mock.calls.map(([message]) => message); + const twoFiles = ['a', 'b'].map((n) => ({ + path: `/${n}.txt`, + name: `${n}.txt`, + analysis: { category: 'documents', suggestedName: `${n.toUpperCase()}.txt` } + })); + const moved = (n) => ({ + success: true, + source: `/${n}.txt`, + destination: `/Documents/${n.toUpperCase()}.txt` + }); + + const runCallback = async (name, payload, overrides = {}) => { + const options = createMockOptions(overrides); + options.executeAction.mockImplementation(async (action) => { + action.callbacks[name](payload); + return { results: [{ success: true }] }; + }); + const { result } = renderHook(() => useOrganization(options)); + await act(async () => { + await result.current.handleOrganizeFiles(); + }); + return options; + }; + + test('one organized file', async () => { + const options = await runCallback('onExecute', { + results: [{ success: true, source: '/test.txt', destination: '/Documents/test.txt' }] + }); + + expect(messages(options)).toContain('Organized 1 file'); + expect(options.executeAction.mock.calls[0][0].label).toBe('Organize 1 file'); + }); + + test('several organized files', async () => { + const options = await runCallback( + 'onExecute', + { results: [moved('a'), moved('b')] }, + { unprocessedFiles: twoFiles } + ); + + expect(messages(options)).toContain('Organized 2 files'); + expect(options.executeAction.mock.calls[0][0].label).toBe('Organize 2 files'); + }); + + test('one organized file and one failure', async () => { + const options = await runCallback( + 'onExecute', + { results: [moved('a'), { success: false, source: '/b.txt', error: 'EEXIST' }] }, + { unprocessedFiles: twoFiles } + ); + + expect( + messages(options).some((m) => m.startsWith('Organized 1 file with 1 failure. ')) + ).toBe(true); + }); + + test('one file restored by undo', async () => { + const options = await runCallback('onUndo', { + results: [{ success: true, source: '/Documents/test.txt', destination: '/test.txt' }], + successCount: 1, + failCount: 0 + }); + + expect(messages(options)).toContain( + 'Undo complete. 1 file restored to its original location.' + ); + }); + + test('several files restored by undo', async () => { + const options = await runCallback('onUndo', { + results: [ + { success: true, source: '/Documents/A.txt', destination: '/a.txt' }, + { success: true, source: '/Documents/B.txt', destination: '/b.txt' } + ], + successCount: 2, + failCount: 0 + }); + + expect(messages(options)).toContain( + 'Undo complete. 2 files restored to original locations.' + ); + }); + + test('one file re-organized by redo', async () => { + const options = await runCallback('onRedo', { + results: [{ success: true, source: '/test.txt', destination: '/Documents/test.txt' }], + successCount: 1, + failCount: 0 + }); + + expect(messages(options)).toContain('Redo complete. 1 file re-organized.'); + }); + }); + + describe('progress at the end of a run that did not finish', () => { + const threeFiles = ['a', 'b', 'c'].map((n) => ({ + path: `/${n}.txt`, + name: `${n}.txt`, + analysis: { category: 'documents', suggestedName: `${n.toUpperCase()}.txt` } + })); + const movedA = { success: true, source: '/a.txt', destination: '/Documents/A.txt' }; + + // Holds the batch open after onExecute so the progress it left behind + // can be read before the hook's cleanup zeroes it. + const progressAfterExecute = async (batchResult) => { + let release; + const options = createMockOptions({ unprocessedFiles: threeFiles }); + options.executeAction.mockImplementation(async (action) => { + action.callbacks.onExecute(batchResult); + await new Promise((resolve) => { + release = resolve; + }); + return batchResult; + }); + const { result } = renderHook(() => useOrganization(options)); + let pending; + await act(async () => { + pending = result.current.handleOrganizeFiles(); + await Promise.resolve(); + }); + const progress = result.current.batchProgress; + await act(async () => { + release(); + await pending; + }); + return progress; + }; + + test('a completed batch fills the bar', async () => { + const progress = await progressAfterExecute({ + results: threeFiles.map((f) => ({ + success: true, + source: f.path, + destination: `/Documents${f.path}` + })) + }); + + expect(progress).toEqual({ current: 3, total: 3, currentFile: '' }); + }); + + test.each([ + ['cancelled', { cancelled: true, cancelledCount: 2 }], + ['interrupted', { interrupted: true }], + ['paused', { paused: true, batchId: 'batch_1', cancelledCount: 2 }], + [ + 'rolled back', + { success: false, rolledBack: true, results: [{ ...movedA, rollbackFailed: true }] } + ] + ])('a %s batch does not report every file as done', async (_name, flags) => { + const progress = await progressAfterExecute({ + success: true, + results: [movedA], + successCount: 1, + completedOperations: 1, + ...flags + }); + + expect(progress.total).toBe(3); + expect(progress.current).toBeLessThan(3); + }); + }); + test('state callbacks still update global state after unmount', async () => { jest.useFakeTimers(); const options = createMockOptions(); diff --git a/test/useOrganizeNotificationFocus.test.js b/test/useOrganizeNotificationFocus.test.js index fea73b5..cacf082 100644 --- a/test/useOrganizeNotificationFocus.test.js +++ b/test/useOrganizeNotificationFocus.test.js @@ -1,7 +1,7 @@ /** * @jest-environment jsdom */ -import { renderHook, waitFor } from '@testing-library/react'; +import { renderHook, waitFor, act } from '@testing-library/react'; const mockDispatch = jest.fn(); let mockOrganizeFocusRequest = null; @@ -168,4 +168,95 @@ describe('useOrganizeNotificationFocus', () => { expect(props.setSelectedFiles).toHaveBeenCalledTimes(1); }); + + describe('a lookup that is still pending when the file lists change', () => { + let resolveHistory; + + const renderWithPendingHistory = (initialLists) => { + window.electronAPI = { + analysisHistory: { + getFileHistory: jest.fn( + () => + new Promise((resolve) => { + resolveHistory = resolve; + }) + ) + } + }; + const callbacks = { + setSelectedFiles: jest.fn(), + setFocusFileIndex: jest.fn(), + setViewingFileDetails: jest.fn(), + setShowHistoryModal: jest.fn(), + addNotification: jest.fn(), + revealFile: jest.fn().mockResolvedValue(undefined) + }; + const view = renderHook((lists) => useOrganizeNotificationFocus({ ...lists, ...callbacks }), { + initialProps: initialLists + }); + return { ...callbacks, rerender: view.rerender }; + }; + + afterEach(() => { + delete window.electronAPI; + }); + + test('still opens the details and clears the request after the lists are replaced', async () => { + mockOrganizeFocusRequest = { + filePath: '/downloads/watched.pdf', + fileName: 'watched.pdf', + openDetails: true, + requestedAt: 5000 + }; + + const { setViewingFileDetails, rerender } = renderWithPendingHistory({ + unprocessedFiles: [], + processedFiles: [] + }); + expect(window.electronAPI.analysisHistory.getFileHistory).toHaveBeenCalledTimes(1); + + // Organize's own mount re-render: same contents, new array references. + rerender({ unprocessedFiles: [], processedFiles: [] }); + + await act(async () => { + resolveHistory([{ fileName: 'watched.pdf', category: 'Finance' }]); + }); + + expect(setViewingFileDetails).toHaveBeenCalledWith( + expect.objectContaining({ name: 'watched.pdf', path: '/downloads/watched.pdf' }) + ); + expect(mockDispatch).toHaveBeenCalledWith({ type: 'ui/clearOrganizeFocus' }); + expect(window.electronAPI.analysisHistory.getFileHistory).toHaveBeenCalledTimes(1); + }); + + test('opens the history modal for an organized file that is listed only after mount', async () => { + mockOrganizeFocusRequest = { + filePath: '/dest/notes.txt', + fileName: 'notes.txt', + openDetails: true, + requestedAt: 6000 + }; + + const { setShowHistoryModal, setViewingFileDetails, revealFile, rerender } = + renderWithPendingHistory({ unprocessedFiles: [], processedFiles: [] }); + + rerender({ + unprocessedFiles: [], + processedFiles: [ + { path: '/dest/notes.txt', originalName: 'notes.txt', analysis: { category: 'Notes' } } + ] + }); + + await act(async () => { + resolveHistory([]); + }); + + expect(setShowHistoryModal).toHaveBeenCalledWith(true); + expect(setViewingFileDetails).toHaveBeenCalledWith( + expect.objectContaining({ path: '/dest/notes.txt', analysis: { category: 'Notes' } }) + ); + expect(revealFile).not.toHaveBeenCalled(); + expect(mockDispatch).toHaveBeenCalledWith({ type: 'ui/clearOrganizeFocus' }); + }); + }); }); From b6aa85734a723404466ab81484d5eb78c0deede6 Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:40 -0500 Subject: [PATCH 06/26] fix(session): graph preferences survive an expired session and the notice shows on any screen --- src/renderer/App.js | 19 ++++++ src/renderer/phases/WelcomePhase.jsx | 13 ---- src/renderer/store/index.js | 72 +++++++++++++---------- test/components/App.viewMode.test.js | 64 ++++++++++++++++++++ test/store.persistedStateRecovery.test.js | 58 ++++++++++++++++++ 5 files changed, 182 insertions(+), 44 deletions(-) diff --git a/src/renderer/App.js b/src/renderer/App.js index c288d44..c980868 100644 --- a/src/renderer/App.js +++ b/src/renderer/App.js @@ -17,6 +17,7 @@ import { fetchLicenseStatus } from './store/slices/licenseSlice'; import { useSettingsSubscription } from './hooks/useSettingsSubscription'; import { useNotificationActivation } from './hooks/useNotificationActivation'; import { useKeyboardShortcuts } from './hooks/useKeyboardShortcuts'; +import { useNotification } from './contexts/NotificationContext'; import { hasDestinationFolder } from './utils/smartFolderDestinations'; // AppShell is a .jsx module (untyped under checkJs); cast at import so JSX @@ -91,6 +92,24 @@ function AppContent() { useSettingsSubscription(handleSettingsSync, { enabled: Boolean(settingsEventsEnabled) }); useNotificationActivation(); + // Raised here, not in WelcomePhase: store/index.js keeps viewMode across a + // TTL expiry, so a dashboard user never mounts a phase at startup and the + // session reset went unexplained. Single consumer — the flag is cleared so + // the notice cannot repeat. + const { addNotification } = useNotification(); + useEffect(() => { + const win = /** @type {any} */ (window); + if (win.__STRATOSORT_STATE_EXPIRED__) { + const ageHours = win.__STRATOSORT_STATE_EXPIRED_AGE_HOURS__ || 24; + addNotification( + `Previous session (${ageHours}h old) was cleared to ensure fresh data. Your files are safe.`, + 'info' + ); + delete win.__STRATOSORT_STATE_EXPIRED__; + delete win.__STRATOSORT_STATE_EXPIRED_AGE_HOURS__; + } + }, [addNotification]); + // Apply the chosen palette as a data-theme attribute on . The stylesheet // re-themes every token under :root[data-theme='light'|'dark']; "primary" is // the base :root. selectedTheme is persisted by the Redux persistence diff --git a/src/renderer/phases/WelcomePhase.jsx b/src/renderer/phases/WelcomePhase.jsx index 8cb7724..b97a689 100644 --- a/src/renderer/phases/WelcomePhase.jsx +++ b/src/renderer/phases/WelcomePhase.jsx @@ -328,19 +328,6 @@ function WelcomePhase() { } }, [modelCheckState, showLoadingEscape, addNotification]); - useEffect(() => { - if (window.__STRATOSORT_STATE_EXPIRED__) { - const ageHours = window.__STRATOSORT_STATE_EXPIRED_AGE_HOURS__ || 24; - addNotification( - `Previous session (${ageHours}h old) was cleared to ensure fresh data. Your files are safe.`, - 'info' - ); - // Clear flag so notification doesn't repeat - delete window.__STRATOSORT_STATE_EXPIRED__; - delete window.__STRATOSORT_STATE_EXPIRED_AGE_HOURS__; - } - }, [addNotification]); - const actions = { advancePhase: (phase) => dispatch(setPhase(phase)) }; diff --git a/src/renderer/store/index.js b/src/renderer/store/index.js index 9335fc8..3e2031f 100644 --- a/src/renderer/store/index.js +++ b/src/renderer/store/index.js @@ -352,10 +352,12 @@ const loadState = () => { navigationError: null, lastOperationError: null, resetCounter: 0, - graphTourDismissed: false, + // "Don't show again" and the dock's collapsed state are durable + // preferences, not session state — keep them across TTL expiry. + graphTourDismissed: resolveDurableGraphTourDismissed(parsed?.ui?.graphTourDismissed), graphInsightsSection: 'insights', - graphLegendHintDismissed: false, - graphInsightsDockCollapsed: false, + graphLegendHintDismissed: parsed?.ui?.graphLegendHintDismissed === true, + graphInsightsDockCollapsed: parsed?.ui?.graphInsightsDockCollapsed === true, // Session-only flag: a fresh boot is never mid dashboard excursion. cameFromDashboard: false, // viewMode is a durable preference, not session state — preserve it @@ -426,7 +428,10 @@ const loadState = () => { _version: CURRENT_STATE_VERSION, ui: { viewMode: expiredState.ui.viewMode, - selectedTheme: expiredState.ui.selectedTheme + selectedTheme: expiredState.ui.selectedTheme, + graphTourDismissed: expiredState.ui.graphTourDismissed, + graphLegendHintDismissed: expiredState.ui.graphLegendHintDismissed, + graphInsightsDockCollapsed: expiredState.ui.graphInsightsDockCollapsed }, files: { selectedFiles: [], @@ -469,28 +474,7 @@ const loadState = () => { navigationError: null, lastOperationError: null, resetCounter: 0, - // "Don't show again" lives in two places by design: persisted Redux - // state (saved by the debounced persistence middleware) and a - // localStorage key that GraphTour writes *synchronously* on dismiss. - // The synchronous key is the durable fallback for the case where the - // debounced Redux save hasn't flushed before the app quits — so we - // read it but MUST NOT delete it here, or an immediate close right - // after dismissing would resurface the tour on next launch. `resetUi` - // already preserves this preference intentionally, so there is no - // "ghost after reset" to clean up; clearing is handled by the explicit - // re-show flow (unchecking the box) which removes both stores. - graphTourDismissed: (() => { - const fromState = parsed.ui?.graphTourDismissed === true; - let fromLegacy = false; - if (typeof localStorage !== 'undefined') { - try { - fromLegacy = localStorage.getItem('graphTourDismissed') === 'true'; - } catch { - /* private mode or quota — accept whatever we read */ - } - } - return fromState || fromLegacy; - })(), + graphTourDismissed: resolveDurableGraphTourDismissed(parsed.ui?.graphTourDismissed), graphInsightsSection: (() => { const v = parsed.ui?.graphInsightsSection; if (v == null) return 'insights'; @@ -561,6 +545,31 @@ function resolveDurableSelectedTheme(/** @type {any} */ parsedTheme) { return DURABLE_THEME_VALUES.includes(parsedTheme) ? parsedTheme : 'primary'; } +/** + * Resolve the graph tour's "Don't show again" flag to rehydrate. It lives in + * two places by design: persisted Redux state (saved by the debounced + * persistence middleware) and a localStorage key that GraphTour writes + * *synchronously* on dismiss. The synchronous key is the durable fallback for + * the case where the debounced Redux save hasn't flushed before the app quits — + * so we read it but MUST NOT delete it here, or an immediate close right after + * dismissing would resurface the tour on next launch. `resetUi` already + * preserves this preference intentionally, so there is no "ghost after reset" + * to clean up; clearing is handled by the explicit re-show flow (unchecking the + * box) which removes both stores. + */ +function resolveDurableGraphTourDismissed(/** @type {any} */ parsedFlag) { + const fromState = parsedFlag === true; + let fromLegacy = false; + if (typeof localStorage !== 'undefined') { + try { + fromLegacy = localStorage.getItem('graphTourDismissed') === 'true'; + } catch { + /* private mode or quota — accept whatever we read */ + } + } + return fromState || fromLegacy; +} + function createStratoSortStore() { const preloadedState = loadState(); return configureStore( @@ -664,10 +673,11 @@ if (isWebpackHmr && windowAny[__REDUX_DEV_HMR_KEY__]) { markStoreReady(); -// NOTE: State-expiration notification is handled by WelcomePhase.jsx (which -// shows a visible toast). The TTL-expired loadState path always resets to the -// WELCOME phase, so WelcomePhase is guaranteed to mount and display the notice. -// A previous duplicate handler here dispatched a silent Redux entry, causing -// two notification records for the same event. +// NOTE: State-expiration notification is handled by App.js (AppContent), which +// shows a visible toast. The TTL-expired loadState path keeps viewMode, so a +// dashboard user never mounts WelcomePhase at startup; the shell is mounted on +// both surfaces. Keep it the only consumer of the flag: a previous duplicate +// handler here dispatched a silent Redux entry, causing two notification +// records for the same event. export default store; diff --git a/test/components/App.viewMode.test.js b/test/components/App.viewMode.test.js index a66d5b7..c43662b 100644 --- a/test/components/App.viewMode.test.js +++ b/test/components/App.viewMode.test.js @@ -21,6 +21,11 @@ jest.mock('../../src/renderer/store/slices/uiSlice', () => ({ updateSettings: jest.fn((payload) => ({ type: 'ui/updateSettings', payload })) })); +const mockAddNotification = jest.fn(); +jest.mock('../../src/renderer/contexts/NotificationContext', () => ({ + useNotification: () => ({ addNotification: mockAddNotification }) +})); + jest.mock('../../src/shared/constants', () => ({ PHASES: { WELCOME: 'welcome', @@ -191,6 +196,65 @@ describe('App view-mode routing', () => { }); }); +describe('App expired-session notice', () => { + // store/index.js keeps viewMode across a TTL expiry, so a dashboard user + // never mounts WelcomePhase at startup. The notice has to come from the + // shell, which is mounted on both surfaces. + beforeEach(() => { + mockDispatch.mockClear(); + mockAddNotification.mockClear(); + delete window.__STRATOSORT_STATE_EXPIRED__; + delete window.__STRATOSORT_STATE_EXPIRED_AGE_HOURS__; + }); + + afterEach(() => { + delete window.__STRATOSORT_STATE_EXPIRED__; + delete window.__STRATOSORT_STATE_EXPIRED_AGE_HOURS__; + }); + + const expiredNotices = () => + mockAddNotification.mock.calls.filter(([message]) => + String(message).includes('Previous session') + ); + + test('is shown once at startup on the dashboard surface and clears the flag', async () => { + window.__STRATOSORT_STATE_EXPIRED__ = true; + window.__STRATOSORT_STATE_EXPIRED_AGE_HOURS__ = 48; + setupState({ viewMode: 'dashboard', currentPhase: 'welcome', smartFolders: [{ id: 'sf-1' }] }); + const { rerender } = render(); + + expect(await screen.findByTestId('dashboard-view')).toBeInTheDocument(); + expect(screen.queryByTestId('phase-renderer')).not.toBeInTheDocument(); + expect(expiredNotices()).toHaveLength(1); + expect(expiredNotices()[0][0]).toContain('48h old'); + expect(expiredNotices()[0][1]).toBe('info'); + expect(window.__STRATOSORT_STATE_EXPIRED__).toBeUndefined(); + expect(window.__STRATOSORT_STATE_EXPIRED_AGE_HOURS__).toBeUndefined(); + + rerender(); + expect(expiredNotices()).toHaveLength(1); + }); + + test('is shown once on the phased surface', async () => { + window.__STRATOSORT_STATE_EXPIRED__ = true; + window.__STRATOSORT_STATE_EXPIRED_AGE_HOURS__ = 30; + setupState({ viewMode: 'phased', currentPhase: 'welcome' }); + render(); + + expect(await screen.findByTestId('phase-renderer')).toBeInTheDocument(); + expect(expiredNotices()).toHaveLength(1); + expect(window.__STRATOSORT_STATE_EXPIRED__).toBeUndefined(); + }); + + test('is not shown when no session expired', async () => { + setupState({ viewMode: 'dashboard', smartFolders: [{ id: 'sf-1' }] }); + render(); + + expect(await screen.findByTestId('dashboard-view')).toBeInTheDocument(); + expect(expiredNotices()).toHaveLength(0); + }); +}); + describe('App settings overlay hoisting', () => { // The settings overlay must be mounted at App level, NOT inside // PhaseRenderer: the dashboard surface replaces PhaseRenderer entirely, so diff --git a/test/store.persistedStateRecovery.test.js b/test/store.persistedStateRecovery.test.js index f9e3466..7531ef3 100644 --- a/test/store.persistedStateRecovery.test.js +++ b/test/store.persistedStateRecovery.test.js @@ -101,6 +101,64 @@ describe('persisted Redux state recovery across reloads', () => { expect(window.__STRATOSORT_STATE_EXPIRED__).toBeUndefined(); }); + test('a TTL-expired blob keeps the graph hint, dock and tour preferences', () => { + const staleBlob = JSON.stringify({ + _version: 4, + timestamp: Date.now() - 48 * 60 * 60 * 1000, + ui: { + currentPhase: 'organize', + graphTourDismissed: true, + graphLegendHintDismissed: true, + graphInsightsDockCollapsed: true + }, + files: { selectedFiles: [], smartFolders: [], fileStates: {} }, + analysis: { results: [] } + }); + const storage = { [REDUX_STATE_KEY]: staleBlob }; + + const first = bootStore(storage).getState(); + expect(window.__STRATOSORT_STATE_EXPIRED__).toBe(true); + expect(first.ui.currentPhase).toBe('welcome'); + expect(first.ui.graphLegendHintDismissed).toBe(true); + expect(first.ui.graphInsightsDockCollapsed).toBe(true); + expect(first.ui.graphTourDismissed).toBe(true); + + // The replacement snapshot carries them, so a reload before the first + // debounced save keeps them too. + const live = JSON.parse(storage[REDUX_STATE_KEY]); + expect(live.ui).toEqual( + expect.objectContaining({ + graphTourDismissed: true, + graphLegendHintDismissed: true, + graphInsightsDockCollapsed: true + }) + ); + const second = bootStore(storage).getState(); + expect(window.__STRATOSORT_STATE_EXPIRED__).toBeUndefined(); + expect(second.ui.graphLegendHintDismissed).toBe(true); + expect(second.ui.graphInsightsDockCollapsed).toBe(true); + expect(second.ui.graphTourDismissed).toBe(true); + }); + + test('a TTL-expired blob reads the tour dismissal from its own storage key and invents no preference', () => { + const staleBlob = JSON.stringify({ + _version: 4, + timestamp: Date.now() - 48 * 60 * 60 * 1000, + ui: { currentPhase: 'organize', graphLegendHintDismissed: 'yes' }, + files: { selectedFiles: [], smartFolders: [], fileStates: {} }, + analysis: { results: [] } + }); + const storage = { [REDUX_STATE_KEY]: staleBlob, graphTourDismissed: 'true' }; + + const state = bootStore(storage).getState(); + expect(window.__STRATOSORT_STATE_EXPIRED__).toBe(true); + expect(state.ui.graphTourDismissed).toBe(true); + expect(state.ui.graphLegendHintDismissed).toBe(false); + expect(state.ui.graphInsightsDockCollapsed).toBe(false); + // The synchronous mirror key is the durable fallback; loading never removes it. + expect(storage.graphTourDismissed).toBe('true'); + }); + test('a future-version blob does not overwrite the last known-good backup', () => { const goodBackup = JSON.stringify({ _version: 4, From cdd4e8475efc000e8e78ce0a0c9f175626962658 Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:40 -0500 Subject: [PATCH 07/26] fix(ipc): network-share paths reach the mover and free text with a path is not blanked --- src/main/ipc/validationSchemas.js | 31 +++++++- src/preload/ipcSanitizer.js | 18 ++++- test/ipcSanitizer.test.js | 82 ++++++++++++++++++++ test/validationSchemas.test.js | 122 ++++++++++++++++++++++++------ 4 files changed, 223 insertions(+), 30 deletions(-) diff --git a/src/main/ipc/validationSchemas.js b/src/main/ipc/validationSchemas.js index 9c9a1ee..3118a90 100644 --- a/src/main/ipc/validationSchemas.js +++ b/src/main/ipc/validationSchemas.js @@ -28,6 +28,7 @@ const { EXPERIENCE_MODE_VALUES } = require('../../shared/experienceMode'); const { collapseDuplicateProtocols } = require('../../shared/urlUtils'); const { logger } = require('../../shared/logger'); const { getEffectiveMaxPathLength } = require('../../shared/securityConfig'); +const { validateFileOperationPathSync } = require('../../shared/pathSanitization'); const { PROVIDER_BASE_URL_RULE, isAllowedProviderBaseUrl @@ -419,6 +420,31 @@ if (!z) { const SAFE_ABSOLUTE_PATH_MESSAGE = 'Path must be absolute and must not be a UNC, URL, or contain ../ traversal segments'; + // Organize / move paths only. Smart folders may be registered on a network + // share, and the handlers behind these schemas accept UNC through + // validateFileOperationPath, with authorizeDestination refusing a UNC + // destination outside a registered Smart Folder. So a \\server\share path + // passes here when that same main-side validation accepts it. Still refused: + // device namespaces (\\?\, \\.\), forward-slash UNC, a server with no share + // and administrative shares (C$, ADMIN$), which would reach system + // directories without matching the dangerous-path list. + /** + * @param {unknown} val + * @returns {boolean} + */ + const isSafeFileOpPath = (val) => { + if (isSafeAbsoluteFsPath(val)) return true; + if (typeof val !== 'string' || process.platform !== 'win32') return false; + const unc = val.match(/^\\\\([^\\/]+)\\([^\\/]+)(?:[\\/]|$)/); + if (!unc) return false; + const [, server, share] = unc; + if (server.startsWith('?') || server.startsWith('.') || share.endsWith('$')) return false; + if (hasPathTraversal(val)) return false; + return validateFileOperationPathSync(val).valid; + }; + const SAFE_FILE_OP_PATH_MESSAGE = + 'Path must be absolute and must not be a URL, a device or administrative share path, or contain ../ traversal segments'; + /** * File path validation - non-empty string with traversal protection */ @@ -746,15 +772,16 @@ if (!z) { */ // IPC-gate path hardening (audit item #1): apply the same safe-absolute-path // refine used by every other path-bearing schema in this file so a 100KB - // string, URL, UNC, or traversal payload fails at the boundary instead of + // string, URL, or traversal payload fails at the boundary instead of // riding all the way to validateFileOperationPath in batchOrganizeHandler.js. + // A network share path is let through on Windows (see isSafeFileOpPath). // Top-level source/destination remain .optional() because move/copy use them // but batch_organize routes through operations[] instead. const fileOpPathSchema = z .string() .min(1) .max(2048) - .refine(isSafeAbsoluteFsPath, SAFE_ABSOLUTE_PATH_MESSAGE); + .refine(isSafeFileOpPath, SAFE_FILE_OP_PATH_MESSAGE); // What the app suggested for this file before the user confirmed. When the // file lands in a different smart folder, batch organize records a diff --git a/src/preload/ipcSanitizer.js b/src/preload/ipcSanitizer.js index d0129d7..937dcd8 100644 --- a/src/preload/ipcSanitizer.js +++ b/src/preload/ipcSanitizer.js @@ -155,13 +155,22 @@ function createIpcSanitizer({ log }) { return value; }; + // Keys whose values are prose typed by the user (chat question, search + // query, folder description, feedback note, log message), never a path. + // Prose that mentions a Windows path ("What is in D:\Docs?") matches the + // path heuristic and used to be blanked before it left the renderer. Exact + // names only; every other key keeps the heuristic, and a key containing + // "path" or "file" always wins. + const FREE_TEXT_KEYS = new Set(['query', 'description', 'text', 'message', 'note']); + /** * @param {any} obj * @param {boolean} [isFilePath] * @param {number} [_depth] + * @param {boolean} [isFreeText] - value sits under a FREE_TEXT_KEYS key * @returns {any} */ - const sanitizeObject = (obj, isFilePath = false, _depth = 0) => { + const sanitizeObject = (obj, isFilePath = false, _depth = 0, isFreeText = false) => { if (_depth > MAX_SANITIZE_DEPTH) { log.warn('[SecureIPC] Sanitization depth limit reached, returning safe fallback value'); return depthLimitedFallback(obj); @@ -172,7 +181,7 @@ function createIpcSanitizer({ log }) { if (!isFilePath && looksLikeSimpleText(stripped)) { return stripped; } - if (isFilePath || looksLikeFilePath(stripped)) { + if (isFilePath || (!isFreeText && looksLikeFilePath(stripped))) { // Only strip ? and * on Windows where they are invalid in filenames. // process.platform may be undefined in bundled preload (process/browser polyfill), // so fall back to navigator.platform which is always available in Electron's renderer context. @@ -230,7 +239,7 @@ function createIpcSanitizer({ log }) { } if (Array.isArray(obj)) { - return obj.map((item) => sanitizeObject(item, isFilePath, _depth + 1)); + return obj.map((item) => sanitizeObject(item, isFilePath, _depth + 1, isFreeText)); } if (obj && typeof obj === 'object') { @@ -256,7 +265,8 @@ function createIpcSanitizer({ log }) { continue; } - sanitized[cleanKey] = sanitizeObject(value, isPathKey, _depth + 1); + const isFreeTextKey = !isPathKey && FREE_TEXT_KEYS.has(key.toLowerCase()); + sanitized[cleanKey] = sanitizeObject(value, isPathKey, _depth + 1, isFreeTextKey); } return sanitized; } diff --git a/test/ipcSanitizer.test.js b/test/ipcSanitizer.test.js index eb93427..6c55084 100644 --- a/test/ipcSanitizer.test.js +++ b/test/ipcSanitizer.test.js @@ -50,4 +50,86 @@ describe('ipcSanitizer', () => { const result = sanitizeObject('/home/user/report%20final.txt', true); expect(result).toBe('/home/user/report%20final.txt'); }); + + // A chat question or a description that mentions a Windows path is prose, + // not a path. Blanking it made main answer "Query must be at least 2 + // characters" for a long question, and saved descriptions empty. + describe('free-text fields that mention a path', () => { + const realPlatform = Object.getOwnPropertyDescriptor(process, 'platform'); + + beforeEach(() => { + Object.defineProperty(process, 'platform', { ...realPlatform, value: 'win32' }); + }); + + afterEach(() => { + Object.defineProperty(process, 'platform', realPlatform); + }); + + test.each([ + ['query', 'What is in D:\\Docs\\Taxes?'], + ['query', 'find invoices under Scans\\2024\\*.pdf'], + ['query', 'C:\\Users\\me what is this?'], + ['query', 'look in Docs\\..\\Old'], + ['description', 'Files from C:\\Scans\\*.pdf'], + ['text', 'Anything under \\\\NAS\\Docs | Archive?'], + ['message', 'Could not open C:\\Scans\\a?.pdf'], + ['note', 'Wrong folder, should be D:\\Taxes\\2024?'] + ])('keeps %s text unchanged: %s', (key, value) => { + const log = { warn: jest.fn() }; + const { sanitizeArguments } = createIpcSanitizer({ log }); + const [sanitized] = sanitizeArguments([{ [key]: value }]); + expect(sanitized[key]).toBe(value); + expect(log.warn).not.toHaveBeenCalled(); + }); + + test('keeps free text inside nested payloads and arrays', () => { + const log = { warn: jest.fn() }; + const { sanitizeArguments } = createIpcSanitizer({ log }); + const [sanitized] = sanitizeArguments([ + { + folder: { description: 'Scans from C:\\Scans\\*.pdf' }, + text: ['Is D:\\Docs\\a.pdf here?'] + } + ]); + expect(sanitized.folder.description).toBe('Scans from C:\\Scans\\*.pdf'); + expect(sanitized.text).toEqual(['Is D:\\Docs\\a.pdf here?']); + }); + + test('still strips script tags and control characters from free text', () => { + const log = { warn: jest.fn() }; + const { sanitizeArguments } = createIpcSanitizer({ log }); + const [sanitized] = sanitizeArguments([ + { query: 'What is in D:\\Docs?\u0007' } + ]); + expect(sanitized.query).toBe('What is in D:\\Docs?'); + }); + + test.each([ + ['a path key with an invalid character', { filePath: 'C:\\a?b.txt' }, 'filePath'], + ['a path key with traversal', { folderPath: 'C:\\safe\\..\\evil' }, 'folderPath'], + ['a source with an invalid character', { source: 'C:\\a?b.txt' }, 'source'], + ['a source with traversal', { source: 'C:\\safe\\..\\evil.txt' }, 'source'], + ['a destination with traversal', { destination: 'Docs\\..\\..\\evil.txt' }, 'destination'], + ['a destination with a wildcard', { destination: 'D:\\Out\\*.pdf' }, 'destination'] + ])('still blanks %s', (_label, payload, key) => { + const log = { warn: jest.fn() }; + const { sanitizeArguments } = createIpcSanitizer({ log }); + const [sanitized] = sanitizeArguments([payload]); + expect(sanitized[key]).toBe(''); + expect(log.warn).toHaveBeenCalled(); + }); + + test('a path key nested under a free-text key is still treated as a path', () => { + const log = { warn: jest.fn() }; + const { sanitizeArguments } = createIpcSanitizer({ log }); + const [sanitized] = sanitizeArguments([{ message: { filePath: 'C:\\safe\\..\\evil.txt' } }]); + expect(sanitized.message.filePath).toBe(''); + }); + + test('a top-level path string is still blanked', () => { + const log = { warn: jest.fn() }; + const { sanitizeArguments } = createIpcSanitizer({ log }); + expect(sanitizeArguments(['C:\\safe\\..\\evil.txt', 'C:\\a?b.txt'])).toEqual(['', '']); + }); + }); }); diff --git a/test/validationSchemas.test.js b/test/validationSchemas.test.js index 2bc755c..732fea4 100644 --- a/test/validationSchemas.test.js +++ b/test/validationSchemas.test.js @@ -454,15 +454,6 @@ describe('validationSchemas (Zod path)', () => { expect(result.success).toBe(false); }); - test('rejects UNC source', () => { - const result = schemas.fileOperation.safeParse({ - type: 'move', - source: '\\\\server\\share\\file.txt', - destination: '/b/file.txt' - }); - expect(result.success).toBe(false); - }); - test('rejects relative source', () => { const result = schemas.fileOperation.safeParse({ type: 'move', @@ -543,14 +534,6 @@ describe('validationSchemas (Zod path)', () => { expect(result.success).toBe(false); }); - test('rejects UNC operations[].destination', () => { - const result = schemas.fileOperation.safeParse({ - type: 'batch_organize', - operations: [{ source: '/in/a.pdf', destination: '\\\\server\\share\\a.pdf' }] - }); - expect(result.success).toBe(false); - }); - test('rejects relative operations[].source', () => { const result = schemas.fileOperation.safeParse({ type: 'batch_organize', @@ -571,6 +554,104 @@ describe('validationSchemas (Zod path)', () => { }); }); + // Smart folders may live on a network share, and the destination policy + // allows a UNC destination inside one. The schema used to refuse every UNC + // path, so that branch could never be reached from the renderer. + describe('file operation paths on a network share', () => { + const realPlatform = Object.getOwnPropertyDescriptor(process, 'platform'); + const setPlatform = (value) => + Object.defineProperty(process, 'platform', { ...realPlatform, value }); + const organizeInto = (destination) => + schemas.fileOperation.safeParse({ + type: 'batch_organize', + operations: [{ source: 'C:\\Users\\kim\\Downloads\\a.pdf', destination }] + }); + + afterEach(() => { + Object.defineProperty(process, 'platform', realPlatform); + }); + + describe('on Windows', () => { + beforeEach(() => setPlatform('win32')); + + test('accepts a UNC destination in batch_organize', () => { + expect(organizeInto('\\\\NAS\\Docs\\Invoices\\a.pdf').success).toBe(true); + }); + + test('accepts a UNC destination in the batchOrganize schema', () => { + const result = schemas.batchOrganize.safeParse({ + operations: [ + { source: 'C:\\Users\\kim\\Downloads\\a.pdf', destination: '\\\\NAS\\Docs\\a.pdf' } + ] + }); + expect(result.success).toBe(true); + }); + + test('accepts a UNC source in a move', () => { + const result = schemas.fileOperation.safeParse({ + type: 'move', + source: '\\\\NAS\\Docs\\Inbox\\a.pdf', + destination: 'C:\\Users\\kim\\Documents\\a.pdf' + }); + expect(result.success).toBe(true); + }); + + test.each([ + ['a device namespace path', '\\\\?\\C:\\Users\\kim\\a.pdf'], + ['a long-path UNC device form', '\\\\?\\UNC\\NAS\\Docs\\a.pdf'], + ['a named pipe', '\\\\.\\pipe\\a'], + ['a server with no share', '\\\\NAS'], + ['a server with an empty share', '\\\\NAS\\'], + ['a forward-slash UNC', '//NAS/Docs/a.pdf'], + ['a mixed-slash UNC', '\\\\NAS/Docs/a.pdf'], + ['an administrative share', '\\\\localhost\\C$\\Windows\\System32\\a.dll'], + ['traversal out of the share', '\\\\NAS\\Docs\\..\\..\\Other\\a.pdf'], + ['an alternate data stream', '\\\\NAS\\Docs\\a.pdf:hidden'], + ['a NUL byte', '\\\\NAS\\Docs\\a.pdf\0.exe'] + ])('still rejects %s', (_label, destination) => { + expect(organizeInto(destination).success).toBe(false); + }); + }); + + describe('off Windows', () => { + beforeEach(() => setPlatform('linux')); + + test('rejects a UNC source', () => { + const result = schemas.fileOperation.safeParse({ + type: 'move', + source: '\\\\server\\share\\file.txt', + destination: '/b/file.txt' + }); + expect(result.success).toBe(false); + }); + + test('rejects a UNC operations[].destination', () => { + const result = schemas.fileOperation.safeParse({ + type: 'batch_organize', + operations: [{ source: '/in/a.pdf', destination: '\\\\server\\share\\a.pdf' }] + }); + expect(result.success).toBe(false); + }); + + test('rejects a UNC operations[].destination in the batchOrganize schema', () => { + const result = schemas.batchOrganize.safeParse({ + operations: [{ source: '/in/a.pdf', destination: '\\\\server\\share\\a.pdf' }] + }); + expect(result.success).toBe(false); + }); + }); + + test('trash and triage schemas keep refusing UNC on Windows', () => { + setPlatform('win32'); + expect(schemas.trashDuplicates.safeParse({ paths: ['\\\\NAS\\Docs\\a.pdf'] }).success).toBe( + false + ); + expect(schemas.downloadsTriageStart.safeParse({ folderPath: '\\\\NAS\\Docs' }).success).toBe( + false + ); + }); + }); + describe('batchOrganize schema — IPC-gate path hardening (audit item #1)', () => { test('rejects traversal in operations[].source', () => { const result = schemas.batchOrganize.safeParse({ @@ -586,13 +667,6 @@ describe('validationSchemas (Zod path)', () => { expect(result.success).toBe(false); }); - test('rejects UNC operations[].destination', () => { - const result = schemas.batchOrganize.safeParse({ - operations: [{ source: '/in/a.pdf', destination: '\\\\server\\share\\a.pdf' }] - }); - expect(result.success).toBe(false); - }); - test('rejects relative operations[].source', () => { const result = schemas.batchOrganize.safeParse({ operations: [{ source: 'relative/file.txt', destination: '/out/a.pdf' }] From 3309cdd53c32de35efa881f532f2a32234c7e3a6 Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:40 -0500 Subject: [PATCH 08/26] fix(discover): recordings analyzed one by one wait their turn instead of failing busy --- src/renderer/phases/discover/useAnalysis.js | 88 ++++++++++- test/useAnalysis.test.js | 167 ++++++++++++++++++++ 2 files changed, 251 insertions(+), 4 deletions(-) diff --git a/src/renderer/phases/discover/useAnalysis.js b/src/renderer/phases/discover/useAnalysis.js index 5c4354b..e6893a7 100644 --- a/src/renderer/phases/discover/useAnalysis.js +++ b/src/renderer/phases/discover/useAnalysis.js @@ -37,7 +37,12 @@ import { import { mapErrorToNotification } from '../../utils/errorMapping'; import { normalizeNamingConventionState } from '../../../shared/namingConventions'; import { toCanonicalPath } from '../../utils/canonicalPath'; -import { filesNotYetReady, isBatchAlreadyRunning, runBatchPathWhenFree } from './batchBusy'; +import { + BATCH_BUSY_RETRY_MS, + filesNotYetReady, + isBatchAlreadyRunning, + runBatchPathWhenFree +} from './batchBusy'; import { setAnalysisRunBase, clearAnalysisRunBase } from '../../store/analysisRunBase'; const logger = createLogger('DiscoverPhase:Analysis'); @@ -45,6 +50,10 @@ const BATCH_ANALYSIS_MIN_FILES = 40; /** Mixed-modality batch threshold when hardware recommends sequential (low VRAM). */ const BATCH_ANALYSIS_LOW_VRAM_MIN_FILES = 3; const BATCH_ANALYSIS_PROGRESS_EVENT = 'operation-progress'; +// How long a recording waits for the media request of an earlier one to end. +// The preload gives that request 330 s and the per-file path 295 s, so a +// request can outlive its file by 35 s. +const MEDIA_BUSY_MAX_WAIT_MS = 42 * 1000; const MEDIA_EXTENSIONS = new Set([ ...(SUPPORTED_AUDIO_EXTENSIONS || []), ...(SUPPORTED_VIDEO_EXTENSIONS || []) @@ -69,13 +78,17 @@ function clearAutoAdvanceTimeoutRef(autoAdvanceTimeoutRef) { * * @param {string} filePath - Path to the file to analyze * @param {number} attempt - Current attempt number (1-based) + * @param {any} [abortSignal] + * @param {any} [namingSettings] + * @param {number} [mediaBusyWaitedMs] - time already spent waiting for an earlier media request * @returns {Promise} Analysis result */ async function analyzeWithRetry( filePath, attempt = 1, - /** @type {any} */ abortSignal = null, - /** @type {any} */ namingSettings = null + abortSignal = null, + namingSettings = null, + mediaBusyWaitedMs = 0 ) { // Ensure electronAPI is available before attempting analysis if (!window.electronAPI?.files?.analyze) { @@ -96,6 +109,32 @@ async function analyzeWithRetry( // service-restart that surfaces as an abort is retried instead of being // bubbled up as a hard failure. const message = String(error?.message || ''); + + // The preload runs one media request per window. A recording that hit + // its limit here still holds that request until the preload's own, + // slightly longer timeout, so the next recording is refused for a short + // while. Wait for it; this is not a failed analysis and uses no attempt. + if ( + error?.code === 'CONCURRENCY_LIMIT' && + isMediaFilePath(filePath) && + mediaBusyWaitedMs < MEDIA_BUSY_MAX_WAIT_MS + ) { + if (abortSignal?.aborted) { + throw new Error('Analysis cancelled by user', { cause: error }); + } + await new Promise((r) => setTimeout(r, BATCH_BUSY_RETRY_MS)); + if (abortSignal?.aborted) { + throw new Error('Analysis cancelled by user', { cause: error }); + } + return analyzeWithRetry( + filePath, + attempt, + abortSignal, + namingSettings, + mediaBusyWaitedMs + BATCH_BUSY_RETRY_MS + ); + } + const isTransient = message.includes('timeout') || message.includes('aborted'); if (attempt < RETRY.MAX_ATTEMPTS_MEDIUM && isTransient) { @@ -109,7 +148,13 @@ async function analyzeWithRetry( if (abortSignal?.aborted) { throw new Error('Analysis cancelled by user', { cause: error }); } - return analyzeWithRetry(filePath, attempt + 1, abortSignal, namingSettings); + return analyzeWithRetry( + filePath, + attempt + 1, + abortSignal, + namingSettings, + mediaBusyWaitedMs + ); } throw error; } @@ -1584,12 +1629,47 @@ export function useAnalysis(options = {}) { } }; + // The preload runs one analysis:analyze-media request per window and + // refuses a second ("Operation already in progress"), so that speech + // recognition is never loaded twice. Workers here run in parallel: + // recordings take turns, documents and images do not wait. + /** @type {Promise} */ + let mediaTurn = Promise.resolve(); + /** @returns {Promise<() => void>} resolves with the call that ends the turn */ + const takeMediaTurn = () => { + const earlier = mediaTurn; + /** @type {() => void} */ + let endTurn = () => {}; + mediaTurn = new Promise((resolve) => { + endTurn = () => resolve(); + }); + return earlier.then(() => endTurn); + }; + const processFile = async (/** @type {any} */ file) => { if (!isActiveRun() || abortSignal.aborted) return; if (processedFiles.has(file.path)) return; // Result already applied by a streamed batch event — nothing left // to do for this file (see appliedResultPaths above). if (appliedResultPaths.has(toCanonicalPath(file.path))) return; + if (!isMediaFilePath(file.path)) { + await analyzeOneFile(file); + return; + } + // Taken before the file is marked as analyzing and before its time + // limit starts: waiting for a turn is not time spent on the file. + const endTurn = await takeMediaTurn(); + try { + await analyzeOneFile(file); + } finally { + endTurn(); + } + }; + + const analyzeOneFile = async (/** @type {any} */ file) => { + if (!isActiveRun() || abortSignal.aborted) return; + if (processedFiles.has(file.path)) return; + if (appliedResultPaths.has(toCanonicalPath(file.path))) return; const fileName = file.name || file.path.split(/[\\/]/).pop(); processedFiles.add(file.path); diff --git a/test/useAnalysis.test.js b/test/useAnalysis.test.js index a4f40ba..59a0b3d 100644 --- a/test/useAnalysis.test.js +++ b/test/useAnalysis.test.js @@ -874,6 +874,173 @@ describe('useAnalysis', () => { await finish(); }); + // The preload lets one analysis:analyze-media request run per window and + // refuses a second while it is pending. This stands in for that lock. + describe('recordings on the per-file path', () => { + const mediaFiles = (names) => names.map((name) => ({ path: '/rec/' + name, name })); + const refusal = () => + Object.assign( + new Error('IPC Error: Operation already in progress: analysis:analyze-media'), + { code: 'CONCURRENCY_LIMIT' } + ); + const errorCalls = () => mockUpdateFileState.mock.calls.filter((call) => call[1] === 'error'); + const workers = (count) => { + mockElectronAPI.system = { + getRecommendedConcurrency: jest + .fn() + .mockResolvedValue({ success: true, maxConcurrent: count }) + }; + }; + + beforeEach(() => workers(3)); + + afterEach(() => { + delete mockElectronAPI.system; + }); + + const readyPaths = () => + mockUpdateFileState.mock.calls.filter((call) => call[1] === 'ready').map((call) => call[0]); + + test('are analyzed one at a time, none refused as already in progress', async () => { + const files = mediaFiles(['a.mp3', 'b.mp3', 'c.mp4']); + let inFlight = 0; + let mostInFlight = 0; + let refused = 0; + const releases = []; + mockElectronAPI.analysis.batch.mockRejectedValueOnce(new Error('Batch route failed')); + mockElectronAPI.files.analyze.mockImplementation(() => { + if (inFlight > 0) { + refused += 1; + return Promise.reject(refusal()); + } + inFlight += 1; + mostInFlight = Math.max(mostInFlight, inFlight); + return new Promise((resolve) => { + releases.push(() => { + inFlight -= 1; + resolve({ category: 'audio', suggestedName: 'done' }); + }); + }); + }); + const hook = renderHook(() => useAnalysis(createMockOptions({ selectedFiles: files }))); + + let analyzePromise; + await act(async () => { + analyzePromise = hook.result.current.analyzeFiles(files); + await jest.advanceTimersByTimeAsync(50); + }); + for (let turn = 0; turn < files.length; turn += 1) { + await act(async () => { + releases.shift()?.(); + await jest.advanceTimersByTimeAsync(50); + }); + } + await act(async () => { + await jest.advanceTimersByTimeAsync(1000); + await analyzePromise; + }); + + expect(refused).toBe(0); + expect(mostInFlight).toBe(1); + expect(errorCalls()).toEqual([]); + expect(readyPaths().sort()).toEqual(['/rec/a.mp3', '/rec/b.mp3', '/rec/c.mp4']); + }); + + test('a document is not held back by a recording in progress', async () => { + const files = [ + { path: '/rec/a.mp3', name: 'a.mp3' }, + { path: '/rec/b.mp3', name: 'b.mp3' }, + { path: '/rec/notes.txt', name: 'notes.txt' } + ]; + const started = []; + mockElectronAPI.analysis.batch.mockRejectedValueOnce(new Error('Batch route failed')); + mockElectronAPI.files.analyze.mockImplementation((filePath) => { + started.push(filePath); + return new Promise(() => {}); + }); + const hook = renderHook(() => useAnalysis(createMockOptions({ selectedFiles: files }))); + + await act(async () => { + hook.result.current.analyzeFiles(files); + await jest.advanceTimersByTimeAsync(50); + }); + + expect(started.sort()).toEqual(['/rec/a.mp3', '/rec/notes.txt']); + + act(() => { + hook.result.current.cancelAnalysis(); + }); + }); + + test('a recording waits for the request of one that timed out to let go', async () => { + // The first recording runs past its limit. Its request stays pending + // in the preload a little longer, and the next recording is refused + // until then. + const files = mediaFiles(['long.mp3', 'next.mp3']); + let held = false; + workers(1); + mockElectronAPI.analysis.batch.mockRejectedValueOnce(new Error('Batch route failed')); + mockElectronAPI.files.analyze.mockImplementation((filePath) => { + if (held) return Promise.reject(refusal()); + if (filePath === '/rec/long.mp3') { + held = true; + setTimeout( + () => { + held = false; + }, + 5 * 60 * 1000 + 20 * 1000 + ); + return new Promise(() => {}); + } + return Promise.resolve({ category: 'audio', suggestedName: 'done' }); + }); + const hook = renderHook(() => useAnalysis(createMockOptions({ selectedFiles: files }))); + + let analyzePromise; + await act(async () => { + analyzePromise = hook.result.current.analyzeFiles(files); + await jest.advanceTimersByTimeAsync(50); + }); + for (let step = 0; step < 34; step += 1) { + await act(async () => { + await jest.advanceTimersByTimeAsync(10 * 1000); + }); + } + await act(async () => { + await analyzePromise; + }); + + expect(errorCalls().map((call) => call[0])).toEqual(['/rec/long.mp3']); + expect(readyPaths()).toEqual(['/rec/next.mp3']); + }); + + test('a recording refused for longer than the wait fails with the refusal', async () => { + const files = mediaFiles(['only.mp3']); + workers(1); + mockElectronAPI.analysis.batch.mockRejectedValueOnce(new Error('Batch route failed')); + mockElectronAPI.files.analyze.mockImplementation(() => Promise.reject(refusal())); + const hook = renderHook(() => useAnalysis(createMockOptions({ selectedFiles: files }))); + + let analyzePromise; + await act(async () => { + analyzePromise = hook.result.current.analyzeFiles(files); + await jest.advanceTimersByTimeAsync(50); + }); + for (let step = 0; step < 6; step += 1) { + await act(async () => { + await jest.advanceTimersByTimeAsync(10 * 1000); + }); + } + await act(async () => { + await analyzePromise; + }); + + expect(errorCalls().map((call) => [call[0], call[2].error])).toEqual([ + ['/rec/only.mp3', 'IPC Error: Operation already in progress: analysis:analyze-media'] + ]); + }); + }); + test('fallback pool skips files whose results already streamed in [F-154]', async () => { const { result } = renderHook(() => useAnalysis(createMockOptions())); const files = Array.from({ length: 40 }, (_, index) => ({ From 092c7e29e240595f2f93e8e978470179e6d079af Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:40 -0500 Subject: [PATCH 09/26] fix(models): early-closed downloads fail and resume, missing projector warns, failed rebuild reverts the form --- src/main/ipc/llama.js | 19 ++- src/main/services/ModelDownloadManager.js | 33 +++++ src/renderer/components/SettingsPanel.jsx | 23 ++- .../settings/ModelSelectionSection.jsx | 13 +- test/ModelDownloadManager.test.js | 133 ++++++++++++++++++ test/components/ModelSelectionSection.test.js | 77 +++++++++- test/components/SettingsPanel.test.js | 46 +++++- test/llamaIpc.handlers.test.js | 86 +++++++++++ 8 files changed, 414 insertions(+), 16 deletions(-) diff --git a/src/main/ipc/llama.js b/src/main/ipc/llama.js index 2cde914..2f49ed2 100644 --- a/src/main/ipc/llama.js +++ b/src/main/ipc/llama.js @@ -625,9 +625,16 @@ function registerLlamaIpc(servicesOrParams) { manager .downloadModel(normalizedName, { onProgress }) .then( - /** @type {() => void} */ ( - () => { - logger.info('[IPC:Llama] Model download completed', { model: normalizedName }); + /** @type {(result: any) => void} */ ( + (result) => { + // A vision model whose projector (mmproj) failed to download + // resolves as degraded: the GGUF is installed, but images + // cannot be analyzed. Report it like backgroundSetup does. + const degraded = Boolean(result?.degraded); + logger.info('[IPC:Llama] Model download completed', { + model: normalizedName, + degraded + }); if (win && !win.isDestroyed()) { const latestRequestedProgress = latestProgressByModel.get(normalizedName) || null; @@ -643,7 +650,8 @@ function registerLlamaIpc(servicesOrParams) { safeSend(win.webContents, IPC_EVENTS.OPERATION_PROGRESS, { type: 'model-download-complete', model: normalizedName, - success: true, + success: !degraded, + ...(degraded ? { degraded: true } : {}), progress: { model: normalizedName, filename: normalizedName, @@ -651,7 +659,8 @@ function registerLlamaIpc(servicesOrParams) { downloadedBytes, totalBytes, speedBps: 0, - etaSeconds: 0 + etaSeconds: 0, + ...(degraded ? { companion: result?.companionDownload || null } : {}) } }); } diff --git a/src/main/services/ModelDownloadManager.js b/src/main/services/ModelDownloadManager.js index 29e5c28..9dfe217 100644 --- a/src/main/services/ModelDownloadManager.js +++ b/src/main/services/ModelDownloadManager.js @@ -691,6 +691,39 @@ class ModelDownloadManager { finalizeFailure(this._wrapWriteStreamError(error, filename), 'error', true); }); + // A connection that closes cleanly before the body is complete (CDN + // recycle, proxy cut) emits 'aborted'/'close' on the response but no + // 'end' and no request 'error', so pipe() never ends the write stream + // and nothing above settles. Fail as ECONNRESET so the retry path + // resumes from the `.partial` with a Range request. + let prematureCloseHandled = false; + const onPrematureClose = (/** @type {any} */ cause) => { + if (prematureCloseHandled || response.complete || settled || failureClaimed) return; + prematureCloseHandled = true; + const interrupted = /** @type {Error & { code?: string, cause?: any }} */ ( + new Error('Download interrupted - connection closed before the file was complete') + ); + interrupted.code = 'ECONNRESET'; + if (cause instanceof Error) interrupted.cause = cause; + const stream = writeStream; + try { + if (stream) response.unpipe(stream); + } catch { + /* defensive */ + } + // Wait for the file to close before failing: the retry re-stats the + // `.partial` to pick its Range start. + if (!stream || stream.closed) { + finalizeFailure(interrupted); + return; + } + stream.once('close', () => finalizeFailure(interrupted)); + stream.destroy(); + }; + response.on('aborted', onPrematureClose); + response.on('error', onPrematureClose); + response.on('close', onPrematureClose); + // Abort listeners are attached immediately after https.get (below) so // cancel during connect/redirect is honored. Keep stream-event cleanup // here as a belt-and-suspenders release if a settle path skips finalize*. diff --git a/src/renderer/components/SettingsPanel.jsx b/src/renderer/components/SettingsPanel.jsx index b89176e..5dc117b 100644 --- a/src/renderer/components/SettingsPanel.jsx +++ b/src/renderer/components/SettingsPanel.jsx @@ -137,6 +137,17 @@ const EXPERIENCE_MODE_COPY = { 'Full control over local models, embeddings, retrieval, and diagnostics.' }; +/** + * Text for a vision model that installed but whose projector (mmproj) failed + * to download: the model is listed, yet it cannot analyze images. + */ +function projectorMissingMessage(modelName, evt) { + const projector = evt?.progress?.companion?.name; + return `Model "${modelName}" installed, but its image projector${ + projector ? ` (${projector})` : '' + } did not download. Image analysis will not work until it does.`; +} + /** * Format a "saved " string without registering a tick interval. * The chip re-renders whenever lastSavedAt updates (next save, next @@ -1058,7 +1069,11 @@ const SettingsPanel = React.memo(function SettingsPanel() { }); } else if (evt.type === 'model-download-complete') { setPullProgress({ modelName: requested, filename: evt.model, percent: 100 }); - addNotification(`Model "${requested}" installed`, 'success'); + if (evt.degraded) { + addNotification(projectorMissingMessage(requested, evt), 'warning'); + } else { + addNotification(`Model "${requested}" installed`, 'success'); + } setNewModel(''); // Refresh on completion — not on IPC ack — so the list reflects // the file actually being on disk. @@ -1185,7 +1200,11 @@ const SettingsPanel = React.memo(function SettingsPanel() { }); } else if (evt?.type === 'model-download-complete' && pendingModels.has(evt.model)) { pendingModels.delete(evt.model); - addNotification(`Model "${evt.model}" downloaded successfully`, 'success'); + if (evt.degraded) { + addNotification(projectorMissingMessage(evt.model, evt), 'warning'); + } else { + addNotification(`Model "${evt.model}" downloaded successfully`, 'success'); + } loadModels(); if (pendingModels.size === 0) { finishAll(); diff --git a/src/renderer/components/settings/ModelSelectionSection.jsx b/src/renderer/components/settings/ModelSelectionSection.jsx index 96746c8..ec767e6 100644 --- a/src/renderer/components/settings/ModelSelectionSection.jsx +++ b/src/renderer/components/settings/ModelSelectionSection.jsx @@ -158,6 +158,7 @@ function ModelSelectionSection({ const confirmChangeAndRebuild = async () => { const targetModel = pendingModel; if (!targetModel) return; + const previousModel = settings.embeddingModel; // Apply the model selection immediately (same effect as "Change only"). // The rebuild handler switches the engine to the override at start, so // leaving the form on the OLD model while the rebuild runs desyncs the @@ -207,13 +208,17 @@ function ModelSelectionSection({ 'The rebuild is taking longer than expected and is still running in the background. Check embeddings status in AI Configuration before starting another rebuild.' }); } else { + // The main process rolls the engine and the saved settings back to + // the previous model when the rebuild fails. Follow it here: a form + // left on the new model would be pushed back by the next auto-save + // of any setting, switching the model and resetting the index again. + setSettings((prev) => + prev.embeddingModel === targetModel ? { ...prev, embeddingModel: previousModel } : prev + ); setRebuildStatus({ variant: 'danger', - message: `Embeddings rebuild failed: ${toErrorMessage(error)}. Your model selection was kept — rebuild embeddings from AI Configuration to apply it.` + message: `Embeddings rebuild failed: ${toErrorMessage(error)}. The embedding model was set back to ${previousModel}. If search results are missing, rebuild embeddings from AI Configuration.` }); - // Model changed but the index wasn't rebuilt — this banner is now - // accurate and points at the recovery path. - setEmbeddingModelChanged(true); } } } finally { diff --git a/test/ModelDownloadManager.test.js b/test/ModelDownloadManager.test.js index 1209772..243a2e1 100644 --- a/test/ModelDownloadManager.test.js +++ b/test/ModelDownloadManager.test.js @@ -559,6 +559,139 @@ describe('ModelDownloadManager', () => { expect(manager.isDownloading('alpha.gguf')).toBe(false); }); + describe('connection closed before the body is complete', () => { + // A clean early close (FIN before Content-Length is satisfied) makes Node + // emit 'aborted' and 'close' on the response, with no 'end' and no error + // on the request, so the piped write stream never finishes. + const makeWriteStream = () => { + const ws = new EventEmitter(); + ws.destroy = jest.fn(() => { + ws.destroyed = true; + setTimeout(() => ws.emit('close'), 0); + }); + return ws; + }; + const makeRequest = () => { + const request = new EventEmitter(); + request.setTimeout = jest.fn(); + request.destroy = jest.fn(); + return request; + }; + const makeTruncatedResponse = () => { + const response = new EventEmitter(); + response.statusCode = 200; + response.statusMessage = 'OK'; + response.headers = { 'content-length': '2048' }; + response.complete = false; + response.resume = jest.fn(); + response.unpipe = jest.fn(); + response.pipe = (ws) => { + setTimeout(() => { + response.emit('data', Buffer.alloc(1000)); + response.emit('aborted'); + response.emit('close'); + }, 0); + return ws; + }; + return response; + }; + + afterEach(() => { + jest.useRealTimers(); + }); + + test('downloadModel fails instead of hanging and keeps the partial for resume', async () => { + jest.useFakeTimers(); + const manager = new ModelDownloadManager(); + const writeStream = makeWriteStream(); + fsModule.createWriteStream.mockReturnValue(writeStream); + mockFs.stat.mockRejectedValueOnce(new Error('no partial')); + + https.get.mockImplementation((_options, onResponse) => { + setTimeout(() => onResponse(makeTruncatedResponse()), 0); + return makeRequest(); + }); + + /** @type {any} */ + let outcome = null; + manager.downloadModel('alpha.gguf', { _maxRetries: 0 }).then( + (value) => { + outcome = { resolved: value }; + }, + (error) => { + outcome = { rejected: error }; + } + ); + await jest.advanceTimersByTimeAsync(100); + + expect(outcome?.rejected?.code).toBe('ECONNRESET'); + expect(writeStream.destroy).toHaveBeenCalled(); + expect(manager.isDownloading('alpha.gguf')).toBe(false); + expect(manager._downloads.has('alpha.gguf')).toBe(false); + expect(mockFs.unlink).not.toHaveBeenCalled(); + }); + + test('downloadModel retries with a Range request from the partial', async () => { + jest.useFakeTimers(); + const manager = new ModelDownloadManager(); + fsModule.createWriteStream.mockImplementation(makeWriteStream); + + // Attempt 1: no partial. Attempt 2: 1000 bytes on disk, then full size. + mockFs.stat + .mockRejectedValueOnce(new Error('no partial')) + .mockResolvedValueOnce({ size: 1000 }) + .mockResolvedValueOnce({ size: 2048 }); + + const requestOptionsSeen = []; + https.get.mockImplementation((requestOptions, onResponse) => { + requestOptionsSeen.push(requestOptions); + if (requestOptionsSeen.length === 1) { + setTimeout(() => onResponse(makeTruncatedResponse()), 0); + return makeRequest(); + } + const response = new EventEmitter(); + response.statusCode = 206; + response.statusMessage = 'Partial Content'; + response.headers = { 'content-length': '1048' }; + response.resume = jest.fn(); + response.pipe = (ws) => { + setTimeout(() => { + response.complete = true; + ws.emit('finish'); + response.emit('close'); + }, 0); + return ws; + }; + setTimeout(() => onResponse(response), 0); + return makeRequest(); + }); + + /** @type {any} */ + let outcome = null; + manager.downloadModel('alpha.gguf').then( + (value) => { + outcome = { resolved: value }; + }, + (error) => { + outcome = { rejected: error }; + } + ); + await jest.advanceTimersByTimeAsync(5000); + + expect(outcome?.resolved).toEqual(expect.objectContaining({ success: true })); + expect(requestOptionsSeen).toHaveLength(2); + expect(requestOptionsSeen[0].headers.Range).toBeUndefined(); + expect(requestOptionsSeen[1].headers.Range).toBe('bytes=1000-'); + expect(fsModule.createWriteStream).toHaveBeenLastCalledWith( + expect.stringContaining('alpha.gguf.partial'), + { flags: 'a' } + ); + expect(mockFs.unlink).not.toHaveBeenCalled(); + expect(mockFs.rename).toHaveBeenCalledTimes(1); + expect(manager.isDownloading('alpha.gguf')).toBe(false); + }); + }); + describe('_wrapWriteStreamError', () => { test('maps ENOSPC to "Disk full" message and preserves code + cause', () => { const manager = new ModelDownloadManager(); diff --git a/test/components/ModelSelectionSection.test.js b/test/components/ModelSelectionSection.test.js index 094e76f..abdaf1d 100644 --- a/test/components/ModelSelectionSection.test.js +++ b/test/components/ModelSelectionSection.test.js @@ -52,8 +52,10 @@ describe('ModelSelectionSection', () => { // Contract: "Change & rebuild now" commits the model selection immediately // (so it can never desync from a rebuild that outlives the IPC timeout) and - // reports the rebuild outcome via an inline status alert. Failures keep the - // selection and tell the user to rebuild from AI Configuration. + // reports the rebuild outcome via an inline status alert. A failed rebuild + // puts the selection back on the previous model, because the main process + // rolls the engine and settings.json back to it; an IPC timeout keeps the + // selection, because the rebuild is still running. const renderWithModels = (setSettings) => render( @@ -103,7 +105,7 @@ describe('ModelSelectionSection', () => { }); }); - test('keeps committed model and surfaces failure alert when rebuild rejects', async () => { + test('commits model and surfaces failure alert when rebuild rejects', async () => { const setSettings = jest.fn(); window.electronAPI = { embeddings: { @@ -174,4 +176,73 @@ describe('ModelSelectionSection', () => { expect(screen.getByText(/embeddings rebuild failed/i)).toBeInTheDocument(); }); }); + + // Holds the settings like SettingsPanel does, so the dropdown shows what the + // next auto-save would send to the main process. + function StatefulSection() { + const [settings, setSettings] = React.useState({ + textModel: 't.gguf', + visionModel: 'v.gguf', + embeddingModel: 'old.gguf' + }); + return ( + + ); + } + + test.each([ + ['returns success: false', () => Promise.resolve({ success: false, error: 'Probe failed' })], + ['rejects', () => Promise.reject(new Error('Probe failed'))] + ])( + 'puts the selection back on the previous model when the rebuild %s', + async (_label, fullRebuild) => { + window.electronAPI = { + embeddings: { + getStats: jest.fn().mockResolvedValue({ totalDocuments: 10 }), + fullRebuild: jest.fn(fullRebuild) + } + }; + + const { unmount } = render(); + changeAndRebuild(); + expect(screen.getAllByRole('combobox')[2]).toHaveValue('new.gguf'); + + await waitFor(() => { + expect(screen.getByText(/embeddings rebuild failed: probe failed/i)).toBeInTheDocument(); + }); + expect(screen.getAllByRole('combobox')[2]).toHaveValue('old.gguf'); + expect(screen.getByText(/set back to old\.gguf/i)).toBeInTheDocument(); + expect(screen.queryByText(/your model selection was kept/i)).not.toBeInTheDocument(); + expect(screen.queryByText(/model changed\./i)).not.toBeInTheDocument(); + // Putting the model back refetches the stats, and that late update + // queues a React render. The shared afterEach clears every timer, and a + // render cancelled that way stalls the renders of the next test. So + // unmount (nothing new can queue) and let the queued one run. + unmount(); + await waitFor(() => expect(screen.queryByRole('alert')).not.toBeInTheDocument()); + } + ); + + test('keeps the new selection when the rebuild only timed out over IPC', async () => { + window.electronAPI = { + embeddings: { + getStats: jest.fn().mockResolvedValue({ totalDocuments: 10 }), + fullRebuild: jest.fn().mockRejectedValue(new Error('IPC timeout after 600000ms')) + } + }; + + render(); + changeAndRebuild(); + + await waitFor(() => { + expect(screen.getByText(/still running in the background/i)).toBeInTheDocument(); + }); + expect(screen.getAllByRole('combobox')[2]).toHaveValue('new.gguf'); + }); }); diff --git a/test/components/SettingsPanel.test.js b/test/components/SettingsPanel.test.js index 072ed5c..1dbaf68 100644 --- a/test/components/SettingsPanel.test.js +++ b/test/components/SettingsPanel.test.js @@ -2,7 +2,7 @@ * @jest-environment jsdom */ import React from 'react'; -import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; import '@testing-library/jest-dom'; let mockState = {}; @@ -187,7 +187,14 @@ jest.mock('../../src/renderer/components/settings/ModelSelectionSection', () => jest.mock('../../src/renderer/components/settings/ChatPersonaSection', () => () => (
Chat persona settings
)); -jest.mock('../../src/renderer/components/settings/ModelManagementSection', () => () => null); +jest.mock('../../src/renderer/components/settings/ModelManagementSection', () => ({ + __esModule: true, + default: ({ onAddModel }) => ( + + ) +})); jest.mock('../../src/renderer/components/settings/EmbeddingRebuildSection', () => () => null); jest.mock('../../src/renderer/components/settings/EmbeddingBehaviorSection', () => () => (
Embedding Behavior Section
@@ -599,6 +606,41 @@ describe('SettingsPanel regression guards', () => { expect(screen.getByText('Diagnostics Logs Section')).toBeInTheDocument(); }); + test('warns instead of reporting success when a vision model installs without its projector', async () => { + const { eventsIpc } = require('../../src/renderer/services/ipc'); + let emitProgress = null; + eventsIpc.onOperationProgress.mockImplementationOnce((callback) => { + emitProgress = callback; + return () => {}; + }); + + render(); + await screen.findByText('Local AI Engine Section'); + fireEvent.click(screen.getByRole('button', { name: 'Advanced' })); + fireEvent.click(await screen.findByTestId('add-vision-model')); + await waitFor(() => expect(emitProgress).toBeInstanceOf(Function)); + mockAddNotification.mockClear(); + + act(() => { + emitProgress({ + type: 'model-download-complete', + model: 'vision.gguf', + success: false, + degraded: true, + progress: { + percent: 100, + companion: { name: 'mmproj-vision.gguf', success: false, error: 'Download timeout' } + } + }); + }); + + expect(mockAddNotification).toHaveBeenCalledWith( + expect.stringContaining('mmproj-vision.gguf'), + 'warning' + ); + expect(mockAddNotification).not.toHaveBeenCalledWith(expect.anything(), 'success'); + }); + // Note: The previous "surfaces backend save error details when saving // fails" test exercised the explicit "Save Settings" button. After the // 2026-04-29 UI optimization removed the button in favor of auto-save diff --git a/test/llamaIpc.handlers.test.js b/test/llamaIpc.handlers.test.js index 2aabf79..f1c7136 100644 --- a/test/llamaIpc.handlers.test.js +++ b/test/llamaIpc.handlers.test.js @@ -550,6 +550,92 @@ describe('llama IPC – extended handlers', () => { }) ); }); + + describe('completion event', () => { + const registerWithWindow = () => { + mockHandlers.clear(); + const win = { isDestroyed: () => false, webContents: {} }; + registerLlamaIpc({ + ipcMain: {}, + IPC_CHANNELS: { + LLAMA: { + GET_MODELS: 'llama:get-models', + GET_CONFIG: 'llama:get-config', + UPDATE_CONFIG: 'llama:update-config', + TEST_CONNECTION: 'llama:test-connection', + DOWNLOAD_MODEL: 'llama:download-model', + DELETE_MODEL: 'llama:delete-model', + GET_DOWNLOAD_STATUS: 'llama:get-download-status', + CANCEL_DOWNLOAD: 'llama:cancel-download', + WARM: 'llama:warm' + } + }, + logger: mockLogger, + systemAnalytics: {}, + getMainWindow: () => win + }); + return win; + }; + const completionEvents = () => + safeSend.mock.calls + .filter( + ([, channel, payload]) => + channel === IPC_EVENTS.OPERATION_PROGRESS && + payload?.type === 'model-download-complete' + ) + .map(([, , payload]) => payload); + const flushDownloadPromise = async () => { + for (let i = 0; i < 5; i += 1) await Promise.resolve(); + }; + + test('reports a vision model whose projector failed to download as degraded', async () => { + registerWithWindow(); + const companionDownload = { + name: 'mmproj-vision.gguf', + success: false, + alreadyPresent: false, + error: 'Download timeout', + downloadedBytes: 0, + totalBytes: 1024 + }; + mockDownloadManager.downloadModel.mockResolvedValueOnce({ + success: true, + degraded: true, + companionDownload + }); + + const result = await getHandler('download-model')({}, 'vision.gguf'); + await flushDownloadPromise(); + + expect(result).toEqual({ success: true, started: true }); + expect(completionEvents()).toEqual([ + expect.objectContaining({ + model: 'vision.gguf', + success: false, + degraded: true, + progress: expect.objectContaining({ percent: 100, companion: companionDownload }) + }) + ]); + }); + + test('reports a complete download as a plain success', async () => { + registerWithWindow(); + mockDownloadManager.downloadModel.mockResolvedValueOnce({ + success: true, + degraded: false, + companionDownload: null + }); + + await getHandler('download-model')({}, 'text.gguf'); + await flushDownloadPromise(); + + const events = completionEvents(); + expect(events).toHaveLength(1); + expect(events[0]).toMatchObject({ model: 'text.gguf', success: true }); + expect(events[0].degraded).toBeUndefined(); + expect(events[0].progress.companion).toBeUndefined(); + }); + }); }); describe('DELETE_MODEL', () => { From 4ef6bf2243c503d13d9e2f84d2424a223a145c26 Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:41 -0500 Subject: [PATCH 10/26] fix(vision): images go to the one-request runtime one at a time --- src/main/services/VisionService.js | 55 ++++++++++++++ test/VisionService.test.js | 116 +++++++++++++++++++++++++++++ 2 files changed, 171 insertions(+) diff --git a/src/main/services/VisionService.js b/src/main/services/VisionService.js index 8ef48e9..9091a41 100644 --- a/src/main/services/VisionService.js +++ b/src/main/services/VisionService.js @@ -1055,6 +1055,10 @@ class VisionService { * @type {Set<{ manager: any }>} */ this._requestHolds = new Set(); + // Settles when the last request that asked for a turn is over. See + // _waitForRequestTurn. + /** @type {Promise} */ + this._requestTurn = Promise.resolve(); /** @type {{ manager: any, type: string } | null} */ this._memoryRegistration = null; /** @type {any} */ @@ -1205,6 +1209,53 @@ class VisionService { }; } + /** + * Wait until the requests ahead of this one are over. + * + * The runtime is started with `--parallel 1`, while the vision inference + * slot can admit two or three images on a large GPU. A second request sent + * anyway waited inside the runtime with its timeout already running, and + * the restart after one request's timeout cut the connection of the other, + * which failed without a retry. One request at a time here means a restart + * never has another request on the runtime. + * + * A caller that gives up while it waits leaves at once and never reaches + * the runtime. + * @param {AbortSignal} [signal] + * @returns {Promise<() => void>} ends the turn + */ + async _waitForRequestTurn(signal) { + const ahead = this._requestTurn; + /** @type {() => void} */ + let endTurn = () => {}; + /** @type {Promise} */ + const mine = new Promise((resolve) => { + endTurn = resolve; + }); + this._requestTurn = ahead.then(() => mine); + if (!signal) { + await ahead; + return endTurn; + } + /** @type {() => void} */ + let onAbort = () => {}; + try { + await Promise.race([ + ahead, + new Promise((_resolve, reject) => { + onAbort = () => reject(createVisionAbortError()); + signal.addEventListener('abort', onAbort, { once: true }); + }) + ]); + } catch (error) { + endTurn(); + throw error; + } finally { + signal.removeEventListener('abort', onAbort); + } + return endTurn; + } + _extractDecodePressureHint() { const recent = String(this._recentStderr || ''); if (!recent) return ''; @@ -2054,7 +2105,10 @@ class VisionService { // read) in which the runtime was taken from under the image that had // started it, and the image fell back to OCR. const endRequest = this._beginRequest(); + /** @type {(() => void) | null} */ + let endTurn = null; try { + endTurn = await this._waitForRequestTurn(signal); return await this._analyzeImageWithRuntime({ imageBase64, imagePath, @@ -2067,6 +2121,7 @@ class VisionService { config }); } finally { + endTurn?.(); endRequest(); // Every way out re-arms the idle timer cleared above. A failure between // the runtime starting and the request going out (image file gone, a diff --git a/test/VisionService.test.js b/test/VisionService.test.js index d94dad5..9aeec56 100644 --- a/test/VisionService.test.js +++ b/test/VisionService.test.js @@ -567,6 +567,122 @@ describe('VisionService', () => { expect(service.hasInFlightRequests()).toBe(false); }); + describe('two images at once on the one-request runtime', () => { + /** + * Requests stay open until the test ends them, as on a runtime that + * is still working. `sent` lists them in the order they went out. + */ + function heldRequests() { + const http = require('http'); + const sent = []; + const spy = jest.spyOn(http, 'request').mockImplementation((options, callback) => { + const req = new EventEmitter(); + const entry = { open: false }; + req.setTimeout = jest.fn(); + req.write = jest.fn(); + req.destroy = jest.fn((error) => { + entry.open = false; + if (error) req.emit('error', error); + }); + req.end = jest.fn(() => { + entry.open = true; + }); + entry.timeOut = () => { + entry.open = false; + req.emit('error', new Error('Vision request timed out')); + }; + entry.answer = (content) => { + entry.open = false; + const res = new EventEmitter(); + res.statusCode = 200; + callback(res); + res.emit('data', JSON.stringify({ choices: [{ message: { content } }] })); + res.emit('end'); + }; + sent.push(entry); + return req; + }); + return { sent, spy }; + } + + const turn = () => new Promise((resolve) => setImmediate(resolve)); + + test('a request that times out does not restart the runtime under the other image', async () => { + await startReadyServer(); + const { sent, spy } = heldRequests(); + /** Requests on the wire each time the runtime was killed. */ + const openAtKill = []; + const runningProcess = () => { + const proc = exitingProcess(); + proc.kill = jest.fn(() => { + openAtKill.push(sent.filter((entry) => entry.open).length); + }); + return proc; + }; + service._process = runningProcess(); + // The restart: the next image finds no runtime and starts one. + service._ensureServer = jest.fn(async () => { + if (!service._process) { + service._process = runningProcess(); + service._port = 4321; + } + }); + + try { + const first = service.analyzeImage(image).catch((error) => error); + const second = service.analyzeImage(image).catch((error) => error); + await turn(); + expect(service.hasInFlightRequests()).toBe(true); + + sent[0].timeOut(); + expect((await first).message).toBe('Vision request timed out'); + + for (let i = 0; i < 5 && sent.length < 2; i += 1) await turn(); + sent[sent.length - 1].answer('a cat'); + + expect(openAtKill).toEqual([0]); + expect(await second).toEqual({ response: 'a cat' }); + expect(service.hasInFlightRequests()).toBe(false); + } finally { + spy.mockRestore(); + } + }); + + test('an image cancelled while it waits its turn leaves the runtime and the other image alone', async () => { + await startReadyServer(); + service._ensureServer = jest.fn().mockResolvedValue(undefined); + service.shutdown = jest.fn().mockResolvedValue(undefined); + const { sent, spy } = heldRequests(); + const controller = new AbortController(); + + try { + const first = service.analyzeImage(image); + const second = service + .analyzeImage({ ...image, signal: controller.signal }) + .catch((error) => error); + await turn(); + + controller.abort(); + const cancelled = await second; + expect(cancelled.name).toBe('AbortError'); + expect(sent).toHaveLength(1); + expect(service.shutdown).not.toHaveBeenCalled(); + + sent[0].answer('a dog'); + await expect(first).resolves.toEqual({ response: 'a dog' }); + + // The cancelled image gave its turn back: the next one runs. + const third = service.analyzeImage(image); + for (let i = 0; i < 5 && sent.length < 2; i += 1) await turn(); + sent[1].answer('a bird'); + await expect(third).resolves.toEqual({ response: 'a bird' }); + expect(service.hasInFlightRequests()).toBe(false); + } finally { + spy.mockRestore(); + } + }); + }); + test('a request that began before the runtime registered is held once and released once', async () => { const end = service._beginRequest(); expect(manager.acquireRef).not.toHaveBeenCalled(); From 2349cb3255c663bf9c9b54ca563ac68699ae6501 Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:41 -0500 Subject: [PATCH 11/26] fix(analysis): extension tokens, instruction-aware caches, archive keywords, fractional confidence --- src/main/analysis/documentAnalysis.js | 21 ++++++-- src/main/analysis/documentLlm.js | 10 ++-- src/main/analysis/fallbackUtils.js | 16 ++++++- src/main/analysis/imageAnalysis.js | 13 ++++- src/main/analysis/modelAnalysisResult.js | 7 ++- test/analysis-fallbackUtils.test.js | 46 ++++++++++++++++++ test/documentAnalysis.extractionGaps.test.js | 30 +++++++++++- test/documentLlm.extended.test.js | 50 +++++++++++++++++++- test/imageAnalysis.behavior.test.js | 40 +++++++++++++++- 9 files changed, 220 insertions(+), 13 deletions(-) diff --git a/src/main/analysis/documentAnalysis.js b/src/main/analysis/documentAnalysis.js index 318c877..f50880d 100644 --- a/src/main/analysis/documentAnalysis.js +++ b/src/main/analysis/documentAnalysis.js @@ -48,6 +48,7 @@ const { TEXT_TRUNCATION_MARKER } = require('./documentExtractors'); const { analyzeTextWithLlama, normalizeCategoryToSmartFolders } = require('./documentLlm'); +const { getOrganizationGuidance } = require('./organizationGuidance'); const { getAnalysisLimits } = require('./analysisLimits'); const { normalizeAnalysisResult } = require('./utils'); const { normalizeExtractedTextForStorage } = require('./analysisTextUtils'); @@ -344,12 +345,20 @@ async function analyzeDocumentFile(filePath, smartFolders = [], options = {}) { }); } + // The user's organization instructions shape the answer, so a result cached + // under other instructions must not be returned for this request. + const guidanceSig = crypto + .createHash('sha256') + .update(String((await getOrganizationGuidance()) || '')) + .digest('hex') + .slice(0, 16); + // Step 1: Attempt to compute file signature and check cache (non-fatal if fails) let fileSignature = null; let fileStats = null; try { fileStats = await fs.stat(filePath); - fileSignature = `${ANALYSIS_SIGNATURE_VERSION}|${modelName}|${smartFolderSig}|${filePath}|${fileStats.size}|${fileStats.mtimeMs}`; + fileSignature = `${ANALYSIS_SIGNATURE_VERSION}|${modelName}|${smartFolderSig}|${guidanceSig}|${filePath}|${fileStats.size}|${fileStats.mtimeMs}`; if (!bypassCache) { const cachedResult = getFileCache(fileSignature); if (cachedResult) { @@ -830,9 +839,14 @@ async function analyzeDocumentFile(filePath, smartFolders = [], options = {}) { } else if (SUPPORTED_ARCHIVE_EXTENSIONS.includes(fileExtension)) { // Archive metadata inspection (best-effort) const archiveInfo = await tryExtractArchiveMetadata(filePath); + // An archive with no readable listing has an empty list, not a missing one. + const listedKeywords = Array.isArray(archiveInfo.keywords) + ? archiveInfo.keywords.slice(0, TRUNCATION.KEYWORDS_MAX) + : []; const keywords = - archiveInfo.keywords?.slice(0, TRUNCATION.KEYWORDS_MAX) || - getIntelligentKeywords(fileName, fileExtension); + listedKeywords.length > 0 + ? listedKeywords + : getIntelligentKeywords(fileName, fileExtension); const category = normalizeCategoryToSmartFolders('archive', smartFolders); @@ -957,6 +971,7 @@ async function analyzeDocumentFile(filePath, smartFolders = [], options = {}) { contentHash, fileName, task: 'analyzeTextWithLlama', + guidance: guidanceSig, folders: Array.isArray(smartFolders) ? smartFolders.map((f) => f?.name || '').join(',') : '' diff --git a/src/main/analysis/documentLlm.js b/src/main/analysis/documentLlm.js index d7d38f4..22b3bb5 100644 --- a/src/main/analysis/documentLlm.js +++ b/src/main/analysis/documentLlm.js @@ -364,7 +364,11 @@ async function analyzeTextWithLlama( const namingContextKey = Array.isArray(namingContext) ? namingContext.slice(0, 5).join('|') : ''; - const cacheSeed = `${originalFileName || ''}|${fileDate || ''}|${namingContextKey}|${truncated}`; + // Global user organization instructions (empty string when unset). They go + // into the prompt, so they are part of the key: an answer given under other + // instructions is not an answer to this request. + const orgGuidance = String((await getOrganizationGuidance()) || ''); + const cacheSeed = `${originalFileName || ''}|${fileDate || ''}|${namingContextKey}|${orgGuidance}|${truncated}`; const cacheKey = cacheService.generateKey(cacheSeed, modelToUse, smartFolders); if (!bypassCache) { const cachedResult = cacheService.get(cacheKey); @@ -399,8 +403,7 @@ async function analyzeTextWithLlama( const fileDateContext = fileDate ? `\nDocument File Date: ${fileDate}` : ''; - // Global user organization instructions (empty string when unset). - const orgGuidanceStr = formatOrganizationGuidanceBlock(await getOrganizationGuidance()); + const orgGuidanceStr = formatOrganizationGuidanceBlock(orgGuidance); // Build naming context string if available let namingContextStr = ''; @@ -452,6 +455,7 @@ ${truncated} contentLength: truncated.length, // Additional uniqueness for truncated content text: truncated, model: modelToUse, + guidance: orgGuidance, folders: smartFolders .map((f) => f?.name || '') .filter(Boolean) diff --git a/src/main/analysis/fallbackUtils.js b/src/main/analysis/fallbackUtils.js index 50c8018..ef7d071 100644 --- a/src/main/analysis/fallbackUtils.js +++ b/src/main/analysis/fallbackUtils.js @@ -43,6 +43,18 @@ const normalizeCategoryToSmartFolders = return match?.name || uncategorized?.name || folders[0]?.name || 'Uncategorized'; }; +/** + * Whether a folder description names the extension as a word of its own + * (".stl", "stl files"), not as letters inside a longer word ("ai" in "email"). + * @param {string} descLower - Lowercased folder description + * @param {string} extNoDot - Lowercased extension without its dot + * @returns {boolean} + */ +function descriptionListsExtension(descLower, extNoDot) { + const escaped = extNoDot.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + return new RegExp(`(?= 2) { // Match extension with or without dot (e.g., ".stl" or "stl") - if (descLower.includes(lowerExtension) || descLower.includes(extNoDot)) { + if (descriptionListsExtension(descLower, extNoDot)) { score += 15; // High score for explicit extension match in description } } @@ -394,7 +406,9 @@ function getIntelligentCategory(fileName, extension, smartFolders = []) { '.rar': 'Archives', '.7z': 'Archives', '.tar': 'Archives', + '.tgz': 'Archives', '.gz': 'Archives', + '.bz2': 'Archives', '.mp3': 'Music', '.wav': 'Music', '.flac': 'Music', diff --git a/src/main/analysis/imageAnalysis.js b/src/main/analysis/imageAnalysis.js index 52beeb8..2cbda8d 100644 --- a/src/main/analysis/imageAnalysis.js +++ b/src/main/analysis/imageAnalysis.js @@ -452,7 +452,8 @@ async function analyzeImageWithLlama( } // Global user organization instructions (empty string when unset). - const orgGuidanceStr = formatOrganizationGuidanceBlock(await getOrganizationGuidance()); + const orgGuidance = String((await getOrganizationGuidance()) || ''); + const orgGuidanceStr = formatOrganizationGuidanceBlock(orgGuidance); // Build OCR grounding context if text was extracted const ocrGroundingStr = @@ -505,6 +506,7 @@ Rules: fileName: originalFileName, imageHash, model: modelToUse, + guidance: orgGuidance, folders: safeFolders .map((f) => f?.name || '') .filter(Boolean) @@ -1417,7 +1419,14 @@ async function runImageAnalysis(filePath, smartFolders, options) { logger.debug(`Image file size`, { bytes: stats.size }); // Cache quick path: signature based on file stats, before reading bytes. - const signature = `${IMAGE_SIGNATURE_VERSION}|${visionModelName}|${smartFolderSig}|${filePath}|${stats.size}|${stats.mtimeMs}`; + // The user's organization instructions shape the answer, so a result cached + // under other instructions must not be returned for this request. + const guidanceSig = crypto + .createHash('sha256') + .update(String((await getOrganizationGuidance()) || '')) + .digest('hex') + .slice(0, 16); + const signature = `${IMAGE_SIGNATURE_VERSION}|${visionModelName}|${smartFolderSig}|${guidanceSig}|${filePath}|${stats.size}|${stats.mtimeMs}`; if (!bypassCache) { const cached = getImageAnalysisCache().get(signature); if (cached != null) { diff --git a/src/main/analysis/modelAnalysisResult.js b/src/main/analysis/modelAnalysisResult.js index 7c86632..6317141 100644 --- a/src/main/analysis/modelAnalysisResult.js +++ b/src/main/analysis/modelAnalysisResult.js @@ -121,7 +121,12 @@ function buildAnalysisFromModelJson(parsedJson, context) { } parsedJson.confidence = Math.min(95, calculatedConfidence); } else { - parsedJson.confidence = Math.round(rawConfidence); + // The prompt asks for 0-100, but models also answer with a 0-1 fraction. + // Same reading as the image route and normalizeConfidencePercent: a value + // in (0, 1] is a fraction. Rounding it as it stands would leave 0 or 1. + const percentConfidence = + rawConfidence > 0 && rawConfidence <= 1 ? rawConfidence * 100 : rawConfidence; + parsedJson.confidence = Math.round(percentConfidence); } // Build validated result object diff --git a/test/analysis-fallbackUtils.test.js b/test/analysis-fallbackUtils.test.js index a4fd86a..b910762 100644 --- a/test/analysis-fallbackUtils.test.js +++ b/test/analysis-fallbackUtils.test.js @@ -101,6 +101,8 @@ describe('Analysis Fallback Utils', () => { test('returns Archives for archive files', () => { expect(getIntelligentCategory('files.zip', '.zip')).toBe('Archives'); + expect(getIntelligentCategory('files.tgz', '.tgz')).toBe('Archives'); + expect(getIntelligentCategory('files.bz2', '.bz2')).toBe('Archives'); }); test('returns Documents for unknown extensions', () => { @@ -191,6 +193,50 @@ describe('Analysis Fallback Utils', () => { const folders = [{ name: 'Financial' }, { name: 'Documents' }, { name: 'Uncategorized' }]; expect(getIntelligentCategory('invoice_2024.pdf', '.pdf', folders)).toBe('Financial'); }); + + describe('an extension listed in a folder description', () => { + test('letters inside a longer word are not the extension', () => { + // "ai" is in "email", "details" and "training"; "mov" is in "removed". + expect( + getIntelligentCategory('logo.ai', '.ai', [ + { name: 'Correspondence', description: 'Email details and training' } + ]) + ).not.toBe('Correspondence'); + expect( + getIntelligentCategory('clip.mov', '.mov', [ + { name: 'Cleanup', description: 'Removed items' } + ]) + ).not.toBe('Cleanup'); + expect( + getIntelligentCategory('run.sh', '.sh', [ + { name: 'Money', description: 'Shipping and cash' } + ]) + ).not.toBe('Money'); + }); + + test('the extension as its own word still matches, with or without the dot', () => { + expect( + getIntelligentCategory('part.xyz', '.xyz', [ + { name: 'Lab', description: 'Instrument output: .xyz, .abc' } + ]) + ).toBe('Lab'); + expect( + getIntelligentCategory('part.xyz', '.xyz', [ + { name: 'Lab', description: 'xyz files from the instrument' } + ]) + ).toBe('Lab'); + expect( + getIntelligentCategory('part.xyz', '.xyz', [ + { name: 'Lab', description: 'Everything saved as XYZ.' } + ]) + ).toBe('Lab'); + expect( + getIntelligentCategory('main.c++', '.c++', [ + { name: 'Code', description: 'Sources (.c++/.h++)' } + ]) + ).toBe('Code'); + }); + }); }); }); diff --git a/test/documentAnalysis.extractionGaps.test.js b/test/documentAnalysis.extractionGaps.test.js index 9baee25..c067621 100644 --- a/test/documentAnalysis.extractionGaps.test.js +++ b/test/documentAnalysis.extractionGaps.test.js @@ -99,7 +99,8 @@ jest.mock('../src/main/analysis/embeddingQueue/stageQueues', () => ({ jest.mock('../src/main/utils/llmOptimization', () => ({ globalDeduplicator: { dedupe: jest.fn((key, fn) => fn()), - deduplicate: jest.fn((key, fn) => fn()) + deduplicate: jest.fn((key, fn) => fn()), + generateKey: jest.fn(() => 'dedup-key') } })); jest.mock('../src/main/analysis/namingContextBm25', () => ({ @@ -109,6 +110,12 @@ jest.mock('../src/main/analysis/analysisLimits', () => ({ getAnalysisLimits: jest.fn(async () => ({ maxTextFileSize: 16 })) })); +const mockGetOrganizationGuidance = jest.fn(async () => ''); +jest.mock('../src/main/analysis/organizationGuidance', () => ({ + ...jest.requireActual('../src/main/analysis/organizationGuidance'), + getOrganizationGuidance: () => mockGetOrganizationGuidance() +})); + const extractors = require('../src/main/analysis/documentExtractors'); const cad = require('../src/main/analysis/extractors/cadDesignExtractors'); const notebook = require('../src/main/analysis/extractors/notebookExtractor'); @@ -122,6 +129,7 @@ describe('documentAnalysis dispatch for the restored file types', () => { beforeEach(() => { jest.clearAllMocks(); documentLlm.analyzeTextWithLlama.mockResolvedValue(LLM_OK); + mockGetOrganizationGuidance.mockResolvedValue(''); }); test.each([ @@ -208,6 +216,8 @@ describe('documentAnalysis dispatch for the restored file types', () => { expect(result.extractionMethod).toBe('archive'); expect(result.purpose).toMatch(/content listing unavailable/); + // No entry names to draw on: the generic archive keywords stand in. + expect(result.keywords).toEqual(expect.arrayContaining(['archive', 'tgz'])); }); test('.rar keeps the bare archive summary (no permissively licensed reader)', async () => { @@ -215,6 +225,7 @@ describe('documentAnalysis dispatch for the restored file types', () => { expect(archives.describeArchive).not.toHaveBeenCalled(); expect(result.purpose).toBe('RAR archive'); + expect(result.keywords).toEqual(expect.arrayContaining(['archive', 'rar'])); }); test('an oversize text file is analyzed from its head instead of failing as too large', async () => { @@ -232,6 +243,23 @@ describe('documentAnalysis dispatch for the restored file types', () => { expect(result.partialContent).toBe(true); }); + test('an unchanged file is analyzed again once the organization instructions change', async () => { + fs.mkdirSync('/guided', { recursive: true }); + fs.writeFileSync('/guided/notes.txt', 'short'); + extractors.readTextFileWithEncoding.mockResolvedValue('short note content here'); + + mockGetOrganizationGuidance.mockResolvedValue('Keep notes in Journal.'); + await analyzeDocumentFile('/guided/notes.txt', []); + await analyzeDocumentFile('/guided/notes.txt', []); + // Same instructions: the second run comes from the file cache. + expect(documentLlm.analyzeTextWithLlama).toHaveBeenCalledTimes(1); + + mockGetOrganizationGuidance.mockResolvedValue('Keep notes in Work.'); + await analyzeDocumentFile('/guided/notes.txt', []); + + expect(documentLlm.analyzeTextWithLlama).toHaveBeenCalledTimes(2); + }); + test('a text file within the limit is read whole', async () => { fs.mkdirSync('/small', { recursive: true }); fs.writeFileSync('/small/notes.txt', 'short'); diff --git a/test/documentLlm.extended.test.js b/test/documentLlm.extended.test.js index eac35ef..fa8b79c 100644 --- a/test/documentLlm.extended.test.js +++ b/test/documentLlm.extended.test.js @@ -83,6 +83,12 @@ jest.mock('../src/main/services/FolderMatchingService', () => ({ })) })); +const mockGetOrganizationGuidance = jest.fn(async () => ''); +jest.mock('../src/main/analysis/organizationGuidance', () => ({ + ...jest.requireActual('../src/main/analysis/organizationGuidance'), + getOrganizationGuidance: () => mockGetOrganizationGuidance() +})); + const { analyzeTextWithLlama } = require('../src/main/analysis/documentLlm'); describe('documentLlm – extended', () => { @@ -96,7 +102,10 @@ describe('documentLlm – extended', () => { mockLlamaService.listModels.mockResolvedValue([{ name: 'test-model.gguf' }]); mockLlamaService.generateText.mockResolvedValue({ response: '{}' }); // Reset cache to miss by default - mockCacheService.get.mockReturnValue(null); + mockCacheService.get.mockReset().mockReturnValue(null); + mockCacheService.set.mockReset(); + mockCacheService.generateKey.mockReset().mockReturnValue('cache-key'); + mockGetOrganizationGuidance.mockResolvedValue(''); }); // ─── JSON repair fallback ────────────────────────────────── @@ -219,6 +228,20 @@ describe('documentLlm – extended', () => { expect(result.confidence).toBe(88); }); + test.each([ + [0.9, 90], + [0.45, 45], + [1, 100], + [0, 0] + ])('reads a fractional confidence of %p as %p percent', async (fromModel, expected) => { + mockLlamaService.generateText.mockResolvedValue({ + response: JSON.stringify({ category: 'documents', keywords: ['doc'], confidence: fromModel }) + }); + + const result = await analyzeTextWithLlama('Text', 'doc.pdf', []); + expect(result.confidence).toBe(expected); + }); + test('overrides out-of-range LLM confidence', async () => { mockLlamaService.generateText.mockResolvedValue({ response: JSON.stringify({ @@ -444,6 +467,31 @@ describe('documentLlm – extended', () => { expect(stored._embeddingForPersistence).toBeUndefined(); }); + test('changed organization instructions are not answered from the cache', async () => { + // A working cache keyed on what the code under test hands to generateKey. + const store = new Map(); + mockCacheService.generateKey.mockImplementation((seed, model) => `${model}|${seed}`); + mockCacheService.get.mockImplementation((key) => store.get(key) ?? null); + mockCacheService.set.mockImplementation((key, value) => store.set(key, value)); + mockLlamaService.generateText.mockResolvedValue({ + response: JSON.stringify({ category: 'Finance', keywords: ['a'], confidence: 80 }) + }); + + mockGetOrganizationGuidance.mockResolvedValue('Keep tax papers in Finance.'); + await analyzeTextWithLlama('Some invoice text', 'invoice.pdf', []); + await analyzeTextWithLlama('Some invoice text', 'invoice.pdf', []); + // Same instructions: the second run is a cache hit. + expect(mockLlamaService.generateText).toHaveBeenCalledTimes(1); + + mockGetOrganizationGuidance.mockResolvedValue('Put every invoice in Clients.'); + await analyzeTextWithLlama('Some invoice text', 'invoice.pdf', []); + + expect(mockLlamaService.generateText).toHaveBeenCalledTimes(2); + expect(mockLlamaService.generateText.mock.calls[1][0].prompt).toContain( + 'Put every invoice in Clients.' + ); + }); + test('the cache stores a copy, not the object handed back to the caller', async () => { mockCacheService.get.mockReturnValue(null); mockLlamaService.generateText.mockResolvedValue({ diff --git a/test/imageAnalysis.behavior.test.js b/test/imageAnalysis.behavior.test.js index 719e13c..2779391 100644 --- a/test/imageAnalysis.behavior.test.js +++ b/test/imageAnalysis.behavior.test.js @@ -182,6 +182,12 @@ jest.mock('../src/shared/folderUtils', () => ({ findContainingSmartFolder: jest.fn() })); +const mockGetOrganizationGuidance = jest.fn(async () => ''); +jest.mock('../src/main/analysis/organizationGuidance', () => ({ + ...jest.requireActual('../src/main/analysis/organizationGuidance'), + getOrganizationGuidance: () => mockGetOrganizationGuidance() +})); + describe('Image Analysis Behavior', () => { let fs; let mockLlamaService; @@ -195,7 +201,9 @@ describe('Image Analysis Behavior', () => { // Get fresh mock references mockCacheInstance = require('../src/main/services/AnalysisCacheService').getImageAnalysisCache(); - mockCacheInstance.get.mockReturnValue(null); // Cache miss by default + mockCacheInstance.get.mockReset().mockReturnValue(null); // Cache miss by default + mockCacheInstance.set.mockReset(); + mockGetOrganizationGuidance.mockResolvedValue(''); // Access nested mock: getServices() -> returns object with matcher -> matcher has embedText mockMatcherInstance = require('../src/main/analysis/semanticFolderMatcher').getServices() @@ -495,6 +503,36 @@ describe('Image Analysis Behavior', () => { expect(mockLlamaService.analyzeImage).not.toHaveBeenCalled(); }); + test('an unchanged image is analyzed again once the organization instructions change', async () => { + // A working cache keyed on the signature the code under test builds. + const store = new Map(); + mockCacheInstance.get.mockImplementation((key) => store.get(key) ?? null); + mockCacheInstance.set.mockImplementation((key, value) => store.set(key, value)); + mockLlamaService.analyzeImage.mockResolvedValue({ + response: JSON.stringify({ + category: 'Finance', + keywords: ['receipt'], + confidence: 90, + suggestedName: 'tax_receipt', + has_text: false + }) + }); + + mockGetOrganizationGuidance.mockResolvedValue('Keep receipts in Finance.'); + await analyzeImageFile('/path/to/guided.png'); + await analyzeImageFile('/path/to/guided.png'); + // Same instructions: the second run is a cache hit. + expect(mockLlamaService.analyzeImage).toHaveBeenCalledTimes(1); + + mockGetOrganizationGuidance.mockResolvedValue('Put every receipt in Clients.'); + await analyzeImageFile('/path/to/guided.png'); + + expect(mockLlamaService.analyzeImage).toHaveBeenCalledTimes(2); + expect(mockLlamaService.analyzeImage.mock.calls[1][0].prompt).toContain( + 'Put every receipt in Clients.' + ); + }); + test('handles AI engine unavailability gracefully', async () => { mockLlamaService.testConnection.mockResolvedValue({ success: false }); mockLlamaService.supportsVisionInput.mockResolvedValue(false); From 519402697c4cdd9b1f65ba11bc5fea8972655bb6 Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:41 -0500 Subject: [PATCH 12/26] fix(watcher): app moves keep history, Explorer renames are matched, re-enable during stop sticks --- src/main/services/DownloadWatcher.js | 25 ++++ src/main/services/SmartFolderWatcher.js | 61 +++++++- test/DownloadWatcher.lifecycle.test.js | 26 ++++ test/SmartFolderWatcher.coverage.test.js | 173 +++++++++++++++++++++++ 4 files changed, 282 insertions(+), 3 deletions(-) diff --git a/src/main/services/DownloadWatcher.js b/src/main/services/DownloadWatcher.js index 32744e2..3c5aacb 100644 --- a/src/main/services/DownloadWatcher.js +++ b/src/main/services/DownloadWatcher.js @@ -129,6 +129,8 @@ class DownloadWatcher { this.downloadsPath = null; this.isStarting = false; this._startPromise = null; // Mutex: concurrent start() callers await the same promise + /** @type {Promise | null} the teardown in flight, awaited by start() */ + this._teardownPromise = null; this.restartAttempts = 0; this.maxRestartAttempts = RETRY.MAX_ATTEMPTS_MEDIUM; this.restartDelay = DEBOUNCE.WATCHER_RESTART_BASE; @@ -202,6 +204,16 @@ class DownloadWatcher { } async start() { + // A teardown keeps this.watcher set until it has drained the downloads in + // flight. Wait for it, or this call reports the dying watcher as running + // and nothing is watched afterwards. + if (this._teardownPromise) { + while (this._teardownPromise) { + await this._teardownPromise.catch(/** @returns {void} */ () => undefined); + } + // That teardown was the app shutting down: stay stopped. + if (this._shutdownRequested) return; + } if (this.watcher) { logger.debug('[DOWNLOAD-WATCHER] Watcher already running'); return; @@ -534,6 +546,19 @@ class DownloadWatcher { * @private */ async _teardown() { + const tearingDown = this._doTeardown(); + this._teardownPromise = tearingDown; + try { + await tearingDown; + } finally { + if (this._teardownPromise === tearingDown) this._teardownPromise = null; + } + } + + /** + * @private + */ + async _doTeardown() { this._stopped = true; // A start() still in flight either sees _stopped and aborts, or lands its diff --git a/src/main/services/SmartFolderWatcher.js b/src/main/services/SmartFolderWatcher.js index 4f6c040..7ac3956 100644 --- a/src/main/services/SmartFolderWatcher.js +++ b/src/main/services/SmartFolderWatcher.js @@ -54,6 +54,7 @@ const { } = require('../../shared/performanceConstants'); const { findUserExclusion } = require('./watcherExclusions'); const { getInstance: getFileOperationTracker } = require('../../shared/fileOperationTracker'); +const { isGhostCleanupDeferred } = require('../../shared/ghostCleanupGuard'); const { normalizePathForIndex, getCanonicalFileId, @@ -198,7 +199,10 @@ const QUEUE_DROP_LOG_INTERVAL_MS = 10_000; const QUEUE_PERSIST_DEBOUNCE_MS = 1000; const QUEUE_PERSIST_DEBOUNCE_LARGE_MS = 10_000; const QUEUE_PERSIST_LARGE_THRESHOLD = 2000; -const MOVE_DETECTION_WINDOW_MS = 3000; +// How long a deletion waits for its matching `add` beyond the time that add +// needs to arrive: chokidar holds it for stabilityThreshold (checked on a +// 200 ms poll) and _handleFileEvent then debounces it. +const MOVE_DETECTION_MARGIN_MS = 2000; const MOVE_MTIME_TOLERANCE_MS = 2000; // File extensions that have dedicated parsers/AI analysis paths. @@ -326,6 +330,8 @@ class SmartFolderWatcher { // stop() has returned and cleared _isStopping (its in-flight wait is // bounded, and an aborted analysis may unwind later than that). this._stopEpoch = 0; + /** @type {Promise | null} the stop() in flight, awaited by start() */ + this._stopPromise = null; // Restart machinery for chokidar 'error' recovery. Without this the // previous behavior was: log + bump errors counter, isRunning stays @@ -407,7 +413,11 @@ class SmartFolderWatcher { const envDepth = parseInt(process.env.STRATOSORT_WATCHER_DEPTH, 10); this.watcherDepth = Number.isFinite(envDepth) && envDepth >= 0 && envDepth <= 32 ? envDepth : 10; - this._moveDetectionWindowMs = MOVE_DETECTION_WINDOW_MS; + // Must outlast stabilityThreshold + debounceDelay: with a shorter window + // the deletion was finalized before the renamed file's add was looked at, + // so every rename in Explorer was a delete plus a full re-analysis. + this._moveDetectionWindowMs = + this.stabilityThreshold + this.debounceDelay + MOVE_DETECTION_MARGIN_MS; this._moveMtimeToleranceMs = MOVE_MTIME_TOLERANCE_MS; // Sync concurrency from settings (non-blocking) @@ -469,7 +479,9 @@ class SmartFolderWatcher { // graceful shutdown to await. const wantsDisabled = settings.smartFolderWatchEnabled === false; const shouldStop = wantsDisabled && (this.isRunning || this.isStarting); - const shouldStart = !wantsDisabled && !this.isRunning && !this.isStarting; + // A watcher that is still stopping counts as stopped: isRunning stays true + // until stop() has closed it, and start() waits for that stop. + const shouldStart = !wantsDisabled && !this.isStarting && (!this.isRunning || this._isStopping); if (shouldStop || shouldStart) { const toggle = shouldStop ? this.stop() : this.start(); this._watchToggleWork = Promise.resolve(toggle).catch((error) => { @@ -581,6 +593,16 @@ class SmartFolderWatcher { * @returns {Promise} True if started successfully */ async start() { + // stop() leaves isRunning true until it has closed the watcher. Wait for + // it, or this call reports the dying watcher as running and nothing is + // watched afterwards. + if (this._stopPromise) { + while (this._stopPromise) { + await this._stopPromise.catch(() => {}); + } + // That stop was the app shutting down: stay stopped. + if (this._shutdownRequested) return false; + } if (this.isRunning) { logger.debug('[SMART-FOLDER-WATCHER] Already running'); return true; @@ -878,6 +900,20 @@ class SmartFolderWatcher { * FIX MED-6: Clean up pending move detection timeouts to prevent callbacks after stop */ async stop() { + const stopping = this._doStop(); + this._stopPromise = stopping; + try { + await stopping; + } finally { + if (this._stopPromise === stopping) this._stopPromise = null; + } + } + + /** + * Internal stop implementation + * @private + */ + async _doStop() { logger.info('[SMART-FOLDER-WATCHER] Stopping...'); this._isStopping = true; this._stopEpoch++; @@ -3800,6 +3836,25 @@ class SmartFolderWatcher { return; } + // An unlink is also what a move out of the folder by the app looks like + // (organize, undo/redo). The file still exists at its new path, and its + // history entry still matches this one through originalPath, so cleanup + // here would delete the analysis of a live file. + if (isGhostCleanupDeferred()) { + logger.debug( + '[SMART-FOLDER-WATCHER] Skipping deletion cleanup: batch organize in progress', + filePath + ); + return; + } + if (getFileOperationTracker().wasRecentlyOperated(filePath)) { + logger.debug( + '[SMART-FOLDER-WATCHER] Skipping deletion cleanup: recently moved by app', + filePath + ); + return; + } + logger.info('[SMART-FOLDER-WATCHER] Detected external file deletion:', filePath); try { diff --git a/test/DownloadWatcher.lifecycle.test.js b/test/DownloadWatcher.lifecycle.test.js index 2a380f6..86f5fb9 100644 --- a/test/DownloadWatcher.lifecycle.test.js +++ b/test/DownloadWatcher.lifecycle.test.js @@ -253,6 +253,32 @@ describe('DownloadWatcher lifecycle', () => { expect(watcher.watcher).toBeNull(); expect(watcher._stopped).toBe(false); }); + + test('start() during a pending stop() leaves a live watcher', async () => { + const watcher = makeWatcher(); + await startAndSettle(watcher); + + // stop() drains the download being processed before it closes chokidar. + let releasePipeline; + const pipeline = new Promise((resolve) => { + releasePipeline = resolve; + }); + watcher._inFlightHandles.add(pipeline); + + const stopping = watcher.stop(); + await flushMicrotasks(); + // Auto-organize is turned on again before the stop has finished. + const starting = watcher.start(); + await flushMicrotasks(); + + watcher._inFlightHandles.delete(pipeline); + releasePipeline(); + await flushMicrotasks(); + await Promise.all([stopping, starting]); + + expect(chokidar.watch).toHaveBeenCalledTimes(2); + expect(watcher.watcher).toBe(mockWatchers[1]); + }); }); describe('_handleFileDeletion gates', () => { diff --git a/test/SmartFolderWatcher.coverage.test.js b/test/SmartFolderWatcher.coverage.test.js index 933289e..9a9fe1f 100644 --- a/test/SmartFolderWatcher.coverage.test.js +++ b/test/SmartFolderWatcher.coverage.test.js @@ -191,6 +191,8 @@ const fs = require('fs').promises; const chokidar = require('chokidar'); const SmartFolderWatcher = require('../src/main/services/SmartFolderWatcher'); const { recordAnalysisResult } = require('../src/main/ipc/analysisUtils'); +const { getInstance: getFileOperationTracker } = require('../src/shared/fileOperationTracker'); +const { deferGhostCleanup, resumeGhostCleanup } = require('../src/shared/ghostCleanupGuard'); describe('SmartFolderWatcher', () => { let watcher; @@ -444,6 +446,177 @@ describe('SmartFolderWatcher', () => { expect(watcher._watchToggleWork).toBeUndefined(); expect(watcher.isRunning).toBe(true); }); + + describe('re-enabling while stop() is still running', () => { + /** Holds the next stop() on its queue flush until the returned function is called. */ + const holdStop = () => { + let release; + watcher._flushQueuePersist = jest + .fn() + .mockResolvedValue(undefined) + .mockImplementationOnce( + () => + new Promise((resolve) => { + release = resolve; + }) + ); + return () => release(); + }; + + test('the watcher is running again once the stop has finished', async () => { + await watcher.start(); + const releaseStop = holdStop(); + + watcher.applySettings({ smartFolderWatchEnabled: false }); + const stopWork = watcher._watchToggleWork; + expect(watcher._isStopping).toBe(true); + + watcher.applySettings({ smartFolderWatchEnabled: true }); + const startWork = watcher._watchToggleWork; + + releaseStop(); + await stopWork; + await startWork; + + expect(watcher.isRunning).toBe(true); + expect(watcher.watcher).not.toBeNull(); + }); + + test('start() waits for the stop instead of reporting the dying watcher as running', async () => { + await watcher.start(); + const dyingWatcher = watcher.watcher; + const releaseStop = holdStop(); + + const stopWork = watcher.stop(); + const startWork = watcher.start(); + + releaseStop(); + await stopWork; + await expect(startWork).resolves.toBe(true); + + expect(watcher.isRunning).toBe(true); + expect(watcher.watcher).not.toBeNull(); + expect(watcher.watcher).not.toBe(dyingWatcher); + }); + + test('a disable saved after the re-enable still wins', async () => { + await watcher.start(); + const releaseStop = holdStop(); + + watcher.applySettings({ smartFolderWatchEnabled: false }); + const stopWork = watcher._watchToggleWork; + watcher.applySettings({ smartFolderWatchEnabled: true }); + const startWork = watcher._watchToggleWork; + // The user turns it off again before the stop has finished. + mockDeps.settingsService.load.mockResolvedValue({ smartFolderWatchEnabled: false }); + + releaseStop(); + await stopWork; + await startWork; + + expect(watcher.isRunning).toBe(false); + expect(watcher.watcher).toBeNull(); + }); + }); + }); + + describe('_handleFileDeletion: a move made by the app is not a deletion', () => { + const movedOut = '/docs/moved-by-app.pdf'; + + beforeEach(() => { + mockDeps.analysisHistoryService.getAnalysisByPath = jest + .fn() + .mockResolvedValue({ fileSize: 100, lastModified: 1000 }); + mockDeps.analysisHistoryService.removeEntriesByPath = jest.fn().mockResolvedValue(); + }); + + test('an unlink of a path the app just moved keeps its history and vectors', async () => { + getFileOperationTracker.mockReturnValueOnce({ + wasRecentlyOperated: jest.fn((p) => p === movedOut), + recordOperation: jest.fn() + }); + const finalize = jest.spyOn(watcher, '_finalizeDeletion'); + + await watcher._handleFileDeletion(movedOut); + + expect(watcher._pendingMoveCandidates.size).toBe(0); + expect(finalize).not.toHaveBeenCalled(); + expect(mockDeps.analysisHistoryService.removeEntriesByPath).not.toHaveBeenCalled(); + }); + + test('an unlink during a batch organize keeps its history and vectors', async () => { + const finalize = jest.spyOn(watcher, '_finalizeDeletion'); + + deferGhostCleanup(); + try { + await watcher._handleFileDeletion(movedOut); + } finally { + resumeGhostCleanup(); + } + + expect(watcher._pendingMoveCandidates.size).toBe(0); + expect(finalize).not.toHaveBeenCalled(); + expect(mockDeps.analysisHistoryService.removeEntriesByPath).not.toHaveBeenCalled(); + }); + + test('a deletion made outside the app is still cleaned up', async () => { + // No history entry to match a move against, so cleanup runs at once. + mockDeps.analysisHistoryService.getAnalysisByPath.mockResolvedValue(null); + + await watcher._handleFileDeletion('/docs/deleted-in-explorer.pdf'); + + expect(mockDeps.analysisHistoryService.removeEntriesByPath).toHaveBeenCalledWith( + '/docs/deleted-in-explorer.pdf' + ); + }); + }); + + describe('move detection window', () => { + test('outlasts the wait before the matching add is looked at', () => { + // chokidar holds an add for stabilityThreshold; the watcher then + // debounces it before it looks for a pending deletion to pair it with. + expect(watcher._moveDetectionWindowMs).toBeGreaterThan( + watcher.stabilityThreshold + watcher.debounceDelay + ); + }); + + test('a rename in Explorer moves the analysis instead of deleting it', async () => { + const oldPath = '/docs/report.pdf'; + const newPath = '/docs/report-final.pdf'; + mockDeps.analysisHistoryService.getAnalysisByPath = jest + .fn() + .mockResolvedValue({ fileSize: 100, lastModified: 1000 }); + mockDeps.analysisHistoryService.removeEntriesByPath = jest.fn().mockResolvedValue(); + const atomicPathUpdate = jest.fn().mockResolvedValue({ success: true }); + watcher.filePathCoordinator = { atomicPathUpdate }; + fs.access.mockResolvedValue(undefined); + + jest.useFakeTimers({ doNotFake: ['nextTick'] }); + try { + // chokidar publishes the unlink at once... + await watcher._handleFileDeletion(oldPath); + expect(watcher._pendingMoveCandidates.size).toBe(1); + + // ...and the add only after the file was stable for stabilityThreshold + // (checked on a 200 ms poll). + await jest.advanceTimersByTimeAsync(watcher.stabilityThreshold + 200); + watcher._handleFileEvent('add', newPath, { + size: 100, + mtimeMs: 1000, + mtime: new Date(1000) + }); + await jest.advanceTimersByTimeAsync(watcher.debounceDelay); + for (let i = 0; i < 5; i++) { + await jest.advanceTimersByTimeAsync(0); + } + + expect(atomicPathUpdate).toHaveBeenCalledWith(oldPath, newPath, { type: 'move' }); + expect(mockDeps.analysisHistoryService.removeEntriesByPath).not.toHaveBeenCalled(); + expect(watcher._pendingMoveCandidates.size).toBe(0); + } finally { + jest.useRealTimers(); + } + }); }); describe('updateWatchedFolders', () => { From f6f6e520a2f606fcea594fc43c785dc7ea755aef Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:41 -0500 Subject: [PATCH 13/26] fix(lifecycle): a second quit waits for cleanup and a failed check keeps Update ready --- src/main/core/autoUpdater.js | 29 ++++++++++------- src/main/core/lifecycle.js | 15 +++++++-- test/autoUpdater.test.js | 63 +++++++++++++++++++++++++++++++++++- test/lifecycle.test.js | 62 +++++++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+), 14 deletions(-) diff --git a/src/main/core/autoUpdater.js b/src/main/core/autoUpdater.js index 26c7b25..5dc9266 100644 --- a/src/main/core/autoUpdater.js +++ b/src/main/core/autoUpdater.js @@ -362,14 +362,21 @@ function notifyRenderer(payload, getMainWindow) { */ function handleError(err, getMainWindow) { logger.error('[UPDATER] Error:', err); - hasDownloadedUpdate = false; - notifyRenderer( - { - status: 'error', - error: err?.message || 'Update error' - }, - getMainWindow - ); + // A failed check does not discard an installer that already finished + // downloading: electron-updater still installs it on quit. Keep the flag + // (lifecycle reads it at exit) and skip the error status, which would hide + // the renderer's "Update ready" button. + if (hasDownloadedUpdate) { + logger.info('[UPDATER] Keeping the downloaded update pending after the error'); + } else { + notifyRenderer( + { + status: 'error', + error: err?.message || 'Update error' + }, + getMainWindow + ); + } // Surface auto-update failures (network, signature mismatch, manifest // fetch failure, missing latest*.yml) on the DegradedStateBanner so a // user who is not actively looking at the update UI can still tell that @@ -556,8 +563,7 @@ async function initializeAutoUpdater(isDev, getMainWindow) { // the check failed. Previously we just logged, which meant the // UpdateIndicator UI sat in "checking" forever on network // outages / 404s / feed timeouts and the user had no signal the - // check had given up. handleError also resets - // `hasDownloadedUpdate` and emits a structured payload via + // check had given up. handleError emits a structured payload via // IPC_EVENTS.APP_UPDATE that the renderer already understands. handleError(e, getMainWindow); } finally { @@ -644,7 +650,8 @@ async function checkForUpdates() { } try { - hasDownloadedUpdate = false; + // hasDownloadedUpdate is left alone: update-available / update-not-available + // reset it when the check succeeds, and a failed check changes nothing. applyCustomUpdateFeedIfConfigured(); await getAutoUpdater().checkForUpdates(); return { success: true }; diff --git a/src/main/core/lifecycle.js b/src/main/core/lifecycle.js index 02d8b40..96740a4 100644 --- a/src/main/core/lifecycle.js +++ b/src/main/core/lifecycle.js @@ -582,6 +582,10 @@ async function handleBeforeQuit(/** @type {any} */ event) { * Handle window-all-closed event */ function handleWindowAllClosed() { + // A shutdown already running ends in its own app.exit(); the window closing + // meanwhile must not start a second quit. + if (_quitCleanupStarted) return; + // Check if background mode is enabled - if so, don't quit when windows are closed const settingsService = lifecycleConfig.getSettingsService?.(); const backgroundMode = settingsService?.get?.('backgroundMode'); @@ -982,14 +986,21 @@ function registerLifecycleHandlers(createWindow) { // Return cleanup functions for proper teardown return { cleanupAppListeners: () => { - app.removeListener('before-quit', handleBeforeQuit); + // handleBeforeQuit runs this cleanup in its first phase and exits through + // app.exit() only after the last one. The quit gate and the + // window-all-closed handler stay registered until then: without them a + // second Quit, or closing the window (Electron quits by default when + // nothing listens), would end the process while services still persist. + if (!_quitCleanupStarted) { + app.removeListener('before-quit', handleBeforeQuit); + app.removeListener('window-all-closed', handleWindowAllClosed); + } app.removeListener('will-quit', willQuitHandler); if (powerMonitor && typeof powerMonitor.removeListener === 'function') { powerMonitor.removeListener('shutdown', shutdownHandler); } detachLockPowerMonitor(); app.removeListener('quit', quitHandler); - app.removeListener('window-all-closed', handleWindowAllClosed); app.removeListener('activate', activateHandler); }, cleanupProcessListeners: () => { diff --git a/test/autoUpdater.test.js b/test/autoUpdater.test.js index 0329ce4..f4499d7 100644 --- a/test/autoUpdater.test.js +++ b/test/autoUpdater.test.js @@ -533,6 +533,65 @@ describe('autoUpdater', () => { // list that status, and strict safeSend blocked it with an error log on // every launch. Drive every path that sends APP_UPDATE and validate each // payload against the real schema so sender and contract cannot drift. + // electron-updater keeps a downloaded installer and still installs it on + // quit when a later check fails, so the app must keep saying so. + describe('a failed check after a completed download', () => { + function sentStatuses() { + const { safeSend } = require('../src/main/ipc/ipcWrappers'); + return safeSend.mock.calls.map((call) => call[2]?.status); + } + + async function initWithDownloadedUpdate() { + process.env.STRATOSORT_ENABLE_AUTO_UPDATES = 'true'; + const mockWindow = { isDestroyed: jest.fn().mockReturnValue(false), webContents: {} }; + mockGetAllWindows.mockReturnValue([mockWindow]); + + const updater = require('../src/main/core/autoUpdater'); + await updater.initializeAutoUpdater(false, jest.fn().mockReturnValue(mockWindow)); + mockAutoUpdaterHandlers.get('update-downloaded')(); + expect(updater.hasPendingDownloadedUpdate()).toBe(true); + return updater; + } + + testIfSupported('an updater error keeps the downloaded update pending and shown', async () => { + const updater = await initWithDownloadedUpdate(); + + mockAutoUpdaterHandlers.get('error')(new Error('feed returned 503')); + + expect(updater.hasPendingDownloadedUpdate()).toBe(true); + // The renderer hides "Update ready" on an error status. + const statuses = sentStatuses(); + expect(statuses.slice(statuses.lastIndexOf('downloaded'))).toEqual(['downloaded']); + // The failure is still reported on the degraded-state banner. + const flags = require('../src/main/services/DegradedStateTracker') + .getInstance() + .list() + .filter((flag) => flag.code === 'auto-updater.unavailable'); + expect(flags).toHaveLength(1); + }); + + testIfSupported('a manual check that fails keeps the downloaded update pending', async () => { + const updater = await initWithDownloadedUpdate(); + mockAutoUpdater.checkForUpdates.mockRejectedValueOnce(new Error('feed unreachable')); + + const result = await updater.checkForUpdates(); + + expect(result).toEqual({ success: false, error: 'feed unreachable' }); + expect(updater.hasPendingDownloadedUpdate()).toBe(true); + }); + + testIfSupported('a newer version found by a later check replaces the download', async () => { + const updater = await initWithDownloadedUpdate(); + + mockAutoUpdaterHandlers.get('update-available')(); + expect(updater.hasPendingDownloadedUpdate()).toBe(false); + + // With no finished download an error is reported to the renderer again. + mockAutoUpdaterHandlers.get('error')(new Error('download failed')); + expect(sentStatuses().at(-1)).toBe('error'); + }); + }); + describe('every app:update payload satisfies the event schema', () => { const setPlatform = (value) => Object.defineProperty(process, 'platform', { value, configurable: true }); @@ -587,9 +646,11 @@ describe('autoUpdater', () => { mockAutoUpdaterHandlers.get('update-available')(); mockAutoUpdaterHandlers.get('update-not-available')(); mockAutoUpdaterHandlers.get('download-progress')({ percent: 42.5 }); - mockAutoUpdaterHandlers.get('update-downloaded')(); + // Errors come before the download completes: once an update is + // downloaded, an error no longer sends a status. mockAutoUpdaterHandlers.get('error')(new Error('feed unreachable')); mockAutoUpdaterHandlers.get('error')(undefined); + mockAutoUpdaterHandlers.get('update-downloaded')(); const payloads = sentUpdatePayloads(); expect(payloads.map((p) => p.status)).toEqual( diff --git a/test/lifecycle.test.js b/test/lifecycle.test.js index 6f70614..421f6a8 100644 --- a/test/lifecycle.test.js +++ b/test/lifecycle.test.js @@ -879,6 +879,68 @@ describe('lifecycle – OS shutdown hooks (LC-07)', () => { ); }); + // The pre-cleanup phase runs every registered listener cleanup. If that + // unhooks the quit gate, a second Quit or a window close ends the process + // while services are still persisting. + test('a second quit or a window close during shutdown does not end the process early', async () => { + electron.app.removeListener.mockClear(); + electron.app.exit.mockClear(); + electron.app.quit.mockClear(); + + let finishServiceShutdown; + let serviceShutdownStarted; + const reachedServiceShutdown = new Promise((resolve) => { + serviceShutdownStarted = resolve; + }); + const serviceIntegration = { + shutdown: jest.fn(() => { + serviceShutdownStarted(); + return new Promise((resolve) => { + finishServiceShutdown = resolve; + }); + }) + }; + + const cleanup = lifecycle.registerLifecycleHandlers(jest.fn()); + lifecycle.initializeLifecycle({ + ...baseConfig(), + getServiceIntegration: () => serviceIntegration, + getEventListeners: () => [cleanup.cleanupAppListeners], + getGlobalProcessListeners: () => [cleanup.cleanupProcessListeners] + }); + const registered = (name) => + electron.app.on.mock.calls + .filter(([event]) => event === name) + .map(([, handler]) => handler) + .filter( + (handler) => + !electron.app.removeListener.mock.calls.some( + ([event, removed]) => event === name && removed === handler + ) + ); + + const [beforeQuit] = registered('before-quit'); + const firstQuit = beforeQuit({ preventDefault: jest.fn() }); + await reachedServiceShutdown; + + // Second Quit (menu, tray, Ctrl+Q): still held off. + const secondEvent = { preventDefault: jest.fn() }; + expect(registered('before-quit')).toHaveLength(1); + await registered('before-quit')[0](secondEvent); + expect(secondEvent.preventDefault).toHaveBeenCalled(); + + // Window X: Electron quits by default when nothing listens for + // window-all-closed, and the app's own handler must not ask to quit. + expect(registered('window-all-closed')).toHaveLength(1); + registered('window-all-closed')[0](); + expect(electron.app.quit).not.toHaveBeenCalled(); + expect(electron.app.exit).not.toHaveBeenCalled(); + + finishServiceShutdown(); + await firstQuit; + expect(electron.app.exit).toHaveBeenCalledTimes(1); + }); + test('attachSessionEndFlush runs the quit cleanup on the window session-end event', async () => { const window = { on: jest.fn(), removeListener: jest.fn() }; const detach = lifecycle.attachSessionEndFlush(window); From 6beef87229cba35fb1bc86ba8ef4e094dba6c74f Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:49:41 -0500 Subject: [PATCH 14/26] fix(search-ui): Timeline list measures its container and the hidden Atlas tab is inert --- .../components/search/TimelineTabContent.jsx | 13 +- .../components/search/UnifiedSearchModal.jsx | 2 + src/renderer/components/ui/Modal.jsx | 3 +- test/components/Modal.focusRestore.test.js | 55 ++++++++- test/components/UnifiedSearchModal.test.js | 27 +++++ .../search/TimelineTabContent.test.js | 114 ++++++++++++++++++ 6 files changed, 207 insertions(+), 7 deletions(-) create mode 100644 test/renderer/components/search/TimelineTabContent.test.js diff --git a/src/renderer/components/search/TimelineTabContent.jsx b/src/renderer/components/search/TimelineTabContent.jsx index e78f005..f204e96 100644 --- a/src/renderer/components/search/TimelineTabContent.jsx +++ b/src/renderer/components/search/TimelineTabContent.jsx @@ -5,7 +5,7 @@ * onOpenFile. Renders a virtualized list of year/month/undated headers and * item rows built by buildTimelineRows(). */ -import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import React, { useCallback, useEffect, useMemo, useState } from 'react'; import PropTypes from 'prop-types'; import { List as VirtualizedList } from 'react-window'; import { buildTimelineRows, formatTimelineDay } from '../../utils/timelineRows'; @@ -125,11 +125,14 @@ export default function TimelineTabContent({ loadMore, onOpenFile }) { - const containerRef = useRef(null); + // The measured container only exists while there are rows, so it is held in + // state: the observer attaches when the first page arrives and re-attaches + // after a refresh emptied the list. + const [containerEl, setContainerEl] = useState(/** @type {HTMLDivElement | null} */ (null)); const [height, setHeight] = useState(400); useEffect(() => { - const el = containerRef.current; + const el = containerEl; if (!el) return; const ro = new ResizeObserver((entries) => { const entry = entries[0]; @@ -140,7 +143,7 @@ export default function TimelineTabContent({ }); ro.observe(el); return () => ro.disconnect(); - }, []); + }, [containerEl]); const rows = useMemo(() => buildTimelineRows(items), [items]); @@ -173,7 +176,7 @@ export default function TimelineTabContent({ )} {rows.length > 0 && ( -
+
)} + {settingsLoadError && !settingsLoaded && ( +
+ + + +
+ )} {pendingExternalSettings && ( { }); }); + describe('a failed settings read', () => { + // The form starts on DEFAULT_SETTINGS. If the read from main fails and the + // panel carries on as if it had loaded, the next edit auto-saves the whole + // default form over every stored setting. + const stored = { + language: 'fr', + defaultSmartFolderLocation: 'C:/Stored', + experienceMode: 'advanced' + }; + + // The read settles on a later tick; let React apply what follows inside + // act so the assertions below do not depend on scheduler timing. + const settle = () => + act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + test('never lets an edit auto-save the default form over stored settings', async () => { + mockSettingsGet.mockRejectedValue(new Error('IPC timeout')); + + render(); + await settle(); + expect(mockSettingsGet).toHaveBeenCalled(); + expect(screen.queryByTestId('settings-loading-overlay')).not.toBeInTheDocument(); + + fireEvent.click(screen.getByTestId('local-edit')); + await act(async () => { + await mockAutoSaveCapture.current(); + }); + + expect(mockDebouncedAutoSave).not.toHaveBeenCalled(); + expect(mockSettingsSave).not.toHaveBeenCalled(); + // Defaults must not reach Redux either: updateSettings marks the store's + // settings as loaded, and the rest of the app would then read defaults. + expect(mockUpdateSettings).not.toHaveBeenCalled(); + }); + + test('says so, and "Try again" loads the stored settings', async () => { + mockSettingsGet.mockRejectedValueOnce(new Error('IPC timeout')); + mockSettingsGet.mockResolvedValue(stored); + + render(); + await settle(); + + const alert = screen.getByRole('alert'); + expect(alert).toHaveTextContent(/couldn't load your settings/i); + expect(mockAddNotification).toHaveBeenCalledWith( + expect.stringMatching(/couldn't load your settings/i), + 'error', + expect.anything(), + 'settings-load-error' + ); + + // Real Redux would hold the stored settings once the retry dispatches. + mockState = { ui: { settings: stored } }; + fireEvent.click(screen.getByRole('button', { name: 'Try again' })); + await settle(); + + await waitFor(() => expect(screen.queryByRole('alert')).not.toBeInTheDocument()); + expect(mockSettingsGet).toHaveBeenCalledTimes(2); + expect(mockUpdateSettings).toHaveBeenCalledWith(stored); + + // Auto-save works again, and saves on top of the stored values. + fireEvent.click(screen.getByTestId('local-edit')); + expect(mockDebouncedAutoSave).toHaveBeenCalled(); + await act(async () => { + await mockAutoSaveCapture.current(); + }); + expect(mockSettingsSave).toHaveBeenCalledWith({ ...stored, language: 'local' }); + }); + + test('an edit made while the read was failing is kept and saved after a retry', async () => { + mockSettingsGet.mockRejectedValueOnce(new Error('IPC timeout')); + mockSettingsGet.mockResolvedValue(stored); + + render(); + await settle(); + expect(screen.getByRole('alert')).toBeInTheDocument(); + + fireEvent.click(screen.getByTestId('local-edit')); + expect(mockDebouncedAutoSave).not.toHaveBeenCalled(); + + mockState = { ui: { settings: stored } }; + fireEvent.click(screen.getByRole('button', { name: 'Try again' })); + await settle(); + await waitFor(() => expect(screen.queryByRole('alert')).not.toBeInTheDocument()); + + // Only the key the user changed goes on top of what was stored. + await waitFor(() => expect(mockDebouncedAutoSave).toHaveBeenCalled()); + await act(async () => { + await mockAutoSaveCapture.current(); + }); + expect(mockSettingsSave).toHaveBeenCalledWith({ ...stored, language: 'local' }); + }); + }); + describe('showLiveActivity belongs to the live-activity thunks, not the panel', () => { // Ctrl/Cmd+Shift+L, the Diagnostics switch and the Live Activity panel's // Close button each save just that key (liveActivityThunks). The panel From 8bebb8930932ecf1c6988c42bfa5cd20b3e519fb Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 13:57:12 -0500 Subject: [PATCH 20/26] fix(chat): scope drop, reopened answers, retry keeps the question, large scopes are saved --- src/main/services/ChatHistoryStore.js | 5 +- src/main/services/chat/SessionMemory.js | 32 ++++- src/renderer/components/search/ChatPanel.jsx | 29 ++-- .../components/search/DocumentScopePanel.jsx | 19 ++- .../components/search/hooks/useChatTab.js | 71 ++++++---- test/ChatHistoryStore.validation.test.js | 17 ++- test/ChatService.sessions.test.js | 10 +- test/SessionMemory.test.js | 88 ++++++++++++ test/components/ChatPanel.test.js | 51 +++++++ test/components/DocumentScopePanel.test.js | 130 ++++++++++++++++++ .../search/useChatTab.scope.test.js | 125 +++++++++++++++++ 11 files changed, 527 insertions(+), 50 deletions(-) create mode 100644 test/components/DocumentScopePanel.test.js create mode 100644 test/renderer/components/search/useChatTab.scope.test.js diff --git a/src/main/services/ChatHistoryStore.js b/src/main/services/ChatHistoryStore.js index 2f10331..69a00f6 100644 --- a/src/main/services/ChatHistoryStore.js +++ b/src/main/services/ChatHistoryStore.js @@ -45,7 +45,10 @@ const SEARCH_DECRYPT_SCAN_CAP = 500; */ const MAX_CONVERSATION_TITLE = 512; const MAX_MESSAGE_TEXT = 64_000; -const MAX_DOCUMENT_SCOPE_ITEMS = 200; +// Matches chatQuerySchema.documentScopeItems (validationSchemas.js). A lower +// limit here made the first save of an accepted request throw, so the whole +// conversation was never stored. +const MAX_DOCUMENT_SCOPE_ITEMS = 1_000; const MAX_DOCUMENT_SCOPE_ITEM_BYTES = 16_384; const MAX_MESSAGE_SOURCES_ITEMS = 1_000; const MAX_MESSAGE_FOLLOWUPS_ITEMS = 50; diff --git a/src/main/services/chat/SessionMemory.js b/src/main/services/chat/SessionMemory.js index 19ef852..5414369 100644 --- a/src/main/services/chat/SessionMemory.js +++ b/src/main/services/chat/SessionMemory.js @@ -128,7 +128,9 @@ class SessionMemory { if (this.chatHistoryStore && memory?.sessionId) { try { const conv = this.chatHistoryStore.getConversation(memory.sessionId); - if (!conv) return ''; + // No stored conversation: a turn that could not be persisted lives in + // the in-memory buffer, so follow-ups still read their history there. + if (!conv) return this._getInMemoryHistoryText(memory); const messages = conv.messages.slice(-(this.memoryWindow * 2)); return messages .map((/** @type {any} */ m) => { @@ -169,6 +171,11 @@ class SessionMemory { } } + return this._getInMemoryHistoryText(memory); + } + + /** @private */ + async _getInMemoryHistoryText(/** @type {any} */ memory) { try { const vars = await memory.loadMemoryVariables({}); return typeof vars?.history === 'string' ? vars.history : ''; @@ -208,10 +215,13 @@ class SessionMemory { // Persist user turn — unless this is a regenerate, where the original // user row already exists and re-inserting duplicates it on reload. - if (!skipUserMessage) { + // Retry after a failed request sends the same flag although nothing + // was saved, so the skip is honoured only when the row is really there. + const userText = typeof input === 'string' ? input : ''; + if (!skipUserMessage || !this._isLastUserMessage(conv, userText)) { this.chatHistoryStore.addMessage(memory.sessionId, { role: 'user', - text: typeof input === 'string' ? input : '' + text: userText }); } @@ -260,6 +270,22 @@ class SessionMemory { } } + /** + * Whether the conversation's most recent user row is this question. + * @private + * @param {any} conv Stored conversation, or null when none exists yet. + * @param {string} userText + * @returns {boolean} + */ + _isLastUserMessage(conv, userText) { + const messages = Array.isArray(conv?.messages) ? conv.messages : []; + for (let i = messages.length - 1; i >= 0; i -= 1) { + if (messages[i]?.role !== 'user') continue; + return String(messages[i].text ?? '').trim() === userText.trim(); + } + return false; + } + /** * Remap per-turn `doc-N` ids to conversation-scoped FIRST-SEEN ordinals so * the same document keeps its number across turns. Pre-fix, the retrieval diff --git a/src/renderer/components/search/ChatPanel.jsx b/src/renderer/components/search/ChatPanel.jsx index 7047af8..74ae311 100644 --- a/src/renderer/components/search/ChatPanel.jsx +++ b/src/renderer/components/search/ChatPanel.jsx @@ -274,18 +274,23 @@ function stripUnresolvedCitations(text, sources) { } function getAssistantDisplayText(message) { - if (typeof message?.text === 'string' && message.text.trim()) { - // Streaming path: the finished message swaps `.text` for the structured, - // source-reconciled answer, so this branch only renders mid-stream text. - return stripUnresolvedCitations( - sanitizeAnswerText(message.text, message.sources), - message.sources - ); - } - - return getPrimaryAnswerItems(message) - .map((item) => item.text) - .join('\n\n'); + const hasText = typeof message?.text === 'string' && message.text.trim(); + // A finished message renders its structured, source-reconciled answer. A + // message reopened from history still carries `.text` (the prompt-memory + // string, with model text and a "[Referenced: ...]" line), so `.text` is + // shown only mid-stream or when there is no structured answer. + const structuredText = + hasText && message.isStreaming + ? '' + : getPrimaryAnswerItems(message) + .map((item) => item.text) + .join('\n\n'); + if (structuredText || !hasText) return structuredText; + + return stripUnresolvedCitations( + sanitizeAnswerText(message.text, message.sources), + message.sources + ); } const LONG_THREAD_MESSAGE_THRESHOLD = 45; diff --git a/src/renderer/components/search/DocumentScopePanel.jsx b/src/renderer/components/search/DocumentScopePanel.jsx index 3677ee8..5e490b6 100644 --- a/src/renderer/components/search/DocumentScopePanel.jsx +++ b/src/renderer/components/search/DocumentScopePanel.jsx @@ -4,6 +4,7 @@ import { X, FileText, Folder, Plus, FolderInput, ChevronRight } from 'lucide-rea import { Button, IconButton } from '../ui'; import { Text } from '../ui/Typography'; import { formatDisplayPath } from '../../utils/pathDisplay'; +import { resolveDroppedFilePath } from '../../utils/dragAndDrop'; import { useSelector } from 'react-redux'; import { selectRedactPaths } from '../../store/selectors'; @@ -81,10 +82,22 @@ export default function DocumentScopePanel({ const droppedFiles = e.dataTransfer?.files; if (droppedFiles && droppedFiles.length > 0) { - const files = Array.from(droppedFiles).map((f) => ({ - path: f.path, + // File-kind items line up with dataTransfer.files; their entries are + // the only synchronous way to tell a dropped folder from a file. + const entries = Array.from(e.dataTransfer?.items || []) + .filter((item) => item?.kind === 'file') + .map((item) => { + try { + return item.webkitGetAsEntry?.() || null; + } catch { + return null; + } + }); + const files = Array.from(droppedFiles).map((f, index) => ({ + // File.path is gone in modern Electron; the preload bridge resolves it. + path: resolveDroppedFilePath(f), name: f.name, - type: 'file' + type: entries[index]?.isDirectory ? 'folder' : 'file' })); onAddToScope(files); } diff --git a/src/renderer/components/search/hooks/useChatTab.js b/src/renderer/components/search/hooks/useChatTab.js index 116a06d..5659884 100644 --- a/src/renderer/components/search/hooks/useChatTab.js +++ b/src/renderer/components/search/hooks/useChatTab.js @@ -528,6 +528,8 @@ export function useChatTab({ isOpen, searchResults, defaultTopK, openFile, setEr const itemPath = typeof item?.path === 'string' ? item.path.trim() : ''; const isFolder = item?.type === 'folder'; if (isFolder) { + // A folder whose path could not be resolved cannot be scanned. + if (!/[\\/]/.test(itemPath)) continue; try { const result = await window.electronAPI.files.getDirectoryContents(itemPath); if (result.success && result.files) { @@ -550,8 +552,14 @@ export function useChatTab({ isOpen, searchResults, defaultTopK, openFile, setEr } try { - const result = await window.electronAPI.embeddings.findFilesByPaths(allPaths); - const directMatches = result?.success && result.results ? result.results : []; + const result = + allPaths.length > 0 + ? await window.electronAPI.embeddings.findFilesByPaths(allPaths) + : { success: true, results: [] }; + if (result?.success === false) { + throw new Error(result.error || 'Index lookup failed'); + } + const directMatches = result?.results ? result.results : []; const nameMatches = []; for (const unresolvedName of unresolvedNames) { @@ -576,36 +584,40 @@ export function useChatTab({ isOpen, searchResults, defaultTopK, openFile, setEr } const combined = [...directMatches, ...nameMatches]; - if (combined.length > 0) { - const newItems = combined - .map((r) => ({ - id: r.id, - path: r.path || getMetadataPath(r, r?.id) || '', - name: r.name || getMetadataName(r, r.path || getMetadataPath(r, r?.id), r?.id) || '', - type: r.type || r?.metadata?.type || 'file' - })) - .filter((item) => item.id && item.path); - const dedupedNewItems = Array.from( - new Map(newItems.map((item) => [item.id, item])).values() - ); - - const resolvedPaths = new Set(dedupedNewItems.map((i) => i.path)); - const droppedPaths = allPaths.filter((p) => !resolvedPaths.has(p)); - const unresolvedFoundByName = new Set( - dedupedNewItems.map((item) => item.name?.toLowerCase()).filter(Boolean) + const newItems = combined + .map((r) => ({ + id: r.id, + path: r.path || getMetadataPath(r, r?.id) || '', + name: r.name || getMetadataName(r, r.path || getMetadataPath(r, r?.id), r?.id) || '', + type: r.type || r?.metadata?.type || 'file' + })) + .filter((item) => item.id && item.path); + const dedupedNewItems = Array.from(new Map(newItems.map((item) => [item.id, item])).values()); + + const resolvedPaths = new Set(dedupedNewItems.map((i) => i.path)); + const droppedPaths = allPaths.filter((p) => !resolvedPaths.has(p)); + const unresolvedFoundByName = new Set( + dedupedNewItems.map((item) => item.name?.toLowerCase()).filter(Boolean) + ); + const droppedNames = unresolvedNames.filter( + (name) => !unresolvedFoundByName.has(name.toLowerCase()) + ); + const droppedEntries = [...droppedPaths, ...droppedNames]; + // Warn even when nothing resolved, so adding to the scope is never a + // silent no-op. + if (droppedEntries.length > 0) { + const visibleNames = droppedEntries.map((p) => p.split(/[/\\]/).pop()).slice(0, 5); + const suffix = droppedEntries.length > 5 ? ` and ${droppedEntries.length - 5} more` : ''; + setChatWarning( + `${droppedEntries.length} file(s) not in index: ${visibleNames.join(', ')}${suffix}. Index these files first.` ); - const droppedNames = unresolvedNames.filter( - (name) => !unresolvedFoundByName.has(name.toLowerCase()) + } else if (dedupedNewItems.length === 0 && items.length > 0) { + setChatWarning( + 'No files were added to the document scope. The items could not be read, or the folder has no files.' ); - const droppedEntries = [...droppedPaths, ...droppedNames]; - if (droppedEntries.length > 0) { - const visibleNames = droppedEntries.map((p) => p.split(/[/\\]/).pop()).slice(0, 5); - const suffix = droppedEntries.length > 5 ? ` and ${droppedEntries.length - 5} more` : ''; - setChatWarning( - `${droppedEntries.length} file(s) not in index: ${visibleNames.join(', ')}${suffix}. Index these files first.` - ); - } + } + if (dedupedNewItems.length > 0) { setDocumentScope((prev) => { const existingIds = new Set(prev.map((i) => i.id)); const uniqueNew = dedupedNewItems.filter((i) => !existingIds.has(i.id)); @@ -614,6 +626,7 @@ export function useChatTab({ isOpen, searchResults, defaultTopK, openFile, setEr } } catch (e) { logger.error('Failed to resolve scope files', e); + setChatWarning('Could not check those files against the index. Please try again.'); } }, []); diff --git a/test/ChatHistoryStore.validation.test.js b/test/ChatHistoryStore.validation.test.js index 27f58f8..681e263 100644 --- a/test/ChatHistoryStore.validation.test.js +++ b/test/ChatHistoryStore.validation.test.js @@ -46,7 +46,7 @@ describe('ChatHistoryStore — input size validation', () => { test('exposes finite numeric limits', () => { expect(CHAT_HISTORY_LIMITS.MAX_CONVERSATION_TITLE).toBe(512); expect(CHAT_HISTORY_LIMITS.MAX_MESSAGE_TEXT).toBe(64_000); - expect(CHAT_HISTORY_LIMITS.MAX_DOCUMENT_SCOPE_ITEMS).toBe(200); + expect(CHAT_HISTORY_LIMITS.MAX_DOCUMENT_SCOPE_ITEMS).toBe(1000); expect(CHAT_HISTORY_LIMITS.MAX_MESSAGE_PAYLOAD_BYTES).toBe(256_000); }); @@ -63,6 +63,21 @@ describe('ChatHistoryStore — input size validation', () => { expect(stmt.run).toHaveBeenCalled(); }); + test('accepts the largest document scope a chat request may carry', () => { + // chatQuerySchema (validationSchemas.js) allows 1000 documentScopeItems. + // A lower limit here made the first save of such a chat throw, so the + // conversation was never stored. + const { store, stmt } = buildStore(); + const scope = Array.from({ length: 1000 }, (_, i) => ({ + id: `file:${i}`, + path: `/synthetic/docs/${i}.pdf`, + name: `${i}.pdf`, + type: 'file' + })); + expect(() => store.createConversation('Big scope', scope, 'big')).not.toThrow(); + expect(stmt.run).toHaveBeenCalled(); + }); + test('rejects title over MAX_CONVERSATION_TITLE chars', () => { const { store, stmt } = buildStore(); const huge = 'x'.repeat(CHAT_HISTORY_LIMITS.MAX_CONVERSATION_TITLE + 1); diff --git a/test/ChatService.sessions.test.js b/test/ChatService.sessions.test.js index e80e2bf..69fe99e 100644 --- a/test/ChatService.sessions.test.js +++ b/test/ChatService.sessions.test.js @@ -124,7 +124,15 @@ describe('ChatService session management', () => { test('_saveMemoryTurn skips the user row on regenerate (skipUserMessage)', async () => { const store = { initialize: jest.fn().mockResolvedValue(undefined), - getConversation: jest.fn(() => ({ id: 'regen', title: 'Existing' })), + // The skip applies only when the question row is really stored. + getConversation: jest.fn(() => ({ + id: 'regen', + title: 'Existing', + messages: [ + { role: 'user', text: 'same question again' }, + { role: 'assistant', text: 'first answer' } + ] + })), createConversation: jest.fn(), addMessage: jest.fn() }; diff --git a/test/SessionMemory.test.js b/test/SessionMemory.test.js index e2819ec..3d07e79 100644 --- a/test/SessionMemory.test.js +++ b/test/SessionMemory.test.js @@ -95,6 +95,94 @@ describe('SessionMemory', () => { expect(history).not.toContain('[doc-1]'); }); + /** In-memory stand-in for ChatHistoryStore: conversations keyed by id. */ + function createFakeStore({ failCreate = false } = {}) { + /** @type {Map} */ + const conversations = new Map(); + return { + conversations, + getConversation: jest.fn((id) => conversations.get(id) || null), + createConversation: jest.fn((title, documentScope, id) => { + if (failCreate) throw new Error('Document scope exceeds limit'); + conversations.set(id, { id, title, documentScope, messages: [] }); + }), + addMessage: jest.fn((id, message) => { + conversations.get(id).messages.push(message); + }) + }; + } + + test('retry after a failed first attempt saves the question with the answer', async () => { + // The failed attempt persisted nothing, so there is no user row to reuse. + const store = createFakeStore(); + const memory = new SessionMemory({ chatHistoryStore: store }); + const session = await memory.getSessionMemory('retry-new'); + + await memory.saveMemoryTurn( + session, + 'What is in the report?', + { text: 'A summary.' }, + { skipUserMessage: true } + ); + + expect(store.conversations.get('retry-new').messages.map((m) => [m.role, m.text])).toEqual([ + ['user', 'What is in the report?'], + ['assistant', 'A summary.'] + ]); + }); + + test('retry of a failed follow-up saves the follow-up question', async () => { + const store = createFakeStore(); + const memory = new SessionMemory({ chatHistoryStore: store }); + const session = await memory.getSessionMemory('retry-followup'); + await memory.saveMemoryTurn(session, 'First question', { text: 'First answer' }); + + await memory.saveMemoryTurn( + session, + 'And the second one?', + { text: 'Second answer' }, + { skipUserMessage: true } + ); + + const rows = store.conversations.get('retry-followup').messages.map((m) => [m.role, m.text]); + expect(rows).toEqual([ + ['user', 'First question'], + ['assistant', 'First answer'], + ['user', 'And the second one?'], + ['assistant', 'Second answer'] + ]); + expect(await memory.getHistoryText(session)).toContain('User: And the second one?'); + }); + + test('regenerate does not duplicate a question that is already saved', async () => { + const store = createFakeStore(); + const memory = new SessionMemory({ chatHistoryStore: store }); + const session = await memory.getSessionMemory('regen'); + await memory.saveMemoryTurn(session, 'Same question', { text: 'First answer' }); + + await memory.saveMemoryTurn( + session, + 'Same question', + { text: 'Regenerated answer' }, + { skipUserMessage: true } + ); + + const roles = store.conversations.get('regen').messages.map((m) => m.role); + expect(roles).toEqual(['user', 'assistant', 'assistant']); + }); + + test('follow-ups keep their history when a turn could not be saved', async () => { + const store = createFakeStore({ failCreate: true }); + const memory = new SessionMemory({ chatHistoryStore: store }); + const session = await memory.getSessionMemory('unsaved'); + + await memory.saveMemoryTurn(session, 'First question', { text: 'First answer' }); + + const history = await memory.getHistoryText(session); + expect(history).toContain('User: First question'); + expect(history).toContain('Assistant: First answer'); + }); + test('delete removes the session from the LRU', async () => { const memory = new SessionMemory(); await memory.getSessionMemory('gone'); diff --git a/test/components/ChatPanel.test.js b/test/components/ChatPanel.test.js index 483cf2b..0c985c5 100644 --- a/test/components/ChatPanel.test.js +++ b/test/components/ChatPanel.test.js @@ -179,6 +179,57 @@ describe('ChatPanel', () => { ).toBeInTheDocument(); }); + test('a reopened conversation shows the same answer as the live one, not the stored memory text', () => { + // A saved assistant row keeps `text` as the prompt-memory string (document + // and model items joined, plus a "[Referenced: ...]" line) next to the + // structured answer. The bubble must render the structured answer. + const { container } = renderWithRedux( +
- {requiredModelsMissing.length > 0 && ( + {requiredModelsMissing.length > 0 && !modelListUnknown && ( Required models are missing. Download them to enable AI features. @@ -1202,7 +1385,15 @@ function ModelSetupWizardBody({ onComplete, onSkip, onDismiss, onStepChange }) { onClick={startDownloads} variant="primary" className="flex-1" - disabled={!selectedModels.embedding || !selectedModels.text || !hasApi || isRefreshing} + disabled={ + !selectedModels.embedding || + !selectedModels.text || + !hasApi || + isRefreshing || + // Downloading again what may already be on disk helps nobody + // while the engine that would load it cannot start. + modelListUnknown + } > Download models @@ -1542,7 +1733,13 @@ function ModelSelector({ const displayName = modelInfo?.displayName || filename; const modelSize = getModelSize(filename); const isInstalled = status === 'ready' || status === 'complete'; - const statusLabel = isInstalled ? 'Installed' : status === 'failed' ? 'Failed' : 'Not installed'; + const statusLabel = isInstalled + ? 'Installed' + : status === 'failed' + ? 'Failed' + : status === 'unknown' + ? 'Not checked' + : 'Not installed'; const statusClass = isInstalled ? 'bg-stratosort-success/10 text-stratosort-success' : status === 'failed' diff --git a/test/components/ModelSetupWizard.test.js b/test/components/ModelSetupWizard.test.js index ee6330a..d3fa8ed 100644 --- a/test/components/ModelSetupWizard.test.js +++ b/test/components/ModelSetupWizard.test.js @@ -882,6 +882,317 @@ describe('ModelSetupWizard', () => { }); }); +describe('ModelSetupWizard download status it cannot trust yet', () => { + const baseProfile = INSTALL_MODEL_PROFILES?.BASE_SMALL?.models || {}; + const embeddingModel = baseProfile.EMBEDDING; + const textModel = baseProfile.TEXT_ANALYSIS; + const visionModel = baseProfile.IMAGE_ANALYSIS; + const allActive = [embeddingModel, textModel, visionModel].map((filename) => ({ + filename, + progress: 1 + })); + + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + delete window.electronAPI; + }); + + /** Let pending promises settle, then move the clock. */ + async function advance(ms = 0) { + await act(async () => { + await jest.advanceTimersByTimeAsync(ms); + }); + } + + async function renderOnChooseStep() { + render(); + await advance(); + expect(screen.getByText(/Choose AI Models/i)).toBeInTheDocument(); + } + + test('a status read that arrives before main has registered the download is not "stopped"', async () => { + // Main answers the status question from memory at once, while the + // download it was just asked for is still behind a few disk checks. + let registered = false; + window.electronAPI = { + llama: { + getConfig: jest.fn(async () => ({ embeddingModel, textModel, visionModel })), + getModels: jest.fn(async () => ({ models: [] })), + getDownloadStatus: jest.fn(async () => ({ + status: { downloads: registered ? allActive : [] } + })), + downloadModel: jest.fn(async () => ({ success: true, started: true })) + }, + events: { onOperationProgress: jest.fn(() => jest.fn()) } + }; + + await renderOnChooseStep(); + fireEvent.click(screen.getByRole('button', { name: /Download Models/i })); + await advance(); + + expect(window.electronAPI.llama.downloadModel).toHaveBeenCalledTimes(3); + expect(screen.getByText(/Downloading Models/i)).toBeInTheDocument(); + expect(screen.queryByText(/downloads stopped/i)).not.toBeInTheDocument(); + + registered = true; + await advance(4100); + expect(screen.getByText(/Downloading Models/i)).toBeInTheDocument(); + expect(screen.queryByText(/downloads stopped/i)).not.toBeInTheDocument(); + }); + + test('downloads that never show up are still reported as stopped, and a retry clears the message', async () => { + let registered = false; + window.electronAPI = { + llama: { + getConfig: jest.fn(async () => ({ embeddingModel, textModel, visionModel })), + getModels: jest.fn(async () => ({ models: [] })), + getDownloadStatus: jest.fn(async () => ({ + status: { downloads: registered ? allActive : [] } + })), + downloadModel: jest.fn(async () => ({ success: true, started: true })) + }, + events: { onOperationProgress: jest.fn(() => jest.fn()) } + }; + + await renderOnChooseStep(); + fireEvent.click(screen.getByRole('button', { name: /Download Models/i })); + await advance(); + await advance(12500); + + expect(screen.getByText(/Choose AI Models/i)).toBeInTheDocument(); + expect( + screen.getByText('Required model downloads stopped. Please retry downloads.') + ).toBeInTheDocument(); + + // The retry starts real downloads: the old message must not sit above them. + registered = true; + fireEvent.click(screen.getByRole('button', { name: /Download Models/i })); + await advance(); + expect(screen.getByText(/Downloading Models/i)).toBeInTheDocument(); + expect(screen.queryByText(/downloads stopped/i)).not.toBeInTheDocument(); + }); + + test('an engine that failed to start is named, and its empty model list is not read as "nothing installed"', async () => { + const downloadModel = jest.fn(async () => ({ success: true, started: true })); + window.electronAPI = { + llama: { + getConfig: jest.fn(async () => ({ + success: false, + error: 'Synthetic engine load failure', + code: 'LLAMA_ERROR' + })), + getModels: jest.fn(async () => ({ + models: [], + categories: { text: [], vision: [], embedding: [] }, + selected: {}, + error: 'Synthetic engine load failure' + })), + getDownloadStatus: jest.fn(async () => ({ status: { downloads: [] } })), + downloadModel + }, + events: { onOperationProgress: jest.fn(() => jest.fn()) } + }; + + await renderOnChooseStep(); + + expect(screen.getByText(/The AI engine could not start/i)).toHaveTextContent( + 'Synthetic engine load failure' + ); + // Nothing is known to be missing, so nothing is offered for download. + expect(screen.queryByText(/Required models are missing/i)).not.toBeInTheDocument(); + expect(screen.queryByText('Not installed')).not.toBeInTheDocument(); + expect(screen.getAllByText('Not checked').length).toBeGreaterThanOrEqual(2); + expect(screen.getByRole('button', { name: /Download Models/i })).toBeDisabled(); + fireEvent.click(screen.getByRole('button', { name: /Download Models/i })); + await advance(); + expect(downloadModel).not.toHaveBeenCalled(); + }); + + test('an engine failure during the download is shown instead of "downloads stopped"', async () => { + let engineFailed = false; + window.electronAPI = { + llama: { + getConfig: jest.fn(async () => ({ embeddingModel, textModel, visionModel })), + getModels: jest.fn(async () => + engineFailed + ? { models: [], selected: {}, error: 'Synthetic engine load failure' } + : { models: [] } + ), + getDownloadStatus: jest.fn(async () => ({ + status: { downloads: engineFailed ? [] : allActive } + })), + downloadModel: jest.fn(async () => ({ success: true, started: true })) + }, + events: { onOperationProgress: jest.fn(() => jest.fn()) } + }; + + await renderOnChooseStep(); + fireEvent.click(screen.getByRole('button', { name: /Download Models/i })); + await advance(); + expect(screen.getByText(/Downloading Models/i)).toBeInTheDocument(); + + // The files finished (nothing active any more) but the engine cannot + // list them: that is not a stopped download. + engineFailed = true; + await advance(12500); + + expect(screen.queryByText(/downloads stopped/i)).not.toBeInTheDocument(); + expect(screen.getByText(/The AI engine could not start/i)).toHaveTextContent( + 'Synthetic engine load failure' + ); + expect(screen.getByText(/Downloading Models/i)).toBeInTheDocument(); + }); +}); + +describe('ModelSetupWizard vision model without its projector file', () => { + const baseProfile = INSTALL_MODEL_PROFILES?.BASE_SMALL?.models || {}; + const embeddingModel = baseProfile.EMBEDDING; + const textModel = baseProfile.TEXT_ANALYSIS; + const visionModel = baseProfile.IMAGE_ANALYSIS; + const projector = 'synthetic-mmproj.gguf'; + + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + delete window.electronAPI; + }); + + async function advance(ms = 0) { + await act(async () => { + await jest.advanceTimersByTimeAsync(ms); + }); + } + + test('is not marked ready, and the retry fetches only the projector', async () => { + let progressListener; + let installed = []; + let active = [embeddingModel, textModel, visionModel].map((filename) => ({ + filename, + progress: 10 + })); + const downloadModel = jest.fn(async () => ({ success: true, started: true })); + window.electronAPI = { + llama: { + getConfig: jest.fn(async () => ({ embeddingModel, textModel, visionModel })), + getModels: jest.fn(async () => ({ models: installed })), + getDownloadStatus: jest.fn(async () => ({ status: { downloads: active } })), + downloadModel, + updateConfig: jest.fn(async () => ({ success: true })) + }, + events: { + onOperationProgress: jest.fn((cb) => { + progressListener = cb; + return jest.fn(); + }) + } + }; + + render(); + await advance(); + fireEvent.click(screen.getByRole('button', { name: /Download Models/i })); + await advance(); + expect(screen.getByText(/Downloading Models/i)).toBeInTheDocument(); + expect(downloadModel).toHaveBeenCalledTimes(3); + + // What main sends when the GGUF landed but its projector did not. + await act(async () => { + progressListener({ type: 'model-download-complete', model: embeddingModel }); + progressListener({ type: 'model-download-complete', model: textModel }); + progressListener({ + type: 'model-download-complete', + model: visionModel, + success: false, + degraded: true, + progress: { + model: visionModel, + percent: 100, + companion: { name: projector, success: false, error: 'network' } + } + }); + }); + + expect(screen.queryByRole('button', { name: /^Continue$/i })).not.toBeInTheDocument(); + expect(screen.getByText(/projector file did not download/i)).toBeInTheDocument(); + expect(screen.getByText('Failed')).toBeInTheDocument(); + + // The GGUF is on disk, so the model list names it; that alone is not ready. + installed = [embeddingModel, textModel, visionModel]; + active = []; + await advance(4100); + expect(screen.queryByText(/Setup complete/i)).not.toBeInTheDocument(); + expect(screen.getByText(/projector file did not download/i)).toBeInTheDocument(); + + downloadModel.mockClear(); + fireEvent.click(screen.getByRole('button', { name: /Retry Failed Downloads/i })); + await advance(); + expect(downloadModel).toHaveBeenCalledTimes(1); + expect(downloadModel).toHaveBeenCalledWith(projector); + + await act(async () => { + progressListener({ type: 'model-download-complete', model: projector }); + }); + installed = [embeddingModel, textModel, visionModel, projector]; + await advance(4100); + expect(screen.queryByText(/projector file did not download/i)).not.toBeInTheDocument(); + expect(screen.getByText(/Setup complete/i)).toBeInTheDocument(); + }); + + test('Cancel during the projector retry cancels the projector download', async () => { + let progressListener; + const cancelDownload = jest.fn(async () => ({ success: true })); + window.electronAPI = { + llama: { + getConfig: jest.fn(async () => ({ embeddingModel, textModel, visionModel })), + getModels: jest.fn(async () => ({ models: [embeddingModel, textModel] })), + getDownloadStatus: jest.fn(async () => ({ + status: { downloads: [{ filename: visionModel, progress: 10 }] } + })), + downloadModel: jest.fn(async () => ({ success: true, started: true })), + cancelDownload, + updateConfig: jest.fn(async () => ({ success: true })) + }, + events: { + onOperationProgress: jest.fn((cb) => { + progressListener = cb; + return jest.fn(); + }) + } + }; + + render(); + await advance(); + fireEvent.click(screen.getByRole('button', { name: /Download Models/i })); + await advance(); + await act(async () => { + progressListener({ + type: 'model-download-complete', + model: visionModel, + success: false, + degraded: true, + progress: { + model: visionModel, + percent: 100, + companion: { name: projector, success: false, error: 'network' } + } + }); + }); + fireEvent.click(screen.getByRole('button', { name: /Retry Failed Downloads/i })); + await advance(); + + fireEvent.click(screen.getByRole('button', { name: /Cancel download of/i })); + await advance(); + expect(cancelDownload).toHaveBeenCalledWith(projector); + expect(screen.getByText('Cancelled')).toBeInTheDocument(); + }); +}); + describe('ModelSetupWizard hardware-aware profile (ML-07)', () => { afterEach(() => { delete window.electronAPI; From f6164bb5003c1ed6d7a4e202c07f07384e63ce43 Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 13:57:13 -0500 Subject: [PATCH 24/26] docs(changelog): record the second sweep fixes --- CHANGELOG.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60f5988..f07e367 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -565,6 +565,29 @@ Changes you will notice: - **"Update ready" stays after a later update check fails.** - **Search window.** The Timeline list fills the window instead of staying 400 px tall, and the hidden Atlas tab is no longer reachable with the Tab key. +- **Organizing many or large files to another drive no longer stops after five minutes.** The batch + was cut off, a timeout was shown and the files already moved could be moved back. A batch that is + still moving files now runs until it is done or you press Stop. +- **A failed settings read can no longer replace your saved settings with the defaults.** Settings + now says the read failed and offers Try again; nothing is saved until the real settings have + loaded. +- **Chat.** Dragging files or folders onto the Document scope panel adds them, and says so when + nothing could be added. A reopened chat shows each answer as it first appeared. Retry after a + failed request saves the question with the answer. Chats scoped to more than 200 documents are + saved and keep their memory. +- **Files whose search indexing hit a temporary error are retried on their own** after a short wait, + also after a restart, instead of waiting until another file is analyzed. +- **Analysis history no longer mixes up two files that used the same path.** Analyzing a new file + where an organized file used to be, or moving or deleting a file from there, no longer removes or + re-points the organized file's history. +- **Setup wizard.** No false "Required model downloads stopped" right after clicking Download. When + the AI engine cannot start, the wizard says so with the reason instead of listing installed models + as missing. An image model whose projector file did not download is shown as failed, and the retry + fetches (and Cancel stops) only that file. +- **Dropping a folder whose files are already in the Discover queue no longer analyzes them again;** + only new files are added. +- **Recordings queued behind one that timed out now really wait their turn.** The earlier fix did + not take effect in the packaged app. ## [5.1.2] - 2026-11-01 From 847ef3866d9af18c0260d415a2864a5211df62ea Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 14:10:15 -0500 Subject: [PATCH 25/26] perf(organize): a bulk category change is saved in one store update --- src/renderer/phases/OrganizePhase.jsx | 19 +++++- .../phases/organize/useFileEditing.js | 16 +++-- src/renderer/store/slices/analysisSlice.js | 13 ++++ test/components/OrganizePhase.test.js | 3 +- test/redux-state-bugfix-regression.test.js | 59 +++++++++++++++++++ test/useFileEditing.test.js | 11 ++++ 6 files changed, 114 insertions(+), 7 deletions(-) diff --git a/src/renderer/phases/OrganizePhase.jsx b/src/renderer/phases/OrganizePhase.jsx index df48870..45f2f40 100644 --- a/src/renderer/phases/OrganizePhase.jsx +++ b/src/renderer/phases/OrganizePhase.jsx @@ -53,7 +53,7 @@ import { useOrganizeSuggestions } from './organize/useOrganizeSuggestions'; import { resolveOrganizeDestination } from './organize/organizeDestination'; import { useOrganizeNotificationFocus } from './organize/useOrganizeNotificationFocus'; import { setFileStates as setFileStatesAction } from '../store/slices/filesSlice'; -import { updateAnalysisResult } from '../store/slices/analysisSlice'; +import { updateAnalysisResult, updateAnalysisResultsByPaths } from '../store/slices/analysisSlice'; import { selectOrganizedCount, selectRedactPaths } from '../store/selectors'; import { PHASE_PREREQUISITE_REASONS } from '../store/slices/uiSlice'; import { formatDisplayPath } from '../utils/pathDisplay'; @@ -167,6 +167,20 @@ function OrganizePhase() { [dispatch] ); + // The bulk category change saves the whole selection in one store update. + const persistBulkFileEdit = useCallback( + (filePaths, field, value) => { + if (field !== 'category' || !Array.isArray(filePaths) || filePaths.length === 0) return; + dispatch( + updateAnalysisResultsByPaths({ + paths: filePaths, + changes: { category: value, userCategory: value } + }) + ); + }, + [dispatch] + ); + const { editingFiles, setEditingFiles, handleEditFile, getFileWithEdits } = useFileEditing({ onEditChange: persistFileEdit }); @@ -264,7 +278,8 @@ function OrganizePhase() { setEditingFiles, setSelectedFiles, addNotification, - onEditChange: persistFileEdit + onEditChange: persistFileEdit, + onBulkEditChange: persistBulkFileEdit }); // Drop edits for files that left the queue so a filtered grid cannot write diff --git a/src/renderer/phases/organize/useFileEditing.js b/src/renderer/phases/organize/useFileEditing.js index 76f91f9..6e4641f 100644 --- a/src/renderer/phases/organize/useFileEditing.js +++ b/src/renderer/phases/organize/useFileEditing.js @@ -289,7 +289,7 @@ export function useFileSelection(files) { /** * Hook for bulk operations - * @param {{ selectedFiles: any[], editingFiles: Record, setEditingFiles: Function, setSelectedFiles: Function, addNotification: Function, onEditChange?: Function }} params + * @param {{ selectedFiles: any[], editingFiles: Record, setEditingFiles: Function, setSelectedFiles: Function, addNotification: Function, onEditChange?: Function, onBulkEditChange?: Function }} params * @returns {Record} Bulk operation state and handlers */ export function useBulkOperations({ @@ -298,7 +298,8 @@ export function useBulkOperations({ setEditingFiles, setSelectedFiles, addNotification, - onEditChange + onEditChange, + onBulkEditChange }) { const [bulkEditMode, setBulkEditMode] = useState(false); const [bulkCategory, setBulkCategory] = useState(''); @@ -308,6 +309,7 @@ export function useBulkOperations({ const bulkCategoryRef = useRef(bulkCategory); const addNotificationRef = useRef(addNotification); const onEditChangeRef = useRef(onEditChange); + const onBulkEditChangeRef = useRef(onBulkEditChange); // Keep refs in sync with latest values useEffect(() => { @@ -324,7 +326,8 @@ export function useBulkOperations({ }, [addNotification]); useEffect(() => { onEditChangeRef.current = onEditChange; - }, [onEditChange]); + onBulkEditChangeRef.current = onBulkEditChange; + }, [onEditChange, onBulkEditChange]); // Debounced bulk category change ref const debouncedBulkCategoryChangeRef = useRef(null); @@ -348,7 +351,12 @@ export function useBulkOperations({ // Save the choice the way a single-card edit does. The card edit state // alone is dropped when Organize remounts or the file is re-analyzed. const persistEdit = onEditChangeRef.current; - if (typeof persistEdit === 'function') { + const persistBulkEdit = onBulkEditChangeRef.current; + if (typeof persistBulkEdit === 'function') { + // One call for the whole selection; a call per file costs a store + // update per file. + persistBulkEdit([...selected], 'category', category); + } else if (typeof persistEdit === 'function') { selected.forEach((/** @type {any} */ key) => persistEdit(key, 'category', category)); } setBulkEditMode(false); diff --git a/src/renderer/store/slices/analysisSlice.js b/src/renderer/store/slices/analysisSlice.js index 810f1c7..ba43de5 100644 --- a/src/renderer/store/slices/analysisSlice.js +++ b/src/renderer/store/slices/analysisSlice.js @@ -166,6 +166,18 @@ const analysisSlice = createSlice({ }; } }, + // One pass for a bulk edit. A dispatch per file re-runs every selector + // over the whole list each time, which froze the window on large + // selections. + updateAnalysisResultsByPaths: (state, action) => { + const { paths, changes } = action.payload || {}; + if (!Array.isArray(paths) || paths.length === 0 || !changes) return; + const targets = new Set(paths.map((/** @type {any} */ p) => toCanonicalPath(p))); + state.results.forEach((/** @type {any} */ r) => { + if (!r || !targets.has(toCanonicalPath(r.path))) return; + r.analysis = { ...(r.analysis || {}), ...changes }; + }); + }, updateEmbeddingState: (state, action) => { const { path, embeddingPolicy, embeddingStatus } = action.payload || {}; if (!path) return; @@ -262,6 +274,7 @@ export const { setAnalysisResults, mergeAnalysisResults, updateAnalysisResult, + updateAnalysisResultsByPaths, updateEmbeddingState, resetAnalysisState, resetToSafeState, diff --git a/test/components/OrganizePhase.test.js b/test/components/OrganizePhase.test.js index e0c0c2f..985a9da 100644 --- a/test/components/OrganizePhase.test.js +++ b/test/components/OrganizePhase.test.js @@ -42,7 +42,8 @@ jest.mock('../../src/renderer/store/slices/filesSlice', () => ({ })); jest.mock('../../src/renderer/store/slices/analysisSlice', () => ({ - updateAnalysisResult: jest.fn() + updateAnalysisResult: jest.fn(), + updateAnalysisResultsByPaths: jest.fn() })); jest.mock('../../src/renderer/components/ui', () => ({ diff --git a/test/redux-state-bugfix-regression.test.js b/test/redux-state-bugfix-regression.test.js index 8ee4cb3..8c99310 100644 --- a/test/redux-state-bugfix-regression.test.js +++ b/test/redux-state-bugfix-regression.test.js @@ -26,6 +26,7 @@ const analysisReducer = require('../src/renderer/store/slices/analysisSlice').de const { mergeAnalysisResults, updateAnalysisResult, + updateAnalysisResultsByPaths, startAnalysis, updateProgress, stopAnalysis, @@ -604,6 +605,64 @@ describe('BUG-8: updateAnalysisResult initializes analysis when absent', () => { }); }); +describe('updateAnalysisResultsByPaths applies one change to many results', () => { + test('updates every named path, in any path shape, and leaves the rest', () => { + const state = { + ...analysisInitial, + results: [ + { path: 'C:\\Docs\\a.pdf', analysis: { category: 'old', tags: ['a'] } }, + { path: 'C:\\Docs\\b.pdf', analysis: null }, + { path: 'C:\\Docs\\c.pdf', analysis: { category: 'keep' } } + ] + }; + + const result = analysisReducer( + state, + updateAnalysisResultsByPaths({ + paths: ['c:/docs/A.pdf', 'C:\\Docs\\b.pdf', 'C:\\Docs\\missing.pdf'], + changes: { category: 'Finance', userCategory: 'Finance' } + }) + ); + + expect(result.results[0].analysis).toEqual({ + category: 'Finance', + userCategory: 'Finance', + tags: ['a'] + }); + expect(result.results[1].analysis).toEqual({ category: 'Finance', userCategory: 'Finance' }); + expect(result.results[2]).toBe(state.results[2]); + }); + + test('no-ops on an empty or malformed payload', () => { + const state = { ...analysisInitial, results: [{ path: '/file.pdf', analysis: { v: 1 } }] }; + + expect(analysisReducer(state, updateAnalysisResultsByPaths({ paths: [], changes: {} }))).toBe( + state + ); + expect(analysisReducer(state, updateAnalysisResultsByPaths(undefined))).toBe(state); + }); + + test('a 5,000-file change is one pass over the results', () => { + const results = Array.from({ length: 5000 }, (_, i) => ({ + path: `C:\\Docs\\file-${i}.pdf`, + analysis: { category: 'old' } + })); + const state = { ...analysisInitial, results }; + + const started = Date.now(); + const result = analysisReducer( + state, + updateAnalysisResultsByPaths({ + paths: results.map((r) => r.path), + changes: { category: 'Finance' } + }) + ); + + expect(result.results.every((r) => r.analysis.category === 'Finance')).toBe(true); + expect(Date.now() - started).toBeLessThan(2000); + }); +}); + // ═════════════════════════════════════════════════════════════════════════════ // BUG-9: IPC timeout protection // ═════════════════════════════════════════════════════════════════════════════ diff --git a/test/useFileEditing.test.js b/test/useFileEditing.test.js index 67e1738..d109d74 100644 --- a/test/useFileEditing.test.js +++ b/test/useFileEditing.test.js @@ -288,6 +288,17 @@ describe('useFileEditing hooks', () => { }); }); + test('saves a bulk category for the whole selection in one call', () => { + const onEditChange = jest.fn(); + const onBulkEditChange = jest.fn(); + const { result } = renderBulk({ onEditChange, onBulkEditChange }); + + applyCategory(result, 'Finance'); + + expect(onBulkEditChange.mock.calls).toEqual([[['/a.txt', '/b.txt'], 'category', 'Finance']]); + expect(onEditChange).not.toHaveBeenCalled(); + }); + test('says "1 file" when one file is selected', () => { const { result, props } = renderBulk({ selectedFiles: new Set(['/a.txt']) }); From ff11cddd54fa2cfd391a4a6f8a9628336ab283a5 Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Tue, 10 Nov 2026 16:39:22 -0500 Subject: [PATCH 26/26] test(e2e): the quick start and clusters-toggle tests follow what the app does Welcome opens Discover only once a destination folder exists, so the quick start test now checks the tab is closed on a new profile and opens after a folder is added. The clusters-toggle test no longer presses Escape after the graph tour is dismissed, which closed Knowledge OS before the buttons were looked up. Both failed on main as well. --- test/e2e/knowledge-graph.spec.js | 3 +- test/e2e/navigation.spec.js | 47 ++++++++++++++++++++++++++------ 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/test/e2e/knowledge-graph.spec.js b/test/e2e/knowledge-graph.spec.js index e1c6f8f..30ff570 100644 --- a/test/e2e/knowledge-graph.spec.js +++ b/test/e2e/knowledge-graph.spec.js @@ -139,8 +139,9 @@ test.describe('Knowledge Graph — Relate Tab UI', () => { await relateTab.click(); await window.waitForTimeout(800); + // No Escape here: once the tour is gone, Escape closes Knowledge OS + // itself and the buttons below are never found. await dismissGraphTour(window); - await window.keyboard.press('Escape').catch(() => {}); await window.waitForTimeout(300); await expect diff --git a/test/e2e/navigation.spec.js b/test/e2e/navigation.spec.js index 7ae9493..6ca1136 100644 --- a/test/e2e/navigation.spec.js +++ b/test/e2e/navigation.spec.js @@ -7,6 +7,9 @@ * Run: npm run test:e2e -- --grep "Navigation" */ +const fs = require('fs'); +const os = require('os'); +const path = require('path'); const { test, expect } = require('@playwright/test'); const { launchApp, @@ -73,17 +76,43 @@ test.describe('Navigation', () => { }); test('should navigate from Welcome to Discover (Quick Start)', async () => { - // From Welcome, Discover should also be accessible (quick start path) - const canNavigate = await nav.isPhaseAccessible(PHASES.DISCOVER); - expect(canNavigate).toBe(true); + // Welcome opens Discover only once files have somewhere to go: a new + // profile has no destination folder, so the tab starts closed. + expect(await nav.isPhaseAccessible(PHASES.DISCOVER)).toBe(false); + + const folderPath = path.join(os.tmpdir(), `stratosort-e2e-nav-${Date.now()}`); + const addResult = await window.evaluate( + async (folder) => + window.electronAPI.smartFolders.add({ + name: 'E2E Navigation Folder', + path: folder, + description: 'Destination for the quick start path' + }), + folderPath + ); + expect(addResult.success).toBe(true); - // Navigate to Discover - const success = await nav.goToPhase(PHASES.DISCOVER); - expect(success).toBe(true); + try { + // The folder was added behind the renderer's back, so reload it to read + // the folder list the way a launch would. + await window.reload(); + await waitForAppReady(window); + expect(await nav.getCurrentPhase()).toBe(PHASES.WELCOME); - await window.waitForTimeout(500); - const currentPhase = await nav.getCurrentPhase(); - expect(currentPhase).toBe(PHASES.DISCOVER); + await expect + .poll(() => nav.isPhaseAccessible(PHASES.DISCOVER), { timeout: 15000 }) + .toBe(true); + + // Navigate to Discover + const success = await nav.goToPhase(PHASES.DISCOVER, { viaSetup: false }); + expect(success).toBe(true); + + await window.waitForTimeout(500); + const currentPhase = await nav.getCurrentPhase(); + expect(currentPhase).toBe(PHASES.DISCOVER); + } finally { + fs.rmSync(folderPath, { recursive: true, force: true }); + } }); test('should enforce phase transition rules', async () => {