fix: FileUpload with list binding in RepeatingGroup fails to render with 2+ pre-populated values#4242
fix: FileUpload with list binding in RepeatingGroup fails to render with 2+ pre-populated values#4242adamhaeger wants to merge 6 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 45 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR fixes a bug where FileUpload components inside RepeatingGroups fail when list bindings contain 2+ pre-populated values. The fix adds explicit deletion of existing field values before writing converted list data, preventing dot-object override conflicts. A regression test validates the update path. ChangesFix for pre-populated list binding write conflicts
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
adamhaeger
added
the
backport
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/features/formData/FormData.test.tsx`:
- Line 791: Replace the type assertion on "list" by performing a runtime type
guard on formData.list: check that formData.list is an array and that every
element is a string, and only then assign it to the variable list; otherwise
fall back to an empty array. Update the assignment to use that guard
(referencing formData.list and the local variable list) so the test no longer
uses "as string[]" and preserves proper runtime safety.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 525e03ee-bd27-4b0f-b4ba-30d415ec76cd
📒 Files selected for processing (2)
src/features/formData/FormData.test.tsxsrc/features/formData/FormDataWriteStateMachine.tsx
| DEFAULT_DEBOUNCE_TIMEOUT, | ||
| 'raw', | ||
| ); | ||
| const list = (formData.list ?? []) as string[]; |
There was a problem hiding this comment.
Avoid type casting in favor of runtime check.
The type assertion as string[] violates the coding guideline. Use a runtime type guard instead to maintain type safety without casting.
✨ Proposed fix using runtime check
- const list = (formData.list ?? []) as string[];
+ const list = Array.isArray(formData.list) ? formData.list : [];As per coding guidelines, avoid using type casting (as type) in TypeScript; instead, improve typing by removing such casts to maintain proper type safety.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const list = (formData.list ?? []) as string[]; | |
| const list = Array.isArray(formData.list) ? formData.list : []; |
🤖 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 `@src/features/formData/FormData.test.tsx` at line 791, Replace the type
assertion on "list" by performing a runtime type guard on formData.list: check
that formData.list is an array and that every element is a string, and only then
assign it to the variable list; otherwise fall back to an empty array. Update
the assignment to use that guard (referencing formData.list and the local
variable list) so the test no longer uses "as string[]" and preserves proper
runtime safety.
adamhaeger
added
the
backport-ignore
phlipsterit
added
the
sync-v9
|
2 participants
Description
A
FileUploadcomponent with adataModelBindings.listbinding inside aRepeatingGroupfailed to render whenever its list field was pre-populated with 2 or more attachment IDs. Instead of showing the uploaded files, the component displayed an error.The root cause was in the form data write path: when the reconciled list of attachment IDs was written back into the data model (via
setLeafValue), the underlyingdot.strcall (with override disabled) threwTrying to redefine non-empty obj['...']because the target path already contained a non-empty array. This crashed node generation for that component.The fix deletes the existing value at the target path before writing, turning the write into a clean replace — mirroring what the invalid-data branch directly above already does. This is a general fix that protects any caller writing an array/object value through
setLeafValue, not onlyFileUpload.Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit