Update account / org deletion logic #179
Conversation
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughExtends deletion workflows from 30 to 60 days for users and organizations, switches to a unified OrgDeletionNotice email, adds Clerk-based org/user lookups and org deletion trigger, enhances comments with threads and replies, refactors Today task item to a shared component, adds conditional PostHog provider, introduces TRPC output types, and updates config/deps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Cron as Scheduler
participant Route as jobs/.../mark-for-deletion (API Route)
participant DB as Database
participant Clerk as Clerk API
participant Mail as Email Sender
Note over Route: 60-day lifecycle
Cron->>Route: POST (run job)
Route->>DB: Step 1: Find inactive entities (>=60d)
DB-->>Route: Inactive users/orgs
Route->>Clerk: Fetch contact/org details
Route->>DB: Mark for deletion (timestamp)
alt 60-day notice window
Route->>Mail: Send 60-day deletion notice (OrgDeletionNotice)
end
alt 7 days before deletion (>=53d since mark)
Route->>Mail: Send 7-day warning
end
alt Deletion time (>=60d since mark)
Route->>Clerk: Delete organization (org jobs)
Note right of Clerk: Emits webhook for cleanup
Route->>DB: Finalize deletion status
end
sequenceDiagram
autonumber
participant User as User
participant UI as CommentsSection/CommentThread
participant Form as CommentForm
participant API as TRPC projects.getComments/...
participant Cache as QueryClient
User->>UI: Expand comment thread
UI->>API: Fetch replies (lazy, enabled on expand)
API-->>UI: Replies data (with replyCount)
User->>Form: Open reply form
Form->>API: Submit reply (roomId or comment-{parentId})
API-->>Form: Success
Form->>Cache: Invalidate parent room and thread
Cache-->>UI: Refetch updated comments
UI->>User: Updated thread shown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing touches🧪 Generate unit tests
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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
trpc/routers/projects.ts (1)
332-345: Missing authorization on deleteComment; any member could delete others’ comments.Enforce either project editor/admin or the comment’s author.
- .mutation(async ({ ctx, input }) => { - await ctx.db.delete(comment).where(eq(comment.id, input.id)); + .mutation(async ({ ctx, input }) => { + const canEdit = await canEditProject(ctx, input.projectId); + const c = await ctx.db.query.comment.findFirst({ + where: eq(comment.id, input.id), + columns: { createdByUser: true }, + }); + if (!canEdit && c?.createdByUser !== ctx.userId) { + throw new TRPCError({ code: "FORBIDDEN", message: "Comment delete access denied" }); + } + await ctx.db.delete(comment).where(eq(comment.id, input.id)); await logActivity({ action: "deleted", type: "comment", projectId: input.projectId, }); }),components/emails/org-deletion-notice.tsx (1)
72-76: Avoid hardcoding the app URL in email templates — use NEXT_PUBLIC_APP_URLHardcoded https://managee.xyz is present in both HTML buttons and plain‑text copies; replace with the env-driven base used elsewhere (process.env.NEXT_PUBLIC_APP_URL || "https://managee.xyz") and centralize the value.
- Update occurrences in email templates:
- components/emails/org-deletion-notice.tsx — Button href (line 72) and plain-text output (line 189).
- components/emails/daily-summary.tsx — Button href (line 148) and plain-text occurrence (line 452).
- components/emails/seven-day-warning.tsx — Button href (line 87) and plain-text occurrence (line 222).
- Use a shared helper or constant (e.g., lib/getAppUrl or export a single BASE_URL) to avoid repeating the fallback across templates; keep the fallback consistent with app/layout.tsx (process.env.NEXT_PUBLIC_APP_URL || "https://managee.xyz").
♻️ Duplicate comments (3)
app/(api)/api/jobs/user/mark-for-deletion/route.ts (3)
5-8: Same naming inconsistency as in the organization deletion routeThe
thirtyDayDeletionNoticePlainTextfunction name doesn't match the 60-day deletion timeline, creating the same confusion as in the organization deletion route.
70-70: Extract retention period to a shared configurationLike the organization deletion route, the 60-day period should be extracted to a constant, ideally shared between both deletion routes.
336-337: Same webhook cleanup concern as organization deletionThe user deletion has the same potential race condition with webhook-based cleanup as the organization deletion.
🧹 Nitpick comments (14)
components/core/posthog-provider.tsx (1)
23-29: Optional: avoid bundling posthog-js when no keyEven with the conditional render, the static import ships posthog-js. Dynamically import posthog-js only when the key exists to trim the bundle for analytics-disabled builds/environments.
Example sketch:
// keep PHProvider static, lazy-load client only const hasPH = Boolean(process.env.NEXT_PUBLIC_POSTHOG_KEY); export function PostHogProvider({ children }: { children: React.ReactNode }) { const [client, setClient] = useState<typeof posthog | null>(null); useEffect(() => { if (!hasPH || client) return; import("posthog-js").then(({ default: ph }) => { ph.init(process.env.NEXT_PUBLIC_POSTHOG_KEY!, { api_host: "/ingest", ui_host: "https://us.posthog.com", capture_pageview: false, capture_pageleave: true, debug: process.env.NODE_ENV === "development", }); setClient(ph); }); }, [client]); if (!hasPH || !client) return <>{children}</>; return ( <PHProvider client={client}> <SuspendedPostHogPageView /> {children} </PHProvider> ); }next.config.mjs (1)
4-4: Don’t disable typedRoutes unless you must; it removes useful type-safety.If there isn’t a concrete issue, consider dropping this flag to keep route typing checks.
Apply this diff if you agree:
- typedRoutes: false,package.json (1)
43-43: Remove unnecessary @sentry/node or confirm and restrict its usepackage.json contains both @sentry/nextjs (^10) and @sentry/node (^10.10.0), but the repo initializes Sentry via @sentry/nextjs (Sentry.init found in sentry.server.config.ts, sentry.edge.config.ts, instrumentation-client.ts; app/global-error.tsx and next.config.mjs integrate Sentry). No direct @sentry/node imports were found.
- Action: remove @sentry/node from package.json if you don't need Node-only APIs.
- If you must keep @sentry/node, restrict imports to node-only entrypoints and ensure you do not call Sentry.init twice (only initialize once per runtime).
trpc/routers/_app.ts (1)
1-1: Optional: also export RouterInputs.Handy for form/state typing symmetry.
-import type { inferRouterOutputs } from "@trpc/server"; +import type { inferRouterInputs, inferRouterOutputs } from "@trpc/server"; @@ export type AppRouter = typeof appRouter; export type RouterOutputs = inferRouterOutputs<AppRouter>; +export type RouterInputs = inferRouterInputs<AppRouter>;Also applies to: 22-22
trpc/routers/projects.ts (2)
2-2: Prep imports for batched reply-count query.You’ll need
inArrayandcountfor the refactor below.-import { and, desc, eq } from "drizzle-orm"; +import { and, desc, eq, inArray, count } from "drizzle-orm";
247-261: Index comment.roomId for comment/reply lookups.Frequent queries on roomId (and reply counts) benefit from an index.
components/emails/org-deletion-notice.tsx (1)
13-17: Rename “ThirtyDayDeletionNoticeProps” and plain-text helper to reflect 60 days.The 30-day naming is now misleading.
-interface ThirtyDayDeletionNoticeProps { +interface DeletionNoticeProps { @@ -export const OrgDeletionNotice = ({ +export const OrgDeletionNotice = ({ @@ -}: ThirtyDayDeletionNoticeProps) => { +}: DeletionNoticeProps) => { @@ -export function thirtyDayDeletionNoticePlainText({ +export function orgDeletionNoticePlainText({ @@ -}: ThirtyDayDeletionNoticeProps): string { +}: DeletionNoticeProps): string {If there are external imports, I can follow up with a PR-wide rename.
Also applies to: 19-23, 160-201
app/(api)/api/jobs/account/mark-for-deletion/route.ts (2)
69-69: Consider using a named constant for the retention periodThe 60-day retention period is hard-coded in multiple places. Consider extracting this to a configuration constant for easier maintenance.
Add a constant at the top of the file:
+const RETENTION_DAYS = 60; +const WARNING_DAYS_BEFORE_DELETION = 7; + export const { POST } = serve(async (context) => {Then update the date calculations:
- const sixtyDaysAgo = new Date(Date.now() - 1000 * 60 * 60 * 24 * 60); + const sixtyDaysAgo = new Date(Date.now() - 1000 * 60 * 60 * 24 * RETENTION_DAYS);And on line 178:
- const fiftyThreeDaysAgo = new Date(Date.now() - 1000 * 60 * 60 * 24 * 53); + const fiftyThreeDaysAgo = new Date(Date.now() - 1000 * 60 * 60 * 24 * (RETENTION_DAYS - WARNING_DAYS_BEFORE_DELETION));
326-327: Add error handling + record that deletion was triggered (don’t rely only on the webhook)Webhook handler exists at app/(api)/api/webhook/auth/route.ts, but the job at app/(api)/api/jobs/account/mark-for-deletion/route.ts (around lines ~324–327) calls clerk.organizations.deleteOrganization(org.id) with no try/catch or ops DB flag. Wrap the Clerk call in try/catch, update opsOrganization (e.g., set deletionTriggeredAt or a deletion status — add the column if missing), and add logging/alerting or retry behavior if the Clerk call or webhook fails.
Suggested change:
- const clerk = await clerkClient(); - await clerk.organizations.deleteOrganization(org.id); + try { + const clerk = await clerkClient(); + await clerk.organizations.deleteOrganization(org.id); + + // Mark deletion as triggered in the ops DB + await db + .update(opsOrganization) + .set({ deletionTriggeredAt: new Date() }) + .where(eq(opsOrganization.id, org.id)); + } catch (error) { + console.error( + `[OrgDeletion] Failed to trigger deletion for org ${org.name} (${org.id}):`, + error, + ); + // Consider alerting / enqueuing manual intervention or retrying + throw error; + }components/project/comment/comments-section.tsx (1)
27-36: Consider adding aria-label for accessibilityThe "Add comment" button could benefit from an aria-label for better screen reader support.
<Button variant="outline" size="sm" onClick={() => setShowCommentForm(true)} className="self-start text-muted-foreground hover:text-foreground" + aria-label="Add a new comment" >components/project/comment/comments.tsx (4)
68-71: Potential state inconsistency when toggling repliesWhen hiding replies, the component resets
shouldLoadRepliesto false. This could cause unnecessary re-fetching if the user toggles replies multiple times, as the query will be re-enabled each time.Consider keeping
shouldLoadRepliestrue once replies have been loaded initially:const toggleReplies = () => { if (!showReplies) { loadReplies(); } else { setShowReplies(false); - // Reset loading state so replies can be loaded again - setShouldLoadReplies(false); } };
129-141: Optimize query invalidation logic in reply success handlerThe component invalidates queries and manages state in the
onSuccesscallback. Consider consolidating the state updates to avoid potential race conditions.onSuccess={() => { setShowReplyForm(false); - onRefresh(); if (!shouldLoadReplies) { setShouldLoadReplies(true); setShowReplies(true); } queryClient.invalidateQueries({ queryKey: trpc.projects.getComments.queryKey({ roomId: `comment-${comment.id}`, }), }); + onRefresh(); }}
208-213: Redundant query invalidation after refetchThe code calls both
refetch()andinvalidateQueries()in the delete success handler. Sincerefetch()already fetches fresh data, the subsequent invalidation is redundant.onSuccess: () => { - // Force refetch of current comments - refetch(); // Also invalidate all comments queries queryClient.invalidateQueries({ queryKey: ["projects", "getComments"], }); },
50-55: Consider adding error handling for replies queryThe replies query doesn't have error handling. Consider adding error state management to handle failed reply fetches gracefully.
- const { data: replies = [], isLoading: repliesLoading } = useQuery({ + const { data: replies = [], isLoading: repliesLoading, error: repliesError } = useQuery({ ...trpc.projects.getComments.queryOptions({ roomId: `comment-${comment.id}`, }), enabled: shouldLoadReplies, });Then handle the error state in the UI:
{comment.replyCount > 0 && ( <Button variant="ghost" size="sm" onClick={toggleReplies} disabled={repliesLoading} className="h-6 px-2 text-xs text-muted-foreground hover:text-foreground" > + {repliesError + ? "Failed to load replies" + : repliesLoading - {repliesLoading ? "Loading..." : showReplies ? "Hide replies" : `${comment.replyCount} ${comment.replyCount === 1 ? "reply" : "replies"}`} </Button> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
README.md(1 hunks)app/(api)/api/jobs/account/mark-for-deletion/route.ts(7 hunks)app/(api)/api/jobs/user/mark-for-deletion/route.ts(7 hunks)app/(dashboard)/[tenant]/today/page.tsx(1 hunks)components/core/posthog-provider.tsx(2 hunks)components/emails/org-deletion-notice.tsx(4 hunks)components/project/comment/comment.tsx(3 hunks)components/project/comment/comments-section.tsx(2 hunks)components/project/comment/comments.tsx(4 hunks)components/today/task-item.tsx(1 hunks)next.config.mjs(1 hunks)package.json(1 hunks)trpc/routers/_app.ts(2 hunks)trpc/routers/projects.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-17T02:06:01.635Z
Learnt from: CR
PR: techulus/manage#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-17T02:06:01.635Z
Learning: Applies to drizzle/schema.ts : Define the main application database schema in drizzle/schema.ts
Applied to files:
trpc/routers/projects.ts
📚 Learning: 2025-08-17T02:06:01.635Z
Learnt from: CR
PR: techulus/manage#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-17T02:06:01.635Z
Learning: Applies to ops/drizzle/schema.ts : Define the operations database schema in ops/drizzle/schema.ts (separate database)
Applied to files:
trpc/routers/projects.ts
📚 Learning: 2025-08-17T02:06:01.635Z
Learnt from: CR
PR: techulus/manage#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-17T02:06:01.635Z
Learning: Applies to trpc/routers/*.ts : Place tRPC routers (user, settings, projects, tasks, events, search) under trpc/routers with one file per router
Applied to files:
trpc/routers/_app.ts
📚 Learning: 2025-08-17T02:06:01.635Z
Learnt from: CR
PR: techulus/manage#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-17T02:06:01.635Z
Learning: Applies to trpc/routers/search.ts : Expose search API endpoints exclusively via the tRPC search router in trpc/routers/search.ts
Applied to files:
trpc/routers/_app.ts
🔇 Additional comments (9)
components/core/posthog-provider.tsx (2)
3-3: LGTM: consolidated Next.js navigation importsImport placement is correct and avoids duplication.
10-12: Prevent double init under React 18 Strict Mode (dev)Add a module-scope idempotency flag so PostHog isn't initialized twice in React 18 Strict Mode; apply the patch below.
- if (!process.env.NEXT_PUBLIC_POSTHOG_KEY) { + if (!process.env.NEXT_PUBLIC_POSTHOG_KEY || phInitialized) { return; }And add:
// module scope (below imports) let phInitialized = false;And set after init:
phInitialized = true;Verification summary: package.json shows posthog-js = ^1.242.2. The search for a /ingest proxy/Next rewrites or middleware failed with "No such file or directory (os error 2)" — proxy/rewrites verification is inconclusive; confirm Next rewrites/middleware if you depend on /ingest.
README.md (1)
21-21: LGTM: Roadmap status updated.No issues spotted.
trpc/routers/_app.ts (1)
1-1: Nice: central RouterOutputs alias for consumer types.This improves DX across callers.
Also applies to: 22-22
components/project/comment/comment.tsx (1)
42-43: Reply threading UX/state invalidation looks good.submitRoomId derivation, dual invalidation, and reply/cancel controls are solid.
Also applies to: 56-67, 83-116
app/(api)/api/jobs/account/mark-for-deletion/route.ts (1)
20-56: Good error handling with detailed loggingThe
getOrgCreatorDetailsfunction properly handles errors and provides clear error messages at each failure point. The try-catch block with specific error messages helps with debugging and monitoring.app/(api)/api/jobs/user/mark-for-deletion/route.ts (2)
31-57: Conservative approach to organization membership checkGood defensive programming - if the organization membership check fails, the function conservatively assumes the user belongs to an organization and prevents deletion. This prevents accidental deletions due to API failures.
154-158: No change needed — OrgDeletionNotice already handles undefined organizationNameOrgDeletionNotice uses isOrganization = !!organizationName and falls back to account-specific preview/copy (the plain-text variant does the same), so passing undefined is safe.
Likely an incorrect or invalid review comment.
components/project/comment/comments-section.tsx (1)
15-43: Clean UX pattern for comment form togglingGood implementation of the show/hide pattern for the comment form. The callbacks properly reset the form visibility on both success and cancellation, providing a clean user experience.
Summary by CodeRabbit
New Features
Changes
Improvements
Refactor
Documentation
Chores