chore: Editable name refactor #37069
Conversation
WalkthroughThe changes introduce a new Changes
Assessment against linked issues
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)
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
the
skip-changelog
github-actions
bot
added
IDE Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (12)
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Rename.tsx (1)
4-6: Consider adding JSDoc documentation for the Props interface.Adding documentation would help other developers understand the purpose of the
disabledprop and when to use it.+/** + * Props for the Rename menu item component + * @property {boolean} disabled - When true, the menu item will be disabled + */ interface Props { disabled?: boolean; }app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Delete.tsx (1)
14-14: Consider adding a default value for the disabled prop.While the optional prop works, setting a default value would make the component's behavior more explicit.
-export const Delete = ({ disabled }: Props) => { +export const Delete = ({ disabled = false }: Props) => {app/client/src/IDE/Components/EditableName/useNameEditor.ts (1)
Line range hint
28-35: Consider enhancing name validation.The validation could be more robust by adding checks for:
- Maximum length
- Leading/trailing spaces
Here's a suggested enhancement:
const validateName = useEventCallback((name: string): string | null => { - if (!name || name.trim().length === 0) { + const trimmedName = name.trim(); + if (!name || trimmedName.length === 0) { return createMessage(ACTION_INVALID_NAME_ERROR); + } else if (trimmedName.length > 64) { + return createMessage(ACTION_INVALID_NAME_ERROR); + } else if (trimmedName !== name) { + return createMessage(ACTION_INVALID_NAME_ERROR); } else if (name !== entityName && !isNameValid(name, usedEntityNames)) { return createMessage(nameErrorMessage, name); }app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx (1)
34-41: Consider grouping enabled actions together for better UX.While the implementation is correct, consider grouping enabled actions together and disabled actions at the end of the menu for better user experience. This is a common pattern in menu design that makes it easier for users to find available actions.
Consider reordering the menu items like this:
return ( <> - <Rename disabled={!isChangePermitted} /> <ConvertToModuleCTA /> + <MenuSeparator /> + <Docs /> + <MenuSeparator /> <Copy disabled={!isChangePermitted} /> <Move disabled={!isChangePermitted} /> - <MenuSeparator /> - <Docs /> - <MenuSeparator /> + <Rename disabled={!isChangePermitted} /> <Delete disabled={!isDeletePermitted} /> </> );app/client/src/IDE/Components/FileTab/FileTab.test.tsx (2)
Line range hint
5-14: Consider adding test cases for inactive state.The current setup only tests the active state (
isActive={true}). Consider adding test cases for inactive tabs to ensure proper behavior in all states.
Line range hint
40-51: Consider adding visual state assertions.While the click handlers are tested, consider adding assertions for visual states (e.g., active/inactive styling) to ensure the UI feedback is correct.
expect(tabElement).toHaveClass('active'); // or similar class nameapp/client/src/PluginActionEditor/components/PluginActionNameEditor.tsx (1)
91-99: Consider adding aria-label for accessibilityThe EditableName implementation should include appropriate aria labels for better accessibility.
<EditableName exitEditing={exitEditMode} icon={<IconContainer>{icon}</IconContainer>} isEditing={isEditing} isLoading={isLoading} name={action.name} onNameSave={handleNameSave} + aria-label={`Edit ${action.name}`} />app/client/src/IDE/index.ts (1)
68-72: Well-placed in the UI Components section.The
EditableNamecomponent's placement in the UI Components section aligns with its reusable nature, making it easily discoverable for use across Tabs and Toolbars.app/client/src/IDE/Components/EditableName/EditableName.tsx (1)
7-15: Add prop types documentation and validation.Consider adding JSDoc comments for the interface and adding validation for the
nameprop to handle empty strings gracefully.+/** + * Props for the EditableName component + * @property {string} name - The current name value (must not be empty) + * @property {boolean} [isLoading] - Whether the component is in loading state + * @property {(name: string) => void} onNameSave - Callback when name is saved + * @property {boolean} isEditing - Whether the component is in edit mode + * @property {() => void} exitEditing - Callback to exit edit mode + * @property {React.ReactNode} icon - Icon to display when not loading + * @property {string} [inputTestId] - Test ID for the input element + */ interface EditableTextProps { name: string; isLoading?: boolean; onNameSave: (name: string) => void; isEditing: boolean; exitEditing: () => void; icon: React.ReactNode; inputTestId?: string; }app/client/src/IDE/Components/EditableName/EditableName.test.tsx (3)
1-8: Consider adding type imports for better type safety.The imports look good, but for TypeScript files, it's recommended to import types for test utilities.
Add these type imports:
+import { RenderResult } from "@testing-library/react"; +import { UserEvent } from "@testing-library/user-event";
13-38: Enhance setup function type safety and return type.The setup function implementation is good, but could benefit from explicit TypeScript types.
Consider adding types:
-const setup = ({ isEditing = false, isLoading = false }) => { +interface SetupProps { + isEditing?: boolean; + isLoading?: boolean; +} + +const setup = ({ isEditing = false, isLoading = false }: SetupProps): RenderResult & { + name: string; + icon: JSX.Element; + onNameSave: jest.Mock; + exitEditing: jest.Mock; + isLoading: boolean; +} => {
60-131: Consider adding loading state tests.While the valid input tests are comprehensive, they don't cover the loading state behavior mentioned in the setup props.
Add a test for loading state:
test("disables input when loading", () => { const { getByRole } = setup({ isEditing: true, isLoading: true }); const input = getByRole("textbox"); expect(input).toBeDisabled(); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
- app/client/src/IDE/Components/EditableName/EditableName.test.tsx (1 hunks)
- app/client/src/IDE/Components/EditableName/EditableName.tsx (1 hunks)
- app/client/src/IDE/Components/EditableName/index.ts (1 hunks)
- app/client/src/IDE/Components/EditableName/useNameEditor.ts (1 hunks)
- app/client/src/IDE/Components/FileTab/FileTab.test.tsx (3 hunks)
- app/client/src/IDE/Components/FileTab/FileTab.tsx (1 hunks)
- app/client/src/IDE/index.ts (1 hunks)
- app/client/src/PluginActionEditor/components/PluginActionNameEditor.tsx (5 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Copy.tsx (2 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Delete.tsx (2 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Move.tsx (2 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Rename.tsx (1 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx (3 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/AddTab.tsx (1 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/EditableTab.tsx (2 hunks)
- app/client/src/pages/Editor/JSEditor/JSEditorToolbar/JSObjectNameEditor/JSObjectNameEditor.tsx (6 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/IDE/Components/EditableName/index.ts
🧰 Additional context used
📓 Learnings (1)
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/JSObjectNameEditor/JSObjectNameEditor.tsx (2)
Learnt from: ankitakinger PR: appsmithorg/appsmith#36560 File: app/client/src/pages/Editor/JSEditor/JSObjectNameEditor.tsx:28-28 Timestamp: 2024-09-26T06:52:44.158Z Learning: When type definitions like `SaveActionNameParams` are declared in multiple places, consolidating them by removing duplicates and reusing a single definition maintains consistency in the codebase.Learnt from: ankitakinger PR: appsmithorg/appsmith#36560 File: app/client/src/pages/Editor/JSEditor/JSObjectNameEditor.tsx:28-28 Timestamp: 2024-10-08T15:32:34.115Z Learning: When type definitions like `SaveActionNameParams` are declared in multiple places, consolidating them by removing duplicates and reusing a single definition maintains consistency in the codebase.
🔇 Additional comments (38)
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Rename.tsx (1)
1-2: LGTM! Imports are clean and necessary.app/client/src/pages/Editor/IDE/EditorTabs/AddTab.tsx (2)
28-29: LGTM! Clean extraction of the template string.The extraction of the template string to a variable improves code maintainability and reusability.
35-38: Verify FileTab component's children prop usage.The changes align with the EditableName refactoring. Let's verify that the FileTab component properly handles the new children prop structure.
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Delete.tsx (2)
10-12: LGTM! Clean Props interface definition.The interface follows React and TypeScript best practices with proper optional prop definition.
33-33: LGTM! Proper implementation of the disabled state.The disabled prop is correctly passed to MenuItem, enhancing the component's configurability.
app/client/src/IDE/Components/EditableName/useNameEditor.ts (1)
Line range hint
17-42: LGTM! Clean and efficient implementation.Good use of memoization with useEventCallback and selector optimization with shallowEqual.
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx (2)
2-2: LGTM: Import changes are clean and properly organized.Also applies to: 17-17
Line range hint
24-30: LGTM: Permission handling is well-structured.The separation of manage and delete permissions is clean and reusable.
Let's verify the consistency of permission handling across related components:
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Copy.tsx (3)
10-12: LGTM: Props interface follows TypeScript best practices.The optional disabled prop is well-defined using the correct TypeScript syntax.
14-14: LGTM: Component signature properly destructures the props.The component correctly implements the new Props interface.
33-33: Verify disabled state handling in parent components.The disabled prop is correctly passed to MenuSubTrigger, but we should ensure consistent disabled state handling across all toolbar menu components.
✅ Verification successful
Disabled state handling is consistent across toolbar menu components
All menu components (Copy, Move, Delete, Rename) consistently:
- Define the same Props interface with optional
disabledprop- Pass the disabled prop to MenuSubTrigger
- Receive disabled state from parent ToolbarMenu based on appropriate permissions
The implementation in Copy.tsx follows the established pattern correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other toolbar menu components to verify consistent disabled prop usage rg -l "MenuSubTrigger.*disabled" "app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/"Length of output: 281
Script:
#!/bin/bash # Check the implementation of disabled prop in both files rg "MenuSubTrigger.*disabled" "app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Move.tsx" -B 2 -A 2 rg "MenuSubTrigger.*disabled" "app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Copy.tsx" -B 2 -A 2 # Check if there are other menu components that should handle disabled state fd . "app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/" -e tsx --exec rg "disabled.*prop|interface.*disabled" {} -lLength of output: 709
Script:
#!/bin/bash # Check the interface/props definition for these components rg "interface.*Props" "app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Move.tsx" -A 5 rg "interface.*Props" "app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Copy.tsx" -A 5 # Check for any other menu components in the directory fd . "app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/" -e tsx -x echo "{}"Length of output: 1120
Script:
#!/bin/bash # Check disabled prop handling in other menu components rg "interface.*Props" "app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Delete.tsx" -A 5 rg "interface.*Props" "app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Rename.tsx" -A 5 # Check the parent ToolbarMenu component for disabled prop handling rg "disabled" "app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx" -A 2 -B 2Length of output: 905
app/client/src/IDE/Components/FileTab/FileTab.test.tsx (1)
19-38: Verify FileTab interface completeness.Let's ensure we're testing all required props of the FileTab component.
✅ Verification successful
All required props are covered in the test setup
The test setup includes all required props from the FileTabProps interface:
isActive(required boolean)title(required string)onClick(required callback)onClose(required event handler)children(required ReactNode)onDoubleClick(optional callback)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find FileTab component interface definition ast-grep --pattern 'interface FileTabProps { $$$ }'Length of output: 675
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Move.tsx (3)
16-18: LGTM! Clean interface declaration.
20-20: LGTM! Clean component signature update.
45-47: Verify disabled state behavior across menu structure.While the disabled state is correctly applied to the trigger, consider whether the menu content should also reflect this state for consistency.
app/client/src/pages/Editor/IDE/EditorTabs/EditableTab.tsx (3)
1-18: LGTM! Clean refactoring of imports and interface.The simplified
EditableTabPropsinterface with explicit props improves type safety and readability.Also applies to: 20-27
Line range hint
29-39: LGTM! Well-structured component with proper permission handling.The component correctly implements feature flag checks and type safety.
65-83: LGTM! Clean implementation of EditableName integration.The render logic effectively composes the FileTab with EditableName, following component composition best practices.
app/client/src/PluginActionEditor/components/PluginActionNameEditor.tsx (4)
Line range hint
1-15: LGTM! Clean import optimizationThe imports have been effectively streamlined to remove unused dependencies while maintaining clear type definitions.
Line range hint
58-81: LGTM! Clean hooks implementationThe component effectively uses hooks and properly handles permissions and feature flags.
Line range hint
38-46: Verify icon dimensions for accessibilityThe fixed dimensions (12x12px) for icons might be too small for accessibility standards. Consider verifying against WCAG guidelines.
83-87: Verify error handling implementationThe handleNameSave callback doesn't appear to handle potential errors from the saveActionName action.
app/client/src/IDE/index.ts (1)
68-72: LGTM! Well-documented export addition.The export and its documentation align well with the existing pattern and clearly describe the component's purpose.
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/JSObjectNameEditor/JSObjectNameEditor.tsx (5)
Line range hint
1-19: LGTM! Clean import refactoringThe import statements have been properly cleaned up, removing unused hooks and adding necessary dependencies for the new implementation.
Line range hint
33-62: LGTM! Well-structured styled componentsGood use of design system tokens and proper styling organization.
Line range hint
64-143: LGTM for overall implementationThe component has been successfully refactored to use the new EditableName component, with proper feature flag handling and fallback to the old implementation.
Line range hint
21-31: Verify SaveActionNameParams type definition uniquenessThe interface looks good, but based on previous learnings, we should verify it's not duplicated elsewhere in the codebase.
#!/bin/bash # Search for duplicate declarations of SaveActionNameParams rg -A 2 "interface SaveActionNameParams"
136-143: 🛠️ Refactor suggestionConsider adding name validation
The EditableName component receives the name directly without validation. Consider adding validation before saving to prevent empty or invalid names.
If EditableName doesn't have built-in validation, consider adding a validate prop:
const validateName = (name: string) => { if (!name.trim()) return "Name cannot be empty"; return ""; }; <EditableName ... validate={validateName} />app/client/src/IDE/Components/EditableName/EditableName.tsx (4)
17-30: LGTM! Clean state management implementation.The component's state management is well-organized with clear separation of concerns.
117-132: LGTM! Clean and efficient render implementation.The render logic is well-structured with appropriate conditional rendering and error handling.
73-90: 🛠️ Refactor suggestionSimplify focusout handler using the extracted save function.
The focusout handler can be simplified using the same extracted save function suggested earlier.
useEventListener( "focusout", function handleFocusOut() { if (isEditing) { - const nameError = validateName(editableName); - exitEditing(); - if (nameError === null) { - onNameSave(editableName); - } else { + if (!handleNameSave(editableName)) { setEditableName(name); setValidationError(null); } } }, inputRef, );Likely invalid or redundant comment.
101-115: Track the focus retention fix in a separate issue.The setTimeout hack for focus retention should be properly tracked and addressed.
Let's check if there's already an issue for this:
Would you like me to create a new issue to track the focus retention fix?
app/client/src/IDE/Components/EditableName/EditableName.test.tsx (2)
40-58: LGTM! Basic rendering tests are well-structured.The basic rendering tests cover both non-editing and editing states effectively.
1-199: Verify test coverage for EditableName component.Let's ensure we have complete coverage of the EditableName component.
✅ Verification successful
Test coverage is comprehensive and well-structured
The test suite adequately covers all props and functionality of the EditableName component:
- Basic rendering (default and editing states)
- All input events (Enter, Escape, focus out, click outside)
- Loading state
- Validation scenarios (valid and invalid inputs)
- Error handling and tooltips
- Name synchronization with prop changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if all props and event handlers of EditableName are tested # Look for the EditableName component definition ast-grep --pattern 'interface EditableNameProps { $$$ }' # Search for any untested props rg -l "EditableName.tsx" | xargs catLength of output: 97
Script:
#!/bin/bash # Let's try a different approach to find the EditableName component and its props # Find the EditableName component file fd "EditableName.tsx" --type f --exec cat {} # Search for prop usages in the test file fd "EditableName.test.tsx" --type f --exec cat {}Length of output: 11030
app/client/src/IDE/Components/FileTab/FileTab.tsx (4)
16-17: Addition ofchildrenandonDoubleClickprops is appropriate.The new props enhance the component's flexibility and functionality.
21-25: Destructured props updated correctly.Including
childrenandonDoubleClickin the component parameters is accurate.
33-33: Event handleronDoubleClickis properly assigned.Passing
onDoubleClickto<Styled.Tab>enables the desired double-click functionality.
35-35: Renderingchildrenwithin the tab.Inserting
{children}allows for customizable tab content inside the tab component.
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/Rename.tsx
Show resolved
Hide resolved
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/JSObjectNameEditor/JSObjectNameEditor.tsx
Show resolved
Hide resolved
app/client/src/IDE/Components/EditableName/EditableName.test.tsx
Outdated
Show resolved
Hide resolved
## Description Refactors the ADS Text edit capabilities into a single Editable Name component for use of entity name edit. This is currently used in Tabs and Toolbars. Fixes appsmithorg#37086 ## Automation /ok-to-test tags="@tag.IDE" ### 🔍 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/11513426772> > Commit: 86ec8d3 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11513426772&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.IDE` > Spec: > <hr>Fri, 25 Oct 2024 07:33:47 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced the `EditableName` component for editing names with validation and loading states. - Added a `Rename` component to the toolbar for renaming tasks. - Enhanced `ToolbarMenu` to include `Copy`, `Move`, and `Delete` components with configurable disabled states. - **Improvements** - Streamlined the `PluginActionNameEditor` and `JSObjectNameEditor` components to utilize the new `EditableName` component for editing functionality. - Simplified the `FileTab` and `EditableTab` components, focusing on static content rendering. - Updated the `AddTab` component to dynamically render titles based on state. - **Bug Fixes** - Improved handling of user interactions and validation errors in the `EditableName` component. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Refactors the ADS Text edit capabilities into a single Editable Name component for use of entity name edit. This is currently used in Tabs and Toolbars.
Fixes #37086
Automation
/ok-to-test tags="@tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11513426772
Commit: 86ec8d3
Cypress dashboard.
Tags:
@tag.IDESpec:
Fri, 25 Oct 2024 07:33:47 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
EditableNamecomponent for editing names with validation and loading states.Renamecomponent to the toolbar for renaming tasks.ToolbarMenuto includeCopy,Move, andDeletecomponents with configurable disabled states.Improvements
PluginActionNameEditorandJSObjectNameEditorcomponents to utilize the newEditableNamecomponent for editing functionality.FileTabandEditableTabcomponents, focusing on static content rendering.AddTabcomponent to dynamically render titles based on state.Bug Fixes
EditableNamecomponent.