chore: fix view analytics #926
Conversation
WalkthroughRefactors analytics fetching in Caps to per-video concurrent requests with stable caching and loading state passed to CapCard. Unifies folder video cards to a single CapCard and derives user from context. Expands VideoThumbnail’s duration prop type. Adjusts folder data access to include public, branded IDs, and foldersData, and updates metadata typing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Caps as Caps.tsx
participant RQ as React Query
participant API as /api/analytics
participant UI as CapCard
User->>Caps: Open Caps page
Caps->>RQ: useQuery(["analytics", sorted(videoIds)], queryFn)
activate RQ
RQ-->>Caps: isLoading = true
Caps->>UI: Render CapCard(isLoadingAnalytics=true)
par For each videoId
RQ->>API: GET /api/analytics?videoId={id}
API-->>RQ: { count } or error
and
RQ->>API: ... (concurrent per-video)
API-->>RQ: ...
end
RQ-->>Caps: analyticsData { [videoId]: count }
deactivate RQ
Caps->>UI: Render CapCard(isLoadingAnalytics=false, analytics)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCardAnalytics.tsx (1)
14-20: CapCardAnalytics: remove or implement unused propsBoth
isLoadingAnalyticsandcapIdare declared in CapCardAnalyticsProps but never consumed by the component. Update the component to either use these props or remove them from the interface.• File: apps/web/app/(org)/dashboard/caps/components/CapCard/CapCardAnalytics.tsx
– Lines 14–20:
ts interface CapCardAnalyticsProps { capId: string; ← declared but unused displayCount: number; totalComments: number; isLoadingAnalytics: boolean;← declared but unused totalReactions: number; }
• Component signature only destructures displayCount, totalComments, totalReactions; neither isLoadingAnalytics nor capId are referenced.Refactor options (mandatory):
- Implement loading state using isLoadingAnalytics (e.g. render skeletons when true):
export const CapCardAnalytics = Object.assign( - ({ displayCount, totalComments, totalReactions }: CapCardAnalyticsProps) => ( + ({ + displayCount, + totalComments, + totalReactions, + isLoadingAnalytics, + }: CapCardAnalyticsProps) => + isLoadingAnalytics ? ( <Shell> <SkeletonItem icon={faEye} /> <SkeletonItem icon={faComment} /> <SkeletonItem icon={faSmile} /> </Shell> + ) : ( <Shell> <Tooltip content={`${displayCount} unique views`}> <IconItem icon={faEye}> <span className="text-sm text-gray-12">{displayCount}</span> </IconItem> </Tooltip> {/* … */} </Shell> + ), { Skeleton: () => ( <Shell>- Remove unused props if consumers don’t need them:
• DeleteisLoadingAnalyticsand/orcapIdfrom CapCardAnalyticsProps and any parent callers.apps/web/lib/folder.ts (1)
216-221:sharedOrganizationsfilter is ineffective; null IDs can leak throughBecause the column is left-joined, MySQL can yield
[ { id: null, name: null, iconUrl: null } ]. The current predicateorganization.id !== nullis a no-op under the stated TypeScript type (id: string) and won’t filter out nulls at runtime.Harden with a type guard:
- sharedOrganizations: Array.isArray(video.sharedOrganizations) - ? video.sharedOrganizations.filter( - (organization) => organization.id !== null, - ) - : [], + sharedOrganizations: Array.isArray(video.sharedOrganizations) + ? (video.sharedOrganizations as Array<{ + id: unknown; + name: unknown; + iconUrl: unknown; + }>).filter( + (o): o is { id: string; name: string; iconUrl: string } => + !!o && + typeof o.id === "string" && + typeof o.name === "string" && + typeof o.iconUrl === "string", + ) + : [],Optional (SQL-side): emit only non-null org rows by aggregating in a subquery that inner-joins
sharedVideos -> organizationsand left-joins that JSON aggregate back tovideos, thenCOALESCEtoJSON_ARRAY(). That removes the need for runtime filtering.
🧹 Nitpick comments (9)
apps/web/components/VideoThumbnail.tsx (3)
16-17: Prop now accepts number — good; consider making formatter accept number | string.Broadening
videoDurationtostring | numberis reasonable. Instead of coercing at the call site, updateformatDurationto acceptnumber | stringto avoid repeated.toString()and to make the contract explicit.Apply:
-const formatDuration = (duration: string) => { - const durationMs = parseFloat(duration); +const formatDuration = (duration: string | number) => { + const durationMs = + typeof duration === "number" ? duration : Number.parseFloat(duration);Then you can pass
videoDurationdirectly without string conversion (see Line 113).
111-115: Remove unnecessary string conversion at render.Once
formatDurationacceptsnumber | string, you can drop.toString(). This also avoids unnecessary allocations on every render.- {formatDuration(videoDuration.toString())} + {formatDuration(videoDuration)}
80-85: Prevent gradient “flicker” by memoizing the random fallback background.
randomGradientis re-generated on every render, causing visual flicker in the error state. Memoize once per mount.-import { memo, useEffect, useRef, useState } from "react"; +import { memo, useEffect, useMemo, useRef, useState } from "react"; -const randomGradient = `linear-gradient(to right, ${generateRandomGrayScaleColor()}, ${generateRandomGrayScaleColor()})`; +const randomGradient = useMemo( + () => + `linear-gradient(to right, ${generateRandomGrayScaleColor()}, ${generateRandomGrayScaleColor()})`, + [], +);apps/web/app/(org)/dashboard/caps/Caps.tsx (2)
84-85: Stabilize and de-duplicate the analytics key with useMemo.Sorting the IDs improves cache stability. Also memoize the sorted array to avoid re-sorting every render.
-import { useEffect, useRef, useState } from "react"; +import { useEffect, useMemo, useRef, useState } from "react"; -const videoIds = data.map((video) => video.id).sort(); +const videoIds = useMemo( + () => data.map((video) => video.id).sort(), + [data], +);
86-129: Make the query conditional and trim GET headers.
- Use
enabledto skip the query entirely whendubApiKeyEnabledis false ordatais empty (avoids constructing promises and logging).Content-Typeon a GET request is unnecessary; drop it to avoid odd proxies/CORS behavior.- Optional: if
datacan be large, consider a batch endpoint or a small concurrency limit.-const { data: analyticsData, isLoading: isLoadingAnalytics } = useQuery({ - queryKey: ["analytics", videoIds], - queryFn: async () => { - if (!dubApiKeyEnabled || data.length === 0) { - return {}; - } +const { data: analyticsData, isLoading: isLoadingAnalytics } = useQuery({ + queryKey: ["analytics", videoIds], + enabled: dubApiKeyEnabled && data.length > 0, + queryFn: async () => { const analyticsPromises = data.map(async (video) => { try { - 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}`);If batching becomes feasible later, a single route like
/api/analytics?ids=...will materially reduce request overhead.apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx (3)
31-32: Remove unusedactiveOrganizationdestructuring.
activeOrganizationis not used in this component and can be dropped to appease linters and reduce noise.-const { activeOrganization, user } = useDashboardContext(); +const { user } = useDashboardContext();
105-147: Align analytics query key with Caps.tsx and gate withenabled.Use a sorted, memoized list of IDs to stabilize caching and add
enabledto skip the query when not applicable.+import { useMemo } from "react"; @@ -const { data: analyticsData, isLoading: isLoadingAnalytics } = useQuery({ - queryKey: ["analytics", initialVideos.map((video) => video.id)], +const videoIds = useMemo( + () => initialVideos.map((v) => v.id).sort(), + [initialVideos], +); +const { data: analyticsData, isLoading: isLoadingAnalytics } = useQuery({ + queryKey: ["analytics", videoIds], + enabled: dubApiKeyEnabled && initialVideos.length > 0, queryFn: async () => { - if (!dubApiKeyEnabled || initialVideos.length === 0) { - return {}; - } + // fetch logic unchangedAlso consider removing the
Content-Typeheader for GET requests here as in Caps.tsx.
146-147:refetchOnMount: trueis fine; confirm intended UX.This forces an analytics refresh whenever the section remounts. If the page often re-mounts this component, this may increase network churn; otherwise OK for freshness.
apps/web/lib/folder.ts (1)
116-119: Initialization is correct; consider DRY-ing with a small helperThe explicit initialization avoids
??=semantics—fine. You can reduce duplication across both loops with a tiny helper.async function getSharedSpacesForVideos(videoIds: string[]) { + const ensureBucket = <T,>(map: Record<string, T[]>, key: string) => { + if (!map[key]) map[key] = []; + return map[key]!; + }; … - spaceSharing.forEach((space) => { - if (!sharedSpacesMap[space.videoId]) { - sharedSpacesMap[space.videoId] = []; - } - sharedSpacesMap[space.videoId].push({ + spaceSharing.forEach((space) => { + ensureBucket(sharedSpacesMap, space.videoId).push({ id: space.id, name: space.name, organizationId: space.organizationId, iconUrl: space.iconUrl || "", isOrg: false, }); }); … - orgSharing.forEach((org) => { - if (!sharedSpacesMap[org.videoId]) { - sharedSpacesMap[org.videoId] = []; - } - sharedSpacesMap[org.videoId].push({ + orgSharing.forEach((org) => { + ensureBucket(sharedSpacesMap, org.videoId).push({ id: org.id, name: org.name, organizationId: org.organizationId, iconUrl: org.iconUrl || "", isOrg: true, }); });Also applies to: 130-134
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/web/app/(org)/dashboard/caps/Caps.tsx(3 hunks)apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx(3 hunks)apps/web/app/(org)/dashboard/caps/components/CapCard/CapCardAnalytics.tsx(1 hunks)apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx(3 hunks)apps/web/components/VideoThumbnail.tsx(2 hunks)apps/web/lib/folder.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx (4)
apps/web/app/(org)/dashboard/caps/Caps.tsx (1)
VideoData(27-47)apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
useUploadingContext(21-29)apps/web/app/(org)/dashboard/Contexts.tsx (1)
useDashboardContext(42-42)apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (1)
CapCard(79-474)
apps/web/lib/folder.ts (2)
packages/database/schema.ts (1)
videos(231-272)packages/web-domain/src/Video.ts (3)
Video(13-35)VideoId(9-9)VideoId(10-10)
⏰ 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 (javascript-typescript)
🔇 Additional comments (4)
apps/web/app/(org)/dashboard/caps/Caps.tsx (1)
336-337: Prop pass-through LGTM.
isLoadingAnalytics={isLoadingAnalytics}aligns with the new analytics loading state.apps/web/lib/folder.ts (3)
154-154: Includingvideos.publicin both SELECT and GROUP BY is correctGiven the aggregates in this query, adding
videos.publicto the projection andGROUP BYis necessary for SQL correctness and aligns with the schema’spublicIndex. No issues here.Also applies to: 191-191
225-231: Good move: switch metadata index signature tounknownChanging
[key: string]: anyto[key: string]: unknownprevents accidental unsafe access and forces narrowing at use sites. Looks good.
233-233: DownstreamfoldersDatausage confirmed safe
I’ve audited all occurrences offoldersDatain lib/folder.ts, both dashboard space pages, the caps page, and the Caps component—the prop is strongly typed asFolderDataType[]and always passed as an array. No code assumes it can beundefinedor non‐array, so defaulting to[]won’t introduce scattered runtime guards.
Summary by CodeRabbit
New Features
Bug Fixes