fix(form-core): 🐛 File input field - Picking a different file does not trigger rerender (deep equality issue with File/Blob objects)#1939
Conversation
|
|
View your CI Pipeline Execution ↗ for commit 452a71f
☁️ 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 #1939 +/- ##
==========================================
- Coverage 90.35% 90.17% -0.19%
==========================================
Files 38 49 +11
Lines 1752 2015 +263
Branches 444 524 +80
==========================================
+ Hits 1583 1817 +234
- Misses 149 178 +29
Partials 20 20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…trigger-rerender' of github-personal.com:joaoGabriel55/form into fix/file-input-field-picking-a-different-file-does-not-trigger-rerender
LeCarbonator
left a comment
LeCarbonator
left a comment
There was a problem hiding this comment.
It looks like the previous comment hasn't been addressed between the previous review and this review request.
The actual required changes don't look that bad, though. Perhaps I'll have some time to help out later this week with that.
…d-picking-a-different-file-does-not-trigger-rerender
…d-picking-a-different-file-does-not-trigger-rerender
|
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)
📝 WalkthroughWalkthroughTreats Blob/File objects in form-core equality checks as referentially compared (avoid key-based comparison). Adds tests validating File/Blob equality semantics and form field behavior when setting File values. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
♻️ Duplicate comments (1)
packages/form-core/tests/FormApi.spec.ts (1)
4104-4128:⚠️ Potential issue | 🟡 MinorGuard this test for runtimes where
Fileis not globally available.
new File(...)on Line 4113 and Line 4119 will throw in environments withoutglobalThis.File, so the test can fail before validating behavior.Proposed patch
it('should detect file value changes when setting a different File', () => { + if (typeof File === 'undefined') { + return + } + const form = new FormApi({ defaultValues: { avatar: undefined as File | undefined, }, })#!/bin/bash set -euo pipefail echo "== Node/runtime constraints ==" if [ -f package.json ]; then jq '.engines // {}, .volta // {}' package.json fi echo "== Vitest config files ==" fd -t f 'vitest.config.*|vite.config.*' echo "== Test setup/environment hints ==" rg -n "environment|jsdom|happy-dom|globalThis\\.File|from 'node:buffer'|undici" -g "**/*vitest*.*" -g "**/*setup*.*" -g "**/*.ts"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/form-core/tests/FormApi.spec.ts` around lines 4104 - 4128, The test constructs new File instances which will throw in runtimes without a global File; guard the test by checking for File availability and skip or short-circuit it when absent. Update the test around the usage of FormApi, setFieldValue and getFieldValue so it only runs if typeof globalThis.File !== 'undefined' (or use your test runner's skip mechanism) to avoid calling new File(...) in environments that don't provide File.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/form-core/tests/FormApi.spec.ts`:
- Around line 4104-4128: The test constructs new File instances which will throw
in runtimes without a global File; guard the test by checking for File
availability and skip or short-circuit it when absent. Update the test around
the usage of FormApi, setFieldValue and getFieldValue so it only runs if typeof
globalThis.File !== 'undefined' (or use your test runner's skip mechanism) to
avoid calling new File(...) in environments that don't provide File.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5bb662cb-1e58-434d-9bfe-9395e9dd2c2a
📒 Files selected for processing (3)
packages/form-core/src/utils.tspackages/form-core/tests/FormApi.spec.tspackages/form-core/tests/utils.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/some-ducks-write.md:
- Line 2: The changeset currently targets `@tanstack/react-form` but the bugfix
lives in form-core (the evaluate function in evaluate within
packages/form-core/src/utils.ts), so update the changeset to target
`@tanstack/form-core` (e.g., replace '@tanstack/react-form': patch with
'@tanstack/form-core': patch) so the core package is versioned and published;
keep the release type (patch) and ensure the changeset title/body reflect the
fix in evaluate to avoid releasing only the framework re-exports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb3ebf72-5caa-472c-af98-dc8df1658fc5
📒 Files selected for processing (1)
.changeset/some-ducks-write.md
|
Sorry for the delay with that @LeCarbonator |
3 participants
🎯 Changes
Issue: #1932
Fix: File input field — picking a different file does not trigger rerender due to deep equality issue with
File/Blobobjects.Root cause:
Object.keys(new File(...))returns[](empty array). Theevaluate()function compares objects by iterating overObject.keys, so two differentFileinstances with zero own enumerable keys are considered "equal" — the for-loop vacuously succeeds. This meanssetFieldValuewith a newFiledoesn't trigger a state change.Fix: Add a
Blobguard inevaluate()(placed after theObject.isreferential check and null checks, but before the generic key comparison). SinceFile extends Blob, this catches both types. If twoBlob/Fileobjects are the same reference,Object.isalready returnstrueupstream. If they're different references, we returnfalse— correctly triggering a rerender.React Native compatibility: The guard uses
typeof Blob !== 'undefined'to preventReferenceErrorin environments whereBlobis not defined. IfBlobdoesn't exist, the check is skipped entirely — which is correct since noBlobobjects can exist in that environment either.Changes:
evaluate()inutils.ts: addedBlobinstance guard (+12 lines)utils.spec.ts: addedevaluatetests forFileandBlobobjects (same ref, different refs, nested in objects)FormApi.spec.ts: added integration test forsetFieldValuewithFilevalues✅ Checklist
pnpm test:pr.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
Tests
Chores