Don't resolve video src until upload finish #1191
Conversation
WalkthroughUpdates tracing attributes in desktop Rust code, adds a mutation-based start-recording flow with pending-state UI, refactors the web video player to use a query-driven URL resolver, extends upload progress states, simplifies a web API check and comments out a cleanup, and adds an instrumentation guard. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as RecordingControls (Desktop)
participant Q as useMutation
participant Cmd as commands.startRecording
participant H as handleRecordingResult
User->>UI: Click "Start Recording"
UI->>Q: startRecording.mutate()
activate Q
Q->>Cmd: invoke startRecording
Cmd-->>Q: result
Q->>H: handleRecordingResult(result)
deactivate Q
note over UI,Q: Button disabled while mutation is pending
sequenceDiagram
autonumber
participant Player as CapVideoPlayer
participant Query as useQuery(resolvedSrc)
participant Resolver as URL Resolver
participant Media as MediaPlayerVideo
Player->>Query: request ["resolvedSrc", videoSrc]
alt upload in progress or fetching
Query-->>Player: skip (no fetch)
else fetchable
Query->>Resolver: resolve final URL + CORS support
Resolver-->>Query: { url, supportsCrossOrigin }
Query-->>Player: data ready
Player->>Media: render with src=url, crossOrigin flag
end
note over Player,Media: Thumbnails/rendering gated by query success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (2)
48-50: Logic is correct; consider simplifying the enabled check.The early return logic properly handles the disabled state, the pending query state (using the new "fetching" status), and missing data scenarios.
The check at Line 48 is technically redundant since the query is already conditionally enabled at Line 36. When
enabledis false, TanStack Query won't execute the query, soquery.isPendingwill be false andquery.datawill be undefined, causing Line 50 to returnnullanyway. However, keeping the explicit guard clause makes the intent clearer, so this is a minor stylistic point rather than an issue.If you prefer to remove the redundancy:
- if (!enabled) return null; if (query.isPending) return { status: "fetching" }; if (!query.data) return null;
54-67: Logic is sound and handles edge cases correctly.The ternary expression properly distinguishes between "failed" (stale for >5 minutes) and "uploading" states. The progress calculation correctly handles the
0/0NaN case by checking iftotal === 0before dividing.Minor note: Line 52 creates a new Date from
query.data.updatedAt, which is already a Date object per the schema. This is harmless but redundant. You could usequery.data.updatedAtdirectly:- const lastUpdated = new Date(query.data.updatedAt); + const lastUpdated = query.data.updatedAt;Alternatively, if you prefer to keep the explicit Date construction for safety (in case the schema changes), the current code is perfectly fine.
apps/web/app/api/desktop/[...route]/video.ts (2)
212-212: Consider using consistent optional chaining.The condition uses
videoCount?.[0]but then accessesvideoCount[0].countwithout optional chaining. While this is safe due to short-circuit evaluation, it's stylistically inconsistent.Apply this diff for consistency:
-if (videoCount?.[0] && videoCount[0].count === 1 && user.email) { +if (videoCount?.[0]?.count === 1 && user.email) {
368-371: Rely on multipart upload handler for cleanup and add orphan fallback
The multipart upload endpoint (apps/web/app/api/upload/[...route]/multipart.ts) already deletesvideoUploadson completion; remove the dead code in the progress update and, to handle clients that never finish uploads, consider adding astatus/completedAtcolumn or a scheduled cleanup job.apps/desktop/src/routes/target-select-overlay.tsx (1)
798-808: Mutation pattern looks good, but consider explicit return.The mutation wraps
commands.startRecordingwithhandleRecordingResult. However, the mutation function doesn't explicitly return the promise fromhandleRecordingResult. While JavaScript will implicitly return it, being explicit would improve clarity.Apply this diff for clarity:
const startRecording = useMutation(() => ({ mutationFn: () => - handleRecordingResult( + return handleRecordingResult( commands.startRecording({ capture_target: props.target, mode: rawOptions.mode, capture_system_audio: rawOptions.captureSystemAudio, }), setOptions, ), }));apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (3)
110-116: Prefer enabled over skipToken-in-queryFn; include enableCrossOrigin in key.
- Using
skipTokeninsidequeryFnis awkward and can obscure fetch conditions. Useenabledto gate the query.enableCrossOriginaffects returned data; include it in the queryKey for cache correctness.Apply this diff:
- const resolvedSrc = useQuery({ - queryKey: ["resolvedSrc", videoSrc], - queryFn: - isUploadProgressPending || isUploading - ? skipToken - : async () => { + const resolvedSrc = useQuery({ + queryKey: ["resolvedSrc", videoSrc, enableCrossOrigin], + enabled: !(isUploadProgressPending || isUploading) && !!videoSrc, + queryFn: async () => { try { const timestamp = Date.now(); const urlWithTimestamp = videoSrc.includes("?") ? `${videoSrc}&_t=${timestamp}` : `${videoSrc}?_t=${timestamp}`; @@ - }, + }, });Also applies to: 162-162
235-248: Avoid manual refetch on source change; rely on queryKey/enabled.
useQueryautomatically fetches onqueryKeychange and whenenabledflips to true. Manualrefetch()here risks double-fetching.Apply this diff:
- resolvedSrc.refetch(); setVideoLoaded(false); setHasError(false); isRetryingRef.current = false; setIsRetrying(false); retryCount.current = 0; startTime.current = Date.now(); @@ - }, [resolvedSrc.refetch, videoSrc, enableCrossOrigin]); + }, [videoSrc, enableCrossOrigin]);
122-125: Minor: use canonical ‘Range’ header casing.Header names are case-insensitive, but conventional casing improves readability.
- const response = await fetch(urlWithTimestamp, { - method: "GET", - headers: { range: "bytes=0-0" }, - }); + const response = await fetch(urlWithTimestamp, { + method: "GET", + headers: { Range: "bytes=0-0" }, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src-tauri/src/api.rs(4 hunks)apps/desktop/src-tauri/src/upload.rs(1 hunks)apps/desktop/src/routes/target-select-overlay.tsx(3 hunks)apps/web/app/api/desktop/[...route]/video.ts(2 hunks)apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx(10 hunks)apps/web/app/s/[videoId]/_components/ProgressCircle.tsx(3 hunks)apps/web/instrumentation.node.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/app/api/desktop/[...route]/video.tsapps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/instrumentation.node.tsapps/desktop/src/routes/target-select-overlay.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/api/desktop/[...route]/video.tsapps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/instrumentation.node.tsapps/desktop/src/routes/target-select-overlay.tsx
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/api/desktop/[...route]/video.tsapps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/instrumentation.node.ts
apps/web/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/app/api/**/*.{ts,tsx}: Prefer Server Actions for API surface; when routes are necessary, implement under app/api and export only the handler from apiToHandler(ApiLive)
Construct API routes with @effect/platform HttpApi/HttpApiBuilder, declare contracts with Schema, and only export the handler
Use HttpAuthMiddleware for required auth and provideOptionalAuth for guests; avoid duplicating session lookups
Map domain errors to transport with HttpApiError.* and keep translation exhaustive (catchTags/tapErrorCause)
Inside HttpApiBuilder.group, acquire services with Effect.gen and provide dependencies via Layer.provide instead of manual provideService
Files:
apps/web/app/api/desktop/[...route]/video.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/app/api/desktop/[...route]/video.tsapps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/instrumentation.node.ts
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/api/desktop/[...route]/video.tsapps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/upload.rsapps/desktop/src-tauri/src/api.rs
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/target-select-overlay.tsx
🧬 Code graph analysis (3)
apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (1)
packages/web-domain/src/Video.ts (4)
Video(16-59)VideoId(12-12)VideoId(13-13)UploadProgress(61-68)
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (1)
apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (1)
useUploadProgress(26-68)
apps/desktop/src/routes/target-select-overlay.tsx (2)
apps/desktop/src/utils/tauri.ts (2)
startRecording(14-16)commands(7-284)apps/desktop/src/utils/recording.ts (1)
handleRecordingResult(6-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (10)
apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (2)
9-19: LGTM! Good addition of the "fetching" status.The new
{ status: "fetching" }variant properly represents the initial query pending state before any data is available. This improves the type's expressiveness and allows consuming components to distinguish between "waiting for first data" vs. "actively uploading" vs. "failed".
26-29: LGTM! More accurate return type.The signature change to return
UploadProgress | nullbetter reflects the function's behavior, as it returnsnullin multiple scenarios (disabled, no data). The multi-line parameter layout also improves readability.apps/desktop/src-tauri/src/upload.rs (1)
619-619: LGTM! Instrumentation adjustment aligns with PR's tracing improvements.Adding
upload_idto the skip list prevents this parameter from appearing in trace output, reducing log verbosity and potentially avoiding sensitive data exposure.apps/desktop/src/routes/target-select-overlay.tsx (2)
7-7: LGTM! Import addition follows coding guidelines.Adding
useMutationfrom@tanstack/solid-queryis necessary for the mutation-based recording flow and aligns with the coding guideline to "Use TanStack Query v5 for all client-side server state and fetching in the web app."As per coding guidelines
820-861: Excellent pending state handling!The implementation effectively prevents double-submission by:
- Disabling the button visually via
data-inactivewhenstartRecording.isPending- Removing hover effects while pending
- Guarding the click handler with an early return if pending
- Maintaining consistent styling across the button and dropdown sections
This provides clear visual feedback to users and prevents race conditions.
apps/desktop/src-tauri/src/api.rs (4)
7-7: LGTM! Import addition supports trace macro usage.Adding
traceto the import from thetracingcrate enables thetrace!macro call added on line 136.
49-49: LGTM! Instrumentation adjustment aligns with PR's tracing improvements.Adding
skip(upload_id)prevents the upload ID from appearing in traces, consistent with the similar change inapps/desktop/src-tauri/src/upload.rs.
113-113: LGTM! Using skip_all for multi-parameter function.The
skip_allattribute is appropriate forupload_multipart_completewhich has multiple parameters (app, video_id, upload_id, parts, meta). This reduces trace verbosity while the explicittrace!call on line 136 provides the necessary visibility.
136-137: LGTM! Explicit trace event improves observability.Adding an explicit
trace!call provides visibility into the upload completion flow, complementing theskip_allinstrumentation attribute on the function.apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (1)
113-115: Clarify gating intent: skip until upload “reported” vs “finished”.PR title says “Don’t resolve until upload finish,” but the description says “skip until upload progress has been reported.” Current logic skips when status is “fetching” OR “uploading,” i.e., effectively until finish. Please confirm the intended behavior.
If the goal is “skip only until progress is first observed,” change the condition to only skip while pending:
- const isUploading = uploadProgress?.status === "uploading"; - const isUploadProgressPending = uploadProgress?.status === "fetching"; + const isUploading = uploadProgress?.status === "uploading"; + const isUploadProgressPending = uploadProgress?.status === "fetching"; // ... - enabled: !(isUploadProgressPending || isUploading) && !!videoSrc, + enabled: !isUploadProgressPending && !!videoSrc,Also applies to: 101-109
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (3)
285-457: Event listeners never attach due toisLoadingguard; also dependencies misaligned.The video element mounts when
resolvedSrc.isSuccessis true, but the effect early-returns when!resolvedSrc.isLoading. As a result, listeners aren’t attached. Fix the guard and deps.Apply:
- const video = videoRef.current; - if (!video || !resolvedSrc.isLoading) return; + const video = videoRef.current; + if (!video) return; ... -}, [hasPlayedOnce, videoSrc, resolvedSrc.isLoading]); +}, [hasPlayedOnce, videoSrc, resolvedSrc.isSuccess]);
391-396: Duplicateloadedmetadatalistener added.
video.addEventListener("loadedmetadata", handleLoadedMetadataWithTracks);is added twice (Lines 391 and 395). Remove one to avoid double handler invocation.video.addEventListener("loadedmetadata", handleLoadedMetadataWithTracks); video.addEventListener("load", handleLoad); video.addEventListener("play", handlePlay); video.addEventListener("error", handleError as EventListener); - video.addEventListener("loadedmetadata", handleLoadedMetadataWithTracks);
249-264: Duration tracking effect tied toisLoadingmay miss when video is ready.If
videoRef.currentisn’t available on the transition away from loading, this effect won’t re-run, andloadedmetadatawon’t be observed. Depend on readiness instead.-}, [resolvedSrc.isLoading]); +}, [videoSrc, resolvedSrc.isSuccess]);
🧹 Nitpick comments (5)
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (1)
552-554: LGTM! Correctly integrates the new "fetching" state.The updated condition properly prevents rendering the overlay during the initial upload progress fetch, addressing the bug described in the PR objectives.
Minor observation: The optional chaining on line 554 (
uploadProgress?.status) is redundant since you've already confirmeduploadProgressexists on line 553. However, this is defensive programming and doesn't affect correctness.If desired, you could simplify to:
-{imageStatus !== "success" && -uploadProgress && -uploadProgress?.status !== "fetching" ? ( +{imageStatus !== "success" && +uploadProgress && +uploadProgress.status !== "fetching" ? (apps/web/instrumentation.node.ts (1)
17-18: Consider adding a log statement when skipping setup.The early return guard effectively prevents migrations and S3 setup in CAP environments. However, adding a log statement would improve observability and help with debugging deployment issues.
Apply this diff to add logging:
export async function register() { - if (process.env.NEXT_PUBLIC_IS_CAP) return; + if (process.env.NEXT_PUBLIC_IS_CAP) { + console.log("Skipping migrations and S3 setup in CAP environment"); + return; + }apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (3)
233-248: Redundant refetch and unstable dependency.
useQueryalready refetches onqueryKeychange (videoSrc). CallingresolvedSrc.refetch()and depending on the function reference is unnecessary and could cause extra renders.
- Remove the manual
refetchcall.- Keep dependencies to
videoSrcandenableCrossOrigin(or includeenableCrossOriginin the queryKey if you want cached separation).- resolvedSrc.refetch(); setVideoLoaded(false); setHasError(false); isRetryingRef.current = false; setIsRetrying(false); retryCount.current = 0; startTime.current = Date.now(); if (retryTimeout.current) { clearTimeout(retryTimeout.current); retryTimeout.current = null; } -}, [resolvedSrc.refetch, videoSrc, enableCrossOrigin]); +}, [videoSrc, enableCrossOrigin]);
110-162: PreferuseEffectQueryfor client queries (consistency with app runtime).For consistency with the app’s EffectRuntime wrappers, consider implementing this fetch via
useEffectQueryand an Effect-based queryFn (wrapping fetch in an Effect), rather than rawuseQuery.
- Keeps client queries uniform and makes it easier to integrate runtime services and observability.
As per coding guidelines
122-125: Nit: header casing.Use
Rangeinstead ofrangefor readability (headers are case-insensitive, butRangeis conventional).-headers: { range: "bytes=0-0" }, +headers: { Range: "bytes=0-0" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/desktop/src-tauri/src/api.rs(4 hunks)apps/desktop/src-tauri/src/upload.rs(1 hunks)apps/desktop/src/routes/target-select-overlay.tsx(3 hunks)apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx(1 hunks)apps/web/app/api/desktop/[...route]/video.ts(2 hunks)apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx(10 hunks)apps/web/app/s/[videoId]/_components/ProgressCircle.tsx(3 hunks)apps/web/instrumentation.node.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsxapps/desktop/src/routes/target-select-overlay.tsxapps/web/instrumentation.node.tsapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/api/desktop/[...route]/video.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsxapps/desktop/src/routes/target-select-overlay.tsxapps/web/instrumentation.node.tsapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/api/desktop/[...route]/video.ts
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsxapps/web/instrumentation.node.tsapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/api/desktop/[...route]/video.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsxapps/web/instrumentation.node.tsapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/api/desktop/[...route]/video.ts
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/api/desktop/[...route]/video.ts
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/target-select-overlay.tsx
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/api.rsapps/desktop/src-tauri/src/upload.rs
apps/web/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/app/api/**/*.{ts,tsx}: Prefer Server Actions for API surface; when routes are necessary, implement under app/api and export only the handler from apiToHandler(ApiLive)
Construct API routes with @effect/platform HttpApi/HttpApiBuilder, declare contracts with Schema, and only export the handler
Use HttpAuthMiddleware for required auth and provideOptionalAuth for guests; avoid duplicating session lookups
Map domain errors to transport with HttpApiError.* and keep translation exhaustive (catchTags/tapErrorCause)
Inside HttpApiBuilder.group, acquire services with Effect.gen and provide dependencies via Layer.provide instead of manual provideService
Files:
apps/web/app/api/desktop/[...route]/video.ts
🧬 Code graph analysis (3)
apps/desktop/src/routes/target-select-overlay.tsx (2)
apps/desktop/src/utils/tauri.ts (2)
startRecording(14-16)commands(7-284)apps/desktop/src/utils/recording.ts (1)
handleRecordingResult(6-48)
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (1)
apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (1)
useUploadProgress(26-68)
apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (1)
packages/web-domain/src/Video.ts (4)
Video(16-59)VideoId(12-12)VideoId(13-13)UploadProgress(61-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (10)
apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (2)
9-19: LGTM! Well-structured type addition.The new
{ status: "fetching" }variant effectively distinguishes the initial query pending state from actual upload progress, preventing premature UI rendering as described in the PR objectives.
26-29: LGTM! Proper guards prevent premature data access.The implementation correctly:
- Returns
nullwhen disabled (line 48)- Returns
{ status: "fetching" }during the initial query (line 49), preventing the bug where/api/playlistwas fetched before upload progress was available- Returns
nullwhen no data exists (line 50)- Computes and returns appropriate status based on
lastUpdatedtiming (lines 54-67)This structured approach with early guards aligns with the PR objective of skipping the fetch until upload progress has been reported.
Also applies to: 48-67
apps/desktop/src-tauri/src/upload.rs (1)
619-619: LGTM!Skipping
upload_idin tracing is consistent with similar changes inapi.rs(lines 49, 113) and helps reduce log volume while avoiding potential exposure of session identifiers in traces.apps/desktop/src-tauri/src/api.rs (1)
7-7: LGTM!The enhanced tracing implementation follows best practices:
skip(upload_id)prevents logging session identifiersskip_allonupload_multipart_completeavoids logging potentially large data structures (parts array, metadata)- Explicit
trace!statement provides targeted observability at the completion checkpointThese changes improve observability without exposing sensitive data or creating excessive log volume.
Also applies to: 49-49, 113-113, 136-137
apps/desktop/src/routes/target-select-overlay.tsx (3)
798-808: LGTM!The refactor to use
useMutationfor starting recordings is appropriate and aligns with TanStack Query v5 best practices. The mutation wrapshandleRecordingResult, which already handles errors internally through dialog messages, so explicit error handling in the mutation config is not required.As per coding guidelines: "Use TanStack Query v5 for all client-side server state and fetching in the web app."
820-833: LGTM!The pending state handling correctly disables the recording button while
startRecording.isPendingis true, preventing duplicate recording starts. The conditional logic properly handles both unauthenticated instant mode and pending mutation states.
835-861: LGTM!The conditional styling appropriately provides visual feedback during the pending state:
- Hover effects are disabled when pending
- The button remains visually consistent while pending
- The dropdown section also respects the pending state
This improves the user experience by clearly indicating when the system is processing the recording start request.
apps/web/app/api/desktop/[...route]/video.ts (2)
212-212: LGTM!The optional chaining refactor simplifies the null-check while preserving the same logic.
368-371: Confirm desktop upload cleanup strategy. Deletion ofvideoUploadsexists in multipart web uploads and on video deletion, but inapps/web/app/api/desktop/[...route]/video.tsthe cleanup onuploaded === totalis disabled. Verify a cleanup process for desktop recordings or restore this deletion to prevent unbounded table growth.apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (1)
661-664: LGTM: gating thumbnails behind resolved src.Only enabling
tooltipThumbnailSrcwhenresolvedSrc.isSuccessavoids CORS/tainted canvas issues during upload/resolve.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (1)
392-399: Duplicate 'loadedmetadata' listener causes double handlers and leaks.You add the same listener twice; cleanup removes it once. Remove the duplicate addEventListener.
video.addEventListener("loadeddata", handleLoadedData); video.addEventListener("canplay", handleCanPlay); video.addEventListener("loadedmetadata", handleLoadedMetadataWithTracks); video.addEventListener("load", handleLoad); video.addEventListener("play", handlePlay); video.addEventListener("error", handleError as EventListener); - video.addEventListener("loadedmetadata", handleLoadedMetadataWithTracks);
♻️ Duplicate comments (1)
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (1)
7-7: Use EffectRuntime’s useEffectQuery in apps/web (not useQuery/skipToken).Per apps/web guidelines, client code should use useEffectQuery from "@/lib/EffectRuntime", not @tanstack/react-query directly.
As per coding guidelines
Apply this import change:
-import { skipToken, useQuery } from "@tanstack/react-query"; +import { useEffectQuery } from "@/lib/EffectRuntime"; +import * as Effect from "effect/Effect";
🧹 Nitpick comments (3)
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (3)
238-250: Remove manual refetch; let queryKey changes drive it.Manual refetch risks duplicate/network thrash. With queryKey including enableCrossOrigin/videoSrc and enabled gating, this effect can drop the refetch and function dep.
- useEffect(() => { - resolvedSrc.refetch(); + useEffect(() => { setVideoLoaded(false); setHasError(false); isRetryingRef.current = false; setIsRetrying(false); retryCount.current = 0; startTime.current = Date.now(); if (retryTimeout.current) { clearTimeout(retryTimeout.current); retryTimeout.current = null; } - }, [resolvedSrc.refetch, videoSrc, enableCrossOrigin]); + }, [videoSrc, enableCrossOrigin]);
266-266: Align dependency with EffectRuntime API: use isPending (or isSuccess).useEffectQuery exposes isPending; prefer that over isLoading to avoid mismatches.
- }, [resolvedSrc.isLoading]); + }, [resolvedSrc.isPending]);
289-292: Guard on readiness, not loading.Bind listeners only when the source is ready.
- if (!video || resolvedSrc.isLoading) return; + if (!video || !resolvedSrc.isSuccess) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx(1 hunks)apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx(10 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
🧠 Learnings (3)
📚 Learning: 2025-09-22T14:19:56.010Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T14:19:56.010Z
Learning: Applies to apps/web/**/*.{ts,tsx,js,jsx} : On the client, always use `useEffectQuery` or `useEffectMutation` from `@/lib/EffectRuntime`; never call `EffectRuntime.run*` directly in components.
Applied to files:
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx
📚 Learning: 2025-10-14T10:15:44.003Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T10:15:44.003Z
Learning: Applies to apps/web/**/*.{ts,tsx} : Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Applied to files:
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx
📚 Learning: 2025-10-14T10:15:44.003Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T10:15:44.003Z
Learning: Applies to apps/web/**/*.{tsx} : Minimize useEffect; compute during render, put logic in event handlers, and clean up subscriptions/timers
Applied to files:
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx
🧬 Code graph analysis (1)
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (1)
apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (1)
useUploadProgress(26-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (1)
552-554: LGTM! Render guard correctly prevents premature overlay display.The added condition
uploadProgress?.status !== "fetching"appropriately defers showing the upload overlay until actual progress data is available, preventing the flash behavior described in the PR objectives. The optional chaining provides safe property access, and the logic correctly hides the overlay during the initial fetch phase.apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (3)
101-109: LGTM: sensible upload-progress gating.Ignoring progress after video loads and gating on pending/uploading looks good.
537-551: LGTM: render gating and crossOrigin application.Only rendering when isSuccess and deriving crossOrigin from resolution looks correct.
664-667: LGTM: safe tooltip thumbnail gating.Skipping thumbnails when mobile or before src resolution avoids expensive canvas work.
| const resolvedSrc = useQuery<{ url: string; supportsCrossOrigin: boolean }>({ | ||
| queryKey: ["resolvedSrc", videoSrc], | ||
| queryFn: | ||
| isUploadProgressPending || isUploading | ||
| ? skipToken | ||
| : async () => { | ||
| try { | ||
| const timestamp = Date.now(); | ||
| const urlWithTimestamp = videoSrc.includes("?") | ||
| ? `${videoSrc}&_t=${timestamp}` | ||
| : `${videoSrc}?_t=${timestamp}`; | ||
|
|
||
| const response = await fetch(urlWithTimestamp, { | ||
| method: "GET", | ||
| headers: { range: "bytes=0-0" }, | ||
| }); | ||
| const finalUrl = response.redirected | ||
| ? response.url | ||
| : urlWithTimestamp; | ||
|
|
||
| // Check if the resolved URL is from a CORS-incompatible service | ||
| const isCloudflareR2 = finalUrl.includes( | ||
| ".r2.cloudflarestorage.com", | ||
| ); | ||
| const isS3 = | ||
| finalUrl.includes(".s3.") || finalUrl.includes("amazonaws.com"); | ||
| const isCorsIncompatible = isCloudflareR2 || isS3; | ||
|
|
||
| let supportsCrossOrigin = enableCrossOrigin; | ||
|
|
||
| // Set CORS based on URL compatibility BEFORE video element is created | ||
| if (isCorsIncompatible) { | ||
| console.log( | ||
| "CapVideoPlayer: Detected CORS-incompatible URL, disabling crossOrigin:", | ||
| finalUrl, | ||
| ); | ||
| supportsCrossOrigin = false; | ||
| } | ||
|
|
||
| setResolvedVideoSrc(finalUrl); | ||
| setUrlResolved(true); | ||
| return finalUrl; | ||
| } catch (error) { | ||
| console.error("CapVideoPlayer: Error fetching new video URL:", error); | ||
| const timestamp = new Date().getTime(); | ||
| const fallbackUrl = videoSrc.includes("?") | ||
| ? `${videoSrc}&_t=${timestamp}` | ||
| : `${videoSrc}?_t=${timestamp}`; | ||
| setResolvedVideoSrc(fallbackUrl); | ||
| setUrlResolved(true); | ||
| return fallbackUrl; | ||
| } | ||
| }, [videoSrc, enableCrossOrigin]); | ||
| return { url: finalUrl, supportsCrossOrigin }; | ||
| } catch (error) { | ||
| console.error( | ||
| "CapVideoPlayer: Error fetching new video URL:", | ||
| error, | ||
| ); | ||
| const timestamp = Date.now(); | ||
| const fallbackUrl = videoSrc.includes("?") | ||
| ? `${videoSrc}&_t=${timestamp}` | ||
| : `${videoSrc}?_t=${timestamp}`; | ||
| return { | ||
| url: fallbackUrl, | ||
| supportsCrossOrigin: enableCrossOrigin, | ||
| }; | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor: useEffectQuery + enabled instead of skipToken; include enableCrossOrigin in key.
- Swap to EffectRuntime’s useEffectQuery.
- Use enabled: !isUploadProgressPending && !isUploading.
- Add enableCrossOrigin to queryKey so changes auto-refetch; then drop manual refetch effect.
As per coding guidelines
Apply this diff:
- const resolvedSrc = useQuery<{ url: string; supportsCrossOrigin: boolean }>({
- queryKey: ["resolvedSrc", videoSrc],
- queryFn:
- isUploadProgressPending || isUploading
- ? skipToken
- : async () => {
- try {
- const timestamp = Date.now();
- const urlWithTimestamp = videoSrc.includes("?")
- ? `${videoSrc}&_t=${timestamp}`
- : `${videoSrc}?_t=${timestamp}`;
-
- const response = await fetch(urlWithTimestamp, {
- method: "GET",
- headers: { range: "bytes=0-0" },
- });
- const finalUrl = response.redirected
- ? response.url
- : urlWithTimestamp;
-
- // Check if the resolved URL is from a CORS-incompatible service
- const isCloudflareR2 = finalUrl.includes(
- ".r2.cloudflarestorage.com",
- );
- const isS3 =
- finalUrl.includes(".s3.") || finalUrl.includes("amazonaws.com");
- const isCorsIncompatible = isCloudflareR2 || isS3;
-
- let supportsCrossOrigin = enableCrossOrigin;
-
- // Set CORS based on URL compatibility BEFORE video element is created
- if (isCorsIncompatible) {
- console.log(
- "CapVideoPlayer: Detected CORS-incompatible URL, disabling crossOrigin:",
- finalUrl,
- );
- supportsCrossOrigin = false;
- }
-
- return { url: finalUrl, supportsCrossOrigin };
- } catch (error) {
- console.error(
- "CapVideoPlayer: Error fetching new video URL:",
- error,
- );
- const timestamp = Date.now();
- const fallbackUrl = videoSrc.includes("?")
- ? `${videoSrc}&_t=${timestamp}`
- : `${videoSrc}?_t=${timestamp}`;
- return {
- url: fallbackUrl,
- supportsCrossOrigin: enableCrossOrigin,
- };
- }
- },
- });
+ const resolvedSrc = useEffectQuery<{ url: string; supportsCrossOrigin: boolean }, unknown>({
+ queryKey: ["resolvedSrc", videoSrc, enableCrossOrigin],
+ enabled: !isUploadProgressPending && !isUploading,
+ queryFn: () =>
+ Effect.tryPromise(
+ async () => {
+ const timestamp = Date.now();
+ const urlWithTimestamp = videoSrc.includes("?")
+ ? `${videoSrc}&_t=${timestamp}`
+ : `${videoSrc}?_t=${timestamp}`;
+
+ const response = await fetch(urlWithTimestamp, {
+ method: "GET",
+ headers: { range: "bytes=0-0" },
+ });
+ const finalUrl = response.redirected ? response.url : urlWithTimestamp;
+
+ const isCloudflareR2 = finalUrl.includes(".r2.cloudflarestorage.com");
+ const isS3 = finalUrl.includes(".s3.") || finalUrl.includes("amazonaws.com");
+ const supportsCrossOrigin = enableCrossOrigin && !(isCloudflareR2 || isS3);
+
+ return { url: finalUrl, supportsCrossOrigin };
+ },
+ (e) => e,
+ ).pipe(
+ Effect.catchAll(() => {
+ const timestamp = Date.now();
+ const fallbackUrl = videoSrc.includes("?")
+ ? `${videoSrc}&_t=${timestamp}`
+ : `${videoSrc}?_t=${timestamp}`;
+ return Effect.succeed({
+ url: fallbackUrl,
+ supportsCrossOrigin: enableCrossOrigin,
+ });
+ }),
+ ),
+ });Note: If your useEffectQuery accepts a Promise, you can drop Effect.* and keep the async queryFn. Based on learnings
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx around lines 110 to
165, the query currently uses useQuery with skipToken and a queryKey that omits
enableCrossOrigin; change this to useEffectQuery, replace skipToken with an
enabled option set to (!isUploadProgressPending && !isUploading), and include
enableCrossOrigin in the queryKey (e.g. ["resolvedSrc", videoSrc,
enableCrossOrigin]) so toggling CORS refetches automatically; remove the manual
refetch effect that relied on skipToken; keep the same async queryFn (or pass
the Promise directly if your useEffectQuery accepts it) and preserve the
existing error fallback logic and supportsCrossOrigin computation.
1 participant
videoUploadrow was deleted as part of the progress update rather than waiting for the recording to complete/api/playlistbefore it was ready/api/playlist, cleaning up someuseStatesSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores