chore: Add redesign for JS Editor Form #36948
Conversation
WalkthroughThe pull request introduces significant changes to the JavaScript editor components. A new interface Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
github-actions
bot
added
IDE Navigation
|
This PR has increased the number of cyclic dependencies by 6, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
app/client/src/pages/Editor/JSEditor/JSEditorForm/old/JSEditorForm.tsx (1)
37-105: Component implementation looks solid.The
OldJSEditorFormcomponent is well-structured and implements the required functionality as per the PR objectives. Good use of conditional rendering and proper configuration of child components.Consider extracting the
LazyCodeEditorconfiguration into a separate constant or useMemo hook to improve readability and potentially optimize performance.Example refactor:
const codeEditorConfig = useMemo(() => ({ AIAssisted: true, blockCompletions: props.blockCompletions, border: CodeEditorBorder.NONE, borderLess: true, // ... other props }), [props.blockCompletions, props.currentJSCollection.name, props.changePermitted, props.onChange, props.theme]); // In JSX <LazyCodeEditor {...codeEditorConfig} />app/client/src/components/editorComponents/CodeEditor/index.tsx (2)
Line range hint
50-203: Improved type safety for block completions.The introduction of the
BlockCompletiontype and its usage inEditorStylePropsenhances type safety. Good job on this improvement.Consider using
readonlyfor theblockCompletionsarray to prevent accidental modifications:- blockCompletions?: Array<BlockCompletion>; + blockCompletions?: readonly BlockCompletion[];
Remove Unused
blockCompletionsPropertyThe
blockCompletionsproperty is destructured frompropsbut not utilized within theCodeEditorcomponent. Consider removing it if it's unnecessary to eliminate redundant code.🔗 Analysis chain
Line range hint
204-1400: Verify usage ofblockCompletionsproperty.The
blockCompletionsproperty has been added toEditorStyleProps, but it's not utilized in the component logic. Is this intentional, or is there a plan to implement its usage in the future?🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of blockCompletions in the component rg "blockCompletions" app/client/src/components/editorComponents/CodeEditor/index.tsxLength of output: 280
app/client/src/pages/Editor/JSEditor/JSEditorForm/JSEditorForm.tsx (1)
64-65: Remove redundant 'borderLess' propSince
borderis set toCodeEditorBorder.NONE, theborderLessprop is redundant. Consider removing it to simplify the code.Apply this diff:
border={CodeEditorBorder.NONE} - borderLess
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- app/client/src/components/editorComponents/CodeEditor/EditorConfig.ts (1 hunks)
- app/client/src/components/editorComponents/CodeEditor/index.tsx (2 hunks)
- app/client/src/pages/Editor/JSEditor/Form.tsx (4 hunks)
- app/client/src/pages/Editor/JSEditor/JSEditorForm/JSEditorForm.tsx (1 hunks)
- app/client/src/pages/Editor/JSEditor/JSEditorForm/index.ts (1 hunks)
- app/client/src/pages/Editor/JSEditor/JSEditorForm/old/JSEditorForm.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/pages/Editor/JSEditor/JSEditorForm/index.ts
🧰 Additional context used
🔇 Additional comments (8)
app/client/src/pages/Editor/JSEditor/JSEditorForm/old/JSEditorForm.tsx (3)
1-20: Imports look good.The necessary components and types are imported correctly.
22-35: Props interface is well-defined.The
Propsinterface provides good type safety for the component. All necessary properties are included with appropriate types.
1-105: Implementation aligns well with PR objectives.The
OldJSEditorFormcomponent successfully implements the redesigned JS Editor Form as outlined in the PR objectives. It consolidates the functionality into a single component while maintaining backward compatibility. The tabbed interface and conditional rendering of settings allow for easy recovery and modifications.Good job on meeting the requirements and maintaining a clean, well-structured implementation.
app/client/src/components/editorComponents/CodeEditor/EditorConfig.ts (2)
57-60: LGTM: New BlockCompletion interface.The new
BlockCompletioninterface is well-defined and follows TypeScript best practices.
69-69: LGTM: Updated FieldEntityInformation interface.The change to use
Array<BlockCompletion>improves type safety and readability.app/client/src/pages/Editor/JSEditor/Form.tsx (3)
5-5: Import statement added correctly.
EditorThemeis correctly imported fromEditorConfig.
55-55: Type imported appropriately.
JSEditorTabtype is correctly imported for type annotations.
65-65: Component imported with alias.
JSEditorFormis imported and aliased asEditorFormappropriately.
| currentJSCollection: JSCollection; | ||
| changePermitted: boolean; | ||
| onChange: (valueOrEvent: React.ChangeEvent | string) => void; | ||
| theme: EditorTheme.LIGHT; |
There was a problem hiding this comment.
Correct the 'theme' prop type in 'Props' interface
The theme property in the Props interface is assigned a value EditorTheme.LIGHT instead of specifying its type. It should be typed as EditorTheme.
Apply this diff to fix the issue:
- theme: EditorTheme.LIGHT;
+ theme: EditorTheme;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| theme: EditorTheme.LIGHT; | |
| theme: EditorTheme; |
| onValueChange={(string) => | ||
| setSelectedConfigTab(string as JSEditorTab) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor onValueChange to improve clarity and type safety.
Avoid using string as a parameter name and unnecessary type assertions.
Consider updating the function as follows:
-onValueChange={(string) =>
- setSelectedConfigTab(string as JSEditorTab)
-}
+onValueChange={(tab: JSEditorTab) =>
+ setSelectedConfigTab(tab)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onValueChange={(string) => | |
| setSelectedConfigTab(string as JSEditorTab) | |
| } | |
| onValueChange={(tab: JSEditorTab) => | |
| setSelectedConfigTab(tab) | |
| } |
## Description Adds the new JS Editor Form behind feature flags. It ensures the old flow is still easy to recover and change by keeping most items the same. Fixes appsmithorg#36943 ## Automation /ok-to-test tags="@tag.JS" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11384887879> > Commit: c32b614 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11384887879&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JS` > Spec: > <hr>Thu, 17 Oct 2024 17:51:50 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new `BlockCompletion` interface for enhanced type safety. - Replaced the `TabbedViewContainer` with a consolidated `EditorForm` component for a streamlined editing experience. - Added a new `JSEditorForm` component for JavaScript code editing, with a fallback to an older version if needed. - **Bug Fixes** - Improved handling of configuration tab selection in the new editor interface. - **Documentation** - Updated export statements to ensure accessibility of the new components. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Adds the new JS Editor Form behind feature flags. It ensures the old flow is still easy to recover and change by keeping most items the same.
Fixes #36943
Automation
/ok-to-test tags="@tag.JS"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11384887879
Commit: c32b614
Cypress dashboard.
Tags:
@tag.JSSpec:
Thu, 17 Oct 2024 17:51:50 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
BlockCompletioninterface for enhanced type safety.TabbedViewContainerwith a consolidatedEditorFormcomponent for a streamlined editing experience.JSEditorFormcomponent for JavaScript code editing, with a fallback to an older version if needed.Bug Fixes
Documentation