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] 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(); + }); + }); +});