[WEB-3748, 3749] feat: work item description version history#6863
[WEB-3748, 3749] feat: work item description version history#6863sriramveeraghanta merged 16 commits intopreviewfrom
Conversation
…t/work-item-versions
WalkthroughThis pull request introduces new URL endpoints and view methods for handling description versions of intake work items and work items. It renames and restructures existing endpoints, updates serializers and permission checks, and revises view logic to trigger asynchronous tasks on create/update operations. Additionally, it enhances localization by adding new translation sections, updates editor hook signatures to allow optional update emissions, and provides new TypeScript declarations. New React components and service classes are also implemented for managing and displaying description version information. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant A as APIServer
participant IV as IntakeWorkItemDescriptionVersionEndpoint
participant DB as Database
C->>A: GET /workspaces/{slug}/projects/{project_id}/intake-work-items/{work_item_id}/description-versions/
A->>IV: Invoke get() method
IV->>DB: Query paginated description versions
DB-->>IV: Return version list
IV-->>A: Send JSON response
A-->>C: 200 OK with version data
sequenceDiagram
participant C as Client
participant A as APIServer
participant WV as WorkItemDescriptionVersionEndpoint
participant PM as ProjectMember Model
participant DB as Database
C->>A: GET /workspaces/{slug}/projects/{project_id}/work-items/{work_item_id}/description-versions/[version_id]
A->>WV: Invoke get() method
WV->>PM: Check user permissions
PM-->>WV: Return permission status
WV->>DB: Fetch description version(s)
DB-->>WV: Return requested data
WV-->>A: JSON response
A-->>C: 200 OK with version details
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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 (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
aaryan610
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (35)
packages/i18n/src/locales/vi-VN/translations.json (2)
2325-2329: Cycle Block Validation:
The"cycle"section now uses a plural formatting pattern with"label": "{count, plural, one {chu kỳ} other {chu kỳ}}"and a message for the empty state. In Vietnamese, pluralization is often handled by context rather than word changes. Please double-check that this behavior is consistent with other locale files and aligns with your UI’s expectations.
2335-2340: New Description Versions Section:
The newly added"description_versions"block defines the keys"last_edited_by","previously_edited_by", and"edited_by"with clear and straightforward translations. These entries will support the added version history feature effectively. Please verify that these translations align with the corresponding labels in other language files and match your overall tone.packages/i18n/src/locales/de/translations.json (1)
2324-2340: Minor Whitespace and Formatting Adjustments in Module-Related KeysThe blocks for
"project_modules","cycle", and"module"appear to show slight whitespace or formatting modifications. These changes do not affect functionality but please ensure that the formatting remains consistent with the rest of the locale files and follows the project’s JSON style guidelines.packages/i18n/src/locales/ru/translations.json (1)
2379-2382: New "description_versions" Section AddedThe newly added translation block provides keys for version history—specifically,
"last_edited_by","previously_edited_by", and"edited_by". The Russian translations are clear and appropriate. Please ensure these keys are consistently used with the corresponding functionality in the UI and that similar changes are mirrored across locales for uniform behavior.packages/i18n/src/locales/ro/translations.json (1)
2373-2377: New Translation Section for Version HistoryThe new
"description_versions"section adds the keys"last_edited_by","previously_edited_by", and"edited_by"with their respective Romanian translations. This is a useful enhancement for supporting version history information. Please verify that these new entries follow the same structure as in other locale files for consistency.packages/i18n/src/locales/ua/translations.json (1)
2337-2341: New Localization Keys for Description Versions AddedThe new
"description_versions"section with keys"last_edited_by","previously_edited_by", and"edited_by"is introduced correctly to support the version history feature for work items. The translations are concise and clear. Please ensure that these terms remain consistent with the corresponding keys in other locale files for uniformity across the application.packages/i18n/src/locales/pl/translations.json (1)
502-503: Formatting Adjustment: Removal of Unnecessary Blank Line
The removal of the blank line between the"new_password_must_be_different_from_old_password"and"edited"entries improves the file’s visual consistency without affecting functionality.web/core/components/issues/description-input.tsx (1)
8-27: Consider adding JSDoc comments for the new propertiesWhile the code changes look good, adding JSDoc comments for the new ref properties would improve code documentation and help other developers understand their purpose and usage.
- editorReadOnlyRef?: React.RefObject<EditorReadOnlyRefApi>; - editorRef?: React.RefObject<EditorRefApi>; + /** Reference to access the read-only editor API methods (for version history, copying, etc.) */ + editorReadOnlyRef?: React.RefObject<EditorReadOnlyRefApi>; + /** Reference to access the editable editor API methods (for version history, copying, etc.) */ + editorRef?: React.RefObject<EditorRefApi>;web/core/components/core/description-versions/dropdown-item.tsx (1)
14-32: Well-implemented dropdown item component with clean structure.The component follows React best practices and properly handles version data display. Consider these minor improvements:
- Add accessibility attributes for better screen reader support
- Include a fallback for when creator information is unavailable
- <CustomMenu.MenuItem key={version.id} className="flex items-center gap-1" onClick={() => onClick(version.id)}> + <CustomMenu.MenuItem + key={version.id} + className="flex items-center gap-1" + onClick={() => onClick(version.id)} + aria-label={`Version by ${versionCreator?.display_name || 'Unknown user'} from ${calculateTimeAgo(version.last_saved_at)}`} + > <span className="flex-shrink-0"> <Avatar name={versionCreator?.display_name} size="sm" src={versionCreator?.avatar_url} /> </span> <p className="text-xs text-custom-text-200 flex items-center gap-1.5"> - <span className="font-medium">{versionCreator?.display_name}</span> + <span className="font-medium">{versionCreator?.display_name || 'Unknown user'}</span> <span>{calculateTimeAgo(version.last_saved_at)}</span> </p> </CustomMenu.MenuItem>apiserver/plane/app/urls/intake.py (1)
60-69: URL patterns for description versions look good, but consider unique naming.The URL patterns are well-structured and follow Django conventions, providing proper access to intake work item description versions.
Both URL patterns use the same name ("intake-work-item-versions"). Consider giving the second URL pattern a more specific name to differentiate it, such as "intake-work-item-version-detail":
path( "workspaces/<str:slug>/projects/<uuid:project_id>/intake-work-items/<uuid:work_item_id>/description-versions/<uuid:pk>/", IntakeWorkItemDescriptionVersionEndpoint.as_view(), - name="intake-work-item-versions", + name="intake-work-item-version-detail", ),This makes URL reverse lookup more explicit when you need to reference a specific version.
web/core/components/core/description-versions/modal.tsx (4)
54-64: Add error handling to clipboard functionality and use translation for message.The copy functionality works, but lacks error handling for the promise and has a hardcoded message that should use translations.
const handleCopyMarkdown = useCallback(() => { if (!editorRef.current) return; - copyTextToClipboard(editorRef.current.getMarkDown()).then(() => - setToast({ - type: TOAST_TYPE.SUCCESS, - title: t("toast.success"), - message: "Markdown copied to clipboard.", - }) - ); + copyTextToClipboard(editorRef.current.getMarkDown()) + .then(() => { + setToast({ + type: TOAST_TYPE.SUCCESS, + title: t("toast.success"), + message: t("common.copied_to_clipboard"), + }); + }) + .catch(() => { + setToast({ + type: TOAST_TYPE.ERROR, + title: t("toast.error"), + message: t("common.errors.default.message"), + }); + }); }, [t]);
66-113: Improve accessibility for the modal interface and navigation.The modal could benefit from better accessibility features for screen readers.
Consider adding:
- An aria-label for the modal to describe its purpose
- Appropriate aria-labels for navigation buttons
- A more semantic header structure
- <ModalCore isOpen={isOpen} handleClose={handleClose}> + <ModalCore isOpen={isOpen} handleClose={handleClose} aria-label={t("description_versions.modal_title", "Description Version History")}> <div className="p-4" data-prevent-outside-click> {/* Header */} - <div className="flex items-center justify-between gap-2 py-0.5"> + <div className="flex items-center justify-between gap-2 py-0.5" role="heading" aria-level={1}> <div className="flex-shrink-0 flex items-center gap-2 text-sm"> <p className="flex items-center gap-1"> {t("description_versions.edited_by")} <span className="flex-shrink-0"> <Avatar size="sm" src={versionCreator?.avatar_url} name={versionCreator?.display_name} /> </span> </p> <p className="flex-shrink-0 text-custom-text-200"> {calculateTimeAgo(activeVersionDetails?.last_saved_at ?? "")} </p> </div> <div className="flex-shrink-0 flex items-center"> <button type="button" onClick={() => handleNavigation("prev")} + aria-label={t("common.previous")} className={cn( "size-6 text-custom-text-200 grid place-items-center rounded outline-none transition-colors", { "hover:bg-custom-background-80": !isPrevDisabled, "opacity-50": isPrevDisabled, } )} disabled={isPrevDisabled} > <ChevronLeft className="size-4" /> </button> <button type="button" onClick={() => handleNavigation("next")} + aria-label={t("common.next")} className={cn( "size-6 text-custom-text-200 grid place-items-center rounded outline-none transition-colors", { "hover:bg-custom-background-80": !isNextDisabled, "opacity-50": isNextDisabled, } )} disabled={isNextDisabled} > <ChevronRight className="size-4" /> </button> </div> </div>
50-51: Simplify workspace ID retrieval with optional chaining.The current workspaceId derivation is more complex than needed.
- const workspaceId = getWorkspaceBySlug(workspaceSlug?.toString() ?? "")?.id ?? ""; + const workspaceId = getWorkspaceBySlug(workspaceSlug)?.id ?? "";Since
workspaceSlugis already typed as a string in your props, there's no need to calltoString()on it.
146-159: Add screen reader text to the copy button for better accessibility.The copy button uses only an icon, which isn't descriptive for screen readers.
<Tooltip tooltipContent={t("common.actions.copy_markdown")}> <button type="button" className={cn( "flex-shrink-0", getButtonStyling("neutral-primary", "sm"), "border-none grid place-items-center" )} onClick={handleCopyMarkdown} + aria-label={t("common.actions.copy_markdown")} > <Copy className="size-4" /> + <span className="sr-only">{t("common.actions.copy_markdown")}</span> </button> </Tooltip>apiserver/plane/app/views/issue/version.py (2)
87-107: Consider using a constant for role code instead of hardcoded valueThe code uses a hardcoded value of
5for the role check. Consider using a constant or enum fromROLEto improve readability and maintainability.- role=5, + role=ROLE.GUEST,
75-75: Maintain consistency in class namingThe class has been renamed to
WorkItemDescriptionVersionEndpoint, but the model it uses is still calledIssueDescriptionVersion, creating an inconsistency in naming. Consider whether the model also needs to be renamed or if this inconsistency is intentional.web/core/services/issue/work_item_version.service.ts (3)
30-32: Enhance error handling with more contextThe current error handling only throws the error response data without any additional context. This makes debugging harder when issues occur. Consider adding more context to the error.
.catch((error) => { - throw error?.response?.data; + const errorData = error?.response?.data; + throw { + message: 'Failed to list description versions', + status: error?.response?.status, + data: errorData + }; });
45-47: Apply the same error handling improvement hereSimilar to the previous method, enhance the error handling here as well for consistency.
.catch((error) => { - throw error?.response?.data; + const errorData = error?.response?.data; + throw { + message: 'Failed to retrieve description version', + status: error?.response?.status, + data: errorData + }; });
16-19: Consider adding type annotation for constructor parameterThe constructor parameter
serviceTypehas a type inference from the default value, but for clarity and to follow best practices, consider adding an explicit type annotation.- constructor(serviceType: TIssueServiceType = EIssueServiceType.WORK_ITEMS) { + constructor(serviceType: TIssueServiceType = EIssueServiceType.WORK_ITEMS) {web/core/components/issues/issue-detail/main-content.tsx (2)
152-171: Consider consistent conditional rendering with isArchived checkThe
DescriptionVersionsRootcomponent is conditionally rendered based onisEditable, but unlike other components (likeIssueTitleInput), it doesn't check forisArchived. For consistency, consider applying the same condition.- {isEditable && ( + {isEditable && !isArchived && (
47-49: Consider initializing the ref with useRef<EditorRefApi | null>To be more explicit about the possibility of the ref being null, consider adding the null union type.
- const editorRef = useRef<EditorRefApi>(null); + const editorRef = useRef<EditorRefApi | null>(null);web/core/services/inbox/intake-work_item_version.service.ts (3)
8-41: Consider abstracting common functionality between version servicesThere's significant code duplication between this service and
WorkItemVersionService. Consider creating a base class or using composition to share common functionality.You could create a base version service class that accepts endpoint configuration:
export class BaseVersionService extends APIService { constructor(private endpointPath: string) { super(API_BASE_URL); } async listDescriptionVersions( workspaceSlug: string, projectId: string, itemId: string ): Promise<TDescriptionVersionsListResponse> { return this.get( `/api/workspaces/${workspaceSlug}/projects/${projectId}/${this.endpointPath}/${itemId}/description-versions/` ) .then((response) => response?.data) .catch((error) => { throw error?.response?.data; }); } // ...other methods } // Then implement specific services: export class IntakeWorkItemVersionService extends BaseVersionService { constructor() { super('intake-work-items'); } } export class WorkItemVersionService extends BaseVersionService { constructor() { super('work-items'); } }
22-24: Enhance error handling with more contextThe current error handling only throws the error response data without any additional context. This makes debugging harder when issues occur.
.catch((error) => { - throw error?.response?.data; + const errorData = error?.response?.data; + throw { + message: 'Failed to list intake work item description versions', + status: error?.response?.status, + data: errorData + }; });
37-39: Apply the same error handling improvement hereSimilar to the previous method, enhance the error handling here as well for consistency.
.catch((error) => { - throw error?.response?.data; + const errorData = error?.response?.data; + throw { + message: 'Failed to retrieve intake work item description version', + status: error?.response?.status, + data: errorData + }; });web/core/components/inbox/content/issue-root.tsx (3)
3-3: Update import organization for better readability.The import list is growing. Consider organizing imports alphabetically within each grouping for easier maintenance and readability.
137-137: Add error logging for issue update failures.The error handling is now suppressing the error object completely. Consider logging the error for debugging purposes while keeping the error handling intact.
- } catch { + } catch (error) { + console.error("Error updating inbox issue:", error);
221-250: Well-structured implementation of version history UI.The component structure effectively integrates the version history feature. The conditional rendering based on
isEditableensures appropriate access control, and the proper props are passed to theDescriptionVersionsRootcomponent.A few suggestions:
- Consider extracting the version-related props into a separate variable for readability
- Add a tooltip explaining the version history feature to users
web/core/components/issues/peek-overview/issue-detail.tsx (3)
2-2: Update import organization for better readability.The React import line is growing. Consider organizing imports alphabetically within each grouping for easier maintenance and readability.
27-27: Initialize service outside component for better performance.The service initialization happens inside the component definition. Move this outside to prevent unnecessary re-initialization on each render.
// services init -const workItemVersionService = new WorkItemVersionService(); +const workItemVersionService = new WorkItemVersionService(); interface IPeekOverviewIssueDetails {
155-167: Consider simplifying the version service method calls.The nested structure of the version service method calls is a bit verbose. Consider creating a helper function to simplify these calls.
fetchHandlers={{ listDescriptionVersions: (issueId) => - workItemVersionService.listDescriptionVersions( - workspaceSlug, - issue.project_id?.toString() ?? "", - issueId - ), + workItemVersionService.listDescriptionVersions(workspaceSlug, issue.project_id?.toString() ?? "", issueId), retrieveDescriptionVersion: (issueId, versionId) => - workItemVersionService.retrieveDescriptionVersion( - workspaceSlug, - issue.project_id?.toString() ?? "", - issueId, - versionId - ), + workItemVersionService.retrieveDescriptionVersion(workspaceSlug, issue.project_id?.toString() ?? "", issueId, versionId), }}web/core/components/core/description-versions/root.tsx (3)
69-74: Consider using a single state update for modal closure.The current implementation uses
setTimeoutto reset the active version ID after the modal closes. Consider using the modal's onClose callback to handle both state updates.handleClose={() => { setIsModalOpen(false); - setTimeout(() => { - setActiveVersionId(null); - }, 300); + // If you need the delay, explain why in a comment + // Otherwise, consider: + setActiveVersionId(null); }}
84-94: Consider adding empty state handling.The component currently disables the dropdown when there are no versions, but it would be better to also show a message explaining why it's disabled.
<div className={cn(className)}> + {versionsCount === 0 && ( + <div className="text-xs text-custom-text-300"> + No version history available + </div> + )} <DescriptionVersionsDropdown disabled={versionsCount === 0} entityInformation={entityInformation} onVersionClick={(versionId) => { setIsModalOpen(true); setActiveVersionId(versionId); }} versions={versions} /> </div>
1-97: Add error handling for version operations.The component doesn't currently handle errors that might occur during fetching operations. Consider adding error handling for a better user experience.
Add error states and appropriate error messages:
// fetch versions list -const { data: versionsListResponse } = useSWR( +const { data: versionsListResponse, error: versionsListError } = useSWR( entityId ? `DESCRIPTION_VERSIONS_LIST_${entityId}` : null, entityId ? () => fetchHandlers.listDescriptionVersions(entityId) : null ); // fetch active version details -const { data: activeVersionResponse } = useSWR( +const { data: activeVersionResponse, error: activeVersionError } = useSWR( entityId && activeVersionId ? `DESCRIPTION_VERSION_DETAILS_${activeVersionId}` : null, entityId && activeVersionId ? () => fetchHandlers.retrieveDescriptionVersion(entityId, activeVersionId) : null ); // Later in the JSX: <div className={cn(className)}> + {(versionsListError || activeVersionError) && ( + <div className="text-xs text-red-500"> + Error loading version history + </div> + )} <DescriptionVersionsDropdown // existing props /> </div>apiserver/plane/app/views/intake/base.py (2)
294-300: Consider adding error handling for the background task.The asynchronous
issue_description_version_taskis triggered without any fallback or compensation if it fails. If this data is critical, consider implementing error handling or transactional logic to recover from task failures.
620-641: Consider returning 403 Forbidden instead of 400 Bad Request.When the user lacks permission to view an issue, returning 400 may be misleading. An HTTP 403 would more accurately signal a permissions deficiency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
apiserver/plane/app/urls/intake.py(2 hunks)apiserver/plane/app/urls/issue.py(2 hunks)apiserver/plane/app/views/__init__.py(2 hunks)apiserver/plane/app/views/intake/base.py(6 hunks)apiserver/plane/app/views/issue/base.py(1 hunks)apiserver/plane/app/views/issue/version.py(4 hunks)packages/constants/src/issue/common.ts(1 hunks)packages/editor/src/core/hooks/use-editor.ts(1 hunks)packages/editor/src/core/hooks/use-read-only-editor.ts(1 hunks)packages/editor/src/core/types/editor.ts(1 hunks)packages/i18n/src/locales/cs/translations.json(1 hunks)packages/i18n/src/locales/de/translations.json(2 hunks)packages/i18n/src/locales/en/translations.json(2 hunks)packages/i18n/src/locales/es/translations.json(1 hunks)packages/i18n/src/locales/fr/translations.json(1 hunks)packages/i18n/src/locales/id/translations.json(1 hunks)packages/i18n/src/locales/it/translations.json(2 hunks)packages/i18n/src/locales/ja/translations.json(1 hunks)packages/i18n/src/locales/ko/translations.json(1 hunks)packages/i18n/src/locales/pl/translations.json(2 hunks)packages/i18n/src/locales/pt-BR/translations.json(1 hunks)packages/i18n/src/locales/ro/translations.json(1 hunks)packages/i18n/src/locales/ru/translations.json(1 hunks)packages/i18n/src/locales/sk/translations.json(1 hunks)packages/i18n/src/locales/ua/translations.json(2 hunks)packages/i18n/src/locales/vi-VN/translations.json(1 hunks)packages/i18n/src/locales/zh-CN/translations.json(1 hunks)packages/i18n/src/locales/zh-TW/translations.json(1 hunks)packages/types/src/description_version.d.ts(1 hunks)packages/types/src/index.d.ts(1 hunks)packages/types/src/issues/issue.d.ts(1 hunks)web/core/components/core/description-versions/dropdown-item.tsx(1 hunks)web/core/components/core/description-versions/dropdown.tsx(1 hunks)web/core/components/core/description-versions/index.ts(1 hunks)web/core/components/core/description-versions/modal.tsx(1 hunks)web/core/components/core/description-versions/root.tsx(1 hunks)web/core/components/inbox/content/issue-root.tsx(7 hunks)web/core/components/issues/description-input.tsx(6 hunks)web/core/components/issues/issue-detail/main-content.tsx(4 hunks)web/core/components/issues/issue-detail/root.tsx(1 hunks)web/core/components/issues/peek-overview/issue-detail.tsx(3 hunks)web/core/components/issues/peek-overview/view.tsx(2 hunks)web/core/services/inbox/index.ts(1 hunks)web/core/services/inbox/intake-work_item_version.service.ts(1 hunks)web/core/services/issue/index.ts(1 hunks)web/core/services/issue/work_item_version.service.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (11)
web/core/components/core/description-versions/dropdown-item.tsx (1)
packages/types/src/description_version.d.ts (1)
TDescriptionVersion(1-10)
apiserver/plane/app/views/issue/base.py (1)
apiserver/plane/app/serializers/issue.py (1)
IssueDetailSerializer(713-719)
web/core/components/core/description-versions/dropdown.tsx (3)
web/core/components/core/description-versions/root.tsx (1)
TDescriptionVersionEntityInformation(11-16)packages/types/src/description_version.d.ts (1)
TDescriptionVersion(1-10)web/core/components/core/description-versions/dropdown-item.tsx (1)
DescriptionVersionsDropdownItem(14-32)
web/core/components/issues/description-input.tsx (2)
packages/editor/src/core/types/editor.ts (2)
EditorReadOnlyRefApi(79-94)EditorRefApi(96-113)web/core/store/router.store.ts (1)
workspaceSlug(69-71)
web/core/services/issue/work_item_version.service.ts (1)
packages/types/src/issues/issue.d.ts (1)
TIssueServiceType(123-123)
web/core/components/core/description-versions/modal.tsx (2)
packages/types/src/description_version.d.ts (1)
TDescriptionVersion(1-10)packages/editor/src/core/types/editor.ts (1)
EditorReadOnlyRefApi(79-94)
apiserver/plane/app/views/issue/version.py (1)
apiserver/plane/app/views/intake/base.py (1)
get(620-681)
apiserver/plane/app/views/intake/base.py (1)
apiserver/plane/app/serializers/issue.py (2)
IssueDetailSerializer(713-719)IssueDescriptionVersionDetailSerializer(796-815)
web/core/components/issues/issue-detail/main-content.tsx (4)
web/core/services/issue/work_item_version.service.ts (1)
WorkItemVersionService(13-49)packages/editor/src/core/types/editor.ts (1)
EditorRefApi(96-113)web/core/components/issues/description-input.tsx (1)
IssueDescriptionInput(39-179)web/core/components/core/description-versions/root.tsx (1)
DescriptionVersionsRoot(30-97)
web/core/components/core/description-versions/root.tsx (3)
packages/types/src/description_version.d.ts (2)
TDescriptionVersionsListResponse(19-29)TDescriptionVersionDetails(12-17)web/core/components/core/description-versions/modal.tsx (1)
DescriptionVersionsModal(29-182)web/core/components/core/description-versions/dropdown.tsx (1)
DescriptionVersionsDropdown(21-59)
apiserver/plane/app/urls/issue.py (1)
apiserver/plane/app/views/issue/version.py (2)
WorkItemDescriptionVersionEndpoint(75-148)IssueVersionEndpoint(23-72)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (70)
packages/i18n/src/locales/vi-VN/translations.json (1)
2330-2334: Module Block Consistency:
The"module"section mirrors the approach used in the"cycle"block by providing a singular/plural label and an empty state message ("no_module": "Không có mô-đun"). This is consistent with typical Vietnamese usage where the singular form is reused. If this consistency is intended across your translation files, then the implementation looks good.packages/i18n/src/locales/de/translations.json (1)
2341-2345: New "description_versions" Translation Section AddedThe new section
"description_versions"with keys"last_edited_by","previously_edited_by", and"edited_by"has been introduced to support the version history feature. The German translations seem appropriate. Please verify that similar updates have been applied consistently across other locale files.packages/i18n/src/locales/it/translations.json (1)
2378-2382: Localization strings added successfully for description versions.The Italian translations for the description version feature have been properly added, following the structure and naming patterns of the existing translation file. The translations accurately represent the context of who edited a description.
web/core/services/issue/index.ts (1)
10-10: Service export added correctly.The work item version service export is properly integrated into the issue service index, making it accessible for components that need to interact with work item version history.
packages/types/src/index.d.ts (1)
6-6: Type declaration export added properly.The description version type definitions are now correctly exported from the main types index file, making them available throughout the application wherever types are imported.
web/core/services/inbox/index.ts (1)
2-2: Intake work item version service exported correctly.The intake work item version service export has been properly added to the inbox service index, allowing components to access version history for intake work items.
web/core/components/core/description-versions/index.ts (1)
1-2: Well-structured module export pattern!Using a barrel file pattern to re-export components from the description versions module is a good practice. This simplifies imports for consumers and provides a cleaner API.
packages/constants/src/issue/common.ts (1)
44-44:Details
❓ Verification inconclusive
Good addition to the enum for the new work item feature.
The addition of
WORK_ITEMS = "work-items"to theEIssueServiceTypeenum aligns with the PR objective of introducing version history for work items. The naming follows the existing pattern.Consider running this script to identify any switch statements or conditional blocks that might need to be updated to handle this new enum value:
🏁 Script executed:
#!/bin/bash # Find any switch statements or conditionals using EIssueServiceType that might need to be updated # Look for switch statements on EIssueServiceType rg -A 10 -B 2 "switch.*EIssueServiceType" # Look for if/else chains comparing against EIssueServiceType values rg -A 5 -B 2 "if.*EIssueServiceType\.(ISSUES|EPICS)"Length of output: 10955
ENUM update approved – please verify WORK_ITEMS handling in conditionals
The addition of
WORK_ITEMS = "work-items"follows the established pattern and supports the new work item version history feature. However, our search results indicate that existing conditional logic in files likeweb/core/services/issue/issue.service.tsand the related stores only explicitly check forISSUESandEPICS. Please review these conditionals and switch statements to determine if they need to be updated to account for the newWORK_ITEMSvalue or if a default behavior is acceptable.web/core/components/issues/issue-detail/root.tsx (1)
369-369:Details
❓ Verification inconclusive
Verify archive status handling inconsistency.
The
isEditableprop is now passed directly toIssueMainContentinstead of using!is_archived && isEditable, whileIssueDetailsSidebarstill uses the combined condition on line 382. This inconsistency might lead to confusion.Please verify that the
IssueMainContentcomponent now properly handles the archived state internally. If not, this change might inadvertently allow editing archived issues.
🏁 Script executed:
#!/bin/bash # Check how IssueMainContent handles the archive state # Look for is_archived or isArchived in the IssueMainContent component rg "isArchived|is_archived" "web/core/components/issues/issue-detail/main-content.tsx" # Check the props and any conditions related to editability rg -A 5 -B 5 "isEditable.*prop|interface.*IssueMainContent" "web/core/components/issues/issue-detail/main-content.tsx"Length of output: 1194
Verify archive state handling across components.
The verification confirms that
IssueMainContentnow receives bothisEditableandisArchivedas separate props and enforces edit restrictions internally (e.g., via conditions likedisabled={isArchived || !isEditable}), whileIssueDetailsSidebarstill gets the combined condition (!is_archived && isEditable). Please verify that this design difference is intentional and that archived issues remain properly non-editable where expected.packages/i18n/src/locales/id/translations.json (1)
2373-2377: New Description Versions Section AddedThe new
"description_versions"block has been correctly introduced with the keys"last_edited_by","previously_edited_by", and"edited_by". The translations ("Terakhir disunting oleh", "Sebelumnya disunting oleh", "Disunting oleh") follow the same naming convention and style as the rest of the file. Please ensure the same section is consistently added to other locale files if not already present.packages/i18n/src/locales/zh-CN/translations.json (1)
2379-2383: Translation section for version history looks good.The new "description_versions" section adds translations for displaying edit history information - last editor, previous editors, and general editing attribution. These translations align with the PR objective of introducing version history for work items.
packages/editor/src/core/types/editor.ts (1)
87-87: API enhancement looks good.The method signature for
setEditorValuehas been updated to include an optionalemitUpdateparameter, allowing control over whether updates are emitted when setting editor content. This provides better flexibility for displaying version history without triggering unnecessary updates.packages/i18n/src/locales/es/translations.json (1)
2381-2385: New Translation Keys for Description Versions Added
The new"description_versions"section with keys"last_edited_by","previously_edited_by", and"edited_by"is correctly added and follows the naming and stylistic conventions used throughout the file. Ensure that these keys are referenced appropriately in the corresponding UI components or view logic.packages/i18n/src/locales/fr/translations.json (1)
2377-2383: New "description_versions" Localization Section AddedThe new section introduces the keys
"last_edited_by","previously_edited_by", and"edited_by"with clear French translations. Their naming and values are consistent with the existing JSON structure and naming conventions, ensuring a smooth integration with the rest of the localization file.packages/i18n/src/locales/pt-BR/translations.json (1)
2374-2378: Translation additions look good for describing edit history.The new "description_versions" section with appropriate Portuguese translations for edit history labels aligns perfectly with the PR objective of introducing version history for work items.
packages/types/src/issues/issue.d.ts (1)
123-123: Type extension correctly supports the new work item versions feature.This change appropriately extends the
TIssueServiceTypeto includeEIssueServiceType.WORK_ITEMS, which is necessary for implementing the version history feature for work items as described in the PR objectives.packages/i18n/src/locales/sk/translations.json (1)
2378-2382: Good addition of translation keys for description version history.This addition of the "description_versions" section with translation keys for showing edit history information is a great enhancement for internationalization support. It aligns well with the version history feature being introduced.
apiserver/plane/app/views/__init__.py (2)
147-147: LGTM: Replacing IssueDescriptionVersionEndpoint with WorkItemDescriptionVersionEndpoint.The import statement properly maintains both old and new endpoints, supporting a transition to the work-item-based approach.
187-191: LGTM: Added IntakeWorkItemDescriptionVersionEndpoint import.The addition of IntakeWorkItemDescriptionVersionEndpoint to the imports from .intake.base is consistent with the PR's objective of introducing version history for intake work items.
packages/editor/src/core/hooks/use-editor.ts (1)
148-149: Added flexibility with optional emitUpdate parameter.This change enhances the setEditorValue method by making it more flexible with the optional emitUpdate parameter. This is useful for controlling whether setting content should trigger update events, which is important when loading version history.
apiserver/plane/app/views/issue/base.py (1)
634-636: Enhanced serialization for issue activity trackingThe serializer has been upgraded from
IssueSerializertoIssueDetailSerializerfor storing the current instance state. This adds two important fields to the tracked data:
description_html: HTML formatted version of the descriptionis_subscribed: Subscription status of the userThis change is directly aligned with the work item version history feature, as it ensures the HTML description is properly captured for version comparison.
web/core/components/issues/peek-overview/view.tsx (2)
189-191: Allow description interaction for archived issuesThe code now enables interaction with issue descriptions for archived issues by removing the
is_archivedcondition from the disabled prop. This supports the new version history feature by allowing users to view historical versions of descriptions even for archived work items.
229-230: Consistent behavior for all view modesThis change maintains consistency with the earlier instance by removing the
is_archivedcheck from the disabled prop in the full-screen view mode as well, ensuring consistent behavior across all view modes.packages/i18n/src/locales/ja/translations.json (1)
2379-2383: Looks good - appropriate translations added for the new version history feature.The Japanese translations for the new "description_versions" section are correctly implemented, providing proper localization for the version history feature. This aligns with the PR's objective of tracking changes made to work items over time.
packages/editor/src/core/hooks/use-read-only-editor.ts (1)
80-81: Good addition of the emitUpdate parameter.The modification to the
setEditorValuemethod signature enhances flexibility by allowing callers to specify whether to emit an update when setting content. This maintains backward compatibility through the default value while supporting the new version history functionality.packages/i18n/src/locales/pl/translations.json (1)
2327-2342: New Translation Section "description_versions" Added
A new section named"description_versions"has been introduced with the keys"last_edited_by","previously_edited_by", and"edited_by", each accompanied by appropriate Polish translations. This enhancement clearly supports the version history feature within the application’s localization strategy.
Please verify that similar updates have been applied consistently across the other locale files to maintain uniformity in naming and structure.packages/i18n/src/locales/ko/translations.json (1)
2379-2385: New "description_versions" Section for Version HistoryThe new section with keys
"last_edited_by","previously_edited_by", and"edited_by"is clear, follows the existing naming conventions, and integrates well with the rest of the JSON structure. This addition will support the new version history feature by providing localized labels in Korean.packages/i18n/src/locales/en/translations.json (2)
633-634: LGTM: Added translation for copying markdownThe addition of the "copy_markdown" translation key aligns with the new functionality for copying markdown content from work item descriptions. This is a useful feature for users who want to share or reuse content.
2212-2216: LGTM: Good addition of description version history translationsThese translation keys provide the necessary text for the new version history feature, allowing users to see who edited a description and when. This is an essential part of implementing the work item version history feature mentioned in the PR objectives.
web/core/components/issues/description-input.tsx (6)
8-8: LGTM: Added necessary editor API importsThe import of
EditorReadOnlyRefApiandEditorRefApitypes from the editor package is necessary for implementing the refs that will enable version history functionality.
26-27: LGTM: Added editor ref properties to support version historyThese new properties allow parent components to access the editor instances, which is essential for implementing features like:
- Capturing version history of descriptions
- Supporting markdown copying functionality
- Exposing editor methods to parent components
The optional nature of these props ensures backward compatibility.
42-43: LGTM: Properly destructured the new ref propertiesThe new editor ref properties are correctly destructured from the props object, following the component's existing pattern.
61-63: LGTM: Improved code organization for workspaceId derivationMoving the workspace lookup earlier in the component and deriving the workspaceId as soon as possible is a good practice. This ensures the ID is available throughout the component and makes the code more maintainable.
157-157: LGTM: Added ref to the editor componentThe
editorRefis correctly passed to the RichTextEditor component, allowing parent components to access the editor instance and its methods like getMarkDown(), which would be necessary for version history and copy markdown functionality.
167-167: LGTM: Added ref to the read-only editor componentThe
editorReadOnlyRefis correctly passed to the RichTextReadOnlyEditor component, ensuring that even in read-only mode, parent components can access the editor's content for version history purposes.packages/i18n/src/locales/cs/translations.json (1)
2377-2381: Translation strings added for version history feature.The new section for "description_versions" adds three translation strings related to the version history feature for work items. The translations are clear and consistent with the rest of the file's format.
packages/types/src/description_version.d.ts (3)
1-10: Well-defined base type for description versions.The
TDescriptionVersiontype defines the core properties needed for tracking version history of work item descriptions. The type correctly uses string for date values and makes nullable fields explicit.
12-17: Comprehensive extension type for version content details.The
TDescriptionVersionDetailstype properly extends the base version type to include different representations of the description content. This allows flexible rendering options depending on the context.
19-29: Complete pagination-aware response type for version lists.The
TDescriptionVersionsListResponsetype includes all necessary pagination information alongside the results array, supporting efficient scrolling through version history. The type is well-structured for API responses.web/core/components/core/description-versions/dropdown.tsx (3)
14-19: Clear and descriptive props interface.The props interface is well-defined with appropriate types for each property. The descriptive naming makes the component's requirements clear to users.
21-31: Effective use of MobX observer pattern and hooks.The component uses the observer pattern for efficient re-rendering and properly extracts necessary information from provided props and hooks. The derivation of the latest version information is clean and handles potential undefined cases.
32-59: Well-structured dropdown UI with proper i18n support.The component renders a dropdown that:
- Shows the last editor with their display name
- Formats time using a utility function
- Provides access to previous versions through a submenu
- Properly uses translation keys for internationalization
The CustomMenu implementation includes appropriate placement and accessibility options.
packages/i18n/src/locales/zh-TW/translations.json (1)
2381-2384: Good addition of localization support for the description versions feature.These translations for the Chinese (Taiwan) locale will properly support the new version history feature for work items, maintaining consistency with the application's internationalization approach.
apiserver/plane/app/views/issue/version.py (1)
138-139: Good addition of ordering for version historyAdding the ordering by
-created_atensures that the most recent versions are displayed first, which improves the user experience when viewing version history.web/core/services/issue/work_item_version.service.ts (1)
13-49: Service implementation looks clean and well-structuredThe implementation of
WorkItemVersionServicefollows good patterns with proper API endpoint construction and promise handling. The design allows for flexibility with theserviceTypeparameter.web/core/components/issues/issue-detail/main-content.tsx (2)
130-139: Good addition of editor referenceAdding the
editorRefto theIssueDescriptionInputcomponent allows for programmatic control of the editor, which is essential for the version history restore functionality.
153-170: Well-structured version history component integrationThe integration of the
DescriptionVersionsRootcomponent with proper entity information, fetch handlers, and restore functionality is well-designed and cleanly implemented.web/core/services/inbox/intake-work_item_version.service.ts (1)
13-25:Details
✅ Verification successful
Ensure consistent error handling with other services
Make sure that error handling approach in this service is consistent with other API services in the application. This will help maintain a predictable error handling pattern throughout the codebase.
🏁 Script executed:
#!/bin/bash # Check how other API services handle errors in the codebase echo "Checking error handling patterns in other API services..." rg -A 3 "\.catch\(" --type ts web/core/services/Length of output: 67052
Error handling consistency verified
The error handling in
listDescriptionVersions()is consistent with the patterns used throughout other API services (e.g.,workspace.service.tsand others), where errors are caught and re-thrown aserror?.response?.data. No changes are required.web/core/components/inbox/content/issue-root.tsx (4)
51-51: Properly type and access the EditorRefApi.Good use of
useReffor accessing the editor's API. This enables the new version restore functionality.
208-208: Good implementation of editor reference passing.Passing the editor reference to the
IssueDescriptionInputcomponent enables the version restoration feature. The implementation is clean and follows React best practices.
245-245: Excellent implementation of version restoration.The handler for restoring versions uses the editor ref properly to update the editor value. The second parameter
trueensures the change is propagated, which is important for triggering any necessary UI updates.
241-243:Details
❓ Verification inconclusive
Verify error handling for version fetching.
The service calls for fetching versions don't have error handling. Ensure errors are properly handled at the service level or consider adding error boundaries.
🏁 Script executed:
#!/bin/bash # Check for error handling in the IntakeWorkItemVersionService echo "Searching for IntakeWorkItemVersionService implementation..." rg -A 20 "class IntakeWorkItemVersionService" --type=typescriptLength of output: 231
I'm updating our investigation with a new shell script to search for the service implementation and any associated error handling in the
retrieveDescriptionVersionmethod. This script uses file globs instead of the unsupported file type to increase our chances of locating the relevant code. Please run the script below and share its output for further analysis:#!/bin/bash echo "Searching for IntakeWorkItemVersionService implementation..." rg -A 30 "class IntakeWorkItemVersionService" -g "*.ts" -g "*.tsx" echo "Searching for error handling in the retrieveDescriptionVersion method in IntakeWorkItemVersionService..." rg -A 30 "retrieveDescriptionVersion" -g "*.ts" -g "*.tsx"Once we have this output, we can better verify whether error handling is incorporated for version fetching and advise on whether additional error boundaries are necessary.
web/core/components/issues/peek-overview/issue-detail.tsx (3)
43-43: Properly type and access the EditorRefApi.Good use of
useReffor accessing the editor's API. This enables the new version restore functionality.
118-118: Improved conditional disabling for issue input fields.The code now properly disables the title and description inputs when the issue is archived, which is a good improvement for user experience.
Also applies to: 129-129
135-174: Well-structured implementation of version history UI.The component structure effectively integrates the version history feature. The conditional rendering based on
!disabledensures appropriate access control, and the proper props are passed to theDescriptionVersionsRootcomponent.The implementation properly handles the
isRestoreDisabledflag based on bothdisabledandisArchivedprops, ensuring consistent behavior.web/core/components/core/description-versions/root.tsx (5)
11-16: Well-defined entity information type.The
TDescriptionVersionEntityInformationtype is clearly defined with all necessary properties for handling version history. TheisRestoreDisabledflag is particularly useful for controlling access to version restoration.
18-28: Well-structured component props with flexible design.The component props are well-designed, allowing for different implementations of version services through the
fetchHandlersprop. This makes the component reusable across different entity types.
38-46: Efficient data fetching with SWR.Good use of SWR for data fetching with proper cache keys based on entity ID. The conditional fetching ensures resources are not wasted when data is not needed.
52-62: Robust navigation function with proper dependency tracking.The
handleNavigationfunction is well-implemented with proper bounds checking. The dependency array correctly includes all necessary variables, ensuring the function is updated when these dependencies change.
77-80: Clear and efficient conditional disabling logic.The props passed to control navigation and restoration disabling are clear and efficient, using array indices and entity information appropriately.
apiserver/plane/app/urls/issue.py (4)
27-28: Good update to import the new endpoint class.The import has been correctly updated to use
WorkItemDescriptionVersionEndpointinstead ofIssueDescriptionVersionEndpoint.
266-266: Improved naming convention for URL patterns.Updating the URL pattern names from "page-versions" to "issue-versions" improves clarity and consistency.
Also applies to: 271-271
274-282: Proper renaming of endpoints for work item versions.The endpoint paths have been correctly updated to use "work-items" instead of "issues" for description version endpoints, with appropriate parameter name changes. This aligns with the architectural shift to focus on work items rather than just issues.
274-282:Details
❓ Verification inconclusive
Verify backwards compatibility with existing code.
The change from
issue_idtowork_item_idin the URL paths is significant. Ensure that all client code that depends on these endpoints has been updated accordingly.
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old endpoint URLs echo "Searching for references to old URL patterns..." rg "issues/[^/]+/description-versions" --type=typescript --type=javascriptLength of output: 216
Ensure backward compatibility with the URL endpoint changes
The URL paths have been updated from using
issue_idtowork_item_id. To fully guarantee backward compatibility, it's critical to confirm that no client code or frontend references the old pattern exists. Please execute the adapted script above to verify that no references to the legacy endpoint pattern remain in the TypeScript or JavaScript source files. If any occurrences are found, update those client references accordingly.--- 🏁 Script executed: ```shell #!/bin/bash # Re-run search for any remaining references to the old endpoint URLs in TypeScript and JavaScript files echo "Searching for references to old URL patterns in .ts files..." rg "issues/[^/]+/description-versions" -g "*.ts" echo "Searching for references to old URL patterns in .js files..." rg "issues/[^/]+/description-versions" -g "*.js"Length of output: 485
Action: Manually Verify Backward Compatibility of URL Changes
The automated search did not reveal any references to the legacy pattern (i.e., URLs containing "issues/…/description-versions") in TypeScript or JavaScript files. However, because the search results were minimal (and the output quality is low), please manually verify that all client and external integrations have been updated to use
work_item_idinstead of the deprecatedissue_idin the endpoint URLs.
- Confirm that no client/consumer code (including external integrations) still references endpoints using the old
issues/…pattern.- Verify that any related API documentation or shared client libraries reflect this updated URL structure.
- Double-check that there are no hidden dependency scripts or frameworks that might still rely on the old pattern.
apiserver/plane/app/views/intake/base.py (6)
30-45: Good addition of relevant imports.Importing
IssueDescriptionVersion,IssueDetailSerializer,IssueDescriptionVersionDetailSerializer,issue_description_version_task,BaseAPIView,user_timezone_converter, andpaginatealigns well with the new feature requirements and appears logically consistent.
95-95: Fixed filterset_fields from 'statulls' to 'status'.This correction ensures proper filtering by status. No further issues found.
410-430: Potential mismatch between 'updated_issue' and the final updated state.In
partial_update, the code callsissue_description_version_task.delay(updated_issue=current_instance, ...)beforeissue_serializer.save(). As a result,current_instancemight represent the old state, not the new updated version. Verify whether you actually want the pre-update data or if the intended behavior is to store the post-update description.
609-615: Neat approach for paginated result post-processing.The
process_paginated_resultmethod converts datetime fields to the user’s timezone in a clean, modular way. No issues found.
642-653: Handle potential 'DoesNotExist' errors gracefully.
IssueDescriptionVersion.objects.get(...)may raise an exception if no record is found. Atry/exceptblock returning a 404 would prevent unhandled errors.
655-682: Pagination logic is clear and straightforward.Using
paginatewith a processing callback is a clean approach. If performance becomes a concern with larger datasets, consider streaming or more advanced pagination strategies.
|
Add type hints |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apiserver/plane/app/views/intake/base.py (2)
609-684: Add type hints to the new class methods.The new
IntakeWorkItemDescriptionVersionEndpointclass looks good but is missing type hints, which would improve code clarity and maintainability as suggested in the PR comments.Consider adding type hints to method parameters and return values:
- def process_paginated_result(self, fields, results, timezone): + def process_paginated_result(self, fields: list, results: QuerySet, timezone: str) -> dict: paginated_data = results.values(*fields) datetime_fields = ["created_at", "updated_at"] paginated_data = user_timezone_converter( paginated_data, datetime_fields, timezone ) return paginated_data - def get(self, request, slug, project_id, work_item_id, pk=None): + def get(self, request, slug: str, project_id: str, work_item_id: str, pk: str = None) -> Response:
621-642: Consider refactoring permission check logic.The permission check logic is duplicated across multiple methods in this file. Consider extracting it into a helper method to improve maintainability.
You could create a helper method like:
def _check_guest_permissions(self, request, slug, project_id, issue, project): """Check if a guest user has permission to view the issue.""" if ( ProjectMember.objects.filter( workspace__slug=slug, project_id=project_id, member=request.user, role=ROLE.GUEST.value, is_active=True, ).exists() and not project.guest_view_all_features and not issue.created_by == request.user ): return False return TrueThen use it in your methods to simplify the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apiserver/plane/app/views/intake/base.py(8 hunks)apiserver/plane/app/views/issue/base.py(3 hunks)apiserver/plane/app/views/issue/version.py(4 hunks)apiserver/plane/app/views/view/base.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apiserver/plane/app/views/issue/version.py
🧰 Additional context used
🧬 Code Definitions (2)
apiserver/plane/app/views/issue/base.py (1)
apiserver/plane/app/serializers/issue.py (1)
IssueDetailSerializer(713-719)
apiserver/plane/app/views/intake/base.py (2)
apiserver/plane/app/serializers/issue.py (2)
IssueDetailSerializer(713-719)IssueDescriptionVersionDetailSerializer(796-815)apiserver/plane/app/views/issue/version.py (2)
process_paginated_result(24-32)process_paginated_result(76-84)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (13)
apiserver/plane/app/views/view/base.py (1)
435-435: Improved HTTP status code for permission errors.Changing from HTTP_400_BAD_REQUEST to HTTP_403_FORBIDDEN is the correct approach here, as this is a permission issue rather than a malformed request. This change aligns with REST API best practices, providing clearer error semantics to API consumers.
apiserver/plane/app/views/issue/base.py (3)
568-568: Improved HTTP status code for permission errors.Changing from HTTP_400_BAD_REQUEST to HTTP_403_FORBIDDEN is the correct approach here, as this is a permission issue rather than a malformed request. This change aligns with REST API best practices and maintains consistency with similar changes throughout the codebase.
635-635: Using appropriate serializer for detailed issue representation.Using
IssueDetailSerializerinstead ofIssueSerializerensures that the serialized data includes thedescription_htmlandis_subscribedfields, which is particularly important for the description version history feature being implemented.
1281-1281: Improved HTTP status code for permission errors.Changing from HTTP_400_BAD_REQUEST to HTTP_403_FORBIDDEN is the correct approach here, as this is a permission issue rather than a malformed request. This maintains consistency with the other similar changes in the codebase.
apiserver/plane/app/views/intake/base.py (9)
30-30: Good updates to imports and model references.The necessary imports have been added to support the new description version history functionality. The imports are properly organized by category.
Also applies to: 34-34, 38-38, 42-45
97-97: Fixed typo in filterset_fields.The field name has been corrected from "statulls" to "status", which will properly enable filtering by status.
228-228: Using ROLE enum is better than magic numbers.Good change using
ROLE.GUEST.valueinstead of hardcoded numeric values, which improves code readability and maintainability.Also applies to: 572-572
297-302: Description version tracking added to issue creation.The implementation correctly triggers an async task to manage description versions when creating a new issue. This supports the new version history feature.
402-404: Updated serializer to IssueDetailSerializer.Using
IssueDetailSerializerinstead ofIssueSerializerprovides more comprehensive data for the issue activity tracking.
428-432: Description version tracking added to issue updates.The implementation correctly triggers an async task to manage description versions when updating an existing issue. This supports the new version history feature.
580-581: Using HTTP status constant instead of hardcoded value.Using
status.HTTP_403_FORBIDDENis cleaner and more maintainable than using the numeric value directly.
644-656: LGTM: Single version retrieval logic is well implemented.The logic for retrieving a single description version is correctly implemented with proper permission checks.
657-684: LGTM: Version history pagination is properly implemented.The pagination logic for retrieving a list of description versions uses the appropriate utilities and handles timezone conversion correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
web/core/components/core/description-versions/modal.tsx (6)
16-27: Consider refining type definitions for optional props.Several props, such as
activeVersionDescriptionandprojectId, are declared asstring | undefined, whereasworkspaceSlugis mandated as a non-optional string. If there's a possibility thatworkspaceSlugcould be missing, consider aligning the type strategy across props to reduce potential confusion.
49-52: Verify the fallback scenario for undefined user details.
getUserDetails(owned_by)may returnundefinedif the user can't be found, but the code usesversionCreator?.display_namewithout a default fallback label. Consider displaying a placeholder or label (such as"Unknown User") for unclear ownership.
55-64: Add error handling for the copy-to-clipboard flow.
handleCopyMarkdownonly handles the success case. If copying fails (e.g., lack of permissions), the user receives no feedback. Consider catching the error and displaying a warning toast.copyTextToClipboard(editorRef.current.getMarkDown()) + .catch(() => { + setToast({ + type: TOAST_TYPE.ERROR, + title: t("toast.failure"), + message: "Failed to copy markdown.", + }); + }) .then(() => { setToast({ type: TOAST_TYPE.SUCCESS, ... }) });
62-62: Externalize success message for translation.The success message
"Markdown copied to clipboard."is hard-coded. Consider usingt("...")to ensure consistent localization.
100-110: Enhance keyboard accessibility for navigation buttons.The
ChevronRightandChevronLeftbuttons rely on click events. For improved accessibility, consider making them keyboard-focusable (e.g.,tabIndex={0}) and handling the Enter key event to support keyboard users.
170-173: Provide feedback if restore operation fails.After calling
handleRestore, the modal closes immediately. IfhandleRestorethrows or cannot restore, the user sees no error. Consider awaiting the restore operation or capturing errors before closing the modal and showing an appropriate toast if restoration fails.Do you want me to propose an updated implementation to handle potential restore errors gracefully?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/core/components/core/description-versions/modal.tsx(1 hunks)web/core/components/issues/description-input.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/components/issues/description-input.tsx
🧰 Additional context used
🧬 Code Definitions (1)
web/core/components/core/description-versions/modal.tsx (2)
packages/types/src/description_version.d.ts (1)
TDescriptionVersion(1-10)packages/editor/src/core/types/editor.ts (1)
EditorReadOnlyRefApi(79-94)
* chore: work item description versions * chore: intake issue description * chore: intake work item description versions * chore: add missing translations * chore: endpoint for intake description version * chore: renamed key to work item * chore: changed the paginator class * chore: authorization added * chore: added the enum validation * chore: removed extra validations * chore: added extra validations * chore: modal position --------- Co-authored-by: NarayanBavisetti <narayan3119@gmail.com> Co-authored-by: Bavisetti Narayan <72156168+NarayanBavisetti@users.noreply.github.com>
5 participants
Description
This PR adds description version history to work items and intake work items.
Type of Change
Media
Screen.Recording.2025-04-02.at.17.59.16.mov
Summary by CodeRabbit