[Pro] Prevent caching RSC renders with errors

[justin808]

Why

Buffered and static RSC helpers could persist an error-containing render in Rails.cache, allowing a later request to receive that rejected output as a cache hit. This closes #4723 while preserving caching for clean renders.

What changed

• Propagate per-chunk hasErrors metadata through both buffered and static RSC render paths.
• Return rejected render output to the current caller while exiting Rails.cache.fetch before its write step.
• Preserve cache-hit metadata, tag registration rules, normal nil caching, and race_condition_ttl behavior.
• Add focused regression coverage for rejected and clean renders, cache hits, default cache options, nested fetches, notifications, tags, and race handling.

The cache-write bypass uses Ruby's uniquely scoped catch/throw control flow instead of skip_nil, because React on Rails Pro still supports ActiveSupport versions whose cache implementation predates skip_nil.

Verification

• Focused Pro helper matrix: 10 examples, 0 failures.
• Rails 5.2 compatibility simulation (cache store ignores skip_nil): rejected buffered/static renders were returned and no cache entry was written.
• Current ActiveSupport executable checks: clean misses write, hits remain hits, rejected renders do not write, and ordinary nil behavior is unchanged.
• Targeted Pro RuboCop: 2 files, 0 offenses.
• Ruby syntax, git diff --check, Pro license-header audit, and commit hooks passed.
• Independent codex review --base origin/main: no actionable findings.

The broad helper spec currently has 23 unrelated failures because this checkout does not contain react_on_rails_pro/spec/dummy/ssr-generated/server-bundle.js; focused tests that do not depend on that generated bundle pass.

Churn and scope

This PR changes only the Pro helper, its focused dummy-app helper spec, and the changelog. No package, generator, or public API changes are included.

Fixes #4723

Summary by CodeRabbit

• Bug Fixes

  • Prevented buffered React component rendering and static RSC caching from saving results when any streamed chunk reports errors.
  • Ensured error-free renders continue to be cached.
  • Improved conditional cache write behavior and avoided recording cache metadata for rejected cache misses.

• Tests

  • Added helper coverage and regression specs validating conditional cache entry acceptance/rejection and the new “no cache on chunk errors” behavior for buffered and static RSC paths.

• Documentation

  • Added an unreleased changelog entry describing the fix.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d071dbc1-83bd-4471-8e65-c8649e4cac11

📥 Commits

Reviewing files that changed from the base of the PR and between 52ce9c2 and 2e99106.

📒 Files selected for processing (1)

• CHANGELOG.md

🚧 Files skipped from review as they are similar to previous changes (1)

• CHANGELOG.md

---

Walkthrough

Buffered and static RSC caching now observes streamed hasErrors flags and skips cache writes for error-containing renders. Shared cache-entry logic supports conditional writes, with tests covering cache semantics and both rendering paths.

Changes

RSC cache error handling

Layer / File(s)	Summary
Buffered rendering and conditional cache writes react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb	Chunk error callbacks flow through buffered rendering, while shared cache helpers conditionally persist rendered results and update cache tags only when appropriate.
Static RSC conditional cache wiring react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb	Static RSC cache misses track chunk errors and pass a conditional write predicate through the cache-entry path.
Cache behavior regression coverage react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb, CHANGELOG.md	Tests cover conditional cache-entry behavior and verify that buffered and static RSC error renders are not cached; the changelog records the fix.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant cached_buffered_stream_react_component
  participant buffered_stream_react_component
  participant RailsCache
  Caller->>cached_buffered_stream_react_component: request cached render
  cached_buffered_stream_react_component->>buffered_stream_react_component: stream chunks with on_chunk_errors
  buffered_stream_react_component-->>cached_buffered_stream_react_component: rendered output and error state
  cached_buffered_stream_react_component->>RailsCache: write only when no chunk has hasErrors true
  RailsCache-->>Caller: cached or freshly rendered output

Loading

Possibly related PRs

• shakacode/react_on_rails#4722: Adds related streamed chunk error detection for other cache-write paths.
• shakacode/react_on_rails#4386: Introduces the static RSC caching path modified here.
• shakacode/react_on_rails#4384: Refactors the shared buffered component caching flow modified here.

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: preventing cached RSC renders with errors.
Linked Issues check	✅ Passed	The helper and spec changes match #4723 by skipping cache writes for error-containing buffered/static RSC renders and preserving clean renders.
Out of Scope Changes check	✅ Passed	The PR stays within the cache-safety fix scope, with only supporting specs and a changelog update.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/rsc-error-cache-safety

---

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.}

[greptile-apps]

Greptile Summary

Prevents rejected React Server Component renders from being persisted while preserving existing cache-hit, tag-registration, nil-value, and race-condition behavior.

• Propagates streamed chunk error metadata through buffered and static RSC render paths.
• Uses scoped catch/throw control flow to return rejected output without allowing Rails.cache.fetch to write it.
• Adds regression coverage for rejected and clean renders, nested fetches, notifications, tags, and race handling.
• Documents the caching fix in the changelog.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb	Adds per-stream error tracking and a cache-fetch abstraction that bypasses writes for rejected buffered and static RSC renders.
react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb	Adds focused coverage for cache rejection, ordinary nil caching, nested fetches, notifications, tags, race-condition TTL, and both affected render helpers.
CHANGELOG.md	Records that buffered and static RSC helpers no longer persist error-containing renders.

Sequence Diagram

sequenceDiagram
  participant Caller
  participant Helper as Pro RSC Helper
  participant Cache as Rails.cache
  participant Renderer
  Caller->>Helper: Render cached component
  Helper->>Cache: fetch(cache_key)
  alt Cache hit
    Cache-->>Helper: Cached clean render
  else Cache miss
    Helper->>Renderer: Stream render
    Renderer-->>Helper: HTML chunks + hasErrors
    alt Any chunk has errors
      Helper-->>Cache: Exit fetch before write
      Helper-->>Caller: Return rejected render
    else Clean render
      Helper-->>Cache: Return value and write entry
      Helper-->>Caller: Return clean render
    end
  end

Loading

_{Reviews (2): Last reviewed commit: "Document cache safety fix attribution" | Re-trigger Greptile}

[claude]

Review Summary

What this PR does: Extends the fix from PR #4722 (streaming caches) to the two other Pro cache paths — cached_buffered_stream_react_component and cached_static_rsc_component. Both now thread a per-chunk on_chunk_errors callback through internal_stream_react_component → buffered_stream_react_component, tracking whether any streamed chunk reported hasErrors: true, and use that flag to bypass the Rails.cache.fetch write step for the current render via a new cache_write_if:-gated fetch_cache_entry helper.

Code quality / correctness

• The catch/throw mechanism in fetch_cache_entry (react_on_rails_pro_helper.rb:492-508) is a clever, well-scoped way to bypass ActiveSupport::Cache::Store#fetch's write step without needing skip_nil (which isn't available on the older ActiveSupport versions Pro still supports). I traced through Store#fetch → save_block_result_to_cache: throwing out of the block skips the write(...) call entirely (it's a non-local jump past that line, not caught by AS's rescue Exception), while ActiveSupport::Notifications.instrument(:generate, ...)'s ensure still fires — which matches the new spec asserting cache_generate fires but cache_write doesn't.
• Each call gets a fresh Object.new as its catch tag, which correctly keeps nested fetch_cache_entry calls (e.g. an outer cached component containing another cached component) from interfering with each other — covered by the "isolated nested rejected cache fetches" spec.
• cache_hit/cache_write_skipped are correctly used downstream to gate tag registration, pack-loading, and attribution normalization only for genuine hits, not rejected-but-fresh renders.
• Left one inline suggestion: the rationale for catch/throw over skip_nil is explained in the PR description but not in a code comment — worth adding near fetch_cache_entry so a future refactor doesn't "simplify" it back to skip_nil and reintroduce the bug on older ActiveSupport.

Test coverage

Strong — the new #fetch_cache_entry describe block and the two new "does not cache a … render when any chunk reports errors" specs cover: rejected vs. clean renders, ordinary nil caching (unaffected), nested rejected fetches, cache-generate/write notifications, tag-registration skipping, and race_condition_ttl (stale entry preserved, not overwritten by the rejected fresh render, until it naturally expires). This is thorough for a caching-correctness fix.

Minor issues

1. CHANGELOG entry missing PR attribution line (CHANGELOG.md:71-74) — every other entry in the file, including the directly-related PR [Pro] Skip stream caches for error-containing renders (fixes #4581) #4722 entry immediately above it, ends with a [PR ####](...) by [username](...) line per this repo's changelog guidelines. This entry stops after the Fixes [Issue 4723] line. Left a suggested fix inline.
2. Minor doc-comment suggestion on fetch_cache_entry (see inline comment) — not blocking.

Security / performance

• No security concerns; this only affects what gets persisted to Rails.cache, tightening (not loosening) what's cached.
• Performance tradeoff worth being aware of (not a bug): if a component's async boundary is persistently erroring (not just transient), every request will now skip the cache and pay full render cost instead of serving a (broken) cached value — that's the intended fix, but it does mean a persistent upstream error now costs more server-side compute per request than before. Given the alternative is silently serving broken HTML to every visitor, this seems like the right tradeoff.

Overall this is a solid, well-tested fix that mirrors the existing pattern from #4722 and closes a real cache-poisoning bug (#4723). The only concrete fix needed is the missing changelog PR link.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb (1)

452-462: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated stream-error-tracking logic between buffered and static RSC cache paths.

render_cached_buffered_stream_react_component and render_cached_static_rsc_component both build an identical stream_has_errors flag, an on_chunk_errors lambda, and a cache_write_if: -> { !stream_has_errors } predicate. Since this triplet is exactly the mechanism that prevents error-containing renders from being cached, keeping it in one place reduces the risk of the two paths silently diverging on this correctness-sensitive logic in a future change.

♻️ Suggested extraction

+  def with_stream_error_cache_write_if
+    stream_has_errors = false
+    on_chunk_errors = ->(chunk_has_errors) { stream_has_errors ||= chunk_has_errors == true }
+    yield(on_chunk_errors, -> { !stream_has_errors })
+  end
+
   def render_cached_buffered_stream_react_component(component_name, cache_options, render_options)
-    stream_has_errors = false
-    fetch_react_component(component_name, cache_options, cache_write_if: -> { !stream_has_errors }) do
-      options = render_options.merge(
-        props: yield,
-        skip_prerender_cache: true,
-        on_chunk_errors: ->(chunk_has_errors) { stream_has_errors ||= chunk_has_errors == true }
-      )
-      buffered_stream_react_component(component_name, options)
+    with_stream_error_cache_write_if do |on_chunk_errors, cache_write_if|
+      fetch_react_component(component_name, cache_options, cache_write_if:) do
+        options = render_options.merge(props: yield, skip_prerender_cache: true, on_chunk_errors:)
+        buffered_stream_react_component(component_name, options)
+      end
     end
   end

Also applies to: 641-658

🤖 Prompt for 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.

In `@react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb` around lines 452
- 462, Extract the shared stream-error tracking and cache-write predicate from
render_cached_buffered_stream_react_component and
render_cached_static_rsc_component into a common helper or wrapper. Update both
methods to reuse that single mechanism while preserving the existing
on_chunk_errors behavior and preventing cache writes when any stream error
occurs.

🤖 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 71-75: Update the new **[Pro]** changelog entry so its closing
attribution includes the required PR number/link and author link, matching the
`Fixes [Issue N]... [PR N](url) by [author](url).` format used by neighboring
entries. Preserve the existing issue reference and inline **[Pro]** marker.

---

Nitpick comments:
In `@react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb`:
- Around line 452-462: Extract the shared stream-error tracking and cache-write
predicate from render_cached_buffered_stream_react_component and
render_cached_static_rsc_component into a common helper or wrapper. Update both
methods to reuse that single mechanism while preserving the existing
on_chunk_errors behavior and preventing cache writes when any stream error
occurs.

🪄 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 Plus

Run ID: 3c7ea805-7908-4e12-886d-f43bf1e39a1d

📥 Commits

Reviewing files that changed from the base of the PR and between 37ddaf5 and 52ce9c2.

📒 Files selected for processing (3)

• CHANGELOG.md
• react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb
• react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[justin808]

CodeRabbit review-summary disposition: the changelog blocker is fixed in 2e991062af61b13d6c6bf0c8436a49830c6721f1.

[auto-deferred]
The suggested shared stream-error-tracking helper extraction is declined for this focused correctness fix: both explicit paths are covered by the same unhappy/clean-path regressions, and refactoring now would widen code churn after final-candidate validation.

[justin808]

Address-review summary

Scan scope: full PR history through 2026-07-27T18:02:46Z; no previous address-review cutoff existed.

Mattered

• The duplicated Claude and CodeRabbit changelog-attribution findings were fixed in 2e991062af61b13d6c6bf0c8436a49830c6721f1. The entry now links PR [Pro] Prevent caching RSC renders with errors #4804 and @justin808. Both threads were replied to and resolved.
• The CodeRabbit review-summary changelog blocker is handled by the same commit.

Optional

• [auto-deferred] The Claude request for an explanatory catch/throw comment was declined: the compatibility rationale is in the PR and focused regression coverage protects the behavior. The thread was replied to and resolved.
• [auto-deferred] The CodeRabbit shared-helper extraction was declined: it would widen a focused correctness patch after final-candidate validation without changing behavior. The review-summary disposition is recorded in [Pro] Prevent caching RSC renders with errors #4804 (comment).

Skipped

• The Copilot quota-limit review summary is status-only and safe to skip.
• The CodeRabbit walkthrough, Greptile 5/5 clean review, and Claude general review summary are informational; their substantive changelog/optional points are dispositioned above.

Autonomously resolved threads

• [Pro] Prevent caching RSC renders with errors #4804 (comment) — verified fixed at the pushed head.
• [Pro] Prevent caching RSC renders with errors #4804 (comment) — duplicate attribution blocker verified fixed at the pushed head.
• [Pro] Prevent caching RSC renders with errors #4804 (comment) — optional comment declined with an evidence-backed [auto-deferred] rationale.

Deferred-work tracking: none; no follow-up issue is warranted for these optional refactors/comments.

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

[justin808]

+ci-status

[justin808]

+ci-run-hosted

[github-actions]

CI Status

Head SHA: 2e991062af61
Changed files: 3
Docs-only heuristic (matches ci-changes-detector metadata paths): no
ready-for-hosted-ci label: absent
force-full-hosted-ci label: absent
Current hosted-CI waiver: not present for this SHA
Automatic release-target hosted mode: inactive
Observed exact-head coverage: modes[missing=9]; successful=0, pending=0, failed=0, missing=9

Only the required gate is active unless hosted CI is requested.

[github-actions]

Hosted CI Requested

Triggered 9 workflow(s) for 2e991062af61.
Skipped 0 workflow(s) with equivalent exact-head coverage.
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.

[claude]

Review Summary

I reviewed the caching-safety fix in react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb (buffered/static RSC caching paths), focusing on correctness, concurrency safety, security, and performance.

Overall: solid, well-reasoned change. No functional bugs found. Highlights of what I verified:

• catch/throw cache-write bypass (fetch_cache_entry): Confirmed this correctly avoids the Rails.cache.fetch write step. ActiveSupport::Notifications::Instrumenter#instrument uses ensure for cache_generate, so that notification still fires, while the subsequent write(...) call in save_block_result_to_cache is skipped because throw unwinds before reaching it — matches the "generate emitted, write skipped" test.
• Uniqueness of the throw tag (skip_cache_write = Object.new per call) correctly avoids symbol-collision issues on nested/recursive cache fetches — validated against the "nested rejected cache fetches" test.
• Fiber/Async safety: Since Ruby catch/throw tags live on the fiber's own call stack, and Async fibers preserve their stack across suspension (e.g. while awaiting the node-renderer HTTP call), the throw correctly unwinds within the same fiber that entered the catch. No cross-fiber leakage.
• Race-condition TTL interplay: When a stale entry is being regenerated (race_condition_ttl), the pre-write extension happens before the block runs (unaffected by the throw), and skipping the final write correctly leaves the extended stale entry in place until its original expiry — matches the dedicated regression test.
• Tag registration: Correctly gated on unless cache_hit || cache_write_skipped, avoiding orphaned TagIndex entries pointing at cache keys that were never actually written — this is an easy mistake the PR avoids.
• Backward compatibility: cache_write_if: defaults to nil in both fetch_react_component and fetch_static_rsc_component_cache_entry, and cache_write_if && !cache_write_if.call short-circuits to false for all pre-existing callers (cached_react_component, cached_react_component_hash, async variants), so unrelated call paths are unaffected.
• Test coverage is thorough: rejected/clean renders, cache hits, nested fetches, notifications, tag registration, and race-condition behavior are all exercised, including a simulated pre-skip_nil ActiveSupport environment.

Minor (non-blocking) observations

• The catch/throw technique relies on an implementation detail of ActiveSupport::Cache::Store#fetch — that the cache write happens strictly after the block returns normally, not in an ensure. This holds today (and is exercised by the Rails 5.2-simulation test), but a short code comment in fetch_cache_entry explaining why this pattern is used (vs. skip_nil) would help future maintainers who might otherwise "simplify" it back to skip_nil and reintroduce the bug for Pro's older-ActiveSupport support matrix.
• Two unrelated pre-existing tests (cached_buffered_stream_react_component "caches the fully buffered result..." and cached_static_rsc_component "caches the stripped static HTML...") had cache_options: { expires_in: 60 } removed. This doesn't appear to weaken any assertion in those tests (no expiry behavior is checked), but it's worth confirming this was intentional cleanup rather than papering over a default-cache-options edge case.

No security or performance concerns identified — this is a pure caching-correctness fix with no new user-input surface, and the added catch overhead is negligible relative to SSR rendering cost.

[justin808]

Current-head Claude review disposition for #4804 (comment):

• No blocker was found.
• [auto-deferred]
  The repeated explanatory-comment suggestion remains declined for this focused final candidate; the PR rationale and regression coverage preserve the ActiveSupport compatibility intent.
• Removing cache_options: { expires_in: 60 } from the two happy-path cases was intentional in commit 908f04f6d846a06295662747bb0502f136f9483f: it makes those cases exercise the default/nil cache-options path fixed by that commit. Expiration behavior is still covered by the surrounding explicit expires_in cases and the rejected-write race_condition_ttl regression, so coverage was shifted and strengthened rather than weakened.

[justin808]

@greptileai review

[justin808]

Address-review summary

Scan scope: review activity since the previous summary at 2026-07-27T18:03:05Z, on exact head 2e991062af61b13d6c6bf0c8436a49830c6721f1.

Blocking findings: None.

Optional:

• Claude current-head review found no functional, security, or performance bug. Its request for an explanatory inline comment repeats the already-declined behavior-preserving nit and remains [auto-deferred] at the final-candidate debounce point.
• Claude asked whether removing explicit expires_in: 60 from two happy-path examples weakens coverage. Confirmed intentional: those examples now cover nil/default cache options, while explicit expiry remains covered by surrounding helper examples and the race/TTL regression. Safe to skip; no code change.

Skipped:

• Hosted-CI command/status comments are workflow metadata.
• Greptile current-head review is clean at the exact head (5/5 confidence, safe to merge); the manual trigger comment is workflow metadata.
• Copilot ended at its quota-limit status without substantive feedback.

Unresolved review threads: 0.

Next default scan begins after this summary timestamp.