Add TanStack Router SSR integration utility#2516
Conversation
Add createTanStackRouterRenderFunction() to the react-on-rails npm package, providing a render function factory for TanStack Router apps with server-side rendering support. This encapsulates the private API workarounds needed for synchronous SSR (router.__store.setState() and router.ssr flag) inside ShakaCode-maintained code, so consumer apps only use public APIs. Server-side: synchronously injects route matches and renders with renderToString. Client-side: hydrates with browser history and dehydrated router state from the server. Also includes an async path (serverRenderTanStackAppAsync) for use with React on Rails Pro NodeRenderer when rendering_returns_promises is enabled. Related: #2298, #2299 Ref: printivity/printivity#2571, shakacode/react_on_rails-demos#104 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds TanStack Router integration and wiring: package export and optional peer dependency; TypeScript types; server sync/async render helpers; client hydration utility; threading of server-returned clientProps into Rails helper merge; docs, tests, demo apps, and example routes. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Rails Controller
participant Server as SSR Process
participant Router as TanStack Router
participant Store as Router.__store
participant Renderer as React Renderer
Controller->>Server: call renderFunction(props with url, railsContext.serverSide=true)
Server->>Router: createRouter(createMemoryHistory(initialEntries=[url]))
Server->>Router: validate internals / matchRoutes(path)
Server->>Store: inject matches sync (__store setState)
Server->>Router: router.ssr = true
Server->>Router: dehydrated = router.dehydrate()
Server->>Renderer: build appElement (RouterProvider + AppWrapper)
Renderer-->>Server: appElement
Server-->>Controller: return { renderedHtml: appElement, clientProps: { __tanstackRouterDehydratedState } }
sequenceDiagram
participant Browser as Client Browser
participant Hydrator as clientHydrateTanStackApp
participant History as createBrowserHistory
participant Router as TanStack Router
participant Effect as post-hydration effect
Browser->>Hydrator: init(props may include dehydratedState)
Hydrator->>History: createBrowserHistory()
Hydrator->>Router: createRouter(browserHistory)
alt dehydratedState present
Hydrator->>Router: router.hydrate(dehydratedState)
Hydrator->>Router: router.ssr = true
else no dehydratedRouter
Hydrator->>Router: router.matchRoutes(path) -> inject matches into __store
end
Browser->>Effect: useEffect -> if dehydratedState then router.load()
Router-->>Browser: RouterProvider renders hydrated app
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is ON, but it could not run because Privacy Mode (Legacy) is turned on. To enable Bugbot Autofix, switch your privacy mode in the Cursor dashboard.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5226885a84
ℹ️ 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".
Greptile SummaryThis PR adds a new The implementation is well-structured with clear comments explaining private-API workarounds. However, three issues need addressing:
Confidence Score: 2/5
Sequence DiagramsequenceDiagram
participant RoR as React on Rails
participant RenderFn as createTanStackRouterRenderFunction
participant Server as serverRenderTanStackApp
participant Client as clientHydrateTanStackApp
participant Router as TanStack Router
Note over RoR,Router: Server-Side Rendering (sync)
RoR->>RenderFn: renderFn(props, railsContext{serverSide:true})
RenderFn->>Server: serverRenderTanStackApp(options, props, railsContext)
Server->>Router: options.createRouter()
Server->>Router: router.update({history: memoryHistory})
Server->>Router: validateRouterInternals(router)
Server->>Router: router.matchRoutes(pathname, search)
Server->>Router: router.__store.setState({status:'idle', matches})
Server->>Router: router.ssr = true
Server->>Router: router.dehydrate()
Server-->>RenderFn: ReactElement (with __tanstackRouterDehydratedState in props)
RenderFn-->>RoR: ReactElement → renderToString()
Note over RoR,Router: Client-Side Hydration
RoR->>RenderFn: renderFn(props, railsContext{serverSide:false})
RenderFn->>Client: clientHydrateTanStackApp(options, props, railsContext)
Client->>Router: options.createRouter()
Client->>Router: router.update({history: browserHistory})
Client->>Router: router.__store.setState({status:'idle', matches})
Client->>Router: router.ssr = true
Client->>Router: router.hydrate(dehydratedState.dehydratedRouter)
Client-->>RenderFn: ReactElement (AppComponent)
RenderFn-->>RoR: ReactElement → hydrateRoot()
Note over Client,Router: useEffect (post-mount): router.load() [currently unreachable]
Last reviewed commit: 5226885 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Review of PR #2516 — TanStack Router SSR integrationGood overall architecture — encapsulating the private API workarounds in a library-maintained module is the right call. A few issues worth addressing before merging: 🔴
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/react-on-rails/src/tanstack-router/types.ts (1)
11-21: Widen search value typing to avoid rejecting valid typed routers.Lines [13] and [20] use
Record<string, string>, which is stricter than many TanStack Router search schemas (often non-string values). This can makecreateRouterreturn types harder to assign.🔧 Proposed change
matchRoutes: ( pathname: string, - locationSearch: Record<string, string>, + locationSearch: Record<string, unknown>, opts?: { throwOnError?: boolean }, ) => unknown[]; @@ location: { pathname: string; - search: Record<string, string>; + search: Record<string, unknown>; searchStr: string; hash: string; href: string; };Also applies to: 25-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails/src/tanstack-router/types.ts` around lines 11 - 21, The type for search is too narrow (Record<string, string>) and rejects valid TanStack Router schemas; update the search typing used in matchRoutes signature and in state.location (and the other occurrences around the same area) to a wider type such as Record<string, unknown> (or Record<string, any>) so non-string search values are accepted; locate the matchRoutes declaration and the state { location { search } } type blocks and replace the Record<string, string> occurrences with the wider type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-on-rails/package.json`:
- Around line 62-67: The peer dependency range for "@tanstack/react-router" is
too loose (">= 1.0.0") and can pull in breaking v2+ changes; update the
peerDependencies entry for "@tanstack/react-router" to constrain to v1.x (for
example ">=1.0.0 <2.0.0") so consumers cannot install incompatible v2+ releases
that break uses of internal APIs like router.__store.setState and router.ssr.
In `@packages/react-on-rails/src/tanstack-router/clientHydrate.ts`:
- Around line 44-47: The initialization forcibly setting status to 'idle'
prevents the later condition that calls router.load() from ever running; remove
the status override so the initial state uses the router's real status and then
unconditionally call router.load() on first client render (respecting router.ssr
= true behavior) to enable client-side navigation — update the clientHydrate
initialization that assigns status and resolvedLocation and ensure router.load()
is invoked (call router.load() directly or remove the status check gating it) so
that Router.load() executes after the first render.
In `@packages/react-on-rails/src/tanstack-router/index.ts`:
- Line 39: The public entrypoint currently only re-exports types
(TanStackRouterOptions, DehydratedRouterState) and the sync factory but omits
the async SSR helper; add an explicit export for serverRenderTanStackAppAsync so
consumers can import it from the package surface (e.g. export {
serverRenderTanStackAppAsync } from './serverRenderTanStackAppAsync' or from the
module where it's implemented) and similarly ensure the same export is added in
the other export block referenced (lines ~106-117) so the async helper is
available from both public entrypoints; reference the symbol
serverRenderTanStackAppAsync when adding the export.
---
Nitpick comments:
In `@packages/react-on-rails/src/tanstack-router/types.ts`:
- Around line 11-21: The type for search is too narrow (Record<string, string>)
and rejects valid TanStack Router schemas; update the search typing used in
matchRoutes signature and in state.location (and the other occurrences around
the same area) to a wider type such as Record<string, unknown> (or
Record<string, any>) so non-string search values are accepted; locate the
matchRoutes declaration and the state { location { search } } type blocks and
replace the Record<string, string> occurrences with the wider type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60276abb-c2aa-4f10-860f-3231180d4b00
📒 Files selected for processing (5)
packages/react-on-rails/package.jsonpackages/react-on-rails/src/tanstack-router/clientHydrate.tspackages/react-on-rails/src/tanstack-router/index.tspackages/react-on-rails/src/tanstack-router/serverRender.tspackages/react-on-rails/src/tanstack-router/types.ts
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: Add TanStack Router SSR integration utilityOverall this is a well-structured PR that thoughtfully encapsulates private API workarounds. The documentation is clear and the intent is solid. Here are my findings across the files: Critical / Bugs1. The packages/react-on-rails:
dependencies:
'@tanstack/react-router':
specifier: '>= 1.0.0'
version: 1.163.3(...)...but 2. In 3. Client-side validation silently skips injection; server-side throws In if (typeof router.matchRoutes === 'function' && router.__store?.setState) {
// inject matches
}If the APIs are absent on the client, the injection is silently skipped — the server has injected matches but the client hasn't, guaranteed hydration mismatch with no indication of the root cause. The client should apply the same error throwing that the server does. Design / API Issues4. The function is exported from 5. In app = createElement(options.AppWrapper, { ...props, children: app } as any);
6.
Minor / Nits7. Pinned-version comment is stale
The installed version is 1.163.3. The comment should be updated (or removed and replaced with a link to the relevant TanStack Router source). 8. No tests for the new module There are no unit tests covering |
size-limit report 📦
|
|
Review: TanStack Router SSR Integration - Good overall structure. See inline comments for specific issues. |
react_on_rails_pro/spec/dummy/app/controllers/tanstack_router_controller.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/building-features/tanstack-router.md`:
- Around line 79-84: Update the "Compatibility Notes" section to explicitly
state that sync SSR (renderToString) does not await TanStack Router async route
loaders, and add recommended alternatives: provide data from the
server/controller before rendering, fetch data on the client after hydration, or
use the async render-function SSR path (Pro) that supports awaiting loaders;
reference the terms renderToString, async route loaders, and async
render-function path in the text so readers can locate relevant implementation
points.
In `@packages/react-on-rails/src/tanstack-router/clientHydrate.ts`:
- Around line 55-83: The guard incorrectly checks router.__store?.setState by
truthiness which can allow non-function values; modify the conditional that
currently reads "typeof router.matchRoutes === 'function' &&
router.__store?.setState" to instead assert the setter is a function e.g.
"typeof router.matchRoutes === 'function' && typeof router.__store?.setState ===
'function'" so that router.matchRoutes(...), router.__store.setState(...) block
only runs when both are functions (same pattern used in serverRender.ts),
ensuring the later hasSsrPayload branch throws the intended compatibility error
when internals are missing or invalid.
In `@react_on_rails/lib/react_on_rails/helper.rb`:
- Around line 606-614: The merge of existing_props and client_props in
ReactOnRails::Helper (where render_options.set_option(:props,
existing_props.merge(client_props)) is called) can leave duplicate keys when one
hash uses symbol keys and the other uses string keys; normalize key types before
merging (e.g., convert existing_props keys to strings or client_props keys to
symbols) so client_props reliably override server props, then set the normalized
merged hash via render_options.set_option(:props, ...); also add a unit test
that supplies existing props using a symbol key (e.g., :name) and client_props
using the string key "name" to verify the client value overrides the server
value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 547c2fcd-6ad9-4cab-b7d7-7b9dd22659d6
📒 Files selected for processing (12)
docs/api-reference/view-helpers-api.mddocs/building-features/tanstack-router.mdpackages/react-on-rails/package.jsonpackages/react-on-rails/src/tanstack-router/clientHydrate.tspackages/react-on-rails/src/tanstack-router/serverRender.tspackages/react-on-rails/tests/tanstackRouter.test.tsreact_on_rails/lib/react_on_rails/helper.rbreact_on_rails/spec/dummy/spec/helpers/react_on_rails_helper_spec.rbreact_on_rails/spec/dummy/spec/system/integration_spec.rbreact_on_rails_pro/spec/dummy/app/controllers/tanstack_router_controller.rbreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/TanStackRouterAppAsync.jsxreact_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
🚧 Files skipped from review as they are similar to previous changes (5)
- react_on_rails/spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
- react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
- packages/react-on-rails/package.json
- docs/api-reference/view-helpers-api.md
- react_on_rails_pro/spec/dummy/app/controllers/tanstack_router_controller.rb
Code ReviewOverall this is a well-structured PR that addresses a real need. The runtime validation of internal TanStack Router APIs is a particularly good defensive pattern, and the test coverage is solid. A few issues to address before merging: Bugs / Correctness
existing_props.transform_keys(&:to_s).merge(client_props.transform_keys(&:to_s))
React 18 Strict Mode issue
Code quality
Demo app patternsUnnecessary wrapper in const TanStackRouterApp = createTanStackRouterRenderFunction(options, deps);
ReactOnRails.register({ TanStackRouterApp });If there's a known reason this can't work directly, please document it. Pro controller pre-seeds Minor / Nitpick
|
|
PR Review: Add TanStack Router SSR Integration Utility - see inline comments for details |
|
Overall this is a well-structured and clearly documented addition. The approach of encapsulating private-API workarounds behind a stable public interface is sound, and the test coverage for the main happy-path flows is good. Key issues worth addressing:
|
|
PR Review: TanStack Router SSR Integration. This is a well-structured addition. The architecture of injecting TanStack Router deps as parameters is the right call for an optional peer dependency. The clientProps pass-through is clean and generic enough for future integrations. Issues to address: (1) Bug - React Strict Mode: isFirstRender ref in clientHydrate.ts causes router.load() to run on the discarded render but not the real mount in React 18 Strict Mode, breaking navigation in development. Fix: remove the guard and call router.load() directly in the effect. (2) Bug - searchStr mismatch: validateRouterInternals accepts either search OR searchStr on router.state.location, but injectRouteMatchesSync only reads location.search. If only searchStr exists, the validator passes but injected routes use the wrong search value. (3) Missing router.ssr in async path: serverRenderTanStackAppAsync never sets router.ssr = true. The sync path sets this to prevent TanStack Router Suspense boundaries from interfering with renderToString. The Pro NodeRenderer calls renderToString on the returned element so the same flag is needed. (4) Duplicated normalizeSearch: identical function defined in both serverRender.ts and clientHydrate.ts. Extract to a shared utils.ts. (5) Minor dead code: the if (!store) early return in injectRouteMatchesSync is unreachable since validateRouterInternals already threw. The docs, Ruby merge_server_rendered_client_props!, and test coverage are all solid. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/react-on-rails/tests/serverRenderReactComponent.test.ts (1)
236-259: Consider assertinghasErrorsis false in asyncclientPropspath.This test validates
htmlandclientProps; addinghasErrorskeeps parity with adjacent async tests and catches partial-failure regressions.Suggested test hardening
const result = await renderResult; expect(result.html).toEqual('<div>Hello with async hash client props</div>'); expect(result.clientProps).toEqual({ __tanstackRouterDehydratedState: { url: '/products?category=tools' }, }); + expect(result.hasErrors).toBeFalsy();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails/tests/serverRenderReactComponent.test.ts` around lines 236 - 259, The async test "serverRenderReactComponent processes async serverRenderHash with clientProps" currently asserts html and clientProps but not the error flag; update the test that registers X6WithClientProps and awaits serverRenderReactComponent (the renderResult/result flow) to also assert that result.hasErrors is false to match adjacent async tests and detect partial failures. Locate the test block referencing X6WithClientProps, serverRenderReactComponent, and the result variable and add an assertion that result.hasErrors === false (e.g., expect(result.hasErrors).toBe(false)).packages/react-on-rails/src/tanstack-router/clientHydrate.ts (1)
34-35: Replace duplicated dehydration-prop literal with a constant.Line 34 and Line 107 use the same hardcoded key; centralizing it reduces typo risk.
🧹 Proposed refactor
+const TANSTACK_DEHYDRATED_STATE_PROP = '__tanstackRouterDehydratedState' as const; + function TanStackHydrationApp({ @@ - const dehydratedState = incomingProps.__tanstackRouterDehydratedState as DehydratedRouterState | undefined; + const dehydratedState = incomingProps[ + TANSTACK_DEHYDRATED_STATE_PROP + ] as DehydratedRouterState | undefined; @@ - delete wrapperProps.__tanstackRouterDehydratedState; + delete wrapperProps[TANSTACK_DEHYDRATED_STATE_PROP];Also applies to: 106-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails/src/tanstack-router/clientHydrate.ts` around lines 34 - 35, The code repeats the literal "__tanstackRouterDehydratedState" in clientHydrate.ts (e.g., the dehydratedState assignment and the later lookup around lines 106-108); replace the duplicated string with a single exported/const symbol (e.g., DEHYDRATION_PROP = "__tanstackRouterDehydratedState") and use that constant wherever the literal appears (including the dehydratedState = incomingProps[DEHYDRATION_PROP] cast and the later hasSsrPayload/lookup), so all occurrences reference the same identifier to avoid typos and centralize the key.packages/react-on-rails/src/tanstack-router/serverRender.ts (1)
137-140: Deduplicate dehydrated-state construction in sync/async paths.Line 137-Line 140 and Line 173-Line 176 repeat identical logic; extracting one helper reduces drift.
♻️ Proposed refactor
+function buildDehydratedState(router: TanStackRouter, url: string): DehydratedRouterState { + return { + url, + dehydratedRouter: typeof router.dehydrate === 'function' ? router.dehydrate() : null, + }; +} + export function serverRenderTanStackApp( @@ - const dehydratedState: DehydratedRouterState = { - url, - dehydratedRouter: typeof router.dehydrate === 'function' ? router.dehydrate() : null, - }; + const dehydratedState = buildDehydratedState(router, url); @@ export async function serverRenderTanStackAppAsync( @@ - const dehydratedState: DehydratedRouterState = { - url, - dehydratedRouter: typeof router.dehydrate === 'function' ? router.dehydrate() : null, - }; + const dehydratedState = buildDehydratedState(router, url);Also applies to: 173-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails/src/tanstack-router/serverRender.ts` around lines 137 - 140, The construction of the DehydratedRouterState is duplicated; extract a small helper (e.g., getDehydratedState or buildDehydratedState) that accepts url and router and returns a DehydratedRouterState with url and dehydratedRouter: typeof router.dehydrate === 'function' ? router.dehydrate() : null, then replace both inline creations (the dehydratedState assignments around dehydrated-state construction) to call that helper to remove duplication and keep sync/async paths consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-on-rails/src/isServerRenderResult.ts`:
- Around line 13-17: The type-guard is missing the 'clientProps' property check
so isServerRenderResult incorrectly returns false for objects with only
clientProps; update the guard in isServerRenderResult (the function
isServerRenderResult) to include "'clientProps' in testValue" alongside the
existing checks ('renderedHtml', 'redirectLocation', 'routeError', 'error') so
any ServerRenderResult with clientProps is recognized as such
(ServerRenderResult is defined in types/index.ts).
---
Nitpick comments:
In `@packages/react-on-rails/src/tanstack-router/clientHydrate.ts`:
- Around line 34-35: The code repeats the literal
"__tanstackRouterDehydratedState" in clientHydrate.ts (e.g., the dehydratedState
assignment and the later lookup around lines 106-108); replace the duplicated
string with a single exported/const symbol (e.g., DEHYDRATION_PROP =
"__tanstackRouterDehydratedState") and use that constant wherever the literal
appears (including the dehydratedState = incomingProps[DEHYDRATION_PROP] cast
and the later hasSsrPayload/lookup), so all occurrences reference the same
identifier to avoid typos and centralize the key.
In `@packages/react-on-rails/src/tanstack-router/serverRender.ts`:
- Around line 137-140: The construction of the DehydratedRouterState is
duplicated; extract a small helper (e.g., getDehydratedState or
buildDehydratedState) that accepts url and router and returns a
DehydratedRouterState with url and dehydratedRouter: typeof router.dehydrate ===
'function' ? router.dehydrate() : null, then replace both inline creations (the
dehydratedState assignments around dehydrated-state construction) to call that
helper to remove duplication and keep sync/async paths consistent.
In `@packages/react-on-rails/tests/serverRenderReactComponent.test.ts`:
- Around line 236-259: The async test "serverRenderReactComponent processes
async serverRenderHash with clientProps" currently asserts html and clientProps
but not the error flag; update the test that registers X6WithClientProps and
awaits serverRenderReactComponent (the renderResult/result flow) to also assert
that result.hasErrors is false to match adjacent async tests and detect partial
failures. Locate the test block referencing X6WithClientProps,
serverRenderReactComponent, and the result variable and add an assertion that
result.hasErrors === false (e.g., expect(result.hasErrors).toBe(false)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 119f685d-2083-48ed-a095-8b643f0ca778
📒 Files selected for processing (12)
docs/building-features/tanstack-router.mdpackages/react-on-rails/src/isServerRenderResult.tspackages/react-on-rails/src/serverRenderReactComponent.tspackages/react-on-rails/src/tanstack-router/clientHydrate.tspackages/react-on-rails/src/tanstack-router/serverRender.tspackages/react-on-rails/src/tanstack-router/utils.tspackages/react-on-rails/src/types/index.tspackages/react-on-rails/tests/serverRenderReactComponent.test.tsreact_on_rails/lib/react_on_rails/helper.rbreact_on_rails/spec/dummy/client/app/startup/TanStackRouterApp.jsxreact_on_rails/spec/dummy/spec/helpers/react_on_rails_helper_spec.rbreact_on_rails_pro/spec/dummy/app/controllers/tanstack_router_controller.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- react_on_rails/spec/dummy/client/app/startup/TanStackRouterApp.jsx
- docs/building-features/tanstack-router.md
| return ( | ||
| 'renderedHtml' in testValue || | ||
| 'redirectLocation' in testValue || | ||
| 'routeError' in testValue || | ||
| 'error' in testValue |
There was a problem hiding this comment.
Missing clientProps check in guard.
The ServerRenderResult interface (see types/index.ts:122-128) defines five optional properties: renderedHtml, clientProps, redirectLocation, routeError, and error. This guard only checks for four of them, omitting clientProps.
A ServerRenderResult with only clientProps populated (e.g., { clientProps: { foo: 'bar' } }) would incorrectly return false, causing createReactOutput to treat it as a React element instead of a server render hash.
Proposed fix
return (
'renderedHtml' in testValue ||
+ 'clientProps' in testValue ||
'redirectLocation' in testValue ||
'routeError' in testValue ||
'error' in testValue
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| 'renderedHtml' in testValue || | |
| 'redirectLocation' in testValue || | |
| 'routeError' in testValue || | |
| 'error' in testValue | |
| return ( | |
| 'renderedHtml' in testValue || | |
| 'clientProps' in testValue || | |
| 'redirectLocation' in testValue || | |
| 'routeError' in testValue || | |
| 'error' in testValue | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-on-rails/src/isServerRenderResult.ts` around lines 13 - 17,
The type-guard is missing the 'clientProps' property check so
isServerRenderResult incorrectly returns false for objects with only
clientProps; update the guard in isServerRenderResult (the function
isServerRenderResult) to include "'clientProps' in testValue" alongside the
existing checks ('renderedHtml', 'redirectLocation', 'routeError', 'error') so
any ServerRenderResult with clientProps is recognized as such
(ServerRenderResult is defined in types/index.ts).
| "Pass props as a Hash, not #{class_name}." | ||
| end | ||
|
|
||
| normalized_existing_props = existing_props.deep_stringify_keys |
There was a problem hiding this comment.
Nil props produces a confusing error message.
When react_component("TanStackApp", prerender: true) is called without a props: keyword, render_options.props is nil. The code reaches existing_props = nil, then the unless existing_props.is_a?(Hash) guard raises:
"Cannot merge result['clientProps'] into non-Hash props. Pass props as a Hash, not NilClass."
A user seeing this error from a nil props call won't understand what to fix. Consider treating nil as an empty hash:
| normalized_existing_props = existing_props.deep_stringify_keys | |
| normalized_existing_props = (existing_props || {}).deep_stringify_keys |
| (router as any).ssr = true; | ||
| } | ||
|
|
||
| routerRef.current = router; |
There was a problem hiding this comment.
Render-time side effects with no cleanup path.
The router.update(), router.__store.setState(), and router.ssr = true calls on lines 46–87 are mutating external state during the render phase. While the routerRef.current === null guard makes this safe from double-initialization in practice (React preserves ref values across StrictMode double invocations in React 18+), there is no cleanup path if the component unmounts and remounts (e.g. in React 18 StrictMode or with transitions). In StrictMode, React intentionally unmounts and remounts the component once to detect side effects — the useEffect on line 100 runs the router.load() call, but there's no corresponding teardown if the effect fires, is cleaned up, and fires again.
Consider whether router.load() should be called in the useEffect even when hasSsrPayload is false (so the router loads when navigating to an unrendered page on the client), and whether a return () => { /* cleanup */ } teardown is needed.
| } | ||
|
|
||
| const result = syncRenderFn(propsOrDefault, railsContext); | ||
| return () => result; |
There was a problem hiding this comment.
Suspicious () => result wrapper on client path.
syncRenderFn already returns a ReactElement on the client side (since TanStackHydrationApp is an element). Wrapping it in () => result here returns a function from this render function rather than a ReactElement.
React on Rails will see a function return value and treat it as a "renderer function" (a special kind that takes a DOM node as a third argument), which is not what's intended here.
If the intent is to work around React on Rails' handling of async render functions on the client, a comment explaining why this wrapper is needed would help. Otherwise, consider returning result directly:
| return () => result; | |
| return result; |
| // promiseResult is string | ServerRenderHashRenderedHtml (both are FinalHtmlResult) | ||
| return promiseResult as FinalHtmlResult; | ||
| // promiseResult can be a FinalHtmlResult or a ServerRenderResult hash. | ||
| return promiseResult; |
There was a problem hiding this comment.
processPromise now returns an untyped value.
The old code had an explicit cast return promiseResult as FinalHtmlResult which was at least type-checked at the call site. The new code returns promiseResult without any cast or type annotation, which widens the inferred return type and loses the type safety that downstream consumers relied on.
The result is immediately passed into isServerRenderHash(asyncResult) in createPromiseResult (line ~129), so the behavior is correct at runtime. But explicitly typing the return would make the intent clear:
| return promiseResult; | |
| // promiseResult can be a FinalHtmlResult or a ServerRenderResult hash. | |
| return promiseResult as FinalHtmlResult | ServerRenderResult; |
| * - Without pre-populated matches, SSR output would be empty | ||
| * | ||
| * Uses private API: router.__store.setState() | ||
| * Pinned behavior from @tanstack/react-router@1.163.3 |
There was a problem hiding this comment.
Version pin conflicts with the broad >= 1.0.0 peer dependency range.
The comment says the behavior was pinned against @tanstack/react-router@1.163.3, but the peerDependencies range in package.json accepts >= 1.0.0, which includes all of v1.x and any future major versions. The validateRouterInternals function does catch breakage at runtime, but a user upgrading @tanstack/react-router from e.g. 1.50 to 2.0 could hit confusing runtime failures in production.
Consider:
- Tightening the optional peer dependency to
>=1.0.0 <2.0.0(or similar), and - Including the compatible version range in the
validateRouterInternalserror message so users know which versions are known-good.
| (testValue as ServerRenderResult).redirectLocation || | ||
| (testValue as ServerRenderResult).routeError || | ||
| (testValue as ServerRenderResult).error | ||
| export function isServerRenderHash(testValue: unknown): testValue is ServerRenderResult { |
There was a problem hiding this comment.
Unused imports after the signature change.
CreateReactOutputResult and RenderFunctionResult are no longer used in this file — the signature changed to testValue: unknown. The import on lines 2–4 is dead code and will cause a TypeScript/lint warning. The RenderStateHtml import on line 5 is also not used here.
| export function isServerRenderHash(testValue: unknown): testValue is ServerRenderResult { | |
| import type { ServerRenderResult } from './types/index.ts'; |
| console.error('react-on-rails/tanstack-router: Error loading routes after hydration:', err); | ||
| }); | ||
| } | ||
| }, [hasSsrPayload]); // eslint-disable-line react-hooks/exhaustive-deps -- router is a ref, intentionally stable |
There was a problem hiding this comment.
router is missing from the dependency array, and the reason given is questionable.
The comment says "router is a ref, intentionally stable" — but router here is routerRef.current (a value snapshot), not routerRef (the ref object). Accessing .current inside a useEffect is generally fine since refs are stable, but eslint-plugin-react-hooks flags routerRef.current reads inside effects as potentially stale. A cleaner pattern is to include routerRef in the deps (always stable) instead of suppressing the warning:
| }, [hasSsrPayload]); // eslint-disable-line react-hooks/exhaustive-deps -- router is a ref, intentionally stable | |
| }, []); // router is derived from routerRef which is stable; hasSsrPayload is derived from props and won't change after mount |
Or restructure to use routerRef directly:
useEffect(() => {
if (hasSsrPayload && routerRef.current) {
routerRef.current.load().catch(...);
}
}, []); // eslint-disable-line react-hooks/exhaustive-deps
Review: TanStack Router SSR IntegrationOverall this is a well-structured addition. The encapsulation of TanStack Router's private API workarounds in one place, runtime validation via Issues FoundBugs / Correctness
Code Quality / Type Safety
Architecture / Compatibility Risk
Minor Notes
|
1 participant
Summary
Adds
createTanStackRouterRenderFunction()to thereact-on-railsnpm package, providing a render function factory for TanStack Router apps with server-side rendering support.This addresses the need identified in printivity/printivity#2571 and react_on_rails-demos#104 where TanStack Router SSR requires private API workarounds (
router.__store.setState()androuter.ssr = true).What this does
__store.setState()and renders withrenderToStringrouter.load()after mountserverRenderTanStackAppAsync()for Pro NodeRenderer withrendering_returns_promises(uses publicawait router.load()— no hacks needed)Usage
Files changed
packages/react-on-rails/src/tanstack-router/— 4 files (index, types, serverRender, clientHydrate)packages/react-on-rails/package.json— new export + optional peer dependencyFuture work (not in this PR)
rendering_returns_promises(Pro issue, Documentation: ExecJS SSR Timer Limitations and Solutions #2299)skipInitialLoadoption to TanStack RouterCloses #2298
Test plan
pnpm --filter react-on-rails run buildpnpm --filter react-on-rails run testpnpm run lint🤖 Generated with Claude Code
Note
Cursor Bugbot is generating a summary for commit 5226885. Configure here.
Summary by CodeRabbit