chore: Use ADS components in Page List#39570
Conversation
WalkthroughThis pull request introduces several new React components for enhanced page management. It adds individual context menu items for actions such as cloning, deleting, renaming, partial import/export, setting a page as the home page, and toggling visibility. A new main ContextMenu component is provided along with its export update, while the PageEntity, PageList, and PagesSection components streamline the display and interaction with pages. An import path has also been refactored to accommodate file restructuring. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CM as ContextMenu
participant A as Action Callback
participant R as Redux Dispatch
participant S as Redux Store
U->>CM: Select a menu item (e.g., Clone, Delete)
CM->>A: Invoke corresponding callback
A->>R: Dispatch Redux action (e.g., clonePageInit, deletePageAction)
R->>S: Process action and update state
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
IDE Navigation
![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
🧹 Nitpick comments (13)
app/client/src/pages/AppIDE/components/PageList/ContextMenu/Visibility.tsx (1)
25-36: Consider adding an accessibility attribute for the visibility toggle.The visibility toggle might benefit from an aria-label to improve accessibility for screen readers.
<MenuItem disabled={disabled} onSelect={setHiddenField} startIcon={isHidden ? "eye-on" : "eye-off"} + aria-label={isHidden ? "Show page" : "Hide page"} >app/client/src/pages/AppIDE/components/PageList/PageList.tsx (2)
19-29: Consider adding null/empty array check for pages.While the component likely receives a valid pages array from its parent, defensive programming would suggest adding a check for this.
if (!isNewADSEnabled) { + if (!pages || pages.length === 0) return null; return ( <> {pages.map((page) => (
31-38: Same null check recommendation for the new ADS path.For consistency, add the same check in both rendering paths.
return ( <> + {(!pages || pages.length === 0) ? null : {pages.map((page) => ( <PageEntity key={page.pageId} onClick={onItemSelected} page={page} /> ))} + } </> );app/client/src/pages/AppIDE/components/PageList/ContextMenu/PartialExport.tsx (1)
15-19: Consider handling potential callback errors.While the implementation is clean, it would be more robust to use a try-catch block when invoking the optional callback to prevent potential errors from breaking the export functionality.
const handlePartialExportClick = useCallback(() => { - if (onItemSelected) onItemSelected(); + if (onItemSelected) { + try { + onItemSelected(); + } catch (error) { + console.error("Error executing item selection callback:", error); + } + } dispatch(openPartialExportModal(true)); }, [onItemSelected, dispatch]);app/client/src/pages/AppIDE/components/PageList/ContextMenu/Delete.tsx (3)
30-37: Add try/catch to handle potential deletion errorsThe
onSelectcallback handles the UI flow well, but there's no error handling for potential failures when deleting pages.const onSelect = useCallback( (e?: Event) => { e?.preventDefault(); - confirmDelete ? deletePageCallback() : setConfirmDelete(true); + if (confirmDelete) { + try { + deletePageCallback(); + } catch (error) { + // Reset confirmation state if deletion fails + setConfirmDelete(false); + // Log error or show notification + } + } else { + setConfirmDelete(true); + } e?.stopPropagation(); }, [confirmDelete, deletePageCallback], );
39-49: Improve conditional styling logicThe current implementation applies 'error-menuitem' class when the menu item is NOT disabled, which is somewhat counter-intuitive.
<MenuItem - className={clsx("single-select", { "error-menuitem": !disabled })} + className={clsx("single-select", { "error-menuitem": !disabled && confirmDelete })} disabled={disabled} onSelect={onSelect} startIcon="trash" >This way, the error styling is only applied when the item is both enabled and in the confirmation state, making the intent clearer.
23-28: Consider resetting confirmation state after successful deletionThe deletion callback logs the event but doesn't reset the confirmation state. While this may not be necessary if the component unmounts after deletion, it would be cleaner to reset it.
const deletePageCallback = useCallback(() => { dispatch(deletePageAction(pageId)); AnalyticsUtil.logEvent("DELETE_PAGE", { pageName: pageName, }); + setConfirmDelete(false); }, [dispatch, pageId, pageName]);app/client/src/pages/AppIDE/components/PageList/ContextMenu/ContextMenu.tsx (3)
37-57: Memoize permission calculations to prevent unnecessary re-rendersThe permission calculations aren't memoized, which could cause unnecessary re-renders if the parent component re-renders frequently.
- const pagePermissions = - useSelector(getPageById(props.pageId))?.userPermissions || []; + const pagePermissions = useSelector( + (state) => getPageById(props.pageId)(state)?.userPermissions || [] + ); - const userAppPermissions = useSelector( - (state: AppState) => getCurrentApplication(state)?.userPermissions ?? [], - ); + const userAppPermissions = useSelector( + (state: AppState) => getCurrentApplication(state)?.userPermissions ?? [] + ); - const canManagePages = getHasManagePagePermission( - isFeatureEnabled, - pagePermissions, - ); + const canManagePages = useMemo( + () => getHasManagePagePermission(isFeatureEnabled, pagePermissions), + [isFeatureEnabled, pagePermissions] + ); - const canCreatePages = getHasCreatePagePermission( - isFeatureEnabled, - userAppPermissions, - ); + const canCreatePages = useMemo( + () => getHasCreatePagePermission(isFeatureEnabled, userAppPermissions), + [isFeatureEnabled, userAppPermissions] + ); - const canDeletePages = getHasDeletePagePermission( - isFeatureEnabled, - pagePermissions, - ); + const canDeletePages = useMemo( + () => getHasDeletePagePermission(isFeatureEnabled, pagePermissions), + [isFeatureEnabled, pagePermissions] + );
59-60: Memoize showPartialImportExport calculationSimilar to the permission calculations, this derived state should be memoized.
- const showPartialImportExport = - props.hasExportPermission && props.isCurrentPage; + const showPartialImportExport = useMemo( + () => props.hasExportPermission && props.isCurrentPage, + [props.hasExportPermission, props.isCurrentPage] + );
34-35: Missing import for React.useMemoIf implementing the suggested memoization improvements, you'll need to import useMemo.
- import React from "react"; + import React, { useMemo } from "react";app/client/src/pages/AppIDE/components/PageList/PageEntity.tsx (3)
66-76: Improve scrollIntoView implementation with a more robust approachThe current implementation might cause issues with certain browser behaviors. Consider using a more robust approach with a cleanup function.
useEffect( function scrollPageIntoView() { if (ref.current && isCurrentPage) { - ref.current.scrollIntoView({ - inline: "nearest", - block: "nearest", - }); + // Use setTimeout to ensure DOM is fully rendered + const timeoutId = setTimeout(() => { + if (ref.current) { + ref.current.scrollIntoView({ + inline: "nearest", + block: "nearest", + behavior: "smooth" + }); + } + }, 0); + + return () => clearTimeout(timeoutId); } }, [ref, isCurrentPage], );
84-99: Consider separating navigation logic from the componentThe
switchPagefunction handles multiple concerns: analytics, toggling onboarding widget selection, and navigation. Consider extracting this logic.You could create a custom hook like
usePageNavigationthat encapsulates all this logic, making the component cleaner and more focused on its UI responsibilities.// hooks/usePageNavigation.js export function usePageNavigation() { const dispatch = useDispatch(); const location = useLocation(); return useCallback((navigateToUrl, pageName, onClick) => { AnalyticsUtil.logEvent("PAGE_NAME_CLICK", { name: pageName, fromUrl: location.pathname, type: "PAGES", toUrl: navigateToUrl, }); dispatch(toggleInOnboardingWidgetSelection(true)); history.push(navigateToUrl, { invokedBy: NavigationMethod.EntityExplorer, }); if (onClick) { onClick(); } }, [dispatch, location.pathname]); } // In your component: // const navigate = usePageNavigation(); // const handleClick = () => navigate(navigateToUrl, page.pageName, onClick);
127-152: Extract name editor configuration to a custom hookThe nameEditorConfig is complex and could be extracted to a custom hook for better reusability and testability.
// hooks/useNameEditor.js export function useNameEditor(entityId, entityName, canEdit, isHidden) { const dispatch = useDispatch(); const { editingEntity, enterEditMode, exitEditMode, updatingEntity } = useNameEditorState(); const validateName = useValidateEntityName({ entityName }); return useMemo(() => ({ canEdit, isEditing: editingEntity === entityId, isLoading: updatingEntity === entityId, onEditComplete: exitEditMode, onNameSave: (newName) => dispatch( updatePageAction({ id: entityId, name: newName, isHidden: !!isHidden, }), ), validateName: (newName) => validateName(newName), normalizeName: false, enterEditMode: () => enterEditMode(entityId), }), [ canEdit, dispatch, editingEntity, exitEditMode, entityId, isHidden, updatingEntity, validateName ]); } // In your component: // const nameEditor = useNameEditor(page.pageId, page.pageName, canManagePages, page.isHidden); // const nameEditorConfig = { ...nameEditor }; // const handleDoubleClick = canManagePages ? nameEditor.enterEditMode : noop;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/client/src/pages/AppIDE/components/PageList/ContextMenu/Clone.tsx(1 hunks)app/client/src/pages/AppIDE/components/PageList/ContextMenu/ContextMenu.tsx(1 hunks)app/client/src/pages/AppIDE/components/PageList/ContextMenu/Delete.tsx(1 hunks)app/client/src/pages/AppIDE/components/PageList/ContextMenu/PartialExport.tsx(1 hunks)app/client/src/pages/AppIDE/components/PageList/ContextMenu/PartialImport.tsx(1 hunks)app/client/src/pages/AppIDE/components/PageList/ContextMenu/Rename.tsx(1 hunks)app/client/src/pages/AppIDE/components/PageList/ContextMenu/SetAsHomePage.tsx(1 hunks)app/client/src/pages/AppIDE/components/PageList/ContextMenu/Visibility.tsx(1 hunks)app/client/src/pages/AppIDE/components/PageList/ContextMenu/index.ts(1 hunks)app/client/src/pages/AppIDE/components/PageList/OldPageEntity.tsx(1 hunks)app/client/src/pages/AppIDE/components/PageList/PageEntity.tsx(1 hunks)app/client/src/pages/AppIDE/components/PageList/PageList.tsx(1 hunks)app/client/src/pages/AppIDE/components/PageList/PagesSection.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/client/src/pages/AppIDE/components/PageList/ContextMenu/index.ts
- app/client/src/pages/AppIDE/components/PageList/OldPageEntity.tsx
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-lint / client-lint
🔇 Additional comments (15)
app/client/src/pages/AppIDE/components/PageList/ContextMenu/PartialImport.tsx (3)
7-10: Props interface is clear and well-defined.The interface correctly defines optional properties with proper TypeScript syntax.
12-20: Good use of useCallback for event handler optimization.The component properly memoizes the event handler which prevents unnecessary re-renders when this component is used within lists or frequently updated parent components.
21-29: MenuItem implementation is correct and follows the ADS pattern.The component correctly uses the MenuItem from the ADS library with proper props including disabled state and icon.
app/client/src/pages/AppIDE/components/PageList/ContextMenu/Visibility.tsx (2)
6-11: Clear and comprehensive props interface.The props interface includes all necessary parameters for the component functionality.
13-24: Effective use of Redux dispatch with useCallback.The component correctly implements the visibility toggle functionality with a memoized callback function for optimal performance.
app/client/src/pages/AppIDE/components/PageList/ContextMenu/SetAsHomePage.tsx (3)
7-11: Props interface is well-defined.The interface clearly specifies required and optional properties with appropriate types.
13-18: Proper implementation of setPageAsDefault action.The component correctly dispatches the action with the required parameters using a memoized callback.
19-28: MenuItem implementation follows ADS conventions.The MenuItem is correctly implemented with appropriate icon and message.
app/client/src/pages/AppIDE/components/PageList/PageList.tsx (2)
9-12: Props interface is clearly defined.The interface correctly defines the necessary props for the component.
14-18: Effective use of feature flag for conditional rendering.The component correctly uses the feature flag to determine which version of the UI to render.
app/client/src/pages/AppIDE/components/PageList/ContextMenu/Rename.tsx (1)
1-30: Well-structured component with proper separation of concerns.The component follows React best practices with proper hook usage and clear responsibilities. The timeout implementation in the setRename function effectively prevents focus issues with the menu trigger.
app/client/src/pages/AppIDE/components/PageList/ContextMenu/Clone.tsx (1)
1-23: Clean implementation following component pattern consistency.The Clone component is well-implemented with appropriate Redux integration and follows the same pattern as other context menu items, maintaining code consistency across the codebase.
app/client/src/pages/AppIDE/components/PageList/PagesSection.tsx (2)
2-2: Component dependencies updated correctly.The import changes properly reflect the refactoring to use the PageList component instead of direct page element rendering.
Also applies to: 19-19
82-82: Good refactoring to improve component modularity.Replacing the direct page element rendering with the PageList component improves code organization and maintainability by delegating the rendering responsibility to a specialized component.
app/client/src/pages/AppIDE/components/PageList/PageEntity.tsx (1)
154-169: The component implementation looks goodThe EntityItem implementation is clean and correctly passes all necessary props, including conditional props like isSelected and isDisabled. The use of right control visibility on hover is a good UI pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/pages/AppIDE/components/PageList/ContextMenu/ContextMenu.tsx (1)
63-63: Consider adding more specific test IDs for menu items.While the context menu has a test ID, adding specific test IDs for each menu item would improve testability.
- <EntityContextMenu dataTestId={EntityClassNames.CONTEXT_MENU}> + <EntityContextMenu dataTestId={`${EntityClassNames.CONTEXT_MENU}-${props.pageId}`}>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/client/src/pages/AppIDE/components/PageList/ContextMenu/ContextMenu.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (7)
app/client/src/pages/AppIDE/components/PageList/ContextMenu/ContextMenu.tsx (7)
1-22: Well-organized imports with clear dependencies.The imports are well-structured, with a logical grouping of core React dependencies, UI components, and internal application utilities.
23-32: Props interface is well-defined with appropriate types.The interface clearly defines all necessary properties with proper typing, making the component's contract explicit and type-safe.
34-43: Feature flag and selector usage follows best practices.The component appropriately uses the feature flag pattern and Redux selectors to access application state, following the recommended patterns for the codebase.
44-58: Permission checks are properly implemented.The permission logic is cleanly separated into different concerns (manage, create, delete) with appropriate helper functions.
59-61: Conditional logic for partial import/export is clear.The condition for showing the partial import/export options is straightforward and easy to understand.
62-102: Context menu structure is well-organized with appropriate separators.The component renders a well-structured menu with logical grouping of related actions separated by
MenuSeparatorcomponents. Permission checks are consistently applied to disable actions when needed.
81-93: Verify the export/import functionality with current page requirement.The partial export/import functionality is only available for the current page. Ensure this is the intended behavior, as users might expect to export/import other pages as well.
|
|
||
| return ( | ||
| <MenuItem | ||
| className={clsx("single-select", { "error-menuitem": !disabled })} |
There was a problem hiding this comment.
We haven't done it this way in Query/JS/UI for the class error-menuitem . Should we also update those?
There was a problem hiding this comment.
This seemed better as red color disabled was looking off. We can update based on what @momcilo-appsmith decides
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13696085722. |
|
Deploy-Preview-URL: https://ce-39570.dp.appsmith.com |
## Description Updates the Page List component to use the new ADS components but behind feature flag Fixes appsmithorg#39569 ## 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/13695609914> > Commit: 6de5d43 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13695609914&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Thu, 06 Mar 2025 10:53:50 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced an enhanced page management menu offering options to clone, rename, delete, toggle visibility, set a page as home, and perform partial export/import. - Revamped page navigation with a unified list that adapts based on user permissions and feature settings for a smoother experience. - Added new components for managing page actions, including `Clone`, `Delete`, `Rename`, `SetAsHomePage`, `PartialExport`, `PartialImport`, and `Visibility`. - Implemented a new `PageEntity` and `PageList` component for improved page representation and management. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Updates the Page List component to use the new ADS components but behind feature flag
Fixes #39569
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/13695609914
Commit: 6de5d43
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 06 Mar 2025 10:53:50 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Clone,Delete,Rename,SetAsHomePage,PartialExport,PartialImport, andVisibility.PageEntityandPageListcomponent for improved page representation and management.