From 3e56696c912bdbf393276ed144bc8fc17244c6e7 Mon Sep 17 00:00:00 2001 From: Anso Date: Sun, 24 May 2026 23:25:52 -0400 Subject: [PATCH] fix(stack-files): prompt before discarding unsaved edits on file switch (#1203) FileViewer tracks dirty state via content !== originalContent, but StackFileExplorer would swap selectedPath on every tree click and silently drop the buffered edit. A user editing a script and clicking a sibling file lost their work with no warning. FileViewer now exposes onDirtyChange so the explorer learns when the viewer is dirty. StackFileExplorer intercepts the tree-node click: if dirty, the next selection is stashed and a ConfirmModal asks whether to discard. Confirm applies the stash, Cancel keeps the current file. Clicking the already-selected file is a no-op (no spurious prompt). The dirty signal is reported via a ref so future consumers passing an inline callback identity each render do not retrigger the unmount cleanup effect. --- frontend/src/components/files/FileViewer.tsx | 23 +++- .../components/files/StackFileExplorer.tsx | 40 +++++- .../files/__tests__/FileViewer.test.tsx | 24 ++++ .../__tests__/StackFileExplorer.test.tsx | 126 ++++++++++++++++++ 4 files changed, 209 insertions(+), 4 deletions(-) create mode 100644 frontend/src/components/files/__tests__/StackFileExplorer.test.tsx diff --git a/frontend/src/components/files/FileViewer.tsx b/frontend/src/components/files/FileViewer.tsx index f1eb9742..399ef982 100644 --- a/frontend/src/components/files/FileViewer.tsx +++ b/frontend/src/components/files/FileViewer.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useMemo, Suspense } from 'react'; +import { useState, useEffect, useMemo, useRef, Suspense } from 'react'; import { Editor } from '@/lib/monacoLoader'; import { AlertCircle, FileIcon, Download, Loader2, Save } from 'lucide-react'; import { Button } from '@/components/ui/button'; @@ -14,6 +14,7 @@ interface FileViewerProps { canEdit: boolean; isDarkMode: boolean; onSaved?: () => void; + onDirtyChange?: (dirty: boolean) => void; } function getFilename(path: string): string { @@ -91,6 +92,7 @@ export function FileViewer({ canEdit, isDarkMode, onSaved, + onDirtyChange, }: FileViewerProps) { const [content, setContent] = useState(''); const [originalContent, setOriginalContent] = useState(''); @@ -102,6 +104,23 @@ export function FileViewer({ const [size, setSize] = useState(0); const readOnly = !canEdit; + const hasChanges = content !== originalContent; + + // Stash the latest callback in a ref so the unmount-cleanup effect can be + // truly unmount-scoped without re-running every time a parent passes a fresh + // function identity. + const onDirtyChangeRef = useRef(onDirtyChange); + useEffect(() => { + onDirtyChangeRef.current = onDirtyChange; + }, [onDirtyChange]); + + useEffect(() => { + onDirtyChangeRef.current?.(hasChanges); + }, [hasChanges]); + + useEffect(() => { + return () => onDirtyChangeRef.current?.(false); + }, []); const editorOptions = useMemo( () => ({ @@ -228,8 +247,6 @@ export function FileViewer({ ); } - const hasChanges = content !== originalContent; - return (
diff --git a/frontend/src/components/files/StackFileExplorer.tsx b/frontend/src/components/files/StackFileExplorer.tsx index dce8b45c..3205a84e 100644 --- a/frontend/src/components/files/StackFileExplorer.tsx +++ b/frontend/src/components/files/StackFileExplorer.tsx @@ -1,6 +1,7 @@ import { useState, useEffect, useCallback } from 'react'; import { Trash2, FolderPlus, Download, Loader2 } from 'lucide-react'; import { Button } from '@/components/ui/button'; +import { ConfirmModal } from '@/components/ui/modal'; import { toast } from '@/components/ui/toast-store'; import { downloadStackFile, listStackDirectory } from '@/lib/stackFilesApi'; import { FileTree } from './FileTree'; @@ -60,15 +61,21 @@ export function StackFileExplorer({ const [permissionsRelPath, setPermissionsRelPath] = useState(''); const [permissionsEntryName, setPermissionsEntryName] = useState(''); + // ── unsaved-changes guard on file switch ── + const [isViewerDirty, setIsViewerDirty] = useState(false); + const [pendingSelection, setPendingSelection] = useState<{ relPath: string; entry: FileEntry } | null>(null); + useEffect(() => { setSelectedPath(null); setSelectedEntry(null); setCurrentDir(''); + setIsViewerDirty(false); + setPendingSelection(null); }, [stackName]); const refresh = useCallback(() => setRefreshKey((k) => k + 1), []); - const handleSelectFile = useCallback((relPath: string, entry: FileEntry) => { + const applySelection = useCallback((relPath: string, entry: FileEntry) => { setSelectedPath(relPath); setSelectedEntry(entry); const parts = relPath.split('/'); @@ -76,6 +83,15 @@ export function StackFileExplorer({ setCurrentDir(parts.join('/')); }, []); + const handleSelectFile = useCallback((relPath: string, entry: FileEntry) => { + if (relPath === selectedPath) return; + if (isViewerDirty) { + setPendingSelection({ relPath, entry }); + return; + } + applySelection(relPath, entry); + }, [selectedPath, isViewerDirty, applySelection]); + const handleDeleted = useCallback(() => { setSelectedPath(null); setSelectedEntry(null); @@ -226,6 +242,7 @@ export function StackFileExplorer({ canEdit={canEdit} isDarkMode={isDarkMode} onSaved={refresh} + onDirtyChange={setIsViewerDirty} />
@@ -298,6 +315,27 @@ export function StackFileExplorer({ entryName={permissionsEntryName} canEdit={canEdit} /> + + {/* Unsaved-changes guard on file switch */} + { if (!next) setPendingSelection(null); }} + onCancel={() => setPendingSelection(null)} + kicker="FILES · UNSAVED CHANGES" + title="Discard unsaved changes?" + description="Switching files will discard the edits in the current viewer." + confirmLabel="Discard and switch" + onConfirm={() => { + if (pendingSelection) { + applySelection(pendingSelection.relPath, pendingSelection.entry); + setPendingSelection(null); + } + }} + > +

+ You have unsaved changes in the current file. Switching to another file will discard them. +

+ ); } diff --git a/frontend/src/components/files/__tests__/FileViewer.test.tsx b/frontend/src/components/files/__tests__/FileViewer.test.tsx index ef2a51d9..4c77116f 100644 --- a/frontend/src/components/files/__tests__/FileViewer.test.tsx +++ b/frontend/src/components/files/__tests__/FileViewer.test.tsx @@ -164,4 +164,28 @@ describe('FileViewer', () => { await waitFor(() => expect(mockReadFile).toHaveBeenCalledTimes(2)); expect(mockReadFile).toHaveBeenNthCalledWith(2, 'my-stack', 'b.txt'); }); + + it('reports clean dirty state on initial load of a text file', async () => { + mockReadFile.mockResolvedValue(textResult()); + const onDirtyChange = vi.fn(); + + render(); + + await waitFor(() => expect(screen.getByTestId('monaco-editor')).toBeInTheDocument()); + // content === originalContent immediately after load → dirty=false + expect(onDirtyChange).toHaveBeenLastCalledWith(false); + }); + + it('resets dirty signal on unmount', async () => { + mockReadFile.mockResolvedValue(textResult()); + const onDirtyChange = vi.fn(); + + const { unmount } = render(); + await waitFor(() => expect(screen.getByTestId('monaco-editor')).toBeInTheDocument()); + + onDirtyChange.mockClear(); + unmount(); + + expect(onDirtyChange).toHaveBeenCalledWith(false); + }); }); diff --git a/frontend/src/components/files/__tests__/StackFileExplorer.test.tsx b/frontend/src/components/files/__tests__/StackFileExplorer.test.tsx new file mode 100644 index 00000000..11cb6ef9 --- /dev/null +++ b/frontend/src/components/files/__tests__/StackFileExplorer.test.tsx @@ -0,0 +1,126 @@ +/** + * Coverage for StackFileExplorer's unsaved-changes interception. + * + * The viewer reports its dirty state up via onDirtyChange. When the user + * clicks a sibling file in the tree, the explorer must intercept the switch + * and show a confirm dialog if there are unsaved edits. The viewer mock + * exposes a "Mark dirty" button so the test can drive the dirty signal + * without instantiating Monaco. + */ +import { describe, it, expect, vi } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import type { FileEntry } from '@/lib/stackFilesApi'; + +vi.mock('@/lib/stackFilesApi', () => ({ + listStackDirectory: vi.fn().mockResolvedValue([]), + downloadStackFile: vi.fn(), + readStackFile: vi.fn(), + writeStackFile: vi.fn(), +})); + +vi.mock('@/components/ui/toast-store', () => ({ + toast: { error: vi.fn(), success: vi.fn(), loading: vi.fn(() => 'id'), dismiss: vi.fn() }, +})); + +vi.mock('../FileUploadDropzone', () => ({ + FileUploadDropzone: () =>
, +})); + +vi.mock('../NewFolderDialog', () => ({ NewFolderDialog: () => null })); +vi.mock('../NewFileDialog', () => ({ NewFileDialog: () => null })); +vi.mock('../DeleteFileConfirm', () => ({ DeleteFileConfirm: () => null })); +vi.mock('../RenameDialog', () => ({ RenameDialog: () => null })); +vi.mock('../FilePermissionsDialog', () => ({ FilePermissionsDialog: () => null })); + +// FileTree mock exposes two buttons that synthesise selection of two siblings. +vi.mock('../FileTree', () => ({ + FileTree: ({ onSelectFile }: { onSelectFile: (rel: string, entry: FileEntry) => void }) => ( +
+ + +
+ ), +})); + +// FileViewer mock exposes a button that flips its dirty signal. +vi.mock('../FileViewer', () => ({ + FileViewer: ({ selectedPath, onDirtyChange }: { + selectedPath: string | null; + onDirtyChange?: (dirty: boolean) => void; + }) => ( +
+
{selectedPath ?? '(none)'}
+ + +
+ ), +})); + +import { StackFileExplorer } from '../StackFileExplorer'; + +function setup() { + return render( + , + ); +} + +describe('StackFileExplorer unsaved-changes interception', () => { + it('switches files immediately when the viewer is clean', async () => { + const user = userEvent.setup(); + setup(); + + await user.click(screen.getByText('select-a')); + expect(screen.getByTestId('viewer-selected').textContent).toBe('a.txt'); + + await user.click(screen.getByText('select-b')); + expect(screen.getByTestId('viewer-selected').textContent).toBe('b.txt'); + expect(screen.queryByText(/discard unsaved changes/i)).not.toBeInTheDocument(); + }); + + it('intercepts the switch when the viewer is dirty and applies on confirm', async () => { + const user = userEvent.setup(); + setup(); + + await user.click(screen.getByText('select-a')); + await user.click(screen.getByText('mark-dirty')); + + // Switching siblings while dirty must NOT swap the selection immediately. + await user.click(screen.getByText('select-b')); + expect(screen.getByTestId('viewer-selected').textContent).toBe('a.txt'); + expect(screen.getByText(/discard unsaved changes/i)).toBeInTheDocument(); + + // Confirming the dialog applies the pending selection. + await user.click(screen.getByRole('button', { name: /discard and switch/i })); + expect(screen.getByTestId('viewer-selected').textContent).toBe('b.txt'); + }); + + it('intercepts the switch when dirty and preserves the original on cancel', async () => { + const user = userEvent.setup(); + setup(); + + await user.click(screen.getByText('select-a')); + await user.click(screen.getByText('mark-dirty')); + + await user.click(screen.getByText('select-b')); + expect(screen.getByText(/discard unsaved changes/i)).toBeInTheDocument(); + + await user.click(screen.getByRole('button', { name: /^cancel$/i })); + expect(screen.getByTestId('viewer-selected').textContent).toBe('a.txt'); + }); + + it('clicking the already-selected file does not prompt even when dirty', async () => { + const user = userEvent.setup(); + setup(); + + await user.click(screen.getByText('select-a')); + await user.click(screen.getByText('mark-dirty')); + + await user.click(screen.getByText('select-a')); + expect(screen.queryByText(/discard unsaved changes/i)).not.toBeInTheDocument(); + }); +});