Start using Effect for HTTP and RPC #869
Conversation
|
Warning Rate limit exceeded@Brendonovich has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughIntroduces a new domain package and backend package with Effect-based services, RPCs, policies, and S3 abstractions; migrates UI flows to an Effect runtime and RPC client; rewrites several API routes to Effect-driven handlers; strengthens branded ID types and TypeScript project references; removes legacy server actions and adds tracing/config updates. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant EffectRuntime
participant RpcClient
participant ERPC
participant Backend
participant DB
participant S3
UI->>EffectRuntime: invoke mutation (e.g. delete/duplicate video)
EffectRuntime->>RpcClient: withRpc(VideoDelete/VideoDuplicate)
RpcClient->>ERPC: POST /api/erpc (RPC call)
ERPC->>Backend: dispatch to Videos/Folders service with auth
Backend->>DB: read/update/delete video or folder records
Backend->>S3: list/copy/delete objects as needed
S3-->>Backend: return result
DB-->>Backend: return result
Backend-->>ERPC: success or mapped error
ERPC-->>RpcClient: response payload
RpcClient-->>EffectRuntime: Exit result
EffectRuntime-->>UI: update state, show toasts, refresh
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 61
🔭 Outside diff range comments (5)
apps/web/actions/organization/update-space.ts (1)
32-44: Replacing all members drops roles, allows self-removal, and can create duplicates
All existing roles (e.g., owner/admin) are lost because new rows default to role "member".
The actor can remove themselves and/or leave the space without an owner.
Duplicates are possible because the composite index on (spaceId, userId) is not unique.
No transaction around updates => inconsistent state if any step fails.
Wrap the updates in a DB transaction.
Preserve existing roles for users that remain, default new users to "member".
Enforce at least one "owner" remains and prevent self-removal if that would leave zero owners.
De-duplicate the member list before insert; consider adding a unique constraint on (spaceId, userId) at the schema level.
Illustrative refactor (adjust to your Drizzle API):
- // Update members (simple replace for now) - await db().delete(spaceMembers).where(eq(spaceMembers.spaceId, id)); - if (members.length > 0) { - await db() - .insert(spaceMembers) - .values( - members.map((userId) => ({ - id: uuidv4().substring(0, nanoIdLength), - spaceId: id, - userId, - })) - ); - } + // Update members with role preservation, dedupe, and safety checks + await db().transaction(async (tx) => { + const uniqueMembers = Array.from(new Set(members)); + + // Preserve existing roles + const existing = await tx + .select({ + userId: spaceMembers.userId, + role: spaceMembers.role, + }) + .from(spaceMembers) + .where(eq(spaceMembers.spaceId, id)); + + const owners = existing.filter((m) => m.role === "owner").map((m) => m.userId); + const actorId = user.id; + + // Prevent removing all owners + const remainingOwners = owners.filter((u) => uniqueMembers.includes(u)); + if (remainingOwners.length === 0 && (!uniqueMembers.includes(actorId) || (existing.find(e => e.userId === actorId)?.role !== "owner"))) { + throw new Error("Operation would leave the space without an owner"); + } + + // Optionally: ensure actor stays a member unless they explicitly remain (policy decision) + if (!uniqueMembers.includes(actorId)) { + uniqueMembers.push(actorId); + } + + // Replace membership, preserving roles when possible + await tx.delete(spaceMembers).where(eq(spaceMembers.spaceId, id)); + if (uniqueMembers.length > 0) { + await tx.insert(spaceMembers).values( + uniqueMembers.map((userId) => ({ + id: uuidv4().substring(0, nanoIdLength), + spaceId: id, + userId, + role: existing.find((m) => m.userId === userId)?.role ?? "member", + })) + ); + } + });Schema follow-up (recommended): make (spaceId, userId) a unique index to prevent duplicates at the DB level.
apps/web/utils/video/ffmpeg/helpers.ts (1)
37-49: HLS correctness: only include AUDIO attribute when audioPlaylistUrl existsCurrently,
AUDIO="audio"is appended even whenaudioPlaylistUrlis null, which yields an invalid master playlist (references an undefined group). Make AUDIO conditional.- const streamInfo = xStreamInfo - ? xStreamInfo + ',AUDIO="audio"' - : `BANDWIDTH=${bandwidth},RESOLUTION=${resolution},AUDIO="audio"`; + const hasAudio = Boolean(audioPlaylistUrl); + const baseInfo = xStreamInfo + ? xStreamInfo + : `BANDWIDTH=${bandwidth},RESOLUTION=${resolution}`; + const streamInfo = hasAudio ? `${baseInfo},AUDIO="audio"` : baseInfo;Optionally, consider adding CODECS if known to improve player compatibility.
apps/web/package.json (2)
130-134: Fix ESLint–eslint-config-next version mismatchYour
apps/web/package.jsonspecifies ESLint 9.x, buteslint-config-next@14.1.0only supports ESLint^7.23.0 || ^8.0.0. This will trigger peer-dependency warnings or broken linting.• File: apps/web/package.json
• Lines: 130–134Recommended fixes (pick one):
- Downgrade ESLint to a supported version, e.g.
"eslint": "^8.44.0"- Upgrade
eslint-config-nextonce a version supporting ESLint 9 is released.
93-101: React 19 is incompatible with Next 14.2.3’s peer dependenciesNext 14.2.3 lists React & ReactDOM
^18.2.0as peers, but yourapps/web/package.jsonis on React^19.1.0. This will cause build/runtime failures.Please update one of the following:
- Downgrade React/ReactDOM to
^18.2.0inapps/web/package.json(lines 93–101), or- Upgrade Next.js to a version that officially supports React 19 (e.g., Next 15+ once it reaches stable).
apps/web/app/(org)/dashboard/caps/page.tsx (1)
167-179: Align aggregate JSON type with actual nullability
JSON_ARRAYAGG(JSON_OBJECT(...))will include entries withnullfields when no organization is joined. The declared TypeScript type currently usesid: string, makingorganization.id !== nulla no-op. Reflect nullability in the type.- sharedOrganizations: sql<{ id: string; name: string; iconUrl: string }[]>` + sharedOrganizations: sql<{ id: string | null; name: string | null; iconUrl: string | null }[]>`With this, the filter becomes type-correct:
- ? video.sharedOrganizations.filter( - (organization) => organization.id !== null - ) + ? video.sharedOrganizations.filter((organization) => organization?.id !== null)Optional: you can also eliminate the client-side filter by changing the aggregation to only emit JSON objects when
organizations.id IS NOT NULL(using aCASE WHENinside the aggregation).Also applies to: 240-244
🧹 Nitpick comments (67)
apps/web/app/Layout/providers.tsx (1)
38-41: Reduce client console noise in prodWarn is appropriate; consider gating it to dev to avoid noisy client logs in production.
Apply:
- console.warn( - "Missing PostHog environment variables. Events will not be tracked." - ); + if (process.env.NODE_ENV !== "production") { + console.warn( + "Missing PostHog environment variables. Events will not be tracked." + ); + }packages/web-api-contract-effect/package.json (1)
8-10: Effect dependency versions are aligned across the workspaceWe ran a quick audit and confirmed that:
- @cap/desktop only depends on
effect@^3.17.7(no@effect/platform)- All other packages (
@cap/effect,@cap/web,@cap/web-api-contract-effect,@cap/web-backend,@cap/web-domain) depend on botheffect@^3.17.7and@effect/platform@^0.90.1No mismatched versions were found.
To enforce a single Effect runtime and avoid duplicate copies in downstream consumers, you may want to move both deps into peerDependencies (and keep them in devDependencies for local builds), for example:
"name": "@cap/web-api-contract-effect", "private": true, "main": "./src/index.ts", "types": "./src/index.ts", "type": "module", - "dependencies": { - "@effect/platform": "^0.90.1", - "effect": "^3.17.7" - } + // If this package is consumed by others, consider extracting Effect into peers: + "peerDependencies": { + "@effect/platform": "^0.90.1", + "effect": "^3.17.7" + }, + "devDependencies": { + "@effect/platform": "^0.90.1", + "effect": "^3.17.7" + }If these are purely internal/private packages and you’re okay bundling Effect directly, the current setup is consistent and you can leave it as-is.
apps/web/app/(org)/dashboard/folder/[id]/page.tsx (1)
7-9: Consider migrating data fetching to the new Effect RPC layerThis page still uses legacy server actions (
getChildFolders,getFolderBreadcrumb,getVideosByFolderId). To align with the PR’s Effect/RPC direction, consider swapping these for RPC calls (e.g., via a server-side Effect runtime or a thin server function invoking domain Rpcs).Happy to sketch the migration using your
Rpcsclient/server utilities.packages/web-domain/src/Rpcs.ts (1)
4-4: Export a stable type alias for consumersExpose the merged RPC type to downstream code for stronger typing.
Apply:
export const Rpcs = VideoRpcs.merge(FolderRpcs); +export type RpcsType = typeof Rpcs;packages/web-domain/src/Errors.ts (1)
3-6: Renametypeto a less ambiguous field, and consider future-proofing
typecan be ambiguous (and is a TS keyword). Considersourceorsystemto clarify intent and avoid conflicts. You may also want acodefor finer-grained classification without leaking internals.Example:
-export class InternalError extends Schema.TaggedError<InternalError>()( - "InternalError", - { type: Schema.Literal("database", "s3") } -) {} +export class InternalError extends Schema.TaggedError<InternalError>()( + "InternalError", + { + source: Schema.Literal("database", "s3"), + // code: Schema.String, // e.g. "DB_QUERY_FAILED" (optional) + } +) {}packages/web-domain/README.md (1)
6-10: Mention the combined RPC surface and add quick import hintsSince the package now aggregates RPCs, consider noting the combined
Rpcsexport and where to import types from for consumers.Example additions:
- Combined RPCs (
Rpcs)- Import example:
import { Rpcs } from "@cap/web-domain/Rpcs"or via@cap/web-domainif re-exported fromindex.tstype RpcsType = typeof Rpcspackages/web-domain/src/Organisation.ts (1)
3-6: Add minimal invariants for name; keep ID brandedSchema is solid. Tighten the model by ensuring name is non-empty.
export class Organisation extends Schema.Class<Organisation>("Organisation")({ id: Schema.String.pipe(Schema.brand("OrganisationId")), - name: Schema.String, + name: Schema.NonEmptyString, }) {}Optionally export the branded ID type for reuse:
export type OrganisationId = Schema.Schema.Type<typeof Organisation>["id"];apps/web/README.md (1)
1-4: Add quickstart and links to the new Effect/RPC architectureNice concise intro. A short Quickstart and pointers to the new Effect runtime/RPC docs would help contributors ramp faster.
Example addition:
Cap's NextJS web app for video sharing. Used for both self hosting and on [cap.so](https://cap.so). + +## Development +- pnpm i +- pnpm -w build +- pnpm dev --filter @cap/web + +## Architecture +- Effect runtime and RPC: see apps/web/lib/EffectRuntime.ts and apps/web/lib/Rpcs.ts +- Shared domain: packages/web-domainpackages/web-backend/src/S3Buckets/S3BucketClientProvider.ts (2)
4-9: Clarify client lifecycle; prefer stable instances or document cachingUsing getters is fine if they return cached, long-lived S3Client instances. Re-creating per call can waste sockets/CPU. Either store clients or guarantee memoization.
Option A (store instances):
export class S3BucketClientProvider extends Context.Tag( "S3BucketClientProvider" )< S3BucketClientProvider, - { getInternal: () => S3Client; getPublic: () => S3Client; bucket: string } + { internal: S3Client; public: S3Client; bucket: string } >() {}Option B (keep getters but memoize in the live layer). Also consider branding
bucket(e.g., S3BucketName) from your domain package for stronger typing.
4-6: Use a more specific tag key to avoid collisionsNamespace the tag identifier to reduce risk of global collisions.
-export class S3BucketClientProvider extends Context.Tag( - "S3BucketClientProvider" +export class S3BucketClientProvider extends Context.Tag( + "cap/web-backend/S3BucketClientProvider" )<packages/effect/src/Organisation.ts (1)
3-6: Promote a reusable branded OrganisationId and reuse in classDefining/exporting the branded ID improves reuse and consistency with other domain models (e.g., VideoId).
+export const OrganisationId = Schema.String.pipe(Schema.brand("OrganisationId")); +export type OrganisationId = typeof OrganisationId.Type; + export class Organisation extends Schema.Class<Organisation>("Organisation")({ - id: Schema.String.pipe(Schema.brand("OrganisationId")), + id: OrganisationId, name: Schema.String, }) {}packages/web-backend/src/Users/index.ts (1)
5-11: Define a service surface earlyAn empty service provides no value to dependents. Consider adding at least method stubs (e.g.,
getById,findByEmail) returningEffectto lock the interface.apps/web/utils/auth.ts (1)
20-20: Unreachable return type variantThe function never returns
"not-org-email". Either remove it from the return union or add logic to return it where appropriate.apps/web/actions/organization/update-space.ts (1)
22-25: Use existence/limit to avoid scanning and reduce payloadThe membership check is correct, but it selects all columns without a limit. Add a limiter (or switch to an existence check) for efficiency.
Apply this minimal diff:
- const [membership] = await db() - .select() - .from(spaceMembers) - .where(and(eq(spaceMembers.spaceId, id), eq(spaceMembers.userId, user.id))); + const [membership] = await db() + .select() + .from(spaceMembers) + .where(and(eq(spaceMembers.spaceId, id), eq(spaceMembers.userId, user.id))) + .limit(1);Alternatively (if available in your Drizzle version), use an existence check to avoid materializing a row.
packages/web-backend/src/Users/UsersRepo.ts (1)
12-16: Return a single row and bound the queryThe repo currently returns an array of rows. Repos typically expose “getById” returning one row or undefined. Also, add a limit for efficiency.
Apply this diff:
- getById: (id: string) => - db.execute((db) => - db.select().from(Db.users).where(Dz.eq(Db.users.id, id)) - ), + getById: (id: string) => + db + .execute((db) => + db + .select() + .from(Db.users) + .where(Dz.eq(Db.users.id, id)) + .limit(1) + ) + .pipe(Effect.map((rows) => rows[0])),Optional:
- Narrow selected columns if you don’t need all fields.
- Type the return as Effect.Effect<InferModel | undefined, DatabaseError> for clarity.
packages/web-domain/tsconfig.json (1)
5-12: Emit declarations explicitly and include maps for DXYou’re already doing emitDeclarationOnly. Add declaration and declarationMap explicitly to avoid ambiguity across TS versions and improve editor jump-to-def.
"compilerOptions": { "baseUrl": ".", "moduleResolution": "bundler", "composite": true, "outDir": "dist", - "noEmit": false, - "emitDeclarationOnly": true + "noEmit": false, + "declaration": true, + "emitDeclarationOnly": true, + "declarationMap": true }packages/web-backend/tsconfig.json (1)
5-13: Small TS ergonomics: declaration flags and baseUrlFor consistency with sibling packages and better DX, explicitly emit declarations/maps and consider baseUrl for relative imports (unless inherited from base config).
"compilerOptions": { - "moduleResolution": "bundler", + "moduleResolution": "bundler", "composite": true, "outDir": "dist", "noEmit": false, - "emitDeclarationOnly": true, + "declaration": true, + "emitDeclarationOnly": true, + "declarationMap": true, "strict": true, "noUncheckedIndexedAccess": true },If not already set in the base config, optionally add:
{ "compilerOptions": { "baseUrl": "." } }packages/database/auth/auth-options.tsx (2)
11-11: Alias import is fine; add type import for getServerSession resultThe alias avoids name shadowing. Consider importing Session type to annotate the wrapper return for clarity.
-import { getServerSession as _getServerSession } from "next-auth"; +import { getServerSession as _getServerSession } from "next-auth"; +import type { Session } from "next-auth";
150-152: Reduce noisy logs in productionThe multi-line log is fine for debugging but consider gating logs via env (NEXT_PUBLIC_DEBUG or NextAuth debug) to avoid noisy prod logs.
apps/web/app/(org)/login/form.tsx (1)
359-360: Nit: Potential low contrast for “Signing in to” text"text-gray-1" on a gray background may be hard to read. Consider a higher-contrast token (e.g., text-gray-9/10) for accessibility.
apps/web/actions/organization/get-organization-sso-data.ts (1)
7-34: Solid server action: validation, minimal return shape, and unified error
- Input validated and errors unified to avoid info leakage.
- Only necessary fields selected.
- Return shape matches client usage.
Optional: add
.limit(1)to the query for clarity, though destructuring the first row is already fine.const [organization] = await db() .select({ workosOrganizationId: organizations.workosOrganizationId, workosConnectionId: organizations.workosConnectionId, name: organizations.name, }) .from(organizations) - .where(eq(organizations.id, organizationId)); + .where(eq(organizations.id, organizationId)) + .limit(1);apps/web/tsconfig.json (1)
23-30: Path alias for “@/services” likely needs wildcard mappingIf you plan to import subpaths (e.g., "@/services/folders"), add a wildcard entry to avoid unresolved module errors.
Apply this diff inside paths:
"@/actions/*": ["actions/*"], - "@/services": ["services"] + "@/services": ["services"], + "@/services/*": ["services/*"]packages/web-backend/src/Database.ts (1)
13-15: Preserve root cause on DatabaseError for observabilityIncluding a cause makes debugging far easier and plays well with Effect’s error cause chains.
-export class DatabaseError extends Data.TaggedError("DatabaseError")<{ - message: string; -}> {} +export class DatabaseError extends Data.TaggedError("DatabaseError")<{ + message: string; + cause?: unknown; +}> {}When constructing, pass the original error as cause.
packages/web-backend/src/Folders/FoldersPolicy.ts (3)
20-21: Clarify comment wordingMinor wording nit: this policy is about folders, not “space properties”.
- // All space members can edit space properties + // All space members can edit folders within the same space
23-33: Prefer existence check and limit result setSelect only the minimally needed column (or use
.limit(1)), and avoid reusingdbin the inner callback for readability.- const [spaceMember] = yield* db.execute((db) => - db - .select() - .from(Db.spaceMembers) + const [spaceMember] = yield* db.execute((conn) => + conn + .select({ id: Db.spaceMembers.userId }) + .from(Db.spaceMembers) .where( Dz.and( Dz.eq(Db.spaceMembers.userId, user.id), Dz.eq(Db.spaceMembers.spaceId, spaceId) ) ) + .limit(1) );
13-41: Confirm desired failure semantics for policiesRight now, any database failure during the policy check will fail the effect (likely with
DatabaseError) rather than deny by default. Ensure this is intended and consistent with other policies (e.g., VideosPolicy). If you prefer “fail closed,” catch DB errors and returnfalse.If you want to fail closed, I can provide a patch that wraps DB calls with
Effect.catchTag("DatabaseError", () => Effect.succeed(false)).packages/web-backend/README.md (3)
7-8: Fix typo: “stc” → “etc.”Minor typo in the domains list.
-Code is organized horizontally by domain/entity (Video, Folder, stc), +Code is organized horizontally by domain/entity (Video, Folder, etc.),
14-14: Fix heading level increments (MD001)Headings jump from h2 to h4. Use h3 for the sub-sections to comply with markdownlint (MD001).
-#### Repository Services +### Repository Services-#### Policy Services +### Policy Services-#### Domain Services +### Domain ServicesAlso applies to: 20-20, 25-25
35-35: Tighten wording (“a lot of” → “often”)Concise wording per style guidance.
-In a lot of cases, endpoint implementations are thin wrappers around Domain Services. +Often, endpoint implementations are thin wrappers around Domain Services.apps/web/app/(org)/dashboard/caps/components/FoldersDropdown.tsx (4)
10-14: Remove unused icon import
faCopyis only referenced in commented code. Drop the import to avoid unused import warnings.-import { - faCopy, - faEllipsis, - faPencil, - faTrash, -} from "@fortawesome/free-solid-svg-icons"; +import { faEllipsis, faPencil, faTrash } from "@fortawesome/free-solid-svg-icons";
36-36: Remove unused value from context
activeSpaceis not used (only in commented code). Remove to avoid an unused variable.- const { activeSpace } = useDashboardContext(); + // const { activeSpace } = useDashboardContext();Additionally, consider removing the
useDashboardContextimport while the duplicate option remains disabled.
67-73: Guard rename UX: ensure caret selection happens after focusSequence is fine; consider using
queueMicrotaskinstead ofsetTimeout(…, 0)for more explicit microtask scheduling.- setTimeout(() => { + queueMicrotask(() => { nameRef.current?.focus(); nameRef.current?.select(); - }, 0); + });
91-92: Ensure menu closes on actionIf
DropdownMenuItemdoesn’t auto-close on click in your UI lib, you might need to call a close API or wrap actions to ensure the menu closes after "Delete".If it doesn’t auto-close, I can propose the minimal wrapper pattern used elsewhere in the project.
apps/web/app/(org)/dashboard/caps/page.tsx (3)
65-75: Normalize shared spaces/orgs “iconUrl” to a non-empty string in one placeYou already coalesce
iconUrlto""during post-processing. Consider doing it directly in the select to centralize normalization.Example:
iconUrl: sql<string>`COALESCE(${organizations.iconUrl}, '')`Also applies to: 91-102
231-233: Avoid double-fetching shared spaces for empty listsYou already guard inside
getSharedSpacesForVideos, but you can skip the call entirely when there are zero videos for clarity.This is already safe; optional for readability:
const sharedSpacesMap = videoIds.length ? await getSharedSpacesForVideos(videoIds) : {};
249-254: Avoidanyin metadata typePrefer a stronger type if available (e.g., from
@cap/web-domain). If not, narrow the index signature.- metadata: video.metadata as - | { - customCreatedAt?: string; - [key: string]: any; - } - | undefined, + metadata: video.metadata as + | { + customCreatedAt?: string; + [key: string]: unknown; + } + | undefined,packages/web-domain/src/S3Bucket.ts (1)
6-16: Consistency with Video: prefer a static decodeSync on the class and drop the free function.Aligns with
Video.Video.decodeSyncusage across the codebase.export class S3Bucket extends Schema.Class<S3Bucket>("S3Bucket")({ id: Schema.String, ownerId: Schema.String, region: Schema.String, endpoint: Schema.OptionFromNullOr(Schema.String), name: Schema.String, accessKeyId: Schema.String, secretAccessKey: Schema.String, -}) {} +}) { + static decodeSync = Schema.decodeSync(S3Bucket); +} -export const decodeSync = Schema.decodeSync(S3Bucket);packages/web-backend/src/Videos/VideosPolicy.ts (1)
12-33: Authorize public videos without password and add tracing name.Short-circuit for public videos and name the effect for better tracing/observability.
- const canView = (videoId: Video.VideoId) => - Policy.publicPolicy( - Effect.fn(function* (user) { + const canView = (videoId: Video.VideoId) => + Policy.publicPolicy( + Effect.fn("VideosPolicy.canView", function* (user) { const res = yield* repo.getById(videoId); if (Option.isNone(res)) return true; const [video, password] = res.value; + // Public videos do not require password or ownership checks + if (video.public) return true; + if ( user.pipe( Option.filter((user) => user.id === video.ownerId), Option.isSome ) ) return true; yield* Video.verifyPassword(video, password); return true; }) );Confirm that
Video.verifyPasswordhandles the case wherepasswordisOption.none(i.e., password-protected video with no supplied password) by denying appropriately.apps/web/app/(org)/dashboard/spaces/[spaceId]/SharedCaps.tsx (2)
104-109: Encode query param and drop unnecessary GET headers.Use
encodeURIComponentfor safety and omit redundant headers for a GET.- const response = await fetch(`/api/analytics?videoId=${video.id}`, { - method: "GET", - headers: { - "Content-Type": "application/json", - }, - }); + const response = await fetch( + `/api/analytics?videoId=${encodeURIComponent(video.id)}` + );
129-132: Simplify fulfilled check.
result.valueis always present whenstatus === "fulfilled".- results.forEach((result) => { - if (result.status === "fulfilled" && result.value) { + results.forEach((result) => { + if (result.status === "fulfilled") { analyticsData[result.value.videoId] = result.value.count; } });packages/web-backend/src/S3Buckets/S3BucketsRepo.ts (4)
19-23: Prefer starting from videos and inner joining s3Buckets; add limit(1).Improves plan selection and avoids scanning s3Buckets first;
limit(1)expresses intent.- db - .select({ bucket: Db.s3Buckets }) - .from(Db.s3Buckets) - .leftJoin(Db.videos, Dz.eq(Db.videos.bucket, Db.s3Buckets.id)) - .where(Dz.and(Dz.eq(Db.videos.id, videoId))) + db + .select({ bucket: Db.s3Buckets }) + .from(Db.videos) + .innerJoin(Db.s3Buckets, Dz.eq(Db.videos.bucket, Db.s3Buckets.id)) + .where(Dz.eq(Db.videos.id, videoId)) + .limit(1)
22-23: Nit: Drop redundant Dz.and wrapper.If you keep the current shape, the
Dz.andaround a single predicate is unnecessary.- .where(Dz.and(Dz.eq(Db.videos.id, videoId))) + .where(Dz.eq(Db.videos.id, videoId))
36-41: Add limit(1) to getById.Expresses intent and can help query planners.
db .select({ bucket: Db.s3Buckets }) .from(Db.s3Buckets) - .where(Dz.eq(Db.s3Buckets.id, id)) + .where(Dz.eq(Db.s3Buckets.id, id)) + .limit(1)
52-53: Declare Database as a dependency of the service.Improves layer wiring clarity and mirrors other services (e.g.,
VideosPolicy).- }), - } + }), + dependencies: [Database.Default], + }Please confirm
Database.Defaultexists (it likely does if created withEffect.Service).packages/web-api-contract-effect/src/index.ts (1)
1-8: Remove unused import
HttpServerErrorisn’t used.import { HttpApi, HttpApiEndpoint, HttpApiError, HttpApiGroup, HttpApiMiddleware, - HttpServerError, } from "@effect/platform";apps/web/lib/server.ts (1)
25-28: Consider making tracing configuration more flexibleThe OpenTelemetry configuration is hardcoded. Consider making it configurable via environment variables for different environments.
Consider extracting configuration:
const TracingLayer = NodeSdk.layer(() => ({ resource: { serviceName: process.env.OTEL_SERVICE_NAME || "cap-web" }, spanProcessor: process.env.OTEL_ENABLED === 'true' ? [new BatchSpanProcessor(new OTLPTraceExporter())] : [], }));apps/web/lib/EffectRuntime.ts (2)
17-20: Avoid type/value shadowing: rename the exported type alias
export type RuntimeLayer = typeof RuntimeLayer;reuses the same identifier for a value and a type, which is legal but confusing. Rename the type for clarity.-const RuntimeLayer = Layer.mergeAll(Rpc.Default, TracingLayer); - -export type RuntimeLayer = typeof RuntimeLayer; +const RuntimeLayer = Layer.mergeAll(Rpc.Default, TracingLayer); + +export type RuntimeServices = typeof RuntimeLayer;
26-26: Naming: consider avoiding theuse*prefix for non-hooks
useRpcClientisn’t a React Hook (no React state/effects). Some linters and readers assumeuse*follows hook rules. ConsidergetRpcClientfor clarity.-export const useRpcClient = () => EffectRuntime.runSync(Rpc); +export const getRpcClient = () => EffectRuntime.runSync(Rpc);packages/web-backend/src/Folders/index.ts (1)
31-33: Optional: avoid deep synchronous recursionVery deep folder trees could risk stack depth. Iterative DFS/BFS can mitigate this.
If needed, switch to an explicit stack:
const stack = [folder]; while (stack.length) { const current = stack.pop()!; const children = yield* db.execute(/* ... */); stack.push(...children); // update videos/spaceVideos for current, then delete current }apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (2)
62-63: TightenonDeletetypeReturning
Promise<any>weakens type safety. PreferPromise<void>(orPromise<unknown>if necessary).- onDelete?: () => Promise<any>; + onDelete?: () => Promise<void>;
343-348: PreferuseEffectMutationfor RPC actions to unify tracing/cancellation/error semanticsYou’ve already exposed
useEffectMutationfrom the runtime. Using it here yields consistent behavior with other components and integrates lifecycle callbacks.Example:
const duplicateCap = useEffectMutation({ mutationKey: ["video", "duplicate", cap.id], mutationFn: () => withRpc((r) => r.VideoDuplicate(cap.id)), onSuccess: () => { toast.success("Cap duplicated successfully"); router.refresh(); }, onError: () => { toast.error("Failed to duplicate cap"); }, }); // ... onClick={async () => duplicateCap.mutate()}apps/web/actions/videos/get-status.ts (1)
50-56: Explicitly mark fire-and-forget to satisfy linters and intentClarify intent and silence no-floating-promises warnings.
- transcribeVideo(videoId, video.ownerId).catch((error) => { + void transcribeVideo(videoId, video.ownerId).catch((error) => { console.error( `[Get Status] Error starting transcription for video ${videoId}:`, error ); });packages/web-backend/src/S3Buckets/index.ts (1)
15-45: Config: simplify optional endpoints handlingThe current
option -> flatten -> catchTagchain is valid but hard to read. ConsiderConfig.withDefaultorEffect.orElseto improve clarity.Example:
const publicEndpoint = yield* Config.string("S3_PUBLIC_ENDPOINT") .pipe(Config.withDefault(yield* Config.string("CAP_AWS_ENDPOINT")));If neither is provided, explicitly fail with a clear
ConfigErrorviaEffect.fail.packages/web-backend/src/Videos/index.ts (1)
70-107: Consider adjustable concurrency for S3 object copying.The duplicate method is well-implemented, correctly excluding sensitive data (password, sharing) from duplication. However, the sequential copying of S3 objects (
concurrency: 1) might be slow when duplicating videos with many files.Consider making the concurrency configurable or increasing it slightly for better performance:
- { concurrency: 1 } + { concurrency: 3 }packages/effect/src/index.ts (1)
269-271: Address TODO for custom bucket handling.The TODO comment indicates incomplete implementation for custom bucket scenarios.
Would you like me to help implement the missing logic for handling different source types with custom buckets?
packages/web-domain/src/Policy.ts (1)
72-75: Clarify environment typing for withPublicPolicyType inference will widen env to
R | R2, but the helper’s type annotation currently hides that, which can mislead users.Consider annotating the return type for clarity:
- From:
<A, E2, R2>(self: Effect.Effect<A, E2, R2>) => Effect.Effect<A, PolicyDeniedError | E | E2, R2>- To:
<A, E2, R2>(self: Effect.Effect<A, E2, R2>) => Effect.Effect<A, PolicyDeniedError | E | E2, R | R2>packages/effect/src/domain.ts (1)
89-108: API param typing opportunityYou can reuse your branded ID type here to improve type-safety, similar to usage in the web route:
- videoId: Schema.String, + videoId: Schema.String, // Consider VideoId branded schema if available herepackages/web-domain/src/Video.ts (1)
53-72: verifyPassword semantics: clarify source of thepasswordargumentThe function assumes
passwordis the required video password (from DB). If that’s correct, logic is fine. If instead it’s a provided user password, the order of checks should compare against the video’s required password and treat None as “no password required.”packages/effect/src/S3Provider.ts (2)
121-131: Nit: unnecessary await when passing getClient to Effect.runPromise
getClient(true)returns an Effect;awaitis unnecessary.- async listObjects(config) { - return Effect.runPromise(await getClient(true)).then((c) => + async listObjects(config) { + return Effect.runPromise(getClient(true)).then((c) => c.send( new ListObjectsV2Command({ Bucket: bucket, Prefix: config?.prefix, MaxKeys: config?.maxKeys, }) ) ); },
40-98: Consider returning Effect in the service interface for consistencyMost of your codebase uses Effect for async and error management. Returning
Effectinstead of raw Promises here would improve composability and typed error handling.I can provide a refactor sketch that wraps these in
Effect.tryPromisewith a tagged S3Error if desired.packages/web-backend/src/S3Buckets/S3BucketAccess.ts (2)
58-67: ListObjectsV2 pagination not handledReturning only the first page can silently truncate results. Expose
ContinuationTokenor provide a helper to list all.
- Add
continuationToken?: stringto the config and returnIsTruncated/NextContinuationToken.- Or add
listAllObjectsthat paginates to exhaustion usingListObjectsV2CommandandContinuationToken.
32-39: Make URL TTL configurableHardcoding 3600s can be restrictive for callers.
Add an optional
expiresIn?: number:- getSignedObjectUrl: (key: string) => + getSignedObjectUrl: (key: string, expiresIn = 3600) => wrapS3Promise((provider) => S3Presigner.getSignedUrl( provider.getPublic(), new S3.GetObjectCommand({ Bucket: provider.bucket, Key: key }), - { expiresIn: 3600 } + { expiresIn } ) ),apps/web/lib/effect-react-query.ts (5)
56-87: Always throwing full Cause from queryFn is correct; consider attaching AbortSignal to effectYou correctly throw the Cause so React Query receives a consistent error shape. Since you already pass
signaltorunPromiseExit, ensure your effect cooperates with aborts.
- Document that query effects should honor
Effect.interruptWhenSignal(args.signal)or equivalent if they perform long I/O.- Optionally wrap
queryEffectwith an operator that binds toargs.signal.
88-121: throwOnError mapping handles UnknownException vs fail causes; minor type noteLogic is sound. One nit: the function type from React Query expects a boolean. Ensure any narrowings don't leak TExposedError mismatches if options change across renders.
- Consider memoizing the derived
throwOnErrorhandler to avoid recreation on each render, as it closes overthrowOnDefect.
123-169: Proxy remapping forerror/failureReasonis careful; add invariant-safe error type guardsCurrent invariant throws if a non-fail Cause is present with
throwOnDefect: true(which should have thrown). Good. Consider guarding in dev only to avoid surprising runtime errors in production.
- Gate the invariant under
process.env.NODE_ENV !== "production".
441-449: mutateAsync wrapper returns Exit; catch type can be UnknownException tooYou always throw the full Cause, so
.catchreceives a Cause. If an unexpected error slips through (e.g., user-land throws non-Cause), it won’t match the type.Hedge defensively:
- .catch((cause: Cause.Cause<TError>) => - Exit.fail(cause) - ) as Promise<Exit.Exit<TData, TError>>; + .catch((err: unknown) => + Exit.fail(err as Cause.Cause<TError>) + ) as Promise<Exit.Exit<TData, TError>>;
337-366: onError handler: invariant throw on non-fail Cause with throwOnDefect=true is good; consider dev-onlySame suggestion as queries to avoid unexpected runtime crashes in prod.
Wrap with a dev guard or convert to
console.error+ rethrow for diagnostics.
| export type S3BucketId = typeof S3BucketId.Type; | ||
|
|
||
| export class S3Bucket extends Schema.Class<S3Bucket>("S3Bucket")({ | ||
| id: Schema.String, |
There was a problem hiding this comment.
Use S3BucketId for the id field.
Keep IDs strongly typed to prevent mixing IDs across domains.
- id: Schema.String,
+ id: S3BucketId,📝 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.
| id: Schema.String, | |
| id: S3BucketId, |
🤖 Prompt for AI Agents
In packages/web-domain/src/S3Bucket.ts at line 7, replace the type of the id
field from Schema.String to S3BucketId to ensure strong typing and prevent
mixing IDs across different domains. Update the id field declaration to use the
S3BucketId type instead of a generic string type.
| if ( | ||
| video.transcriptionStatus === "COMPLETE" && | ||
| !metadata.aiProcessing && | ||
| !metadata.summary && | ||
| video.transcriptionStatus === "COMPLETE" && | ||
| !metadata.aiProcessing && | ||
| !metadata.summary && | ||
| !metadata.chapters && | ||
| !metadata.generationError | ||
| ) { | ||
| console.log(`[Get Status] Transcription complete but no AI data, checking feature flag for video owner ${video.ownerId}`); | ||
|
|
||
| console.log( | ||
| `[Get Status] Transcription complete but no AI data, checking feature flag for video owner ${video.ownerId}` | ||
| ); | ||
|
|
||
| const videoOwnerQuery = await db() | ||
| .select({ | ||
| email: users.email, | ||
| stripeSubscriptionStatus: users.stripeSubscriptionStatus | ||
| .select({ | ||
| email: users.email, | ||
| stripeSubscriptionStatus: users.stripeSubscriptionStatus, | ||
| }) | ||
| .from(users) | ||
| .where(eq(users.id, video.ownerId)) | ||
| .limit(1); | ||
|
|
||
| if (videoOwnerQuery.length > 0 && videoOwnerQuery[0] && (await isAiGenerationEnabled(videoOwnerQuery[0]))) { | ||
| console.log(`[Get Status] Feature flag enabled, triggering AI generation for video ${videoId}`); | ||
|
|
||
| if ( | ||
| videoOwnerQuery.length > 0 && | ||
| videoOwnerQuery[0] && | ||
| (await isAiGenerationEnabled(videoOwnerQuery[0])) | ||
| ) { | ||
| console.log( | ||
| `[Get Status] Feature flag enabled, triggering AI generation for video ${videoId}` | ||
| ); | ||
|
|
There was a problem hiding this comment.
Race condition: aiProcessing is not persisted before background job starts
When you start AI generation, the DB metadata.aiProcessing isn’t set to true. Subsequent calls can enqueue multiple concurrent jobs.
Set the flag before kicking off the background task:
- if (
- video.transcriptionStatus === "COMPLETE" &&
- !metadata.aiProcessing &&
- !metadata.summary &&
- !metadata.chapters &&
- !metadata.generationError
- ) {
+ if (
+ video.transcriptionStatus === "COMPLETE" &&
+ !metadata.aiProcessing &&
+ !metadata.summary &&
+ !metadata.chapters &&
+ !metadata.generationError
+ ) {
// ... feature flag check ...
- (async () => {
+ // Persist the aiProcessing flag to prevent duplicate enqueues
+ await db()
+ .update(videos)
+ .set({
+ metadata: { ...metadata, aiProcessing: true, generationError: null },
+ })
+ .where(eq(videos.id, videoId));
+
+ (async () => {
try {
// start job
await generateAiMetadata(videoId, video.ownerId);
// job success: generator should clear aiProcessing and set metadata itself
} catch (error) {
// on failure: reset flag + store error
try {
- const currentVideo = await db().select().from(videos).where(eq(videos.id, videoId));
+ const currentVideo = await db()
+ .select()
+ .from(videos)
+ .where(eq(videos.id, videoId));
if (currentVideo.length > 0 && currentVideo[0]) {
const currentMetadata = (currentVideo[0].metadata as VideoMetadata) || {};
await db()
.update(videos)
.set({
metadata: {
...currentMetadata,
aiProcessing: false,
generationError: error instanceof Error ? error.message : String(error),
},
})
.where(eq(videos.id, videoId));
}
} catch (resetError) {
console.error(
`[Get Status] Failed to reset AI processing flag for video ${videoId}:`,
resetError
);
}
}
})();
return {
transcriptionStatus: (video.transcriptionStatus as "PROCESSING" | "COMPLETE" | "ERROR") || null,
- aiProcessing: true,
+ aiProcessing: true, // mirrors persisted state
aiTitle: metadata.aiTitle || null,
summary: metadata.summary || null,
chapters: metadata.chapters || null,
generationError: metadata.generationError || null,
};Optionally add a guard in the UPDATE WHERE clause to only set aiProcessing when it’s currently false.
Also applies to: 214-224
🤖 Prompt for AI Agents
In apps/web/actions/videos/get-status.ts between lines 141 and 169, the code
starts AI generation without first updating the database to set
metadata.aiProcessing to true, causing a race condition with multiple jobs
enqueued. To fix this, update the database to set metadata.aiProcessing to true
before triggering the background AI generation job, optionally adding a WHERE
clause to only update if aiProcessing is currently false. Apply the same fix to
lines 214-224 where similar logic occurs.
| console.log( | ||
| `[Get Status] AI generation feature disabled for video owner ${video.ownerId} (email: ${videoOwner?.email}, pro: ${videoOwner?.stripeSubscriptionStatus})` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Avoid logging PII (email) to server logs
Logging user emails is PII. Prefer logging non-PII identifiers (e.g., user ID) or omit entirely.
- console.log(
- `[Get Status] AI generation feature disabled for video owner ${video.ownerId} (email: ${videoOwner?.email}, pro: ${videoOwner?.stripeSubscriptionStatus})`
- );
+ console.log(
+ `[Get Status] AI generation feature disabled for owner ${video.ownerId} (subscription: ${videoOwner?.stripeSubscriptionStatus})`
+ );If you need the email for debugging, feature-gate it behind a secure diagnostic flag and scrub in production.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log( | |
| `[Get Status] AI generation feature disabled for video owner ${video.ownerId} (email: ${videoOwner?.email}, pro: ${videoOwner?.stripeSubscriptionStatus})` | |
| ); | |
| } | |
| console.log( | |
| `[Get Status] AI generation feature disabled for owner ${video.ownerId} (subscription: ${videoOwner?.stripeSubscriptionStatus})` | |
| ); | |
| } |
🤖 Prompt for AI Agents
In apps/web/actions/videos/get-status.ts around lines 226 to 229, the
console.log statement includes the user's email, which is personally
identifiable information (PII) and should not be logged. Remove the email from
the log message or replace it with a non-PII identifier like the user ID. If
email logging is necessary for debugging, gate it behind a secure diagnostic
flag and ensure it is scrubbed in production environments.
| const deleteCaps = useEffectMutation({ | ||
| mutationFn: Effect.fn(function* (ids: Video.VideoId[]) { | ||
| if (ids.length === 0) return; | ||
|
|
||
| setIsDeleting(true); | ||
| const rpc = yield* Rpc; | ||
|
|
||
| try { | ||
| toast.promise( | ||
| async () => { | ||
| const results = await Promise.allSettled( | ||
| selectedCaps.map((capId) => deleteVideo(capId)) | ||
| ); | ||
| const fiber = yield* Effect.gen(function* () { | ||
| const results = yield* Effect.all( | ||
| ids.map((id) => rpc.VideoDelete(id).pipe(Effect.exit)), | ||
| { concurrency: 10 } | ||
| ); | ||
|
|
||
| const successCount = results.filter( | ||
| (result) => result.status === "fulfilled" && result.value.success | ||
| ).length; | ||
| const successCount = results.filter(Exit.isSuccess).length; | ||
|
|
||
| const errorCount = selectedCaps.length - successCount; | ||
| const errorCount = ids.length - successCount; | ||
|
|
||
| if (successCount > 0 && errorCount > 0) { | ||
| return { success: successCount, error: errorCount }; | ||
| } else if (successCount > 0) { | ||
| return { success: successCount }; | ||
| } else { | ||
| throw new Error( | ||
| if (successCount > 0 && errorCount > 0) { | ||
| return { success: successCount, error: errorCount }; | ||
| } else if (successCount > 0) { | ||
| return { success: successCount }; | ||
| } else { | ||
| return yield* Effect.fail( | ||
| new Error( | ||
| `Failed to delete ${errorCount} cap${errorCount === 1 ? "" : "s"}` | ||
| ); | ||
| ) | ||
| ); | ||
| } | ||
| }).pipe(Effect.fork); | ||
|
|
||
| toast.promise(Effect.runPromise(fiber.await.pipe(Effect.flatten)), { | ||
| loading: `Deleting ${selectedCaps.length} cap${ | ||
| selectedCaps.length === 1 ? "" : "s" | ||
| }...`, | ||
| success: (data) => { | ||
| if (data.error) { | ||
| return `Successfully deleted ${data.success} cap${ | ||
| data.success === 1 ? "" : "s" | ||
| }, but failed to delete ${data.error} cap${ | ||
| data.error === 1 ? "" : "s" | ||
| }`; | ||
| } | ||
| return `Successfully deleted ${data.success} cap${ | ||
| data.success === 1 ? "" : "s" | ||
| }`; | ||
| }, | ||
| { | ||
| loading: `Deleting ${selectedCaps.length} cap${selectedCaps.length === 1 ? "" : "s" | ||
| }...`, | ||
| success: (data) => { | ||
| if (data.error) { | ||
| return `Successfully deleted ${data.success} cap${data.success === 1 ? "" : "s" | ||
| }, but failed to delete ${data.error} cap${data.error === 1 ? "" : "s" | ||
| }`; | ||
| } | ||
| return `Successfully deleted ${data.success} cap${data.success === 1 ? "" : "s" | ||
| }`; | ||
| }, | ||
| error: (error) => | ||
| error.message || "An error occurred while deleting caps", | ||
| } | ||
| ); | ||
| error: (error) => | ||
| error.message || "An error occurred while deleting caps", | ||
| }); | ||
|
|
||
| return yield* fiber.await.pipe(Effect.flatten); | ||
| }), | ||
| onSuccess: Effect.fn(function* () { | ||
| setSelectedCaps([]); | ||
| refresh(); | ||
| } catch (error) { | ||
| } finally { | ||
| setIsDeleting(false); | ||
| } | ||
| }; | ||
| router.refresh(); | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Fix generator functions that don't yield.
The onSuccess handler uses Effect.fn but doesn't perform any effectful operations, causing unnecessary generator overhead.
Apply this diff to fix the issue:
- onSuccess: Effect.fn(function* () {
+ onSuccess: () => {
setSelectedCaps([]);
router.refresh();
- }),
+ },The implementation otherwise handles concurrent deletions well with proper error reporting.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const deleteCaps = useEffectMutation({ | |
| mutationFn: Effect.fn(function* (ids: Video.VideoId[]) { | |
| if (ids.length === 0) return; | |
| setIsDeleting(true); | |
| const rpc = yield* Rpc; | |
| try { | |
| toast.promise( | |
| async () => { | |
| const results = await Promise.allSettled( | |
| selectedCaps.map((capId) => deleteVideo(capId)) | |
| ); | |
| const fiber = yield* Effect.gen(function* () { | |
| const results = yield* Effect.all( | |
| ids.map((id) => rpc.VideoDelete(id).pipe(Effect.exit)), | |
| { concurrency: 10 } | |
| ); | |
| const successCount = results.filter( | |
| (result) => result.status === "fulfilled" && result.value.success | |
| ).length; | |
| const successCount = results.filter(Exit.isSuccess).length; | |
| const errorCount = selectedCaps.length - successCount; | |
| const errorCount = ids.length - successCount; | |
| if (successCount > 0 && errorCount > 0) { | |
| return { success: successCount, error: errorCount }; | |
| } else if (successCount > 0) { | |
| return { success: successCount }; | |
| } else { | |
| throw new Error( | |
| if (successCount > 0 && errorCount > 0) { | |
| return { success: successCount, error: errorCount }; | |
| } else if (successCount > 0) { | |
| return { success: successCount }; | |
| } else { | |
| return yield* Effect.fail( | |
| new Error( | |
| `Failed to delete ${errorCount} cap${errorCount === 1 ? "" : "s"}` | |
| ); | |
| ) | |
| ); | |
| } | |
| }).pipe(Effect.fork); | |
| toast.promise(Effect.runPromise(fiber.await.pipe(Effect.flatten)), { | |
| loading: `Deleting ${selectedCaps.length} cap${ | |
| selectedCaps.length === 1 ? "" : "s" | |
| }...`, | |
| success: (data) => { | |
| if (data.error) { | |
| return `Successfully deleted ${data.success} cap${ | |
| data.success === 1 ? "" : "s" | |
| }, but failed to delete ${data.error} cap${ | |
| data.error === 1 ? "" : "s" | |
| }`; | |
| } | |
| return `Successfully deleted ${data.success} cap${ | |
| data.success === 1 ? "" : "s" | |
| }`; | |
| }, | |
| { | |
| loading: `Deleting ${selectedCaps.length} cap${selectedCaps.length === 1 ? "" : "s" | |
| }...`, | |
| success: (data) => { | |
| if (data.error) { | |
| return `Successfully deleted ${data.success} cap${data.success === 1 ? "" : "s" | |
| }, but failed to delete ${data.error} cap${data.error === 1 ? "" : "s" | |
| }`; | |
| } | |
| return `Successfully deleted ${data.success} cap${data.success === 1 ? "" : "s" | |
| }`; | |
| }, | |
| error: (error) => | |
| error.message || "An error occurred while deleting caps", | |
| } | |
| ); | |
| error: (error) => | |
| error.message || "An error occurred while deleting caps", | |
| }); | |
| return yield* fiber.await.pipe(Effect.flatten); | |
| }), | |
| onSuccess: Effect.fn(function* () { | |
| setSelectedCaps([]); | |
| refresh(); | |
| } catch (error) { | |
| } finally { | |
| setIsDeleting(false); | |
| } | |
| }; | |
| router.refresh(); | |
| }), | |
| }); | |
| const deleteCaps = useEffectMutation({ | |
| mutationFn: Effect.fn(function* (ids: Video.VideoId[]) { | |
| if (ids.length === 0) return; | |
| const rpc = yield* Rpc; | |
| const fiber = yield* Effect.gen(function* () { | |
| const results = yield* Effect.all( | |
| ids.map((id) => rpc.VideoDelete(id).pipe(Effect.exit)), | |
| { concurrency: 10 } | |
| ); | |
| const successCount = results.filter(Exit.isSuccess).length; | |
| const errorCount = ids.length - successCount; | |
| if (successCount > 0 && errorCount > 0) { | |
| return { success: successCount, error: errorCount }; | |
| } else if (successCount > 0) { | |
| return { success: successCount }; | |
| } else { | |
| return yield* Effect.fail( | |
| new Error( | |
| `Failed to delete ${errorCount} cap${errorCount === 1 ? "" : "s"}` | |
| ) | |
| ); | |
| } | |
| }).pipe(Effect.fork); | |
| toast.promise(Effect.runPromise(fiber.await.pipe(Effect.flatten)), { | |
| loading: `Deleting ${selectedCaps.length} cap${ | |
| selectedCaps.length === 1 ? "" : "s" | |
| }...`, | |
| success: (data) => { | |
| if (data.error) { | |
| return `Successfully deleted ${data.success} cap${ | |
| data.success === 1 ? "" : "s" | |
| }, but failed to delete ${data.error} cap${ | |
| data.error === 1 ? "" : "s" | |
| }`; | |
| } | |
| return `Successfully deleted ${data.success} cap${ | |
| data.success === 1 ? "" : "s" | |
| }`; | |
| }, | |
| error: (error) => | |
| error.message || "An error occurred while deleting caps", | |
| }); | |
| return yield* fiber.await.pipe(Effect.flatten); | |
| }), | |
| onSuccess: () => { | |
| setSelectedCaps([]); | |
| router.refresh(); | |
| }, | |
| }); |
🧰 Tools
🪛 Biome (2.1.2)
[error] 249-252: This generator function doesn't contain yield.
(lint/correctness/useYield)
🤖 Prompt for AI Agents
In apps/web/app/(org)/dashboard/caps/Caps.tsx around lines 198 to 253, the
onSuccess handler is wrapped in Effect.fn as a generator function but does not
yield any effects, causing unnecessary generator overhead. To fix this, refactor
the onSuccess handler to be a normal function instead of a generator by removing
Effect.fn and the generator syntax, and directly call setSelectedCaps([]) and
router.refresh() inside it.
| const deleteCap = useEffectMutation({ | ||
| mutationFn: (id: Video.VideoId) => withRpc((r) => r.VideoDelete(id)), | ||
| onSuccess: Effect.fn(function* () { | ||
| toast.success("Cap deleted successfully"); | ||
| refresh(); | ||
| } catch (error) { | ||
| router.refresh(); | ||
| }), | ||
| onError: Effect.fn(function* () { | ||
| toast.error("Failed to delete cap"); | ||
| } | ||
| }; | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Fix generator functions in deleteCap mutation.
Both onSuccess and onError handlers use Effect.fn unnecessarily.
Apply this diff:
- onSuccess: Effect.fn(function* () {
+ onSuccess: () => {
toast.success("Cap deleted successfully");
router.refresh();
- }),
- onError: Effect.fn(function* () {
+ },
+ onError: () => {
toast.error("Failed to delete cap");
- }),
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const deleteCap = useEffectMutation({ | |
| mutationFn: (id: Video.VideoId) => withRpc((r) => r.VideoDelete(id)), | |
| onSuccess: Effect.fn(function* () { | |
| toast.success("Cap deleted successfully"); | |
| refresh(); | |
| } catch (error) { | |
| router.refresh(); | |
| }), | |
| onError: Effect.fn(function* () { | |
| toast.error("Failed to delete cap"); | |
| } | |
| }; | |
| }), | |
| }); | |
| const deleteCap = useEffectMutation({ | |
| mutationFn: (id: Video.VideoId) => withRpc((r) => r.VideoDelete(id)), | |
| onSuccess: () => { | |
| toast.success("Cap deleted successfully"); | |
| router.refresh(); | |
| }, | |
| onError: () => { | |
| toast.error("Failed to delete cap"); | |
| }, | |
| }); |
🧰 Tools
🪛 Biome (2.1.2)
[error] 257-260: This generator function doesn't contain yield.
(lint/correctness/useYield)
[error] 261-263: This generator function doesn't contain yield.
(lint/correctness/useYield)
🤖 Prompt for AI Agents
In apps/web/app/(org)/dashboard/caps/Caps.tsx around lines 255 to 264, the
onSuccess and onError handlers in the deleteCap mutation use Effect.fn
unnecessarily for generator functions. Remove Effect.fn wrappers and convert
these handlers to regular functions that directly execute the toast calls and
router refresh to simplify the code.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/web/app/(org)/dashboard/caps/Caps.tsx (1)
132-173: Fix stale selection captured by keydown handler (can delete the wrong items).The effect depends on selectedCaps.length, but the handler uses the full selectedCaps array. If the selection contents change without changing its length, the handler deletes the wrong caps.
- }, [selectedCaps.length, data]); + }, [selectedCaps, data]);If you want to minimize re-subscribing due to large data arrays, you can also compute a memoized list of ids for dependency or use a stable event utility, but the core fix is to depend on selectedCaps.
♻️ Duplicate comments (3)
apps/web/app/(org)/dashboard/caps/Caps.tsx (3)
250-253: Remove unnecessary generator wrapper and preserve failed selections after bulk delete.Convert onSuccess to a normal function (BiOMe error), and keep failed deletions selected to let users retry.
- onSuccess: Effect.fn(function* () { - setSelectedCaps([]); - router.refresh(); - }), + onSuccess: (data: { failedIds?: Video.VideoId[] }) => { + setSelectedCaps(data?.failedIds ?? []); + router.refresh(); + },Follow-up: to populate failedIds, adjust the mutation to return them (see next comment suggestion).
258-261: Remove unnecessary generator wrapper in deleteCap onSuccess.No yields inside; switch to a plain function to satisfy lint and reduce overhead.
- onSuccess: Effect.fn(function* () { - toast.success("Cap deleted successfully"); - router.refresh(); - }), + onSuccess: () => { + toast.success("Cap deleted successfully"); + router.refresh(); + },
262-264: Remove unnecessary generator wrapper in deleteCap onError.Same issue: no yields, so a plain function is appropriate.
- onError: Effect.fn(function* () { - toast.error("Failed to delete cap"); - }), + onError: () => { + toast.error("Failed to delete cap"); + },
🧹 Nitpick comments (4)
apps/web/app/(org)/dashboard/caps/Caps.tsx (4)
95-99: Remove unnecessary headers for GET request.Using "Content-Type" on a GET is unnecessary and sometimes misleading. Prefer the default GET without headers here.
- const response = await fetch(`/api/analytics?videoId=${video.id}`, { - method: "GET", - headers: { - "Content-Type": "application/json", - }, - }); + const response = await fetch(`/api/analytics?videoId=${video.id}`);
228-233: Use ids length for loading message to avoid state mismatch.The loading text uses selectedCaps.length, but the mutation operates on ids (the parameter). If selection changes while the mutation is running, the message can be wrong.
- toast.promise(Effect.runPromise(fiber.await.pipe(Effect.flatten)), { - loading: `Deleting ${selectedCaps.length} cap${ - selectedCaps.length === 1 ? "" : "s" + toast.promise(Effect.runPromise(fiber.await.pipe(Effect.flatten)), { + loading: `Deleting ${ids.length} cap${ + ids.length === 1 ? "" : "s" }...`,
206-219: Return failedIds from bulk delete to enable better UX on partial failures.Returning only counts prevents the UI from keeping failed items selected. Return the ids of failures to allow retrying.
- const results = yield* Effect.all( - ids.map((id) => rpc.VideoDelete(id).pipe(Effect.exit)), - { concurrency: 10 } - ); - - const successCount = results.filter(Exit.isSuccess).length; - - const errorCount = ids.length - successCount; + const results = yield* Effect.all( + ids.map((id) => + rpc.VideoDelete(id).pipe( + Effect.exit, + Effect.map((exit) => ({ id, exit })), + ) + ), + { concurrency: 10 }, + ); + + const successIds = results + .filter(({ exit }) => Exit.isSuccess(exit)) + .map(({ id }) => id); + const failedIds = results + .filter(({ exit }) => Exit.isFailure(exit)) + .map(({ id }) => id); + + const successCount = successIds.length; + const errorCount = failedIds.length; - if (successCount > 0 && errorCount > 0) { - return { success: successCount, error: errorCount }; - } else if (successCount > 0) { - return { success: successCount }; + if (successCount > 0 && errorCount > 0) { + return { success: successCount, error: errorCount, failedIds }; + } else if (successCount > 0) { + return { success: successCount, failedIds: [] }; } else { return yield* Effect.fail( new Error( `Failed to delete ${errorCount} cap${errorCount === 1 ? "" : "s"}` ) ); }This pairs with updating onSuccess to re-select only the failed ids.
Also applies to: 215-226
329-333: Consistency: await mutateAsync for single delete or return the promise.Minor consistency: the bulk path awaits mutateAsync, the single path does not. Either await or return the promise for uniform behavior.
- } else { - deleteCap.mutateAsync(cap.id); - } + } else { + await deleteCap.mutateAsync(cap.id); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/desktop/package.json(1 hunks)apps/web/app/(org)/dashboard/caps/Caps.tsx(13 hunks)apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx(12 hunks)apps/web/app/(org)/dashboard/caps/page.tsx(5 hunks)apps/web/app/embed/[videoId]/page.tsx(1 hunks)apps/web/app/s/[videoId]/page.tsx(1 hunks)apps/web/package.json(4 hunks)apps/web/utils/web-api.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/web/app/s/[videoId]/page.tsx
- apps/web/utils/web-api.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/package.json
- apps/web/app/embed/[videoId]/page.tsx
- apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
- apps/web/app/(org)/dashboard/caps/page.tsx
- apps/web/package.json
🧰 Additional context used
🪛 Biome (2.1.2)
apps/web/app/(org)/dashboard/caps/Caps.tsx
[error] 250-253: This generator function doesn't contain yield.
(lint/correctness/useYield)
[error] 258-261: This generator function doesn't contain yield.
(lint/correctness/useYield)
[error] 262-264: This generator function doesn't contain yield.
(lint/correctness/useYield)
⏰ 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). (1)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
apps/web/app/(org)/dashboard/caps/Caps.tsx (2)
250-254: Remove Effect.fn/generator on onSuccess (no yields)Matches Biome’s lint and prior feedback; use a plain function.
- onSuccess: Effect.fn(function* () { - setSelectedCaps([]); - router.refresh(); - }), + onSuccess: () => { + setSelectedCaps([]); + router.refresh(); + },
258-264: Remove Effect.fn/generators in deleteCap handlers (no yields)Both handlers don’t yield; use normal functions.
- onSuccess: Effect.fn(function* () { - toast.success("Cap deleted successfully"); - router.refresh(); - }), - onError: Effect.fn(function* () { - toast.error("Failed to delete cap"); - }), + onSuccess: () => { + toast.success("Cap deleted successfully"); + router.refresh(); + }, + onError: () => { + toast.error("Failed to delete cap"); + },
🧹 Nitpick comments (2)
apps/web/app/(org)/dashboard/caps/Caps.tsx (2)
95-99: Avoid sending Content-Type on GET without a bodyIt’s unnecessary and can confuse some intermediaries.
- const response = await fetch(`/api/analytics?videoId=${video.id}`, { - method: "GET", - headers: { - "Content-Type": "application/json", - }, - }); + const response = await fetch(`/api/analytics?videoId=${video.id}`, { + method: "GET", + });
119-122: Simplify Promise.allSettled handlingIn the fulfilled branch,
result.valueis always present; the extra guard is redundant.- results.forEach((result) => { - if (result.status === "fulfilled" && result.value) { - analyticsData[result.value.videoId] = result.value.count; - } - }); + results.forEach((result) => { + if (result.status === "fulfilled") { + analyticsData[result.value.videoId] = result.value.count; + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/desktop/package.json(1 hunks)apps/web/app/(org)/dashboard/caps/Caps.tsx(13 hunks)apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx(12 hunks)apps/web/app/(org)/dashboard/caps/page.tsx(5 hunks)apps/web/app/(org)/login/form.tsx(11 hunks)apps/web/app/api/video/delete/route.ts(1 hunks)apps/web/app/embed/[videoId]/page.tsx(1 hunks)apps/web/app/s/[videoId]/page.tsx(1 hunks)apps/web/package.json(4 hunks)apps/web/utils/web-api.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/web/app/s/[videoId]/page.tsx
- apps/web/utils/web-api.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/app/embed/[videoId]/page.tsx
- apps/desktop/package.json
- apps/web/app/(org)/login/form.tsx
- apps/web/package.json
- apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
- apps/web/app/(org)/dashboard/caps/page.tsx
🧰 Additional context used
🪛 Biome (2.1.2)
apps/web/app/(org)/dashboard/caps/Caps.tsx
[error] 250-253: This generator function doesn't contain yield.
(lint/correctness/useYield)
[error] 258-261: This generator function doesn't contain yield.
(lint/correctness/useYield)
[error] 262-264: This generator function doesn't contain yield.
(lint/correctness/useYield)
⏰ 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: Clippy
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (1)
apps/web/app/api/video/delete/route.ts (1)
16-18: Confirm query-param semantics for deleteVideo endpoint
setUrlParams(...)appends?videoId=…to/api/video/delete, matching how other video APIs (OG, transcribe, analytics, playlist) expectvideoIdas a query parameter. I didn’t find any existing client calls to/api/video/delete, but this aligns with your established pattern. No code changes are required here—only add or update your client‐side fetch to passvideoIdin the query string.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/web/app/api/desktop/[...route]/video.ts (2)
144-144: Nit: use object property shorthandYou can simplify to property shorthand.
- videoName: videoName, + videoName,
85-87: Handle blank/whitespace-only names to avoid empty titlesIf the query param
nameis provided as an empty string,name ??won’t fall back to the default. Trim and fallback to ensure a non-empty title.- const videoName = - name ?? - `Cap ${isScreenshot ? "Screenshot" : "Recording"} - ${formattedDate}`; + const defaultName = `Cap ${isScreenshot ? "Screenshot" : "Recording"} - ${formattedDate}`; + const videoName = name && name.trim().length > 0 ? name.trim() : defaultName;Alternatively, normalize at validation time with zod (optional):
- name: z.string().optional(), + name: z.string().transform((s) => s.trim()).optional(),apps/web/lib/tracing.ts (2)
13-21: Tune BatchSpanProcessor for serverless/short-lived runtimesDefaults can drop spans on cold starts/fast terminations. Consider tighter flush settings:
- new BatchSpanProcessor( + new BatchSpanProcessor( new OTLPTraceExporter({ ... - }) - ), + }), + { + maxQueueSize: 512, + maxExportBatchSize: 256, + scheduledDelayMillis: 2000, + exportTimeoutMillis: 3000 + } + ),Adjust numbers per environment. For long-lived servers, defaults may be fine.
11-11: Add richer resource attributes (version/env) for better trace filteringAugment the resource to include version and deployment environment, which greatly helps triaging:
- resource: { serviceName: "cap-web" }, + resource: { + serviceName: "cap-web", + serviceVersion: + process.env.NEXT_PUBLIC_APP_VERSION ?? + process.env.VERCEL_GIT_COMMIT_SHA ?? + "dev", + attributes: { + "deployment.environment": process.env.NODE_ENV ?? "development" + } + },Confirm the exact shape expected by @effect/opentelemetry’s layer config for additional attributes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/app/api/desktop/[...route]/video.ts(1 hunks)apps/web/lib/EffectRuntime.ts(1 hunks)apps/web/lib/server.ts(1 hunks)apps/web/lib/tracing.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/lib/server.ts
- apps/web/lib/EffectRuntime.ts
⏰ 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: Clippy
🔇 Additional comments (2)
apps/web/app/api/desktop/[...route]/video.ts (1)
141-145: Good fix: pass the correct video name to the email templateUsing the locally computed
videoNamehere is correct and prevents undefined content in the email.apps/web/lib/tracing.ts (1)
6-25: Confirmed Server-Only Usage of TracingLayer — No AXIOM_TOKEN LeakageI searched the entire codebase and found that
TracingLayeris only imported in:
apps/web/lib/server.tsapps/web/lib/EffectRuntime.tsThere are no imports in any
.tsxor browser-only files, so bundling the Axiom token into client bundles is not a concern.Optional improvements (no blocker):
- Read dataset and URL from environment (e.g.
AXIOM_DATASET,AXIOM_TRACES_URL) instead of hardcoding.- Gracefully disable or no-op the exporter if the token is absent (to avoid runtime errors).
- Tune
BatchSpanProcessoroptions for serverless or short-lived processes.
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
apps/web/app/(org)/dashboard/caps/Caps.tsx (1)
132-173: Fix potential stale selection in keydown handler.
Effect depends on selectedCaps.length only; selection can change without length changes. Depend on the array to avoid stale closures (or use a ref).- }, [selectedCaps.length, data]); + }, [selectedCaps, data]);
♻️ Duplicate comments (13)
apps/web/app/api/video/delete/route.ts (2)
16-21: Align endpoint contract: add success schema and all error variants from middleware/handler.
Endpoint declares Forbidden/NotFound only; middleware can fail with Unauthorized/InternalServerError, and handler maps PolicyDenied to Unauthorized. Also declare a success schema and return it from the handler.HttpApiEndpoint.del("deleteVideo")`/api/video/delete` .setUrlParams(Schema.Struct({ videoId: Video.VideoId })) .middleware(HttpAuthMiddleware) + .addSuccess(Schema.Boolean) .addError(HttpApiError.Forbidden) .addError(HttpApiError.NotFound) + .addError(HttpApiError.Unauthorized) + .addError(HttpApiError.InternalServerError)
33-42: Fail inside catchTags (don’t return error objects as values).
Handlers currently return error objects, which produces a success value. Wrap with Effect.fail so the builder encodes error responses correctly.- VideoNotFoundError: () => new HttpApiError.NotFound(), - PolicyDenied: () => new HttpApiError.Unauthorized(), + VideoNotFoundError: () => Effect.fail(new HttpApiError.NotFound()), + PolicyDenied: () => Effect.fail(new HttpApiError.Unauthorized()), - DatabaseError: (e) => - Effect.logError(e).pipe( - Effect.andThen(() => new HttpApiError.InternalServerError()) - ), + DatabaseError: (e) => + Effect.logError(e).pipe( + Effect.andThen(Effect.fail(new HttpApiError.InternalServerError())) + ), - S3Error: (e) => - Effect.logError(e).pipe( - Effect.andThen(() => new HttpApiError.InternalServerError()) - ), + S3Error: (e) => + Effect.logError(e).pipe( + Effect.andThen(Effect.fail(new HttpApiError.InternalServerError())) + ),apps/web/app/(org)/dashboard/caps/components/Folder.tsx (1)
75-87: Fix generator callbacks: they don’t yield.
Use Effect.gen and yield* Effect.sync for side effects (router/toast), or convert to plain functions. This satisfies the linter and ensures proper composition.const deleteFolder = useEffectMutation({ mutationFn: (id: Folder.FolderId) => withRpc((r) => r.FolderDelete(id)), - onSuccess: Effect.fn(function* () { - router.refresh(); - toast.success("Folder deleted successfully"); - }), - onError: Effect.fn(function* () { - toast.error("Failed to delete folder"); - }), - onSettled: Effect.fn(function* () { - setConfirmDeleteFolderOpen(false); - }), + onSuccess: Effect.gen(function* () { + yield* Effect.sync(() => router.refresh()); + yield* Effect.sync(() => toast.success("Folder deleted successfully")); + }), + onError: Effect.gen(function* () { + yield* Effect.sync(() => toast.error("Failed to delete folder")); + }), + onSettled: Effect.gen(function* () { + yield* Effect.sync(() => setConfirmDeleteFolderOpen(false)); + }), });packages/web-backend/src/Videos/index.ts (1)
1-1: Avoid shadowing global Array: alias Effect’s Array utilities.
Prevents confusion and satisfies linters.-import { Array, Effect, Option } from "effect"; +import { Array as EffectArray, Effect, Option } from "effect";apps/web/app/(org)/dashboard/caps/Caps.tsx (2)
256-262: deleteCap handlers: remove Effect.fn wrappers.
They don’t yield; use regular functions.- onSuccess: Effect.fn(function* () { - toast.success("Cap deleted successfully"); - router.refresh(); - }), - onError: Effect.fn(function* () { - toast.error("Failed to delete cap"); - }), + onSuccess: () => { + toast.success("Cap deleted successfully"); + router.refresh(); + }, + onError: () => { + toast.error("Failed to delete cap"); + },
248-252: onSuccess is a generator with no yields — convert to a normal function.
Removes unnecessary generator overhead and satisfies linter.- onSuccess: Effect.fn(function* () { - setSelectedCaps([]); - router.refresh(); - }), + onSuccess: () => { + setSelectedCaps([]); + router.refresh(); + },packages/web-domain/src/Policy.ts (2)
18-21: TaggedError builder usage is now correctThe zero-arg generic builder form is used properly; this matches Effect’s pattern and avoids runtime/type errors.
64-67: withPolicy signature accepts Policy — good; consider explicit return type annotationSignature now composes policies requiring CurrentUser. Optional: add an explicit return type to make the widened environment obvious to readers (CurrentUser | R | R2).
export const withPolicy = <E, R>(policy: Policy<E, R>) => - <A, E2, R2>(self: Effect.Effect<A, E2, R2>) => - Effect.zipRight(policy, self); + <A, E2, R2>(self: Effect.Effect<A, E2, R2>): Effect.Effect< + A, + PolicyDeniedError | E | E2, + CurrentUser | R | R2 + > => Effect.zipRight(policy, self);apps/web/app/api/playlist/route.ts (5)
148-155: Good fix: use signed URLs for .ts segmentsSwitching to getSignedObjectUrl prevents corrupting the playlist content and is the correct approach.
55-61: Fix NotFound mapping: return a failing effect from catchTagHandlers must fail the effect, not return values. Also, no need to flatten an error value. Keep flatten if needed for your return type, but make sure catchTag fails.
- const [video] = yield* videos.getById(urlParams.videoId).pipe( - Effect.flatten, - Effect.catchTag( - "NoSuchElementException", - () => new HttpApiError.NotFound() - ) - ); + const [video] = yield* videos.getById(urlParams.videoId).pipe( + Effect.flatten, + Effect.catchTag( + "NoSuchElementException", + () => Effect.fail(new HttpApiError.NotFound()) + ) + );
72-85: Ensure catchTags handlers fail with HttpApiError (don’t return values)All handlers currently return error objects as values. They should return a failing effect.
provideOptionalAuth, Effect.catchTags({ - VerifyVideoPasswordError: () => new HttpApiError.Forbidden(), - PolicyDenied: () => new HttpApiError.Unauthorized(), + VerifyVideoPasswordError: () => + Effect.fail(new HttpApiError.Forbidden()), + PolicyDenied: () => + Effect.fail(new HttpApiError.Unauthorized()), DatabaseError: (e) => Effect.logError(e).pipe( - Effect.andThen(() => new HttpApiError.InternalServerError()) + Effect.andThen( + Effect.fail(new HttpApiError.InternalServerError()) + ) ), S3Error: (e) => Effect.logError(e).pipe( - Effect.andThen(() => new HttpApiError.InternalServerError()) + Effect.andThen( + Effect.fail(new HttpApiError.InternalServerError()) + ) ), })
113-134: Transcription branch must fail on missing object; avoid returning values in Option.matchWrap Option.match in flatMap and return failures/successes as Effects.
- ) { - return yield* s3 - .getObject(`${video.ownerId}/${video.id}/transcription.vtt`) - .pipe( - Effect.andThen( - Option.match({ - onNone: () => new HttpApiError.NotFound(), - onSome: (c) => - HttpServerResponse.text(c).pipe( - HttpServerResponse.setHeaders({ - ...CACHE_CONTROL_HEADERS, - "Content-Type": "text/vtt", - }) - ), - }) - ), - Effect.withSpan("fetchTranscription") - ); - } + ) { + return yield* s3 + .getObject(`${video.ownerId}/${video.id}/transcription.vtt`) + .pipe( + Effect.flatMap( + Option.match({ + onNone: () => Effect.fail(new HttpApiError.NotFound()), + onSome: (c) => + Effect.succeed( + HttpServerResponse.text(c).pipe( + HttpServerResponse.setHeaders({ + ...CACHE_CONTROL_HEADERS, + "Content-Type": "text/vtt", + }) + ) + ), + }) + ), + Effect.withSpan("fetchTranscription") + ); + }
168-179: Handle the “mp4” case explicitly to avoid listing with undefined prefixWhen videoType === "mp4", prefix stays undefined and listObjects may list the whole bucket. Return an early redirect instead.
- let prefix; + let prefix: string | null | undefined; switch (urlParams.videoType) { case "video": prefix = videoPrefix; break; case "audio": prefix = audioPrefix; break; case "master": prefix = null; break; + case "mp4": + return yield* s3 + .getSignedObjectUrl(`${video.ownerId}/${video.id}/result.mp4`) + .pipe(Effect.map(HttpServerResponse.redirect)); }
🧹 Nitpick comments (7)
apps/web/app/(org)/dashboard/caps/components/Folder.tsx (1)
50-53: Remove unused state in drag ref (isAnimating is never read).
Clean up to reduce noise.- const dragStateRef = useRef({ - isDragging: false, - isAnimating: false, - }); + const dragStateRef = useRef({ isDragging: false });packages/web-backend/src/Videos/index.ts (1)
32-63: Optional: add tracing spans around delete/duplicate.
Improves observability like getById’s withSpan.Example:
delete: Effect.fn("Videos.delete")(function* (videoId) { // ... }).pipe(Effect.withSpan("Videos.delete")) duplicate: Effect.fn("Videos.duplicate")(function* (videoId) { // ... }).pipe(Effect.withSpan("Videos.duplicate"))Also applies to: 69-106
apps/web/app/(org)/dashboard/caps/Caps.tsx (1)
228-247: Minor: no-op early return in mutationFn is fine, but can be explicit.
For clarity, return Effect.void when ids is empty.- if (ids.length === 0) return; + if (ids.length === 0) { + return yield* Effect.void; + }packages/web-domain/src/Policy.ts (1)
60-63: Doc comment mismatch: policy failure error typeThese combinators fail with PolicyDeniedError, not an HTTP Forbidden. Suggest updating the comment to avoid confusion.
- * If the policy fails, the effect will fail with Forbidden. + * If the policy fails, the effect will fail with PolicyDeniedError.apps/web/app/api/playlist/route.ts (1)
197-208: Remove unnecessary userId query param in generated playlist URLsuserId isn’t part of GetPlaylistParams and isn’t used server-side. Drop it to reduce noise.
- `${serverEnv().WEB_URL}/api/playlist?userId=${video.ownerId}&videoId=${video.id}&videoType=video`, + `${serverEnv().WEB_URL}/api/playlist?videoId=${video.id}&videoType=video`, audioMetadata - ? `${serverEnv().WEB_URL}/api/playlist?userId=${video.ownerId}&videoId=${video.id}&videoType=audio` + ? `${serverEnv().WEB_URL}/api/playlist?videoId=${video.id}&videoType=audio` : nullapps/web/lib/effect-react-query.ts (2)
268-269: Remove stray console.log in mutation runnerAvoid noisy logs in production.
- console.log({ result });
188-214: Allow retry and retryDelay in mutation options (type-only change)You’re omitting retry and retryDelay from UseMutationOptions. Consider allowing them to pass through.
- } & Override< - Omit< - UseMutationOptions<TData, TExposedError, TVariables, TContext>, - "retry" | "retryDelay" - >, + } & Override< + UseMutationOptions<TData, TExposedError, TVariables, TContext>, { mutationFn?: (variables: TVariables) => Effect.Effect<TData, TError, R>; onMutate?: (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/web/app/(org)/dashboard/caps/Caps.tsx(13 hunks)apps/web/app/(org)/dashboard/caps/components/Folder.tsx(10 hunks)apps/web/app/(org)/dashboard/caps/page.tsx(4 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx(11 hunks)apps/web/app/api/playlist/route.ts(2 hunks)apps/web/app/api/video/delete/route.ts(1 hunks)apps/web/app/embed/[videoId]/page.tsx(3 hunks)apps/web/lib/EffectRuntime.ts(1 hunks)apps/web/lib/effect-react-query.ts(1 hunks)apps/web/lib/server.ts(1 hunks)apps/web/lib/tracing.ts(1 hunks)apps/web/package.json(3 hunks)package.json(1 hunks)packages/web-backend/src/Auth.ts(1 hunks)packages/web-backend/src/Folders/FoldersPolicy.ts(1 hunks)packages/web-backend/src/Folders/FoldersRpcs.ts(1 hunks)packages/web-backend/src/Rpcs.ts(1 hunks)packages/web-backend/src/Videos/index.ts(1 hunks)packages/web-domain/src/Authentication.ts(1 hunks)packages/web-domain/src/Policy.ts(1 hunks)packages/web-domain/src/S3Bucket.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/web/app/(org)/dashboard/caps/page.tsx
- apps/web/app/embed/[videoId]/page.tsx
- apps/web/package.json
- apps/web/lib/tracing.ts
- packages/web-domain/src/S3Bucket.ts
- packages/web-backend/src/Rpcs.ts
- packages/web-backend/src/Folders/FoldersRpcs.ts
- packages/web-backend/src/Folders/FoldersPolicy.ts
- apps/web/lib/server.ts
- apps/web/lib/EffectRuntime.ts
- packages/web-backend/src/Auth.ts
- apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/web-domain/src/Authentication.ts (1)
packages/web-domain/src/Errors.ts (1)
InternalError(3-6)
packages/web-domain/src/Policy.ts (1)
packages/web-domain/src/Authentication.ts (1)
CurrentUser(6-9)
apps/web/lib/effect-react-query.ts (1)
apps/web/lib/EffectRuntime.ts (2)
useEffectQuery(19-19)useEffectMutation(20-20)
🪛 Biome (2.1.2)
packages/web-backend/src/Videos/index.ts
[error] 1-1: Do not shadow the global "Array" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
apps/web/app/(org)/dashboard/caps/Caps.tsx
[error] 248-251: This generator function doesn't contain yield.
(lint/correctness/useYield)
[error] 256-259: This generator function doesn't contain yield.
(lint/correctness/useYield)
[error] 260-262: This generator function doesn't contain yield.
(lint/correctness/useYield)
apps/web/app/(org)/dashboard/caps/components/Folder.tsx
[error] 77-80: This generator function doesn't contain yield.
(lint/correctness/useYield)
[error] 81-83: This generator function doesn't contain yield.
(lint/correctness/useYield)
[error] 84-86: This generator function doesn't contain yield.
(lint/correctness/useYield)
⏰ 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: Clippy
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (1)
packages/web-domain/src/Authentication.ts (1)
6-33: LGTM: cohesive auth context + middleware definitions.
Schemas and provided context align with HTTP/RPC usage elsewhere.
| if (prefix === null) { | ||
| const [videoSegment, audioSegment] = yield* Effect.all([ | ||
| s3.listObjects({ prefix: videoPrefix, maxKeys: 1 }), | ||
| s3.listObjects({ prefix: audioPrefix, maxKeys: 1 }), | ||
| ]); | ||
|
|
||
| const playlist = lines.join("\n"); | ||
|
|
||
| return c.text(playlist, { | ||
| headers: CACHE_CONTROL_HEADERS, | ||
| }); | ||
| } | ||
|
|
||
| if (video.source.type === "desktopMP4") { | ||
| const playlistUrl = await bucket.getSignedObjectUrl( | ||
| `${video.ownerId}/${videoId}/result.mp4` | ||
| let audioMetadata; | ||
| const videoMetadata = yield* s3.headObject( | ||
| videoSegment.Contents?.[0]?.Key ?? "" | ||
| ); | ||
| if (audioSegment?.KeyCount && audioSegment?.KeyCount > 0) { | ||
| audioMetadata = yield* s3.headObject( | ||
| audioSegment.Contents?.[0]?.Key ?? "" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Master playlist: fail with NotFound if no video chunk exists
Avoid headObject on an empty key; return a 404 when no video segments are present.
if (prefix === null) {
const [videoSegment, audioSegment] = yield* Effect.all([
s3.listObjects({ prefix: videoPrefix, maxKeys: 1 }),
s3.listObjects({ prefix: audioPrefix, maxKeys: 1 }),
]);
+ if (!videoSegment?.KeyCount || videoSegment.KeyCount === 0) {
+ return yield* Effect.fail(new HttpApiError.NotFound());
+ }📝 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.
| if (prefix === null) { | |
| const [videoSegment, audioSegment] = yield* Effect.all([ | |
| s3.listObjects({ prefix: videoPrefix, maxKeys: 1 }), | |
| s3.listObjects({ prefix: audioPrefix, maxKeys: 1 }), | |
| ]); | |
| const playlist = lines.join("\n"); | |
| return c.text(playlist, { | |
| headers: CACHE_CONTROL_HEADERS, | |
| }); | |
| } | |
| if (video.source.type === "desktopMP4") { | |
| const playlistUrl = await bucket.getSignedObjectUrl( | |
| `${video.ownerId}/${videoId}/result.mp4` | |
| let audioMetadata; | |
| const videoMetadata = yield* s3.headObject( | |
| videoSegment.Contents?.[0]?.Key ?? "" | |
| ); | |
| if (audioSegment?.KeyCount && audioSegment?.KeyCount > 0) { | |
| audioMetadata = yield* s3.headObject( | |
| audioSegment.Contents?.[0]?.Key ?? "" | |
| if (prefix === null) { | |
| const [videoSegment, audioSegment] = yield* Effect.all([ | |
| s3.listObjects({ prefix: videoPrefix, maxKeys: 1 }), | |
| s3.listObjects({ prefix: audioPrefix, maxKeys: 1 }), | |
| ]); | |
| + if (!videoSegment?.KeyCount || videoSegment.KeyCount === 0) { | |
| + return yield* Effect.fail(new HttpApiError.NotFound()); | |
| + } | |
| let audioMetadata; | |
| const videoMetadata = yield* s3.headObject( | |
| videoSegment.Contents?.[0]?.Key ?? "" | |
| ); | |
| if (audioSegment?.KeyCount && audioSegment.KeyCount > 0) { | |
| audioMetadata = yield* s3.headObject( | |
| audioSegment.Contents?.[0]?.Key ?? "" | |
| ); | |
| } |
🤖 Prompt for AI Agents
In apps/web/app/api/playlist/route.ts around lines 181 to 193, the code calls
headObject on a potentially empty video segment key, which can cause errors.
Before calling headObject on the video segment, add a check to see if any video
segments exist; if none are found, immediately return a 404 NotFound response.
This prevents calling headObject with an empty key and properly handles the case
where no video chunks are available.
| prefix, | ||
| maxKeys: urlParams.thumbnail ? 1 : undefined, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Fix Option handling for thumbnail when setting maxKeys
thumbnail is an Option. Checking it as a boolean is always truthy. Use Option.isSome.
const objects = yield* s3.listObjects({
prefix,
- maxKeys: urlParams.thumbnail ? 1 : undefined,
+ maxKeys: Option.isSome(urlParams.thumbnail) ? 1 : undefined,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const objects = yield* s3.listObjects({ | |
| prefix, | |
| maxKeys: urlParams.thumbnail ? 1 : undefined, | |
| }); | |
| const objects = yield* s3.listObjects({ | |
| prefix, | |
| maxKeys: Option.isSome(urlParams.thumbnail) ? 1 : undefined, | |
| }); |
🤖 Prompt for AI Agents
In apps/web/app/api/playlist/route.ts around lines 215 to 218, the maxKeys
parameter is set based on urlParams.thumbnail, which is an Option type. The
current boolean check always evaluates to true because Option objects are
truthy. Replace the boolean check with Option.isSome(urlParams.thumbnail) to
correctly determine if the thumbnail option is present before setting maxKeys to
1.
| return handlers.handle("deleteVideo", ({ urlParams }) => | ||
| videos.delete(urlParams.videoId).pipe( | ||
| Effect.catchTags({ | ||
| VideoNotFoundError: () => new HttpApiError.NotFound(), | ||
| PolicyDenied: () => new HttpApiError.Unauthorized(), | ||
| DatabaseError: (e) => | ||
| Effect.logError(e).pipe( | ||
| Effect.andThen(() => new HttpApiError.InternalServerError()) | ||
| ), | ||
| S3Error: (e) => | ||
| Effect.logError(e).pipe( | ||
| Effect.andThen(() => new HttpApiError.InternalServerError()) | ||
| ), | ||
| }) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return the declared success schema (boolean) from the handler.
After successful deletion, return true to satisfy the success schema.
- return handlers.handle("deleteVideo", ({ urlParams }) =>
- videos.delete(urlParams.videoId).pipe(
+ return handlers.handle("deleteVideo", ({ urlParams }) =>
+ videos.delete(urlParams.videoId).pipe(
+ Effect.as(true),
Effect.catchTags({
- VideoNotFoundError: () => new HttpApiError.NotFound(),
- PolicyDenied: () => new HttpApiError.Unauthorized(),
- DatabaseError: (e) =>
- Effect.logError(e).pipe(
- Effect.andThen(() => new HttpApiError.InternalServerError())
- ),
- S3Error: (e) =>
- Effect.logError(e).pipe(
- Effect.andThen(() => new HttpApiError.InternalServerError())
- ),
+ VideoNotFoundError: () => Effect.fail(new HttpApiError.NotFound()),
+ PolicyDenied: () => Effect.fail(new HttpApiError.Unauthorized()),
+ DatabaseError: (e) =>
+ Effect.logError(e).pipe(
+ Effect.andThen(Effect.fail(new HttpApiError.InternalServerError()))
+ ),
+ S3Error: (e) =>
+ Effect.logError(e).pipe(
+ Effect.andThen(Effect.fail(new HttpApiError.InternalServerError()))
+ ),
})
)
);📝 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.
| return handlers.handle("deleteVideo", ({ urlParams }) => | |
| videos.delete(urlParams.videoId).pipe( | |
| Effect.catchTags({ | |
| VideoNotFoundError: () => new HttpApiError.NotFound(), | |
| PolicyDenied: () => new HttpApiError.Unauthorized(), | |
| DatabaseError: (e) => | |
| Effect.logError(e).pipe( | |
| Effect.andThen(() => new HttpApiError.InternalServerError()) | |
| ), | |
| S3Error: (e) => | |
| Effect.logError(e).pipe( | |
| Effect.andThen(() => new HttpApiError.InternalServerError()) | |
| ), | |
| }) | |
| ) | |
| ); | |
| return handlers.handle("deleteVideo", ({ urlParams }) => | |
| videos.delete(urlParams.videoId).pipe( | |
| Effect.as(true), | |
| Effect.catchTags({ | |
| VideoNotFoundError: () => Effect.fail(new HttpApiError.NotFound()), | |
| PolicyDenied: () => Effect.fail(new HttpApiError.Unauthorized()), | |
| DatabaseError: (e) => | |
| Effect.logError(e).pipe( | |
| Effect.andThen(Effect.fail(new HttpApiError.InternalServerError())) | |
| ), | |
| S3Error: (e) => | |
| Effect.logError(e).pipe( | |
| Effect.andThen(Effect.fail(new HttpApiError.InternalServerError())) | |
| ), | |
| }) | |
| ) | |
| ); |
🤖 Prompt for AI Agents
In apps/web/app/api/video/delete/route.ts between lines 30 and 45, the handler
for "deleteVideo" does not return the declared success schema, which is a
boolean. After the deletion operation completes successfully, modify the handler
to return true to satisfy the expected success schema.
| throwOnError: | ||
| typeof throwOnError === "function" | ||
| ? (error: Cause.Cause<TError>) => { | ||
| // this is safe because internally when we call useQuery we always throw the full cause or UnknownException | ||
| const cause = error as | ||
| | Cause.Cause<TError> | ||
| | Cause.UnknownException; | ||
|
|
||
| // if the cause is UnknownException, we always return true and throw it | ||
| if (Cause.isUnknownException(cause)) { | ||
| return true; | ||
| } | ||
|
|
||
| const failureOrCause = Cause.failureOrCause(cause); | ||
|
|
||
| if (throwOnDefect) { | ||
| // in this case options.throwOnError expects a TError | ||
|
|
||
| // the cause was a fail, so we have TError | ||
| if (Either.isLeft(failureOrCause)) { | ||
| // this is safe because if throwOnDefect is true then TExposedError is TError | ||
| const exposedError = | ||
| failureOrCause.left as unknown as TExposedError; | ||
| return throwOnError(exposedError); | ||
| } else { | ||
| // the cause was a die or interrupt, so we return true | ||
| return true; | ||
| } | ||
| } else { | ||
| // in this case options.throwOnError expects a Cause<TError> | ||
| // this is safe because if throwOnDefect is false then TExposedError is Cause<TError> | ||
| const exposedError = cause as unknown as TExposedError; | ||
| return throwOnError(exposedError); | ||
| } | ||
| } | ||
| : throwOnError, | ||
| onMutate: |
There was a problem hiding this comment.
Fix mutation throwOnError signature and forward variables/context
The function form must accept (error, variables, context). Currently only error is forwarded, dropping variables/context.
- throwOnError:
- typeof throwOnError === "function"
- ? (error: Cause.Cause<TError>) => {
+ throwOnError:
+ typeof throwOnError === "function"
+ ? (baseError: Cause.Cause<TError>, variables: TVariables, context: TContext | undefined) => {
// this is safe because internally when we call useQuery we always throw the full cause or UnknownException
- const cause = error as
+ const cause = baseError as
| Cause.Cause<TError>
| Cause.UnknownException;
// if the cause is UnknownException, we always return true and throw it
if (Cause.isUnknownException(cause)) {
return true;
}
const failureOrCause = Cause.failureOrCause(cause);
if (throwOnDefect) {
// in this case options.throwOnError expects a TError
// the cause was a fail, so we have TError
if (Either.isLeft(failureOrCause)) {
// this is safe because if throwOnDefect is true then TExposedError is TError
const exposedError =
failureOrCause.left as unknown as TExposedError;
- return throwOnError(exposedError);
+ return (throwOnError as (
+ err: TExposedError,
+ variables: TVariables,
+ context: TContext | undefined
+ ) => boolean)(exposedError, variables, context);
} else {
// the cause was a die or interrupt, so we return true
return true;
}
} else {
// in this case options.throwOnError expects a Cause<TError>
// this is safe because if throwOnDefect is false then TExposedError is Cause<TError>
const exposedError = cause as unknown as TExposedError;
- return throwOnError(exposedError);
+ return (throwOnError as (
+ err: TExposedError,
+ variables: TVariables,
+ context: TContext | undefined
+ ) => boolean)(exposedError, variables, context);
}
}
: throwOnError,📝 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.
| throwOnError: | |
| typeof throwOnError === "function" | |
| ? (error: Cause.Cause<TError>) => { | |
| // this is safe because internally when we call useQuery we always throw the full cause or UnknownException | |
| const cause = error as | |
| | Cause.Cause<TError> | |
| | Cause.UnknownException; | |
| // if the cause is UnknownException, we always return true and throw it | |
| if (Cause.isUnknownException(cause)) { | |
| return true; | |
| } | |
| const failureOrCause = Cause.failureOrCause(cause); | |
| if (throwOnDefect) { | |
| // in this case options.throwOnError expects a TError | |
| // the cause was a fail, so we have TError | |
| if (Either.isLeft(failureOrCause)) { | |
| // this is safe because if throwOnDefect is true then TExposedError is TError | |
| const exposedError = | |
| failureOrCause.left as unknown as TExposedError; | |
| return throwOnError(exposedError); | |
| } else { | |
| // the cause was a die or interrupt, so we return true | |
| return true; | |
| } | |
| } else { | |
| // in this case options.throwOnError expects a Cause<TError> | |
| // this is safe because if throwOnDefect is false then TExposedError is Cause<TError> | |
| const exposedError = cause as unknown as TExposedError; | |
| return throwOnError(exposedError); | |
| } | |
| } | |
| : throwOnError, | |
| onMutate: | |
| throwOnError: | |
| typeof throwOnError === "function" | |
| ? (baseError: Cause.Cause<TError>, variables: TVariables, context: TContext | undefined) => { | |
| // this is safe because internally when we call useQuery we always throw the full cause or UnknownException | |
| const cause = baseError as | |
| | Cause.Cause<TError> | |
| | Cause.UnknownException; | |
| // if the cause is UnknownException, we always return true and throw it | |
| if (Cause.isUnknownException(cause)) { | |
| return true; | |
| } | |
| const failureOrCause = Cause.failureOrCause(cause); | |
| if (throwOnDefect) { | |
| // in this case options.throwOnError expects a TError | |
| // the cause was a fail, so we have TError | |
| if (Either.isLeft(failureOrCause)) { | |
| // this is safe because if throwOnDefect is true then TExposedError is TError | |
| const exposedError = | |
| failureOrCause.left as unknown as TExposedError; | |
| return (throwOnError as ( | |
| err: TExposedError, | |
| variables: TVariables, | |
| context: TContext | undefined | |
| ) => boolean)(exposedError, variables, context); | |
| } else { | |
| // the cause was a die or interrupt, so we return true | |
| return true; | |
| } | |
| } else { | |
| // in this case options.throwOnError expects a Cause<TError> | |
| // this is safe because if throwOnDefect is false then TExposedError is Cause<TError> | |
| const exposedError = cause as unknown as TExposedError; | |
| return (throwOnError as ( | |
| err: TExposedError, | |
| variables: TVariables, | |
| context: TContext | undefined | |
| ) => boolean)(exposedError, variables, context); | |
| } | |
| } | |
| : throwOnError, | |
| onMutate: |
🤖 Prompt for AI Agents
In apps/web/lib/effect-react-query.ts between lines 277 and 313, the
throwOnError function currently only accepts and forwards the error parameter,
but it should also accept and forward variables and context parameters to match
the expected signature. Update the function signature to include (error,
variables, context) and ensure these additional parameters are passed along when
calling throwOnError inside the function.
| const [video] = yield* repo | ||
| .getById(videoId) | ||
| .pipe( | ||
| Effect.flatMap(Effect.catchAll(() => new Video.NotFoundError())) | ||
| ); |
There was a problem hiding this comment.
Incorrect error handling: catchAll returns a value, not a failure.
This converts repository failures into success values. Map to a failure instead.
- const [video] = yield* repo
- .getById(videoId)
- .pipe(
- Effect.flatMap(Effect.catchAll(() => new Video.NotFoundError()))
- );
+ const [video] = yield* repo
+ .getById(videoId)
+ .pipe(
+ Effect.catchAll(() => Effect.fail(new Video.NotFoundError()))
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [video] = yield* repo | |
| .getById(videoId) | |
| .pipe( | |
| Effect.flatMap(Effect.catchAll(() => new Video.NotFoundError())) | |
| ); | |
| const [video] = yield* repo | |
| .getById(videoId) | |
| .pipe( | |
| Effect.catchAll(() => Effect.fail(new Video.NotFoundError())) | |
| ); |
🤖 Prompt for AI Agents
In packages/web-backend/src/Videos/index.ts around lines 33 to 37, the current
use of Effect.catchAll converts repository failures into success values by
returning a new Video.NotFoundError instance. Instead, modify the error handling
to map the caught error into a failure effect, ensuring that the error
propagates correctly as a failure rather than a success value. Use an operator
like Effect.fail or equivalent to convert the error into a failure effect.
| if (listedObjects.Contents?.length) { | ||
| yield* s3.deleteObjects( | ||
| listedObjects.Contents.map((content) => ({ | ||
| Key: content.Key, | ||
| })) | ||
| ); | ||
| } |
There was a problem hiding this comment.
S3 delete may send undefined keys. Filter nulls before delete.
S3 object listings can have undefined Key; guard before calling deleteObjects.
- if (listedObjects.Contents?.length) {
- yield* s3.deleteObjects(
- listedObjects.Contents.map((content) => ({
- Key: content.Key,
- }))
- );
- }
+ if (listedObjects.Contents?.length) {
+ const keys = EffectArray.filterMap(
+ listedObjects.Contents,
+ (obj) => Option.fromNullable(obj.Key)
+ );
+ if (keys.length > 0) {
+ yield* s3.deleteObjects(keys.map((key) => ({ Key: key })));
+ }
+ }📝 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.
| if (listedObjects.Contents?.length) { | |
| yield* s3.deleteObjects( | |
| listedObjects.Contents.map((content) => ({ | |
| Key: content.Key, | |
| })) | |
| ); | |
| } | |
| if (listedObjects.Contents?.length) { | |
| const keys = EffectArray.filterMap( | |
| listedObjects.Contents, | |
| (obj) => Option.fromNullable(obj.Key) | |
| ); | |
| if (keys.length > 0) { | |
| yield* s3.deleteObjects(keys.map((key) => ({ Key: key }))); | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/web-backend/src/Videos/index.ts around lines 55 to 61, the code
calls s3.deleteObjects with keys that may be undefined because some
listedObjects.Contents entries can have undefined Key values. To fix this,
filter the listedObjects.Contents array to exclude any entries where Key is
undefined before mapping and passing to deleteObjects, ensuring only valid keys
are sent for deletion.
| const [video] = yield* repo | ||
| .getById(videoId) | ||
| .pipe( | ||
| Effect.flatMap(Effect.catchAll(() => new Video.NotFoundError())), | ||
| Policy.withPolicy(policy.isOwner(videoId)) | ||
| ); |
There was a problem hiding this comment.
Same error-handling bug in duplicate().
Don’t return a NotFoundError as a success value.
- const [video] = yield* repo
- .getById(videoId)
- .pipe(
- Effect.flatMap(Effect.catchAll(() => new Video.NotFoundError())),
- Policy.withPolicy(policy.isOwner(videoId))
- );
+ const [video] = yield* repo
+ .getById(videoId)
+ .pipe(
+ Effect.catchAll(() => Effect.fail(new Video.NotFoundError())),
+ Policy.withPolicy(policy.isOwner(videoId))
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [video] = yield* repo | |
| .getById(videoId) | |
| .pipe( | |
| Effect.flatMap(Effect.catchAll(() => new Video.NotFoundError())), | |
| Policy.withPolicy(policy.isOwner(videoId)) | |
| ); | |
| const [video] = yield* repo | |
| .getById(videoId) | |
| .pipe( | |
| Effect.catchAll(() => Effect.fail(new Video.NotFoundError())), | |
| Policy.withPolicy(policy.isOwner(videoId)) | |
| ); |
🤖 Prompt for AI Agents
In packages/web-backend/src/Videos/index.ts around lines 72 to 77, the error
handling in the getById call incorrectly returns a NotFoundError as a success
value. Instead of returning the error object, modify the code to properly raise
or throw the NotFoundError so that it is handled as a failure, not a successful
result. Adjust the Effect.catchAll logic to re-raise the error or fail the
effect rather than returning the error instance.
| if (allObjects.Contents) | ||
| yield* Effect.all( | ||
| Array.filterMap(allObjects.Contents, (obj) => | ||
| Option.map(Option.fromNullable(obj.Key), (key) => { | ||
| const newKey = key.replace(prefix, newPrefix); | ||
| return s3.copyObject(`${bucketName}/${obj.Key}`, newKey); | ||
| }) | ||
| ), | ||
| { concurrency: 1 } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use aliased EffectArray and the safe key for copyObject.
Avoids undefined keys and shadowing.
- if (allObjects.Contents)
- yield* Effect.all(
- Array.filterMap(allObjects.Contents, (obj) =>
- Option.map(Option.fromNullable(obj.Key), (key) => {
- const newKey = key.replace(prefix, newPrefix);
- return s3.copyObject(`${bucketName}/${obj.Key}`, newKey);
- })
- ),
- { concurrency: 1 }
- );
+ if (allObjects.Contents) {
+ yield* Effect.all(
+ EffectArray.filterMap(allObjects.Contents, (obj) =>
+ Option.map(Option.fromNullable(obj.Key), (key) => {
+ const newKey = key.replace(prefix, newPrefix);
+ return s3.copyObject(`${bucketName}/${key}`, newKey);
+ })
+ ),
+ { concurrency: 1 }
+ );
+ }📝 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.
| if (allObjects.Contents) | |
| yield* Effect.all( | |
| Array.filterMap(allObjects.Contents, (obj) => | |
| Option.map(Option.fromNullable(obj.Key), (key) => { | |
| const newKey = key.replace(prefix, newPrefix); | |
| return s3.copyObject(`${bucketName}/${obj.Key}`, newKey); | |
| }) | |
| ), | |
| { concurrency: 1 } | |
| ); | |
| if (allObjects.Contents) { | |
| yield* Effect.all( | |
| EffectArray.filterMap(allObjects.Contents, (obj) => | |
| Option.map(Option.fromNullable(obj.Key), (key) => { | |
| const newKey = key.replace(prefix, newPrefix); | |
| return s3.copyObject(`${bucketName}/${key}`, newKey); | |
| }) | |
| ), | |
| { concurrency: 1 } | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/web-backend/src/Videos/index.ts around lines 95 to 104, the code
should use the aliased EffectArray instead of Effect.all for better clarity and
consistency. Also, ensure that the key passed to copyObject is safely accessed
to avoid undefined values and variable shadowing. Replace the current Effect.all
call with EffectArray.all and use the safely extracted key variable when calling
copyObject.
GET /api/playlistandDELETE /api/video/deleteare now implemented with EffectdeleteVideoandduplicateVideoserver actions have been replaced byVideoDeleteandVideoDuplicateRPCs respectively@cap/web-domainhas been created to house full-stack types and utilities for@cap/web(README)@cap/web-backendhas been created to provide Repo, Policy, Domain, RPC and other services for@cap/web(README)Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores