perf(analytics): merge duplicate loadVideoNames DB calls into one#1739
Open
MinitJain wants to merge 2 commits intoCapSoftware:mainfrom
Open
perf(analytics): merge duplicate loadVideoNames DB calls into one#1739MinitJain wants to merge 2 commits intoCapSoftware:mainfrom
MinitJain wants to merge 2 commits intoCapSoftware:mainfrom
Conversation
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; |
Contributor
There was a problem hiding this 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:
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!
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When
capIdis provided to the analytics data loader,loadVideoNameswas called twice sequentially — once for the top caps list and once for the singlecapId. Both hit the samevideostable.Merged into a single call by deduplicating all IDs with
Setbefore fetching.Before / After
Before: 2 sequential DB calls when
capIdis setAfter: 1 DB call always
Test plan
capIdfilter — verify cap name resolves correctlycapId— verify top caps list still loads🤖 Generated with Claude Code
Greptile Summary
This PR consolidates two sequential
loadVideoNamescalls (lines 334–339) into a single query by merging top-cap IDs and the optionalcapIdbefore fetching. The refactoring is correct — whencapIdis set,topCapsRawis 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
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 --> NPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "perf(analytics): merge duplicate loadVid..." | Re-trigger Greptile