Walkthrough

The react_on_rails:doctor task now resolves server bundle source paths by probing for alternative file extensions (.js, .jsx, .ts, .tsx, .mjs, .cjs) before warning about missing bundles, preventing false positives for configurations using different source file types.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A bundle by any extension,
Still hops along with the same intention!
Doctor now finds .ts and .js,
No false alarms—just perfect bliss! 🌟

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 @coderabbitai help to get the list of available commands and usage tips.}

Review

The fix is correct and well-motivated. The false positive occurred because determine_server_bundle_path returned a .js path verbatim, which check_javascript_bundles then tested with File.exist? — naturally failing when the source file was .ts. Resolving sibling extensions before returning the path is the right place to handle this.

Issues

Redundant File.exist? in the candidate loop (see inline on doctor.rb)

resolve_server_bundle_source_path already calls File.exist?(bundle_path) and returns early if it exists. server_bundle_source_extensions_for then prepends the original extension back into the candidate list, so the first loop iteration calls File.exist?("#{base_path}#{extension}") — which reconstructs the original path — a check guaranteed to return false. Excluding the original extension from the candidates avoids this.

Existing test now has implicit filesystem dependency (see inline on doctor_spec.rb)

The "uses Shakapacker API configuration with relative paths" test (unchanged) now runs File.exist? against real paths on the host for all six candidate extensions. It passes only because those files don't exist in the test environment. Adding File.exist? stubs to that test (or a dedicated unit test for resolve_server_bundle_source_path) would make this hermetic.

Minor

• CHANGELOG update is unchecked in the PR description — worth confirming whether this is intentional.
• The new info message ("Source entrypoint can use .js/.jsx/.ts/.tsx suffixes") is placed inside the server_bundle_js_file not configured branch of check_server_bundle_prerender_consistency. The actual fix is transparent to the user (no extra output when the TS file is found), so this hint only surfaces when SSR is enabled but server_bundle_js_file is not set — a reasonable spot.

Review: Doctor accepts TS/TSX server bundle suffixes

The overall approach is sound and addresses a real user pain point. The resolve_server_bundle_source_path helper is clean and the regex (%r{\.[^./]+\z}) correctly strips only the file extension without touching directory components.

A few things worth addressing:

Redundant extension re-check

In server_bundle_source_extensions_for, when a bundle_path has an extension (e.g. .js), the method prepends that same extension to the candidate list:

([extension] + SERVER_BUNDLE_SOURCE_EXTENSIONS).uniq

But by the time this method is called, resolve_server_bundle_source_path has already confirmed the file at bundle_path does not exist. The base_path + original_extension candidate is therefore guaranteed to produce the same non-existent path. The .uniq prevents a duplicate when the extension is already in SERVER_BUNDLE_SOURCE_EXTENSIONS (e.g. .js), but for extensions outside the set (e.g. a hypothetical .es) it adds a redundant File.exist? call. Simplest fix: always return only SERVER_BUNDLE_SOURCE_EXTENSIONS since we already tried the original.

Misleading placement of the new info message

The new line:

💡 Source entrypoint can use .js/.jsx/.ts/.tsx suffixes

is emitted in the branch where server_bundle_js_file is not configured at all. The TS/TSX auto-resolution only kicks in when a bundle filename is configured—so this hint appears in a context where it doesn't yet apply. Consider moving it to the elsif server_bundle_set branch or to the success path where the feature is actually exercised.

Test coverage gap

The new test exercises only .ts. It would be good to add at least one more case (e.g. .tsx) and a case where no alternative extension exists (fallback returns the original .js path unchanged). The latter is implicitly covered by the pre-existing "relative paths" test, but making it explicit would document the intended fallback behaviour.

Greptile Summary

Fixes a false-positive doctor warning where server_bundle_js_file = \"server-bundle.js\" was flagged as missing when the actual Shakapacker source entrypoint was server-bundle.ts (or another TS/JSX variant). The new resolve_server_bundle_source_path helper tries all common source extensions before falling back to the configured name, and a spec covers the primary .js→.ts resolution case.

Confidence Score: 5/5

Safe to merge; both findings are minor style suggestions that do not affect runtime correctness.

All findings are P2 — an incomplete info message string and a test isolation nit. The core fix logic is correct, the regex properly strips extensions, and the fallback behavior is sound.

No files require special attention.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[determine_server_bundle_path] --> B[Build bundle_path from source_path + entry + filename]
    B --> C[resolve_server_bundle_source_path]
    C --> D{File.exist? bundle_path?}
    D -- yes --> E[Return bundle_path]
    D -- no --> F[Strip extension to base_path]
    F --> G[server_bundle_source_extensions_for .js .jsx .ts .tsx .mjs .cjs]
    G --> H{Try each extension}
    H -- exists --> I[Return candidate_path]
    H -- none found --> J[Return original bundle_path]
    E --> K[check_javascript_bundles File.exist? check]
    I --> K
    J --> K
    K -- exists --> L[Success]
    K -- missing --> M[Warning]

_{Reviews (1): Last reviewed commit: "Document doctor TS entrypoint fix in cha..." | Re-trigger Greptile}

Review: Doctor accepts TS/TSX server bundle suffixes

Overall: Looks good — the fix is correct and the core logic is sound. A few things worth addressing before merge.

What works well

• The regex %r{\.[^./]+\z} correctly strips only the final extension and handles directory names that contain dots (e.g. client/app.v2/packs/server-bundle.js).
• Excluding the original extension from the candidate list avoids a redundant re-check after the first File.exist? guard.
• SERVER_BUNDLE_SOURCE_EXTENSIONS is properly frozen.
• The fallback (return original path when no alternative exists) preserves the existing warning behaviour precisely.
• Changelog entry is accurate and well-placed.

Issues to address

1. Filesystem-dependent tests in unchanged contexts (medium)

Three existing contexts now implicitly hit the real filesystem through resolve_server_bundle_source_path but have no File.exist? stubs:

• "with absolute paths" → probes client/app/packs/server-bundle.{js,jsx,ts,tsx,mjs,cjs}
• "with nested absolute paths" → same
• "when Shakapacker gem is not available" → probes app/javascript/packs/server-bundle.*

All three pass today because those files don't exist in CI, but they'll give a false green if extension resolution is broken for those paths. The relative-paths context already shows the right pattern — apply the same allow(File).to receive(:exist?) stub to those three before blocks.

2. No TypeScript test for the rescue path (low)

The rescue branch ("when Shakapacker gem is not available") also routes through resolve_server_bundle_source_path now, but only the happy-path (no TS file found) is tested. A companion it "accepts a TypeScript…" case mirroring the one added here would give complete coverage.

3. server_bundle_source_extensions_for edge case with non-source extensions (low)

If bundle_path carries an extension that isn't in SERVER_BUNDLE_SOURCE_EXTENSIONS (e.g. .map), the method returns the full list. Combined with the base-path strip, this produces paths like server-bundle.js.js. This can't happen with the current callers, but the inline suggestion I left makes the guard explicit and self-documenting.

4. Double File.exist? call (trivial)

resolve_server_bundle_source_path verifies the file exists before returning it, and check_javascript_bundles then calls File.exist? on the returned path again (line 213). No correctness impact — just a redundant syscall.

Review Comment Summary

Triaged 14 review comments across 9 threads. All threads are now resolved.

Fixed (2 items, commit 7570857)

• Test hermiticity: Added File.exist? stubs to 3 test contexts (absolute paths, nested absolute paths, LoadError rescue) that were implicitly filesystem-dependent after resolve_server_bundle_source_path was introduced
• Rescue path TS coverage: Added a TypeScript resolution test for the "Shakapacker gem not available" (LoadError) path

Already addressed in dc646ad (4 items)

• Redundant extension re-check → changed to reject
• SERVER_BUNDLE_SOURCE_EXTENSIONS returned directly → uses reject instead
• Info hint in wrong branch → message removed
• Info message missing .mjs/.cjs → message removed

Declined with rationale (3 items)

• Misconfigured extension passing doctor → doctor resolves source entrypoints, not runtime config validation
• Double File.exist? call → trivial, keeps check_javascript_bundles self-contained
• Non-source extension edge case → can't happen with current callers

Review: Doctor TS/TSX server bundle suffix fix

Overall this is a clean, focused, low-risk fix for a genuine false-positive in react_on_rails:doctor. The logic is correct and the CHANGELOG entry is clear.

How the fix works

resolve_server_bundle_source_path is called from determine_server_bundle_path. It:

1. Returns the original path immediately if it exists (no change in behavior for existing setups).
2. Strips the extension and tries each entry in SERVER_BUNDLE_SOURCE_EXTENSIONS until it finds one that exists on disk.
3. Falls back to the original (non-existing) path if nothing matches — preserving the existing warning behavior in check_server_bundle.

The caller at line 213 then does File.exist? on whatever is returned, so the warning/success messaging remains accurate. Correct end-to-end.

Issues noted (inline)

• server_bundle_source_extensions_for naming + edge case — the method excludes the original extension only when it's in the known list; if someone configures an unlisted extension (e.g. .mts), the full list including .js is tried, which could resolve to an unrelated file. Worth documenting the intent.
• Opaque regex — a one-line comment on the base_path substitution would help future readers understand why [^./] is used.
• Test coverage gap — the absolute-path contexts received before-block stubs but no TS-resolution it block; only .ts is tested across the new cases, not .tsx/.jsx/.mjs/.cjs.

None of these block the merge — the core logic is sound and the primary use case is well-covered.