Skip to content

perf(analytics): merge duplicate loadVideoNames DB calls into one#1739

Open
MinitJain wants to merge 3 commits into
CapSoftware:mainfrom
MinitJain:perf/analytics-dedup-video-names
Open

perf(analytics): merge duplicate loadVideoNames DB calls into one#1739
MinitJain wants to merge 3 commits into
CapSoftware:mainfrom
MinitJain:perf/analytics-dedup-video-names

Conversation

@MinitJain
Copy link
Copy Markdown
Contributor

@MinitJain MinitJain commented Apr 17, 2026

Summary

When capId is provided to the analytics data loader, loadVideoNames was called twice sequentially — once for the top caps list and once for the single capId. Both hit the same videos table.

Merged into a single call by deduplicating all IDs with Set before fetching.

Before / After

Before: 2 sequential DB calls when capId is set

After: 1 DB call always

Test plan

  • Open analytics dashboard with a capId filter — verify cap name resolves correctly
  • Open analytics dashboard without capId — verify top caps list still loads

🤖 Generated with Claude Code

Greptile Summary

This PR consolidates two sequential loadVideoNames calls (lines 334–339) into a single query by merging top-cap IDs and the optional capId before fetching. The refactoring is correct — when capId is set, topCapsRaw is always [] (line 284–286), so the original first call was a no-op returning an empty map, and the new code produces equivalent results with slightly less overhead.

Confidence Score: 5/5

Safe to merge — logic is equivalent to the original, only a minor style suggestion remains.

No P0 or P1 issues found. The single remaining finding is a P2 style suggestion about an unnecessary Set construction that does not affect correctness or behavior.

No files require special attention.

Important Files Changed

Filename Overview
apps/web/app/(org)/dashboard/analytics/data.ts Merges two loadVideoNames calls into one by collecting all needed video IDs before querying; logic is correct and safe given topCapsRaw is always [] when capId is set.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[getOrgAnalyticsData called] --> B{capId provided?}
    B -- Yes --> C[topCapsRaw = empty array]
    B -- No --> D[topCapsRaw = queryTopCaps]
    C --> E[topCapIds = empty]
    D --> F[topCapIds = videoIds from topCapsRaw]
    E --> G["allVideoIds = [capId] via Set dedup"]
    F --> H["allVideoIds = topCapIds"]
    G --> I["loadVideoNames(allVideoIds) — single DB call"]
    H --> I
    I --> J[videoNames Map]
    J --> K{capId provided?}
    K -- Yes --> L["capName = videoNames.get(capId)"]
    K -- No --> M[capName = undefined]
    L --> N[Return OrgAnalyticsResponse]
    M --> N
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/web/app/(org)/dashboard/analytics/data.ts
Line: 337

Comment:
**`new Set` dedup is a no-op in the only case it applies**

When `capId` is set, `topCapsRaw` is hardcoded to `[]` (line 284–286), so `topCapIds` will always be empty. The `new Set([...[], capId])` therefore always produces `[capId]`, making the deduplication unnecessary. The code is still correct, but the spread + `Set` construction adds slight cognitive overhead. Consider simplifying:

```suggestion
	const allVideoIds = capId ? [capId, ...topCapIds] : topCapIds;
```

This still handles the (currently impossible) case where `capId` is already in `topCapIds` correctly for readers, while being more direct about intent.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "perf(analytics): merge duplicate loadVid..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

loadVideoNames was called twice sequentially when capId is present.
Merged into a single call by including capId in the deduplicated ID set.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
const topCapIds = tinybirdData.topCapsRaw
.map((cap: TopCapRow) => cap.videoId)
.filter(Boolean);
const allVideoIds = capId ? [...new Set([...topCapIds, capId])] : topCapIds;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 new Set dedup is a no-op in the only case it applies

When capId is set, topCapsRaw is hardcoded to [] (line 284–286), so topCapIds will always be empty. The new Set([...[], capId]) therefore always produces [capId], making the deduplication unnecessary. The code is still correct, but the spread + Set construction adds slight cognitive overhead. Consider simplifying:

Suggested change
const allVideoIds = capId ? [...new Set([...topCapIds, capId])] : topCapIds;
const allVideoIds = capId ? [capId, ...topCapIds] : topCapIds;

This still handles the (currently impossible) case where capId is already in topCapIds correctly for readers, while being more direct about intent.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/(org)/dashboard/analytics/data.ts
Line: 337

Comment:
**`new Set` dedup is a no-op in the only case it applies**

When `capId` is set, `topCapsRaw` is hardcoded to `[]` (line 284–286), so `topCapIds` will always be empty. The `new Set([...[], capId])` therefore always produces `[capId]`, making the deduplication unnecessary. The code is still correct, but the spread + `Set` construction adds slight cognitive overhead. Consider simplifying:

```suggestion
	const allVideoIds = capId ? [capId, ...topCapIds] : topCapIds;
```

This still handles the (currently impossible) case where `capId` is already in `topCapIds` correctly for readers, while being more direct about intent.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

MinitJain and others added 2 commits April 17, 2026 23:43
When capId is set, topCapsRaw is always [], so topCapIds is always
empty and the Set construction was a no-op.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@superagent-security superagent-security Bot added contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis. labels May 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant