From 269dd0d89b6c846f58a13046319d503bc996af14 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Thu, 3 Sep 2026 22:40:42 +0000 Subject: [PATCH] fix: surface a failed collections fetch instead of a false empty state (#6019) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On /media/collections a rejected listMediaCollections() was swallowed into `[]`. That made a transient 500 or offline blip render the "No collections yet" onboarding copy and hand every image and video in the library to the synthetic "Unsorted" bucket — reading as "all my collections were deleted", with no error banner and no way to recover once the toast faded. Collections now use the repo's sentinel convention: `null` = never fetched or fetch failed, `[]` = the server really has none. A failed read sets an error state and leaves the list alone (the sentinel on first load, the last good list on a later refresh), so the grid never claims the library is unfiled. The page renders a persistent error banner naming the failure with a Retry button, and the onboarding empty state only appears on a genuine zero-collection response. The fetch now passes `silent: true` since the page owns its failure UI. Claude-Session: https://claude.ai/code/session_01RA3pD5YM2dukQwbZ3pC6WA --- client/src/pages/MediaCollections.jsx | 70 +++++++++++++++++++--- client/src/pages/MediaCollections.test.jsx | 52 +++++++++++++++- 2 files changed, 112 insertions(+), 10 deletions(-) diff --git a/client/src/pages/MediaCollections.jsx b/client/src/pages/MediaCollections.jsx index 14a2f7e6d8..fad450c691 100644 --- a/client/src/pages/MediaCollections.jsx +++ b/client/src/pages/MediaCollections.jsx @@ -1,7 +1,8 @@ import { useEffect, useMemo, useRef, useState } from 'react'; import { Link, useNavigate } from 'react-router'; -import { Plus, FolderOpen, Inbox, Trash2, Image as ImageIcon, Film, Search } from 'lucide-react'; +import { Plus, FolderOpen, Inbox, Trash2, Image as ImageIcon, Film, Search, AlertTriangle } from 'lucide-react'; import PageSkeleton from '../components/ui/PageSkeleton'; +import Banner from '../components/ui/Banner'; import EmptyState from '../components/EmptyState'; import toast from '../components/ui/Toast'; import { @@ -69,7 +70,13 @@ export default function MediaCollections() { const nameInputRef = useRef(null); const navigate = useNavigate(); const [searchParams, updateParams] = useUrlParams(); - const [collections, setCollections] = useState([]); + // `null` = collections were never successfully fetched (initial state, or a + // first-load failure); `[]` = the server really has none. Collapsing the two + // is what let a failed fetch render the "No collections yet" onboarding copy + // and hand every image/video in the library to the synthetic "Unsorted" + // bucket, reading as "all my collections were deleted" (#6019). + const [collections, setCollections] = useState(null); + const [collectionsError, setCollectionsError] = useState(null); const [loading, setLoading] = useState(true); const [creating, setCreating] = useState(false); const [name, setName] = useState(''); @@ -82,12 +89,28 @@ export default function MediaCollections() { const refresh = async () => { setLoading(true); + // The collections leg resolves to an array on success and to an + // `{ error }` envelope on failure, so the failure survives `Promise.all` + // as data instead of being flattened into an indistinguishable `[]`. + // `silent: true` because this page owns the failure UI below — the shared + // `request()` toast would fade and leave the banner as the only signal. const [cols, images, videos] = await Promise.all([ - listMediaCollections().catch(() => []), + listMediaCollections({ silent: true }).then( + (list) => (Array.isArray(list) ? list : []), + (err) => ({ error: err?.message || 'The collections list could not be loaded.' }), + ), listImageGallery().catch(() => []), listVideoHistory().catch(() => []), ]); - setCollections(Array.isArray(cols) ? cols : []); + if (Array.isArray(cols)) { + setCollections(cols); + setCollectionsError(null); + } else { + // Deliberately leave `collections` alone: the sentinel on a first-load + // failure, or the last good list on a refresh failure. Either way the + // grid never claims the library is unfiled. + setCollectionsError(cols.error); + } setImagesByName(new Map((images || []).map((i) => [i.filename, i]))); setVideosById(new Map((videos || []).map((v) => [v.id, v]))); setLoading(false); @@ -105,7 +128,10 @@ export default function MediaCollections() { }); setCreating(false); if (created) { - setCollections((prev) => [...prev, created]); + // Only extend a list we actually have — fabricating one from a single + // record while the fetch is still failed would clear the sentinel and + // resurrect the "everything is unsorted" lie. + setCollections((prev) => (prev ? [...prev, created] : prev)); setName(''); toast.success(`Created "${created.name}"`); // Open the new collection instead of dropping the user back on the grid. @@ -119,7 +145,7 @@ export default function MediaCollections() { }; const handleDelete = async (collection) => { - setCollections((prev) => prev.filter((c) => c.id !== collection.id)); + setCollections((prev) => (prev ? prev.filter((c) => c.id !== collection.id) : prev)); await deleteMediaCollection(collection.id, { silent: true }).catch((err) => { toast.error(err.message || 'Delete failed'); refresh(); @@ -128,12 +154,15 @@ export default function MediaCollections() { const images = useMemo(() => Array.from(imagesByName.values()), [imagesByName]); const videos = useMemo(() => Array.from(videosById.values()), [videosById]); + // Without a known collection list there is nothing to diff media against, so + // every item would look unfiled. Skip the synthetic bucket entirely. const unsorted = useMemo( - () => buildUnsortedCollection(collections, images, videos), + () => (collections ? buildUnsortedCollection(collections, images, videos) : null), [collections, images, videos], ); const enriched = useMemo(() => { + if (!collections) return []; // Pinned synthetic "Unsorted" entry first, then real collections. const all = [unsorted, ...collections]; return all.map((c) => { @@ -263,9 +292,32 @@ export default function MediaCollections() { + {collectionsError && ( + + Retry + + )} + > +
{collectionsError}
+ {/* A read failed — say so, or the missing grid reads as data loss. */} +
Nothing was deleted; the list just couldn't be read.
+ + )} + {loading ? ( - ) : ( + ) : collections ? (
{/* Precedence chain, not independent gates. First run wins only when no search is active — gated on the REAL collections, since the @@ -368,7 +420,7 @@ export default function MediaCollections() {
)} - )} + ) : null} ); } diff --git a/client/src/pages/MediaCollections.test.jsx b/client/src/pages/MediaCollections.test.jsx index 297c0c0278..531faa5a88 100644 --- a/client/src/pages/MediaCollections.test.jsx +++ b/client/src/pages/MediaCollections.test.jsx @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { render, screen, waitFor } from '@testing-library/react'; +import { render, screen, waitFor, within } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { MemoryRouter, useLocation, useNavigate } from 'react-router'; import { typeSettled } from '../test/settledInput'; @@ -271,6 +271,56 @@ describe('MediaCollections', () => { expect(screen.queryByText(/No collections yet/)).not.toBeInTheDocument(); }); + it('reports a failed collections fetch instead of claiming there are none', async () => { + const { listMediaCollections } = await import('../services/api'); + listMediaCollections.mockRejectedValueOnce(new Error('Server error (500)')); + renderPage(); + expect(await screen.findByText(/Couldn't load collections/)).toBeInTheDocument(); + expect(screen.getByText('Server error (500)')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Retry' })).toBeInTheDocument(); + // The onboarding copy would tell the user their collections never existed. + expect(screen.queryByText(/No collections yet/)).not.toBeInTheDocument(); + // …and the synthetic bucket would hand them their whole library as unfiled. + expect(screen.queryByText('Unsorted')).not.toBeInTheDocument(); + expect(screen.queryByText(/No collections match that search/)).not.toBeInTheDocument(); + }); + + it('owns the failure UI, so the shared request() toast stays silent', async () => { + const { listMediaCollections } = await import('../services/api'); + listMediaCollections.mockRejectedValueOnce(new Error('Server error (500)')); + renderPage(); + await screen.findByRole('button', { name: 'Retry' }); + expect(listMediaCollections).toHaveBeenCalledWith({ silent: true }); + }); + + it('recovers the grid when Retry succeeds', async () => { + const { listMediaCollections } = await import('../services/api'); + listMediaCollections.mockRejectedValueOnce(new Error('Server error (500)')); + const user = userEvent.setup(); + renderPage('/media/collections?empty=1'); + await user.click(await screen.findByRole('button', { name: 'Retry' })); + expect(await screen.findByText('Alpha')).toBeInTheDocument(); + await waitFor(() => expect(screen.queryByText(/Couldn't load collections/)).not.toBeInTheDocument()); + }); + + it('keeps the last good grid when a later refresh fails', async () => { + const { listMediaCollections, deleteMediaCollection } = await import('../services/api'); + const user = userEvent.setup(); + renderPage('/media/collections?empty=1'); + await screen.findByText('Alpha'); + // A failed delete calls refresh() — the recovery path that re-reads the + // list. If that read fails, the collections already on screen must survive + // rather than collapsing back into the sentinel and blanking the grid. + deleteMediaCollection.mockRejectedValueOnce(new Error('Delete failed')); + listMediaCollections.mockRejectedValueOnce(new Error('Server error (500)')); + // Scope to the Alpha card — every card renders the same delete label, and + // indexing a match list would silently target whichever row sorted first. + const alphaCard = screen.getByTitle('Alpha').closest('.bg-port-card'); + await user.click(within(alphaCard).getByRole('button', { name: 'Delete collection' })); + expect(await screen.findByText(/Couldn't load collections/)).toBeInTheDocument(); + expect(screen.getByText('Beta')).toBeInTheDocument(); + }); + it('offers the three sort options', async () => { renderPage(); await waitFor(() => screen.getByText('Alpha'));