Persist RAW-to-TIFF conversions across reloads (#243) - #249
Merged
Conversation
"The deployed site feels slower than the local build" is not a question inspection can answer: the static export and `out/` are the same bytes, so any difference has to be delivery, and sizing delivery against compute needs measurement. Measured on the CR2 fixture bracket, Chromium both sides: a full JPEG generate takes 26,963 ms local against 31,973 ms deployed, and a 3-frame RAW import 5968 ms against 5959 ms. The deployed penalty is a flat ~5 s of one-time tool fetches, not a multiplier -- RAW conversion shows none at all. Requests are counted through `page.on(...)` rather than Resource Timing, because the wasm is fetched from inside a Worker and a worker's entries never land on the page's performance timeline. The CR2 mode waits on the `<canvas>` each thumbnail appends, not on the container div. Mounting is not gated on conversion, so an earlier draft reported a 269 ms import of ten 21.7 MB frames with zero `/wasm/` requests -- it exited before conversion began. `pipeline.spec.ts` documents the same trap for the responsiveness test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes the gap #243 describes: the RAW-to-TIFF cache lives in memory and dies with the tab, so a reload reconverts every frame -- ~19 s for a 10-frame bracket, and a regression for desktop, which kept this on disk before the WebAssembly port. A persistent tier goes behind the session tier, inside the RAW worker. `createSyncAccessHandle()` is worker-only, and the worker already holds the source bytes, so it can hash them without a second read and the content hash never crosses back to the page. The `tiffFor` seam needs no change. Two design points beyond the issue. The key folds in a tool tag derived from the `dcraw_emu` commit and the `dcrawArgs` flags: without it, rebuilding the wasm would silently serve pixels from a different demosaic, undoing the byte-identical guarantee `raw-preview.ts` exists to hold. And the key must be a content hash rather than a path, because `registerSessionFile` mints `/session/${nextId()}/${name}` from a counter that restarts each session -- so the same string names different bytes across sessions. The OPFS-versus-IndexedDB choice is left to a probe rather than assumed. OPFS is used nowhere in the codebase today and has never been tested in the three Tauri webviews. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ten tasks, probe first. The backend choice is deferred to Task 1's result rather than assumed, with the decision rule stated in advance so the outcome is not rationalised after the fact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The plan chose createWritable over createSyncAccessHandle by reasoning. The probe now times both per host and the choice follows a stated rule, since the sync handle is also the spec's reason for putting this tier in a worker -- a rationale that only holds if the sync path wins. Worker placement stands either way: the worker already holds the bytes, and hashing 22 MB on the main thread would jank the UI the RAW worker exists to protect. The probe now fails loudly where OPFS is absent rather than passing on a vacuous assertion, and the OPFS store gains a test for its use of the API -- in particular that the writable is closed on the failure path, which is what otherwise leaves a zero-length file that reads as a hit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#243 assumes OPFS works in the three Tauri webviews and in Safari. Nothing in the codebase uses OPFS today, so that was an assumption rather than a finding. This records what each engine actually does with a converted-frame-sized blob.
Repo-wide `npm run check` was failing on this file from 7949a51, which made every later task's lint gate ambiguous.
…bKit result A single 67 MB write() failing on WebKit could mean the call shape, not the backend, so the probe now also writes the same blob as 8 MB slices through createWritable and createSyncAccessHandle, on both write paths. Chromium round-trips cleanly at every combination. Chasing the WebKit chunked result led to a 4-byte control write failing identically on the same host, which means both the new chunked number and the single-shot one from the previous commit are confounded by host memory pressure rather than evidence about WebKit's OPFS implementation -- corrected in the design doc rather than left to read as a finding.
Task 1 review returned five findings. The design doc claimed the brief's backend rule doesn't name WebKit -- it does, first in the list -- so the correct framing is that the row's evidence is invalid (host memory pressure, proven by a failing control write), not that the rule is exempt; corrected in place along with an opfsRoundTrips=false cell that read as corruption rather than "never compared," and two dangling references to sections that only exist in the brief. The WebdriverIO port passed an async callback to browser.execute, which classic WebDriver's execute-sync endpoint does not await -- silently producing no measurement on any driver that isn't BiDi-negotiated. Restructured to the suite's own convention: a synchronous callback launches the probe as a page-side IIFE that reports through a window global, polled with browser.waitUntil. The control write that caught the WebKit confound last round is now a permanent first step in both probes, asserted before the OPFS-specific checks so a thrashing host reports itself as inconclusive rather than as an OPFS verdict. WebKit was not rerun on this machine, which is still under memory pressure; its row is marked accordingly rather than re-measured.
presets.ts imports a React config provider, so a worker importing it for one hash function would pull React into the worker bundle.
The RAW cache index is written by the worker on every conversion and cleared from the settings page. Two transactions leave a window where one loses the other's update, and a lost index entry is a blob nothing will evict.
Content-addressed with an LRU bound. Storage is a seam rather than OPFS directly: the eviction and index logic is the part worth testing, and navigator.storage does not exist under Jest.
Two stores can disagree. A phantom index entry self-heals into a miss on read; an orphaned blob is invisible to eviction and would consume disk forever, so it is swept once per session. Also strengthens "never evicts the entry just added": the original two-entry version passed even with the just-added guard removed, since evicting the older entry alone already satisfied budget before the loop could reach the new one. The new version re-grows an already-present key so its lastUsed ties with a later entry's, making it the first eviction candidate by stable-sort order unless the guard excludes it -- verified failing with the guard removed and passing with it restored.
The content hash is required for correctness in a browser, where session paths restart from a counter and name different bytes across visits. The tool tag stops a rebuilt dcraw_emu from silently serving the previous demosaic.
… commit Adds a test that mocks dcrawArgs so a future edit that drops it from the tag's hash (recreating the truncated-template-literal defect this module already fixed once) fails a test instead of passing silently. Also stops toolTag from substituting an "unknown" placeholder when versions.json omits the recorded commit -- two builds that both fail to report one would otherwise collide on the same tag and share a cache entry, each serving the other's pixels. Throwing instead is safe: the caller degrades to converting without a persistent-cache hit or write for the session, per the try/catch task 8 wraps key derivation in.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
navigator.storage.getDirectory is absent in WebKit and in WebKitGTK 605.1.15, the webview Tauri uses on Linux -- not slow, not quota-limited, absent. An OPFS-backed cache would have silently never worked for Safari users or Linux desktop users. IndexedDB round-tripped 67 MB on every engine tested. The decision rule was written before the evidence and is applied as written: OPFS failing on any engine sends this to approach B. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#243 specified OPFS. The probe found navigator.storage.getDirectory absent in WebKit and in WebKitGTK 605.1.15, the webview Tauri uses on Linux -- not slow, not quota-limited, absent. An OPFS cache would have silently never worked for Safari or Linux desktop users. IndexedDB round-tripped 67 MB everywhere.
The cache is consulted before converting and populated after, always before the caller transfers the buffer: postMessage detaches it, and a write after that persists a zero-byte file. Every cache failure falls through to conversion, so the cache can never be why a frame fails.
…e in the IndexedDB store Round-1 review on the blob store found two gaps. The view-vs-slice test compared read-back values, which cannot distinguish a stored slice from a stored view -- IndexedDB's structured clone preserves a view's offset and length either way, so the buggy and correct implementations read back identically. Rewritten to check the stored record's byteLength instead. Separately, DATABASE_VERSION had never changed before this task, so onblocked (fired when another tab holds an older version open) and a cached rejected connection promise were both unreachable in practice. Bumping the version made them reachable: a web user with two tabs open across a deploy could wedge storage -- presets and settings, not just this cache -- until reload. Added onversionchange handling so an old tab yields its connection, and stopped the connection cache from pinning a failure.
A 2 GB cache that a user cannot see or reclaim short of clearing site data is not an honest default. Hidden entirely where there is no IndexedDB, since zero would claim an empty cache rather than no cache.
…t, and make the view-vs-slice test discriminate for real Round-2 review found F1 still open and a Critical regression (F4) from round 1's F2 fix. onversionchange closed the database but left kv.ts's module-level connection promise resolved to the closed handle, so every later getDocument/putDocument in that tab would fail with InvalidStateError permanently. app-storage.ts's readJson swallows read errors, so this would render as an empty app -- no presets, no settings, no run history -- rather than a visible error. Fixed by clearing connection before close(), with a test proving a call after the version change recovers by reopening. F1's checked stored?.byteLength === 3, which a stored *view* also satisfies (a view's byteLength reflects its own length, not its backing buffer's). Rewritten to check the stored record is a plain ArrayBuffer, using toString/isView rather than instanceof -- fake-indexeddb clones values into a different realm, so instanceof ArrayBuffer is unreliable here even for a correct record.
clear() removed each blob, swallowed every per-key failure, and then emptied the index unconditionally. A removal that failed left its blob on disk while usage() -- which only reads the index -- reported 0 B and the settings page toasted success. The Clear button existed to guarantee disk was reclaimed; this let it lie about the one thing it was for. Now clear() keeps in the index exactly the entries whose blobs could not be removed, so usage() still reflects what's really on disk, and it throws naming how many entries failed so the caller can report honestly instead of celebrating a partial no-op. page.tsx re-reads usage() in a finally so a partial failure doesn't leave a stale number next to the error toast, and no longer disables Clear on cacheBytes === 0 -- that reads indexed usage only, so a cache holding nothing but sweep-eligible orphans showed 0 B with Clear disabled and no way to reclaim them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#243's acceptance criterion as a number rather than a claim: same frames, new tab, empty session tier, so any saving is the persistent tier's.
The reload result stands on a real control: with the pre-cache worker, the second import is as slow as the first. What that control cannot settle is why the first import runs above the 5968 ms baseline measured earlier the same day, because it is a single sample and happens to be the fastest run recorded. Attributing the gap to environment slowness was stated more confidently than one data point supports, so it is now an open question with the run that would close it named. Also corrects the benchmark's comment: it is a reload, not a new tab. The reason the measurement is valid is that a reload discards the JS realm, so the session cache and the RAW worker both start fresh. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three Important findings, addressed together since they touch shared files (kv.ts, init.tsx, the design doc): F1 -- a version downgrade (rolled-back deploy, stale HTTP cache, an older desktop reinstall) made kv.ts's open() fail with VersionError, which app-storage.ts's readJson swallowed into the empty-state fallback -- rendering as "you have no data" with the real data intact and no error anywhere. kv.ts now names this DatabaseVersionError; readJson rethrows it instead of falling back; init.tsx probes for it once at startup (mounted app-wide via the root layout) and shows a persistent toast. F2 -- the 2 GB RAW cache budget was never checked against the origin's real navigator.storage.estimate() quota, so on a smaller-quota host the eviction loop never triggered and writes just started failing once the real limit hit, wedging the cache permanently. raw-cache.ts now clamps the effective budget to min(2 GB, 50% of quota) via an injected estimator (raw-cache-quota.ts, kept out of raw-cache.ts itself so it stays testable under Jest), and a write failure now evicts LRU entries -- unconditionally, not gated on the same budget that just failed to predict the failure -- and retries once before giving up. Settings reports the effective budget, not the nominal one. navigator.storage.persist() is called once, best-effort, from both the worker and the page. F3 -- the cache key's tool tag hashed only the dcraw_emu commit, so a rebuild from the same LibRaw commit on a bumped Emscripten toolchain (what #244 automates) would produce an identical key over potentially different bytes, serving a stale TIFF as a valid hit. The tag now folds in versions.json's top-level emscripten field, symmetric with the existing missing-commit guard. Design doc updated to match: the cache key formula, the no-longer-fixed budget, the retry-on-write-failure, and the Settings effective-budget figure. Tests for all three, each verified to fail against the pre-fix code and pass after restoring -- including a targeted mutation on raw-cache.ts's eviction-on-failure path (temporarily making it budget-gated) to confirm the specific test that pins the unconditional-eviction behavior, since a budget-gated version also happens to pass the more obvious retry test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
adulbrich
marked this pull request as ready for review
August 1, 2026 03:05
storage-probe.spec.ts and its WebdriverIO port existed to pick between OPFS and IndexedDB for the persistent RAW cache. They found navigator.storage.getDirectory absent in WebKit and in WebKitGTK 605.1.15 (both opfsAvailable: false in CI), so the cache was built on IndexedDB (raw-cache-idb.ts). The probes kept asserting opfsAvailable === true anyway, which meant a decision the probe itself made permanently red-lines CI on the two engines it made the decision about -- OPFS is not a real dependency of the app that shipped. Both specs now assert idbRoundTrips/idbError, the app's actual dependency, unconditionally. opfsAvailable is recorded and printed but no longer asserted. Where OPFS is present, the control-write check still gates the rest of the OPFS numbers as before (a 4-byte failure under host memory pressure reads as inconclusive, not a negative finding), and where the control write succeeds and OPFS reports no error, the round-trip is still asserted -- an engine that claims OPFS and then corrupts data is still a build-breaking finding. Only OPFS's absence stopped being fatal. Design doc's "Probe results" section records why, alongside the existing WebKit-memory-pressure correction it doesn't contradict: CI's WebKit/WebKitGTK report the API absent; the macOS Playwright row already in the table reports it present but confounded by a loaded host, which is a different thing and still surfaces as inconclusive. Verified locally: chromium passes (idbRoundTrips: true, opfsRoundTrips: true). webkit fails on the control-write assertion here twice in a row with opfsAvailable: true, controlOk: false, under ~81 MB free of 16 GB (memory_pressure) -- the same confound already on record for this Playwright/macOS host, not a regression. CI's WebKit reports opfsAvailable: false, so that branch never runs there and IndexedDB carries the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…already had The "Probe results" section's WebKitGTK table row still read "pending CI" and the "Still needed" paragraph still said the WDIO port "has not run anywhere" -- both stale against the same CI run named in the prior commit's own probe-repoint rationale (WebKitGTK 605.1.15, opfsAvailable: false, per eb0aec8). Left uncorrected this would have reproduced the exact table-vs-prose contradiction that commit just fixed for WebKit, one paragraph down. Table row and "Still needed" now say what actually ran: WebKitGTK on the Ubuntu runner, WKWebView and WebView2 still outstanding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 1, 2026
Collaborator
Author
Known minors, parked deliberatelyThe whole-branch review triaged all of these as shippable — none can produce a wrong result. Recording them here rather than in a scratch file so they survive, and so a reviewer can disagree with any of them. Worth a follow-up commit
Correct behaviour, imprecise documentation
Narrow windows that self-heal
Test-harness limitations, documented in place
Related issues filed or updated from this work
|
adulbrich
added a commit
that referenced
this pull request
Aug 5, 2026
…ucceeds (#253) `run()` resolved on `request.onsuccess`. In IndexedDB a request succeeding and its transaction committing are separate events, in that order, so a transaction that aborts at commit time -- quota exhaustion being the realistic cause -- fired `onabort` after the promise had already settled, where `reject()` is a no-op. The comment above it claimed to handle exactly the case it did not. Every write to this database goes through `run()`: `putDocument`, `putFile`, `putBlob`, `deleteDocument`, `deleteFile`, `deleteBlob`. So a quota-aborted save of a preset reported success and lost the data, and because `readJson` in `app-storage.ts` swallows read errors into a fallback, the loss surfaced later as a missing preset rather than as an error at save time. Resolving on `transaction.oncomplete` is the durable signal, which is what `updateDocument` already does. The result now has to be captured in `onsuccess` and handed over at commit, since `request.result` is only valid inside its own handler. Reads share `run()`, so they settle a tick later too; every action it issues is a single request, so no caller is left holding a transaction that has since committed. #249 makes this worth fixing now rather than later: it added a RAW conversion cache of up to 2 GB to the same origin, and it writes through `putBlob`, so it both raises quota pressure and is subject to the same false success. Tests cover the abort paths, which had none. Closes #250 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
adulbrich
added a commit
that referenced
this pull request
Aug 5, 2026
An audit of PRD.md against the tree, prompted by the route rename. Every path the document cites was checked for existence; four claims had drifted. - Section 3 was headed "Image Generator (Home Page)". The route is `/pipeline` and "Home Page" now names nothing in the codebase. - Section 8 pointed at `src-tauri/src/pipeline.rs` and `src-tauri/src/pipeline/header_editing.rs` for the evalglare ordering fix. The WebAssembly port (#227) deleted both. The entry now says where the fix landed, that the files are gone, and that `orchestrator.ts` carries the ordering today. - Section 9 listed "the RAW-to-TIFF cache is per-session" as a browser limitation and said an OPFS-backed cache "has not been built". #249 built one, on IndexedDB rather than OPFS, because the storage probe found `getDirectory` absent in WebKit and WebKitGTK. Section 4 described the same cache as "in-session". - Section 6 did not mention the RAW cache read-out or its clear control. Added along the way: the RAW worker and conversion cancellation in section 3, the cache's design in section 7, and the one limitation #249 left behind, that a cache hit is still resolved inside the worker and so waits out the queue ahead of it. `src/app/settings/page.tsx` claimed in its docstring that settings save "via Tauri API calls"; they go through Zustand to `localStorage`, which is what lets one code path serve both hosts. The same docstring advertised a user experience level and a debug console; neither exists, the former surviving only as an unused local. Co-Authored-By: Claude Opus 5 (1M context) <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.
Closes #243.
RAW conversion is the most expensive thing this app does — about 2 s per 21.7 MB frame — and it was pure recomputation: the cache died with the tab, so reloading reconverted the whole bracket. The desktop build kept this on disk before the WebAssembly port and lost it, so this was a regression there as well as a gap in the browser.
Result
The control run rebuilds
raw-worker.tsfrom its pre-cache parent and shows the second import staying exactly as slow. That is what makes this causal rather than correlational — the saving is the cache, not warm-up.Measured by
e2e-web/tests/perf.bench.ts, added here.Design
A persistent tier behind the existing in-memory session tier, inside the RAW worker. The worker already holds the source bytes, so it hashes them without a second read and the content hash never crosses back to the page.
The key is
sha256(bytes) + "-" + toolTag, and both halves are correctness rather than optimisation:registerSessionFilemints/session/<n>/<name>from a counter that restarts each session, so the same path names different bytes across visits. A path-keyed cache would serve the wrong image.dcraw_emucommit, the Emscripten version and thedcrawArgsflags. Without it, rebuilding the artifacts — which CI job to rebuild the WebAssembly bundle from pinned sources #244 automates — would serve pixels from a different demosaic while reporting success, breaking the byte-identical guaranteeraw-preview.tsexists to hold.IndexedDB, not OPFS — measured, not assumed
#243 specified OPFS for its
createSyncAccessHandlefast path.e2e-web/tests/storage-probe.spec.tswas written first to check that assumption, and CI foundnavigator.storage.getDirectoryabsent in WebKit and in WebKitGTK 605.1.15, the webview Tauri uses on Linux:Not slow, not quota-limited — not implemented. An OPFS cache would have silently never worked for Safari users or Linux desktop users. IndexedDB round-tripped 67 MB on every engine tested, and already backs presets, settings and run history in production.
The probe performs a 4-byte control write first and reports a run inconclusive if that fails, because an earlier local failure that looked like a WebKit OPFS limit turned out to be a memory-starved host. That distinction is now permanent in the spec.
Storage safety
This cache shares
hdri-calibrationwith users' presets, settings and run history, andapp-storage.tsswallows read errors into fallbacks — so storage failures render as "you have no data" rather than as errors. Three consequences were found in review and fixed here:DATABASE_VERSION1→2 made downgrade possible. A rolled-back deploy, a cached bundle or an older Tauri reinstall opened at v1 against v2, threwVersionError, and showed an empty app with no error. Now surfaced as a distinguishableDatabaseVersionErrorthe user actually sees.onversionchangeclosed the database without clearing the cached connection, so a tab left open across a deploy failed every later storage call permanently.navigator.storage.estimate(),persist()is requested, and a write failure triggers evict-and-retry.Also
postMessagetransfers the buffer. A transfer detaches it, and caching after would persist a zero-byte blob that later reads as a successful hit — the failure class fixed in93ba5fc.Review
384 tests, 57 suites. Every task reviewed, then a whole-branch review, then a fix wave, then a scoped re-review that verified each fix by mutation rather than by trusting the report. Design and plan are in
docs/superpowers/specs/anddocs/superpowers/plans/.Known limitation, stated rather than hidden: the first import ran above the 5,968 ms baseline measured earlier the same day. A single cache-off control also ran above it, which is consistent with environment slowness, but n=1 cannot rule out a first-import write cost. The design doc names the run that would settle it.
Unrelated follow-ups found on the way:
kv.ts'srun()resolves onrequest.onsuccess, so a commit-time quota abort cannot reject — preset and settings writes can report success for a write that never committed. And #245's Windows hdrgen hang did not reproduce here (End-to-end (windows-latest)passed in 7m3s), which is worth revisiting on that issue.🤖 Generated with Claude Code