fix(fileloader): prevent infinite reload loop via untrack #243

marcel · 2026-04-15T22:27:02+02:00

marcel commented

2026-04-15 22:27:02 +02:00

Summary

loadFile() reads fileUrl synchronously before its first await, which causes any calling $effect to accidentally subscribe to fileUrl as a reactive dependency
Every successful file load sets fileUrl = blobUrl, fires the signal, re-runs the $effect, which calls loadFile again — infinite loop
Fix: wrap the fileUrl read in untrack() so callers never subscribe to it

Root cause detail

In Svelte 5, $effect tracks all $state reads that occur synchronously anywhere in the call stack while the effect runs — including inside called functions. The loadFile hook reads fileUrl in its very first sync statement (if (fileUrl) URL.revokeObjectURL(fileUrl)), before the first await. This silently registers fileUrl as a dependency of whichever $effect invokes loadFile.

The loop:

$effect fires → calls loadFile(url) → sync body reads fileUrl → effect now tracks it
Async portion completes → fileUrl = blobUrl → signal fires
$effect re-runs → calls loadFile again → revokes blobUrl, sets fileUrl = ''
fileUrl changed → $effect re-runs → goto 2

This manifested as the PDF viewer cycling endlessly through loading → mounted → loading, visible as "page keeps refreshing".

Test plan

All 5 existing useFileLoader unit tests pass (npx vitest run useFileLoader)
Open a document with a PDF — viewer loads once and stays stable
Navigate away and back — no repeated loading flicker

🤖 Generated with Claude Code

## Summary - `loadFile()` reads `fileUrl` synchronously before its first `await`, which causes any calling `$effect` to accidentally subscribe to `fileUrl` as a reactive dependency - Every successful file load sets `fileUrl = blobUrl`, fires the signal, re-runs the `$effect`, which calls `loadFile` again — infinite loop - Fix: wrap the `fileUrl` read in `untrack()` so callers never subscribe to it ## Root cause detail In Svelte 5, `$effect` tracks *all* `$state` reads that occur synchronously anywhere in the call stack while the effect runs — including inside called functions. The `loadFile` hook reads `fileUrl` in its very first sync statement (`if (fileUrl) URL.revokeObjectURL(fileUrl)`), before the first `await`. This silently registers `fileUrl` as a dependency of whichever `$effect` invokes `loadFile`. The loop: 1. `$effect` fires → calls `loadFile(url)` → sync body reads `fileUrl` → effect now tracks it 2. Async portion completes → `fileUrl = blobUrl` → signal fires 3. `$effect` re-runs → calls `loadFile` again → revokes `blobUrl`, sets `fileUrl = ''` 4. `fileUrl` changed → `$effect` re-runs → goto 2 This manifested as the PDF viewer cycling endlessly through loading → mounted → loading, visible as "page keeps refreshing". ## Test plan - [ ] All 5 existing `useFileLoader` unit tests pass (`npx vitest run useFileLoader`) - [ ] Open a document with a PDF — viewer loads once and stays stable - [ ] Navigate away and back — no repeated loading flicker 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 1 commit 2026-04-15 22:27:03 +02:00

fix(fileloader): use untrack to prevent infinite reload loop

CI / Unit & Component Tests (pull_request) Failing after 2m26s

Details

CI / Backend Unit Tests (pull_request) Failing after 2m43s

Details

CI / Backend Unit Tests (push) Has been cancelled

Details

CI / Unit & Component Tests (push) Has started running

Details

ed12a54339

loadFile() reads fileUrl synchronously before its first await. When
called from a \$effect, Svelte tracks that read and re-runs the effect
every time fileUrl changes — i.e. after every successful load — causing
an infinite cycle of file fetches and PdfViewer remounts.

Fix: wrap the fileUrl read in untrack() so callers never accidentally
subscribe to fileUrl changes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented

2026-04-15 22:46:02 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

Summary

The fix is correct and minimal. The root cause — $effect accidentally tracking fileUrl because loadFile's synchronous preamble reads it before the first await — is a classic Svelte 5 pitfall, and untrack() is exactly the right tool here.

What I checked

Reactive tracking boundary: In Svelte 5, $effect tracks all $state reads that happen synchronously in the entire call stack, including inside functions called from the effect. The old code did if (fileUrl) URL.revokeObjectURL(fileUrl) synchronously, which registered fileUrl as a dependency of the calling $effect. After a successful load, fileUrl was set to a new blob URL, the effect re-ran, revoked it, set fileUrl = '', triggering another re-run — infinite loop.
The fix: const prev = untrack(() => fileUrl) reads fileUrl outside the reactive tracking context. The calling effect never learns about the fileUrl subscription, so it doesn't re-run when fileUrl changes.
Existing tests: All 5 unit tests in useFileLoader.svelte.test.ts pass. They cover revoke-on-reload, revoke-on-destroy, error state, and happy path.
Comment quality: The new inline comment clearly explains the why, not just the what. This is the right call for a non-obvious Svelte 5 subtlety.

One observation (non-blocking)

The unit tests run loadFile in isolation (plain async calls), so they can't detect the reactive loop regression. That loop only manifests inside a Svelte $effect tracking context. A component-level test with @testing-library/svelte that exercises the $effect → loadFile → state-change cycle would be the only way to guard against this class of bug regressing. Not a blocker for this fix — just worth noting for the team's testing strategy.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** ### Summary The fix is correct and minimal. The root cause — `$effect` accidentally tracking `fileUrl` because `loadFile`'s synchronous preamble reads it before the first `await` — is a classic Svelte 5 pitfall, and `untrack()` is exactly the right tool here. ### What I checked - **Reactive tracking boundary**: In Svelte 5, `$effect` tracks all `$state` reads that happen synchronously in the entire call stack, including inside functions called from the effect. The old code did `if (fileUrl) URL.revokeObjectURL(fileUrl)` synchronously, which registered `fileUrl` as a dependency of the calling `$effect`. After a successful load, `fileUrl` was set to a new blob URL, the effect re-ran, revoked it, set `fileUrl = ''`, triggering another re-run — infinite loop. - **The fix**: `const prev = untrack(() => fileUrl)` reads `fileUrl` outside the reactive tracking context. The calling effect never learns about the `fileUrl` subscription, so it doesn't re-run when `fileUrl` changes. - **Existing tests**: All 5 unit tests in `useFileLoader.svelte.test.ts` pass. They cover revoke-on-reload, revoke-on-destroy, error state, and happy path. - **Comment quality**: The new inline comment clearly explains the *why*, not just the *what*. This is the right call for a non-obvious Svelte 5 subtlety. ### One observation (non-blocking) The unit tests run `loadFile` in isolation (plain `async` calls), so they can't detect the reactive loop regression. That loop only manifests inside a Svelte `$effect` tracking context. A component-level test with `@testing-library/svelte` that exercises the `$effect` → `loadFile` → state-change cycle would be the only way to guard against this class of bug regressing. Not a blocker for this fix — just worth noting for the team's testing strategy.

marcel commented

2026-04-15 22:46:15 +02:00

🏗️ Markus Keller — Software Architect

Verdict: ✅ Approved

Summary

Single-file, single-responsibility fix. Nothing to object to architecturally — in fact the change reinforces the encapsulation contract that createFileLoader is supposed to provide.

What I checked

Hook encapsulation boundary: createFileLoader is designed to hide file loading state from callers. The bug was a leaky abstraction: the internal fileUrl state was bleeding out of the hook's reactive boundary and infecting the caller's $effect. The fix seals that leak. The hook's public contract (load a file, expose fileUrl / isLoading / fileError) remains unchanged — callers read fileUrl via the getter and that read is correctly tracked. Only the internal read inside loadFile is now shielded.

No layer boundary violations: The hook stays in $lib/hooks/, dependencies unchanged, no new cross-module coupling introduced.

untrack as a design tool: Using untrack for internal bookkeeping reads is a well-established Svelte 5 pattern (the Svelte docs explicitly recommend it for "previous value" tracking inside $effect). This is boring, correct usage — exactly what I want to see.

Risk of the change: Near-zero. The prev variable is populated with the current fileUrl value before any mutation. The revoke-and-reset logic is identical to before, just protected from reactive tracking. No behavioural change under any load path.

## 🏗️ Markus Keller — Software Architect **Verdict: ✅ Approved** ### Summary Single-file, single-responsibility fix. Nothing to object to architecturally — in fact the change reinforces the encapsulation contract that `createFileLoader` is supposed to provide. ### What I checked **Hook encapsulation boundary**: `createFileLoader` is designed to hide file loading state from callers. The bug was a leaky abstraction: the internal `fileUrl` state was bleeding out of the hook's reactive boundary and infecting the caller's `$effect`. The fix seals that leak. The hook's public contract (load a file, expose `fileUrl` / `isLoading` / `fileError`) remains unchanged — callers read `fileUrl` via the getter and that read *is* correctly tracked. Only the internal read inside `loadFile` is now shielded. **No layer boundary violations**: The hook stays in `$lib/hooks/`, dependencies unchanged, no new cross-module coupling introduced. **`untrack` as a design tool**: Using `untrack` for internal bookkeeping reads is a well-established Svelte 5 pattern (the Svelte docs explicitly recommend it for "previous value" tracking inside `$effect`). This is boring, correct usage — exactly what I want to see. **Risk of the change**: Near-zero. The `prev` variable is populated with the current `fileUrl` value before any mutation. The revoke-and-reset logic is identical to before, just protected from reactive tracking. No behavioural change under any load path.

marcel commented

2026-04-15 22:46:30 +02:00

🧪 Sara Holt — QA Engineer

Verdict: ⚠️ Approved with concerns

Summary

The fix itself is correct and the existing unit tests still pass. My concern is that the test suite cannot actually prevent this class of bug from regressing — and we're approving without closing that gap.

What I checked

Existing test coverage (useFileLoader.svelte.test.ts):

✅ Happy path: fileUrl is set after a successful fetch
✅ Error path: fileError is set on failure
✅ Revoke on reload: previous blob URL is revoked before a new one is created
✅ Destroy: revokeObjectURL is called in destroy()
✅ No revoke on empty: revokeObjectURL is not called if fileUrl is empty

These are all unit-level tests that call loadFile directly in an async context — not inside a Svelte $effect. The infinite loop only manifests when the hook is used inside a reactive tracking context. The existing tests would have passed even with the broken code.

Concern (non-blocking for merge, but flagged)

There is no component-level or integration-level test that exercises the $effect → loadFile → fileUrl change → $effect re-runs? cycle. If someone refactors loadFile and accidentally removes the untrack call (or introduces a new synchronous $state read before the first await), the tests won't catch it.

A minimal regression guard could be a @testing-library/svelte component test that mounts a component using createFileLoader inside an $effect, calls loadFile, and asserts it was called exactly once — not in a loop. This is harder to write than a unit test but would be the only real safety net for this bug class.

I'm not blocking merge since the fix is correct and the inline comment documents the intent. But I'd like to see a follow-up issue opened to track adding that component-level test.

## 🧪 Sara Holt — QA Engineer **Verdict: ⚠️ Approved with concerns** ### Summary The fix itself is correct and the existing unit tests still pass. My concern is that the test suite cannot actually prevent this class of bug from regressing — and we're approving without closing that gap. ### What I checked **Existing test coverage** (`useFileLoader.svelte.test.ts`): - ✅ Happy path: `fileUrl` is set after a successful fetch - ✅ Error path: `fileError` is set on failure - ✅ Revoke on reload: previous blob URL is revoked before a new one is created - ✅ Destroy: `revokeObjectURL` is called in `destroy()` - ✅ No revoke on empty: `revokeObjectURL` is not called if `fileUrl` is empty These are all unit-level tests that call `loadFile` directly in an `async` context — not inside a Svelte `$effect`. **The infinite loop only manifests when the hook is used inside a reactive tracking context.** The existing tests would have passed even with the broken code. ### Concern (non-blocking for merge, but flagged) There is no component-level or integration-level test that exercises the `$effect → loadFile → fileUrl change → $effect re-runs?` cycle. If someone refactors `loadFile` and accidentally removes the `untrack` call (or introduces a new synchronous `$state` read before the first `await`), the tests won't catch it. A minimal regression guard could be a `@testing-library/svelte` component test that mounts a component using `createFileLoader` inside an `$effect`, calls `loadFile`, and asserts it was called exactly once — not in a loop. This is harder to write than a unit test but would be the only real safety net for this bug class. I'm not blocking merge since the fix is correct and the inline comment documents the intent. But I'd like to see a follow-up issue opened to track adding that component-level test.

marcel commented

2026-04-15 22:46:43 +02:00

🔒 Nora "NullX" Steiner — Security Engineer

Verdict: ✅ Approved

Summary

No security surface changes. LGTM.

What I checked

Blob URL lifecycle: The revoke-before-replace pattern is preserved. URL.revokeObjectURL(prev) is called on the previous URL before creating a new one, then fileUrl = '' before the async fetch begins. This means there is no window where an old blob URL remains accessible after the new one is loaded. Memory leak risk is unchanged (unchanged for the better — the blob lifecycle management is correct).

Network requests: fetch(url) is unchanged. The url parameter is still passed in from the caller (in +page.svelte, it's a fixed API path /api/documents/${doc.id}/file). No user-controlled URL concatenation at the hook level.

No new dependencies: untrack is a Svelte built-in export from the svelte package. No third-party packages introduced, no supply-chain risk.

No new state exposure: The hook's public API (three read-only getters + loadFile + destroy) is unchanged. No internal state is newly exposed.

## 🔒 Nora "NullX" Steiner — Security Engineer **Verdict: ✅ Approved** ### Summary No security surface changes. LGTM. ### What I checked **Blob URL lifecycle**: The revoke-before-replace pattern is preserved. `URL.revokeObjectURL(prev)` is called on the previous URL before creating a new one, then `fileUrl = ''` before the async fetch begins. This means there is no window where an old blob URL remains accessible after the new one is loaded. Memory leak risk is unchanged (unchanged for the better — the blob lifecycle management is correct). **Network requests**: `fetch(url)` is unchanged. The `url` parameter is still passed in from the caller (in `+page.svelte`, it's a fixed API path `/api/documents/${doc.id}/file`). No user-controlled URL concatenation at the hook level. **No new dependencies**: `untrack` is a Svelte built-in export from the `svelte` package. No third-party packages introduced, no supply-chain risk. **No new state exposure**: The hook's public API (three read-only getters + `loadFile` + `destroy`) is unchanged. No internal state is newly exposed.

marcel commented

2026-04-15 22:46:54 +02:00

🎨 Leonie Voss — UI/UX Expert

Verdict: ✅ Approved

Summary

This is a pure bug fix with no UI changes — but the UX impact is significant. The infinite reload loop was causing the document page to flash and reload continuously, which is one of the worst possible user experiences. This fix restores a stable, usable document viewer.

What I checked

UX regression fixed: The infinite loop manifested as the document page continuously refreshing — the PDF viewer would flash, the loading spinner would reappear, and any user interaction (scrolling, annotating) would be interrupted. This was a showstopper for usability on the document detail page. The fix resolves it completely.

No visual regressions: The change is confined to useFileLoader.svelte.ts and touches only internal reactive tracking logic. The rendered output — the PDF viewer, loading states, error states — is identical before and after.

Loading state UX is preserved: isLoading is still set to true at the start of loadFile and false in finally, so the loading spinner in DocumentViewer still shows correctly during the fetch. Users get the expected feedback while the file loads.

No concerns from a UI/UX perspective. Ship it.

## 🎨 Leonie Voss — UI/UX Expert **Verdict: ✅ Approved** ### Summary This is a pure bug fix with no UI changes — but the UX impact is significant. The infinite reload loop was causing the document page to flash and reload continuously, which is one of the worst possible user experiences. This fix restores a stable, usable document viewer. ### What I checked **UX regression fixed**: The infinite loop manifested as the document page continuously refreshing — the PDF viewer would flash, the loading spinner would reappear, and any user interaction (scrolling, annotating) would be interrupted. This was a showstopper for usability on the document detail page. The fix resolves it completely. **No visual regressions**: The change is confined to `useFileLoader.svelte.ts` and touches only internal reactive tracking logic. The rendered output — the PDF viewer, loading states, error states — is identical before and after. **Loading state UX is preserved**: `isLoading` is still set to `true` at the start of `loadFile` and `false` in `finally`, so the loading spinner in `DocumentViewer` still shows correctly during the fetch. Users get the expected feedback while the file loads. No concerns from a UI/UX perspective. Ship it.

marcel commented

2026-04-15 22:47:03 +02:00

🚀 Tobias Wendt — DevOps Engineer

Verdict: ✅ Approved

Summary

No infrastructure, build, or deployment concerns. This change is frontend-only and has no operational surface area.

What I checked

No new dependencies: untrack is re-exported from the svelte package which is already a direct dependency. No package.json changes, no lockfile changes, no new supply-chain exposure.

Build pipeline: The change is a single .ts file in the frontend. The existing npm run build, npm run check, and npm run test pipeline steps are unaffected. The fix was already validated locally with npx vitest run useFileLoader.

No environment or config changes: No .env variables, no Docker Compose changes, no Flyway migrations, no Spring config changes.

Deployment risk: Near-zero. This is a frontend-only fix that resolves a client-side infinite loop. No backend restart required, no database changes, no migration window needed. Standard deploy applies.

## 🚀 Tobias Wendt — DevOps Engineer **Verdict: ✅ Approved** ### Summary No infrastructure, build, or deployment concerns. This change is frontend-only and has no operational surface area. ### What I checked **No new dependencies**: `untrack` is re-exported from the `svelte` package which is already a direct dependency. No `package.json` changes, no lockfile changes, no new supply-chain exposure. **Build pipeline**: The change is a single `.ts` file in the frontend. The existing `npm run build`, `npm run check`, and `npm run test` pipeline steps are unaffected. The fix was already validated locally with `npx vitest run useFileLoader`. **No environment or config changes**: No `.env` variables, no Docker Compose changes, no Flyway migrations, no Spring config changes. **Deployment risk**: Near-zero. This is a frontend-only fix that resolves a client-side infinite loop. No backend restart required, no database changes, no migration window needed. Standard deploy applies.

marcel merged commit ed12a54339 into main

2026-04-16 09:09:40 +02:00

marcel deleted branch fix/file-loader-reactive-loop

2026-04-16 09:09:41 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#243