Chore remove posthog events #8465
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR removes PostHog telemetry: deletes PostHog provider, page-view component, and event-tracker helper; removes PostHog deps and env vars; and strips capture*/data-ph-element tracking calls across many frontend components while converting many then/catch flows to async/await. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
sriramveeraghanta
force-pushed
the
chore-remove-posthog-events
branch
from
f995aa6 to
5a69b98
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes PostHog event tracking infrastructure from the codebase. The changes involve removing PostHog packages, environment variables, helper functions, provider components, and all event tracking calls throughout the application.
Key changes:
- Removal of
posthog-jsand@posthog/reactdependencies - Deletion of PostHog configuration and provider components
- Removal of event tracking helper functions and their usage across ~100+ files
- Cleanup of unused imports and data attributes
- Refactoring promise chains to async/await in several components
Reviewed changes
Copilot reviewed 142 out of 143 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Removed PostHog package dependencies and their transitive dependencies |
| apps/web/package.json | Removed PostHog package declarations |
| turbo.json | Removed PostHog environment variable configurations |
| apps/web/helpers/event-tracker.helper.ts | Deleted entire event tracking helper module |
| apps/web/core/lib/posthog-*.tsx | Deleted PostHog provider and view tracking components |
| apps/web/core/hooks/use-page-operations.ts | Removed event tracking, converted to async/await |
| Multiple component files | Removed event tracking calls, cleaned up imports, improved async patterns |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/web/core/components/profile/notification/email-notification-form.tsx (1)
38-44: Log errors to preserve debugging capability.While removing PostHog tracking is intentional, completely ignoring the error (not even
console.error) eliminates valuable debugging information for production issues. This violates the coding guideline: "Use try-catch with proper error types and log errors appropriately."Consider logging the error while keeping the user-facing toast:
🔎 Proposed fix to add error logging
- } catch (_error) { + } catch (error) { + console.error("Failed to update email notification setting:", error); setToast({ title: t("error"), type: TOAST_TYPE.ERROR, message: t("failed_to_update_email_notification_setting"), }); }Based on coding guidelines, error logging is essential for production observability.
apps/web/core/components/project/leave-project-modal.tsx (1)
115-134: Fix validation error message.The validation error message on line 119 says "Label title is required" but should say "Project name is required" since this field validates the project name input.
🔎 Proposed fix
rules={{ - required: "Label title is required", + required: "Project name is required", }}apps/web/core/components/workspace/settings/workspace-details.tsx (1)
70-72: Add error toast to provide user feedback on update failure.The error handling only logs to console but doesn't show any error toast to the user. When the workspace update fails, users receive no feedback, creating confusion about whether their changes were saved.
🔎 Proposed fix to add error toast
} catch (err: unknown) { console.error(err); + setToast({ + title: "Error!", + type: TOAST_TYPE.ERROR, + message: "Failed to update workspace. Please try again.", + }); } finally {apps/web/core/components/project-states/create-update/update.tsx (1)
11-16: Remove unusedshouldTrackEventsprop.The
shouldTrackEventsprop is defined in theTStateUpdatetype but is no longer destructured or used in the component. This is leftover dead code from the event tracking removal.🔎 Proposed fix
type TStateUpdate = { state: IState; updateStateCallback: TStateOperationsCallbacks["updateState"]; - shouldTrackEvents: boolean; handleClose: () => void; }; export const StateUpdate = observer(function StateUpdate(props: TStateUpdate) { const { state, updateStateCallback, handleClose } = props;apps/web/core/components/account/auth-forms/password.tsx (1)
7-7: Remove unusedAUTH_TRACKER_EVENTSimport.
AUTH_TRACKER_EVENTSis imported but never used in this file. OnlyAUTH_TRACKER_ELEMENTSis actively used on lines 83 and 288.apps/web/core/components/automation/auto-archive-automation.tsx (1)
83-91: Remove the orphaned tracking call.The
captureElementAndEventfunction is called here but is no longer imported, which will cause aReferenceErrorat runtime when the toggle is clicked.🔎 Proposed fix
<ToggleSwitch value={autoArchiveStatus} onChange={async () => { if (currentProjectDetails?.archive_in === 0) { await handleChange({ archive_in: 1 }); } else { await handleChange({ archive_in: 0 }); } - captureElementAndEvent({ - element: { - elementName: PROJECT_SETTINGS_TRACKER_ELEMENTS.AUTOMATIONS_ARCHIVE_TOGGLE_BUTTON, - }, - event: { - eventName: PROJECT_SETTINGS_TRACKER_EVENTS.auto_archive_workitems, - state: "SUCCESS", - }, - }); }} size="sm" disabled={!isAdmin} />apps/web/core/components/cycles/analytics-sidebar/sidebar-header.tsx (1)
94-108: AwaitsubmitChangesto prevent showing success before update completes.Line 95 calls
submitChangeswithout awaiting it, causing the success toast (lines 96-100) to display immediately, before the update actually completes. If the update fails, users will see a success message for a failed operation.🔎 Recommended fix: Await the update and handle errors properly
if (isDateValid) { - submitChanges(payload); - setToast({ - type: TOAST_TYPE.SUCCESS, - title: t("project_cycles.action.update.success.title"), - message: t("project_cycles.action.update.success.description"), - }); + try { + await submitChanges(payload); + setToast({ + type: TOAST_TYPE.SUCCESS, + title: t("project_cycles.action.update.success.title"), + message: t("project_cycles.action.update.success.description"), + }); + } catch (error) { + setToast({ + type: TOAST_TYPE.ERROR, + title: t("project_cycles.action.update.failed.title"), + message: t("project_cycles.action.update.failed.description"), + }); + return false; + } } else { setToast({ type: TOAST_TYPE.ERROR, title: t("project_cycles.action.update.failed.title"), message: t("project_cycles.action.update.error.already_exists"), }); }
🧹 Nitpick comments (25)
apps/web/core/components/views/quick-actions.tsx (1)
76-83: Consider simplifying the now-redundant mapping.After removing the tracking calls, this
mapfunction only wraps each action in a new arrow function that immediately invokes the original. You can eliminate this intermediate step entirely by usingMENU_ITEMSdirectly whereverCONTEXT_MENU_ITEMSis referenced (line 97).🔎 Proposed simplification
- const CONTEXT_MENU_ITEMS = MENU_ITEMS.map(function CONTEXT_MENU_ITEMS(item) { - return { - ...item, - action: () => { - item.action(); - }, - }; - }); - return ( <> <CreateUpdateProjectViewModal isOpen={createUpdateViewModal} onClose={() => setCreateUpdateViewModal(false)} workspaceSlug={workspaceSlug} projectId={projectId} data={view} /> <DeleteProjectViewModal data={view} isOpen={deleteViewModal} onClose={() => setDeleteViewModal(false)} /> <PublishViewModal isOpen={isPublishModalOpen} onClose={() => setPublishModalOpen(false)} view={view} /> {additionalModals} - <ContextMenu parentRef={parentRef} items={CONTEXT_MENU_ITEMS} /> + <ContextMenu parentRef={parentRef} items={MENU_ITEMS} /> <CustomMenuapps/web/core/components/modules/delete-module-modal.tsx (1)
1-14: LGTM with a minor nit.The import cleanup and tracking removal are correct. The deletion functionality with error handling and i18n support is preserved.
Nit: Lines 11-12 have orphaned section comments (
// constantsand// hooks) with no imports following// constants. Consider removing the empty// constantscomment.Optional cleanup
// ui import { AlertModalCore } from "@plane/ui"; -// constants // hooks import { useModule } from "@/hooks/store/use-module";apps/web/core/components/modules/module-list-item-action.tsx (1)
11-12: Remove unused tracking constants from imports.
MODULE_TRACKER_EVENTSandMODULE_TRACKER_ELEMENTSare no longer used after removing PostHog telemetry. Please remove them for consistency with the refactoring objective.🔎 Proposed fix
import { MODULE_STATUS, EUserPermissions, EUserPermissionsLevel, IS_FAVORITE_MENU_OPEN, - MODULE_TRACKER_EVENTS, - MODULE_TRACKER_ELEMENTS, } from "@plane/constants";apps/web/core/components/project/send-project-invitation-modal.tsx (1)
74-95: Event tracking removal is clean. Consider adding error feedback for users.The removal of analytics calls preserves all core functionality—success flow still shows a toast and closes the modal, while errors are logged.
However, the error handler only logs to the console without user-facing feedback. Consider adding an error toast to improve UX when member addition fails.
🔎 Optional: Add user-facing error feedback
.catch((error) => { console.error(error); + setToast({ + title: "Error!", + type: TOAST_TYPE.ERROR, + message: "Failed to add members. Please try again.", + }); })apps/web/core/components/issues/issue-modal/draft-issue-layout.tsx (1)
93-99: Consider logging the error for debugging.While the underscore prefix correctly indicates the error is intentionally unused, completely discarding the error details removes valuable debugging information. Consider logging the error to aid troubleshooting in development and production.
🔎 Suggested improvement
- .catch((_error) => { + .catch((error) => { + console.error("Failed to create draft issue:", error); setToast({ type: TOAST_TYPE.ERROR, title: `${t("error")}!`, message: t("workspace_draft_issues.toasts.created.error"), }); });apps/web/core/components/modules/quick-actions.tsx (1)
63-79: Consider logging the error for debugging.The try-catch implementation improves error handling by providing user-friendly feedback. However, the actual error details are discarded. Consider logging the error to aid debugging.
💡 Suggested improvement
- } catch (_error) { + } catch (error) { + console.error("Failed to restore module:", error); setToast({ type: TOAST_TYPE.ERROR, title: "Error!", message: "Module could not be restored. Please try again.", }); }As per coding guidelines, errors should be logged appropriately to facilitate debugging in production environments.
apps/web/core/components/project/leave-project-modal.tsx (1)
56-67: Verify navigation timing in leave project flow.The router navigates to the projects list (line 56) before the
leaveProjectasync operation completes (line 57). If the operation fails, the user will already be on a different page when the error toast appears, which may be confusing.Consider awaiting the operation before navigation:
- router.push(`/${workspaceSlug}/projects`); - return leaveProject(workspaceSlug.toString(), project.id) + return leaveProject(workspaceSlug.toString(), project.id) .then(() => { + router.push(`/${workspaceSlug}/projects`); handleClose(); })This ensures users only navigate away after a successful leave operation.
apps/web/core/components/cycles/quick-actions.tsx (1)
98-105: Consider removing the now-redundant mapping.The
CONTEXT_MENU_ITEMSmapping previously added event tracking to menu actions, but now it just wraps each action in a pass-through function. You can simplify by usingMENU_ITEMSdirectly.🔎 Proposed simplification
- const CONTEXT_MENU_ITEMS = MENU_ITEMS.map(function CONTEXT_MENU_ITEMS(item) { - return { - ...item, - action: () => { - item.action(); - }, - }; - }); - return ( <> {cycleDetails && ( @@ -132,7 +125,7 @@ {additionalModals} </div> )} - <ContextMenu parentRef={parentRef} items={CONTEXT_MENU_ITEMS} /> + <ContextMenu parentRef={parentRef} items={MENU_ITEMS} /> <CustomMenuapps/web/core/components/workspace/views/quick-action.tsx (1)
35-42: Async/await refactor looks good; consider adding error handling.The refactor to async/await is cleaner and ensures the clipboard operation completes before showing the success toast. However, clipboard operations can fail due to permissions or browser compatibility issues, and users would see no feedback on failure.
🔎 Proposed error handling
-const handleCopyText = async () => { - await copyUrlToClipboard(viewLink); - setToast({ - type: TOAST_TYPE.SUCCESS, - title: "Link Copied!", - message: "View link copied to clipboard.", - }); -}; +const handleCopyText = async () => { + try { + await copyUrlToClipboard(viewLink); + setToast({ + type: TOAST_TYPE.SUCCESS, + title: "Link Copied!", + message: "View link copied to clipboard.", + }); + } catch (error) { + setToast({ + type: TOAST_TYPE.ERROR, + title: "Failed to copy link", + message: "Please try again or copy the link manually.", + }); + } +};apps/web/core/components/profile/preferences/language-timezone.tsx (1)
18-50: Clean removal of tracking code and good async/await conversion.The refactoring to async/await and removal of PostHog tracking is well-executed. Both handlers now have consistent, readable error handling.
Consider logging errors for debugging and observability.
Per the coding guidelines, errors should be typed and logged appropriately. Currently, caught errors are only shown to users via toast but not logged for developer debugging. This can make troubleshooting production issues difficult.
🔎 Suggested enhancement for error logging
- const handleTimezoneChange = async (value: string) => { - try { - await updateCurrentUser({ user_timezone: value }); - setToast({ - title: "Success!", - message: "Timezone updated successfully", - type: TOAST_TYPE.SUCCESS, - }); - } catch (_error) { - setToast({ - title: "Error!", - message: "Failed to update timezone", - type: TOAST_TYPE.ERROR, - }); - } - }; + const handleTimezoneChange = async (value: string) => { + try { + await updateCurrentUser({ user_timezone: value }); + setToast({ + title: "Success!", + message: "Timezone updated successfully", + type: TOAST_TYPE.SUCCESS, + }); + } catch (error) { + console.error("Failed to update timezone:", error); + setToast({ + title: "Error!", + message: "Failed to update timezone", + type: TOAST_TYPE.ERROR, + }); + } + }; - const handleLanguageChange = async (value: string) => { - try { - await updateUserProfile({ language: value }); - setToast({ - title: "Success!", - message: "Language updated successfully", - type: TOAST_TYPE.SUCCESS, - }); - } catch (_error) { - setToast({ - title: "Error!", - message: "Failed to update language", - type: TOAST_TYPE.ERROR, - }); - } - }; + const handleLanguageChange = async (value: string) => { + try { + await updateUserProfile({ language: value }); + setToast({ + title: "Success!", + message: "Language updated successfully", + type: TOAST_TYPE.SUCCESS, + }); + } catch (error) { + console.error("Failed to update language:", error); + setToast({ + title: "Error!", + message: "Failed to update language", + type: TOAST_TYPE.ERROR, + }); + } + };As per coding guidelines: "Use try-catch with proper error types and log errors appropriately"
apps/web/core/components/onboarding/steps/role/root.tsx (2)
56-59: Remove commented-out dead code.The commented line on Line 58 can be removed to keep the codebase clean.
🔎 Proposed cleanup
try { - await Promise.all([ - updateUserProfile(profileUpdatePayload), - // totalSteps > 2 && stepChange({ profile_complete: true }), - ]); + await updateUserProfile(profileUpdatePayload); setToast({
84-84: Simplify boolean expression.The ternary can be replaced with a direct boolean expression for better readability.
🔎 Proposed simplification
- const isButtonDisabled = !isSubmitting && isValid ? false : true; + const isButtonDisabled = isSubmitting || !isValid;apps/web/core/components/onboarding/steps/workspace/join-invites.tsx (1)
44-60: Consider adding user feedback when workspace join fails.The error handling currently only logs to the console without notifying the user. During onboarding, users may not understand why they remain on the invitation screen if the join operation fails due to network or server issues.
Consider adding a toast notification to inform users of the failure and allow them to retry.
💡 Example implementation
+import { useToast } from "@plane/ui"; export function WorkspaceJoinInvitesStep(props: Props) { const { invitations, handleNextStep, handleCurrentViewChange } = props; // states const [isJoiningWorkspaces, setIsJoiningWorkspaces] = useState(false); const [invitationsRespond, setInvitationsRespond] = useState<string[]>([]); // store hooks const { fetchWorkspaces } = useWorkspace(); const { fetchCurrentUserSettings } = useUserSettings(); + const { setToastAlert } = useToast(); // ... rest of code const submitInvitations = async () => { // ... validation logic setIsJoiningWorkspaces(true); try { await workspaceService.joinWorkspaces({ invitations: invitationsRespond }); await fetchWorkspaces(); await fetchCurrentUserSettings(); await handleNextStep(); } catch (error: any) { console.error(error); + setToastAlert({ + type: "error", + title: "Failed to join workspace", + message: "Please try again or contact support if the issue persists.", + }); setIsJoiningWorkspaces(false); } };apps/web/core/components/issues/peek-overview/root.tsx (1)
95-101: Consider consistent error logging across all operations.Several catch blocks use
_error(lines 95, 119, 131, 142) without logging, while others (lines 69, 108, 204) includeconsole.errorcalls. While user-facing toasts are consistently present, adding console logging to all catch blocks would improve debugging visibility.Example: Add console logging to remaining catch blocks
For the
removeoperation (lines 95-101):- } catch (_error) { + } catch (error) { + console.error("Error removing issue", error); setToast({Apply similar changes to lines 119, 131, and 142 for consistency.
Also applies to: 119-125, 131-137, 142-148
apps/web/core/components/account/auth-forms/password.tsx (1)
83-83: Consider removing PostHog-specific data attributes.The
data-ph-elementattributes are PostHog-specific DOM markers (lines 83, 288). Since this PR removes PostHog event tracking, these attributes appear to be leftover artifacts that should also be removed for consistency.If these are intentionally kept for future analytics or another tracking system, please clarify.
🔎 Proposed cleanup
Line 83:
<Link - data-ph-element={AUTH_TRACKER_ELEMENTS.FORGOT_PASSWORD_FROM_SIGNIN} href={`/accounts/forgot-password?email=${encodeURIComponent(email)}`} className="text-11 font-medium text-accent-primary" >Line 288:
<Button type="button" - data-ph-element={AUTH_TRACKER_ELEMENTS.SIGN_IN_WITH_UNIQUE_CODE} onClick={redirectToUniqueCodeSignIn} variant="secondary"Also applies to: 288-288
apps/web/core/components/workspace/views/header.tsx (1)
102-112: Consider simplifying the conditional rendering.The empty fragment in the else clause serves no purpose. You can simplify this ternary to a conditional render.
🔎 Suggested simplification
- {isAuthorizedUser ? ( + {isAuthorizedUser && ( <button type="button" className="sticky -right-4 flex flex-shrink-0 items-center justify-center border-transparent bg-surface-1 py-3 hover:border-subtle hover:text-placeholder" onClick={() => setCreateViewModal(true)} > <PlusIcon className="h-4 w-4 text-accent-secondary" /> </button> - ) : ( - <></> )}apps/web/core/components/onboarding/profile-setup.tsx (1)
150-173: Consider refactoring the empty if block for clarity.The empty if block when
formData.passwordis truthy (line 161-162) makes the intent unclear. While the current behavior appears intentional (do nothing when password is set, advance to personalization when not), consider refactoring for better readability.🔎 Suggested refactor for clarity
await Promise.all([ updateCurrentUser(userDetailsPayload), formData.password && handleSetPassword(formData.password), ]).then(() => { - if (formData.password) { - } else { + if (!formData.password) { setProfileSetupStep(EProfileSetupSteps.USER_PERSONALIZATION); } });apps/web/core/components/profile/form.tsx (1)
163-167: Suggest removing redundant promise handlers.The
.then()and.catch()blocks followingsetPromiseToastare redundant. ThesetPromiseToastutility already handles both success and error states (lines 152-162), so these additional handlers serve no purpose and can be removed entirely.🔎 Proposed simplification
- updateUserAndProfile - .then(() => { - return; - }) - .catch(() => {});apps/web/core/components/exporter/export-form.tsx (1)
102-126: PostHog tracking removed cleanly.The removal of event tracking calls from the success and error paths is complete and maintains proper user feedback through toast notifications.
Optional: Consider logging errors for debugging
As per coding guidelines, errors in try-catch blocks should be logged appropriately. While outside the scope of this PR, consider logging the error to help with debugging:
} catch (error) { + console.error("Export failed:", error); setExportLoading(false); setToast({apps/web/core/components/issues/issue-detail-widgets/attachments/helper.tsx (1)
36-52: Inconsistent error handling pattern between create and remove operations.The
createoperation throws errors to the caller (validation error at line 37, upload error at line 51), while theremoveoperation catches all errors and only displays toasts (lines 54-68). This inconsistency means:
- Create: Validation failures throw immediately; upload failures show error toast then re-throw
- Remove: All failures are caught and only show toasts
This creates different caller expectations—code calling
createmust handle errors, but code callingremovewon't receive errors. Consider aligning both operations to use the same pattern for consistency.Verify whether this asymmetry is intentional. If not, consider wrapping the
createoperation in try-catch to matchremove, or removing the try-catch fromremoveto matchcreate.🔎 Proposed fix to align error handling patterns
If you prefer catching all errors and showing toasts (matching
removebehavior):create: async (file) => { + try { if (!workspaceSlug || !projectId || !issueId) throw new Error("Missing required fields"); const attachmentUploadPromise = createAttachment(workspaceSlug, projectId, issueId, file); setPromiseToast(attachmentUploadPromise, { loading: "Uploading attachment...", success: { title: "Attachment uploaded", message: () => "The attachment has been successfully uploaded", }, error: { title: "Attachment not uploaded", message: () => "The attachment could not be uploaded", }, }); await attachmentUploadPromise; + } catch (_error) { + // Error toast already shown by setPromiseToast for upload errors + // Validation errors will also be caught here + } },apps/web/core/components/modules/analytics-sidebar/root.tsx (1)
91-107: Good error handling pattern with toast notifications.The try/catch implementation properly handles both success and error cases with user-visible feedback. This is the correct pattern that should be applied to other async operations in this component.
Optional: Consider logging error for debugging
While the current implementation is acceptable, you could optionally log the error for debugging purposes:
} catch (_error) { + console.error("Failed to delete module link:", _error); setToast({ type: TOAST_TYPE.ERROR, title: "Error!", message: "Some error occurred", }); }apps/web/core/components/views/delete-view-modal.tsx (1)
36-54: Approve the async/await refactor with suggested improvements.The switch from promise chaining to async/await improves readability and aligns with the PostHog removal. However, the error-handling flow can be refined:
Redundant loading state reset: Line 53 sets loading to
falseafter both success and error. In the success case,handleClose()on line 39 already does this (line 31), making line 53 redundant. MovesetIsDeleteLoading(false)into the catch block only and remove line 53.Loading state placement: Move
setIsDeleteLoading(true)(line 37) before the try block to ensure loading state is always set even if an unexpected synchronous error occurs.Error logging: Per coding guidelines, "log errors appropriately." Currently the error is caught but not logged. Consider logging
_errorfor debugging purposes.🔎 Suggested improvements
const handleDeleteView = async () => { if (!workspaceSlug || !projectId) return; + setIsDeleteLoading(true); try { - setIsDeleteLoading(true); await deleteView(workspaceSlug.toString(), projectId.toString(), data.id); handleClose(); router.push(`/${workspaceSlug}/projects/${projectId}/views`); setToast({ type: TOAST_TYPE.SUCCESS, title: "Success!", message: "View deleted successfully.", }); - } catch (_error) { + } catch (error) { + console.error("Failed to delete view:", error); + setIsDeleteLoading(false); setToast({ type: TOAST_TYPE.ERROR, title: "Error!", message: "View could not be deleted. Please try again.", }); } - setIsDeleteLoading(false); };apps/web/core/components/issues/issue-layouts/spreadsheet/issue-column.tsx (1)
30-32: Event tracking removal looks good; consider memoizing the handler.The extraction of event tracking logic aligns with the PR objectives. However, since
handleUpdateIssueis defined inline, it will be recreated on every render, potentially causing unnecessary re-renders of theColumncomponent if it's optimized withReact.memoor similar techniques.🔎 Memoize with useCallback
Import
useCallbackat the top:-import { useRef } from "react"; +import { useCallback, useRef } from "react";Then wrap the handler:
- const handleUpdateIssue = async (issue: TIssue, data: Partial<TIssue>) => { + const handleUpdateIssue = useCallback(async (issue: TIssue, data: Partial<TIssue>) => { if (updateIssue) await updateIssue(issue.project_id, issue.id, data); - }; + }, [updateIssue]);apps/web/core/components/web-hooks/delete-webhook-modal.tsx (2)
41-47: Consider logging the error for debugging while keeping the generic user message.The catch block discards all error details (note the
_errorparameter naming). While showing a generic message to users is good practice, completely discarding the error makes debugging issues difficult for developers.🔎 Suggested enhancement
-} catch (_error) { +} catch (error) { + console.error("Failed to delete webhook:", error); setToast({ type: TOAST_TYPE.ERROR, title: "Error!", message: "Webhook could not be deleted. Please try again.", }); }
48-48: Consider using afinallyblock for cleanup logic.While the current placement of
setIsDeleting(false)works correctly (it executes after both successful and error paths), using afinallyblock would make the intent clearer and is more idiomatic for cleanup operations.🔎 Suggested refactor
try { await removeWebhook(workspaceSlug.toString(), webhookId.toString()); router.replace(`/${workspaceSlug}/settings/webhooks/`); setToast({ type: TOAST_TYPE.SUCCESS, title: "Success!", message: "Webhook deleted successfully.", }); } catch (_error) { setToast({ type: TOAST_TYPE.ERROR, title: "Error!", message: "Webhook could not be deleted. Please try again.", }); +} finally { + setIsDeleting(false); } -setIsDeleting(false);
apps/web/core/components/cycles/list/cycle-list-item-action.tsx
Outdated
Show resolved
Hide resolved
apps/web/core/components/issues/issue-layouts/quick-add/root.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
apps/web/core/components/project-states/create-update/update.tsx (1)
8-13: Remove the unusedshouldTrackEventsproperty.The
shouldTrackEventsproperty on Line 11 is no longer used anywhere in the component (not destructured, not referenced). This is leftover from the PostHog removal and should be deleted to complete the cleanup.🔎 Proposed fix
type TStateUpdate = { state: IState; updateStateCallback: TStateOperationsCallbacks["updateState"]; - shouldTrackEvents: boolean; handleClose: () => void; };apps/web/core/components/workspace/settings/workspace-details.tsx (1)
70-72: Add error toast for failed workspace updates.The catch block only logs errors to the console without providing user feedback. This is inconsistent with the error handling pattern used in
handleRemoveLogo(lines 91-96), which properly displays an error toast.🔎 Proposed fix to add error toast
} catch (err: unknown) { console.error(err); + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to update workspace. Please try again.", + }); } finally {apps/web/core/components/views/quick-actions.tsx (1)
76-83: Remove redundant mapping now that tracking is eliminated.After removing the
captureClicktracking logic, this mapping simply wraps each action in a new function that immediately invokes the original—creating unnecessary overhead with no transformation.🔎 Proposed fix: use MENU_ITEMS directly
- const CONTEXT_MENU_ITEMS = MENU_ITEMS.map(function CONTEXT_MENU_ITEMS(item) { - return { - ...item, - action: () => { - item.action(); - }, - }; - }); - return ( <> <CreateUpdateProjectViewModal isOpen={createUpdateViewModal} onClose={() => setCreateUpdateViewModal(false)} workspaceSlug={workspaceSlug} projectId={projectId} data={view} /> <DeleteProjectViewModal data={view} isOpen={deleteViewModal} onClose={() => setDeleteViewModal(false)} /> <PublishViewModal isOpen={isPublishModalOpen} onClose={() => setPublishModalOpen(false)} view={view} /> {additionalModals} - <ContextMenu parentRef={parentRef} items={CONTEXT_MENU_ITEMS} /> + <ContextMenu parentRef={parentRef} items={MENU_ITEMS} /> <CustomMenuapps/web/core/components/issues/issue-detail/issue-detail-quick-actions.tsx (1)
86-102: Fix typo and add error logging.Two issues identified:
- Line 97 has a trailing space in the translation key:
t("toast.error ")should bet("toast.error")- Errors are not logged, making debugging difficult
🔎 Recommended fixes
await deleteIssue(workspaceSlug, projectId, issueId); router.push(redirectionPath); - } catch (_error) { + } catch (error) { + console.error("Failed to delete issue:", error); setToast({ - title: t("toast.error "), + title: t("toast.error"), type: TOAST_TYPE.ERROR, message: t("entity.delete.failed", { entity: t("issue.label", { count: 1 }) }), }); }As per coding guidelines, errors should be logged appropriately.
apps/web/core/components/onboarding/steps/role/root.tsx (1)
51-78: Critical: Error handling allows progression despite failures.The
handleSubmitUserPersonalizationfunction catches errors and displays a toast but doesn't re-throw the exception. This meansonSubmit(line 76-77) will always proceed to callhandleStepChangeeven when the profile update fails. Users will see an error message but the onboarding will incorrectly advance to the next step.🔎 Proposed fix
const handleSubmitUserPersonalization = async (formData: TProfileSetupFormValues) => { const profileUpdatePayload: Partial<TUserProfile> = { role: formData.role, }; try { await Promise.all([ updateUserProfile(profileUpdatePayload), // totalSteps > 2 && stepChange({ profile_complete: true }), ]); setToast({ type: TOAST_TYPE.SUCCESS, title: "Success", message: "Profile setup completed!", }); } catch { setToast({ type: TOAST_TYPE.ERROR, title: "Error", message: "Profile setup failed. Please try again!", }); + throw new Error("Profile setup failed"); } };apps/web/core/components/onboarding/profile-setup.tsx (1)
150-173: Critical: Empty conditional block breaks password setup flow.Lines 161-162 contain an empty
ifblock whenformData.passwordexists. After successfully setting a password, the onboarding flow does nothing, potentially leaving users stuck. Theelseblock advances to the personalization step when no password is provided, but the password-provided path has no corresponding action.🔎 Verify intended behavior
Based on the onboarding flow, when a password is successfully set, the flow should likely:
- Advance to the personalization step, or
- Complete this step and trigger the next action
Please clarify and implement the intended behavior for the password-provided success path.
apps/web/core/components/issues/issue-layouts/spreadsheet/columns/module-column.tsx (1)
27-41: Removeasynckeyword and wrap the call in try-catch for proper error handling.The function is declared
asyncbut contains noawaitstatements. More critically, thechangeModulesInIssuecall is an async operation that throws errors on failure, but is invoked withoutawaitor error handling. This creates unhandled promise rejections where errors silently fail without logging.Per coding guidelines, errors must be caught and logged appropriately. Wrap the
changeModulesInIssuecall in a try-catch block:const handleModule = useCallback( - async (moduleIds: string[] | null) => { + (moduleIds: string[] | null) => { if (!workspaceSlug || !issue || !issue.project_id || !issue.module_ids || !moduleIds) return; const updatedModuleIds = xor(issue.module_ids, moduleIds); const modulesToAdd: string[] = []; const modulesToRemove: string[] = []; for (const moduleId of updatedModuleIds) { if (issue.module_ids.includes(moduleId)) modulesToRemove.push(moduleId); else modulesToAdd.push(moduleId); } + try { changeModulesInIssue(workspaceSlug.toString(), issue.project_id, issue.id, modulesToAdd, modulesToRemove); + } catch (error) { + console.error("Failed to update modules:", error); + } },apps/web/core/components/issues/issue-detail-widgets/relations/helper.tsx (1)
12-12: Critical: Update the type signature to reflect async return type.The
copyLinkfunction is implemented asasync(line 27), which returnsPromise<void>, but the type signature declares it as returningvoid. This creates a type mismatch that breaks the contract.🔎 Fix the type signature
- copyLink: (path: string) => void; + copyLink: (path: string) => Promise<void>;
♻️ Duplicate comments (8)
apps/web/core/components/views/modal.tsx (1)
61-72: Missing success feedback on view update remains unaddressed.The past review comment correctly identified that
handleUpdateViewlacks a success toast, creating an inconsistency withhandleCreateView(lines 46-49) which does provide success feedback. Users receive no confirmation when a view is successfully updated.apps/web/core/components/issues/peek-overview/header.tsx (1)
141-147: Missing error handling for archive operation.The
handleArchiveIssuefunction lacks a try-catch block, unlikehandleDeleteIssuewhich properly catches errors and shows a toast. IfarchiveIssuefails, users receive no feedback. This inconsistency should be addressed by adding the same error handling pattern used inhandleDeleteIssue.apps/web/core/components/profile/start-of-week-preference.tsx (1)
20-27: Add error logging and type the catch parameter.The issues previously flagged in the past review remain unresolved:
Error logging: Per coding guidelines, errors should be logged appropriately. The catch block only shows a user-facing toast without logging the underlying error for debugging.
Type annotation: The catch parameter should be explicitly typed as
unknownfor TypeScript strict mode compliance.Based on learnings, use try-catch with proper error types and log errors appropriately.
🔎 Proposed fix
const handleStartOfWeekChange = async (val: number) => { try { await updateUserProfile({ start_of_the_week: val }); setToast({ type: TOAST_TYPE.SUCCESS, title: "Success", message: "First day of the week updated successfully" }); - } catch (_error) { + } catch (error: unknown) { + console.error("Failed to update start of week:", error); setToast({ type: TOAST_TYPE.ERROR, title: "Update failed", message: "Please try again later." }); } };apps/web/core/components/workspace/views/delete-view-modal.tsx (1)
29-47: Previous critical issue remains unresolved: cleanup runs even on deletion failure.The cleanup code (lines 43-46) still executes regardless of whether
deleteGlobalViewsucceeds or fails. This is the exact issue flagged in the previous review and has not been addressed.When deletion fails:
- Error toast is shown ✓
- Modal closes immediately ✗ (user cannot retry)
- LocalStorage filters are removed ✗ (creates data inconsistency—server still has the view but local filters are gone)
The cleanup must only run on successful deletion.
🔎 Recommended fix: Move cleanup into try block, use finally for loading state
const handleDeletion = async () => { if (!workspaceSlug) return; setIsDeleteLoading(true); try { await deleteGlobalView(workspaceSlug.toString(), data.id); + // Success: clean up and close modal + handleClose(); + // remove filters from local storage + localStorage.removeItem(`global_view_filters/${data.id}`); } catch (_error) { setToast({ type: TOAST_TYPE.ERROR, title: "Error!", message: "Failed to delete the view. Please try again.", }); + } finally { + setIsDeleteLoading(false); } - - setIsDeleteLoading(false); - handleClose(); - // remove filters from local storage - localStorage.removeItem(`global_view_filters/${data.id}`); };apps/web/core/hooks/use-page-operations.ts (1)
57-64: Missing error handling for clipboard operation.Unlike all other async operations in this file,
copyLinklacks a try/catch block. Clipboard operations can fail (e.g., permissions denied, unsupported browser). For consistency and robustness, consider wrapping this in a try/catch.apps/web/core/components/web-hooks/delete-webhook-modal.tsx (1)
33-48: The issues flagged in the previous review persist: callonClose()after successful deletion and avoid state updates on unmounted component.The refactoring to async/await is correct, but the problems identified in the earlier review remain unresolved:
Missing
onClose()call: Afterrouter.replace()on line 35, the modal should callonClose()to maintain consistency with other delete modals in the codebase (delete-project-modal, delete-module-modal, etc.).State update after unmount: Line 48 executes
setIsDeleting(false)after the try-catch block, meaning it runs even in the success path afterrouter.replace()has unmounted this component—triggering React's "state update on unmounted component" warning.🔎 Recommended fix
Move
onClose()call after navigation and use afinallyblock or remove the unconditionalsetIsDeleting(false):try { await removeWebhook(workspaceSlug.toString(), webhookId.toString()); router.replace(`/${workspaceSlug}/settings/webhooks/`); + onClose(); setToast({ type: TOAST_TYPE.SUCCESS, title: "Success!", message: "Webhook deleted successfully.", }); } catch (_error) { + setIsDeleting(false); setToast({ type: TOAST_TYPE.ERROR, title: "Error!", message: "Webhook could not be deleted. Please try again.", }); } -setIsDeleting(false);Alternatively, follow the pattern in delete-issue-modal: use a
useEffectthat resetsisDeletingwhenisOpenchanges, avoiding state updates after the component unmounts.
Minor: Consider logging errors for debugging.
The error is caught but not logged. Per coding guidelines, errors should be logged appropriately.
🔎 Suggested improvement
} catch (_error) { + console.error("Failed to delete webhook:", _error); setIsDeleting(false); setToast({ type: TOAST_TYPE.ERROR, title: "Error!", message: "Webhook could not be deleted. Please try again.", }); }apps/web/ce/components/projects/create/root.tsx (1)
1-19: Past review comment is outdated.The previous review flagged an unused
PROJECT_TRACKER_EVENTSimport, but this import is not present in the current code. The issue has been resolved.apps/web/core/components/cycles/analytics-sidebar/sidebar-header.tsx (1)
57-60: Critical: Unhandled promise rejection leads to false success feedback.Line 95 calls
submitChanges(payload)without awaiting, and lines 96-100 display a success toast immediately—before the actual update completes. IfupdateCycleDetailsfails at line 59, the error becomes an unhandled rejection while the user sees a false success message.This issue was correctly identified in a previous review and remains unresolved.
🔎 Recommended fix
Option 1: Await
submitChangesand wrap in try-catch at call site (lines 94-107)- if (isDateValid) { - submitChanges(payload); + if (isDateValid) { + try { + await submitChanges(payload); - setToast({ - type: TOAST_TYPE.SUCCESS, - title: t("project_cycles.action.update.success.title"), - message: t("project_cycles.action.update.success.description"), - }); + setToast({ + type: TOAST_TYPE.SUCCESS, + title: t("project_cycles.action.update.success.title"), + message: t("project_cycles.action.update.success.description"), + }); + } catch (error) { + setToast({ + type: TOAST_TYPE.ERROR, + title: t("project_cycles.action.update.failed.title"), + message: t("project_cycles.action.update.failed.description"), + }); + } } else { setToast({Option 2: Add try-catch within
submitChanges(lines 57-60) and handle error thereconst submitChanges = async (data: Partial<ICycle>) => { if (!workspaceSlug || !projectId || !cycleDetails.id) return; + try { - await updateCycleDetails(workspaceSlug.toString(), projectId.toString(), cycleDetails.id.toString(), data); + await updateCycleDetails(workspaceSlug.toString(), projectId.toString(), cycleDetails.id.toString(), data); + } catch (error) { + setToast({ + type: TOAST_TYPE.ERROR, + title: t("project_cycles.action.update.failed.title"), + message: t("project_cycles.action.update.failed.description"), + }); + throw error; // Re-throw so caller knows it failed + } };Then at line 95, await the call:
- if (isDateValid) { - submitChanges(payload); + if (isDateValid) { + try { + await submitChanges(payload); - setToast({ + setToast({Also applies to: 95-100
🧹 Nitpick comments (12)
apps/web/core/components/project/create/header.tsx (1)
79-79: Consider typing the callback parameter more strictly.The
anytype could be replaced with a more specific type from the EmojiPicker component's onChange signature for better type safety.apps/web/core/components/pages/pages-list-main-content.tsx (1)
47-68: Tracking code successfully removed from page creation flow.The event tracking calls have been cleanly removed while preserving the core page creation logic and error handling.
Optional: Refactor to async/await for modern style
Since the function is already
async, you could optionally convert the promise chain to async/await for improved readability:const handleCreatePage = async () => { setIsCreatingPage(true); const payload: Partial<TPage> = { access: pageType === "private" ? EPageAccess.PRIVATE : EPageAccess.PUBLIC, }; - await createPage(payload) - .then((res) => { - const pageId = `/${workspaceSlug}/projects/${currentProjectDetails?.id}/pages/${res?.id}`; - router.push(pageId); - }) - .catch((err) => { - setToast({ - type: TOAST_TYPE.ERROR, - title: "Error!", - message: err?.data?.error || "Page could not be created. Please try again.", - }); - }) - .finally(() => setIsCreatingPage(false)); + try { + const res = await createPage(payload); + const pageId = `/${workspaceSlug}/projects/${currentProjectDetails?.id}/pages/${res?.id}`; + router.push(pageId); + } catch (err) { + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: err?.data?.error || "Page could not be created. Please try again.", + }); + } finally { + setIsCreatingPage(false); + } };apps/web/core/components/labels/delete-label-modal.tsx (1)
32-50: Consider refactoring to use try/catch for consistency.The
handleDeletionfunction is declared asasyncbut uses promise chaining (.then().catch()). For consistency and better readability, consider refactoring to usetry/catchwithawait.🔎 Proposed refactor using try/catch
const handleDeletion = async () => { if (!workspaceSlug || !projectId || !data) return; setIsDeleteLoading(true); - await deleteLabel(workspaceSlug.toString(), projectId.toString(), data.id) - .then(() => { - handleClose(); - }) - .catch((err) => { - setIsDeleteLoading(false); - const error = err?.error || "Label could not be deleted. Please try again."; - setToast({ - type: TOAST_TYPE.ERROR, - title: "Error!", - message: error, - }); - }); + try { + await deleteLabel(workspaceSlug.toString(), projectId.toString(), data.id); + handleClose(); + } catch (err) { + setIsDeleteLoading(false); + const error = (err as any)?.error || "Label could not be deleted. Please try again."; + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: error, + }); + } };apps/web/core/components/workspace/views/quick-action.tsx (1)
35-42: Async/await conversion improves readability.The refactoring from promise chaining to async/await is cleaner and ensures the toast only displays after the clipboard operation completes.
However, consider adding error handling for clipboard failures per the coding guidelines. If
copyUrlToClipboardfails (e.g., permission denied), the error currently propagates unhandled.🔎 Recommended error handling
const handleCopyText = async () => { - await copyUrlToClipboard(viewLink); - setToast({ - type: TOAST_TYPE.SUCCESS, - title: "Link Copied!", - message: "View link copied to clipboard.", - }); + try { + await copyUrlToClipboard(viewLink); + setToast({ + type: TOAST_TYPE.SUCCESS, + title: "Link Copied!", + message: "View link copied to clipboard.", + }); + } catch (error) { + setToast({ + type: TOAST_TYPE.ERROR, + title: "Failed to copy link", + message: "Could not copy link to clipboard. Please try again.", + }); + } };apps/web/core/components/profile/form.tsx (1)
163-167: Consider removing redundant promise chain.Since
setPromiseToastalready handles the promise (lines 152-162), the additional.then().catch()chain is redundant. The empty catch block silences errors that are already handled by the toast.💡 Proposed simplification
setPromiseToast(updateUserAndProfile, { loading: "Updating...", success: { title: "Success!", message: () => `Profile updated successfully.`, }, error: { title: "Error!", message: () => `There was some error in updating your profile. Please try again.`, }, }); - updateUserAndProfile - .then(() => { - return; - }) - .catch(() => {}); };apps/web/core/components/issues/peek-overview/root.tsx (1)
95-101: Consider logging errors for debugging purposes.These catch blocks use the
_errorparameter (intentionally unused) and only display toast messages. While user feedback is provided, the actual error details are discarded, making debugging difficult. Other catch blocks in this file (lines 107-109, 166-168, 203-205) properly log errors. Consider logging these errors as well for consistency and to aid debugging.As per coding guidelines, errors should be logged appropriately.
🔎 Proposed fix to add error logging
- } catch (_error) { + } catch (error) { + console.error("Error removing issue", error); setToast({Apply the same pattern to lines 119, 131, and 142 by changing
_errortoerrorand addingconsole.error()calls with appropriate messages.Also applies to: 119-125, 131-137, 142-148
apps/web/core/components/api-token/delete-token-modal.tsx (1)
51-52: Remove redundant loading state reset.Line 51 calls
handleClose(), which already setsdeleteLoadingtofalseon line 30. The subsequentsetDeleteLoading(false)on line 52 is redundant.🔎 Proposed fix
handleClose(); - setDeleteLoading(false); })apps/web/core/components/issues/issue-detail/root.tsx (1)
88-98: Preferconsole.errorfor error logging.Using
console.errorinstead ofconsole.logbetter indicates error severity and makes debugging easier with proper console filtering.🔎 Suggested improvement
} catch (error) { - console.log("Error in updating issue:", error); + console.error("Error in updating issue:", error); setToast({Note: This pattern appears in multiple operations (lines 92, 110, 122, 163, 182). Consider applying this consistently across all catch blocks.
apps/web/core/components/issues/issue-detail-widgets/sub-issues/helper.ts (1)
137-143: Consider adding error logging for debugging.While the removal of event tracking aligns with the PR's goal, consider adding
console.errorin catch blocks to aid debugging in development and production environments.🔎 Optional: Add console logging
-} catch (_error) { +} catch (error) { + console.error("Failed to update sub-issue:", error); setToast({ type: TOAST_TYPE.ERROR, title: t("toast.error"), message: t("sub_work_item.update.error"), }); }Apply similar pattern to
removeSubIssue(line 170) anddeleteSubIssue(line 189).apps/web/core/components/onboarding/steps/profile/root.tsx (1)
138-139: Consider simplifying the button disable logic.The nested ternary operators make the logic harder to follow. A simplified boolean expression would improve readability.
🔎 Proposed simplification
- const isButtonDisabled = - !isSubmitting && isValid ? (isPasswordAlreadySetup ? false : isValidPassword ? false : true) : true; + const isButtonDisabled = isSubmitting || !isValid || (!isPasswordAlreadySetup && !isValidPassword);This is logically equivalent but more direct: the button is disabled when submitting, form is invalid, or password setup is required but invalid.
apps/web/core/components/issues/issue-detail-widgets/relations/helper.tsx (1)
35-49: Consider logging errors for better observability.While the user-facing toast message is appropriate, the error is completely swallowed without any logging. Consider logging the error for debugging and monitoring purposes.
🔎 Add error logging
- } catch (_error) { + } catch (error) { + console.error("Failed to update issue:", error); setToast({ title: t("toast.error"), type: TOAST_TYPE.ERROR, message: t("entity.update.failed", { entity: entityName }), }); }apps/web/core/components/issues/issue-layouts/quick-add/root.tsx (1)
106-106: Improve type specificity for better type safety.The
setPromiseToastcall uses<any>as the type parameter. Given thatquickAddCallbackreturnsPromise<TIssue | undefined>, consider using a more specific type for better type safety.🔎 Proposed fix
- setPromiseToast<any>(quickAddPromise, { + setPromiseToast<TIssue | undefined>(quickAddPromise, {
| remove: async (workspaceSlug, projectId, issueId) => { | ||
| try { | ||
| return removeIssue(workspaceSlug, projectId, issueId).then(() => { | ||
| captureSuccess({ | ||
| eventName: WORK_ITEM_TRACKER_EVENTS.delete, | ||
| payload: { id: issueId }, | ||
| }); | ||
| }); | ||
| } catch (error) { | ||
| captureError({ | ||
| eventName: WORK_ITEM_TRACKER_EVENTS.delete, | ||
| payload: { id: issueId }, | ||
| error: error as Error, | ||
| }); | ||
| } | ||
| return removeIssue(workspaceSlug, projectId, issueId); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/web/core/components/issues/issue-detail-widgets/relations/helper.tsx | head -80Repository: makeplane/plane
Length of output: 2633
🏁 Script executed:
rg -n 'useRelationOperations|remove\(' apps/web/core/components/issues/issue-detail-widgets/relations/ --type ts --type tsx -A 5 -B 2Repository: makeplane/plane
Length of output: 86
🏁 Script executed:
# Search for remove calls at the application level
rg -n '\.remove\(' apps/web --type ts --type tsx -B 3 -A 8 | grep -A 8 "relation"Repository: makeplane/plane
Length of output: 86
🏁 Script executed:
rg -n 'remove\(' apps/web/core/components/issues/issue-detail-widgets/relations/ -A 5 -B 2Repository: makeplane/plane
Length of output: 41
🏁 Script executed:
# Find files that import useRelationOperations
rg -l 'useRelationOperations' apps/web --type ts --type tsx 2>/dev/null | head -10Repository: makeplane/plane
Length of output: 41
🏁 Script executed:
fd -e tsx -e ts . apps/web/core/components/issues/issue-detail-widgets/relations/ -type fRepository: makeplane/plane
Length of output: 229
🏁 Script executed:
rg "useRelationOperations" apps/web --max-count 10Repository: makeplane/plane
Length of output: 894
🏁 Script executed:
# Search for where remove is actually called/used
rg "\.remove\s*\(" apps/web --max-count 20 -B 3 -A 5Repository: makeplane/plane
Length of output: 13462
🏁 Script executed:
cat apps/web/core/components/issues/relations/issue-list-item.tsxRepository: makeplane/plane
Length of output: 7552
🏁 Script executed:
cat apps/web/core/components/issues/issue-detail-widgets/relations/content.tsxRepository: makeplane/plane
Length of output: 8250
Add error handling to the remove operation to match update's pattern.
The remove function lacks error handling unlike update (lines 35-49), which wraps operations in try-catch and shows error toasts. Without error handling, failures from removeIssue propagate to callers in content.tsx (line 133-138), where there is also no error handling. This leaves users without feedback when deletion fails and can leave the UI in an inconsistent state.
🤖 Prompt for AI Agents
In apps/web/core/components/issues/issue-detail-widgets/relations/helper.tsx
around lines 51 to 53, the remove handler calls removeIssue without error
handling; wrap the call in a try-catch like update does, await removeIssue
inside try, return true on success, catch errors and show a toast error (same
message pattern used in update), log or include the error details in the toast,
and return false in the catch so callers can react to failures.
| copyLink: async (path) => { | ||
| await copyUrlToClipboard(path); | ||
| setToast({ | ||
| type: TOAST_TYPE.SUCCESS, | ||
| title: t("common.link_copied"), | ||
| message: t("entity.link_copied_to_clipboard", { | ||
| entity: | ||
| issueServiceType === EIssueServiceType.ISSUES | ||
| ? t("common.sub_work_items", { count: 1 }) | ||
| : t("issue.label", { count: 1 }), | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Add error handling to provide user feedback on copy failure.
The refactor to async/await is correct, but if copyUrlToClipboard throws, the user receives no feedback. Wrap the operation in try-catch to display an error toast on failure.
🔎 Proposed fix to add error handling
copyLink: async (path) => {
- await copyUrlToClipboard(path);
- setToast({
- type: TOAST_TYPE.SUCCESS,
- title: t("common.link_copied"),
- message: t("entity.link_copied_to_clipboard", {
- entity:
- issueServiceType === EIssueServiceType.ISSUES
- ? t("common.sub_work_items", { count: 1 })
- : t("issue.label", { count: 1 }),
- }),
- });
+ try {
+ await copyUrlToClipboard(path);
+ setToast({
+ type: TOAST_TYPE.SUCCESS,
+ title: t("common.link_copied"),
+ message: t("entity.link_copied_to_clipboard", {
+ entity:
+ issueServiceType === EIssueServiceType.ISSUES
+ ? t("common.sub_work_items", { count: 1 })
+ : t("issue.label", { count: 1 }),
+ }),
+ });
+ } catch {
+ setToast({
+ type: TOAST_TYPE.ERROR,
+ title: t("toast.error"),
+ message: t("common.link_copy_failed"),
+ });
+ }
},📝 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.
| copyLink: async (path) => { | |
| await copyUrlToClipboard(path); | |
| setToast({ | |
| type: TOAST_TYPE.SUCCESS, | |
| title: t("common.link_copied"), | |
| message: t("entity.link_copied_to_clipboard", { | |
| entity: | |
| issueServiceType === EIssueServiceType.ISSUES | |
| ? t("common.sub_work_items", { count: 1 }) | |
| : t("issue.label", { count: 1 }), | |
| }), | |
| }); | |
| copyLink: async (path) => { | |
| try { | |
| await copyUrlToClipboard(path); | |
| setToast({ | |
| type: TOAST_TYPE.SUCCESS, | |
| title: t("common.link_copied"), | |
| message: t("entity.link_copied_to_clipboard", { | |
| entity: | |
| issueServiceType === EIssueServiceType.ISSUES | |
| ? t("common.sub_work_items", { count: 1 }) | |
| : t("issue.label", { count: 1 }), | |
| }), | |
| }); | |
| } catch { | |
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: t("toast.error"), | |
| message: t("common.link_copy_failed"), | |
| }); | |
| } | |
| }, |
🤖 Prompt for AI Agents
In apps/web/core/components/issues/issue-detail-widgets/sub-issues/helper.ts
around lines 40 to 51, the async copyLink handler awaits copyUrlToClipboard but
has no error handling; wrap the await call in a try-catch, on success keep the
existing success toast, and in the catch log the error (optional) and call
setToast with an error type and a localized failure title/message so the user is
informed if copying fails.
| const handleCopyText = async () => { | ||
| try { | ||
| const originURL = typeof window !== "undefined" && window.location.origin ? window.location.origin : ""; | ||
| await copyTextToClipboard(`${originURL}${workItemLink}`); | ||
| setToast({ | ||
| type: TOAST_TYPE.SUCCESS, | ||
| title: t("common.link_copied"), | ||
| message: t("common.copied_to_clipboard"), | ||
| }); | ||
| }); | ||
| } catch (_error) { | ||
| setToast({ | ||
| title: t("toast.error"), | ||
| type: TOAST_TYPE.ERROR, | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error logging for debugging.
While the async/await conversion and user-facing error handling are correct, errors are not logged anywhere. This makes debugging production issues difficult and violates the coding guideline: "Use try-catch with proper error types and log errors appropriately."
🔎 Recommended fix to add error logging
- } catch (_error) {
+ } catch (error) {
+ console.error("Failed to copy issue link:", error);
setToast({
title: t("toast.error"),
type: TOAST_TYPE.ERROR,
});
}As per coding guidelines, errors should be logged appropriately.
📝 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 handleCopyText = async () => { | |
| try { | |
| const originURL = typeof window !== "undefined" && window.location.origin ? window.location.origin : ""; | |
| await copyTextToClipboard(`${originURL}${workItemLink}`); | |
| setToast({ | |
| type: TOAST_TYPE.SUCCESS, | |
| title: t("common.link_copied"), | |
| message: t("common.copied_to_clipboard"), | |
| }); | |
| }); | |
| } catch (_error) { | |
| setToast({ | |
| title: t("toast.error"), | |
| type: TOAST_TYPE.ERROR, | |
| }); | |
| } | |
| }; | |
| const handleCopyText = async () => { | |
| try { | |
| const originURL = typeof window !== "undefined" && window.location.origin ? window.location.origin : ""; | |
| await copyTextToClipboard(`${originURL}${workItemLink}`); | |
| setToast({ | |
| type: TOAST_TYPE.SUCCESS, | |
| title: t("common.link_copied"), | |
| message: t("common.copied_to_clipboard"), | |
| }); | |
| } catch (error) { | |
| console.error("Failed to copy issue link:", error); | |
| setToast({ | |
| title: t("toast.error"), | |
| type: TOAST_TYPE.ERROR, | |
| }); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In apps/web/core/components/issues/issue-detail/issue-detail-quick-actions.tsx
around lines 69 to 84, the catch block swallows errors without logging them;
update the catch to capture the caught error (e.g., error) and log it with
contextual information before showing the toast — for example call
console.error("Failed to copy work item link", error) or send it to the app's
centralized logger/Sentry with the same context so the error message and stack
are available for debugging while preserving the existing user-facing toast.
| const handleArchiveIssue = async () => { | ||
| try { | ||
| await archiveIssue(workspaceSlug, projectId, issueId); | ||
| router.push(`/${workspaceSlug}/projects/${projectId}/issues`); | ||
| captureSuccess({ | ||
| eventName: WORK_ITEM_TRACKER_EVENTS.archive, | ||
| payload: { id: issueId }, | ||
| }); | ||
| } catch (error) { | ||
| captureError({ | ||
| eventName: WORK_ITEM_TRACKER_EVENTS.archive, | ||
| payload: { id: issueId }, | ||
| error: error as Error, | ||
| } catch (_error) { | ||
| setToast({ | ||
| title: t("toast.error"), | ||
| type: TOAST_TYPE.ERROR, | ||
| message: t("issue.archive.failed.message"), | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error logging for debugging.
Errors are not logged in the catch block, which makes debugging production issues difficult and violates coding guidelines.
🔎 Recommended fix to add error logging
await archiveIssue(workspaceSlug, projectId, issueId);
router.push(`/${workspaceSlug}/projects/${projectId}/issues`);
- } catch (_error) {
+ } catch (error) {
+ console.error("Failed to archive issue:", error);
setToast({
title: t("toast.error"),
type: TOAST_TYPE.ERROR,
message: t("issue.archive.failed.message"),
});
}As per coding guidelines, errors should be logged appropriately.
📝 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 handleArchiveIssue = async () => { | |
| try { | |
| await archiveIssue(workspaceSlug, projectId, issueId); | |
| router.push(`/${workspaceSlug}/projects/${projectId}/issues`); | |
| captureSuccess({ | |
| eventName: WORK_ITEM_TRACKER_EVENTS.archive, | |
| payload: { id: issueId }, | |
| }); | |
| } catch (error) { | |
| captureError({ | |
| eventName: WORK_ITEM_TRACKER_EVENTS.archive, | |
| payload: { id: issueId }, | |
| error: error as Error, | |
| } catch (_error) { | |
| setToast({ | |
| title: t("toast.error"), | |
| type: TOAST_TYPE.ERROR, | |
| message: t("issue.archive.failed.message"), | |
| }); | |
| } | |
| }; | |
| const handleArchiveIssue = async () => { | |
| try { | |
| await archiveIssue(workspaceSlug, projectId, issueId); | |
| router.push(`/${workspaceSlug}/projects/${projectId}/issues`); | |
| } catch (error) { | |
| console.error("Failed to archive issue:", error); | |
| setToast({ | |
| title: t("toast.error"), | |
| type: TOAST_TYPE.ERROR, | |
| message: t("issue.archive.failed.message"), | |
| }); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In apps/web/core/components/issues/issue-detail/issue-detail-quick-actions.tsx
around lines 104 to 115, the catch block swallows the error (catch (_error)) and
does not log it; change the catch to capture the error (e.g., catch (error)) and
log a clear contextual message plus the error object (for example via
console.error or the app's logger) before setting the toast so that production
failures can be diagnosed.
| const handleRestore = async () => { | ||
| if (!workspaceSlug || !projectId || !issueId) return; | ||
|
|
||
| await restoreIssue(workspaceSlug.toString(), projectId.toString(), issueId.toString()) | ||
| .then(() => { | ||
| setToast({ | ||
| type: TOAST_TYPE.SUCCESS, | ||
| title: t("issue.restore.success.title"), | ||
| message: t("issue.restore.success.message"), | ||
| }); | ||
| router.push(workItemLink); | ||
| }) | ||
| .catch(() => { | ||
| setToast({ | ||
| type: TOAST_TYPE.ERROR, | ||
| title: t("toast.error"), | ||
| message: t("issue.restore.failed.message"), | ||
| }); | ||
| try { | ||
| await restoreIssue(workspaceSlug.toString(), projectId.toString(), issueId.toString()); | ||
| setToast({ | ||
| type: TOAST_TYPE.SUCCESS, | ||
| title: t("issue.restore.success.title"), | ||
| message: t("issue.restore.success.message"), | ||
| }); | ||
| router.push(workItemLink); | ||
| } catch (_error) { | ||
| setToast({ | ||
| title: t("toast.error"), | ||
| type: TOAST_TYPE.ERROR, | ||
| message: t("issue.restore.failed.message"), | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error logging for debugging.
The async/await refactor and success toast are good improvements. However, errors are not logged in the catch block, which makes debugging production issues difficult and violates coding guidelines.
🔎 Recommended fix to add error logging
message: t("issue.restore.success.message"),
});
router.push(workItemLink);
- } catch (_error) {
+ } catch (error) {
+ console.error("Failed to restore issue:", error);
setToast({
title: t("toast.error"),
type: TOAST_TYPE.ERROR,
message: t("issue.restore.failed.message"),
});
}As per coding guidelines, errors should be logged appropriately.
📝 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 handleRestore = async () => { | |
| if (!workspaceSlug || !projectId || !issueId) return; | |
| await restoreIssue(workspaceSlug.toString(), projectId.toString(), issueId.toString()) | |
| .then(() => { | |
| setToast({ | |
| type: TOAST_TYPE.SUCCESS, | |
| title: t("issue.restore.success.title"), | |
| message: t("issue.restore.success.message"), | |
| }); | |
| router.push(workItemLink); | |
| }) | |
| .catch(() => { | |
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: t("toast.error"), | |
| message: t("issue.restore.failed.message"), | |
| }); | |
| try { | |
| await restoreIssue(workspaceSlug.toString(), projectId.toString(), issueId.toString()); | |
| setToast({ | |
| type: TOAST_TYPE.SUCCESS, | |
| title: t("issue.restore.success.title"), | |
| message: t("issue.restore.success.message"), | |
| }); | |
| router.push(workItemLink); | |
| } catch (_error) { | |
| setToast({ | |
| title: t("toast.error"), | |
| type: TOAST_TYPE.ERROR, | |
| message: t("issue.restore.failed.message"), | |
| }); | |
| } | |
| }; | |
| const handleRestore = async () => { | |
| if (!workspaceSlug || !projectId || !issueId) return; | |
| try { | |
| await restoreIssue(workspaceSlug.toString(), projectId.toString(), issueId.toString()); | |
| setToast({ | |
| type: TOAST_TYPE.SUCCESS, | |
| title: t("issue.restore.success.title"), | |
| message: t("issue.restore.success.message"), | |
| }); | |
| router.push(workItemLink); | |
| } catch (error) { | |
| console.error("Failed to restore issue:", error); | |
| setToast({ | |
| title: t("toast.error"), | |
| type: TOAST_TYPE.ERROR, | |
| message: t("issue.restore.failed.message"), | |
| }); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In apps/web/core/components/issues/issue-detail/issue-detail-quick-actions.tsx
around lines 117 to 134, the catch block for handleRestore swallows errors;
update it to log the caught error with useful context before showing the toast
(e.g., console.error or the app logger) including workspaceSlug, projectId,
issueId and the error/stack so debugging is possible; perform the log inside the
catch (capture error as err) and then keep the existing toast and error
handling.
| .catch((_error) => { | ||
| setToast({ | ||
| type: TOAST_TYPE.ERROR, | ||
| title: `${t("error")}!`, | ||
| message: t("workspace_draft_issues.toasts.created.error"), | ||
| }); | ||
| captureError({ | ||
| eventName: WORK_ITEM_TRACKER_EVENTS.draft.create, | ||
| payload: { id: payload.id }, | ||
| error, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Log the error for debugging.
While removing event tracking is the goal of this PR, the error should still be logged to the console for debugging purposes. Currently, error details are completely lost, making it difficult to troubleshoot draft creation failures.
🔎 Proposed fix to add error logging
- .catch((_error) => {
+ .catch((error) => {
+ console.error("Failed to create draft issue:", error);
setToast({
type: TOAST_TYPE.ERROR,
title: `${t("error")}!`,
message: t("workspace_draft_issues.toasts.created.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.
| .catch((_error) => { | |
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: `${t("error")}!`, | |
| message: t("workspace_draft_issues.toasts.created.error"), | |
| }); | |
| captureError({ | |
| eventName: WORK_ITEM_TRACKER_EVENTS.draft.create, | |
| payload: { id: payload.id }, | |
| error, | |
| }); | |
| }); | |
| .catch((error) => { | |
| console.error("Failed to create draft issue:", error); | |
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: `${t("error")}!`, | |
| message: t("workspace_draft_issues.toasts.created.error"), | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In apps/web/core/components/issues/issue-modal/draft-issue-layout.tsx around
lines 93 to 99 the .catch handler swallows the error; modify it to log the
caught error before showing the toast (e.g., console.error with a short context
message and the error object) so debugging information is preserved, then keep
the existing setToast call unchanged.
| await handleSubmitUserDetail(formData); | ||
| handleStepChange(EOnboardingSteps.PROFILE_SETUP); |
There was a problem hiding this comment.
Error handling issue: User proceeds to next step even if profile update fails.
If handleSubmitUserDetail fails (Lines 82-100), the error is caught and a toast is shown, but the error is not re-thrown. This means onSubmit will continue executing and call handleStepChange, advancing the user to the next onboarding step even though their profile data (name, avatar, password) was not saved.
🔎 Proposed fix
Option 1: Re-throw the error in handleSubmitUserDetail
const handleSubmitUserDetail = async (formData: TProfileSetupFormValues) => {
const userDetailsPayload: Partial<IUser> = {
first_name: formData.first_name,
last_name: formData.last_name,
avatar_url: formData.avatar_url ?? undefined,
};
try {
await Promise.all([
updateCurrentUser(userDetailsPayload),
formData.password && handleSetPassword(formData.password),
]);
} catch {
setToast({
type: TOAST_TYPE.ERROR,
title: "Error",
message: "User details update failed. Please try again!",
});
+ throw error;
}
};Option 2: Wrap the onSubmit call in try-catch
const onSubmit = async (formData: TProfileSetupFormValues) => {
if (!user) return;
updateUserProfile({
has_marketing_email_consent: formData.has_marketing_email_consent,
});
- await handleSubmitUserDetail(formData);
- handleStepChange(EOnboardingSteps.PROFILE_SETUP);
+ try {
+ await handleSubmitUserDetail(formData);
+ handleStepChange(EOnboardingSteps.PROFILE_SETUP);
+ } catch {
+ // Error already shown via toast in handleSubmitUserDetail
+ }
};🤖 Prompt for AI Agents
In apps/web/core/components/onboarding/steps/profile/root.tsx around lines
107-108, the handler calls await handleSubmitUserDetail(formData) then
immediately calls handleStepChange, allowing progression even when submit fails;
fix by either re-throwing the caught error inside handleSubmitUserDetail so the
await rejects and prevents further execution, or modify the onSubmit to wrap
await handleSubmitUserDetail(formData) in a try/catch that only calls
handleStepChange if no error occurred (and preserves showing the toast on
error).
| } catch (_error) { | ||
| setToast({ | ||
| type: TOAST_TYPE.ERROR, | ||
| title: t("workspace_settings.settings.general.delete_modal.error_title"), | ||
| message: t("workspace_settings.settings.general.delete_modal.error_message"), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Consider logging the error for debugging purposes.
While the error toast provides user feedback, the error is not being logged. Per the coding guidelines, "Use try-catch with proper error types and log errors appropriately." Since deleteWorkspace can throw response data that may contain useful debugging information, consider logging the error even though the underscore prefix indicates intentional non-use.
Based on learnings, errors should be logged appropriately.
🔎 Suggested improvement
- } catch (_error) {
+ } catch (error) {
+ console.error("Failed to delete workspace:", error);
setToast({
type: TOAST_TYPE.ERROR,
title: t("workspace_settings.settings.general.delete_modal.error_title"),
message: t("workspace_settings.settings.general.delete_modal.error_message"),
});
}🤖 Prompt for AI Agents
In apps/web/core/components/workspace/delete-workspace-form.tsx around lines 68
to 74, the catch block swallows the error (_error) without logging it; change
the catch to capture the error (e.g., error) and log the full error details
before setting the toast so debugging info from deleteWorkspace (response data,
stack) is preserved—use the existing app logger if available or console.error
with a clear message and the error object.
| const res = await updateGlobalView(workspaceSlug, data.id, payloadData); | ||
| if (res) { | ||
| resetExpression(EIssuesStoreType.GLOBAL, data.id, res.rich_filters); | ||
| setToast({ | ||
| type: TOAST_TYPE.ERROR, | ||
| title: "Error!", | ||
| message: "View could not be updated. Please try again.", | ||
| type: TOAST_TYPE.SUCCESS, | ||
| title: "Success!", | ||
| message: "View updated successfully.", | ||
| }); | ||
| handleClose(); | ||
| } | ||
| } catch (_error) { | ||
| setToast({ | ||
| type: TOAST_TYPE.ERROR, | ||
| title: "Error!", | ||
| message: "View could not be updated. Please try again.", | ||
| }); | ||
| } |
There was a problem hiding this comment.
Silent failure possible when store catches error internally.
Based on the store implementation in global-view.store.ts, updateGlobalView catches errors internally and returns undefined instead of re-throwing. This means:
- If the API call fails, the store catches it, restores state, and returns
undefined - The
if (res)guard correctly prevents accessingres.rich_filtersonundefined - However, the
catchblock here won't execute because no error is thrown - Result: no toast is shown to the user on failure
The if (res) guard is necessary given the current store behavior. Consider adding an else branch for user feedback:
🔎 Proposed fix to handle silent failure
const res = await updateGlobalView(workspaceSlug, data.id, payloadData);
if (res) {
resetExpression(EIssuesStoreType.GLOBAL, data.id, res.rich_filters);
setToast({
type: TOAST_TYPE.SUCCESS,
title: "Success!",
message: "View updated successfully.",
});
handleClose();
+ } else {
+ setToast({
+ type: TOAST_TYPE.ERROR,
+ title: "Error!",
+ message: "View could not be updated. Please try again.",
+ });
}
} catch (_error) {🤖 Prompt for AI Agents
In apps/web/core/components/workspace/views/modal.tsx around lines 73 to 89, the
call to updateGlobalView can fail silently because the store catches errors and
returns undefined, so the current code only handles the success path and the
catch block will not run; add an explicit else branch after the if (res) to
handle the undefined result by showing an error toast (same or similar message
as the catch) and avoid calling resetExpression when res is falsy, ensuring the
user receives feedback when the update silently fails.
| eventName: PROJECT_PAGE_TRACKER_EVENTS.duplicate, | ||
| error, | ||
| }); | ||
| } catch (_error) { |
There was a problem hiding this comment.
Add error logging to all catch blocks.
All catch blocks throughout this file catch errors but never log them. While showing user-facing toasts is good UX, the complete absence of error logging makes debugging production issues extremely difficult. When operations fail, there's no trace of what went wrong (network errors, API failures, permission issues, etc.).
As per coding guidelines: "Use try-catch with proper error types and log errors appropriately."
Consider adding console.error or a proper logging service call in each catch block before showing the toast:
} catch (error) {
console.error("Failed to duplicate page:", error);
setToast({
type: TOAST_TYPE.ERROR,
title: "Error!",
message: "Page could not be duplicated. Please try again later.",
});
}This pattern should be applied to all operations: duplicate, toggleAccess, toggleArchive, toggleFavorite, and toggleLock.
Also applies to: 94-94, 111-111, 126-126, 144-144, 150-150, 160-160, 166-166, 178-178, 193-193
🤖 Prompt for AI Agents
In apps/web/core/hooks/use-page-operations.ts around lines 73, 94, 111, 126,
144, 150, 160, 166, 178 and 193, each catch block currently swallows the error
(uses `_error`) and only shows a toast; change each catch to capture the error
(e.g., `catch (error)` or `catch (err: unknown)`), log the error with context
using console.error or the app logger (e.g., "Failed to duplicate page:", error)
immediately before showing the toast, and keep the existing toast behavior;
ensure the log message is specific to the operation for easier debugging
(duplicate, toggleAccess, toggleArchive, toggleFavorite, toggleLock).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/ce/components/projects/create/root.tsx (2)
67-82: Good error handling with minor style suggestion.The try-catch block properly handles cover image upload failures with logging and user feedback. The error rejection correctly prevents form submission.
💡 Optional: Use
throwinstead ofreturn Promise.reject()In async functions, throwing is more idiomatic than explicitly returning a rejected promise:
} catch (error) { console.error("Error uploading cover image:", error); setToast({ type: TOAST_TYPE.ERROR, title: t("toast.error"), message: error instanceof Error ? error.message : "Failed to upload cover image", }); - return Promise.reject(error); + throw error; }Both approaches work identically, but
throwis more conventional in async/await code.
89-149: Consider converting to async/await for consistency.The function mixes async/await (for
uploadCoverImage) with.then()/.catch()(forcreateProject). SinceonSubmitis already async and the PR aims to modernize promise handling, converting this to async/await would improve consistency and readability.🔎 Refactor to async/await pattern
- return createProject(workspaceSlug.toString(), formData) - .then(async (res) => { - if (uploadedAssetUrl) { - await updateCoverImageStatus(res.id, uploadedAssetUrl); - await updateProject(workspaceSlug.toString(), res.id, { cover_image_url: uploadedAssetUrl }); - } else if (coverImage && coverImage.startsWith("http")) { - await updateCoverImageStatus(res.id, coverImage); - await updateProject(workspaceSlug.toString(), res.id, { cover_image_url: coverImage }); - } - setToast({ - type: TOAST_TYPE.SUCCESS, - title: t("success"), - message: t("project_created_successfully"), - }); - - if (setToFavorite) { - handleAddToFavorites(res.id); - } - handleNextStep(res.id); - }) - .catch((err) => { - try { - // Handle the new error format where codes are nested in arrays under field names - const errorData = err?.data ?? {}; - - const nameError = errorData.name?.includes("PROJECT_NAME_ALREADY_EXIST"); - const identifierError = errorData?.identifier?.includes("PROJECT_IDENTIFIER_ALREADY_EXIST"); - - if (nameError || identifierError) { - if (nameError) { - setToast({ - type: TOAST_TYPE.ERROR, - title: t("toast.error"), - message: t("project_name_already_taken"), - }); - } - - if (identifierError) { - setToast({ - type: TOAST_TYPE.ERROR, - title: t("toast.error"), - message: t("project_identifier_already_taken"), - }); - } - } else { - setToast({ - type: TOAST_TYPE.ERROR, - title: t("toast.error"), - message: t("something_went_wrong"), - }); - } - } catch (error) { - // Fallback error handling if the error processing fails - console.error("Error processing API error:", error); - setToast({ - type: TOAST_TYPE.ERROR, - title: t("toast.error"), - message: t("something_went_wrong"), - }); - } - }); + try { + const res = await createProject(workspaceSlug.toString(), formData); + + if (uploadedAssetUrl) { + await updateCoverImageStatus(res.id, uploadedAssetUrl); + await updateProject(workspaceSlug.toString(), res.id, { cover_image_url: uploadedAssetUrl }); + } else if (coverImage && coverImage.startsWith("http")) { + await updateCoverImageStatus(res.id, coverImage); + await updateProject(workspaceSlug.toString(), res.id, { cover_image_url: coverImage }); + } + + setToast({ + type: TOAST_TYPE.SUCCESS, + title: t("success"), + message: t("project_created_successfully"), + }); + + if (setToFavorite) { + handleAddToFavorites(res.id); + } + handleNextStep(res.id); + } catch (err) { + try { + // Handle the new error format where codes are nested in arrays under field names + const errorData = err?.data ?? {}; + + const nameError = errorData.name?.includes("PROJECT_NAME_ALREADY_EXIST"); + const identifierError = errorData?.identifier?.includes("PROJECT_IDENTIFIER_ALREADY_EXIST"); + + if (nameError || identifierError) { + if (nameError) { + setToast({ + type: TOAST_TYPE.ERROR, + title: t("toast.error"), + message: t("project_name_already_taken"), + }); + } + + if (identifierError) { + setToast({ + type: TOAST_TYPE.ERROR, + title: t("toast.error"), + message: t("project_identifier_already_taken"), + }); + } + } else { + setToast({ + type: TOAST_TYPE.ERROR, + title: t("toast.error"), + message: t("something_went_wrong"), + }); + } + } catch (error) { + // Fallback error handling if the error processing fails + console.error("Error processing API error:", error); + setToast({ + type: TOAST_TYPE.ERROR, + title: t("toast.error"), + message: t("something_went_wrong"), + }); + } + }This eliminates the pattern mixing and makes the code flow more linear and easier to follow.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/members/page.tsxapps/web/ce/components/projects/create/root.tsxapps/web/core/components/project/create/header.tsxapps/web/core/components/workspace/settings/invitations-list-item.tsxapps/web/core/components/workspace/settings/members-list-item.tsx
💤 Files with no reviewable changes (2)
- apps/web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/members/page.tsx
- apps/web/core/components/workspace/settings/members-list-item.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/core/components/workspace/settings/invitations-list-item.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
apps/web/ce/components/projects/create/root.tsxapps/web/core/components/project/create/header.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Enable TypeScript strict mode and ensure all files are fully typed
Files:
apps/web/ce/components/projects/create/root.tsxapps/web/core/components/project/create/header.tsx
**/*.{js,jsx,ts,tsx,json,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier with Tailwind plugin for code formatting, run
pnpm fix:format
Files:
apps/web/ce/components/projects/create/root.tsxapps/web/core/components/project/create/header.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use ESLint with shared config across packages, adhering to max warnings limits per package
Use camelCase for variable and function names, PascalCase for components and types
Use try-catch with proper error types and log errors appropriately
Files:
apps/web/ce/components/projects/create/root.tsxapps/web/core/components/project/create/header.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-01T15:30:17.605Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7888
File: packages/propel/src/avatar/avatar.stories.tsx:2-3
Timestamp: 2025-10-01T15:30:17.605Z
Learning: In the makeplane/plane repository, avoid suggesting inline type imports (e.g., `import { Avatar, type TAvatarSize }`) due to bundler compatibility issues. Keep type imports and value imports as separate statements.
Applied to files:
apps/web/ce/components/projects/create/root.tsx
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.
Applied to files:
apps/web/ce/components/projects/create/root.tsx
📚 Learning: 2025-10-21T17:22:05.204Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7989
File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46
Timestamp: 2025-10-21T17:22:05.204Z
Learning: In the makeplane/plane repository, the refactor from useParams() to params prop is specifically scoped to page.tsx and layout.tsx files in apps/web/app (Next.js App Router pattern). Other components (hooks, regular client components, utilities) should continue using the useParams() hook as that is the correct pattern for non-route components.
Applied to files:
apps/web/ce/components/projects/create/root.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build packages
🔇 Additional comments (2)
apps/web/core/components/project/create/header.tsx (1)
58-58: LGTM! Clean removal of PostHog tracking attribute.The removal of the
data-posthogattribute aligns with the PR objective. The button remains fully functional with itsonClickhandler and accessibility is preserved withtabIndex.apps/web/ce/components/projects/create/root.tsx (1)
1-19: LGTM: Clean import organization.The import reorganization with categorical comments improves readability and follows a logical grouping pattern (external libraries → plane packages → internal components/hooks → types).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/core/components/account/auth-forms/set-password.tsx (2)
61-64: Consider async/await for consistency.The CSRF token fetch uses
.then()chaining while the form submission handler (line 82) usesasync/await. Converting this to async/await would improve consistency.🔎 Proposed refactor
- useEffect(() => { - if (csrfToken === undefined) - authService.requestCSRFToken().then((data) => data?.csrf_token && setCsrfToken(data.csrf_token)); - }, [csrfToken]); + useEffect(() => { + const fetchCSRFToken = async () => { + if (csrfToken === undefined) { + const data = await authService.requestCSRFToken(); + if (data?.csrf_token) setCsrfToken(data.csrf_token); + } + }; + fetchCSRFToken(); + }, [csrfToken]);
109-109: Simplify form submit handler.The inline arrow function wrapper is unnecessary since
handleSubmitalready accepts the event parameter.🔎 Proposed simplification
- <form className="space-y-4" onSubmit={(e) => handleSubmit(e)}> + <form className="space-y-4" onSubmit={handleSubmit}>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/app/(all)/[workspaceSlug]/(projects)/analytics/[tabId]/page.tsxapps/web/ce/components/onboarding/tour/root.tsxapps/web/core/components/account/auth-forms/set-password.tsxapps/web/core/components/automation/auto-archive-automation.tsxapps/web/core/components/pages/modals/delete-page-modal.tsxapps/web/core/components/project-states/create-update/create.tsx
💤 Files with no reviewable changes (2)
- apps/web/core/components/pages/modals/delete-page-modal.tsx
- apps/web/ce/components/onboarding/tour/root.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/core/components/automation/auto-archive-automation.tsx
- apps/web/app/(all)/[workspaceSlug]/(projects)/analytics/[tabId]/page.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
apps/web/core/components/account/auth-forms/set-password.tsxapps/web/core/components/project-states/create-update/create.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Enable TypeScript strict mode and ensure all files are fully typed
Files:
apps/web/core/components/account/auth-forms/set-password.tsxapps/web/core/components/project-states/create-update/create.tsx
**/*.{js,jsx,ts,tsx,json,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier with Tailwind plugin for code formatting, run
pnpm fix:format
Files:
apps/web/core/components/account/auth-forms/set-password.tsxapps/web/core/components/project-states/create-update/create.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use ESLint with shared config across packages, adhering to max warnings limits per package
Use camelCase for variable and function names, PascalCase for components and types
Use try-catch with proper error types and log errors appropriately
Files:
apps/web/core/components/account/auth-forms/set-password.tsxapps/web/core/components/project-states/create-update/create.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-21T17:22:05.204Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7989
File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46
Timestamp: 2025-10-21T17:22:05.204Z
Learning: In the makeplane/plane repository, the refactor from useParams() to params prop is specifically scoped to page.tsx and layout.tsx files in apps/web/app (Next.js App Router pattern). Other components (hooks, regular client components, utilities) should continue using the useParams() hook as that is the correct pattern for non-route components.
Applied to files:
apps/web/core/components/account/auth-forms/set-password.tsx
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.
Applied to files:
apps/web/core/components/account/auth-forms/set-password.tsx
📚 Learning: 2025-12-12T15:20:36.542Z
Learnt from: CR
Repo: makeplane/plane PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T15:20:36.542Z
Learning: Applies to packages/shared-state/**/*.{ts,tsx} : Maintain MobX stores in `packages/shared-state` using reactive patterns
Applied to files:
apps/web/core/components/project-states/create-update/create.tsx
🧬 Code graph analysis (1)
apps/web/core/components/project-states/create-update/create.tsx (1)
packages/propel/src/icons/state/helper.tsx (1)
TStateGroups(20-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check:types
- GitHub Check: check:lint
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/web/core/components/account/auth-forms/set-password.tsx (1)
8-8: LGTM! Clean removal of tracking imports.The import statement has been correctly updated to remove the tracking-related constants while preserving
E_PASSWORD_STRENGTH, which is still used in the component.apps/web/core/components/project-states/create-update/create.tsx (1)
27-58: Previous critical issue resolved - user feedback now always displays.The toasts are now correctly shown unconditionally on both success and error paths. This ensures users always receive feedback about state creation results, independent of any tracking configuration. The fix properly addresses the critical issue flagged in the previous review.
| type TStateCreate = { | ||
| groupKey: TStateGroups; | ||
| shouldTrackEvents: boolean; | ||
| shouldTrackEvents?: boolean; | ||
| createStateCallback: TStateOperationsCallbacks["createState"]; | ||
| handleClose: () => void; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused shouldTrackEvents prop from type definition.
The shouldTrackEvents prop is defined in the TStateCreate type but is never destructured or used in the component (line 17). Since event tracking has been removed, this prop should be deleted entirely from the type definition.
🔎 Proposed fix
type TStateCreate = {
groupKey: TStateGroups;
- shouldTrackEvents?: boolean;
createStateCallback: TStateOperationsCallbacks["createState"];
handleClose: () => void;
};📝 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.
| type TStateCreate = { | |
| groupKey: TStateGroups; | |
| shouldTrackEvents: boolean; | |
| shouldTrackEvents?: boolean; | |
| createStateCallback: TStateOperationsCallbacks["createState"]; | |
| handleClose: () => void; | |
| }; | |
| type TStateCreate = { | |
| groupKey: TStateGroups; | |
| createStateCallback: TStateOperationsCallbacks["createState"]; | |
| handleClose: () => void; | |
| }; |
🤖 Prompt for AI Agents
In apps/web/core/components/project-states/create-update/create.tsx around lines
9 to 14, remove the unused shouldTrackEvents?: boolean property from the
TStateCreate type definition (it is never destructured or used in the component
— see line 17). Update the type to only include groupKey, createStateCallback,
and handleClose, and search the repo for any remaining references to
shouldTrackEvents to delete or refactor them as needed; run TypeScript checks to
ensure no lingering uses remain.
Description
Type of Change
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.