Skip to content

Conversation

@Brendonovich
Copy link
Contributor

@Brendonovich Brendonovich commented Oct 29, 2025

Summary by CodeRabbit

  • Refactor

    • Restructured video data shape to a unified VideoData format and updated sharing UI components to consume the new nested owner/organization fields
    • Simplified Transcript component to derive the current user internally (no user prop)
  • Chores

    • Removed the ImageViewer module
    • Removed an internal authentication log
    • Transcription start now requires a configured DEEPGRAM_API_KEY (environment gated)

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 29, 2025

Warning

Rate limit exceeded

@Brendonovich has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 49 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between caf8f90 and 7c78f5e.

📒 Files selected for processing (2)
  • apps/web/actions/videos/get-status.ts (2 hunks)
  • apps/web/app/s/[videoId]/_components/Sidebar.tsx (4 hunks)

Walkthrough

Consolidates owner-related fields into a nested VideoData.owner object, introduces a shared types.ts with VideoData/VideoOwner, removes ImageViewer, updates share-page components and page data shaping to use the new type, removes user prop from Transcript (now uses useCurrentUser), and gates transcription start on DEEPGRAM_API_KEY.

Changes

Cohort / File(s) Summary
New Types
apps/web/app/s/[videoId]/types.ts
Adds VideoData and VideoOwner types; centralizes owner, organization, and optional fields (hasPassword, orgSettings) into a single exported type.
Page data & fetching
apps/web/app/s/[videoId]/page.tsx
Refactors video fetch to include owner (uses innerJoin(users)), removes top-level ownerId fields, exposes nested owner and organization in the public payload, and updates permission/AI/metadata uses to reference video.owner.id / video.owner.isPro.
Share & UI components
apps/web/app/s/[videoId]/Share.tsx, apps/web/app/s/[videoId]/_components/ShareHeader.tsx, apps/web/app/s/[videoId]/_components/ShareVideo.tsx, apps/web/app/s/[videoId]/_components/Sidebar.tsx, apps/web/app/s/[videoId]/_components/Toolbar.tsx
All updated to accept VideoData (replacing prior inline/inferred owner shapes). References changed from ownerId/ownerIsPro/ownerName/ownerImagedata.owner.*.
Transcript tab
apps/web/app/s/[videoId]/_components/tabs/Transcript.tsx
Props changed to data: VideoData; removed user prop and now derives current user via useCurrentUser; ownership checks use data.owner.id; added internal edit/transcription state.
Component removal
apps/web/app/s/[videoId]/_components/ImageViewer.tsx
Removed the entire ImageViewer component and its export.
Backend auth logging
packages/web-backend/src/Auth.ts
Removed a runtime log in provideOptionalAuth when a current user is present.
Transcription gating
apps/web/actions/videos/get-status.ts
Added serverEnv import and gate: transcription initiation only occurs when not already started and DEEPGRAM_API_KEY is present.

Sequence Diagram(s)

sequenceDiagram
    participant Browser
    participant Page as SharePage (page.tsx)
    participant Server as Backend
    participant Components as UI (ShareHeader/ShareVideo/Sidebar/Transcript)

    Browser->>Page: GET /s/[videoId]
    Page->>Server: fetch video with innerJoin(users)
    Server-->>Page: video payload with owner { id, isPro, name, image } and organization info
    Page-->>Browser: render with VideoData
    Browser->>Components: components receive data: VideoData
    note right of Components #D6EAF8: Components read owner via data.owner.id / data.owner.isPro
Loading
sequenceDiagram
    participant Scheduler as get-status action
    participant Env as serverEnv
    participant TranscriptionService

    Scheduler->>Env: check DEEPGRAM_API_KEY
    alt has key & not started
        Scheduler->>TranscriptionService: start transcription
        TranscriptionService-->>Scheduler: transcription started
    else no key or already started
        note right of Scheduler #FDEBD0: no transcription initiated
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Areas needing focused review:
    • page.tsx data shaping: ensure all consumers use video.owner and no ownerId leaks.
    • Transcript: correctness of useCurrentUser integration and new edit/transcription state.
    • Component updates: consistent replacement of owner-related accessors and imports of the new VideoData type.
    • Removal of ImageViewer: confirm no remaining references.

Possibly related PRs

Poem

🐰 I hopped through fields, both near and wide,

tucked owners in nests where they safely hide,
ImageViewer vanished, transcripts learned to sing,
types stitched together—oh what tidings I bring! ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The PR title states "ensure more limits don't show when self hosting," which suggests changes related to conditional UI logic for self-hosted deployments and limit visibility. However, the actual changes documented in the summary are focused on structural refactoring: introducing a new VideoData type, consolidating data shape from flat ownerId fields to a nested owner object structure, updating component prop contracts, removing the ImageViewer component, and modifying the Transcript component to derive the current user internally. There is no mention in the summary of self-hosting configuration, limit display logic, feature flags, or conditional rendering based on deployment type. The title appears to describe functionality not present in the changeset. The title should be updated to accurately reflect the primary change: a refactoring of the video data model and component types. A more appropriate title might be something like "Refactor video data model to use nested owner structure" or "Update Share page components to use VideoData type," which would clearly communicate the structural changes being made rather than describing unrelated self-hosting limit visibility changes.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

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

⚠️ Outside diff range comments (4)
apps/web/app/s/[videoId]/_components/ShareVideo.tsx (1)

223-246: Hide watermark/upgrade UI when self‑hosting

Current condition !data.owner.isPro will show the Cap watermark and upsell on self‑hosted instances. Gate by hosted env.

- { !data.owner.isPro && (
+ { buildEnv.NEXT_PUBLIC_IS_CAP && !data.owner.isPro && (
    <div className="absolute top-4 left-4 z-30">
      ...
    </div>
  )}

Add:

+ import { buildEnv } from "@cap/env";

This matches the PR goal: “ensure more limits don't show when self hosting”.

apps/web/app/s/[videoId]/_components/ShareHeader.tsx (1)

178-196: Hide upgrade banner and custom domain CTA on self‑host

These should not appear when not running Cap Cloud.

- const userIsOwnerAndNotPro = user?.id === data.owner.id && !data.owner.isPro;
+ const isCapHosted = !!buildEnv.NEXT_PUBLIC_IS_CAP;
+ const userIsOwnerAndNotPro = isCapHosted && user?.id === data.owner.id && !data.owner.isPro;

Add:

+ import { buildEnv } from "@cap/env";

Applies to both the top banner and “Connect a custom domain” CTA.

Also applies to: 283-293

apps/web/app/s/[videoId]/page.tsx (2)

473-509: Update the query to match the new nested owner structure.

The updatedVideoQuery after transcription still uses the old pattern with individual owner fields and leftJoin. This is inconsistent with the main query (lines 275-320) which uses innerJoin and selects the full owner object.

This inconsistency will cause data structure mismatches when Object.assign(video, updatedVideoQuery[0]) merges the updated data back at line 512.

Apply this diff to align with the new structure:

     const updatedVideoQuery = await db()
       .select({
         id: videos.id,
         name: videos.name,
-        ownerId: videos.ownerId,
-        ownerName: users.name,
-        ownerImageUrlOrKey: users.image,
-        ownerIsPro:
-          sql`${users.stripeSubscriptionStatus} IN ('active','trialing','complete','paid') OR ${users.thirdPartyStripeSubscriptionId} IS NOT NULL`.mapWith(
-            Boolean,
-          ),
+        owner: users,
         createdAt: videos.createdAt,
         updatedAt: videos.updatedAt,
         bucket: videos.bucket,
         metadata: videos.metadata,
         public: videos.public,
         videoStartTime: videos.videoStartTime,
         audioStartTime: videos.audioStartTime,
         xStreamInfo: videos.xStreamInfo,
         jobId: videos.jobId,
         jobStatus: videos.jobStatus,
         isScreenshot: videos.isScreenshot,
         skipProcessing: videos.skipProcessing,
         transcriptionStatus: videos.transcriptionStatus,
         source: videos.source,
         sharedOrganization: {
           organizationId: sharedVideos.organizationId,
         },
         orgSettings: organizations.settings,
         videoSettings: videos.settings,
       })
       .from(videos)
       .leftJoin(sharedVideos, eq(videos.id, sharedVideos.videoId))
-      .leftJoin(users, eq(videos.ownerId, users.id))
+      .innerJoin(users, eq(videos.ownerId, users.id))
       .leftJoin(organizations, eq(videos.orgId, organizations.id))
       .where(eq(videos.id, videoId))
       .execute();

769-769: Fix incorrect property access.

The code accesses videoWithOrganizationInfo.ownerIsPro, but the videoWithOrganizationInfo object has the pro status nested under owner.isPro (set at line 729), not at the top level.

Apply this diff to fix the property access:

-        ownerIsPro={videoWithOrganizationInfo.ownerIsPro}
+        ownerIsPro={videoWithOrganizationInfo.owner.isPro}
🧹 Nitpick comments (7)
apps/web/app/s/[videoId]/Share.tsx (1)

56-130: Prefer useEffectQuery wrapper over raw useQuery (apps/web policy)

To align with project guidelines for client data fetching, replace useQuery with the useEffectQuery helper (and keep TanStack v5 under the hood).

- return useQuery({
+ return useEffectQuery({
   queryKey: ["videoStatus", videoId],
   queryFn: async (): Promise<VideoStatusResult> => {
     const res = await getVideoStatus(videoId);
     if ("success" in res && res.success === false)
       throw new Error("Failed to fetch video status");
     return res as VideoStatusResult;
   },
   ...
 });

As per coding guidelines.

apps/web/app/s/[videoId]/_components/ShareVideo.tsx (2)

167-181: URL safety: encode query params

Safer to encode userId and videoId when constructing videoSrc.

- `/api/playlist?userId=${data.owner.id}&videoId=${data.id}&videoType=mp4`
+ `/api/playlist?userId=${encodeURIComponent(data.owner.id)}&videoId=${encodeURIComponent(data.id)}&videoType=mp4`

Apply to all branches (mp4/master/video).


65-66: Forwarded ref likely stale due to empty deps

useImperativeHandle(ref, () => videoRef.current as HTMLVideoElement, []) sets the parent ref once and may never update. Remove the deps array so the handle updates on re‑render, or forward the ref directly to the underlying player.

- useImperativeHandle(ref, () => videoRef.current as HTMLVideoElement, []);
+ useImperativeHandle(ref, () => videoRef.current as HTMLVideoElement | null);

Alternatively, plumb ref into CapVideoPlayer/HLSVideoPlayer and drop useImperativeHandle.

apps/web/app/s/[videoId]/_components/tabs/Transcript.tsx (3)

26-120: Reuse shared transcript utils instead of duplicating parsers

parseVTT and formatTranscriptAsVTT duplicate logic available in ./utils/transcript-utils. Import and reuse to avoid drift.

- const parseVTT = ...
- const formatTranscriptAsVTT = ...
+ import { parseVTT, formatTranscriptAsVTT } from "../utils/transcript-utils";

Adjust relative path as needed.


145-171: Prefer useEffectMutation and/or a Server Action for retry

To match app guidelines, wrap the mutation in useEffectMutation (and consider exposing a server action for retry) rather than manual fetch.

- const retryTranscriptionMutation = useMutation({
+ const retryTranscriptionMutation = useEffectMutation({
    mutationFn: async () => {
-     const response = await fetch(`/api/videos/${data.id}/retry-transcription`, { method: "POST", headers: { "Content-Type": "application/json" } });
+     // e.g., call a Server Action here and return its result
+     return await retryTranscription(data.id);
    },
    onSuccess: () => {
      setIsTranscriptionProcessing(true);
      invalidateTranscript(data.id);
    },
 });

As per coding guidelines.


446-449: Robust null/undefined check for status

data.transcriptionStatus === null misses undefined. Consider a loose null check.

- data.transcriptionStatus === null
+ data.transcriptionStatus == null
apps/web/app/s/[videoId]/page.tsx (1)

280-281: Suggest removing redundant field selections.

The query selects ownerName and ownerImageUrlOrKey individually (lines 280-281) and also selects the entire users object as owner (line 312). Since video.owner already contains name and image, the individual selections are redundant.

Apply this diff to remove the redundancy:

     name: videos.name,
-    ownerName: users.name,
-    ownerImageUrlOrKey: users.image,
     orgId: videos.orgId,

Then update line 730-732 to use the owner object directly:

-        image: video.ownerImageUrlOrKey
-          ? yield* imageUploads.resolveImageUrl(video.ownerImageUrlOrKey)
+        image: video.owner.image
+          ? yield* imageUploads.resolveImageUrl(video.owner.image)
           : null,

Also applies to: 312-312

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0d933a7 and caf8f90.

📒 Files selected for processing (10)
  • apps/web/app/s/[videoId]/Share.tsx (2 hunks)
  • apps/web/app/s/[videoId]/_components/ImageViewer.tsx (0 hunks)
  • apps/web/app/s/[videoId]/_components/ShareHeader.tsx (7 hunks)
  • apps/web/app/s/[videoId]/_components/ShareVideo.tsx (4 hunks)
  • apps/web/app/s/[videoId]/_components/Sidebar.tsx (5 hunks)
  • apps/web/app/s/[videoId]/_components/Toolbar.tsx (1 hunks)
  • apps/web/app/s/[videoId]/_components/tabs/Transcript.tsx (3 hunks)
  • apps/web/app/s/[videoId]/page.tsx (9 hunks)
  • apps/web/app/s/[videoId]/types.ts (1 hunks)
  • packages/web-backend/src/Auth.ts (0 hunks)
💤 Files with no reviewable changes (2)
  • packages/web-backend/src/Auth.ts
  • apps/web/app/s/[videoId]/_components/ImageViewer.tsx
🧰 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 running pnpm format.

Use strict TypeScript and avoid any; leverage shared types

Files:

  • apps/web/app/s/[videoId]/_components/Toolbar.tsx
  • apps/web/app/s/[videoId]/types.ts
  • apps/web/app/s/[videoId]/Share.tsx
  • apps/web/app/s/[videoId]/_components/ShareVideo.tsx
  • apps/web/app/s/[videoId]/_components/Sidebar.tsx
  • apps/web/app/s/[videoId]/_components/tabs/Transcript.tsx
  • apps/web/app/s/[videoId]/page.tsx
  • apps/web/app/s/[videoId]/_components/ShareHeader.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/Toolbar.tsx
  • apps/web/app/s/[videoId]/types.ts
  • apps/web/app/s/[videoId]/Share.tsx
  • apps/web/app/s/[videoId]/_components/ShareVideo.tsx
  • apps/web/app/s/[videoId]/_components/Sidebar.tsx
  • apps/web/app/s/[videoId]/_components/tabs/Transcript.tsx
  • apps/web/app/s/[videoId]/page.tsx
  • apps/web/app/s/[videoId]/_components/ShareHeader.tsx
apps/web/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

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

Files:

  • apps/web/app/s/[videoId]/_components/Toolbar.tsx
  • apps/web/app/s/[videoId]/types.ts
  • apps/web/app/s/[videoId]/Share.tsx
  • apps/web/app/s/[videoId]/_components/ShareVideo.tsx
  • apps/web/app/s/[videoId]/_components/Sidebar.tsx
  • apps/web/app/s/[videoId]/_components/tabs/Transcript.tsx
  • apps/web/app/s/[videoId]/page.tsx
  • apps/web/app/s/[videoId]/_components/ShareHeader.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/Toolbar.tsx
  • apps/web/app/s/[videoId]/types.ts
  • apps/web/app/s/[videoId]/Share.tsx
  • apps/web/app/s/[videoId]/_components/ShareVideo.tsx
  • apps/web/app/s/[videoId]/_components/Sidebar.tsx
  • apps/web/app/s/[videoId]/_components/tabs/Transcript.tsx
  • apps/web/app/s/[videoId]/page.tsx
  • apps/web/app/s/[videoId]/_components/ShareHeader.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/Toolbar.tsx
  • apps/web/app/s/[videoId]/types.ts
  • apps/web/app/s/[videoId]/Share.tsx
  • apps/web/app/s/[videoId]/_components/ShareVideo.tsx
  • apps/web/app/s/[videoId]/_components/Sidebar.tsx
  • apps/web/app/s/[videoId]/_components/tabs/Transcript.tsx
  • apps/web/app/s/[videoId]/page.tsx
  • apps/web/app/s/[videoId]/_components/ShareHeader.tsx
🧬 Code graph analysis (8)
apps/web/app/s/[videoId]/_components/Toolbar.tsx (1)
apps/web/app/s/[videoId]/types.ts (1)
  • VideoData (5-12)
apps/web/app/s/[videoId]/types.ts (4)
packages/database/schema.ts (1)
  • videos (288-343)
packages/web-api-contract-effect/src/index.ts (1)
  • User (55-62)
packages/web-domain/src/Organisation.ts (1)
  • Organisation (21-24)
apps/web/app/(org)/dashboard/dashboard-data.ts (1)
  • OrganizationSettings (34-36)
apps/web/app/s/[videoId]/Share.tsx (1)
apps/web/app/s/[videoId]/types.ts (1)
  • VideoData (5-12)
apps/web/app/s/[videoId]/_components/ShareVideo.tsx (2)
apps/web/app/s/[videoId]/types.ts (1)
  • VideoData (5-12)
packages/env/build.ts (1)
  • NODE_ENV (4-4)
apps/web/app/s/[videoId]/_components/Sidebar.tsx (2)
apps/web/app/s/[videoId]/types.ts (1)
  • VideoData (5-12)
apps/web/app/s/[videoId]/_components/tabs/Transcript.tsx (1)
  • Transcript (122-631)
apps/web/app/s/[videoId]/_components/tabs/Transcript.tsx (2)
apps/web/app/s/[videoId]/types.ts (1)
  • VideoData (5-12)
apps/web/app/Layout/AuthContext.tsx (1)
  • useCurrentUser (40-42)
apps/web/app/s/[videoId]/page.tsx (4)
packages/database/schema.ts (3)
  • users (60-119)
  • videos (288-343)
  • sharedVideos (345-373)
apps/web/lib/transcribe.ts (1)
  • transcribeVideo (17-229)
apps/web/actions/videos/generate-ai-metadata.ts (1)
  • generateAiMetadata (14-310)
packages/utils/src/constants/plans.ts (1)
  • userIsPro (14-38)
apps/web/app/s/[videoId]/_components/ShareHeader.tsx (2)
apps/web/app/s/[videoId]/types.ts (1)
  • VideoData (5-12)
apps/web/components/SignedImageUrl.tsx (1)
  • SignedImageUrl (11-25)
⏰ 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 (13)
apps/web/app/s/[videoId]/_components/Sidebar.tsx (2)

89-92: Ownership check correctly updated to nested owner and org members

isOwnerOrMember now uses data.owner.id and data.organizationMembers; matches the new VideoData contract. No issues spotted.


24-25: Prop and child API alignment with VideoData

SidebarProps.data: VideoData and <Transcript data={data} onSeek={onSeek} /> look consistent with the refactor across the share page.

Also applies to: 179-180

apps/web/app/s/[videoId]/_components/Toolbar.tsx (1)

15-19: Prop type switch to VideoData is consistent

ToolbarProps.data: VideoData matches upstream changes and usage (data.id). Good.

apps/web/app/s/[videoId]/Share.tsx (1)

38-46: Props: VideoData adoption

ShareProps.data: VideoData and downstream usage are consistent. No issues.

apps/web/app/s/[videoId]/_components/ShareVideo.tsx (2)

1-1: Verify "use client" is present

This component uses hooks; ensure it is a Client Component.

Use the script in the Toolbar comment to check and add "use client"; if missing.


40-43: Prop type change to VideoData & { hasActiveUpload?: boolean }

Type migration looks correct; internal references switched to data.owner.*. LGTM.

apps/web/app/s/[videoId]/types.ts (1)

5-12: Type definitions align with the refactor

VideoData and VideoOwner capture the new nested ownership/organization shape cleanly. Good foundation for downstream components.

Also applies to: 14-19

apps/web/app/s/[videoId]/_components/ShareHeader.tsx (1)

22-33: Prop and ownership checks updated correctly

data: VideoData and isOwner using data.owner.id look good.

Also applies to: 63-66

apps/web/app/s/[videoId]/_components/tabs/Transcript.tsx (1)

381-382: Edit permission logic updated correctly

canEdit now checks user?.id === data.owner.id; aligns with VideoData. LGTM.

apps/web/app/s/[videoId]/page.tsx (4)

17-17: LGTM! Good use of shared utility.

The userIsPro helper consolidates pro-status logic in one place, improving maintainability.


384-384: LGTM! Consistent migration to nested owner structure.

All references correctly use video.owner.id instead of the previous video.ownerId.

Also applies to: 426-426, 471-471, 549-549, 587-587


726-737: LGTM! Clean data transformation to nested structure.

The transformation correctly creates nested owner and organization objects with appropriate fields.


316-316: Inconsistent join types for same condition; verify schema constraints and rationale.

Found that line 316 uses innerJoin(users, ...) while line 506 uses leftJoin(users, ...) for the same join condition eq(videos.ownerId, users.id). This inconsistency should be reconciled:

  1. Verify whether videos.ownerId is NOT NULL in the database schema (not located in available files)
  2. Confirm if there are any orphaned video records without valid owners
  3. Align both queries to use the same join type, or document why they differ
  4. If innerJoin is correct, update line 506 for consistency; if leftJoin is defensive, update line 316

Comment on lines 235 to 246
<div className="flex gap-2 items-center">
{data.ownerName && (
{data.name && (
<SignedImageUrl
name={data.ownerName}
image={data.ownerImage}
name={data.name}
image={data.owner.image}
className="size-8"
letterClass="text-base"
/>
)}
<div className="flex flex-col text-left">
<p className="text-sm text-gray-12">{data.ownerName}</p>
<p className="text-sm text-gray-12">{data.name}</p>
<p className="text-xs text-gray-10">
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix owner display: using video title instead of owner name

SignedImageUrl and the text below use data.name (video title). They should use the owner’s name and image.

- {data.name && (
+ {data.owner?.name && (
    <SignedImageUrl
-     name={data.name}
-     image={data.owner.image}
+     name={data.owner.name}
+     image={data.owner.image}
      className="size-8"
      letterClass="text-base"
    />
  )}
...
- <p className="text-sm text-gray-12">{data.name}</p>
+ <p className="text-sm text-gray-12">{data.owner?.name ?? "Unknown user"}</p>
📝 Committable suggestion

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

Suggested change
<div className="flex gap-2 items-center">
{data.ownerName && (
{data.name && (
<SignedImageUrl
name={data.ownerName}
image={data.ownerImage}
name={data.name}
image={data.owner.image}
className="size-8"
letterClass="text-base"
/>
)}
<div className="flex flex-col text-left">
<p className="text-sm text-gray-12">{data.ownerName}</p>
<p className="text-sm text-gray-12">{data.name}</p>
<p className="text-xs text-gray-10">
<div className="flex gap-2 items-center">
{data.owner?.name && (
<SignedImageUrl
name={data.owner.name}
image={data.owner.image}
className="size-8"
letterClass="text-base"
/>
)}
<div className="flex flex-col text-left">
<p className="text-sm text-gray-12">{data.owner?.name ?? "Unknown user"}</p>
<p className="text-xs text-gray-10">
🤖 Prompt for AI Agents
In apps/web/app/s/[videoId]/_components/ShareHeader.tsx around lines 235 to 246,
the owner UI is incorrectly using data.name (the video title); change the
SignedImageUrl component prop and the text elements to use the owner's name
(data.owner.name) instead, and add a safe guard (e.g. data.owner?.name and
data.owner?.image) so the image and name render only when owner exists.

Comment on lines 13 to 17
interface TranscriptProps {
data: typeof videos.$inferSelect;
data: VideoData;
onSeek?: (time: number) => void;
user?: { id: string } | null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Remove unused user prop from TranscriptProps

The component derives the user internally; drop the prop from the type.

 interface TranscriptProps {
   data: VideoData;
   onSeek?: (time: number) => void;
-  user?: { id: string } | null;
 }
🤖 Prompt for AI Agents
In apps/web/app/s/[videoId]/_components/tabs/Transcript.tsx around lines 13 to
17, the TranscriptProps interface still declares an unused user?: { id: string }
| null prop that the component derives internally; remove the user field from
the interface, update the component function signature/props destructuring to no
longer expect user, and remove any places in this file that reference
props.user. Then search the codebase for callers that pass a user prop to
<Transcript ... /> and stop supplying that prop (or update those callers to no
longer include it), and run typecheck to fix any related type errors.

@@ -1,22 +1,18 @@
import type { userSelectProps } from "@cap/database/auth/session";
import type { videos } from "@cap/database/schema";
import { Button } from "@cap/ui";
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Likely missing "use client" directive for a hook-using component

This file uses useEffect and DOM APIs; it should be a Client Component. Add the directive at the top to avoid build/runtime errors.

+ "use client";

Run the script below to confirm which of these components are missing the directive.


🏁 Script executed:

#!/bin/bash
rg -n --glob 'apps/web/app/s/**/_components/{Toolbar,Sidebar,ShareVideo}.tsx' -C0 -e '^\s*"use client";' || true

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Find all _components files with matching names
fd -t f '\.(Toolbar|Sidebar|ShareVideo)\.tsx$' apps/web/app/s

Length of output: 0


🏁 Script executed:

#!/bin/bash
# If files exist, check their first 10 lines for "use client"
for file in $(fd -t f '\.(Toolbar|Sidebar|ShareVideo)\.tsx$' apps/web/app/s 2>/dev/null || true); do
  echo "=== File: $file ==="
  head -5 "$file"
  echo ""
done

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Search for the components more broadly
fd -t f 'Toolbar\.tsx|Sidebar\.tsx|ShareVideo\.tsx' apps/web/

Length of output: 149


🏁 Script executed:

#!/bin/bash
# Also check if the directory structure exists
ls -la apps/web/app/s/ 2>&1 || echo "Directory does not exist"

Length of output: 294


🏁 Script executed:

#!/bin/bash
# Find all _components directories
fd -t d '_components' apps/web/app/

Length of output: 197


🏁 Script executed:

#!/bin/bash
# Check first 5 lines of each component file for "use client" directive
for file in apps/web/app/s/[videoId]/_components/Toolbar.tsx apps/web/app/s/[videoId]/_components/Sidebar.tsx apps/web/app/s/[videoId]/_components/ShareVideo.tsx; do
  echo "=== $file ==="
  head -5 "$file"
  echo ""
done

Length of output: 981


🏁 Script executed:

#!/bin/bash
# Check for React hook imports in all three files
for file in apps/web/app/s/[videoId]/_components/Toolbar.tsx apps/web/app/s/[videoId]/_components/Sidebar.tsx apps/web/app/s/[videoId]/_components/ShareVideo.tsx; do
  echo "=== $file ==="
  grep -E "import.*\{.*use[A-Z]" "$file" || echo "(no hook imports found)"
  echo ""
done

Length of output: 466


🏁 Script executed:

#!/bin/bash
# Also check the parent page component to understand the hierarchy
cat apps/web/app/s/[videoId]/page.tsx

Length of output: 22149


Add "use client" directive to all three Client Components using React hooks

All three components use React hooks and are missing the directive:

  • Toolbar.tsx: uses useEffect, useState, useCurrentUser
  • Sidebar.tsx: uses useState, useCurrentUser
  • ShareVideo.tsx: uses useTranscript

Add "use client"; at the top of each file:

+ "use client";
  import { Button } from "@cap/ui";
📝 Committable suggestion

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

Suggested change
import { Button } from "@cap/ui";
"use client";
import { Button } from "@cap/ui";
🤖 Prompt for AI Agents
In apps/web/app/s/[videoId]/_components/Toolbar.tsx around line 1, add the React
client directive: insert "use client"; as the very first line of the file
(before any imports) because this component uses useEffect, useState and
useCurrentUser; similarly add "use client"; as the first line of
apps/web/app/s/[videoId]/_components/Sidebar.tsx (it uses useState and
useCurrentUser) and as the first line of
apps/web/app/s/[videoId]/_components/ShareVideo.tsx (it uses useTranscript) so
all three are explicit client components and hooks work correctly.

@Brendonovich Brendonovich merged commit 8ea26ec into main Oct 29, 2025
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants