fix(router-plugin): escape single quotes in code-split importer paths#7760
fix(router-plugin): escape single quotes in code-split importer paths#7760thribhuvan003 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds an internal helper to escape backslashes and single quotes when generating single-quoted dynamic import strings in the code-splitter compiler, applies it at both importer generation sites, and adds test and changeset coverage for paths containing apostrophes. ChangesPath escaping fix
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/router-plugin/tests/code-splitter-path-escaping.test.ts (2)
64-64: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard against
nullcompile result for clearer test failures.
compileCodeSplitReferenceRoutecan returnnullwhen no modifications are made. If the route stops being split in a future refactor,compileResult.codethrows aTypeErrorrather than a clear assertion failure. A quick null check would improve diagnostics:🛡️ Proposed guard
- expect(getImporterUrls(compileResult.code)).toContain( + expect(compileResult).not.toBeNull() + expect(getImporterUrls(compileResult!.code)).toContain(🤖 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 `@packages/router-plugin/tests/code-splitter-path-escaping.test.ts` at line 64, Add a null guard around the result of compileCodeSplitReferenceRoute in code-splitter-path-escaping.test.ts before accessing compileResult.code, so the test fails with a clear assertion instead of a TypeError. Update the assertion near getImporterUrls to first verify compileResult is not null, using the compileResult variable name and the compileCodeSplitReferenceRoute call site as the lookup points.
38-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a backslash path test case.
The test covers single-quote escaping but doesn't exercise backslash escaping, which is the other half of
escapeSingleQuotedString. A path likeC:\Users\dev\project\src\routes\index.tsx(or a POSIX path with a literal backslash) would verify both branches of the helper. This is optional since both branches share the same function and the single-quote case is the reported bug.🤖 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 `@packages/router-plugin/tests/code-splitter-path-escaping.test.ts` around lines 38 - 67, Add a second path-escaping test in the code-splitter path escaping suite to cover backslash handling in addition to the existing single-quote case. Extend the coverage around compileCodeSplitReferenceRoute and getImporterUrls by using a filename that contains literal backslashes (for example a Windows-style path or a POSIX path with a backslash) so escapeSingleQuotedString is exercised for both escaping branches. Keep the existing single-quote test and verify the generated importer URL round-trips the full filename exactly.
🤖 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 `@packages/router-plugin/tests/code-splitter-path-escaping.test.ts`:
- Line 64: Add a null guard around the result of compileCodeSplitReferenceRoute
in code-splitter-path-escaping.test.ts before accessing compileResult.code, so
the test fails with a clear assertion instead of a TypeError. Update the
assertion near getImporterUrls to first verify compileResult is not null, using
the compileResult variable name and the compileCodeSplitReferenceRoute call site
as the lookup points.
- Around line 38-67: Add a second path-escaping test in the code-splitter path
escaping suite to cover backslash handling in addition to the existing
single-quote case. Extend the coverage around compileCodeSplitReferenceRoute and
getImporterUrls by using a filename that contains literal backslashes (for
example a Windows-style path or a POSIX path with a backslash) so
escapeSingleQuotedString is exercised for both escaping branches. Keep the
existing single-quote test and verify the generated importer URL round-trips the
full filename exactly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b417562e-ecfd-46b9-b81b-7d37e3ee1899
📒 Files selected for processing (2)
packages/router-plugin/src/core/code-splitter/compilers.tspackages/router-plugin/tests/code-splitter-path-escaping.test.ts
1 participant
when the project path contains a single quote (e.g.
/Users/dev/it's a repro/...), the code-splitter interpolates the filename straight intoimport('<path>?tsr-split=...'), so the apostrophe terminates the string literal early and babel throws while parsing the generated statement — every route in the project fails to compile.fixed by escaping
'and\before interpolating. kept the single-quoted style instead of e.g.JSON.stringifyso the existing snapshots stay byte-identical (tried stringify first, it churned all 177 of them for no reason).added a test that compiles a route from such a path for both react and solid — it fails without the fix, and the full router-plugin suite is unchanged with it.
fixes #7754
Summary by CodeRabbit
Bug Fixes
') and backslashes (\).?tsr-split=componentquery so compiled routes generate valid output.Tests
import('...')specifier.