fix(ui): stop SwitchWithFormSupport from double-submitting under one name (PP-msjp) - #2125
timothyfroehlich wants to merge 2 commits into
Conversation
…name (PP-msjp)
A named SwitchWithFormSupport passed `name` to both the Radix Root and its
own custom hidden input, so an enabled, checked switch submitted two FormData
entries under one name: Radix's form bubble ("on") and the hidden input
("on"). formData.getAll(name) therefore returned ["on", "on"]. It was latent
because the only consumer (updateNotificationPreferencesAction) reads
.get() (the first value), but a switch to .getAll() — or any ordering change
that let the hidden input's explicit "off" be shadowed — would have been
surprised.
Fix (bead option a): make the Radix Root nameless so the custom hidden input
is the sole FormData carrier. It already emits the explicit "off" that Radix's
bubble cannot, and it already renders nothing while disabled, so every
submission case is preserved and now yields exactly one entry.
Extend the existing switch regression tests with getAll()-based assertions
(the prior tests all used .get(), so none caught the duplicate).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SqqP7PcYtf1yq1Upa3k82m
… renders (PP-msjp) Review nit: the Radix bubble input renders whenever the switch is inside a form regardless of `name`; `name` controls whether it is *submitted*. Say "submits" so a future debugger isn't surprised to find the nameless bubble still in the DOM. Comment-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SqqP7PcYtf1yq1Upa3k82m
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe form-supported Switch no longer passes ChangesSwitch form submission
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The Switch submits one correct value while preserving unchecked and disabled behavior, with no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
What
SwitchWithFormSupportpassednameto both the RadixRootand its own custom hidden<input>. Inside a form, Radix's Root submits its own bubble input (value"on"), so an enabled, checked switch submitted two FormData entries under one name —formData.getAll(name)returned["on", "on"].It was latent because the only consumer (
updateNotificationPreferencesAction) readsformData.get(name)— the first value. The trap: switching that read to.getAll(), or any ordering change that let the hidden input's explicit"off"be shadowed, would have been surprised. (Filed as PP-msjp from a PR #2059 review; deliberately not fixed there to keep that change scoped.)Fix (bead option a)
Make the Radix
Rootnameless so the custom hidden input is the sole FormData carrier.nameis destructured out of props, so it never reaches the Root. The hidden input already emits the explicit"off"that Radix's bubble cannot, and already renders nothing while disabled (PP-bhd7.7), so every submission case is preserved and now yields exactly one entry:getAll(name)before["on", "on"]["on"]["off"]["off"][][][][]No consumer changes: only
notification-preferences-form.tsxpasses aname, and it reads.get(). The other four importers drive the switch purely viachecked/onCheckedChangewith noname, so they never touched FormData.Tests
Extended the existing
switch.test.tsxregression suite with agetAll()-based assertion for the checked case — the prior tests all used.get(), so none could catch the duplicate. Verified non-vacuous: the new assertion reads["on", "on"]against the pre-fix code. Also added a caution comment on the base (unexported)Switch, which has the same double-submit shape if ever given anamein a form.Scope
Two files. Follow-up PP-r5xz filed for an adjacent, pre-existing latent observation (the hidden input derives from React state, so it ignores a native
<form>reset) — out of scope for this P3.Review
Reviewed adversarially by subagents across correctness, bead-fidelity, and maintainability lenses; their findings (a vacuous unchecked test, a comment-hardening suggestion, and a "renders" → "submits" precision nit) are folded in.
Opened by the unattended nightly bead session, so it carries
ownerless— nobody is driving its CI or review threads until an attended session adopts it.🤖 Generated with Claude Code
https://claude.ai/code/session_01SqqP7PcYtf1yq1Upa3k82m
Generated by Claude Code
Summary by CodeRabbit
"on"when enabled or"off"when disabled.