feat(form-core): 5-10x faster makePathArray#2152
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 (2)
📝 WalkthroughWalkthroughThe PR optimizes the path parsing function ChangesPath Parsing Optimization
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/form-core/src/utils.ts (1)
227-230: 💤 Low valueIncomplete comment — "for these because." is a sentence fragment.
✏️ Suggested fix
- // for these because. + // for these, because the old regex pipeline always produced at least one empty string + // from the split even when the input was only separator/meta characters.🤖 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/form-core/src/utils.ts` around lines 227 - 230, The comment above the conditional that checks "if (!result.length) result.push('')" is a fragment; update it to a complete sentence that explains why the old implementation returned [''] (i.e., when the input contained only phantom characters like ']', '[]', '[]]' producing no segments, the old behavior produced a single empty segment to represent an empty path). Edit the comment near the result variable and the conditional so it reads as a full explanatory sentence referencing the phantom-char inputs and the intended empty-segment fallback.
🤖 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 `@packages/form-core/src/utils.ts`:
- Around line 188-198: The current logic in the segment parsing branch
(variables: treatAsNumber, allDigits, segLen, str.charCodeAt(segStart) ===
CC_ZERO, parseInt call and result.push) unconditionally pushes parseInt(...)
which loses precision for integers > Number.MAX_SAFE_INTEGER; add a round-trip
guard: when treatAsNumber is true, parse the segment into a number (e.g., parsed
= parseInt(str.slice(segStart, i), 10)) but only push parsed if String(parsed)
=== the original segment string; otherwise push the original slice
(str.slice(segStart, i)) so large integer strings are preserved as strings.
In `@packages/form-core/tests/utils.bench.ts`:
- Around line 4-6: The file contains a temporary snapshot and an inline legacy
function makePathArrayOld plus a "remove this" comment and paired benches that
must not land on main; remove the makePathArrayOld function and the accompanying
comment and any benchmark comparisons that reference it (e.g., paired benches)
and restructure the test to benchmark only the current live implementation (or
delete the whole bench file if benchmarking was never intended to be shipped);
ensure any helper regex like reLineOfOnlyDigits and tests only used by the old
implementation are also removed or repurposed so the file contains solely the
intended, production benchmark code.
In `@packages/react-form/tests/field-render-perf.test.tsx`:
- Line 29: This test file is marked for removal ("NOTE: This file is intended to
be removed before merge.") and should not land in main; remove the temporary
perf test from the PR (or move it out of the commit into a dedicated draft
branch) and instead open a follow-up issue or add a CI-safe permanent test in
the proper tests directory so the throwaway file does not get merged.
- Around line 178-184: The test uses import.meta.dirname when building mountPath
and unmountPath which fails on Node <21.2.0; add a fallback that computes a
dirname from fileURLToPath(import.meta.url) and use that variable instead of
import.meta.dirname. Specifically, near the top of the test module define a
const (e.g., testDir or dirname) that sets dirname = import.meta.dirname ??
path.dirname(fileURLToPath(import.meta.url)) and then update the join calls that
create mountPath and unmountPath (and any other uses of import.meta.dirname) to
use that dirname; reference functions/symbols: mountPath, unmountPath,
PROFILE_N, join, import.meta.dirname, import.meta.url, fileURLToPath.
---
Nitpick comments:
In `@packages/form-core/src/utils.ts`:
- Around line 227-230: The comment above the conditional that checks "if
(!result.length) result.push('')" is a fragment; update it to a complete
sentence that explains why the old implementation returned [''] (i.e., when the
input contained only phantom characters like ']', '[]', '[]]' producing no
segments, the old behavior produced a single empty segment to represent an empty
path). Edit the comment near the result variable and the conditional so it reads
as a full explanatory sentence referencing the phantom-char inputs and the
intended empty-segment fallback.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2789ad5-a68f-4f50-a809-80d8c5d11698
📒 Files selected for processing (7)
.gitignorepackages/form-core/package.jsonpackages/form-core/src/utils.tspackages/form-core/tests/utils.bench.tspackages/form-core/tests/utils.spec.tspackages/form-core/vite.config.tspackages/react-form/tests/field-render-perf.test.tsx
|
This is awesome, thank you! Consider the new code "LGTM"'d. Let's go ahead and remove the Let me know what you need from us! |
|
View your CI Pipeline Execution ↗ for commit 5ca4c18
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2152 +/- ##
==========================================
+ Coverage 90.35% 90.46% +0.10%
==========================================
Files 38 49 +11
Lines 1752 2065 +313
Branches 444 546 +102
==========================================
+ Hits 1583 1868 +285
- Misses 149 177 +28
Partials 20 20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| stats-hydration.json | ||
| stats.json | ||
| stats.html | ||
| *.cpuprofile |
There was a problem hiding this comment.
Figured I might as well leave this
|
@crutchcorn Amazing! I removed all the benchmarks and cleaned up the comments in the new tests, so I think it's ready to be merged now. But let me know if you need anything else. |
|
Just a little bump as it has been 2 weeks. Would be great to see this merged, we've run into the same performance bottleneck :) |
crutchcorn
left a comment
crutchcorn
left a comment
There was a problem hiding this comment.
Sorry for delay in merging - been overwhelmed lately with other stuff. Once it passes CI, we'll merge and release
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2152 +/- ##
==========================================
+ Coverage 90.35% 91.43% +1.08%
==========================================
Files 38 59 +21
Lines 1752 2336 +584
Branches 444 582 +138
==========================================
+ Hits 1583 2136 +553
- Misses 149 179 +30
- Partials 20 21 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
🎯 Changes
NB: I used Claude Code when doing this work (but not to write this description).
In #2150 I noted that mounting
<form.Field>is slow because it exhibits O(N^2) complexity, where N is the number of fields in the form. This PR does not fix the O(N^2) behavior, but when profiling it, I noticed thatmakePathArrayis a super hot path.On
main,makePathArrayincludes a bunch of regex string munging. I rewrote it as a single-pass for loop. This produces 5–10x speed up in microbenchmarks, and what appears to be ~2x faster<form.Field>mounting/unmounting.I also wrote additional tests for
makePathArrayto try to capture its edge case behavior (what I think are malformed inputs—things not allowed byDeepKeys<T>). I did my best to maintain backwards compatibility, but you will see there is one BC-break I identified:I may have missed some edge cases not covered by my new tests. It's possible to match the old behavior more closely. Claude's original implementation was much more complex and handled more edge cases, but I opted to simplify so that the code was easier to understand. Happy to adjust.
Benchmarks
This branch includes benchmark files I would not expect to be merged, but wanted to include temporarily so others can validate my results (run on my M4 Pro MackBook Pro).
Here's the output for the new
utils.bench.ts:In addition, I (well, Claude) wrote
field-render-perf.test.tsx, which is an automated version of the reproduction I put together for #2150.Results with the old
makePathArray():Results with the new
makePathArray():~2x faster!
✅ Checklist
pnpm test:pr.🚀 Release Impact
This change is docs/CI/dev-only (no release).Summary by CodeRabbit