refactor: page actions menu #6076
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces several enhancements across the application, primarily focusing on the Changes
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (17)
web/ce/components/pages/modals/move-page-modal.tsx (1)
4-8: Consider renaming the type to follow React conventionsThe
Tprefix inTMovePageModalPropsis not a common convention in React/TypeScript. Consider renaming toMovePageModalPropsto align with React component prop naming patterns.-export type TMovePageModalProps = { +export type MovePageModalProps = { isOpen: boolean; onClose: () => void; page: IPage; };web/core/components/pages/list/block.tsx (2)
43-43: LGTM! Consider cleaning up the interface.The prop simplification looks good. However, the
TPageListBlockinterface still includesworkspaceSlugandprojectIdwhich are no longer passed toBlockItemAction.Consider updating the interface to reflect only the props being used:
type TPageListBlock = { workspaceSlug: string; projectId: string; pageId: string; };
43-43: Consider the trade-offs of routing parameter retrieval.Moving from prop drilling to using
useParamsinBlockItemActionmakes the component more tightly coupled to Next.js routing. While this reduces prop passing, it might impact:
- Component reusability in different routing contexts
- Test complexity as routing context needs to be mocked
- Performance if multiple instances exist in a list
Consider whether maintaining loose coupling through prop drilling might be more beneficial for long-term maintainability.
packages/ui/src/dropdowns/context-menu/item.tsx (1)
39-55: Consider enhancing accessibility and semantics.While the implementation is functionally sound, consider these improvements:
- Add ARIA attributes for better screen reader support
- Use semantic HTML for the content structure
Here's a suggested improvement:
- {item.customContent ?? ( - <> - {item.icon && <item.icon className={cn("h-3 w-3", item.iconClassName)} />} - <div> - <h5>{item.title}</h5> - {item.description && ( - <p - className={cn("text-custom-text-300 whitespace-pre-line", { - "text-custom-text-400": item.disabled, - })} - > - {item.description} - </p> - )} - </div> - </> + {item.customContent ?? ( + <> + {item.icon && ( + <span className="flex-shrink-0" aria-hidden="true"> + <item.icon className={cn("h-3 w-3", item.iconClassName)} /> + </span> + )} + <span className="flex-grow"> + <span className="block" role="menuitemtext">{item.title}</span> + {item.description && ( + <span + className={cn("block text-custom-text-300 whitespace-pre-line", { + "text-custom-text-400": item.disabled, + })} + role="note" + > + {item.description} + </span> + )} + </span> + </> )}This improves:
- Screen reader support with ARIA roles
- Semantic structure using appropriate HTML elements
- Layout flexibility with flex utilities
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(list)/header.tsx (1)
Line range hint
82-90: Simplify the conditional rendering.The empty fragment fallback is unnecessary and can be simplified.
Consider this cleaner approach:
- {canCurrentUserCreatePage ? ( + {canCurrentUserCreatePage && ( <Header.RightItem> <Button variant="primary" size="sm" onClick={handleCreatePage} loading={isCreatingPage}> {isCreatingPage ? "Adding" : "Add page"} </Button> </Header.RightItem> - ) : ( - <></> - )} + )}apiserver/plane/app/serializers/page.py (2)
Line range hint
32-43: Consider explicitly declaring all description-related fieldsWhile
description_htmlis explicitly declared inPageDetailSerializer, the basedescriptionanddescription_binaryfields are implicitly handled through model fields. For better code clarity and maintainability, consider explicitly declaring all description-related fields in the serializer.Add these field declarations to the PageSerializer:
class PageSerializer(BaseSerializer): is_favorite = serializers.BooleanField(read_only=True) + description = serializers.CharField(required=False, allow_blank=True) + description_binary = serializers.CharField(required=False, allow_blank=True) labels = serializers.ListField( child=serializers.PrimaryKeyRelatedField(queryset=Label.objects.all()), write_only=True, required=False, )
Line range hint
32-43: Document the relationship between description fieldsThe code handles three different description fields (
description,description_binary,description_html) but their relationship and purpose aren't clear. This could lead to confusion for other developers.Add docstring to explain the purpose and relationship of these fields:
class PageSerializer(BaseSerializer): + """ + Serializer for Page model. + + Description fields: + - description: Plain text description + - description_binary: Binary format for rich text storage + - description_html: HTML rendered version of the description + """Also applies to: 76-77
packages/ui/src/dropdowns/custom-menu.tsx (1)
Line range hint
15-15: Consider adding JSDoc documentation for better maintainability.Since this is a reusable UI component in a shared package, adding comprehensive JSDoc documentation for the component props and examples would improve maintainability and developer experience.
Example documentation:
/** * A customizable dropdown menu component with keyboard navigation and accessibility support. * * @example * ```tsx * <CustomMenu label="Actions"> * <CustomMenu.MenuItem onClick={() => console.log('clicked')}> * Action 1 * </CustomMenu.MenuItem> * </CustomMenu> * ``` * * @prop {string} [buttonClassName] - Additional classes for the button element * @prop {React.ReactNode} [customButton] - Custom button element to trigger the dropdown * @prop {boolean} [disabled=false] - Whether the dropdown is disabled * ... */web/core/components/pages/editor/header/options-dropdown.tsx (3)
64-69: Remove unnecessary onChange handler from ToggleSwitch.The ToggleSwitch component has an empty onChange handler while the actual toggle functionality is handled by the parent action. This creates unnecessary event handling overhead.
-<ToggleSwitch value={isFullWidth} onChange={() => {}} /> +<ToggleSwitch value={isFullWidth} />
115-229: Consider refactoring repetitive error handling patterns.The error handling pattern is repeated across multiple operations (toggleAccess, toggleArchive, toggleLock). Consider extracting this into a reusable utility function to reduce code duplication.
Here's a suggested implementation:
const handlePageOperation = async ( operation: () => Promise<void>, successMessage: string, errorMessage: string ) => { try { await operation(); setToast({ type: TOAST_TYPE.SUCCESS, title: "Success!", message: successMessage, }); } catch { setToast({ type: TOAST_TYPE.ERROR, title: "Error!", message: errorMessage, }); } }; // Usage example: toggleLock: async () => { if (is_locked) { await handlePageOperation( unlock, "Page unlocked successfully.", "Page could not be unlocked. Please try again later." ); } else { await handlePageOperation( lock, "Page locked successfully.", "Page could not be locked. Please try again later." ); } }
279-289: Consider using enum or const object for options order.The options order array uses string literals which could lead to typos. Consider defining these as constants or an enum for better type safety and maintainability.
const PAGE_ACTION_OPTIONS = { FULL_SCREEN: 'full-screen', COPY_MARKDOWN: 'copy-markdown', COPY_LINK: 'copy-link', MAKE_COPY: 'make-a-copy', MOVE: 'move', ARCHIVE_RESTORE: 'archive-restore', DELETE: 'delete', VERSION_HISTORY: 'version-history', EXPORT: 'export', } as const; // Usage optionsOrder={Object.values(PAGE_ACTION_OPTIONS)}web/core/store/pages/project-page.store.ts (1)
304-311: Add return type documentation for movePage methodThe JSDoc comment should include the
@returnstag to document the Promise return type.Suggested improvement:
/** * @description move a page to a new project * @param {string} workspaceSlug * @param {string} projectId * @param {string} pageId * @param {string} newProjectId + * @returns {Promise<void>} Promise that resolves when the page is moved */web/core/components/pages/dropdowns/actions.tsx (1)
146-148: Handle missing keys inoptionsOrderto prevent silent omissionsIf a key in
optionsOrderdoesn't match any item inMENU_ITEMS, it's currently filtered out silently. This could lead to unexpected menu behaviors. Consider adding a check to handle or log missing keys.Apply this diff to log a warning when a key is not found:
const arrangedOptions = optionsOrder - .map((key) => MENU_ITEMS.find((item) => item.key === key)) + .map((key) => { + const item = MENU_ITEMS.find((item) => item.key === key); + if (!item) { + console.warn(`Menu item with key '${key}' not found in MENU_ITEMS.`); + } + return item; + }) .filter((item) => !!item);web/core/components/pages/list/block-item-action.tsx (4)
63-70: Add error handling for clipboard copy failures.In the
copyLinkfunction, consider handling potential errors fromcopyUrlToClipboardto notify the user if the link copy operation fails.Apply this diff to include error handling:
copyLink: () => { copyUrlToClipboard(pageLink) .then(() => { setToast({ type: TOAST_TYPE.SUCCESS, title: "Link Copied!", message: "Page link copied to clipboard.", }); + }) + .catch(() => { + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to copy the page link. Please try again.", + }); + }); },
87-87: Implement themovefunction or remove it if unnecessary.The
movefunction is currently empty. Consider implementing it or removing it to prevent confusion or unintended usage.
142-172: SimplifytoggleLockfunction to reduce code duplication.The
toggleLockfunction contains duplicated code for handling lock and unlock operations. Refactor it to eliminate redundancy and improve maintainability.Apply this diff to refactor the function:
toggleLock: async () => { - if (is_locked) { - try { - await unlock(); - setToast({ - type: TOAST_TYPE.SUCCESS, - title: "Success!", - message: "Page unlocked successfully.", - }); - } catch { - setToast({ - type: TOAST_TYPE.ERROR, - title: "Error!", - message: "Page could not be unlocked. Please try again later.", - }); - } - } else { - try { - await lock(); - setToast({ - type: TOAST_TYPE.SUCCESS, - title: "Success!", - message: "Page locked successfully.", - }); - } catch { - setToast({ - type: TOAST_TYPE.ERROR, - title: "Error!", - message: "Page could not be locked. Please try again later.", - }); - } - } + const action = is_locked ? unlock : lock; + const actionMessage = is_locked ? "unlocked" : "locked"; + try { + await action(); + setToast({ + type: TOAST_TYPE.SUCCESS, + title: "Success!", + message: `Page ${actionMessage} successfully.`, + }); + } catch { + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: `Page could not be ${actionMessage}. Please try again later.`, + }); + } },
58-189: Handle potential state updates on unmounted components.Asynchronous operations in
pageOperationsmay attempt state updates after the component has unmounted, leading to memory leaks or warnings. Consider adding cleanup mechanisms or cancellation to prevent this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (24)
apiserver/plane/app/serializers/page.py(2 hunks)apiserver/plane/app/urls/page.py(2 hunks)apiserver/plane/app/views/__init__.py(1 hunks)apiserver/plane/app/views/page/base.py(2 hunks)packages/ui/src/dropdowns/context-menu/item.tsx(1 hunks)packages/ui/src/dropdowns/context-menu/root.tsx(1 hunks)packages/ui/src/dropdowns/custom-menu.tsx(2 hunks)web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(list)/header.tsx(3 hunks)web/ce/components/pages/index.ts(1 hunks)web/ce/components/pages/modals/index.ts(1 hunks)web/ce/components/pages/modals/move-page-modal.tsx(1 hunks)web/core/components/pages/dropdowns/actions.tsx(1 hunks)web/core/components/pages/dropdowns/index.ts(1 hunks)web/core/components/pages/dropdowns/quick-actions.tsx(0 hunks)web/core/components/pages/editor/header/extra-options.tsx(2 hunks)web/core/components/pages/editor/header/mobile-root.tsx(3 hunks)web/core/components/pages/editor/header/options-dropdown.tsx(3 hunks)web/core/components/pages/editor/header/root.tsx(2 hunks)web/core/components/pages/editor/page-root.tsx(1 hunks)web/core/components/pages/list/block-item-action.tsx(2 hunks)web/core/components/pages/list/block.tsx(1 hunks)web/core/services/page/project-page.service.ts(1 hunks)web/core/store/pages/page.ts(6 hunks)web/core/store/pages/project-page.store.ts(8 hunks)
💤 Files with no reviewable changes (1)
- web/core/components/pages/dropdowns/quick-actions.tsx
✅ Files skipped from review due to trivial changes (2)
- web/ce/components/pages/modals/index.ts
- web/core/components/pages/dropdowns/index.ts
🧰 Additional context used
🪛 Ruff
apiserver/plane/app/views/__init__.py
205-205: .page.base.PageDuplicateEndpoint imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🔇 Additional comments (34)
web/ce/components/pages/index.ts (1)
2-2: LGTM! Clean and consistent export.
The new export statement follows the established pattern and is properly placed between related modules.
Let's verify there are no circular dependencies between these modules:
✅ Verification successful
No circular dependencies detected - export is safe
The verification shows that:
- Both
editorandmodalsdirectories exist and contain their respective components - No cross-imports were found between these modules (no matches in the ripgrep search)
- The directory structure and file organization follows a clean separation of concerns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential circular dependencies between the modules
# Search for cross-imports between the modules
echo "Checking for circular dependencies..."
for module in "editor" "modals" "extra-actions"; do
echo "Checking imports in ./${module}..."
rg -l "from ['\"]\.\./(editor|modals|extra-actions)['\"]" "./web/ce/components/pages/${module}"
done
Length of output: 845
Script:
#!/bin/bash
# Let's verify the directory structure and imports more thoroughly
# First, check what directories actually exist
echo "Checking directory structure:"
fd --type d "^(editor|modals|extra-actions)$" web/ce/components/pages
# Then check imports in existing directories
echo -e "\nChecking imports in modals directory:"
rg -l "from ['\"]\.\./" web/ce/components/pages/modals
echo -e "\nChecking imports in editor directory:"
rg -l "from ['\"]\.\./" web/ce/components/pages/editor
# Let's also check the content of these directories
echo -e "\nListing files in modals directory:"
fd . web/ce/components/pages/modals --type f
echo -e "\nListing files in editor directory:"
fd . web/ce/components/pages/editor --type f
Length of output: 1147
web/ce/components/pages/modals/move-page-modal.tsx (1)
1-10: Verify permission checks in parent components
Ensure that this modal is only rendered when canCurrentUserMovePage is true in the parent components.
✅ Verification successful
Let me gather more information about the actual usage of MovePageModal to verify the permission checks.
Let me check the context around the MovePageModal usage to verify the permission checks.
Based on the gathered information, I can now provide the final response:
Permission checks are properly implemented
The MovePageModal is correctly guarded by permissions through:
- The
canCurrentUserMovePagepermission check in the store - The
pageConfig.canMoveflag in the PageActions component that controls the visibility of the move option - The move action is only rendered when
shouldRenderis true, which is tied topageConfig.canMove
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if MovePageModal usage is properly guarded by permissions
# Search for MovePageModal usage and verify permission checks
ast-grep --pattern 'const $_ = () => {
$$$
return (
$$$
<MovePageModal
$$$
/>
$$$
)
}'
# Search for any direct references to check implementation
rg -A 5 "canCurrentUserMovePage"
Length of output: 4368
Script:
#!/bin/bash
# Search for MovePageModal usage in the codebase
rg -A 10 "<MovePageModal"
# Also check for any move-related action handlers
rg -A 5 "handleMove|onMove"
Length of output: 1806
Script:
#!/bin/bash
# Check the context of actions.tsx where MovePageModal is used
rg -B 20 -A 5 "movePageModal" web/core/components/pages/dropdowns/actions.tsx
# Also check if there's any permission check in the component props
ast-grep --pattern 'type $_ = {
$$$
canMove: $_
$$$
}'
Length of output: 2673
packages/ui/src/dropdowns/context-menu/item.tsx (1)
39-55: LGTM! Clean implementation of flexible content rendering.
The conditional rendering pattern using the nullish coalescing operator is clean and maintainable. The fallback structure is well-organized and handles disabled states appropriately.
web/core/components/pages/editor/header/mobile-root.tsx (3)
2-2: LGTM: Clean import statement
The import of editor API types is well-organized and properly typed.
22-23: LGTM: Clean props destructuring
The props destructuring is well-organized and properly formatted for readability.
42-42: Verify PageExtraOptions usage across the codebase
The removal of handleDuplicatePage prop aligns with the refactoring. Let's verify this change is consistent across all usages of PageExtraOptions.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information. The results show that:
- No instances of
handleDuplicatePageprop were found in PageExtraOptions usage - All usages of PageExtraOptions consistently use the same three props:
editorRef,page, andreadOnlyEditorRef - The component is used in two locations:
mobile-root.tsxandroot.tsx, both with identical prop patterns
Props are consistent across all PageExtraOptions usages
The removal of handleDuplicatePage prop has been consistently applied across the codebase. All instances of PageExtraOptions now use the same set of props: editorRef, page, and readOnlyEditorRef.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of handleDuplicatePage prop in PageExtraOptions
# Expected: No instances should be found as this prop has been removed
# Search for PageExtraOptions usage with handleDuplicatePage
rg "PageExtraOptions.*handleDuplicatePage"
# Search for all PageExtraOptions usages for context
rg -A 2 "PageExtraOptions.*=.*"
Length of output: 1036
web/core/components/pages/editor/header/root.tsx (3)
Line range hint 13-22: LGTM! Props interface is well-structured.
The removal of handleDuplicatePage prop aligns with the architectural change to handle page duplication through a dedicated service, making the component more focused.
24-25: LGTM! Props destructuring is clean and consistent.
The destructuring aligns with the Props interface changes and maintains good code formatting.
56-56: Verify PageExtraOptions component compatibility.
The removal of handleDuplicatePage prop from PageExtraOptions needs verification to ensure the component handles page duplication correctly through the new service approach.
web/core/components/pages/editor/header/extra-options.tsx (2)
88-88: LGTM: Proper conditional editor reference handling
The PageOptionsDropdown component correctly receives the appropriate editor reference based on content editability, and the page prop is properly passed through.
25-25: Verify the removal of handleDuplicatePage prop
The removal of handleDuplicatePage from props aligns with the refactoring to move page duplication logic elsewhere. However, we should ensure this change doesn't break existing functionality.
✅ Verification successful
Removal of handleDuplicatePage prop is correct and safe
The removal of handleDuplicatePage prop is verified to be correct because:
- The page duplication logic is now handled directly through the
duplicatemethod from thepageobject - Both
block-item-action.tsxandoptions-dropdown.tsxuse the same pattern of callingduplicate()directly from the page object - The
PageOptionsDropdowncomponent correctly receives the necessary props (editorRefandpage) and uses theduplicatemethod from the page object in itspageOperations - The duplication functionality is properly guarded by
canCurrentUserDuplicatePagepermission check in thepageConfig
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that handleDuplicatePage is properly moved and not referenced elsewhere
# Look for any remaining references to handleDuplicatePage in related components
# Search for any remaining references to handleDuplicatePage
rg "handleDuplicatePage" --type ts --type tsx
# Look for the new location of page duplication logic
ast-grep --pattern 'duplicate($$$)'
Length of output: 290
Script:
#!/bin/bash
# Let's examine the implementation of duplicate() in both files
rg -A 10 "duplicate\(\)" --type ts --type tsx
# Also check if there are any references to PageDuplicateEndpoint
rg "PageDuplicateEndpoint" --type ts --type tsx
# Look for the Props type definition in the options-dropdown file
rg -A 10 "interface Props" web/core/components/pages/editor/header/options-dropdown.tsx
Length of output: 241
Script:
#!/bin/bash
# Let's try with just the file extension
rg -A 10 "duplicate\(\)" -g "*.ts" -g "*.tsx"
# Check for PageDuplicateEndpoint with correct file extensions
rg "PageDuplicateEndpoint" -g "*.ts" -g "*.tsx"
# Look at the options-dropdown file content
cat web/core/components/pages/editor/header/options-dropdown.tsx
Length of output: 10459
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(list)/header.tsx (1)
16-16: LGTM! Clean permission handling refactor.
The refactoring simplifies permission management by centralizing it through the useProjectPages hook, making the code more maintainable and easier to understand.
Also applies to: 28-28
apiserver/plane/app/urls/page.py (2)
11-11: LGTM! Import follows conventions
The addition of PageDuplicateEndpoint to the imports is properly grouped with related view imports.
115-119: URL pattern looks good, verify HTTP methods
The URL pattern follows RESTful conventions and maintains consistency with existing patterns. The path structure and parameter types are appropriate.
Let's verify the HTTP methods implemented in the PageDuplicateEndpoint:
✅ Verification successful
HTTP methods are properly implemented
The PageDuplicateEndpoint class correctly defines the POST method for page duplication, which is appropriate for creating a copy of an existing resource. The implementation follows RESTful conventions and inherits from BaseAPIView.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of PageDuplicateEndpoint to ensure it properly defines HTTP methods
# Search for the PageDuplicateEndpoint class definition and its methods
ast-grep --pattern 'class PageDuplicateEndpoint$$$'
Length of output: 53
Script:
#!/bin/bash
# Let's try a different approach to find the PageDuplicateEndpoint implementation
# First, search for files containing PageDuplicateEndpoint
rg -l "PageDuplicateEndpoint"
# Then search for the class definition and surrounding context
rg "class PageDuplicateEndpoint" -A 10
Length of output: 993
web/core/components/pages/editor/page-root.tsx (2)
Line range hint 43-52: Good error handling implementation!
The page fallback implementation with connection failure handling and async operations is well-structured. The separation of concerns between fetching and updating page descriptions is clean and maintainable.
Line range hint 89-127: Well-structured component with clean separation of concerns!
The component maintains a clear structure with:
- Proper version management
- Clean prop passing
- Type safety
- Modular subcomponents
The removal of page creation/duplication functionality hasn't impacted the core editing features.
packages/ui/src/dropdowns/context-menu/root.tsx (1)
14-15: Verify handling of optional title without customContent
While making title optional allows for more flexibility with custom content, there's a potential edge case where both title and customContent are undefined, which could lead to empty menu items.
Let's verify how the ContextMenuItem component handles this case:
Consider either:
- Making
customContentrequired whentitleis undefined, or - Adding runtime validation to ensure at least one of them is provided
Example type improvement:
type TContextMenuItem = {
key: string;
} & (
| { title: string; customContent?: React.ReactNode }
| { title?: never; customContent: React.ReactNode }
);apiserver/plane/app/views/__init__.py (1)
205-205: LGTM! The import follows the module's established pattern.
The addition of PageDuplicateEndpoint to the imports aligns with the module's pattern of exposing view-related classes through __init__.py.
🧰 Tools
🪛 Ruff
205-205: .page.base.PageDuplicateEndpoint imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
apiserver/plane/app/serializers/page.py (1)
66-67: Verify description field handling in page duplication
Since these fields are part of a larger refactoring that includes page duplication functionality, we should verify that the page duplication endpoint correctly handles these description fields.
Also applies to: 76-77
✅ Verification successful
Based on the code analysis, I can now provide a definitive response:
Page duplication properly handles description fields
The PageDuplicateEndpoint correctly handles both description and description_binary fields during page duplication. The implementation in apiserver/plane/app/views/page/base.py shows that:
- The original page data is retrieved with all fields including
description_binary - The serializer for creating the duplicate page explicitly includes both fields in its context:
"description": new_page_data.description,
"description_binary": new_page_data.description_binary- The PageSerializer properly saves these fields when creating the new page:
page = Page.objects.create(
**validated_data,
description=description,
description_binary=description_binary,
...
)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the page duplication endpoint to verify description field handling
ast-grep --pattern 'class PageDuplicateEndpoint:
$$$
def post($$$):
$$$'
# Search for any tests related to page duplication
rg -l "test.*page.*duplicate" --type python
Length of output: 159
Script:
#!/bin/bash
# Search for page duplication endpoint with more generic pattern
ast-grep --pattern 'class PageDuplicate$_:
$$$'
# Search for page duplication related code with ripgrep
rg "page.*duplicate" -i -A 5
# Search for test files without file type filter
rg -l "test.*page.*duplicate"
# Find Python files containing "duplicate" in their name
fd "duplicate.*\.py$"
Length of output: 166459
Script:
#!/bin/bash
# Search for page duplication endpoint implementation
rg -A 10 "class PageDuplicateEndpoint" --type py
# Search for page duplication tests
rg -A 5 "test.*page.*duplicate" --type py
# Search for page duplication serializer usage
rg "description.*binary" -A 5 -B 5 --type py
Length of output: 20201
web/core/services/page/project-page.service.ts (2)
162-168: LGTM! Clean implementation of the duplicate functionality.
The implementation follows the established patterns in the service class with proper error handling and consistent endpoint structure.
Let's verify the endpoint integration:
✅ Verification successful
Frontend and backend integration for page duplication is properly implemented
The endpoint integration is verified:
- Frontend service correctly calls
/api/workspaces/${workspaceSlug}/projects/${projectId}/pages/${pageId}/duplicate/ - Backend has matching URL route in
apiserver/plane/app/urls/page.py PageDuplicateEndpointview is implemented inapiserver/plane/app/views/page/base.pywith proper page duplication logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the duplicate endpoint integration
# Expected: Find the corresponding endpoint definition and route
# Check for the endpoint route definition
rg -A 2 "duplicate/" --glob "*.{py,ts,tsx}"
# Check for the endpoint class definition
ast-grep --pattern 'class $endpoint {
$$$
duplicate($$$) {
$$$
}
$$$
}'
Length of output: 710
Script:
#!/bin/bash
# Let's verify the endpoint implementation in the backend
rg -A 10 "class PageDuplicateEndpoint" --glob "*.py"
Length of output: 867
161-161: Ensure type safety for the duplicate operation.
Consider adding explicit type definitions for the duplicate operation's response in the TPage type to maintain strong type safety throughout the application.
Let's check the type definitions:
packages/ui/src/dropdowns/custom-menu.tsx (2)
57-57: LGTM! Clean callback handling using optional chaining.
The use of the optional chaining operator ?. is a good improvement for handling the optional onMenuClose callback.
212-212: LGTM! Consistent callback handling pattern.
The use of optional chaining for the onClick handler is consistent with the codebase's callback handling pattern and improves code readability.
web/core/store/pages/project-page.store.ts (1)
112-119: LGTM! Well-implemented permission check
The computed property implementation is robust with proper null safety and permission checking.
web/core/store/pages/page.ts (3)
28-28: LGTM: Interface additions are well-typed and consistent
The new interface members canCurrentUserMovePage and duplicate() are properly typed and align with the existing pattern.
Also applies to: 47-47
140-140: LGTM: MobX decorators are correctly applied
The new members are properly decorated with MobX annotations - computed for the permission check and action for the duplicate method.
Also applies to: 155-155
296-307: LGTM: Permission check implementation is consistent and well-documented
The canCurrentUserMovePage getter follows the established pattern for permission checks and correctly restricts page movement to owners and admins.
apiserver/plane/app/views/page/base.py (1)
129-132: 🛠️ Refactor suggestion
Add input validation for description fields
The description fields are directly passed from request.data without validation. Consider:
- Validating the size and format of description_binary
- Sanitizing the description HTML content
- Validating the description JSON structure
serializer = PageSerializer(
data=request.data,
context={
"project_id": project_id,
"owned_by_id": request.user.id,
- "description": request.data.get("description", {}),
- "description_binary": request.data.get(
- "description_binary", None
- ),
+ "description": self.validate_description(request.data.get("description", {})),
+ "description_binary": self.validate_binary(request.data.get("description_binary")),
"description_html": request.data.get(
"description_html", "<p></p>"
),
},
)web/core/components/pages/dropdowns/actions.tsx (2)
80-141: Ensure useMemo dependencies cover all used variables
Verify that all variables used within the useMemo hook are included in the dependencies array. If MENU_ITEMS relies on nested properties of pageConfig or pageOperations that might change, the memoization may not update as expected.
150-180: Excellent implementation of the PageActions component
The component is well-structured, and the use of React hooks and conditional rendering is handled effectively. The integration of menu items with state management provides a seamless user experience.
web/core/components/pages/list/block-item-action.tsx (4)
26-26: Verify that useParams provides correct parameters in client components.
By replacing props with useParams(), ensure that workspaceSlug and projectId are accurately retrieved in the client-side context, especially since useParams relies on the Next.js app router.
271-280: Verify that optionsOrder keys match PageActions options.
Ensure that the keys provided in optionsOrder correspond to valid action identifiers recognized by the PageActions component to prevent runtime errors.
88-88: Ensure pageLink has the correct URL format in openInNewTab.
In the openInNewTab function, confirm that pageLink includes a leading slash or the full URL to correctly open the page in a new tab.
Run the following script to verify the pageLink format:
✅ Verification successful
The pageLink construction is correct and safe
Looking at the code, pageLink is properly constructed in the useMemo hook with the correct format:
const pageLink = projectId ? `${workspaceSlug}/projects/${projectId}/pages/${id}` : `${workspaceSlug}/pages/${id}`;When used in openInNewTab, an additional leading slash is added:
openInNewTab: () => window.open(`/${pageLink}`, "_blank"),This ensures the URL will always have the correct format:
- With project:
/workspaceSlug/projects/projectId/pages/pageId - Without project:
/workspaceSlug/pages/pageId
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if `pageLink` starts with a slash.
# Test: Print the `pageLink` variable to ensure it has the correct format.
ast-grep --pattern '$$$const pageLink = $$$' web/core/components/pages/list/block-item-action.tsx | rg 'const pageLink'
# Expected output: `const pageLink = ...` showing the construction of `pageLink`.
Length of output: 121
Script:
#!/bin/bash
# Let's try to find how pageLink is defined in the file
cat web/core/components/pages/list/block-item-action.tsx
Length of output: 8190
33-54: Remove unused destructured properties from page.
It appears some properties destructured from page are not used in the component. Removing unused variables can improve readability and reduce clutter.
Run the following script to identify unused variables:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
web/core/components/pages/list/block-item-action.tsx (3)
7-7: LGTM! Good architectural improvement.The introduction of
usePageOperationshook and consolidation of imports improves code organization and maintainability by centralizing page-related operations.Also applies to: 9-9, 15-15
23-23: LGTM! Good props simplification.The changes effectively reduce prop drilling by:
- Removing unnecessary props (workspaceSlug, projectId)
- Leveraging the usePageOperations hook
- Properly destructuring page values
Consider destructuring pageOperations directly in the props destructuring for even cleaner code:
- const { pageId, parentRef } = props; - const { pageOperations } = usePageOperations(page); + const { pageId, parentRef } = props; + const { pageOperations: { toggleFavorite } } = usePageOperations(page);Also applies to: 27-28, 30-30
69-80: Consider extracting options array as a constant.While the implementation is good, the hardcoded array of options could be extracted into a named constant for better maintainability and reuse.
Consider applying this change:
+const PAGE_OPTIONS_ORDER = [ + "toggle-lock", + "toggle-access", + "open-in-new-tab", + "copy-link", + "make-a-copy", + "archive-restore", + "delete", +] as const; <PageActions - optionsOrder={[ - "toggle-lock", - "toggle-access", - "open-in-new-tab", - "copy-link", - "make-a-copy", - "archive-restore", - "delete", - ]} + optionsOrder={PAGE_OPTIONS_ORDER} page={page} parentRef={parentRef} />web/core/components/pages/editor/header/options-dropdown.tsx (1)
39-90: Consider improving the ToggleSwitch implementation.The ToggleSwitch component has an empty onChange handler while the actual toggle functionality is handled by the parent action. This could be confusing for maintenance.
Consider this improvement:
- <ToggleSwitch value={isFullWidth} onChange={() => {}} /> + <ToggleSwitch + value={isFullWidth} + onChange={(value) => handleFullWidth(value)} + />Then simplify the action:
- action: () => handleFullWidth(!isFullWidth), + action: undefined, // Let the ToggleSwitch handle the actionweb/core/components/pages/dropdowns/actions.tsx (2)
111-113: Simplify boolean expressions.The double negation (
!!) is redundant when the value is already being used in a boolean context.-title: !!archived_at ? "Restore" : "Archive", -icon: !!archived_at ? ArchiveRestoreIcon : ArchiveIcon, +title: archived_at ? "Restore" : "Archive", +icon: archived_at ? ArchiveRestoreIcon : ArchiveIcon,🧰 Tools
🪛 Biome
[error] 111-111: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
[error] 112-112: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
155-155: Improve null check for page ID.The nullish coalescing with empty string (
??) could be handled more gracefully with proper type validation.-pageId={page.id ?? ""} +pageId={page.id || throw new Error("Page ID is required")}This change would fail fast if the page ID is missing, making it easier to debug issues.
web/core/hooks/use-page-operations.ts (3)
47-55: RefactorcopyLinkto useasync/awaitfor consistency and error handlingCurrently, the
copyLinkfunction uses.then()for promise handling, whereas other functions in the hook useasync/await. Refactoring toasync/awaitwill ensure consistency across your codebase and allow proper error handling.Apply this diff to refactor the
copyLinkfunction:-copyLink: () => { - copyUrlToClipboard(pageLink).then(() => { +copyLink: async () => { + try { + await copyUrlToClipboard(pageLink); setToast({ type: TOAST_TYPE.SUCCESS, title: "Link Copied!", message: "Page link copied to clipboard.", }); + } catch { + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to copy the page link. Please try again.", + }); + } },
126-144: RefactortoggleFavoriteto useasync/awaitand handle errorsThe
toggleFavoritefunction uses.then()for promise handling and lacks error handling for rejected promises. Refactoring toasync/awaitwill improve code consistency and help manage errors effectively.Apply this diff to refactor the
toggleFavoritefunction:-toggleFavorite: () => { +toggleFavorite: async () => { if (is_favorite) { - removePageFromFavorites().then(() => + try { + await removePageFromFavorites(); setToast({ type: TOAST_TYPE.SUCCESS, title: "Success!", message: "Page removed from favorites.", }); + } catch { + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to remove page from favorites. Please try again.", + }); + } } else { - addToFavorites().then(() => + try { + await addToFavorites(); setToast({ type: TOAST_TYPE.SUCCESS, title: "Success!", message: "Page added to favorites.", }); + } catch { + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to add page to favorites. Please try again.", + }); + } } },
72-72: Remove or implement the emptymovefunctionThe
movefunction is currently empty and may cause confusion. If the move operation is not yet implemented, consider removing it frompageOperationsuntil it's ready. Alternatively, add a comment to indicate that it's a placeholder for future development.Apply this diff to remove the empty
movefunction:- move: async () => {},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
web/core/components/pages/dropdowns/actions.tsx(1 hunks)web/core/components/pages/editor/header/options-dropdown.tsx(3 hunks)web/core/components/pages/list/block-item-action.tsx(2 hunks)web/core/hooks/use-page-operations.ts(1 hunks)
🧰 Additional context used
🪛 Biome
web/core/components/pages/dropdowns/actions.tsx
[error] 111-111: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 112-112: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (8)
web/core/components/pages/list/block-item-action.tsx (1)
62-62: LGTM! Good encapsulation of favorite toggle logic.
The change simplifies the component by delegating favorite toggle functionality to the page operations hook, improving maintainability and reusability.
web/core/components/pages/editor/header/options-dropdown.tsx (4)
Line range hint 3-19: LGTM! Clean and well-organized imports and type definitions.
The imports are properly categorized and the Props interface has been simplified to focus on essential properties.
Also applies to: 21-24
27-38: LGTM! Clean component setup with proper state management.
The component follows React best practices with appropriate use of hooks and state management.
107-107: Verify the 'move' operation implementation.
The 'move' operation is included in the optionsOrder array, but based on previous review comments, its implementation might be incomplete.
Let's verify if the move operation has been implemented:
#!/bin/bash
# Search for move operation implementation in PageActions component
ast-grep --pattern 'case "move": { $$$'Line range hint 93-114: LGTM! Clean and well-structured rendering logic.
The component's rendering logic is well-organized with:
- Proper modal implementation for export functionality
- Clear hierarchy of menu options through optionsOrder
- Consistent props passing to child components
web/core/components/pages/dropdowns/actions.tsx (2)
1-49: LGTM! Well-structured type definitions and imports.
The type definitions and imports are well-organized, with clear separation of concerns and comprehensive documentation.
51-183: LGTM! Well-implemented component with good practices.
The component demonstrates good use of React patterns and practices:
- Proper state management with hooks
- Memoization for performance
- Type-safe implementations
- Clear separation of concerns
- Good error handling
The suggested improvements are minor optimizations to an already solid implementation.
🧰 Tools
🪛 Biome
[error] 111-111: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 112-112: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
web/core/hooks/use-page-operations.ts (1)
74-92: Verify the logic in toggleAccess for toggling page access levels
The toggleAccess function seems to have a potential logic issue with the access value and the corresponding action:
- The
changedPageTypevariable is set based onaccess === 0to determine the new state. - The condition
if (access === 0)callsmakePrivate(), which may not align with the intended access levels.
Please verify that:
- The
accessvalue correctly represents the current access state. - The functions
makePrivate()andmakePublic()are called appropriately to toggle the access. - The
changedPageTypeaccurately reflects the new state in success and error messages.
| @@ -54,7 +54,7 @@ const CustomMenu = (props: ICustomMenuDropdownProps) => { | |||
| if (referenceElement) referenceElement.focus(); | |||
| }; | |||
| const closeDropdown = () => { | |||
| isOpen && onMenuClose && onMenuClose(); | |||
| if (isOpen) onMenuClose?.(); | |||
There was a problem hiding this comment.
Wouldn't it be cleaner if its written as
if(isOpen && onMenuClose)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
web/core/components/pages/dropdowns/actions.tsx (3)
32-44: Consider using an enum for page actions instead of string literalsConverting
TPageActionsto an enum would provide better type safety and maintainability. This aligns with the existing pattern of using enums (e.g.,EPageAccess).export enum EPageActions { FULL_SCREEN = "full-screen", COPY_MARKDOWN = "copy-markdown", TOGGLE_LOCK = "toggle-lock", // ... other actions } export type TPageActions = `${EPageActions}`;
73-145: Extract MENU_ITEMS array outside componentConsider extracting the
MENU_ITEMSarray outside the component and using a factory function to create it. This would improve readability and reduce the cognitive load within the component.const createMenuItems = ( page: IPage, pageOperations: ReturnType<typeof usePageOperations>['pageOperations'], handlers: { setDeletePageModal: (value: boolean) => void, setMovePageModal: (value: boolean) => void } ) => [ { key: "toggle-lock", action: pageOperations.toggleLock, // ... rest of the items }, // ... other items ]; export const PageActions: React.FC<Props> = observer((props) => { // ... other code const menuItems = useMemo( () => createMenuItems(page, pageOperations, { setDeletePageModal, setMovePageModal }), [page, pageOperations] ); });🧰 Tools
🪛 Biome
[error] 113-113: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
[error] 114-114: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
166-170: Memoize event handlersThe click event handler for menu items should be memoized to prevent unnecessary re-renders.
const handleMenuItemClick = useCallback( (action?: () => void) => (e: React.MouseEvent) => { e.preventDefault(); e.stopPropagation(); action?.(); }, [] ); // Usage in JSX onClick={handleMenuItemClick(item.action)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/core/components/pages/dropdowns/actions.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
web/core/components/pages/dropdowns/actions.tsx
[error] 113-113: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 114-114: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
| title: !!archived_at ? "Restore" : "Archive", | ||
| icon: !!archived_at ? ArchiveRestoreIcon : ArchiveIcon, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify boolean conditions
Remove redundant double negation operators for better readability.
- title: !!archived_at ? "Restore" : "Archive",
- icon: !!archived_at ? ArchiveRestoreIcon : ArchiveIcon,
+ title: archived_at ? "Restore" : "Archive",
+ icon: archived_at ? ArchiveRestoreIcon : ArchiveIcon,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| title: !!archived_at ? "Restore" : "Archive", | |
| icon: !!archived_at ? ArchiveRestoreIcon : ArchiveIcon, | |
| title: archived_at ? "Restore" : "Archive", | |
| icon: archived_at ? ArchiveRestoreIcon : ArchiveIcon, |
🧰 Tools
🪛 Biome
[error] 113-113: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 114-114: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
web/core/components/pages/dropdowns/actions.tsx (4)
46-51: Add JSDoc documentation to Props type.Consider adding JSDoc documentation to improve code maintainability and developer experience.
+/** + * Props for the PageActions component + * @property {Array<TContextMenuItem & { key: TPageActions }>} extraOptions - Additional menu options to be appended + * @property {Array<TPageActions>} optionsOrder - Desired order of menu options + * @property {IPage} page - Page instance to operate on + * @property {React.RefObject<HTMLElement>} [parentRef] - Optional reference to parent element for context menu positioning + */ type Props = { extraOptions?: (TContextMenuItem & { key: TPageActions })[]; optionsOrder: TPageActions[]; page: IPage; parentRef?: React.RefObject<HTMLElement>; };
113-114: Remove redundant double negation.The double negation (
!!) is unnecessary asarchived_atwill already be coerced to a boolean in this context.- title: !!archived_at ? "Restore" : "Archive", - icon: !!archived_at ? ArchiveRestoreIcon : ArchiveIcon, + title: archived_at ? "Restore" : "Archive", + icon: archived_at ? ArchiveRestoreIcon : ArchiveIcon,🧰 Tools
🪛 Biome
[error] 113-113: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
[error] 114-114: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
73-145: Consider extracting MENU_ITEMS definition.The MENU_ITEMS array could be defined as a factory function outside the component to improve readability and reusability.
const createMenuItems = ( page: IPage, pageOperations: ReturnType<typeof usePageOperations>, handlers: { setDeletePageModal: (value: boolean) => void, setMovePageModal: (value: boolean) => void } ): (TContextMenuItem & { key: TPageActions })[] => [ // ... menu items definition ]; // Inside component: const MENU_ITEMS = useMemo( () => createMenuItems(page, pageOperations, { setDeletePageModal, setMovePageModal }), [/* ... dependencies ... */] );🧰 Tools
🪛 Biome
[error] 113-113: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
[error] 114-114: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
158-158: Improve error handling for page.id.The nullish coalescing to empty string (
??) could hide potential issues. Consider handling the case where page.id is undefined more explicitly.- <DeletePageModal isOpen={deletePageModal} onClose={() => setDeletePageModal(false)} pageId={page.id ?? ""} /> + {page.id && ( + <DeletePageModal + isOpen={deletePageModal} + onClose={() => setDeletePageModal(false)} + pageId={page.id} + /> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/core/components/pages/dropdowns/actions.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
web/core/components/pages/dropdowns/actions.tsx
[error] 113-113: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 114-114: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (2)
web/core/components/pages/dropdowns/actions.tsx (2)
1-45: LGTM! Well-structured imports and type definitions.
The code demonstrates good organization with properly grouped imports and comprehensive type definitions for page actions.
1-186: Overall implementation looks solid!
The PageActions component is well-implemented with:
- Strong TypeScript typing
- Proper performance optimizations
- Clean and maintainable code structure
- Good separation of concerns
The suggested improvements are minor refinements to an already solid implementation.
🧰 Tools
🪛 Biome
[error] 113-113: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 114-114: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
web/core/components/pages/dropdowns/actions.tsx (3)
61-71: Memoize derived values from page object.The destructured values from the page object should be memoized to prevent unnecessary re-renders when other parts of the component state change.
- const { - access, - archived_at, - is_locked, - canCurrentUserArchivePage, - canCurrentUserChangeAccess, - canCurrentUserDeletePage, - canCurrentUserDuplicatePage, - canCurrentUserLockPage, - canCurrentUserMovePage, - } = page; + const pageProperties = useMemo( + () => ({ + access: page.access, + archived_at: page.archived_at, + is_locked: page.is_locked, + canCurrentUserArchivePage: page.canCurrentUserArchivePage, + canCurrentUserChangeAccess: page.canCurrentUserChangeAccess, + canCurrentUserDeletePage: page.canCurrentUserDeletePage, + canCurrentUserDuplicatePage: page.canCurrentUserDuplicatePage, + canCurrentUserLockPage: page.canCurrentUserLockPage, + canCurrentUserMovePage: page.canCurrentUserMovePage, + }), + [page] + );
113-114: Remove redundant double negation operators.The double negation operators (
!!) are unnecessary as the values will already be coerced to boolean.- title: !!archived_at ? "Restore" : "Archive", - icon: !!archived_at ? ArchiveRestoreIcon : ArchiveIcon, + title: archived_at ? "Restore" : "Archive", + icon: archived_at ? ArchiveRestoreIcon : ArchiveIcon,🧰 Tools
🪛 Biome
[error] 113-113: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
[error] 114-114: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
169-173: Memoize event handler to prevent recreating function on each render.The click handler function is recreated on each render. Consider memoizing it for better performance.
+ const handleClick = useMemo( + () => (e: React.MouseEvent) => { + e.preventDefault(); + e.stopPropagation(); + item.action?.(); + }, + [item.action] + ); <CustomMenu.MenuItem key={item.key} - onClick={(e) => { - e.preventDefault(); - e.stopPropagation(); - item.action?.(); - }} + onClick={handleClick} className={cn("flex items-center gap-2", item.className)} disabled={item.disabled} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/core/components/pages/dropdowns/actions.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
web/core/components/pages/dropdowns/actions.tsx
[error] 113-113: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 114-114: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (2)
web/core/components/pages/dropdowns/actions.tsx (2)
1-51: LGTM! Well-structured type definitions and imports.
The type definitions are comprehensive and the imports are organized logically with clear comments.
150-156: LGTM! Clean and efficient options arrangement.
The arrangement logic is well-implemented with proper memoization and type safety.
This PR refactors the pages actions' menu and makes it a generic component that can be used anywhere in context of a page.
optionsOrder, which is an array of option keys used to arrange the options inside the menu in any manner according to the use case.Along with all these changes, a new endpoint to duplicate a page is introduced.
Summary by CodeRabbit
Release Notes
New Features
description,description_binary, anddescription_html) to improve page descriptions during creation and updates.PageActionscomponent for managing various page actions, including locking, archiving, and moving pages.usePageOperationsfor streamlined page management operations.Bug Fixes
Refactor
Chores