[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 (
|
![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, 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
isVersionsOverlayOpencould be renamed toversionsOverlayVisiblefor 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.PageVersionDetailSerializerimported 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.serviceimproves 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: Exportingversioncomponent.The addition of the export for the
versioncomponent enhances the modularity and usability of the components related to pages. This allows other parts of the application to access theversionfunctionality directly.apiserver/plane/app/views/page/version.py (2)
30-30: Verify the usage ofPageVersionDetailSerializer.Ensure that
PageVersionDetailSerializeris 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
PageVersionDetailSerializerclass 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_permissiondecorator.Ensure that the
@allow_permissiondecorator is correctly implemented and that it effectively manages the roles specified.Run the following script to verify the decorator implementation:
Verification successful
The
@allow_permissiondecorator is correctly implemented.The
allow_permissiondecorator is defined inapiserver/plane/app/permissions/base.pyand 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
PlaneVersionsSidebarListItemcomponent correctly utilizes MobX by importing and using theobserverfrommobx-react. This indicates that MobX is being used to manage state in this component as expected.
- The import statement for
observeris 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
PlaneVersionsSidebarListItemcomponent is correctly used within thePageVersionsSidebarcomponent. The propshref,isActive, andversionare 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 theTPageVersiontype 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 thecnhelper function.Ensure that the
cnfunction is correctly handling conditional class names, especially whenisOpenchanges.Run the following script to verify the
cnfunction implementation:Verification successful
The
cnfunction correctly handles conditional class names. The implementation usesclsxandtwMerge, which are suitable for managing conditional class names based on theisOpenstate. 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 theclearEditormethod signature change.The
clearEditormethod now accepts an optionalemitUpdateparameter. 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
clearEditormethod:web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx (1)
67-67: Verify the impact of therelativeclass addition.The addition of the
relativeclass to thedivelement 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
relativeclass in the context of the component's styling:Verification successful
No Negative Impact from
relativeClass AdditionThe addition of the
relativeclass 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
observerfrommobx-reactis necessary. If the component does not observe any MobX state, it might be redundant.
94-110: Ensure proper handling ofinitialValue.The
initialValuefor 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
useEffecthook 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
handleCloseVersionsOverlaycorrectly 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
PageVersionSerializernow explicitly lists fields, which improves clarity and control. Ensure that all necessary fields are included.
187-203: Verify field necessity inPageVersionDetailSerializer.The
PageVersionDetailSerializerincludes 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, anduseSearchParamsis 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=currentquery 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.pushand 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
useSearchParamsis 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
isVersionHistoryOverlayActivelooks correct. Ensure that this condition accurately reflects the intended behavior in all scenarios.Run the following script to verify the usage of
isVersionHistoryOverlayActiveand ensure it aligns with the intended rendering logic:Verification successful
Conditional Rendering Logic Verified
The conditional rendering logic for the button based on
isVersionHistoryOverlayActiveis 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
manuallyUpdateDescriptionfunction 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
manuallyUpdateDescriptionfunction 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
manuallyUpdateDescriptionand ensure it aligns with the intended functionality:Verification successful
Verified:
manuallyUpdateDescriptionis correctly exported and used.The function
manuallyUpdateDescriptionis correctly exported and utilized within thepage-root.tsxcomponent. 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 theemitUpdateparameter.The
clearEditorfunction now accepts an optional parameteremitUpdate, which is passed to theclearContentcommand. 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
emitUpdateparameter is consistent and intentional.The
clearEditorfunction is used across various components, with theemitUpdateparameter being explicitly set totrueonly 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 setsemitUpdatetotrue.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.
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.
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.
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.
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.
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.
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
TParamsToAddtype 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.
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.
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
Propstype 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
Propstype 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.
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); | |
| }; |
4 participants
What's new?
Introducing
Version historyfor 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
PageVersionDetailSerializerfor enhanced API support in detailed page version requests.PageOptionsDropdownto include a "Version history" menu item.PageVersionsSidebarcomponent for easy navigation through version history.useQueryParamshook for better management of URL query parameters.Improvements
Bug Fixes
Documentation