refactor(web): Add Slider and use in settings #1266
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (1)server/pkg/builtin/manifest_ja.yml (1)
The Japanese translation for the camera FOV field description is well-written and provides accurate technical details about the field's purpose, units, and supported range. Let's verify the consistency with the English version: 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-web canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (13)
web/src/beta/ui/fields/SliderField.tsx (1)
7-17: Consider adding prop validation and performance optimization.The component implementation is clean, but could benefit from the following improvements:
- Add prop validation for critical slider values
- Filter props before spreading to avoid passing unwanted props
- Memoize the component for performance optimization
Consider applying these improvements:
-const SliderField: FC<SliderFieldProps> = ({ +const SliderField: FC<SliderFieldProps> = React.memo(({ title, description, ...props }) => { + // Validate critical props + if (props.min >= props.max) { + console.warn('SliderField: min should be less than max'); + } + + // Filter slider-specific props + const { + min, max, step, value, onChange, + ...restProps + } = props; + + const sliderProps = { + min, max, step, value, onChange + }; + return ( <CommonField title={title} description={description}> - <Slider {...props} /> + <Slider {...sliderProps} /> </CommonField> ); -}; +}); + +SliderField.displayName = 'SliderField';web/src/beta/lib/reearth-ui/components/index.ts (2)
Line range hint
22-22: Remove duplicate export of PopupMenuThe
PopupMenucomponent is exported twice (see line 6 and line 22).Apply this diff to remove the duplicate:
-export * from "./PopupMenu";
32-32: Consider grouping related slider components togetherWhile the addition of
RangeSliderexport is correct, consider grouping it with the relatedSlidercomponent export for better code organization.Apply this diff to group related components:
-export * from "./Slider"; -export * from "./Slide"; -export * from "./ClickAway"; -export * from "./RangeSlider"; +export * from "./Slide"; +export * from "./Slider"; +export * from "./RangeSlider"; +export * from "./ClickAway";web/src/beta/lib/reearth-ui/components/Slider/index.stories.tsx (2)
18-18: Consider adding an interactive story variantWhile the simplified implementation is clean, consider adding another story variant that demonstrates the interactive behavior of the Slider component.
export const Interactive: Story = () => { const [value, setValue] = React.useState(0.5); return ( <Wrapper> <div> <Slider value={value} onChange={setValue} min={0} max={1} step={0.1} /> <div>Current value: {value}</div> </div> </Wrapper> ); };
24-34: Improve Disabled story implementationWhile the implementation works, consider these improvements:
- Move the values array to story args for better configurability
- Use a more stable key than array index
-export const Disabled: Story = (args: SliderProps) => { +export const Disabled: Story = ({ values = [10, 25], ...args }: SliderProps & { values?: number[] }) => { return ( <Wrapper> - {[10, 25].map((value, index) => ( + {values.map((value) => ( <div key={`slider-${value}`}> <Slider {...args} value={value} disabled={true} /> </div> ))} </Wrapper> ); };web/src/beta/lib/reearth-ui/components/Slider/index.tsx (2)
21-41: Handle undefined values explicitly.While the controlled component implementation is good, consider handling undefined values explicitly to prevent potential issues.
Consider this improvement:
- const [currentValue, setCurrentValue] = useState(value); + const [currentValue, setCurrentValue] = useState(value ?? 0); useEffect(() => { - setCurrentValue(value); + setCurrentValue(value ?? 0); }, [value]);
46-83: Consider performance optimization for box-shadow.The styling implementation is thorough and maintains consistency with the theme. However, box-shadow can impact rendering performance, especially during slider interactions.
Consider this optimization:
".rc-slider-rail": { height: "8px", backgroundColor: theme.outline.weaker, - boxShadow: theme.shadow.input + // Use a more performant alternative like border or background-image + borderBottom: `1px solid ${theme.outline.weak}` },web/src/beta/lib/reearth-ui/components/RangeSlider/index.tsx (2)
15-33: Improve step calculation robustness and readabilityConsider these improvements:
- Use strict equality comparison
- Add input validation
- Simplify the calculation logic
const calculateStep = ( min?: number, max?: number, step?: number | null ): number => { - if (step != undefined) { + if (step !== undefined && step !== null) { + if (step <= 0) { + console.warn('Step must be positive, defaulting to 1'); + return 1; + } return step; } else if (min !== undefined && max !== undefined) { + if (min >= max) { + console.warn('Min must be less than max, defaulting to 1'); + return 1; + } const range = max - min; - let calculatedStep = range / 10; - if (range % calculatedStep !== 0) { - const steps = Math.ceil(range / calculatedStep); - calculatedStep = range / steps; - } + const steps = 10; // desired number of steps + const calculatedStep = range / steps; return calculatedStep; } else { return 1; } };
78-115: Enhance accessibility and interaction statesConsider adding:
- ARIA attributes for better accessibility
- Hover and focus states for better user interaction
const SliderStyled = styled("div")<{ disabled: boolean }>( ({ disabled, theme }) => ({ width: "100%", + "[aria-disabled='true']": { + cursor: "not-allowed", + }, ".rc-slider-disabled": { backgroundColor: "transparent", opacity: disabled ? 0.6 : 1, cursor: disabled ? "not-allowed" : "inherit" }, ".rc-slider-handle": { backgroundColor: theme.item.default, border: theme.primary.weak, height: "12px", width: "12px", - marginTop: "-2px" + marginTop: "-2px", + "&:hover": { + border: `2px solid ${theme.primary.main}`, + }, + "&:focus": { + border: `2px solid ${theme.primary.main}`, + boxShadow: `0 0 0 3px ${theme.primary.weak}`, + outline: "none", + } },web/src/beta/ui/fields/Properties/ProprtyField.tsx (2)
147-169: Consider making the step value configurableThe step value is hardcoded to 0.1, which might not be suitable for all number fields. Consider deriving it from the schema like min/max values.
<SliderField key={schema.id} title={schema.name} value={value as number} min={schema.min} max={schema.max} - step={0.1} + step={schema.step ?? 0.1} description={schema.description} onChange={handleChange} />
Line range hint
1-205: Consider refactoring to reduce component complexityThe PropertyField component has grown quite large with multiple conditional branches for different field types. Consider breaking it down into smaller, more focused components using a strategy pattern or a field registry pattern.
This would:
- Improve maintainability
- Make testing easier
- Allow for better code organization
- Make it easier to add new field types
Example approach:
// fieldRegistry.ts const fieldRegistry = { string: { default: InputField, datetime: TimePointField, color: ColorField, // ... other string field types }, number: { default: NumberField, slider: SliderField, }, // ... other field types }; // PropertyField.tsx const PropertyField: FC<Props> = ({ schema, ...props }) => { const FieldComponent = fieldRegistry[schema.type]?.[schema.ui] ?? fieldRegistry[schema.type]?.default; return FieldComponent ? <FieldComponent {...props} /> : null; };server/pkg/builtin/manifest.yml (1)
272-276: LGTM! The FOV configuration changes look good.The updated field of view (FOV) configuration improves the camera settings by:
- Setting more practical min/max bounds (0.1 to 2.0 radians)
- Using a reasonable default value of 1 radian (≈57.3 degrees)
- Providing a clear description of the FOV's purpose
Consider adding degree equivalents in the description for better user understanding:
- description: The field of view (FOV) of the camera, in radians. It defines the vertical angular extent of the scene visible to the camera. And the range supported in our system is from 0.1 to 2.0 + description: The field of view (FOV) of the camera, in radians. It defines the vertical angular extent of the scene visible to the camera. The range supported in our system is from 0.1 to 2.0 radians (approximately 5.7° to 114.6°)web/src/beta/ui/fields/RangeField.tsx (1)
1-19: Consider renaming the file to match the component nameThe component has been renamed from
RangeFieldtoRangeSliderField, but the file name remainsRangeField.tsx. For consistency and to prevent confusion, it's recommended to rename the file toRangeSliderField.tsx.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
server/pkg/builtin/manifest.yml(1 hunks)server/pkg/builtin/manifest_ja.yml(1 hunks)web/src/beta/lib/reearth-ui/components/RangeSlider/index.stories.tsx(1 hunks)web/src/beta/lib/reearth-ui/components/RangeSlider/index.tsx(1 hunks)web/src/beta/lib/reearth-ui/components/Slider/index.stories.tsx(1 hunks)web/src/beta/lib/reearth-ui/components/Slider/index.tsx(2 hunks)web/src/beta/lib/reearth-ui/components/index.ts(1 hunks)web/src/beta/ui/fields/Properties/ProprtyField.tsx(3 hunks)web/src/beta/ui/fields/RangeField.tsx(1 hunks)web/src/beta/ui/fields/SliderField.tsx(1 hunks)
🔇 Additional comments (11)
web/src/beta/ui/fields/SliderField.tsx (3)
1-6: LGTM! Well-structured imports and type definition.
The imports are properly organized, and the type composition is clear and follows best practices.
19-19: LGTM! Proper export statement.
The default export is appropriate for a single component file.
1-19: Verify integration with PropertyField component.
Let's verify that this component is properly integrated with the PropertyField component and used correctly when schema type is "number" with UI "slider".
✅ Verification successful
SliderField integration verified successfully
The component is properly integrated:
- Correctly used in PropertyField when schema type is "number" and UI is "slider"
- Properly receives required props: value, min, max, step, and onChange handler
- Follows the same pattern as other field components using CommonField wrapper
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PropertyField component usage of SliderField
ast-grep --pattern 'case "slider": {
$$$
return <SliderField $$$>;
}'
# Search for any direct usage of SliderField
rg -l "SliderField"
Length of output: 223
Script:
#!/bin/bash
# Let's check the implementation of PropertyField to verify integration
rg -A 10 "SliderField" "web/src/beta/ui/fields/Properties/ProprtyField.tsx"
# Also check RangeField implementation since it showed up in results
rg -A 10 "SliderField" "web/src/beta/ui/fields/RangeField.tsx"
Length of output: 1403
Script:
#!/bin/bash
# Let's verify the schema type handling in PropertyField
ast-grep --pattern 'case $_: {
$$$
return <SliderField $$$>;
}'
# Also check the schema type definition
rg -B 5 "slider" "web/src/beta/ui/fields/Properties/ProprtyField.tsx"
Length of output: 325
web/src/beta/lib/reearth-ui/components/RangeSlider/index.stories.tsx (2)
1-12: LGTM! Clean and well-structured Storybook setup
The imports and metadata configuration follow Storybook best practices with proper type definitions.
32-48: 🛠️ Refactor suggestion
Maintain consistency between stories and enhance accessibility
- The styling is inconsistent with the Default story (missing height property)
- Same issues with step value and initial values as noted in the Default story
- Missing accessibility attributes for disabled state
Here's the suggested implementation:
export const Disabled: Story = {
render: () => (
<div
style={{
+ minHeight: "40px",
padding: "12px"
}}
>
<RangeSlider
- value={[2, 50]}
+ value={[0, 50]}
min={0}
max={100}
- step={10}
+ step={1}
disabled={true}
+ aria-label="Disabled range slider"
+ aria-valuemin={0}
+ aria-valuemax={100}
+ aria-disabled="true"
/>
</div>
)
};Let's verify if the disabled state is visually distinct:
✅ Verification successful
Styling and accessibility recommendations can be simplified
The component already has proper disabled state styling with:
- Reduced opacity (0.6)
- Not-allowed cursor
- Transparent background
- Inherited theme-based colors
The review can be updated to focus only on essential changes:
export const Disabled: Story = {
render: () => (
<div
style={{
+ minHeight: "40px",
padding: "12px"
}}
>
<RangeSlider
- value={[2, 50]}
+ value={[0, 50]}
min={0}
max={100}
- step={10}
+ step={1}
disabled={true}
+ aria-label="Disabled range slider"
/>
</div>
)
};The component already handles disabled state visually through styled-components. Additional aria attributes like aria-valuemin, aria-valuemax, and aria-disabled are unnecessary as they are automatically handled by the underlying rc-slider component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for disabled state styling in the component implementation
rg -A 5 "disabled.*{" web/src/beta/lib/reearth-ui/components/RangeSlider/
Length of output: 161
Script:
#!/bin/bash
# Check for disabled state styling in the component implementation
rg -A 5 "disabled" web/src/beta/lib/reearth-ui/components/RangeSlider/
# Also check the component implementation for styling
cat web/src/beta/lib/reearth-ui/components/RangeSlider/index.tsx
Length of output: 5525
web/src/beta/lib/reearth-ui/components/Slider/index.stories.tsx (2)
37-42: LGTM! Well-defined default arguments
The default arguments are sensible and provide good granularity with the 0.1 step value.
44-49: LGTM! Appropriate arguments for disabled state demonstration
The arguments effectively demonstrate the disabled state with a clear value range and step size.
web/src/beta/lib/reearth-ui/components/Slider/index.tsx (1)
3-3: LGTM! Clean import restructuring and type definition.
The changes improve code organization by using specific imports and a more concise type definition.
Also applies to: 14-14
web/src/beta/lib/reearth-ui/components/RangeSlider/index.tsx (2)
1-13: LGTM! Clean imports and well-structured type definition.
The type definition properly extends the base slider props while adding custom properties.
1-116: Verify component integration with specified use cases
The PR objectives mention this component will be used for Camera FOV, light intensity, opacity, and zoom level controls. Let's verify the implementation supports these use cases.
✅ Verification successful
Component implementation is suitable for the intended use cases
The RangeSlider component is properly integrated into the property field system and supports the intended use cases:
- It's used as a generic range input component through
RangeSliderField - The implementation in
PropertyField.tsxshows it's specifically used for array type properties with UI type "range" - The component supports all necessary features (min, max, step calculation, value updates) required for camera FOV, light intensity, opacity, and zoom level controls
- The styling implementation ensures consistent UI appearance with proper theming support
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the RangeSlider is properly integrated with the mentioned controls
# Check for RangeSlider usage in camera controls
echo "Checking camera controls integration..."
rg -l "RangeSlider.*['\"]fov['\"]" --type typescript
# Check for RangeSlider usage in light controls
echo "Checking light controls integration..."
rg -l "RangeSlider.*intensity" --type typescript
# Check for RangeSlider usage in opacity controls
echo "Checking opacity controls integration..."
rg -l "RangeSlider.*opacity" --type typescript
# Check for RangeSlider usage in zoom controls
echo "Checking zoom controls integration..."
rg -l "RangeSlider.*zoom" --type typescript
Length of output: 696
Script:
#!/bin/bash
# Description: Check if the RangeSlider is properly integrated with the mentioned controls
# Check for RangeSlider usage in camera controls
echo "Checking camera controls integration..."
rg -l "RangeSlider.*fov"
# Check for RangeSlider usage in light controls
echo "Checking light controls integration..."
rg -l "RangeSlider.*intensity"
# Check for RangeSlider usage in opacity controls
echo "Checking opacity controls integration..."
rg -l "RangeSlider.*opacity"
# Check for RangeSlider usage in zoom controls
echo "Checking zoom controls integration..."
rg -l "RangeSlider.*zoom"
# Let's also check for any imports of RangeSlider to find usage locations
echo "Checking RangeSlider imports..."
rg -l "import.*RangeSlider"
Length of output: 731
Script:
#!/bin/bash
# Let's examine the actual usage in the files that import RangeSlider
echo "Checking RangeField.tsx content..."
cat web/src/beta/ui/fields/RangeField.tsx
echo -e "\nChecking PropertyField.tsx content..."
cat web/src/beta/ui/fields/Properties/ProprtyField.tsx
# Also check for any other potential usage patterns
echo -e "\nChecking for any Range-related components in property fields..."
rg -l "Range.*Field"
Length of output: 7052
server/pkg/builtin/manifest_ja.yml (1)
139-140: LGTM! The FOV field description is clear and accurate.
The description properly explains:
- The purpose (camera's field of view)
- The unit (radians)
- What it controls (vertical angle range)
- The valid range (0.1 to 2.0)
This aligns well with the PR's objective to enhance camera settings UI and matches the changes made in the English version.
Let's verify the consistency with the English version:
✅ Verification successful
FOV field descriptions are consistent across language versions
The Japanese translation accurately matches the English version:
- Both specify the same range (0.1 to 2.0 radians)
- Both explain it's for camera's field of view in radians
- Both clarify it defines the vertical angular extent visible to the camera
- The technical terminology and measurements are preserved correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the FOV range values are consistent between ja and en manifests
# Expected: Both files should specify the same range (0.1 to 2.0)
echo "Checking FOV field in both language versions..."
rg -A 1 "fov:" server/pkg/builtin/manifest*.yml
Length of output: 298
Script:
#!/bin/bash
# Let's check the complete FOV field definition in both files
echo "Checking complete FOV field in Japanese manifest..."
rg -B 2 -A 5 "fov:" server/pkg/builtin/manifest_ja.yml
echo -e "\nChecking complete FOV field in English manifest..."
rg -B 2 -A 5 "fov:" server/pkg/builtin/manifest.yml
Length of output: 593
Script:
#!/bin/bash
# Let's try a different approach to find the English FOV field
# by searching for the word "FOV" in both files
echo "Searching for FOV mentions in both manifest files..."
rg -i "FOV" server/pkg/builtin/manifest*.yml
Length of output: 803
web/src/beta/lib/reearth-ui/components/RangeSlider/index.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
web/src/beta/ui/fields/RangeSliderField.tsx (1)
7-17: Consider adding prop validation for edge cases.While the implementation is clean and follows React best practices, consider adding validation for edge cases:
- Ensure range values are within acceptable bounds
- Handle undefined or null values gracefully
Example implementation:
const RangeSliderField: FC<RangeSliderFieldProps> = ({ title, description, + value, + min, + max, ...props }) => { + // Validate range values + const validatedValue = Array.isArray(value) + ? value.map(v => Math.min(Math.max(v, min ?? -Infinity), max ?? Infinity)) + : value; + return ( <CommonField title={title} description={description}> - <RangeSlider {...props} /> + <RangeSlider {...props} value={validatedValue} /> </CommonField> ); };web/src/beta/features/Visualizer/shared/hooks/useFieldComponent.tsx (1)
Line range hint
248-260: Add type safety and validationSeveral improvements could enhance the robustness of this implementation:
- Type assertions should be replaced with proper type checking
- Value bounds should be validated
- Null/undefined cases should be handled explicitly
Consider applying these changes:
<RangeSliderField key={field.id} title={field.title} - value={field?.value as number[]} + value={Array.isArray(field?.value) ? field.value : []} min={field.min} max={field.max} - defaultValue={field.defaultValue as number[]} + defaultValue={Array.isArray(field.defaultValue) ? field.defaultValue : []} description={field.description} + validate={value => { + if (!Array.isArray(value)) return false; + return value.every(v => + typeof v === 'number' && + (!field.min || v >= field.min) && + (!field.max || v <= field.max) + ); + }} onChange={handlePropertyValueUpdate( groupId, propertyId, fieldId, field?.type )} />web/src/beta/ui/fields/index.stories.tsx (1)
20-22: Clean up import organization.Remove the empty line between imports to maintain consistent spacing.
NumberField, - CheckboxField, RangeSliderField
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
web/src/beta/features/Visualizer/shared/hooks/useFieldComponent.tsx(2 hunks)web/src/beta/lib/reearth-ui/components/RangeSlider/index.stories.tsx(1 hunks)web/src/beta/lib/reearth-ui/components/RangeSlider/index.tsx(1 hunks)web/src/beta/ui/fields/Properties/ProprtyField.tsx(3 hunks)web/src/beta/ui/fields/RangeSliderField.tsx(1 hunks)web/src/beta/ui/fields/index.stories.tsx(2 hunks)web/src/beta/ui/fields/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/beta/lib/reearth-ui/components/RangeSlider/index.stories.tsx
- web/src/beta/lib/reearth-ui/components/RangeSlider/index.tsx
- web/src/beta/ui/fields/Properties/ProprtyField.tsx
🔇 Additional comments (8)
web/src/beta/ui/fields/RangeSliderField.tsx (4)
1-4: LGTM! Clean and specific imports.
The imports are well-organized, using named imports which is good for tree-shaking.
6-6: LGTM! Clear type definition.
The type definition is well-structured, combining CommonFieldProps and RangeSliderProps using intersection type, and properly exported for reuse.
19-19: LGTM! Clear default export.
The default export is appropriate for a single component file.
1-19: Verify integration with property field schema system.
Since this component is designed to be used with property fields where the schema indicates a slider UI, let's verify the integration:
✅ Verification successful
Based on the search results, I can now provide a final response:
RangeSliderField is properly integrated with the property field system
The verification shows proper integration:
- The component is used in
ProprtyField.tsxfor handling property fields with slider UI schema - The property schema system supports slider UI through
PropertySchemaFieldUienum and "ui" field inPropertySchemaFieldtype - The component is properly imported and used in the property field system's component selection logic (
useFieldComponent.tsx) - The implementation follows the same pattern as other field components, extending
CommonFieldPropsand integrating with the base UI component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the RangeSliderField is integrated with the property field system
# Check for property field schema definitions that specify slider UI
rg -A 5 '"ui":\s*"slider"' --type json
# Check for RangeSliderField usage in property related files
rg -A 5 "RangeSliderField" --type typescript
# Check for any existing slider-related schema validation
ast-grep --pattern 'interface $_ {
$$$
ui?: $_
$$$
}'
Length of output: 182
Script:
#!/bin/bash
# Let's try different approaches to verify the integration
# Check for RangeSliderField usage (using .ts and .tsx extensions)
rg -A 5 "RangeSliderField" -g "*.ts" -g "*.tsx"
# Look for property field type definitions
rg -A 10 "type PropertyField" -g "*.ts" -g "*.tsx"
# Search for slider-related schema or UI configurations
rg -A 5 "slider" -g "*.ts" -g "*.tsx"
# Check for property schema definitions
ast-grep --pattern 'type PropertySchemaField = {
$$$
}'
# Look for UI schema related types
ast-grep --pattern 'type $_ = {
ui?: $_
$$$
}'
Length of output: 33223
web/src/beta/ui/fields/index.ts (1)
18-18: LGTM! Verify the new slider components.
The addition of RangeSliderField and SliderField exports aligns with the PR objectives to enhance UI controls. These components will provide better user experience for numeric inputs like Camera FOV, light intensity, and opacity settings.
Let's verify the implementation of these new components:
Also applies to: 20-20
✅ Verification successful
Let me gather more information about the implementation details of these components.
Components are properly implemented and integrated
The verification shows that both slider components are well-implemented and correctly used:
SliderFieldis used for single-value numeric inputs withui: "slider"schema typeRangeSliderFieldis used for array-type inputs withui: "range"schema type- Both components are properly integrated in:
PropertyFieldfor schema-based property editinguseFieldComponenthook for dynamic field rendering- Both extend the common field interface and use base slider components from reearth-ui
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation and usage of the new slider components
# Check the implementation of SliderField
echo "Checking SliderField implementation..."
ast-grep --pattern 'export default function SliderField' -A 10
# Check the implementation of RangeSliderField
echo "Checking RangeSliderField implementation..."
ast-grep --pattern 'export default function RangeSliderField' -A 10
# Check where these components are being used
echo "Checking usage of slider components..."
rg -l "SliderField|RangeSliderField"
Length of output: 719
Script:
#!/bin/bash
# Check the actual implementation of SliderField
echo "=== SliderField Implementation ==="
cat web/src/beta/ui/fields/SliderField.tsx
echo -e "\n=== RangeSliderField Implementation ==="
cat web/src/beta/ui/fields/RangeSliderField.tsx
echo -e "\n=== Usage in PropertyField ==="
cat "web/src/beta/ui/fields/Properties/ProprtyField.tsx"
echo -e "\n=== Usage in useFieldComponent ==="
cat "web/src/beta/features/Visualizer/shared/hooks/useFieldComponent.tsx"
Length of output: 13341
web/src/beta/features/Visualizer/shared/hooks/useFieldComponent.tsx (2)
7-7: LGTM: Import statement is correctly placed
The RangeSliderField import follows the established pattern and is appropriately grouped with other field components.
Line range hint 256-260: Verify the onChange handler performance
The switch from onBlur to onChange means the handler will be called more frequently. Ensure that:
- The backend can handle the increased update frequency
- No performance issues occur with rapid slider movements
- The UX remains smooth with immediate updates
✅ Verification successful
Based on the analysis of the codebase, I can now provide a final response:
Switch from onBlur to onChange is safe and follows existing patterns
The codebase analysis shows that:
handlePropertyValueUpdateis already used withonChangehandlers in multiple components- Several components (Text, Link, Layer, Camera) implement their own debouncing when needed
- The implementation is consistent with other field components in the UI
The switch from onBlur to onChange aligns with the existing patterns in the codebase and won't cause performance issues since:
- Components that need debouncing already implement it locally
- The handler is properly memoized with
useCallback - Similar field components already use
onChangewithout issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other onChange handlers in field components to verify consistency
rg -A 2 "onChange={" "web/src/beta/ui/fields"
# Check if handlePropertyValueUpdate is debounced elsewhere
rg "handlePropertyValueUpdate.*debounce" "web/src/beta"
Length of output: 185
Script:
#!/bin/bash
# Search for other onChange handlers in field components to verify consistency
rg -l "onChange=" "web/src/beta/ui/fields" || true
# Check handlePropertyValueUpdate implementation
ast-grep --pattern 'const handlePropertyValueUpdate = $_'
# Search for debounce usage in the file
rg "debounce" "web/src/beta/features/Visualizer/shared/hooks/useFieldComponent.tsx"
# Look for similar field components with value updates
rg -A 3 "handlePropertyValueUpdate|handleValueUpdate|onValueChange" "web/src/beta/features/Visualizer"
Length of output: 34438
web/src/beta/ui/fields/index.stories.tsx (1)
142-144: Verify complete migration from RangeField to RangeSliderField.
Let's ensure all RangeField usages have been updated to RangeSliderField across the codebase.
✅ Verification successful
Let me try a different approach to search for these patterns.
Let me try one more search to specifically look for any remaining RangeField references:
Migration from RangeField to RangeSliderField is complete
The migration appears to be complete. The only remaining reference to "RangeField" is in a title prop of a RangeSliderField component in index.stories.tsx, which is just descriptive text and not a component reference. All actual component usages are properly using RangeSliderField with consistent prop patterns (value, defaultValue, etc.).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to RangeField
echo "Checking for remaining RangeField references..."
rg "RangeField" --type ts --type tsx
# Search for RangeSliderField usage patterns to verify proper prop usage
echo "Checking RangeSliderField usage patterns..."
ast-grep --pattern 'RangeSliderField value={$_} defaultValue={$_}'
Length of output: 335
Script:
#!/bin/bash
# Search for RangeField references with ripgrep, fixing the file type flags
echo "Checking for remaining RangeField references..."
rg "RangeField" -t typescript -t tsx
# Search for RangeSliderField references
echo "Checking RangeSliderField references..."
rg "RangeSliderField" -t typescript -t tsx
# Use ast-grep with correct syntax for JSX/TSX
echo "Checking RangeSliderField usage patterns..."
ast-grep --pattern '
<RangeSliderField
$$$
/>'
Length of output: 3447
Script:
#!/bin/bash
# Search for RangeField using just 'rg' without file type restrictions
echo "Checking for RangeField (without type restrictions)..."
rg "RangeField" --type-not md --type-not json
# Also check for any potential imports of RangeField
echo "Checking for RangeField imports..."
ast-grep --pattern 'import { $$$RangeField$$$ } from'
Length of output: 367
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
web/src/beta/lib/reearth-ui/components/Slider/index.tsx (3)
24-31: Consider simplifying precision calculation and adding documentation.The step calculation logic is sound, but could be improved:
- The default step of 0.1 should be documented
- The precision calculation could be simplified
Consider this improvement:
const getCalculatedStep = (min?: number, max?: number, step?: number) => { + // Return provided step or calculate based on min/max precision + // Defaults to 0.1 if no precision can be determined if (step !== undefined) return step; - const getPrecision = (num?: number) => - num ? num.toString().split(".")[1]?.length || 0 : 0; + const getPrecision = (num?: number) => + ((num?.toString().split(".")[1] || "").length) || 0; const precision = Math.max(getPrecision(min), getPrecision(max)); return precision > 0 ? Math.pow(10, -precision) : 0.1; };
33-40: Consider memoizing props to prevent unnecessary renders.The component could benefit from memoizing props to prevent unnecessary re-renders when parent components update.
Consider using
useCallbackfor theonAfterChangehandler:+const handleAfterChange = useCallback( + (val: number) => { + onAfterChange?.(val); + }, + [onAfterChange] +); return ( <SliderStyled disabled={props.disabled as boolean}> <SliderWithTooltip value={currentValue} step={calculatedStep} onChange={handleChange} - onAfterChange={onAfterChange} + onAfterChange={handleAfterChange} {...props} /> </SliderStyled> );Also applies to: 59-69
126-128: Document the reason for transition override.The transition override with
!importantsuggests there might be conflicting styles. Consider adding a comment explaining why this override is necessary.".rc-slider-handle, .rc-slider-tooltip-inner": { + // Disable transitions to prevent visual lag during rapid value changes transition: "none !important" }web/src/beta/lib/reearth-ui/components/Collapse/index.tsx (1)
Line range hint
22-63: Consider enhancing accessibility and performance.The component implementation is solid, but here are some suggested improvements:
- Add ARIA attributes for better accessibility
- Consider adding a smooth height transition animation for collapse/expand
- Optimize re-renders using React.memo
Here's how you could enhance the component:
-export const Collapse: FC<CollapseProps> = ({ +export const Collapse = memo<CollapseProps>(({ title, background, headerBg, size = "normal", weight = "regular", iconPosition = "right", collapsed, disabled, noPadding, actions, children, onCollapse -}) => { +}) => { const [isCollapsed, setIsCollapsed] = useState<boolean>(collapsed ?? false); useEffect(() => { setIsCollapsed(collapsed ?? false); }, [collapsed]); const handleCollapse = useCallback(() => { if (disabled) return; setIsCollapsed(!isCollapsed); onCollapse?.(!isCollapsed); }, [disabled, isCollapsed, onCollapse]); return ( <StyledWrapper> <StyledHeader onClick={handleCollapse} isCollapsed={isCollapsed} size={size} headerBg={headerBg} iconPosition={iconPosition} disabled={disabled} + role="button" + aria-expanded={!isCollapsed} + aria-controls="collapse-content" > <Typography size="body" weight={weight}> {title} </Typography> <ActionsWrapper> {actions} {!disabled && ( <IconWrapper isCollapsed={isCollapsed} iconPosition={iconPosition}> <Icon size="small" icon="triangle" /> </IconWrapper> )} </ActionsWrapper> </StyledHeader> {!isCollapsed && ( <ChildWrapper size={size} background={background} noPadding={noPadding} + id="collapse-content" + role="region" + aria-labelledby="collapse-header" > {children} </ChildWrapper> )} </StyledWrapper> ); -}); +}); + +Collapse.displayName = "Collapse";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
web/src/beta/lib/reearth-ui/components/Collapse/index.tsx(1 hunks)web/src/beta/lib/reearth-ui/components/Slider/index.tsx(2 hunks)web/src/beta/ui/fields/Properties/ProprtyField.tsx(3 hunks)web/src/beta/ui/fields/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/beta/ui/fields/Properties/ProprtyField.tsx
- web/src/beta/ui/fields/index.ts
🔇 Additional comments (3)
web/src/beta/lib/reearth-ui/components/Slider/index.tsx (2)
2-2: LGTM! Well-structured imports and type definitions.
The addition of debouncing options and React hooks is clean and maintains backward compatibility.
Also applies to: 4-11, 20-21
Line range hint 1-130: Verify integration with PropertyField component.
The Slider component looks solid, but let's verify its integration with PropertyField to ensure it meets the requirements for property fields where schema indicates a slider UI.
✅ Verification successful
Integration with PropertyField component is properly implemented
The verification shows that:
- SliderField is correctly integrated into the PropertyField component
- It's properly used when schema.type is "number" and schema.ui is "slider"
- The implementation includes all necessary props (min, max, debounced) and handles value changes appropriately
- The component is exported and available for use in the fields system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PropertyField usage of Slider component
ast-grep --pattern 'PropertyField {
$$$
ui="slider"
$$$
}'
# Search for SliderField component implementation
rg -A 5 "SliderField.*=.*{"
Length of output: 194
Script:
#!/bin/bash
# Search for PropertyField component usage patterns
rg -l "PropertyField.*slider"
# Search for any files containing both PropertyField and Slider imports/usage
rg -l "Slider.*PropertyField|PropertyField.*Slider"
# Search for files that might be using the Slider component
rg -l "import.*Slider.*from.*reearth-ui"
Length of output: 167
Script:
#!/bin/bash
# Let's examine the SliderField implementation
cat web/src/beta/ui/fields/SliderField.tsx
# Search for any other potential slider-related field components
rg -A 10 "import.*Slider" web/src/beta/ui/fields/
Length of output: 1302
Script:
#!/bin/bash
# Let's check how CommonField is implemented to ensure proper integration
cat web/src/beta/ui/fields/CommonField.tsx
# Let's also check where SliderField is being used
rg -l "SliderField"
Length of output: 1058
Script:
#!/bin/bash
# Let's check the PropertyField implementation to see how it uses SliderField
cat web/src/beta/ui/fields/Properties/ProprtyField.tsx
# Let's also check the fields index file to see how SliderField is exported
cat web/src/beta/ui/fields/index.ts
# And check the stories file to see usage examples
cat web/src/beta/ui/fields/index.stories.tsx
Length of output: 13839
web/src/beta/lib/reearth-ui/components/Collapse/index.tsx (1)
145-146: LGTM with a verification request.
The addition of overflowX: "hidden" while maintaining overflowY: "auto" is a reasonable enhancement for controlling content overflow. However, please verify that this change doesn't inadvertently hide any critical UI elements, particularly in the context of the new slider components being added in this PR.
Let's verify the impact on UI elements:
✅ Verification successful
Adding overflowX: "hidden" is safe and appropriate
The verification shows that:
- No Sliders, RangeSliders, ColorInputs, or NumberInputs are directly rendered inside Collapse components
- The Collapse component has minimal width constraints (only an icon width of 16px)
- The overflow changes are isolated to the content wrapper and won't affect child component functionality
The change is safe to implement as it provides better content control without impacting any critical UI elements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for components that might be affected by overflow changes
# Look for any slider-related components that might be rendered inside Collapse
rg -l "Collapse.*Slider|Slider.*Collapse" --type tsx
# Check for components with explicit width settings that might get cut off
ast-grep --pattern 'styled($_, $_) {
$$$
width: $_;
$$$
}'
Length of output: 149
Script:
#!/bin/bash
# Search for any slider-related components that might be rendered inside Collapse
rg -l "Collapse.*Slider|Slider.*Collapse"
# Look for components with explicit width settings that might get cut off
rg "width:" "web/src/beta/lib/reearth-ui/components"
# Check for any components that are commonly rendered inside Collapse
rg -A 5 "<Collapse" "web/src/beta/lib/reearth-ui"
Length of output: 9490
Script:
#!/bin/bash
# Check for components that are rendered inside Collapse in actual usage
rg -B 3 -A 3 "<Collapse.*>.*<(Slider|RangeSlider|ColorInput|NumberInput)" web/src/beta
# Check the Collapse component's content area width constraints
rg "width|maxWidth" web/src/beta/lib/reearth-ui/components/Collapse/index.tsx
# Look for any potential horizontal scroll triggers in child components
rg "overflow.*:" web/src/beta/lib/reearth-ui/components/Collapse/
Length of output: 408
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
web/src/beta/lib/reearth-ui/components/Slider/index.tsx (3)
19-26: Improve step calculation logic.The current implementation has a few edge cases to consider:
- Returns 0.1 as default even when min/max are integers
- Doesn't validate for negative steps
- Could be memoized at component level
Consider this enhanced implementation:
const getCalculatedStep = (min?: number, max?: number, step?: number) => { if (step !== undefined) return Math.abs(step); const getPrecision = (num?: number) => num ? num.toString().split(".")[1]?.length || 0 : 0; const precision = Math.max(getPrecision(min), getPrecision(max)); - return precision > 0 ? Math.pow(10, -precision) : 0.1; + return precision > 0 ? Math.pow(10, -precision) : 1; };
41-53: Consider debouncing onChange events for performance.Frequent value changes could lead to performance issues, especially when the slider is being actively dragged.
Consider debouncing the onChange handler:
+import debounce from 'lodash/debounce'; export const Slider: FC<SliderProps> = ({ // ... props }) => { + const debouncedOnChange = useMemo( + () => debounce((val: number) => { + onChange?.(val); + }, 100), + [onChange] + ); + + useEffect(() => { + return () => { + debouncedOnChange.cancel(); + }; + }, [debouncedOnChange]); const handleChange = useCallback( (val: number) => { setCurrentValue(val); - onChange?.(val); + debouncedOnChange(val); }, [onChange] );
70-109: Extract magic numbers into theme variables.The styling contains several hardcoded values that could be moved to theme variables for better maintainability.
Consider extracting these values:
const SliderStyled = styled("div")<{ disabled: boolean }>( ({ disabled, theme }) => ({ width: "100%", ".rc-slider-rail": { - height: "8px", + height: theme.slider?.railHeight ?? "8px", backgroundColor: theme.outline.weaker, boxShadow: theme.shadow.input }, ".rc-slider-handle": { backgroundColor: theme.item.default, border: theme.primary.weak, - height: "12px", - width: "12px", - marginTop: "-2px" + height: theme.slider?.handleSize ?? "12px", + width: theme.slider?.handleSize ?? "12px", + marginTop: theme.slider?.handleOffset ?? "-2px" },web/src/beta/lib/reearth-ui/components/RangeSlider/index.tsx (1)
9-17: Strengthen type definitions for better type safetyThe
valueprop beingundefinedcould lead to runtime issues. Consider making it required and using a default value in the parent component if needed.export type RangeSliderProps = { - value: number[] | undefined; + value: [number, number]; min?: number; max?: number; step?: number; disabled?: boolean; onChange?: (value: number[]) => void; onChangeComplete?: (value: number[]) => void; };web/src/beta/features/Editor/Visualizer/hooks.ts (1)
128-134: Consider optimizing the camera state updateWhile the FOV update logic is correct, it could be optimized to reduce potential state updates.
Consider this optimization:
- if (initialCamera?.fov && initialCamera.fov !== prevFOV.current) { - prevFOV.current = initialCamera.fov; - setCurrentCamera((c) => - !c ? undefined : { ...c, fov: initialCamera.fov } - ); - } + const newFov = initialCamera?.fov; + if (newFov && newFov !== prevFOV.current) { + prevFOV.current = newFov; + setCurrentCamera((c) => c && { ...c, fov: newFov }); + }This change:
- Extracts FOV value to avoid multiple optional chaining
- Simplifies the conditional expression
- Uses a more concise syntax for the camera update
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
web/src/beta/features/Editor/Visualizer/hooks.ts(3 hunks)web/src/beta/lib/reearth-ui/components/RangeSlider/index.tsx(1 hunks)web/src/beta/lib/reearth-ui/components/Slider/index.stories.tsx(1 hunks)web/src/beta/lib/reearth-ui/components/Slider/index.tsx(1 hunks)web/src/beta/ui/fields/Properties/ProprtyField.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/beta/lib/reearth-ui/components/Slider/index.stories.tsx
- web/src/beta/ui/fields/Properties/ProprtyField.tsx
🔇 Additional comments (5)
web/src/beta/lib/reearth-ui/components/Slider/index.tsx (1)
1-110: Verify consistent usage of the Slider component.
Let's ensure the component is used correctly throughout the codebase.
✅ Verification successful
Let me gather more specific information about the Slider usage patterns.
Let me check the actual implementation of SliderField and its usages to complete the verification.
Slider component implementation and usage is consistent
The verification shows that:
- The Slider component is properly wrapped in a SliderField component that handles common field properties
- All usages of the Slider are through the SliderField wrapper
- The component properly handles all required props (value, min, max, onChange) and is used in a controlled manner
- No direct usage of the Slider component outside of the designated wrapper was found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage patterns of the Slider component
# Find all imports of the Slider component
echo "Checking Slider component imports..."
rg -l "import.*Slider.*from.*reearth-ui/components/Slider"
# Find potential uncontrolled usage (missing value prop)
echo "Checking for potential uncontrolled usage..."
ast-grep --pattern 'Slider $props' -l
# Find usage of min/max props to verify boundaries
echo "Checking min/max usage patterns..."
rg "Slider.*min=|max=" -A 2
Length of output: 4657
Script:
#!/bin/bash
# Search for Slider component usage with value prop
echo "Checking Slider value prop usage..."
rg "<Slider.*value=" -A 2
# Search for Slider component usage with onChange/onChangeComplete
echo "Checking Slider onChange handlers..."
rg "<Slider.*(onChange|onChangeComplete)=" -A 2
# Find TypeScript imports to verify if the correct type is being used
echo "Checking Slider type imports..."
rg "import.*\{.*Slider.*\}.*from.*reearth-ui" -A 1
Length of output: 819
Script:
#!/bin/bash
# Check SliderField implementation
echo "Checking SliderField implementation..."
cat web/src/beta/ui/fields/SliderField.tsx
# Find SliderField usages
echo "Checking SliderField usages..."
rg "<SliderField" -A 3
# Check if there are any direct Slider usages without SliderField wrapper
echo "Checking direct Slider usages..."
rg "<Slider\s" -l | grep -v "SliderField.tsx\|index.stories.tsx\|index.tsx"
Length of output: 1644
web/src/beta/lib/reearth-ui/components/RangeSlider/index.tsx (2)
82-119: LGTM! Well-structured styling implementation
The styled component:
- Properly uses theme variables for consistency
- Handles disabled state appropriately
- Follows design specifications for the settings UI
1-119: Verify component usage in settings
Let's verify that the RangeSlider component is properly integrated into the settings UI as mentioned in the PR objectives.
web/src/beta/features/Editor/Visualizer/hooks.ts (2)
109-110: LGTM: Well-structured state management for FOV tracking
The addition of prevFOV ref follows React best practices for tracking previous values without triggering unnecessary re-renders.
155-155: Verify the stability of setCurrentCamera
The addition of setCurrentCamera to the dependency array is necessary, but we should ensure it's stable across renders to prevent unnecessary recalculations of the memoized value.
✅ Verification successful
Let me gather more information about the atom implementation to verify its stability.
setCurrentCamera is stable and dependency array is correct
The setCurrentCamera setter is provided by Jotai's useAtom hook, which guarantees stable references across renders. The current implementation using a basic Jotai atom is correct, and the inclusion of setCurrentCamera in the dependency array is appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if useCurrentCamera hook ensures stable setter
# Look for useCallback or similar stabilization in the hook implementation
# Search for useCurrentCamera implementation
ast-grep --pattern 'export const useCurrentCamera = () => {
$$$
}'
# Search for atom implementation that might be using jotai or recoil
rg -A 5 "useCurrentCamera|createAtom.*Camera"
Length of output: 2115
Script:
#!/bin/bash
# Search for currentCamera atom definition
rg -A 5 "currentCamera.*=.*atom"
# Search for the atom implementation file
fd "atoms.ts" --exec cat {}
Length of output: 697
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/src/beta/features/Visualizer/hooks.ts (2)
51-53: Good addition of cleanup logic to prevent stale camera references.The cleanup function properly resets the visualizer camera state when the component unmounts or when
currentCamerachanges. This is a good practice to prevent memory leaks and ensure proper state cleanup.Consider memoizing the cleanup function to optimize performance:
useLayoutEffect(() => { setVisualizerCamera(currentCamera); - return () => { - setVisualizerCamera(undefined); - }; + const cleanup = useCallback(() => { + setVisualizerCamera(undefined); + }, [setVisualizerCamera]); + return cleanup; }, [currentCamera, setVisualizerCamera]);
Line range hint
1-64: Consider documenting camera state management for slider integration.Since this hook manages camera state that will be controlled by the new FOV slider component, it would be helpful to add documentation about the state flow and any constraints that slider implementers should be aware of.
Consider adding a comment block above the
useHooksfunction explaining:
- The relationship between camera state and FOV slider
- Valid camera state transitions
- Any debouncing or performance considerations for slider updates
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1266 +/- ##
==========================================
- Coverage 23.13% 18.12% -5.01%
==========================================
Files 1015 628 -387
Lines 105755 67884 -37871
Branches 622 625 +3
==========================================
- Hits 24462 12304 -12158
+ Misses 80047 55580 -24467
+ Partials 1246 0 -1246
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Overview
In this PR, I updated the
Slidercomponent to align with the design specifications. Additionally, I introduced aSliderFieldcomponent to be used in property fields where schema.ui === "slider". This update enhances the scene settings UI by improving controls for:What I've done
Slidercomponent and addedSliderFieldSliderFieldwhen theschema.type === "number"andschema.ui === "slider"What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
RangeSlidercomponent for enhanced user interaction with customizable range selection.SliderFieldcomponent for improved numeric input handling.Enhancements
Slidercomponent's functionality and state management for a smoother user experience.SliderFieldto the UI fields for better integration.Bug Fixes
PropertyFieldcomponent.Documentation
SliderandRangeSlidercomponents to showcase default and disabled states.