Skip to content

Conversation

Brendonovich
Copy link
Member

@Brendonovich Brendonovich commented Oct 10, 2025

Summary by CodeRabbit

  • New Features

    • Thumbnails now show a duration overlay when available.
  • Refactor

    • Thumbnail loading rebuilt to use batched, hook-driven fetching for faster, more reliable thumbnails.
    • Dashboard thumbnails refresh immediately after a successful upload to avoid stale images.
    • Runtime/data-loading wiring updated to support the new thumbnail flow.
  • Chores

    • Added a new dependency to enable the improved data-loading system.

Copy link
Contributor

coderabbitai bot commented Oct 10, 2025

Walkthrough

Adds a batched thumbnail fetch pipeline: new ThumbnailRequest data-loader and query key, Effect-based thumbnail hook in the client, runtime layer exposure and wiring, new backend RPC and Videos.getThumbnailURL, UploadCapButton now refetches via ThumbnailRequest.queryKey, plus a new dependency @effect/experimental.

Changes

Cohort / File(s) Summary
Client UI: Thumbnail consumption
apps/web/components/VideoThumbnail.tsx
Replaces react-query image URL flow with an Effect-based useThumnailQuery(Video.VideoId) hook driven by ThumbnailRequest; prop videoId changed to Video.VideoId; re-enables duration overlay; removes imageUrlQuery export.
Client UI: Upload refetch
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
After thumbnail upload, switches refetch to queryClient.refetchQueries({ queryKey: ThumbnailRequest.queryKey(uploadId) }) and updates import to ThumbnailRequest.
Effect runtime & loader
apps/web/lib/EffectRuntime.ts, apps/web/lib/ThumbnailRequest.ts
Exposes RuntimeLayer publicly and merges ThumbnailRequest.DataLoaderResolver.Default into it; adds ThumbnailRequest namespace with queryKey, Request class, and a batched DataLoader resolver (10ms window) that calls the RPC and propagates per-request results.
Backend: Videos service and RPC
packages/web-backend/src/Videos/index.ts, packages/web-backend/src/Videos/VideosRpcs.ts
Adds Videos.getThumbnailURL(videoId) returning Option signed URL; adds VideosGetThumbnails RPC to fetch multiple thumbnails with per-item Exit results and mapped errors.
Domain: RPC contract
packages/web-domain/src/Video.ts
Declares VideosGetThumbnails RPC: input VideoId[] (max 50), output Array<Exit<Option<string>, UnionErrors>>, RPC-level InternalError.
Deps & minor cleanups
apps/web/package.json, apps/web/utils/helpers.ts, packages/web-domain/src/Policy.ts
Adds @effect/experimental dependency; removes unused serverEnv import from helpers and unused Brand import from Policy.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant UI as VideoThumbnail (client)
  participant Hook as useThumnailQuery
  participant DL as ThumbnailRequest DataLoader
  participant RPC as VideosGetThumbnails (backend)
  participant SVC as Videos.getThumbnailURL
  participant S3 as S3 Bucket

  UI->>Hook: request thumbnail for VideoId
  Hook->>DL: enqueue ThumbnailRequest({ videoId })
  rect rgb(230,245,255)
    note right of DL: requests batched (10ms)
    DL->>RPC: fetch [VideoId...]
    RPC->>SVC: for each id, getThumbnailURL
    SVC->>S3: list objects, find screen-capture.jpg
    S3-->>SVC: key or none
    SVC-->>RPC: Option<signedUrl>
    RPC-->>DL: Exit[] (per-id success/failure)
  end
  DL-->>Hook: resolve per-request (success/error)
  Hook-->>UI: thumbnailUrl | error | loading
  UI-->>UI: render image or fallback
Loading
sequenceDiagram
  autonumber
  participant U as UploadCapButton
  participant QS as QueryClient
  participant Key as ThumbnailRequest.queryKey
  U->>QS: refetchQueries({ queryKey: Key(uploadId) })
  QS->>DL: trigger data loader for uploadId
  DL->>RPC: VideosGetThumbnails([uploadId])
  RPC-->>DL: Exit[]
  DL-->>QS: update cache for ["thumbnail", uploadId]
  QS-->>U: refetch complete
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

A rabbit hums where thumbnails hide,
I batch the hops and fetch the tide.
From buckets deep a URL peeks,
Exit by Exit my whisker seeks.
Keys click, refetch — tiny bytes take flight. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly captures the introduction of an RPC-based thumbnail fetch mechanism and dataloader integration for video thumbnails, which aligns precisely with the primary changes across both front-end and back-end. It is a clear, single-sentence summary without extraneous details, accurately reflecting the main objective of the pull request.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch thumbnail-rpc-dataloader

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)

20-500: Fix missing thumbnailQueryKey export.

Typecheck is currently failing (Module '"@/components/VideoThumbnail"' has no exported member 'thumbnailQueryKey'), so this import breaks the build. Either export thumbnailQueryKey from VideoThumbnail.tsx or import it from the module that actually defines it (e.g. @/lib/ThumbnailRequest).

🧹 Nitpick comments (3)
packages/web-domain/src/Video.ts (1)

172-188: Rename the RPC to fix the typo.

We’re introducing this endpoint for the first time, so it’s worth correcting the name to VideosGetThumbnails now before it spreads across clients and backend layers.

Apply this diff:

-	Rpc.make("VideosGetThumbanils", {
+	Rpc.make("VideosGetThumbnails", {
apps/web/components/VideoThumbnail.tsx (1)

51-75: Rename the new hook to useThumbnailQuery.

The exported hook name is misspelled (useThumnailQuery). This propagates an inconsistent API surface and will keep the typo wherever the hook is reused. Please rename it (and its usages) before we lock in the public API.

apps/web/lib/ThumbnailRequest.ts (1)

8-8: Tighten the queryKey helper’s typing.

Since callers pass Video.VideoId, the helper should accept that branded type (e.g. queryKey = (videoId: Video.VideoId) => [...]). Keeping it as string erodes the brand guarantees and can let mismatched IDs slip in.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cd8e32f and 5a55634.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (10)
  • apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (2 hunks)
  • apps/web/components/VideoThumbnail.tsx (5 hunks)
  • apps/web/lib/EffectRuntime.ts (1 hunks)
  • apps/web/lib/ThumbnailRequest.ts (1 hunks)
  • apps/web/package.json (1 hunks)
  • apps/web/utils/helpers.ts (1 hunks)
  • packages/web-backend/src/Videos/VideosRpcs.ts (2 hunks)
  • packages/web-backend/src/Videos/index.ts (1 hunks)
  • packages/web-domain/src/Policy.ts (1 hunks)
  • packages/web-domain/src/Video.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx,js,jsx,rs}

📄 CodeRabbit inference engine (CLAUDE.md)

Do not add inline, block, or docstring comments in any language; code must be self-explanatory

Files:

  • packages/web-backend/src/Videos/index.ts
  • apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
  • packages/web-backend/src/Videos/VideosRpcs.ts
  • packages/web-domain/src/Video.ts
  • apps/web/components/VideoThumbnail.tsx
  • apps/web/lib/ThumbnailRequest.ts
  • apps/web/utils/helpers.ts
  • apps/web/lib/EffectRuntime.ts
  • packages/web-domain/src/Policy.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Use strict TypeScript and avoid any; leverage shared types from packages

**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by running pnpm format.

Files:

  • packages/web-backend/src/Videos/index.ts
  • apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
  • packages/web-backend/src/Videos/VideosRpcs.ts
  • packages/web-domain/src/Video.ts
  • apps/web/components/VideoThumbnail.tsx
  • apps/web/lib/ThumbnailRequest.ts
  • apps/web/utils/helpers.ts
  • apps/web/lib/EffectRuntime.ts
  • packages/web-domain/src/Policy.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:

  • packages/web-backend/src/Videos/index.ts
  • apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
  • packages/web-backend/src/Videos/VideosRpcs.ts
  • packages/web-domain/src/Video.ts
  • apps/web/components/VideoThumbnail.tsx
  • apps/web/lib/ThumbnailRequest.ts
  • apps/web/utils/helpers.ts
  • apps/web/lib/EffectRuntime.ts
  • packages/web-domain/src/Policy.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 data fetching in the web app
Web mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData rather than broad invalidations
Client code should use useEffectQuery/useEffectMutation and useRpcClient from apps/web/lib/EffectRuntime.ts; do not create ManagedRuntime inside components

Files:

  • apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
  • apps/web/components/VideoThumbnail.tsx
  • apps/web/lib/ThumbnailRequest.ts
  • apps/web/utils/helpers.ts
  • apps/web/lib/EffectRuntime.ts
apps/web/app/**/*.{tsx,ts}

📄 CodeRabbit inference engine (CLAUDE.md)

Prefer Server Components for initial data in the Next.js App Router and pass initialData to client components

Files:

  • apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
apps/web/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

On the client, always use useEffectQuery or useEffectMutation from @/lib/EffectRuntime; never call EffectRuntime.run* directly in components.

Files:

  • apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
  • apps/web/components/VideoThumbnail.tsx
  • apps/web/lib/ThumbnailRequest.ts
  • apps/web/utils/helpers.ts
  • apps/web/lib/EffectRuntime.ts
🧠 Learnings (1)
📚 Learning: 2025-09-22T14:17:47.407Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-22T14:17:47.407Z
Learning: Applies to apps/web/**/*.{ts,tsx} : Client code should use useEffectQuery/useEffectMutation and useRpcClient from apps/web/lib/EffectRuntime.ts; do not create ManagedRuntime inside components

Applied to files:

  • apps/web/lib/EffectRuntime.ts
🧬 Code graph analysis (6)
packages/web-backend/src/Videos/index.ts (3)
packages/web-domain/src/Video.ts (3)
  • Video (16-59)
  • VideoId (12-12)
  • VideoId (13-13)
packages/web-domain/src/Policy.ts (2)
  • Policy (8-12)
  • policy (29-41)
packages/web-backend/src/S3Buckets/index.ts (1)
  • S3Buckets (13-205)
packages/web-backend/src/Videos/VideosRpcs.ts (2)
packages/web-domain/src/Errors.ts (1)
  • InternalError (3-6)
packages/web-backend/src/Auth.ts (1)
  • provideOptionalAuth (91-113)
packages/web-domain/src/Video.ts (4)
apps/web/lib/Rpcs.ts (1)
  • Rpc (11-14)
packages/web-domain/src/Folder.ts (1)
  • NotFoundError (17-21)
packages/web-domain/src/Policy.ts (1)
  • PolicyDeniedError (20-24)
packages/web-domain/src/Errors.ts (1)
  • InternalError (3-6)
apps/web/components/VideoThumbnail.tsx (3)
packages/web-domain/src/Video.ts (3)
  • Video (16-59)
  • VideoId (12-12)
  • VideoId (13-13)
apps/web/lib/EffectRuntime.ts (1)
  • useEffectQuery (20-20)
apps/web/lib/ThumbnailRequest.ts (1)
  • ThumbnailRequest (10-14)
apps/web/lib/ThumbnailRequest.ts (2)
packages/web-domain/src/Video.ts (3)
  • Video (16-59)
  • VideoId (12-12)
  • VideoId (13-13)
apps/web/lib/Rpcs.ts (1)
  • Rpc (11-14)
apps/web/lib/EffectRuntime.ts (2)
apps/web/lib/ThumbnailRequest.ts (1)
  • ThumbnailRequest (10-14)
apps/web/lib/Rpcs.ts (1)
  • Rpc (11-14)
🪛 GitHub Check: Typecheck
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx

[failure] 20-20:
Module '"@/components/VideoThumbnail"' has no exported member 'thumbnailQueryKey'.

⏰ 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 (1)
packages/web-domain/src/Policy.ts (1)

4-4: LGTM! Clean removal of unused import.

The Brand import is not used anywhere in this file. Removing it reduces unnecessary dependencies and improves code clarity.

Comment on lines +167 to +193
const videoOpt = yield* repo
.getById(videoId)
.pipe(Policy.withPublicPolicy(policy.canView(videoId)));

return yield* videoOpt.pipe(
Effect.transposeMapOption(
Effect.fn(function* ([video]) {
const [bucket] = yield* S3Buckets.getBucketAccess(video.bucketId);

const listResponse = yield* bucket.listObjects({
prefix: `${video.ownerId}/${video.id}/`,
});
const contents = listResponse.Contents || [];

const thumbnailKey = contents.find((item) =>
item.Key?.endsWith("screen-capture.jpg"),
)?.Key;

if (!thumbnailKey) return Option.none();

return Option.some(
yield* bucket.getSignedObjectUrl(thumbnailKey),
);
}),
),
Effect.map(Option.flatten),
);
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Don’t call .pipe on the resolved tuple.

After yield*, videoOpt is just the [video] tuple, so videoOpt.pipe(...) throws at runtime and the Effect.transposeMapOption never runs. Keep everything inside the effect pipeline instead.

Apply this diff:

-			getThumbnailURL: Effect.fn("Videos.getThumbnailURL")(function* (
-				videoId: Video.VideoId,
-			) {
-				const videoOpt = yield* repo
-					.getById(videoId)
-					.pipe(Policy.withPublicPolicy(policy.canView(videoId)));
-
-				return yield* videoOpt.pipe(
-					Effect.transposeMapOption(
-						Effect.fn(function* ([video]) {
-							const [bucket] = yield* S3Buckets.getBucketAccess(video.bucketId);
-
-							const listResponse = yield* bucket.listObjects({
-								prefix: `${video.ownerId}/${video.id}/`,
-							});
-							const contents = listResponse.Contents || [];
-
-							const thumbnailKey = contents.find((item) =>
-								item.Key?.endsWith("screen-capture.jpg"),
-							)?.Key;
-
-							if (!thumbnailKey) return Option.none();
-
-							return Option.some(
-								yield* bucket.getSignedObjectUrl(thumbnailKey),
-							);
-						}),
-					),
-					Effect.map(Option.flatten),
-				);
-			}),
+			getThumbnailURL: Effect.fn("Videos.getThumbnailURL")((videoId) =>
+				repo
+					.getById(videoId)
+					.pipe(
+						Policy.withPublicPolicy(policy.canView(videoId)),
+						Effect.map(([video]) => video),
+						Effect.flatMap((video) =>
+							Effect.gen(function* () {
+								const [bucket] = yield* S3Buckets.getBucketAccess(
+									video.bucketId,
+								);
+
+								const listResponse = yield* bucket.listObjects({
+									prefix: `${video.ownerId}/${video.id}/`,
+								});
+
+								const thumbnailKey = listResponse.Contents?.find((item) =>
+									item.Key?.endsWith("screen-capture.jpg"),
+								)?.Key;
+
+								if (!thumbnailKey) return Option.none<string>();
+
+								const url = yield* bucket.getSignedObjectUrl(thumbnailKey);
+								return Option.some(url);
+							}),
+						),
+					),
+			),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const videoOpt = yield* repo
.getById(videoId)
.pipe(Policy.withPublicPolicy(policy.canView(videoId)));
return yield* videoOpt.pipe(
Effect.transposeMapOption(
Effect.fn(function* ([video]) {
const [bucket] = yield* S3Buckets.getBucketAccess(video.bucketId);
const listResponse = yield* bucket.listObjects({
prefix: `${video.ownerId}/${video.id}/`,
});
const contents = listResponse.Contents || [];
const thumbnailKey = contents.find((item) =>
item.Key?.endsWith("screen-capture.jpg"),
)?.Key;
if (!thumbnailKey) return Option.none();
return Option.some(
yield* bucket.getSignedObjectUrl(thumbnailKey),
);
}),
),
Effect.map(Option.flatten),
);
getThumbnailURL: Effect.fn("Videos.getThumbnailURL")((videoId) =>
repo
.getById(videoId)
.pipe(
Policy.withPublicPolicy(policy.canView(videoId)),
Effect.map(([video]) => video),
Effect.flatMap((video) =>
Effect.gen(function* () {
const [bucket] = yield* S3Buckets.getBucketAccess(video.bucketId);
const listResponse = yield* bucket.listObjects({
prefix: `${video.ownerId}/${video.id}/`,
});
const thumbnailKey = listResponse.Contents?.find((item) =>
item.Key?.endsWith("screen-capture.jpg"),
)?.Key;
if (!thumbnailKey) return Option.none<string>();
const url = yield* bucket.getSignedObjectUrl(thumbnailKey);
return Option.some(url);
}),
),
),
),
🤖 Prompt for AI Agents
In packages/web-backend/src/Videos/index.ts around lines 167 to 193, the code
yields the result of repo.getById(...) into videoOpt and then calls .pipe on
that resolved tuple (causing a runtime error). Instead, keep the operations
inside the Effect pipeline: call
repo.getById(videoId).pipe(Policy.withPublicPolicy(...)).pipe(Effect.transposeMapOption(...),
Effect.map(Option.flatten)) and return yield* of that whole pipeline; do not
assign the yielded tuple to videoOpt or call .pipe on a resolved value. Ensure
any yield* remain only inside the inner generator (e.g., when calling
S3Buckets.getBucketAccess and bucket methods).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai videoOpt is an Option<T>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🌐 Web query:

Effect-TS transposeMapOption how does it work with Option pipe

💡 Result:

Short answer

  • Effect.transposeMapOption lets you run an Effect only when an Option is Some; None immediately becomes Effect.succeed(Option.none()). When Some(a), it runs f(a) and wraps the result in Some. See signature and examples. [1]

How it works (concise)

  • Input: Option.
  • If Option is None: returns an Effect that succeeds with Option.none (no effect f run).
  • If Option is Some(a): runs f(a) :: Effect<B, E, R> and returns Effect that succeeds with Option.some(b) (or fails if f fails).
  • Signature: (f: A => Effect<B, E, R>) => (opt: Option) => Effect<Option, E, R>. [1]

Example

  • Using pipe:
    import { pipe } from "effect"
    import * as Option from "effect/Option"
    import * as Effect from "effect/Effect"

    const noneEff = pipe(Option.none(), Effect.transposeMapOption(() => Effect.succeed(42)))
    // noneEff runs to Option.none

    const someEff = pipe(Option.some(21), Effect.transposeMapOption(a => Effect.succeed(a * 2)))
    // someEff runs to Option.some(42)

Reference

  • Effect.transposeMapOption docs. [1]
  • Related Either.transposeMapOption (same transpose idea for Either/Option). [2]

@Brendonovich, you're absolutely right! I apologize for the incorrect analysis. If videoOpt is Option<T> as you've confirmed, then Policy.withPublicPolicy must be returning Effect<Option<[Video]>, E, R> in this context, making the .pipe() call and use of Effect.transposeMapOption entirely correct.

Thank you for the clarification. I'll disregard my previous suggestion.

    (\  /)
    (o.o)  ← learning moment!
    (> <)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (5)
packages/web-backend/src/Videos/index.ts (1)

164-194: Consider optimizing object listing for thumbnail lookup.

The current implementation lists all objects under the video prefix to find the thumbnail. For better performance, consider:

  1. If the thumbnail filename is deterministic, construct the key directly: ${video.ownerId}/${video.id}/screen-capture.jpg
  2. If listing is necessary, add maxKeys: 100 to the listObjects call to limit the response size

Example optimization if the filename is deterministic:

 getThumbnailURL: Effect.fn("Videos.getThumbnailURL")(function* (
   videoId: Video.VideoId,
 ) {
   const videoOpt = yield* repo
     .getById(videoId)
     .pipe(Policy.withPublicPolicy(policy.canView(videoId)));

   return yield* videoOpt.pipe(
     Effect.transposeMapOption(
       Effect.fn(function* ([video]) {
         const [bucket] = yield* S3Buckets.getBucketAccess(video.bucketId);

-        const listResponse = yield* bucket.listObjects({
-          prefix: `${video.ownerId}/${video.id}/`,
-        });
-        const contents = listResponse.Contents || [];
-
-        const thumbnailKey = contents.find((item) =>
-          item.Key?.endsWith("screen-capture.jpg"),
-        )?.Key;
-
-        if (!thumbnailKey) return Option.none();
+        const thumbnailKey = `${video.ownerId}/${video.id}/screen-capture.jpg`;

         return Option.some(
           yield* bucket.getSignedObjectUrl(thumbnailKey),
         );
       }),
     ),
     Effect.map(Option.flatten),
   );
 }),
apps/web/lib/ThumbnailRequest.ts (2)

1-14: Align types to domain export and strengthen queryKey typing.

Use VideoId directly (the domain exports it at top-level), and type queryKey accordingly.

-import type { Video } from "@cap/web-domain";
+import type { VideoId } from "@cap/web-domain";
@@
-export const queryKey = (videoId: string) => ["thumbnail", videoId] as const;
+export const queryKey = (videoId: VideoId) => ["thumbnail", videoId] as const;
@@
-  { videoId: Video.VideoId }
+  { videoId: VideoId }

22-45: Avoid index-coupling; map responses by id for robustness.

If the RPC ever reorders results, index-based matching misroutes completions. Prefer an id->result map and resolve by request.videoId.

apps/web/components/VideoThumbnail.tsx (2)

51-61: Rename hook: useThumnailQuery → useThumbnailQuery.

Fix spelling and update usage.

-export const useThumnailQuery = (videoId: Video.VideoId) => {
+export const useThumbnailQuery = (videoId: VideoId) => {
   return useEffectQuery({
     queryKey: ThumbnailRequest.queryKey(videoId),
     queryFn: Effect.fn(function* () {
       return yield* Effect.request(
         new ThumbnailRequest.ThumbnailRequest({ videoId }),
         yield* ThumbnailRequest.DataLoaderResolver,
       );
     }),
   });
 };
@@
-    const thumbnailUrl = useThumnailQuery(videoId);
+    const thumbnailUrl = useThumbnailQuery(videoId);

Also applies to: 74-74


7-7: Remove any-cast on objectFit; type it precisely.

Use React’s CSSProperties type and drop the cast.

-import { memo, useEffect, useRef } from "react";
+import { memo, useEffect, useRef, type CSSProperties } from "react";
@@
-  objectFit?: string;
+  objectFit?: CSSProperties["objectFit"];
@@
-            style={{ objectFit: objectFit as any }}
+            style={{ objectFit }}

Also applies to: 17-17, 112-112

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cd8e32f and ac553b1.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (10)
  • apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (2 hunks)
  • apps/web/components/VideoThumbnail.tsx (5 hunks)
  • apps/web/lib/EffectRuntime.ts (1 hunks)
  • apps/web/lib/ThumbnailRequest.ts (1 hunks)
  • apps/web/package.json (1 hunks)
  • apps/web/utils/helpers.ts (1 hunks)
  • packages/web-backend/src/Videos/VideosRpcs.ts (2 hunks)
  • packages/web-backend/src/Videos/index.ts (1 hunks)
  • packages/web-domain/src/Policy.ts (1 hunks)
  • packages/web-domain/src/Video.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx,js,jsx,rs}

📄 CodeRabbit inference engine (CLAUDE.md)

Do not add inline, block, or docstring comments in any language; code must be self-explanatory

Files:

  • packages/web-backend/src/Videos/VideosRpcs.ts
  • packages/web-backend/src/Videos/index.ts
  • apps/web/lib/ThumbnailRequest.ts
  • apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
  • packages/web-domain/src/Video.ts
  • apps/web/utils/helpers.ts
  • packages/web-domain/src/Policy.ts
  • apps/web/components/VideoThumbnail.tsx
  • apps/web/lib/EffectRuntime.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Use strict TypeScript and avoid any; leverage shared types from packages

**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by running pnpm format.

Files:

  • packages/web-backend/src/Videos/VideosRpcs.ts
  • packages/web-backend/src/Videos/index.ts
  • apps/web/lib/ThumbnailRequest.ts
  • apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
  • packages/web-domain/src/Video.ts
  • apps/web/utils/helpers.ts
  • packages/web-domain/src/Policy.ts
  • apps/web/components/VideoThumbnail.tsx
  • apps/web/lib/EffectRuntime.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:

  • packages/web-backend/src/Videos/VideosRpcs.ts
  • packages/web-backend/src/Videos/index.ts
  • apps/web/lib/ThumbnailRequest.ts
  • apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
  • packages/web-domain/src/Video.ts
  • apps/web/utils/helpers.ts
  • packages/web-domain/src/Policy.ts
  • apps/web/components/VideoThumbnail.tsx
  • apps/web/lib/EffectRuntime.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 data fetching in the web app
Web mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData rather than broad invalidations
Client code should use useEffectQuery/useEffectMutation and useRpcClient from apps/web/lib/EffectRuntime.ts; do not create ManagedRuntime inside components

Files:

  • apps/web/lib/ThumbnailRequest.ts
  • apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
  • apps/web/utils/helpers.ts
  • apps/web/components/VideoThumbnail.tsx
  • apps/web/lib/EffectRuntime.ts
apps/web/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

On the client, always use useEffectQuery or useEffectMutation from @/lib/EffectRuntime; never call EffectRuntime.run* directly in components.

Files:

  • apps/web/lib/ThumbnailRequest.ts
  • apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
  • apps/web/utils/helpers.ts
  • apps/web/components/VideoThumbnail.tsx
  • apps/web/lib/EffectRuntime.ts
apps/web/app/**/*.{tsx,ts}

📄 CodeRabbit inference engine (CLAUDE.md)

Prefer Server Components for initial data in the Next.js App Router and pass initialData to client components

Files:

  • apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-22T14:17:47.407Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-22T14:17:47.407Z
Learning: Applies to apps/web/**/*.{ts,tsx} : Client code should use useEffectQuery/useEffectMutation and useRpcClient from apps/web/lib/EffectRuntime.ts; do not create ManagedRuntime inside components

Applied to files:

  • apps/web/lib/EffectRuntime.ts
🧬 Code graph analysis (7)
packages/web-backend/src/Videos/VideosRpcs.ts (2)
packages/web-domain/src/Errors.ts (1)
  • InternalError (3-6)
packages/web-backend/src/Auth.ts (1)
  • provideOptionalAuth (91-113)
packages/web-backend/src/Videos/index.ts (3)
packages/web-domain/src/Video.ts (3)
  • Video (16-59)
  • VideoId (12-12)
  • VideoId (13-13)
packages/web-domain/src/Policy.ts (2)
  • Policy (8-12)
  • policy (29-41)
packages/web-backend/src/S3Buckets/index.ts (1)
  • S3Buckets (13-205)
apps/web/lib/ThumbnailRequest.ts (2)
packages/web-domain/src/Video.ts (3)
  • Video (16-59)
  • VideoId (12-12)
  • VideoId (13-13)
apps/web/lib/Rpcs.ts (1)
  • Rpc (11-14)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)
apps/web/lib/ThumbnailRequest.ts (1)
  • ThumbnailRequest (10-14)
packages/web-domain/src/Video.ts (2)
packages/web-domain/src/Policy.ts (1)
  • PolicyDeniedError (20-24)
packages/web-domain/src/Errors.ts (1)
  • InternalError (3-6)
apps/web/components/VideoThumbnail.tsx (3)
packages/web-domain/src/Video.ts (3)
  • Video (16-59)
  • VideoId (12-12)
  • VideoId (13-13)
apps/web/lib/EffectRuntime.ts (1)
  • useEffectQuery (20-20)
apps/web/lib/ThumbnailRequest.ts (1)
  • ThumbnailRequest (10-14)
apps/web/lib/EffectRuntime.ts (2)
apps/web/lib/ThumbnailRequest.ts (1)
  • ThumbnailRequest (10-14)
apps/web/lib/Rpcs.ts (1)
  • Rpc (11-14)
⏰ 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). (4)
  • GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
  • GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
  • GitHub Check: Typecheck
  • GitHub Check: Analyze (rust)
🔇 Additional comments (9)
apps/web/package.json (1)

31-31: LGTM!

The @effect/experimental dependency addition is appropriate for the new DataLoader-based thumbnail fetching architecture introduced in this PR.

apps/web/utils/helpers.ts (1)

1-1: LGTM!

Valid cleanup of unused serverEnv import.

packages/web-domain/src/Policy.ts (1)

4-4: LGTM!

Valid cleanup of unused Brand import.

apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (2)

20-20: LGTM!

The import change aligns with the new ThumbnailRequest-based data loading architecture.


498-500: LGTM!

The query key update correctly integrates with the new ThumbnailRequest-based thumbnail fetching flow.

packages/web-backend/src/Videos/VideosRpcs.ts (1)

2-2: LGTM!

The import additions correctly support the new RPC implementation's control flow and error handling.

apps/web/lib/EffectRuntime.ts (2)

8-8: LGTM!

The import addition correctly supports the expanded RuntimeLayer.


10-14: LGTM!

The RuntimeLayer expansion correctly integrates the ThumbnailRequest data loader, enabling batched thumbnail fetching throughout the application via the centralized runtime.

Based on learnings

apps/web/components/VideoThumbnail.tsx (1)

51-61: RuntimeLayer already includes ThumbnailRequest.DataLoaderResolver.Default. No further action needed.

@@ -1,14 +1,17 @@
import { LogoSpinner } from "@cap/ui";
import { queryOptions, useQuery } from "@tanstack/react-query";
import type { Video } from "@cap/web-domain";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Use VideoId type from domain (not Video.VideoId).

Domain exports VideoId at top-level. Update imports and annotations.

-import type { Video } from "@cap/web-domain";
+import type { VideoId } from "@cap/web-domain";
@@
-  videoId: Video.VideoId;
+  videoId: VideoId;
@@
-export const useThumnailQuery = (videoId: Video.VideoId) => {
+export const useThumbnailQuery = (videoId: VideoId) => {

Also applies to: 14-14, 51-51

🤖 Prompt for AI Agents
In apps/web/components/VideoThumbnail.tsx around lines 2, 14, and 51, the code
currently references Video.VideoId but the domain exports VideoId at the top
level; update the import to pull VideoId from "@cap/web-domain" (alongside or
instead of Video) and replace all usages of Video.VideoId with the top-level
VideoId type in props, annotations and any local variables so the file uses
VideoId directly.

Comment on lines 22 to 34
const requestResolver = RequestResolver.makeBatched(
(requests: NonEmptyArray<ThumbnailRequest>) => {
return rpc.VideosGetThumbanils(requests.map((r) => r.videoId)).pipe(
Effect.flatMap(
// biome-ignore lint/suspicious/useIterableCallbackReturn: effect
Effect.forEach((result, index) =>
Effect.matchEffect(Effect.flatten(result), {
onSuccess: (v) => Request.succeed(requests[index]!, v),
onFailure: (e) => Request.fail(requests[index]!, e),
}),
),
),
Effect.catchAll((error) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Batched resolver never iterates results (incorrect Effect.forEach usage).

You’re passing only the callback to Effect.forEach. It needs the source iterable. This prevents requests from being completed. Fix by mapping over results and discarding the output.

-              Effect.flatMap(
-                // biome-ignore lint/suspicious/useIterableCallbackReturn: effect
-                Effect.forEach((result, index) =>
-                  Effect.matchEffect(Effect.flatten(result), {
-                    onSuccess: (v) => Request.succeed(requests[index]!, v),
-                    onFailure: (e) => Request.fail(requests[index]!, e),
-                  }),
-                ),
-              ),
+              Effect.flatMap((results) =>
+                Effect.forEach(
+                  requests,
+                  (request, index) =>
+                    Effect.matchEffect(Effect.flatten(results[index]!), {
+                      onSuccess: (v) => Request.succeed(request, v),
+                      onFailure: (e) => Request.fail(request, e),
+                    }),
+                  { discard: true },
+                ),
+              ),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const requestResolver = RequestResolver.makeBatched(
(requests: NonEmptyArray<ThumbnailRequest>) => {
return rpc.VideosGetThumbanils(requests.map((r) => r.videoId)).pipe(
Effect.flatMap(
// biome-ignore lint/suspicious/useIterableCallbackReturn: effect
Effect.forEach((result, index) =>
Effect.matchEffect(Effect.flatten(result), {
onSuccess: (v) => Request.succeed(requests[index]!, v),
onFailure: (e) => Request.fail(requests[index]!, e),
}),
),
),
Effect.catchAll((error) =>
const requestResolver = RequestResolver.makeBatched(
(requests: NonEmptyArray<ThumbnailRequest>) => {
return rpc.VideosGetThumbanils(requests.map((r) => r.videoId)).pipe(
Effect.flatMap((results) =>
Effect.forEach(
requests,
(request, index) =>
Effect.matchEffect(Effect.flatten(results[index]!), {
onSuccess: (v) => Request.succeed(request, v),
onFailure: (e) => Request.fail(request, e),
}),
{ discard: true },
),
),
Effect.catchAll((error) =>
🤖 Prompt for AI Agents
In apps/web/lib/ThumbnailRequest.ts around lines 22 to 34, the batched resolver
calls Effect.forEach with only a callback so it never iterates the results; fix
by passing the results iterable as the first argument to Effect.forEach and keep
the same callback that flattens each result and calls
Request.succeed/Request.fail with the corresponding requests[index]; finally
discard the forEach output (e.g., map/asUnit) so the surrounding pipeline
remains typed correctly.


const requestResolver = RequestResolver.makeBatched(
(requests: NonEmptyArray<ThumbnailRequest>) => {
return rpc.VideosGetThumbanils(requests.map((r) => r.videoId)).pipe(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Typo in RPC method name (“VideosGetThumbanils”).

Public API name has a spelling error; please rename to VideosGetThumbnails across frontend and backend before merging.

-            return rpc.VideosGetThumbanils(requests.map((r) => r.videoId)).pipe(
+            return rpc.VideosGetThumbnails(requests.map((r) => r.videoId)).pipe(

Would you like a follow-up script to find and rewrite usages repo-wide?


🏁 Script executed:

#!/bin/bash
rg -n "VideosGetThumbanils"
rg -n "VideosGetThumbnails"

Length of output: 269


Fix typo in RPC method name: VideosGetThumbanils → VideosGetThumbnails

  • Replace VideosGetThumbanils with VideosGetThumbnails in:
    • packages/web-backend/src/Videos/VideosRpcs.ts (RPC definition)
    • packages/web-domain/src/Video.ts (Rpc.make call)
    • apps/web/lib/ThumbnailRequest.ts (rpc.VideosGet… usage)
🤖 Prompt for AI Agents
In apps/web/lib/ThumbnailRequest.ts around line 24, the RPC method name is
misspelled as VideosGetThumbanils; update this call to rpc.VideosGetThumbnails.
Also fix the same typo in packages/web-backend/src/Videos/VideosRpcs.ts (RPC
export/definition) and in packages/web-domain/src/Video.ts where Rpc.make is
invoked so the exported name and all callers match VideosGetThumbnails; after
renaming, update any imports/uses if necessary and run a TypeScript build to
ensure there are no remaining references to the old misspelled identifier.

Comment on lines 172 to 188
Rpc.make("VideosGetThumbanils", {
payload: Schema.Array(VideoId).pipe(
Schema.filter((a) => a.length <= 50 || "Maximum of 50 videos at a time"),
),
success: Schema.Array(
Schema.Exit({
success: Schema.Option(Schema.String),
failure: Schema.Union(
NotFoundError,
PolicyDeniedError,
VerifyVideoPasswordError,
),
defect: Schema.Unknown,
}),
),
error: InternalError,
}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix typo in RPC name.

The RPC name VideosGetThumbanils has a typo. It should be VideosGetThumbnails (note the spelling of "Thumbnails").

Apply this diff to fix the typo:

-	Rpc.make("VideosGetThumbanils", {
+	Rpc.make("VideosGetThumbnails", {
 		payload: Schema.Array(VideoId).pipe(
 			Schema.filter((a) => a.length <= 50 || "Maximum of 50 videos at a time"),
 		),

Note: This typo also appears in the backend implementation at packages/web-backend/src/Videos/VideosRpcs.ts line 47 and will need to be fixed there as well.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Rpc.make("VideosGetThumbanils", {
payload: Schema.Array(VideoId).pipe(
Schema.filter((a) => a.length <= 50 || "Maximum of 50 videos at a time"),
),
success: Schema.Array(
Schema.Exit({
success: Schema.Option(Schema.String),
failure: Schema.Union(
NotFoundError,
PolicyDeniedError,
VerifyVideoPasswordError,
),
defect: Schema.Unknown,
}),
),
error: InternalError,
}),
Rpc.make("VideosGetThumbnails", {
payload: Schema.Array(VideoId).pipe(
Schema.filter((a) => a.length <= 50 || "Maximum of 50 videos at a time"),
),
success: Schema.Array(
Schema.Exit({
success: Schema.Option(Schema.String),
failure: Schema.Union(
NotFoundError,
PolicyDeniedError,
VerifyVideoPasswordError,
),
defect: Schema.Unknown,
}),
),
error: InternalError,
}),
🤖 Prompt for AI Agents
In packages/web-domain/src/Video.ts around lines 172 to 188 the RPC name is
misspelled as "VideosGetThumbanils"; update the RPC key string to
"VideosGetThumbnails" to correct the typo. Also mirror this fix in the backend
at packages/web-backend/src/Videos/VideosRpcs.ts line ~47 by renaming the RPC
registration/handler from "VideosGetThumbanils" to "VideosGetThumbnails" so
client and server names match.

@Brendonovich Brendonovich merged commit 31df433 into main Oct 10, 2025
14 of 15 checks passed
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
apps/web/lib/ThumbnailRequest.ts (1)

22-34: Effect.forEach called with only callback—batched resolver never iterates results.

Line 27 passes only a callback to Effect.forEach without the required iterable first argument. This prevents the resolver from completing any requests, as it never iterates over the batch results.

Apply this diff to fix the incorrect Effect.forEach usage:

 				const requestResolver = RequestResolver.makeBatched(
 					(requests: NonEmptyArray<ThumbnailRequest>) => {
 						return rpc.VideosGetThumbnails(requests.map((r) => r.videoId)).pipe(
-							Effect.flatMap(
-								// biome-ignore lint/suspicious/useIterableCallbackReturn: effect
-								Effect.forEach((result, index) =>
-									Effect.matchEffect(Effect.flatten(result), {
-										onSuccess: (v) => Request.succeed(requests[index]!, v),
-										onFailure: (e) => Request.fail(requests[index]!, e),
-									}),
-								),
-							),
+							Effect.flatMap((results) =>
+								Effect.forEach(
+									results,
+									(result, index) =>
+										Effect.matchEffect(Effect.flatten(result), {
+											onSuccess: (v) => Request.succeed(requests[index]!, v),
+											onFailure: (e) => Request.fail(requests[index]!, e),
+										}),
+									{ discard: true },
+								),
+							),
 							Effect.catchAll((error) =>
 								Effect.forEach(
 									requests,
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ac553b1 and e5d0d4b.

📒 Files selected for processing (3)
  • apps/web/lib/ThumbnailRequest.ts (1 hunks)
  • packages/web-backend/src/Videos/VideosRpcs.ts (2 hunks)
  • packages/web-domain/src/Video.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/web-domain/src/Video.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,rs}

📄 CodeRabbit inference engine (CLAUDE.md)

Do not add inline, block, or docstring comments in any language; code must be self-explanatory

Files:

  • packages/web-backend/src/Videos/VideosRpcs.ts
  • apps/web/lib/ThumbnailRequest.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Use strict TypeScript and avoid any; leverage shared types from packages

**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by running pnpm format.

Files:

  • packages/web-backend/src/Videos/VideosRpcs.ts
  • apps/web/lib/ThumbnailRequest.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:

  • packages/web-backend/src/Videos/VideosRpcs.ts
  • apps/web/lib/ThumbnailRequest.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 data fetching in the web app
Web mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData rather than broad invalidations
Client code should use useEffectQuery/useEffectMutation and useRpcClient from apps/web/lib/EffectRuntime.ts; do not create ManagedRuntime inside components

Files:

  • apps/web/lib/ThumbnailRequest.ts
apps/web/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

On the client, always use useEffectQuery or useEffectMutation from @/lib/EffectRuntime; never call EffectRuntime.run* directly in components.

Files:

  • apps/web/lib/ThumbnailRequest.ts
🧬 Code graph analysis (2)
packages/web-backend/src/Videos/VideosRpcs.ts (2)
packages/web-domain/src/Errors.ts (1)
  • InternalError (3-6)
packages/web-backend/src/Auth.ts (1)
  • provideOptionalAuth (91-113)
apps/web/lib/ThumbnailRequest.ts (2)
packages/web-domain/src/Video.ts (3)
  • Video (16-59)
  • VideoId (12-12)
  • VideoId (13-13)
apps/web/lib/Rpcs.ts (1)
  • Rpc (11-14)
⏰ 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). (4)
  • GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
  • GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
  • GitHub Check: Typecheck
  • GitHub Check: Analyze (rust)

Comment on lines +47 to +72
VideosGetThumbnails: (videoIds) =>
Effect.all(
videoIds.map((id) =>
videos.getThumbnailURL(id).pipe(
Effect.catchTags({
DatabaseError: () => new InternalError({ type: "database" }),
S3Error: () => new InternalError({ type: "s3" }),
}),
Effect.matchEffect({
onSuccess: (v) => Effect.succeed(Exit.succeed(v)),
onFailure: (e) =>
Schema.is(InternalError)(e)
? Effect.fail(e)
: Effect.succeed(Exit.fail(e)),
}),
Effect.map((v) => Unify.unify(v)),
),
),
{ concurrency: 10 },
).pipe(
provideOptionalAuth,
Effect.catchTags({
DatabaseError: () => new InternalError({ type: "database" }),
UnknownException: () => new InternalError({ type: "unknown" }),
}),
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Per-item errors fail the entire batch instead of being wrapped in Exit.

The current error handling converts DatabaseError and S3Error to InternalError via catchTags (lines 51-54), then uses matchEffect to check if the error is InternalError and fails the entire batch if true (lines 58-59). This means any per-item database or S3 error (e.g., video not found, thumbnail missing) fails the entire batch instead of being recorded as an individual failure.

For a batched endpoint, the typical pattern is:

  • Wrap per-item successes in Exit.succeed
  • Wrap per-item failures in Exit.fail
  • Return an array of Exit results
  • Only fail the batch for system-level errors (connection failures, etc.)

The frontend resolver in ThumbnailRequest.ts expects an array of Exit results and calls Request.succeed or Request.fail on each request individually (lines 27-31), confirming this expectation.

Apply this diff to wrap per-item errors in Exit.fail instead of failing the batch:

 			VideosGetThumbnails: (videoIds) =>
 				Effect.all(
 					videoIds.map((id) =>
 						videos.getThumbnailURL(id).pipe(
-							Effect.catchTags({
-								DatabaseError: () => new InternalError({ type: "database" }),
-								S3Error: () => new InternalError({ type: "s3" }),
-							}),
 							Effect.matchEffect({
 								onSuccess: (v) => Effect.succeed(Exit.succeed(v)),
-								onFailure: (e) =>
-									Schema.is(InternalError)(e)
-										? Effect.fail(e)
-										: Effect.succeed(Exit.fail(e)),
+								onFailure: (e) => {
+									const mappedError =
+										e._tag === "DatabaseError"
+											? new InternalError({ type: "database" })
+											: e._tag === "S3Error"
+												? new InternalError({ type: "s3" })
+												: e;
+									return Effect.succeed(Exit.fail(mappedError));
+								},
 							}),
-							Effect.map((v) => Unify.unify(v)),
 						),
 					),
 					{ concurrency: 10 },
 				).pipe(
 					provideOptionalAuth,
 					Effect.catchTags({
 						DatabaseError: () => new InternalError({ type: "database" }),
 						UnknownException: () => new InternalError({ type: "unknown" }),
 					}),
 				),

This ensures per-item errors are wrapped in Exit.fail while system-level errors (from provideOptionalAuth or Effect.all) still fail the batch. The Unify.unify call is also removed as it's unnecessary after matchEffect always returns Effect<Exit<A, E>>.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
VideosGetThumbnails: (videoIds) =>
Effect.all(
videoIds.map((id) =>
videos.getThumbnailURL(id).pipe(
Effect.catchTags({
DatabaseError: () => new InternalError({ type: "database" }),
S3Error: () => new InternalError({ type: "s3" }),
}),
Effect.matchEffect({
onSuccess: (v) => Effect.succeed(Exit.succeed(v)),
onFailure: (e) =>
Schema.is(InternalError)(e)
? Effect.fail(e)
: Effect.succeed(Exit.fail(e)),
}),
Effect.map((v) => Unify.unify(v)),
),
),
{ concurrency: 10 },
).pipe(
provideOptionalAuth,
Effect.catchTags({
DatabaseError: () => new InternalError({ type: "database" }),
UnknownException: () => new InternalError({ type: "unknown" }),
}),
),
VideosGetThumbnails: (videoIds) =>
Effect.all(
videoIds.map((id) =>
videos.getThumbnailURL(id).pipe(
Effect.matchEffect({
onSuccess: (v) => Effect.succeed(Exit.succeed(v)),
onFailure: (e) => {
const mappedError =
e._tag === "DatabaseError"
? new InternalError({ type: "database" })
: e._tag === "S3Error"
? new InternalError({ type: "s3" })
: e;
return Effect.succeed(Exit.fail(mappedError));
},
}),
),
),
{ concurrency: 10 },
).pipe(
provideOptionalAuth,
Effect.catchTags({
DatabaseError: () => new InternalError({ type: "database" }),
UnknownException: () => new InternalError({ type: "unknown" }),
}),
),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant