From 4e1af0f35675d0264ae2951fc14664def28be06e Mon Sep 17 00:00:00 2001 From: claudio Date: Wed, 13 May 2026 20:50:17 +0200 Subject: [PATCH] refactor: simplify after self-review - drop the redundant isLoadingMore boolean from the background-loading store; derive from inFlight > 0 in the selector - pull _active_source_root_paths out of get_library_stats and reuse it from get_duplicate_groups (same is_active + admin-scope check, now in one place) - drop p.rstrip('/') in the duplicates folder-scope filter (SourceRoot paths are never written with a trailing slash) - match Timeline's initial-load affordance to the new bottom indicator (Loader2 spinner + ellipsis instead of plain "Loading photos...") - trim narrative comments that explained what the surrounding code does Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/app/routers/library.py | 45 ++++++++----------- frontend/src/components/timeline/Timeline.tsx | 15 ++----- frontend/src/hooks/usePhotosQuery.ts | 15 +++---- .../src/store/photosBackgroundLoadingStore.ts | 29 +++--------- 4 files changed, 34 insertions(+), 70 deletions(-) diff --git a/backend/app/routers/library.py b/backend/app/routers/library.py index 5c3acb3..a460905 100644 --- a/backend/app/routers/library.py +++ b/backend/app/routers/library.py @@ -34,6 +34,17 @@ def _owner_filter(user: User, scope: str | None): return sa_true() return Photo.user_id == user.id + +async def _active_source_root_paths( + db: AsyncSession, user: User, scope: str | None +) -> list[str]: + """Active SourceRoot.path values visible to this user, honouring the + admin ?scope=global escape hatch.""" + q = select(SourceRoot.path).where(SourceRoot.is_active.is_(True)) + if not (scope == "global" and user.role == "admin"): + q = q.where(SourceRoot.user_id == user.id) + return (await db.execute(q)).scalars().all() + # Media types we accept in the regenerate-thumbnails request body. Mirrors # the values produced by `app.tasks.scan.get_media_type`. _VALID_MEDIA_TYPES = {'photo', 'raw', 'heic', 'video'} @@ -114,15 +125,7 @@ async def get_library_stats( size = (await db.execute(select(func.sum(Photo.file_size)).where(owner))).scalar() or 0 - # Source root directories (active ones only). Scoped to the - # requesting user unless they're an admin asking for global view — - # otherwise the Settings panel would leak other users' NC paths. - sr_query = select(SourceRoot.path).where(SourceRoot.is_active.is_(True)) - if not (scope == "global" and current_user.role == "admin"): - sr_query = sr_query.where(SourceRoot.user_id == current_user.id) - roots = ( - await db.execute(sr_query.order_by(SourceRoot.path)) - ).scalars().all() + roots = sorted(await _active_source_root_paths(db, current_user, scope)) return { "all_photos": all_photos_count, @@ -731,28 +734,18 @@ async def get_duplicate_groups( """ owner = _owner_filter(current_user, scope) - # Restrict to photos that actually live under an active SourceRoot - # in the user's settings. The folder→source_root link alone is not - # enough: Nextcloud's "move to trash" flow can leave a Folder row at - # a path like `…/files/.delete/purge-1` still wired to the original - # source_root_id, which then leaks its photos into the duplicates - # view even though that path is outside everything the user - # configured. Filtering on the folder's path being a descendant of - # the active root's path matches what the user actually expects to - # see in Settings → Source folders. - sr_path_query = select(SourceRoot.path).where(SourceRoot.is_active.is_(True)) - if not (scope == "global" and current_user.role == "admin"): - sr_path_query = sr_path_query.where(SourceRoot.user_id == current_user.id) - active_root_paths = (await db.execute(sr_path_query)).scalars().all() + # Restrict to photos under an active SourceRoot in the user's + # settings. Folder.source_root_id alone isn't trustworthy: Nextcloud's + # "move to trash" flow leaves Folder rows like `…/files/.delete/ + # purge-1` still wired to the original source_root_id, leaking their + # photos into this view as ghost paths the user never configured. + active_root_paths = await _active_source_root_paths(db, current_user, scope) if not active_root_paths: return {"groups": [], "total_groups": 0, "total_members": 0} - # Match the source root path itself OR a strict child (path + '/'). - # `startswith` alone would accept `/files/Photos2` for a `/files/Photos` - # root. folder_in_scope = or_( *[ - (Folder.path == p) | (Folder.path.like(p.rstrip('/') + '/%')) + (Folder.path == p) | (Folder.path.like(p + '/%')) for p in active_root_paths ] ) diff --git a/frontend/src/components/timeline/Timeline.tsx b/frontend/src/components/timeline/Timeline.tsx index b0716fb..418a655 100644 --- a/frontend/src/components/timeline/Timeline.tsx +++ b/frontend/src/components/timeline/Timeline.tsx @@ -199,10 +199,6 @@ export function Timeline() { // Shared photos query — both Timeline and PreviewView use the same hook so // they share one cache entry, regardless of filter state. const { data: photos = [], isLoading } = usePhotosQuery() - // Background cursor loop (after the initial page resolves) still - // streaming more photos? Drives the bottom-of-grid spinner so the - // user has a signal that more results are on the way and a sudden - // "end of list" is real rather than mid-fetch. const isLoadingMore = usePhotosLoadingMore() // Tracks the previous viewMode so the "preview just closed" scroll @@ -736,8 +732,9 @@ export function Timeline() { if (isLoading) { return ( -
-
Loading photos...
+
+ + Loading photos…
) } @@ -794,11 +791,7 @@ export function Timeline() { position: 'relative', }} > - {/* Bottom loading indicator. Positioned absolutely just below - * the virtualizer's last row so a mid-grid spinner doesn't - * jitter the layout when pages flip. Sits inside the same - * positioned wrapper that hosts the virtual items so the - * parent's translate space stays self-contained. */} + {/* Absolute-positioned so layout doesn't jitter when pages flip. */} {isLoadingMore && photos.length > 0 && (
s.isLoadingMore) + return usePhotosBackgroundLoadingStore((s) => s.inFlight > 0) } /** @@ -108,12 +105,10 @@ export function usePhotosQuery() { if (nextCursor) { // Fire-and-forget background loop using cursor chaining. - // The background store is reactive — Timeline et al. subscribe - // to render a "loading more…" indicator at the bottom of the - // grid while pages keep arriving. start/stop is balanced in a - // try/finally so an aborted/erroring loop can't leak the flag. - const bg = usePhotosBackgroundLoadingStore.getState() - bg.start() + // start/stop balanced in try/finally so an aborted or erroring + // loop can't leak the in-flight counter that drives the + // bottom-of-grid spinner. + usePhotosBackgroundLoadingStore.getState().start() void (async () => { try { for (let i = 0; i < MAX_PAGES && nextCursor; i++) { diff --git a/frontend/src/store/photosBackgroundLoadingStore.ts b/frontend/src/store/photosBackgroundLoadingStore.ts index 9affcf9..29dea09 100644 --- a/frontend/src/store/photosBackgroundLoadingStore.ts +++ b/frontend/src/store/photosBackgroundLoadingStore.ts @@ -1,34 +1,17 @@ import { create } from 'zustand' -/** - * Tracks whether usePhotosQuery's background cursor-chasing loop is - * still pulling pages. Lives outside react-query because the loop runs - * after the initial fetch resolves and react-query's isFetching only - * covers fetches it owns. Consumers (Timeline, RatedView, ColorsView…) - * use isLoadingMore to decide whether to render the "loading more - * photos…" indicator at the bottom of the grid. - */ +// Counter (not boolean) so an aborted-then-restarted loop on rapid +// filter changes can't flip the flag false while a fresh loop is still +// active. react-query's isFetching doesn't cover this loop because it +// runs after queryFn resolves and writes via setQueryData directly. interface PhotosBackgroundLoadingState { - /** Count of in-flight background pagers. Stored as a counter so an - * aborted-then-restarted loop on rapid filter changes can't flip the - * flag false while a fresh loop is still active. */ inFlight: number - isLoadingMore: boolean start: () => void stop: () => void } export const usePhotosBackgroundLoadingStore = create((set) => ({ inFlight: 0, - isLoadingMore: false, - start: () => - set((s) => ({ - inFlight: s.inFlight + 1, - isLoadingMore: true, - })), - stop: () => - set((s) => { - const next = Math.max(0, s.inFlight - 1) - return { inFlight: next, isLoadingMore: next > 0 } - }), + start: () => set((s) => ({ inFlight: s.inFlight + 1 })), + stop: () => set((s) => ({ inFlight: Math.max(0, s.inFlight - 1) })), }))