feat: issue version history #6112
Conversation
…t/issue-version-history
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request primarily involve renaming and restructuring type definitions related to version management in the editor components. The type Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (24)
web/core/components/editor/version-history/editors/index.ts (1)
3-8: Add JSDoc comments to clarify property relationshipsThe type structure is well-defined, but the relationship between
activeVersionandisCurrentVersionActivecould be clearer.Consider adding JSDoc comments:
+/** + * Props for version editor components + * @property activeVersion - The ID of the currently selected version + * @property currentVersionDescription - Description of the current version + * @property isCurrentVersionActive - Whether the current version is being displayed + * @property versionDetails - Detailed information about the version + */ export type TVersionEditorProps = { activeVersion: string | null; currentVersionDescription: string | null; isCurrentVersionActive: boolean; versionDetails: TEditorVersion | undefined; };web/core/services/page/project-page-version.service.ts (2)
Line range hint
13-19: Consider enhancing error handling with specific error typesWhile the error handling is functional, it could be more robust by defining specific error types for better error handling upstream.
Consider this enhancement:
+interface VersionAPIError { + message: string; + code: string; +} async fetchAllVersions(workspaceSlug: string, projectId: string, pageId: string): Promise<TEditorVersion[]> { return this.get(`/api/workspaces/${workspaceSlug}/projects/${projectId}/pages/${pageId}/versions/`) .then((response) => response?.data) - .catch((error) => { - throw error?.response?.data; + .catch((error: VersionAPIError) => { + throw { + message: error?.response?.data?.message || 'Failed to fetch versions', + code: error?.response?.data?.code || 'UNKNOWN_ERROR' + }; }); }
Line range hint
21-32: Consider extracting common error handling logicThe error handling pattern is duplicated between
fetchAllVersionsandfetchVersionById. This could be extracted into a shared utility method.Consider this enhancement:
+private handleVersionError(error: any) { + throw { + message: error?.response?.data?.message || 'Version operation failed', + code: error?.response?.data?.code || 'UNKNOWN_ERROR' + }; +} async fetchVersionById( workspaceSlug: string, projectId: string, pageId: string, versionId: string ): Promise<TEditorVersion> { return this.get(`/api/workspaces/${workspaceSlug}/projects/${projectId}/pages/${pageId}/versions/${versionId}/`) .then((response) => response?.data) - .catch((error) => { - throw error?.response?.data; + .catch(this.handleVersionError.bind(this)); }web/core/services/issue/issue_version.service.ts (1)
21-32: Apply DRY principle to error handling and URL construction.The method duplicates error handling logic and URL construction patterns from
fetchAllVersions.Consider extracting common functionality:
+private handleError(error: any) { + throw error?.response?.data ?? error; +} + +private getVersionsEndpoint(workspaceSlug: string, projectId: string, issueId: string) { + return `${this.BASE_ENDPOINT}/${workspaceSlug}/projects/${projectId}/issues/${issueId}/versions`; +} + async fetchVersionById( workspaceSlug: string, projectId: string, issueId: string, versionId: string ): Promise<TEditorVersion> { - return this.get(`/api/workspaces/${workspaceSlug}/projects/${projectId}/issues/${issueId}/versions/${versionId}/`) + const endpoint = `${this.getVersionsEndpoint(workspaceSlug, projectId, issueId)}/${versionId}/`; + return this.get(endpoint) .then((response) => response?.data) - .catch((error) => { - throw error?.response?.data; - }); + .catch(this.handleError); }web/core/components/editor/version-history/sidebar-root.tsx (1)
Line range hint
15-36: LGTM! Consider minor accessibility enhancementThe component implementation is clean and consistent with the refactoring. Props are correctly passed down to child components.
Consider adding an
aria-labelto the close button for better accessibility:<button type="button" onClick={handleClose} + aria-label="Close version history sidebar" className="flex-shrink-0 size-6 grid place-items-center text-custom-text-300 hover:text-custom-text-100 transition-colors" >web/core/components/editor/version-history/sidebar-list-item.tsx (1)
Line range hint
39-45: Consider adding null check for owner details in Avatar props.While the current implementation works, we could improve type safety for the Avatar component props.
<Avatar - src={getFileURL(ownerDetails?.avatar_url ?? "")} - name={ownerDetails?.display_name} + src={ownerDetails ? getFileURL(ownerDetails.avatar_url ?? "") : ""} + name={ownerDetails?.display_name ?? "Unknown User"} shape="square" size="sm" className="flex-shrink-0" />packages/types/src/pages.d.ts (2)
Line range hint
57-57: Consider strengthening the type safety of description_jsonThe
description_jsonfield uses a genericobjecttype, which provides minimal type safety. Consider defining a more specific interface for the expected JSON structure.- description_json?: object; + description_json?: { + type: string; + content: Array<{ + type: string; + attrs?: Record<string, unknown>; + content?: Array<unknown>; + }>; + };
Line range hint
66-70: Improve consistency and type safety in TDocumentPayloadA few suggestions to enhance this type:
- Align the property name
descriptionwithdescription_jsonused inTEditorVersion- Consider making fields optional for flexibility
- Add stronger typing for the description object
export type TDocumentPayload = { - description_binary: string; - description_html: string; - description: object; + description_binary?: string; + description_html?: string; + description_json?: { + type: string; + content: Array<{ + type: string; + attrs?: Record<string, unknown>; + content?: Array<unknown>; + }>; + }; }web/core/components/editor/version-history/root.tsx (1)
Line range hint
22-67: Consider enhancing modal accessibilityWhile the implementation is solid, consider adding ARIA attributes and keyboard handling for better accessibility:
<div className={cn( "absolute inset-0 z-20 size-full bg-custom-background-100 flex overflow-hidden opacity-0 pointer-events-none transition-opacity", { "opacity-100 pointer-events-auto": isOpen, } )} + role="dialog" + aria-modal="true" + aria-labelledby="version-history-title" + onKeyDown={(e) => { + if (e.key === "Escape") handleClose(); + }} >web/core/components/editor/version-history/editors/issue-version-editor.tsx (5)
8-8: Consider using absolute imports for type definitions.Using relative imports for types can make the codebase harder to maintain if the file structure changes. Consider using an absolute import path instead.
-import { TVersionEditorProps } from "."; +import { TVersionEditorProps } from "@/core/components/editor/version-history/editors";
13-13: Add type safety to useParams hook.The
useParamshook from Next.js doesn't provide type safety by default. Consider adding type definitions for the route parameters.-const { workspaceSlug, projectId } = useParams(); +interface Params { + workspaceSlug: string; + projectId: string; +} +const { workspaceSlug, projectId } = useParams<Params>();
15-55: Optimize loader implementation for better maintainability.The loader implementation contains repeated code patterns and could be simplified. Consider:
- Extracting the loader pattern into a separate component
- Using a map to generate repeated loader items
- Using design system spacing tokens instead of arbitrary padding
Example refactor:
const VersionLoader: React.FC = () => ( <div className="size-full p-design-5"> <Loader className="relative space-y-4"> <Loader.Item width="50%" height="36px" /> {[ { width: "100%", height: "36px" }, { width: "80%", height: "22px" }, // ... other items ].map((item, index) => ( <Loader.Item key={index} {...item} /> ))} </Loader> </div> );
57-69: Enhance code quality and maintainability.Consider the following improvements:
- Extract magic strings and values into constants
- Improve type safety with proper null checks
- Use optional chaining consistently
+const EMPTY_EDITOR_CONTENT = "<p></p>"; + const description = isCurrentVersionActive ? currentVersionDescription : versionDetails?.description_html; -if (description === undefined || description?.trim() === "") return null; +if (!description?.trim()) return null; return ( <RichTextReadOnlyEditor - id={activeVersion ?? ""} - initialValue={description ?? "<p></p>"} + id={activeVersion || ""} + initialValue={description || EMPTY_EDITOR_CONTENT} containerClassName="p-0 pb-64 border-none" editorClassName="pl-10" - workspaceSlug={workspaceSlug?.toString() ?? ""} - projectId={projectId?.toString() ?? ""} + workspaceSlug={String(workspaceSlug || "")} + projectId={String(projectId || "")} /> );
10-70: Consider adding error handling and error boundaries.The component currently lacks explicit error handling for cases where:
- The rich text editor fails to load
- The version details fetch fails
- The workspace/project parameters are invalid
Consider wrapping the component with an error boundary and adding proper error states.
Example implementation:
import { ErrorBoundary } from '@/components/error-boundary'; export const IssueVersionEditorWithErrorBoundary: React.FC<TVersionEditorProps> = (props) => ( <ErrorBoundary fallback={<VersionEditorError />}> <IssueVersionEditor {...props} /> </ErrorBoundary> );web/core/components/issues/issue-detail/version-history.tsx (3)
12-13: Consider moving service initialization inside componentInitializing the service outside the component creates a shared instance across all uses of this component. This could lead to potential state sharing issues in certain scenarios.
Consider moving the service initialization inside the component:
-const issueVersionService = new IssueVersionService(); export const IssueVersionHistory: React.FC<Props> = (props) => { + const issueVersionService = new IssueVersionService();
15-20: Add JSDoc comments to document props interfaceThe props interface is well-structured but could benefit from JSDoc comments to provide more context about each prop's purpose.
Consider adding documentation:
+/** + * Props for the IssueVersionHistory component + * @property {boolean} disabled - Whether the editor is in disabled state + * @property {EditorRefApi} editorRef - Reference to the main editor + * @property {string} issueId - Unique identifier of the issue + * @property {EditorReadOnlyRefApi} readOnlyEditorRef - Reference to the read-only editor + */ type Props = { disabled: boolean; editorRef: EditorRefApi; issueId: string; readOnlyEditorRef: EditorReadOnlyRefApi; };
36-42: Consider debouncing version state changesThe current implementation might be susceptible to race conditions if version changes occur rapidly. Consider adding cleanup and debouncing.
Consider this improvement:
useEffect(() => { + const timeoutId = setTimeout(() => { if (!version) { setIsVersionsOverlayOpen(false); return; } setIsVersionsOverlayOpen(true); + }, 100); + + return () => clearTimeout(timeoutId); }, [version]);web/core/components/editor/version-history/sidebar-list.tsx (1)
35-36: Consider using a constant for the SWR cache key prefix.To improve maintainability and prevent typos, consider extracting the cache key prefix to a constant.
+const EDITOR_VERSIONS_CACHE_KEY_PREFIX = 'EDITOR_VERSIONS_LIST_'; + const { data: versionsList, error: versionsListError, mutate: mutateVersionsList, } = useSWR( - entityId && isOpen ? `EDITOR_VERSIONS_LIST_${entityId}` : null, + entityId && isOpen ? `${EDITOR_VERSIONS_CACHE_KEY_PREFIX}${entityId}` : null, entityId && isOpen ? () => fetchAllVersions(entityId) : null );web/core/components/editor/version-history/editors/page-version-editor.tsx (2)
4-4: Consider grouping related imports togetherThe imports are well-organized, but could be slightly improved by grouping the Plane-related imports (
@plane/editor,@plane/types) together.// plane packages import { DocumentReadOnlyEditorWithRef } from "@plane/editor"; import { IUserLite } from "@plane/types"; import { Loader } from "@plane/ui"; // local types import { TVersionEditorProps } from ".";Also applies to: 6-6, 15-16
Line range hint
42-43: Consider adding a loading check for required propsThe loading check could be more comprehensive by verifying all required props are available.
- if (!isCurrentVersionActive && !versionDetails) + if ((!isCurrentVersionActive && !versionDetails) || !workspaceSlug || !projectId)web/core/components/editor/version-history/main-content.tsx (3)
45-46: Consider improving the SWR implementationThe current implementation could be enhanced for better maintainability and reliability:
- Consider extracting the condition to a variable to avoid repetition
- Use a more structured cache key format to prevent potential conflicts
+ const shouldFetch = entityId && activeVersion && activeVersion !== "current"; + const cacheKey = shouldFetch ? `editor/versions/${entityId}/${activeVersion}` : null; useSWR( - entityId && activeVersion && activeVersion !== "current" ? `EDITOR_VERSION_${activeVersion}` : null, - entityId && activeVersion && activeVersion !== "current" ? () => fetchVersionDetails(entityId, activeVersion) : null + cacheKey, + () => shouldFetch ? fetchVersionDetails(entityId, activeVersion) : null );
57-66: Consider enhancing error messagesWhile the error handling is implemented correctly, the messages could be more informative to help users understand what went wrong.
setToast({ type: TOAST_TYPE.SUCCESS, - title: "Version restored.", + title: "Version restored successfully", + message: "The content has been updated to reflect the selected version." }); setToast({ type: TOAST_TYPE.ERROR, - title: "Failed to restore version.", + title: "Failed to restore version", + message: "An error occurred while restoring the version. Please try again." });
Line range hint
113-122: Consider improving overflow handlingThe current overflow handling might cause issues with very long content. Consider adding max-height constraints and improving the scrollbar experience.
- <div className="pt-8 h-full overflow-y-scroll vertical-scrollbar scrollbar-sm"> + <div className="pt-8 h-full overflow-y-auto vertical-scrollbar scrollbar-sm max-h-[calc(100vh-200px)]">web/core/components/pages/editor/page-root.tsx (1)
Line range hint
89-93: Add error handling for version restoration.The
handleRestoreVersionfunction should include error handling to manage failed restoration attempts.Consider implementing error handling:
const handleRestoreVersion = async (descriptionHTML: string) => { + try { editorRef.current?.clearEditor(); editorRef.current?.setEditorValue(descriptionHTML); + } catch (error) { + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to restore version. Please try again.", + }); + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
packages/types/src/pages.d.ts(1 hunks)web/core/components/editor/index.ts(1 hunks)web/core/components/editor/version-history/editors/index.ts(1 hunks)web/core/components/editor/version-history/editors/issue-version-editor.tsx(1 hunks)web/core/components/editor/version-history/editors/page-version-editor.tsx(1 hunks)web/core/components/editor/version-history/index.ts(1 hunks)web/core/components/editor/version-history/main-content.tsx(3 hunks)web/core/components/editor/version-history/root.tsx(2 hunks)web/core/components/editor/version-history/sidebar-list-item.tsx(2 hunks)web/core/components/editor/version-history/sidebar-list.tsx(3 hunks)web/core/components/editor/version-history/sidebar-root.tsx(2 hunks)web/core/components/issues/issue-detail/index.ts(1 hunks)web/core/components/issues/issue-detail/root.tsx(2 hunks)web/core/components/issues/issue-detail/version-history.tsx(1 hunks)web/core/components/pages/editor/page-root.tsx(2 hunks)web/core/services/issue/index.ts(1 hunks)web/core/services/issue/issue_version.service.ts(1 hunks)web/core/services/page/project-page-version.service.ts(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- web/core/components/editor/version-history/index.ts
- web/core/components/issues/issue-detail/index.ts
🔇 Additional comments (28)
web/core/components/editor/index.ts (1)
4-4: LGTM! Verify the version-history module.
The new export statement follows the existing pattern and maintains alphabetical ordering.
Let's verify the existence and structure of the version-history module:
✅ Verification successful
Export statement is valid and version-history module is well-structured
The verification confirms:
- The version-history module exists with a complete set of components
- All necessary exports are in place, including editors, sidebar, and main content components
- The types
TEditorVersionandTVersionEditorPropsare properly used throughout the module - The module follows a clean architecture with separate components for different functionalities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the version-history module exists and contains the expected components
# Check if the version-history directory exists and list its contents
fd --type f "." "web/core/components/editor/version-history"
# Verify the exports from version-history match the AI summary
rg --type ts "export" "web/core/components/editor/version-history"
# Verify the type changes mentioned in the summary
rg --type ts "TEditorVersion|TVersionEditorProps" "web/core/components/editor/version-history"
Length of output: 5293
web/core/components/editor/version-history/editors/index.ts (2)
1-1: Verify the impact of TPageVersion to TEditorVersion renaming
The type renaming looks good and makes the type more generic. Let's verify all occurrences have been updated.
✅ Verification successful
Based on the search results, I can generate the final response now:
Type renaming from TPageVersion to TEditorVersion is complete and consistent
The codebase shows consistent usage of TEditorVersion across all relevant files with no remaining references to TPageVersion. The type is properly defined in packages/types/src/pages.d.ts and imported correctly in all consuming components and services.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to TPageVersion
# and verify TEditorVersion is used consistently
# Check for any remaining TPageVersion references
echo "Checking for remaining TPageVersion references:"
rg "TPageVersion"
# Check TEditorVersion usage for consistency
echo "Checking TEditorVersion usage:"
rg "TEditorVersion"
Length of output: 2533
10-11: Verify PagesVersionEditor to PageVersionEditor renaming
The export structure looks good. Let's verify the component renaming impact.
✅ Verification successful
Based on the results, I can generate the final response as we have clear evidence that:
- There are no remaining references to "PagesVersionEditor" in the codebase
- "PageVersionEditor" is consistently used in the expected locations (component definition and its usage)
Component renaming verification completed successfully
The renaming from PagesVersionEditor to PageVersionEditor has been properly implemented across the codebase. The component is correctly defined in page-version-editor.tsx and properly imported and used in page-root.tsx. No legacy references remain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to PagesVersionEditor
# and verify PageVersionEditor is used consistently
# Check for any remaining PagesVersionEditor references
echo "Checking for remaining PagesVersionEditor references:"
rg "PagesVersionEditor"
# Check PageVersionEditor usage
echo "Checking PageVersionEditor usage:"
rg "PageVersionEditor"
Length of output: 625
web/core/services/issue/index.ts (2)
10-10: LGTM! Well-organized export structure.
The new export for issue_version.service is logically placed with other issue-related services and maintains alphabetical ordering.
10-10: Verify the existence of the referenced service file.
Let's ensure the referenced service file exists and is properly implemented.
✅ Verification successful
Service file exists and is properly implemented
The issue_version.service.ts file exists at the correct location and contains a well-implemented IssueVersionService class with proper methods for fetching issue versions. The file exports the service class which makes it available for the import statement in question.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of issue_version.service.ts
# Check if the service file exists
fd "issue_version.service.ts" web/core/services/issue/
# Check the service implementation
ast-grep --pattern 'class IssueVersionService {
$$$
}'
Length of output: 162
Script:
#!/bin/bash
# Let's check the content of the service file to ensure it's properly implemented
cat web/core/services/issue/issue_version.service.ts
Length of output: 1080
web/core/services/page/project-page-version.service.ts (1)
2-2: Verify type consistency across the codebase
The import has been updated from TPageVersion to TEditorVersion. Let's verify that this type is consistently used across the codebase to prevent any type mismatches.
✅ Verification successful
Type renaming from TPageVersion to TEditorVersion is consistent across the codebase
The verification shows that:
- No remaining references to
TPageVersionexist in the codebase TEditorVersionis consistently used across all relevant components and services:- Core services:
project-page-version.service.tsandissue_version.service.ts - Editor version history components
- Type definition in
packages/types/src/pages.d.ts
- Core services:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to TPageVersion and verify TEditorVersion usage
# Check for any remaining TPageVersion references
echo "Checking for remaining TPageVersion references:"
rg "TPageVersion" --type ts
# Check TEditorVersion usage pattern
echo -e "\nVerifying TEditorVersion usage:"
rg "TEditorVersion" --type ts -A 2
Length of output: 6109
web/core/services/issue/issue_version.service.ts (2)
1-11: LGTM! Clean setup with proper organization.
The imports are well-structured, and the class setup follows best practices with proper inheritance from APIService.
14-14: Verify API endpoint consistency across the codebase.
Let's ensure the API endpoint structure is consistent with other services and follows the established pattern.
Also applies to: 27-27
✅ Verification successful
API endpoint structure is consistent with codebase patterns
The endpoint /api/workspaces/${workspaceSlug}/projects/${projectId}/issues/${issueId}/versions/ follows the established pattern in the codebase:
- It matches the RESTful structure used across all issue-related services
- Similar version endpoints exist (e.g., in project-page-version.service.ts)
- The path parameters and nesting hierarchy align with other issue endpoints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in API endpoint structure across services
# Search for similar API endpoint patterns
rg -t ts "\/api\/workspaces\/.*\/projects\/.*\/issues\/" --glob "!**/issue_version.service.ts"
# Look for other version-related endpoints
rg -t ts "\/versions\/" --glob "!**/issue_version.service.ts"
Length of output: 351
Script:
#!/bin/bash
# Description: Check for consistency in API endpoint structure across services
# Search for similar API endpoint patterns without escape sequences
rg -t ts "/api/workspaces/.*/projects/.*/issues/" --glob "!**/issue_version.service.ts"
# Look for other version-related endpoints
rg -t ts "/versions/" --glob "!**/issue_version.service.ts"
# Search for similar service files to understand the pattern
fd -e ts "service.ts$" -x grep -l "api/workspaces" {}
Length of output: 8502
web/core/components/editor/version-history/sidebar-root.tsx (2)
3-5: LGTM! Clean import organization
The imports are well-organized with clear separation between external types and local components. The type rename from TPageVersion to TEditorVersion is consistent with the broader refactoring.
8-13: Verify the entityId usage across the application
The Props interface has been updated to use entityId instead of pageId, suggesting a more generic approach to version history. Let's verify this change is consistently applied.
✅ Verification successful
Props interface changes are consistently implemented
The verification shows that the migration from pageId to entityId has been thoroughly and consistently implemented across all version history components:
- No remaining
pageIdreferences in the version history components entityIdis properly used in all related files:sidebar-list.tsx: Used in props and data fetchingmain-content.tsx: Used in version details fetchingroot.tsx: Correctly passesentityIdto child components- All components use the same prop type and function signatures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining pageId references in version history components
# and verify entityId usage consistency
# Check for any remaining pageId references in version history files
echo "Checking for remaining pageId references in version history components..."
rg -l "pageId" "web/core/components/editor/version-history"
# Check for consistent entityId usage in related components
echo "Checking entityId usage pattern in version history components..."
rg -l "entityId" "web/core/components/editor/version-history"
Length of output: 645
Script:
#!/bin/bash
# Let's examine the actual usage of entityId in these files to ensure consistency
echo "Checking entityId usage in sidebar-list.tsx..."
rg "entityId" "web/core/components/editor/version-history/sidebar-list.tsx" -A 2
echo -e "\nChecking entityId usage in main-content.tsx..."
rg "entityId" "web/core/components/editor/version-history/main-content.tsx" -A 2
echo -e "\nChecking entityId usage in root.tsx..."
rg "entityId" "web/core/components/editor/version-history/root.tsx" -A 2
# Let's also check the type definition of TEditorVersion to understand the version structure
echo -e "\nChecking TEditorVersion type definition..."
rg "type TEditorVersion" -A 5
Length of output: 2503
web/core/components/editor/version-history/sidebar-list-item.tsx (1)
4-4: LGTM! Type and component renaming changes look good.
The changes align well with the broader refactoring effort, maintaining type safety and consistent naming conventions.
Also applies to: 17-17, 20-20
packages/types/src/pages.d.ts (1)
Line range hint 52-64: Verify the migration from TPageVersion to TEditorVersion
The type structure looks good, but let's ensure all consumers have been updated to use the new type name and handle the removed page property correctly.
✅ Verification successful
Migration from TPageVersion to TEditorVersion is complete and correct
The verification shows that:
- No references to the old
TPageVersiontype remain in the codebase - All imports and usages of
TEditorVersionhave been properly updated - No code is attempting to access a
.pageproperty on version objects - The type is being used consistently across services and components for both issues and pages version history
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find any remaining references to TPageVersion and verify the migration
# Check for any remaining references to TPageVersion
echo "Checking for remaining TPageVersion references..."
rg "TPageVersion" --type ts
# Look for potential version-related imports that might need updating
echo "Checking version-related imports..."
rg "import.*Version.*from.*types" --type ts
Length of output: 1100
Script:
#!/bin/bash
# Let's verify the actual usage of TEditorVersion in these files to ensure they're not still expecting the 'page' property
echo "Checking usage patterns of TEditorVersion..."
for file in $(rg -l "TEditorVersion" --type ts); do
echo "=== $file ==="
rg "TEditorVersion" -B 2 -A 2 "$file"
done
# Also check for any direct access to a 'page' property on version objects
echo -e "\nChecking for potential 'page' property access..."
rg "version.*\.page" --type ts
rg "\.page\b.*version" --type ts
Length of output: 6162
web/core/components/editor/version-history/root.tsx (3)
3-7: LGTM! Import changes align with type renaming.
The change from TPageVersion to TEditorVersion maintains consistency with the broader refactoring effort.
49-65: LGTM! Consistent prop passing to child components
The entityId prop is correctly passed down to both child components, maintaining consistency with the interface changes.
13-15: Verify the entityId implementation across consumers
The change from pageId to entityId suggests a more generic approach. Let's verify that all consumers are updated accordingly.
✅ Verification successful
Let me gather more information to verify the implementation thoroughly.
The change from pageId to entityId is correctly implemented across the codebase
The verification shows that the change is part of a well-implemented abstraction:
EditorVersionHistoryOverlayis used consistently withentityIdfor both pages (page.id) and issues (issueId)- The component properly handles different entity types through dedicated services (
ProjectPageVersionServiceandIssueVersionService) - All version-related components and interfaces are using the shared
TEditorVersiontype - The abstraction allows the version history functionality to work seamlessly for both pages and issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to pageId in version history related files
rg -A 3 "pageId.*version"
# Search for usage of EditorVersionHistoryOverlay to ensure entityId is properly passed
ast-grep --pattern 'EditorVersionHistoryOverlay entityId={$_}'
Length of output: 1374
Script:
#!/bin/bash
# Search for all usages of EditorVersionHistoryOverlay component
rg -A 5 "EditorVersionHistoryOverlay"
# Search for any version-related function calls to see the parameter naming
ast-grep --pattern 'fetchVersionDetails($$$)'
# Search for version-related imports and type definitions
rg -A 3 "TEditorVersion"
Length of output: 10542
web/core/components/editor/version-history/sidebar-list.tsx (4)
6-6: LGTM: Type and import changes align with editor versioning.
The renaming from page-specific to editor-specific types and the switch from pageId to entityId makes the component more generic and reusable across different editor contexts.
Also applies to: 13-14, 18-19
23-24: LGTM: Component naming improved for clarity.
The new name EditorVersionHistorySidebarList better describes the component's purpose and scope.
Line range hint 81-86: LGTM: Version list item implementation.
The rendering logic is clean and maintains good UX with proper loading states.
23-24: Verify component renaming across the codebase.
Let's ensure all imports and usages have been updated to use the new component name.
✅ Verification successful
All references to the new component name are properly updated
The verification shows that:
- No occurrences of old component names (
PageVersionsSidebarListorPlaneVersionsSidebarListItem) were found in the codebase - The new component
EditorVersionHistorySidebarListis properly referenced in:- sidebar-list.tsx (component definition)
- sidebar-list-item.tsx
- sidebar-root.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old component names
# Search for old component names that should have been updated
echo "Checking for old component names..."
rg -l "PageVersionsSidebarList|PlaneVersionsSidebarListItem"
# Search for imports of the new component to verify usage
echo "Checking new component usage..."
rg -l "EditorVersionHistorySidebarList"
Length of output: 441
web/core/components/editor/version-history/editors/page-version-editor.tsx (3)
18-18: LGTM! Component declaration follows best practices
The renaming from PagesVersionEditor to PageVersionEditor improves clarity, and the component is properly typed and decorated with MobX observer.
Line range hint 19-91: LGTM! Well-structured implementation with proper error handling
The component demonstrates good practices:
- Proper handling of undefined states
- Detailed loading skeleton for better UX
- Well-configured editor with required handlers
18-18: Verify component renaming across the codebase
Let's ensure all references to the old component name PagesVersionEditor have been updated.
✅ Verification successful
Component renaming has been properly implemented
The verification shows that there are no remaining references to the old component name PagesVersionEditor in the codebase. The new component name PageVersionEditor is correctly used in both its definition and imports.
- Component defined in
web/core/components/editor/version-history/editors/page-version-editor.tsx - Properly imported and used in
web/core/components/pages/editor/page-root.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old component name
rg "PagesVersionEditor" --type ts --type tsx
# Search for imports of the new component name to verify usage
rg "PageVersionEditor" --type ts --type tsx
Length of output: 144
Script:
#!/bin/bash
# Search for any remaining references to the old component name without file type restrictions
rg "PagesVersionEditor"
# Search for imports and usage of the new component name
rg "PageVersionEditor"
Length of output: 431
web/core/components/editor/version-history/main-content.tsx (3)
6-6: LGTM: Type definitions are properly updated
The change from TPageVersion to TEditorVersion and pageId to entityId is consistent with the PR's objective of restructuring version management types.
Also applies to: 18-19
25-36: LGTM: Component structure follows best practices
The component name change and props destructuring are well-organized and follow React best practices.
Line range hint 72-76: LGTM: Retry mechanism is well-implemented
The retry logic effectively uses SWR's mutate function with proper loading state management.
web/core/components/pages/editor/page-root.tsx (2)
11-11: LGTM! Import changes align with the new naming convention.
The renamed components better reflect their purposes and maintain consistency with the editor-centric approach.
109-113:
Handle potential undefined entityId.
The entityId prop is set to page.id ?? "", but passing an empty string when page.id is undefined might cause issues with version history functionality.
Let's verify how undefined page IDs are handled in the version history components:
Consider adding a guard clause:
-entityId={page.id ?? ""}
+entityId={page.id}
+isOpen={isVersionsOverlayOpen && Boolean(page.id)}web/core/components/issues/issue-detail/root.tsx (1)
12-12: LGTM: Import statement is correctly structured
The addition of IssueVersionHistory to the existing import statement is clean and follows the project's import conventions.
| async fetchAllVersions(workspaceSlug: string, projectId: string, issueId: string): Promise<TEditorVersion[]> { | ||
| return this.get(`/api/workspaces/${workspaceSlug}/projects/${projectId}/issues/${issueId}/versions/`) | ||
| .then((response) => response?.data) | ||
| .catch((error) => { | ||
| throw error?.response?.data; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and URL construction.
Consider the following improvements:
- Strengthen error handling to preserve stack traces and handle cases where response data might be undefined
- Consider extracting the URL path construction to a constant or utility function for better maintainability
Here's a suggested implementation:
+private readonly BASE_ENDPOINT = '/api/workspaces';
+
async fetchAllVersions(workspaceSlug: string, projectId: string, issueId: string): Promise<TEditorVersion[]> {
- return this.get(`/api/workspaces/${workspaceSlug}/projects/${projectId}/issues/${issueId}/versions/`)
+ const endpoint = `${this.BASE_ENDPOINT}/${workspaceSlug}/projects/${projectId}/issues/${issueId}/versions/`;
+ return this.get(endpoint)
.then((response) => response?.data)
.catch((error) => {
- throw error?.response?.data;
+ throw error?.response?.data ?? error;
});
}📝 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, issueId: string): Promise<TEditorVersion[]> { | |
| return this.get(`/api/workspaces/${workspaceSlug}/projects/${projectId}/issues/${issueId}/versions/`) | |
| .then((response) => response?.data) | |
| .catch((error) => { | |
| throw error?.response?.data; | |
| }); | |
| } | |
| private readonly BASE_ENDPOINT = '/api/workspaces'; | |
| async fetchAllVersions(workspaceSlug: string, projectId: string, issueId: string): Promise<TEditorVersion[]> { | |
| const endpoint = `${this.BASE_ENDPOINT}/${workspaceSlug}/projects/${projectId}/issues/${issueId}/versions/`; | |
| return this.get(endpoint) | |
| .then((response) => response?.data) | |
| .catch((error) => { | |
| throw error?.response?.data ?? error; | |
| }); | |
| } |
| editorRef?.clearEditor(); | ||
| editorRef?.setEditorValue(descriptionHTML); | ||
| }; | ||
| const currentVersionDescription = disabled ? readOnlyEditorRef?.getDocument().html : editorRef?.getDocument().html; |
There was a problem hiding this comment.
Add null safety checks for editor refs
The current version retrieval could throw if editor refs are null. Consider adding more defensive checks.
Consider this safer implementation:
-const currentVersionDescription = disabled ? readOnlyEditorRef?.getDocument().html : editorRef?.getDocument().html;
+const currentVersionDescription = disabled
+ ? readOnlyEditorRef?.getDocument()?.html ?? null
+ : editorRef?.getDocument()?.html ?? 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 currentVersionDescription = disabled ? readOnlyEditorRef?.getDocument().html : editorRef?.getDocument().html; | |
| const currentVersionDescription = disabled | |
| ? readOnlyEditorRef?.getDocument()?.html ?? null | |
| : editorRef?.getDocument()?.html ?? null; |
| const handleRestoreVersion = async (descriptionHTML: string) => { | ||
| editorRef?.clearEditor(); | ||
| editorRef?.setEditorValue(descriptionHTML); | ||
| }; |
There was a problem hiding this comment.
Add error handling to version restore function
The restore handler should validate input and handle potential errors when modifying the editor.
Consider adding error handling:
const handleRestoreVersion = async (descriptionHTML: string) => {
+ if (!descriptionHTML) {
+ console.error("Cannot restore empty version");
+ return;
+ }
+ try {
editorRef?.clearEditor();
editorRef?.setEditorValue(descriptionHTML);
+ } catch (error) {
+ console.error("Failed to restore version:", error);
+ // Consider showing user feedback
+ }
};📝 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 handleRestoreVersion = async (descriptionHTML: string) => { | |
| editorRef?.clearEditor(); | |
| editorRef?.setEditorValue(descriptionHTML); | |
| }; | |
| const handleRestoreVersion = async (descriptionHTML: string) => { | |
| if (!descriptionHTML) { | |
| console.error("Cannot restore empty version"); | |
| return; | |
| } | |
| try { | |
| editorRef?.clearEditor(); | |
| editorRef?.setEditorValue(descriptionHTML); | |
| } catch (error) { | |
| console.error("Failed to restore version:", error); | |
| // Consider showing user feedback | |
| } | |
| }; |
| fetchAllVersions={async (issueId) => { | ||
| if (!workspaceSlug || !projectId) return; | ||
| return await issueVersionService.fetchAllVersions(workspaceSlug.toString(), projectId.toString(), issueId); | ||
| }} | ||
| fetchVersionDetails={async (issueId, versionId) => { | ||
| if (!workspaceSlug || !projectId) return; | ||
| return await issueVersionService.fetchVersionById( | ||
| workspaceSlug.toString(), | ||
| projectId.toString(), | ||
| issueId, | ||
| versionId | ||
| ); | ||
| }} |
There was a problem hiding this comment.
Improve error handling in fetch functions
The fetch functions should handle errors more explicitly and provide better type safety.
Consider this improvement:
fetchAllVersions={async (issueId) => {
if (!workspaceSlug || !projectId) return;
+ try {
return await issueVersionService.fetchAllVersions(
workspaceSlug.toString(),
projectId.toString(),
issueId
);
+ } catch (error) {
+ console.error("Failed to fetch versions:", error);
+ return [];
+ }
}}
fetchVersionDetails={async (issueId, versionId) => {
if (!workspaceSlug || !projectId) return;
+ try {
return await issueVersionService.fetchVersionById(
workspaceSlug.toString(),
projectId.toString(),
issueId,
versionId
);
+ } catch (error) {
+ console.error("Failed to fetch version details:", error);
+ return 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.
| fetchAllVersions={async (issueId) => { | |
| if (!workspaceSlug || !projectId) return; | |
| return await issueVersionService.fetchAllVersions(workspaceSlug.toString(), projectId.toString(), issueId); | |
| }} | |
| fetchVersionDetails={async (issueId, versionId) => { | |
| if (!workspaceSlug || !projectId) return; | |
| return await issueVersionService.fetchVersionById( | |
| workspaceSlug.toString(), | |
| projectId.toString(), | |
| issueId, | |
| versionId | |
| ); | |
| }} | |
| fetchAllVersions={async (issueId) => { | |
| if (!workspaceSlug || !projectId) return; | |
| try { | |
| return await issueVersionService.fetchAllVersions( | |
| workspaceSlug.toString(), | |
| projectId.toString(), | |
| issueId | |
| ); | |
| } catch (error) { | |
| console.error("Failed to fetch versions:", error); | |
| return []; | |
| } | |
| }} | |
| fetchVersionDetails={async (issueId, versionId) => { | |
| if (!workspaceSlug || !projectId) return; | |
| try { | |
| return await issueVersionService.fetchVersionById( | |
| workspaceSlug.toString(), | |
| projectId.toString(), | |
| issueId, | |
| versionId | |
| ); | |
| } catch (error) { | |
| console.error("Failed to fetch version details:", error); | |
| return null; | |
| } | |
| }} |
| const { projectId, workspaceSlug } = useParams(); | ||
| // router |
There was a problem hiding this comment.
Add validation for required route parameters
The component assumes workspaceSlug and projectId will be available from route params, but there's no validation or error handling if they're missing.
Consider adding validation:
-const { projectId, workspaceSlug } = useParams();
+const params = useParams();
+const workspaceSlug = params.workspaceSlug;
+const projectId = params.projectId;
+
+if (!workspaceSlug || !projectId) {
+ throw new Error("Workspace slug and project ID are required");
+}📝 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 { projectId, workspaceSlug } = useParams(); | |
| // router | |
| const params = useParams(); | |
| const workspaceSlug = params.workspaceSlug; | |
| const projectId = params.projectId; | |
| if (!workspaceSlug || !projectId) { | |
| throw new Error("Workspace slug and project ID are required"); | |
| } | |
| // router |
| {/* version history overlay */} | ||
| <IssueVersionHistory /> |
There was a problem hiding this comment.
💡 Codebase verification
Props are required but not passed to IssueVersionHistory component
The verification reveals that IssueVersionHistory requires props but they aren't being passed:
- Required props from interface:
disablededitorRefissueIdreadOnlyEditorRef
The component also uses workspace and project context internally via useParams(), so those don't need to be passed explicitly.
🔗 Analysis chain
Verify required props for IssueVersionHistory component
The component is added without any props, but it might need access to the current issue context (workspaceSlug, projectId, issueId) for fetching and displaying version history. Consider:
- Passing necessary context props
- Adding error boundaries for the overlay component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if IssueVersionHistory component requires props in its implementation
# Search for props definition in the component
ast-grep --pattern 'interface $props {
$$$
}' web/core/components/issues/issue-detail/version-history.tsx
# Search for context usage in the component
rg -A 5 'useIssueDetail|useParams|workspaceSlug|projectId|issueId' web/core/components/issues/issue-detail/version-history.tsx
Length of output: 1663
Summary by CodeRabbit
Release Notes
New Features
IssueVersionHistorycomponent for enhanced version tracking in issue details.IssueVersionEditorcomponent for managing version edits.EditorVersionHistoryOverlayfor displaying version history in the editor.Improvements
TEditorVersionfor consistency across components.Bug Fixes
Documentation