feat(Suggestions): Add suggestion button and centralized menu state#100
Open
mmelko wants to merge 2 commits intoKaotoIO:mainfrom
Open
feat(Suggestions): Add suggestion button and centralized menu state#100mmelko wants to merge 2 commits intoKaotoIO:mainfrom
mmelko wants to merge 2 commits intoKaotoIO:mainfrom
Conversation
- Add currentOpenMenu/setCurrentOpenMenu to SuggestionContext for
centralized menu visibility control (single menu open at a time)
- Replace local isVisible state with context-driven visibility using
unique menuId per hook instance (useId)
- Change useSuggestions return type from ReactNode to
{ suggestionsMenu, openSuggestions } object
- Update all field components and tests to use new return type
📝 WalkthroughWalkthroughThe suggestions system was refactored to use shared context state ( Changes
Sequence DiagramsequenceDiagram
participant User
participant FieldWrapper
participant FieldComponent as PasswordField/StringField/TextAreaField/KeyValueField
participant Hook as useSuggestions
participant Context as SuggestionContext
User->>FieldWrapper: Click idea-button (or press Ctrl+Space)
FieldWrapper->>FieldComponent: onOpenSuggestions()
FieldComponent->>Hook: openSuggestions()
Hook->>Context: setCurrentOpenMenu(menuId)
Context-->>Hook: currentOpenMenu updated
Hook-->>FieldComponent: suggestionsMenu (now non-null)
FieldComponent-->>FieldWrapper: render suggestionsMenu
FieldWrapper-->>User: suggestions menu visible
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/form/KeyValue/KeyValueField.tsx (1)
25-31:⚠️ Potential issue | 🟠 MajorExpose a discoverable suggestions trigger for this field.
At Line 25,
openSuggestionsis not consumed, and at Line 52 only the menu node is rendered. This misses the new explicit-open UX added to other fields, so key-value inputs don’t get the same visible trigger.💡 Proposed fix
-import { TextInput } from '@carbon/react'; +import { IconButton, TextInput } from '@carbon/react'; +import { Idea } from '@carbon/icons-react'; ... - const { suggestionsMenu } = useSuggestions({ + const { suggestionsMenu, openSuggestions } = useSuggestions({ propName: name, schema: STRING_SCHEMA, inputRef, value: value ?? '', setValue: onChange, }); ... - <div> + <div> + <div style={{ display: 'flex', alignItems: 'center', gap: '0.25rem' }}> <TextInput labelText="" type="text" id={id} name={name} data-testid={dataTestId} onChange={handleChange} ref={inputRef} onFocus={onFocus} onBlur={onBlur} placeholder={placeholder} value={value} /> + <IconButton + kind="ghost" + size="sm" + label="Open suggestions (Ctrl+Space)" + onClick={openSuggestions} + data-testid={`${name}__open-suggestions-button`} + > + <Idea /> + </IconButton> + </div> {suggestionsMenu} </div>Also applies to: 52-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/form/KeyValue/KeyValueField.tsx` around lines 25 - 31, The suggestions hook return value includes openSuggestions which is not used in KeyValueField; update the KeyValueField component to consume openSuggestions from useSuggestions (alongside suggestionsMenu) and render a visible trigger (e.g., a small button or icon near the input) that calls openSuggestions to explicitly open the suggestions menu; locate the call to useSuggestions (propName: name, schema: STRING_SCHEMA, inputRef, value, setValue) and the render site where suggestionsMenu is currently returned and add the trigger adjacent to that menu so key-value inputs match the explicit-open UX of other fields.src/form/hooks/suggestions.tsx (1)
67-79:⚠️ Potential issue | 🟠 MajorBlock the keyboard shortcut on non-editable inputs too.
openSuggestions()guardsdisabled/readOnly, but thisCtrl+Space/Alt+Escapepath does not. A focused read-only input can still open the menu, which defeats the new non-editable-field guard and may still allow a value change ifsetValueis wired.Suggested fix
const handleInputKeyDown = useCallback( (event: Event) => { if (!(event instanceof KeyboardEvent)) return; + const input = inputRef.current; if ((event.ctrlKey && event.code === 'Space') || (event.altKey && event.code === 'Escape')) { + if (input?.disabled || input?.readOnly) return; event.preventDefault(); setSearchValue(''); setCurrentOpenMenu(menuId); } else if (event.key === 'Escape') { onEscapeKey(event); } }, - [onEscapeKey, setCurrentOpenMenu, menuId], + [inputRef, onEscapeKey, setCurrentOpenMenu, menuId], );Also applies to: 181-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/form/hooks/suggestions.tsx` around lines 67 - 79, The keyboard shortcut handler handleInputKeyDown currently ignores input editability and can open suggestions on readOnly/disabled fields; update the handler to inspect the event.target (cast to HTMLElement/HTMLInputElement|HTMLTextAreaElement) and if the focused element is a form control with readOnly or disabled true, return early (no preventDefault/setSearchValue/setCurrentOpenMenu). Apply the same guard to the other shortcut handler mentioned (lines ~181-187) so both Ctrl+Space / Alt+Escape paths respect non-editable inputs and avoid opening suggestions or triggering setValue.
🧹 Nitpick comments (2)
src/form/hooks/suggestions.tsx (1)
181-187: Consider wiringopenSuggestionsintoKeyValueFieldas well.
src/form/KeyValue/KeyValueField.tsx:19-54still consumes onlysuggestionsMenuand renders a bareTextInput, so suggestion-enabled key/value inputs miss the new lightbulb/button trigger and now behave differently from the other field types.Also applies to: 308-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/form/hooks/suggestions.tsx` around lines 181 - 187, The KeyValueField component is not wired to the new suggestions trigger so key/value inputs miss the lightbulb/button; update KeyValueField to accept and pass the suggestionsMenu/openSuggestions handler into the inner TextInput (or the same prop used by other field types) so it renders the suggestion trigger and invokes openSuggestions when the button is clicked or activated; specifically, locate the KeyValueField component and ensure it forwards the suggestionsMenu prop (and/or a prop named openSuggestions) to the TextInput rendered inside KeyValueField so behavior matches the other field types that use suggestionsMenu and openSuggestions.src/form/hooks/suggestions.test.tsx (1)
7-20: Add a two-instance regression for the shared-menu behavior.This new wrapper models shared visibility state, but the suite still exercises
useSuggestionsone consumer at a time. Please add a case that opens menu A, then menu B, and asserts the first one closes; that is the main guaranteecurrentOpenMenuis meant to protect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/form/hooks/suggestions.test.tsx` around lines 7 - 20, Add a new test in suggestions.test.tsx that mounts two consumers under StatefulSuggestionProvider (using the existing getProviders jest.Mock and SuggestionContext), simulates opening menu "A" (via whatever event your test harness uses to invoke the useSuggestions consumer's open action), then opens menu "B", and assert that the consumer for "A" has closed (i.e., its visible state is falsy) while "B" is open; target the StatefulSuggestionProvider, SuggestionContext, and the consumer hook usage (useSuggestions or the component that wraps it) to simulate user interactions and assert the shared currentOpenMenu behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/form/fields/PasswordField.test.tsx`:
- Around line 9-15: The TypeScript props annotation for
StatefulSuggestionProvider is misformatted for Prettier; update the props typing
for the component (the object containing children: ReactNode and getProviders:
jest.Mock) so it conforms to prettier rules—either collapse the inline type to a
single line (e.g. ({ children, getProviders }: { children: ReactNode;
getProviders: jest.Mock }) => { … }) or extract a named Props interface/type and
apply it (e.g. type Props = { children: ReactNode; getProviders: jest.Mock };
const StatefulSuggestionProvider = ({ children, getProviders }: Props) => { … })
to fix the lint error while keeping the same symbols
(StatefulSuggestionProvider, getProviders).
In `@src/form/fields/StringField.test.tsx`:
- Around line 10-16: Prettier is unhappy with the inline object type formatting
for the StatefulSuggestionProvider props; update the props typing for
StatefulSuggestionProvider so it is formatted on one line or use a named
type/interface (for example replace the inline block "}: { children: ReactNode;
getProviders: jest.Mock; } =>" with a single-line type annotation or a declared
interface/type and reference it in the function signature) to satisfy Prettier
while keeping the same types for children and getProviders.
In `@src/form/fields/TextAreaField.test.tsx`:
- Around line 133-136: The test over-mocks useSuggestions by returning
suggestionsMenu directly which bypasses the Ctrl+Space event path; change the
test to use the real hook by wrapping the rendered TextAreaField in the stateful
SuggestionContext (the same pattern used in StringField.test.tsx and
PasswordField.test.tsx), remove the useSuggestions.mockImplementation(...) stub
for this scenario, and instead provide a SuggestionContext provider that uses
the real useSuggestions behavior so the Ctrl+Space key event triggers opening
the SuggestionsMenu (refer to useSuggestions and SuggestionsMenu to locate the
hook and component to wire into SuggestionContext).
In `@src/form/KeyValue/KeyValueField.test.tsx`:
- Around line 81-84: The test currently mocks useSuggestions to always return
<SuggestionsMenu />, which bypasses keyboard-trigger logic; change the test to
use the real hook and provider (same pattern as StringField.test.tsx) so
Ctrl+Space keydown actually drives suggestions via useSuggestions, or if you
must keep a mock, make useSuggestions mock return SuggestionsMenu only when
openSuggestions has been invoked (or expose a flag the test toggles) and assert
openSuggestions was called on Ctrl+Space; reference useSuggestions,
SuggestionsMenu, openSuggestions and follow the provider/wrapping approach used
in StringField.test.tsx to restore real behavior.
---
Outside diff comments:
In `@src/form/hooks/suggestions.tsx`:
- Around line 67-79: The keyboard shortcut handler handleInputKeyDown currently
ignores input editability and can open suggestions on readOnly/disabled fields;
update the handler to inspect the event.target (cast to
HTMLElement/HTMLInputElement|HTMLTextAreaElement) and if the focused element is
a form control with readOnly or disabled true, return early (no
preventDefault/setSearchValue/setCurrentOpenMenu). Apply the same guard to the
other shortcut handler mentioned (lines ~181-187) so both Ctrl+Space /
Alt+Escape paths respect non-editable inputs and avoid opening suggestions or
triggering setValue.
In `@src/form/KeyValue/KeyValueField.tsx`:
- Around line 25-31: The suggestions hook return value includes openSuggestions
which is not used in KeyValueField; update the KeyValueField component to
consume openSuggestions from useSuggestions (alongside suggestionsMenu) and
render a visible trigger (e.g., a small button or icon near the input) that
calls openSuggestions to explicitly open the suggestions menu; locate the call
to useSuggestions (propName: name, schema: STRING_SCHEMA, inputRef, value,
setValue) and the render site where suggestionsMenu is currently returned and
add the trigger adjacent to that menu so key-value inputs match the
explicit-open UX of other fields.
---
Nitpick comments:
In `@src/form/hooks/suggestions.test.tsx`:
- Around line 7-20: Add a new test in suggestions.test.tsx that mounts two
consumers under StatefulSuggestionProvider (using the existing getProviders
jest.Mock and SuggestionContext), simulates opening menu "A" (via whatever event
your test harness uses to invoke the useSuggestions consumer's open action),
then opens menu "B", and assert that the consumer for "A" has closed (i.e., its
visible state is falsy) while "B" is open; target the
StatefulSuggestionProvider, SuggestionContext, and the consumer hook usage
(useSuggestions or the component that wraps it) to simulate user interactions
and assert the shared currentOpenMenu behavior.
In `@src/form/hooks/suggestions.tsx`:
- Around line 181-187: The KeyValueField component is not wired to the new
suggestions trigger so key/value inputs miss the lightbulb/button; update
KeyValueField to accept and pass the suggestionsMenu/openSuggestions handler
into the inner TextInput (or the same prop used by other field types) so it
renders the suggestion trigger and invokes openSuggestions when the button is
clicked or activated; specifically, locate the KeyValueField component and
ensure it forwards the suggestionsMenu prop (and/or a prop named
openSuggestions) to the TextInput rendered inside KeyValueField so behavior
matches the other field types that use suggestionsMenu and openSuggestions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8157eb0-168d-43fe-8c15-8fd6c9108881
⛔ Files ignored due to path filters (4)
src/form/__snapshots__/KaotoForm.test.tsx.snapis excluded by!**/*.snapsrc/form/fields/__snapshots__/PasswordField.test.tsx.snapis excluded by!**/*.snapsrc/form/fields/__snapshots__/StringField.test.tsx.snapis excluded by!**/*.snapsrc/form/fields/__snapshots__/TextAreaField.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
src/form/KeyValue/KeyValueField.test.tsxsrc/form/KeyValue/KeyValueField.tsxsrc/form/fields/FieldWrapper.tsxsrc/form/fields/PasswordField.test.tsxsrc/form/fields/PasswordField.tsxsrc/form/fields/StringField.test.tsxsrc/form/fields/StringField.tsxsrc/form/fields/TextAreaField.test.tsxsrc/form/fields/TextAreaField.tsxsrc/form/hooks/suggestions.test.tsxsrc/form/hooks/suggestions.tsxsrc/form/providers/SuggestionRegistryProvider.tsx
- Add Idea (lightbulb) icon button in FieldWrapper label row for opening the suggestions menu - Add double-click handler on input fields to toggle suggestions - Guard openSuggestions against disabled/readOnly inputs - Use setTimeout to reliably focus search input after Carbon Menu's internal focus management completes
mmelko
force-pushed
the
feat/suggestions-button-main
branch
from
6b72aea to
1d2b66c
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/form/hooks/suggestions.tsx (1)
218-235:⚠️ Potential issue | 🟡 MinorRemove unused
groupedSuggestionsfrom dependency array.
groupedSuggestionsis included in the dependency array but is not used in the effect body. This causes the effect to re-run (and reset the focus timeout) every time suggestions are fetched or filtered, potentially leading to inconsistent focus timing.🔧 Proposed fix
- }, [isVisible, inputRef, groupedSuggestions]); + }, [isVisible, inputRef]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/form/hooks/suggestions.tsx` around lines 218 - 235, The useEffect that sets menuPosition and focuses searchInputRef unnecessarily lists groupedSuggestions in its dependency array, causing reruns when suggestions change; update the useEffect dependencies to only include isVisible and inputRef (remove groupedSuggestions), leaving the timeout/cleanup logic unchanged so focusTimeout is still cleared on unmount; locate the effect by the useEffect block that reads inputRef.current, calls setMenuPosition, and sets focus via searchInputRef.current?.focus().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/form/hooks/suggestions.tsx`:
- Around line 218-235: The useEffect that sets menuPosition and focuses
searchInputRef unnecessarily lists groupedSuggestions in its dependency array,
causing reruns when suggestions change; update the useEffect dependencies to
only include isVisible and inputRef (remove groupedSuggestions), leaving the
timeout/cleanup logic unchanged so focusTimeout is still cleared on unmount;
locate the effect by the useEffect block that reads inputRef.current, calls
setMenuPosition, and sets focus via searchInputRef.current?.focus().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2597898-fd7b-4a36-ad98-a5a408160d84
⛔ Files ignored due to path filters (4)
src/form/__snapshots__/KaotoForm.test.tsx.snapis excluded by!**/*.snapsrc/form/fields/__snapshots__/PasswordField.test.tsx.snapis excluded by!**/*.snapsrc/form/fields/__snapshots__/StringField.test.tsx.snapis excluded by!**/*.snapsrc/form/fields/__snapshots__/TextAreaField.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
src/form/KeyValue/KeyValueField.test.tsxsrc/form/fields/FieldWrapper.tsxsrc/form/fields/PasswordField.test.tsxsrc/form/fields/PasswordField.tsxsrc/form/fields/StringField.test.tsxsrc/form/fields/StringField.tsxsrc/form/fields/TextAreaField.test.tsxsrc/form/fields/TextAreaField.tsxsrc/form/hooks/suggestions.tsx
✅ Files skipped from review due to trivial changes (1)
- src/form/fields/TextAreaField.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/form/fields/StringField.test.tsx
- src/form/fields/PasswordField.tsx
- src/form/fields/StringField.tsx
- src/form/fields/TextAreaField.test.tsx
- src/form/KeyValue/KeyValueField.test.tsx
- src/form/fields/PasswordField.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SuggestionContextso only one menu can be open at a time across all fieldsuseSuggestionsreturn type to{ suggestionsMenu, openSuggestions }for better composabilitysetTimeoutto reliably focus the search input after Carbon Menu's internal focus managementScreen.Recording.2026-03-19.at.16.24.32.mov
Summary by CodeRabbit
New Features
Behavior
Tests