fix(ui): Select shows items in the list, small gap fixes for icons, d… #697
Conversation
WalkthroughThe pull request introduces modifications to three TypeScript React components: Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-flow ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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 (3)
ui/src/components/SchemaForm/Templates/ArrayFieldItemTemplate.tsx (1)
35-39: Consider maintaining consistent vertical paddingWhile the added gap utilities improve spacing between elements, changing
py-4topt-4removes bottom padding which might affect layout consistency. Consider if bottom padding is still needed for visual balance.- <div className="pt-4"> + <div className="py-4">ui/src/components/SchemaForm/Widgets/SelectWidget.tsx (1)
59-70: Enhance accessibility for the dropdown triggerThe dropdown trigger button lacks proper ARIA attributes for better accessibility.
<DropdownMenuTrigger className={`flex h-8 w-full items-center justify-between rounded border bg-background px-3 hover:bg-accent ${ rawErrors.length > 0 ? "border-destructive" : "" }`} disabled={disabled || readonly} onBlur={handleBlur} - onFocus={handleFocus}> + onFocus={handleFocus} + aria-label={`Select ${placeholder}`} + aria-invalid={rawErrors.length > 0}>ui/src/features/Editor/components/RightPanel/components/ParamEditor/index.tsx (1)
Line range hint
20-21: Address TODO comment for nodeParameters typeThe TODO comment indicates that the type for nodeParameters needs to be defined.
Would you like me to help define a proper type for the nodeParameters based on its usage in the codebase?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/src/components/SchemaForm/Templates/ArrayFieldItemTemplate.tsx(1 hunks)ui/src/components/SchemaForm/Widgets/SelectWidget.tsx(2 hunks)ui/src/features/Editor/components/RightPanel/components/ParamEditor/index.tsx(1 hunks)
🔇 Additional comments (2)
ui/src/components/SchemaForm/Widgets/SelectWidget.tsx (1)
72-83: 🛠️ Refactor suggestion
Improve type safety in enumOptions mapping
The mapping of enumOptions uses any type and could be more type-safe.
- {enumOptions?.map(({ value: optionValue, label }: any, i: number) => {
+ {enumOptions?.map(({ value: optionValue, label }, i) => {Likely invalid or redundant comment.
ui/src/features/Editor/components/RightPanel/components/ParamEditor/index.tsx (1)
Line range hint 62-67: LGTM: Improved tab spacing
The addition of gap-2 class improves the visual spacing between tab triggers, aligning with the PR's spacing improvement objectives.
| const { enumOptions, enumDisabled } = options; | ||
| const [selectedLabel, setSelectedLabel] = useState(placeholder); | ||
|
|
||
| // const emptyValue = multiple ? [] : ""; | ||
| const getCurrentLabel = () => { | ||
| const option = enumOptions?.find((opt: any) => opt.value === value); | ||
| return option ? option.label : placeholder; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety to enumOptions handling
The enumOptions handling lacks proper TypeScript types, which could lead to runtime errors.
- const { enumOptions, enumDisabled } = options;
+ interface EnumOption {
+ value: string;
+ label: string;
+ }
+ const { enumOptions, enumDisabled } = options as {
+ enumOptions?: EnumOption[];
+ enumDisabled?: string[];
+ };
- const getCurrentLabel = () => {
- const option = enumOptions?.find((opt: any) => opt.value === value);
+ const getCurrentLabel = () => {
+ const option = enumOptions?.find((opt: EnumOption) => opt.value === value);
return option ? option.label : placeholder;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { enumOptions, enumDisabled } = options; | |
| const [selectedLabel, setSelectedLabel] = useState(placeholder); | |
| // const emptyValue = multiple ? [] : ""; | |
| const getCurrentLabel = () => { | |
| const option = enumOptions?.find((opt: any) => opt.value === value); | |
| return option ? option.label : placeholder; | |
| }; | |
| interface EnumOption { | |
| value: string; | |
| label: string; | |
| } | |
| const { enumOptions, enumDisabled } = options as { | |
| enumOptions?: EnumOption[]; | |
| enumDisabled?: string[]; | |
| }; | |
| const [selectedLabel, setSelectedLabel] = useState(placeholder); | |
| const getCurrentLabel = () => { | |
| const option = enumOptions?.find((opt: EnumOption) => opt.value === value); | |
| return option ? option.label : placeholder; | |
| }; |
| const handleBlur = () => { | ||
| if (onBlur) { | ||
| onBlur(id, value); | ||
| } | ||
| }; | ||
|
|
||
| const handleFocus = () => { | ||
| if (onFocus) { | ||
| onFocus(id, value); | ||
| } | ||
| }; |
There was a problem hiding this comment.
| const handleBlur = () => { | |
| if (onBlur) { | |
| onBlur(id, value); | |
| } | |
| }; | |
| const handleFocus = () => { | |
| if (onFocus) { | |
| onFocus(id, value); | |
| } | |
| }; | |
| const handleBlur = () => onBlur?.(id, value); | |
| const handleFocus = () => onFocus?.(id, value); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ui/src/components/SchemaForm/Widgets/SelectWidget.tsx (3)
24-31: Consider adding stricter TypeScript types for the propsWhile the component uses generics, some props could benefit from more specific types:
valuecould be constrained to match enumOption valuesrawErrorscould have a specific error type>({ id, options, disabled, readonly, - value, + value: string | number | null, onChange, onBlur, onFocus, placeholder, - rawErrors = [], + rawErrors: string[] = [], }: WidgetProps<T, S, F>)
41-47: Optimize event handlers with useCallbackConsider memoizing the event handlers to prevent unnecessary re-renders of child components:
- const handleSelect = (value: any, label: string) => { + const handleSelect = useCallback((value: any, label: string) => { setSelectedLabel(label); onChange(value); - }; + }, [onChange]); - const handleBlur = () => onBlur?.(id, value); - const handleFocus = () => onFocus?.(id, value); + const handleBlur = useCallback(() => onBlur?.(id, value), [onBlur, id, value]); + const handleFocus = useCallback(() => onFocus?.(id, value), [onFocus, id, value]);
Line range hint
50-76: Enhance accessibility featuresWhile using Radix UI provides good accessibility foundations, consider these improvements:
- Add aria-label for the dropdown trigger
- Add aria-invalid when there are errors
- Consider adding aria-describedby for error messages
<DropdownMenuTrigger className={`flex h-8 w-full items-center justify-between rounded border bg-background px-3 hover:bg-accent ${ rawErrors.length > 0 ? "border-destructive" : "" }`} disabled={disabled || readonly} + aria-label={`Select ${placeholder}`} + aria-invalid={rawErrors.length > 0} onBlur={handleBlur} onFocus={handleFocus}>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/components/SchemaForm/Widgets/SelectWidget.tsx(2 hunks)
🔇 Additional comments (3)
ui/src/components/SchemaForm/Widgets/SelectWidget.tsx (3)
33-34: Add type safety to enumOptions handling
The enumOptions handling still lacks proper TypeScript types, which could lead to runtime errors.
36-39:
Potential state synchronization issue with selectedLabel
The selectedLabel state might get out of sync with the actual value when the value prop changes externally. Consider using a useEffect to update the selectedLabel when the value changes:
+ useEffect(() => {
+ const label = getCurrentLabel();
+ setSelectedLabel(label);
+ }, [value, enumOptions]);Likely invalid or redundant comment.
33-39: Verify handling of null ofAny values
The PR mentions an issue with the ofAny function in react-jsonschema-form. Let's verify the handling of null values:
✅ Verification successful
Proper null handling is implemented in SelectWidget
The implementation correctly handles null/undefined values through:
- Optional chaining on
enumOptions?.find()andenumDisabled?.includes() - Fallback to placeholder when no option is found
- Safe rendering with null checks in the template
The changes properly address the display issues by:
- Maintaining selected label state
- Providing fallback to placeholder for null values
- Safe iteration over options with proper null checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ofAny usage and null handling
rg -A 5 "ofAny"
rg -A 5 "const { enumOptions"
Length of output: 157
Script:
#!/bin/bash
# Search for ofAny usage with proper escaping and context
rg -A 5 "ofAny\("
# Search for enumOptions with proper escaping
rg -A 5 "enumOptions.*="
# Search for SelectWidget implementations
ast-grep --pattern 'const SelectWidget = $_'
Length of output: 8832
…isplay fixes
Overview
Select component was not displaying options when selected and other stylistic issues.
What I've done
What I haven't done
How I tested
Manually
Screenshot
Which point I want you to review particularly
Memo
ofAny for react-jsonschema-form issue with null means that select will not work properly if OfAny has null.
Summary by CodeRabbit
New Features
Bug Fixes