fix(traces): retain, filter and report storage honestly - #50
Merged
Merged
Conversation
- Four places swallowed an IndexedDB failure and fell back to memory without a word, and each one produced a different false statement. An unreadable history looked like an empty one. Rows that were never written looked retained, with a counter that had not moved. A delete that failed reported success — the rows were still in storage and came back on the next reload, which is the resurrection #48 fixed arriving by another route. - `loadOlder` had no handler at all: the rejection escaped and the spinner turned forever. A control claiming work is in progress that has already failed is worse than a silent one. - Report the failing operation and the `DOMException.name` verbatim — `QuotaExceededError` tells the operator to clear history, and turning the rest into prose would invent a cause we do not know. Storage gets its own badge rather than sharing the malformed-frame one: the platform misbehaving and this browser refusing to store are different problems and point at different fixes. - Reported on the first failure, unlike a malformed frame's three. A dropped frame is one sample of many arriving every second; an unwritten row is data the user believes is saved. - Only the operation that failed lifts its own mark. Clearing on any success looked equivalent and was not: the store seeds itself from storage on connect, that read all but always succeeds, and it landed in the same tick as a failed write — recording the failure and erasing it before anything could show it. - A failed delete no longer empties the view. A clear that did not happen has to look like a clear that did not happen. Tags: #indexeddb #traces Co-Authored-By: htjulia <htjulia1@gmail.com>
- The retention limit was never enforced on the way in: rows were inserted and the pruner ran on a count that never came back down, so a full store meant the browser refused every later batch for the same reason. The failure kept itself alive. Eviction now runs on every batch, and a refused insert frees room and retries once. - The eviction count is driven by how many rows the insert actually added, measured across it. `fresh.length` is the whole re-persisted live window (up to 500) and nearly all of it is already stored, so at factor 1 a single overflowing row would have evicted hundreds. - The four values worth choosing are now settings, each in its own `pc:` key and validated on the way out of storage as well as in — a hand-edited entry otherwise reaches the pruner as NaN and retention silently stops being enforced. - The theme button becomes an options button with the theme toggle and settings inside it. Dialog, dropdown-menu, field and toggle-group come from the shadcn base-nova registry rather than hand-rolled equivalents. test: cover retention, quota retry and settings validation - Scenario tests drive eviction against a real IndexedDB at a small limit; the shipped 50,000 is asserted separately so the two cannot drift. Every new guard was verified by reverting it: 4, 1, 3, 2 and 3 tests fail respectively. - jsdom 30 as vitest builds it defines the `localStorage` getter but never creates the object, so every settings read fell into `readStored`'s catch and read back as defaults. The setup file now installs a working Storage. - `does not report a deletion that did not happen` snapshotted the store mid-write and failed about once in a dozen gate runs. It now waits for the store and IndexedDB to agree first — 10 consecutive clean runs of the failing sequence. Tags: #retention #settings #shadcn #scenario-tests Co-Authored-By: htjulia <htjulia1@gmail.com>
…otes
- The logic under these screens was covered and the screens were not.
`settings.test.ts` proved `validate()` rejects 49,999; nothing proved
the dialog shows that rejection, refuses the save, or leaves storage
alone on Cancel. `storage-failure.test.ts` proved the store records a
refused write; nothing proved the table paints it. A field nobody
paints is as silent as the swallowed exception it replaced.
- Each new group was verified by reverting the thing it guards: an
unguarded Save catches 3, an unrendered error catches 1, a Cancel
that saves catches 3, a dead Theme item 1, a dead Settings item 3,
a dropped theme label 2, an unpainted storage note 4, a failed
clear that empties the view 2, and collapsing the three storage
sentences into one catches 4.
- Driven with `fireEvent` rather than adding `user-event` as a
dependency; it fires the same handlers the browser does, which is
the path checked by hand against the running console.
- Two measurements shaped these tests. Testing Library registers no
auto-cleanup without vitest `globals`, so a portalled dialog
survived into the next test and every query found two of
everything — cleanup is explicit. And the trace table body is
virtualized, so jsdom paints no rows: counting `tbody tr` compared
0 to 0 and passed vacuously, so the assertions read the header's
own "N shown · M retained" instead.
refactor(settings): drop a guard that guarded nothing
- `{open && <SettingsForm/>}` claimed to be what reset the draft.
Measured: removing it changed no test and no rendered output, because
DialogContent's portal already renders nothing while closed. The
comment now says where the reset actually comes from and which test
pins it.
Tags: #ui-tests #revert-verification #settings
Co-Authored-By: htjulia <htjulia1@gmail.com>
…nst it - The retry after a refused write rested on an assumption no test could reach: that IndexedDB can still delete when it is full. Every test of that path mocked our own `upsertTraces` to reject, which says what the store does with a rejection and nothing about whether the eviction it then attempts can commit. `fake-indexeddb` implements no quota, so the question stayed open. - `src/test/idb-quota.ts` enforces a byte budget inside `ObjectStore.storeRecord` — the operation fake-indexeddb runs within the request, which is where a browser's quota check lives. Throwing there fails the request, hands the transaction that error and aborts it: the same three events in the same order as a real QuotaExceededError. Accounting lives in the same functions, so an aborted transaction's rollback keeps the budget honest by replaying through them. - Seven scenarios with nothing mocked: the store fills, a write is refused, cursor deletes over `byEpoch` commit while full, and the retry lands. Reverting the retry catches 3, dropping the `freed === 0` guard catches 5, and making eviction free nothing catches 3. fix(traces): observe the abort a refused write causes - A refused write rejects the request; the browser then aborts the transaction. The caller learns it from the request and never reaches `await tx.done`, so that promise rejected with an AbortError nobody was waiting for and the page logged an unhandled rejection for a failure already handled and reported. `idb` creates it eagerly, so not touching it does not help. - Only reachable once a real quota abort was possible: without the guard the suite raises 16 unhandled AbortErrors and vitest exits 1. test(storage): pin a boundary the quota work uncovered - Eviction buys room for the batch, and the batch is the whole live window re-persisted. When that window does not fit in the quota at all there is nothing to free that is not written straight back, and the retry fails exactly as the first attempt did. The console's answer is right — it says storage is refusing and keeps showing the traces — but the state now has a test instead of waiting to be found in a browser. Tags: #quota #indexeddb #scenario-tests #revert-verification Co-Authored-By: htjulia <htjulia1@gmail.com>
- Filtering ran against the loaded page and leaned on infinite scroll to widen it, and all three consequences were wrong. The scroll trigger compares against the *filtered* count, so a filter matching almost nothing satisfied it forever: measured in a browser on a 47,320-row store, pressing "Errors only" issued 189 IndexedDB page reads in three seconds and blinked the control between "Load older" and "loading…", on its way to paging the entire store into memory. It made the operator press a button until matches appeared — the two 404s in that store sat 4,381 rows down, 22 presses. And until they did, the panel said "No traces match", which was false. - The store now answers a filter with one cursor scan over `byEpoch`, stopping early once a page of matches is collected, and reports one searching state while it runs. Same store, same click: 1 read, one spinner, both 404s in about a second, nothing pressed. - The result is held together with the filter it answers, so the view is derived during render — "still searching" is just "filtering, and no result for *this* filter yet". That covers the debounce, an in-flight scan and a filter changed mid-scan with no state to reset, which is the rule this project already follows elsewhere. - The filter rule itself moves to `lib/trace-filter.ts` because two callers must agree on it: the scan, and the render-time filter that keeps a newly arrived match visible — a scan cannot know about traces that did not exist when it ran. - Empty state now says "Searched all N retained traces", which is true. fix(traces): keep the infinite-scroll trigger from running away - Gated on the list actually overflowing its box. Overflow is what proximity was assuming all along: only a list taller than its viewport can be scrolled to the end. Extracted as `shouldLoadOlder()` because the body is virtualized and jsdom has no layout, so DOM-level assertions here pass for the wrong reason. fix(traces): stop overstating what is still retained after a failed write - The count moved only on success, which made it a lie in the one situation that matters: the retry evicts before it writes, so a batch that ends in failure has still deleted rows. Measured under a refused write — the header said 49,985 retained while the store held 48,212, and overstated further with every batch. test: 25 new cases, each verified by reverting what it guards - Filtering on the loaded page catches 7, dropping the live merge 1, merging unfiltered 8, the stale-scan guard 2, lying about an early exit 2, treating a blank query as active 1, the null-field guard 1, the overflow gate 3, the itemCount gate 1, the proximity window 1. - Two guards caught nothing at first: the stale-scan guard, because changing the filter inside the debounce means the first scan never starts, and the null-field guard, because no assertion searched for the text "null". Both now have a case that reaches them. Tags: #filter #indexeddb #revert-verification Co-Authored-By: htjulia <htjulia1@gmail.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.
Five defects, all of which made the console state something that was not true, plus the
test scaffolding that made two of them findable at all. Squash merge loses the commit
bodies, so the reasoning is here.
Where to look hardest
src/hooks/use-trace-store.tscarries four of the five changes and is the file toreview closely. Two of them touch the same
catchblock.src/test/idb-quota.tspatches
fake-indexeddb's internals — read the header comment before judging theapproach; the obvious seam (
IDBObjectStore.put) was tried first and was wrong.1. The retention limit did not retain
Once the store held 50,000 rows, eviction ran after the insert. A write refused for
quota therefore skipped eviction, freed nothing, and the next batch failed for the same
reason — the failure kept itself alive and nothing was ever saved again. Eviction now
runs on every batch, and a refused insert frees room and retries once.
The eviction count is driven by how many rows the insert actually added, measured
across it.
fresh.lengthis the whole re-persisted live window (up to 500) and nearlyall of it is already stored, so at factor 1 a single overflowing row would have evicted
hundreds — the store would sit far below its limit and prune on every batch forever.
Measured in a browser at the real limit: 50,000 rows, 12 requests → 12 stored, exactly
12 oldest evicted, count stays at 50,000. At factor 4×, 5 requests → 20 evicted, count
drops to 49,985, which is what the option promises.
2. Filtering searched only what happened to be loaded
Three consequences, all measured on a 47,320-row store:
matching almost nothing satisfied it forever: 189 IndexedDB page reads in three
seconds, the control blinking between "Load older" and "loading…", on its way to
paging the entire store into memory.
sat 4,381 rows down — 22 presses.
A filter is now a query the store answers with one cursor scan over
byEpoch,stopping early once a page of matches is collected, with a single searching state while
it runs. Same store, same click: 1 read, one spinner, both 404s in about a second,
nothing pressed. The empty state now says "Searched all N retained traces" and means it.
Two details worth checking in review:
during render — "still searching" is just "filtering, and no result for this filter
yet". That covers the debounce window, an in-flight scan and a filter changed mid-scan
with no state to reset, which is the rule this repo already follows in
module-detail-panel.src/lib/trace-filter.tsbecause two callers must agree onit: the scan, and the render-time filter that keeps a newly arrived match visible. A
scan cannot know about traces that did not exist when it ran.
Infinite scroll itself is now gated on the list actually overflowing its box — the
precondition proximity was assuming all along. Extracted as
shouldLoadOlder()becausethe table body is virtualized and jsdom has no layout, so DOM-level assertions there
pass for the wrong reason (measured: counting
tbody trcompared 0 to 0 and passedvacuously).
3. IndexedDB failures were silent
Read, write and delete failures each fell into a different lie: an unread history looked
empty, unwritten rows looked retained, and a failed delete looked successful. Each now
gets its own sentence carrying the
DOMException.nameverbatim.🔴 A failed delete does not empty the view. Emptying it would report a deletion that
did not happen, and the rows would come back on reload — the resurrection #48 fixed,
arriving by another route.
The mark is lifted only by a success of the same operation. Clearing on any success
looked equivalent and was not: the store seeds itself from storage on connect, that read
all but always succeeds, and it landed in the same tick as a failed write. A probe
showed
total: 1, failed: 0— the failure was recorded and erased before anything couldshow it.
4. The retained count overstated what was kept
It moved only on success, which made it a lie in the one situation that matters: the
retry evicts before it writes, so a batch that ends in failure has still deleted rows.
Measured under a refused write — the header said 49,985 retained while the store held
48,212, and overstated further with every batch. The note said rows were not being
saved; the number beside it claimed the rest were still there.
5. A refused write logged an unhandled rejection
The caller learns about a refusal from the request and never reaches
await tx.done, sothat promise rejected with an
AbortErrornobody was waiting for — the page logged anunhandled rejection for a failure the console had already handled and reported.
idbcreates the promise eagerly, so not touching it does not help. Only reachable once a
real quota abort was possible (see below); without the guard the suite raises 16
unhandled
AbortErrors and vitest exits 1.Options UI
The theme button becomes an options button with the theme toggle and settings inside it.
Four values: retention limit (minimum 50,000), eviction factor (1/2/4/8×), load-older
page size, sample-data delay. One
pc:key per field so the two settings that alreadyship do not have to move.
Validated on the way out of storage, not only in.
readStoredparses JSON and stopsthere, so a hand-edited entry would otherwise reach the pruner as
NaNand retentionwould stop being enforced with nothing on screen to say so.
Deliberately not exposed:
TRACE_WINDOW(the window design is still open inprotean-console-realtime-latency-window; exposing it first lets a user paper over thesymptom with 5000 and makes the option meaningless once the design changes), and the
STALE_TIMEOUT_MS/RECONNECT_DELAY_MS/MALFORMED_TOLERANCE/ RESTTIMEOUT_MSgroup, which are interlocked — #49 had to separate the watchdog deadline from stream age
for exactly that reason, and individually adjustable values let a user build a state
nobody can observe. A preset is the right shape if that is ever needed.
UI components come from the shadcn
base-novaregistry (dialog,dropdown-menu,field,toggle-group) rather than hand-rolled equivalents; the hand-written first passwas discarded.
Tests: 83 → 152, and what they cost to trust
Every guard was verified by reverting it. The counts, in order of the sections above:
eviction input 4, prune-then-retry 3,
freed === 05, eviction freeing nothing 3,filtering the loaded page 7, dropping the live merge 1, merging unfiltered 8, the
stale-scan guard 2, lying about an early exit 2, a blank query treated as active 1, the
null-field guard 1, the overflow gate 3, the
itemCountgate 1, the proximity window 1,an unrendered storage note 4, a failed clear that empties the view 2, collapsing the
three storage sentences into one 4,
loadSettingstrusting storage 3, the settingsclamp 2, an unguarded Save 3, an unrendered validation error 1, a Cancel that saves 3,
a dead Theme item 1, a dead Settings item 3, a dropped theme label 2,
observeAbort16 unhandled errors.
src/test/idb-quota.tsgivesfake-indexeddba byte budget, which is what made theretry testable at all. The retry rested on an assumption no test could reach — that
IndexedDB can still delete when it is full — because every other test of that path mocked
our own
upsertTracesto reject. The budget is enforced insideObjectStore.storeRecord,the operation
fake-indexeddbruns within the request, which is where a browser'squota check lives: throwing there fails the request, hands the transaction that error and
aborts it, in that order. Enforcing it on
IDBObjectStore.putinstead — the firstattempt — raises the error synchronously, so the caller's
Promise.allnever forms andthe already-issued puts reject unobserved; the suite drowned in 40 unhandled
AbortErrors no browser would produce.Three test-side facts found on the way, each of which had been hiding a real gap:
localStoragegetter but never creates theobject. Every settings read fell into
readStored's catch and came back as defaults.No test failed; they simply were not exercising storage.
does not report a deletion that did not happensnapshotted the store mid-writeand failed about once in three gate runs. It now waits for the store and IndexedDB to
agree — 10 consecutive clean runs of the sequence that failed.
filter inside the debounce means the first scan never starts, and the null-field guard,
because no assertion searched for the text
"null". Both now have a case that reachesthem.
Known limits, stated rather than hidden
buys room for the batch, and the batch is the whole live window re-persisted; if that
does not fit, the retry fails exactly as the first attempt did. The console's answer is
right (it says storage is refusing and keeps showing the traces) and the state now has
a test instead of waiting to be found in a browser.
histories multiply; that belongs to the runtime-server-selection work.
fake-indexeddb's budget is not a browser's accounting — no index or recordoverhead, no page granularity, and the refusal is synchronous rather than an async
request error. It cannot catch a defect that depends on other work interleaving before
the error lands.