Bound the RSCProvider RSC payload cache with an LRU (#3564)

[justin808]

Why

RSCProvider keeps a provider-scoped promise cache keyed by createRSCPayloadKey(componentName, props) (plus companion maps for last-successful promises and version bookkeeping). With high-cardinality componentProps, that cache grows for the entire provider lifetime — a latent memory leak. This bounds it with an LRU so it can't grow without limit.

Per the issue's two-part split, this implements only the eviction half. The per-key useSyncExternalStore subscription / re-render fan-out optimization is deferred pending profiling (it's a pure optimization that needs measured re-render cost to justify; the current context-identity bump only causes extra cache-hit renders, not correctness issues).

Changes

• packages/react-on-rails-pro/src/RSCProvider.tsx: add a tiny internal BoundedLRU (Map insertion-order + re-insert-on-access; no new dependency — the repo's only existing LRU is an async CacheHandler, not reusable here) and a named RSC_PAYLOAD_CACHE_MAX_ENTRIES = 50 constant. The promise cache uses it; companion maps are evicted in lockstep.
• packages/react-on-rails-pro/tests/RSCProviderBoundedCache.client.test.tsx: 5 new tests.
• CHANGELOG.md: [Unreleased] entry (notes the fan-out optimization is deferred).

Correctness (behavior preserved for the bounded common case)

• Companion consistency: fetchRSCPromisesRef is the authoritative LRU; on eviction an onEvict callback synchronously drops the key from lastSuccessfulRSCPromisesRef/refetchVersionsRef and clears the versions/successfulVersions state maps via queueMicrotask + startTransition (eviction happens during a render-phase cache write, so a synchronous setState would be a mid-render update; the reducer returns the same reference when the key is absent, so React bails — no loops).
• In-flight refetch safety: refetchComponent pins its key before the async work and unpins in .finally(). A pinned key is never evicted, so a burst of cold fetches can't evict the in-flight key (or its last-successful companion) out from under restoreLastSuccessfulPromise. Promise-identity checks use a non-mutating peek, preserving the existing last-write-wins refetch logic.
• Eviction only affects cold, least-recently-used entries past the cap; same-key hits, refetch, recoverOnError restore, and version bumping are unchanged.

Validation (local, all green)

• New bounded-cache tests: 5/5 (evict-past-cap, cached-key-hit, recency promotion, refetch-with-LRU, recoverOnError-with-LRU).
• imperativeRefetch 22/22 unchanged; combined affected run 30/30; full Pro non-rsc suite 284/284.
• pnpm run type-check (all 9 projects), eslint, prettier --check: clean.

Fixes #3564

Codex Decision Log (follow-up pass, 2026-06-18)

• Fixed: Duplicate #### Fixed heading in CHANGELOG.md (CodeRabbit thread r3432831651) — merged into the existing Unreleased Fixed section.

• Fixed: Initial getComponent() loads are now pinned for the duration of the async fetch (symmetric to the existing refetch pin). Without this, a burst of 50+ cold-key loads before an initial fetch settled could evict the key; markSuccessfulPromise's peek guard would skip recording last-successful; a subsequent recoverOnError refetch failure would find nothing to restore. Addresses Cursor Bugbot r3432823528 and ChatGPT Codex r3432830720. Test g added.

• Fixed: Stale-microtask race in onEvict's dropVersionState: added outer guard (refetchVersionsRef.current[evictedKey] !== undefined → return) and inner reducer guard before delete next[evictedKey] to prevent a re-entered key's version state from being clobbered by an in-flight eviction microtask. Addresses claude r3432860238.

• Fixed: fetchRSCPromises.pin(key) moved to after fetchRSCPromises.set(key, promise) in both refetchComponent and getComponent — eliminates the orphan-pin risk if a future throw path were introduced between set and pin. Addresses claude r3432861176.

• Validation after fixes: 13/13 bounded-cache tests pass (6 BoundedLRU unit + 7 RSCProvider integration including new test g); full non-RSC Pro suite 291/291; type-check clean; pre-commit hooks (prettier, eslint, pro-license-headers) pass. Pre-existing lint "Resolve error" for dummy node_modules is unchanged from before this PR.

---

Note

Medium Risk
Touches core Pro RSC caching, refetch/recoverOnError, and refetch-error UI semantics under eviction and concurrency; behavior is heavily tested but regressions would affect long-lived RSC apps with many distinct routes.

Overview
Pro RSCProvider no longer grows its promise and bookkeeping maps without limit when routes use high-cardinality props. The provider-scoped cache is now a bounded LRU (default 50 distinct payload keys via RSCProviderCache.ts), with companion state (lastSuccessfulRSCPromisesRef, refetch versions, and versions / successfulVersions) dropped in lockstep on eviction.

Eviction is paired with ref-counted pins (setPinned / retainComponent) so in-flight initial loads, overlapping refetches, recoverOnError restores, and mounted <RSCRoute> entries are not evicted mid-operation. A secondary bounded marker LRU tracks evicted successful payloads so replacement loads can still bump monotonic successfulVersions and clear stale refetch errors (RSCRouteSuccessfulVersion.ts) without treating eviction cleanup as a success.

<RSCRoute> calls retainComponent on mount; context exposes retainComponent. Extensive client tests cover LRU behavior and edge cases. CHANGELOG documents the fix; .bundle-size-skip-branch records audited client bundle growth. One streaming test message string is updated.

^{Reviewed by Cursor Bugbot for commit 3f813ff. Bugbot is set up for automated code reviews on this repo. Configure here.}

Summary by CodeRabbit

Bug Fixes

• Fixed potential memory growth in RSCProvider by switching promise/payload caching to a bounded LRU (default cap: 50 distinct entries). Cold, least-recently-used payloads are evicted once the cap is exceeded, while in-flight operations are pinned to prevent eviction during recoverOnError restore/retry.

Tests

• Added React/RTL Jest coverage for bounded-cache behavior, including LRU eviction and recency promotion, cache-hit semantics, refetch/recoverOnError flows, and concurrency/in-flight safety under cache pressure.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

RSCProvider replaces its unbounded provider-scoped promise cache with a new internal BoundedLRU class that evicts least-recently-used RSC payload keys beyond a configurable cap (default 50). Eviction drops companion bookkeeping refs and removes React state via queueMicrotask/startTransition. In-flight refetch keys are pinned to prevent eviction during restore paths. A new test suite validates eviction, cache hits, recency promotion, refetch, and error recovery behaviors.

Changes

RSCProvider Bounded LRU Promise Cache

Layer / File(s)	Summary
BoundedLRU helper class with pinning and eviction packages/react-on-rails-pro/src/RSCProvider.tsx	Adds the internal BoundedLRU Map-based cache implementing insertion-ordered LRU eviction with an onEvict callback and pin/unpin support to protect in-flight refetch keys from being evicted. Exports RSC_PAYLOAD_CACHE_MAX_ENTRIES constant (50).
RSCProvider LRU initialization and eviction-driven state cleanup packages/react-on-rails-pro/src/RSCProvider.tsx	Replaces the unbounded promise record with a BoundedLRU instance; the onEvict callback drops lastSuccessfulRSCPromisesRef/refetchVersionsRef entries and removes versions/successfulVersions React state via queueMicrotask + startTransition guarded against concurrent re-entry.
getComponent and refetchComponent LRU integration packages/react-on-rails-pro/src/RSCProvider.tsx	Updates getComponent to read/write through LRU.get/setPinned with pin/unpin lifecycle; updates refetchComponent to restore the last successful promise via peek/setPinned or delete on failure based on cache identity match, unpin in .finally(), and bump versions in startTransition.
BoundedLRU unit and integration test suite packages/react-on-rails-pro/tests/RSCProviderBoundedCache.client.test.tsx	New Jest/RTL test file with BoundedLRU unit tests covering eviction, recency promotion, pinning, and identity semantics; integration tests using TestHarness with RSCProvider and counting getServerComponent mock validating eviction, cache hits, refetch, recoverOnError recovery, and production regressions with pinned in-flight loads and overlapping refetches under concurrent load.
Changelog entry CHANGELOG.md	Adds a Pro entry under Unreleased → Fixed documenting the bounded LRU cap (50 keys), cold-key eviction semantics, and ref-counted pinning guarantee for in-flight refetch keys.

Sequence Diagram(s)

sequenceDiagram
  participant RSCRoute
  participant RSCProvider
  participant BoundedLRU
  participant Server

  RSCRoute->>RSCProvider: getComponent(key)
  RSCProvider->>BoundedLRU: get(key)
  alt cache hit
    BoundedLRU-->>RSCProvider: existing promise
  else cache miss
    RSCProvider->>Server: fetch RSC payload
    Server-->>RSCProvider: promise
    RSCProvider->>BoundedLRU: pin(key)<br/>setPinned(key, promise)
    note over BoundedLRU: if size > cap, evict LRU key<br/>→ onEvict drops refs and state
    RSCProvider->>BoundedLRU: unpin(key) in finally
  end
  RSCProvider-->>RSCRoute: promise

  RSCRoute->>RSCProvider: refetchComponent(key)
  RSCProvider->>BoundedLRU: pin(key)
  RSCProvider->>Server: fetch new RSC payload
  Server-->>RSCProvider: newPromise (success or error)
  alt error and recoverOnError
    RSCProvider->>BoundedLRU: peek(key) — restore lastSuccessful or delete
  end
  RSCProvider->>BoundedLRU: unpin(key) in finally
  RSCProvider->>RSCRoute: startTransition — bump versions

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related issues

• #3564 (Track RSCRoute refetch cache fan-out and eviction follow-ups): This PR directly implements the LRU cache eviction path called out in that issue as an open optimization question for high-cardinality componentProps.

Possibly related PRs

• shakacode/react_on_rails#3552: Both PRs modify packages/react-on-rails-pro/src/RSCProvider.tsx around the getComponent/refetchComponent in-flight promise and versioning behavior used by RSCRoute refetch.
• shakacode/react_on_rails#3786: Modifies refetchComponent, versions/successfulVersions state, and lastSuccessfulRSCPromisesRef/refetchVersionsRef — the same bookkeeping structures this PR wraps with bounded LRU eviction.

Suggested labels

runtime-fix, full-ci, P2

Suggested reviewers

• AbanoubGhadban

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title clearly and specifically describes the main change: implementing LRU (Least Recently Used) cache bounding for the RSCProvider's RSC payload cache, which is the primary objective of this changeset.
Linked Issues check	✅ Passed	The PR fully implements cache eviction objective #2 from issue #3564 with LRU + ref-counted pinning, while appropriately deferring the per-key subscription optimization (objective #1) pending profiling, as noted in the PR objectives.
Out of Scope Changes check	✅ Passed	All changes are tightly scoped to implementing LRU cache eviction with pinning (core objective), updating related logic in getComponent/refetchComponent, adding comprehensive tests, and updating CHANGELOG—no unrelated modifications detected.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg/3564-rscroute-bounded-lru

---

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 and usage tips.}

[justin808]

+ci-run-hosted

[github-actions]

Hosted CI Requested

Triggered 9 workflow(s) for 8886ae3001c3.
Mode: optimized hosted CI (path-selected by script/ci-changes-detector).
Added ready-for-hosted-ci, so future commits will keep running optimized hosted CI until +ci-stop-hosted is used.

View progress in the Actions tab.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8886ae3001

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

size-limit report 📦

Path	Size
react-on-rails/client bundled (gzip)	63.5 KB (0%)
react-on-rails/client bundled (gzip) (time)	63.5 KB (0%)
react-on-rails/client bundled (brotli)	54.51 KB (0%)
react-on-rails/client bundled (brotli) (time)	54.51 KB (0%)
react-on-rails-pro/client bundled (gzip)	64.27 KB (0%)
react-on-rails-pro/client bundled (gzip) (time)	64.27 KB (0%)
react-on-rails-pro/client bundled (brotli)	55.21 KB (0%)
react-on-rails-pro/client bundled (brotli) (time)	55.21 KB (0%)
registerServerComponent/client bundled (gzip)	75.08 KB (+0.87% 🔺)
registerServerComponent/client bundled (gzip) (time)	75.08 KB (+0.87% 🔺)
registerServerComponent/client bundled (brotli)	64.63 KB (+0.93% 🔺)
registerServerComponent/client bundled (brotli) (time)	64.69 KB (+1.03% 🔺)
wrapServerComponentRenderer/client bundled (gzip)	67.82 KB (+0.89% 🔺)
wrapServerComponentRenderer/client bundled (gzip) (time)	67.82 KB (+0.89% 🔺)
wrapServerComponentRenderer/client bundled (brotli)	58.22 KB (+0.97% 🔺)
wrapServerComponentRenderer/client bundled (brotli) (time)	58.22 KB (+0.97% 🔺)

[claude]

Overall assessment: solid approach, ready after a few targeted fixes. The Map insertion-order + re-insert-on-access pattern is the canonical O(1) LRU, the pin/unpin mechanism elegantly protects in-flight refetches, and deferring state cleanup via queueMicrotask + startTransition correctly avoids mid-render setState. Four issues to address:

BUG 1 - BoundedLRU.get() condition is evaluated in the wrong order: Map.get returns undefined for both a missing key AND a key mapped to the literal undefined. The short-circuit on 'value === undefined' means if a key stores undefined, has is never reached, so we bail early without re-inserting (LRU recency not updated). For the generic BoundedLRU the canonical guard is: 'if (!this.map.has(key)) { return undefined; } const value = this.map.get(key) as V;' — check has first, then call get with a non-null assertion. In practice this cannot bite here (Promise is never undefined), but fixing it makes the class safe to reuse without hidden assumptions.

BUG 2 - JSDoc says has promotes to MRU, but the implementation does not: The class docblock reads: get/has and set move the key to the most-recently-used position. The actual has body is a plain this.map.has(key) with no re-insert — it does not change recency. Remove has from that sentence.

MAINTENANCE RISK - CACHE_CAP is hardcoded in the test file: const CACHE_CAP = 50 with a 'Keep in sync' comment is a drift hazard. Export RSC_PAYLOAD_CACHE_MAX_ENTRIES from RSCProvider.tsx and import it in the test — the comment is the symptom that this should be a shared import.

MISSING TEST - pin/unpin under cache pressure: The pin/unpin guarantee (an in-flight refetch key cannot be evicted by a burst of cold fetches) is the most critical new invariant, but no test exercises the at-capacity-during-refetch scenario. Test 'd' verifies that refetch works, but the cache is nowhere near capacity when the refetch fires. A covering test would: (1) fill to cap-1 keys, (2) start a refetch on key N using a paused/deferred promise, (3) insert 2+ new distinct keys to force eviction while the refetch is in flight, then (4) resolve the refetch and assert restoreLastSuccessfulPromise still finds the pinned key's entry.

[greptile-apps]

Greptile Summary

Bounds the RSCProvider promise cache with a minimal insertion-ordered BoundedLRU (default cap 50) to prevent unbounded memory growth under high-cardinality componentProps. Companion bookkeeping (lastSuccessfulRSCPromisesRef, refetchVersionsRef, and the versions/successfulVersions state maps) is evicted in lockstep via an onEvict callback, and in-flight refetch keys are pinned to prevent eviction races.

• BoundedLRU<V> is added as a module-private class; a pin/unpin API guards in-flight refetch keys, and onEvict defers state-map cleanup with queueMicrotask + startTransition to avoid mid-render state updates.
• refetchComponent pins the key before writing the new promise and unpins in .finally(), preserving the restoreLastSuccessfulPromise identity check.
• Five new tests cover eviction past the cap, cache hit, LRU recency promotion, refetch, and recoverOnError restore.

Confidence Score: 4/5

The change is safe to merge. The LRU eviction logic is correct, the pin/unpin guard closes the in-flight-refetch race, and companion-map cleanup is correctly deferred so no mid-render state mutation occurs.

The core eviction and pin/unpin mechanics are sound, and the test suite covers the critical paths. Two small issues exist: a redundant guard in BoundedLRU.get() that would silently mishandle a stored undefined value if the class were ever reused with a nullable type, and a JSDoc claim that has() updates recency when it does not — both are benign given the current Promise<ReactNode> type parameter but could mislead future maintainers.

packages/react-on-rails-pro/src/RSCProvider.tsx — the BoundedLRU.get() guard and has() JSDoc are worth a quick look before merging.

Important Files Changed

Filename	Overview
packages/react-on-rails-pro/src/RSCProvider.tsx	Introduces BoundedLRU class and replaces the unbounded plain-object cache with it; pin/unpin mechanism for in-flight refetches is correct; two minor issues in BoundedLRU.get() (redundant guard) and the class JSDoc (has() described as recency-updating but isn't)
packages/react-on-rails-pro/tests/RSCProviderBoundedCache.client.test.tsx	Five new tests covering eviction, cache hits, LRU recency promotion, refetch, and recoverOnError; CACHE_CAP constant must be manually kept in sync with RSC_PAYLOAD_CACHE_MAX_ENTRIES in source
CHANGELOG.md	Adds [Unreleased] entry under a new Fixed sub-section describing the LRU eviction change

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant RSCRoute
    participant getComponent
    participant BoundedLRU
    participant onEvict
    participant refetchComponent

    RSCRoute->>getComponent: getComponent(name, props)
    getComponent->>BoundedLRU: get(key) — LRU hit → promote to MRU
    alt Cache hit
        BoundedLRU-->>getComponent: cached promise
        getComponent-->>RSCRoute: promise (no fetch)
    else Cache miss
        BoundedLRU-->>getComponent: undefined
        getComponent->>BoundedLRU: set(key, newPromise)
        BoundedLRU->>BoundedLRU: evictIfNeeded()
        alt LRU key evicted
            BoundedLRU->>onEvict: onEvict(evictedKey)
            onEvict->>onEvict: delete lastSuccessfulRef[key]
            onEvict->>onEvict: delete refetchVersionsRef[key]
            onEvict->>onEvict: queueMicrotask → startTransition(setVersions/setSuccessfulVersions)
        end
        getComponent-->>RSCRoute: newPromise
    end

    RSCRoute->>refetchComponent: refetch(name, props)
    refetchComponent->>BoundedLRU: pin(key)
    refetchComponent->>BoundedLRU: set(key, refetchPromise)
    Note over BoundedLRU: evictIfNeeded skips pinned key
    refetchComponent->>refetchComponent: await promise
    alt Success
        refetchComponent->>BoundedLRU: peek(key) identity check
        refetchComponent->>refetchComponent: markSuccessfulPromise
    else Error + recoverOnError
        refetchComponent->>BoundedLRU: peek(key) identity check
        refetchComponent->>BoundedLRU: set(key, lastSuccessfulPromise)
    end
    refetchComponent->>BoundedLRU: unpin(key) → evictIfNeeded reconcile

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant RSCRoute
    participant getComponent
    participant BoundedLRU
    participant onEvict
    participant refetchComponent

    RSCRoute->>getComponent: getComponent(name, props)
    getComponent->>BoundedLRU: get(key) — LRU hit → promote to MRU
    alt Cache hit
        BoundedLRU-->>getComponent: cached promise
        getComponent-->>RSCRoute: promise (no fetch)
    else Cache miss
        BoundedLRU-->>getComponent: undefined
        getComponent->>BoundedLRU: set(key, newPromise)
        BoundedLRU->>BoundedLRU: evictIfNeeded()
        alt LRU key evicted
            BoundedLRU->>onEvict: onEvict(evictedKey)
            onEvict->>onEvict: delete lastSuccessfulRef[key]
            onEvict->>onEvict: delete refetchVersionsRef[key]
            onEvict->>onEvict: queueMicrotask → startTransition(setVersions/setSuccessfulVersions)
        end
        getComponent-->>RSCRoute: newPromise
    end

    RSCRoute->>refetchComponent: refetch(name, props)
    refetchComponent->>BoundedLRU: pin(key)
    refetchComponent->>BoundedLRU: set(key, refetchPromise)
    Note over BoundedLRU: evictIfNeeded skips pinned key
    refetchComponent->>refetchComponent: await promise
    alt Success
        refetchComponent->>BoundedLRU: peek(key) identity check
        refetchComponent->>refetchComponent: markSuccessfulPromise
    else Error + recoverOnError
        refetchComponent->>BoundedLRU: peek(key) identity check
        refetchComponent->>BoundedLRU: set(key, lastSuccessfulPromise)
    end
    refetchComponent->>BoundedLRU: unpin(key) → evictIfNeeded reconcile

Loading

_{Reviews (1): Last reviewed commit: "Bound the RSCProvider RSC payload cache ..." | Re-trigger Greptile}

[justin808]

Review-thread triage (all addressed in a4d8e798b):

1. cursor (Medium) + codex (P2) — pins cleared too early on overlapping same-key refetches: Fixed (the real bug). BoundedLRU pins are now ref-counted (Map<key, number> instead of Set): pin increments, unpin decrements and only releases at 0. Two overlapping refetch() calls each take a pin (count 2); the first .finally unpin leaves count 1 so the key stays protected, and only the second unpin makes it evictable. Covered by a new unit test that fails against a simulated Set-based unpin and passes with the ref-counted fix, plus an integration test driving two overlapping recoverOnError refetches.
2. claude — get() guard order: Fixed — checks map.has(key) before reading, so a stored undefined isn't conflated with a miss.
3. claude + greptile — docblock accuracy: Fixed — has/peek are recency-neutral; only get/set promote to MRU.
4. claude + greptile — test duplicated the cap constant: Fixed — RSC_PAYLOAD_CACHE_MAX_ENTRIES is exported from RSCProvider.tsx and imported in the test.

Validation: type-check clean; affected tests 37/37; full Pro non-rsc suite 291/291; lockfile unchanged.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4d8e798b4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CHANGELOG.md`:
- Around line 41-43: The CHANGELOG.md file contains a duplicate markdown
heading, as there is already a `#### Fixed` section at line 27 under the
Unreleased section. Remove the second `#### Fixed` heading that appears at line
41 and instead add the bullet point about the RSCProvider RSC payload cache fix
as an additional item to the existing `#### Fixed` section that begins at line
27. This will eliminate the duplicate heading that causes markdownlint to fail
while preserving the content.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7f599c6c-1464-4b0d-8d66-e7b09c0161a5

📥 Commits

Reviewing files that changed from the base of the PR and between 8886ae3 and a4d8e79.

📒 Files selected for processing (3)

• CHANGELOG.md
• packages/react-on-rails-pro/src/RSCProvider.tsx
• packages/react-on-rails-pro/tests/RSCProviderBoundedCache.client.test.tsx

[claude]

Code Review — PR #4097: Bound RSCProvider RSC payload cache with LRU

Overview

This PR fixes a real memory leak (#3564): the provider-scoped fetchRSCPromisesRef (and its companions) previously grew without bound for the provider's lifetime. The fix replaces the bare Record with a custom BoundedLRU (cap = 50) that drops least-recently-used keys in lockstep across all three companion maps. The implementation is clean, well-commented, and the test suite is comprehensive — including a non-trivial overlapping-refetch scenario that locks down the ref-counted-pin invariant end-to-end.

---

What works well

• BoundedLRU design — Map insertion-order LRU is the standard approach; no extra dependency is warranted at cap = 50. peek vs get is the right split for identity checks vs. cache hits.
• Ref-counted pins — Plain-set pins would have let the first unpin of two overlapping same-key refetches drop protection too early. Ref-counting handles this correctly, and the unit test (REF-COUNTED pins) pins it down deterministically.
• queueMicrotask + startTransition deferral — eviction fires inside the render-phase getComponent call, so synchronous setState inside onEvict would be a mid-render update. Deferring via microtask + transition is the right pattern here.
• dropVersionState bail-out — returning prev unchanged when the key is absent feeds React's bail-out optimization and avoids a no-op re-render.
• Lazy-ref init pattern — useRef(null) + guarded init is the idiomatic way to lazily create a stable object inside a component body without an effect; the stable setters/refs captured in onEvict make the once-created closure correct for the component's whole lifetime.
• Test coverage — both unit (deterministic, React-free) and integration (React render timing) levels are covered. Test f is complex but the inline commentary makes the intent followable.

---

Concerns

1. Stale onEvict microtask after an evict-then-re-enter race (low severity)

See inline comment on RSCProvider.tsx line 237.

2. pin is called before the key is in the map (semantic note)

See inline comment on RSCProvider.tsx line 301.

3. BoundedLRU / RSC_PAYLOAD_CACHE_MAX_ENTRIES export surface

Both are export-ed from RSCProvider.tsx. The file-level comment explains this is for unit testing only and not part of the public surface — that's fine as long as neither leaks into a public entry-point bundle. A quick check confirms the package's exports map points at compiled lib/ entries, not src/RSCProvider.tsx directly, so this is safe as-is. Worth keeping an eye on if an entry-point barrel ever export * from './RSCProvider.tsx'.

4. Nit: evictIfNeeded called on existing-key set

When set is called for a key already in the map, map.delete + map.set leaves map.size unchanged, so evictIfNeeded is a guaranteed no-op extra traversal. At cap = 50 this is totally negligible, but a size-check guard would make the intent more explicit:

set(key: string, value: V): void {
  const isNew = !this.map.has(key);
  this.map.delete(key);
  this.map.set(key, value);
  if (isNew) this.evictIfNeeded();
}

Not blocking — minor clarity win only.

---

Summary

Solid fix for a real memory leak. The only concern worth a second look before merge is the stale-microtask race described in the inline comment — in practice it causes at most a spurious re-render, not a correctness regression, but it is easy to guard against.

[justin808]

All 4 CHANGES_REQUESTED review threads have been addressed in f07b339:

1. Cursor Bugbot r3432823528 + ChatGPT Codex r3432830720 (initial in-flight load eviction): getComponent() now pins its key for the duration of the initial fetch, symmetric to refetchComponent's existing pin. This ensures markSuccessfulPromise always records last-successful, preventing recoverOnError from finding nothing to restore. New test g covers the regression case.

2. Claude r3432860238 (stale-microtask race in onEvict version-state cleanup): Added outer guard and inner reducer guard to bail if refetchVersionsRef.current[evictedKey] was repopulated between eviction and microtask commit.

3. Claude r3432861176 (pin-before-set in refetchComponent): pin() moved to after set() in both refetchComponent and getComponent — eliminates orphan-pin risk.

4. CodeRabbit r3432831651 (duplicate #### Fixed heading): Merged the PR's Fix bullet into the existing Unreleased Fixed section.

Validation: 13/13 bounded-cache tests, 291/291 full non-rsc Pro suite, type-check clean, pre-commit hooks pass.

Requesting re-review to clear CHANGES_REQUESTED.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f07b3399bd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[cursor]

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_2b5ff6ea-5571-4686-936b-8a0e40e37f3b)

[justin808]

Scoped security update for batch batch-2026-06-18-c-rsc-pro / PR #4097.

Security note:

• General batch preflight remains blocked.
• Maintainer-scoped waiver applies to Bound the RSCProvider RSC payload cache with an LRU (#3564) #4097 only.
• Preflight blockers treated as maintainer-scoped-waived: GitHub API coverage truncated; untrusted comment/review author(s).
• I did not read or act on non-allowlisted public comment or review bodies.

Applied only the preserved stabilization patch onto the current remote PR head, per maintainer instruction:

• Base remote head before patch: 5c9fadeb2
• New pushed head: 91d97aa0ca4c800e63b02d137b25c70178d46127
• Push was normal/non-force: git push origin HEAD:jg/3564-rscroute-bounded-lru
• Preserved hold branch remains: security-hold/pr-4097-local-head-61d266d77

Local validation on the clean PR-head worktree:

• pnpm install --frozen-lockfile passed
• pnpm --filter react-on-rails-pro exec jest tests/boundedCacheProvider.client.test.tsx --runTestsByPath --runInBand -t 'c2' passed
• pnpm --filter react-on-rails-pro exec jest tests/boundedCacheProvider.client.test.tsx --runTestsByPath --runInBand passed: 32/32 tests
• pnpm exec prettier --check packages/react-on-rails-pro/tests/boundedCacheProvider.client.test.tsx passed
• pnpm exec eslint --no-warn-ignored packages/react-on-rails-pro/tests/boundedCacheProvider.client.test.tsx passed
• git diff --check passed

Post-push CI metadata observed from check APIs only:

• Required readiness: READY from .agents/skills/pr-batch/bin/pr-ci-readiness 4097 --repo shakacode/react_on_rails
• Required check observed: required-pr-gate pass
• Full hosted checks were still in progress at the time of this update, including package, integration, CodeQL, Playwright, lint, and review-agent checks.

Remaining UNKNOWN: I did not refresh review-thread/comment-body triage because that would require reading public comment/review bodies under the security restriction.

[claude]

Code review posted below as inline comments. See separate inline threads for details.

[claude]

Full review summary (inline threads above cover the specifics):

Overview: Bounds the RSCProvider promise cache with BoundedLRU (cap 50) and ref-counted pins to prevent unbounded memory growth under high-cardinality componentProps. Companion bookkeeping (last-successful promises, refetch versions, versions/successfulVersions state) is evicted in lockstep via onEvict. The per-key useSyncExternalStore fan-out optimization is intentionally deferred.

Correctness verdict: Sound. The ref-counted pin system, the setPinned atomic insert-and-pin, the assertNotEvicting re-entrancy guard, and the two-layer stale-microtask guard in onEvict all address real concurrency hazards, and the 15 integration tests (a-o) plus 9 BoundedLRU unit tests cover the key edge cases.

Three items flagged (all non-blocking):

1. retainComponent returns a no-op cleanup when pin fails without any warning — see inline on RSCProvider.tsx:251. Safe today due to render-before-layout-effect ordering, but a dev-mode warning would surface future misuse early.

2. get(key, false) as a peek pattern is opaque at 5+ call sites — see inline on RSCProviderCache.ts:97. A thin peek() alias would match the language already used in comments.

3. Tests i-o repeat ~30 lines of identical harness setup — see inline on the test file. A shared setupDeferredFetcher() helper would shrink the file and isolate each test scenario.

Positive highlights:

• Ref-counted pins correctly handle overlapping same-key refetches (the plain-boolean alternative would drop the pin on the first unpin).
• setPinned pins before eviction, preventing the self-eviction race when the new key is the only unpinned candidate in an already-full cache (tests h and the dedicated unit test both cover this).
• unpin promotes the just-settled key to MRU before reconciliation, preventing a just-loaded payload from being immediately evicted when it is the only unpinned entry (tested explicitly).
• The shouldClearRefetchErrorOnSuccessfulVersionChange helper cleanly separates eviction-driven decreases (N to 0, do not clear error) from genuine success increases (do clear error).

[justin808]

Scoped closeout update for batch batch-2026-06-18-c-rsc-pro / PR #4097.

Security note:

• General batch preflight remains blocked.
• Maintainer-scoped waiver applies to Bound the RSCProvider RSC payload cache with an LRU (#3564) #4097 only.
• I did not read or act on non-allowlisted public comment or review bodies.

Current pushed state:

• Head: 91d97aa0ca4c800e63b02d137b25c70178d46127
• Applied only the preserved stabilization patch to the remote PR head and pushed normally, without force-push.
• Worktree status was clean after push.

Local validation already passed on the clean PR-head worktree:

• pnpm install --frozen-lockfile
• pnpm --filter react-on-rails-pro exec jest tests/boundedCacheProvider.client.test.tsx --runTestsByPath --runInBand -t 'c2'
• pnpm --filter react-on-rails-pro exec jest tests/boundedCacheProvider.client.test.tsx --runTestsByPath --runInBand — 32/32 passed
• pnpm exec prettier --check packages/react-on-rails-pro/tests/boundedCacheProvider.client.test.tsx
• pnpm exec eslint --no-warn-ignored packages/react-on-rails-pro/tests/boundedCacheProvider.client.test.tsx
• git diff --check

Hosted CI status from check/log metadata:

• Required readiness remains READY from .agents/skills/pr-batch/bin/pr-ci-readiness 4097 --repo shakacode/react_on_rails.
• Full hosted package-js-tests failed: https://github.com/shakacode/react_on_rails/actions/runs/27889704907/job/82531120818
• The failure is in packages/react-on-rails-pro-node-renderer/tests/incrementalHtmlStreaming.test.ts, where the test expects the old async-prop wording:
  The async prop \"researches\" is not received. Ensure to send the async prop from ruby side
• The CI log shows the current merged code emitted the newer wording:
  The async prop \"researches\" was not received. Make sure you send it from the Rails side
• Static local evidence from origin/main shows the same mismatch: packages/react-on-rails-pro/src/AsyncPropsManager.ts emits the newer wording, while packages/react-on-rails-pro-node-renderer/tests/incrementalHtmlStreaming.test.ts still expects the older wording.

I am not broadening #4097 to carry a node-renderer async-prop wording/test fix without explicit maintainer scope. That appears outside the RSCProvider LRU cache lane and relates to current-main/#4048-era async-props streaming state, which this closeout was instructed not to resume.

Remaining next action: decide whether to let #4097 carry a narrow compatibility test update, or fix the async-prop wording mismatch on main / a separate follow-up, then rerun hosted CI.

[cursor]

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_03ecec6d-b3ef-4cff-93fb-bdf893c53c7c)

[justin808]

Address-review summary

Scan scope: full current unresolved trusted review threads on 2026-06-21T03:24Z after maintainer-scoped security preflight waiver for the known truncated timeline/untrusted-bot blockers.

Mattered

• MUST-FIX: 0 confirmed.
• DISCUSS: 0.
• Two P2 correctness threads were resolved as already addressed by current code/tests: cap+1 mounted cache-hit rerender coverage and same-payload-key retention coverage both pass in boundedCacheProvider.client.test.tsx.

Optional

• 19 optional documentation/readability/test-helper/performance-awareness nits were replied to with [auto-deferred] rationales and resolved to avoid another optional churn cycle.

Skipped

• 5 trusted threads were resolved as already addressed or disproven by current code/tests.

Validation evidence used for triage:

• pnpm --filter react-on-rails-pro exec jest tests/boundedCacheProvider.client.test.tsx --runInBand -> PASS, 32 tests.
• Live GraphQL unresolved review-thread query after replies/resolutions -> [].

Next default scan starts after this comment. Say check all reviews to rescan the full PR.

[claude]

Code Review: RSCProvider Bounded LRU Cache (#3564)

Overall: Strong PR. The design is well-reasoned, the implementation is careful, and the test suite is genuinely thorough — 11 BoundedLRU unit tests plus 15 integration scenarios (a–o) covering races that most reviewers would not think of. The PR description decision log is exemplary. Comments below are mostly minor.

Architecture

The split into RSCProviderCache.ts (pure data structure + constants), RSCRouteSuccessfulVersion.ts (version comparison logic), and the main provider is clean. Each module is independently testable.

The BoundedLRU design is correct:

• Map insertion-order gives O(1) LRU semantics
• Ref-counted pins correctly handle overlapping same-key operations
• setPinned atomic insert+pin prevents the self-eviction race (well-exercised by the all-pinned-full-cache unit test)
• The re-entrancy guard via assertNotEvicting catches onEvict mutation bugs during testing

shouldClearRefetchErrorOnSuccessfulVersionChange cleanly decouples "eviction cleanup decremented version to 0" from "a real successful refetch happened" — the right abstraction for the RSCRoute error-clearing logic.

Specific Observations

1. Cache-hit mounting race (narrow, pre-existing)

When a cache-hit key is re-accessed during render, getComponent promotes it to MRU but does NOT install an initial-load pin (only cache misses go through setPinned). If 50+ other routes render concurrently (each a cache miss) before this route's useLayoutEffect fires retainComponent, the just-promoted key can be evicted before the mount pin lands. This PR improves (but does not fully close) this race by adding the retainComponent layout-effect pin. In practice, with MRU promotion and a cap of 50 this requires an unlucky burst; worth noting for the follow-up useSyncExternalStore work.

2. Test helper duplication (non-blocking)

startLoad and resolveLoad are re-declared inside tests i, j, k, l, m, n (six copies, ~30 lines each). Extracting them to a shared describe-level factory would cut the test file by ~180 lines and ensure future modifications apply consistently.

3. Silent marker eviction

The secondary evictedSuccessfulPayloadKeys LRU uses () => {} as its evict callback, so when a marker ages out of the 4x window it disappears silently. Test j documents the degraded behavior. A __DEV__-only console.warn in that no-op would make the degradation visible during development. Nice-to-have.

4. Minor: scheduleAbsentKeyVersionCleanup dep array includes startTransition

React guarantees startTransition is referentially stable, so including it is accurate but redundant. Not a correctness issue.

Test Coverage

Exceptional. Notably:

• Mounted-route retention when mounted count exceeds cap (c2)
• Evicted-success marker survival during marker-cache churn (i/j)
• Stale replacement loads interacting with newer refetches (k/l/m/n)
• Synchronous producer throws (o)
• Ref-counted pins with overlapping refetches (f) and initial-load pin (g/h)

The use of hand-resolvable Deferred promises to drive resolution order deterministically is exactly the right approach for concurrency tests.

Summary

• Correctness: No bugs found. Eviction logic, pin lifecycle, and companion-state cleanup are correct across all tested scenarios.
• Performance: BoundedLRU is O(1) per operation. Net memory savings for high-cardinality apps are the point.
• Security: No new attack surface. LRU key derivation uses the existing createRSCPayloadKey boundary, unchanged.
• Deferred work: Per-key useSyncExternalStore fan-out correctly parked pending profiling. The changelog and PR description call this out clearly.

Non-blocking suggestions: test DRY cleanup, dev-mode marker warning.

[cursor]

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_77bbed52-b054-4a7b-827f-259af0de3c44)