Repository navigation
Add Subform container for Enter handling, drop Combobox onEnter - #3422
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes what Enter does in every Combobox across the app (Enter in a closed combobox now reaches and submits the enclosing form), a human should still confirm that is the intended UX for every form, beyond the pickers the e2e tests cover.
What was reviewed:
Subform's keydown guard (defaultPrevented, IMEisComposing, and the target being anHTMLInputElement), and its reliance on Headless UI calling preventDefault/stopPropagation when the options list is open.- The Combobox refactor from a render prop to a plain child. No
onEnterorisOpenRefreferences remain anywhere inapp/. - The
<Subform>wrapping in firewall-rules-common, network-interface-edit and webhook-create, plus the new browser spec and the e2e Enter cases.
Extended reasoning...
The change adds a Subform wrapper component, removes the per-field Enter handlers and the Combobox's onEnter and open-state tracking, and updates the firewall rule, webhook secret and transit IP subforms along with the e2e tests. It touches no security-sensitive surface: no auth, data exposure or injection. The code is coherent and has test coverage, but it changes Enter behavior in the shared Combobox for every form in the app, which is a UX decision rather than a mechanical change. That is the reason for deferring instead of approving.
| name="value" | ||
| {...getFilterValueProps(valueType)} | ||
| description="Select an option or enter a custom value" | ||
| <Subform onSubmit={submitSubform} className="space-y-6"> |
There was a problem hiding this comment.
what do you think of either explicitly using ox-form here or adding another selector for, idk ox-subform or ox-form-spacing?
diff --git a/app/ui/styles/components/form.css b/app/ui/styles/components/form.css
index 82d35e84..516d4140 100644
--- a/app/ui/styles/components/form.css
+++ b/app/ui/styles/components/form.css
@@ -18,7 +18,8 @@
.ox-form,
.ox-form .ox-tabs-panel,
-.ox-form .ox-accordion-content {
+.ox-form .ox-accordion-content,
+.ox-form .ox-subform {
@apply space-y-6;
}if we ever adjust the ox-form spacing i assume we want that change propagated here
There was a problem hiding this comment.
Yeah, good idea. Turns out the different Subforms have slightly different spacing situations but I think I can make them all the same.
There was a problem hiding this comment.
Done in 20c52c0 — diff is better with whitespace hidden. Had to get slightly more invasive by bringing the buttons inside the form, but it's a better end result. The layout changes are barely visible: buttons slightly closer to the last field in the first one, and like 4px more (lol) between fields in the second.
| onClear={() => subform.reset()} | ||
| onSubmit={submitSubform} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
This feels pretty good overall. It makes subforms a lot harder to mess up.
The diff is unreadable unless you hide whitespace changes because it adds a wrapper to existing code.
This is an alternative to #3417 that deletes the sad logic inside Combobox instead of fixing it. We get rid of
onEnter, and instead of having to keep track of whether the list is open withisOpenRefand decide when topreventDefault, Combobox never callspreventDefault(unless it's done for it by Headless UI when selecting an option).But in subforms we still need to capture those enter keypresses before they hit the parent form, so we wrap subforms in a
Subformcomponent with anonSubmitprop. The advantage is we no longer have to remember to do this for every field we add to a subform (and we have messed this up before).This approach fixes the same problem cases as #3417, all of which are noops on
main. Enter in a closed combobox now:Subform.browser.spec.tsxtests Enter in a combobox, text input, and button inside a subform, and in a combobox outside a subform. In the e2e tests, Enter adds a firewall rule target, port and ICMP type, a webhook secret, and a transit IP. Enter in a closed combobox applies the alert class filter, attaches a disk, and creates an instance from the image picker (these tests fail on main).