Sync recent changes from canary into integrations/makeswift#2657
Sync recent changes from canary into integrations/makeswift#2657jorgemoya wants to merge 1 commit into
canary into integrations/makeswift#2657Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| import { Input } from '@/vibes/soul/form/input'; | ||
| import { NumberInput } from '@/vibes/soul/form/number-input'; | ||
| import { RadioGroup } from '@/vibes/soul/form/radio-group'; | ||
| import { SelectField } from '@/vibes/soul/form/select-field'; | ||
| import { Select } from '@/vibes/soul/form/select'; | ||
| import { SwatchRadioGroup } from '@/vibes/soul/form/swatch-radio-group'; | ||
| import { Textarea } from '@/vibes/soul/form/textarea'; | ||
| import { Button, ButtonProps } from '@/vibes/soul/primitives/button'; |
There was a problem hiding this comment.
Bug: DynamicForm now uses Select directly, omitting a hidden input workaround for Radix UI, which will prevent select field values from being submitted.
Severity: CRITICAL | Confidence: 0.98
🔍 Detailed Analysis
The DynamicForm component was modified to use Select directly instead of SelectField. The SelectField component includes a hidden input that serves as a workaround for a known Radix UI issue (radix-ui/primitives#3198) where the Select primitive does not correctly handle form submission. Without this hidden input, the selected value from a Select field will not be included in the FormData when the form is submitted via a server action. This will lead to validation failures and data loss for select field values, specifically impacting forms like the gift-certificate-purchase-section that utilize DynamicForm with select inputs.
💡 Suggested Fix
Revert the change in dynamic-form/index.tsx to use SelectField instead of Select to reintroduce the necessary hidden input workaround. Alternatively, implement an equivalent hidden input mechanism for Select to ensure correct form value capture.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: core/vibes/soul/form/dynamic-form/index.tsx#L31-L37
Potential issue: The `DynamicForm` component was modified to use `Select` directly
instead of `SelectField`. The `SelectField` component includes a hidden input that
serves as a workaround for a known Radix UI issue
(https://github.com/radix-ui/primitives/issues/3198) where the `Select` primitive does
not correctly handle form submission. Without this hidden input, the selected value from
a `Select` field will not be included in the `FormData` when the form is submitted via a
server action. This will lead to validation failures and data loss for select field
values, specifically impacting forms like the `gift-certificate-purchase-section` that
utilize `DynamicForm` with select inputs.
Did we get this right? 👍 / 👎 to inform future reviews.
matthewvolk
left a comment
matthewvolk
left a comment
There was a problem hiding this comment.
Make sure to delete non-@bigcommerce/catalyst-makeswift changeset files
There was a problem hiding this comment.
One thing to note when merging canary into integrations/makeswift: Changesets for @bigcommerce/catalyst-core should be deleted before merging into integrations/makeswift. Actually, all changesets that aren't targeting @bigcommerce/catalyst-makeswift should just be deleted (I usually just delete them in the merge commit). This is because Changesets isn't able to find that package on integrations/makeswift branch and will throw an error when this PR is merged.
10b0bd0 to
b214224
Compare
2 participants
What/Why?
Sync changes for 1.3 release.
Testing
Manual review.
Migration
N/A