diff --git a/app/core/src/main/java/stirling/software/SPDF/exception/GlobalExceptionHandler.java b/app/core/src/main/java/stirling/software/SPDF/exception/GlobalExceptionHandler.java index 6b43b66ff..504dd36f5 100644 --- a/app/core/src/main/java/stirling/software/SPDF/exception/GlobalExceptionHandler.java +++ b/app/core/src/main/java/stirling/software/SPDF/exception/GlobalExceptionHandler.java @@ -24,6 +24,7 @@ import org.springframework.web.multipart.MaxUploadSizeExceededException; import org.springframework.web.multipart.support.MissingServletRequestPartException; import org.springframework.web.server.ResponseStatusException; import org.springframework.web.servlet.NoHandlerFoundException; +import org.springframework.web.servlet.resource.NoResourceFoundException; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; @@ -993,6 +994,49 @@ public class GlobalExceptionHandler { .body(problemDetail); } + /** Unmapped path → clean 404 instead of falling through to the generic 500 catch-all. */ + @ExceptionHandler(NoResourceFoundException.class) + public ResponseEntity handleNoResourceFound( + NoResourceFoundException ex, HttpServletRequest request) { + // /api/* miss = likely missing controller (operator-relevant); other paths = favicons, + // robots.txt, scanner noise. Demote the latter so prod logs aren't flooded. + String uri = request.getRequestURI(); + if (uri != null && uri.startsWith("/api/")) { + log.warn("No resource at {}: {}", uri, ex.getMessage()); + } else { + log.debug("No resource at {}: {}", uri, ex.getMessage()); + } + + String title = getLocalizedMessage("error.notFound.title", ErrorTitles.NOT_FOUND_DEFAULT); + String detail = + getLocalizedMessage( + "error.notFound.detail", + String.format( + "No endpoint found for %s %s", + request.getMethod(), request.getRequestURI()), + request.getMethod(), + request.getRequestURI()); + + ProblemDetail problemDetail = + createBaseProblemDetail(HttpStatus.NOT_FOUND, detail, request); + problemDetail.setType(URI.create(ErrorTypes.NOT_FOUND)); + problemDetail.setTitle(title); + problemDetail.setProperty("title", title); + problemDetail.setProperty("method", request.getMethod()); + addStandardHints( + problemDetail, + "error.notFound.hints", + List.of( + "Verify the URL path and HTTP method are correct.", + "Check the API base path and version if applicable.", + "Ensure there are no typos in the endpoint path.")); + problemDetail.setProperty("actionRequired", "Use a valid endpoint URL and method."); + + return ResponseEntity.status(HttpStatus.NOT_FOUND) + .contentType(PROBLEM_JSON) + .body(problemDetail); + } + /** * Handle IllegalArgumentException. * diff --git a/app/core/src/test/java/stirling/software/SPDF/exception/GlobalExceptionHandlerTest.java b/app/core/src/test/java/stirling/software/SPDF/exception/GlobalExceptionHandlerTest.java index 8e2275a72..4de6398e4 100644 --- a/app/core/src/test/java/stirling/software/SPDF/exception/GlobalExceptionHandlerTest.java +++ b/app/core/src/test/java/stirling/software/SPDF/exception/GlobalExceptionHandlerTest.java @@ -18,6 +18,7 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.context.MessageSource; import org.springframework.core.env.Environment; +import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; import org.springframework.http.ProblemDetail; import org.springframework.http.ResponseEntity; @@ -27,6 +28,7 @@ import org.springframework.web.bind.MissingServletRequestParameterException; import org.springframework.web.multipart.MaxUploadSizeExceededException; import org.springframework.web.multipart.support.MissingServletRequestPartException; import org.springframework.web.servlet.NoHandlerFoundException; +import org.springframework.web.servlet.resource.NoResourceFoundException; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; @@ -208,6 +210,19 @@ class GlobalExceptionHandlerTest { assertEquals(HttpStatus.NOT_FOUND, resp.getStatusCode()); } + // ---- NoResourceFoundException ---- + // Regression guard: was falling through to the 500 catch-all. + + @Test + void handleNoResourceFound_returns_404_not_500() { + when(request.getMethod()).thenReturn("GET"); + NoResourceFoundException ex = + new NoResourceFoundException(HttpMethod.GET, "/api/v1/storage/folders", ""); + ResponseEntity resp = handler.handleNoResourceFound(ex, request); + assertEquals(HttpStatus.NOT_FOUND, resp.getStatusCode()); + assertEquals("GET", resp.getBody().getProperties().get("method")); + } + // ---- IllegalArgumentException ---- @Test diff --git a/frontend/editor/src/core/contexts/FolderContext.test.tsx b/frontend/editor/src/core/contexts/FolderContext.test.tsx index 861ddd331..46de8f6d7 100644 --- a/frontend/editor/src/core/contexts/FolderContext.test.tsx +++ b/frontend/editor/src/core/contexts/FolderContext.test.tsx @@ -3,6 +3,7 @@ import { describe, expect, test, vi, beforeEach } from "vitest"; import { render, screen, waitFor, act } from "@testing-library/react"; import { FolderProvider, useFolders } from "@app/contexts/FolderContext"; +import { createFolderId, FolderId, FolderRecord } from "@app/types/folder"; import { expectConsole } from "@app/tests/failOnConsole"; /** @@ -26,21 +27,43 @@ import { expectConsole } from "@app/tests/failOnConsole"; // require a running backend (folderSyncService) and a populated IDB // (folderStorage). Both are out of scope for testing the error gate. const mockList = vi.fn(); +const mockUpdate = vi.fn(); +const mockDelete = vi.fn(); vi.mock("@app/services/folderSyncService", () => ({ folderSyncService: { list: () => mockList(), create: vi.fn(), - update: vi.fn(), - delete: vi.fn(), + update: (...args: unknown[]) => mockUpdate(...args), + delete: (...args: unknown[]) => mockDelete(...args), }, })); +// Stateful IDB mock - the revision-driven refresh re-reads getAllFolders +// after every state change, so a stateless [] mock would clobber pull results. +const { mockIdb } = vi.hoisted(() => ({ + mockIdb: { folders: [] as { id: string }[] }, +})); vi.mock("@app/services/folderStorage", () => ({ folderStorage: { - getAllFolders: vi.fn().mockResolvedValue([]), - replaceAll: vi.fn().mockResolvedValue(undefined), - upsert: vi.fn().mockResolvedValue(undefined), + getAllFolders: vi.fn(() => Promise.resolve([...mockIdb.folders])), + replaceAll: vi.fn((next: { id: string }[]) => { + mockIdb.folders = [...next]; + return Promise.resolve(); + }), + upsert: vi.fn(), + upsertFolder: vi.fn((next: { id: string }) => { + mockIdb.folders = [ + ...mockIdb.folders.filter((f) => f.id !== next.id), + next, + ]; + return Promise.resolve(); + }), delete: vi.fn().mockResolvedValue(undefined), + removeFolders: vi.fn((ids: string[]) => { + const drop = new Set(ids); + mockIdb.folders = mockIdb.folders.filter((f) => !drop.has(f.id)); + return Promise.resolve(); + }), }, })); @@ -105,6 +128,8 @@ async function renderAndWaitForPull(): Promise { describe("FolderContext sync-banner gating", () => { beforeEach(() => { mockList.mockReset(); + mockUpdate.mockReset(); + mockDelete.mockReset(); }); test("401 (unauthorized) does NOT surface a banner", async () => { @@ -157,3 +182,163 @@ describe("FolderContext sync-banner gating", () => { expect(screen.getByTestId("reachable").textContent).toBe("false"); }); }); + +/** Per-folder mutation 404 = silent cleanup (drop subtree, no banner, pull). */ +describe("FolderContext stale-folder 404 cleanup", () => { + beforeEach(() => { + mockList.mockReset(); + mockUpdate.mockReset(); + mockDelete.mockReset(); + // Reset the stateful IDB mock so each test starts with an empty cache. + mockIdb.folders = []; + }); + + function makeFolder(name: string, parentFolderId: FolderId | null = null) { + return { + id: createFolderId(), + name, + parentFolderId, + createdAt: 0, + updatedAt: 0, + } as FolderRecord; + } + + type ProbeApi = { + error: string | null; + folderCount: number; + currentFolderId: FolderId | null; + setCurrentFolderId: (id: FolderId | null) => void; + rename: (id: FolderId, name: string) => Promise; + delete: (id: FolderId) => Promise; + }; + + function ApiProbe(props: { onReady: (api: ProbeApi) => void }) { + const f = useFolders(); + React.useEffect(() => { + props.onReady({ + error: f.error, + folderCount: f.folders.length, + currentFolderId: f.currentFolderId, + setCurrentFolderId: f.setCurrentFolderId, + rename: (id, name) => f.renameFolder(id, name), + delete: (id) => f.deleteFolder(id), + }); + }, [f, props]); + return ( + <> +
{f.error ?? ""}
+
{String(f.serverReachable)}
+
{f.folders.length}
+
{f.currentFolderId ?? ""}
+ + ); + } + + async function setupWithFolders( + initial: FolderRecord[], + ): Promise<{ current: ProbeApi }> { + // Ref (not plain object) so tests see the latest f-bound api after re-renders. + mockList.mockResolvedValueOnce(initial); + const apiRef: { current: ProbeApi | null } = { current: null }; + render( + + (apiRef.current = api)} /> + , + ); + await waitFor(() => + expect(screen.getByTestId("count").textContent).toBe( + String(initial.length), + ), + ); + if (!apiRef.current) throw new Error("ApiProbe never reported ready"); + return apiRef as { current: ProbeApi }; + } + + test("renameFolder 404 silently drops folder + descendants, no banner", async () => { + const parent = makeFolder("parent"); + const child = makeFolder("child", parent.id); + const sibling = makeFolder("sibling"); + const api = await setupWithFolders([parent, child, sibling]); + + // Convergence pull returns just sibling after the local drop. + mockList.mockResolvedValueOnce([sibling]); + mockUpdate.mockRejectedValueOnce(axiosError(404, "Folder not found")); + + await act(async () => { + const result = await api.current.rename(parent.id, "newname"); + expect(result).toBeNull(); + }); + + expect(screen.getByTestId("error").textContent).toBe(""); + await waitFor(() => + expect(screen.getByTestId("count").textContent).toBe("1"), + ); + expect(screen.getByTestId("reachable").textContent).toBe("true"); + }); + + test("deleteFolder 404 is treated as already-deleted, returns [id], no banner", async () => { + const target = makeFolder("target"); + const other = makeFolder("other"); + const api = await setupWithFolders([target, other]); + + mockList.mockResolvedValueOnce([other]); + mockDelete.mockRejectedValueOnce(axiosError(404, "Folder not found")); + + let result: unknown; + await act(async () => { + result = await api.current.delete(target.id); + }); + expect(result).toEqual([target.id]); + + expect(screen.getByTestId("error").textContent).toBe(""); + await waitFor(() => + expect(screen.getByTestId("count").textContent).toBe("1"), + ); + expect(screen.getByTestId("reachable").textContent).toBe("true"); + }); + + test("stale 404 strand-resets currentFolderId when user is inside the dropped subtree", async () => { + // User is parked inside the about-to-be-deleted subtree → must navigate out. + const parent = makeFolder("parent"); + const child = makeFolder("child", parent.id); + const sibling = makeFolder("sibling"); + const api = await setupWithFolders([parent, child, sibling]); + + act(() => { + api.current.setCurrentFolderId(child.id); + }); + await waitFor(() => + expect(screen.getByTestId("current").textContent).toBe(child.id), + ); + + mockList.mockResolvedValueOnce([sibling]); + mockUpdate.mockRejectedValueOnce(axiosError(404, "Folder not found")); + + await act(async () => { + await api.current.rename(parent.id, "doomed-rename"); + }); + + // ROOT_FOLDER_ID is null → probe renders "". + expect(screen.getByTestId("current").textContent).toBe(""); + expect(screen.getByTestId("error").textContent).toBe(""); + }); + + test("non-404 mutation errors still surface (regression guard)", async () => { + const target = makeFolder("target"); + const api = await setupWithFolders([target]); + + mockUpdate.mockRejectedValueOnce(axiosError(500, "boom")); + + let threw = false; + await act(async () => { + try { + await api.current.rename(target.id, "newname"); + } catch { + threw = true; + } + }); + expect(threw).toBe(true); + expect(screen.getByTestId("count").textContent).toBe("1"); + expect(screen.getByTestId("error").textContent).not.toBe(""); + }); +}); diff --git a/frontend/editor/src/core/contexts/FolderContext.tsx b/frontend/editor/src/core/contexts/FolderContext.tsx index 9c69fd0cf..e218072ec 100644 --- a/frontend/editor/src/core/contexts/FolderContext.tsx +++ b/frontend/editor/src/core/contexts/FolderContext.tsx @@ -178,6 +178,40 @@ function reachabilityFromError(err: unknown): boolean { return status !== undefined && status >= 400 && status < 500; } +/** Root + every local descendant via parentFolderId. Bounded for corrupted chains. */ +function collectLocalSubtreeIds( + rootId: FolderId, + folders: FolderRecord[], +): Set { + const childrenByParent = new Map(); + for (const f of folders) { + if (f.parentFolderId === null) continue; + const list = childrenByParent.get(f.parentFolderId) ?? []; + list.push(f.id); + childrenByParent.set(f.parentFolderId, list); + } + const result = new Set([rootId]); + const stack: FolderId[] = [rootId]; + const MAX_LOCAL_SUBTREE_NODES = 10_000; + while (stack.length > 0 && result.size < MAX_LOCAL_SUBTREE_NODES) { + const cur = stack.pop()!; + const children = childrenByParent.get(cur); + if (!children) continue; + for (const childId of children) { + if (!result.has(childId)) { + result.add(childId); + stack.push(childId); + } + } + } + return result; +} + +/** Extract HTTP status off an axios-style error, or undefined on network failure. */ +function errorStatus(err: unknown): number | undefined { + return (err as { response?: { status?: number } })?.response?.status; +} + /** * True if `currentId` or any of its ancestors is in `removedSet`. Walks up via * `parentFolderId` using the pre-removal `folders` snapshot so the chain is @@ -390,24 +424,58 @@ export function FolderProvider({ children }: FolderProviderProps) { // ─── mutations: server-first, cache update on success ────────────── + // Lifted up so handleStaleFolder below can reuse the file-detach primitive. + const { clearFolderForFiles } = useIndexedDB(); + + /** Treat a per-folder 404 as "deleted elsewhere": drop subtree, strand-reset, pull. */ + const handleStaleFolder = useCallback( + (staleId: FolderId, foldersSnapshot: FolderRecord[]) => { + const subtree = collectLocalSubtreeIds(staleId, foldersSnapshot); + if (mountedRef.current) { + setServerReachable(true); + setError(null); + setFolders((prev) => prev.filter((f) => !subtree.has(f.id))); + if ( + currentFolderId !== null && + shouldStrandedReset(currentFolderId, subtree, foldersSnapshot) + ) { + setCurrentFolderId(ROOT_FOLDER_ID); + } + } + const subtreeIds = [...subtree]; + // Best-effort - pullFromServer is authoritative if these miss. + void folderStorage + .removeFolders(subtreeIds) + .catch((e) => console.warn("[FolderContext] stale cache cleanup", e)); + void clearFolderForFiles(subtreeIds).catch((e) => + console.warn("[FolderContext] stale file-folder cleanup", e), + ); + bumpFolderRevision(); + void pullFromServer(); + }, + [bumpFolderRevision, clearFolderForFiles, currentFolderId, pullFromServer], + ); + /** - * Centralised mutation wrapper: - * 1. Calls the server op. - * 2. On success: flips `serverReachable=true`, updates in-memory state - * from the server response, then best-effort writes the cache (cache - * failure does NOT roll back; the in-memory truth came from the server). - * 3. On failure: updates `serverReachable` per the error class, surfaces - * via {@link setError}, re-throws so the caller's dialog can stay open. + * Server-first mutation wrapper. On 404 with `staleFolderId`, hands off to + * handleStaleFolder and resolves `null`; other errors surface + re-throw. */ const runFolderMutation = useCallback( async ( serverOp: () => Promise, onSuccess: (result: T) => void | Promise, - ): Promise => { + staleFolderId: FolderId | null = null, + ): Promise => { let result: T; try { result = await serverOp(); } catch (err) { + if (errorStatus(err) === 404 && staleFolderId !== null) { + // `folders` is a closure snapshot; functional setFolders + the pull + // keep this race-safe even if a concurrent mutation shifted state. + handleStaleFolder(staleFolderId, folders); + return null; + } if (mountedRef.current) { setServerReachable(reachabilityFromError(err)); setError(formatServerError(err)); @@ -421,18 +489,13 @@ export function FolderProvider({ children }: FolderProviderProps) { try { await onSuccess(result); } catch (cacheErr) { - // The server is authoritative - a cache write failure must not be - // surfaced as if the operation failed. Log + leave the in-memory - // state authoritative; next pullFromServer will re-seed the cache. - console.warn( - "[FolderContext] cache update failed after successful mutation", - cacheErr, - ); + // Cache write failure is non-fatal; next pull re-seeds. + console.warn("[FolderContext] cache update after mutation", cacheErr); } bumpFolderRevision(); return result; }, - [bumpFolderRevision], + [bumpFolderRevision, folders, handleStaleFolder], ); const createFolder = useCallback( @@ -441,10 +504,9 @@ export function FolderProvider({ children }: FolderProviderProps) { parentFolderId: FolderId | null = currentFolderId, ): Promise => { const color = pickFolderColor(name); - // Generate id client-side so the server's idempotency check makes - // retries safe (network blip → second POST returns the same row). + // Client-side id makes server idempotency check safe on retry. const id = createFolderId(); - return runFolderMutation( + const result = await runFolderMutation( () => folderSyncService.create({ id, @@ -460,6 +522,11 @@ export function FolderProvider({ children }: FolderProviderProps) { await folderStorage.upsertFolder(record); }, ); + // No staleFolderId passed → null branch can't fire; defensive throw. + if (result === null) { + throw new Error("createFolder unexpectedly returned null"); + } + return result; }, [currentFolderId, runFolderMutation], ); @@ -474,6 +541,7 @@ export function FolderProvider({ children }: FolderProviderProps) { ); await folderStorage.upsertFolder(record); }, + id, ); }, [runFolderMutation], @@ -493,6 +561,7 @@ export function FolderProvider({ children }: FolderProviderProps) { ); await folderStorage.upsertFolder(record); }, + id, ); }, [runFolderMutation], @@ -515,13 +584,12 @@ export function FolderProvider({ children }: FolderProviderProps) { ); await folderStorage.upsertFolder(record); }, + id, ); }, [runFolderMutation], ); - const { clearFolderForFiles } = useIndexedDB(); - const deleteFolder = useCallback( async (id: FolderId): Promise => { // Custom path (not runFolderMutation) because we have two best-effort @@ -532,6 +600,11 @@ export function FolderProvider({ children }: FolderProviderProps) { try { removed = await folderSyncService.delete(id); } catch (err) { + if (errorStatus(err) === 404) { + // Already gone; treat as success. Return [id]; pull is authoritative. + handleStaleFolder(id, folders); + return [id]; + } if (mountedRef.current) { setServerReachable(reachabilityFromError(err)); setError(formatServerError(err)); @@ -580,7 +653,13 @@ export function FolderProvider({ children }: FolderProviderProps) { } return removed; }, - [bumpFolderRevision, clearFolderForFiles, currentFolderId, folders], + [ + bumpFolderRevision, + clearFolderForFiles, + currentFolderId, + folders, + handleStaleFolder, + ], ); const value = useMemo(