fix(ui): stop range-based fetching hooks from spinning in a render loop - #9439
Open
lstein wants to merge 3 commits into
Open
fix(ui): stop range-based fetching hooks from spinning in a render loop#9439lstein wants to merge 3 commits into
lstein wants to merge 3 commits into
Conversation
`fetchItems` cleared the accumulated ranges with `setPendingRanges([])`, and `pendingRanges` is a dependency of the effect that calls `fetchItems`. A fresh `[]` is a new identity every time, so the effect re-ran, re-armed the 500ms throttle, and cleared again — a self-sustaining render loop that ran as fast as the throttle allowed, with no user input, for as long as the gallery grid was mounted. Clear with the shared stable `EMPTY_ARRAY` reference instead, so React bails out rather than re-running the effect. The queue variant returned early — before clearing — when nothing was uncached, which happened to prevent the loop while everything was cached, at the cost of letting ranges accumulate for the lifetime of the list and growing the scan on every pass. It now clears on both paths, with the stable reference doing the work of stopping the loop. Retry on failure explicitly, because the loop was doing it accidentally. These bulk fetches are the only fetcher for their rows: `ImageAtPosition` and `QueueItemAtPosition` both consume the cache with `skip: isUninitialized`, so a row whose DTO never arrived does not fetch for itself, and images have no retry affordance. Without this, a transient failure would leave placeholders until the user happened to scroll, where before the loop re-tried until it succeeded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
requested review from
JPPhoto,
Pfannkuchensack,
blessedcoolant and
dunkeroni
as code owners
August 2, 2026 02:06
Render both hooks with React act + fake timers in a happy-dom environment (scoped per-file via a @vitest-environment docblock; happy-dom is the only new dev dependency) and mock only the thin API-endpoint modules, so the tests exercise the real state/effect/throttle cycle the fix changed. Covered per hook: - a reported range fetches its uncached items once, then renders and fetches both go quiet (the pre-fix loop re-rendered every throttle window forever, and in the gallery hook ran from mount even with nothing to fetch) - items that never land in the cache (deleted image, multiuser ownership filter) are not re-requested indefinitely — bounded, then quiet, where the pre-fix loop was a permanent one-request-per-window stream - a failed bulk fetch is retried until it succeeds, then goes quiet — the explicit replacement for the retry the loop provided accidentally - every range reported within a throttle window is fetched, not just the last (the pendingRanges accumulation onRangeChanged exists for) - handled ranges are dropped, not accumulated: an item evicted from a long-handled range is not re-requested by later passes (the queue hook's pre-fix early return without clearing regressed exactly this) - new ranges after settling still fetch, and enabled=false fetches nothing The time-advance helper steps in small increments with an act flush per step; a single long advance would defer effect re-runs to the end of the act scope and break the very feedback cycle (state update -> effect -> throttle -> fetch) the suite exists to detect. Mutation-verified: reverting the EMPTY_ARRAY clears, restoring the queue hook's early return, dropping onRangeChanged's accumulation, or neutering the retry catch each makes at least one test fail; all pass with the fix in place. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix (performance).
useRangeBasedImageFetchinganduseRangeBasedQueueItemFetchingspin in a self-sustaining render loop for as long as the gallery grid / queue list is mounted.The shape of the bug, in
useRangeBasedImageFetching:setPendingRanges([])installs a fresh array, which is neverObject.is-equal to the previous one, so the effect re-runs.useThrottledCallbackresolves to{maxWait: 500, leading: true, trailing: true}, so the re-entry schedules a trailing invocation, which callsfetchItems, which clears again. Round and round, several times a second, indefinitely, with no user input. In the gallery hook the clear is unconditional, so the loop runs from mount even when there is nothing to fetch; the queue hook returned early before clearing when everything was cached, so there it only ran while items were genuinely uncached.Most of the time this only burns CPU and re-renders. It turns into a permanent 2Hz request stream whenever a name in the visible range never lands in the cache — because
getImageDTOsByNames.onQueryStartedupserts only the DTOs the server actually returned:A requested name that comes back missing (a deleted image still present in the name list, an item filtered out by ownership in multiuser mode) therefore never gets a
getImageDTOcache entry,selectCachedArgsForQuerynever reports it, and it is re-requested on every pass — twice a second, forever.The change: clear with the shared stable
EMPTY_ARRAYreference fromapp/store/constants.ts, which is already used across the app for exactly this purpose. Setting state to the value it already holds makes React bail out instead of re-running the effect. Real range changes still flow throughonRangeChanged, which setslastRangeto a new object, so fetching on scroll is unaffected.The queue variant also returned early without clearing when nothing was uncached, letting ranges accumulate for the lifetime of the list and growing the scan on every subsequent pass. It now clears on both paths — the ranges have been handled either way.
The loop was also an accidental retry, so the retry is now explicit. These bulk fetches are the only fetcher for their rows.
ImageAtPositionandQueueItemAtPositionboth consume the cache with the documented "subscribe once it has data" hack:so a row whose DTO never arrived does not fetch for itself,
onQueryStartedswallows the failure incatch {}, and nobody reads the mutation's error state. Videos have a retry button; images and queue items do not. Pre-fix, a failed bulk fetch was simply re-attempted by the loop until it succeeded. Removing the loop without replacing that would mean a transient failure — a backend restart, a 502 from a reverse proxy — leaves grey placeholders until the user happens to scroll, since nothing else changes any dependency of the effect (RTK Query'sstructuralSharingpreserves theimageNamesreference even across a refetch). So the failure path now restores the pending ranges, which re-runs the effect; the throttle bounds the retry rate to what it was before.Related Issues / Discussions
Found while investigating an unrelated report of repeated socket connections; see #9438 for that one. No existing issue.
QA Instructions
The loop is easiest to see with React DevTools:
For the network half, you need a name that the server will not return — e.g. delete an image directly from the DB (or via another client) so it stays in the cached name list, then scroll it into view. Before:
POST /api/v1/images/images_by_namesrepeats every ~500ms indefinitely in the Network tab. After: it fires once per range change.Regression checks:
imageNames).Notes for reviewers
Two things worth knowing, both found by adversarially reviewing this diff:
EMPTY_ARRAYinapp/store/constants.tsisnever[]— mutable, unfrozen, and now referenced from ~30 files and held as component state by these hooks. Nothing mutates it today (audited), but a futurependingRanges.push(...)would compile fine and silently corrupt unrelated selectors app-wide.Object.freeze([])there would close that off without any type churn. Happy to do it separately if wanted.Automated: regression tests added for both hooks (
useRangeBasedImageFetching.test.ts,useRangeBasedQueueItemFetching.test.ts). They render the real hooks with Reactact+ fake timers in ahappy-domenvironment (scoped per-file via a@vitest-environmentdocblock —happy-domis the only new dev dependency; the rest of the suite stays in the node environment), mocking only the thin API-endpoint modules so the actual state/effect/throttle cycle is exercised. Covered per hook: a reported range fetches its uncached items once and then renders and fetches go quiet; never-cached items (the deleted-image / multiuser-filter case) are re-requested boundedly rather than forever; a failed bulk fetch retries until it succeeds, then goes quiet; every range reported within a throttle window is fetched, not just the last; handled ranges are dropped rather than accumulated (an item evicted from a long-handled range is not re-requested — the queue hook's pre-fix early return regressed exactly this); new ranges after settling still fetch;enabled: falsefetches nothing. Mutation-verified: reverting theEMPTY_ARRAYclears, restoring the queue hook's early return, droppingonRangeChanged's accumulation, or neutering the retry catch each makes at least one test fail; all 17 pass with the fix in place.pnpm test:no-watch— 144 files / 1729 tests pass.pnpm lint:eslint,pnpm lint:prettier,pnpm lint:tsc,pnpm lint:knipall clean.Merge Plan
Ordinary merge. No redux slice changes, so no migration.
Checklist
What's Newcopy (if doing a release after this PR) — n/a