chore: Updating plugin action name editor component to use ADS text component #36960
Conversation
…d of blueprint component
…o chore/update-editable-action-name
…o chore/update-editable-action-name
…o chore/update-editable-action-name
WalkthroughThe changes in this pull request introduce several enhancements across multiple components in the application. Key modifications include the addition of a new optional property Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
IDE Pod
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11398296521. |
|
This PR has increased the number of cyclic dependencies by 36, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
app/client/src/ce/entities/IDE/constants.ts (1)
125-125: LGTM. Consider adding JSDoc comment.The addition of
userPermissionsaligns well with the PR objectives. To improve maintainability, consider adding a JSDoc comment explaining the purpose and expected format of this property.+ /** Array of permission strings associated with this entity. */ userPermissions?: string[];app/client/src/components/editorComponents/ActionNameEditor.tsx (1)
86-86: LGTM. Consider adding a comment for clarity.The change to set
iconSizeto a static value of "md" aligns with the PR objective and simplifies the component. Good job on removing the feature flag dependency.Consider adding a brief comment explaining why "md" was chosen as the default size, for future maintainability:
- iconSize={"md"} + iconSize={"md"} // Default medium size for consistency across the appapp/client/src/IDE/Components/FileTab/FileTab.tsx (1)
94-95: LGTM with a minor suggestion: Consider improving readability.The logic correctly implements the new permission check. However, consider using an if-else statement for improved readability:
const handleDoubleClick = () => { if (editorConfig && isChangePermitted) { handleEnterEditMode(); } };This approach might be easier to understand and maintain, especially if more conditions are added in the future.
app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (2)
126-145: LGTM: Enhanced EditableTab with entity prop.The changes correctly integrate the new
entityprop into theEditableTabcomponent, providing more context as per the PR objectives.Consider optimizing the entity lookup by using a Map for O(1) access:
const entityMap = new Map(entities.map(entity => [entity.key, entity])); // ... const entity = entityMap.get(tab.key);This could improve performance, especially with a large number of entities.
130-143: LGTM: EditableTab updated with entity prop.The addition of the
entityprop toEditableTabis consistent with the earlier changes and aligns with the PR objectives.Consider adding a comment or updating the component's documentation to describe the purpose and expected shape of the
entityprop for future maintainability.app/client/src/ce/selectors/entitiesSelector.ts (1)
1707-1718: Function implementation is correct, but consider adding error handling.The
getIsSavingEntityNamefunction correctly checks for API and JS object name saving states. However, it might be beneficial to add error handling for unexpected segment values.Consider adding a default case to handle unexpected segment values:
export const getIsSavingEntityName = ( state: AppState, { id, segment }: IsSavingEntityNameParams, ) => { let isSavingEntityName = getIsSavingForApiName(state, id); if (EditorEntityTab.JS === segment) { isSavingEntityName = getIsSavingForJSObjectName(state, id); } + // Handle unexpected segment values + if (![EditorEntityTab.API, EditorEntityTab.JS].includes(segment)) { + console.warn(`Unexpected segment value: ${segment}`); + } return isSavingEntityName; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- app/client/src/IDE/Components/FileTab/FileTab.tsx (3 hunks)
- app/client/src/PluginActionEditor/components/PluginActionNameEditor.tsx (2 hunks)
- app/client/src/ce/entities/IDE/constants.ts (1 hunks)
- app/client/src/ce/entities/IDE/utils.ts (2 hunks)
- app/client/src/ce/selectors/entitiesSelector.ts (4 hunks)
- app/client/src/components/editorComponents/ActionNameEditor.tsx (1 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/EditableTab.tsx (2 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (2 hunks)
- app/client/src/utils/hooks/useNameEditor.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (27)
app/client/src/utils/hooks/useNameEditor.ts (1)
14-17: LGTM. Verify usage in other files.The export of
NameSaveActionParamslooks good. It aligns with the PR objectives and doesn't introduce any issues.Let's check if this interface is now being imported elsewhere:
✅ Verification successful
The previous command used an unsupported file type. Retrying with correct file extensions.
Export of
NameSaveActionParamsis verified and utilized in other files.
- Imported in
app/client/src/ce/entities/IDE/utils.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for imports of NameSaveActionParams in other files rg --type typescript "import.*NameSaveActionParams.*from.*useNameEditor" -g '!app/client/src/utils/hooks/useNameEditor.ts'Length of output: 159
Script:
#!/bin/bash # Description: Check for imports of NameSaveActionParams in other files rg "import.*NameSaveActionParams.*from.*useNameEditor" -g '*.ts' -g '*.tsx' -g '!app/client/src/utils/hooks/useNameEditor.ts'Length of output: 238
app/client/src/IDE/Components/FileTab/FileTab.tsx (2)
16-16: LGTM: New prop for permission control.The addition of
isChangePermittedaligns with the PR objective and is correctly typed.
36-36: LGTM: Sensible default for new permission prop.Setting
isChangePermittedtofalseby default is a safe choice and maintains backward compatibility.app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (1)
38-38: LGTM: New selector added correctly.The addition of the
entitiesselector usinguseSelectorwithshallowEqualis a good practice. It aligns with the PR objectives and should help prevent unnecessary re-renders.app/client/src/ce/selectors/entitiesSelector.ts (2)
1701-1705: New interface looks good.The
IsSavingEntityNameParamsinterface is well-defined and includes all necessary properties for its purpose.
1657-1657: Good addition of user permissions.The inclusion of
userPermissionsin thegetQuerySegmentItemsandgetJSSegmentItemsfunctions is a positive change. It enhances the security model by allowing for more granular access control.Also applies to: 1672-1672
app/client/src/pages/Editor/IDE/EditorTabs/EditableTab.tsx (5)
21-21: Adding optional 'entity' prop enhances component flexibilityIncluding
entity?: EntityIteminEditableTabPropsallows the component to handle entity-specific logic when necessary.
28-32: Feature flag integration for 'license_gac_enabled' is appropriateUtilizing
useFeatureFlagwithFEATURE_FLAG.license_gac_enabledand checking permissions withgetEditableTabPermissionsensures that feature-specific behavior is correctly managed based on the user's permissions and the application's license.
37-37: Passing 'entity' to 'saveEntityName' aligns with updated action requirementsIncluding
entityin thesaveEntityNameparameters provides the necessary context for the name-saving action, accommodating any entity-specific logic required during the save operation.
41-41: Selector update ensures accurate loading stateAdding
entityto thegetIsSavingEntityNameselector call allows for precise tracking of the loading state associated with the specific entity being edited.
63-63: Verify that 'FileTab' component accepts 'isChangePermitted' propEnsure that the
FileTabcomponent is designed to handle theisChangePermittedprop to control the editability of the tab based on user permissions.Run the following script to confirm that
FileTabaccepts theisChangePermittedprop:✅ Verification successful
'FileTab' Component Correctly Accepts 'isChangePermitted' Prop
The
FileTabcomponent accepts theisChangePermittedprop and uses it to control the editability of the tab based on user permissions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if 'FileTab' component accepts 'isChangePermitted' prop. # Search for 'isChangePermitted' in 'FileTab' component files rg --type javascript --type typescript 'isChangePermitted' -A 5 -B 5 -g '*FileTab*'Length of output: 118
Script:
#!/bin/bash # Description: Re-verify if 'FileTab' component accepts 'isChangePermitted' prop with corrected file types. # Search for 'isChangePermitted' in 'FileTab' component files rg --type js --type ts 'isChangePermitted' -A 5 -B 5 -g '*FileTab*'Length of output: 2453
app/client/src/ce/entities/IDE/utils.ts (2)
67-75: Ensure proper handling whenentityis undefinedIn
getEditableTabPermissions, whenentityis undefined,entity?.userPermissions || []results in an empty array being passed togetHasManageActionPermission. Please verify that passing an empty permissions array is appropriate and thatgetHasManageActionPermissionhandles this scenario correctly.Consider reviewing the implementation of
getHasManageActionPermissionto ensure it handles empty permission arrays as expected.
51-60: Verify thatsaveEntityNamehandles all segments correctlyThe function
saveEntityNamedefaults to usingsaveActionNamewhen thesegmentis notEditorEntityTab.JS. Please ensure thatsaveActionNameis appropriate for all other segments. Consider explicitly handling other segments if different actions are required.To confirm all segments are handled correctly, you can run the following script:
app/client/src/PluginActionEditor/components/PluginActionNameEditor.tsx (14)
1-16: Imports are correctly organizedAll necessary modules are imported appropriately.
29-51: Styled components are properly definedThe
NameWrapper,IconContainer, andTextstyled components are well-structured.
62-75: State and hooks are initialized correctlyThe component's state variables and hooks are appropriately set up.
77-82: Editing state is managed effectivelyThe
useBooleanhook efficiently controls the editing state.
89-90: Current title computation is accurateThe logic for determining
currentTitlehandles different states correctly.
94-112: Key events are handled appropriatelyThe
handleKeyUpfunction processes 'Enter' and 'Escape' keys effectively.
115-119: Title changes are updated correctlyThe
handleTitleChangefunction normalizes and updates the title as expected.
121-124: Edit mode initiation is correctEntering edit mode is handled properly in
handleEnterEditMode.
126-127: Permission checks are implementedDouble-click functionality respects user permissions.
128-141: Input properties are optimizedUsing
useMemoforinputPropsenhances performance.
143-160: Focus out event is handled properlyThe
useEventListenerfor 'focusout' manages title saving on focus loss.
162-169: Title synchronization is maintainedThe
useEffecthook keepseditableTitlein sync withtitle.
171-182: Input focus is correctly managedFocusing the input when editing ensures a smooth user experience.
185-199: Rendering components appropriatelyThe JSX structure correctly displays all UI elements with proper conditional rendering.
|
Deploy-Preview-URL: https://ce-36960.dp.appsmith.com |
…o chore/update-editable-action-name
|
This PR has increased the number of cyclic dependencies by 36, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
1 similar comment
|
This PR has increased the number of cyclic dependencies by 36, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/IDE/Components/FileTab/FileTab.test.tsx (1)
Add tests for
isChangePermittedprop to ensure proper functionality.
- The new
isChangePermittedprop is not currently covered by any tests inFileTab.test.tsx.🔗 Analysis chain
Line range hint
1-238: Verify test coverage for new prop.The new
isChangePermittedprop may require additional test cases to ensure proper functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test coverage of isChangePermitted prop # Test: Search for tests related to isChangePermitted rg --type typescript 'test.*isChangePermitted' app/client/src/IDE/Components/FileTabLength of output: 121
Script:
#!/bin/bash # Description: Retry searching for tests related to isChangePermitted using the correct file type # Test: Search for tests related to isChangePermitted in TypeScript React files rg --type typescriptreact 'isChangePermitted' app/client/src/IDE/Components/FileTabLength of output: 123
Script:
#!/bin/bash # Description: Search for tests related to isChangePermitted using file extension patterns # Test: Search for isChangePermitted in .ts and .tsx files rg 'isChangePermitted' app/client/src/IDE/Components/FileTab --glob '*.ts' --glob '*.tsx'Length of output: 448
Script:
#!/bin/bash # Description: Search for test cases and assertions related to isChangePermitted # Test: Look for expect statements or test scenarios involving isChangePermitted rg 'expect.*isChangePermitted' app/client/src/IDE/Components/FileTab/FileTab.test.tsxLength of output: 87
…o chore/update-editable-action-name
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/ce/entities/IDE/utils.ts (1)
67-75: LGTM with a minor suggestion for clarity.The function looks good. Consider destructuring
userPermissionsfor improved readability:export const getEditableTabPermissions = ({ entity, isFeatureEnabled, }: EditableTabPermissions) => { const { userPermissions = [] } = entity || {}; return getHasManageActionPermission(isFeatureEnabled, userPermissions); };This change makes it clearer that we're using an empty array as a fallback when
entityoruserPermissionsis undefined.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/ce/entities/IDE/utils.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
app/client/src/ce/entities/IDE/utils.ts (3)
10-13: LGTM: New imports are relevant and correctly added.
15-22: LGTM: SaveEntityName interface is well-structured.The interface provides a clear contract for saving entity names.
62-65: LGTM: EditableTabPermissions interface is well-defined.
…omponent (appsmithorg#36960) ## Description Updating plugin action name editor component to use ADS text component instead of EditableText from blueprint. Also, adding permission checks for editable tabs to not allow the name edits when the user doesn't have the permission. Fixes [appsmithorg#36793](appsmithorg#36793) ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11402567877> > Commit: 0b695e8 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11402567877&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Fri, 18 Oct 2024 12:36:26 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced conditional editing functionality in the FileTab component based on user permissions. - Enhanced the PluginActionNameEditor with real-time validation and improved user interaction. - Added user permissions information to entity items and selectors for better access control. - Integrated new functionality for saving entity names and managing editable tab permissions. - Expanded the EditorTabs component to include contextual entity information for each tab. - **Bug Fixes** - Simplified the ActionNameEditor by removing unnecessary feature flag checks. - **Documentation** - Updated interfaces and components to improve clarity and functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Updating plugin action name editor component to use ADS text component instead of EditableText from blueprint. Also, adding permission checks for editable tabs to not allow the name edits when the user doesn't have the permission.
Fixes #36793
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11402567877
Commit: 0b695e8
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 18 Oct 2024 12:36:26 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation