[Makisuo]

First step from the web fetching-layer audit: read cache hygiene.

What changed

• data/read-cache.ts: the 400-entry bound claimed LRU but evicted by insertion order (a hit called Map.set on an existing key, which keeps its position). A hit or release now re-inserts the entry. Past the bound only the excess goes, ranked idle-expired first, then least recently read, in-flight reads last. Expired entries below the bound stay on purpose: a page still holding one revives it on release.
• page/preload.ts: superseding re-reads put a \u0001N generation on their key. Preload held and released those raw keys, which never match a cache entry, so the release was a no-op. It now holds the plain request key (baseKey).
• update.ts: the page Model walk that collects live keys used to run on every page message (hover, typing). It's now a thunk, called only when a step asks for new keys.

invalidate(prefix) from the audit is left out: no write path reads through the cache yet, so it lands with the query builder instead.

Verification

• New read-cache.test.ts cases (LRU order, expired-first eviction, only the excess goes with a held entry revived on release). All three fail on main's cache, and the last one also fails on a sweep-all version.
• New preload.test.ts cases: slot keys strip the generation, and a step with no new keys never calls the live-keys walk.
• vitest run src/data src/page/preload.test.ts src/update.test.ts: 45 passed. tsrs --noEmit clean, oxlint clean on the changed files.
• Two review passes (correctness + Effect conventions). The first found that capped swept every expired entry, which is fixed and covered by the third test.

---

^{Need help on this PR? Tag @codesmith-bot with what you need.}

---

Summary by CodeRabbit

• Performance
  • Improved data loading efficiency by keeping recently used content available while removing stale entries when storage limits are reached.
  • Enhanced preload handling when navigating between pages, preserving content that remains visible and avoiding unnecessary work.
  • Improved consistency when replacing or releasing preload requests, helping reduce redundant loading and resource usage.
  • Improved handling of asynchronous content loads by updating usage information when loading completes.

[maple-review-bot]

Maple review

🟢 Confidence 10/10 · safe to merge
Eviction rewrite is pinned by three new tests that discriminate against main and a sweep-all version; only a recency-position nit remains.
_{1 note · tests covered · risk medium}

Rewrites the read cache's bound to evict expired-then-least-recently-read entries, holds plain request keys in preload so releases reach the cache, and makes the page Model walk lazy. The eviction math and key handling check out; safe to merge.

• capped evicts only the excess: expired first, then least recently read, in-flight last
• release and cache hits re-insert the entry so Map order is recency
• Preload.keyOfCommand/liveKeysOf strip a superseding re-read's \u0001N generation
• Preload.held takes liveKeys as a thunk, walked only when pruning

Findings

🔵 Note · F1 · settle keeps the entry's old position, so a late-settling read is evicted last-resort

correctness · apps/web/src/data/read-cache.ts:85

capped reads Map order as recency, but settle writes the settled entry with next.set, which keeps its original insertion slot instead of moving it to the end the way touched does on a hit. A load slow enough for the bound's worth of other reads to land behind it therefore looks like the least recently read settled entry, and the next insert evicts the value it just fetched, so the page re-reads it.

			if (Exit.isSuccess(exit)) touched(next, key, { ...entry, touchedAtMs: nowMs, isSettled: true })

🤖 Prompt to fix this finding with an AI agent

Findings from an automated review of commit 15b5687c1d754e1e7b8ecb9c9bd6de9007c2572a. Verify each one against the current code before changing anything, fix only those that still apply, and keep each fix to the lines it names.

---

F1 · Note · correctness · apps/web/src/data/read-cache.ts:85
`settle` keeps the entry's old position, so a late-settling read is evicted last-resort
`capped` reads Map order as recency, but `settle` writes the settled entry with `next.set`, which keeps its original insertion slot instead of moving it to the end the way `touched` does on a hit. A load slow enough for the bound's worth of other reads to land behind it therefore looks like the least recently read settled entry, and the next insert evicts the value it just fetched, so the page re-reads it.
Replace those lines with:
			if (Exit.isSuccess(exit)) touched(next, key, { ...entry, touchedAtMs: nowMs, isSettled: true })

What was checked

• Plain-key premise holds: sources/new/command.ts:20 reads through the cache as loadSources(baseKey(key), refresh), the only readThrough page that supersedes
• Eviction ranks by isExpired/isSettled with a stable sort, so only excess keys go and a held expired entry survives (read-cache.test.ts:142)
• isSettled matches the old Deferred.isDone liveness: a success settles, a failure deletes the entry

_{15b5687 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: bc579e14-a6f8-46a3-9c33-ee3ce24eeb30

📥 Commits

Reviewing files that changed from the base of the PR and between 15b5687 and 9f6bb47.

📒 Files selected for processing: 2

• apps/web/src/data/read-cache.test.ts
• apps/web/src/data/read-cache.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

📝 Walkthrough

The read cache now tracks settlement and read recency to prioritize eviction when it exceeds its limit. Preload keys are normalized, and live keys are computed lazily during same-holder pruning.

Changes: Read Cache Eviction

Layer / File(s)	Summary
Track cache state and apply eviction priority apps/web/src/data/read-cache.ts, apps/web/src/data/read-cache.test.ts	Cache entries track settlement and recency. When the cache exceeds 400 entries, eviction removes expired settled entries first, then other settled entries by least-recently-read order, and in-flight entries last. Tests cover eviction order, cache bounds, settlement timing, and release behavior.

Preload Key Handling

Layer / File(s)	Summary
Normalize preload keys and defer live-key evaluation apps/web/src/page/preload.ts, apps/web/src/update.ts, apps/web/src/page/preload.test.ts	Command and slot keys are normalized through baseKey. Preload.held accepts a callback for live keys, and writePage passes that callback. Tests cover key retention, empty windows, superseding reread keys, and deferred evaluation.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 9f6bb

No outstanding issue is established that would prevent merging after normal checks.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the two main changes: recency-based read-cache eviction and plain request keys for preload holding.
Docstring Coverage		No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 5…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 5 files.

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

[github-actions]

🍁 Maple PR preview

Note

Preview resources were removed when this pull request closed.

Final commit 9f6bb47 · View workflow run

[Makisuo]

Maple review F1 fixed in 9f6bb47: settle now moves the entry to the recency end (touched), so a slow load is no longer evicted right after it lands. New test "a read counts as recent from when it settles, not when it started" fails without the fix.

[maple-review-bot]

Maple review

🟢 Confidence 10/10 · safe to merge
Eviction, release revival and settle recency are each pinned by a new test that fails on the old cache; preload keys now match the cache's plain keys.
_{no findings · tests covered · risk medium}

Makes the web read cache evict by recency (expired first, in-flight last), moves a settled read to the recency end, and has preload hold and release the plain request key so its release actually matches a cache entry. Behaviour is pinned by new cache and preload tests; safe to merge.

• capped evicts only the excess, ranking expired then least recently read then in-flight
• touched re-inserts on hit, release and settle, so map order is recency
• keyOfCommand and slotKeyOf strip a superseding re-read's generation via baseKey
• held takes a liveKeys thunk, so the page Model is walked only when pruning

Fixed since the last review

• ✅ F1 · settle keeps the entry's old position, so a late-settling read is evicted last-resort

What was checked

• settle writes isSettled/touchedAtMs into the map before Deferred.done, so the isExpired reuse check cannot serve a done-but-unsettled entry
• touched only mutates maps freshly built by ofOrg/new Map(current), never the committed Entries
• Every Preload.held caller at head passes a thunk (only writePage, update.ts:290)

_{9f6bb47 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}

[Makisuo]

Preview test on app-pr-1604.maple.dev @ 9f6bb47 (separate test agent, Preview org, real browser). API requests counted with a PerformanceObserver.

Check	Result
Overview, Services, Traces, trace detail, Logs, Errors, Releases, Replays	Pass: all load, no 4xx/5xx, no console errors after sign-in (Releases/Replays show their normal empty states)
Hover a trace row, then click	Pass: hover sent 1 span-hierarchy; the click sent 0 more (the preload was reused). A trace opened without hovering sends it once on click
Services → service → back within ~8 s	Pass: 0 new execute-batch on return (overview read reused). service-health-baseline is re-requested; it doesn't go through the shared cache (unchanged by this PR)
5 time-range changes ~400 ms apart on Traces	Pass: settles on the last range (6h), counts match, nothing stuck loading

Noticed, not from this PR: /v2/onboarding/checklist is requested on almost every navigation (5× in ~8 s). Worth a follow-up.

[Makisuo]

Traces from the preview test (Maple, deployment.environment = pr-1604, 15:15 to 15:38 UTC)

• maple-api: 180 HTTP requests in the window, p50 139 ms, p95 503 ms, max 1224 ms, 0 errors. The only errors in the environment are 2 alerting TickDispatchErrors (the alert tick answering 500), unrelated to the web app.
• Slowest trace 12c9cb540662589529b72b43cb228c31, the trace detail's span hierarchy:

  maple-web  FetchSpanHierarchy                                     2366 ms
   └ POST /_api/internal/query-engine/span-hierarchy                2364 ms
     └ maple-web-worker POST /_api/*                                1795 ms
       └ maple-api POST /internal/query-engine/span-hierarchy       1224 ms
         ├ OrganizationRegionService.ensureServedHere                 84 ms
         ├ QueryEngineService.cachedDirect → Tinybird executeSql     897 ms
         └ AuditLogService.record                                    151 ms
  

  The time goes to the warehouse query on the preview's Tinybird branch, not to the client cache. Each preloaded span-hierarchy read shows up once per trace opened, matching the request counts in the preview test above.
• Side finding, not from this PR: browser spans from the preview report deployment.environment = production (resource attribute), while the worker and API report pr-1604. So preview browser telemetry is mixed into production's browser data.