Skip to content
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

Merged
merged 21 commits into from
Oct 31, 2024
Merged

Conversation

hetunandu
Copy link
Member

@hetunandu hetunandu commented Oct 28, 2024

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?

  • Yes
  • No

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced the InputCursorMoveIcon for enhanced visual representation.
    • Added RenameMenuItem component for improved renaming functionality.
    • New useIsRenaming hook to manage renaming state effectively.
    • Added context menu option for renaming JavaScript collections.
  • Enhancements

    • Updated EditableName component to improve editing control and validation.
    • Enhanced EditableText component with better styling and error handling.
    • Streamlined EditableTab component integration with Redux for renaming interactions.
  • Bug Fixes

    • Refined test cases for the EditableName component to ensure accurate behavior during editing.
  • Chores

    • Consolidated exports in the IDE module for better organization.

# 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
@hetunandu hetunandu requested a review from ayushpahwa as a code owner October 28, 2024 09:36
Copy link
Contributor

coderabbitai bot commented Oct 28, 2024

Walkthrough

The pull request introduces several enhancements to the IDE, including the addition of a new icon, InputCursorMoveIcon, and the creation of a RenameMenuItem component. The EditableName component has been updated to support new props for improved editing behavior, while tests for this component have been expanded to cover new scenarios. Additionally, new Redux actions, selectors, and state management features have been implemented to facilitate entity renaming. Overall, these changes improve the functionality and user experience of the renaming process within the IDE.

Changes

File Path Change Summary
app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx Added new icon InputCursorMoveIcon to ICON_LOOKUP.
app/client/src/IDE/Components/EditableName/EditableName.test.tsx Modified tests: enhanced setup and added tests for validation error messages.
app/client/src/IDE/Components/EditableName/EditableName.tsx Updated EditableTextProps interface; added new props and functions for improved editing logic.
app/client/src/IDE/Components/EditableName/RenameMenuItem.tsx Introduced new RenameMenuItem component for renaming entities.
app/client/src/IDE/Components/EditableName/index.ts Exported new RenameMenuItem and useIsRenaming hooks.
app/client/src/IDE/Components/EditableName/useIsRenaming.ts Added useIsRenaming hook for managing renaming state.
app/client/src/IDE/index.ts Updated exports to include RenameMenuItem and useIsRenaming.
app/client/src/actions/ideActions.ts Added setRenameEntity action creator for renaming entities.
app/client/src/ce/constants/ReduxActionConstants.tsx Introduced new action type SET_RENAME_ENTITY.
app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx Replaced Rename component with RenameMenuItem.
app/client/src/components/editorComponents/EditableText.tsx Enhanced styling and error handling in EditableText.
app/client/src/pages/Editor/IDE/EditorTabs/EditableTab.tsx Updated to use useIsRenaming for managing edit state and modified handleNameSave.
app/client/src/reducers/uiReducers/ideReducer.ts Added renameEntity state and SET_RENAME_ENTITY action handler.
app/client/src/selectors/ideSelectors.tsx Introduced getRenameEntity and getIsRenaming selectors for renaming state management.

Assessment against linked issues

Objective Addressed Explanation
Add the new context menu option called rename in App IDE query and Js (where tabs show up) (#36968)

Possibly related PRs

Suggested labels

Enhancement, Bug

Suggested reviewers

  • AmanAgarwal041
  • albinAppsmith
  • ankitakinger
  • sagar-qa007

🌟 In the IDE where we create and play,
A new icon shines bright, leading the way.
With renaming now swift, and tests that expand,
The EditableName dances, just as we planned!
So let’s celebrate code, with joy in our hearts,
For each little change is a masterpiece of arts! 🎉


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added IDE Navigation Issues/feature requests related to IDE navigation, and context switching IDE Pod Issues that new developers face while exploring the IDE IDE Product Issues related to the IDE Product IDE tabs query and js tabs Task A simple Todo skip-changelog Adding this label to a PR prevents it from being listed in the changelog labels Oct 28, 2024
@hetunandu
Copy link
Member Author

/build-deploy-preview

Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11551218267.
Workflow: On demand build Docker image and deploy preview.
skip-tests: . env: .
PR: 37116.
recreate: .

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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:

  1. A more specific type for the id parameter (e.g., EntityId)
  2. 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 dispatch

The 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 documentation

Add 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 mode

The 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 props

Ensure 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 constant

Avoid 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

📥 Commits

Files that changed from the base of the PR and between 836bf20 and 68d4d07.

⛔ 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 icon
  • RenameMenuItem: React component with disabled state and entity ID props
  • useIsRenaming: 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.ts

Length 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:

  1. An aria-label for better screen reader support
  2. 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 tsx

Length 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 2

Length 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 suggestion

Consider 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 of id in exitEditMode.

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 an id parameter and is used in two places:

  1. In RenameMenuItem.tsx, it's called with entityId to start renaming
  2. In useIsRenaming.ts, it's called with an empty string to clear the renaming state

Let me verify if this pattern is consistent by checking the reducer and any other related components.


The id dependency in exitEditMode is correct, but the implementation is valid

The current implementation of exitEditMode correctly passes an empty string to clear the rename state, which is consistent with the Redux action and reducer implementation. The id dependency is justified as the callback is recreated when the component's id 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 2

Length of output: 1307


Script:

#!/bin/bash
# Search for the reducer handling SET_RENAME_ENTITY
rg "SET_RENAME_ENTITY" -A 5

Length of output: 1156

app/client/src/ce/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx (1)

17-17: LGTM: Clean import statement

The 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 in app/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 1

Length 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 2

Length 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 functionality

The new imports support the Redux integration and renaming functionality.

Also applies to: 17-17


40-41: LGTM: Improved state management with useIsRenaming

The switch to useIsRenaming provides better semantics and additional states needed for the rename functionality.


78-78: Verify EditableName handles needsInteractionBeforeExit correctly

Please 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 10

Length 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 and useIsRenaming 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 tsx

Length 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 configured

The empty string is an appropriate default value for the renaming state.


114-121: Verify state handling in dependent components

Let'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 typescriptreact

Length 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 2

Length of output: 3670

app/client/src/IDE/Components/EditableName/EditableName.test.tsx (2)

20-24: LGTM: Setup function changes look good

The setup function is well-structured with appropriate defaults.

Also applies to: 30-30


98-98: LGTM: Assertion change is correct

The 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 ts

Length 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.ts

Length of output: 328

Copy link

Deploy-Preview-URL: https://ce-37116.dp.appsmith.com

@hetunandu
Copy link
Member Author

/build-deploy-preview

Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11551516421.
Workflow: On demand build Docker image and deploy preview.
skip-tests: . env: .
PR: 37116.
recreate: .

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 prop

The 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 suggestion

Consider 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 suggestion

The dependency array for inputProps includes inputTestId 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 fix

The 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

📥 Commits

Files that changed from the base of the PR and between 68d4d07 and de8d9fe.

📒 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.

Copy link

Deploy-Preview-URL: https://ce-37116.dp.appsmith.com

Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11569205687.
Workflow: On demand build Docker image and deploy preview.
skip-tests: . env: .
PR: 37116.
recreate: .

@hetunandu
Copy link
Member Author

@ankitakinger

  1. On outside click, the new name is not getting saved
    Done
  2. JS objects doesn't have the rename context menu added, will that be handled in a separate PR?
    Done
  3. Wasn't able to check for modules, workflows & module instances. Do we have a EE PR as well for testing?
    https://github.com/appsmithorg/appsmith-ee/pull/5456

@hetunandu hetunandu added the ok-to-test Required label for CI label Oct 29, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Files that changed from the base of the PR and between ca018cd and 838023e.

📒 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.

Comment on lines 59 to 70
const validate = (name: string) => {
const nameError = validateName(name);

if (nameError === null) {
setValidationError(null);
} else {
setValidationError(nameError);
}

return nameError;
};

Copy link
Contributor

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.

Copy link

Deploy-Preview-URL: https://ce-37116.dp.appsmith.com

@ankitakinger
Copy link
Contributor

@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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 workaround
  • useDropdown.tsx: Using timeout for focus with FOCUS_TIMEOUT constant
  • GlobalSearch/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:

  1. Use proper ripgrep type flags for TypeScript files
  2. Expand the search patterns to catch different focus-related timeout implementations
  3. 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

📥 Commits

Files that changed from the base of the PR and between 838023e and 56726c4.

📒 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 a ReduxAction, 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 consolidation

Found 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

📥 Commits

Reviewing files that changed from the base of the PR and between 56726c4 and da150b1.

📒 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" 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IDE Navigation Issues/feature requests related to IDE navigation, and context switching IDE Pod Issues that new developers face while exploring the IDE IDE Product Issues related to the IDE Product IDE tabs query and js tabs ok-to-test Required label for CI skip-changelog Adding this label to a PR prevents it from being listed in the changelog Task A simple Todo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Add Rename context menu option
3 participants