From cbb34aeef595ea56fba8f0a1540908aada50a080 Mon Sep 17 00:00:00 2001 From: Hamdi LAADHARI Date: Wed, 2 Sep 2026 16:47:07 +0200 Subject: [PATCH] refactor(session): extract useFileSession, fixing in-place state mutation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit App.tsx owned SessionState (files/currentIndex/deletedFiles/keptFiles/ folderPath/undoStack) via useState, mutating arrays in place inside setState updaters (.push()/.pop() on the previous state's arrays after only a shallow copy) — a real correctness bug, not just a style issue. FileViewer.tsx co-owned the "perform an action" flow with no single place owning what happens on delete. Extract src/hooks/useFileSession.ts: a 5-method interface (startSession, keep, deleteFile, undo, reset) that owns the whole session and does immutable state transitions throughout. deleteFile is the one async method — it awaits moveToTrash and only commits the "deleted" transition on success, so a failed trash operation can no longer be recorded as if it succeeded. A failure propagates as a rejection for FileViewer to catch and show as an alert, instead of the previous fire-and-forget .catch(console.error). The hook also exposes a derived isComplete flag, so App.tsx no longer reaches into currentIndex/files.length itself to guess whether the session just finished; it reacts to isComplete via an effect instead. Adds src/hooks/useFileSession.test.ts, testing the hook's public interface via renderHook, including the core fix: state stays byte-for- byte unchanged when moveToTrash rejects. Co-Authored-By: Claude Sonnet 5 --- CONTRIBUTING.md | 2 +- src/App.tsx | 104 ++++---------------- src/components/FileViewer.tsx | 15 ++- src/hooks/useFileSession.test.ts | 164 +++++++++++++++++++++++++++++++ src/hooks/useFileSession.ts | 93 ++++++++++++++++++ 5 files changed, 290 insertions(+), 88 deletions(-) create mode 100644 src/hooks/useFileSession.test.ts create mode 100644 src/hooks/useFileSession.ts diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6fb5abb..d045be6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -70,7 +70,7 @@ npm test # once npm run test:watch # while developing ``` -Coverage is partial by design — the suite currently covers file-type classification, the formatting helpers, file operations, settings persistence, and the update notifier. Extending it is tracked in [#9](https://github.com/killerwolf/QuickToss/issues/9); the undo stack (`src/App.tsx`) is next. +Coverage is partial by design — the suite currently covers file-type classification, the formatting helpers, file operations, settings persistence, session state (`src/hooks/useFileSession.ts` — keep/delete/undo), and the update notifier. Extending it is tracked in [#9](https://github.com/killerwolf/QuickToss/issues/9). Logic worth testing should live outside `electron/main.ts`, which instantiates the app at import time and can't be loaded from a test. `electron/file-types.ts`, `electron/file-operations.ts`, and `electron/settings-store.ts` are the pattern to follow: pure functions (or a factory taking its dependencies as parameters) the main process calls, importable on their own. diff --git a/src/App.tsx b/src/App.tsx index 6045d25..5e50e3a 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -1,23 +1,25 @@ -import { useState } from "react"; +import { useEffect, useState } from "react"; import CompletionScreen from "./components/CompletionScreen"; import FileViewer from "./components/FileViewer"; import UpdateNotifier from "./components/UpdateNotifier"; import WelcomeScreen from "./components/WelcomeScreen"; -import type { SessionState } from "./types"; +import { useFileSession } from "./hooks/useFileSession"; import "./electron.d.ts"; type AppState = "welcome" | "viewing" | "completed"; function App() { const [appState, setAppState] = useState("welcome"); - const [sessionState, setSessionState] = useState({ - files: [], - currentIndex: 0, - deletedFiles: [], - keptFiles: [], - folderPath: "", - undoStack: [], - }); + const { sessionState, isComplete, startSession, keep, deleteFile, undo, reset } = + useFileSession(); + + // Moves to the completion screen once the session actually reports itself + // done, rather than the caller re-deriving that from currentIndex/files. + useEffect(() => { + if (isComplete && appState === "viewing") { + setAppState("completed"); + } + }, [isComplete, appState]); const handleFolderSelected = async (folderPath: string) => { try { @@ -28,14 +30,7 @@ function App() { return; } - setSessionState({ - files, - currentIndex: 0, - deletedFiles: [], - keptFiles: [], - folderPath, - undoStack: [], - }); + startSession(files, folderPath); setAppState("viewing"); } catch (error) { console.error("Error scanning folder:", error); @@ -43,66 +38,16 @@ function App() { } }; - const handleFileAction = (action: "delete" | "keep", fileIndex: number) => { - const file = sessionState.files[fileIndex]; - - setSessionState((prev) => { - const newState = { ...prev }; - - // Add to undo stack - newState.undoStack.push({ - action, - fileIndex, - file, - }); - - // Update appropriate array - if (action === "delete") { - newState.deletedFiles.push(file); - // Actually move file to trash - window.electronAPI.moveToTrash(file.path).catch(console.error); - } else { - newState.keptFiles.push(file); - } - - // Move to next file - newState.currentIndex = fileIndex + 1; - - return newState; - }); - - // Check if we've processed all files - if (fileIndex + 1 >= sessionState.files.length) { - setAppState("completed"); + const handleFileAction = async (action: "delete" | "keep") => { + if (action === "delete") { + await deleteFile(); + } else { + keep(); } }; - const handleUndo = async () => { - if (sessionState.undoStack.length === 0) return; - - const lastAction = sessionState.undoStack[sessionState.undoStack.length - 1]; - - setSessionState((prev) => { - const newState = { ...prev }; - - // Remove from undo stack - newState.undoStack.pop(); - - // Reverse the action - if (lastAction.action === "delete") { - newState.deletedFiles = newState.deletedFiles.filter( - (f) => f.path !== lastAction.file.path - ); - // Note: We can't restore from trash automatically, but we remove from deleted list - } else { - newState.keptFiles = newState.keptFiles.filter((f) => f.path !== lastAction.file.path); - } - - // Go back to previous file - newState.currentIndex = lastAction.fileIndex; - - return newState; - }); + const handleUndo = () => { + undo(); // If we undid from completion screen, go back to viewing if (appState === "completed") { @@ -111,14 +56,7 @@ function App() { }; const handleStartOver = () => { - setSessionState({ - files: [], - currentIndex: 0, - deletedFiles: [], - keptFiles: [], - folderPath: "", - undoStack: [], - }); + reset(); setAppState("welcome"); }; diff --git a/src/components/FileViewer.tsx b/src/components/FileViewer.tsx index b4a7346..f18d38f 100644 --- a/src/components/FileViewer.tsx +++ b/src/components/FileViewer.tsx @@ -7,7 +7,7 @@ import SettingsComponent from "./Settings"; interface FileViewerProps { sessionState: SessionState; - onFileAction: (action: "delete" | "keep", fileIndex: number) => void; + onFileAction: (action: "delete" | "keep") => Promise; onUndo: () => void; onBack: () => void; } @@ -172,12 +172,19 @@ const FileViewer: React.FC = ({ sessionState, onFileAction, onU playActionSound(action); // Trigger action after brief delay for feedback - setTimeout(() => { - onFileAction(action, currentIndex); + setTimeout(async () => { + try { + await onFileAction(action); + } catch (error) { + console.error(`Error performing ${action}:`, error); + window.alert( + `Couldn't ${action === "delete" ? "delete" : "keep"} "${currentFile.name}". Please try again.` + ); + } setActionFeedback({ show: false, type: action }); }, 200); }, - [onFileAction, currentIndex, playActionSound, settings.confirmDelete, currentFile.name] + [onFileAction, playActionSound, settings.confirmDelete, currentFile.name] ); // Keyboard shortcuts diff --git a/src/hooks/useFileSession.test.ts b/src/hooks/useFileSession.test.ts new file mode 100644 index 0000000..2dab743 --- /dev/null +++ b/src/hooks/useFileSession.test.ts @@ -0,0 +1,164 @@ +import { act, renderHook } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { FileItem } from "../types"; +import { useFileSession } from "./useFileSession"; + +const file = (name: string): FileItem => ({ + name, + path: `/folder/${name}`, + size: 100, + modified: new Date("2024-01-01"), + extension: ".txt", + type: "document", +}); + +let moveToTrash: ReturnType; + +beforeEach(() => { + moveToTrash = vi.fn().mockResolvedValue(true); + window.electronAPI = { moveToTrash } as unknown as typeof window.electronAPI; +}); + +describe("isComplete", () => { + it("is false for a fresh, never-started session", () => { + const { result } = renderHook(() => useFileSession()); + expect(result.current.isComplete).toBe(false); + }); + + it("is false until the last file is processed, then true", () => { + const { result } = renderHook(() => useFileSession()); + act(() => result.current.startSession([file("a.txt"), file("b.txt")], "/folder")); + expect(result.current.isComplete).toBe(false); + + act(() => result.current.keep()); + expect(result.current.isComplete).toBe(false); + + act(() => result.current.keep()); + expect(result.current.isComplete).toBe(true); + }); + + it("goes back to false after undoing the last file", () => { + const { result } = renderHook(() => useFileSession()); + act(() => result.current.startSession([file("a.txt")], "/folder")); + act(() => result.current.keep()); + expect(result.current.isComplete).toBe(true); + + act(() => result.current.undo()); + expect(result.current.isComplete).toBe(false); + }); +}); + +describe("startSession", () => { + it("sets up a fresh session at index 0", () => { + const { result } = renderHook(() => useFileSession()); + + act(() => { + result.current.startSession([file("a.txt"), file("b.txt")], "/folder"); + }); + + expect(result.current.sessionState.files).toHaveLength(2); + expect(result.current.sessionState.currentIndex).toBe(0); + expect(result.current.sessionState.folderPath).toBe("/folder"); + expect(result.current.sessionState.deletedFiles).toEqual([]); + expect(result.current.sessionState.keptFiles).toEqual([]); + expect(result.current.sessionState.undoStack).toEqual([]); + }); +}); + +describe("keep", () => { + it("moves the current file to keptFiles, advances the index, and records an undo entry", () => { + const { result } = renderHook(() => useFileSession()); + act(() => result.current.startSession([file("a.txt"), file("b.txt")], "/folder")); + + act(() => result.current.keep()); + + expect(result.current.sessionState.keptFiles.map((f) => f.name)).toEqual(["a.txt"]); + expect(result.current.sessionState.currentIndex).toBe(1); + expect(result.current.sessionState.undoStack).toEqual([ + { action: "keep", fileIndex: 0, file: file("a.txt") }, + ]); + }); +}); + +describe("deleteFile", () => { + it("moves the file to trash, then commits it to deletedFiles and advances the index", async () => { + const { result } = renderHook(() => useFileSession()); + act(() => result.current.startSession([file("a.txt"), file("b.txt")], "/folder")); + + await act(async () => { + await result.current.deleteFile(); + }); + + expect(moveToTrash).toHaveBeenCalledWith("/folder/a.txt"); + expect(result.current.sessionState.deletedFiles.map((f) => f.name)).toEqual(["a.txt"]); + expect(result.current.sessionState.currentIndex).toBe(1); + expect(result.current.sessionState.undoStack).toEqual([ + { action: "delete", fileIndex: 0, file: file("a.txt") }, + ]); + }); + + it("does not commit the deletion when moveToTrash fails", async () => { + moveToTrash.mockRejectedValue(new Error("permission denied")); + const { result } = renderHook(() => useFileSession()); + act(() => result.current.startSession([file("a.txt")], "/folder")); + + await act(async () => { + await expect(result.current.deleteFile()).rejects.toThrow("permission denied"); + }); + + expect(result.current.sessionState.deletedFiles).toEqual([]); + expect(result.current.sessionState.currentIndex).toBe(0); + expect(result.current.sessionState.undoStack).toEqual([]); + }); +}); + +describe("undo", () => { + it("reverses a keep", () => { + const { result } = renderHook(() => useFileSession()); + act(() => result.current.startSession([file("a.txt")], "/folder")); + act(() => result.current.keep()); + + act(() => result.current.undo()); + + expect(result.current.sessionState.keptFiles).toEqual([]); + expect(result.current.sessionState.currentIndex).toBe(0); + expect(result.current.sessionState.undoStack).toEqual([]); + }); + + it("reverses a delete's bookkeeping without restoring the file from trash", async () => { + const { result } = renderHook(() => useFileSession()); + act(() => result.current.startSession([file("a.txt")], "/folder")); + await act(async () => { + await result.current.deleteFile(); + }); + + act(() => result.current.undo()); + + expect(result.current.sessionState.deletedFiles).toEqual([]); + expect(result.current.sessionState.currentIndex).toBe(0); + expect(result.current.sessionState.undoStack).toEqual([]); + }); + + it("is a no-op when there's nothing to undo", () => { + const { result } = renderHook(() => useFileSession()); + act(() => result.current.startSession([file("a.txt")], "/folder")); + + act(() => result.current.undo()); + + expect(result.current.sessionState.currentIndex).toBe(0); + }); +}); + +describe("reset", () => { + it("clears the session back to empty", () => { + const { result } = renderHook(() => useFileSession()); + act(() => result.current.startSession([file("a.txt")], "/folder")); + act(() => result.current.keep()); + + act(() => result.current.reset()); + + expect(result.current.sessionState.files).toEqual([]); + expect(result.current.sessionState.folderPath).toBe(""); + expect(result.current.sessionState.keptFiles).toEqual([]); + }); +}); diff --git a/src/hooks/useFileSession.ts b/src/hooks/useFileSession.ts new file mode 100644 index 0000000..c240145 --- /dev/null +++ b/src/hooks/useFileSession.ts @@ -0,0 +1,93 @@ +import { useCallback, useState } from "react"; +import type { FileItem, SessionState, UndoAction } from "../types"; + +const EMPTY_SESSION: SessionState = { + files: [], + currentIndex: 0, + deletedFiles: [], + keptFiles: [], + folderPath: "", + undoStack: [], +}; + +function applyKeep(prev: SessionState): SessionState { + const file = prev.files[prev.currentIndex]; + if (!file) return prev; + + const action: UndoAction = { action: "keep", fileIndex: prev.currentIndex, file }; + return { + ...prev, + keptFiles: [...prev.keptFiles, file], + undoStack: [...prev.undoStack, action], + currentIndex: prev.currentIndex + 1, + }; +} + +function applyDelete(prev: SessionState, fileIndex: number, file: FileItem): SessionState { + const action: UndoAction = { action: "delete", fileIndex, file }; + return { + ...prev, + deletedFiles: [...prev.deletedFiles, file], + undoStack: [...prev.undoStack, action], + currentIndex: fileIndex + 1, + }; +} + +function applyUndo(prev: SessionState): SessionState { + if (prev.undoStack.length === 0) return prev; + + const lastAction = prev.undoStack[prev.undoStack.length - 1]; + // Undoing a delete doesn't restore the file from trash — it can't be + // automated — it only removes it from the deleted list so it's tracked as + // "not yet decided" again. + const field = lastAction.action === "delete" ? "deletedFiles" : "keptFiles"; + + return { + ...prev, + undoStack: prev.undoStack.slice(0, -1), + [field]: prev[field].filter((f) => f.path !== lastAction.file.path), + currentIndex: lastAction.fileIndex, + }; +} + +// Owns the whole triage session: which files are left, which were kept or +// deleted, and the undo stack. deleteFile is the one async method — it awaits +// moveToTrash and only commits the "deleted" transition on success, so a +// failed trash operation can never be recorded as if it succeeded. Callers +// (FileViewer) are expected to catch a rejection and surface it. +export function useFileSession() { + const [sessionState, setSessionState] = useState(EMPTY_SESSION); + + const startSession = useCallback((files: FileItem[], folderPath: string) => { + setSessionState({ ...EMPTY_SESSION, files, folderPath }); + }, []); + + const reset = useCallback(() => { + setSessionState(EMPTY_SESSION); + }, []); + + const keep = useCallback(() => { + setSessionState(applyKeep); + }, []); + + const deleteFile = useCallback(async () => { + const { currentIndex, files } = sessionState; + const file = files[currentIndex]; + if (!file) return; + + await window.electronAPI.moveToTrash(file.path); + + setSessionState((prev) => applyDelete(prev, currentIndex, file)); + }, [sessionState]); + + const undo = useCallback(() => { + setSessionState(applyUndo); + }, []); + + // A fresh (never-started) session isn't "complete" just because + // currentIndex (0) already meets files.length (0). + const isComplete = + sessionState.files.length > 0 && sessionState.currentIndex >= sessionState.files.length; + + return { sessionState, isComplete, startSession, keep, deleteFile, undo, reset }; +}