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) <noreply@anthropic.com>
This commit is contained in:
@@ -34,6 +34,17 @@ def _owner_filter(user: User, scope: str | None):
|
|||||||
return sa_true()
|
return sa_true()
|
||||||
return Photo.user_id == user.id
|
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
|
# Media types we accept in the regenerate-thumbnails request body. Mirrors
|
||||||
# the values produced by `app.tasks.scan.get_media_type`.
|
# the values produced by `app.tasks.scan.get_media_type`.
|
||||||
_VALID_MEDIA_TYPES = {'photo', 'raw', 'heic', 'video'}
|
_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
|
size = (await db.execute(select(func.sum(Photo.file_size)).where(owner))).scalar() or 0
|
||||||
|
|
||||||
# Source root directories (active ones only). Scoped to the
|
roots = sorted(await _active_source_root_paths(db, current_user, scope))
|
||||||
# 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()
|
|
||||||
|
|
||||||
return {
|
return {
|
||||||
"all_photos": all_photos_count,
|
"all_photos": all_photos_count,
|
||||||
@@ -731,28 +734,18 @@ async def get_duplicate_groups(
|
|||||||
"""
|
"""
|
||||||
owner = _owner_filter(current_user, scope)
|
owner = _owner_filter(current_user, scope)
|
||||||
|
|
||||||
# Restrict to photos that actually live under an active SourceRoot
|
# Restrict to photos under an active SourceRoot in the user's
|
||||||
# in the user's settings. The folder→source_root link alone is not
|
# settings. Folder.source_root_id alone isn't trustworthy: Nextcloud's
|
||||||
# enough: Nextcloud's "move to trash" flow can leave a Folder row at
|
# "move to trash" flow leaves Folder rows like `…/files/.delete/
|
||||||
# a path like `…/files/.delete/purge-1` still wired to the original
|
# purge-1` still wired to the original source_root_id, leaking their
|
||||||
# source_root_id, which then leaks its photos into the duplicates
|
# photos into this view as ghost paths the user never configured.
|
||||||
# view even though that path is outside everything the user
|
active_root_paths = await _active_source_root_paths(db, current_user, scope)
|
||||||
# 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()
|
|
||||||
if not active_root_paths:
|
if not active_root_paths:
|
||||||
return {"groups": [], "total_groups": 0, "total_members": 0}
|
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_in_scope = or_(
|
||||||
*[
|
*[
|
||||||
(Folder.path == p) | (Folder.path.like(p.rstrip('/') + '/%'))
|
(Folder.path == p) | (Folder.path.like(p + '/%'))
|
||||||
for p in active_root_paths
|
for p in active_root_paths
|
||||||
]
|
]
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -199,10 +199,6 @@ export function Timeline() {
|
|||||||
// Shared photos query — both Timeline and PreviewView use the same hook so
|
// Shared photos query — both Timeline and PreviewView use the same hook so
|
||||||
// they share one cache entry, regardless of filter state.
|
// they share one cache entry, regardless of filter state.
|
||||||
const { data: photos = [], isLoading } = usePhotosQuery()
|
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()
|
const isLoadingMore = usePhotosLoadingMore()
|
||||||
|
|
||||||
// Tracks the previous viewMode so the "preview just closed" scroll
|
// Tracks the previous viewMode so the "preview just closed" scroll
|
||||||
@@ -736,8 +732,9 @@ export function Timeline() {
|
|||||||
|
|
||||||
if (isLoading) {
|
if (isLoading) {
|
||||||
return (
|
return (
|
||||||
<div className="flex items-center justify-center h-full">
|
<div className="flex h-full items-center justify-center gap-2 text-text-muted">
|
||||||
<div className="text-text-muted">Loading photos...</div>
|
<Loader2 className="h-4 w-4 animate-spin" />
|
||||||
|
Loading photos…
|
||||||
</div>
|
</div>
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
@@ -794,11 +791,7 @@ export function Timeline() {
|
|||||||
position: 'relative',
|
position: 'relative',
|
||||||
}}
|
}}
|
||||||
>
|
>
|
||||||
{/* Bottom loading indicator. Positioned absolutely just below
|
{/* Absolute-positioned so layout doesn't jitter when pages flip. */}
|
||||||
* 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. */}
|
|
||||||
{isLoadingMore && photos.length > 0 && (
|
{isLoadingMore && photos.length > 0 && (
|
||||||
<div
|
<div
|
||||||
className="pointer-events-none absolute left-0 right-0 flex items-center justify-center gap-2 py-4 text-xs text-text-muted"
|
className="pointer-events-none absolute left-0 right-0 flex items-center justify-center gap-2 py-4 text-xs text-text-muted"
|
||||||
|
|||||||
@@ -5,11 +5,8 @@ import { usePhotosBackgroundLoadingStore } from '../store/photosBackgroundLoadin
|
|||||||
import api from '../services/api'
|
import api from '../services/api'
|
||||||
import type { Photo } from '../types/photo'
|
import type { Photo } from '../types/photo'
|
||||||
|
|
||||||
/** Reactive selector for the "more pages still streaming" flag. Used by
|
|
||||||
* thumbnail views to render a bottom-of-list spinner while
|
|
||||||
* usePhotosQuery's background cursor loop is still pulling pages. */
|
|
||||||
export function usePhotosLoadingMore(): boolean {
|
export function usePhotosLoadingMore(): boolean {
|
||||||
return usePhotosBackgroundLoadingStore((s) => s.isLoadingMore)
|
return usePhotosBackgroundLoadingStore((s) => s.inFlight > 0)
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -108,12 +105,10 @@ export function usePhotosQuery() {
|
|||||||
|
|
||||||
if (nextCursor) {
|
if (nextCursor) {
|
||||||
// Fire-and-forget background loop using cursor chaining.
|
// Fire-and-forget background loop using cursor chaining.
|
||||||
// The background store is reactive — Timeline et al. subscribe
|
// start/stop balanced in try/finally so an aborted or erroring
|
||||||
// to render a "loading more…" indicator at the bottom of the
|
// loop can't leak the in-flight counter that drives the
|
||||||
// grid while pages keep arriving. start/stop is balanced in a
|
// bottom-of-grid spinner.
|
||||||
// try/finally so an aborted/erroring loop can't leak the flag.
|
usePhotosBackgroundLoadingStore.getState().start()
|
||||||
const bg = usePhotosBackgroundLoadingStore.getState()
|
|
||||||
bg.start()
|
|
||||||
void (async () => {
|
void (async () => {
|
||||||
try {
|
try {
|
||||||
for (let i = 0; i < MAX_PAGES && nextCursor; i++) {
|
for (let i = 0; i < MAX_PAGES && nextCursor; i++) {
|
||||||
|
|||||||
@@ -1,34 +1,17 @@
|
|||||||
import { create } from 'zustand'
|
import { create } from 'zustand'
|
||||||
|
|
||||||
/**
|
// Counter (not boolean) so an aborted-then-restarted loop on rapid
|
||||||
* Tracks whether usePhotosQuery's background cursor-chasing loop is
|
// filter changes can't flip the flag false while a fresh loop is still
|
||||||
* still pulling pages. Lives outside react-query because the loop runs
|
// active. react-query's isFetching doesn't cover this loop because it
|
||||||
* after the initial fetch resolves and react-query's isFetching only
|
// runs after queryFn resolves and writes via setQueryData directly.
|
||||||
* 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.
|
|
||||||
*/
|
|
||||||
interface PhotosBackgroundLoadingState {
|
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
|
inFlight: number
|
||||||
isLoadingMore: boolean
|
|
||||||
start: () => void
|
start: () => void
|
||||||
stop: () => void
|
stop: () => void
|
||||||
}
|
}
|
||||||
|
|
||||||
export const usePhotosBackgroundLoadingStore = create<PhotosBackgroundLoadingState>((set) => ({
|
export const usePhotosBackgroundLoadingStore = create<PhotosBackgroundLoadingState>((set) => ({
|
||||||
inFlight: 0,
|
inFlight: 0,
|
||||||
isLoadingMore: false,
|
start: () => set((s) => ({ inFlight: s.inFlight + 1 })),
|
||||||
start: () =>
|
stop: () => set((s) => ({ inFlight: Math.max(0, s.inFlight - 1) })),
|
||||||
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 }
|
|
||||||
}),
|
|
||||||
}))
|
}))
|
||||||
|
|||||||
Reference in New Issue
Block a user