fix(solid-form): prevent full array re-renders in array mode#2169
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 as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR makes Solid form fields mode-aware for arrays: exports ChangesArray Mode Support for Solid Form Fields
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
View your CI Pipeline Execution ↗ for commit 5f945e4
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview3 package(s) bumped directly, 11 bumped as dependents. 🟨 Minor bumps
🟩 Patch bumps
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/solid-form/src/createField.tsx (1)
90-130:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
modevariable is not defined in scope.The function
makeFieldReactivedeclares only one parameterfieldApi(lines 90–130), but line 177 referencesmodedirectly, which is not in scope. Additionally, line 331 callsmakeFieldReactive(extendedApi as never, options.mode)with two arguments, but the function signature accepts only one.Fix: Add
modeas a second parameter tomakeFieldReactive:🐛 Proposed fix
Update the function signature to accept
modeas a second parameter:>( fieldApi: FieldApi< // ... (type parameters omitted for brevity) > & SolidFieldApi< // ... > & - FieldOptionsMode, + FieldOptionsMode, + mode?: 'value' | 'array', ): () => FieldApi<Alternatively, if
modeshould be accessed fromfieldApi:const reactiveStateValue = useStore(fieldApi.store, (state) => - mode === 'array' + fieldApi.mode === 'array' ? Object.keys((state.value as unknown) ?? []).length : state.value, )And update the call site:
- >(extendedApi as never, options.mode) + >(Object.assign(extendedApi, { mode: options.mode }) as never)Also applies to: 170-180
🤖 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/solid-form/src/createField.tsx` around lines 90 - 130, The function makeFieldReactive currently only accepts fieldApi but references mode and is invoked with two args (e.g., makeFieldReactive(extendedApi as never, options.mode)); fix by adding a second parameter named mode to makeFieldReactive's signature and use that parameter instead of referencing an undefined variable, and update any call sites to pass the correct options.mode (or alternatively read mode from fieldApi if intended) — ensure references inside makeFieldReactive and all invocations (such as the call with extendedApi and options.mode) are consistent with the new two-argument signature.
🧹 Nitpick comments (1)
packages/solid-form/tests/createField.test.tsx (1)
571-597: ⚖️ Poor tradeoffConsider testing the array-length optimization.
The current test verifies that array mode works, but doesn't confirm that the optimization (tracking array length instead of full value) prevents unnecessary re-renders when nested properties change.
Consider adding a test that mutates a nested property and verifies the parent field doesn't re-render.
🤖 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/solid-form/tests/createField.test.tsx` around lines 571 - 597, Add a new test that verifies the array-length optimization prevents parent re-renders when a nested item is mutated: render a component using createForm with defaultValues.test = [{ x: 1 }] and a form.Field name="test" mode="array" that increments a render counter (ref or jest.fn) each render and displays field().state.value; then mutate a nested property on the first item via the nested-field API (e.g., call the nested item's setter / setValue / setAt) and assert the nested item updates but the parent field's render counter did not increment (i.e., parent did not re-render), while ensuring the visible nested value changed; reference createForm, form.Field, mode="array", field().state.value and the nested field setter when adding this assertion.
🤖 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.
Outside diff comments:
In `@packages/solid-form/src/createField.tsx`:
- Around line 90-130: The function makeFieldReactive currently only accepts
fieldApi but references mode and is invoked with two args (e.g.,
makeFieldReactive(extendedApi as never, options.mode)); fix by adding a second
parameter named mode to makeFieldReactive's signature and use that parameter
instead of referencing an undefined variable, and update any call sites to pass
the correct options.mode (or alternatively read mode from fieldApi if intended)
— ensure references inside makeFieldReactive and all invocations (such as the
call with extendedApi and options.mode) are consistent with the new two-argument
signature.
---
Nitpick comments:
In `@packages/solid-form/tests/createField.test.tsx`:
- Around line 571-597: Add a new test that verifies the array-length
optimization prevents parent re-renders when a nested item is mutated: render a
component using createForm with defaultValues.test = [{ x: 1 }] and a form.Field
name="test" mode="array" that increments a render counter (ref or jest.fn) each
render and displays field().state.value; then mutate a nested property on the
first item via the nested-field API (e.g., call the nested item's setter /
setValue / setAt) and assert the nested item updates but the parent field's
render counter did not increment (i.e., parent did not re-render), while
ensuring the visible nested value changed; reference createForm, form.Field,
mode="array", field().state.value and the nested field setter when adding this
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6607fe6c-9dfd-469d-a578-fb5e3260a148
📒 Files selected for processing (5)
docs/framework/solid/guides/arrays.mdexamples/solid/array/src/index.tsxpackages/solid-form/src/createField.tsxpackages/solid-form/src/types.tspackages/solid-form/tests/createField.test.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2169 +/- ##
==========================================
+ Coverage 90.35% 97.65% +7.30%
==========================================
Files 38 4 -34
Lines 1752 128 -1624
Branches 444 12 -432
==========================================
- Hits 1583 125 -1458
+ Misses 149 3 -146
+ Partials 20 0 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Mirrors #1963 but for the Solid.js adapter
Summary by CodeRabbit
Documentation
Bug Fixes
Tests
Chores