-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WEB-2293] feat: pages version history #5417
Conversation
WalkthroughThe update introduces enhanced functionalities for managing page versions across various components and services. Key changes include the addition of Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant API
participant Service
User->>Browser: Requests Page Version
Browser->>API: GET Page Version Endpoint
API->>Service: fetchVersionById()
Service-->>API: Return Version Data
API-->>Browser: Send Version Data
Browser-->>User: Display Page Version
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (3)
web/core/components/pages/version/editor.tsx (1)
1-2
: Ensure consistent import usage.Consider grouping related imports together for better readability. For instance, group all imports from
@plane/*
together.web/core/components/pages/editor/page-root.tsx (2)
1-3
: Ensure consistent import usage.Consider grouping related imports together for better readability. For instance, group all imports from
@plane/*
together.
34-34
: Consider renamingisVersionsOverlayOpen
.The state variable
isVersionsOverlayOpen
could be renamed toversionsOverlayVisible
for consistency withsidePeekVisible
.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (20)
- apiserver/plane/app/serializers/init.py (1 hunks)
- apiserver/plane/app/serializers/page.py (1 hunks)
- apiserver/plane/app/views/page/version.py (2 hunks)
- packages/editor/src/core/hooks/use-editor.ts (1 hunks)
- packages/editor/src/core/types/editor.ts (1 hunks)
- packages/types/src/pages.d.ts (1 hunks)
- web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx (1 hunks)
- web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/header.tsx (4 hunks)
- web/core/components/pages/editor/header/options-dropdown.tsx (3 hunks)
- web/core/components/pages/editor/page-root.tsx (3 hunks)
- web/core/components/pages/index.ts (1 hunks)
- web/core/components/pages/version/editor.tsx (1 hunks)
- web/core/components/pages/version/index.ts (1 hunks)
- web/core/components/pages/version/main-content.tsx (1 hunks)
- web/core/components/pages/version/root.tsx (1 hunks)
- web/core/components/pages/version/sidebar-list-item.tsx (1 hunks)
- web/core/components/pages/version/sidebar.tsx (1 hunks)
- web/core/hooks/use-page-description.ts (2 hunks)
- web/core/services/page/index.ts (1 hunks)
- web/core/services/page/project-page-version.service.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- web/core/components/pages/version/index.ts
Additional context used
Ruff
apiserver/plane/app/serializers/__init__.py
95-95:
.page.PageVersionDetailSerializer
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
Additional comments not posted (29)
web/core/services/page/index.ts (1)
1-2
: Enhancement: Exportingproject-page-version.service
.The addition of the export for
project-page-version.service
improves the modularity and usability of the services related to pages. This aligns with the PR's objective to manage page versions.web/core/components/pages/index.ts (1)
7-7
: Enhancement: Exportingversion
component.The addition of the export for the
version
component enhances the modularity and usability of the components related to pages. This allows other parts of the application to access theversion
functionality directly.apiserver/plane/app/views/page/version.py (2)
30-30
: Verify the usage ofPageVersionDetailSerializer
.Ensure that
PageVersionDetailSerializer
is correctly implemented and provides the necessary details for a single page version.Run the following script to verify the serializer usage:
Verification successful
PageVersionDetailSerializer is correctly implemented.
The
PageVersionDetailSerializer
class is defined with appropriate fields and read-only fields to handle a single page version, ensuring it provides the necessary details.
- Located in
apiserver/plane/app/serializers/page.py
- Fields include
id
,workspace
,page
,last_saved_at
, and others.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `PageVersionDetailSerializer`. # Test: Search for the serializer definition. Expect: Correct implementation. rg --type python 'class PageVersionDetailSerializer'Length of output: 85
Script:
#!/bin/bash # Description: Verify the implementation of `PageVersionDetailSerializer`. # Test: Search for the serializer definition. Expect: Correct implementation. rg --type py 'class PageVersionDetailSerializer'Length of output: 141
Script:
#!/bin/bash # Description: Extract the implementation details of `PageVersionDetailSerializer`. # Test: Retrieve the class implementation to verify its correctness. rg -A 20 'class PageVersionDetailSerializer' apiserver/plane/app/serializers/page.pyLength of output: 612
17-19
: Verify the implementation of the@allow_permission
decorator.Ensure that the
@allow_permission
decorator is correctly implemented and that it effectively manages the roles specified.Run the following script to verify the decorator implementation:
Verification successful
The
@allow_permission
decorator is correctly implemented.The
allow_permission
decorator is defined inapiserver/plane/app/permissions/base.py
and appears to be implemented to manage permissions based on roles and other conditions. This aligns with its intended use in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `@allow_permission` decorator. # Test: Search for the decorator definition. Expect: Correct implementation. rg --type python '@allow_permission'Length of output: 67
Script:
#!/bin/bash # Description: Verify the implementation of the `@allow_permission` decorator. # Test: Search for the decorator definition. Expect: Correct implementation. rg '@allow_permission'Length of output: 11112
Script:
#!/bin/bash # Description: Locate the definition of the `allow_permission` decorator. # Test: Search for the definition of `allow_permission`. Expect: Locate the function or decorator definition. rg 'def allow_permission'Length of output: 148
web/core/components/pages/version/sidebar-list-item.tsx (2)
1-1
: Verify the use of MobX in the component.Ensure that MobX is correctly utilized in the component and that it effectively manages the state.
Run the following script to verify the MobX usage:
Verification successful
MobX Usage Verified in Component
The
PlaneVersionsSidebarListItem
component correctly utilizes MobX by importing and using theobserver
frommobx-react
. This indicates that MobX is being used to manage state in this component as expected.
- The import statement for
observer
is present inweb/core/components/pages/version/sidebar-list-item.tsx
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the use of MobX in the component. # Test: Search for MobX usage in the component. Expect: Correct implementation. rg --type tsx 'observer'Length of output: 52
Script:
#!/bin/bash # Description: Verify the use of MobX in the component. # Test: Search for MobX usage in the component. Expect: Correct implementation. rg --glob '*.tsx' 'observer'Length of output: 181805
19-47
: Verify the rendering logic and props handling.Ensure that the component correctly handles props and renders the UI as expected.
Run the following script to verify the rendering logic:
Verification successful
Rendering Logic and Props Handling Verified
The
PlaneVersionsSidebarListItem
component is correctly used within thePageVersionsSidebar
component. The propshref
,isActive
, andversion
are appropriately passed and handled, ensuring the component renders as expected. No issues found with the rendering logic or props handling.
- File:
web/core/components/pages/version/sidebar.tsx
- Component Usage: Verified
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the rendering logic and props handling in the component. # Test: Search for the component usage. Expect: Correct rendering logic. rg --type tsx 'PlaneVersionsSidebarListItem'Length of output: 72
Script:
#!/bin/bash # Description: Verify the rendering logic and props handling in the component. # Test: Search for the component usage. Expect: Correct rendering logic. rg 'PlaneVersionsSidebarListItem'Length of output: 373
Script:
#!/bin/bash # Description: Extract usage of PlaneVersionsSidebarListItem in sidebar.tsx to verify props handling. # Extract the relevant lines around the usage of PlaneVersionsSidebarListItem rg -A 10 -B 10 'PlaneVersionsSidebarListItem' web/core/components/pages/version/sidebar.tsxLength of output: 1419
packages/types/src/pages.d.ts (1)
52-66
: Verify the completeness and accuracy of theTPageVersion
type definition.Ensure that all properties are correctly defined and optional fields are appropriately marked.
Run the following script to verify the type definition:
web/core/components/pages/version/root.tsx (1)
33-38
: Verify the usage of thecn
helper function.Ensure that the
cn
function is correctly handling conditional class names, especially whenisOpen
changes.Run the following script to verify the
cn
function implementation:Verification successful
The
cn
function correctly handles conditional class names. The implementation usesclsx
andtwMerge
, which are suitable for managing conditional class names based on theisOpen
state. The function is defined inpackages/editor/src/core/helpers/common.ts
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `cn` function. # Test: Search for the `cn` function definition. Expect: Correct handling of conditional class names. rg --type javascript --type typescript --type tsx -A 5 $'function cn'Length of output: 105
Script:
#!/bin/bash # Description: Verify the implementation of the `cn` function. # Test: Search for the `cn` function definition. Expect: Correct handling of conditional class names. rg --type js --type ts --type tsx -A 5 $'function cn'Length of output: 82
Script:
#!/bin/bash # Description: Verify the implementation of the `cn` function. # Test: Search for the `cn` function definition. Expect: Correct handling of conditional class names. rg --type javascript --type typescript --type typescriptreact -A 5 $'function cn'Length of output: 117
Script:
#!/bin/bash # Description: Verify the implementation of the `cn` function. # Test: Search for the `cn` function definition. Expect: Correct handling of conditional class names. rg --type js --type ts --type tsx -A 5 $'function cn'Length of output: 82
Script:
#!/bin/bash # Description: Verify the implementation of the `cn` function. # Test: Search for the `cn` function definition using file extensions. Expect: Correct handling of conditional class names. rg --iglob '*.js' --iglob '*.ts' --iglob '*.tsx' -A 5 $'function cn'Length of output: 549
packages/editor/src/core/types/editor.ts (1)
9-9
: Verify the impact of theclearEditor
method signature change.The
clearEditor
method now accepts an optionalemitUpdate
parameter. Ensure that all usages of this method are updated to handle the new parameter correctly.Run the following script to verify the usage of the
clearEditor
method:web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx (1)
67-67
: Verify the impact of therelative
class addition.The addition of the
relative
class to thediv
element may affect the layout of its child components. Ensure that this change does not negatively impact the visual rendering of the page.Run the following script to verify the usage of the
relative
class in the context of the component's styling:Verification successful
No Negative Impact from
relative
Class AdditionThe addition of the
relative
class does not have any specific CSS or LESS rules affecting it in the codebase, indicating no negative impact on the layout. The class likely follows the default behavior of positioning elements relatively.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the `relative` class addition on the layout. # Test: Search for CSS rules affecting the `relative` class. Expect: No negative impact on layout. rg --type css --type scss --type less -A 5 $'\.relative'Length of output: 86
Script:
#!/bin/bash # Description: Verify the impact of the `relative` class addition on the layout. # Test: Search for CSS rules affecting the `relative` class. Exclude SCSS due to unrecognized file type error. rg --type css --type less -A 5 '\.relative'Length of output: 45
web/core/components/pages/version/sidebar.tsx (3)
1-11
: Imports are well-organized and used correctly.All imported modules are utilized in the component.
13-74
: Component structure is logical and well-organized.The component is well-structured, with clear separation of concerns and logical flow.
25-70
: Logic for generating version links and rendering is correct.The use of hooks and conditional rendering is appropriate and functions as intended.
web/core/components/pages/version/main-content.tsx (3)
1-11
: Imports are well-organized and used correctly.All imported modules are utilized in the component.
13-82
: Component structure is logical and well-organized.The component is well-structured, with clear separation of concerns and logical flow.
26-79
: Logic for data fetching and version restoration is correct.The use of SWR and state management is appropriate and functions as intended.
web/core/components/pages/version/editor.tsx (2)
21-22
: Verify the use ofobserver
.Ensure that the use of
observer
frommobx-react
is necessary. If the component does not observe any MobX state, it might be redundant.
94-110
: Ensure proper handling ofinitialValue
.The
initialValue
for the editor defaults to"<p></p>"
if no description is available. Verify that this behavior aligns with user expectations.web/core/components/pages/editor/page-root.tsx (2)
82-89
: Verify the effect's dependency array.The
useEffect
hook depends onversion
. Ensure that this is the only dependency needed, as missing dependencies could lead to unexpected behavior.
91-94
: Ensure proper routing on overlay close.Verify that the routing logic in
handleCloseVersionsOverlay
correctly handles all edge cases, such as invalid or missing page IDs.apiserver/plane/app/serializers/page.py (2)
170-184
: Explicitly list serializer fields.The
PageVersionSerializer
now explicitly lists fields, which improves clarity and control. Ensure that all necessary fields are included.
187-203
: Verify field necessity inPageVersionDetailSerializer
.The
PageVersionDetailSerializer
includes additional fields. Verify that all fields are necessary for the intended use cases.web/core/components/pages/editor/header/options-dropdown.tsx (2)
4-4
: Ensure consistent use of navigation hooks.The addition of
usePathname
,useRouter
, anduseSearchParams
is appropriate for managing routing and search parameters. Ensure these hooks are used consistently throughout the component to maintain clarity and functionality.
152-163
: Verify the URL update logic for version history.The logic for updating the URL with the
version=current
query parameter looks correct. Ensure that this change does not conflict with other URL parameters and that it behaves as expected in different scenarios.Run the following script to verify the usage of
router.push
and ensure it aligns with the intended navigation logic:web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/header.tsx (2)
5-5
: Ensure consistent use ofuseSearchParams
.The addition of
useSearchParams
is appropriate for managing URL search parameters. Ensure that this hook is used consistently throughout the component to maintain clarity and functionality.
59-60
: Verify the conditional rendering logic for the button.The logic for conditionally rendering the button based on
isVersionHistoryOverlayActive
looks correct. Ensure that this condition accurately reflects the intended behavior in all scenarios.Run the following script to verify the usage of
isVersionHistoryOverlayActive
and ensure it aligns with the intended rendering logic:Verification successful
Conditional Rendering Logic Verified
The conditional rendering logic for the button based on
isVersionHistoryOverlayActive
is correctly implemented. The button is rendered only when the version history overlay is not active, which aligns with the intended behavior.
- The button is conditionally rendered with the condition:
{isContentEditable && !isVersionHistoryOverlayActive}
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the conditional rendering logic based on `isVersionHistoryOverlayActive`. # Test: Search for `isVersionHistoryOverlayActive` usage. Expect: Correct conditional rendering logic. rg --type tsx -A 5 $'isVersionHistoryOverlayActive'Length of output: 78
Script:
#!/bin/bash # Description: Verify the conditional rendering logic based on `isVersionHistoryOverlayActive`. # Test: Search for `isVersionHistoryOverlayActive` usage in .tsx files. Expect: Correct conditional rendering logic. rg --glob '*.tsx' -A 5 'isVersionHistoryOverlayActive'Length of output: 1715
web/core/hooks/use-page-description.ts (2)
185-196
: Ensure robust error handling inmanuallyUpdateDescription
.The
manuallyUpdateDescription
function includes a try-catch block for error handling. Ensure that any issues during the update process are logged appropriately and consider providing user feedback if necessary.
205-205
: Verify the export ofmanuallyUpdateDescription
.The
manuallyUpdateDescription
function is newly exported. Ensure that it is used correctly in the components that utilize this hook and that it aligns with the intended functionality.Run the following script to verify the usage of
manuallyUpdateDescription
and ensure it aligns with the intended functionality:Verification successful
Verified:
manuallyUpdateDescription
is correctly exported and used.The function
manuallyUpdateDescription
is correctly exported and utilized within thepage-root.tsx
component. It is integrated into the component's logic as a handler, aligning with its intended functionality. No issues found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `manuallyUpdateDescription`. # Test: Search for `manuallyUpdateDescription` usage. Expect: Correct usage in components. rg --type tsx -A 5 $'manuallyUpdateDescription'Length of output: 74
Script:
#!/bin/bash # Description: Verify the usage of `manuallyUpdateDescription` in the codebase. # Search for `manuallyUpdateDescription` usage in all files. rg 'manuallyUpdateDescription' -A 5Length of output: 1654
packages/editor/src/core/hooks/use-editor.ts (1)
129-130
: Verify the usage of theemitUpdate
parameter.The
clearEditor
function now accepts an optional parameteremitUpdate
, which is passed to theclearContent
command. Ensure that this change is correctly integrated and does not introduce unexpected behavior in scenarios where state updates are managed.Run the following script to verify the function usage:
Verification successful
Usage of
emitUpdate
parameter is consistent and intentional.The
clearEditor
function is used across various components, with theemitUpdate
parameter being explicitly set totrue
only in specific cases. This suggests that the default behavior of not emitting updates is appropriate for most scenarios. The integration appears correct and does not introduce unexpected behavior.
web/core/hooks/use-page-description.ts
: Explicitly setsemitUpdate
totrue
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `emitUpdate` parameter in the `clearEditor` function. # Test: Search for the function usage. Expect: Occurrences where `emitUpdate` is used correctly. rg --type typescript -A 5 $'clearEditor'Length of output: 74
Script:
#!/bin/bash # Description: Verify the usage of the `emitUpdate` parameter in the `clearEditor` function. # Test: Search for the function usage. Expect: Occurrences where `emitUpdate` is used correctly. rg --type ts -A 5 $'clearEditor'Length of output: 4802
async fetchAllVersions(workspaceSlug: string, projectId: string, pageId: string): Promise<TPageVersion[]> { | ||
return this.get(`/api/workspaces/${workspaceSlug}/projects/${projectId}/pages/${pageId}/versions/`) | ||
.then((response) => response?.data) | ||
.catch((error) => { | ||
throw error?.response?.data; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve error handling in fetchAllVersions
.
Consider adding more context to the error or logging it for better traceability. This will help in debugging issues related to fetching page versions.
You could log the error before throwing it:
.catch((error) => {
+ console.error("Error fetching all versions:", error);
throw error?.response?.data;
});
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.
async fetchAllVersions(workspaceSlug: string, projectId: string, pageId: string): Promise<TPageVersion[]> { | |
return this.get(`/api/workspaces/${workspaceSlug}/projects/${projectId}/pages/${pageId}/versions/`) | |
.then((response) => response?.data) | |
.catch((error) => { | |
throw error?.response?.data; | |
}); | |
} | |
async fetchAllVersions(workspaceSlug: string, projectId: string, pageId: string): Promise<TPageVersion[]> { | |
return this.get(`/api/workspaces/${workspaceSlug}/projects/${projectId}/pages/${pageId}/versions/`) | |
.then((response) => response?.data) | |
.catch((error) => { | |
console.error("Error fetching all versions:", error); | |
throw error?.response?.data; | |
}); | |
} |
async fetchVersionById( | ||
workspaceSlug: string, | ||
projectId: string, | ||
pageId: string, | ||
versionId: string | ||
): Promise<TPageVersion> { | ||
return this.get(`/api/workspaces/${workspaceSlug}/projects/${projectId}/pages/${pageId}/versions/${versionId}/`) | ||
.then((response) => response?.data) | ||
.catch((error) => { | ||
throw error?.response?.data; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve error handling in fetchVersionById
.
Consider adding more context to the error or logging it for better traceability. This will help in debugging issues related to fetching a specific page version.
You could log the error before throwing it:
.catch((error) => {
+ console.error("Error fetching version by ID:", error);
throw error?.response?.data;
});
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.
async fetchVersionById( | |
workspaceSlug: string, | |
projectId: string, | |
pageId: string, | |
versionId: string | |
): Promise<TPageVersion> { | |
return this.get(`/api/workspaces/${workspaceSlug}/projects/${projectId}/pages/${pageId}/versions/${versionId}/`) | |
.then((response) => response?.data) | |
.catch((error) => { | |
throw error?.response?.data; | |
}); | |
} | |
async fetchVersionById( | |
workspaceSlug: string, | |
projectId: string, | |
pageId: string, | |
versionId: string | |
): Promise<TPageVersion> { | |
return this.get(`/api/workspaces/${workspaceSlug}/projects/${projectId}/pages/${pageId}/versions/${versionId}/`) | |
.then((response) => response?.data) | |
.catch((error) => { | |
console.error("Error fetching version by ID:", error); | |
throw error?.response?.data; | |
}); | |
} |
const { data: versionsList } = useSWR( | ||
pageId && isOpen ? `PAGE_VERSIONS_LIST_${pageId}` : null, | ||
pageId && isOpen ? () => fetchAllVersions(pageId) : null | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding error handling for data fetching.
The SWR hook is used for fetching the versions list, but there is no error handling implemented. Consider adding error handling to manage scenarios where data fetching might fail.
You can use the error
property from the SWR response to handle errors.
const { data: versionsList, error: versionsError } = useSWR(
pageId && isOpen ? `PAGE_VERSIONS_LIST_${pageId}` : null,
pageId && isOpen ? () => fetchAllVersions(pageId) : null
);
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.
const { data: versionsList } = useSWR( | |
pageId && isOpen ? `PAGE_VERSIONS_LIST_${pageId}` : null, | |
pageId && isOpen ? () => fetchAllVersions(pageId) : null | |
); | |
const { data: versionsList, error: versionsError } = useSWR( | |
pageId && isOpen ? `PAGE_VERSIONS_LIST_${pageId}` : null, | |
pageId && isOpen ? () => fetchAllVersions(pageId) : null | |
); |
if (!isCurrentVersionActive && !versionDetails) | ||
return ( | ||
<div className="size-full px-5"> | ||
<Loader className="relative space-y-4"> | ||
<Loader.Item width="50%" height="36px" /> | ||
<div className="space-y-2"> | ||
<div className="py-2"> | ||
<Loader.Item width="100%" height="36px" /> | ||
</div> | ||
<Loader.Item width="80%" height="22px" /> | ||
<div className="relative flex items-center gap-2"> | ||
<Loader.Item width="30px" height="30px" /> | ||
<Loader.Item width="30%" height="22px" /> | ||
</div> | ||
<div className="py-2"> | ||
<Loader.Item width="60%" height="36px" /> | ||
</div> | ||
<Loader.Item width="70%" height="22px" /> | ||
<Loader.Item width="30%" height="22px" /> | ||
<div className="relative flex items-center gap-2"> | ||
<Loader.Item width="30px" height="30px" /> | ||
<Loader.Item width="30%" height="22px" /> | ||
</div> | ||
<div className="py-2"> | ||
<Loader.Item width="50%" height="30px" /> | ||
</div> | ||
<Loader.Item width="100%" height="22px" /> | ||
<div className="py-2"> | ||
<Loader.Item width="30%" height="30px" /> | ||
</div> | ||
<Loader.Item width="30%" height="22px" /> | ||
<div className="relative flex items-center gap-2"> | ||
<div className="py-2"> | ||
<Loader.Item width="30px" height="30px" /> | ||
</div> | ||
<Loader.Item width="30%" height="22px" /> | ||
</div> | ||
</div> | ||
</Loader> | ||
</div> | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve loading state handling.
The loading state is handled with a series of Loader.Item
components. Consider abstracting this into a separate component for better readability and reusability.
<PageVersionsOverlay | ||
activeVersion={version} | ||
fetchAllVersions={async (pageId) => { | ||
if (!workspaceSlug || !projectId) return; | ||
return await projectPageVersionService.fetchAllVersions( | ||
workspaceSlug.toString(), | ||
projectId.toString(), | ||
pageId | ||
); | ||
}} | ||
fetchVersionDetails={async (pageId, versionId) => { | ||
if (!workspaceSlug || !projectId) return; | ||
return await projectPageVersionService.fetchVersionById( | ||
workspaceSlug.toString(), | ||
projectId.toString(), | ||
pageId, | ||
versionId | ||
); | ||
}} | ||
handleRestore={manuallyUpdateDescription} | ||
isOpen={isVersionsOverlayOpen} | ||
onClose={handleCloseVersionsOverlay} | ||
pageId={page.id ?? ""} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimize asynchronous functions.
Consider extracting the asynchronous functions fetchAllVersions
and fetchVersionDetails
into separate functions outside the JSX for better readability and performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- web/core/components/pages/editor/header/options-dropdown.tsx (5 hunks)
- web/core/components/pages/editor/page-root.tsx (3 hunks)
- web/core/components/pages/version/sidebar.tsx (1 hunks)
- web/core/hooks/use-query-params.ts (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- web/core/components/pages/editor/header/options-dropdown.tsx
- web/core/components/pages/editor/page-root.tsx
- web/core/components/pages/version/sidebar.tsx
Additional comments not posted (2)
web/core/hooks/use-query-params.ts (2)
1-2
: Imports are appropriate.The imported hooks are necessary for managing query parameters in a Next.js application.
3-5
: Type definition is correct.The
TParamsToAdd
type ensures that query parameters are strings, which is appropriate for URL parameters.
export const useQueryParams = () => { | ||
// next navigation | ||
const searchParams = useSearchParams(); | ||
const pathname = usePathname(); | ||
|
||
const updateQueryParams = ({ | ||
paramsToAdd = {}, | ||
paramsToRemove = [], | ||
}: { | ||
paramsToAdd?: TParamsToAdd; | ||
paramsToRemove?: string[]; | ||
}) => { | ||
const currentParams = new URLSearchParams(searchParams.toString()); | ||
|
||
// add or update query parameters | ||
Object.keys(paramsToAdd).forEach((key) => { | ||
currentParams.set(key, paramsToAdd[key]); | ||
}); | ||
|
||
// remove specified query parameters | ||
paramsToRemove.forEach((key) => { | ||
currentParams.delete(key); | ||
}); | ||
|
||
// construct the new route with the updated query parameters | ||
const newRoute = `${pathname}?${currentParams.toString()}`; | ||
return newRoute; | ||
}; | ||
|
||
return { | ||
updateQueryParams, | ||
}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding error handling for edge cases.
The updateQueryParams
function correctly manages query parameters. However, adding error handling for invalid parameter values or types could improve robustness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- web/core/components/pages/version/index.ts (1 hunks)
- web/core/components/pages/version/main-content.tsx (1 hunks)
- web/core/components/pages/version/root.tsx (1 hunks)
- web/core/components/pages/version/sidebar-list.tsx (1 hunks)
- web/core/components/pages/version/sidebar-root.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- web/core/components/pages/version/index.ts
- web/core/components/pages/version/main-content.tsx
- web/core/components/pages/version/root.tsx
Additional comments not posted (8)
web/core/components/pages/version/sidebar-root.tsx (4)
1-5
: LGTM!The import statements are correctly importing the necessary modules.
The code changes are approved.
7-13
: LGTM!The
Props
type is correctly defined and includes all necessary props.The code changes are approved.
15-16
: LGTM!The component definition and props destructuring are correctly implemented.
The code changes are approved.
18-37
: LGTM!The JSX is correctly implemented and follows best practices for readability and maintainability.
The code changes are approved.
web/core/components/pages/version/sidebar-list.tsx (4)
1-14
: LGTM!The import statements are correctly importing the necessary modules.
The code changes are approved.
16-21
: LGTM!The
Props
type is correctly defined and includes all necessary props.The code changes are approved.
23-24
: LGTM!The component definition and props destructuring are correctly implemented.
The code changes are approved.
45-98
: LGTM!The JSX is correctly implemented and follows best practices for readability and maintainability. The error handling and loading states are well-managed.
The code changes are approved.
// states | ||
const [isRetrying, setIsRetrying] = useState(false); | ||
// update query params | ||
const { updateQueryParams } = useQueryParams(); | ||
|
||
const { | ||
data: versionsList, | ||
error: versionsListError, | ||
mutate: mutateVersionsList, | ||
} = useSWR( | ||
pageId && isOpen ? `PAGE_VERSIONS_LIST_${pageId}` : null, | ||
pageId && isOpen ? () => fetchAllVersions(pageId) : null | ||
); | ||
|
||
const handleRetry = async () => { | ||
setIsRetrying(true); | ||
await mutateVersionsList(); | ||
setIsRetrying(false); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding error handling for the mutateVersionsList
call.
The SWR hook and state management are correctly implemented. However, adding error handling for the mutateVersionsList
call can improve robustness.
Apply this diff to add error handling:
const handleRetry = async () => {
setIsRetrying(true);
- await mutateVersionsList();
+ try {
+ await mutateVersionsList();
+ } catch (error) {
+ console.error('Failed to retry fetching versions list:', error);
+ }
setIsRetrying(false);
};
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.
// states | |
const [isRetrying, setIsRetrying] = useState(false); | |
// update query params | |
const { updateQueryParams } = useQueryParams(); | |
const { | |
data: versionsList, | |
error: versionsListError, | |
mutate: mutateVersionsList, | |
} = useSWR( | |
pageId && isOpen ? `PAGE_VERSIONS_LIST_${pageId}` : null, | |
pageId && isOpen ? () => fetchAllVersions(pageId) : null | |
); | |
const handleRetry = async () => { | |
setIsRetrying(true); | |
await mutateVersionsList(); | |
setIsRetrying(false); | |
}; | |
const [isRetrying, setIsRetrying] = useState(false); | |
// update query params | |
const { updateQueryParams } = useQueryParams(); | |
const { | |
data: versionsList, | |
error: versionsListError, | |
mutate: mutateVersionsList, | |
} = useSWR( | |
pageId && isOpen ? `PAGE_VERSIONS_LIST_${pageId}` : null, | |
pageId && isOpen ? () => fetchAllVersions(pageId) : null | |
); | |
const handleRetry = async () => { | |
setIsRetrying(true); | |
try { | |
await mutateVersionsList(); | |
} catch (error) { | |
console.error('Failed to retry fetching versions list:', error); | |
} | |
setIsRetrying(false); | |
}; |
What's new?
Introducing
Version history
for Pages. Users can now access the last 20 versions of a page and restore to any of those.Media:
Screen.Recording.2024-08-23.at.16.36.28.mov
GitHub issue: #4619
Plane issue: WEB-2293
Summary by CodeRabbit
New Features
PageVersionDetailSerializer
for enhanced API support in detailed page version requests.PageOptionsDropdown
to include a "Version history" menu item.PageVersionsSidebar
component for easy navigation through version history.useQueryParams
hook for better management of URL query parameters.Improvements
Bug Fixes
Documentation