web: use icon keys instead of urls #1259
Conversation
WalkthroughStore image references as S3 object keys (not full URLs); add GetSignedImageUrl RPC + hook and SignedImageUrl component; resolve signed URLs on demand; attempt safe deletion of previous S3 objects on replace; propagate key-based image fields across frontend types/UIs; improve FileInput/ProfileImage preview lifecycle. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Web Frontend
participant Backend as RPC Backend
participant S3
User->>Frontend: Upload new image
Frontend->>Frontend: derive old image key (if any)
alt old key found and allowed prefix
Frontend->>S3: deleteObject (via backend/Eff, errors logged)
S3-->>Frontend: delete result
end
Frontend->>S3: putObject -> returns fileKey
Frontend->>Backend: persist fileKey in DB
Backend-->>Frontend: success
User->>Frontend: Render page with image key
Frontend->>Backend: GetSignedImageUrl(key,type)
Backend->>S3: generate signed GET URL
S3-->>Backend: signed URL
Backend-->>Frontend: { url }
Frontend->>User: display image via signed URL (SignedImageUrl)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (5)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/web/**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/web/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/web/app/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/web/actions/organization/create-space.ts (1)
121-121: Remove or revise the description field.The description field exposes the internal S3 key structure to users, which is an implementation detail and not user-friendly. Consider either removing this field entirely or providing a more meaningful description.
Apply this diff to remove the implementation detail:
- description: iconUrl ? `Space with custom icon: ${iconUrl}` : null, + description: null,Or provide a more user-friendly description:
- description: iconUrl ? `Space with custom icon: ${iconUrl}` : null, + description: iconUrl ? "Space with custom icon" : null,apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (1)
394-401: Duplicate “loadedmetadata” listener registration causes double handlers.You add handleLoadedMetadataWithTracks twice, leading to duplicated side effects.
Apply this diff to remove the duplicate add:
- video.addEventListener("loadedmetadata", handleLoadedMetadataWithTracks); video.addEventListener("load", handleLoad); video.addEventListener("play", handlePlay); video.addEventListener("error", handleError as EventListener); - video.addEventListener("loadedmetadata", handleLoadedMetadataWithTracks); + video.addEventListener("loadedmetadata", handleLoadedMetadataWithTracks);Optional cleanup: also drop one of the duplicated removeEventListener calls for "loadedmetadata" in the cleanup blocks to keep symmetry.
apps/web/app/(org)/dashboard/_components/Navbar/SpaceDialog.tsx (1)
185-193: Fix: Editing a space with no icon change will remove the icon
selectedFilestarts asnull. In edit mode, the code sendsremoveIcon=truewheneverselectedFile === null && space.iconUrl, which is true even if the user didn’t touch the icon. Track an explicit “changed” flag.- const [selectedFile, setSelectedFile] = useState<File | null>(null); + const [selectedFile, setSelectedFile] = useState<File | null>(null); + const [iconChanged, setIconChanged] = useState(false); @@ - <FileInput + <FileInput id="space-icon" name="icon" type="organization" initialPreviewUrl={space?.iconUrl || null} notDraggingClassName="hover:bg-gray-3" - onChange={setSelectedFile} + onChange={(f) => { + setSelectedFile(f); + setIconChanged(true); + }} disabled={isUploading} isLoading={isUploading} /> @@ if (edit && space?.id) { formData.append("id", space.id); // If the user removed the icon, send a removeIcon flag - if (selectedFile === null && space.iconUrl) { + if (iconChanged && selectedFile === null && space.iconUrl) { formData.append("removeIcon", "true"); } await updateSpace(formData);Also remove unused imports:
-import { useEffect, useId, useRef, useState } from "react"; +import { useEffect, useRef, useState } from "react"; @@ -import { SignedImageUrl } from "@/components/SignedImageUrl";Also applies to: 156-158, 277-287, 21-21, 27-27
apps/web/app/(org)/dashboard/settings/account/components/ProfileImage.tsx (1)
48-55: Align client validation with server (3MB, JPEG/PNG only) and remove SVG from acceptServer allows PNG/JPEG up to 3MB. Here it’s 1MB and the input accepts SVG with no type check, causing UX mismatch and unnecessary errors.
Apply these diffs:
const handleFileChange = () => { const file = fileInputRef.current?.files?.[0]; if (!file) return; - const sizeLimit = 1024 * 1024 * 1; - if (file.size > sizeLimit) { - toast.error("File size must be 1MB or less"); - return; - } + // Validate type and size (match server) + const allowedTypes = new Set(["image/jpeg", "image/png", "image/jpg"]); + const normalizedType = file.type.toLowerCase(); + if (!allowedTypes.has(normalizedType)) { + toast.error("Please select a PNG or JPEG image"); + if (fileInputRef.current) fileInputRef.current.value = ""; + return; + } + const sizeLimit = 3 * 1024 * 1024; // 3MB + if (file.size > sizeLimit) { + toast.error("File size must be 3MB or less"); + if (fileInputRef.current) fileInputRef.current.value = ""; + return; + }- accept="image/jpeg, image/jpg, image/png, image/svg+xml" + accept="image/jpeg, image/jpg, image/png"Also applies to: 100-106, 111-112
🧹 Nitpick comments (21)
apps/web/app/(org)/dashboard/dashboard-data.ts (1)
50-50: Optional: Remove redundant field selection.The
iconUrlfield is already included in theorganization: organizationsselection on line 47. The separate projection is unused (line 207 accesses it viaactiveOrgInfo.organization.iconUrlinstead).- iconUrl: organizations.iconUrl,Note: Line 48 follows a similar pattern with
settings. If that's intentional for consistency, you may keep both.apps/web/actions/organization/create-space.ts (1)
68-68: Consider renamingiconUrltoiconKeyoriconUrlOrKeyfor clarity.The variable
iconUrlnow stores an S3 key rather than a URL. For consistency with the broader refactor mentioned in the AI summary (where fields are being renamed to*UrlOrKeypatterns), consider renaming this variable to better reflect its current purpose.This would improve code clarity and align with the naming conventions used elsewhere in this PR:
- let iconUrl = null; + let iconKey = null;And update all references accordingly (lines 102, 120, 187). Note: If the database schema column
spaces.iconUrlhasn't been renamed, you may want to keep the variable name aligned with the column name, or rename the column as well.Also applies to: 102-102, 120-120, 187-187
apps/web/app/(org)/dashboard/spaces/[spaceId]/loading.tsx (1)
30-30: Unnecessary but harmless key conversion.Converting numeric keys to strings is unnecessary—React accepts both numbers and strings as keys. This change has no functional impact.
Also applies to: 43-43
apps/web/app/(org)/dashboard/spaces/browse/loading.tsx (1)
78-78: Unnecessary but harmless key conversion.Converting numeric keys to strings is unnecessary—React accepts both numbers and strings as keys. This change has no functional impact.
apps/web/app/(org)/dashboard/caps/loading.tsx (1)
30-30: Unnecessary but harmless key conversion.Converting numeric keys to strings is unnecessary—React accepts both numbers and strings as keys. This change has no functional impact.
Also applies to: 43-43
packages/ui/src/components/Avatar.tsx (1)
84-84: Class reordering has no functional impact.The Tailwind class order change from
"rounded-full object-cover size-4"to"object-cover rounded-full size-4"is cosmetic and doesn't affect rendering.apps/web/app/(org)/dashboard/_components/Notifications/Skeleton.tsx (1)
62-62: Unnecessary but harmless key conversion.Converting numeric keys to strings is unnecessary—React accepts both numbers and strings as keys. This change has no functional impact.
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (1)
238-253: Redundant manual refetch; queryKey already drives fetch.Since queryKey includes videoSrc, TanStack Query will refetch automatically on source change. Calling resolvedSrc.refetch() here is unnecessary.
Apply this diff:
- useEffect(() => { - resolvedSrc.refetch(); + useEffect(() => { setVideoLoaded(false); setHasError(false); isRetryingRef.current = false; setIsRetrying(false); retryCount.current = 0; startTime.current = Date.now();As per coding guidelines.
apps/web/app/(org)/dashboard/spaces/browse/page.tsx (1)
3-3: Minor: remove unused Avatar import.Avatar isn’t used after the switch to SignedImageUrl; let Biome drop it.
apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (1)
1-11: Minor: Avatar import appears unused.Now that cards use SignedImageUrl, drop Avatar to satisfy lint.
apps/web/lib/use-signed-image-url.ts (1)
15-27: Avoid RPC for direct URLs; degrade gracefully on any error.Currently any non-empty key triggers GetSignedImageUrl, even for http(s) URLs, causing needless RPCs and noisy errors. Short-circuit when not an S3 key and catch all failures to return null.
Apply this diff:
export function useSignedImageUrl( key: string | null | undefined, type: "user" | "organization", ) { - return useEffectQuery({ + const isS3Key = (k: string | null | undefined) => + !!k && (k.startsWith("users/") || k.startsWith("organizations/")); + + return useEffectQuery({ queryKey: ["signedImageUrl", key, type], queryFn: () => { - if (!key) { - return Effect.succeed(key); - } - - return withRpc((rpc) => rpc.GetSignedImageUrl({ key, type })) - .pipe(Effect.map((result) => result.url)) - .pipe(Effect.catchTag("InternalError", () => Effect.succeed(null))); + if (!key) return Effect.succeed(null); + if (!isS3Key(key)) return Effect.succeed(key); // direct URL passthrough + return withRpc((rpc) => rpc.GetSignedImageUrl({ key, type })) + .pipe(Effect.map((result) => result.url)) + .pipe(Effect.catchAll(() => Effect.succeed(null))); }, - enabled: !!key, + enabled: !!key, // compute but RPC only runs for S3 keys }); }This keeps the hook inside EffectRuntime, avoids extra network work, and ensures avatars don’t break on transient RPC errors. As per coding guidelines.
apps/web/components/SignedImageUrl.tsx (2)
20-35: Avoid unnecessary RPC calls for non‑S3 URLs
useSignedImageUrlruns even for direct http(s) URLs. ComputeisS3Keyfirst and passnullwhen not an S3 key to disable the query.export function SignedImageUrl({ image, name, type, className, letterClass, }: SignedImageUrlProps) { - const { data: signedUrl, isLoading } = useSignedImageUrl(image, type); - - function isS3Key(imageKeyOrUrl: string | null | undefined): boolean { + function isS3Key(imageKeyOrUrl: string | null | undefined): boolean { if (!imageKeyOrUrl) return false; return ( imageKeyOrUrl.startsWith("users/") || imageKeyOrUrl.startsWith("organizations/") ); } + const shouldSign = isS3Key(image); + const { data: signedUrl, isLoading } = useSignedImageUrl( + shouldSign ? image : null, + type, + );Also applies to: 27-35
6-13: Remove or implementnoFallbackprop
noFallbackis declared but unused. Delete it or implement behavior (e.g., hide letter avatar). Keeping unused props degrades API clarity.interface SignedImageUrlProps { image: string | null | undefined; name: string; type: "user" | "organization"; className?: string; letterClass?: string; - noFallback?: boolean; }apps/web/actions/organization/upload-space-icon.ts (2)
6-6: Remove unused import
serverEnvis not used.-import { serverEnv } from "@cap/env";
55-60: Derive extension from MIME type with fallbackUsing
file.name.split(".").pop()can be undefined or incorrect. Prefer a MIME→extension map and lowercase.- // Prepare new file key - const fileExtension = file.name.split(".").pop(); - const fileKey = `organizations/${ - space.organizationId - }/spaces/${spaceId}/icon-${Date.now()}.${fileExtension}`; + // Prepare new file key + const extByMime: Record<string, string> = { + "image/png": "png", + "image/jpeg": "jpg", + "image/webp": "webp", + "image/gif": "gif", + "image/svg+xml": "svg", + "image/avif": "avif", + }; + const fileExtension = + extByMime[file.type] ?? file.name.split(".").pop()?.toLowerCase() ?? "bin"; + const fileKey = `organizations/${space.organizationId}/spaces/${spaceId}/icon-${Date.now()}.${fileExtension}`;apps/web/app/s/[videoId]/page.tsx (2)
270-276: Remove redundantownerImageselectionYou now use
ownerImageUrlOrKey. DropownerImagefrom the projection to reduce payload and avoid confusion.- ownerName: users.name, - ownerImage: users.image, - ownerImageUrlOrKey: users.image, + ownerName: users.name, + ownerImageUrlOrKey: users.image,
472-476: Consistent rename in refresh queryGreat to keep fields consistent during refresh. Consider removing the legacy
ownerImageeverywhere to avoid dual shapes creeping back in.apps/web/actions/account/upload-profile-image.ts (1)
83-84: Use sanitized file’s content type when uploadingIf sanitizeFile ever alters the MIME, we should upload with sanitizedFile.type, not file.type.
- yield* bucket.putObject(fileKey, bodyBytes, { - contentType: file.type, - }); + yield* bucket.putObject(fileKey, bodyBytes, { + contentType: sanitizedFile.type, + });apps/web/components/FileInput.tsx (3)
254-264: Avoid empty img src while signed URL is loadingWhen previewUrl is a key, signedUrl can be null initially, yielding src="". Render the image only once signedUrl is available; otherwise show a tiny placeholder.
- {previewUrl && ( - <img - src={ - isS3Key(previewUrl) ? (signedUrl ?? "") : previewUrl - } - alt="File preview" - width={32} - height={32} - loading="eager" - referrerPolicy="no-referrer" - className="object-cover rounded-full size-8" - /> - )} + {previewUrl && + (!isS3Key(previewUrl) || signedUrl ? ( + <img + src={isS3Key(previewUrl) ? (signedUrl as string) : previewUrl} + alt="File preview" + width={32} + height={32} + loading="eager" + referrerPolicy="no-referrer" + className="object-cover rounded-full size-8" + /> + ) : ( + <div className="rounded-full bg-gray-5 animate-pulse size-8" /> + ))}
17-20: Include "image/jpg" in allowed types (minor UX polish)Some browsers report JPG as image/jpg; adding it reduces false negatives.
-const ALLOWED_IMAGE_TYPES = new Set(["image/jpeg", "image/png"]); +const ALLOWED_IMAGE_TYPES = new Set(["image/jpeg", "image/png", "image/jpg"]);
64-70: DRY: centralize isS3Key helperThis logic exists here and in SignedImageUrl. Consider a shared util (e.g., lib/images/is-s3-key.ts) to avoid drift.
Happy to extract a shared helper and update call sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
apps/web/actions/account/remove-profile-image.ts(2 hunks)apps/web/actions/account/upload-profile-image.ts(2 hunks)apps/web/actions/organization/create-space.ts(1 hunks)apps/web/actions/organization/remove-icon.ts(2 hunks)apps/web/actions/organization/update-space.ts(1 hunks)apps/web/actions/organization/upload-organization-icon.ts(1 hunks)apps/web/actions/organization/upload-space-icon.ts(2 hunks)apps/web/actions/videos/new-comment.ts(1 hunks)apps/web/app/(org)/dashboard/_components/MobileTab.tsx(3 hunks)apps/web/app/(org)/dashboard/_components/Navbar/Items.tsx(3 hunks)apps/web/app/(org)/dashboard/_components/Navbar/SpaceDialog.tsx(5 hunks)apps/web/app/(org)/dashboard/_components/Navbar/SpacesList.tsx(4 hunks)apps/web/app/(org)/dashboard/_components/Navbar/Top.tsx(3 hunks)apps/web/app/(org)/dashboard/_components/Notifications/Skeleton.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx(3 hunks)apps/web/app/(org)/dashboard/caps/loading.tsx(2 hunks)apps/web/app/(org)/dashboard/dashboard-data.ts(3 hunks)apps/web/app/(org)/dashboard/folder/[id]/components/ClientMyCapsLink.tsx(2 hunks)apps/web/app/(org)/dashboard/settings/account/Settings.tsx(1 hunks)apps/web/app/(org)/dashboard/settings/account/components/ProfileImage.tsx(5 hunks)apps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsx(5 hunks)apps/web/app/(org)/dashboard/settings/organization/page.tsx(3 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsx(1 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/loading.tsx(2 hunks)apps/web/app/(org)/dashboard/spaces/browse/loading.tsx(1 hunks)apps/web/app/(org)/dashboard/spaces/browse/page.tsx(2 hunks)apps/web/app/(org)/onboarding/components/Bottom.tsx(1 hunks)apps/web/app/s/[videoId]/Share.tsx(1 hunks)apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx(1 hunks)apps/web/app/s/[videoId]/_components/ShareHeader.tsx(3 hunks)apps/web/app/s/[videoId]/_components/ShareVideo.tsx(2 hunks)apps/web/app/s/[videoId]/_components/Toolbar.tsx(2 hunks)apps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsx(1 hunks)apps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsx(2 hunks)apps/web/app/s/[videoId]/page.tsx(4 hunks)apps/web/components/FileInput.tsx(7 hunks)apps/web/components/SignedImageUrl.tsx(1 hunks)apps/web/components/Tooltip.tsx(1 hunks)apps/web/components/forms/NewOrganization.tsx(1 hunks)apps/web/components/forms/server.ts(1 hunks)apps/web/lib/use-signed-image-url.ts(1 hunks)packages/ui/src/components/Avatar.tsx(2 hunks)packages/web-backend/src/Users/UsersRpcs.ts(2 hunks)packages/web-domain/src/User.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsxapps/web/components/forms/server.tsapps/web/actions/videos/new-comment.tsapps/web/app/(org)/dashboard/spaces/browse/page.tsxapps/web/app/(org)/onboarding/components/Bottom.tsxapps/web/app/(org)/dashboard/settings/account/Settings.tsxapps/web/app/s/[videoId]/Share.tsxapps/web/app/(org)/dashboard/_components/Notifications/Skeleton.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/actions/organization/upload-space-icon.tsapps/web/app/s/[videoId]/_components/Toolbar.tsxapps/web/actions/organization/create-space.tsapps/web/app/s/[videoId]/page.tsxapps/web/app/(org)/dashboard/_components/Navbar/Top.tsxpackages/web-domain/src/User.tspackages/web-backend/src/Users/UsersRpcs.tsapps/web/app/(org)/dashboard/dashboard-data.tsapps/web/actions/organization/upload-organization-icon.tsapps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/lib/use-signed-image-url.tsapps/web/app/(org)/dashboard/caps/loading.tsxapps/web/actions/organization/remove-icon.tsapps/web/app/(org)/dashboard/spaces/browse/loading.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxpackages/ui/src/components/Avatar.tsxapps/web/components/Tooltip.tsxapps/web/actions/organization/update-space.tsapps/web/components/forms/NewOrganization.tsxapps/web/app/(org)/dashboard/_components/MobileTab.tsxapps/web/app/(org)/dashboard/caps/components/SharingDialog.tsxapps/web/actions/account/remove-profile-image.tsapps/web/components/SignedImageUrl.tsxapps/web/app/s/[videoId]/_components/ShareHeader.tsxapps/web/actions/account/upload-profile-image.tsapps/web/app/(org)/dashboard/spaces/[spaceId]/loading.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsxapps/web/app/(org)/dashboard/settings/organization/page.tsxapps/web/app/(org)/dashboard/settings/account/components/ProfileImage.tsxapps/web/app/(org)/dashboard/_components/Navbar/SpacesList.tsxapps/web/app/(org)/dashboard/_components/Navbar/SpaceDialog.tsxapps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsxapps/web/components/FileInput.tsxapps/web/app/(org)/dashboard/folder/[id]/components/ClientMyCapsLink.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsxapps/web/components/forms/server.tsapps/web/actions/videos/new-comment.tsapps/web/app/(org)/dashboard/spaces/browse/page.tsxapps/web/app/(org)/onboarding/components/Bottom.tsxapps/web/app/(org)/dashboard/settings/account/Settings.tsxapps/web/app/s/[videoId]/Share.tsxapps/web/app/(org)/dashboard/_components/Notifications/Skeleton.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/actions/organization/upload-space-icon.tsapps/web/app/s/[videoId]/_components/Toolbar.tsxapps/web/actions/organization/create-space.tsapps/web/app/s/[videoId]/page.tsxapps/web/app/(org)/dashboard/_components/Navbar/Top.tsxpackages/web-domain/src/User.tspackages/web-backend/src/Users/UsersRpcs.tsapps/web/app/(org)/dashboard/dashboard-data.tsapps/web/actions/organization/upload-organization-icon.tsapps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/lib/use-signed-image-url.tsapps/web/app/(org)/dashboard/caps/loading.tsxapps/web/actions/organization/remove-icon.tsapps/web/app/(org)/dashboard/spaces/browse/loading.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxpackages/ui/src/components/Avatar.tsxapps/web/components/Tooltip.tsxapps/web/actions/organization/update-space.tsapps/web/components/forms/NewOrganization.tsxapps/web/app/(org)/dashboard/_components/MobileTab.tsxapps/web/app/(org)/dashboard/caps/components/SharingDialog.tsxapps/web/actions/account/remove-profile-image.tsapps/web/components/SignedImageUrl.tsxapps/web/app/s/[videoId]/_components/ShareHeader.tsxapps/web/actions/account/upload-profile-image.tsapps/web/app/(org)/dashboard/spaces/[spaceId]/loading.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsxapps/web/app/(org)/dashboard/settings/organization/page.tsxapps/web/app/(org)/dashboard/settings/account/components/ProfileImage.tsxapps/web/app/(org)/dashboard/_components/Navbar/SpacesList.tsxapps/web/app/(org)/dashboard/_components/Navbar/SpaceDialog.tsxapps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsxapps/web/components/FileInput.tsxapps/web/app/(org)/dashboard/folder/[id]/components/ClientMyCapsLink.tsx
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsxapps/web/components/forms/server.tsapps/web/actions/videos/new-comment.tsapps/web/app/(org)/dashboard/spaces/browse/page.tsxapps/web/app/(org)/onboarding/components/Bottom.tsxapps/web/app/(org)/dashboard/settings/account/Settings.tsxapps/web/app/s/[videoId]/Share.tsxapps/web/app/(org)/dashboard/_components/Notifications/Skeleton.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/actions/organization/upload-space-icon.tsapps/web/app/s/[videoId]/_components/Toolbar.tsxapps/web/actions/organization/create-space.tsapps/web/app/s/[videoId]/page.tsxapps/web/app/(org)/dashboard/_components/Navbar/Top.tsxapps/web/app/(org)/dashboard/dashboard-data.tsapps/web/actions/organization/upload-organization-icon.tsapps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/lib/use-signed-image-url.tsapps/web/app/(org)/dashboard/caps/loading.tsxapps/web/actions/organization/remove-icon.tsapps/web/app/(org)/dashboard/spaces/browse/loading.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/components/Tooltip.tsxapps/web/actions/organization/update-space.tsapps/web/components/forms/NewOrganization.tsxapps/web/app/(org)/dashboard/_components/MobileTab.tsxapps/web/app/(org)/dashboard/caps/components/SharingDialog.tsxapps/web/actions/account/remove-profile-image.tsapps/web/components/SignedImageUrl.tsxapps/web/app/s/[videoId]/_components/ShareHeader.tsxapps/web/actions/account/upload-profile-image.tsapps/web/app/(org)/dashboard/spaces/[spaceId]/loading.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsxapps/web/app/(org)/dashboard/settings/organization/page.tsxapps/web/app/(org)/dashboard/settings/account/components/ProfileImage.tsxapps/web/app/(org)/dashboard/_components/Navbar/SpacesList.tsxapps/web/app/(org)/dashboard/_components/Navbar/SpaceDialog.tsxapps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsxapps/web/components/FileInput.tsxapps/web/app/(org)/dashboard/folder/[id]/components/ClientMyCapsLink.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsxapps/web/components/forms/server.tsapps/web/actions/videos/new-comment.tsapps/web/app/(org)/dashboard/spaces/browse/page.tsxapps/web/app/(org)/onboarding/components/Bottom.tsxapps/web/app/(org)/dashboard/settings/account/Settings.tsxapps/web/app/s/[videoId]/Share.tsxapps/web/app/(org)/dashboard/_components/Notifications/Skeleton.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/actions/organization/upload-space-icon.tsapps/web/app/s/[videoId]/_components/Toolbar.tsxapps/web/actions/organization/create-space.tsapps/web/app/s/[videoId]/page.tsxapps/web/app/(org)/dashboard/_components/Navbar/Top.tsxapps/web/app/(org)/dashboard/dashboard-data.tsapps/web/actions/organization/upload-organization-icon.tsapps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/lib/use-signed-image-url.tsapps/web/app/(org)/dashboard/caps/loading.tsxapps/web/actions/organization/remove-icon.tsapps/web/app/(org)/dashboard/spaces/browse/loading.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/components/Tooltip.tsxapps/web/actions/organization/update-space.tsapps/web/components/forms/NewOrganization.tsxapps/web/app/(org)/dashboard/_components/MobileTab.tsxapps/web/app/(org)/dashboard/caps/components/SharingDialog.tsxapps/web/actions/account/remove-profile-image.tsapps/web/components/SignedImageUrl.tsxapps/web/app/s/[videoId]/_components/ShareHeader.tsxapps/web/actions/account/upload-profile-image.tsapps/web/app/(org)/dashboard/spaces/[spaceId]/loading.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsxapps/web/app/(org)/dashboard/settings/organization/page.tsxapps/web/app/(org)/dashboard/settings/account/components/ProfileImage.tsxapps/web/app/(org)/dashboard/_components/Navbar/SpacesList.tsxapps/web/app/(org)/dashboard/_components/Navbar/SpaceDialog.tsxapps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsxapps/web/components/FileInput.tsxapps/web/app/(org)/dashboard/folder/[id]/components/ClientMyCapsLink.tsx
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsxapps/web/app/(org)/dashboard/spaces/browse/page.tsxapps/web/app/(org)/onboarding/components/Bottom.tsxapps/web/app/(org)/dashboard/settings/account/Settings.tsxapps/web/app/s/[videoId]/Share.tsxapps/web/app/(org)/dashboard/_components/Notifications/Skeleton.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/s/[videoId]/_components/Toolbar.tsxapps/web/app/s/[videoId]/page.tsxapps/web/app/(org)/dashboard/_components/Navbar/Top.tsxapps/web/app/(org)/dashboard/dashboard-data.tsapps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/app/(org)/dashboard/caps/loading.tsxapps/web/app/(org)/dashboard/spaces/browse/loading.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/app/(org)/dashboard/_components/MobileTab.tsxapps/web/app/(org)/dashboard/caps/components/SharingDialog.tsxapps/web/app/s/[videoId]/_components/ShareHeader.tsxapps/web/app/(org)/dashboard/spaces/[spaceId]/loading.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsxapps/web/app/(org)/dashboard/settings/organization/page.tsxapps/web/app/(org)/dashboard/settings/account/components/ProfileImage.tsxapps/web/app/(org)/dashboard/_components/Navbar/SpacesList.tsxapps/web/app/(org)/dashboard/_components/Navbar/SpaceDialog.tsxapps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsxapps/web/app/(org)/dashboard/folder/[id]/components/ClientMyCapsLink.tsx
apps/web/actions/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All Groq/OpenAI calls must execute in Next.js Server Actions under apps/web/actions; do not invoke AI providers elsewhere
Files:
apps/web/actions/videos/new-comment.tsapps/web/actions/organization/upload-space-icon.tsapps/web/actions/organization/create-space.tsapps/web/actions/organization/upload-organization-icon.tsapps/web/actions/organization/remove-icon.tsapps/web/actions/organization/update-space.tsapps/web/actions/account/remove-profile-image.tsapps/web/actions/account/upload-profile-image.ts
packages/ui/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Component files in
packages/uishould use PascalCase naming if they define React/Solid components.
Files:
packages/ui/src/components/Avatar.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-14T10:15:44.019Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T10:15:44.019Z
Learning: Applies to apps/web/**/*.{tsx} : Styling uses Tailwind CSS only; use Next/Image for remote assets; prefer memoization and code-splitting for performance
Applied to files:
apps/web/app/(org)/dashboard/_components/Navbar/Top.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/app/(org)/dashboard/_components/Navbar/SpacesList.tsx
🧬 Code graph analysis (22)
apps/web/app/(org)/dashboard/spaces/browse/page.tsx (1)
apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(20-64)
apps/web/app/s/[videoId]/page.tsx (1)
packages/database/schema.ts (1)
users(58-117)
apps/web/app/(org)/dashboard/_components/Navbar/Top.tsx (1)
apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(20-64)
packages/web-domain/src/User.ts (2)
packages/web-domain/src/Errors.ts (1)
InternalError(3-6)packages/web-domain/src/Authentication.ts (1)
RpcAuthMiddleware(34-40)
packages/web-backend/src/Users/UsersRpcs.ts (3)
packages/web-backend/src/Users/UsersOnboarding.ts (1)
UsersOnboarding(10-318)packages/database/schema.ts (1)
s3Buckets(433-443)packages/web-domain/src/Errors.ts (1)
InternalError(3-6)
apps/web/app/(org)/dashboard/dashboard-data.ts (1)
packages/database/schema.ts (1)
organizations(170-202)
apps/web/actions/organization/upload-organization-icon.ts (3)
apps/web/lib/sanitizeFile.ts (1)
sanitizeFile(4-17)packages/web-backend/src/S3Buckets/index.ts (1)
S3Buckets(14-195)apps/web/lib/server.ts (1)
runPromise(123-135)
apps/web/lib/use-signed-image-url.ts (2)
apps/web/lib/EffectRuntime.ts (1)
useEffectQuery(22-22)apps/web/lib/Rpcs.ts (1)
withRpc(16-17)
apps/web/actions/organization/remove-icon.ts (2)
packages/web-backend/src/S3Buckets/index.ts (1)
S3Buckets(14-195)apps/web/lib/server.ts (1)
runPromise(123-135)
apps/web/app/(org)/dashboard/_components/Navbar/Items.tsx (1)
apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(20-64)
apps/web/app/(org)/dashboard/_components/MobileTab.tsx (1)
apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(20-64)
apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (1)
apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(20-64)
apps/web/actions/account/remove-profile-image.ts (1)
apps/web/lib/server.ts (1)
runPromise(123-135)
apps/web/components/SignedImageUrl.tsx (2)
apps/web/lib/use-signed-image-url.ts (1)
useSignedImageUrl(11-28)packages/ui/src/components/Avatar.tsx (1)
Avatar(73-109)
apps/web/app/s/[videoId]/_components/ShareHeader.tsx (1)
apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(20-64)
apps/web/actions/account/upload-profile-image.ts (4)
apps/web/lib/sanitizeFile.ts (1)
sanitizeFile(4-17)apps/web/lib/server.ts (1)
runPromise(123-135)packages/database/index.ts (1)
db(18-25)packages/database/schema.ts (1)
users(58-117)
apps/web/app/(org)/dashboard/settings/organization/page.tsx (1)
packages/database/schema.ts (3)
organizations(170-202)users(58-117)organizationMembers(205-227)
apps/web/app/(org)/dashboard/settings/account/components/ProfileImage.tsx (1)
apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(20-64)
apps/web/app/(org)/dashboard/_components/Navbar/SpacesList.tsx (1)
apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(20-64)
apps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsx (1)
apps/web/app/(org)/dashboard/Contexts.tsx (1)
useDashboardContext(51-51)
apps/web/components/FileInput.tsx (1)
apps/web/lib/use-signed-image-url.ts (1)
useSignedImageUrl(11-28)
apps/web/app/(org)/dashboard/folder/[id]/components/ClientMyCapsLink.tsx (1)
apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(20-64)
🪛 GitHub Check: CodeQL
apps/web/actions/organization/upload-organization-icon.ts
[failure] 72-72: Incomplete URL substring sanitization
'amazonaws.com' can be anywhere in the URL, and arbitrary hosts may come before or after it.
apps/web/actions/organization/remove-icon.ts
[failure] 42-42: Incomplete URL substring sanitization
'amazonaws.com' can be anywhere in the URL, and arbitrary hosts may come before or after it.
apps/web/actions/account/remove-profile-image.ts
[failure] 26-26: Incomplete URL substring sanitization
'amazonaws.com' can be anywhere in the URL, and arbitrary hosts may come before or after it.
apps/web/actions/account/upload-profile-image.ts
[failure] 62-62: Incomplete URL substring sanitization
'amazonaws.com' can be anywhere in the URL, and arbitrary hosts may come before or after it.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (36)
apps/web/app/(org)/onboarding/components/Bottom.tsx (1)
46-46: Good fix, but unrelated to PR objectives.The change correctly binds the spinner to
skipToDashboard.isPendinginstead of a constant value, which is consistent with the button's disabled state (line 49) and text (line 53). However, this bug fix appears unrelated to the PR's stated objective of refactoring image handling to use S3 keys instead of URLs.apps/web/app/(org)/dashboard/dashboard-data.ts (1)
207-207: Good fix: Using actual organization icon.This correctly replaces the hardcoded
nullwith the actual organization'siconUrl, ensuring the "All spaces" entry displays the proper organization icon.apps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsx (2)
87-100: LGTM—field rename aligns with PR objectives.The rename from
authorImagetoauthorImageUrlOrKeyis consistent with the PR's refactoring of image handling to support both S3 keys and URLs.
130-143: LGTM—field rename aligns with PR objectives.The rename from
authorImagetoauthorImageUrlOrKeyin the reply optimistic state is consistent with the broader refactoring.apps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsx (1)
104-106: Good practice: strict equality operator.Replacing loose equality (
==) with strict equality (===) is the correct approach for string comparison and aligns with TypeScript/JavaScript best practices.apps/web/actions/organization/update-space.ts (1)
55-58: LGTM—robust key extraction for transition period.The logic correctly handles both S3 keys (starting with
"organizations/") and legacy URLs by attempting regex extraction. This approach provides a smooth migration path.apps/web/components/forms/server.ts (1)
80-80: LGTM! S3 key storage aligns with PR objectives.Storing the S3 key directly instead of constructing a full URL is the correct approach. The SignedImageUrl component will handle fetching signed URLs when needed.
apps/web/app/(org)/dashboard/_components/Navbar/SpacesList.tsx (2)
282-293: LGTM! SignedImageUrl component usage is correct.The migration from conditional Image/Avatar rendering to the SignedImageUrl component standardizes icon handling across the codebase and properly supports both S3 keys and direct URLs.
353-356: LGTM! Proper state refresh after space updates.Adding the onSpaceUpdated callback ensures the UI reflects changes after space modifications and provides good UX by closing the dialog.
apps/web/app/(org)/dashboard/_components/MobileTab.tsx (1)
78-84: LGTM! Consistent SignedImageUrl adoption.Both organization icon rendering locations correctly use the SignedImageUrl component with appropriate props and sizing for their respective contexts.
Also applies to: 138-144
apps/web/actions/videos/new-comment.ts (1)
70-70: LGTM! Field rename is consistent with PR changes.The rename from
authorImagetoauthorImageUrlOrKeyaligns with the broader refactor and clearly indicates the field can contain either an S3 key or a direct URL.apps/web/app/(org)/dashboard/settings/account/Settings.tsx (1)
177-177: LGTM! userName prop supports fallback rendering.Adding the userName prop enables the ProfileImage component to display initials when no image is available, consistent with the SignedImageUrl pattern used throughout the codebase.
apps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsx (1)
81-81: LGTM! Consistent field rename.The change from
authorImagetoauthorImageUrlOrKeyis consistent with the field rename in the new-comment action and maintains proper fallback behavior.apps/web/components/forms/NewOrganization.tsx (1)
102-102: LGTM! Type prop enables proper file input handling.Adding the
type="organization"prop allows the FileInput component to apply organization-specific handling and validation for icon uploads.apps/web/app/s/[videoId]/_components/Toolbar.tsx (1)
43-46: LGTM: optimistic comment uses url-or-key field consistently.authorImageUrlOrKey aligns with the new shape and remains nullable. No concerns.
Also applies to: 85-88
apps/web/app/(org)/dashboard/folder/[id]/components/ClientMyCapsLink.tsx (1)
127-134: Consistent icon rendering via SignedImageUrl.Correct type="organization" and prop mapping. This removes Image/Avatar branching cleanly.
apps/web/app/(org)/dashboard/spaces/browse/page.tsx (1)
135-141: LGTM: spaces now use SignedImageUrl.Consistent with the key-based image model and centralizes behavior.
apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (2)
265-286: Public toggle container tweak looks fine.Pure UI change; no logic impact.
419-425: LGTM: SignedImageUrl for space avatars.Correct props; keeps card concise and consistent with the new image flow.
apps/web/app/s/[videoId]/_components/ShareVideo.tsx (2)
30-33: Type shape updated to authorImageUrlOrKey.Matches the PR direction; no behavior change.
200-207: LGTM: propagates authorImageUrlOrKey to player.Undefined fallback is appropriate for optional prop.
apps/web/app/s/[videoId]/_components/ShareHeader.tsx (1)
239-245: ****The upstream data source is not providing only
ownerImageUrlOrKey. The database query at line 267–290 ofapps/web/app/s/[videoId]/page.tsxselects bothownerImage: users.image(line 274) andownerImageUrlOrKey: users.image(line 275) in parallel. ShareHeader correctly receives and usesdata.ownerImage, which is populated from the upstream query. No migration to an exclusiveownerImageUrlOrKeyfield has occurred, so the component will not render a blank avatar.Likely an incorrect or invalid review comment.
apps/web/app/(org)/dashboard/settings/organization/components/OrganizationIcon.tsx (2)
4-4: LGTM: Router refresh pattern for server-side data updates.The
router.refresh()calls appropriately refresh server component data after successful icon mutations, ensuring the UI reflects the updated state.Also applies to: 13-13, 34-34, 53-53
76-80: LGTM: FileInput integration with new icon handling.The addition of
type="organization"and passingexistingIconUrldirectly align with the new signed URL architecture for icon management.apps/web/actions/organization/upload-organization-icon.ts (2)
81-84: LGTM: Resilient error handling for S3 deletion.The error handling properly logs failures and continues with the upload, preventing S3 deletion issues from blocking legitimate icon updates.
95-95: LGTM: Storing S3 key instead of full URL.This change aligns with the PR's objective of storing keys instead of URLs, enabling signed URL generation on-demand.
apps/web/app/(org)/dashboard/_components/Navbar/Top.tsx (1)
103-111: LGTM: Consistent adoption of SignedImageUrl component.The migration to
SignedImageUrlwith appropriate type discriminators (type="organization"andtype="user") properly integrates with the new signed URL infrastructure while maintaining existing styling and layout.Also applies to: 264-270
apps/web/actions/account/remove-profile-image.ts (1)
38-42: LGTM: Resilient error handling.The error handling ensures that S3 deletion failures don't prevent the database update, maintaining operation continuity.
apps/web/app/(org)/dashboard/_components/Navbar/Items.tsx (1)
125-138: LGTM: Consistent SignedImageUrl integration.The component correctly adopts
SignedImageUrlfor both the organization header and list items, with propertype="organization"discriminator and dynamic sizing based on sidebar state.Also applies to: 216-222
packages/web-domain/src/User.ts (1)
71-90: LGTM: Well-structured RPC schema definition.The
GetSignedImageUrlRPC is properly defined with type-safe payloads, discriminated by a literal union fortype, and appropriately configured with authentication middleware.apps/web/app/s/[videoId]/Share.tsx (1)
26-34: LGTM: Type naming aligns with new architecture.The rename from
authorImagetoauthorImageUrlOrKeyaccurately reflects that the field now stores either S3 keys or URLs, consistent with the PR's objectives.apps/web/actions/organization/remove-icon.ts (1)
54-58: LGTM: Resilient error handling.The error handling ensures S3 deletion failures don't prevent the database update from completing.
apps/web/app/(org)/dashboard/settings/account/components/ProfileImage.tsx (1)
92-98: LGTM: using SignedImageUrl for preview is the right moveConsistent with the new key-based image flow and avoids leaking direct URLs.
apps/web/actions/account/upload-profile-image.ts (2)
48-49: Overall action flow looks solidEffect integration, key naming, DB update, and revalidatePath are consistent with the new key-based model.
102-103: No breaking changes found—the return value property is never accessed at the call site.The only call site at
apps/web/app/(org)/dashboard/settings/account/Settings.tsx:96handles the result by checkingresult.successonly (line 99). TheimageorimageUrlproperties are never accessed, so changing the return shape has no runtime impact.Likely an incorrect or invalid review comment.
apps/web/components/FileInput.tsx (1)
76-90: LGTM: robust local preview lifecycleNice job tracking/revoking blob URLs on prop changes, user actions, and unmount. This prevents leaks and flicker.
Also applies to: 95-100, 176-190, 201-213
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/web-backend/src/Users/UsersRpcs.ts (1)
42-57: Critical: Authorization and prefix validation still missing in GetSignedImageUrl RPC.The RPC receives a
typeparameter but never validates it against thekey. Any authenticated user can request signed URLs for arbitrary keys in the bucket, including other users' images or organizations they don't belong to. This is a significant security and privacy issue.Apply this minimal hardening as previously recommended:
- GetSignedImageUrl: (payload: { - key: string; - type: "user" | "organization"; - }) => + GetSignedImageUrl: (payload: { key: string; type: "user" | "organization" }) => Effect.gen(function* () { + // Basic prefix validation by declared type (tighten with ownership checks next) + const isAllowed = + (payload.type === "user" && payload.key.startsWith("users/")) || + (payload.type === "organization" && payload.key.startsWith("organizations/")); + if (!isAllowed) { + return yield* Effect.fail(new InternalError({ type: "unknown" })); + } + const [bucket] = yield* s3Buckets.getBucketAccess(Option.none()); const url = yield* bucket.getSignedObjectUrl(payload.key); return { url }; }).pipe(Additionally, enforce ownership validation:
- For
type === "user", verify thatpayload.key.startsWith(\users/${currentUser.id}/`)`.- For
type === "organization", verify the user is a member of the organization encoded in the key path.
🧹 Nitpick comments (7)
apps/web/actions/organization/upload-organization-icon.ts (2)
67-96: Good: Hostname validation now addresses the previous security issue.The URL validation correctly checks that the hostname matches S3 patterns before extracting the key, addressing the past security concern. The
startsWith("organizations/")guard provides an additional safety layer.Consider adding explicit validation against
..segments in the extracted key for defensive coding, even though S3 keys are flat strings and don't support filesystem-style traversal:oldS3Key = url.pathname.substring(1); // Remove leading slash + // Reject keys with path traversal segments + if (oldS3Key.includes("..")) { + return; + } } else { return; }The error handling that logs but doesn't fail the upload is appropriate for resilience.
106-106: LGTM: Key-based storage aligns with PR objectives.Storing the S3 key directly instead of constructing a URL matches the PR's goal to use icon keys with on-demand signed-URL retrieval.
Note: The field name
iconUrlis now semantically misleading since it stores a key rather than a URL. If not already tracked, consider renaming toiconKeyoriconUrlOrKeyin a future refactor to improve clarity.apps/web/actions/organization/remove-icon.ts (2)
57-57: Consider explicit path traversal validation.While the
startsWith("organizations/")check provides good protection, consider adding explicit validation to reject keys containing..segments for defense-in-depth.// Only delete if it looks like an organization icon key - if (s3Key.startsWith("organizations/")) { + if (s3Key.startsWith("organizations/") && !s3Key.includes("..")) { await Effect.gen(function* () {
63-66: Consider structured logging for production monitoring.Console logging may not be sufficient for production error tracking. Consider using a structured logging service or error monitoring tool.
apps/web/actions/account/remove-profile-image.ts (1)
21-52: Centralize “extract-and-validate S3 key” utility.This logic appears in multiple flows (upload/replace/remove). Factor into a shared helper (e.g., parseS3KeyFromImageRef) with tests to prevent drift.
apps/web/app/(org)/dashboard/dashboard-data.ts (2)
33-39: Add memberImage to the Spaces type (optional) for stronger typingYou’re selecting memberImage in the query but the Spaces type doesn’t declare it. Make it optional to avoid surprises for entries (like “All spaces”) that don’t include it.
export type Spaces = Omit< typeof spaces.$inferSelect, "createdAt" | "updatedAt" > & { memberCount: number; videoCount: number; + memberImage?: string | null; };
50-50: Remove redundant iconUrl alias from the organizations selectorganization: organizations already includes iconUrl. The extra top-level iconUrl duplicates data and widens the row type unnecessarily.
- iconUrl: organizations.iconUrl,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/web/actions/account/remove-profile-image.ts(2 hunks)apps/web/actions/account/upload-profile-image.ts(2 hunks)apps/web/actions/organization/create-space.ts(2 hunks)apps/web/actions/organization/remove-icon.ts(2 hunks)apps/web/actions/organization/upload-organization-icon.ts(1 hunks)apps/web/app/(org)/dashboard/dashboard-data.ts(4 hunks)apps/web/app/(org)/dashboard/settings/organization/page.tsx(2 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/components/MemberSelect.tsx(3 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersDialog.tsx(3 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersIndicator.tsx(2 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsx(3 hunks)apps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsx(2 hunks)packages/web-backend/src/Users/UsersRpcs.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsx
- apps/web/actions/organization/create-space.ts
- apps/web/actions/account/upload-profile-image.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/MemberSelect.tsxapps/web/actions/account/remove-profile-image.tsapps/web/app/(org)/dashboard/settings/organization/page.tsxapps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersDialog.tsxpackages/web-backend/src/Users/UsersRpcs.tsapps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsxapps/web/actions/organization/upload-organization-icon.tsapps/web/app/(org)/dashboard/dashboard-data.tsapps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersIndicator.tsxapps/web/actions/organization/remove-icon.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/MemberSelect.tsxapps/web/actions/account/remove-profile-image.tsapps/web/app/(org)/dashboard/settings/organization/page.tsxapps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersDialog.tsxpackages/web-backend/src/Users/UsersRpcs.tsapps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsxapps/web/actions/organization/upload-organization-icon.tsapps/web/app/(org)/dashboard/dashboard-data.tsapps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersIndicator.tsxapps/web/actions/organization/remove-icon.ts
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/MemberSelect.tsxapps/web/actions/account/remove-profile-image.tsapps/web/app/(org)/dashboard/settings/organization/page.tsxapps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersDialog.tsxapps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsxapps/web/actions/organization/upload-organization-icon.tsapps/web/app/(org)/dashboard/dashboard-data.tsapps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersIndicator.tsxapps/web/actions/organization/remove-icon.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/MemberSelect.tsxapps/web/actions/account/remove-profile-image.tsapps/web/app/(org)/dashboard/settings/organization/page.tsxapps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersDialog.tsxapps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsxapps/web/actions/organization/upload-organization-icon.tsapps/web/app/(org)/dashboard/dashboard-data.tsapps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersIndicator.tsxapps/web/actions/organization/remove-icon.ts
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/MemberSelect.tsxapps/web/app/(org)/dashboard/settings/organization/page.tsxapps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersDialog.tsxapps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsxapps/web/app/(org)/dashboard/dashboard-data.tsapps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersIndicator.tsx
apps/web/actions/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All Groq/OpenAI calls must execute in Next.js Server Actions under apps/web/actions; do not invoke AI providers elsewhere
Files:
apps/web/actions/account/remove-profile-image.tsapps/web/actions/organization/upload-organization-icon.tsapps/web/actions/organization/remove-icon.ts
🧬 Code graph analysis (9)
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/MemberSelect.tsx (1)
apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(20-64)
apps/web/actions/account/remove-profile-image.ts (1)
apps/web/lib/server.ts (1)
runPromise(123-135)
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersDialog.tsx (1)
apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(20-64)
packages/web-backend/src/Users/UsersRpcs.ts (3)
packages/web-backend/src/Users/UsersOnboarding.ts (1)
UsersOnboarding(10-318)packages/database/schema.ts (1)
s3Buckets(433-443)packages/web-domain/src/Errors.ts (1)
InternalError(3-6)
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsx (1)
apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(20-64)
apps/web/actions/organization/upload-organization-icon.ts (3)
apps/web/lib/sanitizeFile.ts (1)
sanitizeFile(4-17)packages/web-backend/src/S3Buckets/index.ts (1)
S3Buckets(14-195)apps/web/lib/server.ts (1)
runPromise(123-135)
apps/web/app/(org)/dashboard/dashboard-data.ts (1)
packages/database/schema.ts (4)
organizations(170-202)users(58-117)spaces(587-613)spaceMembers(615-636)
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersIndicator.tsx (1)
apps/web/components/SignedImageUrl.tsx (1)
SignedImageUrl(20-64)
apps/web/actions/organization/remove-icon.ts (2)
packages/web-backend/src/S3Buckets/index.ts (1)
S3Buckets(14-195)apps/web/lib/server.ts (1)
runPromise(123-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (15)
apps/web/actions/organization/upload-organization-icon.ts (1)
54-55: LGTM: Old icon retrieval for deletion.Correctly retrieves the existing icon reference before uploading the new one, enabling cleanup of stale S3 objects.
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersIndicator.tsx (1)
168-174: LGTM! SignedImageUrl integration is correct.The replacement of Avatar with SignedImageUrl is properly implemented with correct prop mapping (
image,name,type="user"), and aligns with the PR's objective to handle S3 keys and signed URLs.apps/web/app/(org)/dashboard/spaces/[spaceId]/components/MemberSelect.tsx (2)
198-204: LGTM! Dropdown item avatar rendering updated correctly.The SignedImageUrl component is properly integrated with correct props (
image,name,type="user").
222-228: LGTM! Selected member tag rendering updated correctly.The SignedImageUrl usage is consistent with the dropdown items and properly configured.
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/OrganizationIndicator.tsx (2)
75-81: LGTM! Member avatar rendering updated correctly.SignedImageUrl is properly integrated with correct props. Note that
type="user"is appropriate here since these are organization member avatars.
95-97: LGTM! Role check improved with strict equality.The change from loose to strict equality (
member.role === "owner") is a good practice and more precise.apps/web/app/(org)/dashboard/settings/organization/page.tsx (1)
41-43: LGTM! Authorization guard fixed correctly.The updated guard
if (!member || member.role !== "owner")now properly handles both cases: when the user is not a member AND when they are a member but not the owner. This addresses the critical security issue from the previous review.apps/web/app/(org)/dashboard/spaces/[spaceId]/components/MembersDialog.tsx (2)
17-17: LGTM! Type extended to support member images.The
memberImagefield addition aligns with the data model changes for storing image references.
45-51: LGTM! Member avatar rendering migrated to SignedImageUrl.The SignedImageUrl component is properly integrated with correct prop mapping from the updated type.
apps/web/actions/organization/remove-icon.ts (2)
6-6: LGTM! Imports are appropriate for S3 cleanup.The imports for S3Buckets, Effect, Option, and runPromise are correctly added to support the S3 deletion logic.
Also applies to: 9-9, 11-11
58-61: No issues found — code is correct.The verification confirms that the
Dependencieslayer explicitly includesS3Buckets.Default, so the Effect at lines 58–61 will execute correctly. The code properly usesrunPromiseper the coding guidelines for server-side effects.apps/web/actions/account/remove-profile-image.ts (2)
8-11: Good use of ManagedRuntime for effects.Importing runPromise from apps/web/lib/server and executing the Effect on the server complies with our guidelines.
As per coding guidelines.
1-58: Review comment verified—no additional unsafe patterns found in critical S3 operations.The codebase shows consistent fixes across S3 deletion operations:
remove-profile-image.ts(reviewed file),remove-icon.ts,upload-organization-icon.ts, andupload-profile-image.tsall correctly useurl.hostname.endsWith(".amazonaws.com")upload.tschecks a config string by design (inverted logic to detect non-AWS endpoints)CapVideoPlayer.tsxhas a.includes("amazonaws.com")check, but this is for frontend CORS detection (non-security-critical)The reviewed code implements the safe pattern correctly and aligns with the existing codebase conventions for S3 URL validation.
apps/web/app/(org)/dashboard/dashboard-data.ts (2)
209-209: LGTM: use the org icon key for the “All spaces” entryUsing activeOrgInfo.organization.iconUrl here matches the “key-not-URL” approach and keeps the list visually consistent.
Confirm the downstream UI uses SignedImageUrl (or hook) to resolve this key.
134-145: Now I need to examine the broader context of the file to verify the fix and ensure theuservariable is available:Based on my comprehensive verification:
MySQL ONLY_FULL_GROUP_BY: MySQL 5.7.5+ rejects queries where the select list refers to non-aggregated columns that are neither named in the GROUP BY clause nor are functionally dependent on them. The current code violates this constraint.
MAX() with string columns: MAX() may take a string argument and returns the maximum string value. Aggregate functions ignore NULL values, so the proposed fix using
MAX(${users.image})is appropriate.Drizzle ORM syntax: Drizzle supports leftJoin with condition using eq() and the and() operator combines multiple conditions, confirming the proposed diff is syntactically valid.
Script verification: The ripgrep search confirmed both line 134 (
memberImage: users.image) and line 158 (.groupBy(spaces.id, users.image)) match the patterns identified in the review.The review comment's core analysis is correct: joining all space members then grouping by
users.imagecreates duplicate rows and violates MySQL's ONLY_FULL_GROUP_BY constraint. The suggested fix to aggregate the image with MAX() and restrict the join to the current user is sound.However, I cannot verify the availability of the
uservariable or whethermemberImageexists in the Spaces type without inspecting the full file context.
Fix duplicate rows from broad join; aggregate memberImage and restrict join to current user
When selecting a column that is not part of the GROUP BY, there could be multiple values for that column within the groups, but there will only be space for a single value in the results. The current join on all members then grouping by
users.imageproduces duplicate space rows. Restrict the join to the current user and aggregate the image so you can group byspaces.idonly.Apply:
- memberImage: users.image, + // Aggregate since ONLY_FULL_GROUP_BY requires non-grouped selected cols to be aggregated. + memberImage: sql<string | null>`MAX(${users.image})`, ... - .from(spaces) - .leftJoin(spaceMembers, eq(spaces.id, spaceMembers.spaceId)) - .leftJoin(users, eq(spaceMembers.userId, users.id)) + .from(spaces) + // Join at most one row per space by constraining to the current user. + .leftJoin( + spaceMembers, + and(eq(spaces.id, spaceMembers.spaceId), eq(spaceMembers.userId, user.id)), + ) + .leftJoin(users, eq(spaceMembers.userId, users.id)) ... - .groupBy(spaces.id, users.image); + .groupBy(spaces.id);This yields one row per space, keeps the WHERE logic intact, and removes the need to group by
users.image.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes