[PE-97] refactor: pages actions #6234
Conversation
|
Warning Rate limit exceeded@aaryan610 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis pull request introduces comprehensive changes across multiple components and services related to page management in a web application. The modifications focus on enhancing page serialization, adding new endpoints for page duplication, refactoring page actions, and improving user access control. Key changes include updating serializers to support new fields like Changes
Sequence DiagramsequenceDiagram
participant User
participant PageController
participant PageService
participant PageSerializer
participant Database
User->>PageController: Request Page Duplication
PageController->>PageService: duplicate(workspaceSlug, projectId, pageId)
PageService->>PageSerializer: Prepare New Page Data
PageSerializer-->>PageService: Serialized Page Data
PageService->>Database: Create New Page
Database-->>PageService: Page Created
PageService-->>PageController: Duplicated Page
PageController-->>User: Return Duplicated Page
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
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: 6
🔭 Outside diff range comments (1)
web/core/components/pages/editor/page-root.tsx (1)
Line range hint
41-48: Enhance connection failure handling.The current implementation of connection failure handling could be more robust.
Consider implementing a retry mechanism:
usePageFallback({ editorRef, fetchPageDescription: async () => { if (!page.id) return; - return await projectPageService.fetchDescriptionBinary(workspaceSlug, projectId, page.id); + const maxRetries = 3; + let retries = 0; + while (retries < maxRetries) { + try { + return await projectPageService.fetchDescriptionBinary(workspaceSlug, projectId, page.id); + } catch (error) { + retries++; + if (retries === maxRetries) throw error; + await new Promise(resolve => setTimeout(resolve, 1000 * retries)); + } + } }, hasConnectionFailed, updatePageDescription: async (data) => await updateDescription(data), });
🧹 Nitpick comments (14)
web/core/components/pages/dropdowns/actions.tsx (2)
119-120: Remove redundant double negation
Double negation (e.g., !!archived_at) is unnecessary because the condition will be implicitly coerced to a boolean.Proposed diff to remove the redundant double negations:
- title: !!archived_at ? "Restore" : "Archive", - icon: !!archived_at ? ArchiveRestoreIcon : ArchiveIcon, + title: archived_at ? "Restore" : "Archive", + icon: archived_at ? ArchiveRestoreIcon : ArchiveIcon,🧰 Tools
🪛 Biome (1.9.4)
[error] 119-119: 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] 120-120: 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)
125-129: Enhance delete logic with confirmation
Currently, the delete action sets a modal to confirm page deletion only after the page is already archived. Consider adding an extra confirmation step (e.g., an “Are you sure?”) for valuable pages or making the requirement for archival optional.web/core/hooks/use-page-operations.ts (3)
71-71: Implement the move operation
The move function is currently empty. This may cause confusion for other developers and degrade user experience if the operation button is visible but non-functional.Would you like a follow-up snippet to implement a move request or open a GitHub issue to track this?
74-91: Validate toggling access
When toggling public/private access, reflect on concurrency or potential conflicts if multiple users change the page’s access simultaneously. Consider adding checks (optimistic concurrency or server re-check) to confirm final status.
92-124: Reduce code repetition in archive/restore
The pattern for archiving/restoring is similar. Factor out the repeated code to improve readability. For example, you can create a helper function that accepts the message to send and the success/failure messages.web/ce/components/pages/modals/move-page-modal.tsx (1)
10-10: Implement the modal or add a placeholder message
Returning null can confuse end-users expecting a UI. Quickly add a small placeholder message, or if you plan to implement soon, place a TODO comment describing the modal’s upcoming features.packages/ui/src/dropdowns/context-menu/item.tsx (1)
39-55: LGTM! Consider adding type safety for customContentThe implementation of conditional rendering using the nullish coalescing operator is clean and maintains backward compatibility. The fallback to default content structure is well-organized.
Consider adding type safety for customContent in the TContextMenuItem interface:
type TContextMenuItem = { // ... existing properties customContent?: React.ReactNode; };web/core/hooks/use-page-filters.ts (1)
73-73: Fix incorrect JSDoc description for handleFontStyleThe JSDoc comment incorrectly states this is for
font_sizewhen it should be forfont_style.- * @description action to update font_size value - * @param {TEditorFontSize} value + * @description action to update font_style value + * @param {TEditorFontStyle} valueweb/core/components/pages/list/block-item-action.tsx (1)
71-82: Consider adding type safety for optionsOrderWhile the implementation is correct, consider defining an enum or const array for valid option values to prevent typos and ensure type safety.
// Add at the top of the file export const PAGE_ACTION_OPTIONS = [ 'toggle-lock', 'toggle-access', 'open-in-new-tab', 'copy-link', 'make-a-copy', 'archive-restore', 'delete', ] as const; type PageActionOption = typeof PAGE_ACTION_OPTIONS[number]; // Then use in props optionsOrder: PageActionOption[]web/core/components/pages/editor/header/extra-options.tsx (1)
Line range hint
35-48: Consider moving toast logic to usePageOperations hookThe toast messages for favorite actions should be centralized in the usePageOperations hook for consistency across the application.
// In usePageOperations hook const toggleFavorite = async () => { if (is_favorite) { await removePageFromFavorites(); setToast({ type: TOAST_TYPE.SUCCESS, title: "Success!", message: "Page removed from favorites.", }); } else { await addToFavorites(); setToast({ type: TOAST_TYPE.SUCCESS, title: "Success!", message: "Page added to favorites.", }); } };web/core/components/pages/editor/header/options-dropdown.tsx (1)
39-89: Well-structured menu options with performance optimization.The use of
useMemofor menu options is appropriate, and the type definitions are clear. However, the error handling in the copy markdown action could be improved.Consider enhancing the error handling in the copy markdown action:
{ key: "copy-markdown", action: () => { if (!editorRef) return; - copyTextToClipboard(editorRef.getMarkDown()).then(() => + copyTextToClipboard(editorRef.getMarkDown()) + .then(() => setToast({ type: TOAST_TYPE.SUCCESS, title: "Success!", message: "Markdown copied to clipboard.", }) + ) + .catch(() => + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to copy markdown to clipboard.", + }) + ); }, title: "Copy markdown", icon: Clipboard, shouldRender: true, }web/core/components/pages/editor/page-root.tsx (1)
39-39: Consider adding error boundary for editor state.The destructuring of page properties has been simplified, but error handling for editor state could be improved.
Consider wrapping the editor state management in an error boundary:
const EditorStateWrapper: React.FC<{ page: IPage }> = ({ page }) => { const { isContentEditable, updateDescription } = page; if (!page) return null; return ( <ErrorBoundary fallback={<EditorErrorState />}> {/* Editor content */} </ErrorBoundary> ); };web/core/hooks/use-collaborative-page-actions.tsx (1)
86-88: Remove debug console.log statementsThese debug statements should be removed before merging to production.
- console.log(realTimeStatelessMessageListener); const handleStatelessMessage = (message: { payload: TDocumentEventsClient }) => { - console.log("aaa", message);packages/ui/src/dropdowns/custom-menu.tsx (1)
57-58: Simplify closeDropdown callback.The
isOpencheck is redundant since optional chaining is used. The callback will only execute if defined.- if (isOpen) onMenuClose?.(); + onMenuClose?.(); setIsOpen(false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
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/editor/src/core/constants/document-collaborative-events.ts(1 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/(detail)/[pageId]/page.tsx(2 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(2 hunks)web/core/components/pages/editor/header/options-dropdown.tsx(2 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/hooks/use-collaborative-page-actions.tsx(4 hunks)web/core/hooks/use-page-filters.ts(2 hunks)web/core/hooks/use-page-operations.ts(1 hunks)web/core/services/page/project-page.service.ts(1 hunks)web/core/store/pages/page.ts(8 hunks)web/core/store/pages/project-page.store.ts(5 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
🪛 Biome (1.9.4)
web/core/components/pages/dropdowns/actions.tsx
[error] 119-119: 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] 120-120: 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/store/pages/page.ts
[error] 415-415: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 439-439: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (27)
web/core/components/pages/dropdowns/actions.tsx (3)
79-87: Check condition for lock toggling
When a page is locked, the user cannot modify it. Double-check that it’s always valid for the user to lock/unlock the page. If there are concurrency or permission checks beyond is_locked, consider adding them here to prevent potential conflicts.
164-194: Consider fallback for non-rendering parentRef
You conditionally render the only when parentRef is present (line 168). If parentRef is optional, consider how the user accesses these actions without that context (e.g., in mobile layouts). Provide a fallback strategy or ensure parentRef is not null when the component is used.
34-47: Ensure consistent naming of action keys in TPageActions
All enum-like keys (e.g., "full-screen", "copy-markdown") are well-defined, but kindly confirm they match the associated operations in the codebase to avoid confusion.
Run this script to search for references throughout the repository and confirm that each TPageActions key is invoked properly:
web/ce/components/pages/index.ts (1)
2-2: Verify the exported modals
Re-exporting modules is a good way to streamline imports. Ensure that “modals” includes everything needed (e.g., MovePageModal). If items are missing, consider explicitly exporting them for clarity.
packages/editor/src/core/constants/document-collaborative-events.ts (1)
6-7: LGTM! Verify event handlers implementation
The new collaborative events for public/private access changes follow the established pattern and support the real-time sync functionality.
Let's verify the event handlers implementation:
✅ Verification successful
Event handlers are properly implemented and connected
The verification shows a complete implementation chain for the new collaborative events:
- Events are properly defined in
document-collaborative-events.ts - Events are triggered in
use-page-operations.tswhen changing page access - Handlers are registered in
use-collaborative-page-actions.tsxconnecting topage.makePublic()andpage.makePrivate()methods - Error handling is in place with appropriate error messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for handlers of these new collaborative events
rg -A 5 "made-public|made-private" --type typescript
Length of output: 89
Script:
#!/bin/bash
# Search for handlers of these new collaborative events without file type restriction
rg -A 5 "made-public|made-private"
Length of output: 389
Script:
#!/bin/bash
# Search for make-public/make-private event handlers
rg -A 5 "make-public|make-private"
Length of output: 2037
Script:
#!/bin/bash
# Find all TypeScript files that might handle these events
fd -e ts -e tsx | xargs rg -l "made-public|made-private|make-public|make-private"
Length of output: 239
Script:
#!/bin/bash
# Check the page class methods for makePublic and makePrivate implementations
ast-grep --pattern 'makePublic($_) { $$$ }'
ast-grep --pattern 'makePrivate($_) { $$$ }'
Length of output: 92
web/core/components/pages/list/block.tsx (1)
43-43: Verify removal of workspace and project props
The removal of workspaceSlug and projectId props from BlockItemAction aligns with the action refactoring. However, we should verify that these props aren't needed for any operations within the component.
Let's check the BlockItemAction implementation:
✅ Verification successful
From the output, I can see the implementation of BlockItemAction component and its usage. The component's props type definition only includes pageId and parentRef, and in the implementation, it uses usePage hook to get the page details. The workspace and project information are not used in the component directly. Let me verify one more thing to be completely sure.
From the analysis of the code, I can now generate the final response. The BlockItemAction component's implementation shows that it doesn't use workspaceSlug or projectId directly. Instead, it uses the usePage hook to get page details and usePageOperations for actions. The workspace and project context is handled internally by these hooks when needed. The removal of these props is safe.
Props removal is correctly implemented
The removal of workspaceSlug and projectId props from BlockItemAction is correct as:
- The component uses
usePagehook which provides all necessary page data - Page operations are handled through
usePageOperationshook which internally manages workspace/project context - All required data for actions and displays is obtained through these hooks without needing direct access to workspace/project IDs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if BlockItemAction uses workspaceSlug or projectId internally
ast-grep --pattern 'const BlockItemAction = $_({ $_ }) => {
$$$
workspaceSlug
$$$
}'
ast-grep --pattern 'const BlockItemAction = $_({ $_ }) => {
$$$
projectId
$$$
}'
Length of output: 181
Script:
#!/bin/bash
# Let's first locate the BlockItemAction component file
fd "BlockItemAction" --type f
# Then examine its implementation and usages
rg "BlockItemAction" -A 5 -B 5
# Also check for any hooks or utilities that might use these props
rg "useWorkspace|useProject" --type typescript
Length of output: 2641
Script:
#!/bin/bash
# Check the usePage hook implementation to see if it internally needs workspace/project
rg "usePage" -A 10 --type ts
# Also check the full BlockItemAction implementation
cat web/core/components/pages/list/block-item-action.tsx
Length of output: 32787
web/core/components/pages/editor/header/mobile-root.tsx (1)
30-36: Verify null handling in child components
Ensure that PageSummaryPopover and PageExtraOptions components properly handle null editorRef.
✅ Verification successful
Let's search for these components first to locate their definitions and analyze how they handle the editorRef prop.
Let's examine how these components internally handle the editorRef to verify their null handling.
Let's examine the full implementation of these components to understand how they handle the editorRef prop.
We need to check how PageContentBrowser and PageInfoPopover handle the editorRef since they receive it as a prop from our components under review.
Based on the code analysis, I can now provide the final response:
Both components handle null editorRef appropriately
The components handle the null editorRef case safely:
-
PageSummaryPopoveronly passes the editorRef toPageContentBrowser, which safely handles null:- Uses optional chaining in
editorRef?.onHeadingChange - Null coalescing in
editorRef?.getHeadings() ?? [] - No direct access to editorRef properties without checks
- Uses optional chaining in
-
PageExtraOptionsonly passes editorRef to child components and doesn't directly access any properties, making it safe for null values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check null handling in components
ast-grep --pattern 'const PageSummaryPopover = $_({ editorRef, $_ }) => {
$$$
editorRef?.
$$$
}'
ast-grep --pattern 'const PageExtraOptions = $_({ editorRef, $_ }) => {
$$$
editorRef?.
$$$
}'
Length of output: 207
Script:
#!/bin/bash
# Search for PageSummaryPopover and PageExtraOptions components and their usage of editorRef
rg -U "PageSummaryPopover|PageExtraOptions" -A 10 -B 10
# Search for files containing these components
fd "PageSummaryPopover|PageExtraOptions" --type f
Length of output: 11110
Script:
#!/bin/bash
# Let's check how these components use editorRef internally
rg -U "editorRef\." -B 5 -A 5 "web/core/components/pages/editor/summary/popover.tsx" "web/core/components/pages/editor/header/extra-options.tsx"
# Also check for any conditional rendering or null checks
rg -U "editorRef(\?|\s*&&)" -B 5 -A 5 "web/core/components/pages/editor/summary/popover.tsx" "web/core/components/pages/editor/header/extra-options.tsx"
Length of output: 293
Script:
#!/bin/bash
# Let's get the full content of these components to analyze their implementation
echo "=== PageSummaryPopover ==="
cat web/core/components/pages/editor/summary/popover.tsx
echo -e "\n=== PageExtraOptions ==="
cat web/core/components/pages/editor/header/extra-options.tsx
Length of output: 5658
Script:
#!/bin/bash
# Let's check the PageContentBrowser and PageInfoPopover components for editorRef handling
echo "=== PageContentBrowser ==="
cat web/core/components/pages/editor/summary/content-browser.tsx
echo -e "\n=== PageInfoPopover ==="
rg -l "PageInfoPopover" --type file
cat $(rg -l "PageInfoPopover" --type file)
Length of output: 2227
web/core/components/pages/editor/header/root.tsx (2)
27-30: Good improvement in ref handling!
The introduction of resolvedEditorRef improves code clarity and reduces repeated access to editorRef.current. The early null check is a good practice.
53-53: Verify the removal of page duplication handling
The removal of handleDuplicatePage prop suggests this functionality has been moved. Let's verify the new location of this functionality.
Also applies to: 57-57
✅ Verification successful
Page duplication functionality is properly maintained
The page duplication functionality has been moved to a more organized location and is still fully functional. It's now handled through:
- The
usePageOperationshook inweb/core/hooks/use-page-operations.ts - Implemented in the Page store at
web/core/store/pages/page.ts - Accessible through the page actions dropdown in
web/core/components/pages/dropdowns/actions.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for page duplication handling in the new common actions file
rg -A 5 "duplicate.*page" web/core/components/pages/
Length of output: 54
Script:
#!/bin/bash
# Let's try a broader search for duplication-related functionality
rg -i "duplicat" web/core/components/pages/
# Also search for any references to copying pages
rg -i "copy.*page" web/core/components/pages/
# Let's also check the PageExtraOptions component specifically
ast-grep --pattern 'const PageExtraOptions = $_'
Length of output: 493
Script:
#!/bin/bash
# Let's check the pageOperations to see where the duplicate functionality is implemented
rg -A 5 "pageOperations" web/core/components/pages/
# Also check the implementation of canCurrentUserDuplicatePage
ast-grep --pattern 'const canCurrentUserDuplicatePage = $_'
Length of output: 4813
Script:
#!/bin/bash
# Let's check the usePageOperations hook implementation
rg -A 5 "usePageOperations" web/core/
# And check for the duplicate operation implementation
rg -A 5 "duplicate.*=" web/core/
Length of output: 12836
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx (1)
25-25: Good implementation of access control!
The addition of canCurrentUserAccessPage check improves security and user experience. The error handling provides clear feedback and a helpful navigation option.
Let's verify the consistency of access control implementation:
Also applies to: 47-47
✅ Verification successful
Access control implementation is correctly implemented and consistent
The canCurrentUserAccessPage check is properly implemented in the codebase:
- It's defined in the page store (
web/core/store/pages/page.ts) - The logic checks if the page is public or if the current user is the owner
- The implementation in the page component correctly uses this check for access control
- No traces of the old
canCurrentUserEditPagepattern were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of canCurrentUserAccessPage to ensure consistent implementation
rg "canCurrentUserAccessPage" --type typescript
# Check for any remaining old access control patterns
rg "canCurrentUserEditPage" --type typescript
Length of output: 163
Script:
#!/bin/bash
# Search for other uses of canCurrentUserAccessPage without file type restriction
rg "canCurrentUserAccessPage"
# Check for any remaining old access control patterns
rg "canCurrentUserEditPage"
# Let's also check the specific file to understand the context better
cat "web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx"
Length of output: 3121
Script:
#!/bin/bash
# Let's check the implementation of canCurrentUserAccessPage in the store
cat "web/core/store/pages/page.ts"
Length of output: 18887
web/core/hooks/use-page-filters.ts (1)
26-34: LGTM! Well-implemented performance optimizations
The use of useMemo and useCallback hooks is appropriate here, with correct dependency arrays. This will prevent unnecessary recalculations and function recreations.
Also applies to: 36-80
web/core/components/pages/list/block-item-action.tsx (1)
27-30: LGTM! Clean integration with usePageOperations
The refactoring successfully centralizes page operations and simplifies the component's interface.
Also applies to: 64-64
web/core/components/pages/editor/header/extra-options.tsx (2)
86-87: LGTM! Clean prop passing
The simplified prop passing to PageInfoPopover and PageOptionsDropdown components is cleaner.
19-19: Consider using non-null assertion for editorRef
The type change from React.RefObject<EditorRefApi> to EditorRefApi | null might lead to runtime errors if not properly handled in child components.
apiserver/plane/app/urls/page.py (1)
82-86: LGTM! URL pattern follows REST conventions.
The new duplicate endpoint is well-structured and consistent with existing patterns. Consider verifying the response format matches other endpoints.
✅ Verification successful
Let me gather more information about the response formats by checking the serializers.
Let me try a different approach to find the serializer implementation.
Let me check the complete fields list and the base serializer to fully understand the response format.
Based on the code analysis, I can now generate the final response:
Response format is consistent across endpoints ✓
The duplicate endpoint uses the same PageSerializer as other page endpoints, ensuring consistent response structure with standard fields like id, name, access, labels, etc. The serializer inherits from BaseSerializer maintaining the common field patterns across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify response format consistency across endpoints
# Compare response structure of duplicate endpoint with other page endpoints
# Get sample responses from both endpoints
echo "Comparing response formats between duplicate and create endpoints..."
rg -A 10 "def post\(" "apiserver/plane/app/views/page/"
Length of output: 2036
Script:
#!/bin/bash
# Check the serializer implementations to verify response format consistency
ast-grep --pattern 'class PageSerializer' -A 20
Length of output: 49
Script:
#!/bin/bash
# Search for PageSerializer using ripgrep for better context
rg -l "class PageSerializer"
rg "class PageSerializer" -A 15
Length of output: 1298
Script:
#!/bin/bash
# Get complete fields list of PageSerializer and check BaseSerializer
rg "class PageSerializer" -A 30 apiserver/plane/app/serializers/page.py
rg "class BaseSerializer" -A 10
Length of output: 4056
web/core/components/pages/editor/header/options-dropdown.tsx (1)
100-116: Verify the order of menu options matches design requirements.
The options order array defines a specific sequence for menu items. Please ensure this matches the intended UX design.
✅ Verification successful
Based on the gathered information, I can now provide a final response. The code shows that the menu options order is explicitly defined in the type system and matches the implementation. The order in the component aligns with all available actions defined in the TPageActions type, and the sequence appears to be intentionally structured for a logical user flow (from common actions like "full-screen" and "copy" to more critical actions like "delete" near the end).
The menu options order is correctly implemented and follows a logical progression.
- Common actions ("full-screen", "copy-markdown", "copy-link") are placed at the top for easy access
- Access control options ("toggle-lock", "toggle-access") are grouped together
- Destructive actions ("delete") and important but less frequently used options ("version-history") are placed towards the end
- The order matches all available actions defined in the
TPageActionstype
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any design documentation or specs
# that might define the menu order
echo "Searching for design documentation..."
rg -l "menu.*order|action.*order" "docs/"
Length of output: 172
Script:
#!/bin/bash
# Let's check for any related component files or tests that might define the menu order
echo "Searching for related component files and tests..."
fd -e tsx -e ts "options|menu|actions" --type f
# Let's also check for any constants or configuration files
echo -e "\nSearching for menu order definitions in the codebase..."
rg -i "menu.*options.*order|action.*options.*order"
Length of output: 68805
Script:
#!/bin/bash
# Let's check for any design or menu order specifications in the component and its related files
echo "Checking the options-dropdown component and related files..."
rg -A 10 "EXTRA_MENU_OPTIONS|PageActions" web/core/components/pages/editor/header/
# Let's also check for any test files that might verify the menu order
echo -e "\nChecking for related test files..."
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts "options-dropdown|page-actions"
Length of output: 3594
Script:
#!/bin/bash
# Let's check the PageActions component to understand the menu options implementation
echo "Checking PageActions component implementation..."
rg -A 20 "type TPageActions|interface.*PageActions" web/core/components/pages/
# Let's also check for any constants or types that define these actions
echo -e "\nChecking for action type definitions..."
rg "TPageActions.*=|enum.*PageActions" web/core/components/pages/
Length of output: 2276
web/core/hooks/use-collaborative-page-actions.tsx (2)
17-20: LGTM! Props interface improves type safety
Good refactoring to use a Props interface instead of individual parameters. This makes the hook more maintainable and easier to extend.
46-53: Verify real-time sync behavior for access changes
The new collaborative actions for making pages public/private look good. However, we should verify the real-time sync behavior works as expected.
packages/ui/src/dropdowns/context-menu/root.tsx (1)
14-15: LGTM! Enhanced menu item flexibility
Good enhancement to support both custom content and optional titles. This makes the context menu more versatile.
✅ Verification successful
Let me search for title usage and type definitions to verify the impact of making the title optional.
Let me try searching without file type restrictions since the previous commands had issues with file type recognition.
Based on the search results, I can now generate the final response:
Making title optional is safe and improves flexibility
The changes to make title optional and add customContent in the context menu are safe. The codebase search shows that there are no direct dependencies requiring a mandatory title prop for context menu items. This enhancement allows for more versatile menu items that can use either a simple title or custom content.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any existing menu items rely on required title
rg "title: .*," --type typescript
Length of output: 70
Script:
#!/bin/bash
# Search for title usage in ts/tsx files
rg "title: .*," -t ts -t tsx
# Search for any direct usage of title prop in context menu components
ast-grep --pattern 'title={$_}'
# Look for type definitions and interfaces related to context menu
rg "interface.*ContextMenu|type.*ContextMenu" -t ts -t tsx -A 3
Length of output: 186
Script:
#!/bin/bash
# Search for title usage in all files
rg "title: .*,"
# Search for any direct usage of title prop
rg "title=[\{\"']"
# Look for type definitions and interfaces related to context menu
rg "interface.*ContextMenu|type.*ContextMenu" -A 3
Length of output: 66344
apiserver/plane/app/views/__init__.py (1)
158-158: LGTM! Clean import addition
The PageDuplicateEndpoint import follows the existing pattern and organization.
web/core/services/page/project-page.service.ts (1)
162-168: LGTM! Well-structured implementation.
The duplicate method follows the service's established patterns for API calls and error handling, maintaining consistency across the codebase.
packages/ui/src/dropdowns/custom-menu.tsx (1)
219-221: LGTM! Clean callback implementation.
The use of optional chaining for the onClick handler improves code readability while maintaining the same functionality.
web/core/store/pages/project-page.store.ts (1)
21-21: LGTM: Role permissions constant is well-defined
The constant clearly defines which roles can create pages, improving code maintainability.
web/core/store/pages/page.ts (3)
18-26: LGTM: Permission properties are well-structured
The permission-related properties are clearly defined and follow a consistent naming pattern.
605-612: LGTM: Page duplication is well-implemented
The duplicate method is concise and follows the established pattern of error handling.
218-220: Verify the simplified access control logic
The new canCurrentUserAccessPage implementation only checks if the page is public or if the user is the owner. This might be too permissive compared to the previous implementation that considered project role permissions.
✅ Verification successful
Let me gather more information about the access control implementation to make a more informed decision.
Based on the gathered information, I can now provide the final response:
The simplified access control is appropriate and secure
The current implementation of canCurrentUserAccessPage that checks for public access or ownership is correct because:
- Pages have only two access levels (PUBLIC=0, PRIVATE=1) as defined in
EPageAccess - Other sensitive operations (duplicate, lock, change access, archive, delete) still maintain strict permission checks requiring owner or admin role
- The page access check is consistently used in the UI to prevent unauthorized access, showing a "Page not found" message when access is denied
- This matches the application's page visibility model where pages are either public (viewable by all) or private (viewable only by owners)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential security implications of the simplified access control
# Test: Search for usages of canCurrentUserAccessPage to ensure the simplified logic is safe
# Search for all usages of canCurrentUserAccessPage
rg -A 5 'canCurrentUserAccessPage'
# Search for any existing access control patterns
ast-grep --pattern 'access === EPageAccess.PUBLIC || $_'
Length of output: 3088
Script:
#!/bin/bash
# Check for other access control related methods and their implementations
ast-grep --pattern 'canCurrentUser$_() {
$$$
}'
# Search for the definition of isCurrentUserOwner
rg -A 5 'isCurrentUserOwner'
# Look for EPageAccess enum definition and usage
rg -A 5 'EPageAccess'
Length of output: 16787
apiserver/plane/app/views/page/base.py (1)
124-126: LGTM: Description context is well-structured
The addition of description-related context parameters improves the page creation process.
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
web/core/components/pages/editor/header/root.tsx (1)
Line range hint
44-49: Maintain consistency in editor ref usageThere are inconsistencies in how the editor ref is accessed throughout the component.
Consider standardizing the usage:
-editorRef={editorRef.current} +editorRef={resolvedEditorRef} -editorRef={editorRef?.current} +editorRef={resolvedEditorRef}
🧹 Nitpick comments (14)
packages/ui/src/dropdowns/context-menu/item.tsx (1)
39-55: Consider adding ARIA attributes for custom content.When using customContent, ensure that appropriate ARIA attributes are maintained for accessibility.
- {item.customContent ?? ( + {item.customContent ? ( + <div role="menuitem" aria-disabled={item.disabled}> + {item.customContent} + </div> + ) : (packages/ui/src/dropdowns/context-menu/root.tsx (1)
14-15: Add JSDoc comments for type definition.Consider adding documentation to clarify the relationship between
titleandcustomContent.export type TContextMenuItem = { + /** Custom content to override default rendering. When provided, title and description are ignored */ customContent?: React.ReactNode; + /** Title of the menu item. Required if customContent is not provided */ title?: string;web/core/components/pages/dropdowns/actions.tsx (3)
3-5: Rename or clarify usage of useMemo
Consider adding a descriptive name or comment to clarify the purpose of this memoized block, ensuring contributors understand why it’s necessary.
79-88: Use descriptive condition or invert if-block
While this block is correct, you might consider using an if-else with a more descriptive condition (e.g., if (!canCurrentUserLockPage) return null) to avoid nesting and improve clarity.
165-194: Consider a fallback or empty state for arrangedOptions
When no valid menu items remain, the UI might render an empty menu. Consider providing a fallback or disabling the menu in such a scenario to enhance UX.web/core/hooks/use-page-operations.ts (2)
56-70: Handle partial successes
If the duplication partially succeeds (e.g., some sub-operation fails), ensure that the user is informed with more granular error reporting.
71-71: Unused placeholder function
The “move” method is defined but not implemented. This is acceptable if it’s a known TODO, but consider removing or providing a clear comment.web/ce/components/pages/modals/move-page-modal.tsx (1)
10-10: Implement or remove stub component
Returning null indicates the modal is not yet implemented. If it’s a placeholder, add a TODO comment or partial UI to clarify intended future functionality.packages/editor/src/core/constants/document-collaborative-events.ts (1)
6-7: Add descriptive comments for new events
Including short JSDoc-style comments describing “make-public” and “make-private” helps future contributors understand the intended usage/flow.web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx (1)
Line range hint
47-60: Consider differentiating between "not found" and "no access" messagesWhile combining both cases provides a clean UI, it might be helpful for users to know whether the page doesn't exist or they lack permissions.
- <div className="h-full w-full flex flex-col items-center justify-center"> - <h3 className="text-lg font-semibold text-center">Page not found</h3> - <p className="text-sm text-custom-text-200 text-center mt-3"> - The page you are trying to access doesn{"'"}t exist or you don{"'"}t have permission to view it. - </p> + <div className="h-full w-full flex flex-col items-center justify-center"> + <h3 className="text-lg font-semibold text-center"> + {pageDetailsError ? "Page not found" : "Access denied"} + </h3> + <p className="text-sm text-custom-text-200 text-center mt-3"> + {pageDetailsError + ? "The page you are trying to access doesn't exist." + : "You don't have permission to view this page."} + </p>web/core/hooks/use-page-filters.ts (1)
36-44: Consider adding error handling for config updatesWhile the handleUpdateConfig implementation is clean, it might benefit from error handling for edge cases.
const handleUpdateConfig = useCallback( (payload: Partial<TPagesPersonalizationConfig>) => { + try { setPagesConfig({ ...(pagesConfig ?? DEFAULT_PERSONALIZATION_VALUES), ...payload, }); + } catch (error) { + console.error('Failed to update page config:', error); + // Consider adding error notification here + } }, [pagesConfig, setPagesConfig] );web/core/components/pages/editor/header/options-dropdown.tsx (1)
100-116: Consider extracting options order to a constantThe options order array could be extracted to a constant or configuration file for better maintainability, especially since it might be reused across different components.
+ const PAGE_OPTIONS_ORDER = [ + "full-screen", + "copy-markdown", + "copy-link", + "toggle-lock", + "toggle-access", + "make-a-copy", + "archive-restore", + "delete", + "version-history", + "export", + ] as const; return ( <> <ExportPageModal editorRef={editorRef} isOpen={isExportModalOpen} onClose={() => setIsExportModalOpen(false)} pageTitle={name ?? ""} /> <PageActions editorRef={editorRef} extraOptions={EXTRA_MENU_OPTIONS} - optionsOrder={[ - "full-screen", - "copy-markdown", - "copy-link", - "toggle-lock", - "toggle-access", - "make-a-copy", - "archive-restore", - "delete", - "version-history", - "export", - ]} + optionsOrder={PAGE_OPTIONS_ORDER} page={page} /> </> );web/core/hooks/use-collaborative-page-actions.tsx (1)
86-86: Remove debug console.log statementsThese console.log statements appear to be debugging code that should be removed before merging.
- console.log(realTimeStatelessMessageListener); const handleStatelessMessage = (message: { payload: TDocumentEventsClient }) => { - console.log("aaa", message);Also applies to: 88-88
apiserver/plane/app/views/page/base.py (1)
124-127: Add validation for description formatsConsider adding validation to ensure consistency between different description formats.
Add validation in the serializer:
def validate(self, data): description = self.context.get('description') description_html = self.context.get('description_html') description_binary = self.context.get('description_binary') if description_binary and not description_html: raise serializers.ValidationError("description_html is required when description_binary is provided") return data
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
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/editor/src/core/constants/document-collaborative-events.ts(1 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/(detail)/[pageId]/page.tsx(2 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(2 hunks)web/core/components/pages/editor/header/options-dropdown.tsx(2 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/hooks/use-collaborative-page-actions.tsx(4 hunks)web/core/hooks/use-page-filters.ts(2 hunks)web/core/hooks/use-page-operations.ts(1 hunks)web/core/services/page/project-page.service.ts(1 hunks)web/core/store/pages/page.ts(8 hunks)web/core/store/pages/project-page.store.ts(5 hunks)
💤 Files with no reviewable changes (1)
- web/core/components/pages/dropdowns/quick-actions.tsx
✅ Files skipped from review due to trivial changes (3)
- web/ce/components/pages/modals/index.ts
- web/ce/components/pages/index.ts
- web/core/components/pages/dropdowns/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
web/core/store/pages/page.ts
[error] 415-415: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 439-439: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
web/core/components/pages/dropdowns/actions.tsx
[error] 119-119: 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] 120-120: 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 (27)
packages/ui/src/dropdowns/context-menu/item.tsx (1)
39-55: LGTM! Clean implementation of flexible content rendering.
The conditional rendering approach using the nullish coalescing operator is well-implemented, making the component more reusable while maintaining backward compatibility.
packages/ui/src/dropdowns/context-menu/root.tsx (1)
14-15: Verify handling of items without title.
Making the title optional could lead to empty menu items if neither title nor customContent is provided.
web/core/components/pages/editor/header/extra-options.tsx (2)
24-24: LGTM! Props cleanup aligns with refactoring goals
The removal of handleDuplicatePage from props destructuring is consistent with the PR's objective to refactor page actions into a common actions file.
86-87: Verify null handling in child components
The direct passing of editorRef (which can be null) to child components requires proper null handling in PageInfoPopover and PageOptionsDropdown.
✅ Verification successful
Proper null handling confirmed in child components
Both components properly handle the nullable editorRef:
PageInfoPopover: Uses optional chaining (editorRef?.getDocumentInfo()) with a fallback valuePageOptionsDropdown: Has explicit null check (if (!editorRef) return) before using the ref
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for null handling in the child components
echo "Checking PageInfoPopover implementation:"
ast-grep --pattern 'interface Props { editorRef: $_ }' | rg -A 10 "PageInfoPopover"
echo "Checking PageOptionsDropdown implementation:"
ast-grep --pattern 'interface Props { editorRef: $_ }' | rg -A 10 "PageOptionsDropdown"
Length of output: 361
Script:
#!/bin/bash
# Let's try a different approach to find these components and their implementations
echo "Searching for PageInfoPopover component:"
fd PageInfoPopover.tsx
rg -l "PageInfoPopover"
echo -e "\nSearching for PageOptionsDropdown component:"
fd PageOptionsDropdown.tsx
rg -l "PageOptionsDropdown"
echo -e "\nChecking implementations:"
rg -A 10 "export.*PageInfoPopover|interface.*PageInfoPopover|type.*PageInfoPopover"
rg -A 10 "export.*PageOptionsDropdown|interface.*PageOptionsDropdown|type.*PageOptionsDropdown"
Length of output: 2987
Script:
#!/bin/bash
# Let's check the Props interface and editorRef usage in both components
echo "Checking Props and editorRef usage in PageInfoPopover:"
rg -B 5 -A 15 "interface Props|type Props" web/core/components/pages/editor/header/info-popover.tsx
echo -e "\nChecking Props and editorRef usage in PageOptionsDropdown:"
rg -B 5 -A 15 "interface Props|type Props" web/core/components/pages/editor/header/options-dropdown.tsx
# Let's also check how editorRef is used within these components
echo -e "\nChecking editorRef usage in PageInfoPopover:"
rg "editorRef" web/core/components/pages/editor/header/info-popover.tsx
echo -e "\nChecking editorRef usage in PageOptionsDropdown:"
rg "editorRef" web/core/components/pages/editor/header/options-dropdown.tsx
Length of output: 2637
web/core/components/pages/dropdowns/actions.tsx (1)
124-129: Clarify the delete condition
This logic checks if the page is already archived before rendering “Delete.” Ensure that this is the intended behavior and not an accidental limitation.
✅ Verification successful
Let me gather more context about the delete functionality and archived pages.
Let me try a different approach to gather information about the deletion and archival logic.
Based on the codebase analysis, I can now provide a clear response about the delete condition.
The delete condition is correct and intentional
The logic requiring a page to be archived (!!archived_at) before showing the delete option is part of a two-step deletion process where pages must first be archived before they can be permanently deleted. This is a common pattern for data protection, allowing users to recover accidentally archived pages before permanent deletion.
Evidence from the codebase:
- The
archived_atfield is consistently used across the application (pages, issues, projects, etc.) as a soft-delete mechanism - The page operations hook (
use-page-operations.ts) shows a clear separation between archive and delete operations - The archive functionality is reversible, with methods for both archiving and restoring pages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Script to verify references to 'setDeletePageModal(true)' ensuring
# consistent usage of archived_at. Searching the codebase:
rg -A 5 $'setDeletePageModal\\(true\\)'
Length of output: 503
Script:
#!/bin/bash
# Search for archived_at usage in relation to page deletion
rg -B 5 -A 5 "archived_at.*delete" --type typescript
# Search for the DeletePageModal component to understand the deletion logic
ast-grep --pattern 'const DeletePageModal = $_'
# Look for any comments or documentation about page deletion and archival
rg -i "delete.*page" README.md docs/ 2>/dev/null
Length of output: 178
Script:
#!/bin/bash
# Search for archived_at usage in relation to page deletion
rg -B 5 -A 5 "archived_at"
# Search for DeletePageModal component usage and definition
rg -B 5 -A 5 "DeletePageModal"
# Look for any archive-related functionality
rg -B 5 -A 5 "canCurrentUserDeletePage"
Length of output: 65782
web/core/hooks/use-page-operations.ts (1)
125-143: Check for race conditions in favorite toggling
When removing or adding a page to favorites in quick succession, there’s a slight chance of competing requests. Consider a locking mechanism or immediate UI feedback.
web/core/components/pages/list/block.tsx (1)
43-43: Verify BlockItemAction functionality after prop removal
The removal of workspaceSlug and projectId props from BlockItemAction aligns with the refactoring goal, but we should verify that these values are properly accessible where needed.
✅ Verification successful
Let me check the implementation of usePageOperations hook to understand how it handles these values.
Let me try searching with correct paths and different patterns.
From the results, I can see that usePageOperations hook internally uses useParams to get workspaceSlug and projectId. Let me verify the complete implementation of BlockItemAction to ensure it's using this hook correctly.
Removal of workspaceSlug and projectId props is correct
The verification confirms that the removal of workspaceSlug and projectId props from BlockItemAction is safe because:
- These values are obtained through
useParamshook inside theusePageOperationshook - The component correctly uses
usePageOperationswith just thepageobject - All required operations (favorite, access control, etc.) work through the
pageOperationsobject
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if BlockItemAction or its dependencies still reference workspaceSlug/projectId
ast-grep --pattern 'workspaceSlug' web/core/components/pages/list/block-item-action.tsx
ast-grep --pattern 'projectId' web/core/components/pages/list/block-item-action.tsx
# Check if these values are now provided through a context or store
rg -A 5 'usePageOperations|PageContext' web/core/components/pages/list/block-item-action.tsx
Length of output: 661
Script:
#!/bin/bash
# Check usePageOperations hook implementation
rg -A 10 'export.*usePageOperations' hooks/use-page-operations.ts
# Check if the page object contains workspaceSlug and projectId
ast-grep --pattern 'type Page = {
$$$
}'
Length of output: 181
Script:
#!/bin/bash
# Find and check the usePageOperations hook implementation
fd -e ts -e tsx usePageOperations
rg -A 10 'export.*usePageOperations'
# Check the page type definition
fd -e ts -e tsx -e d.ts "types" --exec rg -l "type Page"
rg -A 10 "type Page"
# Check if BlockItemAction uses any context or store to get these values
rg -A 5 "useContext|useSelector" web/core/components/pages/list/block-item-action.tsx
Length of output: 1661
Script:
#!/bin/bash
# Check the complete implementation of BlockItemAction
cat web/core/components/pages/list/block-item-action.tsx
# Also check if there are any other components using BlockItemAction to ensure the change is consistent
rg -l "BlockItemAction" web/core/components/
Length of output: 2869
web/core/components/pages/editor/header/mobile-root.tsx (1)
12-12: LGTM: Improved type safety with explicit null handling
The change from React.RefObject<EditorRefApi> to EditorRefApi | null makes null handling more explicit and removes a layer of indirection.
web/core/components/pages/editor/header/root.tsx (1)
27-30: LGTM: Good optimization with resolvedEditorRef
Caching editorRef.current in resolvedEditorRef and adding an early return improves code clarity and potentially performance by avoiding multiple .current accesses.
web/core/components/pages/list/block-item-action.tsx (2)
71-82: Great implementation of configurable actions!
The new PageActions component with optionsOrder prop perfectly aligns with the PR objective of making actions scalable and reorderable. The explicit ordering of actions improves maintainability and flexibility.
27-30: Nice abstraction of page operations
The introduction of usePageOperations hook and simplified toggle operation is a good example of consolidating actions into a common location, reducing code duplication.
Also applies to: 64-64
web/core/hooks/use-page-filters.ts (1)
26-34: Good use of useMemo for config values
The memoization of config values prevents unnecessary recalculations and improves performance.
apiserver/plane/app/urls/page.py (2)
11-11: LGTM: Import added for new endpoint
The PageDuplicateEndpoint import is properly added to support the new duplication functionality.
82-86: Verify HTTP methods for duplicate endpoint
The URL pattern is well-structured and follows the existing pattern. However, the HTTP methods allowed for this endpoint should be explicitly defined.
✅ Verification successful
HTTP methods are properly defined for the duplicate endpoint
The PageDuplicateEndpoint class explicitly defines only the POST method with proper role-based permissions using the @allow_permission decorator. This is appropriate for a duplication operation which should be handled as a POST request.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the view implementation for allowed HTTP methods
ast-grep --pattern 'class PageDuplicateEndpoint$$$'
Length of output: 53
Script:
#!/bin/bash
# Search for PageDuplicateEndpoint class definition using ripgrep
rg -l "class PageDuplicateEndpoint" --type py
# After finding the file, show the class implementation
rg "class PageDuplicateEndpoint" -A 20 --type py
Length of output: 1778
web/core/components/pages/editor/header/options-dropdown.tsx (1)
39-89: Well-structured menu options with proper memoization
The EXTRA_MENU_OPTIONS array is properly memoized with complete dependencies. The structure provides good type safety with TContextMenuItem & TPageActions.
web/core/components/pages/editor/page-root.tsx (2)
Line range hint 41-48: LGTM: Robust page fallback implementation
The page fallback mechanism is well-implemented with proper async/await handling and clear separation of concerns.
39-39: Verify error handling in description updates
The destructuring of page properties has been simplified, but ensure proper error handling exists in the updateDescription callback.
✅ Verification successful
Let me check the implementation of updateDescription to understand the error handling.
Based on the results, I can now generate the final response as I can see the complete error handling flow. The updateDescription is properly wrapped in try-catch blocks in the store implementation (web/core/store/pages/page.ts) and propagates through a well-defined service layer.
Error handling for description updates is properly implemented ✅
The updateDescription method has comprehensive error handling:
- Input validation for required parameters (workspaceSlug, projectId, id)
- Try-catch block around the service call
- Error state handling through
runInAction
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for error handling in page description updates
ast-grep --pattern 'updateDescription($$$)'
Length of output: 169
Script:
#!/bin/bash
# Search for updateDescription implementation
ast-grep --pattern 'updateDescription = $_'
# Also check for any try-catch blocks around updateDescription calls
rg "updateDescription" -B 2 -A 2
Length of output: 3074
web/core/hooks/use-collaborative-page-actions.tsx (2)
17-20: LGTM: Props type improves type safety
The introduction of a Props type with proper TypeScript definitions enhances type safety and makes the component's requirements more explicit.
46-53: LGTM: New collaborative actions for access control
The addition of make-public and make-private actions aligns with the PR objective of enabling real-time synchronization for access setting changes.
apiserver/plane/app/views/__init__.py (1)
158-158: LGTM: Clean import addition
The PageDuplicateEndpoint import is properly organized within the existing page-related imports.
apiserver/plane/app/serializers/page.py (2)
67-69: LGTM: Proper field assignment
The description fields are correctly passed to Page.objects.create.
57-59: Verify required fields and add validation
The new description fields are extracted from context without validation. Consider:
- Validating if these fields are required
- Adding type checking or sanitization
- Handling cases where fields might be missing from context
web/core/services/page/project-page.service.ts (1)
161-168: LGTM! The duplicate method is well-implemented.
The implementation follows the established patterns in the service:
- Consistent error handling
- RESTful endpoint structure
- Type safety with Promise return type
packages/ui/src/dropdowns/custom-menu.tsx (1)
57-57: LGTM! Improved callback handling with optional chaining.
The changes simplify the code by using optional chaining (?.) for callback invocations, which is a cleaner approach than explicit null checks while maintaining the same functionality.
Also applies to: 219-219
web/core/store/pages/project-page.store.ts (1)
21-22: LGTM! Well-defined permission constant and its usage.
The ROLE_PERMISSIONS_TO_CREATE_PAGE constant improves code maintainability and the updated canCurrentUserCreatePage implementation is more explicit about the allowed roles.
Also applies to: 114-114
web/core/store/pages/page.ts (1)
410-427: LGTM: Robust implementation of access modification methods
The implementation includes proper error handling with state rollback and conditional synchronization with the backend.
Also applies to: 434-451
🧰 Tools
🪛 Biome (1.9.4)
[error] 415-415: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
apiserver/plane/app/views/page/base.py (1)
Line range hint 269-282: LGTM: Robust error handling and permission checks
The implementation includes proper error handling with clear error messages and appropriate permission checks.
| class PageDuplicateEndpoint(BaseAPIView): | ||
| @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) | ||
| def post(self, request, slug, project_id, page_id): | ||
| page = Page.objects.filter( | ||
| pk=page_id, workspace__slug=slug, projects__id=project_id | ||
| ).values() | ||
| new_page_data = list(page)[0] | ||
| new_page_data.name = f"{new_page_data.name} (Copy)" | ||
|
|
||
| serializer = PageSerializer( | ||
| data=new_page_data, | ||
| context={ | ||
| "project_id": project_id, | ||
| "owned_by_id": request.user.id, | ||
| "description": new_page_data.description, | ||
| "description_binary": new_page_data.description_binary, | ||
| "description_html": new_page_data.description_html, | ||
| }, | ||
| ) | ||
|
|
||
| if serializer.is_valid(): | ||
| serializer.save() | ||
| # capture the page transaction | ||
| page_transaction.delay(request.data, None, serializer.data["id"]) | ||
| page = Page.objects.get(pk=serializer.data["id"]) | ||
| serializer = PageDetailSerializer(page) | ||
| return Response(serializer.data, status=status.HTTP_201_CREATED) | ||
| return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) |
There was a problem hiding this comment.
Fix critical issues in page duplication endpoint
The current implementation has several issues that need to be addressed:
- Unsafe dictionary access
- Missing transaction handling
- Potential race conditions
Consider this improved implementation:
@allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST])
def post(self, request, slug, project_id, page_id):
+ try:
+ with transaction.atomic():
+ page = Page.objects.select_for_update().get(
+ pk=page_id,
+ workspace__slug=slug,
+ projects__id=project_id
+ )
+
+ new_page_data = model_to_dict(page)
+ new_page_data['name'] = f"{page.name} (Copy)"
serializer = PageSerializer(
data=new_page_data,
context={
"project_id": project_id,
"owned_by_id": request.user.id,
"description": page.description,
"description_binary": page.description_binary,
"description_html": page.description_html,
},
)
if serializer.is_valid():
new_page = serializer.save()
page_transaction.delay(new_page_data, None, new_page.id)
return Response(
PageDetailSerializer(new_page).data,
status=status.HTTP_201_CREATED
)
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
+ except Page.DoesNotExist:
+ return Response(
+ {"error": "Page not found"},
+ status=status.HTTP_404_NOT_FOUND
+ )📝 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.
| class PageDuplicateEndpoint(BaseAPIView): | |
| @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) | |
| def post(self, request, slug, project_id, page_id): | |
| page = Page.objects.filter( | |
| pk=page_id, workspace__slug=slug, projects__id=project_id | |
| ).values() | |
| new_page_data = list(page)[0] | |
| new_page_data.name = f"{new_page_data.name} (Copy)" | |
| serializer = PageSerializer( | |
| data=new_page_data, | |
| context={ | |
| "project_id": project_id, | |
| "owned_by_id": request.user.id, | |
| "description": new_page_data.description, | |
| "description_binary": new_page_data.description_binary, | |
| "description_html": new_page_data.description_html, | |
| }, | |
| ) | |
| if serializer.is_valid(): | |
| serializer.save() | |
| # capture the page transaction | |
| page_transaction.delay(request.data, None, serializer.data["id"]) | |
| page = Page.objects.get(pk=serializer.data["id"]) | |
| serializer = PageDetailSerializer(page) | |
| return Response(serializer.data, status=status.HTTP_201_CREATED) | |
| return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) | |
| class PageDuplicateEndpoint(BaseAPIView): | |
| @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) | |
| def post(self, request, slug, project_id, page_id): | |
| try: | |
| with transaction.atomic(): | |
| page = Page.objects.select_for_update().get( | |
| pk=page_id, | |
| workspace__slug=slug, | |
| projects__id=project_id | |
| ) | |
| new_page_data = model_to_dict(page) | |
| new_page_data['name'] = f"{page.name} (Copy)" | |
| serializer = PageSerializer( | |
| data=new_page_data, | |
| context={ | |
| "project_id": project_id, | |
| "owned_by_id": request.user.id, | |
| "description": page.description, | |
| "description_binary": page.description_binary, | |
| "description_html": page.description_html, | |
| }, | |
| ) | |
| if serializer.is_valid(): | |
| new_page = serializer.save() | |
| page_transaction.delay(new_page_data, None, new_page.id) | |
| return Response( | |
| PageDetailSerializer(new_page).data, | |
| status=status.HTTP_201_CREATED | |
| ) | |
| return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) | |
| except Page.DoesNotExist: | |
| return Response( | |
| {"error": "Page not found"}, | |
| status=status.HTTP_404_NOT_FOUND | |
| ) |
Description
This PR refactors the pages' actions menu in the following ways-
Other improvements-
Type of Change
Summary by CodeRabbit
Release Notes
New Features
description,description_binary, anddescription_htmlto enhance page creation and detail serialization.Improvements
Bug Fixes
Chores