feat(web): add extension settings panel to plugin playground #1383
Conversation
WalkthroughThis pull request streamlines type management and YAML parsing within the Plugin Playground module. It centralizes type imports, removes redundant local definitions and utility functions, and updates function signatures—most notably in the hook. New helper functions and components are added to improve schema field processing and extension settings management. The changes also reconfigure the import paths for utilities and extend the plugin presets with a new extension category. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant ES as ExtensionSettings
participant Utils as YAML Utility
participant PI as PropertyItem
U->>ES: Selects plugin/extension
ES->>Utils: Call getYmlJson(file)
Utils-->>ES: Return YAML parsing result
alt Parsing Error
ES->>U: Display error message
else Valid YAML
ES->>ES: Verify extensions & schema fields
ES->>PI: Render PropertyItem for each field
PI->>ES: onUpdate event → update fieldValues
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
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 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: 5
🧹 Nitpick comments (3)
web/src/beta/features/PluginPlayground/utils.ts (1)
6-17: Add case-insensitive extension matching.The function should handle uppercase file extensions (e.g., .JS, .YML).
export const getLanguageByFileExtension = (fileTitle: string) => { - const ext = fileTitle.split(".").pop(); + const ext = fileTitle.split(".").pop()?.toLowerCase(); switch (ext) { case "js": return "javascript";web/src/beta/features/PluginPlayground/Plugins/presets/custom/myPlugin.ts (1)
20-36: Consider improving schema field definitions.The schema structure could be enhanced for better maintainability:
- Use descriptive IDs instead of numeric values (e.g., "text_field" instead of "1")
- Remove redundancy between
nameandtitleproperties as they serve similar purposesschema: groups: - id: default title: Data fields: - - id: 1 + - id: text_field type: string title: Text - name: Title - - id: 2 + - id: number_field type: number title: Number - name: Number - - id: 3 + - id: boolean_field type: bool title: Boolean - name: Booleanweb/src/beta/features/PluginPlayground/WidgetsList/index.tsx (1)
22-24: Use optional chaining for better code readability.The current nested check can be simplified using optional chaining.
- const ymlFile = - selectedPlugin.files && - selectedPlugin.files.find((f) => f.title.endsWith("reearth.yml")); + const ymlFile = selectedPlugin.files?.find((f) => f.title.endsWith("reearth.yml"));🧰 Tools
🪛 Biome (1.9.4)
[error] 23-24: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
web/src/beta/features/PluginPlayground/Code/hook.ts(1 hunks)web/src/beta/features/PluginPlayground/Code/index.tsx(1 hunks)web/src/beta/features/PluginPlayground/Code/utils.ts(0 hunks)web/src/beta/features/PluginPlayground/Plugins/presets/custom/myPlugin.ts(1 hunks)web/src/beta/features/PluginPlayground/WidgetsList/PropertyItem.tsx(1 hunks)web/src/beta/features/PluginPlayground/WidgetsList/index.tsx(1 hunks)web/src/beta/features/PluginPlayground/hooks.tsx(2 hunks)web/src/beta/features/PluginPlayground/index.tsx(2 hunks)web/src/beta/features/PluginPlayground/types.ts(1 hunks)web/src/beta/features/PluginPlayground/utils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- web/src/beta/features/PluginPlayground/Code/utils.ts
✅ Files skipped from review due to trivial changes (1)
- web/src/beta/features/PluginPlayground/Code/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
web/src/beta/features/PluginPlayground/WidgetsList/index.tsx
[error] 23-24: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 56-56: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-web / ci
- GitHub Check: ci-server / ci-server-test
🔇 Additional comments (5)
web/src/beta/features/PluginPlayground/utils.ts (1)
19-34: Consider adding validation for the parsed YAML structure.While the function handles parsing errors well, it doesn't validate if the parsed data matches the expected
ReearthYMLstructure.Consider adding runtime type checking:
try { const data = yaml.load(file.sourceCode) as ReearthYML; + if (!data?.id || !data?.name || !data?.version) { + return { + success: false, + message: "Invalid YAML structure: missing required fields" + } as const; + } return { success: true, data } as const;✅ Verification successful
Runtime validation for ReearthYML structure is indeed necessary
The suggestion is valid as the current type assertion (
as ReearthYML) only provides compile-time checking. The proposed validation ensures the required fields (id,name,version) are present at runtime, preventing potential issues with malformed YAML files that pass parsing but lack mandatory fields.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find ReearthYML type definition ast-grep --pattern 'type ReearthYML = {$$$}' # Find interface definition if type not found ast-grep --pattern 'interface ReearthYML {$$$}' # Find usages of getYmlJson rg "getYmlJson" -A 2 # Look for any existing validation patterns rg "ReearthYML" -A 5Length of output: 6101
web/src/beta/features/PluginPlayground/types.ts (1)
7-28: Consider making schema required for widget extensions.The
schemaproperty is optional, but widgets typically require configuration schema to be functional.export type ReearthYML = { id: string; name: string; version: string; extensions?: { id: string; type: string; name: string; description: string; widgetLayout?: { extended: boolean; defaultLocation: { zone: WidgetLocation["zone"]; section: WidgetLocation["section"]; area: WidgetLocation["area"]; }; }; - schema?: { + schema: { groups: Group; }; }[]; };web/src/beta/features/PluginPlayground/index.tsx (1)
35-37: LGTM! Panel implementation follows existing patterns.The WidgetsPanel integration is clean and consistent with the existing layout structure.
web/src/beta/features/PluginPlayground/Code/hook.ts (1)
8-9: LGTM! Good refactoring of types and utilities.The changes improve code organization by:
- Centralizing type definitions
- Moving YAML parsing logic to a dedicated utility file
web/src/beta/features/PluginPlayground/hooks.tsx (1)
192-194: LGTM! Clean integration of the widgets panel.The WidgetsPanel component is well-integrated and follows the established pattern of panel components in the codebase.
web/src/beta/features/PluginPlayground/WidgetsList/PropertyItem.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/PluginPlayground/WidgetsList/PropertyItem.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
web/src/beta/features/PluginPlayground/types.ts (1)
7-28: Consider improving type safety for extension type.The
typeproperty in extensions could be more strictly typed to prevent invalid values.extensions?: { id: string; - type: string; + type: "widget" | "block" | string; name: string; description: string;Also, consider adding JSDoc comments to explain the purpose of
widgetLayout.extended.web/src/beta/features/PluginPlayground/WidgetsList/PropertyItem.tsx (1)
44-69: Improve type safety in render logic.Consider using type narrowing instead of type assertions for better type safety.
- value={value as number} + value={typeof value === 'number' ? value : 0} - value={value as boolean} + value={typeof value === 'boolean' ? value : false} - value={value as string} + value={typeof value === 'string' ? value : ''}web/src/beta/features/PluginPlayground/WidgetsList/index.tsx (3)
22-32: Improve code readability and error handling.
- Use optional chaining for better readability
- Consider using a proper error state component instead of empty div
- const ymlFile = - selectedPlugin.files && - selectedPlugin.files.find((f) => f.title.endsWith("reearth.yml")); + const ymlFile = selectedPlugin.files?.find((f) => + f.title.endsWith("reearth.yml") + ); if (!ymlFile) { setNotification({ type: "error", text: "No YAML configuration file found" }); - return <div />; + return <EmptyState message="No YAML configuration file found" />; }🧰 Tools
🪛 Biome (1.9.4)
[error] 23-24: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
66-72: Improve error message specificity.The error message "No schema found in widget extension" is used for different error conditions. Consider using more specific messages:
if (!widgetSchema || widgetSchema.length == 0) { setNotification({ type: "error", - text: "No schema found in widget extension" + text: "Widget schema is empty or undefined" }); - return <div />; + return <EmptyState message="No widget schema available" />; }
74-81: Add support for multiple schema groups.The current implementation only renders fields from the first group. Consider supporting multiple groups and add error handling for empty fields.
- const { fields } = widgetSchema[0]; + return ( + <Wrapper> + {widgetSchema.map(group => ( + <div key={group.id}> + <h3>{group.title}</h3> + {group.fields?.map((field) => ( + <PropertyItem key={field.id} field={field} /> + )) ?? <EmptyState message="No fields in this group" />} + </div> + ))} + </Wrapper> + ); - return ( - <Wrapper> - {fields.map((field) => ( - <PropertyItem key={field.id} field={field} /> - ))} - </Wrapper> - );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/src/beta/features/PluginPlayground/WidgetsList/PropertyItem.tsx(1 hunks)web/src/beta/features/PluginPlayground/WidgetsList/index.tsx(1 hunks)web/src/beta/features/PluginPlayground/types.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
web/src/beta/features/PluginPlayground/WidgetsList/PropertyItem.tsx
[error] 32-32: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
web/src/beta/features/PluginPlayground/WidgetsList/index.tsx
[error] 23-24: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-server / ci-server-test
- GitHub Check: ci-web / ci
🔇 Additional comments (3)
web/src/beta/features/PluginPlayground/types.ts (2)
30-30: LGTM!Clean and effective type extraction from Visualizer component props.
43-47: LGTM! Type-safe implementation achieved.The
defaultValueproperty uses a proper union type as previously suggested, which enhances type safety.web/src/beta/features/PluginPlayground/WidgetsList/PropertyItem.tsx (1)
15-24: LGTM! Clean state management implementation.Good use of a single state variable with type-safe initialization.
web/src/beta/features/PluginPlayground/WidgetsList/PropertyItem.tsx
Outdated
Show resolved
Hide resolved
mulengawilfred
changed the title
mulengawilfred
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
web/src/beta/features/PluginPlayground/utils.ts (2)
6-17: Consider using a Map for language mapping.The switch statement could be replaced with a Map for better maintainability and performance.
-export const getLanguageByFileExtension = (fileTitle: string) => { - const ext = fileTitle.split(".").pop(); - switch (ext) { - case "js": - return "javascript"; - case "yml": - case "yaml": - return "yaml"; - default: - return "plaintext"; - } -}; +const languageMap = new Map([ + ["js", "javascript"], + ["yml", "yaml"], + ["yaml", "yaml"] +]); + +export const getLanguageByFileExtension = (fileTitle: string) => { + const ext = fileTitle.split(".").pop(); + return languageMap.get(ext ?? "") ?? "plaintext"; +};
36-48: Add type guard for array check.The array check could be more type-safe.
-export const customSchemaFieldsToObject = (array: CustomSchemaField[]) => { +export const customSchemaFieldsToObject = (array: CustomSchemaField[] | undefined) => { if (Array.isArray(array)) { return array.reduce( (acc, item) => { const { id, ...rest } = item; acc[id as string] = rest; return acc; }, {} as Record<string, unknown> ); } return {}; };web/src/beta/features/PluginPlayground/Plugins/presets/custom/myPlugin.ts (1)
20-34: Consider adding validation constraints to schema fields.The schema fields could benefit from additional validation constraints.
- id: bgColor type: string title: Background Color name: Background Color ui: color + required: true + defaultValue: "#ffffff" - id: title type: string title: Title name: Title + required: true + maxLength: 100 + pattern: "^[a-zA-Z0-9\\s]+$"web/src/beta/features/PluginPlayground/WidgetsList/index.tsx (3)
41-43: Use optional chaining for better readability.The nested property access could be simplified using optional chaining.
- const ymlFile = - selectedPlugin.files && - selectedPlugin.files.find((f) => f.title.endsWith("reearth.yml")); + const ymlFile = selectedPlugin.files?.find((f) => f.title.endsWith("reearth.yml"));🧰 Tools
🪛 Biome (1.9.4)
[error] 42-43: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
37-115: Consider memoizing the component for better performance.Since the component receives props that could frequently change, memoization could improve performance.
-const WidgetsList: FC<Props> = ({ +const WidgetsList: FC<Props> = memo(({ selectedPlugin, setUpdatedField -}): JSX.Element => { +}) => { // ... component logic ... -}; +}); + +WidgetsList.displayName = "WidgetsList";🧰 Tools
🪛 Biome (1.9.4)
[error] 42-43: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
47-51: Centralize error messages for better maintainability.Consider moving error messages to a centralized constants file.
// constants.ts export const WIDGET_ERRORS = { NO_YML: "This plugin does not have a reearth.yml file.", INVALID_YML: "This plugin does not have a valid reearth.yml file.", NO_WIDGET: "This plugin does not have a valid widget extension.", NO_WIDGETS: "This plugin does not have any widgets." } as const;Also applies to: 59-61, 69-73, 84-88, 96-98
web/src/beta/features/PluginPlayground/WidgetsList/PropertyItem.tsx (2)
57-73: Add value validation in handleChange.The current implementation doesn't validate values before updating the state.
Add validation to ensure values match the expected type:
const handleChange = ({ newValue, fieldId }: { newValue: | boolean | LatLng | number | number[] | string | string[] | SpacingValues; fieldId: string; }): void => { + // Validate value based on field type + if ( + (field.type === "number" && typeof newValue !== "number") || + (field.type === "bool" && typeof newValue !== "boolean") || + (field.type === "array" && !Array.isArray(newValue)) + ) { + console.warn(`Invalid value type for field ${field.id}`); + return; + } setValue(newValue); setUpdatedField({ fieldId, value: newValue }); };
205-210: Handle undefined values consistently in NumberField.The current implementation uses a fallback of 0 for undefined values, which might not be appropriate for all number fields.
Consider using the field's minimum value as a fallback:
onChange={(newValue?: number) => handleChange({ - newValue: newValue ?? 0, + newValue: newValue ?? field.min ?? 0, fieldId: field.id }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
web/src/beta/features/PluginPlayground/Code/hook.ts(3 hunks)web/src/beta/features/PluginPlayground/Plugins/presets/custom/myPlugin.ts(1 hunks)web/src/beta/features/PluginPlayground/WidgetsList/PropertyItem.tsx(1 hunks)web/src/beta/features/PluginPlayground/WidgetsList/index.tsx(1 hunks)web/src/beta/features/PluginPlayground/hooks.tsx(3 hunks)web/src/beta/features/PluginPlayground/types.ts(1 hunks)web/src/beta/features/PluginPlayground/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/beta/features/PluginPlayground/hooks.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
web/src/beta/features/PluginPlayground/WidgetsList/index.tsx
[error] 42-43: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
web/src/beta/features/PluginPlayground/Code/hook.ts
[error] 211-211: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci-web / ci
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-server / ci-server-test
🔇 Additional comments (3)
web/src/beta/features/PluginPlayground/utils.ts (1)
19-34: LGTM! Well-structured error handling.The function properly handles empty files and YAML parsing errors with clear error messages.
web/src/beta/features/PluginPlayground/types.ts (1)
8-59: LGTM! Well-structured type definitions.The types are comprehensive, properly nested, and follow TypeScript best practices.
web/src/beta/features/PluginPlayground/Code/hook.ts (1)
121-125: Improve type safety in widget property initialization.The current implementation assumes schema fields are always defined when creating widget properties.
Add a null check before accessing schema fields:
property: { - default: customSchemaFieldsToObject( - schemaFields as CustomSchemaField[] - ) + default: schemaFields ? + customSchemaFieldsToObject(schemaFields) : {} },✅ Verification successful
Error: No shell scripts or web queries found
Improve type safety by adding a null-check on schemaFields.
It’s confirmed that in file web/src/beta/features/PluginPlayground/Code/hook.ts (lines 121–125) the widget property is initialized using:default: customSchemaFieldsToObject(schemaFields as CustomSchemaField[])This directly assumes that
schemaFieldsis always defined. The improvement suggested—checking ifschemaFieldsexists before callingcustomSchemaFieldsToObjectand defaulting to an empty object if not—enhances type safety and guards against potential runtime errors.
- File to update:
web/src/beta/features/PluginPlayground/Code/hook.ts(lines 121–125)- Suggested change:
property: { - default: customSchemaFieldsToObject( - schemaFields as CustomSchemaField[] - ) + default: schemaFields ? + customSchemaFieldsToObject(schemaFields) : {} },
web/src/beta/features/PluginPlayground/WidgetsList/PropertyItem.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
web/src/beta/features/PluginPlayground/types.ts (1)
46-58: Consider adding validation constraints to FieldValue type.While the type definition is correct, consider adding validation constraints or branded types for specific field types (e.g., range validation for numbers, pattern validation for strings).
web/src/beta/features/PluginPlayground/ExtensionSettings/index.tsx (1)
43-45: Improve optional chaining usage.Optimize the code by using optional chaining operator.
- const ymlFile = - selectedPlugin.files && - selectedPlugin.files.find((f) => f.title.endsWith("reearth.yml")); + const ymlFile = selectedPlugin.files?.find((f) => f.title.endsWith("reearth.yml"));🧰 Tools
🪛 Biome (1.9.4)
[error] 44-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
web/src/beta/features/PluginPlayground/ExtensionSettings/PropertyItem.tsx (1)
58-198: Refactor conditional rendering for better maintainability.The deep nesting of conditional rendering makes the code hard to maintain. Consider extracting each field type into a separate component.
Example refactor for string fields:
const StringField: FC<FieldProps> = ({ field, value, handleChange }) => { switch (field.ui) { case 'datetime': return <TimePointField ... />; case 'selection': return <SelectField ... />; // ... other cases default: return <InputField ... />; } };web/src/beta/features/PluginPlayground/Code/hook.ts (1)
230-230: Optimize useCallback dependencies.The
schemaFieldsdependency in theuseCallbackhook's dependency array might cause unnecessary re-renders as it's only used in the type assertion.Remove the unnecessary dependency:
- }, [updatedField, schemaFields, files, resetVisualizer, setNotification]); + }, [updatedField, files, resetVisualizer, setNotification]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
web/src/beta/features/PluginPlayground/Code/hook.ts(3 hunks)web/src/beta/features/PluginPlayground/ExtensionSettings/PropertyItem.tsx(1 hunks)web/src/beta/features/PluginPlayground/ExtensionSettings/index.tsx(1 hunks)web/src/beta/features/PluginPlayground/Plugins/presets/custom/myPlugin.ts(1 hunks)web/src/beta/features/PluginPlayground/hooks.tsx(3 hunks)web/src/beta/features/PluginPlayground/index.tsx(2 hunks)web/src/beta/features/PluginPlayground/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/beta/features/PluginPlayground/index.tsx
- web/src/beta/features/PluginPlayground/Plugins/presets/custom/myPlugin.ts
🧰 Additional context used
🪛 Biome (1.9.4)
web/src/beta/features/PluginPlayground/ExtensionSettings/index.tsx
[error] 44-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
web/src/beta/features/PluginPlayground/Code/hook.ts
[error] 209-209: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-server / ci-server-test
- GitHub Check: ci-web / ci
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
🔇 Additional comments (7)
web/src/beta/features/PluginPlayground/types.ts (1)
10-31: LGTM! Well-structured type definition for ReearthYML.The type definition is comprehensive and properly typed, with clear hierarchical structure for plugin configuration.
web/src/beta/features/PluginPlayground/hooks.tsx (1)
200-207: Avoid type casting and add runtime validation.The type casting of schemaFields could be unsafe. Consider adding runtime validation before the cast.
web/src/beta/features/PluginPlayground/Code/hook.ts (5)
10-44: LGTM! Well-structured type definitions.The type definitions are comprehensive and properly encapsulate all necessary functionality for the widget panel, including schema field management.
46-62: LGTM! Clean state management setup.The state management for schema fields and field updates is well-organized and properly typed.
209-209: Fix performance issue in schema field accumulation.The spread operator in the accumulator causes O(n²) time complexity. This can impact performance when dealing with a large number of schema fields.
🧰 Tools
🪛 Biome (1.9.4)
[error] 209-209: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
212-227: Improve type safety in field value updates.The current implementation assumes the value type is string when updating the field value, which could lead to type-related issues.
121-123: Add null check for schema fields.The type assertion
schemaFields as CustomSchemaField[]could be unsafe ifschemaFieldsis undefined.Add a null check or provide a default value:
- property: customSchemaFieldsToObject( - schemaFields as CustomSchemaField[] - ), + property: customSchemaFieldsToObject( + schemaFields ?? [] + ),✅ Verification successful
Add null-check for
schemaFieldsinhook.tsThe review comment is valid. Relying on a type assertion (
schemaFields as CustomSchemaField[]) is unsafe ifschemaFieldsisundefined, which could lead to runtime issues. Updating the code to use the nullish coalescing operator (i.e.,schemaFields ?? []) ensures a safe default value is provided.
- Location:
web/src/beta/features/PluginPlayground/Code/hook.ts, lines 121–123- Action: Replace the type assertion with a null check:
- property: customSchemaFieldsToObject( - schemaFields as CustomSchemaField[] - ), + property: customSchemaFieldsToObject( + schemaFields ?? [] + ),
web/src/beta/features/PluginPlayground/ExtensionSettings/index.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/PluginPlayground/ExtensionSettings/PropertyItem.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
web/src/beta/features/PluginPlayground/ExtensionSettings/PropertyItem.tsx (3)
23-33: Add JSDoc comments to document the Props type.Consider adding JSDoc comments to document the purpose and usage of each prop for better maintainability.
+/** + * Props for the PropertyItem component + * @property {FieldValue} initialValue - The initial value of the field + * @property {SchemaField} field - The schema definition for the field + * @property {Function} setUpdatedField - Callback function to update the field value + */ type Props = { initialValue: FieldValue; field: SchemaField; setUpdatedField: ({
37-47: Simplify the assetTypes computation logic.The nested ternary operators make the code harder to read. Consider using a more straightforward approach.
- const assetTypes: "image"[] | "file"[] | undefined = useMemo( - () => - field.type === "url" - ? field.ui === "image" - ? ["image" as const] - : field.ui === "file" - ? ["file" as const] - : undefined - : undefined, - [field.type, field.ui] - ); + const assetTypes = useMemo(() => { + if (field.type !== "url") return undefined; + if (field.ui === "image") return ["image" as const]; + if (field.ui === "file") return ["file" as const]; + return undefined; + }, [field.type, field.ui]);
63-63: Add type guards to avoid type assertions.Replace type assertions with proper type guards to improve type safety.
const isStringValue = (value: FieldValue): value is string => typeof value === 'string'; const isNumberValue = (value: FieldValue): value is number => typeof value === 'number'; // Then in your JSX: value={isStringValue(value) ? value : ''}Also applies to: 71-71, 87-87, 97-97, 104-104, 119-119, 126-126, 137-137, 145-145, 155-155, 167-167, 175-175, 183-183
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/beta/features/PluginPlayground/ExtensionSettings/PropertyItem.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-server / ci-server-test
- GitHub Check: ci-web / ci
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
🔇 Additional comments (1)
web/src/beta/features/PluginPlayground/ExtensionSettings/PropertyItem.tsx (1)
49-54: Replace setTimeout with proper debounce utility.Using setTimeout for debouncing can lead to memory leaks. Consider using a proper debounce utility.
web/src/beta/features/PluginPlayground/ExtensionSettings/PropertyItem.tsx
Outdated
Show resolved
Hide resolved
|
Will put this as a |
mulengawilfred
changed the title
airslice
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/src/beta/features/PluginPlayground/ExtensionSettings/PropertyItem.tsx (1)
52-192: 🛠️ Refactor suggestionRefactor conditional rendering logic for better maintainability.
The nested ternary operators and if-else chains make the code hard to maintain. This is a duplicate of a previous review comment, but it's still valid.
Additionally, consider adding error boundaries to handle potential runtime errors in field rendering:
+import { ErrorBoundary } from 'react-error-boundary'; + +const FieldErrorFallback = ({ error, field }) => ( + <div> + <p>Error rendering {field.name}: {error.message}</p> + </div> +); + const PropertyItem: FC<Props> = ({ id, value, field, onUpdate }) => { // ... existing code ... return ( + <ErrorBoundary + FallbackComponent={({ error }) => ( + <FieldErrorFallback error={error} field={field} /> + )} + > {/* existing rendering logic */} + </ErrorBoundary> ); };
🧹 Nitpick comments (4)
web/src/beta/features/PluginPlayground/ExtensionSettings/index.tsx (1)
31-33: Simplify with optional chaining.The check for selectedPlugin.files can be simplified.
- const ymlFile = - selectedPlugin.files && - selectedPlugin.files.find((f) => f.title.endsWith("reearth.yml")); + const ymlFile = selectedPlugin.files?.find((f) => f.title.endsWith("reearth.yml"));🧰 Tools
🪛 Biome (1.9.4)
[error] 32-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
web/src/beta/features/PluginPlayground/ExtensionSettings/PropertyItem.tsx (1)
1-21: Consider organizing imports for better maintainability.The imports could be better organized by grouping them into:
- External dependencies
- Internal UI components
- Types and utilities
+// External dependencies import { FC, useCallback, useMemo } from "react"; +// Internal types and utilities import { SpacingValues } from "@reearth/beta/ui/fields/SpacingField"; import { Camera, LatLng } from "@reearth/beta/utils/value"; import { SchemaField } from "@reearth/services/api/propertyApi/utils"; import { FieldValue } from "../types"; +// UI components import { ColorField, InputField, SelectField, TimePointField, TextareaField, AssetField, SpacingField, SwitchField, SliderField, NumberField, TwinInputField, CameraField, RangeField } from "../../../ui/fields";web/src/beta/features/PluginPlayground/hooks.tsx (2)
29-31: Consider persisting field values in local storage.Based on the PR objectives mentioning "input field data will be stored locally", consider persisting the field values in local storage to maintain state across page reloads.
+import { useLocalStorage } from '@reearth/beta/hooks'; + -const [fieldValues, setFieldValues] = useState<Record<string, FieldValue>>({}); +const [fieldValues, setFieldValues] = useLocalStorage<Record<string, FieldValue>>( + 'plugin-playground-field-values', + {} +);
197-204: Consider adding loading and error states.The ExtensionSettingsPanel should handle loading and error states when interacting with field values.
const ExtensionSettingsPanel: FC = () => ( + <Suspense fallback={<div>Loading settings...</div>}> + <ErrorBoundary fallback={<div>Error loading settings</div>}> <ExtensionSettings selectedPlugin={selectedPlugin} selectedFile={selectedFile} fieldValues={fieldValues} setFieldValues={setFieldValues} /> + </ErrorBoundary> + </Suspense> );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
web/src/beta/features/PluginPlayground/Code/hook.ts(4 hunks)web/src/beta/features/PluginPlayground/ExtensionSettings/PropertyItem.tsx(1 hunks)web/src/beta/features/PluginPlayground/ExtensionSettings/index.tsx(1 hunks)web/src/beta/features/PluginPlayground/Plugins/presets/extension/extensionProperty.ts(1 hunks)web/src/beta/features/PluginPlayground/Plugins/presets/index.ts(2 hunks)web/src/beta/features/PluginPlayground/hooks.tsx(4 hunks)web/src/beta/features/PluginPlayground/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/beta/features/PluginPlayground/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
web/src/beta/features/PluginPlayground/ExtensionSettings/index.tsx
[error] 32-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
🔇 Additional comments (6)
web/src/beta/features/PluginPlayground/Plugins/presets/extension/extensionProperty.ts (1)
1-67: LGTM! Well-structured plugin definition.The implementation is clean and follows best practices:
- Clear separation between schema (YAML) and UI (widget) code
- Proper event handling for property updates
- Good use of constants and types
web/src/beta/features/PluginPlayground/Plugins/presets/index.ts (1)
8-8: LGTM! Clean addition of extension category.The changes properly integrate the new extension property plugin into the preset plugins structure.
Also applies to: 99-103
web/src/beta/features/PluginPlayground/ExtensionSettings/index.tsx (1)
25-102: LGTM! Well-structured component with proper error handling.The component implementation:
- Properly validates YAML structure
- Handles errors gracefully
- Uses appropriate React patterns
🧰 Tools
🪛 Biome (1.9.4)
[error] 32-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
web/src/beta/features/PluginPlayground/Code/hook.ts (1)
32-184: LGTM! Clean implementation of field value handling.The hook implementation:
- Properly integrates field values
- Has good error handling
- Uses appropriate React patterns with useCallback
🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 135-135: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
web/src/beta/features/PluginPlayground/ExtensionSettings/PropertyItem.tsx (1)
43-50: Consider adding error handling for undefined values.The
handleChangecallback silently ignores undefined values. Consider whether this is the intended behavior or if we should handle these cases explicitly.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for similar patterns in the codebase to verify the convention rg -A 5 'const handleChange.*useCallback.*\(.*\).*=>'Length of output: 55
🏁 Script executed:
#!/bin/bash # Let's search within the file to see the full context of handleChange and similar patterns. rg -A 10 "const handleChange.*useCallback" web/src/beta/features/PluginPlayground/ExtensionSettings/PropertyItem.tsxLength of output: 305
Based on the information provided, the callback’s behavior is to simply ignore
undefinedvalues by not callingonUpdatewhennewValueis not provided. This pattern appears intentional and consistent with the function’s annotated behavior (using an optional parameter), as there’s no evidence elsewhere in the file (or from similar examples in the codebase) that suggests a need to treatundefinedvalues as an error case.Therefore, the current behavior seems acceptable if the intended design is to only trigger updates when a valid value is provided.
Final Recommendation:
- The design choice to ignore
undefinedvalues appears deliberate.- No additional error handling seems necessary unless future requirements dictate stricter checking.
web/src/beta/features/PluginPlayground/hooks.tsx (1)
49-53: Verify the impact of fieldValues on code execution.The PR comments mention a race condition affecting state management. Let's verify the usage of fieldValues in the useCode hook.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for fieldValues usage in useCode hook ast-grep --pattern 'const useCode = ($params) => { $$$ fieldValues $$$ }'Length of output: 81
I'll generate additional scripts to search for the definition of the
useCodehook and all its references tofieldValuesin the codebase.
🏁 Script executed:
#!/bin/bash echo "Searching for the definition of the useCode hook:" rg -n "const useCode\\s*=" . echo "" echo "Searching for all occurrences of 'fieldValues' in the codebase:" rg -n "fieldValues" .Length of output: 1811
Subject: FieldValues Handling in useCode Hook is Appropriate
After re-examining the implementation across the PluginPlayground:
- The
fieldValuesstate is declared and managed inhooks.tsxand then passed to theuseCodehook.- In
web/src/beta/features/PluginPlayground/Code/hook.ts,fieldValuesis consistently used—most notably as part of a dependency array (e.g., on line 184) and when computing derived values—ensuring updated state is properly captured.These findings indicate that changes to
fieldValuestrigger the intended hook re-execution, mitigating the race condition concerns noted in the PR comments.
Overview
Adds a widget panel to plugin playground. Converts the widget schema groups found in
web/src/beta/features/PluginPlayground/Plugins/presets/custom/myPlugin.tsinto UI input fields to display in widget panel. This shows that the plugin developer can adjust the widget schema to display different UI components. Stores the input field data locally.What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Would need to confirm what other UI components we wish to display. Currently only text, number and boolean fields were added. The fields also store the values in local state. Will need to confirm if these values can be used after editing.
Memo
Summary by CodeRabbit
New Features
PropertyItem, for rendering various input fields based on schema properties.Refactor