Performance improvements #183
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
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 WalkthroughAdds a tenant migration workflow and script, updates Node version, introduces a ClientRedirect component and replaces server redirects, expands webhook handling to provision/sync tenant and ops databases, simplifies database utilities, adjusts user/ops TRPC and utility signatures to use UserJSON, and makes targeted TRPC/query and layout import/refactor tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Clerk as Auth Provider
participant Webhook as Webhook Route
participant OpsDB as Ops Database
participant Mgmt as Management DB
participant Tenant as Tenant DB
Note over Clerk,Webhook: organization/user events (created/updated/invite accepted)
Clerk->>Webhook: POST webhook (event + payload)
alt organizationCreated
Webhook->>Mgmt: createTenantDatabase(ownerId) (ensure DB + migrate)
Webhook->>OpsDB: upsert organization
Webhook->>Tenant: insert org and creator (on conflict update)
else organizationUpdated
Webhook->>OpsDB: upsert organization
else userCreated
Webhook->>Mgmt: createTenantDatabase(userId) (ensure DB + migrate)
Webhook->>Tenant: upsert user
Webhook->>OpsDB: upsert opsUser
else userUpdated
Webhook->>Tenant: upsert user
Webhook->>OpsDB: upsert opsUser
else organizationInvitationAccepted
Webhook->>Tenant: upsert invited user in org
end
Webhook-->>Clerk: 200 OK
Note over Webhook: Logs successes/errors
sequenceDiagram
actor User
participant App as Server Component
participant Client as ClientRedirect
participant Browser as window.location
User->>App: Request page
App-->>User: HTML with <ClientRedirect path="/start" />
User->>Client: Mount component
Client->>Browser: set window.location.href = path
Browser-->>User: Navigate to target
sequenceDiagram
participant GH as GitHub Actions
participant Script as migrate-all-tenants.ts
participant OpsDB as Ops Database
participant Mgmt as Management DB
participant Tenant as Tenant DB
GH->>Script: Run migrate:tenants
Script->>OpsDB: Fetch tenant owner IDs
loop For each tenant
Script->>Mgmt: Check database exists
alt Exists
Script->>Tenant: Connect and run migrations
Script-->>GH: Record success/failed
else Missing
Script-->>GH: Record skipped
end
end
Script-->>GH: Summary + exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/(api)/api/webhook/auth/route.ts (1)
82-83: Stop logging full webhook payload (PII)Logging evt.data can leak emails and identifiers. Log only masked IDs and event type.
- console.log("Webhook payload:", id, evt.data); + console.log("[Webhook]", "type:", eventType, "id:", id?.slice(0,4) + "...");
♻️ Duplicate comments (1)
.github/workflows/migrate-tenants.yml (1)
2-3: Good: explicit minimal GITHUB_TOKEN permissionsThis addresses the previous “workflow does not contain permissions” warning.
🧹 Nitpick comments (13)
scripts/migrate-all-tenants.ts (3)
63-69: DB existence check: prefer rows.length over rowCountrowCount may be driver-specific. rows.length is more robust.
- if (checkDb.rowCount === 0) { + const rows = (checkDb as any).rows ?? []; + if (rows.length === 0) { return { success: true, skipped: true }; }
55-62: Reuse ops DB client and avoid mismatched schemaUse getOpsDatabase() here; passing tenant schema for the manage DB is inconsistent.
- const ownerDb = drizzle(`${process.env.DATABASE_URL}/manage${sslMode}`, { - schema, - }); + const ownerDb = await getOpsDatabase();
8-10: Avoid duplicate DB name logic; reuse shared helperThis logic diverges from lib/utils/useDatabase.getDatabaseName (validation + normalization). Reuse it to keep one source of truth.
If importing the helper is awkward here, at least mirror its validation (e.g., require org_/user_ prefix).
.github/workflows/migrate-tenants.yml (2)
23-73: Add concurrency to prevent overlapping migrationsAvoid running multiple tenant-migration jobs simultaneously on the same branch/environment.
Add at the top level:
concurrency: group: migrate-tenants-${{ github.ref }} cancel-in-progress: falseOptionally, per job:
concurrency: group: migrate-tenants-${{ github.job }}-${{ github.ref }} cancel-in-progress: false
36-39: Pin Bun version for reproducibilitylatest can drift. Pin a stable minor (update periodically).
- with: - bun-version: latest + with: + bun-version: 1.1.xapp/(api)/api/webhook/auth/route.ts (2)
43-75: Harden createTenantDatabase: client, existence check, and idempotency
- Prefer using getOpsDatabase() for the manage connection.
- Use rows.length for existence check.
- Handle concurrent CREATE DATABASE (code 42P04) gracefully.
async function createTenantDatabase(ownerId: string): Promise<void> { const databaseName = getDatabaseName(ownerId).match( (value) => value, () => { throw new Error("Database name not found"); }, ); const sslMode = process.env.DATABASE_SSL === "true" ? "?sslmode=require" : ""; - const ownerDb = drizzle(`${process.env.DATABASE_URL}/manage${sslMode}`, { - schema, - }); + const ownerDb = await getOpsDatabase(); - const checkDb = await ownerDb.execute( + const checkDb = await ownerDb.execute( sql`SELECT 1 FROM pg_database WHERE datname = ${databaseName}`, ); - if (checkDb.rowCount === 0) { - await ownerDb.execute(sql`CREATE DATABASE ${sql.identifier(databaseName)}`); + const exists = Array.isArray((checkDb as any).rows) && (checkDb as any).rows.length > 0; + if (!exists) { + try { + await ownerDb.execute(sql`CREATE DATABASE ${sql.identifier(databaseName)}`); + } catch (e: any) { + // 42P04: duplicate_database – another request may have created it + if (!(e && e.code === "42P04")) throw e; + } console.log(`Created database for tenant: ${databaseName}`); }
119-129: Make organizationCreated insert idempotentClerk webhooks may retry. Add onConflictDoNothing/Update to avoid duplicate key errors.
await db .insert(opsOrganization) .values({ id: orgData.id, name: orgData.name, rawData: orgData, lastActiveAt: new Date(), }) - .execute(); + .onConflictDoUpdate({ + target: opsOrganization.id, + set: { + name: orgData.name, + rawData: orgData, + lastActiveAt: new Date(), + }, + }) + .execute();lib/utils/useDatabase.ts (2)
9-14: Tighten ownerId validationPrevent unexpected names by enforcing allowed characters explicitly.
-export function getDatabaseName(ownerId: string): Result<string, string> { - if (!ownerId.startsWith("org_") && !ownerId.startsWith("user_")) { +export function getDatabaseName(ownerId: string): Result<string, string> { + if (!/^(org|user)_[A-Za-z0-9]+$/.test(ownerId)) { return err("Invalid owner ID"); } return ok(ownerId.toLowerCase().replace(/ /g, "_")); }
60-76: Same drizzle client concern for manage DB in deleteDatabaseUse a pg Pool here as well for consistency with node-postgres.
-const ownerDb = drizzle(`${process.env.DATABASE_URL}/manage${sslMode}`, { - schema, -}); +const ownerDb = drizzle(new Pool({ + connectionString: `${process.env.DATABASE_URL}/manage${sslMode}`, +}), { schema });trpc/routers/settings.ts (1)
38-38: Consider DB-side timestamps for consistency.Using
new Date()can introduce clock skew across services. Prefer DB-generated timestamps or column defaults forlastActiveAt.Example options:
- Use a DB default/trigger for
lastActiveAt.- Or set via SQL function (e.g.,
sqlnow()``) if supported in your dialects.Also applies to: 47-47
app/(dashboard)/[tenant]/layout.tsx (1)
18-19: Client-side redirect trade-off — confirm intent.Client redirect defers navigation until hydration, adding a visual hop. If you want instant navigation + better SEO, prefer server
redirect("/start"). If a full reload is desired, keep as-is.components/core/client-redirect.tsx (1)
1-13: Prefer router.replace, guard same-path, and drop console logging.Avoids full reload, avoids duplicate history entries, and reduces console noise.
Apply this diff:
"use client"; -import { useEffect } from "react"; +import { useEffect } from "react"; +import { useRouter } from "next/navigation"; import Loading from "@/app/loading"; export const ClientRedirect = ({ path }: { path: string }) => { - useEffect(() => { - console.log("Client redirect to", path); - window.location.href = path; - }, [path]); + const router = useRouter(); + useEffect(() => { + // Avoid redirect loops and extra history entries + if (typeof window !== "undefined" && window.location.pathname !== path) { + router.replace(path); + } + }, [path, router]); return <Loading />; };trpc/routers/user.ts (1)
76-82: Push permission filtering to SQL to reduce rows fetched.You currently fetch all projects for the statuses and filter in JS. Prefer limiting in the query (e.g., creator match OR an EXISTS on projectPermission for ctx.userId).
Example direction (Drizzle-style):
- Left join
projectPermissionon project.id and userId, and add awherewith(project.createdByUser = ctx.userId) OR (projectPermission.userId IS NOT NULL).Also applies to: 94-105
📜 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 (23)
.github/workflows/migrate-tenants.yml(1 hunks).nvmrc(1 hunks)CLAUDE.md(1 hunks)app/(api)/api/webhook/auth/route.ts(3 hunks)app/(dashboard)/[tenant]/layout.tsx(2 hunks)app/layout.tsx(1 hunks)app/loading.tsx(1 hunks)app/page.tsx(1 hunks)app/start/page.tsx(1 hunks)components/core/client-redirect.tsx(1 hunks)components/layout/navbar.tsx(1 hunks)hooks/use-keyboard-shortcut.ts(0 hunks)hooks/use-project-prefetch.ts(0 hunks)lib/utils/todayData.ts(0 hunks)lib/utils/useDatabase.ts(3 hunks)lib/utils/useUser.ts(1 hunks)ops/useOps.ts(3 hunks)package.json(3 hunks)scripts/migrate-all-tenants.ts(1 hunks)trpc/query-client.ts(1 hunks)trpc/routers/events.ts(2 hunks)trpc/routers/settings.ts(3 hunks)trpc/routers/user.ts(2 hunks)
💤 Files with no reviewable changes (3)
- hooks/use-project-prefetch.ts
- lib/utils/todayData.ts
- hooks/use-keyboard-shortcut.ts
🧰 Additional context used
🪛 ast-grep (0.39.6)
scripts/migrate-all-tenants.ts
[warning] 19-19: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(tenantId, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 20-20: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(databaseName, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (20)
trpc/query-client.ts (1)
1-6: LGTM! Import reordering with no functional impact.The relocation of the
toMsimport is a cosmetic change that does not affect the module's behavior or public API.components/layout/navbar.tsx (1)
5-11: LGTM! Performance improvement by removing unnecessary query invalidation.Removing the global
queryClient.invalidateQueries()on mount eliminates redundant refetches. The component still properly fetches data viauseQueries, but now avoids the performance overhead of invalidating all queries unnecessarily.app/loading.tsx (1)
1-3: LGTM! Import ordering cleanup.The import reordering has no functional impact and improves consistency.
app/layout.tsx (1)
1-2: LGTM! Import organization cleanup.Moving external library imports to the top follows common convention and has no functional impact.
package.json (2)
13-14: LGTM! New tenant migration script added.The new
migrate:tenantsscript aligns with the documentation in CLAUDE.md and supports the multi-tenant migration workflow.
65-112: Dependency updates look good - mostly patch versions.The dependency updates are conservative:
- Next.js: 15.5.5 (patch)
- React & React-DOM: 19.2.0 (patch)
- @types/react & @types/react-dom: 19.2.2 (patch)
- eslint-config-next: 15.5.5 (matches Next.js version)
These are low-risk patch updates that should be compatible with the existing codebase.
ops/useOps.ts (2)
35-48: LGTM! Field mappings correctly updated for UserJSON.The field mappings have been correctly updated to match the UserJSON type structure:
emailAddresses→email_addressesfirstName→first_namelastName→last_nameimageUrl→image_urlBoth the insert and onConflictDoUpdate clauses are consistent.
21-27: Verified: all calls toaddUserToOpsDbpass the requiredUserJSONargument.lib/utils/useUser.ts (3)
22-35: LGTM! Field mappings correctly updated for UserJSON.The field mappings are consistent with the UserJSON type structure and match the changes in
ops/useOps.ts. Both insert and update operations use the correct field names.
7-14: Verify callers andgetDatabaseForOwnerimplementation
- Calls in
app/(api)/api/webhook/auth/route.ts(lines 95, 107): ensureuserDatacannot be null/undefined before invokingaddUserToTenantDb.- Confirm
getDatabaseForOwneris correctly implemented and imported.
16-16: getDatabaseForOwner routes correctly
It validatesownerId, constructs the connection URL with SSL mode and Upstash cache, and returns a proper DrizzleDatabaseinstance.CLAUDE.md (1)
14-18: Documentation accurate; tenant ID masking implemented
Verified thatmaskIdandsanitizeErrorare used throughoutmigrate-all-tenants.tsto mask tenant IDs and sanitize errors, matching the security note in CLAUDE.md..nvmrc (1)
1-1: Define and pin Node.js version and verify compatibility
- Add an
engines.nodefield to package.json with a specificv22.xpin for reproducible builds- Audit dependencies (especially native modules and build tools) for compatibility with Node 22
lib/utils/useDatabase.ts (2)
35-45: Verify Upstash cache isolation across tenantsGlobal cache can risk cross-tenant data if keys don't include DB identity. Ensure key prefix/namespace includes databaseName or connection string.
If unsupported, consider disabling global cache for tenant DBs or injecting a per-tenant prefix.
33-45: Confirm drizzle client type and refactor to use pg.Pooldrizzle-orm/node-postgres’s drizzle factory doesn’t accept a raw DSN string—pass a
new Pool({ connectionString })instead of a plain${process.env.DATABASE_URL}/…connection string. Update all usages in:
• ops/useOps.ts
• scripts/migrate-all-tenants.ts
• lib/utils/useDatabase.ts
• app/(api)/api/webhook/auth/route.tsapp/page.tsx (1)
8-12: LGTM: client-side redirect for authenticated usersMatches the new redirect pattern; simple and clear.
trpc/routers/events.ts (1)
388-391: Formatting-only change — OK.Argument split over lines; behavior unchanged.
app/start/page.tsx (1)
10-12: LGTM.Server resolves auth, then delegates navigation to client-side redirect. No functional issues spotted.
app/(dashboard)/[tenant]/layout.tsx (1)
21-21: LGTM.Fetching notifications wire once before render is straightforward.
trpc/routers/user.ts (1)
85-93: Verify admin default role.Admins get
userRole: "editor"when no explicit permission exists. Is "editor" the intended UI role for admins, or should it be "admin"/owner?
Summary by CodeRabbit