Fix SSR renderer URL VM global

[justin808]

Closes #780

Follow-up for shakacode/react_on_rails#3823.

• inject Node's standard URL constructor into the renderer VM context
• cover SSR of the RouterApp NavLink path that requires URL

Validation:

• focused request spec against a fresh isolated renderer
• bin/conductor-exec bin/ci against a fresh isolated renderer

No React on Rails dependency or lockfile changes.

Summary by CodeRabbit

• Bug Fixes

  • Improved server-rendered navigation so URL-based links render correctly.
  • Fixed missing URL support in server-rendered pages.

• Tests

  • Added request spec coverage to verify server-rendered navigation link presence and content.

[github-actions]

🚀 Quick Review App Commands

Welcome! Here are the commands you can use in this PR:
They require the repository to have cpflow review apps configured, including the CPLN_TOKEN_STAGING secret.

+review-app-deploy

Deploy your PR branch for testing.

+review-app-delete

Remove the review app when done.

+review-app-help

Show detailed instructions, environment setup, and configuration options.

Comment +review-app-help for full setup details.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 416a3daa-3b0b-467b-b385-3b08aab78c81

📥 Commits

Reviewing files that changed from the base of the PR and between 4f2ee3c and 2f24def.

📒 Files selected for processing (1)

• spec/requests/server_render_check_spec.rb

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

• spec/requests/server_render_check_spec.rb

---

Walkthrough

The renderer VM receives Node’s URL global, and a server-rendering request spec verifies the RouterApp NavLink output.

Changes

SSR URL global

Layer / File(s)	Summary
Renderer URL context and SSR regression coverage renderer/node-renderer.js, spec/requests/server_render_check_spec.rb	Imports URL from node:url for the renderer VM context and verifies the server-rendered RouterApp NavLink has the expected href and text.

Estimated code review effort: 1 (Trivial) | ~5 minutes

🚥 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 matches the main change: restoring the SSR renderer's URL VM global.
Linked Issues check	✅ Passed	The code injects URL into the renderer VM and adds a focused regression spec, matching issue #780's requirements.
Out of Scope Changes check	✅ Passed	The changes are limited to the renderer fix and its regression test, with no unrelated scope introduced.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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-codex/fix-780-url-global

---

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

This PR exposes URL to the SSR renderer VM. The main changes are:

• Imported Node's standard URL constructor in the renderer.
• Passed the constructor through the existing renderer VM context configuration.
• Added request coverage for RouterApp NavLink server rendering.

Confidence Score: 5/5

This looks safe to merge.

• No blocking issues found in the changed code.

Important Files Changed

Filename	Overview
renderer/node-renderer.js	Imports Node's URL constructor so the renderer VM context can expose it during SSR.
spec/requests/server_render_check_spec.rb	Adds request coverage for the RouterApp NavLink SSR path that depends on the URL global.

_{Reviews (1): Last reviewed commit: "fix: provide URL to SSR renderer VM" | Re-trigger Greptile}

[claude]

Review

Overview: Minimal, well-scoped fix for #780 — injects Node's built-in URL constructor into the renderer's VM additionalContext so SSR of react-router-dom's NavLink (via encodeLocation) doesn't throw ReferenceError: URL is not defined, and adds a request spec that exercises the RouterApp NavLink path.

Code quality

• renderer/node-renderer.js: the added const { URL } = require('node:url'); is correct and matches the existing explanatory comment a few lines below (additionalContext: { URL, AbortController }). Making the dependency explicit rather than relying on URL resolving as an ambient global is good practice — it's not fragile to any future sandboxing/global changes and makes the intent obvious at the import site.
• Minor nit: the new import uses the node: protocol prefix while the adjacent const path = require('path'); doesn't. Not a functional issue (repo requires Node >=22, and there's no unicorn/prefer-node-protocol-style lint rule enforcing consistency either way), just a small stylistic inconsistency worth a follow-up cleanup if you want the file to read uniformly.

Test coverage

• The new spec (spec/requests/server_render_check_spec.rb) is a solid regression test: it asserts the actual rendered href/text of a NavLink that exercises the code path that previously threw during SSR. This is exactly the kind of test that would have caught the original bug (an unhandled SSR exception here would either 500 or silently omit the RouterApp markup, both of which the assertion would catch).
• One gap: there's no direct/unit coverage on renderer/node-renderer.js itself (e.g., verifying additionalContext actually contains a URL key), so the regression protection relies entirely on the request spec plus a correctly-configured/warmed isolated renderer process picking up the change. Given the PR description notes this was validated against "a fresh isolated renderer," it may be worth calling out in the PR/README that the renderer bundle cache needs to be cleared for this fix to take effect in already-running environments — easy to forget when deploying.

Security / performance

• No concerns. URL is a standard, side-effect-free global; exposing it in the VM's additionalContext doesn't broaden the sandbox's capabilities beyond what's already effectively available (the object literal previously already referenced bare URL, which resolved to the same global class instance either way).

Overall: correct, small, and appropriately tested. Nothing blocking.

[coderabbitai]

🧹 Nitpick comments (1)

spec/requests/server_render_check_spec.rb (1)

15-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Prefer response.parsed_body for response parsing.

This keeps the regression spec aligned with Rails conventions while preserving the same selector assertion.

🤖 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 `@spec/requests/server_render_check_spec.rb` around lines 15 - 18, Update the
response parsing in the root_path request spec to use response.parsed_body
instead of manually constructing a Nokogiri document from response.body.
Preserve the existing selector and expected “Route” assertion unchanged.

Source: Linters/SAST tools

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

Nitpick comments:
In `@spec/requests/server_render_check_spec.rb`:
- Around line 15-18: Update the response parsing in the root_path request spec
to use response.parsed_body instead of manually constructing a Nokogiri document
from response.body. Preserve the existing selector and expected “Route”
assertion unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 42cf23d0-91fc-4caf-ba02-f502c405d92a

📥 Commits

Reviewing files that changed from the base of the PR and between 4830aa3 and 4f2ee3c.

📒 Files selected for processing (2)

• renderer/node-renderer.js
• spec/requests/server_render_check_spec.rb

[claude]

Review

Overview

Small, well-scoped fix for #780: explicitly imports Node's URL constructor (require('node:url')) in renderer/node-renderer.js so the value passed into additionalContext: { URL, AbortController } is guaranteed to be Node's real URL class rather than relying on an implicit/ambient global lookup at config-build time. This is what gets injected into the SSR VM sandbox, and react-router-dom's NavLink/encodeLocation needs a working URL global during server rendering. A request spec is added that exercises the actual Layout.jsx NavLink (to="/react-router", text "Route") to catch regressions.

Code quality

• The fix is minimal and targeted — one import line, no unrelated changes.
• Good defensive practice: sourcing URL from node:url explicitly is more robust across JS realms/sandboxes than depending on it being auto-populated as a bare global identifier, which is presumably the root cause of the original ReferenceError: URL is not defined.
• Minor nit: require('node:url') uses the node: protocol prefix while the adjacent require('path') on line 1 doesn't. Not a functional issue, but worth using a consistent style (either both with or both without the prefix) for readability.
• AbortController is left relying on the ambient global (there's no core module to import it from), which is consistent/expected — only URL needed the explicit import.

Test coverage

• The new spec (spec/requests/server_render_check_spec.rb) targets the real component (Layout.jsx's <Link to="/react-router">), asserting both the rendered href and link text, which gives good confidence the SSR path that previously threw is now exercised.
• It uses response.parsed_body while the neighboring test in the same file uses Nokogiri::HTML(response.body) explicitly — both work under Rails 8, but the inconsistency is a minor style nit within the same file.
• The spec description mentions an implementation detail ("VM global") — harmless, but slightly leaks internals into the spec name.

Security / performance

• No security concerns — this only exposes Node's standard URL constructor to the SSR sandbox, which is expected/necessary for correct react-router-dom behavior and doesn't broaden capabilities beyond what's already relied upon.
• No meaningful performance impact.

Overall

LGTM — a focused, low-risk fix with a regression test tied to real application code. Only nitpicks (require style consistency, parsed_body vs Nokogiri::HTML consistency) — nothing blocking.

[github-actions]

✅ Review App Deleted

Review app for PR #781 is deleted

🎮 Control Plane Console
📋 View Workflow Logs