Address resource loading with GCS and restore domain signup restrictions #972
Conversation
WalkthroughRemoves a debug echo in CI; eliminates the playlistUrl API route; changes thumbnail/screenshot endpoints to always use per-bucket providers for signed URLs; updates frontend embed/share components to call the internal playlist API; normalizes S3 provider method signatures; adds a NextAuth signIn callback enforcing allowed signup domains; marks two notification routes as force-dynamic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant ScreenshotAPI as /api/screenshot
participant ThumbnailAPI as /api/thumbnail
participant Auth as AppAuth
participant BP as BucketProvider
participant S3 as S3
Client->>ScreenshotAPI: GET /api/screenshot?userId&videoId...
ScreenshotAPI->>Auth: validate user/visibility
Auth-->>ScreenshotAPI: auth result
ScreenshotAPI->>BP: createBucketProvider(bucket)
ScreenshotAPI->>BP: getSignedObjectUrl(screenshotKey)
BP->>S3: presign request
S3-->>BP: signed URL
BP-->>ScreenshotAPI: signed URL
ScreenshotAPI-->>Client: 200 { url }
sequenceDiagram
autonumber
actor OAuthUser
participant NextAuth as NextAuth signIn callback
participant DB as Database
participant Env as ServerEnv
OAuthUser->>NextAuth: signIn({ user, email, credentials })
NextAuth->>Env: read CAP_ALLOWED_SIGNUP_DOMAINS
alt no domains configured
NextAuth-->>OAuthUser: allow (true)
else domains configured
NextAuth->>DB: find user by email
DB-->>NextAuth: existingUser | null
alt existingUser
NextAuth-->>OAuthUser: allow (true)
else new user
NextAuth->>NextAuth: isEmailAllowedForSignup(email)
alt allowed
NextAuth-->>OAuthUser: allow (true)
else not allowed
NextAuth-->>OAuthUser: deny (false)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/api/screenshot/route.ts (1)
71-72: Use trusted ownerId from DB for S3 prefix, not user-supplied userId.Building the key prefix from the request
userIdcan cause false 404s and makes access control fragile. Usevideo.ownerId.- const screenshotPrefix = `${userId}/${videoId}/`; + const screenshotPrefix = `${video.ownerId}/${videoId}/`;
🧹 Nitpick comments (12)
apps/web/app/api/video/playlistUrl/route.ts (1)
21-27: userId query param is unused; either validate or remove.Currently ignored but still required by clients. Either validate it matches
video.ownerIdor drop it from the contract to avoid confusion.Example validation:
- const userId = searchParams.get("userId") || ""; + const userId = searchParams.get("userId") || ""; ... - if (!userId || !videoId) { + if (!userId || !videoId) { ... } + if (userId && userId !== video.ownerId) { + return new Response(JSON.stringify({ error: true, message: "Invalid user/video pair" }), { status: 401, headers: getHeaders(origin) }); + }apps/web/app/api/screenshot/route.ts (1)
74-81: Prefer deterministic selection when multiple PNGs exist.
findreturns the first match; S3 listing order isn’t guaranteed. Sort byLastModifieddesc and pick the newest.- const screenshot = objects.Contents?.find((object) => - object.Key?.endsWith(".png"), - ); + const screenshot = objects.Contents + ?.filter((o) => o.Key?.endsWith(".png")) + .sort((a, b) => { + const ta = a.LastModified ? new Date(a.LastModified).getTime() : 0; + const tb = b.LastModified ? new Date(b.LastModified).getTime() : 0; + return tb - ta; + })[0];packages/database/auth/auth-options.tsx (1)
193-223: Solid restoration of domain-restricted signup; consider a couple refinements.
- Lowercase before DB/domain checks to avoid collation surprises.
- Allow invited users regardless of domain (optional).
- const userEmail = + let userEmail = user?.email || (typeof email === "string" ? email : typeof credentials?.email === "string" ? credentials.email : null); - if (!userEmail || typeof userEmail !== "string") return true; + if (!userEmail || typeof userEmail !== "string") return true; + userEmail = userEmail.toLowerCase(); const [existingUser] = await db() .select() .from(users) .where(eq(users.email, userEmail)) .limit(1); // Only apply domain restrictions for new users, existing ones can always sign in if ( !existingUser && !isEmailAllowedForSignup(userEmail, allowedDomains) ) { console.warn(`Signup blocked for email domain: ${userEmail}`); return false; }If you want to allow invites across domains, check
organization_invites.invitedEmail = userEmailbefore blocking..github/workflows/docker-build-web.yml (1)
41-51: Docs suggestion: add a Storage (GCS via S3) guide.Create docs/storage/gcs.md and link from README. Include HMAC key creation steps, required envs (CAP_AWS_* and endpoints), and notes on path-style addressing and CORS.
I can draft this guide with screenshots and a checklist.
apps/web/app/api/thumbnail/route.ts (5)
38-46: Use 404 (Not Found) instead of 401 when the video doesn’t exist.401 implies authentication and usually requires WWW-Authenticate. This is a resource existence case.
- status: 401, + status: 404,
48-57: Return 404 (Not Found) for missing resource, not 401.- status: 401, + status: 404,
29-36: Optional: verify the video belongs to the supplied userId.Prevents mismatched userId/videoId from probing other tenants’ buckets. If thumbnails are intended public, skip; otherwise constrain by owner.
-import { eq } from "drizzle-orm"; +import { and, eq } from "drizzle-orm"; ... - .where(eq(videos.id, videoId)); + .where(and(eq(videos.id, videoId), eq(videos.ownerId, userId)));
62-71: Type the S3 objects instead ofany.This avoids accidental property typos and improves IDE help.
+import type { _Object as S3Object } from "@aws-sdk/client-s3"; ... - const contents = listResponse.Contents || []; - const thumbnailKey = contents.find((item: any) => + const contents = (listResponse.Contents || []) as S3Object[]; + const thumbnailKey = contents.find((item) => item.Key?.endsWith("screen-capture.jpg"), )?.Key;
10-15: Add an OPTIONS handler for CORS preflight.You already set CORS headers; answering OPTIONS avoids 404 preflight on cross-origin embeds.
Add outside the changed block:
export async function OPTIONS(request: NextRequest) { const origin = request.headers.get("origin") as string; return new Response(null, { status: 204, headers: getHeaders(origin) }); }apps/web/app/s/[videoId]/_components/ShareVideo.tsx (1)
128-142: DRY: extractvideoSrcresolution into a helper shared with Embed.Both Share and Embed duplicate this branching. A small util reduces drift.
apps/web/app/embed/[videoId]/_components/EmbedVideo.tsx (2)
184-190: Fix event listener cleanup to prevent leaks.Handlers added inline can’t be removed; store references and remove those.
- player.addEventListener("play", () => listener(true)); - player.addEventListener("pause", () => listener(false)); + const handlePlay = () => listener(true); + const handlePause = () => listener(false); + player.addEventListener("play", handlePlay); + player.addEventListener("pause", handlePause); return () => { - player.removeEventListener("play", () => listener(true)); - player.removeEventListener("pause", () => listener(false)); + player.removeEventListener("play", handlePlay); + player.removeEventListener("pause", handlePause); player.removeEventListener("loadedmetadata", handleLoadedMetadata); };
152-166: Consider consolidatingvideoSrclogic with ShareVideo.A shared helper avoids future divergence (e.g., adding a new type).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
.github/workflows/docker-build-web.yml(2 hunks)apps/web/app/api/screenshot/route.ts(1 hunks)apps/web/app/api/thumbnail/route.ts(1 hunks)apps/web/app/api/video/playlistUrl/route.ts(2 hunks)apps/web/app/embed/[videoId]/_components/EmbedVideo.tsx(1 hunks)apps/web/app/s/[videoId]/_components/ShareVideo.tsx(1 hunks)apps/web/utils/s3.ts(1 hunks)packages/database/auth/auth-options.tsx(2 hunks)packages/database/migrations/meta/0008_snapshot.json(1 hunks)packages/database/migrations/meta/_journal.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/web/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When HTTP routes are necessary, implement them under app/api/*, configure CORS correctly, and set precise revalidation
Files:
apps/web/app/api/screenshot/route.tsapps/web/app/api/video/playlistUrl/route.tsapps/web/app/api/thumbnail/route.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for client-side server state and data fetching in the web app
Mutations should call Server Actions and perform precise cache updates with setQueryData/setQueriesData, avoiding broad invalidations
Prefer Server Components for initial data and pass initialData to client components for React Query hydration
Files:
apps/web/app/api/screenshot/route.tsapps/web/app/api/video/playlistUrl/route.tsapps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/app/api/thumbnail/route.tsapps/web/app/embed/[videoId]/_components/EmbedVideo.tsxapps/web/utils/s3.ts
{apps/web,packages/ui}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps/web,packages/ui}/**/*.{ts,tsx}: Use Tailwind CSS exclusively for styling in the web app and shared React UI components
Component naming: React components in PascalCase; hooks in camelCase starting with 'use'
Files:
apps/web/app/api/screenshot/route.tsapps/web/app/api/video/playlistUrl/route.tsapps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/app/api/thumbnail/route.tsapps/web/app/embed/[videoId]/_components/EmbedVideo.tsxapps/web/utils/s3.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
Files:
apps/web/app/api/screenshot/route.tsapps/web/app/api/video/playlistUrl/route.tsapps/web/app/s/[videoId]/_components/ShareVideo.tsxpackages/database/auth/auth-options.tsxapps/web/app/api/thumbnail/route.tsapps/web/app/embed/[videoId]/_components/EmbedVideo.tsxapps/web/utils/s3.ts
🧬 Code graph analysis (3)
apps/web/app/api/video/playlistUrl/route.ts (3)
packages/database/index.ts (1)
db(30-35)packages/database/schema.ts (1)
s3Buckets(362-372)apps/web/utils/s3.ts (1)
createBucketProvider(374-397)
packages/database/auth/auth-options.tsx (4)
packages/env/server.ts (1)
serverEnv(80-84)packages/database/index.ts (1)
db(30-35)packages/database/schema.ts (1)
users(45-87)packages/database/auth/domain-utils.ts (1)
isEmailAllowedForSignup(3-19)
apps/web/app/api/thumbnail/route.ts (1)
apps/web/utils/helpers.ts (1)
getHeaders(18-26)
🪛 GitHub Actions: Validate Migrations
packages/database/migrations/meta/_journal.json
[error] 2-2: Migration journal version cannot be changed (was: $BASE_VERSION, now: $CURRENT_VERSION)
🪛 Checkov (3.2.334)
.github/workflows/docker-build-web.yml
[MEDIUM] 42-43: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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: Clippy
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (8)
packages/database/migrations/meta/_journal.json (1)
62-62: Migration meta file is up-to-date; no action needed.
Verified no diffs againstmainforpackages/database/migrations/meta/_journal.json(version 5 matches).packages/database/migrations/meta/0008_snapshot.json (1)
1748-1748: Ignore snapshot revert — no actual changes detected
packages/database/migrations/meta/0008_snapshot.jsonis identical tomain; no trailing newline or schema change to revert.Likely an incorrect or invalid review comment.
apps/web/app/api/video/playlistUrl/route.ts (1)
69-71: Signed URL cacheability: align TTL with response caching.Ensure
bucketProvider.getSignedObjectUrlTTL is >= any CDN/browser caching implied byCACHE_CONTROL_HEADERS; otherwise clients may cache expired URLs.What is the signed URL expiration used here? If it’s short (e.g., 60s), consider reducing cache max-age or adding a query param nonce and short max-age.
apps/web/app/api/screenshot/route.ts (1)
91-91: LGTM: unified on signed URLs.Switching to
bucketProvider.getSignedObjectUrlremoves brittle public URL paths and fixes 403s.packages/database/auth/auth-options.tsx (1)
17-17: LGTM: brings back domain restriction utilities.Import is correct and scoped.
apps/web/utils/s3.ts (1)
264-292: LGTM: async provider methods bring consistency and correct typing.The
headObject,putObject, andcopyObjectasync forms match the interface and remove implicitanys.apps/web/app/s/[videoId]/_components/ShareVideo.tsx (1)
139-142: LGTM: unify non-desktop sources to internal signed playlist URL.Aligns Share flow with API-based signed URLs and removes public S3 coupling.
apps/web/app/embed/[videoId]/_components/EmbedVideo.tsx (1)
163-166: LGTM: route all non-desktop sources via/api/playlist?videoType=video.Removes environment/public S3 dependencies and uses signed URLs.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/docker-build-web.yml (2)
41-51: Stop writing secrets to .env in CI; use masked build-args or BuildKit secrets.These lines embed DB creds, AWS/GCS HMAC keys, and NEXTAUTH_SECRET into the image build context and CI logs/layers. Prior comments already flagged this; issue still present.
Apply (minimal safer change):
- name: Create .env file run: | echo "WEB_URL=http://localhost:3000" > .env echo "NEXT_PUBLIC_DOCKER_BUILD=true" >> .env echo "NEXT_PUBLIC_CAP_AWS_BUCKET=capso" >> .env echo "NEXT_PUBLIC_CAP_AWS_REGION=us-east-1" >> .env - # Add required build-time environment variables - echo "DATABASE_URL=mysql://build:build@build:3306/build" >> .env - echo "CAP_AWS_BUCKET=build-bucket" >> .env - echo "CAP_AWS_REGION=us-east-1" >> .env - echo "CAP_AWS_ACCESS_KEY=build-access-key" >> .env - echo "CAP_AWS_SECRET_KEY=build-secret-key" >> .env - echo "NEXTAUTH_SECRET=build-nextauth-secret-placeholder-32-chars" >> .env - echo "NEXTAUTH_URL=https://build.placeholder.com" >> .env - echo "CAP_AWS_ENDPOINT=https://build.placeholder.com" >> .env - echo "S3_PUBLIC_ENDPOINT=https://build.placeholder.com" >> .env - echo "S3_INTERNAL_ENDPOINT=https://build.placeholder.com" >> .env + # Keep secrets out of .env in CI—provide via masked build-args/secrets belowCheckov’s “Basic Auth Credentials” finding (lines 42–43) is addressed by this removal.
71-81: Do not bake secrets via build-args; pass as masked secrets (BuildKit).Move sensitive values (DB URL, access key, secret key, NEXTAUTH_SECRET) to the action’s “secrets” and keep only non-sensitive args as build-args.
with: context: . file: apps/web/Dockerfile platforms: linux/${{ matrix.platform }} push: true outputs: type=image,name=ghcr.io/${{ github.repository_owner }}/cap-web,push-by-digest=true cache-from: type=gha,scope=buildx-${{ matrix.platform }} cache-to: type=gha,mode=max,scope=buildx-${{ matrix.platform }} - build-args: | - DATABASE_URL=mysql://build:build@build:3306/build - CAP_AWS_BUCKET=build-bucket - CAP_AWS_REGION=us-east-1 - CAP_AWS_ACCESS_KEY=build-access-key - CAP_AWS_SECRET_KEY=build-secret-key - NEXTAUTH_SECRET=build-nextauth-secret-placeholder-32-chars - NEXTAUTH_URL=https://build.placeholder.com - CAP_AWS_ENDPOINT=https://build.placeholder.com - S3_PUBLIC_ENDPOINT=https://build.placeholder.com - S3_INTERNAL_ENDPOINT=https://build.placeholder.com + build-args: | + # non-sensitive + CAP_AWS_BUCKET=${{ secrets.CAP_AWS_BUCKET }} + CAP_AWS_REGION=${{ secrets.CAP_AWS_REGION }} + CAP_AWS_ENDPOINT=${{ secrets.CAP_AWS_ENDPOINT }} + S3_PUBLIC_ENDPOINT=${{ secrets.S3_PUBLIC_ENDPOINT }} + S3_INTERNAL_ENDPOINT=${{ secrets.S3_INTERNAL_ENDPOINT }} + NEXTAUTH_URL=${{ secrets.NEXTAUTH_URL }} + secrets: | + DATABASE_URL=${{ secrets.BUILD_DATABASE_URL }} + CAP_AWS_ACCESS_KEY=${{ secrets.CAP_AWS_ACCESS_KEY }} + CAP_AWS_SECRET_KEY=${{ secrets.CAP_AWS_SECRET_KEY }} + NEXTAUTH_SECRET=${{ secrets.NEXTAUTH_SECRET }}If Dockerfile changes are possible, mount them with RUN --mount=type=secret,id=... and read from /run/secrets/*.
🧹 Nitpick comments (4)
apps/web/app/api/video/playlistUrl/route.ts (4)
40-44: Query by id with leftJoin is good; consider limit(1).Adds a small safeguard and communicates intent to the planner; negligible perf impact.
- .leftJoin(s3Buckets, eq(videos.bucket, s3Buckets.id)) - .where(eq(videos.id, videoId)); + .leftJoin(s3Buckets, eq(videos.bucket, s3Buckets.id)) + .where(eq(videos.id, videoId)) + .limit(1);
56-65: Use 404 for missing video (and 400 for missing params earlier).“Not found” should be 404, not 401. Also the earlier “!userId || !videoId” case should be 400 since it’s a bad request, not auth failure.
- { - status: 404, - headers: getHeaders(origin), - }, + { status: 404, headers: getHeaders(origin) },Optional follow-up: drop userId requirement entirely (unused) or verify it matches video.ownerId.
70-79: Return 403 for non-public unauthorized access (not 401).401 implies missing/invalid authentication; here the user is authenticated but forbidden if not the owner.
- return new Response( - JSON.stringify({ error: true, message: "Video is not public" }), - { status: 401, headers: getHeaders(origin) }, - ); + return new Response( + JSON.stringify({ error: true, message: "Forbidden" }), + { status: 403, headers: getHeaders(origin) }, + );
81-84: Consider explicit signed URL TTL and centralize playlistKey.
- If supported, pass a short expiresIn (e.g., 5–15m) when generating the URL.
- Extract playlist key construction to a shared helper to avoid drift across routes.
Would you like me to propose a helper (e.g., buildPlaylistKey(video)) and wire an expiresIn option if available in createBucketProvider?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/docker-build-web.yml(2 hunks)apps/web/app/api/video/playlistUrl/route.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/web/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When HTTP routes are necessary, implement them under app/api/*, configure CORS correctly, and set precise revalidation
Files:
apps/web/app/api/video/playlistUrl/route.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for client-side server state and data fetching in the web app
Mutations should call Server Actions and perform precise cache updates with setQueryData/setQueriesData, avoiding broad invalidations
Prefer Server Components for initial data and pass initialData to client components for React Query hydration
Files:
apps/web/app/api/video/playlistUrl/route.ts
{apps/web,packages/ui}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps/web,packages/ui}/**/*.{ts,tsx}: Use Tailwind CSS exclusively for styling in the web app and shared React UI components
Component naming: React components in PascalCase; hooks in camelCase starting with 'use'
Files:
apps/web/app/api/video/playlistUrl/route.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
Files:
apps/web/app/api/video/playlistUrl/route.ts
🧬 Code graph analysis (1)
apps/web/app/api/video/playlistUrl/route.ts (3)
packages/database/schema.ts (2)
videos(231-277)s3Buckets(362-372)apps/web/utils/helpers.ts (1)
getHeaders(18-26)apps/web/utils/s3.ts (1)
createBucketProvider(374-397)
🪛 Checkov (3.2.334)
.github/workflows/docker-build-web.yml
[MEDIUM] 42-43: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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). (2)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (1)
apps/web/app/api/video/playlistUrl/route.ts (1)
2-3: LGTM: moved to per-bucket provider with session import.Imports align with the new signed-URL flow and bucket-aware lookups.
Also applies to: 8-9
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 (8)
apps/web/app/api/notifications/route.ts (2)
26-33: Fix HTTP statuses and response consistencyUnauthorized currently uses
new Response, and the error path returns 200 with{ status: 500 }in the body. UseNextResponse.json(..., { status })for both.Apply:
@@ - if (!currentUser) { - return new Response(JSON.stringify({ error: "Unauthorized" }), { - status: 401, - }); - } + if (!currentUser) { + return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); + } @@ - return NextResponse.json({ - status: 500, - error: "Failed to fetch notifications", - }); + return NextResponse.json( + { error: "Failed to fetch notifications" }, + { status: 500 }, + );Also applies to: 124-130
93-119: Strengthen type-narrowing after filter
filter(Boolean)doesn’t narrow to the parsed schema type. Use a type guard tied to the Zod schema.- }) - .filter(Boolean); + }) + .filter((n): n is APINotificationT => Boolean(n));Add once near the top (after imports):
type APINotificationT = z.infer<typeof APINotification>;apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (6)
1-1: Add "use client" directive; this module uses hooks.Without it, Next.js will treat this as a Server Component and fail at build/runtime.
+"use client";
62-65: Introduce a single embedBaseUrl derived from public env.const [activeTab, setActiveTab] = useState<(typeof tabs)[number]>("Share"); + // Public base URL for embeds; falls back to hosted default + const { NEXT_PUBLIC_WEB_URL } = usePublicEnv(); + const embedBaseUrl = NEXT_PUBLIC_WEB_URL ?? "https://cap.so";
185-191: Build the embed code from embedBaseUrl instead of branching on process.env.- const embedCode = `<div style="position: relative; padding-bottom: 56.25%; height: 0;"><iframe src="${ - process.env.NODE_ENV === "development" - ? process.env.NEXT_PUBLIC_WEB_URL - : "https://cap.so" - }/embed/${capId}" frameborder="0" webkitallowfullscreen mozallowfullscreen allowfullscreen style="position: absolute; top: 0; left: 0; width: 100%; height: 100%;"></iframe></div>`; + const embedCode = `<div style="position: relative; padding-bottom: 56.25%; height: 0;"><iframe src="${embedBaseUrl}/embed/${capId}" frameborder="0" webkitallowfullscreen mozallowfullscreen allowfullscreen style="position: absolute; top: 0; left: 0; width: 100%; height: 100%;"></iframe></div>`;
328-333: Render the same embed code preview that handleCopyEmbedCode copies.- {`<div style="position: relative; padding-bottom: 56.25%; height: 0;"><iframe src="${ - process.env.NODE_ENV === "development" - ? process.env.NEXT_PUBLIC_WEB_URL - : "https://cap.so" - }/embed/${capId}" frameborder="0" webkitallowfullscreen mozallowfullscreen allowfullscreen style="position: absolute; top: 0; left: 0; width: 100%; height: 100%;"></iframe></div>`} + {`<div style="position: relative; padding-bottom: 56.25%; height: 0;"><iframe src="${embedBaseUrl}/embed/${capId}" frameborder="0" webkitallowfullscreen mozallowfullscreen allowfullscreen style="position: absolute; top: 0; left: 0; width: 100%; height: 100%;"></iframe></div>`}
81-148: Avoid stale-closure races in onSuccess; use mutation variables.Users can toggle UI while the mutation is pending; closures may drift from the committed payload, leading to incorrect toasts and callbacks.
- onSuccess: () => { - const newSelectedSpaces = Array.from(selectedSpaces); - const initialSpaces = Array.from(initialSelectedSpaces); + onSuccess: (_data, variables) => { + const newSelectedSpaces = [...variables.spaceIds]; + const initialSpaces = Array.from(initialSelectedSpaces); - const addedSpaceIds = newSelectedSpaces.filter( + const addedSpaceIds = newSelectedSpaces.filter( (id) => !initialSpaces.includes(id), ); - const removedSpaceIds = initialSpaces.filter( + const removedSpaceIds = initialSpaces.filter( (id) => !newSelectedSpaces.includes(id), ); - const publicChanged = publicToggle !== initialPublicState; + const publicChanged = variables.public !== initialPublicState; @@ - onSharingUpdated(newSelectedSpaces); + onSharingUpdated(newSelectedSpaces); onClose(); },
164-171: Fix “shared via organization” check; should key off organizationId.Currently it checks the space’s own id, not the org-level selection.
const isSpaceSharedViaOrganization = useCallback( (spaceId: string) => { const space = spacesData?.find((s) => s.id === spaceId); if (!space) return false; - return sharedSpaceIds.has(space.id); + // Org entry itself isn't "via org" + const isOrgEntry = space.id === space.organizationId && space.primary === true; + if (isOrgEntry) return false; + // A space is implicitly shared if its organization entry is selected + return selectedSpaces.has(space.organizationId); }, - [spacesData, sharedSpaceIds], + [spacesData, selectedSpaces], );
🧹 Nitpick comments (6)
apps/web/app/api/notifications/route.ts (2)
5-9: Remove unused/incorrect imports
ColumnBaseConfig,MySqlColumn, andAvcProfileInfoare unused; the latter also imports via anode_modules/...path, which is brittle.-import { and, ColumnBaseConfig, desc, eq, isNull, sql } from "drizzle-orm"; -import { MySqlColumn } from "drizzle-orm/mysql-core"; +import { and, desc, eq, isNull, sql } from "drizzle-orm"; import { NextResponse } from "next/server"; -import { AvcProfileInfo } from "node_modules/@remotion/media-parser/dist/containers/avc/parse-avc";
19-23: Drop unused types
NotificationsKeysandNotificationsKeysWithRepliesaren’t referenced.-type NotificationsKeys = (typeof notifications.$inferSelect)["type"]; -type NotificationsKeysWithReplies = - | Exclude<`${NotificationsKeys}s`, "replys"> - | "replies";apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx (4)
281-285: Disable the public toggle while save is pending to prevent state drift.<Switch checked={publicToggle} - onCheckedChange={setPublicToggle} + onCheckedChange={setPublicToggle} + disabled={updateSharing.isPending} />
451-451: Conflicting background classes; last one wins.bg-gray-4 overrides bg-green-500; the check badge won’t be green.
- className="flex absolute -top-2 -right-2 z-10 justify-center items-center bg-green-500 rounded-full bg-gray-4 size-4" + className="flex absolute -top-2 -right-2 z-10 justify-center items-center rounded-full bg-green-500 size-4"
314-314: Grid span mismatch (grid-cols-4 but col-span-5).Use col-span-full for clarity.
- <div className="flex col-span-5 gap-2 justify-center items-center text-sm"> + <div className="flex col-span-full gap-2 justify-center items-center text-sm">
1-1: Rename file to kebab-case per guidelines.SharingDialog.tsx → sharing-dialog.tsx.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/docker-build-web.yml(0 hunks)apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx(1 hunks)apps/web/app/api/notifications/preferences/route.ts(1 hunks)apps/web/app/api/notifications/route.ts(1 hunks)apps/web/app/api/video/playlistUrl/route.ts(0 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/docker-build-web.yml
- apps/web/app/api/video/playlistUrl/route.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/web/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When HTTP routes are necessary, implement them under app/api/*, configure CORS correctly, and set precise revalidation
Files:
apps/web/app/api/notifications/preferences/route.tsapps/web/app/api/notifications/route.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for client-side server state and data fetching in the web app
Mutations should call Server Actions and perform precise cache updates with setQueryData/setQueriesData, avoiding broad invalidations
Prefer Server Components for initial data and pass initialData to client components for React Query hydration
Files:
apps/web/app/api/notifications/preferences/route.tsapps/web/app/api/notifications/route.tsapps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx
{apps/web,packages/ui}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps/web,packages/ui}/**/*.{ts,tsx}: Use Tailwind CSS exclusively for styling in the web app and shared React UI components
Component naming: React components in PascalCase; hooks in camelCase starting with 'use'
Files:
apps/web/app/api/notifications/preferences/route.tsapps/web/app/api/notifications/route.tsapps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
**/*.{ts,tsx}: Use Biome to format/lint TypeScript with a 2-space indent
TypeScript file names should be kebab-case (e.g., user-menu.tsx)
Files:
apps/web/app/api/notifications/preferences/route.tsapps/web/app/api/notifications/route.tsapps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
React/Solid components should be named using PascalCase
Files:
apps/web/app/(org)/dashboard/caps/components/SharingDialog.tsx
🧬 Code graph analysis (2)
apps/web/app/api/notifications/preferences/route.ts (2)
apps/web/app/api/notifications/route.ts (1)
dynamic(24-24)apps/web/app/(org)/dashboard/layout.tsx (1)
dynamic(16-16)
apps/web/app/api/notifications/route.ts (3)
apps/web/app/api/notifications/preferences/route.ts (1)
dynamic(17-17)apps/web/app/(org)/dashboard/layout.tsx (1)
dynamic(16-16)apps/web/app/s/[videoId]/page.tsx (1)
dynamic(36-36)
⏰ 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). (2)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (3)
apps/web/app/api/notifications/preferences/route.ts (1)
17-18: LGTM: forcing this route dynamic is appropriatePer-user data, session-bound. No caching hazards now.
apps/web/app/api/notifications/route.ts (2)
24-25: LGTM: force this API route dynamicCorrect for user/session-scoped notifications; avoids accidental caching.
1-132: CORS sanity-check (only if cross-origin)If this route is ever called from a different origin (embeds, admin domain), add explicit CORS handling; otherwise ignore.
| import { useDashboardContext } from "@/app/(org)/dashboard/Contexts"; | ||
| import type { Spaces } from "@/app/(org)/dashboard/dashboard-data"; | ||
| import { Tooltip } from "@/components/Tooltip"; | ||
| import { usePublicEnv } from "@/utils/public-env"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use usePublicEnv to drive the embed base URL (aligns with PR goal and self‑hosting).
You imported the hook; wire it up so the embed URL doesn’t branch on process.env at runtime.
Apply alongside the following changes (see separate comments at Lines 62‑65, 186‑191, 328‑333).
|
Thanks for this, sorry about the domain restriction being removed. For the GCS guide, we don't really want to maintain one ourselves but I'm happy to add a link to one, would you be alright putting the steps in a public gist? |
|
Hi @Brendonovich , no worries. I appreciate your confirmation. Here's a public gist for the GCS setup steps: https://gist.github.com/mogita/b14b1cc7bdc15a72952b80d5f0700bc7 |
2 participants
CAP_AWS_ACCESS_KEY: GCS HMAC Access Key IDCAP_AWS_SECRET_KEY: GCS HMAC Secret for the keyCAP_AWS_BUCKET: set to the bucket nameCAP_AWS_REGION: put anything should work, GCS doesn't care but the S3 library might need it non-emptyCAP_AWS_ENDPOINT: set tohttps://storage.googleapis.comS3_PUBLIC_ENDPOINT: set tohttps://storage.googleapis.comS3_INTERNAL_ENDPOINT: set tohttps://storage.googleapis.comCAP_ALLOWED_SIGNUP_DOMAINSwas not removed in the original PR, I presumed thesignIncallback was removed by accident. But please correct me if otherwise.P.S. I'm not sure where to put the GCS settings guide. Let me know, in case it's needed, if there's a good place to document it.
Summary by CodeRabbit