feat(web): plugin playground/reflect code editor to viewer #1224
Conversation
WalkthroughThe changes introduce a new React functional component in Changes
Possibly related PRs
Suggested reviewers
Poem
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. |
…eearth/reearth-visualizer into playground/reflect-code-to-visual
devshun
changed the title
…eearth/reearth-visualizer into playground/reflect-code-to-visual
devshun
dismissed
mulengawilfred’s stale review
The base branch was changed.
…into playground/reflect-code-to-visual
…into playground/reflect-code-to-visual
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
web/src/beta/features/PluginPlayground/Plugins/constants.ts (1)
18-31: LGTM! Consider enhancing maintainability.The YAML structure is well-organized with clear separation of plugin metadata and extension configuration. However, consider these improvements:
- Use template literals with proper YAML indentation for better readability
- Add comments explaining the expected schema
- Consider a more descriptive widget description
Here's a suggested improvement:
sourceCode: `id: demo-widget name: Test plugin version: 1.0.0 +# Plugin extensions define the components and their configurations extensions: - id: demo-widget type: widget name: Demo Widget - description: Demo widget + description: A demo widget showcasing plugin reflection capabilities widgetLayout: defaultLocation: zone: outer section: left area: top `,web/src/beta/features/PluginPlayground/Code/index.tsx (2)
34-34: Consider adding execution feedback statesThe execute button could benefit from visual feedback during code execution. Consider:
- Adding a loading state while code is executing
- Providing visual feedback for successful/failed execution
- Disabling the button during execution
- <Button icon="playRight" iconButton onClick={executeCode} /> + <Button + icon={isExecuting ? "loading" : "playRight"} + iconButton + onClick={executeCode} + disabled={isExecuting} + />
37-41: Consider debouncing source code changesFor better performance, consider debouncing the
onChangeSourceCodecallback to prevent excessive updates when typing rapidly.+import { useCallback } from "react"; +import debounce from "lodash/debounce"; const Code: FC<Props> = ({ ... }) => { + const debouncedOnChange = useCallback( + debounce((value: string | undefined) => onChangeSourceCode(value), 300), + [onChangeSourceCode] + ); + return ( ... <CodeInput language={getLanguageByFileExtension(fileTitle)} value={sourceCode} - onChange={onChangeSourceCode} + onChange={debouncedOnChange} />web/src/beta/features/PluginPlayground/hooks.tsx (2)
97-103: Consider optimizing the source code update handlerWhile the implementation is functional, consider these improvements:
- The callback could be simplified
- Consider debouncing source code updates to prevent excessive updates during typing
Here's a suggested improvement:
- executeCode={executeCode} - onChangeSourceCode={(value) => { - updateFileSourceCode( - value ?? selectedFile.sourceCode, - selectedFile.id - ); - }} + executeCode={executeCode} + onChangeSourceCode={useCallback( + (value) => updateFileSourceCode(value ?? selectedFile.sourceCode, selectedFile.id), + [selectedFile.id, selectedFile.sourceCode, updateFileSourceCode] + )}Consider adding debouncing:
const debouncedUpdateSourceCode = useMemo( () => debounce(updateFileSourceCode, 300), [updateFileSourceCode] );
115-115: Consider consistent dependency orderingFor better maintainability, consider grouping related dependencies together.
- [selectedFile, handlePluginDownload, executeCode, updateFileSourceCode] + [selectedFile, executeCode, updateFileSourceCode, handlePluginDownload]web/src/beta/features/PluginPlayground/Plugins/hook.ts (2)
15-41: Consider improving the demo widget implementation.The demo widget implementation could be enhanced in several ways:
- Move the demo widget template to a separate constants file for better maintainability
- Consider using CSS-in-JS or a separate stylesheet instead of inline styles
- Clean up unnecessary whitespace in the template literal
Here's a suggested refactor:
+ // In constants.ts + export const DEMO_WIDGET_SOURCE = `reearth.ui.show(\` + <style> + body { margin: 0; } + #wrapper { + background: #232226; + height: 100%; + color: white; + border: 3px dotted red; + border-radius: 5px; + padding: 20px 0; + } + </style> + <div id="wrapper"> + <h2 style="text-align: center; margin: 0;">Hello World</h2> + </div> + \`);`; - sourceCode: `reearth.ui.show(\` - <style> - body { - margin: 0; - } - #wrapper { - background: #232226; - height: 100%; - color: white; - border: 3px dotted red; - border-radius: 5px; - padding: 20px 0; - } - </style> - <div id="wrapper"> - <h2 style="text-align: center; margin: 0;">Hello World</h2> - </div> - \`); - - ` + sourceCode: DEMO_WIDGET_SOURCE
131-147: Consider adding error handling for invalid file IDs.The implementation looks good and follows the established patterns in the codebase. However, consider adding error handling for cases where the file ID doesn't exist.
Here's a suggested enhancement:
const updateFileSourceCode = useCallback( (sourceCode: string, id: string) => { + const fileExists = selectedPlugin.files.some(file => file.id === id); + if (!fileExists) { + setNotification({ + type: "error", + text: "Cannot update source code: File not found" + }); + return; + } setPlugins((plugins) => plugins.map((plugin) => plugin.id === selectedPlugin.id ? { ...plugin, files: plugin.files.map((file) => file.id === id ? { ...file, sourceCode } : file ) } : plugin ) ); }, - [selectedPlugin] + [selectedPlugin, setNotification] );web/src/beta/features/PluginPlayground/Code/hook.ts (2)
51-51: Name the exported functional component for better debugging and clarity.Naming the exported functional component can improve stack traces and enhance code readability.
Apply this diff to name the component:
-export default ({ files }: Props) => { +export default function PluginExecutor({ files }: Props) {
93-95: Handle the case where the JavaScript file corresponding to the extension is not found.When the corresponding JavaScript file for an extension is not found, it might be helpful to notify the user or log a warning. This improves debuggability and user feedback.
Consider adding a notification:
if (!file) { + setNotification({ + type: "warning", + text: `JavaScript file for extension "${cur.id}" not found.`, + }); return prv; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
web/src/beta/features/PluginPlayground/Code/hook.ts(1 hunks)web/src/beta/features/PluginPlayground/Code/index.tsx(2 hunks)web/src/beta/features/PluginPlayground/Plugins/constants.ts(1 hunks)web/src/beta/features/PluginPlayground/Plugins/hook.ts(3 hunks)web/src/beta/features/PluginPlayground/Viewer/index.tsx(2 hunks)web/src/beta/features/PluginPlayground/hooks.tsx(4 hunks)
🧰 Additional context used
🪛 GitHub Check: ci-web / ci
web/src/beta/features/PluginPlayground/Code/hook.ts
[failure] 60-60:
'outputs' is assigned a value but never used. Allowed unused vars must match /^_/u
🪛 Biome
web/src/beta/features/PluginPlayground/Code/hook.ts
[error] 108-108: 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)
🔇 Additional comments (9)
web/src/beta/features/PluginPlayground/Viewer/index.tsx (3)
6-8: LGTM! Well-structured type definition.
The Props type is well-defined using ComponentProps utility type, ensuring type safety by properly extracting the widgets type from the Visualizer component.
10-10: LGTM! Component signature follows React best practices.
The functional component signature is properly typed with FC and correctly destructures the widgets prop.
29-29: Verify widget integration with Visualizer component.
The widgets prop is correctly passed to the Visualizer component. However, let's verify the widget integration implementation.
web/src/beta/features/PluginPlayground/Plugins/constants.ts (1)
26-30: Verify widget layout configuration compatibility.
The widget layout configuration specifies positioning in the outer zone's left section. Let's verify this is a valid configuration and that all specified zones/sections are supported.
✅ Verification successful
Widget layout configuration is compatible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for widget layout configuration patterns and documentation
# Check for layout configuration patterns
echo "Searching for other widget layout configurations..."
rg -A 3 'widgetLayout:\s*defaultLocation:'
# Check for zone/section/area definitions or documentation
echo "Searching for layout zone definitions..."
rg -A 5 'zone:\s*(outer|inner)'
# Check for any layout-related type definitions
echo "Searching for layout type definitions..."
ast-grep --pattern 'type $_ = {
$$$
zone?: $_
$$$
}'
Length of output: 5227
Script:
#!/bin/bash
# Description: Verify sections and areas in widget layout configurations
# Check for section definitions and usages
echo "Searching for section definitions..."
rg -A 3 'section:\s*(left|right|center)'
# Check for area definitions and usages
echo "Searching for area definitions..."
rg -A 3 'area:\s*(top|bottom)'
Length of output: 2838
web/src/beta/features/PluginPlayground/Code/index.tsx (2)
25-30: LGTM! Clean component signature
The component signature follows React best practices with proper TypeScript typing and clean props destructuring.
8-9: Verify handling of undefined sourceCode values
The onChangeSourceCode callback accepts undefined as a valid value. Ensure that the parent component properly handles this case to prevent potential runtime issues.
web/src/beta/features/PluginPlayground/hooks.tsx (2)
21-21: LGTM!
The addition of updateFileSourceCode aligns well with the PR's objective of implementing plugin reflection.
38-38: LGTM!
The Viewer component props and dependency array are correctly updated to handle the widgets prop.
Also applies to: 41-41
web/src/beta/features/PluginPlayground/Plugins/hook.ts (1)
239-239: LGTM!
The new function is properly exposed in the return object, maintaining consistency with the existing pattern.
There was a problem hiding this comment.
Looks good. Added a few issues identified by code-rabbit. Kindly take a look at those potential issues and see which ones need to be resolved.
Also if we can, lets add a to yml in variable names. e.g ReearthYML -> ReearthYAML , getYmlResult -> getYamlResult, ymlJson -> yamlJson.
The minor extra verbosity can bring more clarity to the reader.
The eslint ci-build seems to fail for this but mention me or Shaun to investigate if the fix is not obvious as we have quite a number of issues there.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
web/src/beta/features/PluginPlayground/Code/hook.ts(1 hunks)
🧰 Additional context used
🪛 Biome
web/src/beta/features/PluginPlayground/Code/hook.ts
[error] 96-96: 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)
…into playground/reflect-code-to-visual
Co-authored-by: lby <icesunex@hotmail.com>
Overview
What I've done
I implemented the process to execute reearth.yml and other JS files and reflect the plugins in the visualizer.
2024-11-07.22.12.10.mov
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
Codecomponent for improved interaction with source code changes and execution capabilities.Improvements
Viewercomponent.Bug Fixes