Fix session update after leaving organization #781
Conversation
📝 WalkthroughWalkthroughAdds organization-aware user denormalization and sync, role/authorization utilities and rules, new desktop organization creation and team-management flows (role changes, leave/remove), a desktop cache-clear tRPC router, global alert UI, API client, and multiple dependency updates (notably zod and Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Desktop as Desktop App
participant Form as CreateOrg Form
participant API as Backend API
participant DB as Database
participant AuthClient as Auth Client
User->>Desktop: Open /create-organization
Desktop->>Form: Render form
Form->>API: Debounced slug check
API->>DB: Query org by slug
DB-->>API: Slug status
API-->>Form: Available/Taken
User->>Form: Submit name+slug
Form->>API: Create organization
API->>DB: Insert organization
DB-->>API: New org ID
API-->>Form: Created
Form->>AuthClient: Set active organization
AuthClient-->>Form: Session updated
Form->>Desktop: Navigate to authenticated root
sequenceDiagram
actor User
participant UI as MemberActions UI
participant Dialog as Alerter/Dialog
participant TRPC as Backend TRPC
participant DB as Database
participant Session as Session Store
User->>UI: Click "Change role"
UI->>UI: Compute available roles (getAvailableRoleChanges)
User->>UI: Select new role
alt Self-demotion
UI->>Dialog: Show destructive confirm
User->>Dialog: Confirm
end
UI->>TRPC: updateMemberRole(memberId, role)
TRPC->>DB: Validate & update role (owner protection)
DB-->>TRPC: Updated member
TRPC->>Session: Trigger session refetch/update
TRPC-->>UI: Return success
UI->>User: Show toast / refresh list
sequenceDiagram
actor User
participant Dropdown as Org Dropdown UI
participant TRPC as Local TRPC
participant Electron as Electron Session
User->>Dropdown: Switch organization
Dropdown->>TRPC: Call cache.clearElectricCache()
TRPC->>Electron: Clear storage for API origin
Electron-->>TRPC: Success
TRPC-->>Dropdown: Success
Dropdown->>User: Organization switched
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
44-51: Replace PR build resolution with a released version of@electric-sql/client.The PR build
https://pkg.pr.new/@electric-sql/client@3724is no longer available (HTTP 404). This will cause installations to fail. The resolution is duplicated acrosspackage.json,apps/desktop/package.json, andapps/api/package.json. Pin a released version instead.
🤖 Fix all issues with AI agents
In `@apps/api/package.json`:
- Line 14: Replace the PR-hosted dependency entry "@electric-sql/client":
"https://pkg.pr.new/@electric-sql/client@3724" with the published npm registry
version (e.g. "@electric-sql/client": "1.0.10" or newer) in package.json and run
yarn/npm install to update lockfile; then run your test suite and any
integration flows that use the Electric client, and separately validate Zod
schemas against Zod v4.3.5 (upgrade Zod in package.json if needed) to check for
v3→v4 breaking changes and adjust schema definitions where failures occur.
In `@apps/api/src/app/api/electric/`[...path]/utils.ts:
- Around line 96-100: Replace the raw SQL fragment in the "auth.users" case (the
fragment variable and params array) with a Drizzle sql tagged-template so the
condition uses Drizzle's SQL builder instead of a plain string; specifically,
build the ANY(...) comparison with sql`...` referencing the "organization_ids"
column and the organizationId value (so you return { fragment: sql`...` } and
drop the separate params array), keeping the same variable names (fragment,
organizationId) and return shape as the other cases.
In `@apps/api/src/app/api/upload/route.ts`:
- Around line 1-92: The POST handler currently casts request.json() to
HandleUploadBody and JSON.parse(clientPayload/tokenPayload) without validation;
add Zod schemas to validate the incoming upload body (same shape as
HandleUploadBody) and the client token payload (object with required
organizationId string) before using them in handleUpload/onBeforeGenerateToken
and onUploadCompleted, and return NextResponse with status 400 on Zod validation
errors; update usages in POST, onBeforeGenerateToken (validate parsed
clientPayload and require organizationId) and onUploadCompleted (validate
tokenPayload and require organizationId) to use the validated objects instead of
raw JSON.
In `@apps/desktop/package.json`:
- Around line 39-40: The package.json lists an unavailable dependency version:
update the `@tanstack/electric-db-collection` entry (referenced alongside
"@electric-sql/client" and "@hookform/resolvers") to a published version (e.g.,
change 0.2.24 to the latest published v0.2.19) or confirm and pin the intended
release; also ensure the PR-hosted "@electric-sql/client@3724" reference is
validated in your CI/build pipeline or replaced with a verifiable package before
merging.
In `@packages/auth/src/server.ts`:
- Around line 60-64: The template uses hardcoded literals for team name and slug
generation; extract the magic number and string fragments into module-level
constants (e.g., ID_SLICE_LENGTH = 8, TEAM_NAME_SUFFIX = "'s Team", SLUG_SUFFIX
= "-team") and update the call to auth.api.createOrganization (the name:
`${user.name}${TEAM_NAME_SUFFIX}` and slug: `${user.id.slice(0,
ID_SLICE_LENGTH)}${SLUG_SUFFIX}`) so the magic values are centralized and
clearer.
In `@packages/trpc/src/router/organization/organization.ts`:
- Around line 202-211: The console logs in the organization leave flow are
emitting raw identifiers (ctx.session.user.id, input.organizationId,
ctx.session.session.activeOrganizationId); update the logging in the
organization.leave handler to avoid PII by either removing these console.log
calls or replacing them with debug-only logging (use the app's logger at debug
level) and redact identifiers (e.g., mask/hide all but last 4 chars or log a
derived non-reversible hash) when emitting org/user IDs; ensure references to
ctx.session.user.id, input.organizationId, and
ctx.session.session.activeOrganizationId are the ones modified.
In `@packages/trpc/src/trpc.ts`:
- Around line 9-10: The middleware for protectedProcedure is currently replacing
the full TRPCContext with an object containing only { session }, which drops
ctx.auth and ctx.headers; update the middleware (the protectedProcedure
middleware in packages/trpc/src/trpc.ts) to return the full context with
preserved fields (e.g., return { ...ctx, session } or explicitly include auth
and headers such as { session, auth: ctx.auth, headers: ctx.headers, ...otherCtx
} ) so downstream procedures keep ctx.auth and ctx.headers intact.
In `@packages/ui/src/atoms/Alert/Alert.tsx`:
- Around line 12-39: The Alerter component currently assigns the global
showAlertFn during render which can create stale closures and unsafe updates;
move the assignment into a useEffect inside the Alerter component so showAlertFn
= (options) => { setAlertOptions(options); setIsOpen(true); } is created once
and cleaned up on unmount (set showAlertFn back to null in the effect cleanup).
Ensure the effect does not capture stale state by only referencing the stable
setters setAlertOptions and setIsOpen, and keep the same behavior for opening
alerts and the cleanup to avoid updates after unmount.
♻️ Duplicate comments (6)
apps/admin/package.json (1)
40-40: Same Zod 4.3.5 upgrade — please verify compatibility once at the app level.
No new concerns beyond the workspace-wide Zod v4 verification already noted.apps/marketing/package.json (1)
35-35: Same Zod 4.3.5 upgrade — please verify compatibility once at the app level.
No new concerns beyond the workspace-wide Zod v4 verification already noted.apps/docs/package.json (1)
26-26: Same Zod 4.3.5 upgrade — please verify compatibility once at the app level.
No new concerns beyond the workspace-wide Zod v4 verification already noted.packages/local-db/package.json (1)
24-24: Same Zod 4.3.5 upgrade — please verify compatibility once at the package level.
No new concerns beyond the workspace-wide Zod v4 verification already noted.packages/db/package.json (1)
44-44: Same Zod upgrade verification as inpackages/trpc/package.json.apps/web/package.json (1)
42-42: Same Zod upgrade verification as inpackages/trpc/package.json.
🧹 Nitpick comments (11)
apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx (1)
21-23: Avoid a blank screen on collections loading / no active org.
Returningnullremoves all UI whilecollectionsis falsy. If this happens during session loading or when the active org is missing, users can see a blank screen. Consider a minimal loading/empty-state fallback or explicitly handling “no active org” vs “loading.” Also please confirm the redirect/guard flow covers the no‑org case so users don’t get stuck.Possible lightweight fallback (no extra imports)
- if (!collections) { - return null; - } + if (!collections) { + return ( + <div role="status" aria-busy="true" aria-live="polite" /> + ); + }packages/ui/src/atoms/Alert/Alert.tsx (1)
48-50: Align console prefixes with the[domain/operation]convention.The current logs use
[alert]only; the guideline expects[domain/operation].✅ Suggested prefix updates
- console.error("[alert] Confirmation failed:", error); + console.error("[alert/confirm] Confirmation failed:", error); @@ - console.error( - "[alert] Alerter not mounted. Make sure to render <Alerter /> in your app", - ); + console.error( + "[alert/init] Alerter not mounted. Make sure to render <Alerter /> in your app", + );As per coding guidelines, console logs should follow the
[domain/operation]prefix pattern.Also applies to: 93-96
packages/shared/src/auth/roles/index.ts (1)
1-1: Optional: avoid extra barrel layer if not needed.If minimizing barrel layers is a goal, consider re-exporting from the concrete
roles.tsmodule at the higher-level auth index instead of adding a dedicated roles barrel.As per coding guidelines, prefer concrete-file imports/exports where feasible.
packages/db/drizzle/0010_add_organization_ids_to_users.sql (1)
15-28: Trigger function handles edge cases correctly, but consider concurrency implications.The function properly uses
COALESCEto handle both INSERT/UPDATE (usingNEW) and DELETE (usingOLD) contexts, and correctly returns an empty array when no memberships remain.However, since this is a per-row AFTER trigger, concurrent membership changes for the same user could lead to race conditions where one update overwrites another's aggregation result. Consider whether this denormalized column needs stronger consistency guarantees (e.g., using
FOR UPDATElocks on the user row).packages/trpc/src/router/organization/organization.ts (1)
319-342: Centralize role permission checks into a lookup/helper.
The repeated role-specific conditionals are easy to drift from shared policy. A small permission map keeps rules centralized and easier to extend. As per coding guidelines, prefer lookup objects over repeated role conditionals.♻️ Proposed refactor
- if (actorRole === "admin" && targetRole === "owner") { - throw new TRPCError({ - code: "FORBIDDEN", - message: "Admins cannot modify owners", - }); - } - - if (actorRole === "admin" && input.role === "owner") { - throw new TRPCError({ - code: "FORBIDDEN", - message: "Admins cannot promote members to owner", - }); - } - - if (actorRole === "member") { - throw new TRPCError({ - code: "FORBIDDEN", - message: "Members cannot modify roles", - }); - } + const roleRules = { + owner: { + canModifyOwners: true, + canPromoteToOwner: true, + canModifyRoles: true, + }, + admin: { + canModifyOwners: false, + canPromoteToOwner: false, + canModifyRoles: true, + }, + member: { + canModifyOwners: false, + canPromoteToOwner: false, + canModifyRoles: false, + }, + } as const; + + const actorRules = roleRules[actorRole]; + + if (!actorRules.canModifyRoles) { + throw new TRPCError({ + code: "FORBIDDEN", + message: "Members cannot modify roles", + }); + } + + if (!actorRules.canModifyOwners && targetRole === "owner") { + throw new TRPCError({ + code: "FORBIDDEN", + message: "Admins cannot modify owners", + }); + } + + if (!actorRules.canPromoteToOwner && input.role === "owner") { + throw new TRPCError({ + code: "FORBIDDEN", + message: "Admins cannot promote members to owner", + }); + }apps/desktop/src/renderer/screens/main/components/TopBar/OrganizationDropdown.tsx (1)
57-67: Consider adding a guard to avoid unnecessary reload when switching to the same organization.The function doesn't check if
newOrgIdequals the currentactiveOrganizationIdbefore triggeringsetActive(), cache clear, and reload. This could cause an unnecessary full page reload if users accidentally click on the already-active organization.🔧 Suggested fix
const switchOrganization = async (newOrgId: string) => { + if (newOrgId === activeOrganizationId) { + return; + } + await authClient.organization.setActive({ organizationId: newOrgId, });apps/desktop/src/renderer/routes/_authenticated/settings/team/page.tsx (1)
183-188: Redundant type cast.
member.roleis already typed asOrganizationRolefrom the mapping on line 61, making this cast unnecessary.🔧 Suggested fix
canRemove={canRemoveMember( currentUserRole, - member.role as OrganizationRole, + member.role, isCurrentUserRow, ownerCount, )}apps/desktop/src/renderer/routes/create-organization/page.tsx (3)
43-43: Consider stronger typing for file input.Using
z.any()bypasses type safety. While this avoids bundle overhead from zod file validators, consider usingz.instanceof(File).optional()or a custom type for better type inference.🔧 Suggested improvement
- logoFile: z.any().optional(), + logoFile: z.custom<File>((val) => val === undefined || val instanceof File).optional(),
70-79: Potential infinite loop risk withformin dependency array.While
react-hook-formmemoizes the form object, includingformin dependencies alongside callingform.setValuecould cause re-renders in edge cases. Consider extractinggetValuesandsetValueto stable references.🔧 Suggested improvement
+ const { getValues, setValue } = form; + // Auto-generate slug from organization name const nameValue = form.watch("name"); useEffect(() => { const slug = nameValue .toLowerCase() .replace(/[^a-z0-9]+/g, "-") .replace(/^-+|-+$/g, ""); - if (slug && slug !== form.getValues("slug")) { - form.setValue("slug", slug, { shouldValidate: false }); + if (slug && slug !== getValues("slug")) { + setValue("slug", slug, { shouldValidate: false }); } - }, [nameValue, form]); + }, [nameValue, getValues, setValue]);
195-195: Hard reload after organization creation.Using
window.location.reload()is a brute-force approach that works but loses any in-memory state. Given the PR objective mentions switching toorganization.setActive()for fresh tokens, consider whether navigation alone (e.g.,navigate({ to: "/" })) would suffice here, similar to the pattern inMemberActions.tsx.apps/desktop/src/renderer/routes/_authenticated/settings/team/components/MemberActions/MemberActions.tsx (1)
154-156: Missingawaiton async function call.
handleChangeRoleis an async function but called withoutawait. While this doesn't break functionality (the loading state is managed internally), it means any error thrown won't propagate to the caller. Consider addingawaitfor consistency with the destructive alert path.🔧 Suggested fix
} else { - handleChangeRole(newRole); + void handleChangeRole(newRole); }Using
voidmakes the intentional fire-and-forget explicit, or addawaitand makehandleRoleSelectionasync.
| }, | ||
| "dependencies": { | ||
| "@electric-sql/client": "1.4.0", | ||
| "@electric-sql/client": "https://pkg.pr.new/@electric-sql/client@3724", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the full package.json context around the specified lines
cat -n apps/api/package.json | head -50Repository: superset-sh/superset
Length of output: 184
🏁 Script executed:
# List the directory structure to understand the project layout
git ls-files apps/api/ | head -30Repository: superset-sh/superset
Length of output: 184
🏁 Script executed:
# Search for zod usage in the codebase to understand compatibility concerns
rg "from ['\"]zod['\"]|import.*zod" --type ts --type tsx --type js -A 2Repository: superset-sh/superset
Length of output: 184
🏁 Script executed:
# Check if there are any version constraints or compatibility notes
rg "@electric-sql" --type ts --type tsx --type js -A 1 -B 1Repository: superset-sh/superset
Length of output: 184
🌐 Web query:
pkg.pr.new package registry ephemeral builds dependency
💡 Result:
Brief summary — what pkg.pr.new does and how its ephemeral preview packages & dependencies work:
-
What it is: a GitHub App + CLI that publishes per-commit / per-PR preview releases to a temporary, npm‑compatible registry so you can install unreleased builds without publishing to npm. [1][2]
-
How to use it: run the CLI (npx pkg-pr-new publish) from a GitHub Actions workflow (supports monorepos/workspaces and templates for StackBlitz). The CLI produces installable URLs like npm i https://pkg.pr.new/@ or shorter compact forms when configured. [2][4]
-
Ephemeral storage / retention: preview packages are stored in Cloudflare R2; pkg.pr.new is intentionally temporary — unused packages are garbage‑collected (examples: cleanup rules removing packages not downloaded for ~1 month and older-than‑6‑months removals are used in implementations). This means previews are not permanent and may be removed after inactivity/time. [3]
-
Dependency handling: pkg.pr.new publishes packed artifacts (npm pack style). It has flags to influence packing and dependency behavior (e.g., --pnpm / --yarn, and a --peerDeps option to control how peerDependencies are represented). Installs from the preview registry are npm‑compatible so package managers resolve them the same way as normal tarball URLs. [2][4]
Sources:
- Announcement / blog (overview, usage examples). [1]
- pkg.pr.new GitHub repo (setup, CLI flags, URL formats, templates, workflow usage). [2]
- Implementation notes / R2 storage & cleanup details (ephemeral retention). [3]
- CLI/source code showing pack/peerDeps behavior. [4]
🌐 Web query:
zod version 4.3.5 npm registry stability
💡 Result:
Short answer: Zod 4.3.5 is part of Zod 4 (declared stable by the author) and has no known direct security vulnerabilities in public vulnerability databases; however there are recent build/packaging issues reported (module-federation warning / package.json metadata) you should test for your use-case. Sources:
- Zod 4 is considered stable by the project. [1]
- No known direct vulnerabilities for [email protected] (Snyk security scan). [2]
- [email protected] metadata/packaging and related build issues have active GitHub reports (module federation / package.json version in dist). [3]
- Registry/package listing showing recent 4.x releases. [4]
References:
[1] https://zod.dev/v4/versioning
[2] https://security.snyk.io/package/npm/zod%404.3.5
[3] colinhacks/zod#4656
[4] https://socket.dev/npm/package/zod
🌐 Web query:
@electric-sql/client npm package versions
💡 Result:
Latest published version: 1.0.10 (see npm). [1]
To list all published versions locally run:
- npm view
@electric-sql/clientversions --json
Sources:
- npm package page for
@electric-sql/client. [1] - unpkg package files (package README and archive). [2]
Replace PR-hosted @electric-sql/client with published registry version; validate Zod v4 compatibility.
The pkg.pr.new registry publishes ephemeral preview packages with automatic garbage collection (removed after ~1 month of inactivity or 6+ months old), making it unsuitable for stable dependencies. Switch to the latest published version (@electric-sql/client@1.0.10 or newer from npm registry). Additionally, test Zod v4.3.5 behavior against your schemas to ensure compatibility with any v3→v4 breaking changes.
🤖 Prompt for AI Agents
In `@apps/api/package.json` at line 14, Replace the PR-hosted dependency entry
"@electric-sql/client": "https://pkg.pr.new/@electric-sql/client@3724" with the
published npm registry version (e.g. "@electric-sql/client": "1.0.10" or newer)
in package.json and run yarn/npm install to update lockfile; then run your test
suite and any integration flows that use the Electric client, and separately
validate Zod schemas against Zod v4.3.5 (upgrade Zod in package.json if needed)
to check for v3→v4 breaking changes and adjust schema definitions where failures
occur.
| case "auth.users": { | ||
| const orgMembers = await db.query.members.findMany({ | ||
| where: eq(members.organizationId, organizationId), | ||
| columns: { userId: true }, | ||
| }); | ||
| if (orgMembers.length === 0) { | ||
| return { fragment: "1 = 0", params: [] }; | ||
| } | ||
| const userIds = [...new Set(orgMembers.map((m) => m.userId))]; | ||
| const whereExpr = inArray(sql`${sql.identifier(users.id.name)}`, userIds); | ||
| const qb = new QueryBuilder(); | ||
| const { sql: query, params } = qb | ||
| .select() | ||
| .from(users) | ||
| .where(whereExpr) | ||
| .toSQL(); | ||
| const fragment = query.replace(/^select .* from .* where\s+/i, ""); | ||
| return { fragment, params }; | ||
| // Use the denormalized organization_ids array column | ||
| // Check if the requested organization_id is in the user's array | ||
| const fragment = `$1 = ANY("organization_ids")`; | ||
| return { fragment, params: [organizationId] }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "buildWhereClause" apps packages --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
rg -n "buildWhereClause" apps packages -t tsRepository: superset-sh/superset
Length of output: 374
🏁 Script executed:
cat -n apps/api/src/app/api/electric/[...path]/utils.ts | head -120Repository: superset-sh/superset
Length of output: 3560
🏁 Script executed:
cat -n apps/api/src/app/api/electric/[...path]/route.tsRepository: superset-sh/superset
Length of output: 2267
🏁 Script executed:
rg -n "ANY\|sql\." apps/api/src -A2 -B2 | head -40Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
rg -n "sql\`" apps/api/src/app/api/electric/[...path]/utils.tsRepository: superset-sh/superset
Length of output: 184
🏁 Script executed:
# Check what Drizzle imports and operations are available
rg -n "from \"drizzle-orm" apps/api/src/app/api/electric/[...path]/utils.tsRepository: superset-sh/superset
Length of output: 233
🏁 Script executed:
# Check if there are any examples of array operators in the codebase
rg -n "ANY\|array\|inArray" apps/api/src -t ts | head -20Repository: superset-sh/superset
Length of output: 46
Use Drizzle ORM for the array operator instead of raw SQL fragment.
The auth.users case violates the coding guideline requiring Drizzle ORM for all database operations. All other cases in this function use Drizzle's QueryBuilder, but this case uses a hard-coded raw SQL fragment. Use Drizzle's sql function to construct the array operator consistently with the rest of the function, for example: sqlANY(ARRAY[${organizationId}])`` or similar, depending on the exact column type and comparison needed.
🤖 Prompt for AI Agents
In `@apps/api/src/app/api/electric/`[...path]/utils.ts around lines 96 - 100,
Replace the raw SQL fragment in the "auth.users" case (the fragment variable and
params array) with a Drizzle sql tagged-template so the condition uses Drizzle's
SQL builder instead of a plain string; specifically, build the ANY(...)
comparison with sql`...` referencing the "organization_ids" column and the
organizationId value (so you return { fragment: sql`...` } and drop the separate
params array), keeping the same variable names (fragment, organizationId) and
return shape as the other cases.
apps/api/src/app/api/upload/route.ts
Outdated
| import { auth } from "@superset/auth/server"; | ||
| import { db } from "@superset/db/client"; | ||
| import { members, organizations } from "@superset/db/schema"; | ||
| import { type HandleUploadBody, handleUpload } from "@vercel/blob/client"; | ||
| import { and, eq } from "drizzle-orm"; | ||
| import { NextResponse } from "next/server"; | ||
|
|
||
| export async function POST(request: Request) { | ||
| try { | ||
| const body = (await request.json()) as HandleUploadBody; | ||
|
|
||
| const jsonResponse = await handleUpload({ | ||
| body, | ||
| request, | ||
| onBeforeGenerateToken: async (_pathname, clientPayload) => { | ||
| // Verify user is authenticated | ||
| const session = await auth.api.getSession({ headers: request.headers }); | ||
| if (!session?.user) { | ||
| throw new Error("Unauthorized"); | ||
| } | ||
|
|
||
| // Parse and validate client payload | ||
| const payload = JSON.parse(clientPayload || "{}"); | ||
| const organizationId = payload.organizationId; | ||
| if (!organizationId) { | ||
| throw new Error("Organization ID required"); | ||
| } | ||
|
|
||
| // Verify user is a member of the organization | ||
| const membership = await db.query.members.findFirst({ | ||
| where: and( | ||
| eq(members.organizationId, organizationId), | ||
| eq(members.userId, session.user.id), | ||
| ), | ||
| }); | ||
|
|
||
| if (!membership) { | ||
| throw new Error("Not a member of this organization"); | ||
| } | ||
|
|
||
| // Return allowed content types and token payload | ||
| return { | ||
| allowedContentTypes: [ | ||
| "image/jpeg", | ||
| "image/jpg", | ||
| "image/png", | ||
| "image/webp", | ||
| ], | ||
| tokenPayload: JSON.stringify({ | ||
| organizationId, | ||
| userId: session.user.id, | ||
| }), | ||
| }; | ||
| }, | ||
| onUploadCompleted: async ({ blob, tokenPayload }) => { | ||
| // Update organization logo in database | ||
| if (!tokenPayload) { | ||
| console.error("[upload] Missing token payload"); | ||
| return; | ||
| } | ||
|
|
||
| const payload = JSON.parse(tokenPayload) as { | ||
| organizationId?: string; | ||
| userId?: string; | ||
| }; | ||
|
|
||
| if (!payload.organizationId) { | ||
| console.error("[upload] Missing organizationId in token payload"); | ||
| return; | ||
| } | ||
|
|
||
| await db | ||
| .update(organizations) | ||
| .set({ logo: blob.url }) | ||
| .where(eq(organizations.id, payload.organizationId)); | ||
|
|
||
| console.log("[upload] Organization logo updated:", { | ||
| organizationId: payload.organizationId, | ||
| url: blob.url, | ||
| }); | ||
| }, | ||
| }); | ||
|
|
||
| return NextResponse.json(jsonResponse); | ||
| } catch (error) { | ||
| console.error("[upload] Upload failed:", error); | ||
| return NextResponse.json( | ||
| { | ||
| error: error instanceof Error ? error.message : "Upload failed", | ||
| }, | ||
| { status: 500 }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists
find . -name "route.ts" -path "*/api/upload/*" | head -20Repository: superset-sh/superset
Length of output: 184
🏁 Script executed:
# Look for existing Zod usage patterns in apps/api
fd -e ts -e tsx -path "*/apps/api/*" -exec grep -l "import.*from.*zod" {} \; | head -10Repository: superset-sh/superset
Length of output: 184
🏁 Script executed:
# Check the schema definition to understand organizationId type
fd -e ts -path "*/db/schema*" -type f | head -5Repository: superset-sh/superset
Length of output: 184
Validate upload body and client payload with Zod before use.
The request body is only cast and clientPayload is parsed without schema; malformed inputs will surface as 500s and bypass boundary validation. Add Zod parsing with proper 400 error handling for the body and organizationId as required by the coding guideline: validate API route bodies with Zod schemas at boundaries.
🛠️ Proposed fix
import { auth } from "@superset/auth/server";
import { db } from "@superset/db/client";
import { members, organizations } from "@superset/db/schema";
import { type HandleUploadBody, handleUpload } from "@vercel/blob/client";
import { and, eq } from "drizzle-orm";
import { NextResponse } from "next/server";
+import { z, ZodError } from "zod";
+
+const handleUploadBodySchema = z
+ .object({
+ clientPayload: z.string().optional(),
+ })
+ .passthrough();
+const clientPayloadSchema = z.object({
+ organizationId: z.string().uuid(),
+});
export async function POST(request: Request) {
try {
- const body = (await request.json()) as HandleUploadBody;
+ const bodyJson = await request.json();
+ const body = handleUploadBodySchema.parse(bodyJson) as HandleUploadBody;
const jsonResponse = await handleUpload({
body,
request,
onBeforeGenerateToken: async (_pathname, clientPayload) => {
// Verify user is authenticated
const session = await auth.api.getSession({ headers: request.headers });
if (!session?.user) {
throw new Error("Unauthorized");
}
// Parse and validate client payload
- const payload = JSON.parse(clientPayload || "{}");
- const organizationId = payload.organizationId;
- if (!organizationId) {
- throw new Error("Organization ID required");
- }
+ const payload = clientPayloadSchema.parse(
+ JSON.parse(clientPayload || "{}"),
+ );
+ const organizationId = payload.organizationId;
// Verify user is a member of the organization
const membership = await db.query.members.findFirst({
where: and(
eq(members.organizationId, organizationId),
eq(members.userId, session.user.id),
),
});
if (!membership) {
throw new Error("Not a member of this organization");
}
// Return allowed content types and token payload
return {
allowedContentTypes: [
"image/jpeg",
"image/jpg",
"image/png",
"image/webp",
],
tokenPayload: JSON.stringify({
organizationId,
userId: session.user.id,
}),
};
},
onUploadCompleted: async ({ blob, tokenPayload }) => {
// Update organization logo in database
if (!tokenPayload) {
console.error("[upload] Missing token payload");
return;
}
const payload = JSON.parse(tokenPayload) as {
organizationId?: string;
userId?: string;
};
if (!payload.organizationId) {
console.error("[upload] Missing organizationId in token payload");
return;
}
await db
.update(organizations)
.set({ logo: blob.url })
.where(eq(organizations.id, payload.organizationId));
console.log("[upload] Organization logo updated:", {
organizationId: payload.organizationId,
url: blob.url,
});
},
});
return NextResponse.json(jsonResponse);
- } catch (error) {
+ } catch (error) {
+ if (error instanceof ZodError) {
+ return NextResponse.json(
+ { error: "Invalid upload payload" },
+ { status: 400 },
+ );
+ }
console.error("[upload] Upload failed:", error);
return NextResponse.json(
{
error: error instanceof Error ? error.message : "Upload failed",
},
{ status: 500 },
);🤖 Prompt for AI Agents
In `@apps/api/src/app/api/upload/route.ts` around lines 1 - 92, The POST handler
currently casts request.json() to HandleUploadBody and
JSON.parse(clientPayload/tokenPayload) without validation; add Zod schemas to
validate the incoming upload body (same shape as HandleUploadBody) and the
client token payload (object with required organizationId string) before using
them in handleUpload/onBeforeGenerateToken and onUploadCompleted, and return
NextResponse with status 400 on Zod validation errors; update usages in POST,
onBeforeGenerateToken (validate parsed clientPayload and require organizationId)
and onUploadCompleted (validate tokenPayload and require organizationId) to use
the validated objects instead of raw JSON.
| "@electric-sql/client": "https://pkg.pr.new/@electric-sql/client@3724", | ||
| "@hookform/resolvers": "^5.2.2", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Checking PR-hosted `@electric-sql/client` package ==="
if curl -Ifs https://pkg.pr.new/@electric-sql/client@3724 > /dev/null 2>&1; then
echo "✓ `@electric-sql/client`@3724 is accessible"
else
echo "✗ `@electric-sql/client`@3724 is NOT accessible"
fi
echo ""
echo "=== Checking npm package availability ==="
# Check each package version
packages=(
"@hookform/[email protected]"
"[email protected]"
"@tanstack/[email protected]"
"@tanstack/[email protected]"
"@vercel/[email protected]"
"[email protected]"
)
if command -v npm >/dev/null 2>&1; then
for pkg in "${packages[@]}"; do
version=$(npm view "$pkg" version 2>/dev/null || echo "NOT FOUND")
echo "$pkg: $version"
done
else
echo "npm not available; checking via curl to registry"
for pkg in "${packages[@]}"; do
pkg_name=$(echo "$pkg" | cut -d'@' -f1-$(echo "$pkg" | grep -o '@' | wc -l))
pkg_version=$(echo "$pkg" | rev | cut -d'@' -f1 | rev)
result=$(curl -s "https://registry.npmjs.org/${pkg_name}/${pkg_version}" | grep -q '"version"' && echo "EXISTS" || echo "NOT FOUND")
echo "$pkg: $result"
done
fiRepository: superset-sh/superset
Length of output: 184
🌐 Web query:
@hookform/resolvers npm registry version 5.2.2
💡 Result:
Yes — @hookform/resolvers version 5.2.2 is published. Install with:
npm install @hookform/resolvers@5.2.2
Sources: npm registry / package data. [1][2]
References
- npm package listing (jsDelivr/npm): version 5.2.2. [1]
- Package overview / registry snapshot (Socket.dev). [2]
🌐 Web query:
react-hook-form npm registry version 7.71.1
💡 Result:
react-hook-form version 7.71.1 is published on the npm registry. Install with:
npm install [email protected]
You can verify the published version via the package registry listings. [1][2]
Sources:
- Snyk package page for react-hook-form (shows latest 7.71.1). [1]
- Socket / package listing for react-hook-form (version activity). [2]
🌐 Web query:
@vercel/blob npm registry version 2.0.0
💡 Result:
Yes — @vercel/blob@2.0.0 is published on the npm registry. See the package.json for v2.0.0 and CDN listings. [1][2][3]
You can install it with:
npm install @vercel/blob@2.0.0
Sources:
- package.json for
@vercel/blob@2.0.0 (unpkg). [1] - jsDelivr package page showing v2.0.0. [2]
- Snyk package page listing latest version 2.0.0. [3]
🌐 Web query:
zod npm registry version 4.3.5
💡 Result:
zod version 4.3.5 is published to the npm registry (published Jan 4, 2026). Install with:
npm install [email protected]
Sources: [1][2][3]
References
[1] Socket package page showing version 4.3.5. (socket.dev)
[2] Yarn package page (lists 4.3.5, Jan 4, 2026). (classic.yarnpkg.com)
[3] Snyk package page for zod (latest 4.3.5). (security.snyk.io)
Citations:
- 1: https://socket.dev/npm/package/zod?utm_source=openai
- 2: https://classic.yarnpkg.com/en/package/zod?utm_source=openai
- 3: https://security.snyk.io/package/npm/zod%404.3.5?utm_source=openai
🌐 Web query:
@tanstack/react-db npm registry
💡 Result:
Yes — @tanstack/react-db is published on the npm registry.
- Install: npm i
@tanstack/react-db. [1][2] - Repository: github.com/TanStack/db. [3]
- Docs (React adapter + usage): tanstack.com/db (React docs). [2]
- npm package page (metadata, version, downloads, license):
@tanstack/react-dbon npm. [1]
Sources:
[1] npm package page for @tanstack/react-db.
[2] TanStack DB docs — React adapter / installation.
[3] TanStack DB GitHub repository.
🌐 Web query:
@electric-sql/client PR-hosted package pkg.pr.new 3724
💡 Result:
I’m missing context — do you want the pkg.pr.new preview URL for PR #3724 of the @electric-sql/client repo, or information about a specific preview package build (logs, artifacts, or install command)? Which repo/PR link should I look up?
🌐 Web query:
@tanstack/react-db version 0.1.64 npm
💡 Result:
Yes — @tanstack/react-db v0.1.64 has been released (listed in release tracking). [1][2]
Sources:
- Release listing (newreleases.io) showing
@tanstack/react-db@0.1.64. [1] - jsDelivr package index (shows nearby versions for
@tanstack/react-db). [2]
🌐 Web query:
@tanstack/electric-db-collection version 0.2.24 npm
💡 Result:
I couldn't find an npm release for @tanstack/electric-db-collection at version 0.2.24. The npm package page shows v0.1.12 (recently published). A package index (socket.dev) lists up to v0.2.19 as the latest. [1][2]
Sources:
- npm package page for
@tanstack/electric-db-collection(shows v0.1.12). [1] - Socket.dev package index (lists v0.2.19 as latest). [2]
🌐 Web query:
electric-sql/client pkg.pr.new preview package
💡 Result:
I’m not sure what you mean by “pkg.pr.new.” Do you want:
- information about the
@electric-sql/clientpreview package on npm (latest preview/experimental releases), or - instructions for installing a preview/prerelease package for Electric (e.g., an npm tag like next/canary/experimental), or
- something else (link, changelog, or how to test a preview)?
Tell me which and I’ll fetch the exact package name, version, and install command.
@tanstack/electric-db-collection version mismatch: 0.2.24 does not exist.
Most dependencies verify successfully (@hookform/resolvers@5.2.2, [email protected], @vercel/blob@2.0.0, [email protected], @tanstack/react-db@0.1.64 are all published), but @tanstack/electric-db-collection@0.2.24 is not available in the npm registry—the latest release is v0.2.19. Update the version in package.json or confirm the intended version. The PR-hosted @electric-sql/client@3724 package cannot be verified through public sources and should be tested in your build/deployment pipeline.
🤖 Prompt for AI Agents
In `@apps/desktop/package.json` around lines 39 - 40, The package.json lists an
unavailable dependency version: update the `@tanstack/electric-db-collection`
entry (referenced alongside "@electric-sql/client" and "@hookform/resolvers") to
a published version (e.g., change 0.2.24 to the latest published v0.2.19) or
confirm and pin the intended release; also ensure the PR-hosted
"@electric-sql/client@3724" reference is validated in your CI/build pipeline or
replaced with a verifiable package before merging.
| const org = await auth.api.createOrganization({ | ||
| body: { | ||
| name: `${user.name}'s Org`, | ||
| slug: `${user.id.slice(0, 8)}-org`, | ||
| name: `${user.name}'s Team`, | ||
| slug: `${user.id.slice(0, 8)}-team`, | ||
| userId: user.id, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the specific file and lines mentioned
head -n 70 packages/auth/src/server.ts | tail -n 15Repository: superset-sh/superset
Length of output: 514
🏁 Script executed:
# Check the full context around lines 60-64
sed -n '50,75p' packages/auth/src/server.tsRepository: superset-sh/superset
Length of output: 791
🏁 Script executed:
# Find the user type definition to check if name can be null
fd -t f "server.ts" packages/auth/src/Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Find the user type definition - check packages/auth for schema/types
fd -t f "\.ts$" packages/auth/src/ | head -20Repository: superset-sh/superset
Length of output: 144
🏁 Script executed:
# Search for where user type is defined or imported
rg -n "user" packages/auth/src/server.ts | head -20Repository: superset-sh/superset
Length of output: 578
🏁 Script executed:
# Look for the auth client/schema definition
rg -n "databaseHooks\|createOrganization" packages/auth/src/server.ts -B5 -A10Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's a schema definition for users
fd -t f "schema\|types\|user" packages/db/ | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the auth initialization and find better-auth setup
rg -n "createAuthClient\|new Auth\|auth =" packages/auth/src/server.ts -B3 -A3Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look for package.json to understand which auth library is being used
cat packages/auth/package.json | grep -A5 -B5 '"dependencies"'Repository: superset-sh/superset
Length of output: 388
🏁 Script executed:
# Check for better-auth version and user type docs
rg -n "better-auth\|User\|user" packages/auth/src/server.ts | head -30Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look at the auth setup to understand the user structure
sed -n '1,60p' packages/auth/src/server.tsRepository: superset-sh/superset
Length of output: 1665
🏁 Script executed:
# Search for auth initialization and plugins
rg -n "const auth\|export const auth\|new Auth\|betterAuth" packages/auth/src/server.ts -B2 -A5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the full server.ts to understand the complete context
wc -l packages/auth/src/server.tsRepository: superset-sh/superset
Length of output: 95
🏁 Script executed:
# Look at the rest of server.ts to understand the complete picture
tail -n 50 packages/auth/src/server.tsRepository: superset-sh/superset
Length of output: 1303
🏁 Script executed:
# Check if there are any type definitions or interfaces related to user in the codebase
rg -n "interface.*User\|type.*User\|User.*=" packages/auth/src/ -A3Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search the db schema for user table definition to understand if name is nullable
rg -n "name" packages/db/schema/ -g '*auth*' -C2Repository: superset-sh/superset
Length of output: 123
🏁 Script executed:
# Find db schema files
fd -t f "schema" packages/db/Repository: superset-sh/superset
Length of output: 96
🏁 Script executed:
# Search for user/auth schema definition
fd -t f "\.ts$" packages/db/ | head -20Repository: superset-sh/superset
Length of output: 456
🏁 Script executed:
# Look for any auth schema files
rg -l "users\|user" packages/db/ -t ts | head -10Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the auth schema definition
cat packages/db/src/schema/auth.tsRepository: superset-sh/superset
Length of output: 4620
Extract hardcoded values to named constants.
The name field is guaranteed non-null per the schema, so no fallback is needed. However, extract the magic number 8 and hardcoded strings ("'s Team", "-team") to module-level constants for maintainability and clarity, as per coding guidelines.
🔧 Suggested adjustment
+const DEFAULT_TEAM_SUFFIX = "Team";
+const ORG_SLUG_ID_LENGTH = 8;
+const ORG_SLUG_SEPARATOR = "-team";
+
const org = await auth.api.createOrganization({
body: {
- name: `${user.name}'s Team`,
- slug: `${user.id.slice(0, 8)}-team`,
+ name: `${user.name}'s ${DEFAULT_TEAM_SUFFIX}`,
+ slug: `${user.id.slice(0, ORG_SLUG_ID_LENGTH)}${ORG_SLUG_SEPARATOR}`,
userId: user.id,
},
});📝 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 org = await auth.api.createOrganization({ | |
| body: { | |
| name: `${user.name}'s Org`, | |
| slug: `${user.id.slice(0, 8)}-org`, | |
| name: `${user.name}'s Team`, | |
| slug: `${user.id.slice(0, 8)}-team`, | |
| userId: user.id, | |
| const DEFAULT_TEAM_SUFFIX = "Team"; | |
| const ORG_SLUG_ID_LENGTH = 8; | |
| const ORG_SLUG_SEPARATOR = "-team"; | |
| const org = await auth.api.createOrganization({ | |
| body: { | |
| name: `${user.name}'s ${DEFAULT_TEAM_SUFFIX}`, | |
| slug: `${user.id.slice(0, ORG_SLUG_ID_LENGTH)}${ORG_SLUG_SEPARATOR}`, | |
| userId: user.id, |
🤖 Prompt for AI Agents
In `@packages/auth/src/server.ts` around lines 60 - 64, The template uses
hardcoded literals for team name and slug generation; extract the magic number
and string fragments into module-level constants (e.g., ID_SLICE_LENGTH = 8,
TEAM_NAME_SUFFIX = "'s Team", SLUG_SUFFIX = "-team") and update the call to
auth.api.createOrganization (the name: `${user.name}${TEAM_NAME_SUFFIX}` and
slug: `${user.id.slice(0, ID_SLICE_LENGTH)}${SLUG_SUFFIX}`) so the magic values
are centralized and clearer.
| console.log( | ||
| "[organization/leave] START - userId:", | ||
| ctx.session.user.id, | ||
| "orgId:", | ||
| input.organizationId, | ||
| ); | ||
| console.log( | ||
| "[organization/leave] Current activeOrganizationId:", | ||
| ctx.session.session.activeOrganizationId, | ||
| ); |
There was a problem hiding this comment.
Avoid logging raw user identifiers in leave flow.
These logs include userId (and org IDs) which can be PII; consider redacting or gating behind debug-only logging.
🛠️ Proposed fix
- console.log(
- "[organization/leave] START - userId:",
- ctx.session.user.id,
- "orgId:",
- input.organizationId,
- );
+ console.log(
+ "[organization/leave] START - orgId:",
+ input.organizationId,
+ );📝 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.
| console.log( | |
| "[organization/leave] START - userId:", | |
| ctx.session.user.id, | |
| "orgId:", | |
| input.organizationId, | |
| ); | |
| console.log( | |
| "[organization/leave] Current activeOrganizationId:", | |
| ctx.session.session.activeOrganizationId, | |
| ); | |
| console.log( | |
| "[organization/leave] START - orgId:", | |
| input.organizationId, | |
| ); | |
| console.log( | |
| "[organization/leave] Current activeOrganizationId:", | |
| ctx.session.session.activeOrganizationId, | |
| ); |
🤖 Prompt for AI Agents
In `@packages/trpc/src/router/organization/organization.ts` around lines 202 -
211, The console logs in the organization leave flow are emitting raw
identifiers (ctx.session.user.id, input.organizationId,
ctx.session.session.activeOrganizationId); update the logging in the
organization.leave handler to avoid PII by either removing these console.log
calls or replacing them with debug-only logging (use the app's logger at debug
level) and redact identifiers (e.g., mask/hide all but last 4 chars or log a
derived non-reversible hash) when emitting org/user IDs; ensure references to
ctx.session.user.id, input.organizationId, and
ctx.session.session.activeOrganizationId are the ones modified.
| auth: typeof auth; | ||
| headers: Headers; |
There was a problem hiding this comment.
Preserve auth/headers in protectedProcedure context.
After adding auth and headers to TRPCContext, the middleware now strips them by passing only { session }, which can break downstream procedures that rely on ctx.auth or ctx.headers.
🛠️ Suggested fix
- return next({ ctx: { session: ctx.session } });
+ return next({ ctx: { ...ctx, session: ctx.session } });Also applies to: 35-43
🤖 Prompt for AI Agents
In `@packages/trpc/src/trpc.ts` around lines 9 - 10, The middleware for
protectedProcedure is currently replacing the full TRPCContext with an object
containing only { session }, which drops ctx.auth and ctx.headers; update the
middleware (the protectedProcedure middleware in packages/trpc/src/trpc.ts) to
return the full context with preserved fields (e.g., return { ...ctx, session }
or explicitly include auth and headers such as { session, auth: ctx.auth,
headers: ctx.headers, ...otherCtx } ) so downstream procedures keep ctx.auth and
ctx.headers intact.
| import { useState } from "react"; | ||
|
|
||
| type AlertOptions = { | ||
| title: string; | ||
| description: string; | ||
| confirmText?: string; | ||
| cancelText?: string; | ||
| onConfirm: () => void | Promise<void>; | ||
| onCancel?: () => void; | ||
| }; | ||
|
|
||
| type InternalAlertOptions = AlertOptions & { | ||
| variant: "default" | "destructive"; | ||
| }; | ||
|
|
||
| let showAlertFn: ((options: InternalAlertOptions) => void) | null = null; | ||
|
|
||
| const Alerter = () => { | ||
| const [alertOptions, setAlertOptions] = useState<InternalAlertOptions | null>( | ||
| null, | ||
| ); | ||
| const [isOpen, setIsOpen] = useState(false); | ||
| const [isLoading, setIsLoading] = useState(false); | ||
|
|
||
| showAlertFn = (options) => { | ||
| setAlertOptions(options); | ||
| setIsOpen(true); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/ui/src/atoms/Alert/Alert.tsxRepository: superset-sh/superset
Length of output: 3286
🏁 Script executed:
# Verify the file type and learning applicability
file packages/ui/src/atoms/Alert/Alert.tsxRepository: superset-sh/superset
Length of output: 108
Register the global alert handler in a useEffect to avoid stale closures.
Assigning showAlertFn during render creates a new closure on every render. In Strict Mode or concurrent rendering, this can leave a stale handler reference, leading to state updates on an unmounted component. Move the assignment to a useEffect with proper cleanup.
🔧 Suggested fix
-import { useState } from "react";
+import { useCallback, useEffect, useState } from "react";
@@
- showAlertFn = (options) => {
- setAlertOptions(options);
- setIsOpen(true);
- };
+ const showAlert = useCallback((options: InternalAlertOptions) => {
+ setAlertOptions(options);
+ setIsOpen(true);
+ }, []);
+
+ useEffect(() => {
+ showAlertFn = showAlert;
+ return () => {
+ if (showAlertFn === showAlert) showAlertFn = null;
+ };
+ }, [showAlert]);🤖 Prompt for AI Agents
In `@packages/ui/src/atoms/Alert/Alert.tsx` around lines 12 - 39, The Alerter
component currently assigns the global showAlertFn during render which can
create stale closures and unsafe updates; move the assignment into a useEffect
inside the Alerter component so showAlertFn = (options) => {
setAlertOptions(options); setIsOpen(true); } is created once and cleaned up on
unmount (set showAlertFn back to null in the effect cleanup). Ensure the effect
does not capture stale state by only referencing the stable setters
setAlertOptions and setIsOpen, and keep the same behavior for opening alerts and
the cleanup to avoid updates after unmount.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/auth/index.ts`:
- Line 87: Replace the silent swallow of fs.unlink(TOKEN_FILE).catch(() => {})
so that deletion failures are handled: call fs.unlink(TOKEN_FILE) inside a
try/catch (or attach .catch(err => { ... })), log the error with context
including TOKEN_FILE when err.code !== 'ENOENT', and surface or rethrow
unexpected errors instead of returning success; treat ENOENT as non-fatal
(optional debug log) but log and fail on other errors. Use the existing
TOKEN_FILE identifier and the surrounding auth router function where the unlink
is called to locate and update the code.
In `@apps/desktop/src/renderer/routes/create-organization/page.tsx`:
- Around line 93-107: Update logging to use the consistent "[domain/operation]"
prefix and add error-handling for sign-out and submit flows: change the
slug-check console.error to use a prefix like "[org/create]/slug-check" (where
setSlugAvailable and setIsCheckingSlug are used), wrap handleSignOut body
(authClient.signOut and signOutMutation.mutate) in try/catch, log failures with
a prefix like "[org/create]/sign-out" and surface a user-facing error (e.g., set
a state error or call the existing toast/error handler), and do the same for the
onSubmit error paths (log with "[org/create]/submit" and show a user-visible
error). Ensure all new logs reference the same domain/operation format and reuse
existing state handlers (setSlugAvailable, setIsCheckingSlug, signOutMutation,
onSubmit).
- Around line 77-103: The debounced slug-check effect can update state from
stale in-flight responses; modify the useEffect that watches slugValue to guard
updates by tracking the current request (e.g., use a local `cancelled` flag or
an AbortController and capture the slugValue at request start) and only call
setSlugAvailable / setIsCheckingSlug if the response corresponds to the latest
slug. Specifically, inside the effect before calling
authClient.organization.checkSlug capture the current slug (slugValue) and set a
`cancelled` boolean or use an AbortController; on cleanup set cancelled or abort
the controller; after the await, verify the request was not cancelled and that
the returned slug matches the captured slug before updating via setSlugAvailable
or setIsCheckingSlug. Ensure errors and finally blocks also respect the
cancellation check so stale responses don't flip isCheckingSlug or availability
for a newer slug.
In
`@apps/desktop/src/renderer/screens/main/components/TopBar/OrganizationDropdown.tsx`:
- Around line 67-69: The helper function openExternal in
OrganizationDropdown.tsx currently calls window.open(url, "_blank"); replace its
implementation to call the tRPC mutation exposed for opening external URLs:
electronTrpcClient.external.openUrl.mutate(url). Ensure electronTrpcClient is
imported or available in this module (reference
electronTrpcClient.external.openUrl.mutate) and remove the window.open call so
the renderer delegates to the main process shell.openExternal; apply the same
replacement pattern wherever window.open is used for external links (e.g.,
ConfigFilePreview, SetupConfigModal, PortsList).
🧹 Nitpick comments (5)
apps/desktop/src/renderer/screens/main/components/TopBar/OrganizationDropdown.tsx (4)
57-60: Missing error handling on cache mutation.The
clearCacheMutation.mutate()call has no error callback. If the cache clear fails, the error is silently swallowed. Per coding guidelines, errors should at minimum be logged with context.Suggested improvement
async function switchOrganization(newOrgId: string): Promise<void> { await authClient.organization.setActive({ organizationId: newOrgId }); - clearCacheMutation.mutate(); + clearCacheMutation.mutate(undefined, { + onError: (error) => { + console.error("[OrganizationDropdown/switchOrganization] Failed to clear cache:", error); + }, + }); }
62-65: Missing error handling on sign-out mutation.Similar to the cache mutation,
signOutMutation.mutate()lacks error handling. A failed sign-out should be logged to aid debugging.Suggested improvement
async function handleSignOut(): Promise<void> { await authClient.signOut(); - signOutMutation.mutate(); + signOutMutation.mutate(undefined, { + onError: (error) => { + console.error("[OrganizationDropdown/handleSignOut] Sign out mutation failed:", error); + }, + }); }
148-156: Inconsistent use ofonClickvsonSelecton DropdownMenuItem.These items use
onClickwhile other items in this component (lines 96, 103, 126, 186) useonSelect. ForDropdownMenuItemfrom Radix-based libraries,onSelectis the idiomatic handler that also handles keyboard interaction and automatically closes the menu. Consider usingonSelectconsistently throughout.Suggested change for consistency
-<DropdownMenuItem - onClick={() => openExternal(COMPANY.REPORT_ISSUE_URL)} -> +<DropdownMenuItem + onSelect={() => openExternal(COMPANY.REPORT_ISSUE_URL)} +> <HiOutlineBugAnt className="h-4 w-4" /> Report Issue </DropdownMenuItem> -<DropdownMenuItem - onClick={() => navigate({ to: "/settings/keyboard" })} -> +<DropdownMenuItem + onSelect={() => navigate({ to: "/settings/keyboard" })} +> <LuKeyboard className="h-4 w-4" /> Keyboard Shortcuts
169-179: SameonClickvsonSelectinconsistency in Contact Us submenu.These submenu items also use
onClickinstead ofonSelect. Apply the same consistency fix here.Suggested change
-<DropdownMenuItem onClick={() => openExternal(COMPANY.DISCORD_URL)}> +<DropdownMenuItem onSelect={() => openExternal(COMPANY.DISCORD_URL)}> <FaDiscord className="h-4 w-4" /> Discord </DropdownMenuItem> -<DropdownMenuItem onClick={() => openExternal(COMPANY.X_URL)}> +<DropdownMenuItem onSelect={() => openExternal(COMPANY.X_URL)}> <FaXTwitter className="h-4 w-4" />X </DropdownMenuItem> -<DropdownMenuItem onClick={() => openExternal(COMPANY.MAIL_TO)}> +<DropdownMenuItem onSelect={() => openExternal(COMPANY.MAIL_TO)}> <HiOutlineEnvelope className="h-4 w-4" /> Email Founders </DropdownMenuItem>apps/desktop/src/lib/trpc/routers/auth/index.ts (1)
64-68: Extract the state TTL to a named constant.
Inline10 * 60 * 1000is a magic number; a module-level constant improves readability and reuse.♻️ Proposed change (replace usage)
- const cutoff = Date.now() - 10 * 60 * 1000; + const cutoff = Date.now() - STATE_TTL_MS;Add at module top:
const STATE_TTL_MS = 10 * 60 * 1000;As per coding guidelines, extract magic numbers to named constants.
| try { | ||
| await fs.unlink(TOKEN_FILE); | ||
| } catch {} | ||
| await fs.unlink(TOKEN_FILE).catch(() => {}); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/auth/index.ts | head -100Repository: superset-sh/superset
Length of output: 3227
Don't silently ignore token deletion failures. Swallowing unlink errors can leave a valid token on disk while reporting success. At minimum log with context, and distinguish between expected (file not found) and unexpected errors.
🐛 Suggested fix (log + fail on unexpected errors)
- await fs.unlink(TOKEN_FILE).catch(() => {});
+ await fs.unlink(TOKEN_FILE).catch((err: NodeJS.ErrnoException) => {
+ if (err?.code !== "ENOENT") {
+ console.warn("[auth/signOut] failed to delete token file", err);
+ throw err;
+ }
+ });📝 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.
| await fs.unlink(TOKEN_FILE).catch(() => {}); | |
| await fs.unlink(TOKEN_FILE).catch((err: NodeJS.ErrnoException) => { | |
| if (err?.code !== "ENOENT") { | |
| console.warn("[auth/signOut] failed to delete token file", err); | |
| throw err; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/auth/index.ts` at line 87, Replace the
silent swallow of fs.unlink(TOKEN_FILE).catch(() => {}) so that deletion
failures are handled: call fs.unlink(TOKEN_FILE) inside a try/catch (or attach
.catch(err => { ... })), log the error with context including TOKEN_FILE when
err.code !== 'ENOENT', and surface or rethrow unexpected errors instead of
returning success; treat ENOENT as non-fatal (optional debug log) but log and
fail on other errors. Use the existing TOKEN_FILE identifier and the surrounding
auth router function where the unlink is called to locate and update the code.
| // Debounced slug validation (500ms) | ||
| const slugValue = form.watch("slug"); | ||
| useEffect(() => { | ||
| const timer = setTimeout(async () => { | ||
| if (!slugValue || slugValue.length < 3) { | ||
| setSlugAvailable(null); | ||
| return; | ||
| } | ||
|
|
||
| setIsCheckingSlug(true); | ||
| try { | ||
| const result = await authClient.organization.checkSlug({ | ||
| slug: slugValue, | ||
| }); | ||
| // status: true means slug is available, false means taken | ||
| setSlugAvailable(result.data?.status ?? null); | ||
| } catch (error) { | ||
| console.error("[create-org] Slug check failed:", error); | ||
| setSlugAvailable(null); | ||
| } finally { | ||
| setIsCheckingSlug(false); | ||
| } | ||
| }, 500); | ||
|
|
||
| return () => clearTimeout(timer); | ||
| }, [slugValue]); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the CreateOrganization page file
cat -n apps/desktop/src/renderer/routes/create-organization/page.tsx | head -150Repository: superset-sh/superset
Length of output: 5229
Guard against stale slug availability results from in-flight requests.
The debounced async check can have an in-flight network request complete after the effect cleans up, updating slugAvailable with stale data from an older slug. For example, if a request for "abc" is in-flight when the user changes to "abcd", the "abc" response can arrive after the effect cleanup and incorrectly update state.
Consider guarding state updates with a cancelled flag and comparing the returned slug against the current slug value to ensure only the latest result updates state.
♻️ Proposed fix (guard stale results)
useEffect(() => {
- const timer = setTimeout(async () => {
+ let cancelled = false;
+ const currentSlug = slugValue;
+ const timer = setTimeout(async () => {
if (!slugValue || slugValue.length < 3) {
setSlugAvailable(null);
return;
}
setIsCheckingSlug(true);
try {
const result = await authClient.organization.checkSlug({
- slug: slugValue,
+ slug: currentSlug,
});
- // status: true means slug is available, false means taken
- setSlugAvailable(result.data?.status ?? null);
+ // status: true means slug is available, false means taken
+ if (!cancelled && currentSlug === slugValue) {
+ setSlugAvailable(result.data?.status ?? null);
+ }
} catch (error) {
console.error("[create-org] Slug check failed:", error);
- setSlugAvailable(null);
+ if (!cancelled) {
+ setSlugAvailable(null);
+ }
} finally {
- setIsCheckingSlug(false);
+ if (!cancelled) {
+ setIsCheckingSlug(false);
+ }
}
}, 500);
- return () => clearTimeout(timer);
+ return () => {
+ cancelled = true;
+ clearTimeout(timer);
+ };
}, [slugValue]);📝 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.
| // Debounced slug validation (500ms) | |
| const slugValue = form.watch("slug"); | |
| useEffect(() => { | |
| const timer = setTimeout(async () => { | |
| if (!slugValue || slugValue.length < 3) { | |
| setSlugAvailable(null); | |
| return; | |
| } | |
| setIsCheckingSlug(true); | |
| try { | |
| const result = await authClient.organization.checkSlug({ | |
| slug: slugValue, | |
| }); | |
| // status: true means slug is available, false means taken | |
| setSlugAvailable(result.data?.status ?? null); | |
| } catch (error) { | |
| console.error("[create-org] Slug check failed:", error); | |
| setSlugAvailable(null); | |
| } finally { | |
| setIsCheckingSlug(false); | |
| } | |
| }, 500); | |
| return () => clearTimeout(timer); | |
| }, [slugValue]); | |
| // Debounced slug validation (500ms) | |
| const slugValue = form.watch("slug"); | |
| useEffect(() => { | |
| let cancelled = false; | |
| const currentSlug = slugValue; | |
| const timer = setTimeout(async () => { | |
| if (!slugValue || slugValue.length < 3) { | |
| setSlugAvailable(null); | |
| return; | |
| } | |
| setIsCheckingSlug(true); | |
| try { | |
| const result = await authClient.organization.checkSlug({ | |
| slug: currentSlug, | |
| }); | |
| // status: true means slug is available, false means taken | |
| if (!cancelled && currentSlug === slugValue) { | |
| setSlugAvailable(result.data?.status ?? null); | |
| } | |
| } catch (error) { | |
| console.error("[create-org] Slug check failed:", error); | |
| if (!cancelled) { | |
| setSlugAvailable(null); | |
| } | |
| } finally { | |
| if (!cancelled) { | |
| setIsCheckingSlug(false); | |
| } | |
| } | |
| }, 500); | |
| return () => { | |
| cancelled = true; | |
| clearTimeout(timer); | |
| }; | |
| }, [slugValue]); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/routes/create-organization/page.tsx` around lines
77 - 103, The debounced slug-check effect can update state from stale in-flight
responses; modify the useEffect that watches slugValue to guard updates by
tracking the current request (e.g., use a local `cancelled` flag or an
AbortController and capture the slugValue at request start) and only call
setSlugAvailable / setIsCheckingSlug if the response corresponds to the latest
slug. Specifically, inside the effect before calling
authClient.organization.checkSlug capture the current slug (slugValue) and set a
`cancelled` boolean or use an AbortController; on cleanup set cancelled or abort
the controller; after the await, verify the request was not cancelled and that
the returned slug matches the captured slug before updating via setSlugAvailable
or setIsCheckingSlug. Ensure errors and finally blocks also respect the
cancellation check so stale responses don't flip isCheckingSlug or availability
for a newer slug.
| } catch (error) { | ||
| console.error("[create-org] Slug check failed:", error); | ||
| setSlugAvailable(null); | ||
| } finally { | ||
| setIsCheckingSlug(false); | ||
| } | ||
| }, 500); | ||
|
|
||
| return () => clearTimeout(timer); | ||
| }, [slugValue]); | ||
|
|
||
| async function handleSignOut(): Promise<void> { | ||
| await authClient.signOut(); | ||
| signOutMutation.mutate(); | ||
| } |
There was a problem hiding this comment.
Log and prefix errors consistently in sign-out and create flows.
Errors in handleSignOut and onSubmit aren’t logged, and the slug-check log prefix doesn’t follow the [domain/operation] pattern. Please log failures with a consistent context prefix and surface a user-facing error when sign-out fails.
🔧 Suggested logging + error handling
- console.error("[create-org] Slug check failed:", error);
+ console.error("[create-organization/slug-check] Slug check failed:", error);
setSlugAvailable(null);
} finally {
setIsCheckingSlug(false);
}
}, 500);
async function handleSignOut(): Promise<void> {
- await authClient.signOut();
- signOutMutation.mutate();
+ try {
+ await authClient.signOut();
+ signOutMutation.mutate();
+ } catch (error) {
+ console.error("[create-organization/sign-out] Sign-out failed:", error);
+ toast.error("Failed to sign out");
+ }
}
...
} catch (error) {
+ console.error("[create-organization/create] Create organization failed:", error);
toast.error(
error instanceof Error
? error.message
: "Failed to create organization",
);
}As per coding guidelines, please log errors with a [domain/operation] prefix.
Also applies to: 153-159
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/routes/create-organization/page.tsx` around lines
93 - 107, Update logging to use the consistent "[domain/operation]" prefix and
add error-handling for sign-out and submit flows: change the slug-check
console.error to use a prefix like "[org/create]/slug-check" (where
setSlugAvailable and setIsCheckingSlug are used), wrap handleSignOut body
(authClient.signOut and signOutMutation.mutate) in try/catch, log failures with
a prefix like "[org/create]/sign-out" and surface a user-facing error (e.g., set
a state error or call the existing toast/error handler), and do the same for the
onSubmit error paths (log with "[org/create]/submit" and show a user-visible
error). Ensure all new logs reference the same domain/operation format and reuse
existing state handlers (setSlugAvailable, setIsCheckingSlug, signOutMutation,
onSubmit).
| function openExternal(url: string): void { | ||
| window.open(url, "_blank"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for existing shell.openExternal usage
rg -n "shell\.openExternal|openExternal" --type=ts --type=tsx -C 2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check tRPC routes to see if there's already an IPC endpoint for opening URLs
rg -n "shell\." --type=ts --type=tsx -C 2 | head -100Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Look at the file structure and webPreferences configuration
find . -name "webPreferences*" -o -name "main.ts" -o -name "preload.ts" | head -20Repository: superset-sh/superset
Length of output: 103
🏁 Script executed:
# Check how other external operations are handled via tRPC in the codebase
rg -n "openExternal|open.*external" --type=ts --type=tsx -B 2 -A 2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Examine the current implementation of window.open usage in the codebase
rg -n "window\.open" --type=ts --type=tsx -B 2 -A 2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Search for shell.openExternal or openExternal patterns
rg -n "shell\.openExternal|openExternal" --type=ts -C 2Repository: superset-sh/superset
Length of output: 6218
🏁 Script executed:
# Check the main window configuration for webPreferences
cat ./apps/desktop/src/main/windows/main.ts | head -100Repository: superset-sh/superset
Length of output: 3211
🏁 Script executed:
# Search for window.open usage in the codebase
rg -n "window\.open" --type=ts -C 2Repository: superset-sh/superset
Length of output: 5281
🏁 Script executed:
# Find and examine tRPC router definitions
find ./apps/desktop/src -name "*router*" -o -name "*trpc*" | head -20Repository: superset-sh/superset
Length of output: 322
🏁 Script executed:
# Look for shell module imports in the main process
rg -n "require.*shell|from.*shell|from.*electron" --type=ts -C 1 | head -50Repository: superset-sh/superset
Length of output: 4212
Use the tRPC openUrl endpoint instead of window.open to open URLs in the system browser.
In Electron's renderer process, window.open opens URLs in a new BrowserWindow rather than the user's default system browser. The codebase already exposes shell.openExternal via a tRPC endpoint at apps/desktop/src/lib/trpc/routers/external/index.ts with the openUrl mutation. Use electronTrpcClient.external.openUrl.mutate(url) instead of the local window.open helper to ensure consistent behavior across all external URL handling.
This same pattern should be applied to other renderer files using window.open for external links (e.g., ConfigFilePreview.tsx, SetupConfigModal.tsx, PortsList.tsx, etc.).
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/TopBar/OrganizationDropdown.tsx`
around lines 67 - 69, The helper function openExternal in
OrganizationDropdown.tsx currently calls window.open(url, "_blank"); replace its
implementation to call the tRPC mutation exposed for opening external URLs:
electronTrpcClient.external.openUrl.mutate(url). Ensure electronTrpcClient is
imported or available in this module (reference
electronTrpcClient.external.openUrl.mutate) and remove the window.open call so
the renderer delegates to the main process shell.openExternal; apply the same
replacement pattern wherever window.open is used for external links (e.g.,
ConfigFilePreview, SetupConfigModal, PortsList).
- Add window.location.reload() after leaving org to ensure Electric and all app state reinitialize with new activeOrganizationId - Prevents 'No active organization' errors from stale session state - Ensures user is properly redirected if they have no remaining orgs
- Call organization.setActive() to get fresh bearer token with updated activeOrganizationId - Support setting activeOrganizationId to null when leaving last org - Add sign out button and auth guard to create-organization page - Add console logging for debugging leave flow - Remove afterRemoveMember hook logic (no longer needed with setActive approach) - Remove test script that was accidentally committed The key issue was that bearer tokens encode session data in JWT claims. Simply refetching the session wasn't enough - we needed to call setActive() to get a new token with updated claims. This works for both switching orgs and clearing the active org (null).
- Remove organization logo upload from create-organization page - Delete /api/upload route handler - Remove unused Avatar, HiUpload, useCallback, and env imports - Simplify form schema to only include name and slug The blob upload implementation wasn't complete and needs to be revisited. Removing it for now to keep the create org flow simpler.
- Remove window.location.reload() from org switcher dropdown - Replace reload with navigation in create-organization page - Calling setActive() is sufficient to update the session token Since setActive() returns a fresh bearer token with updated activeOrganizationId in JWT claims, React Query cache will pick up the changes and components will re-render automatically. No need for full page reload.
- Remove verbose debug logging from auth flows - Extract helper functions (leaveOrganization, removeMember, renderSlugStatus, openExternal) - Replace arrow functions with function declarations and explicit return types - Consolidate 5 separate handlers into single openExternal helper - Remove nested conditionals in JSX for better readability
saddlepaddle
force-pushed
the
change-roles-org
branch
from
b9dfb0d to
dc8088b
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/cache/index.ts`:
- Around line 11-22: Update the logging prefix and normalize the API origin
before calling session.defaultSession.clearStorageData: replace the "[cache]"
prefix in the console.log and console.error calls with "[cache/clear]" and
compute the origin via new URL(env.NEXT_PUBLIC_API_URL).origin (use that
variable when calling session.defaultSession.clearStorageData and in the log
message) to ensure clearStorageData receives a protocol+host+port origin; leave
the try/catch and return { success: true } behavior intact.
In `@apps/desktop/src/renderer/routes/_authenticated/settings/team/page.tsx`:
- Around line 71-75: currentUserRole is being force-cast from currentMember even
though currentMember can be undefined while loading, which can pass an invalid
role into permission checks like canRemoveMember; update the logic around
currentUserId/currentMember/currentUserRole to safely handle undefined by either
(a) deriving currentUserRole as currentMember?.role ?? OrganizationRole.Member
(or another safe default) or (b) checking currentMember exists before calling
permission helpers (e.g., wrap calls to canRemoveMember with a guard like if
(!currentMember) return false), and apply the same safeguard to the other
similar usage around the canRemoveMember checks later in the file.
- Around line 33-54: The query currently uses activeOrganizationId ?? "" which
can produce a bogus filter while the session/org is loading; update the
useLiveQuery callback in the component (the call to useLiveQuery that populates
membersData) to short-circuit when activeOrganizationId is not available: inside
the callback for useLiveQuery, check if (!activeOrganizationId) return [] (or an
empty array) before building the query so no invalid where clause is used, and
separately render a loading/select-org UI when activeOrganizationId is falsy
(based on session/session.activeOrganizationId or
authClient.useActiveOrganization data) instead of showing an empty members list.
In `@packages/db/drizzle/0010_add_organization_ids_to_users.sql`:
- Around line 1-12: Add a GIN index on the new array column to speed ANY
queries: create a GIN index (e.g., idx_users_organization_ids_gin) on
"auth"."users" USING GIN ("organization_ids"); prefer CREATE INDEX CONCURRENTLY
to avoid long locks during migration; include this CREATE INDEX statement in the
migration after the backfill (and ensure rollback drops the same index).
♻️ Duplicate comments (5)
packages/ui/src/atoms/Alert/Alert.tsx (1)
36-39: MoveshowAlertFnassignment touseEffectwith cleanup.Assigning
showAlertFnduring render creates a new closure on every render, risking stale handler references in Strict Mode or concurrent rendering. This was previously flagged - the fix involves usinguseEffectto register/unregister the handler.packages/auth/src/server.ts (1)
62-63: Extract hardcoded values to named constants.The magic number
8and hardcoded strings ("'s Team","-team") should be extracted to module-level constants for maintainability, as per coding guidelines.♻️ Suggested refactor
+const DEFAULT_TEAM_SUFFIX = "'s Team"; +const ORG_SLUG_ID_LENGTH = 8; +const ORG_SLUG_SUFFIX = "-team"; + // Inside the databaseHooks.user.create.after: const org = await auth.api.createOrganization({ body: { - name: `${user.name}'s Team`, - slug: `${user.id.slice(0, 8)}-team`, + name: `${user.name}${DEFAULT_TEAM_SUFFIX}`, + slug: `${user.id.slice(0, ORG_SLUG_ID_LENGTH)}${ORG_SLUG_SUFFIX}`, userId: user.id, }, });apps/desktop/src/lib/trpc/routers/auth/index.ts (1)
86-89: Don't silently ignore token deletion failures.Swallowing
unlinkerrors can leave a valid token on disk while reporting success. At minimum, log unexpected errors with context. Based on learnings, errors should never be swallowed silently.🐛 Suggested fix
- await fs.unlink(TOKEN_FILE).catch(() => {}); + await fs.unlink(TOKEN_FILE).catch((err: NodeJS.ErrnoException) => { + if (err?.code !== "ENOENT") { + console.warn("[auth/signOut] failed to delete token file", err); + throw err; + } + });packages/trpc/src/router/organization/organization.ts (1)
202-211: Avoid logging raw user/org identifiers.
These logs emituserId,organizationId, andactiveOrganizationId, which can be PII. Please redact or gate behind debug-only logging.🛠️ Suggested redaction
- console.log( - "[organization/leave] START - userId:", - ctx.session.user.id, - "orgId:", - input.organizationId, - ); + console.log("[organization/leave] START"); console.log( - "[organization/leave] Current activeOrganizationId:", - ctx.session.session.activeOrganizationId, + "[organization/leave] Current activeOrganizationId: [redacted]", ); @@ - console.log( - "[organization/leave] Other membership found:", - otherMembership?.organizationId ?? "null", - ); + console.log( + "[organization/leave] Other membership found:", + otherMembership ? "[redacted]" : "null", + ); @@ - console.log( - "[organization/leave] Updated all sessions to new activeOrganizationId:", - otherMembership?.organizationId ?? "null", - ); + console.log( + "[organization/leave] Updated all sessions to new activeOrganizationId:", + otherMembership ? "[redacted]" : "null", + );Also applies to: 259-262, 276-279
apps/desktop/src/renderer/screens/main/components/TopBar/OrganizationDropdown.tsx (1)
67-69: Route external URLs through tRPC instead ofwindow.open.Using
window.openin the renderer opens a new BrowserWindow rather than the system browser. Prefer the existing IPC route (e.g.,external.openUrl) so the main process usesshell.openExternal.🛠️ Suggested update
- function openExternal(url: string): void { - window.open(url, "_blank"); - } + const openUrlMutation = electronTrpc.external.openUrl.useMutation(); + + function openExternal(url: string): void { + openUrlMutation.mutate(url); + }Based on learnings, use tRPC for Electron IPC instead of direct renderer calls.
Also applies to: 147-177
🧹 Nitpick comments (7)
packages/ui/src/atoms/Alert/Alert.tsx (1)
48-49: Consider surfacing confirmation errors to the user.When
onConfirmthrows, the error is logged but the user receives no visual feedback—the dialog simply remains open. Consider adding error state to display a message, or propagate the error via a callback so the caller can handle it.💡 Example approach with error state
const [isLoading, setIsLoading] = useState(false); +const [error, setError] = useState<string | null>(null); const handleConfirm = async () => { if (!alertOptions) return; setIsLoading(true); + setError(null); try { await alertOptions.onConfirm(); setIsOpen(false); } catch (error) { console.error("[alert] Confirmation failed:", error); + setError(error instanceof Error ? error.message : "Confirmation failed"); } finally { setIsLoading(false); } };Then render
{error && <p className="text-destructive text-sm">{error}</p>}in the dialog content.packages/shared/src/auth/roles/index.ts (1)
1-1: Consider avoiding the new barrel if it’s not strictly needed. Prefer direct imports from./rolesto reduce circular dependency risk. As per coding guidelines, ...packages/db/drizzle/0010_add_organization_ids_to_users.sql (1)
30-39: Consider performance impact of row-level triggers on bulk operations.The
FOR EACH ROWtriggers will fire multiple times during bulk member operations, potentially causing multiple redundant updates to the same user. For typical single-member operations this is fine, but if bulk member imports/deletions are expected, aFOR EACH STATEMENTtrigger with deferred processing might be more efficient.packages/shared/src/auth/authorization/authorization.ts (2)
21-60: Consider using object parameters for better self-documentation.Per coding guidelines, functions with 2+ parameters should accept a single params object with named properties. This improves readability at call sites and extensibility.
♻️ Suggested refactor
-export function getAvailableRoleChanges( - actorRole: OrganizationRole, - targetRole: OrganizationRole, - ownerCount: number, -): OrganizationRole[] { +export function getAvailableRoleChanges(params: { + actorRole: OrganizationRole; + targetRole: OrganizationRole; + ownerCount: number; +}): OrganizationRole[] { + const { actorRole, targetRole, ownerCount } = params;
76-98: Consider using object parameters for consistency.Same recommendation as
getAvailableRoleChanges- using an object parameter would improve call-site readability, especially with theisTargetSelfboolean which benefits from named properties to avoid boolean blindness.♻️ Suggested refactor
-export function canRemoveMember( - actorRole: OrganizationRole, - targetRole: OrganizationRole, - isTargetSelf: boolean, - ownerCount: number, -): boolean { +export function canRemoveMember(params: { + actorRole: OrganizationRole; + targetRole: OrganizationRole; + isTargetSelf: boolean; + ownerCount: number; +}): boolean { + const { actorRole, targetRole, isTargetSelf, ownerCount } = params;apps/desktop/src/lib/trpc/routers/auth/index.ts (1)
64-68: Consider extracting the expiry duration to a named constant.The
10 * 60 * 1000value represents a 10-minute state expiry. While the inline comment helps, extracting to a constant likeSTATE_EXPIRY_MSwould improve clarity.♻️ Optional refactor
+const STATE_EXPIRY_MS = 10 * 60 * 1000; // 10 minutes + // Inside signIn mutation: -// Clean up expired states (10 minutes) -const cutoff = Date.now() - 10 * 60 * 1000; +// Clean up expired states +const cutoff = Date.now() - STATE_EXPIRY_MS;apps/desktop/src/renderer/screens/main/components/TopBar/OrganizationDropdown.tsx (1)
54-60: Handle cache-clear failures instead of fire-and-forget.
clearCacheMutation.mutate()ignores failures (including{ success: false }), and the cache might remain. Consider awaitingmutateAsync()and logging unsuccessful responses.🛠️ Suggested update
async function switchOrganization(newOrgId: string): Promise<void> { await authClient.organization.setActive({ organizationId: newOrgId }); - clearCacheMutation.mutate(); + try { + const result = await clearCacheMutation.mutateAsync(); + if (!result.success) { + console.error( + "[cache/clear] Cache clear failed after org switch:", + result.error, + ); + } + } catch (error) { + console.error("[cache/clear] Cache clear threw after org switch:", error); + } }As per coding guidelines, avoid silently swallowing errors.
| await session.defaultSession.clearStorageData({ | ||
| origin: env.NEXT_PUBLIC_API_URL, | ||
| }); | ||
|
|
||
| console.log( | ||
| "[cache] Cleared Electric cache for origin:", | ||
| env.NEXT_PUBLIC_API_URL, | ||
| ); | ||
|
|
||
| return { success: true }; | ||
| } catch (error) { | ||
| console.error("[cache] Failed to clear Electric cache:", error); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd apps/desktop/src/lib/trpc/routers/cache && cat -n index.ts | head -30Repository: superset-sh/superset
Length of output: 1135
🏁 Script executed:
rg "\[cache" apps/desktop --type ts --type tsx -A 1 | head -50Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
rg "NEXT_PUBLIC_API_URL" --type ts --type tsx | head -20Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
rg "\[.*\/.*\]" apps/desktop --type ts -B 1 -A 1 | head -40Repository: superset-sh/superset
Length of output: 144
🏁 Script executed:
rg "clearStorageData" --type ts -B 3 -A 3Repository: superset-sh/superset
Length of output: 659
🏁 Script executed:
rg "NEXT_PUBLIC_API_URL" --type ts -B 2 -A 2 | head -30Repository: superset-sh/superset
Length of output: 1615
🏁 Script executed:
rg "console\.(log|error)" apps/desktop/src/lib/trpc --type ts -A 0 | head -30Repository: superset-sh/superset
Length of output: 3024
🏁 Script executed:
rg "\[domain.*operation\]|\[.*\/.*\] " apps --type ts -A 0 | head -40Repository: superset-sh/superset
Length of output: 189
🏁 Script executed:
cat packages/trpc/src/env.ts | grep -A 2 -B 2 "NEXT_PUBLIC_API_URL"Repository: superset-sh/superset
Length of output: 265
🏁 Script executed:
rg "console\.(log|error)" apps/desktop/src/lib/trpc --type ts -B 1 -A 1 | grep "\[" | head -20Repository: superset-sh/superset
Length of output: 2797
🏁 Script executed:
cat apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts | grep console.log | head -5Repository: superset-sh/superset
Length of output: 206
🏁 Script executed:
python3 << 'EOF'
# Check if NEXT_PUBLIC_API_URL is validated as a full URL
import re
test_urls = [
"https://api.example.com",
"https://api.example.com/path",
"https://api.example.com:3000",
]
for url in test_urls:
parsed = url.split('?')[0].split('#')[0] # Remove query/fragment
origin = parsed.rsplit('/', 1)[0] if '/' in parsed[8:] else parsed # Simple extraction
print(f"URL: {url}")
print(f" Would use origin: {origin}")
print(f" Different from URL: {origin != url}")
print()
EOFRepository: superset-sh/superset
Length of output: 150
🏁 Script executed:
cat apps/desktop/src/lib/trpc/routers/cache/index.ts | head -35Repository: superset-sh/superset
Length of output: 993
🏁 Script executed:
cd apps/desktop && cat tsconfig.json | grep -A 20 "paths"Repository: superset-sh/superset
Length of output: 322
🏁 Script executed:
rg "new URL" apps/desktop --type ts -B 2 -A 2 | head -20Repository: superset-sh/superset
Length of output: 1530
Update log prefix to follow [domain/operation] convention and normalize API origin.
Per coding guidelines, log prefixes should use [domain/operation] format. The current [cache] prefix needs updating to [cache/clear]. Additionally, clearStorageData expects an origin (protocol + domain + port); if NEXT_PUBLIC_API_URL includes a path, it may not work correctly. Use new URL(...).origin to extract the origin safely.
🛠️ Suggested update
- await session.defaultSession.clearStorageData({
- origin: env.NEXT_PUBLIC_API_URL,
- });
-
- console.log(
- "[cache] Cleared Electric cache for origin:",
- env.NEXT_PUBLIC_API_URL,
- );
+ const apiOrigin = new URL(env.NEXT_PUBLIC_API_URL).origin;
+ await session.defaultSession.clearStorageData({ origin: apiOrigin });
+
+ console.log("[cache/clear] Cleared Electric cache for origin:", apiOrigin);🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/cache/index.ts` around lines 11 - 22,
Update the logging prefix and normalize the API origin before calling
session.defaultSession.clearStorageData: replace the "[cache]" prefix in the
console.log and console.error calls with "[cache/clear]" and compute the origin
via new URL(env.NEXT_PUBLIC_API_URL).origin (use that variable when calling
session.defaultSession.clearStorageData and in the log message) to ensure
clearStorageData receives a protocol+host+port origin; leave the try/catch and
return { success: true } behavior intact.
| const activeOrganizationId = session?.session?.activeOrganizationId; | ||
| const { data: activeOrg } = authClient.useActiveOrganization(); | ||
|
|
||
| const { data: membersData, isLoading } = useLiveQuery( | ||
| (q) => | ||
| q | ||
| .from({ members: collections.members }) | ||
| .leftJoin({ users: collections.users }, ({ members, users }) => | ||
| .innerJoin({ users: collections.users }, ({ members, users }) => | ||
| eq(members.userId, users.id), | ||
| ) | ||
| .select(({ members, users }) => ({ | ||
| ...users, | ||
| ...members, | ||
| memberId: members.id, | ||
| userId: members.userId, | ||
| name: users?.name ?? null, | ||
| email: users?.email ?? "", | ||
| image: users?.image ?? null, | ||
| role: members.role, | ||
| joinedAt: members.createdAt, | ||
| organizationId: members.organizationId, | ||
| })) | ||
| .where(({ members }) => | ||
| eq(members.organizationId, activeOrganizationId ?? ""), | ||
| ) | ||
| .orderBy(({ members }) => members.role, "asc") | ||
| .orderBy(({ members }) => members.createdAt, "asc"), | ||
| [collections], | ||
| [collections, activeOrganizationId], | ||
| ); |
There was a problem hiding this comment.
Guard the query when activeOrganizationId is not ready.
Using activeOrganizationId ?? "" can yield a misleading empty state (or an invalid filter) while the session is loading. Consider rendering a “select org / loading” state until an active org is available, or otherwise short-circuit the empty-org case.
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/routes/_authenticated/settings/team/page.tsx`
around lines 33 - 54, The query currently uses activeOrganizationId ?? "" which
can produce a bogus filter while the session/org is loading; update the
useLiveQuery callback in the component (the call to useLiveQuery that populates
membersData) to short-circuit when activeOrganizationId is not available: inside
the callback for useLiveQuery, check if (!activeOrganizationId) return [] (or an
empty array) before building the query so no invalid where clause is used, and
separately render a loading/select-org UI when activeOrganizationId is falsy
(based on session/session.activeOrganizationId or
authClient.useActiveOrganization data) instead of showing an empty members list.
| const currentUserId = session?.user?.id; | ||
| const currentMember = members.find((m) => m.userId === currentUserId); | ||
| const isOwner = currentMember?.role === "owner"; | ||
| const currentMember = activeOrg?.members?.find( | ||
| (m) => m.userId === currentUserId, | ||
| ); | ||
| const currentUserRole = currentMember?.role as OrganizationRole; |
There was a problem hiding this comment.
Avoid passing an undefined role into permission checks.
currentUserRole is asserted as OrganizationRole, but currentMember can be undefined while data is loading. That can silently feed an invalid role into canRemoveMember. Consider guarding or defaulting safely.
🛠️ One safe guard approach
- const currentUserRole = currentMember?.role as OrganizationRole;
+ const currentUserRole = currentMember?.role as OrganizationRole | undefined;
@@
- <MemberActions
- member={member}
- currentUserRole={currentUserRole}
- ownerCount={ownerCount}
- isCurrentUser={isCurrentUserRow}
- canRemove={canRemoveMember(
- currentUserRole,
- member.role as OrganizationRole,
- isCurrentUserRow,
- ownerCount,
- )}
- />
+ <MemberActions
+ member={member}
+ currentUserRole={currentUserRole ?? "member"}
+ ownerCount={ownerCount}
+ isCurrentUser={isCurrentUserRow}
+ canRemove={
+ currentUserRole
+ ? canRemoveMember(
+ currentUserRole,
+ member.role as OrganizationRole,
+ isCurrentUserRow,
+ ownerCount,
+ )
+ : false
+ }
+ />Also applies to: 183-188
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/routes/_authenticated/settings/team/page.tsx`
around lines 71 - 75, currentUserRole is being force-cast from currentMember
even though currentMember can be undefined while loading, which can pass an
invalid role into permission checks like canRemoveMember; update the logic
around currentUserId/currentMember/currentUserRole to safely handle undefined by
either (a) deriving currentUserRole as currentMember?.role ??
OrganizationRole.Member (or another safe default) or (b) checking currentMember
exists before calling permission helpers (e.g., wrap calls to canRemoveMember
with a guard like if (!currentMember) return false), and apply the same
safeguard to the other similar usage around the canRemoveMember checks later in
the file.
| ALTER TABLE "auth"."users" ADD COLUMN "organization_ids" uuid[] DEFAULT '{}' NOT NULL; | ||
|
|
||
| -- Populate existing organization_ids from members table | ||
| UPDATE "auth"."users" AS u | ||
| SET "organization_ids" = ( | ||
| SELECT ARRAY_AGG(DISTINCT m."organization_id") | ||
| FROM "auth"."members" AS m | ||
| WHERE m."user_id" = u."id" | ||
| ) | ||
| WHERE EXISTS ( | ||
| SELECT 1 FROM "auth"."members" WHERE "user_id" = u."id" | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full content of the migration file
cat -n packages/db/drizzle/0010_add_organization_ids_to_users.sqlRepository: superset-sh/superset
Length of output: 1530
🏁 Script executed:
# Search for organization_ids usage in the codebase
rg "organization_ids" --type ts --type js -C 2 | head -100Repository: superset-sh/superset
Length of output: 899
Migration logic is correct.
The column addition with DEFAULT '{}' and NOT NULL is appropriate. The backfill query correctly handles users with existing memberships—the WHERE EXISTS clause ensures ARRAY_AGG always returns a non-null result for updated rows, while users without memberships retain the empty array default.
The trigger function properly maintains the denormalized organization_ids column on INSERT, UPDATE, and DELETE operations with appropriate COALESCE handling. The split between the combined INSERT/UPDATE trigger and separate DELETE trigger is correct.
Consider adding a GIN index on organization_ids since the codebase queries this array with the ANY operator (e.g., $1 = ANY("organization_ids") in the Electric SQL filters). This would improve filter performance, especially as membership counts grow.
🤖 Prompt for AI Agents
In `@packages/db/drizzle/0010_add_organization_ids_to_users.sql` around lines 1 -
12, Add a GIN index on the new array column to speed ANY queries: create a GIN
index (e.g., idx_users_organization_ids_gin) on "auth"."users" USING GIN
("organization_ids"); prefer CREATE INDEX CONCURRENTLY to avoid long locks
during migration; include this CREATE INDEX statement in the migration after the
backfill (and ensure rollback drops the same index).
Summary
organization.setActive()to get fresh bearer tokenactiveOrganizationIdto null)afterRemoveMemberhook (no longer needed)Problem
After leaving an organization, the desktop client retained the old
activeOrganizationIdeven after hard refresh. This happened because:refetchSession()validates the existing token, which still contains old claimsSolution
Call
authClient.organization.setActive()after leaving to get a new bearer token with updated session claims. This works for:nullto clear active org)Changes
setActive()with newactiveOrganizationId(or null) after leave mutation succeedsafterRemoveMemberhook logicTest Plan
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.