feat(desktop): Implement organization settings, reorganize settings routes#820
Merged
saddlepaddle merged 18 commits intomainfrom Jan 19, 2026
Merged
feat(desktop): Implement organization settings, reorganize settings routes#820saddlepaddle merged 18 commits intomainfrom
saddlepaddle merged 18 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces team-centric settings with organization + members, adds Organization and Members UI and components, implements desktop image selection via Electron ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Renderer as Desktop Renderer\n(OrganizationSettings)
participant Window as Electron Main\n(selectImageFile)
participant TRPC as TRPC Server\n(organization.uploadLogo)
participant Blob as Vercel Blob
participant DB as Database
User->>Renderer: Click "Upload Logo"
Renderer->>Window: call selectImageFile()
Window-->>Renderer: { canceled, dataUrl }
alt user selected image
Renderer->>TRPC: call organization.uploadLogo(fileData, fileName, mimeType)
TRPC->>DB: verify org & user is owner
TRPC->>Blob: delete existing blob (if any)
TRPC->>TRPC: decode base64, validate mime & size
TRPC->>Blob: put new blob (upload)
Blob-->>TRPC: return url
TRPC->>DB: update organization.logo = url
TRPC-->>Renderer: { success, url, organization }
Renderer->>User: update preview & show toast
else canceled
Renderer->>User: no change
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/trpc/src/router/organization/organization.ts (1)
101-149: Block empty update payloads before callingupdate().set().With all fields optional, a request containing only
idyields an emptydataobject;set({})will produce invalid SQL. Add a guard to require at least one field.🛡️ Suggested guard
.mutation(async ({ ctx, input }) => { const { id, ...data } = input; + + if (!Object.keys(data).length) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: "No fields provided to update", + }); + }
🤖 Fix all issues with AI agents
In `@packages/trpc/src/router/organization/organization.ts`:
- Around line 152-275: The uploadLogo mutation accepts unvalidated
input.fileName, input.mimeType and decodes fileData before checking size; fix by
adding allowlists for acceptable MIME types (e.g., image/png, image/jpeg,
image/webp) and file extensions (png, jpg, jpeg, webp) and reject mismatches in
uploadLogo, estimate decoded size from the base64 string length (use the base64
length -> decoded bytes ≈ (len * 3/4) - padding) before calling Buffer.from to
enforce the 4.5MB limit and return a BAD_REQUEST if exceeded, and only decode to
Buffer and call put(pathname, buffer, ...) after these validations succeed; also
validate that the file extension inferred from input.fileName corresponds to the
allowed MIME types to avoid spoofing.
🧹 Nitpick comments (6)
apps/desktop/src/lib/trpc/routers/window.ts (1)
76-83: Consider adding error handling for file read operations.If
fs.readFilefails (e.g., file deleted between selection and read, or permission issues), the error will propagate unhandled. Consider wrapping in try-catch with contextual logging.Proposed improvement
// Read the file and convert to base64 data URL const fs = await import("node:fs/promises"); const path = await import("node:path"); const filePath = result.filePaths[0]; - const buffer = await fs.readFile(filePath); - const ext = path.extname(filePath).slice(1).toLowerCase(); - const mimeType = ext === "jpg" ? "jpeg" : ext; - const base64 = buffer.toString("base64"); - const dataUrl = `data:image/${mimeType};base64,${base64}`; - - return { canceled: false, dataUrl }; + try { + const buffer = await fs.readFile(filePath); + const ext = path.extname(filePath).slice(1).toLowerCase(); + const mimeType = ext === "jpg" ? "jpeg" : ext; + const base64 = buffer.toString("base64"); + const dataUrl = `data:image/${mimeType};base64,${base64}`; + return { canceled: false, dataUrl }; + } catch (error) { + console.error("[window/selectImageFile] Failed to read file:", error); + return { canceled: true, dataUrl: null }; + }apps/desktop/src/renderer/routes/_authenticated/settings/organization/components/OrganizationLogo/OrganizationLogo.tsx (1)
15-22: Consider improving alt text accessibility.The alt text is hardcoded as "Organization logo". For better accessibility, consider including the organization name:
- alt="Organization logo" + alt={`${name} logo`}This provides more context for screen reader users, especially when multiple organization logos might be displayed.
apps/desktop/src/renderer/routes/_authenticated/settings/members/page.tsx (1)
183-188: Redundant type cast.The
member.roleis already typed asOrganizationRole(from theMembertype and the cast on lines 61-62), so the cast on line 185 is unnecessary:canRemove={canRemoveMember( currentUserRole, - member.role as OrganizationRole, + member.role, isCurrentUserRow, ownerCount, )}apps/desktop/src/renderer/routes/_authenticated/settings/organization/page.tsx (2)
88-118: Potential stale state update after unmount.The async operation inside the debounced timer can complete after the component unmounts, leading to state updates on an unmounted component. Consider adding an abort flag:
useEffect(() => { if (!originalSlug || !isSlugDialogOpen) return; + let cancelled = false; const timer = setTimeout(async () => { if (slugValue === originalSlug) { setSlugAvailable(null); return; } if (!slugValue || slugValue.length < 3) { setSlugAvailable(null); return; } setIsCheckingSlug(true); try { const result = await authClient.organization.checkSlug({ slug: slugValue, }); + if (cancelled) return; setSlugAvailable(result.data?.status ?? null); } catch (error) { + if (cancelled) return; console.error("[organization-settings] Slug check failed:", error); setSlugAvailable(null); } finally { + if (!cancelled) setIsCheckingSlug(false); - setIsCheckingSlug(false); } }, 500); - return () => clearTimeout(timer); + return () => { + cancelled = true; + clearTimeout(timer); + }; }, [slugValue, originalSlug, isSlugDialogOpen]);This is a minor concern since React 18+ handles this more gracefully, but the pattern prevents potential console warnings.
120-145: Consider showing loading state during logo upload.The upload operation can take time, but there's no visual feedback while
selectImageMutationor the subsequentuploadLogocall is in progress. Consider disabling the button or showing a spinner:<button type="button" onClick={handleLogoUpload} - className="relative w-8 h-8 cursor-pointer group" + className="relative w-8 h-8 cursor-pointer group" + disabled={selectImageMutation.isPending} >Or add a loading overlay to indicate the upload is in progress.
packages/trpc/src/router/organization/organization.ts (1)
85-99: Extract slug/name constraints into module constants.Keeps validation rules consistent and avoids scattering magic numbers/regex literals. As per coding guidelines, please extract these values to named constants.
♻️ Proposed refactor
import { z } from "zod"; import { protectedProcedure, publicProcedure } from "../../trpc"; +const ORG_NAME_MAX_LEN = 100; +const SLUG_MIN_LEN = 3; +const SLUG_MAX_LEN = 50; +const SLUG_ALLOWED_RE = /^[a-z0-9-]+$/; +const SLUG_START_RE = /^[a-z0-9]/; +const SLUG_END_RE = /[a-z0-9]$/; + export const organizationRouter = { // ... update: protectedProcedure .input( z.object({ id: z.string().uuid(), - name: z.string().min(1).max(100).optional(), + name: z.string().min(1).max(ORG_NAME_MAX_LEN).optional(), slug: z .string() - .min(3, "Slug must be at least 3 characters") - .max(50) + .min(SLUG_MIN_LEN, "Slug must be at least 3 characters") + .max(SLUG_MAX_LEN) .regex( - /^[a-z0-9-]+$/, + SLUG_ALLOWED_RE, "Slug can only contain lowercase letters, numbers, and hyphens", ) - .regex(/^[a-z0-9]/, "Slug must start with a letter or number") - .regex(/[a-z0-9]$/, "Slug must end with a letter or number") + .regex(SLUG_START_RE, "Slug must start with a letter or number") + .regex(SLUG_END_RE, "Slug must end with a letter or number") .optional(), logo: z.string().url().optional(), }), )
Comment on lines
+152
to
+275
| uploadLogo: protectedProcedure | ||
| .input( | ||
| z.object({ | ||
| organizationId: z.string().uuid(), | ||
| fileData: z.string(), // base64 string | ||
| fileName: z.string(), | ||
| mimeType: z.string(), | ||
| }), | ||
| ) | ||
| .mutation(async ({ ctx, input }) => { | ||
| console.log( | ||
| "[organization/uploadLogo] START - orgId:", | ||
| input.organizationId, | ||
| ); | ||
|
|
||
| // Check if user is a member and is owner | ||
| const membership = await db.query.members.findFirst({ | ||
| where: and( | ||
| eq(members.organizationId, input.organizationId), | ||
| eq(members.userId, ctx.session.user.id), | ||
| ), | ||
| }); | ||
|
|
||
| if (!membership) { | ||
| throw new TRPCError({ | ||
| code: "FORBIDDEN", | ||
| message: "You are not a member of this organization", | ||
| }); | ||
| } | ||
|
|
||
| if (membership.role !== "owner") { | ||
| throw new TRPCError({ | ||
| code: "FORBIDDEN", | ||
| message: "Only owners can update organization settings", | ||
| }); | ||
| } | ||
|
|
||
| // Get current organization to check for old logo | ||
| const organization = await db.query.organizations.findFirst({ | ||
| where: eq(organizations.id, input.organizationId), | ||
| }); | ||
|
|
||
| if (!organization) { | ||
| throw new TRPCError({ | ||
| code: "NOT_FOUND", | ||
| message: "Organization not found", | ||
| }); | ||
| } | ||
|
|
||
| // Delete old logo from blob storage if it exists and is a blob URL | ||
| if (organization.logo?.includes("blob.vercel-storage.com")) { | ||
| try { | ||
| // Extract pathname from blob URL | ||
| const url = new URL(organization.logo); | ||
| const pathname = url.pathname.slice(1); // Remove leading slash | ||
| console.log("[organization/uploadLogo] Deleting old logo:", pathname); | ||
| await del(pathname); | ||
| } catch (error) { | ||
| console.error( | ||
| "[organization/uploadLogo] Failed to delete old logo:", | ||
| error, | ||
| ); | ||
| // Continue anyway - don't fail the upload if deletion fails | ||
| } | ||
| } | ||
|
|
||
| // Generate unique pathname: organization/{orgId}/logo/{timestamp}-{randomId}.{ext} | ||
| const timestamp = Date.now(); | ||
| const randomId = Math.random().toString(36).substring(2, 15); | ||
| const ext = input.fileName.split(".").pop()?.toLowerCase() || "png"; | ||
| const pathname = `organization/${input.organizationId}/logo/${timestamp}-${randomId}.${ext}`; | ||
|
|
||
| console.log("[organization/uploadLogo] Uploading to pathname:", pathname); | ||
|
|
||
| // Convert base64 to Buffer | ||
| // Handle both data URL format and raw base64 | ||
| const base64Data = input.fileData.includes("base64,") | ||
| ? input.fileData.split("base64,")[1] || input.fileData | ||
| : input.fileData; | ||
| const buffer = Buffer.from(base64Data, "base64"); | ||
|
|
||
| // Check file size (4.5 MB limit for server uploads) | ||
| const sizeInMB = buffer.length / (1024 * 1024); | ||
| if (sizeInMB > 4.5) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: `File too large (${sizeInMB.toFixed(2)}MB). Maximum size is 4.5MB`, | ||
| }); | ||
| } | ||
|
|
||
| // Upload to Vercel Blob | ||
| try { | ||
| const blob = await put(pathname, buffer, { | ||
| access: "public", | ||
| contentType: input.mimeType, | ||
| }); | ||
|
|
||
| console.log( | ||
| "[organization/uploadLogo] Upload successful, URL:", | ||
| blob.url, | ||
| ); | ||
|
|
||
| // Update database with new blob URL | ||
| const [updatedOrg] = await db | ||
| .update(organizations) | ||
| .set({ logo: blob.url }) | ||
| .where(eq(organizations.id, input.organizationId)) | ||
| .returning(); | ||
|
|
||
| console.log("[organization/uploadLogo] COMPLETE"); | ||
|
|
||
| return { | ||
| success: true, | ||
| url: blob.url, | ||
| organization: updatedOrg, | ||
| }; | ||
| } catch (error) { | ||
| console.error("[organization/uploadLogo] Upload failed:", error); | ||
| throw new TRPCError({ | ||
| code: "INTERNAL_SERVER_ERROR", | ||
| message: "Failed to upload logo", | ||
| }); | ||
| } | ||
| }), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the file exists and check its current state
cat -n packages/trpc/src/router/organization/organization.ts | head -300Repository: superset-sh/superset
Length of output: 9915
🏁 Script executed:
# Check if there's any documentation or comments about Vercel Blob in the codebase
rg -i "vercel.*blob" --type ts --type tsx -A 3 -B 3 | head -50Repository: superset-sh/superset
Length of output: 91
🌐 Web query:
Vercel Blob storage MIME type validation security restrictions
💡 Result:
Short summary — Vercel Blob enforces several MIME-type–related security restrictions and upload controls:
-
Response headers to prevent MIME sniffing and embedding: Vercel Blob sets Content-Security-Policy: default-src "none", X-Frame-Options: DENY, and X-Content-Type-Options: nosniff for served blobs (helps stop browsers from treating a file as a different MIME type). [1]
-
Content-disposition behavior to avoid rendering dangerous types: blobs are served with content-disposition either inline or attachment depending on the MIME type; many types (e.g., image/, audio/, video/*, text/plain, application/json, application/pdf, text/xml) are inline, all other MIME types default to attachment (so arbitrary HTML or unknown types are forced to download rather than execute/display). [2]
-
Upload-side validation controls: the
@vercel/blobSDK/CLI lets you set/override Content-Type when uploading (it will infer from extension if not provided) and provides an allowedContentTypes array (supports wildcards) plus maximumSizeInBytes and other upload constraints to restrict what clients can upload. Use server-side tokens and these options to enforce MIME constraints. [3]
References:
- Security / response headers and protections. [1]
- Viewing/download behavior and which MIME types are served inline vs attachment. [2]
- SDK upload options: allowedContentTypes, content-type handling, size limits. [3]
Add MIME type and extension validation; estimate base64 size before decoding.
mimeType and fileName are client-controlled inputs without validation, allowing arbitrary content to be stored at public URLs. Additionally, the base64 payload is decoded into a Buffer before the size check, which can spike memory usage for very large payloads. Add allowlists for MIME types and file extensions, and estimate the decoded size from the base64 string length before decoding.
Proposed hardening
.mutation(async ({ ctx, input }) => {
console.log(
"[organization/uploadLogo] START - orgId:",
input.organizationId,
);
+
+ const MAX_LOGO_BYTES = 4.5 * 1024 * 1024;
+ const ALLOWED_MIME_TYPES = new Set([
+ "image/png",
+ "image/jpeg",
+ "image/webp",
+ "image/svg+xml",
+ ]);
+ const ALLOWED_EXTS = new Set(["png", "jpg", "jpeg", "webp", "svg"]);
+
+ if (!ALLOWED_MIME_TYPES.has(input.mimeType)) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "Unsupported logo mime type",
+ });
+ }
+
+ const ext = input.fileName.split(".").pop()?.toLowerCase();
+ if (!ext || !ALLOWED_EXTS.has(ext)) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "Unsupported logo file extension",
+ });
+ }
// Generate unique pathname: organization/{orgId}/logo/{timestamp}-{randomId}.{ext}
const timestamp = Date.now();
const randomId = Math.random().toString(36).substring(2, 15);
- const ext = input.fileName.split(".").pop()?.toLowerCase() || "png";
const pathname = `organization/${input.organizationId}/logo/${timestamp}-${randomId}.${ext}`;
console.log("[organization/uploadLogo] Uploading to pathname:", pathname);
@@
const base64Data = input.fileData.includes("base64,")
? input.fileData.split("base64,")[1] || input.fileData
: input.fileData;
- const buffer = Buffer.from(base64Data, "base64");
+ const estimatedBytes = Math.floor((base64Data.length * 3) / 4);
+ if (estimatedBytes > MAX_LOGO_BYTES) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: `File too large (${(estimatedBytes / (1024 * 1024)).toFixed(2)}MB). Maximum size is ${(MAX_LOGO_BYTES / (1024 * 1024)).toFixed(2)}MB`,
+ });
+ }
+ const buffer = Buffer.from(base64Data, "base64");
+ if (buffer.length === 0) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "Invalid base64 file data",
+ });
+ }
// Check file size (4.5 MB limit for server uploads)
const sizeInMB = buffer.length / (1024 * 1024);
- if (sizeInMB > 4.5) {
+ if (buffer.length > MAX_LOGO_BYTES) {
throw new TRPCError({
code: "BAD_REQUEST",
- message: `File too large (${sizeInMB.toFixed(2)}MB). Maximum size is 4.5MB`,
+ message: `File too large (${sizeInMB.toFixed(2)}MB). Maximum size is ${(MAX_LOGO_BYTES / (1024 * 1024)).toFixed(2)}MB`,
});
}📝 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.
Suggested change
| uploadLogo: protectedProcedure | |
| .input( | |
| z.object({ | |
| organizationId: z.string().uuid(), | |
| fileData: z.string(), // base64 string | |
| fileName: z.string(), | |
| mimeType: z.string(), | |
| }), | |
| ) | |
| .mutation(async ({ ctx, input }) => { | |
| console.log( | |
| "[organization/uploadLogo] START - orgId:", | |
| input.organizationId, | |
| ); | |
| // Check if user is a member and is owner | |
| const membership = await db.query.members.findFirst({ | |
| where: and( | |
| eq(members.organizationId, input.organizationId), | |
| eq(members.userId, ctx.session.user.id), | |
| ), | |
| }); | |
| if (!membership) { | |
| throw new TRPCError({ | |
| code: "FORBIDDEN", | |
| message: "You are not a member of this organization", | |
| }); | |
| } | |
| if (membership.role !== "owner") { | |
| throw new TRPCError({ | |
| code: "FORBIDDEN", | |
| message: "Only owners can update organization settings", | |
| }); | |
| } | |
| // Get current organization to check for old logo | |
| const organization = await db.query.organizations.findFirst({ | |
| where: eq(organizations.id, input.organizationId), | |
| }); | |
| if (!organization) { | |
| throw new TRPCError({ | |
| code: "NOT_FOUND", | |
| message: "Organization not found", | |
| }); | |
| } | |
| // Delete old logo from blob storage if it exists and is a blob URL | |
| if (organization.logo?.includes("blob.vercel-storage.com")) { | |
| try { | |
| // Extract pathname from blob URL | |
| const url = new URL(organization.logo); | |
| const pathname = url.pathname.slice(1); // Remove leading slash | |
| console.log("[organization/uploadLogo] Deleting old logo:", pathname); | |
| await del(pathname); | |
| } catch (error) { | |
| console.error( | |
| "[organization/uploadLogo] Failed to delete old logo:", | |
| error, | |
| ); | |
| // Continue anyway - don't fail the upload if deletion fails | |
| } | |
| } | |
| // Generate unique pathname: organization/{orgId}/logo/{timestamp}-{randomId}.{ext} | |
| const timestamp = Date.now(); | |
| const randomId = Math.random().toString(36).substring(2, 15); | |
| const ext = input.fileName.split(".").pop()?.toLowerCase() || "png"; | |
| const pathname = `organization/${input.organizationId}/logo/${timestamp}-${randomId}.${ext}`; | |
| console.log("[organization/uploadLogo] Uploading to pathname:", pathname); | |
| // Convert base64 to Buffer | |
| // Handle both data URL format and raw base64 | |
| const base64Data = input.fileData.includes("base64,") | |
| ? input.fileData.split("base64,")[1] || input.fileData | |
| : input.fileData; | |
| const buffer = Buffer.from(base64Data, "base64"); | |
| // Check file size (4.5 MB limit for server uploads) | |
| const sizeInMB = buffer.length / (1024 * 1024); | |
| if (sizeInMB > 4.5) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: `File too large (${sizeInMB.toFixed(2)}MB). Maximum size is 4.5MB`, | |
| }); | |
| } | |
| // Upload to Vercel Blob | |
| try { | |
| const blob = await put(pathname, buffer, { | |
| access: "public", | |
| contentType: input.mimeType, | |
| }); | |
| console.log( | |
| "[organization/uploadLogo] Upload successful, URL:", | |
| blob.url, | |
| ); | |
| // Update database with new blob URL | |
| const [updatedOrg] = await db | |
| .update(organizations) | |
| .set({ logo: blob.url }) | |
| .where(eq(organizations.id, input.organizationId)) | |
| .returning(); | |
| console.log("[organization/uploadLogo] COMPLETE"); | |
| return { | |
| success: true, | |
| url: blob.url, | |
| organization: updatedOrg, | |
| }; | |
| } catch (error) { | |
| console.error("[organization/uploadLogo] Upload failed:", error); | |
| throw new TRPCError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: "Failed to upload logo", | |
| }); | |
| } | |
| }), | |
| uploadLogo: protectedProcedure | |
| .input( | |
| z.object({ | |
| organizationId: z.string().uuid(), | |
| fileData: z.string(), // base64 string | |
| fileName: z.string(), | |
| mimeType: z.string(), | |
| }), | |
| ) | |
| .mutation(async ({ ctx, input }) => { | |
| console.log( | |
| "[organization/uploadLogo] START - orgId:", | |
| input.organizationId, | |
| ); | |
| const MAX_LOGO_BYTES = 4.5 * 1024 * 1024; | |
| const ALLOWED_MIME_TYPES = new Set([ | |
| "image/png", | |
| "image/jpeg", | |
| "image/webp", | |
| "image/svg+xml", | |
| ]); | |
| const ALLOWED_EXTS = new Set(["png", "jpg", "jpeg", "webp", "svg"]); | |
| if (!ALLOWED_MIME_TYPES.has(input.mimeType)) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Unsupported logo mime type", | |
| }); | |
| } | |
| const ext = input.fileName.split(".").pop()?.toLowerCase(); | |
| if (!ext || !ALLOWED_EXTS.has(ext)) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Unsupported logo file extension", | |
| }); | |
| } | |
| // Check if user is a member and is owner | |
| const membership = await db.query.members.findFirst({ | |
| where: and( | |
| eq(members.organizationId, input.organizationId), | |
| eq(members.userId, ctx.session.user.id), | |
| ), | |
| }); | |
| if (!membership) { | |
| throw new TRPCError({ | |
| code: "FORBIDDEN", | |
| message: "You are not a member of this organization", | |
| }); | |
| } | |
| if (membership.role !== "owner") { | |
| throw new TRPCError({ | |
| code: "FORBIDDEN", | |
| message: "Only owners can update organization settings", | |
| }); | |
| } | |
| // Get current organization to check for old logo | |
| const organization = await db.query.organizations.findFirst({ | |
| where: eq(organizations.id, input.organizationId), | |
| }); | |
| if (!organization) { | |
| throw new TRPCError({ | |
| code: "NOT_FOUND", | |
| message: "Organization not found", | |
| }); | |
| } | |
| // Delete old logo from blob storage if it exists and is a blob URL | |
| if (organization.logo?.includes("blob.vercel-storage.com")) { | |
| try { | |
| // Extract pathname from blob URL | |
| const url = new URL(organization.logo); | |
| const pathname = url.pathname.slice(1); // Remove leading slash | |
| console.log("[organization/uploadLogo] Deleting old logo:", pathname); | |
| await del(pathname); | |
| } catch (error) { | |
| console.error( | |
| "[organization/uploadLogo] Failed to delete old logo:", | |
| error, | |
| ); | |
| // Continue anyway - don't fail the upload if deletion fails | |
| } | |
| } | |
| // Generate unique pathname: organization/{orgId}/logo/{timestamp}-{randomId}.{ext} | |
| const timestamp = Date.now(); | |
| const randomId = Math.random().toString(36).substring(2, 15); | |
| const pathname = `organization/${input.organizationId}/logo/${timestamp}-${randomId}.${ext}`; | |
| console.log("[organization/uploadLogo] Uploading to pathname:", pathname); | |
| // Convert base64 to Buffer | |
| // Handle both data URL format and raw base64 | |
| const base64Data = input.fileData.includes("base64,") | |
| ? input.fileData.split("base64,")[1] || input.fileData | |
| : input.fileData; | |
| const estimatedBytes = Math.floor((base64Data.length * 3) / 4); | |
| if (estimatedBytes > MAX_LOGO_BYTES) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: `File too large (${(estimatedBytes / (1024 * 1024)).toFixed(2)}MB). Maximum size is ${(MAX_LOGO_BYTES / (1024 * 1024)).toFixed(2)}MB`, | |
| }); | |
| } | |
| const buffer = Buffer.from(base64Data, "base64"); | |
| if (buffer.length === 0) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Invalid base64 file data", | |
| }); | |
| } | |
| // Check file size (4.5 MB limit for server uploads) | |
| const sizeInMB = buffer.length / (1024 * 1024); | |
| if (buffer.length > MAX_LOGO_BYTES) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: `File too large (${sizeInMB.toFixed(2)}MB). Maximum size is ${(MAX_LOGO_BYTES / (1024 * 1024)).toFixed(2)}MB`, | |
| }); | |
| } | |
| // Upload to Vercel Blob | |
| try { | |
| const blob = await put(pathname, buffer, { | |
| access: "public", | |
| contentType: input.mimeType, | |
| }); | |
| console.log( | |
| "[organization/uploadLogo] Upload successful, URL:", | |
| blob.url, | |
| ); | |
| // Update database with new blob URL | |
| const [updatedOrg] = await db | |
| .update(organizations) | |
| .set({ logo: blob.url }) | |
| .where(eq(organizations.id, input.organizationId)) | |
| .returning(); | |
| console.log("[organization/uploadLogo] COMPLETE"); | |
| return { | |
| success: true, | |
| url: blob.url, | |
| organization: updatedOrg, | |
| }; | |
| } catch (error) { | |
| console.error("[organization/uploadLogo] Upload failed:", error); | |
| throw new TRPCError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: "Failed to upload logo", | |
| }); | |
| } | |
| }), |
🤖 Prompt for AI Agents
In `@packages/trpc/src/router/organization/organization.ts` around lines 152 -
275, The uploadLogo mutation accepts unvalidated input.fileName, input.mimeType
and decodes fileData before checking size; fix by adding allowlists for
acceptable MIME types (e.g., image/png, image/jpeg, image/webp) and file
extensions (png, jpg, jpeg, webp) and reject mismatches in uploadLogo, estimate
decoded size from the base64 string length (use the base64 length -> decoded
bytes ≈ (len * 3/4) - padding) before calling Buffer.from to enforce the 4.5MB
limit and return a BAD_REQUEST if exceeded, and only decode to Buffer and call
put(pathname, buffer, ...) after these validations succeed; also validate that
the file extension inferred from input.fileName corresponds to the allowed MIME
types to avoid spoofing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/window.ts`:
- Around line 75-85: Wrap the file read and data URL construction in a try/catch
around the calls to fs.readFile and subsequent processing (the block that
produces buffer, ext, mimeType, base64 and dataUrl); on error, do not let the
exception propagate raw — either throw a TRPCError (e.g., new TRPCError({ code:
"INTERNAL", message: "Failed to read selected file", cause: err })) or return a
structured error response (e.g., { canceled: true, error: err.message }) and
ensure the original error is preserved/logged so callers can diagnose the
failure.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/organization/components/SlugDialog/SlugDialog.tsx`:
- Around line 119-123: The toast messages in SlugDialog.tsx use the term "URL"
but should say "slug" for consistency; update the success toast in the try block
(toast.success("Organization URL updated!")) to something like "Organization
slug updated!" and update the error fallback message in the catch (currently
"Failed to update URL") to "Failed to update slug" and ensure the error variable
handling (error instanceof Error ? error.message : ...) remains unchanged so
only the user-facing term is replaced.
🧹 Nitpick comments (5)
apps/desktop/src/lib/trpc/routers/window.ts (2)
76-77: Consider using static imports for Node.js modules.Since this router runs exclusively in the main process (not renderer), dynamic imports for
node:fs/promisesandnode:pathare unnecessary and add overhead. Static imports at the top of the file would be cleaner.Suggested change
Add at the top of the file:
import fs from "node:fs/promises"; import path from "node:path";Then replace lines 76-77:
- const fs = await import("node:fs/promises"); - const path = await import("node:path"); const filePath = result.filePaths[0];
60-69: Consider adding a file size limit.Large image files could consume significant memory when read into a base64 string. Consider validating file size before reading, or documenting the expected size constraints.
const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB const stats = await fs.stat(filePath); if (stats.size > MAX_FILE_SIZE) { return { canceled: true, dataUrl: null, error: "File too large" }; }apps/desktop/src/renderer/routes/_authenticated/settings/organization/components/SlugDialog/SlugDialog.tsx (1)
73-103: Potential state update on unmounted component.If the dialog closes after the debounce timer fires but before the async
checkSlugcompletes, state updates (setIsCheckingSlug,setSlugAvailable) may target an unmounted component. Consider using an abort flag or theAbortControllerpattern.Suggested improvement
useEffect(() => { if (!open) return; + let cancelled = false; const timer = setTimeout(async () => { if (slugValue === currentSlug) { setSlugAvailable(null); return; } if (!slugValue || slugValue.length < 3) { setSlugAvailable(null); return; } setIsCheckingSlug(true); try { const result = await authClient.organization.checkSlug({ slug: slugValue, }); - - setSlugAvailable(result.data?.status ?? null); + if (!cancelled) { + setSlugAvailable(result.data?.status ?? null); + } } catch (error) { console.error("[slug-dialog] 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, currentSlug, open]);apps/desktop/src/renderer/routes/_authenticated/settings/organization/page.tsx (2)
37-42: Dependency array uses optional chaining on potentially undefined object.Using
organization?.idin the dependency array means the effect runs whenorganizationtransitions fromundefinedto an object. However, accessingorganization.nameandorganization.logoinside could theoretically cause issues if there's a re-render timing edge case. The current implementation with the early return on line 39 mitigates this, but consider usingorganizationas the dependency for clarity.The current implementation is safe due to the early return, but an alternative approach:
// biome-ignore lint/correctness/useExhaustiveDependencies: intentionally only sync on organization change useEffect(() => { if (!organization) return; setNameValue(organization.name); setLogoPreview(organization.logo ?? null); - }, [organization?.id]); + }, [organization]);This would also re-sync if name/logo change server-side, which may or may not be desired.
162-171: Add accessible label for logo upload button.The button lacks an accessible name for screen readers. Consider adding
aria-labelfor better accessibility.<button type="button" onClick={handleLogoUpload} className="relative w-8 h-8 cursor-pointer group" + aria-label="Change organization logo" >
Comment on lines
+75
to
+85
| // Read the file and convert to base64 data URL | ||
| const fs = await import("node:fs/promises"); | ||
| const path = await import("node:path"); | ||
| const filePath = result.filePaths[0]; | ||
| const buffer = await fs.readFile(filePath); | ||
| const ext = path.extname(filePath).slice(1).toLowerCase(); | ||
| const mimeType = ext === "jpg" ? "jpeg" : ext; | ||
| const base64 = buffer.toString("base64"); | ||
| const dataUrl = `data:image/${mimeType};base64,${base64}`; | ||
|
|
||
| return { canceled: false, dataUrl }; |
Contributor
There was a problem hiding this comment.
Missing error handling for file read operations.
If fs.readFile fails (e.g., file deleted between selection and read, permission denied), the error propagates unhandled. Per guidelines, errors should not be swallowed silently.
Proposed fix with error handling
// Read the file and convert to base64 data URL
const fs = await import("node:fs/promises");
const path = await import("node:path");
const filePath = result.filePaths[0];
- const buffer = await fs.readFile(filePath);
- const ext = path.extname(filePath).slice(1).toLowerCase();
- const mimeType = ext === "jpg" ? "jpeg" : ext;
- const base64 = buffer.toString("base64");
- const dataUrl = `data:image/${mimeType};base64,${base64}`;
-
- return { canceled: false, dataUrl };
+ try {
+ const buffer = await fs.readFile(filePath);
+ const ext = path.extname(filePath).slice(1).toLowerCase();
+ const mimeType = ext === "jpg" ? "jpeg" : ext;
+ const base64 = buffer.toString("base64");
+ const dataUrl = `data:image/${mimeType};base64,${base64}`;
+ return { canceled: false, dataUrl };
+ } catch (error) {
+ console.error("[window/selectImageFile] Failed to read file:", error);
+ return { canceled: true, dataUrl: null };
+ }📝 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.
Suggested change
| // Read the file and convert to base64 data URL | |
| const fs = await import("node:fs/promises"); | |
| const path = await import("node:path"); | |
| const filePath = result.filePaths[0]; | |
| const buffer = await fs.readFile(filePath); | |
| const ext = path.extname(filePath).slice(1).toLowerCase(); | |
| const mimeType = ext === "jpg" ? "jpeg" : ext; | |
| const base64 = buffer.toString("base64"); | |
| const dataUrl = `data:image/${mimeType};base64,${base64}`; | |
| return { canceled: false, dataUrl }; | |
| // Read the file and convert to base64 data URL | |
| const fs = await import("node:fs/promises"); | |
| const path = await import("node:path"); | |
| const filePath = result.filePaths[0]; | |
| try { | |
| const buffer = await fs.readFile(filePath); | |
| const ext = path.extname(filePath).slice(1).toLowerCase(); | |
| const mimeType = ext === "jpg" ? "jpeg" : ext; | |
| const base64 = buffer.toString("base64"); | |
| const dataUrl = `data:image/${mimeType};base64,${base64}`; | |
| return { canceled: false, dataUrl }; | |
| } catch (error) { | |
| console.error("[window/selectImageFile] Failed to read file:", error); | |
| return { canceled: true, dataUrl: null }; | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/window.ts` around lines 75 - 85, Wrap the
file read and data URL construction in a try/catch around the calls to
fs.readFile and subsequent processing (the block that produces buffer, ext,
mimeType, base64 and dataUrl); on error, do not let the exception propagate raw
— either throw a TRPCError (e.g., new TRPCError({ code: "INTERNAL", message:
"Failed to read selected file", cause: err })) or return a structured error
response (e.g., { canceled: true, error: err.message }) and ensure the original
error is preserved/logged so callers can diagnose the failure.
Comment on lines
+119
to
+123
| toast.success("Organization URL updated!"); | ||
| } catch (error) { | ||
| const message = | ||
| error instanceof Error ? error.message : "Failed to update URL"; | ||
| toast.error(message); |
Contributor
There was a problem hiding this comment.
Inconsistent terminology: "URL" should be "slug".
The PR objectives state the label was changed from "URL" to "Slug", but the toast messages still reference "URL". This creates inconsistent user-facing messaging.
Suggested fix
- toast.success("Organization URL updated!");
+ toast.success("Organization slug updated!");
} catch (error) {
const message =
- error instanceof Error ? error.message : "Failed to update URL";
+ error instanceof Error ? error.message : "Failed to update slug";
toast.error(message);📝 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.
Suggested change
| toast.success("Organization URL updated!"); | |
| } catch (error) { | |
| const message = | |
| error instanceof Error ? error.message : "Failed to update URL"; | |
| toast.error(message); | |
| toast.success("Organization slug updated!"); | |
| } catch (error) { | |
| const message = | |
| error instanceof Error ? error.message : "Failed to update slug"; | |
| toast.error(message); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/organization/components/SlugDialog/SlugDialog.tsx`
around lines 119 - 123, The toast messages in SlugDialog.tsx use the term "URL"
but should say "slug" for consistency; update the success toast in the try block
(toast.success("Organization URL updated!")) to something like "Organization
slug updated!" and update the error fallback message in the catch (currently
"Failed to update URL") to "Failed to update slug" and ensure the error variable
handling (error instanceof Error ? error.message : ...) remains unchanged so
only the user-facing term is replaced.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/trpc/src/router/organization/organization.ts`:
- Around line 191-196: The empty catch around the deletion of the old logo (the
del(organization.logo) call when organization.logo is truthy) swallows
errors—change it to catch the error (e.g., catch (err)) and log the failure with
context (include organization id or name and the logo key/path and the error).
Use the module's existing logger (e.g., ctx.logger or processLogger) to emit a
descriptive error message so del and organization.logo failures are observable
and debuggable.
♻️ Duplicate comments (1)
packages/trpc/src/router/organization/organization.ts (1)
199-222: Reject oversized uploads before decoding base64 to avoid memory spikes.
Buffer.fromruns before any size guard, so a very large payload can allocate huge memory and crash the process. Estimate decoded size from the base64 length first, then decode only if it’s within the limit. Also hoist magic values to module-level constants. As per coding guidelines.🛠️ Suggested fix
+const MAX_LOGO_BYTES = 4.5 * 1024 * 1024; +const ALLOWED_MIME_TYPES = new Set(["image/png", "image/jpeg", "image/webp"]); + const allowedMimeTypes = ["image/png", "image/jpeg", "image/webp"]; - if (!allowedMimeTypes.includes(input.mimeType)) { + if (!ALLOWED_MIME_TYPES.has(input.mimeType)) { throw new TRPCError({ code: "BAD_REQUEST", message: "Invalid image type. Only PNG, JPEG, and WebP are allowed", }); } const ext = input.mimeType.split("/")[1]?.replace("jpeg", "jpg") || "png"; const randomId = Math.random().toString(36).substring(2, 15); const pathname = `organization/${input.organizationId}/logo/${randomId}.${ext}`; const base64Data = input.fileData.includes("base64,") ? input.fileData.split("base64,")[1] || input.fileData : input.fileData; + const padding = + base64Data.endsWith("==") ? 2 : base64Data.endsWith("=") ? 1 : 0; + const estimatedBytes = Math.floor((base64Data.length * 3) / 4) - padding; + if (estimatedBytes > MAX_LOGO_BYTES) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: `File too large (${(estimatedBytes / (1024 * 1024)).toFixed(2)}MB). Maximum size is ${(MAX_LOGO_BYTES / (1024 * 1024)).toFixed(2)}MB`, + }); + } const buffer = Buffer.from(base64Data, "base64"); const sizeInMB = buffer.length / (1024 * 1024); - if (sizeInMB > 4.5) { + if (buffer.length > MAX_LOGO_BYTES) { throw new TRPCError({ code: "BAD_REQUEST", - message: `File too large (${sizeInMB.toFixed(2)}MB). Maximum size is 4.5MB`, + message: `File too large (${sizeInMB.toFixed(2)}MB). Maximum size is ${(MAX_LOGO_BYTES / (1024 * 1024)).toFixed(2)}MB`, }); }
Comment on lines
+191
to
+196
| if (organization.logo) { | ||
| try { | ||
| await del(organization.logo); | ||
| } catch { | ||
| // Old logo doesn't exist or isn't in blob storage - that's fine | ||
| } |
Contributor
There was a problem hiding this comment.
Log failures when deleting the previous logo.
The empty catch swallows errors; please log with context so deletion issues are observable and debuggable. As per coding guidelines.
🛠️ Suggested fix
if (organization.logo) {
try {
await del(organization.logo);
- } catch {
- // Old logo doesn't exist or isn't in blob storage - that's fine
+ } catch (error) {
+ console.warn(
+ "[organization/uploadLogo] Failed to delete previous logo",
+ {
+ organizationId: input.organizationId,
+ logoUrl: organization.logo,
+ error,
+ },
+ );
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (organization.logo) { | |
| try { | |
| await del(organization.logo); | |
| } catch { | |
| // Old logo doesn't exist or isn't in blob storage - that's fine | |
| } | |
| if (organization.logo) { | |
| try { | |
| await del(organization.logo); | |
| } catch (error) { | |
| console.warn( | |
| "[organization/uploadLogo] Failed to delete previous logo", | |
| { | |
| organizationId: input.organizationId, | |
| logoUrl: organization.logo, | |
| error, | |
| }, | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/trpc/src/router/organization/organization.ts` around lines 191 -
196, The empty catch around the deletion of the old logo (the
del(organization.logo) call when organization.logo is truthy) swallows
errors—change it to catch the error (e.g., catch (err)) and log the failure with
context (include organization id or name and the logo key/path and the error).
Use the module's existing logger (e.g., ctx.logger or processLogger) to emit a
descriptive error message so del and organization.logo failures are observable
and debuggable.
…o members - Rename /settings/team route to /settings/members - Change "URL" label to "Slug" in organization settings - Remove "app.superset.sh/" prefix from slug display - Extract OrganizationLogo and EditIcon into proper subcomponents per repo guidelines - Remove unnecessary console.log statements - Update navigation links in settings sidebar and organization dropdown
- Remove GIF support from organization logo upload - Replace custom EditIcon with HiOutlinePencil from react-icons/hi2 - Extract SlugDialog into separate component following repo guidelines - Clean up organization settings page imports and state
…eneration - Validate mimeType against allowed types (png, jpeg, webp) - Derive file extension from mimeType instead of unreliable fileName - Simplify pathname to use only randomId (removed redundant timestamp) - Use del() directly with full URL instead of manual path extraction
saddlepaddle
force-pushed
the
organization
branch
from
717c239 to
7d43c76
Compare
- Add customSessionClient<typeof auth> to packages/auth/src/client.ts - Enables type-safe access to session.session.role in web app - Session structure: session.session.role contains user's organization role
saddlepaddle
force-pushed
the
organization
branch
from
c48f29f to
7bc6272
Compare
…lugin Move activeOrganizationId initialization from session create hook into customSession plugin. This consolidates all organization/role logic in one place and simplifies the codebase. Changes: - Remove session create hook that set activeOrganizationId - Update customSession to find and persist activeOrganizationId when missing - Add rootDir to auth package tsconfig to resolve export map ambiguity
Consolidate three return statements into one by: - Conditionally querying membership (by active org or first membership) - Conditionally updating DB if activeOrganizationId needs to be set - Single return statement with computed values This makes the logic easier to follow and maintain.
The effect accesses organization.name and organization.logo but only depended on organization?.id. This meant updates to name/logo wouldn't trigger a re-render. Changed to depend on the full organization object so the effect runs when: - Switching organizations (ID changes) - Current organization data changes (name/logo updated)
This was referenced Jan 20, 2026
5 tasks
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/settings/teamroute to/settings/membersfor clearer terminologyapp.superset.sh/prefix from slug display (shows just the slug value now)Changes
Route Migration
team/directory tomembers//_authenticated/settings/team/to/_authenticated/settings/members/TeamSettingsPagetoMembersSettingsPageTeamMembertoMemberOrganization Settings Improvements
app.superset.sh/prefix from slug input displayOrganizationLogocomponent tocomponents/OrganizationLogo/EditIconcomponent tocomponents/EditIcon/Test plan
/settings/membersSummary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.