fix(rest-dsl): keyboard Enter now correctly sets HTTP method in Add Operation dialog#3516
Conversation
📝 WalkthroughWalkthrough
ChangesREST method modal
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ui/src/pages/RestDslEditor/components/AddMethodModal.tsx (1)
70-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider enabling "Enter to submit" functionality.
Currently, pressing
Enterinside the text inputs will not submit the modal because there is no<form>wrapping them. To improve keyboard accessibility, consider wrapping the modal body content in a<form>and associating it with the Add button.⌨️ Proposed refactor for standard form submission
- <Stack gap={5}> + <form id="add-method-form" onSubmit={(e) => { e.preventDefault(); handleAdd(); }}> + <Stack gap={5}> <Select id="add-method-http-method" ... data-testid="add-method-id" /> - </Stack> + </Stack> + </form>Note: You would also need to update the Add button in the
<ModalFooter>to act as the submit trigger for this form:<Button kind="primary" type="submit" form="add-method-form" data-testid="add-method-modal-add-btn"> Add </Button>🤖 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 `@packages/ui/src/pages/RestDslEditor/components/AddMethodModal.tsx` around lines 70 - 108, Wrap the AddMethodModal input content in a form with a stable identifier such as add-method-form, and connect submission to the existing Add action handler. Update the ModalFooter Add button to use type="submit" and reference that form so pressing Enter in the HTTP method, path, or ID fields triggers the same submission behavior.
🤖 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 `@packages/ui/src/pages/RestDslEditor/components/AddMethodModal.tsx`:
- Around line 44-47: Reset the AddMethodModal form state whenever it transitions
to open, including method, path, id, and pathError, so reopening starts with
default values. Add the necessary useEffect import and anchor the reset logic to
the component’s open prop while preserving the existing initial defaults.
---
Nitpick comments:
In `@packages/ui/src/pages/RestDslEditor/components/AddMethodModal.tsx`:
- Around line 70-108: Wrap the AddMethodModal input content in a form with a
stable identifier such as add-method-form, and connect submission to the
existing Add action handler. Update the ModalFooter Add button to use
type="submit" and reference that form so pressing Enter in the HTTP method,
path, or ID fields triggers the same submission behavior.
🪄 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: 6f528a27-f6ea-414a-b903-51f76014efe9
📒 Files selected for processing (2)
packages/ui/src/pages/RestDslEditor/components/AddMethodModal.test.tsxpackages/ui/src/pages/RestDslEditor/components/AddMethodModal.tsx
…peration dialog (KaotoIO#3474) Problem The HTTP Method field in the Add Operation dialog was rendered as a typeahead select (from @kaoto/forms EnumField). When the user typed a valid method (e.g. post) and pressed Enter, the dropdown visually closed and showed 'post' but the underlying onChangeProp callback was never fired. As a result, formModel.method remained 'get'. Solution We replaced the typeahead EnumField (via KaotoForm) for the HTTP Method field with a plain Carbon React Select dropdown. Since there are only 6 valid HTTP methods, a simple <Select> is simpler and has correct keyboard behavior by default (no typeahead edge cases). The path and id fields were replaced with TextInput components to match, removing the KaotoForm / KaotoFormApi dependency from this modal entirely. Modal state is reset on open. Fixes KaotoIO#3474
3d92095 to
e31cd77
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/ui/src/pages/RestDslEditor/components/AddMethodModal.test.tsx (3)
163-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify undefined property assertion.
Using
not.toHaveProperty('id', expect.anything())is technically correct but a bit complex to parse mentally. Asserting that the property is directlyundefinedexpresses the intent more clearly.♻️ Proposed refactor
// id should be undefined, not included in the call - expect(mockOnAddMethod.mock.calls[0][0]).not.toHaveProperty('id', expect.anything()); + expect(mockOnAddMethod.mock.calls[0][0].id).toBeUndefined();🤖 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 `@packages/ui/src/pages/RestDslEditor/components/AddMethodModal.test.tsx` around lines 163 - 164, In the AddMethodModal test, simplify the assertion on the first argument passed to mockOnAddMethod so it directly verifies the id property is undefined, replacing the negated toHaveProperty/expect.anything() expression while preserving the existing intent.
193-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
actwrapper.
fireEventoperations are automatically wrapped inactby@testing-library/react. The explicitawait act(async () => { ... })wrapper is unnecessary, especially sincefireEvent.changeis fully synchronous.
(Note: You can also remove theactimport from@testing-library/reactat the top of the file as it will become unused if you apply this.)♻️ Proposed refactor
- // Start typing in path — error should clear - await act(async () => { - fireEvent.change(getPathInput(), { target: { value: '/' } }); - }); - expect(screen.queryByText('Path is required')).not.toBeInTheDocument(); + // Start typing in path — error should clear + fireEvent.change(getPathInput(), { target: { value: '/' } }); + expect(screen.queryByText('Path is required')).not.toBeInTheDocument();🤖 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 `@packages/ui/src/pages/RestDslEditor/components/AddMethodModal.test.tsx` around lines 193 - 197, Remove the explicit act wrapper around fireEvent.change in the AddMethodModal test, calling the synchronous event directly. Then remove the act import from the test file if it is no longer used, while preserving the existing assertion that the path error clears.
100-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
it.eachfor parameterized test cases.Instead of iterating through methods inside a single test case, using
it.eachruns each HTTP method as a separate, isolated test case. This improves test reporting (each method will have its own pass/fail status) and properly hooks into thebeforeEachlifecycle to automatically clear mocks, eliminating the need for manual mock clearing and explicitunmount()calls.♻️ Proposed refactor
- it('should update formModel correctly for all supported HTTP methods', async () => { - const methods = ['get', 'post', 'put', 'delete', 'patch', 'head'] as const; - - for (const m of methods) { - mockOnAddMethod.mockClear(); - mockOnClose.mockClear(); - - const { unmount } = render(<AddMethodModal open onClose={mockOnClose} onAddMethod={mockOnAddMethod} />); - const select = screen.getByLabelText('HTTP Method') as HTMLSelectElement; - const pathInput = screen.getByLabelText('Path') as HTMLInputElement; - const addBtn = screen.getByRole('button', { name: 'Add' }); - - fireEvent.change(select, { target: { value: m } }); - fireEvent.change(pathInput, { target: { value: '/test' } }); - fireEvent.click(addBtn); - - await waitFor(() => { - expect(mockOnAddMethod).toHaveBeenCalledWith(expect.objectContaining({ method: m })); - }); - - unmount(); - } - }); + it.each(['get', 'post', 'put', 'delete', 'patch', 'head'] as const)( + 'should update formModel correctly for HTTP method %s', + async (m) => { + const { getMethodSelect, getPathInput, getAddButton } = setupModal(); + + fireEvent.change(getMethodSelect(), { target: { value: m } }); + fireEvent.change(getPathInput(), { target: { value: '/test' } }); + fireEvent.click(getAddButton()); + + await waitFor(() => { + expect(mockOnAddMethod).toHaveBeenCalledWith(expect.objectContaining({ method: m })); + }); + }, + );🤖 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 `@packages/ui/src/pages/RestDslEditor/components/AddMethodModal.test.tsx` around lines 100 - 122, Refactor the “should update formModel correctly for all supported HTTP methods” test to use it.each with the methods list, making each HTTP method an isolated test case. Remove the per-iteration mockClear calls and explicit unmount, relying on the existing beforeEach/test lifecycle while preserving the current render, form input, submission, and method assertion behavior.
🤖 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.
Nitpick comments:
In `@packages/ui/src/pages/RestDslEditor/components/AddMethodModal.test.tsx`:
- Around line 163-164: In the AddMethodModal test, simplify the assertion on the
first argument passed to mockOnAddMethod so it directly verifies the id property
is undefined, replacing the negated toHaveProperty/expect.anything() expression
while preserving the existing intent.
- Around line 193-197: Remove the explicit act wrapper around fireEvent.change
in the AddMethodModal test, calling the synchronous event directly. Then remove
the act import from the test file if it is no longer used, while preserving the
existing assertion that the path error clears.
- Around line 100-122: Refactor the “should update formModel correctly for all
supported HTTP methods” test to use it.each with the methods list, making each
HTTP method an isolated test case. Remove the per-iteration mockClear calls and
explicit unmount, relying on the existing beforeEach/test lifecycle while
preserving the current render, form input, submission, and method assertion
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6aec56df-08f9-4c89-b593-3318e6068216
📒 Files selected for processing (2)
packages/ui/src/pages/RestDslEditor/components/AddMethodModal.test.tsxpackages/ui/src/pages/RestDslEditor/components/AddMethodModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/pages/RestDslEditor/components/AddMethodModal.tsx
|
|
Hello! Thanks for checking this issue. Do you think you could possibly tackled it with the current forms library without the need to implement the carbon form directly here? We are already planning migration but it will go through the forms library in a later stage. |
|
Hi @PVinaches, Good catch, and yes, we can stay within @kaoto/forms. The root cause isn't in this modal at all: it's in Typeahead.js's onInputKeyDown, which only fires onChange when the typed value is a custom (not-in-list) match: So typing "post", an exact enum match , and hitting Enter skips onChange entirely, and formModel.method stays 'get'. EnumField always renders via Typeahead, and AddMethodModal just inherits that bug through the schema's enum. Rather than dropping to Carbon's , I'd suggest using the same customFieldsFactory extension point we already use for MediaTypeField and UriField: Add a SelectEnumField that uses useFieldValue from @kaoto/forms for model wiring, but renders a PatternFly (native select) instead of Typeahead, sidesteps the broken keydown handler entirely since there's no typeahead/combobox logic involved. Register it in a local addMethodFieldFactory, scoped to small closed enums (e.g. schema.enum.length <= 8), so it only applies here. Pass that factory to in AddMethodModal — everything else (KaotoFormApi, validation, onChangeProp) stays exactly as-is. That keeps this PR consistent with the forms-library direction and doesn't introduce a second form system. I'll file the Typeahead.js bug as a separate issue against @kaoto/forms since it likely affects any EnumField consumer, not just this modal , worth fixing upstream once the migration reaches that package. |
|
Hello again!
|
2 participants
Fixes #3474
Problem
The HTTP Method field in the Add Operation dialog was rendered as a typeahead select (from
@kaoto/formsEnumField). When the user typed a valid method (e.g.post) and pressed Enter, the dropdown visually closed and showed "post" but the underlyingonChangePropcallback was never fired. As a result,formModel.methodremained'get'.Solution
We replaced the typeahead
EnumField(viaKaotoForm) for the HTTP Method field with a plain Carbon ReactSelectdropdown. Since there are only 6 valid HTTP methods, a simple<Select>is simpler and has correct keyboard behavior by default (no typeahead edge cases).The
pathandidfields were replaced withTextInputcomponents to match, removing theKaotoForm/KaotoFormApidependency from this modal entirely.Testing
#3474: selecting a method by keyboard now produces the correct method in theonAddMethodcallback.@jichenggepeter-dev I am very sorry! I know you were assigned to this issue recently. I actually already started working on a fix for this bug locally and had the solution ready. I apologize for the overlap and stepping on your toes here.
Summary by CodeRabbit