-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Add Rename context menu #37116
Conversation
# Conflicts: # app/client/src/IDE/Components/EditableName/EditableName.test.tsx # app/client/src/IDE/Components/EditableName/EditableName.tsx # app/client/src/IDE/Components/EditableName/index.ts # app/client/src/IDE/index.ts # app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx # app/client/src/pages/Editor/IDE/EditorTabs/EditableTab.tsx
WalkthroughThe pull request introduces several enhancements to the IDE, including the addition of a new icon, 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
|
/build-deploy-preview |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11551218267. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
app/client/src/IDE/Components/EditableName/useIsRenaming.ts (1)
34-35
: Add return type interface.Consider adding an explicit interface for the return type to improve type safety and documentation.
interface UseIsRenamingReturn { isEditing: boolean; forcedEdit: boolean; enterEditMode: () => void; exitEditMode: () => void; }app/client/src/actions/ideActions.ts (1)
65-70
: Consider adding type safety and documentation.The action creator follows Redux patterns but could benefit from:
- A more specific type for the id parameter (e.g., EntityId)
- JSDoc documentation explaining the purpose and usage
Consider this enhancement:
+ /** + * Creates an action to initiate entity renaming mode + * @param id - The unique identifier of the entity to rename + */ - export const setRenameEntity = (id: string) => { + export const setRenameEntity = (id: EntityId) => { return { type: ReduxActionTypes.SET_RENAME_ENTITY, payload: id, }; };app/client/src/selectors/ideSelectors.tsx (1)
70-73
: Consider adding type safety improvements.While the implementation is correct, we can enhance type safety.
Consider this improvement:
-export const getIsRenaming = (id: string) => +export const getIsRenaming = (id: string): ((state: AppState) => boolean) => createSelector(getRenameEntity, (entityId) => { return entityId === id; });app/client/src/pages/Editor/IDE/EditorTabs/EditableTab.tsx (1)
54-55
: Consider adding type annotation for dispatchThe Redux integration looks good, but consider adding type safety:
- const dispatch = useDispatch(); + const dispatch = useDispatch<AppDispatch>();Also applies to: 57-61
app/client/src/reducers/uiReducers/ideReducer.ts (1)
129-129
: Consider adding JSDoc documentationAdd documentation to explain the purpose and lifecycle of this state property.
+ /** ID of the entity currently being renamed, empty string when not renaming */ renameEntity: string;
app/client/src/IDE/Components/EditableName/EditableName.test.tsx (1)
216-251
: Consider adding validation tests for force edit modeThe force edit test suite covers the basic focus behavior well. Consider adding tests for:
- Validation errors during force edit
- Save behavior with invalid input while in force edit mode
Example test case:
test("shows validation error in force edit mode", () => { const { getByRole } = setup({ isEditing: true, forceEdit: true }); const input = getByRole("textbox"); fireEvent.change(input, { target: { value: "" } }); expect(getByRole("tooltip")).toHaveTextContent("Please enter a valid name"); expect(document.activeElement).toBe(input); // Should maintain focus });app/client/src/components/editorComponents/EditableText.tsx (2)
Line range hint
106-115
: Consider adding transition for smoother hover effects.The hover effects look good, but could benefit from a smooth transition.
&&& .${Classes.EDITABLE_TEXT_CONTENT} { + transition: all 0.2s ease; &:hover { text-decoration: ${(props) => (props.minimal ? "underline" : "none")}; text-decoration-color: var(--ads-v2-color-border); } }
Line range hint
64-115
: Enhance keyboard accessibility.Consider adding keyboard focus styles and ARIA attributes for better accessibility:
const EditableTextWrapper = styled.div<{ isEditing: boolean; minimal: boolean; useFullWidth: boolean; }>` && { display: flex; flex-direction: column; justify-content: flex-start; align-items: flex-start; width: 100%; + &:focus-within { + outline: 2px solid var(--ads-v2-color-border); + outline-offset: 2px; + }app/client/src/IDE/Components/EditableName/EditableName.tsx (2)
12-22
: Improve JSDoc comments for new propsEnsure the documentation comments for the new properties are clear and follow consistent JSDoc formatting.
Apply this diff to enhance the comments:
- /** Used in conjunction with exit editing to control - * this component input editable state */ + /** + * Controls whether the input is in editing mode. + */ isEditing: boolean; - /** When this is true, the exit out - * of edit mode will be blocked till the - * user has a keyboard interaction **/ + /** + * If true, prevents exiting edit mode until the user interacts with the input. + */ needsInteractionBeforeExit?: boolean; - /** Used in conjunction with exit editing to control this component - * input editable state This function will be called when the - * user is trying to exit the editing mode. This can be - * restricted by the needsInteractionBeforeExit prop **/ + /** + * Callback function invoked when attempting to exit editing mode. + * May be restricted by `needsInteractionBeforeExit`. + */ exitEditing: () => void;
143-144
: Define timeout duration as a constantAvoid hardcoding the timeout value in
setTimeout
. Declare it as a constant for better maintainability.Apply this diff:
+ const REFOCUS_TIMEOUT = 400; if (isEditing && input) { setTimeout(() => { input.focus(); - }, 400); + }, REFOCUS_TIMEOUT); } else { setHasInteracted(false); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/client/packages/design-system/ads/src/__assets__/icons/ads/input-cursor-move.svg
is excluded by!**/*.svg
📒 Files selected for processing (14)
- app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx (2 hunks)
- app/client/src/IDE/Components/EditableName/EditableName.test.tsx (3 hunks)
- app/client/src/IDE/Components/EditableName/EditableName.tsx (5 hunks)
- app/client/src/IDE/Components/EditableName/RenameMenuItem.tsx (1 hunks)
- app/client/src/IDE/Components/EditableName/index.ts (1 hunks)
- app/client/src/IDE/Components/EditableName/useIsRenaming.ts (1 hunks)
- app/client/src/IDE/index.ts (1 hunks)
- app/client/src/actions/ideActions.ts (1 hunks)
- app/client/src/ce/constants/ReduxActionConstants.tsx (1 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx (2 hunks)
- app/client/src/components/editorComponents/EditableText.tsx (4 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/EditableTab.tsx (5 hunks)
- app/client/src/reducers/uiReducers/ideReducer.ts (2 hunks)
- app/client/src/selectors/ideSelectors.tsx (1 hunks)
🔇 Additional comments (23)
app/client/src/IDE/Components/EditableName/index.ts (1)
1-3
: LGTM! Well-organized barrel exports.The exports are logically structured, with the main component followed by related utilities. This organization aligns well with the rename context menu feature requirements.
Let's verify the exported entities are properly implemented:
✅ Verification successful
Exports verified and properly implemented
All three exports are correctly implemented as React components and hooks:
EditableName
: React component with props for exit handling and iconRenameMenuItem
: React component with disabled state and entity ID propsuseIsRenaming
: Custom hook for managing rename state with Redux integration🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of exported components and hooks # Check if the source files exist and contain the expected exports echo "Checking source files for exports..." fd -e tsx -e ts "EditableName|RenameMenuItem|useIsRenaming" app/client/src/IDE/Components/EditableName/ --exec rg -l "export (const|function|class) (EditableName|RenameMenuItem|useIsRenaming)"Length of output: 459
Script:
#!/bin/bash # Let's verify the actual implementation of these exports echo "Checking EditableName implementation..." rg "export (const|function|class) EditableName" -A 2 app/client/src/IDE/Components/EditableName/EditableName.tsx echo -e "\nChecking RenameMenuItem implementation..." rg "export (const|function|class) RenameMenuItem" -A 2 app/client/src/IDE/Components/EditableName/RenameMenuItem.tsx echo -e "\nChecking useIsRenaming implementation..." rg "export (const|function|class) useIsRenaming" -A 2 app/client/src/IDE/Components/EditableName/useIsRenaming.tsLength of output: 924
app/client/src/IDE/Components/EditableName/RenameMenuItem.tsx (4)
1-9
: LGTM! Clean imports and well-defined interface.The Props interface and imports are minimal, specific, and follow TypeScript best practices.
11-13
: LGTM! Component follows React best practices.Clean component definition with proper Redux hook usage.
14-16
: LGTM! Properly memoized callback with correct dependencies.The callback is optimized using useCallback with the appropriate dependency array.
18-26
: Consider enhancing accessibility and verifying the icon.While the MenuItem implementation is clean, consider adding:
- An aria-label for better screen reader support
- A keyboard shortcut indicator if applicable
Let's verify the icon exists in the design system:
✅ Verification successful
Let me search for the icon definition with corrected file type flags and expanded search patterns to verify its existence in the codebase.
Let me check the icon definition and look for existing accessibility patterns in MenuItems to provide accurate recommendations.
Icon exists, but accessibility improvements recommended
Add an aria-label to improve screen reader support:
<MenuItem aria-label="Rename item" disabled={disabled} onSelect={setRename} startIcon="input-cursor-move" > Rename </MenuItem>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for input-cursor-move icon definition rg -l "input-cursor-move" --type ts --type tsxLength of output: 74
Script:
#!/bin/bash # Search for input-cursor-move icon definition with correct file types rg -t typescript -t javascriptreact -t typescriptreact "input-cursor-move" # Also search in potential icon definition files fd -e ts -e tsx -e js -e jsx "icon" -X rg -l "input-cursor-move" # Search specifically in design system or icon related paths fd -e ts -e tsx -e js -e jsx . | grep -i "icon\|design" | xargs rg -l "input-cursor-move"Length of output: 394
Script:
#!/bin/bash # Let's verify the icon definition in the provider file cat app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx # Also check if there are any existing MenuItems with aria-labels for reference rg -t typescript -t javascriptreact -t typescriptreact "MenuItem.*aria-label" -A 2Length of output: 48918
app/client/src/IDE/Components/EditableName/useIsRenaming.ts (3)
6-35
: Implementation aligns well with PR objectives.The hook provides a clean and reusable way to manage rename state, which is essential for the new Rename context menu feature.
7-9
: 🛠️ Refactor suggestionConsider memoizing dispatch.
For consistency with React hooks best practices, consider memoizing the dispatch to prevent unnecessary re-renders.
-const dispatch = useDispatch(); +const dispatch = useCallback(useDispatch(), []);Likely invalid or redundant comment.
28-32
: Verify the usage ofid
inexitEditMode
.The
id
parameter is included in the dependency array but not used in the callback. This might lead to inconsistent behavior.✅ Verification successful
Based on the search results, I can see that
setRenameEntity
action creator accepts anid
parameter and is used in two places:
- In
RenameMenuItem.tsx
, it's called withentityId
to start renaming- In
useIsRenaming.ts
, it's called with an empty string to clear the renaming stateLet me verify if this pattern is consistent by checking the reducer and any other related components.
The
id
dependency inexitEditMode
is correct, but the implementation is validThe current implementation of
exitEditMode
correctly passes an empty string to clear the rename state, which is consistent with the Redux action and reducer implementation. Theid
dependency is justified as the callback is recreated when the component'sid
changes, maintaining proper React hooks usage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of setRenameEntity to verify if id should be included rg "setRenameEntity" -A 2Length of output: 1307
Script:
#!/bin/bash # Search for the reducer handling SET_RENAME_ENTITY rg "SET_RENAME_ENTITY" -A 5Length of output: 1156
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx (1)
17-17
: LGTM: Clean import statementThe import follows the project's conventions and properly utilizes module aliasing.
app/client/src/actions/ideActions.ts (1)
65-70
: Verify action type constant definition.The action uses
ReduxActionTypes.SET_RENAME_ENTITY
. Let's verify this constant is properly defined.✅ Verification successful
Let me search for any occurrence of this constant to verify its definition and usage. The previous search might have been too specific with the
=
pattern. This broader search should help us locate the constant definition.
Action type constant is properly defined and used correctly
The constant
SET_RENAME_ENTITY
is defined inapp/client/src/ce/constants/ReduxActionConstants.tsx
and is being used correctly in both the action creator and reducer:
- ✓ Defined in ReduxActionConstants
- ✓ Used in ideReducer.ts
- ✓ Used in ideActions.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SET_RENAME_ENTITY constant definition rg "SET_RENAME_ENTITY.*=" -A 1Length of output: 32
Script:
#!/bin/bash # Search for SET_RENAME_ENTITY in all files, including potential enum or const definitions rg "SET_RENAME_ENTITY" -A 2 # Also search specifically in redux related files fd "redux|actions" -t f -e ts -e js --exec rg "SET_RENAME_ENTITY" {} -A 2Length of output: 785
app/client/src/selectors/ideSelectors.tsx (1)
68-68
: LGTM: Clean and consistent selector implementation.The selector follows the established pattern and maintains type safety.
app/client/src/pages/Editor/IDE/EditorTabs/EditableTab.tsx (3)
7-8
: LGTM: Import changes align with new functionalityThe new imports support the Redux integration and renaming functionality.
Also applies to: 17-17
40-41
: LGTM: Improved state management with useIsRenamingThe switch to useIsRenaming provides better semantics and additional states needed for the rename functionality.
78-78
: Verify EditableName handles needsInteractionBeforeExit correctlyPlease ensure that the EditableName component properly implements the interaction behavior for this new prop.
✅ Verification successful
EditableName component correctly implements needsInteractionBeforeExit prop
The implementation properly handles the needsInteractionBeforeExit prop in the focusout event handler:
- Tracks user interaction state with
hasInteracted
state- Re-focuses input when no interaction occurred and needsInteractionBeforeExit is true
- Only allows exit without saving after user interaction or when prop is false
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for EditableName component implementation and its prop types ast-grep --pattern 'interface $name { $$$ needsInteractionBeforeExit?: $_ $$$ }'Length of output: 88
Script:
#!/bin/bash # Search for EditableName component and its implementation ast-grep --pattern 'const EditableName = ($$$) => { $$$ }' # Also search for any type definitions related to EditableName ast-grep --pattern 'type EditableNameProps = { $$$ }' # Search for EditableName class if it's a class component ast-grep --pattern 'class EditableName extends $_ { $$$ }' # Backup search using ripgrep for any EditableName related code rg "EditableName" -A 10Length of output: 30167
app/client/src/IDE/index.ts (1)
72-76
: LGTM! Export changes align with rename functionality.The grouped exports maintain good organization by keeping related rename functionality together.
Let's verify the new exports are properly implemented:
✅ Verification successful
Exports are properly implemented and actively used in the codebase
Both
RenameMenuItem
anduseIsRenaming
are:
- Correctly implemented in their respective source files
- Properly exported through the IDE barrel file
- Actively used in:
- Editor tabs (
EditorTabs/EditableTab.tsx
)- Plugin action editor (
ToolbarMenu/ToolbarMenu.tsx
)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the new exports exist in their source files ast-grep --pattern 'export const RenameMenuItem' ast-grep --pattern 'export const useIsRenaming' # Verify these exports are being used in the codebase rg -l "RenameMenuItem|useIsRenaming" --type ts --type tsxLength of output: 4297
Script:
#!/bin/bash # Let's fix the file type and search for usages rg -l "RenameMenuItem|useIsRenaming" --type typescript --type-add 'typescript:*.{ts,tsx}' # Also check for imports of these components rg 'import.*\{.*(?:RenameMenuItem|useIsRenaming).*\}' --type typescript --type-add 'typescript:*.{ts,tsx}'Length of output: 793
app/client/src/reducers/uiReducers/ideReducer.ts (2)
18-18
: LGTM: Initial state properly configuredThe empty string is an appropriate default value for the renaming state.
114-121
: Verify state handling in dependent componentsLet's verify that components handle state transitions correctly.
Also applies to: 129-129
✅ Verification successful
State handling implementation is properly integrated
The renameEntity state is well-integrated across the codebase with clear patterns:
- Selector defined in
ideSelectors.tsx
for controlled access- Action creator in
ideActions.ts
for dispatching changes- Type-safe reducer implementation in
ideReducer.ts
- Used in E2E tests via Cypress commands
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for components that consume the renameEntity state echo "Components consuming renameEntity state:" rg -l "renameEntity" --type ts --type tsx # Search for potential race conditions in state updates echo "\nState update patterns:" ast-grep --pattern 'useSelector((state) => state.ide.renameEntity)'Length of output: 289
Script:
#!/bin/bash # Search for components that consume the renameEntity state with proper file extensions echo "Components consuming renameEntity state:" rg -l "renameEntity" --type typescript --type javascriptreact --type typescriptreact # Search for state usage patterns echo "\nState usage patterns:" rg "renameEntity" -A 2 -B 2 --type typescript --type javascriptreact --type typescriptreact # Search for dispatch patterns echo "\nDispatch patterns:" rg "SET_RENAME_ENTITY" -A 2 -B 2 --type typescript --type javascriptreact --type typescriptreactLength of output: 577
Script:
#!/bin/bash # Search for components that consume the renameEntity state echo "Components consuming renameEntity state:" rg -t js -t ts "renameEntity" # Search for state usage patterns with context echo "\nState usage patterns:" rg -t js -t ts "renameEntity" -A 2 -B 2 # Search for dispatch patterns with context echo "\nDispatch patterns:" rg -t js -t ts "SET_RENAME_ENTITY" -A 2 -B 2Length of output: 3670
app/client/src/IDE/Components/EditableName/EditableName.test.tsx (2)
20-24
: LGTM: Setup function changes look goodThe setup function is well-structured with appropriate defaults.
Also applies to: 30-30
98-98
: LGTM: Assertion change is correctThe negative assertion correctly verifies that onNameSave isn't called on click outside.
app/client/src/components/editorComponents/EditableText.tsx (2)
Line range hint
64-77
: LGTM: Enhanced styling for better visual feedback.The background color change during editing mode provides clear visual feedback to users, improving the UX.
Line range hint
1-267
: Implementation aligns well with PR objectives.The enhanced visual feedback and hover states support the rename functionality requirements effectively.
app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx (1)
1071-1073
: LGTM! Clean implementation of the new icon.The addition of
InputCursorMoveIcon
follows the established patterns for icon imports and mappings.Also applies to: 1370-1370
app/client/src/ce/constants/ReduxActionConstants.tsx (1)
503-503
: LGTM! The new action type aligns with the PR objectives.The addition of
SET_RENAME_ENTITY
constant follows the established naming convention and is appropriately placed in the IDEActionTypes object.Let's verify the integration with related files:
✅ Verification successful
The action type is properly integrated with corresponding action creator and reducer
The verification shows that:
- Action type is defined in ReduxActionConstants
- Action creator is implemented in ideActions.ts
- Reducer case is handled in ideReducer.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for corresponding action creators and reducers # Test: Search for action creators using this constant rg "SET_RENAME_ENTITY" --type ts --type tsx # Test: Search for reducer cases handling this action ast-grep --pattern 'case $action: { $$$ type: "SET_RENAME_ENTITY" $$$ }'Length of output: 152
Script:
#!/bin/bash # Search for SET_RENAME_ENTITY usage across TypeScript files rg "SET_RENAME_ENTITY" -t ts # Search for reducer handling this action type rg "case.*SET_RENAME_ENTITY" -t ts # Look for action creators or dispatches rg "dispatch.*SET_RENAME_ENTITY" -t ts # Check for any test files using this constant rg "SET_RENAME_ENTITY.*test" -t tsLength of output: 412
Script:
#!/bin/bash # Check the implementation in ideReducer rg -A 5 "SET_RENAME_ENTITY" app/client/src/reducers/uiReducers/ideReducer.ts # Check the action creator implementation rg -A 5 "SET_RENAME_ENTITY" app/client/src/actions/ideActions.tsLength of output: 328
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx
Show resolved
Hide resolved
Deploy-Preview-URL: https://ce-37116.dp.appsmith.com |
/build-deploy-preview |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11551516421. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
app/client/src/IDE/Components/EditableName/useIsRenaming.ts (2)
6-10
: Consider adding input validation for the id parameter.The hook should validate that the
id
parameter is not an empty string to prevent potential issues with the Redux selector.export const useIsRenaming = (id: string) => { + if (!id) { + throw new Error("id parameter is required"); + } const dispatch = useDispatch();
39-40
: Add explicit return type for better type safety.Consider adding an interface for the return type to improve type safety and documentation.
+interface RenameState { + isEditing: boolean; + forcedEdit: boolean; + enterEditMode: () => void; + exitEditMode: () => void; +} + export const useIsRenaming = (id: string) => { // ... existing code ... - return { isEditing, forcedEdit, enterEditMode, exitEditMode }; + return { isEditing, forcedEdit, enterEditMode, exitEditMode } satisfies RenameState;app/client/src/IDE/Components/EditableName/EditableName.tsx (4)
9-10
: Enhance JSDoc for isLoading propThe current JSDoc could be more descriptive about when and why the loading state is shown.
- /** isLoading true will show a spinner **/ + /** When true, displays a loading spinner instead of the icon. + * Use this to indicate background operations like save in progress. */
34-41
: LGTM with minor suggestionConsider grouping all default prop values together for better maintainability.
export const EditableName = ({ exitEditing, icon, inputTestId, isEditing, isLoading = false, name, needsInteractionBeforeExit = false, // Add default onNameSave, }: EditableTextProps) => {
105-107
: LGTM with optimization suggestionThe dependency array for
inputProps
includesinputTestId
which is static and could be moved outside the useMemo.const inputProps = useMemo( () => ({ - ["data-testid"]: inputTestId, onKeyUp: handleKeyUp, onChange: handleTitleChange, autoFocus: true, style: { paddingTop: 0, paddingBottom: 0, left: -1, top: -1 }, + ["data-testid"]: inputTestId, }), - [handleKeyUp, handleTitleChange, inputTestId], + [handleKeyUp, handleTitleChange], );
Line range hint
135-144
: Track the temporary focus retention fixThe TODO comment indicates this is a temporary solution. We should create an issue to track the proper implementation of focus retention.
Would you like me to create a GitHub issue to track the focus retention improvement task? I can help draft the issue with the necessary context and acceptance criteria.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/src/IDE/Components/EditableName/EditableName.test.tsx (3 hunks)
- app/client/src/IDE/Components/EditableName/EditableName.tsx (5 hunks)
- app/client/src/IDE/Components/EditableName/useIsRenaming.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/IDE/Components/EditableName/EditableName.test.tsx
🔇 Additional comments (2)
app/client/src/IDE/Components/EditableName/useIsRenaming.ts (2)
1-5
: LGTM! Imports are well-organized and necessary.
11-26
: LGTM! External edit handling is well-implemented.The effect includes proper cleanup and is correctly dependent only on external edit state changes.
Deploy-Preview-URL: https://ce-37116.dp.appsmith.com |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11569205687. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/IDE/Components/EditableName/EditableName.test.tsx (3 hunks)
- app/client/src/IDE/Components/EditableName/EditableName.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/IDE/Components/EditableName/EditableName.test.tsx
🔇 Additional comments (3)
app/client/src/IDE/Components/EditableName/EditableName.tsx (3)
44-51
: Function attemptSave
is well-implemented
The attemptSave
function effectively validates the name and proceeds to save when there are no errors. This encapsulation improves code readability and maintainability.
53-57
: exitWithoutSaving
correctly resets state
The exitWithoutSaving
function appropriately handles the reset of editableName
and validationError
, ensuring the component exits editing mode without persisting changes.
74-77
: Confirm intention when editableName
is unchanged
In the handleKeyUp
function, pressing "Enter" with an unmodified editableName
triggers exitWithoutSaving()
. Please verify that this is the desired behavior and that it aligns with user expectations.
const validate = (name: string) => { | ||
const nameError = validateName(name); | ||
|
||
if (nameError === null) { | ||
setValidationError(null); | ||
} else { | ||
setValidationError(nameError); | ||
} | ||
|
||
return nameError; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider simplifying the validate
function
Since the validate
function is used in only two places and is relatively concise, you might consider inlining its logic to reduce indirection and improve readability.
Deploy-Preview-URL: https://ce-37116.dp.appsmith.com |
@hetunandu when I'm trying to edit the name, and end up giving the same name and press "enter" or click outside, it calls the API and gives error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/client/src/IDE/Components/EditableName/useNameEditor.ts (1)
30-34
: Consider enhancing error messages for better UX.
While the validation logic is solid, consider adding more specific error messages for different validation failures (e.g., empty string vs whitespace-only).
if (!name || name.trim().length === 0) {
- return createMessage(ACTION_INVALID_NAME_ERROR);
+ return createMessage(
+ name.trim().length === 0
+ ? ACTION_INVALID_NAME_EMPTY_ERROR
+ : ACTION_INVALID_NAME_ERROR
+ );
}
app/client/src/IDE/Components/EditableName/EditableName.tsx (2)
15-27
: Improve props documentation consistency
The JSDoc comments for props have inconsistent formatting and some props lack type documentation.
Apply this diff to improve documentation:
/** isLoading true will show a spinner **/
isLoading?: boolean;
- /** if a valid name is entered, the onNameSave
- * will be called with the new name */
+ /** Callback fired when a valid name is saved
+ * @param name - The new name to save
+ */
onNameSave: (name: string) => void;
- /** Used in conjunction with exit editing to control
- * this component input editable state */
+ /** Controls the editable state of the component */
isEditing: boolean;
Focus management needs systematic improvement across the codebase
The search reveals multiple instances of setTimeout-based focus management:
EditableName.tsx
: Current implementation with focus retention workarounduseDropdown.tsx
: Using timeout for focus withFOCUS_TIMEOUT
constantGlobalSearch/index.tsx
: Direct setTimeout for focus on global search
This pattern indicates a systemic issue rather than an isolated case. Consider creating a centralized focus management utility to handle these cases consistently.
🔗 Analysis chain
Line range hint 137-152
: Track technical debt: Focus management needs improvement
While the current implementation works, relying on timeouts for focus management is fragile. The TODO comment indicates this will be addressed in a future task.
Let's track this technical debt:
Would you like me to create a GitHub issue to track the focus management improvement task?
The previous command failed because we need to adjust our search strategy. I've modified the commands to:
- Use proper ripgrep type flags for TypeScript files
- Expand the search patterns to catch different focus-related timeout implementations
- Include broader focus management patterns to understand the context better
Let's try with explicit file glob patterns instead of file type flags since ripgrep seems to have issues with the typescript type recognition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of setTimeout being used for focus management
rg -n 'setTimeout.*focus' --type ts --type tsx
Length of output: 76
Script:
#!/bin/bash
# Search for setTimeout being used with focus, accounting for both .ts and .tsx extensions
rg -t typescript -t tsx 'setTimeout.*focus'
# Also search for focus-related timeouts with different syntax
rg -t typescript -t tsx 'setTimeout.*\.focus\(\)'
# Look for any focus management patterns
rg -t typescript -t tsx 'focus.*management|focus.*retention'
Length of output: 265
Script:
#!/bin/bash
# Search for setTimeout with focus using just the ts extension
rg 'setTimeout.*focus' -g '*.ts'
# Search for setTimeout with focus using tsx extension
rg 'setTimeout.*focus' -g '*.tsx'
# Look for focus management patterns in both ts and tsx files
rg 'focus.*management|focus.*retention' -g '*.{ts,tsx}'
# Search for any setTimeout usage with focus() calls
rg 'setTimeout.*\.focus\(' -g '*.{ts,tsx}'
Length of output: 1459
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/IDE/Components/EditableName/EditableName.tsx (4 hunks)
- app/client/src/IDE/Components/EditableName/useNameEditor.ts (2 hunks)
🔇 Additional comments (4)
app/client/src/IDE/Components/EditableName/useNameEditor.ts (2)
28-39
: LGTM! Proper memoization implementation.
The switch to useCallback
with correct dependencies array is a good improvement. The validation logic is comprehensive, handling both empty names and conflicts.
32-33
: Verify the name conflict check behavior.
The name conflict check is skipped when the new name matches the current name, which is correct. However, let's verify this behavior works as expected across the codebase.
✅ Verification successful
Let me gather more context about the name validation implementation.
Let me check the implementation of isNameValid
function to complete the verification.
Name conflict check behavior is correctly implemented
The validation logic in useNameEditor.ts
correctly skips the name conflict check when the new name matches the current name (name !== entityName
). The isNameValid
function is consistently used across the codebase to validate names against JavaScript keywords, data tree keywords, and other invalid names. The implementation is sound and follows the same pattern in all usage locations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other name validation implementations to ensure consistency
# Look for similar name validation patterns in action-related files
rg -A 3 "isNameValid.*usedEntityNames"
Length of output: 422
Script:
#!/bin/bash
# Find the isNameValid function implementation
ast-grep --pattern 'function isNameValid($_) {
$$$
}'
# Also check for any other usage of isNameValid
rg "isNameValid" -B 2 -A 2
# Look for the file content to understand the complete context
cat app/client/src/IDE/Components/EditableName/useNameEditor.ts
Length of output: 7816
Script:
#!/bin/bash
# Get the implementation of isNameValid from utils/helpers
cat app/client/src/utils/helpers.tsx | grep -A 20 "export const isNameValid"
Length of output: 649
app/client/src/IDE/Components/EditableName/EditableName.tsx (2)
71-87
: LGTM! Improved state management implementation
The attemptSave
function now correctly handles validation results and includes proper handling for when the name hasn't changed.
Line range hint 89-116
: LGTM! Event handlers properly implemented
Event handlers are correctly memoized and integrated with the new validation and save logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/client/src/PluginActionEditor/components/PluginActionNameEditor.tsx (1)
Line range hint
21-24
: Update SaveActionNameParams interface to match implementation.The interface indicates
saveActionName
returns aReduxAction
, but the implementation shows it's now used as an action creator that gets dispatched. The interface should be updated to reflect this.Apply this change to fix the type definition:
export interface PluginActionNameEditorProps { saveActionName: ( params: SaveActionNameParams, - ) => ReduxAction<SaveActionNameParams>; + ) => void; }Also applies to: 72-78
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/JSObjectNameEditor/JSObjectNameEditor.tsx (2)
Line range hint
103-112
: Add type safety for feature flag constant.Consider using a type-safe approach for feature flag constants to prevent potential typos or misconfigurations.
- FEATURE_FLAG.release_actions_redesign_enabled, + FEATURE_FLAG.release_actions_redesign_enabled as const,
Duplicate interface
SaveActionNameParams
found - needs consolidationFound identical interface definitions in:
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/JSObjectNameEditor/JSObjectNameEditor.tsx
app/client/src/PluginActionEditor/components/PluginActionNameEditor.tsx
Move the interface to a shared types file to avoid duplication and maintain consistency.
🔗 Analysis chain
Line range hint
20-23
: Consider consolidating type definitions.Based on previous learnings, the
SaveActionNameParams
interface might be duplicated elsewhere. Consider moving it to a shared types file to maintain consistency.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for duplicate definitions of SaveActionNameParams rg -A 3 "interface SaveActionNameParams"Length of output: 857
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/src/PluginActionEditor/components/PluginActionNameEditor.tsx
(4 hunks)app/client/src/pages/Editor/JSEditor/JSEditorToolbar/JSObjectNameEditor/JSObjectNameEditor.tsx
(2 hunks)
🧰 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 (5)
app/client/src/PluginActionEditor/components/PluginActionNameEditor.tsx (3)
2-2
: LGTM: Import changes align with the new functionality.
The new imports support the Redux integration and renaming functionality.
Also applies to: 11-11, 14-14
60-60
: LGTM: Proper integration with global renaming state.
The switch to useIsRenaming
hook properly integrates with the new global renaming state management system.
74-78
: Verify error handling for rename operations.
Given the reported issue with API errors when entering the same name, we should verify the error handling implementation.
app/client/src/pages/Editor/JSEditor/JSEditorToolbar/JSObjectNameEditor/JSObjectNameEditor.tsx (2)
18-18
: Clean implementation of the new renaming hook!
The migration to useIsRenaming
hook is well-structured and follows React hooks best practices.
Also applies to: 88-90
Line range hint 96-102
: Add error handling for failed save operations.
Given the reported issues with name saving, consider adding error handling and user feedback:
const handleSaveName = useCallback(
(name: string) => {
if (currentJSObjectConfig) {
- dispatch(saveJSObjectName({ id: currentJSObjectConfig.id, name }));
+ try {
+ dispatch(saveJSObjectName({ id: currentJSObjectConfig.id, name }));
+ } catch (error) {
+ // Handle save failure
+ exitEditMode();
+ // Show error toast or feedback
+ }
}
},
[currentJSObjectConfig, saveJSObjectName],
);
Let's verify the current error handling:
#!/bin/bash
# Search for error handling patterns in name saving operations
rg -l "saveJSObjectName.*catch"
Description
Adds the Rename context menu in the Action Redesign Epic
Fixes #36968
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/11606861448
Commit: da150b1
Cypress dashboard.
Tags:
@tag.IDE
Spec:
Thu, 31 Oct 2024 06:29:54 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
InputCursorMoveIcon
for enhanced visual representation.RenameMenuItem
component for improved renaming functionality.useIsRenaming
hook to manage renaming state effectively.Enhancements
EditableName
component to improve editing control and validation.EditableText
component with better styling and error handling.EditableTab
component integration with Redux for renaming interactions.Bug Fixes
EditableName
component to ensure accurate behavior during editing.Chores