From 4b8dd95d9e887d915ebc2b2be84612af9c9976ed Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Thu, 12 Nov 2026 12:51:05 -0500 Subject: [PATCH 1/2] fix(organize): stop losing identical files, report move outcomes, allow external drives From a tester's report of a Mac sorting run that ended with files missing, an unclear result and an external drive that could not be used. File loss - handleDuplicateMove compared a file against files the same run had still to move. Two byte-identical files renamed or moved in one run were each taken for a copy already in place, both went to the trash, the move check failed and the run was undone with the two files gone. A file the run has still to move no longer counts as a copy in place, and a rename inside one folder never removes the file. Move outcomes - A run that did not move everything stays on Organize with a banner: how many moved, how many did not and why, or that the run was undone. - Identical copies that were removed are listed as duplicates with the copy that was kept, and are no longer counted as moved. Sources that are also destinations - A notice on Organize and a mark in Preview say which files stay where they are, or are only renamed there. External drives - A folder outside the user folder can be a Smart Folder, can be scanned and can have folders created in it. sanitizeFolderPath no longer bounds paths to the home tree; the destination policy decides, and asks once for a folder that is not inside a Smart Folder or Documents and was not picked in a dialog. - Still refused: system folders and links into them, startup folders, device paths and traversal. LINK_OUTSIDE_ALLOWED becomes LINK_INTO_SYSTEM; ALLOWED_APP_PATHS is removed. - A refused or unreadable folder on Discover gives the reason instead of "No files found". Not changed: one file failing the move check still undoes the whole run. Not verified: macOS, and a packaged build of this branch. --- CHANGELOG.md | 23 + CLAUDE.md | 2 +- docs/USER_GUIDE.md | 19 +- scripts/verify-checkjs-ratchet.js | 2 +- src/main/ipc/files/batchOrganizeHandler.js | 84 +++- src/main/ipc/files/folderHandlers.js | 74 ++- src/main/ipc/smartFolders.js | 110 ++--- src/main/services/OrganizeResumeService.js | 8 +- .../services/triage/DownloadsTriageService.js | 8 +- src/main/utils/fileDedup.js | 86 +++- .../components/organize/InPlaceMoveNotice.jsx | 60 +++ .../organize/OrganizePreviewModal.jsx | 28 +- .../organize/OrganizeSummaryBanner.jsx | 82 ++++ .../organize/VirtualizedProcessedFiles.jsx | 125 ++--- src/renderer/components/organize/index.js | 2 + src/renderer/components/ui/AlertBox.jsx | 10 +- src/renderer/phases/CompletePhase.jsx | 178 ++++--- src/renderer/phases/OrganizePhase.jsx | 32 +- .../phases/discover/useFileHandlers.js | 36 ++ .../phases/organize/useOrganization.js | 378 ++++++++++++--- src/renderer/utils/canonicalPath.js | 24 + src/renderer/utils/duplicateRemoval.js | 26 ++ src/renderer/utils/errorMapping.js | 17 +- src/shared/securityConfig.js | 16 - test/a11y/uiComponents.a11y.test.js | 12 + ...chOrganize.duplicateSources.realfs.test.js | 295 ++++++++++++ test/batchOrganizeHandler.test.js | 30 ++ test/canonicalPath.folderKeys.test.js | 79 ++++ test/components/CompletePhase.test.js | 109 ++++- test/components/InPlaceMoveNotice.test.js | 98 ++++ test/components/OrganizePhase.test.js | 70 +++ test/components/OrganizePreviewModal.test.js | 37 ++ test/components/OrganizeSummaryBanner.test.js | 72 +++ .../VirtualizedProcessedFiles.test.js | 62 +++ test/duplicateRemoval.test.js | 28 ++ test/errorMapping.fileOperations.test.js | 34 +- test/fileDedup.pendingSources.test.js | 155 ++++++ ...olderHandlers.createFolderLocation.test.js | 281 +++++++++++ .../folderHandlers.linkOutsideAllowed.test.js | 180 ------- test/securityConfig.test.js | 18 - test/smartFolders.sanitizeFolderPath.test.js | 99 +++- test/smartFolders.watcherLifecycle.test.js | 3 +- test/smartFoldersIpc.coverage.test.js | 3 +- test/smartFoldersIpc.test.js | 101 +++- test/useFileHandlers.test.js | 110 +++++ test/useOrganization.inPlaceMoves.test.js | 193 ++++++++ test/useOrganization.rolledBackBatch.test.js | 10 +- test/useOrganization.runOutcome.test.js | 441 ++++++++++++++++++ 48 files changed, 3333 insertions(+), 617 deletions(-) create mode 100644 src/renderer/components/organize/InPlaceMoveNotice.jsx create mode 100644 src/renderer/components/organize/OrganizeSummaryBanner.jsx create mode 100644 src/renderer/utils/duplicateRemoval.js create mode 100644 test/batchOrganize.duplicateSources.realfs.test.js create mode 100644 test/canonicalPath.folderKeys.test.js create mode 100644 test/components/InPlaceMoveNotice.test.js create mode 100644 test/components/OrganizeSummaryBanner.test.js create mode 100644 test/duplicateRemoval.test.js create mode 100644 test/fileDedup.pendingSources.test.js create mode 100644 test/folderHandlers.createFolderLocation.test.js delete mode 100644 test/folderHandlers.linkOutsideAllowed.test.js create mode 100644 test/useOrganization.inPlaceMoves.test.js create mode 100644 test/useOrganization.runOutcome.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e3f3a6..08bbf81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,29 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixed +- **Two identical files in one folder are no longer both sent to the trash when one run renames or + moves them.** Each was taken for a copy of the other that was already in place, so both were + removed instead of moved, and the run could then fail and be undone with the two files missing + from the folder. A file the same run has still to move no longer counts as a copy already in + place, and renaming a file inside its own folder never removes it. A file removed this way went to + the system trash. +- **An organize run that did not move everything says so and stays on the Organize screen.** A + banner above the file list says how many files moved, how many did not and why, or that the whole + run was undone and the files are back where they were. Before, a run that failed part-way or was + undone could move on to the Complete screen as if it had worked. An identical copy that was + removed instead of moved is listed as a duplicate, with the copy that was kept, and is no longer + counted as a moved file. +- **Files that are already in the folder chosen for them are pointed out before the run.** When a + folder you sort from is also one of your Smart Folders, files matched to it stay where they are + (or are renamed there). A notice on the Organize screen and a mark in Preview now say which, so a + run no longer reports files as organized while nothing appears anywhere new. +- **A folder on an external or second drive can be a Smart Folder, can be scanned and can have + folders created in it.** These were refused for any folder outside your user folder, with a + message about a link or short name. Folders inside system folders, or reached through a link that + leads into one, are still refused. A folder that is not inside a Smart Folder or Documents and was + not picked in a file dialog is confirmed once in a prompt. +- **A folder the app will not scan, or cannot read, gives the reason.** Choosing or dropping such a + folder on the Discover screen said that no files were found. - **An image that arrives just as a text model starts loading no longer starts the vision runtime beside it.** The image looked for a loading model before the load began and found none. The vision runtime now checks the memory budget again at the moment it starts; if a load began meanwhile, the diff --git a/CLAUDE.md b/CLAUDE.md index 0045550..a5c3aab 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -103,7 +103,7 @@ tree. `ci`) enforces a monotonic floor and blocks new .js under `COVERED_DIRS` (`src/shared`, `src/preload`, `src/main/{ipc,services,analysis,errors,utils,workers,transports,core}`, `src/renderer/{services,hooks,phases,store,utils,components}` today (every `.js` under `src/` is - now opted in — 495/495 as of 2026-11-11, and `GLOBAL_DIRECTIVE_FLOOR` matches at 495) — see + now opted in — 496/496 as of 2026-11-12, and `GLOBAL_DIRECTIVE_FLOOR` matches at 496) — see `scripts/verify-checkjs-ratchet.js` for the authoritative list) from landing without the directive. When adding a file to a covered dir: put `// @ts-check` on line 1 and fix the errors before commit. When opting in a new file elsewhere: bump `GLOBAL_DIRECTIVE_FLOOR` in diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index 7917cd6..6e43569 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -292,17 +292,18 @@ Then run a small batch first and review outcomes before scaling up. - Review naming defaults in **Settings -> Default Locations**. - Confirm convention/date/case/separator values. -### "That folder is inside a link that points outside your allowed locations." +### "That folder leads into a system folder through a link or short name." -You can see this when you use **Create directory** on a Smart Folder marked **Missing** in Setup. On -this computer, StratoSort creates folders only inside your home folder, Documents, Downloads or -Desktop, and never inside a system folder. It goes by where the folder really is, not by the path as -written. If the path runs through a shortcut-style link (a symbolic link, or a junction on Windows) -that leads somewhere outside those places, or into a system folder, the folder is refused. +You can see this when you use **Create directory** on a Smart Folder marked **Missing** in Setup. +StratoSort creates folders on any drive, an external one included, but never inside a system folder. +It goes by where the folder really is, not by the path as written. If the path runs through a +shortcut-style link (a symbolic link, or a junction on Windows) or a Windows short name that leads +into a system folder, the folder is refused. -- Edit the Smart Folder to use the real folder the link points to. -- If that real folder is outside those places, create it yourself in File Explorer or Finder, then - choose it as the Smart Folder's location with **Browse**. +- Edit the Smart Folder to use a folder outside the system folders. + +A folder that is not inside one of your Smart Folders or Documents is created after you confirm it +in a prompt. If the path runs through a link, the prompt names the folder the link really leads to. ### Need to see logs diff --git a/scripts/verify-checkjs-ratchet.js b/scripts/verify-checkjs-ratchet.js index b63c700..fcb010e 100644 --- a/scripts/verify-checkjs-ratchet.js +++ b/scripts/verify-checkjs-ratchet.js @@ -43,7 +43,7 @@ const COVERED_DIRS = [ 'src/renderer/utils', 'src/renderer/components' ]; -const GLOBAL_DIRECTIVE_FLOOR = 495; +const GLOBAL_DIRECTIVE_FLOOR = 496; // ──────────────────────────────────────────────────────────────────────── const DIRECTIVE_SCAN_LINES = 5; diff --git a/src/main/ipc/files/batchOrganizeHandler.js b/src/main/ipc/files/batchOrganizeHandler.js index b83b87e..1d7d77d 100644 --- a/src/main/ipc/files/batchOrganizeHandler.js +++ b/src/main/ipc/files/batchOrganizeHandler.js @@ -586,6 +586,13 @@ async function handleBatchOrganize(params) { } const batchOperationTotal = batch.operations.length; const limit = pLimit(effectiveConcurrency); + // Files this batch has still to move. The duplicate check must not + // take one of them for a copy that is already filed: it may be moved + // or renamed away later in this batch. Built here because each + // operation rewrites its paths to the validated form once it starts. + const pendingSources = batch.operations + .filter((op) => op.status !== 'done' && typeof op.source === 'string') + .map((op) => op.source); // Abort reasons the USER deliberately triggered (renderer buttons). // Anything else on the signal — 'ipc-timeout', 'shutdown' (abortAll), @@ -814,7 +821,13 @@ async function handleBatchOrganize(params) { const { signal } = abortController; if (signal.aborted) startGrace(); else signal.addEventListener('abort', startGrace, { once: true }); - const movePromise = performFileMove(op, log, computeFileChecksum, signal); + const movePromise = performFileMove( + op, + log, + computeFileChecksum, + signal, + pendingSources + ); try { const outcome = await Promise.race([movePromise, abandoned]); if (outcome !== MOVE_ABANDONED) return outcome; @@ -909,17 +922,36 @@ async function handleBatchOrganize(params) { } if (moveResult && moveResult.skipped) { - skippedCount++; const resolvedDestination = moveResult.destination || op.destination; const currentFileName = path.basename(op.source); - results.push({ - success: true, + + // Verify state is consistent (dest exists, source gone) before + // anything reports the file as filed. + try { + await verifyMoveCompletion(op.source, resolvedDestination, log); + } catch (verifyError) { + // The source was removed as a duplicate and the copy it + // duplicated cannot be confirmed. Rollback has nothing to + // copy back, so the message is where the user learns that the + // file is in the trash. + if (moveResult.sourceRemoval === 'trashed' && verifyError instanceof Error) { + verifyError.message += + '. The original was moved to the system Trash / Recycle Bin as a duplicate and can be restored from there.'; + } + throw verifyError; + } + + consecutivePermissionFailures = 0; + // A duplicate skip still mutates disk state by deleting the source file. + // Track it so a later critical failure can restore the original path. + completedOperations.push({ + index: i, source: op.source, destination: resolvedDestination, - operation: op.type || 'move', - skipped: true, - reason: moveResult.reason + originalDestination: op._originalDestination, + rollbackAction: 'restore_duplicate_source' }); + renewBatchLock(batchId); // One info line per skipped file in every build: the Live // Activity feed shows "Skipped (already in place)". File @@ -932,20 +964,22 @@ async function handleBatchOrganize(params) { destination: path.basename(resolvedDestination) }); - // Verify state is consistent (dest exists, source gone) - await verifyMoveCompletion(op.source, resolvedDestination, log); + // Rollback started while this skip was being verified: it + // restores the source and reports the row (see the same check + // on the move path below). + if (shouldRollback) return; - consecutivePermissionFailures = 0; - // A duplicate skip still mutates disk state by deleting the source file. - // Track it so a later critical failure can restore the original path. - completedOperations.push({ - index: i, + skippedCount++; + results.push({ + success: true, source: op.source, destination: resolvedDestination, - originalDestination: op._originalDestination, - rollbackAction: 'restore_duplicate_source' + operation: op.type || 'move', + skipped: true, + reason: moveResult.reason, + // 'trashed' or 'deleted': where the source went. + sourceRemoval: moveResult.sourceRemoval }); - renewBatchLock(batchId); // Not awaited (see the success path below for why). void processingState?.markOrganizeOpDone?.(batchId, i, { @@ -1629,8 +1663,11 @@ function reconcileRolledBackResults(rollbackOutcome, completedOperations, rollba * @param {LoggerLike} log * @param {typeof computeFileChecksum} checksumFn * @param {AbortSignal|null} [abortSignal] + * @param {string[]} [pendingSources] - Every source the caller's batch has + * still to move (the same array for each call), so the duplicate check does + * not count one of them as a copy already at the destination. */ -async function performFileMove(op, log, checksumFn, abortSignal = null) { +async function performFileMove(op, log, checksumFn, abortSignal = null, pendingSources) { // This prevents creating numbered copies of files that already exist. const duplicateResult = await handleDuplicateMove({ sourcePath: op.source, @@ -1639,7 +1676,8 @@ async function performFileMove(op, log, checksumFn, abortSignal = null) { logger: log, logPrefix: '[FILE-OPS]', dedupContext: 'batchOrganize', - removeEmbeddings: removeEmbeddingsForPathBestEffort + removeEmbeddings: removeEmbeddingsForPathBestEffort, + pendingSources }); if (duplicateResult) return duplicateResult; @@ -2431,9 +2469,15 @@ async function previewBatchOrganize({ operation, log }) { // PREVIEW_MAX_NUMERIC_ATTEMPTS so the preview stays snappy even when the // destination directory is densely packed. The real executor still // walks up to MAX_NUMERIC_RETRIES on collision before UUID-fallback. + // + // A file that is already at its destination (same path, or the same name + // in another case where the filesystem ignores case) occupies that path + // itself, so the executor's move does not collide with anything. let finalDestination = op.destination; let collision = false; - if (await pathExistsSilent(op.destination)) { + const alreadyAtDestination = + _canonicalizeForOverlapCheck(op.source) === _canonicalizeForOverlapCheck(op.destination); + if (!alreadyAtDestination && (await pathExistsSilent(op.destination))) { collision = true; conflicts += 1; const ext = path.extname(op.destination); diff --git a/src/main/ipc/files/folderHandlers.js b/src/main/ipc/files/folderHandlers.js index cb6b562..4ec854c 100644 --- a/src/main/ipc/files/folderHandlers.js +++ b/src/main/ipc/files/folderHandlers.js @@ -9,15 +9,13 @@ const path = require('path'); const fs = require('fs').promises; -const os = require('os'); const { app } = require('electron'); const { createHandler, safeHandle, z } = require('../ipcWrappers'); const { createLogger } = require('../../../shared/logger'); const { validateFileOperationPath, resolveCanonicalPath, - isPathDangerous, - isPathWithinAllowed + isPathDangerous } = require('../../../shared/pathSanitization'); const { isNotFoundError, @@ -33,13 +31,13 @@ const logger = createLogger('IPC:Files:Folders'); const schemaFilePath = z ? z.string().min(1) : null; const { IpcServiceContext, createFromLegacyParams } = require('../IpcServiceContext'); -// Returned when the folder's spelling is inside an allowed location but a -// symlink or junction along it leads outside. The renderer shows the message -// as is: the code avoids the substrings (PATH, FOLDER, FILE, ...) that its -// error mapping would replace with generic copy. -const LINK_OUTSIDE_ALLOWED = 'LINK_OUTSIDE_ALLOWED'; -const LINK_OUTSIDE_ALLOWED_MESSAGE = - 'That folder is inside a link that points outside your allowed locations. Choose the real folder instead.'; +// Returned when the folder's spelling is acceptable but a symlink, junction or +// 8.3 short name along it leads into a system directory. The renderer shows +// the message as is: the code avoids the substrings (PATH, FOLDER, FILE, ...) +// that its error mapping would replace with generic copy. +const LINK_INTO_SYSTEM = 'LINK_INTO_SYSTEM'; +const LINK_INTO_SYSTEM_MESSAGE = + 'That folder leads into a system folder through a link or short name. Choose a different folder.'; // Only runs after a refusal; bounded because a link can lead to a network // mount whose realpath hangs. const LINK_RESOLVE_TIMEOUT_MS = 2000; @@ -47,20 +45,18 @@ const LINK_RESOLVE_TIMEOUT_MS = 2000; /** * Was this create-folder path refused only because of where a link along it * leads? True when the path as spelled passes every lexical check, but its - * real location (links and junctions followed) is a different place that is - * outside the allowed bases or a system directory. False for anything else, - * including a check that timed out, so those keep the generic refusal. + * real location (links, junctions and short names resolved) is a different + * place inside a system directory. False for anything else, including a + * check that timed out, so those keep the generic refusal. * * @param {string} normalizedPath - * @param {string[]|null} allowedBasePaths * @returns {Promise} */ -async function wasRefusedForLinkTarget(normalizedPath, allowedBasePaths) { +async function wasRefusedForLinkTarget(normalizedPath) { /** @type {ReturnType|undefined} */ let timer; try { const asSpelled = await validateFileOperationPath(normalizedPath, { - allowedBasePaths, checkSymlinks: false, disallowUNC: false }); @@ -80,8 +76,7 @@ async function wasRefusedForLinkTarget(normalizedPath, allowedBasePaths) { const fold = (/** @type {string} */ p) => (caseInsensitive ? p.toLowerCase() : p); if (fold(path.resolve(real)) === fold(normalizedPath)) return false; - if (isPathDangerous(real)) return true; - return Boolean(allowedBasePaths) && !isPathWithinAllowed(real, allowedBasePaths || []); + return isPathDangerous(real); } catch { return false; } finally { @@ -126,8 +121,8 @@ function registerFolderHandlers(servicesOrParams) { // Reject relative paths up front. `path.resolve(relative)` would // otherwise silently mix in `process.cwd()` (the Electron // install directory on packaged builds, or wherever the app - // was spawned from), letting a caller reach files outside the - // allowed base paths if cwd happens to sit under one. + // was spawned from), so the folder would land somewhere the + // caller never named. if (!path.isAbsolute(fullPath)) { logger.warn('[FILE-OPS] Folder creation rejected - non-absolute path', { path: fullPath @@ -141,55 +136,50 @@ function registerFolderHandlers(servicesOrParams) { const normalizedPath = path.resolve(fullPath); - // Validate path is within allowed directories to prevent path traversal - const allowedPaths = [ - os.homedir(), - app.getPath('documents'), - app.getPath('downloads'), - app.getPath('desktop') - ]; - const isUNC = isUNCPath(normalizedPath); + // Any drive passes, an external one included. The path must not be + // a system directory, as spelled or once links and short names are + // resolved; where the folder may go is the destination policy's + // decision, below. const validation = await validateFileOperationPath(normalizedPath, { - allowedBasePaths: isUNC ? null : allowedPaths, checkSymlinks: true, disallowUNC: false }); if (!validation.valid) { - // A folder inside a link (symlink or junction) that leads outside - // the allowed locations is refused on its real location. Say so, + // A folder inside a link (symlink or junction) that leads into a + // system directory is refused on its real location. Say so, // instead of the generic refusal that reads like a typo. The log // keeps the spelled path only (the logger reduces it to its // name), never the link's target. - if (await wasRefusedForLinkTarget(normalizedPath, isUNC ? null : allowedPaths)) { - logger.warn('[FILE-OPS] Folder creation blocked - link leads outside allowed', { + if (await wasRefusedForLinkTarget(normalizedPath)) { + logger.warn('[FILE-OPS] Folder creation blocked - link leads into a system folder', { path: normalizedPath, - errorCode: LINK_OUTSIDE_ALLOWED + errorCode: LINK_INTO_SYSTEM }); return { success: false, - error: LINK_OUTSIDE_ALLOWED_MESSAGE, - errorCode: LINK_OUTSIDE_ALLOWED + error: LINK_INTO_SYSTEM_MESSAGE, + errorCode: LINK_INTO_SYSTEM }; } - logger.warn('[FILE-OPS] Folder creation blocked - path traversal attempt', { + logger.warn('[FILE-OPS] Folder creation blocked - path failed validation', { path: fullPath, normalized: normalizedPath, error: validation.error }); return { success: false, - error: validation.error || 'Path is outside allowed directories', + error: validation.error || 'That folder path cannot be used', errorCode: 'PATH_NOT_ALLOWED' }; } - // The allowlist above spans the whole home directory. The parent - // the folder goes into is held to the destination policy as a - // file would be: Documents and the trusted roots need no prompt, a - // startup folder is refused, anywhere else is confirmed by the user. + // The parent the folder goes into is held to the destination policy + // as a file would be: Documents and the trusted roots need no + // prompt, a startup folder is refused, anywhere else is confirmed + // by the user. /** @type {string[]} */ const knownRoots = []; try { diff --git a/src/main/ipc/smartFolders.js b/src/main/ipc/smartFolders.js index d53e962..0929947 100644 --- a/src/main/ipc/smartFolders.js +++ b/src/main/ipc/smartFolders.js @@ -23,12 +23,7 @@ const { enrichFileTextForEmbedding } = require('../analysis/semanticExtensionMap'); -// Import centralized security configuration -const { ALLOWED_APP_PATHS } = require('../../shared/securityConfig'); -const { - validateFileOperationPathSync, - validateFileOperationPath -} = require('../../shared/pathSanitization'); +const { validateFileOperationPath } = require('../../shared/pathSanitization'); const { renameNoReplace } = require('../../shared/atomicFileOperations'); const { authorizeSmartFolderRoot } = require('./files/destinationPolicy'); const { buildFolderRenamePathChanges } = require('../utils/folderRenamePathChanges'); @@ -311,77 +306,40 @@ async function syncSmartFolderEmbeddings({ } /** - * CRITICAL SECURITY FIX: Sanitize and validate folder paths to prevent path traversal attacks - * @param {string} inputPath - User-provided path to validate - * @returns {string} - Sanitized, validated path - * @throws {Error} - If path is invalid or violates security constraints + * Check a folder path from the renderer before it is scanned or saved as a + * smart folder. The path must be absolute, free of traversal, not a URL or a + * Windows device path, and not a system directory, either as spelled or once + * the links and 8.3 short names along it are resolved. + * + * Any drive passes, so a folder on an external or secondary drive is as good + * as one under the home directory. Whether a location may become a smart + * folder is decided afterwards by the destination policy + * ({@link vetSmartFolderRoot}), not here. + * + * @param {string} inputPath - Path from the renderer + * @returns {Promise} The normalized path, in the caller's spelling + * @throws {Error} When the path fails a check */ -function sanitizeFolderPath(inputPath) { - // Get allowed base paths from centralized config - const ALLOWED_BASE_PATHS = ALLOWED_APP_PATHS.map((appPath) => { - try { - return app.getPath(/** @type {Parameters[0]} */ (appPath)); - } catch { - return null; - } - }).filter(Boolean); - - // Network shares are exempt from the allowed-base containment below, so a - // DOS device path (`\\.\C:\Temp`, `\\?\C:\Temp`) must not be mistaken for - // one: it would name any local drive while skipping containment entirely. +async function sanitizeFolderPath(inputPath) { + // A DOS device path (`\\.\C:\Temp`, `\\?\C:\Temp`) names a local drive in + // the shape of a network share. Shares skip the real-location check below, + // so the device form is refused outright. if (isWindowsDeviceNamespacePath(inputPath)) { throw new Error('Invalid path: Windows device namespace paths are not allowed'); } - const isUNC = isUNCPath(inputPath); - - const validation = validateFileOperationPathSync( - inputPath, - isUNC ? null : ALLOWED_BASE_PATHS, // Bypass allowed base paths for UNC - { - requireAbsolute: true, - disallowUNC: false, // Allow UNC paths - disallowUrlSchemes: true - } - ); - - if (validation.valid) { - return validation.normalizedPath; - } - - // Cross-platform fallback: allow POSIX-style paths on Windows in test/mocked environments. - if ( - process.platform === 'win32' && - typeof inputPath === 'string' && - inputPath.startsWith('/') && - ALLOWED_BASE_PATHS.some((base) => typeof base === 'string' && base.startsWith('/')) - ) { - const normalized = path.posix.normalize(inputPath); - if (!path.posix.isAbsolute(normalized)) { - throw new Error('Invalid path: must be absolute'); - } - const parts = normalized.split('/').filter(Boolean); - if (parts.some((part) => part === '..')) { - throw new Error('Invalid path: path traversal detected'); - } - const allowed = ALLOWED_BASE_PATHS.some((base) => { - if (!base || !base.startsWith('/')) return false; - const baseNormalized = path.posix.normalize(base); - if (normalized === baseNormalized) return true; - return normalized.startsWith(`${baseNormalized}/`); - }); - if (!allowed) { - throw new Error( - 'Invalid path: must be within allowed directories (Documents, Downloads, Desktop, Pictures, Videos, Music, or Home)' - ); - } - return normalized; + const validation = await validateFileOperationPath(inputPath, { + requireAbsolute: true, + disallowUNC: false, + disallowUrlSchemes: true, + // A share is judged as spelled: resolving one that is offline hangs until + // the check times out, and a timed-out check refuses the path. + checkSymlinks: !isUNCPath(inputPath) + }); + if (!validation.valid) { + throw new Error(validation.error || 'Invalid path'); } - - throw new Error( - validation.error || - 'Invalid path: must be within allowed directories (Documents, Downloads, Desktop, Pictures, Videos, Music, or Home)' - ); + return validation.normalizedPath; } const { IpcServiceContext, createFromLegacyParams } = require('./IpcServiceContext'); @@ -642,7 +600,7 @@ async function handleSmartFoldersScanStructure({ logger, scanDirectory }, event, // CRITICAL SECURITY FIX: Sanitize path to prevent directory traversal let sanitizedPath; try { - sanitizedPath = sanitizeFolderPath(rootPath); + sanitizedPath = await sanitizeFolderPath(rootPath); } catch (securityError) { return { success: false, @@ -797,7 +755,7 @@ async function handleSmartFoldersAdd( // CRITICAL SECURITY FIX: Sanitize path before any operations let normalizedPath; try { - normalizedPath = sanitizeFolderPath(folder.path); + normalizedPath = await sanitizeFolderPath(folder.path); if (isUNCPath(normalizedPath)) { logger.info('[SMART-FOLDERS] UNC path accepted for new folder:', normalizedPath); } @@ -1509,7 +1467,7 @@ async function handleSmartFoldersEdit( if (updatedFolder.path) { try { // CRITICAL SECURITY FIX: Sanitize path before any operations - const normalizedPath = sanitizeFolderPath(updatedFolder.path); + const normalizedPath = await sanitizeFolderPath(updatedFolder.path); if (isUNCPath(normalizedPath)) { logger.info('[SMART-FOLDERS] UNC path accepted for folder update:', normalizedPath); } @@ -1775,7 +1733,7 @@ async function handleSmartFoldersUpdateCustom( for (const folder of folders) { if (folder.path) { try { - folder.path = sanitizeFolderPath(folder.path); + folder.path = await sanitizeFolderPath(folder.path); } catch (sanitizeError) { return { success: false, @@ -1890,7 +1848,7 @@ async function handleSmartFoldersSave( for (const folder of folders) { if (folder.path) { try { - folder.path = sanitizeFolderPath(folder.path); + folder.path = await sanitizeFolderPath(folder.path); } catch (sanitizeError) { return { success: false, diff --git a/src/main/services/OrganizeResumeService.js b/src/main/services/OrganizeResumeService.js index da2c442..39b4815 100644 --- a/src/main/services/OrganizeResumeService.js +++ b/src/main/services/OrganizeResumeService.js @@ -337,6 +337,11 @@ async function resumeBatch( } const total = batch.operations.length; + // Files this replay has still to move: the duplicate check must not count + // one of them as a copy already in its destination folder. + const pendingSources = batch.operations + .filter((/** @type {any} */ op) => op.status !== 'done' && typeof op.source === 'string') + .map((/** @type {any} */ op) => op.source); for (let i = 0; i < total; i += 1) { const op = batch.operations[i]; if (op.status === 'done') { @@ -479,7 +484,8 @@ async function resumeBatch( logger, logPrefix: '[RESUME]', dedupContext: 'organizeResume', - removeEmbeddings: removeEmbeddingsForPathBestEffort + removeEmbeddings: removeEmbeddingsForPathBestEffort, + pendingSources }); if (duplicateResult) { finalDestination = duplicateResult.destination; diff --git a/src/main/services/triage/DownloadsTriageService.js b/src/main/services/triage/DownloadsTriageService.js index bcc2a93..e607a2c 100644 --- a/src/main/services/triage/DownloadsTriageService.js +++ b/src/main/services/triage/DownloadsTriageService.js @@ -1178,6 +1178,12 @@ class DownloadsTriageService { .filter((p) => typeof p === 'string' && p.length > 0) .map((p) => path.resolve(p)); + // Every file this action still has to move: the duplicate check must + // not count one of them as a copy already in its destination folder. + const pendingSources = filesInSession + .map((f) => f?.path) + .filter((p) => typeof p === 'string' && p.length > 0); + for (const f of filesInSession) { const srcRaw = f?.path; const destDirRaw = f?.suggestedFolderPath; @@ -1253,7 +1259,7 @@ class DownloadsTriageService { await fs.mkdir(path.dirname(op.destination), { recursive: true }); const moveResult = await withTimeout( - performFileMove(op, logger, computeFileChecksum), + performFileMove(op, logger, computeFileChecksum, null, pendingSources), TIMEOUTS.FILE_COPY || 120000, `Triage organize ${path.basename(op.source)}` ); diff --git a/src/main/utils/fileDedup.js b/src/main/utils/fileDedup.js index e91354a..9a438e0 100644 --- a/src/main/utils/fileDedup.js +++ b/src/main/utils/fileDedup.js @@ -115,12 +115,44 @@ async function statSafe(/** @type {any} */ filePath) { function pathsAreSameFilePath(a, b) { if (a === b) return true; if (typeof a !== 'string' || typeof b !== 'string') return false; - const na = path.normalize(a); - const nb = path.normalize(b); - if (isCaseInsensitiveFileSystem()) { - return na.toLowerCase() === nb.toLowerCase(); + return filePathKey(a) === filePathKey(b); +} + +/** + * Comparison key for a path: separators normalized, Unicode composed (macOS + * can list a name decomposed while the app holds it composed), case folded + * where the filesystem ignores case. Errs toward "same" for the reason given + * above. + * @param {string} filePath + * @returns {string} + */ +function filePathKey(filePath) { + const normalized = path.normalize(filePath).normalize('NFC'); + return isCaseInsensitiveFileSystem() ? normalized.toLowerCase() : normalized; +} + +/** @type {WeakMap>} */ +const pendingSourceKeyCache = new WeakMap(); + +/** + * Whether `filePath` is one of the files the caller's batch has still to + * move. Keys are built once per `pendingSources` array, so a batch passes the + * same array for every move. + * @param {string[] | null | undefined} pendingSources + * @param {string} filePath + * @returns {boolean} + */ +function isPendingSource(pendingSources, filePath) { + if (!Array.isArray(pendingSources) || pendingSources.length === 0) return false; + let keys = pendingSourceKeyCache.get(pendingSources); + if (!keys) { + keys = new Set(); + for (const source of pendingSources) { + if (typeof source === 'string' && source) keys.add(filePathKey(source)); + } + pendingSourceKeyCache.set(pendingSources, keys); } - return na === nb; + return keys.has(filePathKey(filePath)); } /** @@ -183,6 +215,7 @@ function isDirectoryStat(/** @type {any} */ stat) { * @param {number} [params.maxCandidates] * @param {number} [params.maxDirEntries] * @param {any} [params.logger] + * @param {string[]} [params.pendingSources] - see handleDuplicateMove * @returns {Promise} Path to duplicate if found. */ /** @param {any} params */ @@ -194,7 +227,8 @@ async function findDuplicateInDirectory({ maxDirEntries = DEFAULTS.maxDirEntries, logger, returnSourceHash = false, - hashCache = resolveHashCache() + hashCache = resolveHashCache(), + pendingSources }) { if (!sourcePath || !destinationDir) return null; const sourceStat = await statSafe(sourcePath); @@ -222,6 +256,7 @@ async function findDuplicateInDirectory({ if (!entry.isFile()) continue; const fullPath = path.join(destinationDir, entry.name); if (pathsAreSameFilePath(fullPath, sourcePath)) continue; + if (isPendingSource(pendingSources, fullPath)) continue; const stat = await statSafe(fullPath); if (!isFileStat(stat)) continue; @@ -278,6 +313,7 @@ async function findDuplicateInDirectory({ * @param {any} [params.logger] * @param {number} [params.maxCandidates] * @param {number} [params.maxDirEntries] + * @param {string[]} [params.pendingSources] - see handleDuplicateMove * @returns {Promise} */ /** @param {any} params */ @@ -289,7 +325,8 @@ async function findDuplicateForDestination({ maxCandidates, maxDirEntries, returnSourceHash = false, - hashCache = resolveHashCache() + hashCache = resolveHashCache(), + pendingSources }) { if (!sourcePath || !destinationPath) return null; @@ -303,7 +340,11 @@ async function findDuplicateForDestination({ if (!isFileStat(sourceStat)) return null; const destStat = await statSafe(destinationPath); - if (isFileStat(destStat) && !isSameFileByStat(destStat, sourceStat)) { + if ( + isFileStat(destStat) && + !isSameFileByStat(destStat, sourceStat) && + !isPendingSource(pendingSources, destinationPath) + ) { try { const [sourceHash, destHash] = await Promise.all([ checksumFn(sourcePath, statForChecksum(sourceStat, hashCache)), @@ -324,6 +365,12 @@ async function findDuplicateForDestination({ } const destinationDir = path.dirname(destinationPath); + // A rename within one folder adds nothing to that folder, so an identical + // file elsewhere in it is no reason to remove the source: the user would + // lose one of two files they already kept side by side. Two identical + // files renamed in place in one batch each took the other for the kept + // copy and both went to the trash. + if (pathsAreSameFilePath(destinationDir, path.dirname(sourcePath))) return null; const dirStat = await statSafe(destinationDir); if (!isDirectoryStat(dirStat)) return null; @@ -335,11 +382,24 @@ async function findDuplicateForDestination({ maxDirEntries, logger, returnSourceHash, - hashCache + hashCache, + pendingSources }); } -/** @param {any} params */ +/** + * Remove the source instead of moving it when identical content is already + * at the destination (the exact path, or any file in its folder). + * + * `pendingSources` lists the sources the caller's batch has still to move. + * None of them counts as a copy already at the destination: it may be moved + * or renamed away later in the same batch, leaving nothing where the removed + * source was said to be. Pass the same array for every move of the batch. + * + * @param {any} params + * @returns {Promise<{ skipped: true, destination: string, reason: 'duplicate', sourceRemoval: string } | null>} + * null when the caller should move the file. + */ async function handleDuplicateMove({ sourcePath, destinationPath, @@ -349,7 +409,8 @@ async function handleDuplicateMove({ dedupContext = 'unknown', removeEmbeddings, unlinkFn, - hashCache = resolveHashCache() + hashCache = resolveHashCache(), + pendingSources }) { const duplicateMatch = await findDuplicateForDestination({ sourcePath, @@ -357,7 +418,8 @@ async function handleDuplicateMove({ checksumFn, logger, returnSourceHash: true, - hashCache + hashCache, + pendingSources }); const duplicatePath = duplicateMatch?.path; if (!duplicatePath) return null; diff --git a/src/renderer/components/organize/InPlaceMoveNotice.jsx b/src/renderer/components/organize/InPlaceMoveNotice.jsx new file mode 100644 index 0000000..9927ae8 --- /dev/null +++ b/src/renderer/components/organize/InPlaceMoveNotice.jsx @@ -0,0 +1,60 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import { AlertBox } from '../ui'; + +const FOLDERS_SHOWN = 2; + +/** "Bing data", "Bing data and GSC data", "A, B and 2 more". */ +function listFolders(names) { + const shown = names.slice(0, FOLDERS_SHOWN); + const more = names.length - shown.length; + if (more > 0) return `${shown.join(', ')} and ${more} more`; + return shown.join(' and '); +} + +/** + * Says, before a run, which ready files will not change folder: the smart + * folder chosen for them is the folder they are in. Without it the run + * reports them as moved and nothing appears anywhere new. + */ +function InPlaceMoveNotice({ moves, className = '' }) { + if (!Array.isArray(moves) || moves.length === 0) return null; + + const one = moves.length === 1; + const folders = Array.from(new Set(moves.map((m) => m.folderName).filter(Boolean))); + const renamedCount = moves.filter((m) => m.renamed).length; + let outcome; + if (renamedCount === moves.length) { + outcome = `${one ? 'It' : 'They'} will be renamed there, not moved to another folder.`; + } else if (renamedCount === 0) { + outcome = `${one ? 'It' : 'They'} will stay as ${one ? 'it is' : 'they are'}.`; + } else { + outcome = 'They will stay where they are, and the ones with a new name are renamed there.'; + } + + return ( + + + {one ? '1 file is' : `${moves.length} files are`} already in the folder chosen for{' '} + {one ? 'it' : 'them'} + {folders.length > 0 ? `: ${listFolders(folders)}` : ''} + +
+ {outcome} To send a file somewhere else, choose a different folder on its card. Preview + marks the files that stay. +
+
+ ); +} + +InPlaceMoveNotice.propTypes = { + moves: PropTypes.arrayOf( + PropTypes.shape({ + folderName: PropTypes.string, + renamed: PropTypes.bool + }) + ), + className: PropTypes.string +}; + +export default InPlaceMoveNotice; diff --git a/src/renderer/components/organize/OrganizePreviewModal.jsx b/src/renderer/components/organize/OrganizePreviewModal.jsx index 2f5d877..f7c1cbf 100644 --- a/src/renderer/components/organize/OrganizePreviewModal.jsx +++ b/src/renderer/components/organize/OrganizePreviewModal.jsx @@ -6,6 +6,7 @@ import { Button, StatusBadge } from '../ui'; import { Text } from '../ui/Typography'; import { Inline } from '../layout'; import { formatDisplayFileName, formatDisplayPath } from '../../utils/pathDisplay'; +import { toParentFolderKey } from '../../utils/canonicalPath'; const SKIP_REASONS = { 'source-missing': 'File is no longer at its original location', @@ -22,8 +23,9 @@ function splitPath(p) { /** * Shows the dry-run plan from previewBatchOrganize before anything moves: * every file grouped under the folder it will land in, its new name when the - * move renames it, folders that will be created, and name clashes the move - * will resolve with a numeric suffix. + * move renames it, folders that will be created, name clashes the move will + * resolve with a numeric suffix, and files that are already in the folder + * chosen for them and so do not change folder. */ function OrganizePreviewModal({ isOpen, plan, onClose, onConfirm, redactPaths, isOrganizing }) { const { groups, newFolders } = useMemo(() => { @@ -34,11 +36,14 @@ function OrganizePreviewModal({ isOpen, plan, onClose, onConfirm, redactPaths, i const dest = splitPath(item?.destination); const src = splitPath(item?.source); const list = byFolder.get(dest.folder) || []; + const fromFolderKey = toParentFolderKey(item?.source); list.push({ key: item?.source || `${dest.folder}|${dest.name}`, fromName: src.name, toName: dest.name, - collision: Boolean(item?.collision) + collision: Boolean(item?.collision), + staysInFolder: + fromFolderKey !== null && fromFolderKey === toParentFolderKey(item?.destination) }); byFolder.set(dest.folder, list); } @@ -129,10 +134,19 @@ function OrganizePreviewModal({ isOpen, plan, onClose, onConfirm, redactPaths, i )} - {f.collision && ( - - Name taken — number added - + {(f.staysInFolder || f.collision) && ( + + {f.staysInFolder && ( + + {renamed ? 'Already in this folder' : 'Already here — no change'} + + )} + {f.collision && ( + + Name taken — number added + + )} + )} ); diff --git a/src/renderer/components/organize/OrganizeSummaryBanner.jsx b/src/renderer/components/organize/OrganizeSummaryBanner.jsx new file mode 100644 index 0000000..b0ebd99 --- /dev/null +++ b/src/renderer/components/organize/OrganizeSummaryBanner.jsx @@ -0,0 +1,82 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import { AlertCircle, AlertTriangle, CheckCircle2, X } from 'lucide-react'; +import { IconButton } from '../ui'; +import { Text } from '../ui/Typography'; + +const TONES = { + success: { + container: 'bg-stratosort-success/10 border-stratosort-success/25', + icon: 'text-stratosort-success', + Icon: CheckCircle2 + }, + warning: { + container: 'bg-stratosort-warning/12 border-stratosort-warning/30', + icon: 'text-stratosort-warning', + Icon: AlertTriangle + }, + danger: { + container: 'bg-stratosort-danger/10 border-stratosort-danger/30', + icon: 'text-stratosort-danger', + Icon: AlertCircle + } +}; + +const TONE_BY_OUTCOME = { + moved: 'success', + partial: 'warning', + none: 'danger', + rolledBack: 'danger' +}; + +/** How the last organize run ended, shown above the file list. */ +function OrganizeSummaryBanner({ summary, onDismiss }) { + const toneName = TONE_BY_OUTCOME[summary.outcome] || 'success'; + const { container, icon, Icon } = TONES[toneName]; + const urgent = toneName === 'danger'; + const detail = + summary.detail || + (summary.outcome === 'moved' + ? 'You can review organized files in the status panel or continue with the next batch.' + : ''); + + return ( +
+
+ ); +} + +OrganizeSummaryBanner.propTypes = { + summary: PropTypes.shape({ + outcome: PropTypes.oneOf(['moved', 'partial', 'none', 'rolledBack']).isRequired, + headline: PropTypes.string.isRequired, + detail: PropTypes.string + }).isRequired, + onDismiss: PropTypes.func.isRequired +}; + +export default OrganizeSummaryBanner; diff --git a/src/renderer/components/organize/VirtualizedProcessedFiles.jsx b/src/renderer/components/organize/VirtualizedProcessedFiles.jsx index 3abeaed..b91eb51 100644 --- a/src/renderer/components/organize/VirtualizedProcessedFiles.jsx +++ b/src/renderer/components/organize/VirtualizedProcessedFiles.jsx @@ -7,6 +7,7 @@ import { Text } from '../ui/Typography'; import { UI_VIRTUALIZATION } from '../../../shared/constants'; import { selectRedactPaths } from '../../store/selectors'; import { formatDisplayFileName } from '../../utils/pathDisplay'; +import { describeDuplicateRemoval, isRemovedDuplicate } from '../../utils/duplicateRemoval'; const ITEM_HEIGHT = UI_VIRTUALIZATION.PROCESSED_FILES_ITEM_HEIGHT; const VIRTUALIZATION_THRESHOLD = UI_VIRTUALIZATION.THRESHOLD; @@ -23,9 +24,6 @@ const getListHeight = (itemCount, viewportHeight) => { return Math.max(ITEM_HEIGHT * 2, Math.min(contentHeight, maxHeight)); }; -/** - * Individual processed file row component - */ /** * Format date safely with fallback */ @@ -38,37 +36,82 @@ const formatDate = (dateString) => { } }; -const ProcessedFileRow = memo(function ProcessedFileRow({ index, style, data }) { - const { files, redactPaths } = data || {}; - const file = files && files[index]; - - if (!file) return null; - +/** + * One organized-history entry. Both the virtualized and the plain list render + * it, so the two cannot drift apart. + */ +function ProcessedFileCard({ file, redactPaths, fill = false }) { const originalName = formatDisplayFileName(file.originalName || 'Unknown', { redact: redactPaths }); const newName = formatDisplayFileName(file.newName || 'Unknown', { redact: redactPaths }); const smartFolder = redactPaths ? 'destination hidden' : file.smartFolder || 'Unknown folder'; const organizedDate = formatDate(file.organizedAt); + // Not moved: the destination already held an identical copy. + const duplicate = isRemovedDuplicate(file); + const outcome = duplicate + ? `Not moved: an identical copy is already in ${ + redactPaths ? 'the destination' : smartFolder + }. ${describeDuplicateRemoval(file)}` + : redactPaths + ? 'Moved successfully' + : `Moved to ${smartFolder}`; return ( -
-
-
+
+
+ {duplicate ? ( + Duplicate + ) : ( OK -
- - {originalName} -> {newName} - - - {redactPaths ? 'Moved successfully' : `Moved to ${smartFolder}`} | {organizedDate} - -
+ )} +
+ + {duplicate ? originalName : `${originalName} -> ${newName}`} + + + {outcome} | {organizedDate} +
- - Organized -
+ + {duplicate ? 'Not moved' : 'Organized'} + +
+ ); +} + +ProcessedFileCard.propTypes = { + file: PropTypes.object.isRequired, + redactPaths: PropTypes.bool, + fill: PropTypes.bool +}; + +/** + * Individual processed file row component + */ +const ProcessedFileRow = memo(function ProcessedFileRow({ index, style, data }) { + const { files, redactPaths } = data || {}; + const file = files && files[index]; + + if (!file) return null; + + return ( +
+
); }); @@ -147,15 +190,6 @@ function VirtualizedProcessedFiles({ files, isLoading = false }) { return (
{files.map((file, index) => { - const originalName = formatDisplayFileName(file.originalName || 'Unknown', { - redact: redactPaths - }); - const newName = formatDisplayFileName(file.newName || 'Unknown', { redact: redactPaths }); - const smartFolder = redactPaths - ? 'destination hidden' - : file.smartFolder || 'Unknown folder'; - const organizedDate = formatDate(file.organizedAt); - // Key precedence: originalPath (most specific) → originalPath+newName // composite (handles duplicate originalPath after re-process) → name + // timestamp + index tiebreaker (handles missing path AND collisions @@ -164,29 +198,9 @@ function VirtualizedProcessedFiles({ files, isLoading = false }) { // reorder, so index stability is fine. const stableKey = file.originalPath ? `${file.originalPath}|${file.newName || ''}` - : `${originalName}|${file.organizedAt || ''}|${index}`; + : `${file.originalName || 'Unknown'}|${file.organizedAt || ''}|${index}`; - return ( -
-
- OK -
- - {originalName} -> {newName} - - - {redactPaths ? 'Moved successfully' : `Moved to ${smartFolder}`} | {organizedDate} - -
-
- - Organized - -
- ); + return ; })}
); @@ -199,7 +213,8 @@ VirtualizedProcessedFiles.propTypes = { originalName: PropTypes.string, newName: PropTypes.string, smartFolder: PropTypes.string, - organizedAt: PropTypes.string + organizedAt: PropTypes.string, + duplicateRemoval: PropTypes.oneOf(['trashed', 'deleted']) }) ).isRequired, isLoading: PropTypes.bool diff --git a/src/renderer/components/organize/index.js b/src/renderer/components/organize/index.js index 6052837..bfc69a6 100644 --- a/src/renderer/components/organize/index.js +++ b/src/renderer/components/organize/index.js @@ -4,6 +4,8 @@ export { default as TargetFolderList } from './TargetFolderList'; export { default as ReadyFileItem } from './ReadyFileItem'; export { default as BulkOperations } from './BulkOperations'; export { default as OrganizeProgress } from './OrganizeProgress'; +export { default as OrganizeSummaryBanner } from './OrganizeSummaryBanner'; +export { default as InPlaceMoveNotice } from './InPlaceMoveNotice'; export { default as OrganizationSuggestions } from './OrganizationSuggestions'; export { default as BatchOrganizationSuggestions } from './BatchOrganizationSuggestions'; export { default as FolderImprovementSuggestions } from './FolderImprovementSuggestions'; diff --git a/src/renderer/components/ui/AlertBox.jsx b/src/renderer/components/ui/AlertBox.jsx index ae57fa8..a1b79e3 100644 --- a/src/renderer/components/ui/AlertBox.jsx +++ b/src/renderer/components/ui/AlertBox.jsx @@ -41,12 +41,15 @@ const DEFAULT_ICONS = { * @param {React.ElementType} icon - Override the default icon for the variant * @param {React.ReactNode} children - Alert content (text or elements) * @param {string} className - Additional CSS classes + * @param {'alert'|'status'} role - 'status' for standing advice that should not + * interrupt a screen reader; the default announces at once */ const AlertBox = memo(function AlertBox({ variant = 'info', icon: IconOverride, children, - className = '' + className = '', + role = 'alert' }) { const styles = VARIANT_STYLES[variant] || VARIANT_STYLES.info; const Icon = IconOverride || DEFAULT_ICONS[variant] || Info; @@ -54,7 +57,7 @@ const AlertBox = memo(function AlertBox({ return (
@@ -68,7 +71,8 @@ AlertBox.propTypes = { variant: PropTypes.oneOf(['info', 'warning', 'danger', 'success']), icon: PropTypes.elementType, children: PropTypes.node.isRequired, - className: PropTypes.string + className: PropTypes.string, + role: PropTypes.oneOf(['alert', 'status']) }; export default AlertBox; diff --git a/src/renderer/phases/CompletePhase.jsx b/src/renderer/phases/CompletePhase.jsx index 083a538..8529952 100644 --- a/src/renderer/phases/CompletePhase.jsx +++ b/src/renderer/phases/CompletePhase.jsx @@ -1,7 +1,7 @@ import React, { memo, useCallback, useMemo } from 'react'; import PropTypes from 'prop-types'; import { motion, useReducedMotion } from 'framer-motion'; -import { CheckCircle2, Check, ArrowLeft, FolderOpen, FilePlus } from 'lucide-react'; +import { CheckCircle2, Check, Copy, ArrowLeft, FolderOpen, FilePlus } from 'lucide-react'; import { PHASES } from '../../shared/constants'; import { useAppSelector, useAppDispatch } from '../store/hooks'; import { setPhase, resetUi } from '../store/slices/uiSlice'; @@ -20,6 +20,7 @@ import { logger } from '../../shared/logger'; import { toCanonicalPath } from '../utils/canonicalPath'; import { collectOrganizedPathKeys, mergeOrganizedPathIndex } from '../utils/organizedPathIndex'; import { useFileActions } from '../hooks/useFileActions'; +import { describeDuplicateRemoval, isRemovedDuplicate } from '../utils/duplicateRemoval'; /** * Pick a folder path safe to hand to shell.openPath — prefers `newLocation` @@ -62,23 +63,40 @@ const FileRow = memo(function FileRow({ file, index, redactPaths, onReveal }) { : 'Organized'; // Say where the file went and, when the move renamed it, what it is called // now — the original name alone does not tell the user what to look for. - const destinationLine = - renamed && !redactPaths ? `Renamed to ${newName} · in ${displayFolder}` : displayFolder; - const rowTitle = redactPaths - ? `${displayName} -> ${displayFolder}` - : `${originalName} → ${filePath || displayFolder}`; + // A removed duplicate never went anywhere: `path` is the copy that was kept. + const duplicate = isRemovedDuplicate(file); + const destinationLine = duplicate + ? `Not moved: an identical copy is already in ${ + redactPaths ? 'the destination' : displayFolder + }. ${describeDuplicateRemoval(file)}` + : renamed && !redactPaths + ? `Renamed to ${newName} · in ${displayFolder}` + : displayFolder; + const rowTitle = duplicate + ? displayName + : redactPaths + ? `${displayName} -> ${displayFolder}` + : `${originalName} → ${filePath || displayFolder}`; return (
- + {duplicate ? ( + + ) : ( + + )}
{displayName} - + {destinationLine}
@@ -88,8 +106,12 @@ const FileRow = memo(function FileRow({ file, index, redactPaths, onReveal }) { variant="ghost" size="sm" className="flex-shrink-0" - aria-label={`Show ${displayName} in its folder`} - title="Show this file in its folder" + aria-label={ + duplicate + ? `Show the kept copy of ${displayName} in its folder` + : `Show ${displayName} in its folder` + } + title={duplicate ? 'Show the kept copy in its folder' : 'Show this file in its folder'} > @@ -116,44 +138,59 @@ function CompletePhase() { [organizedPathIndex, organizedFiles] ); - const { filesToRender, overflowCount, destinationCount, totalFiles, primaryDestination } = - useMemo(() => { - const safeFiles = Array.isArray(organizedFiles) ? organizedFiles : []; - const destinations = new Set(); - // Track folder counts separately from the destination set so the - // "Open organized folder" CTA can pick a real directory path, even if - // some entries fall back to the synthetic 'Organized' placeholder. - const folderCounts = new Map(); - safeFiles.forEach((file) => { - if (!file || typeof file !== 'object') return; - // A destination is a folder. Counting file paths made every file its - // own destination. - const folder = pickOpenableFolder(file); + const { + filesToRender, + overflowCount, + destinationCount, + totalFiles, + movedCount, + duplicateCount, + primaryDestination + } = useMemo(() => { + const safeFiles = Array.isArray(organizedFiles) ? organizedFiles : []; + const destinations = new Set(); + // Track folder counts separately from the destination set so the + // "Open organized folder" CTA can pick a real directory path, even if + // some entries fall back to the synthetic 'Organized' placeholder. + const folderCounts = new Map(); + let duplicates = 0; + safeFiles.forEach((file) => { + if (!file || typeof file !== 'object') return; + // A destination is a folder. Counting file paths made every file its + // own destination. + const folder = pickOpenableFolder(file); + if (isRemovedDuplicate(file)) { + // Nothing was moved there, so it is not a destination of this run. + duplicates += 1; + } else { destinations.add(folder ? toCanonicalPath(folder) : file.smartFolder || 'Organized'); - if (folder) { - folderCounts.set(folder, (folderCounts.get(folder) || 0) + 1); - } - }); + } + if (folder) { + folderCounts.set(folder, (folderCounts.get(folder) || 0) + 1); + } + }); - let topFolder = null; - let topCount = 0; - folderCounts.forEach((count, folder) => { - if (count > topCount) { - topCount = count; - topFolder = folder; - } - }); + let topFolder = null; + let topCount = 0; + folderCounts.forEach((count, folder) => { + if (count > topCount) { + topCount = count; + topFolder = folder; + } + }); - const displayed = safeFiles.slice(0, 12); - return { - filesToRender: displayed, - overflowCount: Math.max(organizedCount - displayed.length, 0), - destinationCount: destinations.size, - // Same source as Dashboard "Organized" (selectOrganizedCount). - totalFiles: organizedCount, - primaryDestination: topFolder - }; - }, [organizedFiles, organizedCount]); + const displayed = safeFiles.slice(0, 12); + return { + filesToRender: displayed, + overflowCount: Math.max(organizedCount - displayed.length, 0), + destinationCount: destinations.size, + // Same source as Dashboard "Organized" (selectOrganizedCount). + totalFiles: organizedCount, + movedCount: Math.max(organizedCount - duplicates, 0), + duplicateCount: duplicates, + primaryDestination: topFolder + }; + }, [organizedFiles, organizedCount]); // Analyzed files still waiting to organize — exclude already-moved paths so // the Complete screen does not treat remapped results as "skipped"/unmoved. @@ -176,10 +213,17 @@ function CompletePhase() { return 'This session did not move any files.'; } - return `${totalFiles} file${totalFiles !== 1 ? 's' : ''} moved into ${destinationCount} destination${ - destinationCount !== 1 ? 's' : '' - }.`; - }, [destinationCount, totalFiles]); + const moved = + movedCount > 0 + ? `${movedCount} file${movedCount !== 1 ? 's' : ''} moved into ${destinationCount} destination${ + destinationCount !== 1 ? 's' : '' + }.` + : 'No files needed moving.'; + if (duplicateCount === 0) return moved; + return `${moved} ${ + duplicateCount === 1 ? '1 identical copy was' : `${duplicateCount} identical copies were` + } removed instead of moved.`; + }, [destinationCount, duplicateCount, movedCount, totalFiles]); const actions = useMemo( () => ({ @@ -235,6 +279,15 @@ function CompletePhase() { ? `Opens ${redactPaths ? 'the folder' : primaryDestination} — it received the most files. Use the folder button on a row to see any other file.` : `Opens ${redactPaths ? 'the folder these files were moved to' : primaryDestination}`; + const listCaption = [ + movedCount > 0 ? `${movedCount} file${movedCount !== 1 ? 's' : ''} organized` : '', + duplicateCount > 0 + ? `${duplicateCount} duplicate${duplicateCount !== 1 ? 's' : ''} removed` + : '' + ] + .filter(Boolean) + .join(', '); + const showCelebration = totalFiles > 0 && !shouldReduceMotion; return ( @@ -288,11 +341,13 @@ function CompletePhase() { className="max-w-xl mx-auto" > - {totalFiles > 0 - ? `Your reviewed batch is complete. ${totalFiles} ${ - totalFiles === 1 ? 'file is' : 'files are' + {movedCount > 0 + ? `Your reviewed batch is complete. ${movedCount} ${ + movedCount === 1 ? 'file is' : 'files are' } now in place.` - : 'No files were organized in this session.'} + : totalFiles > 0 + ? 'Your reviewed batch is complete. Every file already had an identical copy in its destination folder.' + : 'No files were organized in this session.'} @@ -344,10 +399,16 @@ function CompletePhase() { > - {totalFiles > 0 && ( + {movedCount > 0 && ( - {totalFiles} file{totalFiles !== 1 ? 's' : ''} moved + {movedCount} file{movedCount !== 1 ? 's' : ''} moved + + )} + {duplicateCount > 0 && ( + + + {duplicateCount} duplicate{duplicateCount !== 1 ? 's' : ''} removed )} {analyzedCount > 0 && ( @@ -371,9 +432,9 @@ function CompletePhase() { Session summary - {totalFiles > 0 && ( + {listCaption && ( - {totalFiles} file{totalFiles !== 1 ? 's' : ''} organized + {listCaption} )}
@@ -465,7 +526,8 @@ FileRow.propTypes = { path: PropTypes.string, newName: PropTypes.string, newLocation: PropTypes.string, - destination: PropTypes.string + destination: PropTypes.string, + duplicateRemoval: PropTypes.oneOf(['trashed', 'deleted']) }), index: PropTypes.number.isRequired, redactPaths: PropTypes.bool, diff --git a/src/renderer/phases/OrganizePhase.jsx b/src/renderer/phases/OrganizePhase.jsx index cbf1e7d..f8aac2b 100644 --- a/src/renderer/phases/OrganizePhase.jsx +++ b/src/renderer/phases/OrganizePhase.jsx @@ -28,6 +28,8 @@ import { TargetFolderList, BulkOperations, OrganizeProgress, + OrganizeSummaryBanner, + InPlaceMoveNotice, VirtualizedFileGrid, VirtualizedProcessedFiles, UncategorizedActionsBar @@ -227,7 +229,9 @@ function OrganizePhase() { handleOrganizeFiles, handlePreviewOrganize, organizeConflicts, + inPlaceMoves, lastOrganizeSummary, + dismissOrganizeSummary, cancelOrganize, isCancelling, pauseOrganize, @@ -599,30 +603,7 @@ function OrganizePhase() { {lastOrganizeSummary && ( -
-
+ )} {/* Main Content */} @@ -790,6 +771,9 @@ function OrganizePhase() {
)} + {!isOrganizing && !isPaused && ( + + )} {(isOrganizing || isPaused || progressLinger) && (
`Skipped ${count} item${count > 1 ? 's' : ''} because no stable absolute path was available`; +/** + * A folder scan that main refuses, or that fails, resolves with + * `success: false` and no `files`; it does not reject. Give the reason to show + * for such a result, or null when the scan ran (an empty folder is a scan that + * ran). + * @param {any} scanResult + * @returns {string | null} + */ +const getScanFailureReason = (scanResult) => { + if (scanResult?.success !== false) return null; + return mapErrorToNotification({ error: scanResult.error, errorCode: scanResult.errorCode }) + .message; +}; + /** * The file picker offers "All Files", and main leaves unsupported, oversized * and unreadable picks out of `files`, listing them in `skippedFiles` with a @@ -413,6 +427,17 @@ export function useFileHandlers({ if (scanTimeoutId) clearTimeout(scanTimeoutId); }); + const failureReason = getScanFailureReason(scanResult); + if (failureReason) { + addNotification( + `Dropped folder not scanned: ${dir.name || 'Selected folder'}. ${failureReason}`, + 'error', + 5000, + 'drop-folder-error' + ); + continue; + } + if (scanResult?.partial) { addNotification( 'Folder scan returned partial results (slow drive or too many files). Try a smaller folder.', @@ -615,6 +640,17 @@ export function useFileHandlers({ if (scanTimeoutId) clearTimeout(scanTimeoutId); }); + const failureReason = getScanFailureReason(scanResult); + if (failureReason) { + addNotification( + `The selected folder was not scanned. ${failureReason}`, + 'error', + 5000, + 'folder-scan-error' + ); + return; + } + if (scanResult?.partial) { addNotification( 'Folder scan returned partial results (slow drive or too many files). Try a smaller folder.', diff --git a/src/renderer/phases/organize/useOrganization.js b/src/renderer/phases/organize/useOrganization.js index 63f27b2..6e6bb8a 100644 --- a/src/renderer/phases/organize/useOrganization.js +++ b/src/renderer/phases/organize/useOrganization.js @@ -7,7 +7,7 @@ * @module organize/useOrganization */ -import { useState, useCallback, useEffect, useRef } from 'react'; +import { useState, useCallback, useEffect, useMemo, useRef } from 'react'; import { IPC_CHANNELS, PHASES } from '../../../shared/constants'; import { createLogger } from '../../../shared/logger'; import { extractExtension } from '../../../shared/namingConventions'; @@ -42,15 +42,144 @@ function countOf(count, noun = 'file') { return `${count} ${count === 1 ? noun : `${noun}s`}`; } +/** + * True for a row main reports as done without a move: the destination already + * held the same content, so the source was removed instead. + * @param {any} row + * @returns {boolean} + */ +function isDuplicateSkip(row) { + return Boolean(row && row.success && row.skipped && row.reason === 'duplicate'); +} + +/** + * One sentence on the files that were removed as duplicates instead of + * moved, or '' when there were none. + * @param {any[]} duplicateRows + * @returns {string} + */ +function describeDuplicates(duplicateRows) { + const count = duplicateRows.length; + if (count === 0) return ''; + const one = count === 1; + const where = duplicateRows.every((r) => r.sourceRemoval === 'trashed') + ? `${one ? 'It is' : 'They are'} in the system Trash.` + : `${one ? 'It was' : 'They were'} removed.`; + return `${one ? '1 identical copy was' : `${count} identical copies were`} not moved: the destination already had the same content. ${where}`; +} + +/** + * How the last organize run ended, for the banner on the Organize screen. + * `count` is the number of files moved; a duplicate that was removed instead + * is not one of them. + * @typedef {object} OrganizeSummary + * @property {'moved' | 'partial' | 'none' | 'rolledBack'} outcome + * @property {number} count + * @property {string} headline + * @property {string} detail + */ + +// Main keeps an interrupted batch's journal open and OrganizeResumeService +// replays it at the next launch. +const INTERRUPTED_REASON = + 'The run was interrupted. The remaining moves resume the next time StratoSort starts.'; + +/** + * The toast and the banner for a run main did not roll back, or null when the + * run left nothing to report. A row that never started is not a failure: the + * cancel and pause messages cover the ones the user stopped, and an + * interrupted run is reported as unfinished. + * @param {object} run + * @param {any[]} run.rows - the batch's per-op results + * @param {number} run.movedCount + * @param {any[]} run.duplicateRows + * @param {string} [run.batchError] - the error that ended the batch early + * @param {boolean} [run.interrupted] - stopped by a timeout or shutdown + * @returns {{ summary: OrganizeSummary, toast: string, severity: string, duration: number } | null} + */ +function describeFinishedRun({ rows, movedCount, duplicateRows, batchError, interrupted }) { + const failedCount = rows.filter((r) => r && !r.success && !r.cancelled && !r.interrupted).length; + const duplicateNote = describeDuplicates(duplicateRows); + const tail = duplicateNote ? ` ${duplicateNote}` : ''; + + if (failedCount > 0 || batchError || interrupted) { + const reason = + failedCount === 0 && !batchError + ? INTERRUPTED_REASON + : describeFirstFailure( + batchError ? [...rows, { success: false, error: batchError }] : rows + ); + if (movedCount === 0) { + return { + summary: { + outcome: 'none', + count: 0, + headline: 'No files were organized', + detail: `${reason}${tail}` + }, + toast: `No files were organized. ${reason}${tail}`, + severity: 'error', + duration: 9000 + }; + } + const shortfall = + failedCount > 0 ? `${failedCount} could not be moved` : 'the run did not finish'; + return { + summary: { + outcome: 'partial', + count: movedCount, + headline: `Moved ${countOf(movedCount)}; ${shortfall}`, + detail: `${reason}${tail} Files that were not moved are still listed.` + }, + toast: + failedCount > 0 + ? `Organized ${countOf(movedCount)} with ${countOf(failedCount, 'failure')}. ${reason}${tail}` + : `Organized ${countOf(movedCount)}, but the run did not finish. ${reason}${tail}`, + severity: 'warning', + duration: 8000 + }; + } + + if (movedCount > 0) { + return { + summary: { + outcome: 'moved', + count: movedCount, + headline: `Moved ${countOf(movedCount)} to ${movedCount === 1 ? 'its destination' : 'their destinations'}`, + detail: duplicateNote + }, + toast: `Organized ${countOf(movedCount)}${duplicateNote ? `.${tail}` : ''}`, + severity: 'success', + duration: duplicateNote ? 10000 : 7000 + }; + } + if (duplicateRows.length > 0) { + return { + summary: { + outcome: 'moved', + count: 0, + headline: 'No files needed moving', + detail: duplicateNote + }, + toast: `No files needed moving. ${duplicateNote}`, + severity: 'info', + duration: 10000 + }; + } + return null; +} + const ROLLBACK_NAMES_SHOWN = 3; +const ROLLED_BACK_HEADLINE = 'Organization was rolled back'; /** - * The one message for a batch the main process rolled back after a critical - * error. Restored rows come back `success: false, rolledBack: true`; a row - * whose restore failed keeps `success: true, rollbackFailed: true` because - * that file is still at its destination, and those files are named so the - * user can find them. Main deletes the recovery manifest when every restore - * worked, so its path is only shown when something was left behind. + * What a batch the main process rolled back after a critical error left + * behind, to follow ROLLED_BACK_HEADLINE. Restored rows come back + * `success: false, rolledBack: true`; a row whose restore failed keeps + * `success: true, rollbackFailed: true` because that file is still at its + * destination, and those files are named so the user can find them. Main + * deletes the recovery manifest when every restore worked, so its path is + * only shown when something was left behind. * @param {any} result - the batch handler's rollback outcome * @param {any[]} rows - its per-op results * @returns {string} @@ -65,9 +194,8 @@ function describeRolledBackBatch(result, rows) { leftBehind.length, Number.isInteger(result?.rollbackFailCount) ? result.rollbackFailCount : 0 ); - const head = `Organization was rolled back. ${reason}`; if (leftBehindCount === 0) { - return `${head} Files already moved were returned to their original locations.`; + return `${reason} Files already moved were returned to their original locations.`; } const names = leftBehind .map((r) => @@ -84,10 +212,10 @@ function describeRolledBackBatch(result, rows) { typeof result?.recoveryPath === 'string' && result.recoveryPath ? ` Recovery details: ${result.recoveryPath}` : ''; - return `${head} ${leftBehindCount} ${fileWord} could not be returned and remain in the destination folder${named}.${recovery}`; + return `${reason} ${leftBehindCount} ${fileWord} could not be returned and remain in the destination folder${named}.${recovery}`; } -import { toCanonicalPath } from '../../utils/canonicalPath'; +import { toCanonicalPath, toFolderKey, toParentFolderKey } from '../../utils/canonicalPath'; import { resolveOrganizeDestination, suggestionForOperation } from './organizeDestination'; const logger = createLogger('OrganizePhase-Organization'); @@ -122,7 +250,11 @@ function collectPathsToUnmarkAfterUndo( } return Array.from(paths); } -/** Auto-clear inline organize success banner (FI-02) */ +/** + * Auto-clear the inline organize banner (FI-02) after a clean run. A run that + * left something unmoved keeps its banner until it is dismissed or the next + * run starts. + */ const LAST_ORGANIZE_SUMMARY_CLEAR_MS = 12000; /** * Hook for progress tracking @@ -358,6 +490,17 @@ const joinPath = (root, ...parts) => { return [cleanRoot, ...cleanParts].join(separator); }; +const UNCATEGORIZED = 'Uncategorized'; + +/** + * Where a smart folder's files go: its own path, or a folder of its name + * under the default location when it has none. + * @param {any} folder + * @param {string} defaultLocation + */ +const folderDestinationDir = (folder, defaultLocation) => + folder?.path || joinPath(defaultLocation, folder?.name || UNCATEGORIZED); + const sanitizeDestinationFileName = (/** @type {any} */ value, fallback = 'untitled') => { const toSafeBaseName = (/** @type {any} */ input) => { if (typeof input !== 'string') return ''; @@ -408,13 +551,11 @@ function processFileForOrganization({ // Only a fall-through to Uncategorized is a "category changed" notice: a // folder the engine or name matching picked is already shown on the card. const categoryChanged = !smartFolder && Boolean(originalCategory); - const currentCategory = smartFolder ? smartFolder.name : 'Uncategorized'; + const currentCategory = smartFolder ? smartFolder.name : UNCATEGORIZED; // Always use a validated smart folder path; never build a path from a raw category. - const resolvedFolder = smartFolder || findSmartFolderForCategory('Uncategorized'); - const destinationDir = resolvedFolder - ? resolvedFolder.path || joinPath(defaultLocation, resolvedFolder.name || 'Uncategorized') - : joinPath(defaultLocation, 'Uncategorized'); + const resolvedFolder = smartFolder || findSmartFolderForCategory(UNCATEGORIZED); + const destinationDir = folderDestinationDir(resolvedFolder, defaultLocation); const suggestedName = edits.suggestedName || fileWithEdits.analysis?.suggestedName || file.name; const safeSuggestedName = sanitizeDestinationFileName(suggestedName, file.name); @@ -431,6 +572,7 @@ function processFileForOrganization({ return { newName, normalized, + destinationDir, categoryChanged, originalCategory, finalCategory: currentCategory, @@ -438,6 +580,57 @@ function processFileForOrganization({ }; } +/** + * A file the plan leaves in the folder it is already in, because that folder + * is also the smart folder chosen for it. The run renames it there, or leaves + * it as it is when its name does not change either, and reports it as moved. + * @typedef {object} InPlaceMove + * @property {string} sourcePath + * @property {string} fileName + * @property {string} folderName - the smart folder it stays in + * @property {boolean} renamed - whether the run gives it a new name + */ + +/** + * The files whose destination folder is the folder that holds them now. + * @param {Omit & { files: any[] }} params + * @returns {InPlaceMove[]} + */ +export function findInPlaceMoves({ files, smartFolders = [], defaultLocation, ...planParams }) { + // This runs while the screen renders; without a default location there is + // no plan to describe yet. + if (typeof defaultLocation !== 'string' || !defaultLocation) return []; + // Only a file that sits directly in a folder the plan can send files to can + // stay where it is. In most queues that is none of them, so the per-file + // destination is only worked out for those. + const targetKeys = new Set( + [ + ...smartFolders.map((folder) => folderDestinationDir(folder, defaultLocation)), + joinPath(defaultLocation, UNCATEGORIZED) + ].map(toFolderKey) + ); + /** @type {InPlaceMove[]} */ + const inPlace = []; + for (const file of Array.isArray(files) ? files : []) { + const parentKey = toParentFolderKey(file?.path); + if (parentKey === null || !targetKeys.has(parentKey)) continue; + const { newName, destinationDir, finalCategory } = processFileForOrganization({ + ...planParams, + file, + smartFolders, + defaultLocation + }); + if (toFolderKey(destinationDir) !== parentKey) continue; + inPlace.push({ + sourcePath: file.path, + fileName: file.name, + folderName: finalCategory, + renamed: newName !== file.name + }); + } + return inPlace; +} + /** * Build file operations for organization * @param {Object} params - Parameters @@ -587,13 +780,16 @@ export function useOrganization({ chunkedResultsRef, resetChunkedResults } = useProgressTracking(); - /** Inline success banner on Organize (FI-02); cleared on new run or after timeout */ - const [lastOrganizeSummary, setLastOrganizeSummary] = useState(null); + /** Inline outcome banner on Organize (FI-02); cleared when a new run starts */ + const [lastOrganizeSummary, setLastOrganizeSummary] = useState( + /** @type {OrganizeSummary | null} */ (null) + ); useEffect(() => { - if (!lastOrganizeSummary) return undefined; + if (lastOrganizeSummary?.outcome !== 'moved') return undefined; const id = setTimeout(() => setLastOrganizeSummary(null), LAST_ORGANIZE_SUMMARY_CLEAR_MS); return () => clearTimeout(id); }, [lastOrganizeSummary]); + const dismissOrganizeSummary = useCallback(() => setLastOrganizeSummary(null), []); const currentPhaseRef = useRef(currentPhase); useEffect(() => { currentPhaseRef.current = currentPhase; @@ -693,6 +889,7 @@ export function useOrganization({ setIsOrganizing(true); setOrganizingState(true); setOrganizeConflicts([]); + setLastOrganizeSummary(null); resetChunkedResults(); const filesToProcess = resumeInfo @@ -844,6 +1041,8 @@ export function useOrganization({ logger.warn('[ORGANIZE] Preview generation failed (non-fatal):', previewError.message); } + // How onExecute reported this run, once it has. + const reported = { outcome: /** @type {OrganizeSummary['outcome'] | null} */ (null) }; const stateCallbacks = { onExecute: (/** @type {any} */ result) => { try { @@ -903,9 +1102,27 @@ export function useOrganization({ (r.destination || operationMap.get(r.source) || '').split(/[\\/]/).pop() || '', smartFolder: actualSmartFolder, - organizedAt: new Date().toISOString() + organizedAt: new Date().toISOString(), + // Not moved: `path` is the identical copy that was kept. + ...(isDuplicateSkip(r) + ? { duplicateRemoval: r.sourceRemoval === 'trashed' ? 'trashed' : 'deleted' } + : {}) }; }); + const duplicateRows = resArray.filter( + (/** @type {any} */ r) => !r.rolledBack && isDuplicateSkip(r) + ); + const report = rolledBackBatch + ? null + : describeFinishedRun({ + rows: resArray, + movedCount: uiResults.length - duplicateRows.length, + duplicateRows, + batchError: result?.partialFailure + ? String(result.error || 'The batch stopped early') + : undefined, + interrupted: result?.interrupted === true + }); const successfulMoves = resArray .filter((/** @type {any} */ r) => r.success && !r.rolledBack && r.source) .map((/** @type {any} */ r) => ({ @@ -919,6 +1136,43 @@ export function useOrganization({ dispatch(updateResultPathsAfterMove({ oldPaths, newPaths })); dispatch(updateFilePathsAfterMove({ oldPaths, newPaths })); } + + // One toast and one banner per run. The reason a file was + // refused is logged in main and shown nowhere else. + if (rolledBackBatch) { + logger.warn('[ORGANIZE] Batch rolled back by main', { + batchId: result?.batchId, + rollbackFailCount: result?.rollbackFailCount + }); + const detail = describeRolledBackBatch(result, resArray); + addNotification(`${ROLLED_BACK_HEADLINE}. ${detail}`, 'error', 12000); + setLastOrganizeSummary({ + outcome: 'rolledBack', + count: uiResults.length, + headline: ROLLED_BACK_HEADLINE, + detail + }); + reported.outcome = 'rolledBack'; + } else if (report) { + if (report.summary.outcome !== 'moved') { + logger.warn('[ORGANIZE] Batch left files unmoved', { + outcome: report.summary.outcome, + movedCount: report.summary.count, + duplicateCount: duplicateRows.length, + error: result?.error + }); + } + // A run the user stopped gets its count from the cancel or + // pause message; a second toast would only repeat it. + const stoppedByUser = + cancelledRef.current || Boolean(result?.cancelled || result?.paused); + if (report.summary.outcome !== 'moved' || !stoppedByUser) { + addNotification(report.toast, report.severity, report.duration); + } + setLastOrganizeSummary(report.summary); + reported.outcome = report.summary.outcome; + } + if (uiResults.length > 0) { addOrganizedFiles(uiResults); @@ -927,25 +1181,6 @@ export function useOrganization({ markFilesAsProcessed(collectPathsForProcessedTracking(uiResults)); - // When some files fail during batch operation, warn user about partial completion - const failedCount = resArray.filter((/** @type {any} */ r) => !r.success).length; - if (rolledBackBatch) { - // Reported once, after this block. - } else if (result?.partialFailure || failedCount > 0) { - addNotification( - `Organized ${countOf(uiResults.length)} with ${countOf(failedCount, 'failure')}. ${describeFirstFailure(resArray)}`, - 'warning', - 8000 - ); - logger.warn('[ORGANIZE] Partial failure in batch operation', { - successCount: uiResults.length, - failCount: failedCount, - error: result?.error - }); - } else { - addNotification(`Organized ${countOf(uiResults.length)}`, 'success', 7000); - } - // Files moved on disk but the search index / history / undo // stack could not be fully synced. Without surfacing this, // search results and undo silently point at stale paths. @@ -965,14 +1200,6 @@ export function useOrganization({ }); } - if (!rolledBackBatch) { - setLastOrganizeSummary({ - count: uiResults.length, - partial: Boolean(result?.partialFailure || failedCount > 0), - failedCount: failedCount || 0 - }); - } - // 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. @@ -989,22 +1216,6 @@ export function useOrganization({ } } else if (!rolledBackBatch) { logger.warn('[ORGANIZE] onExecute: No successful results to process'); - if (resArray.length > 0) { - // Every file was refused. The reason was logged in main and - // never shown; the first refusal names the rule. - addNotification( - `No files were organized. ${describeFirstFailure(resArray)}`, - 'error', - 9000 - ); - } - } - if (rolledBackBatch) { - logger.warn('[ORGANIZE] Batch rolled back by main', { - batchId: result?.batchId, - rollbackFailCount: result?.rollbackFailCount - }); - addNotification(describeRolledBackBatch(result, resArray), 'error', 12000); } } catch (callbackError) { logger.error('[ORGANIZE] onExecute callback failed:', callbackError.message); @@ -1293,11 +1504,18 @@ export function useOrganization({ ); return; } + // Some files were left where they were. Stay on Organize, where the + // banner says why and those files are still listed; Complete has + // no place for either. + if (reported.outcome === 'partial' || reported.outcome === 'none') return; actions.advancePhase(PHASES?.COMPLETE ?? 'complete'); - } else { + } else if (reported.outcome === null) { + // onExecute had nothing to report, so this is the only message. logger.warn('[ORGANIZE] No successful operations - phase will not advance'); addNotification( - 'No files were organized successfully. Check the logs for details.', + cancelledRef.current || result?.cancelled + ? 'Organization cancelled before any file was moved.' + : 'No files were organized successfully. Check the logs for details.', 'warning' ); } @@ -1414,6 +1632,32 @@ export function useOrganization({ [handleOrganizeFiles] ); + // Ready files the plan leaves in the folder they are in, so the screen can + // say so before the run reports them as moved. + const inPlaceMoves = useMemo( + () => + findInPlaceMoves({ + files: (unprocessedFiles || []).filter((/** @type {any} */ f) => f && f.analysis), + editingFiles, + getFileWithEdits, + findSmartFolderForCategory, + findSmartFolderForFile, + defaultLocation, + smartFolders, + suggestionsByPath + }), + [ + unprocessedFiles, + editingFiles, + getFileWithEdits, + findSmartFolderForCategory, + findSmartFolderForFile, + defaultLocation, + smartFolders, + suggestionsByPath + ] + ); + return { isOrganizing, batchProgress, @@ -1422,7 +1666,9 @@ export function useOrganization({ handlePreviewOrganize, setBatchProgress, organizeConflicts, + inPlaceMoves, lastOrganizeSummary, + dismissOrganizeSummary, cancelOrganize, isCancelling, pauseOrganize, diff --git a/src/renderer/utils/canonicalPath.js b/src/renderer/utils/canonicalPath.js index fa954b7..d0787bc 100644 --- a/src/renderer/utils/canonicalPath.js +++ b/src/renderer/utils/canonicalPath.js @@ -20,3 +20,27 @@ export const toCanonicalPath = (/** @type {any} */ value) => { const normalized = value.replace(/[\\/]+/g, '/'); return isWindowsStylePath(value) ? normalized.toLowerCase() : normalized; }; + +/** + * Key for comparing folders: canonical form, Unicode composed (macOS can list + * a name decomposed while a saved folder holds it composed), no trailing + * slash. A root is '' ('/') or its drive ('c:'). Null when there is no path. + * + * POSIX paths keep their case, so on a case-insensitive Mac volume two + * spellings of one folder give different keys. Use this for advice shown to + * the user, never to decide what a file operation does. + */ +export const toFolderKey = (/** @type {any} */ value) => { + const canonical = toCanonicalPath(value); + return canonical ? canonical.normalize('NFC').replace(/\/$/, '') : null; +}; + +/** + * The toFolderKey of the folder that directly holds `value`, or null when the + * path has no folder part. + */ +export const toParentFolderKey = (/** @type {any} */ value) => { + const canonical = toCanonicalPath(value).normalize('NFC'); + const cut = canonical.lastIndexOf('/'); + return cut < 0 ? null : canonical.slice(0, cut); +}; diff --git a/src/renderer/utils/duplicateRemoval.js b/src/renderer/utils/duplicateRemoval.js new file mode 100644 index 0000000..b53cea8 --- /dev/null +++ b/src/renderer/utils/duplicateRemoval.js @@ -0,0 +1,26 @@ +// @ts-check +/** + * Organized-file entries for a file that was not moved because its + * destination folder already held an identical copy. useOrganization sets + * `duplicateRemoval` on those entries; their `path` is the copy that was kept. + */ + +/** + * @param {any} file - an organized-file entry + * @returns {boolean} + */ +export function isRemovedDuplicate(file) { + return Boolean(file && typeof file === 'object' && file.duplicateRemoval); +} + +/** + * What became of a removed duplicate, or '' for a file that was moved. + * @param {any} file - an organized-file entry + * @returns {string} + */ +export function describeDuplicateRemoval(file) { + if (!isRemovedDuplicate(file)) return ''; + return file.duplicateRemoval === 'trashed' + ? 'This copy is in the system Trash.' + : 'This copy was removed.'; +} diff --git a/src/renderer/utils/errorMapping.js b/src/renderer/utils/errorMapping.js index fdbff06..7a06765 100644 --- a/src/renderer/utils/errorMapping.js +++ b/src/renderer/utils/errorMapping.js @@ -94,14 +94,14 @@ const FILE_OPERATION_CODE_MAP = { severity: 'warning', message: 'The operation was cancelled before it finished.' }, - // Create folder (folderHandlers.js): the path is inside an allowed location - // as spelled, but a symlink or junction along it leads outside. The handler - // writes the copy, so its message is shown as is; the text here is the same - // sentence for a result that arrives without one. - LINK_OUTSIDE_ALLOWED: { + // Create folder (folderHandlers.js): the path is acceptable as spelled, but + // a symlink, junction or short name along it leads into a system folder. + // The handler writes the copy, so its message is shown as is; the text here + // is the same sentence for a result that arrives without one. + LINK_INTO_SYSTEM: { severity: 'warning', message: - 'That folder is inside a link that points outside your allowed locations. Choose the real folder instead.', + 'That folder leads into a system folder through a link or short name. Choose a different folder.', useHandlerMessage: true } }; @@ -278,9 +278,12 @@ const ERROR_CODE_MAP = { message: 'Cannot access the parent folder.' }, // ── Security ── + // A folder path main refused: a system folder, a path with `..`, a device + // path, or a location check that timed out. The handler says which. [AI_ERROR_CODES.SECURITY_PATH_VIOLATION]: { severity: 'error', - message: 'Refused: that path is outside the permitted locations.' + message: 'Refused: that location cannot be used.', + useHandlerMessage: true }, // ── Smart folder operations ── [AI_ERROR_CODES.SAVE_FAILED]: { diff --git a/src/shared/securityConfig.js b/src/shared/securityConfig.js index 040fa73..e1094b1 100644 --- a/src/shared/securityConfig.js +++ b/src/shared/securityConfig.js @@ -166,21 +166,6 @@ function _getWindowsDriveLetters() { */ const PROTOTYPE_POLLUTION_KEYS = ['__proto__', 'constructor', 'prototype']; -/** - * Allowed Electron app paths for file operations - * Used to determine what user directories are safe to access - */ -const ALLOWED_APP_PATHS = [ - 'userData', // App data directory - 'documents', // User documents - 'downloads', // Downloads - 'desktop', // Desktop - 'pictures', // Pictures - 'videos', // Videos - 'music', // Music - 'home' // Home directory -]; - /** * Settings validation configuration */ @@ -504,7 +489,6 @@ module.exports = { DANGEROUS_PATHS, getDangerousPaths, PROTOTYPE_POLLUTION_KEYS, - ALLOWED_APP_PATHS, // Shell security EXECUTABLE_EXTENSIONS, diff --git a/test/a11y/uiComponents.a11y.test.js b/test/a11y/uiComponents.a11y.test.js index 73c9691..a8b7b95 100644 --- a/test/a11y/uiComponents.a11y.test.js +++ b/test/a11y/uiComponents.a11y.test.js @@ -85,6 +85,18 @@ describe('UI primitives accessibility (axe)', () => { await expectNoA11yViolations(Heads up); } ); + + test('announces at once by default, and politely as role="status"', async () => { + await expectNoA11yViolations( + <> + Now + Standing advice + + ); + + expect(document.querySelector('[role="alert"]').textContent).toBe('Now'); + expect(document.querySelector('[role="status"]').textContent).toBe('Standing advice'); + }); }); describe('StatusBadge', () => { diff --git a/test/batchOrganize.duplicateSources.realfs.test.js b/test/batchOrganize.duplicateSources.realfs.test.js new file mode 100644 index 0000000..64319f5 --- /dev/null +++ b/test/batchOrganize.duplicateSources.realfs.test.js @@ -0,0 +1,295 @@ +/** + * @jest-environment node + */ +/** + * Byte-identical files in one batch (the same report exported twice) must + * not be removed as duplicates of each other. Runs the real batch handler + * and the real duplicate check against a temp dir; only the OS trash is + * replaced, by a `_TRASH` folder the test can inspect. + * + * The failure this pins: a Smart Folder that is also the input folder, so + * some files are renamed in place. Each of two identical files found the + * other in "the destination folder", both went to the trash, verification + * then failed for the missing kept copy, and the whole batch rolled back. + * + * The same setup leaves some files at the path they already have. The run + * must not touch them, and the dry-run preview must not show them as a name + * clash with themselves. + */ + +jest.unmock('fs'); +jest.unmock('fs/promises'); + +jest.mock('../src/main/services/TrashService', () => ({ + TrashService: class { + /** @param {string} filePath */ + async trashOne(filePath) { + const fsp = require('fs').promises; + const pathMod = require('path'); + const trashDir = pathMod.join(pathMod.dirname(pathMod.dirname(filePath)), '_TRASH'); + await fsp.mkdir(trashDir, { recursive: true }); + await fsp.rename(filePath, pathMod.join(trashDir, pathMod.basename(filePath))); + return { success: true }; + } + } +})); + +jest.mock('../src/main/ipc/files/embeddingSync', () => ({ + removeEmbeddingsForPathBestEffort: jest.fn(), + syncEmbeddingForMove: jest.fn(), + verifyEmbeddingAfterMove: jest.fn().mockResolvedValue({ valid: true, issues: [] }) +})); + +jest.mock('../src/main/ipc/files/batchLockManager', () => ({ + acquireBatchLock: jest.fn().mockResolvedValue(true), + renewBatchLock: jest.fn().mockReturnValue(true), + releaseBatchLock: jest.fn() +})); + +jest.mock('../src/main/ipc/files/batchProgressReporter', () => ({ + sendFileOperationBatchComplete: jest.fn(), + sendOperationProgress: jest.fn(), + sendChunkedResults: jest.fn().mockResolvedValue({ sent: true }) +})); + +jest.mock('../src/main/services/ServiceContainer', () => ({ + container: { has: jest.fn(() => false), resolve: jest.fn(), tryResolve: jest.fn(() => null) }, + ServiceIds: { FILE_PATH_COORDINATOR: 'filePathCoordinator', LEARNING_FEEDBACK: 'lf' } +})); + +jest.mock('../src/main/ipc/files/destinationPolicy', () => ({ + authorizeDestinations: jest.fn().mockResolvedValue(new Map()), + authorizeDestination: jest.fn().mockResolvedValue({ allowed: true }), + decisionForDestination: jest.fn(() => ({ allowed: true })), + buildStrictFileOpsAllowlist: jest.fn(() => ({ enabled: false, allowlist: [] })) +})); + +jest.mock('../src/shared/pathSanitization', () => ({ + validateFileOperationPath: jest.fn(async (p) => ({ + valid: true, + normalizedPath: require('path').resolve(p) + })), + normalizePathForIndex: jest.fn((p) => String(p).replace(/\\/g, '/')) +})); + +jest.mock('../src/main/services/FolderMatchingService', () => ({ + matchCategoryToFolder: jest.fn(() => null) +})); + +jest.mock('../src/main/ipc/semantic', () => ({ + getSearchServiceInstance: jest.fn(() => null) +})); + +const fs = require('fs'); +const fsp = fs.promises; +const path = require('path'); +const realOs = jest.requireActual('os'); + +const { + handleBatchOrganize, + previewBatchOrganize +} = require('../src/main/ipc/files/batchOrganizeHandler'); + +const silentLog = { info() {}, warn() {}, error() {}, debug() {} }; + +/** @type {string[]} */ +const tempRoots = []; + +async function makeRoot() { + const root = await fsp.mkdtemp(path.join(realOs.tmpdir(), 'ss-dup-sources-')); + tempRoots.push(root); + return root; +} + +afterAll(async () => { + for (const root of tempRoots) { + await fsp.rm(root, { recursive: true, force: true }); + } +}); + +/** + * Every file under `dir`, as sorted forward-slash paths relative to it. + * @param {string} dir + * @param {string} [base] + * @returns {Promise} + */ +async function tree(dir, base = dir) { + const out = []; + for (const entry of await fsp.readdir(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) out.push(...(await tree(full, base))); + else out.push(path.relative(base, full).replace(/\\/g, '/')); + } + return out.sort(); +} + +/** + * @param {string} filePath + * @param {string} content + */ +async function writeFile(filePath, content) { + await fsp.mkdir(path.dirname(filePath), { recursive: true }); + await fsp.writeFile(filePath, content); +} + +/** @param {{ source: string, destination: string }[]} moves */ +function organize(moves) { + return handleBatchOrganize({ + operation: { operations: moves.map((move) => ({ type: 'move', ...move })) }, + logger: silentLog, + getServiceIntegration: () => null, + getMainWindow: () => null + }); +} + +describe('identical files in one organize batch (real filesystem)', () => { + test('two identical files renamed in place both survive under their new names', async () => { + const root = await makeRoot(); + const input = path.join(root, 'Input'); + await writeFile(path.join(input, 'a.csv'), 'SAME'); + await writeFile(path.join(input, 'b.csv'), 'SAME'); + + const result = await organize([ + { source: path.join(input, 'a.csv'), destination: path.join(input, 'A new.csv') }, + { source: path.join(input, 'b.csv'), destination: path.join(input, 'B new.csv') } + ]); + + expect(result.success).toBe(true); + expect(result.rolledBack).toBeFalsy(); + expect(result.successCount).toBe(2); + expect(result.failCount).toBe(0); + expect(await tree(root)).toEqual(['Input/A new.csv', 'Input/B new.csv']); + }); + + test('an identical pair renamed in place does not roll back the rest of the batch', async () => { + const root = await makeRoot(); + const input = path.join(root, 'Input'); + const moves = []; + for (let k = 0; k < 12; k++) { + if (k === 4) { + await writeFile(path.join(input, 'a.csv'), 'SAME'); + await writeFile(path.join(input, 'b.csv'), 'SAME'); + moves.push( + { source: path.join(input, 'a.csv'), destination: path.join(input, 'A new.csv') }, + { source: path.join(input, 'b.csv'), destination: path.join(input, 'B new.csv') } + ); + } + await writeFile(path.join(input, `u${k}.csv`), `UNIQUE-${k}`); + moves.push({ + source: path.join(input, `u${k}.csv`), + destination: path.join(root, k % 2 ? 'Reports' : 'Exports', `U${k}.csv`) + }); + } + + const result = await organize(moves); + + expect(result.success).toBe(true); + expect(result.rolledBack).toBeFalsy(); + expect(result.successCount).toBe(14); + expect(result.results.filter((/** @type {any} */ row) => !row.success)).toEqual([]); + const after = await tree(root); + expect(after.filter((file) => file.startsWith('_TRASH/'))).toEqual([]); + expect(after.filter((file) => file.startsWith('Input/'))).toEqual([ + 'Input/A new.csv', + 'Input/B new.csv' + ]); + expect(after).toHaveLength(14); + }); + + test('identical files swapping folders are both moved, neither removed for the other', async () => { + const root = await makeRoot(); + await writeFile(path.join(root, 'Left', 'a.csv'), 'SAME'); + await writeFile(path.join(root, 'Right', 'b.csv'), 'SAME'); + + const result = await organize([ + { + source: path.join(root, 'Left', 'a.csv'), + destination: path.join(root, 'Right', 'A new.csv') + }, + { + source: path.join(root, 'Right', 'b.csv'), + destination: path.join(root, 'Left', 'B new.csv') + } + ]); + + expect(result.success).toBe(true); + expect(result.successCount).toBe(2); + expect(await tree(root)).toEqual(['Left/B new.csv', 'Right/A new.csv']); + }); + + test('a copy that was already filed before the batch still dedupes the incoming file', async () => { + const root = await makeRoot(); + await writeFile(path.join(root, 'Input', 'a.csv'), 'SAME'); + await writeFile(path.join(root, 'Reports', 'filed last month.csv'), 'SAME'); + + const result = await organize([ + { + source: path.join(root, 'Input', 'a.csv'), + destination: path.join(root, 'Reports', 'A new.csv') + } + ]); + + expect(result.success).toBe(true); + expect(result.results).toEqual([ + expect.objectContaining({ + success: true, + skipped: true, + reason: 'duplicate', + destination: path.join(root, 'Reports', 'filed last month.csv') + }) + ]); + expect(await tree(root)).toEqual(['Reports/filed last month.csv', '_TRASH/a.csv']); + }); +}); + +describe('files already in the folder chosen for them (real filesystem)', () => { + test('a file already at its destination stays as it is, and the preview says the same', async () => { + const root = await makeRoot(); + const filed = path.join(root, 'Input', 'report.csv'); + await writeFile(filed, 'REPORT'); + const moves = [{ source: filed, destination: filed }]; + + const plan = await previewBatchOrganize({ operation: { operations: moves }, log: silentLog }); + + expect(plan.conflicts).toBe(0); + expect(plan.wouldMove).toEqual([ + { source: filed, destination: filed, originalDestination: filed, collision: false } + ]); + + const result = await organize(moves); + + expect(result.success).toBe(true); + expect(result.results).toEqual([ + expect.objectContaining({ success: true, source: filed, destination: filed }) + ]); + expect(await tree(root)).toEqual(['Input/report.csv']); + expect(await fsp.readFile(filed, 'utf8')).toBe('REPORT'); + }); + + test('a rename in place onto a name another file has is still a clash', async () => { + const root = await makeRoot(); + const input = path.join(root, 'Input'); + await writeFile(path.join(input, 'draft.csv'), 'DRAFT'); + await writeFile(path.join(input, 'report.csv'), 'REPORT'); + const moves = [ + { source: path.join(input, 'draft.csv'), destination: path.join(input, 'report.csv') } + ]; + + const plan = await previewBatchOrganize({ operation: { operations: moves }, log: silentLog }); + + expect(plan.conflicts).toBe(1); + expect(plan.wouldMove).toEqual([ + expect.objectContaining({ + destination: path.join(input, 'report_1.csv'), + collision: true + }) + ]); + + const result = await organize(moves); + + expect(result.success).toBe(true); + expect(await tree(root)).toEqual(['Input/report.csv', 'Input/report_1.csv']); + expect(await fsp.readFile(path.join(input, 'report.csv'), 'utf8')).toBe('REPORT'); + expect(await fsp.readFile(path.join(input, 'report_1.csv'), 'utf8')).toBe('DRAFT'); + }); +}); diff --git a/test/batchOrganizeHandler.test.js b/test/batchOrganizeHandler.test.js index 2737f2d..b354067 100644 --- a/test/batchOrganizeHandler.test.js +++ b/test/batchOrganizeHandler.test.js @@ -770,6 +770,36 @@ describe('Batch Organize Handler', () => { ); }); + test('a duplicate skip whose kept copy is missing is one failed row that names the trash', async () => { + fileDedup.handleDuplicateMove.mockResolvedValueOnce({ + skipped: true, + destination: '/vanished/kept.pdf', + reason: 'duplicate', + sourceRemoval: 'trashed' + }); + fs.access.mockImplementation(async (p) => { + if (p.includes('vanished') || p.includes('src')) throw { code: 'ENOENT' }; + }); + params.operation.operations = [ + { source: '/src/doc1.pdf', destination: '/dest/doc1.pdf', type: 'move' } + ]; + + const result = await handleBatchOrganize(params); + + // The row used to be pushed as a success before the check ran, so the + // file was counted as organized next to its own failure. + const rows = result.results.filter((row) => row.source === '/src/doc1.pdf'); + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ success: false }); + expect(rows[0].error).toContain('destination does not exist after move: /vanished/kept.pdf'); + expect(rows[0].error).toContain('Trash / Recycle Bin'); + expect(result.successCount).toBe(0); + expect(result.failCount).toBe(1); + expect( + mockLogger.info.mock.calls.filter(([msg]) => msg === '[FILE-OPS] Move skipped (duplicate)') + ).toEqual([]); + }); + test('handles non-Error thrown values without crashing', async () => { validateFileOperationPath.mockImplementationOnce(() => { throw 'path-validation-failed'; diff --git a/test/canonicalPath.folderKeys.test.js b/test/canonicalPath.folderKeys.test.js new file mode 100644 index 0000000..f9d0e67 --- /dev/null +++ b/test/canonicalPath.folderKeys.test.js @@ -0,0 +1,79 @@ +/** + * Folder keys answer one question for the renderer: "is this file already in + * that folder?". The answer is advice shown to the user (a notice, a badge), + * so a missed match is harmless and a false match is not. + */ + +import { toFolderKey, toParentFolderKey } from '../src/renderer/utils/canonicalPath'; + +describe('toFolderKey', () => { + test('one Windows folder has one key however it is spelled', () => { + const key = toFolderKey('C:\\Users\\Hans\\Strato Input'); + expect(key).toBe('c:/users/hans/strato input'); + expect(toFolderKey('c:/users/hans/strato input/')).toBe(key); + expect(toFolderKey('C:\\Users\\Hans\\\\Strato Input\\')).toBe(key); + }); + + test('a POSIX folder keeps its case and loses a trailing slash', () => { + expect(toFolderKey('/Volumes/SSD/Strato Input/')).toBe('/Volumes/SSD/Strato Input'); + expect(toFolderKey('/Volumes/SSD/strato input')).not.toBe( + toFolderKey('/Volumes/SSD/Strato Input') + ); + }); + + test('a name macOS lists decomposed matches the composed spelling', () => { + const composed = '/Users/hans/Caf\u00e9'; + const decomposed = '/Users/hans/Cafe\u0301'; + expect(decomposed).not.toBe(composed); + expect(toFolderKey(decomposed)).toBe(toFolderKey(composed)); + }); + + test('a root is its drive, or empty for /', () => { + expect(toFolderKey('C:\\')).toBe('c:'); + expect(toFolderKey('/')).toBe(''); + }); + + test('no path gives null, which matches nothing', () => { + expect(toFolderKey('')).toBeNull(); + expect(toFolderKey(undefined)).toBeNull(); + expect(toFolderKey(42)).toBeNull(); + }); +}); + +describe('toParentFolderKey', () => { + test('is the key of the folder that holds the file', () => { + expect(toParentFolderKey('C:\\Users\\Hans\\Strato Input\\report.csv')).toBe( + toFolderKey('C:\\Users\\Hans\\Strato Input') + ); + expect(toParentFolderKey('/Volumes/SSD/Strato Input/report.csv')).toBe( + toFolderKey('/Volumes/SSD/Strato Input/') + ); + }); + + test('a file in a subfolder is not in the folder above it', () => { + expect(toParentFolderKey('/in/2025/report.csv')).not.toBe(toFolderKey('/in')); + }); + + test('a folder whose name starts the same is a different folder', () => { + expect(toParentFolderKey('/in/Strato Input 2/report.csv')).not.toBe( + toFolderKey('/in/Strato Input') + ); + }); + + test('a file at a root has the root key', () => { + expect(toParentFolderKey('C:\\report.csv')).toBe(toFolderKey('C:\\')); + expect(toParentFolderKey('/report.csv')).toBe(toFolderKey('/')); + }); + + test('a decomposed folder name matches the composed folder', () => { + expect(toParentFolderKey('/Users/hans/Cafe\u0301/menu.pdf')).toBe( + toFolderKey('/Users/hans/Caf\u00e9') + ); + }); + + test('a bare name or no path has no folder', () => { + expect(toParentFolderKey('report.csv')).toBeNull(); + expect(toParentFolderKey('')).toBeNull(); + expect(toParentFolderKey(null)).toBeNull(); + }); +}); diff --git a/test/components/CompletePhase.test.js b/test/components/CompletePhase.test.js index 91f4804..2d38bf5 100644 --- a/test/components/CompletePhase.test.js +++ b/test/components/CompletePhase.test.js @@ -48,8 +48,8 @@ jest.mock('../../src/renderer/store/middleware/persistenceMiddleware', () => ({ })); jest.mock('../../src/renderer/components/ui', () => ({ - Button: ({ children, className, onClick }) => ( - ), @@ -111,6 +111,7 @@ jest.mock('framer-motion', () => ({ jest.mock('lucide-react', () => ({ CheckCircle2: () => , Check: () => , + Copy: () => , FilePlus: () => , ArrowLeft: () => , FolderOpen: () => @@ -313,4 +314,108 @@ describe('CompletePhase premium finish', () => { ).toBeInTheDocument(); }); }); + + // A file whose destination already held an identical copy is removed, not + // moved. `path` on its entry is the copy that was kept. + describe('duplicates that were removed instead of moved', () => { + const { useAppSelector } = require('../../src/renderer/store/hooks'); + const { selectOrganizedCount } = require('../../src/renderer/store/selectors'); + const defaultSelector = useAppSelector.getMockImplementation(); + + const movedFile = { + originalName: 'report.pdf', + path: 'C:\\Docs\\Finance\\report.pdf', + smartFolder: 'Finance' + }; + const duplicateOf = (name, duplicateRemoval = 'trashed') => ({ + originalName: name, + path: 'C:\\Docs\\Archive\\report.pdf', + smartFolder: 'Archive', + duplicateRemoval + }); + + const renderWithOrganized = (organizedFiles, { redact = false } = {}) => { + selectRedactPaths.mockReturnValue(redact); + selectOrganizedCount.mockReturnValue(organizedFiles.length); + useAppSelector.mockImplementation((selector) => + selector({ + files: { organizedFiles, organizedPathIndex: [] }, + analysis: { results: [] } + }) + ); + render(); + }; + + afterEach(() => { + useAppSelector.mockImplementation(defaultSelector); + selectOrganizedCount.mockReturnValue(2); + selectRedactPaths.mockReturnValue(false); + }); + + test('are not counted as moved files or as destinations', () => { + renderWithOrganized([movedFile, duplicateOf('report (1).pdf')]); + + expect( + screen.getByText( + '1 file moved into 1 destination. 1 identical copy was removed instead of moved.' + ) + ).toBeInTheDocument(); + expect( + screen.getByText('Your reviewed batch is complete. 1 file is now in place.') + ).toBeInTheDocument(); + expect(screen.getByText('1 file organized, 1 duplicate removed')).toBeInTheDocument(); + expect(screen.queryByText(/\d+ destinations$/)).not.toBeInTheDocument(); + }); + + test('the row says the file was not moved and where its copy went', () => { + mockRevealFile.mockClear(); + renderWithOrganized([movedFile, duplicateOf('report (1).pdf')]); + + expect( + screen.getByText( + 'Not moved: an identical copy is already in C:\\Docs\\Archive. This copy is in the system Trash.' + ) + ).toBeInTheDocument(); + const reveal = screen.getByRole('button', { + name: 'Show the kept copy of report (1).pdf in its folder' + }); + fireEvent.click(reveal); + expect(mockRevealFile).toHaveBeenCalledWith('C:\\Docs\\Archive\\report.pdf'); + }); + + test('a copy that did not go to the Trash is reported as removed', () => { + renderWithOrganized([movedFile, duplicateOf('report (1).pdf', 'deleted')]); + + expect(screen.getByText(/This copy was removed\.$/)).toBeInTheDocument(); + expect(screen.queryByText(/system Trash/)).not.toBeInTheDocument(); + }); + + test('a session of nothing but duplicates does not claim a move', () => { + renderWithOrganized([duplicateOf('report (1).pdf'), duplicateOf('report (2).pdf')]); + + expect( + screen.getByText( + 'No files needed moving. 2 identical copies were removed instead of moved.' + ) + ).toBeInTheDocument(); + expect( + screen.getByText( + 'Your reviewed batch is complete. Every file already had an identical copy in its destination folder.' + ) + ).toBeInTheDocument(); + expect(screen.queryByText(/files? moved$/)).not.toBeInTheDocument(); + expect(screen.getByText('2 duplicates removed', { selector: 'span' })).toBeInTheDocument(); + expect(screen.queryByText(/0 files/)).not.toBeInTheDocument(); + }); + + test('the destination stays hidden when privacy redaction is on', () => { + renderWithOrganized([duplicateOf('report (1).pdf')], { redact: true }); + + expect( + screen.getByText( + 'Not moved: an identical copy is already in the destination. This copy is in the system Trash.' + ) + ).toBeInTheDocument(); + }); + }); }); diff --git a/test/components/InPlaceMoveNotice.test.js b/test/components/InPlaceMoveNotice.test.js new file mode 100644 index 0000000..101420c --- /dev/null +++ b/test/components/InPlaceMoveNotice.test.js @@ -0,0 +1,98 @@ +/** + * @jest-environment jsdom + * + * The notice that says, before a run, which files stay in the folder they + * are in because that folder is also the smart folder chosen for them. + */ +import React from 'react'; +import { render, screen } from '@testing-library/react'; +import '@testing-library/jest-dom'; + +import InPlaceMoveNotice from '../../src/renderer/components/organize/InPlaceMoveNotice'; + +const move = (folderName, renamed = false) => ({ + sourcePath: `/in/${folderName}/file.csv`, + fileName: 'file.csv', + folderName, + renamed +}); + +describe('InPlaceMoveNotice', () => { + test('renders nothing when every file changes folder', () => { + const { container, rerender } = render(); + expect(container).toBeEmptyDOMElement(); + + rerender(); + expect(container).toBeEmptyDOMElement(); + }); + + test('is standing advice, not an interruption', () => { + render(); + + expect(screen.getByRole('status')).toBeInTheDocument(); + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + }); + + test('one file that keeps its name', () => { + render(); + + const notice = screen.getByRole('status'); + expect(notice).toHaveTextContent('1 file is already in the folder chosen for it: Strato Input'); + expect(notice).toHaveTextContent('It will stay as it is.'); + expect(notice).toHaveTextContent('choose a different folder on its card'); + }); + + test('one file that gets a new name', () => { + render(); + + expect(screen.getByRole('status')).toHaveTextContent( + 'It will be renamed there, not moved to another folder.' + ); + }); + + test('several files, all renamed', () => { + render(); + + const notice = screen.getByRole('status'); + expect(notice).toHaveTextContent( + '2 files are already in the folder chosen for them: Strato Input' + ); + expect(notice).toHaveTextContent('They will be renamed there, not moved to another folder.'); + }); + + test('several files, none renamed', () => { + render(); + + expect(screen.getByRole('status')).toHaveTextContent('They will stay as they are.'); + }); + + test('a mix of renamed and unchanged files', () => { + render(); + + expect(screen.getByRole('status')).toHaveTextContent( + 'They will stay where they are, and the ones with a new name are renamed there.' + ); + }); + + test('names each folder once, and counts the ones it does not show', () => { + const { rerender } = render( + + ); + expect(screen.getByRole('status')).toHaveTextContent( + '3 files are already in the folder chosen for them: Strato Input and Bing data' + ); + + rerender( + + ); + expect(screen.getByRole('status')).toHaveTextContent(': Strato Input, Bing data and 2 more'); + }); + + test('passes its class to the box', () => { + render(); + + expect(screen.getByRole('status')).toHaveClass('mb-4'); + }); +}); diff --git a/test/components/OrganizePhase.test.js b/test/components/OrganizePhase.test.js index 4ad074f..a74fcd5 100644 --- a/test/components/OrganizePhase.test.js +++ b/test/components/OrganizePhase.test.js @@ -115,6 +115,13 @@ jest.mock('../../src/renderer/components/organize', () => ({ TargetFolderList: () =>
TARGET_FOLDER_LIST
, BulkOperations: () =>
BULK_OPERATIONS
, OrganizeProgress: () =>
ORGANIZE_PROGRESS
, + OrganizeSummaryBanner: ({ summary, onDismiss }) => ( + + ), + InPlaceMoveNotice: ({ moves }) => + moves?.length ?
IN_PLACE_NOTICE:{moves.length}
: null, VirtualizedFileGrid: () =>
FILE_GRID
, VirtualizedProcessedFiles: () =>
PROCESSED_FILES
, UncategorizedActionsBar: () =>
UNCATEGORIZED_ACTIONS_BAR
@@ -322,6 +329,69 @@ describe('OrganizePhase premium preflight copy', () => { expect(await screen.findByText('ORGANIZE_PROGRESS')).toBeInTheDocument(); }); + test('shows how the last run ended, and lets the user dismiss it', () => { + const { useOrganization } = require('../../src/renderer/phases/organize/useOrganization'); + const dismissOrganizeSummary = jest.fn(); + const hookValue = { + isOrganizing: false, + batchProgress: null, + organizePreview: [], + handleOrganizeFiles: jest.fn(), + organizeConflicts: [], + lastOrganizeSummary: null, + dismissOrganizeSummary + }; + useOrganization.mockReturnValue(hookValue); + + const { rerender } = render(); + expect(screen.queryByText(/SUMMARY_BANNER/)).not.toBeInTheDocument(); + + useOrganization.mockReturnValue({ + ...hookValue, + lastOrganizeSummary: { + outcome: 'partial', + count: 1, + headline: 'Moved 1 file; 1 could not be moved', + detail: 'EEXIST: destination exists' + } + }); + rerender(); + + fireEvent.click(screen.getByText('SUMMARY_BANNER:partial')); + expect(dismissOrganizeSummary).toHaveBeenCalledTimes(1); + }); + + test('says which files stay in their folder, but not while a run is under way', () => { + const { useOrganization } = require('../../src/renderer/phases/organize/useOrganization'); + const hookValue = { + isOrganizing: false, + isPaused: false, + batchProgress: null, + organizePreview: [], + handleOrganizeFiles: jest.fn(), + organizeConflicts: [], + inPlaceMoves: [ + { sourcePath: '/in/a.csv', fileName: 'a.csv', folderName: 'Strato Input', renamed: false } + ] + }; + useOrganization.mockReturnValue(hookValue); + + const { rerender } = render(); + expect(screen.getByText('IN_PLACE_NOTICE:1')).toBeInTheDocument(); + + useOrganization.mockReturnValue({ ...hookValue, isOrganizing: true }); + rerender(); + expect(screen.queryByText(/IN_PLACE_NOTICE/)).not.toBeInTheDocument(); + + useOrganization.mockReturnValue({ ...hookValue, isPaused: true }); + rerender(); + expect(screen.queryByText(/IN_PLACE_NOTICE/)).not.toBeInTheDocument(); + + useOrganization.mockReturnValue({ ...hookValue, inPlaceMoves: [] }); + rerender(); + expect(screen.queryByText(/IN_PLACE_NOTICE/)).not.toBeInTheDocument(); + }); + test('keeps edits for files hidden only while their move is in flight (L11)', () => { const { useOrganization } = require('../../src/renderer/phases/organize/useOrganization'); const { useFileEditing } = require('../../src/renderer/phases/organize/useFileEditing'); diff --git a/test/components/OrganizePreviewModal.test.js b/test/components/OrganizePreviewModal.test.js index 5437c2d..27b7b58 100644 --- a/test/components/OrganizePreviewModal.test.js +++ b/test/components/OrganizePreviewModal.test.js @@ -93,6 +93,43 @@ describe('OrganizePreviewModal', () => { expect(screen.getByText('File is no longer at its original location')).toBeInTheDocument(); }); + describe('a file that is already in its destination folder', () => { + const stayPlan = (destination, collision = false) => ({ + wouldMove: [{ source: 'C:\\Docs\\Finance\\budget.xlsx', destination, collision }], + skipped: [], + wouldCreateFolders: [], + conflicts: collision ? 1 : 0 + }); + + test('is marked as staying, with its new name', () => { + renderModal({ plan: stayPlan('c:/docs/finance/Budget 2025.xlsx') }); + + expect(screen.getByText('Already in this folder')).toBeInTheDocument(); + expect(screen.getByText('Budget 2025.xlsx')).toBeInTheDocument(); + expect(screen.queryByText(/Name taken/)).not.toBeInTheDocument(); + }); + + test('is marked as unchanged when its name stays too', () => { + renderModal({ plan: stayPlan('C:\\Docs\\Finance\\budget.xlsx') }); + + expect(screen.getByText('Already here — no change')).toBeInTheDocument(); + expect(screen.queryByText('Already in this folder')).not.toBeInTheDocument(); + }); + + test('keeps the name-taken mark when its new name belongs to another file', () => { + renderModal({ plan: stayPlan('C:\\Docs\\Finance\\report_1.xlsx', true) }); + + expect(screen.getByText('Already in this folder')).toBeInTheDocument(); + expect(screen.getByText(/Name taken/)).toBeInTheDocument(); + }); + + test('a file arriving from another folder is not marked', () => { + renderModal(); + + expect(screen.queryByText(/^Already/)).not.toBeInTheDocument(); + }); + }); + test('the move button commits and reports the count', () => { const { onConfirm } = renderModal(); diff --git a/test/components/OrganizeSummaryBanner.test.js b/test/components/OrganizeSummaryBanner.test.js new file mode 100644 index 0000000..cd9a469 --- /dev/null +++ b/test/components/OrganizeSummaryBanner.test.js @@ -0,0 +1,72 @@ +/** + * @jest-environment jsdom + */ +import React from 'react'; +import { render, screen, fireEvent } from '@testing-library/react'; +import '@testing-library/jest-dom'; + +import OrganizeSummaryBanner from '../../src/renderer/components/organize/OrganizeSummaryBanner'; + +const summaryOf = (outcome, extra = {}) => ({ + outcome, + count: 0, + headline: `headline for ${outcome}`, + detail: '', + ...extra +}); + +describe('OrganizeSummaryBanner', () => { + test('a clean run is a polite status with the next-step hint', () => { + render(); + + const banner = screen.getByRole('status'); + expect(banner).toHaveAttribute('data-outcome', 'moved'); + expect(banner).toHaveTextContent('headline for moved'); + expect(banner).toHaveTextContent(/review organized files in the status panel/i); + }); + + test('a clean run with a detail shows the detail instead of the hint', () => { + render( + + ); + + const banner = screen.getByRole('status'); + expect(banner).toHaveTextContent('1 identical copy was not moved.'); + expect(banner).not.toHaveTextContent(/status panel/i); + }); + + test('a run that left some files behind is a status, not an interruption', () => { + render( + + ); + + const banner = screen.getByRole('status'); + expect(banner).toHaveAttribute('aria-live', 'polite'); + expect(banner).toHaveTextContent('EEXIST: destination exists'); + }); + + test.each(['none', 'rolledBack'])('a run that moved nothing (%s) is an alert', (outcome) => { + render(); + + const banner = screen.getByRole('alert'); + expect(banner).toHaveAttribute('aria-live', 'assertive'); + expect(banner).toHaveAttribute('data-outcome', outcome); + // No next-step hint for a run that organized nothing. + expect(banner).not.toHaveTextContent(/status panel/i); + }); + + test('can be dismissed', () => { + const onDismiss = jest.fn(); + render(); + + fireEvent.click(screen.getByRole('button', { name: 'Dismiss' })); + + expect(onDismiss).toHaveBeenCalledTimes(1); + }); +}); diff --git a/test/components/VirtualizedProcessedFiles.test.js b/test/components/VirtualizedProcessedFiles.test.js index dde1bca..f10cdcf 100644 --- a/test/components/VirtualizedProcessedFiles.test.js +++ b/test/components/VirtualizedProcessedFiles.test.js @@ -93,4 +93,66 @@ describe('VirtualizedProcessedFiles', () => { expect(container.querySelector('.truncate')).toBeInTheDocument(); expect(container.querySelector('.overflow-hidden')).toBeInTheDocument(); }); + + // A file whose destination already held an identical copy is removed, not + // moved, and must not read as "Organized". + describe('a duplicate that was removed instead of moved', () => { + const duplicate = { + originalPath: '/in/report (1).pdf', + originalName: 'report (1).pdf', + newName: 'report.pdf', + smartFolder: 'Finance', + organizedAt: '2026-03-24T00:00:00.000Z', + duplicateRemoval: 'trashed' + }; + const movedFile = (n) => ({ + originalPath: `/in/file-${n}.pdf`, + originalName: `file-${n}.pdf`, + newName: `file-${n}.pdf`, + smartFolder: 'Finance', + organizedAt: '2026-03-24T00:00:00.000Z' + }); + const duplicateLine = + /^Not moved: an identical copy is already in Finance\. This copy is in the system Trash\. \| /; + + test('says so in a short list', () => { + selectRedactPaths.mockReturnValue(false); + + render(); + + expect(screen.getByText('Duplicate')).toBeInTheDocument(); + expect(screen.getByText('report (1).pdf')).toBeInTheDocument(); + expect(screen.getByText(duplicateLine)).toBeInTheDocument(); + expect(screen.getByText('Not moved')).toBeInTheDocument(); + // The file that did move still reads as before. + expect(screen.getByText('file-1.pdf -> file-1.pdf')).toBeInTheDocument(); + expect(screen.getAllByText('Organized')).toHaveLength(1); + }); + + test('says so in a long, virtualized list', () => { + selectRedactPaths.mockReturnValue(false); + const many = Array.from({ length: 31 }, (_, n) => movedFile(n)); + + render(); + + expect(screen.getByText(/virtualized for performance/)).toBeInTheDocument(); + expect(screen.getByText(duplicateLine)).toBeInTheDocument(); + expect(screen.getAllByText('Not moved')).toHaveLength(1); + expect(screen.getAllByText('Organized')).toHaveLength(31); + }); + + test('reports a copy that did not go to the Trash as removed, with the folder hidden under redaction', () => { + selectRedactPaths.mockReturnValue(true); + + render(); + + expect( + screen.getByText( + /^Not moved: an identical copy is already in the destination\. This copy was removed\. \| / + ) + ).toBeInTheDocument(); + expect(screen.queryByText(/Finance/)).not.toBeInTheDocument(); + expect(screen.queryByText(/report/)).not.toBeInTheDocument(); + }); + }); }); diff --git a/test/duplicateRemoval.test.js b/test/duplicateRemoval.test.js new file mode 100644 index 0000000..65b0cdb --- /dev/null +++ b/test/duplicateRemoval.test.js @@ -0,0 +1,28 @@ +import { + describeDuplicateRemoval, + isRemovedDuplicate +} from '../src/renderer/utils/duplicateRemoval'; + +describe('duplicateRemoval', () => { + test('an entry is a removed duplicate only when it says how the copy was removed', () => { + expect(isRemovedDuplicate({ duplicateRemoval: 'trashed' })).toBe(true); + expect(isRemovedDuplicate({ duplicateRemoval: 'deleted' })).toBe(true); + expect(isRemovedDuplicate({ path: '/dest/a.pdf' })).toBe(false); + expect(isRemovedDuplicate(null)).toBe(false); + expect(isRemovedDuplicate('trashed')).toBe(false); + }); + + test('says where the copy went', () => { + expect(describeDuplicateRemoval({ duplicateRemoval: 'trashed' })).toBe( + 'This copy is in the system Trash.' + ); + expect(describeDuplicateRemoval({ duplicateRemoval: 'deleted' })).toBe( + 'This copy was removed.' + ); + }); + + test('says nothing about a file that was moved', () => { + expect(describeDuplicateRemoval({ path: '/dest/a.pdf' })).toBe(''); + expect(describeDuplicateRemoval(undefined)).toBe(''); + }); +}); diff --git a/test/errorMapping.fileOperations.test.js b/test/errorMapping.fileOperations.test.js index 0d714d4..9f0af43 100644 --- a/test/errorMapping.fileOperations.test.js +++ b/test/errorMapping.fileOperations.test.js @@ -44,14 +44,14 @@ describe('errorMapping: file operation codes', () => { expect(mapped.message).toMatch(/Network folders/); }); - describe('LINK_OUTSIDE_ALLOWED (create folder through a link that leads outside)', () => { + describe('LINK_INTO_SYSTEM (create folder through a link that leads into a system folder)', () => { const HANDLER_MESSAGE = - 'That folder is inside a link that points outside your allowed locations. Choose the real folder instead.'; + 'That folder leads into a system folder through a link or short name. Choose a different folder.'; test('shows the handler message as is, as a warning', () => { const mapped = mapErrorToNotification({ error: HANDLER_MESSAGE, - errorCode: 'LINK_OUTSIDE_ALLOWED' + errorCode: 'LINK_INTO_SYSTEM' }); expect(mapped).toMatchObject({ severity: 'warning', message: HANDLER_MESSAGE }); }); @@ -59,14 +59,14 @@ describe('errorMapping: file operation codes', () => { test('a newer handler wording wins over the copy kept here', () => { const mapped = mapErrorToNotification({ error: 'That folder is reached through a link. Choose the real folder.', - errorCode: 'LINK_OUTSIDE_ALLOWED' + errorCode: 'LINK_INTO_SYSTEM' }); expect(mapped.message).toBe('That folder is reached through a link. Choose the real folder.'); expect(mapped.severity).toBe('warning'); }); test('without a handler message it still says what to do', () => { - expect(mapErrorToNotification({ errorCode: 'LINK_OUTSIDE_ALLOWED' })).toMatchObject({ + expect(mapErrorToNotification({ errorCode: 'LINK_INTO_SYSTEM' })).toMatchObject({ severity: 'warning', message: HANDLER_MESSAGE }); @@ -74,7 +74,7 @@ describe('errorMapping: file operation codes', () => { test('read from the result envelope too', () => { const mapped = mapErrorToNotification({ - error: { success: false, error: HANDLER_MESSAGE, errorCode: 'LINK_OUTSIDE_ALLOWED' } + error: { success: false, error: HANDLER_MESSAGE, errorCode: 'LINK_INTO_SYSTEM' } }); // The envelope carries its text in `error`, not `message`; the mapped // copy is the same sentence. @@ -82,6 +82,28 @@ describe('errorMapping: file operation codes', () => { }); }); + describe('SECURITY_PATH_VIOLATION (a folder path main refused)', () => { + test('shows which rule the path broke, as an error', () => { + const mapped = mapErrorToNotification({ + error: 'Invalid path: access to system directories is not allowed', + errorCode: 'SECURITY_PATH_VIOLATION' + }); + expect(mapped).toEqual( + expect.objectContaining({ + severity: 'error', + message: 'Invalid path: access to system directories is not allowed' + }) + ); + }); + + test('without a handler message it still says the location was refused', () => { + expect(mapErrorToNotification({ errorCode: 'SECURITY_PATH_VIOLATION' })).toMatchObject({ + severity: 'error', + message: 'Refused: that location cannot be used.' + }); + }); + }); + test('an unknown code falls back to the handler message', () => { const mapped = mapErrorToNotification({ error: 'Invalid destination path: something odd', diff --git a/test/fileDedup.pendingSources.test.js b/test/fileDedup.pendingSources.test.js new file mode 100644 index 0000000..ab5cee3 --- /dev/null +++ b/test/fileDedup.pendingSources.test.js @@ -0,0 +1,155 @@ +/** + * @jest-environment node + */ +/** + * handleDuplicateMove removes the source when identical content is already + * in the destination folder. Two cases where that "already there" copy is + * not one the user can count on: + * + * - the destination folder is the folder the source is already in (a rename + * in place): an identical file beside it is a second file the user kept, + * not a filed copy; + * - the identical file is itself waiting to be moved by the same batch. + * + * Both cost real files: two identical files renamed in place each took the + * other for the kept copy, and both went to the trash. + */ + +jest.unmock('fs'); +jest.unmock('fs/promises'); +jest.unmock('os'); + +const path = require('path'); +const fs = require('fs').promises; +const os = require('os'); + +const { handleDuplicateMove } = require('../src/main/utils/fileDedup'); + +describe('fileDedup: copies that do not count as already filed', () => { + let testDir; + + beforeEach(async () => { + testDir = await fs.mkdtemp(path.join(os.tmpdir(), 'dedup-pending-')); + }); + + afterEach(async () => { + try { + await fs.rm(testDir, { recursive: true, force: true }); + } catch { + // best-effort tmp cleanup + } + }); + + /** + * @param {string[]} parts + * @param {string} content + */ + async function write(parts, content) { + const filePath = path.join(testDir, ...parts); + await fs.mkdir(path.dirname(filePath), { recursive: true }); + await fs.writeFile(filePath, content); + return filePath; + } + + // Removal through the OS trash is covered by fileDedup.trash.test.js; these + // tests are about whether the source is removed at all. + const dedup = (/** @type {Record} */ params) => + handleDuplicateMove({ logger: null, unlinkFn: fs.unlink, ...params }); + + test('a rename in place keeps the source although an identical file sits beside it', async () => { + const source = await write(['Inbox', 'export.csv'], 'same-bytes'); + const sibling = await write(['Inbox', 'export (1).csv'], 'same-bytes'); + + const result = await dedup({ + sourcePath: source, + destinationPath: path.join(testDir, 'Inbox', '2026-10 Search performance.csv') + }); + + expect(result).toBeNull(); + expect(await fs.readFile(source, 'utf8')).toBe('same-bytes'); + expect(await fs.readFile(sibling, 'utf8')).toBe('same-bytes'); + }); + + test('two identical files renamed in place: neither is removed for the other', async () => { + const first = await write(['Inbox', 'a.csv'], 'same-bytes'); + const second = await write(['Inbox', 'b.csv'], 'same-bytes'); + + const results = await Promise.all([ + dedup({ sourcePath: first, destinationPath: path.join(testDir, 'Inbox', 'A new.csv') }), + dedup({ sourcePath: second, destinationPath: path.join(testDir, 'Inbox', 'B new.csv') }) + ]); + + expect(results).toEqual([null, null]); + expect((await fs.readdir(path.join(testDir, 'Inbox'))).sort()).toEqual(['a.csv', 'b.csv']); + }); + + test('an identical file at the exact destination still dedupes within one folder', async () => { + const source = await write(['Inbox', 'incoming.pdf'], 'same-bytes'); + const destination = await write(['Inbox', 'kept.pdf'], 'same-bytes'); + + const result = await dedup({ sourcePath: source, destinationPath: destination }); + + expect(result).toMatchObject({ skipped: true, destination, reason: 'duplicate' }); + await expect(fs.access(source)).rejects.toMatchObject({ code: 'ENOENT' }); + }); + + test('an identical file that the batch has still to move is not a filed copy', async () => { + const source = await write(['Inbox', 'a.csv'], 'same-bytes'); + const pendingTwin = await write(['Reports', 'b.csv'], 'same-bytes'); + const destinationPath = path.join(testDir, 'Reports', 'A new.csv'); + + const result = await dedup({ + sourcePath: source, + destinationPath, + pendingSources: [source, pendingTwin] + }); + + expect(result).toBeNull(); + expect(await fs.readFile(source, 'utf8')).toBe('same-bytes'); + }); + + test('a pending source is recognised through a differently written path', async () => { + const source = await write(['Inbox', 'a.csv'], 'same-bytes'); + await write(['Reports', 'b.csv'], 'same-bytes'); + // What a batch holds before it validates its paths: unresolved segments. + const pendingTwinAsQueued = path.join(testDir, 'Reports', '..', 'Reports', 'b.csv'); + + const result = await dedup({ + sourcePath: source, + destinationPath: path.join(testDir, 'Reports', 'A new.csv'), + pendingSources: [pendingTwinAsQueued] + }); + + expect(result).toBeNull(); + }); + + test('a pending source at the exact destination is not a filed copy either', async () => { + const source = await write(['Inbox', 'a.csv'], 'same-bytes'); + const pendingTwin = await write(['Reports', 'a.csv'], 'same-bytes'); + + const result = await dedup({ + sourcePath: source, + destinationPath: pendingTwin, + pendingSources: [source, pendingTwin] + }); + + expect(result).toBeNull(); + expect(await fs.readFile(source, 'utf8')).toBe('same-bytes'); + }); + + test('an identical file in another folder that is not pending still dedupes', async () => { + const source = await write(['Inbox', 'a.csv'], 'same-bytes'); + const filed = await write(['Reports', 'already filed.csv'], 'same-bytes'); + const otherPending = await write(['Inbox', 'c.csv'], 'other-bytes'); + + const result = await dedup({ + sourcePath: source, + destinationPath: path.join(testDir, 'Reports', 'A new.csv'), + pendingSources: [source, otherPending] + }); + + expect(result).toMatchObject({ skipped: true, destination: filed, reason: 'duplicate' }); + await expect(fs.access(source)).rejects.toMatchObject({ code: 'ENOENT' }); + expect(await fs.readFile(filed, 'utf8')).toBe('same-bytes'); + }); +}); diff --git a/test/folderHandlers.createFolderLocation.test.js b/test/folderHandlers.createFolderLocation.test.js new file mode 100644 index 0000000..0eb5f40 --- /dev/null +++ b/test/folderHandlers.createFolderLocation.test.js @@ -0,0 +1,281 @@ +/** + * Where create-folder may make a folder. Any drive is acceptable, an external + * one included: the home directory is not a boundary. The destination policy + * decides whether the location is one the user agreed to. The handler itself + * refuses only a path that is, or really leads into, a system directory, and + * a link into one gets its own code and message instead of the generic + * PATH_NOT_ALLOWED that reads like a typo. Real filesystem in os.tmpdir(). + */ + +jest.mock('fs', () => jest.requireActual('fs')); +jest.mock('fs/promises', () => jest.requireActual('fs/promises')); + +// The temp directory usually sits inside the real home. Point the home +// directory and every Electron user folder at one folder inside the test's +// temp root, so its sibling `outside` is outside all of them. +const mockHome = { base: '' }; +jest.mock('os', () => { + const actual = jest.requireActual('os'); + return { ...actual, homedir: () => mockHome.base || actual.homedir() }; +}); +jest.mock('electron', () => ({ + app: { getPath: jest.fn(() => mockHome.base) } +})); + +jest.mock('../src/shared/logger', () => { + const logger = { + setContext: jest.fn(), + info: jest.fn(), + debug: jest.fn(), + warn: jest.fn(), + error: jest.fn() + }; + return { logger, createLogger: jest.fn(() => logger) }; +}); + +jest.mock('../src/main/ipc/ipcWrappers', () => ({ + createHandler: jest.fn(({ handler }) => handler), + safeHandle: (ipcMain, channel, handler) => ipcMain.handle(channel, handler), + z: null +})); + +// The destination policy's prompt needs a window. Each test says what the +// policy answers; its own rules are covered in destinationPolicy*.test.js. +jest.mock('../src/main/ipc/files/destinationPolicy', () => ({ + authorizeDestination: jest.fn() +})); + +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { isPathDangerous } = require('../src/shared/pathSanitization'); +const { authorizeDestination } = require('../src/main/ipc/files/destinationPolicy'); + +const CHANNEL = 'files:create-folder-direct'; +const isWindows = process.platform === 'win32'; +const linkType = isWindows ? 'junction' : 'dir'; + +// The temp root as a user would pass it. On Windows expand 8.3 short names +// (RUNNER~1) so it matches what the code resolves. On macOS keep the OS +// spelling: /var/folders/... resolves to /private/var/..., which is on the +// system-directory denylist as typed, and no user ever passes that form. +function spelledTempPath(p) { + return process.platform === 'darwin' ? p : fs.realpathSync.native(p); +} + +/** + * A real system directory the denylist covers, used as a link target. + * @returns {string|null} + */ +function pickSystemDirectory() { + const candidates = isWindows + ? [process.env.SystemRoot || path.win32.join('C:/', 'Windows')] + : ['/etc', '/usr/sbin', '/sbin', '/bin']; + for (const candidate of candidates) { + try { + const real = fs.realpathSync.native(candidate); + if (fs.statSync(real).isDirectory() && isPathDangerous(real)) return real; + } catch { + // try the next one + } + } + return null; +} + +describe('where create-folder may make a folder', () => { + /** @type {string} */ + let tmpRoot; + /** @type {string} */ + let home; + /** @type {string} */ + let outside; + /** @type {string[]} */ + let links; + /** @type {Record} */ + let handlers; + + /** + * @param {string} target + * @param {string} name + * @returns {string|null} The link, or null when links cannot be made here + */ + function linkTo(target, name) { + const link = path.join(home, name); + try { + fs.symlinkSync(target, link, linkType); + } catch { + return null; + } + links.push(link); + return link; + } + + beforeAll(() => { + tmpRoot = spelledTempPath(fs.mkdtempSync(path.join(os.tmpdir(), 'strato-mkdir-where-'))); + home = path.join(tmpRoot, 'home'); + outside = path.join(tmpRoot, 'outside'); + fs.mkdirSync(home); + fs.mkdirSync(outside); + mockHome.base = home; + links = []; + }); + + afterAll(() => { + // Unlink before the recursive remove so it can never follow a link. + for (const link of links) { + try { + fs.unlinkSync(link); + } catch { + // ignore + } + } + fs.rmSync(tmpRoot, { recursive: true, force: true }); + }); + + beforeEach(() => { + authorizeDestination.mockReset(); + authorizeDestination.mockResolvedValue({ allowed: true, reason: 'test' }); + require('../src/shared/logger').logger.warn.mockClear(); + + handlers = {}; + const ipcMain = { handle: jest.fn((channel, fn) => (handlers[channel] = fn)) }; + const { registerFolderHandlers } = require('../src/main/ipc/files/folderHandlers'); + registerFolderHandlers({ + ipcMain, + IPC_CHANNELS: { + FILES: { + CREATE_FOLDER_DIRECT: CHANNEL, + OPEN_FOLDER: 'files:open-folder', + DELETE_FOLDER: 'files:delete-folder' + } + }, + shell: { openPath: jest.fn() } + }); + }); + + test('a folder outside the home directory is created once the policy allows it', async () => { + const target = path.join(outside, 'Sorted', 'Invoices'); + + const result = await handlers[CHANNEL]({}, target); + + expect(result).toEqual( + expect.objectContaining({ success: true, path: target, alreadyExisted: false }) + ); + expect(fs.statSync(target).isDirectory()).toBe(true); + expect(authorizeDestination).toHaveBeenCalledWith(target, expect.any(Object)); + }); + + test('a folder the policy refuses is not created, and its reason is passed on', async () => { + authorizeDestination.mockResolvedValue({ + allowed: false, + reason: 'declined', + error: 'Destination folder was not approved.', + errorCode: 'DESTINATION_NOT_ALLOWED' + }); + const target = path.join(outside, 'Declined'); + + const result = await handlers[CHANNEL]({}, target); + + expect(result).toEqual({ + success: false, + error: 'Destination folder was not approved.', + errorCode: 'DESTINATION_NOT_ALLOWED' + }); + expect(fs.existsSync(target)).toBe(false); + }); + + test('a folder in the home directory is still created', async () => { + const target = path.join(home, 'Projects', 'Receipts'); + + const result = await handlers[CHANNEL]({}, target); + + expect(result).toEqual( + expect.objectContaining({ success: true, path: target, alreadyExisted: false }) + ); + expect(fs.statSync(target).isDirectory()).toBe(true); + }); + + test('a folder reached through a link is put to the policy as it was spelled', async () => { + const link = linkTo(outside, 'elsewhere-link'); + // Junctions need no privilege on Windows, so this must never skip there. + if (isWindows) expect(link).not.toBeNull(); + if (!link) return; // directory symlinks not creatable here + const target = path.join(link, 'Through Link'); + + const result = await handlers[CHANNEL]({}, target); + + // The policy resolves the link itself and names the real folder when it + // asks; the handler must not refuse before the policy sees the path. + expect(authorizeDestination).toHaveBeenCalledWith(target, expect.any(Object)); + expect(result.success).toBe(true); + expect(fs.statSync(path.join(outside, 'Through Link')).isDirectory()).toBe(true); + }); + + describe('a link into a system directory', () => { + /** @type {string|null} */ + let systemLink = null; + /** @type {string|null} */ + let systemDir = null; + + beforeAll(() => { + systemDir = pickSystemDirectory(); + systemLink = systemDir ? linkTo(systemDir, 'system-link') : null; + }); + + beforeEach(() => { + // Were the path check ever to let these through, the policy refuses, so + // the test cannot create anything in a real system directory. + authorizeDestination.mockResolvedValue({ + allowed: false, + reason: 'test-guard', + error: 'guard', + errorCode: 'DESTINATION_NOT_ALLOWED' + }); + }); + + test('a folder inside it is refused with the link code and message', async () => { + if (isWindows) expect(systemLink).not.toBeNull(); + if (!systemLink || !systemDir) return; + const target = path.join(systemLink, 'strato-test-never-created'); + + const result = await handlers[CHANNEL]({}, target); + + expect(result).toEqual({ + success: false, + error: + 'That folder leads into a system folder through a link or short name. Choose a different folder.', + errorCode: 'LINK_INTO_SYSTEM' + }); + expect(authorizeDestination).not.toHaveBeenCalled(); + + // The log names the folder as spelled; never the link's target. + const { logger } = require('../src/shared/logger'); + const logged = JSON.stringify(logger.warn.mock.calls); + expect(logged).toContain('LINK_INTO_SYSTEM'); + expect(logged).not.toContain(JSON.stringify(systemDir).slice(1, -1)); + }); + + test('the link itself is refused the same way', async () => { + if (!systemLink) return; + + const result = await handlers[CHANNEL]({}, systemLink); + + expect(result.success).toBe(false); + expect(result.errorCode).toBe('LINK_INTO_SYSTEM'); + expect(authorizeDestination).not.toHaveBeenCalled(); + }); + }); + + test('a system directory as spelled keeps the generic refusal', async () => { + const systemDir = pickSystemDirectory(); + if (!systemDir) return; + authorizeDestination.mockResolvedValue({ allowed: false, reason: 'test-guard' }); + + const result = await handlers[CHANNEL]({}, path.join(systemDir, 'strato-test-never-created')); + + expect(result.success).toBe(false); + expect(result.errorCode).toBe('PATH_NOT_ALLOWED'); + expect(result.error).toMatch(/system directories/); + expect(authorizeDestination).not.toHaveBeenCalled(); + }); +}); diff --git a/test/folderHandlers.linkOutsideAllowed.test.js b/test/folderHandlers.linkOutsideAllowed.test.js deleted file mode 100644 index eb1a043..0000000 --- a/test/folderHandlers.linkOutsideAllowed.test.js +++ /dev/null @@ -1,180 +0,0 @@ -/** - * Create-folder judges where the folder would really land: a folder spelled - * inside an allowed location but reached through a link (a Windows junction, - * or a directory symlink elsewhere) that leads outside is refused. The - * refusal must say why, with its own code, instead of the generic - * PATH_NOT_ALLOWED that reads like a typo. Real filesystem in os.tmpdir(). - */ - -jest.mock('fs', () => jest.requireActual('fs')); -jest.mock('fs/promises', () => jest.requireActual('fs/promises')); - -// The handler allows the home directory and Documents/Downloads/Desktop. The -// temp directory usually sits inside the real home, so point every allowed -// location at a folder inside the test's temp root instead. -const mockAllowed = { base: '' }; -jest.mock('os', () => { - const actual = jest.requireActual('os'); - return { ...actual, homedir: () => mockAllowed.base || actual.homedir() }; -}); -jest.mock('electron', () => ({ - app: { getPath: jest.fn(() => mockAllowed.base) } -})); - -jest.mock('../src/shared/logger', () => { - const logger = { - setContext: jest.fn(), - info: jest.fn(), - debug: jest.fn(), - warn: jest.fn(), - error: jest.fn() - }; - return { logger, createLogger: jest.fn(() => logger) }; -}); - -jest.mock('../src/main/ipc/ipcWrappers', () => ({ - createHandler: jest.fn(({ handler }) => handler), - safeHandle: (ipcMain, channel, handler) => ipcMain.handle(channel, handler), - z: null -})); - -// The destination policy's prompt needs a window; approve every parent so -// the test is about the path check alone. -jest.mock('../src/main/ipc/files/destinationPolicy', () => ({ - authorizeDestination: jest.fn(async () => ({ allowed: true, reason: 'test' })) -})); - -const fs = require('fs'); -const os = require('os'); -const path = require('path'); - -const CHANNEL = 'files:create-folder-direct'; - -// The temp root as a user would pass it. On Windows expand 8.3 short names -// (RUNNER~1) so it matches what the code resolves. On macOS keep the OS -// spelling: /var/folders/... resolves to /private/var/..., which is on the -// system-directory denylist as typed, and no user ever passes that form. -function spelledTempPath(p) { - return process.platform === 'darwin' ? p : fs.realpathSync.native(p); -} - -describe('create-folder through a link that leads outside the allowed locations', () => { - /** @type {string} */ - let tmpRoot; - /** @type {string} */ - let allowedBase; - /** @type {string} */ - let outside; - /** @type {string|null} */ - let escapeLink = null; - /** @type {Record} */ - let handlers; - - beforeAll(() => { - tmpRoot = spelledTempPath(fs.mkdtempSync(path.join(os.tmpdir(), 'strato-mkdir-link-'))); - allowedBase = path.join(tmpRoot, 'home'); - outside = path.join(tmpRoot, 'outside'); - fs.mkdirSync(allowedBase); - fs.mkdirSync(outside); - mockAllowed.base = allowedBase; - const link = path.join(allowedBase, 'escape'); - try { - fs.symlinkSync(outside, link, process.platform === 'win32' ? 'junction' : 'dir'); - escapeLink = link; - } catch { - escapeLink = null; - } - }); - - afterAll(() => { - try { - if (escapeLink) fs.unlinkSync(escapeLink); - } catch { - // ignore - } - fs.rmSync(tmpRoot, { recursive: true, force: true }); - }); - - beforeEach(() => { - handlers = {}; - const ipcMain = { handle: jest.fn((channel, fn) => (handlers[channel] = fn)) }; - const { registerFolderHandlers } = require('../src/main/ipc/files/folderHandlers'); - registerFolderHandlers({ - ipcMain, - IPC_CHANNELS: { - FILES: { - CREATE_FOLDER_DIRECT: CHANNEL, - OPEN_FOLDER: 'files:open-folder', - DELETE_FOLDER: 'files:delete-folder' - } - }, - shell: { openPath: jest.fn() } - }); - }); - - test('a folder inside the junction is refused with the link code and message', async () => { - // Junctions need no privilege on Windows, so this must never skip there. - if (process.platform === 'win32') expect(escapeLink).not.toBeNull(); - if (!escapeLink) return; // directory symlinks not creatable here - const target = path.join(escapeLink, 'New Folder'); - - const result = await handlers[CHANNEL]({}, target); - - expect(result).toEqual({ - success: false, - error: - 'That folder is inside a link that points outside your allowed locations. Choose the real folder instead.', - errorCode: 'LINK_OUTSIDE_ALLOWED' - }); - expect(fs.existsSync(path.join(outside, 'New Folder'))).toBe(false); - - // The log names the folder as spelled; never the link's target. - const { logger } = require('../src/shared/logger'); - const logged = JSON.stringify(logger.warn.mock.calls); - expect(logged).toContain('LINK_OUTSIDE_ALLOWED'); - expect(logged).not.toContain(JSON.stringify(outside).slice(1, -1)); - }); - - test('the junction itself is refused the same way', async () => { - if (!escapeLink) return; - const result = await handlers[CHANNEL]({}, escapeLink); - expect(result.success).toBe(false); - expect(result.errorCode).toBe('LINK_OUTSIDE_ALLOWED'); - }); - - test('a normal folder in the allowed location is still created', async () => { - const target = path.join(allowedBase, 'Projects', 'Receipts'); - - const result = await handlers[CHANNEL]({}, target); - - expect(result).toEqual( - expect.objectContaining({ success: true, path: target, alreadyExisted: false }) - ); - expect(fs.statSync(target).isDirectory()).toBe(true); - }); - - test('a junction to a folder inside the allowed location still works', async () => { - const realDir = path.join(allowedBase, 'real-docs'); - fs.mkdirSync(realDir, { recursive: true }); - const insideLink = path.join(allowedBase, 'docs-link'); - try { - fs.symlinkSync(realDir, insideLink, process.platform === 'win32' ? 'junction' : 'dir'); - } catch { - return; - } - try { - const result = await handlers[CHANNEL]({}, path.join(insideLink, 'Sub')); - expect(result.success).toBe(true); - expect(fs.statSync(path.join(realDir, 'Sub')).isDirectory()).toBe(true); - } finally { - fs.unlinkSync(insideLink); - } - }); - - test('a path outside the allowed locations as spelled keeps the generic refusal', async () => { - const result = await handlers[CHANNEL]({}, path.join(outside, 'Elsewhere')); - expect(result.success).toBe(false); - expect(result.errorCode).toBe('PATH_NOT_ALLOWED'); - expect(fs.existsSync(path.join(outside, 'Elsewhere'))).toBe(false); - }); -}); diff --git a/test/securityConfig.test.js b/test/securityConfig.test.js index c115ca3..4885090 100644 --- a/test/securityConfig.test.js +++ b/test/securityConfig.test.js @@ -112,24 +112,6 @@ describe('securityConfig', () => { }); }); - describe('ALLOWED_APP_PATHS', () => { - test('includes userData', () => { - expect(securityConfig.ALLOWED_APP_PATHS).toContain('userData'); - }); - - test('includes documents', () => { - expect(securityConfig.ALLOWED_APP_PATHS).toContain('documents'); - }); - - test('includes downloads', () => { - expect(securityConfig.ALLOWED_APP_PATHS).toContain('downloads'); - }); - - test('includes home', () => { - expect(securityConfig.ALLOWED_APP_PATHS).toContain('home'); - }); - }); - describe('SETTINGS_VALIDATION', () => { test('has allowedKeys as a Set', () => { expect(securityConfig.SETTINGS_VALIDATION.allowedKeys).toBeInstanceOf(Set); diff --git a/test/smartFolders.sanitizeFolderPath.test.js b/test/smartFolders.sanitizeFolderPath.test.js index a7aa68e..838a52d 100644 --- a/test/smartFolders.sanitizeFolderPath.test.js +++ b/test/smartFolders.sanitizeFolderPath.test.js @@ -1,5 +1,10 @@ /** * @jest-environment node + * + * sanitizeFolderPath checks a folder path from the renderer before it is + * scanned or saved as a smart folder. It keeps out traversal, URLs, device + * paths and system directories; it does not confine folders to the home + * directory, so a folder on an external or secondary drive passes. */ jest.mock('electron', () => ({ app: { @@ -27,6 +32,7 @@ jest.mock('../src/main/services/SmartFoldersLLMService', () => ({ const { sanitizeFolderPath } = require('../src/main/ipc/smartFolders'); const winOnly = process.platform === 'win32' ? test : test.skip; +const posixOnly = process.platform === 'win32' ? test.skip : test; describe('sanitizeFolderPath', () => { test.each([ @@ -37,20 +43,97 @@ describe('sanitizeFolderPath', () => { '\\\\.\\UNC\\server\\share\\x', '\\\\?\\UNC\\server\\share\\x', '\\\\?\\GLOBALROOT\\Device\\HarddiskVolume1' - ])('rejects Windows device-namespace path %s', (input) => { - expect(() => sanitizeFolderPath(input)).toThrow( + ])('rejects Windows device-namespace path %s', async (input) => { + await expect(sanitizeFolderPath(input)).rejects.toThrow( 'Invalid path: Windows device namespace paths are not allowed' ); }); - winOnly('still accepts a real UNC share outside the allowed app paths', () => { - expect(sanitizeFolderPath('\\\\server\\share\\folder')).toBe('\\\\server\\share\\folder'); + winOnly('accepts a real UNC share', async () => { + await expect(sanitizeFolderPath('\\\\server\\share\\folder')).resolves.toBe( + '\\\\server\\share\\folder' + ); }); - winOnly('still enforces allowed-base containment for local drive paths', () => { - expect(sanitizeFolderPath('C:\\Users\\Test\\Documents\\Work')).toBe( - 'C:\\Users\\Test\\Documents\\Work' + describe('any drive is acceptable', () => { + winOnly('a folder under the home directory', async () => { + await expect(sanitizeFolderPath('C:\\Users\\Test\\Documents\\Work')).resolves.toBe( + 'C:\\Users\\Test\\Documents\\Work' + ); + }); + + winOnly('a folder elsewhere on the system drive', async () => { + await expect(sanitizeFolderPath('C:\\Temp')).resolves.toBe('C:\\Temp'); + }); + + // The drive letter need not exist: a drive that is unplugged is judged as + // spelled, and the failure to reach it surfaces where the folder is used. + winOnly.each(['F:\\Sorted', 'F:\\', 'Q:\\Strato Input\\2025'])( + 'a folder on another drive: %s', + async (input) => { + await expect(sanitizeFolderPath(input)).resolves.toBe(input); + } + ); + + posixOnly.each(['/Volumes/Samsung T7/Sorted', '/media/hans/SSD/Sorted', '/mnt/data/Sorted'])( + 'a folder on a mounted volume: %s', + async (input) => { + await expect(sanitizeFolderPath(input)).resolves.toBe(input); + } + ); + }); + + describe('still refused', () => { + test.each(['', ' ', null, undefined, 42])('no path: %p', async (input) => { + await expect(sanitizeFolderPath(input)).rejects.toThrow(/Invalid path/); + }); + + test.each(['Sorted', './Sorted', 'Documents/Sorted'])('a relative path: %s', async (input) => { + await expect(sanitizeFolderPath(input)).rejects.toThrow(/absolute path/); + }); + + winOnly.each(['F:\\Sorted\\..\\..\\Windows', 'C:\\Users\\Test\\..\\..\\Windows\\System32'])( + 'a path with traversal: %s', + async (input) => { + await expect(sanitizeFolderPath(input)).rejects.toThrow(/Invalid path/); + } + ); + + posixOnly('a path with traversal', async () => { + await expect(sanitizeFolderPath('/Volumes/SSD/../../etc')).rejects.toThrow(/Invalid path/); + }); + + test.each(['https://example.com/folder', 'smb://server/share/folder', 'ftp://host/folder'])( + 'a URL: %s', + async (input) => { + await expect(sanitizeFolderPath(input)).rejects.toThrow(/URL schemes are not allowed/); + } + ); + + // A file:// URL is read as the folder it names (a dropped folder can + // arrive in that form), so it is held to the same rules as a plain path. + winOnly('a file:// URL is judged as the folder it names', async () => { + await expect(sanitizeFolderPath('file:///F:/Sorted')).resolves.toBe('F:\\Sorted'); + await expect(sanitizeFolderPath('file:///C:/Windows/System32')).rejects.toThrow( + /system directories/ + ); + }); + + winOnly.each([ + 'C:\\Windows\\System32', + 'C:\\Program Files\\Sorted', + 'D:\\Windows', + 'F:\\System Volume Information', + 'F:\\Program Files (x86)\\Sorted' + ])('a system directory on any drive: %s', async (input) => { + await expect(sanitizeFolderPath(input)).rejects.toThrow(/system directories/); + }); + + posixOnly.each(['/etc/Sorted', '/usr/sbin', '/proc/1'])( + 'a system directory: %s', + async (input) => { + await expect(sanitizeFolderPath(input)).rejects.toThrow(/system directories/); + } ); - expect(() => sanitizeFolderPath('C:\\Temp')).toThrow(/allowed directories/); }); }); diff --git a/test/smartFolders.watcherLifecycle.test.js b/test/smartFolders.watcherLifecycle.test.js index 9b8f7a4..c631112 100644 --- a/test/smartFolders.watcherLifecycle.test.js +++ b/test/smartFolders.watcherLifecycle.test.js @@ -96,7 +96,8 @@ jest.mock('../src/main/utils/jsonRepair', () => ({ })); jest.mock('../src/shared/pathSanitization', () => ({ - validateFileOperationPathSync: jest.fn((p) => ({ valid: true, normalizedPath: p })) + validateFileOperationPathSync: jest.fn((p) => ({ valid: true, normalizedPath: p })), + validateFileOperationPath: jest.fn(async (p) => ({ valid: true, normalizedPath: p })) })); jest.mock('../src/shared/crossPlatformUtils', () => ({ diff --git a/test/smartFoldersIpc.coverage.test.js b/test/smartFoldersIpc.coverage.test.js index 2843206..8c2e070 100644 --- a/test/smartFoldersIpc.coverage.test.js +++ b/test/smartFoldersIpc.coverage.test.js @@ -83,7 +83,8 @@ jest.mock('../src/main/utils/jsonRepair', () => ({ })); jest.mock('../src/shared/pathSanitization', () => ({ - validateFileOperationPathSync: jest.fn((p) => ({ valid: true, normalizedPath: p })) + validateFileOperationPathSync: jest.fn((p) => ({ valid: true, normalizedPath: p })), + validateFileOperationPath: jest.fn(async (p) => ({ valid: true, normalizedPath: p })) })); jest.mock('../src/shared/crossPlatformUtils', () => ({ diff --git a/test/smartFoldersIpc.test.js b/test/smartFoldersIpc.test.js index 69f59d2..d01fc80 100644 --- a/test/smartFoldersIpc.test.js +++ b/test/smartFoldersIpc.test.js @@ -91,7 +91,8 @@ jest.mock('../src/main/utils/jsonRepair', () => ({ })); jest.mock('../src/shared/pathSanitization', () => ({ - validateFileOperationPathSync: jest.fn((p) => ({ valid: true, normalizedPath: p })) + validateFileOperationPathSync: jest.fn((p) => ({ valid: true, normalizedPath: p })), + validateFileOperationPath: jest.fn(async (p) => ({ valid: true, normalizedPath: p })) })); jest.mock('../src/shared/crossPlatformUtils', () => ({ @@ -397,6 +398,53 @@ describe('Smart Folders IPC', () => { expect(result).toEqual(expect.objectContaining({ success: true })); }); + test('a folder on another drive is put to the destination policy and registered once it agrees', async () => { + const handler = getHandler(IPC_CHANNELS.SMART_FOLDERS.ADD); + const { enhanceSmartFolderWithLLM } = require('../src/main/services/SmartFoldersLLMService'); + enhanceSmartFolderWithLLM.mockResolvedValueOnce(null); + mockAuthorizeSmartFolderRoot.mockResolvedValueOnce({ allowed: true, reason: 'confirmed' }); + fs.stat + .mockResolvedValueOnce({ isDirectory: () => true }) + .mockRejectedValueOnce({ code: 'ENOENT' }) + .mockResolvedValueOnce({ isDirectory: () => true }); + fs.mkdir.mockResolvedValue(); + fs.writeFile.mockResolvedValue(); + fs.unlink.mockResolvedValue(); + + const result = await handler({}, { name: 'Invoices', path: 'F:\\Sorted\\Invoices' }); + + expect(result).toEqual(expect.objectContaining({ success: true })); + expect(mockAuthorizeSmartFolderRoot).toHaveBeenCalledWith( + 'F:\\Sorted\\Invoices', + expect.any(Object) + ); + const savedFolders = mockFoldersService.setCustomFolders.mock.calls[0][0]; + expect(savedFolders.map((f) => f.path)).toContain('F:\\Sorted\\Invoices'); + }); + + test('a path that fails the check is refused before the destination policy is asked', async () => { + const { validateFileOperationPath } = require('../src/shared/pathSanitization'); + validateFileOperationPath.mockResolvedValueOnce({ + valid: false, + normalizedPath: '', + error: 'Invalid path: access to system directories is not allowed' + }); + + const result = await getHandler(IPC_CHANNELS.SMART_FOLDERS.ADD)( + {}, + { name: 'System', path: 'F:\\System Volume Information' } + ); + + expect(result).toEqual({ + success: false, + error: 'Invalid path: access to system directories is not allowed', + errorCode: 'SECURITY_PATH_VIOLATION' + }); + expect(mockAuthorizeSmartFolderRoot).not.toHaveBeenCalled(); + expect(mockFoldersService.saveCustomFolders).not.toHaveBeenCalled(); + expect(fs.mkdir).not.toHaveBeenCalled(); + }); + test('validates folder name characters', async () => { const handler = getHandler(IPC_CHANNELS.SMART_FOLDERS.ADD); const invalidFolder = { name: 'Bad:Name', path: 'C:\\Test' }; @@ -557,6 +605,57 @@ describe('Smart Folders IPC', () => { expect(result.files).toHaveLength(1); expect(result.files[0].name).toBe('doc.pdf'); }); + + test('scans a folder on another drive', async () => { + mockFoldersService.scanDirectory.mockResolvedValue([ + { type: 'file', name: 'scan.pdf', path: 'F:\\Strato Input\\scan.pdf', size: 100 } + ]); + + const result = await getHandler(IPC_CHANNELS.SMART_FOLDERS.SCAN_STRUCTURE)( + {}, + 'F:\\Strato Input' + ); + + expect(result.success).toBe(true); + expect(result.files).toHaveLength(1); + expect(mockFoldersService.scanDirectory).toHaveBeenCalledWith( + 'F:\\Strato Input', + undefined, + expect.any(Object) + ); + }); + + test('a path that fails the check is not scanned, and the reason is passed on', async () => { + const { validateFileOperationPath } = require('../src/shared/pathSanitization'); + validateFileOperationPath.mockResolvedValueOnce({ + valid: false, + normalizedPath: '', + error: 'Invalid path: access to system directories is not allowed' + }); + + const result = await getHandler(IPC_CHANNELS.SMART_FOLDERS.SCAN_STRUCTURE)( + {}, + 'C:\\Windows\\System32' + ); + + expect(result).toEqual({ + success: false, + error: 'Invalid path: access to system directories is not allowed', + errorCode: 'SECURITY_PATH_VIOLATION' + }); + expect(mockFoldersService.scanDirectory).not.toHaveBeenCalled(); + }); + + test('a scan that fails reports the failure instead of an empty folder', async () => { + mockFoldersService.scanDirectory.mockRejectedValue(new Error('EIO: i/o error, scandir')); + + const result = await getHandler(IPC_CHANNELS.SMART_FOLDERS.SCAN_STRUCTURE)( + {}, + 'F:\\Unplugged' + ); + + expect(result).toEqual({ success: false, error: 'EIO: i/o error, scandir' }); + }); }); describe('SMART_FOLDERS.WATCHER_START', () => { diff --git a/test/useFileHandlers.test.js b/test/useFileHandlers.test.js index ec19300..3d4944c 100644 --- a/test/useFileHandlers.test.js +++ b/test/useFileHandlers.test.js @@ -398,6 +398,64 @@ describe('useFileHandlers', () => { ); }); + // Main answers a refused or failed scan with `success: false` and no + // `files`. That is not an empty folder, and must not read like one. + test('a folder main refuses to scan shows the refusal, not "no files"', async () => { + mockElectronAPI.files.selectDirectory.mockResolvedValue({ + success: true, + path: 'C:\\Windows\\System32' + }); + mockElectronAPI.smartFolders.scanStructure.mockResolvedValue({ + success: false, + error: 'Invalid path: access to system directories is not allowed', + errorCode: 'SECURITY_PATH_VIOLATION' + }); + + const { result } = renderHook(() => useFileHandlers(createMockOptions())); + + await act(async () => { + await result.current.handleFolderSelection(); + }); + + expect(mockAddNotification).toHaveBeenCalledTimes(1); + expect(mockAddNotification).toHaveBeenCalledWith( + 'The selected folder was not scanned. Invalid path: access to system directories is not allowed', + 'error', + 5000, + 'folder-scan-error' + ); + expect(mockAddSelectedFiles).not.toHaveBeenCalled(); + expect(mockAnalyzeFiles).not.toHaveBeenCalled(); + expect(result.current.isScanning).toBe(false); + }); + + test('a scan that fails shows the failure, not "no files"', async () => { + mockElectronAPI.files.selectDirectory.mockResolvedValue({ + success: true, + path: 'F:\\Unplugged' + }); + mockElectronAPI.smartFolders.scanStructure.mockResolvedValue({ + success: false, + error: 'The drive could not be read' + }); + + const { result } = renderHook(() => useFileHandlers(createMockOptions())); + + await act(async () => { + await result.current.handleFolderSelection(); + }); + + expect(mockAddNotification).toHaveBeenCalledTimes(1); + expect(mockAddNotification).toHaveBeenCalledWith( + expect.stringMatching(/^The selected folder was not scanned\. \S/), + 'error', + 5000, + 'folder-scan-error' + ); + expect(mockAddSelectedFiles).not.toHaveBeenCalled(); + expect(result.current.isScanning).toBe(false); + }); + test('handles folder selection errors', async () => { mockElectronAPI.files.selectDirectory.mockRejectedValue(new Error('Folder error')); @@ -572,6 +630,58 @@ describe('useFileHandlers', () => { isDirectory: true }); + test('a dropped folder main refuses to scan is named with the reason', async () => { + mockElectronAPI.files.getStats.mockResolvedValueOnce(directoryStats()); + mockElectronAPI.smartFolders.scanStructure.mockResolvedValueOnce({ + success: false, + error: 'Invalid path: access to system directories is not allowed', + errorCode: 'SECURITY_PATH_VIOLATION' + }); + + const { result } = renderHook(() => useFileHandlers(createMockOptions())); + + await act(async () => { + await result.current.handleFileDrop([{ path: '/dropped-folder', name: 'dropped-folder' }]); + }); + + expect(mockAddNotification).toHaveBeenCalledWith( + 'Dropped folder not scanned: dropped-folder. Invalid path: access to system directories is not allowed', + 'error', + 5000, + 'drop-folder-error' + ); + expect(mockAddSelectedFiles).not.toHaveBeenCalled(); + expect(mockAnalyzeFiles).not.toHaveBeenCalled(); + }); + + test('a refused folder does not stop the files dropped with it', async () => { + mockElectronAPI.files.getStats.mockResolvedValueOnce(directoryStats()); + mockElectronAPI.smartFolders.scanStructure.mockResolvedValueOnce({ + success: false, + error: 'Invalid path: access to system directories is not allowed', + errorCode: 'SECURITY_PATH_VIOLATION' + }); + + const { result } = renderHook(() => useFileHandlers(createMockOptions())); + + await act(async () => { + await result.current.handleFileDrop([ + { path: '/dropped-folder', name: 'dropped-folder' }, + { path: '/dropped.txt', name: 'dropped.txt' } + ]); + }); + + expect(mockAddNotification).toHaveBeenCalledWith( + expect.stringContaining('Dropped folder not scanned: dropped-folder.'), + 'error', + 5000, + 'drop-folder-error' + ); + expect(mockAnalyzeFiles).toHaveBeenCalledWith([ + expect.objectContaining({ path: '/dropped.txt' }) + ]); + }); + test('dropping a folder whose files are all queued adds and analyzes nothing', async () => { mockElectronAPI.files.getStats.mockResolvedValueOnce(directoryStats()); mockElectronAPI.smartFolders.scanStructure.mockResolvedValueOnce({ diff --git a/test/useOrganization.inPlaceMoves.test.js b/test/useOrganization.inPlaceMoves.test.js new file mode 100644 index 0000000..fdf0398 --- /dev/null +++ b/test/useOrganization.inPlaceMoves.test.js @@ -0,0 +1,193 @@ +/** + * A folder can be both where files wait and a smart folder files are sent + * to. A file routed to the folder it is already in is renamed there, or left + * untouched, and the run still counts it as moved; nothing appears anywhere + * new. `inPlaceMoves` lists those files before the run so the screen can say + * so. + */ + +import { renderHook } from '@testing-library/react'; + +jest.mock('../src/shared/logger', () => { + const logger = { + setContext: jest.fn(), + info: jest.fn(), + debug: jest.fn(), + warn: jest.fn(), + error: jest.fn() + }; + return { logger, createLogger: jest.fn(() => logger) }; +}); + +jest.mock('../src/shared/constants', () => ({ + PHASES: { DISCOVER: 'discover', ORGANIZE: 'organize', COMPLETE: 'complete' }, + IPC_CHANNELS: { + FILES: { PERFORM_OPERATION: 'files:perform-operation' } + } +})); + +jest.mock('../src/renderer/components/UndoRedoSystem', () => ({ + createOrganizeBatchAction: jest.fn() +})); + +const { + findInPlaceMoves, + useOrganization +} = require('../src/renderer/phases/organize/useOrganization'); +const { toCanonicalPath } = require('../src/renderer/utils/canonicalPath'); + +const INPUT = { id: 'in', name: 'Strato Input', path: '/Users/hans/Strato Input' }; +const BING = { id: 'bing', name: 'Bing data', path: '/Users/hans/Sorted/Bing data' }; + +const file = (path, category, suggestedName) => { + const name = path.split(/[\\/]/).pop(); + return { path, name, analysis: { category, suggestedName: suggestedName || name } }; +}; + +/** Plan inputs the way OrganizePhase passes them, with name matching only. */ +function plan(files, { smartFolders = [INPUT, BING], editingFiles = {}, ...rest } = {}) { + return { + files, + editingFiles, + getFileWithEdits: jest.fn((f) => f), + findSmartFolderForCategory: jest.fn( + (label) => + smartFolders.find((folder) => folder.name.toLowerCase() === String(label).toLowerCase()) || + null + ), + defaultLocation: '/Users/hans/Documents', + smartFolders, + ...rest + }; +} + +describe('findInPlaceMoves', () => { + beforeEach(() => { + global.window = global.window || {}; + global.window.electronAPI = { files: { normalizePath: jest.fn((p) => p) } }; + }); + + test('lists a file sent to the folder it is in, and says whether it gets a new name', () => { + const renamed = file('/Users/hans/Strato Input/a.csv', 'Strato Input', 'Keywords 2025.csv'); + const untouched = file('/Users/hans/Strato Input/b.csv', 'Strato Input'); + + expect(findInPlaceMoves(plan([renamed, untouched]))).toEqual([ + { sourcePath: renamed.path, fileName: 'a.csv', folderName: 'Strato Input', renamed: true }, + { sourcePath: untouched.path, fileName: 'b.csv', folderName: 'Strato Input', renamed: false } + ]); + }); + + test('leaves out a file that is sent to another folder', () => { + const leaving = file('/Users/hans/Strato Input/bing.csv', 'Bing data'); + expect(findInPlaceMoves(plan([leaving]))).toEqual([]); + }); + + test('a file drops out once the user picks another folder on its card', () => { + const picked = file('/Users/hans/Strato Input/a.csv', 'Strato Input'); + const editingFiles = { [toCanonicalPath(picked.path)]: { category: 'Bing data' } }; + + expect(findInPlaceMoves(plan([picked]))).toHaveLength(1); + expect(findInPlaceMoves(plan([picked], { editingFiles }))).toEqual([]); + }); + + test('a file in a subfolder of a smart folder is moved, so it is not listed', () => { + const nested = file('/Users/hans/Strato Input/2025/a.csv', 'Strato Input'); + expect(findInPlaceMoves(plan([nested]))).toEqual([]); + }); + + test('does not work out a destination for files outside every smart folder', () => { + const inputs = plan([file('/Users/hans/Downloads/a.csv', 'Strato Input')]); + + expect(findInPlaceMoves(inputs)).toEqual([]); + expect(inputs.getFileWithEdits).not.toHaveBeenCalled(); + expect(inputs.findSmartFolderForCategory).not.toHaveBeenCalled(); + }); + + test('a smart folder without a path lives under the default location', () => { + const reports = { id: 'r', name: 'Reports' }; + const filed = file('/Users/hans/Documents/Reports/q3.pdf', 'Reports'); + + expect(findInPlaceMoves(plan([filed], { smartFolders: [reports] }))).toEqual([ + { sourcePath: filed.path, fileName: 'q3.pdf', folderName: 'Reports', renamed: false } + ]); + }); + + test('a file already in Uncategorized that matches no folder stays there', () => { + const stray = file('/Users/hans/Documents/Uncategorized/notes.txt', 'Recipes'); + + expect(findInPlaceMoves(plan([stray]))).toEqual([ + { sourcePath: stray.path, fileName: 'notes.txt', folderName: 'Uncategorized', renamed: false } + ]); + }); + + test('Windows spellings of one folder match', () => { + const smartFolders = [{ id: 'in', name: 'Strato Input', path: 'c:/users/hans/strato input/' }]; + const filed = file('C:\\Users\\Hans\\Strato Input\\a.csv', 'Strato Input'); + + expect( + findInPlaceMoves( + plan([filed], { smartFolders, defaultLocation: 'C:\\Users\\Hans\\Documents' }) + ) + ).toEqual([ + { sourcePath: filed.path, fileName: 'a.csv', folderName: 'Strato Input', renamed: false } + ]); + }); + + test('nothing to list without files or a default location', () => { + const filed = file('/Users/hans/Strato Input/a.csv', 'Strato Input'); + + expect(findInPlaceMoves(plan(undefined))).toEqual([]); + expect(findInPlaceMoves(plan([null, {}, { path: 42 }]))).toEqual([]); + expect(findInPlaceMoves(plan([filed], { defaultLocation: undefined }))).toEqual([]); + expect(findInPlaceMoves(plan([filed], { defaultLocation: '' }))).toEqual([]); + }); +}); + +describe('useOrganization: inPlaceMoves', () => { + const hookOptions = (unprocessedFiles) => { + const { files: _files, ...inputs } = plan(unprocessedFiles); + return { + ...inputs, + unprocessedFiles, + analysisResults: [], + markFilesAsProcessed: jest.fn(), + unmarkFilesAsProcessed: jest.fn(), + actions: { setPhaseData: jest.fn(), advancePhase: jest.fn() }, + phaseData: { organizedFiles: [] }, + addNotification: jest.fn(), + dispatch: jest.fn(), + executeAction: jest.fn(), + addOrganizedFiles: jest.fn(), + removeOrganizedFiles: jest.fn(), + setOrganizingState: jest.fn() + }; + }; + + beforeEach(() => { + global.window = global.window || {}; + global.window.electronAPI = { + events: { onOperationProgress: jest.fn(() => jest.fn()) }, + files: { normalizePath: jest.fn((p) => p) } + }; + }); + + test('covers every ready file in the queue, and only ready files', () => { + const ready = file('/Users/hans/Strato Input/a.csv', 'Strato Input'); + const notAnalyzed = { path: '/Users/hans/Strato Input/b.csv', name: 'b.csv' }; + const leaving = file('/Users/hans/Strato Input/bing.csv', 'Bing data'); + + const options = hookOptions([ready, notAnalyzed, leaving]); + const { result } = renderHook(() => useOrganization(options)); + + expect(result.current.inPlaceMoves).toEqual([ + { sourcePath: ready.path, fileName: 'a.csv', folderName: 'Strato Input', renamed: false } + ]); + }); + + test('is empty when no file stays where it is', () => { + const options = hookOptions([file('/Users/hans/Downloads/a.csv', 'Bing data')]); + const { result } = renderHook(() => useOrganization(options)); + + expect(result.current.inPlaceMoves).toEqual([]); + }); +}); diff --git a/test/useOrganization.rolledBackBatch.test.js b/test/useOrganization.rolledBackBatch.test.js index bf73635..c56db6f 100644 --- a/test/useOrganization.rolledBackBatch.test.js +++ b/test/useOrganization.rolledBackBatch.test.js @@ -146,7 +146,15 @@ describe('useOrganization: rolled-back batch (fileops-organize-2)', () => { expect(options.dispatch).not.toHaveBeenCalled(); expect(options.markFilesAsProcessed).not.toHaveBeenCalled(); expect(options.actions.advancePhase).not.toHaveBeenCalled(); - expect(hook.current.lastOrganizeSummary).toBeNull(); + // The banner repeats the toast and stays after the toast is gone. + expect(hook.current.lastOrganizeSummary).toMatchObject({ + outcome: 'rolledBack', + count: 0, + headline: 'Organization was rolled back' + }); + expect(message).toBe( + `Organization was rolled back. ${hook.current.lastOrganizeSummary.detail}` + ); }); test('partially restored batch: names files left at their destination and the recovery file', async () => { diff --git a/test/useOrganization.runOutcome.test.js b/test/useOrganization.runOutcome.test.js new file mode 100644 index 0000000..f14d09d --- /dev/null +++ b/test/useOrganization.runOutcome.test.js @@ -0,0 +1,441 @@ +/** + * How a finished organize run is reported: one toast and one banner + * (`lastOrganizeSummary`) that say what was moved, what was not and why. + * + * A file that was not moved must never be counted as organized: a failed row, + * a row the run never reached, and a duplicate that was removed because the + * destination already held the same content are each reported for what they + * are. A run that left files behind stays on Organize. + */ + +import { renderHook, act } from '@testing-library/react'; + +jest.mock('../src/shared/logger', () => { + const logger = { + setContext: jest.fn(), + info: jest.fn(), + debug: jest.fn(), + warn: jest.fn(), + error: jest.fn() + }; + return { logger, createLogger: jest.fn(() => logger) }; +}); + +jest.mock('../src/shared/constants', () => ({ + PHASES: { DISCOVER: 'discover', ORGANIZE: 'organize', COMPLETE: 'complete' }, + IPC_CHANNELS: { + FILES: { PERFORM_OPERATION: 'files:perform-operation' } + } +})); + +jest.mock('../src/renderer/components/UndoRedoSystem', () => ({ + createOrganizeBatchAction: jest.fn((label, operations, callbacks, options) => ({ + type: 'organize-batch', + label, + operations, + callbacks, + options + })) +})); + +const files = ['a', 'b', 'c'].map((n) => ({ + path: `/in/${n}.txt`, + name: `${n}.txt`, + analysis: { category: 'documents', suggestedName: `${n.toUpperCase()}.txt` } +})); + +const moved = (n) => ({ + success: true, + source: `/in/${n}.txt`, + destination: `/Documents/${n.toUpperCase()}.txt` +}); +const failed = (n, error = 'EEXIST: destination exists') => ({ + success: false, + source: `/in/${n}.txt`, + destination: `/Documents/${n.toUpperCase()}.txt`, + error +}); +const duplicate = (n, sourceRemoval = 'trashed') => ({ + success: true, + source: `/in/${n}.txt`, + destination: '/Documents/A.txt', + operation: 'move', + skipped: true, + reason: 'duplicate', + sourceRemoval +}); +const notStarted = (n, flag) => ({ + success: false, + source: `/in/${n}.txt`, + destination: `/Documents/${n.toUpperCase()}.txt`, + operation: 'move', + [flag]: true +}); + +function buildOptions(batchResult, overrides = {}) { + return { + unprocessedFiles: files, + editingFiles: {}, + getFileWithEdits: jest.fn((file) => file), + findSmartFolderForCategory: jest.fn(() => null), + defaultLocation: '/Documents', + smartFolders: [], + analysisResults: [], + markFilesAsProcessed: jest.fn(), + unmarkFilesAsProcessed: jest.fn(), + actions: { setPhaseData: jest.fn(), advancePhase: jest.fn() }, + phaseData: { organizedFiles: [] }, + addNotification: jest.fn(), + dispatch: jest.fn(), + // Like the real executor: run the action's onExecute, then resolve. + executeAction: jest.fn().mockImplementation(async (action) => { + action?.callbacks?.onExecute?.(batchResult); + return batchResult; + }), + addOrganizedFiles: jest.fn(), + removeOrganizedFiles: jest.fn(), + setOrganizingState: jest.fn(), + ...overrides + }; +} + +// The routing note about categories mapped to Uncategorized is unrelated to +// the batch outcome; these tests look only at the outcome toasts. +function outcomeToasts(options) { + return options.addNotification.mock.calls.filter((call) => call[3] !== 'category-changed'); +} + +describe('useOrganization: how a finished run is reported', () => { + let useOrganization; + + async function run(batchResult, overrides) { + const options = buildOptions(batchResult, overrides); + const rendered = renderHook(() => useOrganization(options)); + await act(async () => { + await rendered.result.current.handleOrganizeFiles(); + }); + return { options, hook: rendered.result }; + } + + beforeEach(() => { + jest.clearAllMocks(); + global.window = global.window || {}; + global.window.electronAPI = { + events: { onOperationProgress: jest.fn(() => jest.fn()) }, + files: { normalizePath: jest.fn((p) => p) } + }; + useOrganization = require('../src/renderer/phases/organize/useOrganization').useOrganization; + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + describe('every file moved', () => { + test('one success toast, a banner with the count, and on to Complete', async () => { + const { options, hook } = await run({ + success: true, + results: [moved('a'), moved('b'), moved('c')], + successCount: 3 + }); + + expect(outcomeToasts(options)).toEqual([['Organized 3 files', 'success', 7000]]); + expect(hook.current.lastOrganizeSummary).toEqual({ + outcome: 'moved', + count: 3, + headline: 'Moved 3 files to their destinations', + detail: '' + }); + expect(options.actions.advancePhase).toHaveBeenCalledWith('complete'); + }); + + test('the banner clears itself', async () => { + jest.useFakeTimers(); + const { hook } = await run({ success: true, results: [moved('a')], successCount: 1 }); + expect(hook.current.lastOrganizeSummary).toMatchObject({ + headline: 'Moved 1 file to its destination' + }); + + act(() => { + jest.advanceTimersByTime(12000); + }); + + expect(hook.current.lastOrganizeSummary).toBeNull(); + }); + }); + + describe('some files not moved', () => { + const partial = { + success: true, + results: [moved('a'), moved('b'), failed('c')], + successCount: 2, + failCount: 1 + }; + + test('one warning toast with the reason, and the run stays on Organize', async () => { + const { options, hook } = await run(partial); + + const toasts = outcomeToasts(options); + expect(toasts).toHaveLength(1); + const [message, level] = toasts[0]; + expect(level).toBe('warning'); + expect(message).toMatch(/^Organized 2 files with 1 failure\. \S/); + expect(hook.current.lastOrganizeSummary).toMatchObject({ + outcome: 'partial', + count: 2, + headline: 'Moved 2 files; 1 could not be moved' + }); + expect(hook.current.lastOrganizeSummary.detail).toMatch( + /Files that were not moved are still listed\.$/ + ); + expect(options.actions.advancePhase).not.toHaveBeenCalled(); + // The files that did move are still recorded. + expect(options.addOrganizedFiles).toHaveBeenCalledWith([ + expect.objectContaining({ originalPath: '/in/a.txt' }), + expect.objectContaining({ originalPath: '/in/b.txt' }) + ]); + }); + + test('the banner stays until it is dismissed', async () => { + jest.useFakeTimers(); + const { hook } = await run(partial); + + act(() => { + jest.advanceTimersByTime(60000); + }); + expect(hook.current.lastOrganizeSummary).toMatchObject({ outcome: 'partial' }); + + act(() => { + hook.current.dismissOrganizeSummary(); + }); + expect(hook.current.lastOrganizeSummary).toBeNull(); + }); + + test('the next run clears the previous banner when it starts', async () => { + const { options, hook } = await run(partial); + let finish; + options.executeAction.mockImplementation( + () => + new Promise((resolve) => { + finish = () => resolve({ success: true, results: [], successCount: 0 }); + }) + ); + + let second; + await act(async () => { + second = hook.current.handleOrganizeFiles(); + await Promise.resolve(); + }); + expect(hook.current.lastOrganizeSummary).toBeNull(); + + await act(async () => { + finish(); + await second; + }); + }); + }); + + describe('no file moved', () => { + test('one error toast with the reason and nothing else', async () => { + const { options, hook } = await run({ + success: false, + results: [failed('a'), failed('b'), failed('c')], + successCount: 0, + failCount: 3 + }); + + const toasts = outcomeToasts(options); + expect(toasts).toHaveLength(1); + const [message, level] = toasts[0]; + expect(level).toBe('error'); + expect(message).toMatch(/^No files were organized\. \S/); + expect(hook.current.lastOrganizeSummary).toMatchObject({ + outcome: 'none', + count: 0, + headline: 'No files were organized' + }); + expect(options.addOrganizedFiles).not.toHaveBeenCalled(); + expect(options.actions.advancePhase).not.toHaveBeenCalled(); + }); + + test('a batch that stopped on an error reports that error', async () => { + const { options, hook } = await run({ + success: false, + partialFailure: true, + results: [moved('a')], + successCount: 1, + failCount: 0, + error: 'The batch lock could not be acquired', + errorCode: 'PARTIAL_FAILURE' + }); + + const toasts = outcomeToasts(options); + expect(toasts).toHaveLength(1); + expect(toasts[0][0]).toMatch(/^Organized 1 file, but the run did not finish\. \S/); + expect(toasts[0][1]).toBe('warning'); + expect(hook.current.lastOrganizeSummary).toMatchObject({ + outcome: 'partial', + headline: 'Moved 1 file; the run did not finish' + }); + expect(options.actions.advancePhase).not.toHaveBeenCalled(); + }); + }); + + describe('duplicates removed instead of moved', () => { + test('are counted apart from the files that moved', async () => { + const { options, hook } = await run({ + success: true, + results: [moved('a'), duplicate('b'), moved('c')], + // Main counts a removed duplicate as a success. + successCount: 3 + }); + + expect(outcomeToasts(options)).toEqual([ + [ + 'Organized 2 files. 1 identical copy was not moved: the destination already had the same content. It is in the system Trash.', + 'success', + 10000 + ] + ]); + expect(hook.current.lastOrganizeSummary).toEqual({ + outcome: 'moved', + count: 2, + headline: 'Moved 2 files to their destinations', + detail: + '1 identical copy was not moved: the destination already had the same content. It is in the system Trash.' + }); + expect(options.actions.advancePhase).toHaveBeenCalledWith('complete'); + }); + + test('are recorded as removed duplicates that point at the kept copy', async () => { + const { options } = await run({ + success: true, + results: [moved('a'), duplicate('b')], + successCount: 2 + }); + + const [entries] = options.addOrganizedFiles.mock.calls[0]; + expect(entries).toHaveLength(2); + expect(entries[0]).not.toHaveProperty('duplicateRemoval'); + expect(entries[1]).toMatchObject({ + originalPath: '/in/b.txt', + path: '/Documents/A.txt', + duplicateRemoval: 'trashed' + }); + }); + + test('say "removed" when a copy did not go to the Trash', async () => { + const { options } = await run({ + success: true, + results: [moved('a'), duplicate('b', 'trashed'), duplicate('c', 'deleted')], + successCount: 3 + }); + + const [message] = outcomeToasts(options)[0]; + expect(message).toBe( + 'Organized 1 file. 2 identical copies were not moved: the destination already had the same content. They were removed.' + ); + const [entries] = options.addOrganizedFiles.mock.calls[0]; + expect(entries.map((e) => e.duplicateRemoval)).toEqual([undefined, 'trashed', 'deleted']); + }); + + test('a run of nothing but duplicates does not claim a move', async () => { + const { options, hook } = await run( + { success: true, results: [duplicate('b')], successCount: 1 }, + { unprocessedFiles: [files[1]] } + ); + + expect(outcomeToasts(options)).toEqual([ + [ + 'No files needed moving. 1 identical copy was not moved: the destination already had the same content. It is in the system Trash.', + 'info', + 10000 + ] + ]); + expect(hook.current.lastOrganizeSummary).toMatchObject({ + outcome: 'moved', + count: 0, + headline: 'No files needed moving' + }); + }); + + test('are still reported when another file failed', async () => { + const { options, hook } = await run({ + success: true, + results: [duplicate('b'), failed('c')], + successCount: 1, + failCount: 1 + }); + + const toasts = outcomeToasts(options); + expect(toasts).toHaveLength(1); + expect(toasts[0][1]).toBe('error'); + expect(toasts[0][0]).toMatch(/^No files were organized\. /); + expect(toasts[0][0]).toMatch(/1 identical copy was not moved/); + expect(hook.current.lastOrganizeSummary).toMatchObject({ outcome: 'none', count: 0 }); + expect(options.actions.advancePhase).not.toHaveBeenCalled(); + }); + }); + + describe('a run that was stopped', () => { + test('files the user cancelled before they started are not failures', async () => { + const { options, hook } = await run({ + success: true, + cancelled: true, + cancelledCount: 2, + results: [moved('a'), notStarted('b', 'cancelled'), notStarted('c', 'cancelled')], + successCount: 1, + completedOperations: 3 + }); + + const toasts = outcomeToasts(options); + expect(toasts).toEqual([ + ['Organization cancelled — 1 file organized, 2 skipped.', 'warning'] + ]); + expect(hook.current.lastOrganizeSummary).toMatchObject({ outcome: 'moved', count: 1 }); + expect(options.actions.advancePhase).not.toHaveBeenCalled(); + }); + + test('a cancel before any file moved says so once', async () => { + const { options, hook } = await run({ + success: true, + cancelled: true, + cancelledCount: 3, + results: ['a', 'b', 'c'].map((n) => notStarted(n, 'cancelled')), + successCount: 0, + completedOperations: 3 + }); + + expect(outcomeToasts(options)).toEqual([ + ['Organization cancelled before any file was moved.', 'warning'] + ]); + expect(hook.current.lastOrganizeSummary).toBeNull(); + }); + + test('an interrupted run is reported as unfinished, with what happens next', async () => { + const { options, hook } = await run({ + success: true, + interrupted: true, + interruptedCount: 2, + abortReason: 'ipc-timeout', + results: [moved('a'), notStarted('b', 'interrupted'), notStarted('c', 'interrupted')], + successCount: 1, + completedOperations: 3 + }); + + expect(outcomeToasts(options)).toEqual([ + [ + 'Organized 1 file, but the run did not finish. The run was interrupted. The remaining moves resume the next time StratoSort starts.', + 'warning', + 8000 + ] + ]); + expect(hook.current.lastOrganizeSummary).toMatchObject({ + outcome: 'partial', + count: 1, + headline: 'Moved 1 file; the run did not finish' + }); + expect(options.actions.advancePhase).not.toHaveBeenCalled(); + }); + }); +}); From c53ee4caa25856127fe6c428bde10e1a987b64e0 Mon Sep 17 00:00:00 2001 From: Levy Tate <78818969+iLevyTate@users.noreply.github.com> Date: Thu, 12 Nov 2026 14:44:52 -0500 Subject: [PATCH 2/2] fix: folder matching, preview, progress and download-speed follow-ups from the Mac report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folder matching - A category counts as a misspelling of a Smart Folder name only within an allowance that grows with the name's length (none under four characters, one per four, three at most). A fixed allowance of two sent "SEO" to "Web". - A generic category with no Uncategorized folder returns no match instead of the first folder in the list. - A strategy suggestion's explanation says what named its folder (category, file type or purpose) instead of the template's title. - The analysis cache key uses each Smart Folder's whole description, not its first 64 characters. Organize preview - handlePreviewOrganize throws when the dry run fails or is refused, and returns the files the batch sends to one name. The Organize screen shows the reason, shows the button as working and ignores a second press. - The preview marks same-name files and keeps Move off while any exist. Discover - Analysis progress reads "Analyzing 3 of 10 files" and "30% complete", with the percentage once. - The notice for an analysis that continues after a restart says so. Model download - Speed and time left are the rate over the last ten seconds, reported once three seconds of data exist. The wizard row reads "Calculating speed…" until then. Docs - The Install Guide, README and release notes say which Mac file to take for Apple Silicon and for Intel. --- .github/workflows/release.yml | 1 + CHANGELOG.md | 32 ++++ README.md | 4 +- docs/INSTALL_GUIDE.md | 13 ++ src/main/services/AnalysisCacheService.js | 6 +- src/main/services/FolderMatchingService.js | 25 ++- src/main/services/ModelDownloadManager.js | 45 +++-- .../OrganizationSuggestionServiceCore.js | 9 +- src/main/services/organization/strategies.js | 16 +- .../services/organization/suggestionRanker.js | 16 +- src/renderer/components/ModelSetupWizard.jsx | 4 +- src/renderer/components/UndoRedoSystem.jsx | 17 +- .../components/discover/AnalysisProgress.jsx | 40 ++-- .../organize/OrganizePreviewModal.jsx | 55 +++++- src/renderer/phases/OrganizePhase.jsx | 57 ++++-- src/renderer/phases/discover/useAnalysis.js | 7 +- .../phases/organize/useOrganization.js | 20 +- ...lysisCacheService.characterization.test.js | 13 ++ test/FolderMatchingService.test.js | 34 +++- test/ModelDownloadManager.test.js | 90 ++++++++- ...gestionServiceCore.purposeFallback.test.js | 24 +++ test/components/AnalysisProgress.test.js | 25 ++- test/components/ModelSetupWizard.test.js | 68 +++++++ test/components/OrganizePhase.test.js | 105 ++++++++++- test/components/OrganizePreviewModal.test.js | 76 ++++++++ test/strategies.test.js | 44 +++++ test/suggestionRanker.test.js | 59 ++++++ test/useAnalysis.test.js | 6 +- test/useOrganization.preview.test.js | 171 ++++++++++++++++++ 29 files changed, 968 insertions(+), 114 deletions(-) create mode 100644 test/useOrganization.preview.test.js diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index d15a611..fdbc20a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -349,6 +349,7 @@ jobs: "- Tesseract OCR is auto-installed or uses bundled tesseract.js fallback.", "- Use **Download Base Models** after install to enable AI features.", '- **Windows:** the Setup installer is the recommended download and updates itself. The `-win-x64.zip` runs without installing: extract it once to a short folder such as `C:\StratoSort` and start `StratoSort Stack.exe`; it does not auto-update.', + ('- **macOS:** download `StratoSortStack-{0}-mac-arm64.dmg` for a Mac with Apple Silicon (M1 or later) or `StratoSortStack-{0}-mac-x64.dmg` for an Intel Mac. To check, open the Apple menu and choose **About This Mac**: "Chip" means Apple Silicon, "Processor" means Intel.' -f $version), $releaseIntegrityNote, $macIntegrityNote, "- See the [Install Guide](https://stratosort.com/docs/INSTALL_GUIDE.html) for Windows, Mac & Linux — one download, no CLI.", diff --git a/CHANGELOG.md b/CHANGELOG.md index 08bbf81..a5411f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,38 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - **A text model larger than the memory budget no longer unloads models that hold no GPU memory.** An embedding model on the CPU and Whisper on the CPU were unloaded to make room, which freed nothing, and were loaded again for the next file. +- **A short category is no longer taken for a misspelling of a short Smart Folder name.** Any name + within two letters of a folder's name counted as that folder, so a file labeled "SEO" could be + sent to a folder named "Web" or "Dev". The allowance now grows with the length of the name: none + under four letters, one letter per four, three at most. +- **A file with a generic category ("document", "file", "image") is no longer sent to the first + Smart Folder in the list when there is no Uncategorized folder.** It is left without a match, and + the other suggestions decide. +- **A suggestion says what named its folder.** "Using Date-Based organization method" described a + folder that was named after the file's category, its type or its purpose; the explanation now says + which. +- **Editing a Smart Folder's description past its first 64 characters is used by the next + analysis.** The saved answer for a file was looked up by the start of each description only, so a + change further in returned the answer for the old wording. +- **Preview on the Organize screen says why when it cannot open.** A preview that failed or was + refused said only "Nothing to preview."; it now gives the reason. The button shows that it is + working, and pressing it twice no longer asks for two previews. +- **Preview marks files that would get the same name in the same folder.** A move with such files + does not start; Preview now shows which files they are and keeps its Move button off until they + have different names. +- **Analysis progress shows one figure for files and one for percent.** It read "Analyzing 3 of 10" + with the percentage twice; it now reads "Analyzing 3 of 10 files" and "30% complete". +- **An analysis that continues after a restart says so.** The notice now reads "Continuing the + analysis that was interrupted" with the number of files left, and stays up for six seconds. +- **Model download speed and time left follow the current speed.** They were averaged from the start + of the download and shown after the first second, so the first figures could be far off and a + change in speed took a long time to show. The speed is now measured over the last ten seconds and + shown once three seconds of data have arrived; until then the row reads "Calculating speed…". + +### Documentation + +- **The Install Guide, the README and each release's notes say which Mac file to download:** + `mac-arm64` for Apple Silicon, `mac-x64` for Intel, and how to check in About This Mac. ## [5.2.0] - 2026-11-10 diff --git a/README.md b/README.md index 46db66c..805338b 100644 --- a/README.md +++ b/README.md @@ -58,7 +58,9 @@ no telemetry. ## Install 1. **Download** the installer for your system from - [GitHub Releases](https://github.com/iLevyTate/StratoSortStack/releases). + [GitHub Releases](https://github.com/iLevyTate/StratoSortStack/releases). On a Mac, take + `…-mac-arm64.dmg` for Apple Silicon (M1 or later) or `…-mac-x64.dmg` for Intel; Apple menu → + **About This Mac** shows which one you have. 2. **Launch it** and choose a model profile. StratoSort downloads its AI models once: about 7 GB for Base, or about 10 GB for Better Quality. 3. **Add a folder** and review the suggestions. diff --git a/docs/INSTALL_GUIDE.md b/docs/INSTALL_GUIDE.md index 7d92b90..5add79a 100644 --- a/docs/INSTALL_GUIDE.md +++ b/docs/INSTALL_GUIDE.md @@ -45,6 +45,19 @@ download the single file that matches your operating system. Replace **X.X.X** with the version number shown on the Releases page (e.g. `5.2.0`). The Windows arm64 build ships without bundled FFmpeg, so audio/video analysis there needs FFmpeg on your `PATH`. +### Mac -- Apple Silicon or Intel? + +Open the Apple menu and choose **About This Mac**. + +| About This Mac shows | Your Mac is | Download this file | +| :--------------------------------- | :------------ | :------------------------------------ | +| **Chip**: Apple M1, M2, M3, M4 ... | Apple Silicon | `StratoSortStack-X.X.X-mac-arm64.dmg` | +| **Processor**: ... Intel Core ... | Intel | `StratoSortStack-X.X.X-mac-x64.dmg` | + +The `arm64` file does not open on an Intel Mac. On an Apple Silicon Mac, take the `arm64` file: only +that build runs the local AI on the Mac's GPU. The `.zip` files with the same names hold the same +app and are what the in-app updater downloads; you do not need them for a first install. + ### Windows -- installer vs zip | File | What it does | Recommended? | diff --git a/src/main/services/AnalysisCacheService.js b/src/main/services/AnalysisCacheService.js index 0a2806e..edc5a82 100644 --- a/src/main/services/AnalysisCacheService.js +++ b/src/main/services/AnalysisCacheService.js @@ -162,10 +162,10 @@ class AnalysisCacheService { hasher.update(String(model || '')); hasher.update('|'); try { + // The whole description, not a prefix: the prompt reads past any prefix, + // so an edit further in must not return the answer to the old wording. const foldersKey = Array.isArray(smartFolders) - ? smartFolders - .map((f) => `${f?.name || ''}:${(f?.description || '').slice(0, 64)}`) - .join(',') + ? smartFolders.map((f) => `${f?.name || ''}:${f?.description || ''}`).join(',') : ''; hasher.update(foldersKey); } catch { diff --git a/src/main/services/FolderMatchingService.js b/src/main/services/FolderMatchingService.js index 46e2693..6824372 100644 --- a/src/main/services/FolderMatchingService.js +++ b/src/main/services/FolderMatchingService.js @@ -1739,12 +1739,26 @@ class FolderMatchingService { return matrix[b.length][a.length]; } + /** + * How many edits two names may differ by and still count as one name + * misspelled: one per four characters of the longer name, at most three. + * A fixed allowance treats short names as typos of each other ("Web", "Dev" + * and "Geo" are each two edits from "SEO"). + * @param {string} a - First string + * @param {string} b - Second string + * @returns {number} Allowed edit distance (0 for names under four characters) + */ + static allowedSpellingEdits(a, b) { + return Math.min(3, Math.floor(Math.max(a.length, b.length) / 4)); + } + /** * Match a category string to a smart folder using fuzzy matching logic * (Static helper to allow usage without instantiation) * @param {string} category - The raw category string from LLM * @param {any[]} smartFolders - List of available smart folders - * @returns {string} The matched folder name + * @returns {string|null} The matched folder name, or null when nothing + * matches and there is no Uncategorized folder */ static matchCategoryToFolder(category, smartFolders) { const folders = Array.isArray(smartFolders) ? smartFolders : []; @@ -1814,18 +1828,19 @@ class FolderMatchingService { normalizedRaw === 'images' || normalizedRaw === 'default' ) { - return uncategorized?.name || folders[0].name; + return uncategorized?.name || null; } - // Levenshtein distance check for typos (allow 2 edits for short strings, 3 for long) + // Levenshtein distance check for typos let bestLevenshtein = null; let minDistance = Infinity; for (const f of folders) { const name = String(f?.name || '').trim(); if (!name) continue; - const dist = FolderMatchingService.levenshteinDistance(normalizedRaw, name.toLowerCase()); - const threshold = name.length > 10 ? 3 : 2; + const lowerName = name.toLowerCase(); + const dist = FolderMatchingService.levenshteinDistance(normalizedRaw, lowerName); + const threshold = FolderMatchingService.allowedSpellingEdits(normalizedRaw, lowerName); if (dist <= threshold && dist < minDistance) { minDistance = dist; diff --git a/src/main/services/ModelDownloadManager.js b/src/main/services/ModelDownloadManager.js index 9dfe217..f093018 100644 --- a/src/main/services/ModelDownloadManager.js +++ b/src/main/services/ModelDownloadManager.js @@ -25,6 +25,12 @@ const DEFAULT_DOWNLOAD_MAX_RETRIES = 2; const RETRY_BASE_DELAY_MS = 500; const REQUIRED_SHA256_REGEX = /^[a-f0-9]{64}$/i; const ALLOWED_MODEL_DOWNLOAD_HOSTS = Object.freeze(['huggingface.co', 'hf.co']); +// Download speed is the rate over the last SPEED_WINDOW_MS, measured from the +// first byte. It is not reported until SPEED_MIN_SPAN_MS of data has arrived: +// an estimate from the first second can be off by orders of magnitude. +const SPEED_WINDOW_MS = 10_000; +const SPEED_MIN_SPAN_MS = 3_000; +const SPEED_SAMPLE_INTERVAL_MS = 250; class ModelDownloadManager { constructor() { @@ -310,8 +316,8 @@ class ModelDownloadManager { url: validatedDownloadUrl, totalBytes: modelInfo.size, downloadedBytes: startByte, - startByte, // Track initial byte offset for accurate speed calculation - startTime: Date.now(), + /** @type {Array<{ time: number, bytes: number }>} */ + speedSamples: [], status: 'downloading', abortController: internalAbortController }; @@ -522,7 +528,6 @@ class ModelDownloadManager { logger.info('[Download] Server ignored range request, restarting from beginning'); startByte = 0; downloadState.downloadedBytes = 0; - downloadState.startByte = 0; isResume = false; } @@ -548,6 +553,7 @@ class ModelDownloadManager { response.on('data', (chunk) => { downloaded += chunk.length; downloadState.downloadedBytes = downloaded; + this._recordSpeedSample(downloadState); const progress = { filename, @@ -906,16 +912,33 @@ class ModelDownloadManager { return wrapped; } - _calculateSpeed(/** @type {any} */ state) { - const elapsed = (Date.now() - state.startTime) / 1000; - if (elapsed < 1) return 0; - // Subtract startByte to only measure bytes downloaded in THIS session - const sessionBytes = state.downloadedBytes - (state.startByte || 0); - return Math.round(sessionBytes / elapsed); + /** + * Note how many bytes have arrived by now. Samples older than the speed + * window are dropped, except the newest of them: it is where the window + * starts. + */ + _recordSpeedSample(/** @type {any} */ state, now = Date.now()) { + const samples = state.speedSamples; + const last = samples[samples.length - 1]; + if (last && now - last.time < SPEED_SAMPLE_INTERVAL_MS) return; + samples.push({ time: now, bytes: state.downloadedBytes }); + while (samples.length > 1 && now - samples[1].time >= SPEED_WINDOW_MS) samples.shift(); + } + + /** + * Bytes per second over the speed window, or 0 while there is too little + * data to tell (the screen then shows no speed and no time left). + */ + _calculateSpeed(/** @type {any} */ state, now = Date.now()) { + const windowStart = state.speedSamples[0]; + if (!windowStart) return 0; + const span = now - windowStart.time; + if (span < SPEED_MIN_SPAN_MS) return 0; + return Math.round(((state.downloadedBytes - windowStart.bytes) * 1000) / span); } - _calculateETA(/** @type {any} */ state) { - const speed = this._calculateSpeed(state); + _calculateETA(/** @type {any} */ state, now = Date.now()) { + const speed = this._calculateSpeed(state, now); if (speed === 0) return Infinity; const remaining = state.totalBytes - state.downloadedBytes; return Math.round(remaining / speed); diff --git a/src/main/services/organization/OrganizationSuggestionServiceCore.js b/src/main/services/organization/OrganizationSuggestionServiceCore.js index c9f4546..15570ba 100644 --- a/src/main/services/organization/OrganizationSuggestionServiceCore.js +++ b/src/main/services/organization/OrganizationSuggestionServiceCore.js @@ -587,14 +587,15 @@ class OrganizationSuggestionServiceCore { ); // Verify match quality to avoid boosting fallbacks - // matchCategoryToFolder may return the first folder as a fallback if no match is found. + // matchCategoryToFolder can land on a folder through its description, its + // keywords or the Uncategorized fallback. // We only want to boost if the LLM output actually resembles the folder name. let isHighQualityMatch = false; if (folderMatch) { const rawLower = rawCategory.toLowerCase(); const matchLower = matchedFolder.toLowerCase(); - // Check for exact/partial match or reasonable edit distance + // Check for exact/partial match or a spelling variant of the name if ( rawLower === matchLower || rawLower.includes(matchLower) || @@ -603,9 +604,7 @@ class OrganizationSuggestionServiceCore { isHighQualityMatch = true; } else { const dist = FolderMatchingService.levenshteinDistance(rawLower, matchLower); - const len = Math.max(rawLower.length, matchLower.length); - // Allow ~30% difference or up to 3 chars - if (dist <= 3 || dist / len < 0.3) { + if (dist <= FolderMatchingService.allowedSpellingEdits(rawLower, matchLower)) { isHighQualityMatch = true; } } diff --git a/src/main/services/organization/strategies.js b/src/main/services/organization/strategies.js index 7f44f93..4d4e479 100644 --- a/src/main/services/organization/strategies.js +++ b/src/main/services/organization/strategies.js @@ -108,12 +108,22 @@ function matchesStrategyPattern(filename, pattern) { * @param {Record} file - File object * @param {Record} strategy - Strategy definition * @param {Array} smartFolders - Available smart folders - * @returns {Record} Folder name and path + * @returns {Record} Folder name and path, and what named the + * folder (`namedBy`: 'category', 'fileType', 'purpose' or null) */ function mapFileToStrategy(file, strategy, smartFolders) { const analysis = file.analysis || {}; const { pattern } = strategy; + // The folder is the last part of the pattern, so that part's placeholder is + // what picks it. A suggestion is explained by this, not by the template's + // title: "Date-Based" names its folder after the analysis category. + const lastPart = pattern.split('/').pop(); + let namedBy = null; + if (lastPart === '{category}' && analysis.category) namedBy = 'category'; + else if (lastPart === '{file_type}') namedBy = 'fileType'; + else if (lastPart === '{specific_folder}' && analysis.purpose) namedBy = 'purpose'; + // Replace pattern variables with actual values const folderPath = pattern .replace('{project_name}', analysis.project || 'General') @@ -136,7 +146,8 @@ function mapFileToStrategy(file, strategy, smartFolders) { return { name: matchingFolder?.name || path.basename(folderPath), - path: matchingFolder?.path || folderPath + path: matchingFolder?.path || folderPath, + namedBy }; } @@ -162,6 +173,7 @@ function getStrategyBasedSuggestions(file, smartFolders, threshold = 0.3) { confidence: score, strategy: strategyId, strategyName: strategy.name, + namedBy: folder.namedBy, pattern: strategy.pattern, method: 'strategy_based' }); diff --git a/src/main/services/organization/suggestionRanker.js b/src/main/services/organization/suggestionRanker.js index d6af1ac..1d867c3 100644 --- a/src/main/services/organization/suggestionRanker.js +++ b/src/main/services/organization/suggestionRanker.js @@ -45,6 +45,9 @@ function rankSuggestions(suggestions) { if (suggestion.confidence > existing.confidence) { existing.source = suggestion.source; existing.method = suggestion.method; + // The explanation is written from these, so they follow the source. + existing.strategyName = suggestion.strategyName; + existing.namedBy = suggestion.namedBy; } // Then update to max scores @@ -115,11 +118,22 @@ function generateExplanation(suggestion, file) { return 'No clear match found. Consider creating a new folder.'; } + // A strategy template names its folder after one part of the file. Say + // which part when it is known; the template's title describes none of them. + /** @type {Record} */ + const strategyFolderNamedBy = { + category: `The analysis labeled this file "${suggestion.folder}"`, + fileType: `"${suggestion.folder}" matches this file's type`, + purpose: `The analysis described this file as "${suggestion.folder}"` + }; + /** @type {Record} */ const explanations = { semantic: `This file's meaning matches "${suggestion.folder}"`, user_pattern: `You've organized similar files this way before`, - strategy: `Using ${suggestion.strategyName || 'your preferred'} organization method`, + strategy: + strategyFolderNamedBy[suggestion.namedBy] || + `Using ${suggestion.strategyName || 'your preferred'} organization method`, llm: `Based on the file's content and purpose`, pattern: `This is where ${(file?.extension || 'these').toUpperCase()} files usually go`, llm_creative: suggestion.reasoning || 'Alternative way to organize this file', diff --git a/src/renderer/components/ModelSetupWizard.jsx b/src/renderer/components/ModelSetupWizard.jsx index 69e6072..fc047b5 100644 --- a/src/renderer/components/ModelSetupWizard.jsx +++ b/src/renderer/components/ModelSetupWizard.jsx @@ -1509,7 +1509,9 @@ function ModelSetupWizardBody({ onComplete, onSkip, onDismiss, onStepChange }) { ? 'Download failed' : status === 'cancelled' ? 'Download cancelled' - : 'Starting…'} + : downloadedBytes > 0 + ? 'Calculating speed…' + : 'Starting…'}
{progress.etaSeconds ? `ETA ${formatDuration(progress.etaSeconds * 1000)}` : ''} diff --git a/src/renderer/components/UndoRedoSystem.jsx b/src/renderer/components/UndoRedoSystem.jsx index 5561e99..a9f7533 100644 --- a/src/renderer/components/UndoRedoSystem.jsx +++ b/src/renderer/components/UndoRedoSystem.jsx @@ -329,16 +329,21 @@ export const createSettingsAction = (description, newSettings, oldSettings) => ( * acquiring the batch lock or writing to disk. Backed by the * `operation.dryRun` short-circuit added to handleBatchOrganize on * the main side — see src/main/ipc/files/batchOrganizeHandler.js. + * A batch the handler refuses (empty, too large, an operation without a + * source or destination) comes back as `{ success: false, error, errorCode }` + * with no plan. * * @param {Array<{source: string, destination: string}>} operations * @returns {Promise<{ * success: boolean, - * dryRun: true, - * total: number, - * wouldMove: Array<{source: string, destination: string, originalDestination: string, collision: boolean}>, - * skipped: Array<{source: string, destination: string, reason: string}>, - * wouldCreateFolders: string[], - * conflicts: number + * dryRun?: true, + * total?: number, + * wouldMove?: Array<{source: string, destination: string, originalDestination: string, collision: boolean}>, + * skipped?: Array<{source: string, destination: string, reason: string}>, + * wouldCreateFolders?: string[], + * conflicts?: number, + * error?: string, + * errorCode?: string * }>} */ export const previewBatchOrganize = async (operations) => { diff --git a/src/renderer/components/discover/AnalysisProgress.jsx b/src/renderer/components/discover/AnalysisProgress.jsx index 654b7d4..7eb7f6e 100644 --- a/src/renderer/components/discover/AnalysisProgress.jsx +++ b/src/renderer/components/discover/AnalysisProgress.jsx @@ -1,7 +1,6 @@ import React, { memo } from 'react'; import PropTypes from 'prop-types'; import { FolderOpen } from 'lucide-react'; -import { StatusBadge } from '../ui'; import Card from '../ui/Card'; import { Text } from '../ui/Typography'; @@ -22,28 +21,25 @@ const AnalysisProgress = memo(function AnalysisProgress({ const Content = (
-
-
-
-
-
- -
-
-
- - {isDone - ? 'Analysis complete' - : hasTotals - ? `Analyzing ${current} of ${total}` - : 'Preparing analysis...'} - - - {hasTotals ? `${percent}%` : 'This may take a moment'} - +
+
+
+
+
- {hasTotals && {percent}%} +
+ + {isDone + ? 'Analysis complete' + : hasTotals + ? `Analyzing ${current} of ${total} ${total === 1 ? 'file' : 'files'}` + : 'Preparing analysis...'} + + + {hasTotals ? `${percent}% complete` : 'This may take a moment'} + +
@@ -55,7 +51,7 @@ const AnalysisProgress = memo(function AnalysisProgress({ aria-valuemin={0} aria-valuemax={100} aria-valuenow={percent} - aria-valuetext={`${percent} percent`} + aria-valuetext={`${percent} percent complete`} >
{ + const { groups, newFolders, sameNameCount } = useMemo(() => { const created = new Set(Array.isArray(plan?.wouldCreateFolders) ? plan.wouldCreateFolders : []); + const sameName = new Set( + (Array.isArray(plan?.sameDestination) ? plan.sameDestination : []).flatMap((clash) => + (Array.isArray(clash?.files) ? clash.files : []).map((f) => f?.sourcePath) + ) + ); /** @type {Map>} */ const byFolder = new Map(); for (const item of Array.isArray(plan?.wouldMove) ? plan.wouldMove : []) { @@ -42,6 +48,7 @@ function OrganizePreviewModal({ isOpen, plan, onClose, onConfirm, redactPaths, i fromName: src.name, toName: dest.name, collision: Boolean(item?.collision), + sameName: sameName.has(item?.source), staysInFolder: fromFolderKey !== null && fromFolderKey === toParentFolderKey(item?.destination) }); @@ -49,12 +56,19 @@ function OrganizePreviewModal({ isOpen, plan, onClose, onConfirm, redactPaths, i } return { groups: Array.from(byFolder.entries()).sort((a, b) => b[1].length - a[1].length), - newFolders: created + newFolders: created, + sameNameCount: sameName.size }; }, [plan]); const moveCount = Array.isArray(plan?.wouldMove) ? plan.wouldMove.length : 0; const skipped = Array.isArray(plan?.skipped) ? plan.skipped : []; + const summary = + moveCount === 0 + ? 'None of these files will move.' + : `${moveCount} file${moveCount !== 1 ? 's' : ''} into ${groups.length} folder${ + groups.length !== 1 ? 's' : '' + }. Nothing has moved yet.`; const folderLabel = (folder) => redactPaths ? formatDisplayPath(folder, { redact: true, segments: 1 }) : folder; const nameLabel = (name) => formatDisplayFileName(name, { redact: redactPaths }); @@ -64,9 +78,7 @@ function OrganizePreviewModal({ isOpen, plan, onClose, onConfirm, redactPaths, i isOpen={isOpen} onClose={onClose} title="Preview: where your files will go" - description={`${moveCount} file${moveCount !== 1 ? 's' : ''} into ${groups.length} folder${ - groups.length !== 1 ? 's' : '' - }. Nothing has moved yet.`} + description={summary} size="lg" dataTestId="organize-preview-modal" footer={ @@ -78,7 +90,7 @@ function OrganizePreviewModal({ isOpen, plan, onClose, onConfirm, redactPaths, i onClick={onConfirm} variant="success" size="sm" - disabled={moveCount === 0 || isOrganizing} + disabled={moveCount === 0 || sameNameCount > 0 || isOrganizing} > {`Move ${moveCount} file${moveCount !== 1 ? 's' : ''}`} @@ -87,6 +99,18 @@ function OrganizePreviewModal({ isOpen, plan, onClose, onConfirm, redactPaths, i } >
+ {sameNameCount > 0 && ( +
+ + + {sameNameCount} files would get the same name in the same folder, so the move will not + start. Close this preview, give the marked files different names, then preview again. + +
+ )} {groups.map(([folder, files]) => (
@@ -134,8 +158,13 @@ function OrganizePreviewModal({ isOpen, plan, onClose, onConfirm, redactPaths, i )} - {(f.staysInFolder || f.collision) && ( + {(f.staysInFolder || f.collision || f.sameName) && ( + {f.sameName && ( + + Same name as another file + + )} {f.staysInFolder && ( {renamed ? 'Already in this folder' : 'Already here — no change'} @@ -194,7 +223,13 @@ OrganizePreviewModal.propTypes = { skipped: PropTypes.arrayOf( PropTypes.shape({ source: PropTypes.string, reason: PropTypes.string }) ), - wouldCreateFolders: PropTypes.arrayOf(PropTypes.string) + wouldCreateFolders: PropTypes.arrayOf(PropTypes.string), + sameDestination: PropTypes.arrayOf( + PropTypes.shape({ + destination: PropTypes.string, + files: PropTypes.arrayOf(PropTypes.shape({ sourcePath: PropTypes.string })) + }) + ) }), onClose: PropTypes.func.isRequired, onConfirm: PropTypes.func.isRequired, diff --git a/src/renderer/phases/OrganizePhase.jsx b/src/renderer/phases/OrganizePhase.jsx index f8aac2b..c68b406 100644 --- a/src/renderer/phases/OrganizePhase.jsx +++ b/src/renderer/phases/OrganizePhase.jsx @@ -89,6 +89,8 @@ function OrganizePhase() { const [showHistoryModal, setShowHistoryModal] = React.useState(false); const [focusFileIndex, setFocusFileIndex] = React.useState(null); const [previewPlan, setPreviewPlan] = React.useState(null); + const [isPreviewing, setIsPreviewing] = React.useState(false); + const previewInFlightRef = useRef(false); const { revealFile } = useFileActions(); const { @@ -462,6 +464,43 @@ function OrganizePhase() { } }, [selectedFiles.size, approveSelectedFiles, handleOrganizeFiles]); + // The plan checks every file on disk, so a large queue takes a moment: the + // button shows it is working, and a second press waits for the first. + const handlePreviewClick = useCallback(async () => { + if (previewInFlightRef.current) return; + previewInFlightRef.current = true; + setIsPreviewing(true); + try { + const filesToProcess = + selectedFiles.size > 0 + ? unprocessedFiles.filter((f) => selectedFiles.has(toCanonicalPath(f.path))) + : unprocessedFiles; + const plan = await handlePreviewOrganize(filesToProcess); + if (!plan) { + addNotification( + 'None of these files has a destination yet, so there is nothing to preview.', + 'info', + 4000, + 'organize-preview' + ); + return; + } + // Show each file's destination folder and new name — a count-only + // toast did not say where anything would go. + setPreviewPlan(plan); + } catch (error) { + addNotification( + `The preview could not be built: ${error?.message || 'unknown error'}`, + 'error', + 8000, + 'organize-preview' + ); + } finally { + previewInFlightRef.current = false; + setIsPreviewing(false); + } + }, [selectedFiles, unprocessedFiles, handlePreviewOrganize, addNotification]); + // Refresh the smart-folder list after the user creates a folder inline. // useLoadInitialData only fetches when the list is empty; this fetches // unconditionally so a newly-created folder immediately becomes available @@ -806,27 +845,13 @@ function OrganizePhase() { {!isOrganizing && !isPaused ? (
diff --git a/src/renderer/phases/discover/useAnalysis.js b/src/renderer/phases/discover/useAnalysis.js index 21bb3d3..081eb70 100644 --- a/src/renderer/phases/discover/useAnalysis.js +++ b/src/renderer/phases/discover/useAnalysis.js @@ -2393,10 +2393,13 @@ export function useAnalysis(options = {}) { hasAttemptedResumeRef.current = true; if (remaining.length > 0) { + // Nobody pressed a button for this run, so say why it is running. addNotification( - `Resuming analysis of ${remaining.length} files...`, + `Continuing the analysis that was interrupted: ${remaining.length} ${ + remaining.length === 1 ? 'file' : 'files' + } left`, 'info', - 3000, + 6000, 'analysis-resume' ); if (analyzeFilesRef.current) { diff --git a/src/renderer/phases/organize/useOrganization.js b/src/renderer/phases/organize/useOrganization.js index 6e6bb8a..6868940 100644 --- a/src/renderer/phases/organize/useOrganization.js +++ b/src/renderer/phases/organize/useOrganization.js @@ -1580,8 +1580,11 @@ export function useOrganization({ * previewBatchOrganize which short-circuits the main-process handler * before any disk write. * - * Returns the main-process plan or null when the API surface is - * unavailable. Caller decides how to surface (notification, modal, + * Returns the main-process plan plus `sameDestination`, the files this + * batch sends to one name (the move refuses to start while any exist), or + * null when no file has a destination to preview. Throws when the request + * fails or is refused, so the caller can say why instead of reporting an + * empty preview. Caller decides how to surface (notification, modal, * etc.) — this hook intentionally doesn't push UI side-effects so * different phases can render the plan their own way. */ @@ -1593,7 +1596,7 @@ export function useOrganization({ : unprocessedFiles || []; if (filesToProcess.length === 0) return null; - const { operations } = buildOperations({ + const planInputs = { filesToProcess, unprocessedFiles, editingFiles, @@ -1603,16 +1606,21 @@ export function useOrganization({ defaultLocation, smartFolders, suggestionsByPath - }); + }; + const { operations } = buildOperations(planInputs); const validOps = operations.filter((/** @type {any} */ op) => op?.source && op?.destination); if (validOps.length === 0) return null; try { - return await previewBatchOrganize(validOps); + const plan = await previewBatchOrganize(validOps); + if (!plan || plan.success === false) { + throw new Error(plan?.error || 'The preview request was refused'); + } + return { ...plan, sameDestination: buildPreview(planInputs).conflicts }; } catch (error) { logger.warn('[ORGANIZE] Preview failed', { error: error?.message }); - return null; + throw error; } }, [ diff --git a/test/AnalysisCacheService.characterization.test.js b/test/AnalysisCacheService.characterization.test.js index 3f4bdb2..c217807 100644 --- a/test/AnalysisCacheService.characterization.test.js +++ b/test/AnalysisCacheService.characterization.test.js @@ -174,6 +174,19 @@ describe('AnalysisCacheService Characterization Tests', () => { expect(key1).not.toBe(key2); }); + test('generateKey() changes when a folder description is edited past its first 64 characters', () => { + const shared = + 'Client contracts, signed agreements and statements of work for all accounts. '; + expect(shared.length).toBeGreaterThan(64); + const before = cache.generateKey('text', 'model', [ + { name: 'Legal', description: `${shared}Invoices do not belong here.` } + ]); + const after = cache.generateKey('text', 'model', [ + { name: 'Legal', description: `${shared}Invoices belong here too.` } + ]); + expect(after).not.toBe(before); + }); + test('generateKey() handles null/undefined inputs', () => { expect(() => cache.generateKey(null, null, null)).not.toThrow(); expect(() => cache.generateKey(undefined, undefined, undefined)).not.toThrow(); diff --git a/test/FolderMatchingService.test.js b/test/FolderMatchingService.test.js index e53267a..d3de02c 100644 --- a/test/FolderMatchingService.test.js +++ b/test/FolderMatchingService.test.js @@ -119,13 +119,13 @@ describe('FolderMatchingService.matchCategoryToFolder — name matching pipeline }); test('Levenshtein typo within threshold matches', () => { - // distance("lab co", "lab corp") = 2, threshold for 8-char name = 2 + // distance("lab co", "lab corp") = 2, and an 8-char name allows 2 edits const folders = [{ name: 'Lab Corp' }]; expect(FolderMatchingService.matchCategoryToFolder('lab co', folders)).toBe('Lab Corp'); }); test('prefix-subsumption: short variant matches longer folder name (forward)', () => { - // "Lab" → "Lab Corp" — Levenshtein distance 5 > threshold 2, but the + // "Lab" → "Lab Corp" — Levenshtein distance 5 > the 2 edits allowed, but the // canonical "lab" is a prefix of "lab corp" at a word boundary. const folders = [{ name: 'Lab Corp' }]; expect(FolderMatchingService.matchCategoryToFolder('Lab', folders)).toBe('Lab Corp'); @@ -146,11 +146,11 @@ describe('FolderMatchingService.matchCategoryToFolder — name matching pipeline // (delta 9). Closest length wins to avoid drifting toward the longer // folder when the LLM gave a near-complete short form. const folders = [{ name: 'Lab Corporation' }, { name: 'Lab Corp' }, { name: 'Other Folder' }]; - // Note: "lab co" → "lab corp" Levenshtein dist=2 ≤ threshold 2 already + // Note: "lab co" → "lab corp" Levenshtein dist=2 ≤ the 2 edits allowed already // matches Lab Corp, so this test mostly verifies Levenshtein still wins. // Use a case that bypasses Levenshtein: const result = FolderMatchingService.matchCategoryToFolder('Lab Cor', folders); - // distance("lab cor", "lab corp")=1 ≤ threshold 2 → Levenshtein matches + // distance("lab cor", "lab corp")=1 ≤ the 2 edits allowed → Levenshtein matches expect(result).toBe('Lab Corp'); }); @@ -182,6 +182,32 @@ describe('FolderMatchingService.matchCategoryToFolder — name matching pipeline expect(FolderMatchingService.matchCategoryToFolder('documents', folders)).toBe('Uncategorized'); }); + test('a generic label with no Uncategorized folder matches nothing, not the first folder', () => { + const folders = [{ name: 'SEO' }, { name: 'Lab Corp' }]; + expect(FolderMatchingService.matchCategoryToFolder('documents', folders)).toBeNull(); + }); + + test('allows one edit per four characters of the longer name, at most three', () => { + expect(FolderMatchingService.allowedSpellingEdits('web', 'seo')).toBe(0); + expect(FolderMatchingService.allowedSpellingEdits('memo', 'seo')).toBe(1); + expect(FolderMatchingService.allowedSpellingEdits('lab co', 'lab corp')).toBe(2); + expect(FolderMatchingService.allowedSpellingEdits('presentation', 'presentations')).toBe(3); + expect(FolderMatchingService.allowedSpellingEdits('a'.repeat(40), 'b'.repeat(40))).toBe(3); + }); + + test.each(['Web', 'Dev', 'Geo', 'Memo'])( + 'short label "%s" is not read as a misspelling of a short folder name', + (label) => { + expect( + FolderMatchingService.matchCategoryToFolder(label, [ + { name: 'SEO' }, + { name: 'Uncategorized' } + ]) + ).toBe('Uncategorized'); + expect(FolderMatchingService.matchCategoryToFolder(label, [{ name: 'SEO' }])).toBeNull(); + } + ); + test('generic bucket word matches a real folder of that exact name (regression)', () => { // Default file-type taxonomy: "Documents"/"Images" are real folders, so an // LLM answer of "Documents" must land there, NOT be forced to Uncategorized diff --git a/test/ModelDownloadManager.test.js b/test/ModelDownloadManager.test.js index 243a2e1..c9c7959 100644 --- a/test/ModelDownloadManager.test.js +++ b/test/ModelDownloadManager.test.js @@ -285,19 +285,89 @@ describe('ModelDownloadManager', () => { expect(cb).toHaveBeenCalledTimes(1); }); - test('_calculateSpeed and _calculateETA handle elapsed time', () => { - const manager = new ModelDownloadManager(); - const state = { - startTime: Date.now() - 2000, - downloadedBytes: 200, - totalBytes: 1000 + // Speed is the rate over the last ten seconds, measured from the first + // byte. An average since the request was created counted the time spent + // connecting, so the first seconds produced estimates of days. + describe('download speed and time left', () => { + const T0 = 1_000_000; + /** Feed `bytesPerSecond` for `seconds`, one chunk every 250 ms. */ + const feed = (manager, state, from, seconds, bytesPerSecond) => { + const steps = seconds * 4; + for (let i = 1; i <= steps; i += 1) { + state.downloadedBytes += bytesPerSecond / 4; + manager._recordSpeedSample(state, from + i * 250); + } + return from + seconds * 1000; }; + const newState = (downloadedBytes = 0, totalBytes = 10_000_000) => ({ + downloadedBytes, + totalBytes, + speedSamples: [] + }); + + test('reports nothing before the first byte', () => { + const manager = new ModelDownloadManager(); + const state = newState(); + + expect(manager._calculateSpeed(state, T0)).toBe(0); + expect(manager._calculateETA(state, T0)).toBe(Infinity); + }); + + test('waits for three seconds of data before giving a figure', () => { + const manager = new ModelDownloadManager(); + const state = newState(); + manager._recordSpeedSample(state, T0); + + const at2s = feed(manager, state, T0, 2, 1000); + expect(manager._calculateSpeed(state, at2s)).toBe(0); + expect(manager._calculateETA(state, at2s)).toBe(Infinity); + + const at3s = feed(manager, state, at2s, 1, 1000); + expect(manager._calculateSpeed(state, at3s)).toBe(1000); + expect(manager._calculateETA(state, at3s)).toBe((10_000_000 - 3000) / 1000); + }); + + test('follows the current rate, not the average since the start', () => { + const manager = new ModelDownloadManager(); + const state = newState(); + manager._recordSpeedSample(state, T0); - const speed = manager._calculateSpeed(state); - const eta = manager._calculateETA(state); + const slowEnd = feed(manager, state, T0, 20, 1000); + expect(manager._calculateSpeed(state, slowEnd)).toBe(1000); - expect(speed).toBe(100); - expect(eta).toBe(8); + const fastEnd = feed(manager, state, slowEnd, 10, 100_000); + expect(manager._calculateSpeed(state, fastEnd)).toBe(100_000); + }); + + test('a resumed download does not count the bytes already on disk', () => { + const manager = new ModelDownloadManager(); + const state = newState(5_000_000); + state.downloadedBytes += 250; + manager._recordSpeedSample(state, T0); + + const at4s = feed(manager, state, T0, 4, 1000); + expect(manager._calculateSpeed(state, at4s)).toBe(1000); + }); + + test('keeps only the samples the window needs', () => { + const manager = new ModelDownloadManager(); + const state = newState(); + manager._recordSpeedSample(state, T0); + const end = feed(manager, state, T0, 60, 1000); + + expect(state.speedSamples.length).toBeLessThanOrEqual(42); + expect(end - state.speedSamples[0].time).toBe(10_000); + }); + + test('chunks that arrive within a quarter second share one sample', () => { + const manager = new ModelDownloadManager(); + const state = newState(); + manager._recordSpeedSample(state, T0); + manager._recordSpeedSample(state, T0 + 10); + manager._recordSpeedSample(state, T0 + 249); + + expect(state.speedSamples).toHaveLength(1); + }); }); test('deleteModel removes files', async () => { diff --git a/test/OrganizationSuggestionServiceCore.purposeFallback.test.js b/test/OrganizationSuggestionServiceCore.purposeFallback.test.js index cf6362a..8a971a5 100644 --- a/test/OrganizationSuggestionServiceCore.purposeFallback.test.js +++ b/test/OrganizationSuggestionServiceCore.purposeFallback.test.js @@ -295,4 +295,28 @@ describe('_buildLlmCategorySuggestion: no high-confidence lift for fallback anal expect(result.folder).toBe('Reports'); expect(result.score).toBeCloseTo(0.6, 5); }); + + // The matcher can reach a folder through its description or keywords. The + // lift is for a label that names the folder, and three letters apart is not + // a spelling of a three-letter name. + test('a short label that only resembles a short folder name keeps its own confidence', () => { + matchSpy.mockReturnValue('SEO'); + const folders = [...SMART_FOLDERS, { id: 'seo', name: 'SEO', path: '/docs/SEO' }]; + + const result = service._buildLlmCategorySuggestion( + { analysis: { category: 'Web', confidence: 60, extractionMethod: 'pdf-text' } }, + folders + ); + + expect(result.folder).toBe('SEO'); + expect(result.score).toBeCloseTo(0.6, 5); + }); + + test('a folder name one letter off is still lifted', () => { + const result = service._buildLlmCategorySuggestion( + { analysis: { category: 'Raports', confidence: 60, extractionMethod: 'pdf-text' } }, + SMART_FOLDERS + ); + expect(result.score).toBeCloseTo(0.95, 5); + }); }); diff --git a/test/components/AnalysisProgress.test.js b/test/components/AnalysisProgress.test.js index c5a7f0e..8fd7aca 100644 --- a/test/components/AnalysisProgress.test.js +++ b/test/components/AnalysisProgress.test.js @@ -5,11 +5,6 @@ import React from 'react'; import { render, screen } from '@testing-library/react'; import '@testing-library/jest-dom'; -jest.mock('../../src/renderer/components/ui', () => ({ - StatusBadge: ({ children }) => {children}, - Card: ({ children, ...props }) =>
{children}
-})); - jest.mock('../../src/renderer/components/ui/Typography', () => ({ Text: ({ children }) => {children} })); @@ -33,3 +28,23 @@ describe('AnalysisProgress live region (SW-79)', () => { expect(screen.getByRole('status')).toBeInTheDocument(); }); }); + +describe('AnalysisProgress wording', () => { + test('the percentage is labeled as progress and shown once', () => { + render(); + + expect(screen.getByText('Analyzing 26 of 39 files')).toBeInTheDocument(); + expect(screen.getByText('67% complete')).toBeInTheDocument(); + expect(screen.queryByText('67%')).not.toBeInTheDocument(); + expect(screen.getByRole('progressbar')).toHaveAttribute( + 'aria-valuetext', + '67 percent complete' + ); + }); + + test('one file is not "files"', () => { + render(); + + expect(screen.getByText('Analyzing 0 of 1 file')).toBeInTheDocument(); + }); +}); diff --git a/test/components/ModelSetupWizard.test.js b/test/components/ModelSetupWizard.test.js index d3fa8ed..ba1bb83 100644 --- a/test/components/ModelSetupWizard.test.js +++ b/test/components/ModelSetupWizard.test.js @@ -250,6 +250,74 @@ describe('ModelSetupWizard', () => { expect(screen.queryByText('The operation was aborted')).not.toBeInTheDocument(); }); + test('says the speed is being calculated until the download reports one', async () => { + const baseProfile = INSTALL_MODEL_PROFILES?.BASE_SMALL?.models || {}; + const embeddingModel = baseProfile.EMBEDDING; + const textModel = baseProfile.TEXT_ANALYSIS; + const visionModel = baseProfile.IMAGE_ANALYSIS; + + let progressListener; + window.electronAPI = { + llama: { + getConfig: jest.fn(async () => ({ embeddingModel, textModel, visionModel })), + getModels: jest.fn(async () => ({ models: [] })), + getDownloadStatus: jest.fn(async () => ({ status: { downloads: [] } })), + downloadModel: jest.fn(async () => ({ success: true, started: true })), + cancelDownload: jest.fn(async () => ({ success: true })) + }, + events: { + onOperationProgress: jest.fn((cb) => { + progressListener = cb; + return jest.fn(); + }) + } + }; + + render(); + + await waitFor(() => { + expect(screen.getByText(/Choose AI Models/i)).toBeInTheDocument(); + }); + fireEvent.click(screen.getByRole('button', { name: /Download Models/i })); + await waitFor(() => { + expect(screen.getByText(/Downloading Models/i)).toBeInTheDocument(); + }); + + // No bytes yet: every row is still starting. + expect(screen.queryByText('Calculating speed…')).not.toBeInTheDocument(); + expect(screen.getAllByText('Starting…').length).toBeGreaterThan(0); + + // Bytes are arriving, but the manager has no speed to report yet. + await act(async () => { + progressListener({ + type: 'model-download', + model: textModel, + percent: 1, + downloadedBytes: 5 * 1024 * 1024, + totalBytes: 500 * 1024 * 1024, + speedBps: 0, + etaSeconds: 0 + }); + }); + expect(screen.getByText('Calculating speed…')).toBeInTheDocument(); + expect(screen.queryByText(/^ETA /)).not.toBeInTheDocument(); + + await act(async () => { + progressListener({ + type: 'model-download', + model: textModel, + percent: 4, + downloadedBytes: 20 * 1024 * 1024, + totalBytes: 500 * 1024 * 1024, + speedBps: 4 * 1024 * 1024, + etaSeconds: 120 + }); + }); + expect(screen.queryByText('Calculating speed…')).not.toBeInTheDocument(); + expect(screen.getByText(/\/s$/)).toBeInTheDocument(); + expect(screen.getByText(/^ETA /)).toBeInTheDocument(); + }); + test('leaving the downloading step while a download runs dismisses instead of skipping', async () => { const baseProfile = INSTALL_MODEL_PROFILES?.BASE_SMALL?.models || {}; const embeddingModel = baseProfile.EMBEDDING; diff --git a/test/components/OrganizePhase.test.js b/test/components/OrganizePhase.test.js index a74fcd5..b471b42 100644 --- a/test/components/OrganizePhase.test.js +++ b/test/components/OrganizePhase.test.js @@ -2,7 +2,7 @@ * @jest-environment jsdom */ import React from 'react'; -import { render, screen, fireEvent } from '@testing-library/react'; +import { render, screen, fireEvent, act, waitFor } from '@testing-library/react'; import '@testing-library/jest-dom'; const mockAddNotification = jest.fn(); @@ -132,6 +132,11 @@ jest.mock('../../src/renderer/components/ui/Modal', () => ({ default: ({ isOpen, children }) => (isOpen ?
{children}
: null) })); +jest.mock('../../src/renderer/components/organize/OrganizePreviewModal', () => ({ + __esModule: true, + default: ({ isOpen, plan }) => (isOpen ?
PREVIEW_MODAL:{plan.wouldMove.length}
: null) +})); + jest.mock('../../src/renderer/components/AnalysisDetails', () => ({ __esModule: true, default: () =>
ANALYSIS_DETAILS
@@ -424,6 +429,104 @@ describe('OrganizePhase premium preflight copy', () => { }); }); +describe('OrganizePhase Preview button', () => { + const PLAN = { + success: true, + dryRun: true, + wouldMove: [{ source: '/tmp/invoice.pdf', destination: '/dest/finance/invoice.pdf' }], + skipped: [], + wouldCreateFolders: [], + sameDestination: [] + }; + + const renderWithPreview = (handlePreviewOrganize) => { + const { useOrganization } = require('../../src/renderer/phases/organize/useOrganization'); + useOrganization.mockReturnValue({ + isOrganizing: false, + batchProgress: null, + organizePreview: [], + handleOrganizeFiles: jest.fn(), + handlePreviewOrganize, + organizeConflicts: [] + }); + render(); + return screen.getByRole('button', { name: 'Preview' }); + }; + + afterEach(() => { + jest.clearAllMocks(); + }); + + test('opens the plan for every file in the queue', async () => { + const handlePreviewOrganize = jest.fn().mockResolvedValue(PLAN); + fireEvent.click(renderWithPreview(handlePreviewOrganize)); + + expect(await screen.findByText('PREVIEW_MODAL:1')).toBeInTheDocument(); + expect(handlePreviewOrganize.mock.calls[0][0].map((f) => f.path)).toEqual([ + '/tmp/invoice.pdf', + '/tmp/notes.txt' + ]); + expect(mockAddNotification).not.toHaveBeenCalled(); + }); + + // A large queue takes a moment to check on disk. Pressing again must not + // start a second request. + test('a second press while the plan is being built does not ask again', async () => { + let finish; + const handlePreviewOrganize = jest.fn( + () => + new Promise((resolve) => { + finish = resolve; + }) + ); + const preview = renderWithPreview(handlePreviewOrganize); + + fireEvent.click(preview); + fireEvent.click(preview); + expect(handlePreviewOrganize).toHaveBeenCalledTimes(1); + + await act(async () => finish(PLAN)); + expect(screen.getByText('PREVIEW_MODAL:1')).toBeInTheDocument(); + + // Once the first request has finished, the button works again. + fireEvent.click(preview); + expect(handlePreviewOrganize).toHaveBeenCalledTimes(2); + await act(async () => finish(PLAN)); + }); + + test('says why when the plan could not be built, and opens nothing', async () => { + const handlePreviewOrganize = jest + .fn() + .mockRejectedValue(new Error('Batch size exceeds maximum of 1000')); + fireEvent.click(renderWithPreview(handlePreviewOrganize)); + + await waitFor(() => + expect(mockAddNotification).toHaveBeenCalledWith( + 'The preview could not be built: Batch size exceeds maximum of 1000', + 'error', + 8000, + 'organize-preview' + ) + ); + expect(screen.queryByText(/PREVIEW_MODAL/)).not.toBeInTheDocument(); + }); + + test('says so when no file has a destination to preview', async () => { + const handlePreviewOrganize = jest.fn().mockResolvedValue(null); + fireEvent.click(renderWithPreview(handlePreviewOrganize)); + + await waitFor(() => + expect(mockAddNotification).toHaveBeenCalledWith( + 'None of these files has a destination yet, so there is nothing to preview.', + 'info', + 4000, + 'organize-preview' + ) + ); + expect(screen.queryByText(/PREVIEW_MODAL/)).not.toBeInTheDocument(); + }); +}); + describe('OrganizePhase and license state (CP-19)', () => { // There is no trial gate. A snapshot left over from a trial build must not // disable the move CTA. diff --git a/test/components/OrganizePreviewModal.test.js b/test/components/OrganizePreviewModal.test.js index 27b7b58..384b458 100644 --- a/test/components/OrganizePreviewModal.test.js +++ b/test/components/OrganizePreviewModal.test.js @@ -130,6 +130,82 @@ describe('OrganizePreviewModal', () => { }); }); + // Two files in one batch that end up with the same name in the same folder: + // the move refuses to start, so the preview says so instead of offering it. + describe('files this batch sends to one name', () => { + const clashPlan = { + wouldMove: [ + { + source: 'C:\\Downloads\\scan 1.pdf', + destination: 'C:\\Docs\\Finance\\invoice.pdf', + collision: false + }, + { + source: 'C:\\Downloads\\scan 2.pdf', + destination: 'C:\\Docs\\Finance\\invoice-1.pdf', + collision: true + }, + { + source: 'C:\\Downloads\\budget.xlsx', + destination: 'C:\\Docs\\Finance\\budget.xlsx', + collision: false + } + ], + skipped: [], + wouldCreateFolders: [], + conflicts: 1, + sameDestination: [ + { + destination: 'C:\\Docs\\Finance\\invoice.pdf', + files: [ + { fileName: 'invoice.pdf', sourcePath: 'C:\\Downloads\\scan 1.pdf' }, + { fileName: 'invoice.pdf', sourcePath: 'C:\\Downloads\\scan 2.pdf' } + ] + } + ] + }; + + test('are marked, with a notice that the move will not start', () => { + renderModal({ plan: clashPlan }); + + expect(screen.getByRole('alert')).toHaveTextContent( + '2 files would get the same name in the same folder, so the move will not start.' + ); + expect(screen.getAllByText('Same name as another file')).toHaveLength(2); + }); + + test('the move button is off until they are renamed', () => { + const { onConfirm } = renderModal({ plan: clashPlan }); + + const move = screen.getByRole('button', { name: /Move 3 files/ }); + expect(move).toBeDisabled(); + fireEvent.click(move); + expect(onConfirm).not.toHaveBeenCalled(); + }); + + test('a plan without any shows no notice', () => { + renderModal({ plan: { ...clashPlan, sameDestination: [] } }); + + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + expect(screen.queryByText('Same name as another file')).not.toBeInTheDocument(); + expect(screen.getByRole('button', { name: /Move 3 files/ })).toBeEnabled(); + }); + }); + + test('says that no file will move when every file is skipped', () => { + renderModal({ + plan: { + wouldMove: [], + skipped: [{ source: 'C:\\Downloads\\gone.pdf', reason: 'source-missing' }], + wouldCreateFolders: [], + conflicts: 0 + } + }); + + expect(screen.getByText('None of these files will move.')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /Move 0 files/ })).toBeDisabled(); + }); + test('the move button commits and reports the count', () => { const { onConfirm } = renderModal(); diff --git a/test/strategies.test.js b/test/strategies.test.js index 5e90ce3..b33fc09 100644 --- a/test/strategies.test.js +++ b/test/strategies.test.js @@ -223,6 +223,32 @@ describe('Organization Strategies', () => { expect(result.path).toBeDefined(); expect(result.name).toBeDefined(); }); + + test('reports which part of the file named the folder', () => { + const file = { + name: 'notes.md', + extension: 'md', + analysis: { category: 'SEO', purpose: 'Keyword research' } + }; + + expect(mapFileToStrategy(file, strategies['date-based'], [])).toMatchObject({ + name: 'SEO', + namedBy: 'category' + }); + expect(mapFileToStrategy(file, strategies['project-based'], []).namedBy).toBe('fileType'); + expect(mapFileToStrategy(file, strategies.hierarchical, [])).toMatchObject({ + name: 'Keyword research', + namedBy: 'purpose' + }); + }); + + test('names no part when the folder is a placeholder default', () => { + const file = { name: 'notes.md', extension: 'md', analysis: {} }; + + expect(mapFileToStrategy(file, strategies['date-based'], []).namedBy).toBeNull(); + expect(mapFileToStrategy(file, strategies.hierarchical, []).namedBy).toBeNull(); + expect(mapFileToStrategy(file, strategies['type-based'], []).namedBy).toBeNull(); + }); }); describe('getStrategyBasedSuggestions', () => { @@ -252,6 +278,24 @@ describe('Organization Strategies', () => { expect(suggestions).toHaveLength(0); }); + test('carries what named the folder alongside the template name', () => { + const file = { + name: 'notes.md', + extension: 'md', + analysis: { date: '2026-01-01', category: 'SEO' } + }; + + const dateBased = getStrategyBasedSuggestions(file, [], 0.3).find( + (s) => s.strategy === 'date-based' + ); + + expect(dateBased).toMatchObject({ + folder: 'SEO', + strategyName: 'Date-Based', + namedBy: 'category' + }); + }); + test('sorts by score', () => { const file = { name: 'project_report.pdf', diff --git a/test/suggestionRanker.test.js b/test/suggestionRanker.test.js index ff0927c..0e7a839 100644 --- a/test/suggestionRanker.test.js +++ b/test/suggestionRanker.test.js @@ -65,6 +65,49 @@ describe('Suggestion Ranker', () => { expect(result[0].confidence).toBe(0.7); }); + test('a duplicate that takes over the source brings what its explanation is written from', () => { + const result = rankSuggestions([ + { folder: 'SEO', score: 0.4, confidence: 0.4, source: 'semantic', method: 'semantic' }, + { + folder: 'seo', + score: 0.6, + confidence: 0.6, + source: 'strategy', + method: 'strategy_based', + strategyName: 'Date-Based', + namedBy: 'category' + } + ]); + + expect(result).toHaveLength(1); + expect(result[0]).toMatchObject({ + folder: 'SEO', + source: 'strategy', + strategyName: 'Date-Based', + namedBy: 'category' + }); + }); + + test('a weaker duplicate leaves the source and its explanation fields alone', () => { + const result = rankSuggestions([ + { + folder: 'SEO', + score: 0.6, + confidence: 0.6, + source: 'strategy', + strategyName: 'Date-Based', + namedBy: 'category' + }, + { folder: 'seo', score: 0.4, confidence: 0.4, source: 'semantic' } + ]); + + expect(result[0]).toMatchObject({ + source: 'strategy', + strategyName: 'Date-Based', + namedBy: 'category' + }); + }); + test('sorts by weighted score', () => { const suggestions = [ { folder: 'Low', score: 0.5, confidence: 0.5, source: 'strategy' }, @@ -217,6 +260,22 @@ describe('Suggestion Ranker', () => { expect(explanation).toContain('Project-Based'); }); + test('explains a strategy suggestion by what named its folder, not by the template title', () => { + const file = { extension: '.md' }; + const strategy = { source: 'strategy', strategyName: 'Date-Based', folder: 'SEO' }; + + const byCategory = generateExplanation({ ...strategy, namedBy: 'category' }, file); + expect(byCategory).toBe('The analysis labeled this file "SEO"'); + expect(byCategory).not.toContain('Date-Based'); + + expect( + generateExplanation({ ...strategy, folder: 'Documents', namedBy: 'fileType' }, file) + ).toBe('"Documents" matches this file\'s type'); + expect( + generateExplanation({ ...strategy, folder: 'Keyword research', namedBy: 'purpose' }, file) + ).toBe('The analysis described this file as "Keyword research"'); + }); + test('generates llm explanation', () => { const explanation = generateExplanation({ source: 'llm' }, { extension: '.pdf' }); expect(explanation).toContain('content and purpose'); diff --git a/test/useAnalysis.test.js b/test/useAnalysis.test.js index 93957a7..3084dd0 100644 --- a/test/useAnalysis.test.js +++ b/test/useAnalysis.test.js @@ -1904,11 +1904,11 @@ describe('useAnalysis', () => { jest.advanceTimersByTime(100); }); - // Should show resume notification + // Nobody pressed a button for this run, so the notice says why it runs. expect(mockAddNotification).toHaveBeenCalledWith( - expect.stringContaining('Resuming analysis'), + 'Continuing the analysis that was interrupted: 1 file left', 'info', - 3000, + 6000, 'analysis-resume' ); }); diff --git a/test/useOrganization.preview.test.js b/test/useOrganization.preview.test.js new file mode 100644 index 0000000..8a78041 --- /dev/null +++ b/test/useOrganization.preview.test.js @@ -0,0 +1,171 @@ +/** + * The Organize "Preview" asks the main process for a dry-run plan of the + * same operations a Move would send. `handlePreviewOrganize` returns that + * plan, adds the files this batch sends to one name (the Move refuses to + * start while any exist), and throws when the request fails or is refused, + * so the screen can say why instead of showing an empty preview. + */ + +import { renderHook } from '@testing-library/react'; + +jest.mock('../src/shared/logger', () => { + const logger = { + setContext: jest.fn(), + info: jest.fn(), + debug: jest.fn(), + warn: jest.fn(), + error: jest.fn() + }; + return { logger, createLogger: jest.fn(() => logger) }; +}); + +jest.mock('../src/shared/constants', () => ({ + PHASES: { DISCOVER: 'discover', ORGANIZE: 'organize', COMPLETE: 'complete' }, + IPC_CHANNELS: { + FILES: { PERFORM_OPERATION: 'files:perform-operation' } + } +})); + +jest.mock('../src/renderer/components/UndoRedoSystem', () => ({ + createOrganizeBatchAction: jest.fn(), + previewBatchOrganize: jest.fn() +})); + +const { previewBatchOrganize } = require('../src/renderer/components/UndoRedoSystem'); +const { useOrganization } = require('../src/renderer/phases/organize/useOrganization'); + +const FINANCE = { id: 'fin', name: 'Finance', path: '/Users/hans/Sorted/Finance' }; + +const file = (path, category, suggestedName) => { + const name = path.split(/[\\/]/).pop(); + return { path, name, analysis: { category, suggestedName: suggestedName || name } }; +}; + +const hookOptions = (unprocessedFiles) => ({ + unprocessedFiles, + editingFiles: {}, + getFileWithEdits: (f) => f, + findSmartFolderForCategory: (label) => + String(label).toLowerCase() === FINANCE.name.toLowerCase() ? FINANCE : null, + defaultLocation: '/Users/hans/Documents', + smartFolders: [FINANCE], + analysisResults: [], + markFilesAsProcessed: jest.fn(), + unmarkFilesAsProcessed: jest.fn(), + actions: { setPhaseData: jest.fn(), advancePhase: jest.fn() }, + phaseData: { organizedFiles: [] }, + addNotification: jest.fn(), + dispatch: jest.fn(), + executeAction: jest.fn(), + addOrganizedFiles: jest.fn(), + removeOrganizedFiles: jest.fn(), + setOrganizingState: jest.fn() +}); + +const dryRunPlan = (wouldMove) => ({ + success: true, + dryRun: true, + total: wouldMove.length, + wouldMove, + skipped: [], + wouldCreateFolders: [], + conflicts: 0 +}); + +describe('useOrganization: handlePreviewOrganize', () => { + beforeEach(() => { + previewBatchOrganize.mockReset(); + global.window = global.window || {}; + global.window.electronAPI = { + events: { onOperationProgress: jest.fn(() => jest.fn()) }, + files: { normalizePath: jest.fn((p) => p) } + }; + }); + + test('sends the operations a Move would send and returns the plan', async () => { + const invoice = file('/Users/hans/Downloads/scan.pdf', 'Finance', 'Invoice March.pdf'); + const moved = [ + { source: invoice.path, destination: '/Users/hans/Sorted/Finance/Invoice March.pdf' } + ]; + previewBatchOrganize.mockResolvedValue(dryRunPlan(moved)); + + const { result } = renderHook(() => useOrganization(hookOptions([invoice]))); + const plan = await result.current.handlePreviewOrganize(); + + expect(previewBatchOrganize).toHaveBeenCalledWith([ + expect.objectContaining({ + type: 'move', + source: invoice.path, + destination: '/Users/hans/Sorted/Finance/Invoice March.pdf' + }) + ]); + expect(plan.wouldMove).toEqual(moved); + expect(plan.sameDestination).toEqual([]); + }); + + test('lists the files this batch sends to one name', async () => { + const first = file('/Users/hans/Downloads/scan 1.pdf', 'Finance', 'Invoice.pdf'); + const second = file('/Users/hans/Downloads/scan 2.pdf', 'Finance', 'Invoice.pdf'); + const other = file('/Users/hans/Downloads/budget.xlsx', 'Finance'); + previewBatchOrganize.mockResolvedValue(dryRunPlan([])); + + const { result } = renderHook(() => useOrganization(hookOptions([first, second, other]))); + const plan = await result.current.handlePreviewOrganize(); + + expect(plan.sameDestination).toEqual([ + { + destination: '/Users/hans/Sorted/Finance/Invoice.pdf', + files: [ + { fileName: 'Invoice.pdf', sourcePath: first.path }, + { fileName: 'Invoice.pdf', sourcePath: second.path } + ] + } + ]); + }); + + test('previews only the files it is given', async () => { + const picked = file('/Users/hans/Downloads/a.pdf', 'Finance'); + const left = file('/Users/hans/Downloads/b.pdf', 'Finance'); + previewBatchOrganize.mockResolvedValue(dryRunPlan([])); + + const { result } = renderHook(() => useOrganization(hookOptions([picked, left]))); + await result.current.handlePreviewOrganize([picked]); + + expect(previewBatchOrganize.mock.calls[0][0].map((op) => op.source)).toEqual([picked.path]); + }); + + test('throws the reason when the main process refuses the batch', async () => { + previewBatchOrganize.mockResolvedValue({ + success: false, + error: 'Batch size exceeds maximum of 1000', + errorCode: 'BATCH_TOO_LARGE' + }); + + const { result } = renderHook(() => + useOrganization(hookOptions([file('/Users/hans/Downloads/a.pdf', 'Finance')])) + ); + + await expect(result.current.handlePreviewOrganize()).rejects.toThrow( + 'Batch size exceeds maximum of 1000' + ); + }); + + test('throws when the request itself fails', async () => { + previewBatchOrganize.mockRejectedValue(new Error('File operations API not available')); + + const { result } = renderHook(() => + useOrganization(hookOptions([file('/Users/hans/Downloads/a.pdf', 'Finance')])) + ); + + await expect(result.current.handlePreviewOrganize()).rejects.toThrow( + 'File operations API not available' + ); + }); + + test('returns null, without asking, when there is no file to preview', async () => { + const { result } = renderHook(() => useOrganization(hookOptions([]))); + + await expect(result.current.handlePreviewOrganize()).resolves.toBeNull(); + expect(previewBatchOrganize).not.toHaveBeenCalled(); + }); +});