Add groupBy top_folders, top_link_tags, and top_domains #3022
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds three analytics groupBy endpoints ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Analytics API
participant TB as DB/Models
participant Parser as Zod Parser
rect rgba(200,220,255,0.14)
note over Client,API: New groupBy enrichment (top_folders/top_link_tags/top_domains)
Client->>API: GET /analytics?groupBy=top_link_tags
API->>TB: Run analytics pipe -> items (ids/tag_ids + metrics)
TB-->>API: raw items
API->>TB: Batch fetch Tag records by ids
TB-->>API: tag details
API->>Parser: Attach details -> analyticsResponse.top_link_tags.parse
Parser-->>Client: Enriched response
end
rect rgba(220,255,220,0.10)
note over Client,API: Existing top_partners/top_links path preserved
Client->>API: GET /analytics?groupBy=top_partners
API->>TB: Run analytics pipe -> items
TB-->>API: raw items
API->>Parser: analyticsResponse.top_partners.parse
Parser-->>Client: Response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–40 minutes
Possibly related PRs
Suggested reviewers
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: 0
🧹 Nitpick comments (5)
apps/web/ui/analytics/utils.ts (1)
55-57: Omit keys look correct; can defer to singular map if addedReturning "tagIds" and "folderId" here is correct. If you add singular aliases in constants, you can drop these two branches and rely on SINGULAR_ANALYTICS_ENDPOINTS fallback for consistency.
apps/web/ui/analytics/use-analytics-filters.tsx (1)
496-511: Tag options mapping is correct; consider guarding duplicate selectionsMapping id/name/color matches analyticsResponse.top_link_tags. Optional: when adding a tag in onSelect, prevent duplicates in tagIds (e.g., Set) to keep chips tidy. Based on learnings.
- tagIds: selectedTagIds.concat(value).join(","), + tagIds: Array.from(new Set(selectedTagIds.concat(value))).join(","),apps/web/lib/analytics/get-analytics.ts (2)
239-257: Select only needed tag fields + use Map + drop groupByField before parseAvoid over-fetching and convert N×M finds to O(1). Also prevent passing groupByField through to Zod.
- } else if (groupBy === "top_link_tags") { - const tags = await prisma.tag.findMany({ - where: { - id: { - in: response.data.map((item) => item.groupByField), - }, - }, - }); - - return response.data - .map((item) => { - const tag = tags.find((t) => t.id === item.groupByField); - if (!tag) return null; - return analyticsResponse[groupBy].parse({ - ...item, - tagId: item.groupByField, - tag, - }); - }) - .filter((d) => d !== null); + } else if (groupBy === "top_link_tags") { + const ids = response.data.map((item) => item.groupByField); + const tags = await prisma.tag.findMany({ + where: { id: { in: ids } }, + select: { id: true, name: true, color: true }, + }); + const tagById = new Map(tags.map((t) => [t.id, t])); + return response.data + .map(({ groupByField, ...rest }) => { + const tag = tagById.get(groupByField); + if (!tag) return null; + return analyticsResponse[groupBy].parse({ + ...rest, + tagId: groupByField, + tag, + }); + }) + .filter((d) => d !== null); }
259-283: Folders: keep select + use Map + drop groupByField before parseMirror the tags improvement for performance and cleanliness.
- } else if (groupBy === "top_link_folders") { - const folders = await prisma.folder.findMany({ - where: { - id: { - in: response.data.map((item) => item.groupByField), - }, - }, - select: { - id: true, - name: true, - accessLevel: true, - }, - }); - - return response.data - .map((item) => { - const folder = folders.find((f) => f.id === item.groupByField); - if (!folder) return null; - return analyticsResponse[groupBy].parse({ - ...item, - folderId: item.groupByField, - folder, - }); - }) - .filter((d) => d !== null); + } else if (groupBy === "top_link_folders") { + const ids = response.data.map((item) => item.groupByField); + const folders = await prisma.folder.findMany({ + where: { id: { in: ids } }, + select: { id: true, name: true, accessLevel: true }, + }); + const folderById = new Map(folders.map((f) => [f.id, f])); + return response.data + .map(({ groupByField, ...rest }) => { + const folder = folderById.get(groupByField); + if (!folder) return null; + return analyticsResponse[groupBy].parse({ + ...rest, + folderId: groupByField, + folder, + }); + }) + .filter((d) => d !== null); }apps/web/lib/analytics/constants.ts (1)
132-133: Add singular aliases for new endpoints to SINGULAR_ANALYTICS_ENDPOINTS for consistencyThe new endpoints
top_link_tagsandtop_link_folderscurrently require special-case handling inapps/web/ui/analytics/utils.ts(lines 55–56). Adding them toSINGULAR_ANALYTICS_ENDPOINTSeliminates redundancy and aligns with the map-based pattern used elsewhere.export const SINGULAR_ANALYTICS_ENDPOINTS = { continents: "continent", regions: "region", countries: "country", cities: "city", devices: "device", browsers: "browser", referers: "referer", referer_urls: "refererUrl", os: "os", triggers: "trigger", utm_sources: "utm_source", utm_mediums: "utm_medium", utm_campaigns: "utm_campaign", utm_terms: "utm_term", utm_contents: "utm_content", // extra fields top_links: "link", top_urls: "url", + top_link_tags: "tagId", + top_link_folders: "folderId", timeseries: "start", };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/lib/analytics/constants.ts(1 hunks)apps/web/lib/analytics/get-analytics.ts(2 hunks)apps/web/lib/zod/schemas/analytics-response.ts(2 hunks)apps/web/ui/analytics/use-analytics-filters.tsx(4 hunks)apps/web/ui/analytics/utils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-16T19:21:23.506Z
Learnt from: TWilson023
PR: dubinc/dub#2519
File: apps/web/ui/analytics/utils.ts:35-37
Timestamp: 2025-06-16T19:21:23.506Z
Learning: In the `useAnalyticsFilterOption` function in `apps/web/ui/analytics/utils.ts`, the pattern `options?.context ?? useContext(AnalyticsContext)` is intentionally designed as a complete replacement strategy, not a merge. When `options.context` is provided, it should contain all required fields (`baseApiPath`, `queryString`, `selectedTab`, `requiresUpgrade`) and completely replace the React context, not be merged with it. This is used for dependency injection or testing scenarios.
Applied to files:
apps/web/ui/analytics/use-analytics-filters.tsx
🧬 Code graph analysis (3)
apps/web/lib/analytics/get-analytics.ts (2)
packages/prisma/index.ts (1)
prisma(3-9)apps/web/lib/zod/schemas/analytics-response.ts (1)
analyticsResponse(30-529)
apps/web/lib/zod/schemas/analytics-response.ts (1)
apps/web/lib/types.ts (1)
FolderAccessLevel(515-515)
apps/web/ui/analytics/use-analytics-filters.tsx (1)
apps/web/ui/analytics/utils.ts (1)
useAnalyticsFilterOption(22-77)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (6)
apps/web/lib/zod/schemas/analytics-response.ts (2)
493-528: Schemas LGTM; confirm Tag.color guaranteesThe new top_link_tags and top_link_folders shapes look consistent with UI usage (TagBadge expects color; FolderIcon expects accessLevel).
- Confirm Tag.color is non-null in DB/Prisma; otherwise consider z.string().nullable() and handle in UI icon.
4-4: Review comment is based on incorrect assumptions — no Prisma bundling hazard existsThe
FolderAccessLevelenum is not bundled into the client because:
lib/analytics/types.tsonly exports types, not the runtimeanalyticsResponseschema object- UI code imports only type definitions (e.g.,
AnalyticsResponseOptions), never the schemauseAnalyticsFilterOptionmakes API calls via SWR; validation happens server-side in API routesget-analytics.tsis only imported by server API routes, not client codeThe import at line 4 is safe and correctly scoped to server-side validation.
apps/web/ui/analytics/use-analytics-filters.tsx (3)
231-240: New data sources wired correctlyFetching linkTags/linkFolders via useAnalyticsFilterOption with omitGroupByFilterKey aligns with the API changes.
465-488: Folder options mapping + icon usage look goodUsing data.folder for FolderIcon ensures accessLevel/name are present. Nice alignment with analyticsResponse.top_link_folders.
821-827: Deps updated appropriatelyAdding linkTags/linkFolders to the filters memo deps is correct to prevent stale options.
apps/web/lib/analytics/get-analytics.ts (1)
118-121: Confirm intentional routing: top_links uses v3_group_by by designThe code explicitly routes top_links to v3_group_by (line 120 fallback), while top_link_tags and top_link_folders route to v3_group_by_link_metadata (line 119). This suggests the design separates primary entities (links) from their metadata dimensions (tags, folders). Confirm whether this routing strategy is intentional or if top_links should align with the other link metadata groupings.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/lib/zod/schemas/analytics-response.ts (1)
4-5: Avoid runtime import of @prisma/client in shared schema modules.Using z.nativeEnum(FolderAccessLevel) pulls @prisma/client at runtime, which can leak into client/edge bundles and break edge deployments. Prefer an isomorphic enum/union defined in app code (e.g., z.enum([...]) from a shared constant) and keep Prisma strictly server-only.
-import { FolderAccessLevel } from "@prisma/client"; +// import a local, isomorphic enum/union instead (example) +import { folderAccessLevelSchema } from "./folders"; // z.enum([...]) - accessLevel: z - .nativeEnum(FolderAccessLevel) - .describe("The access level of the folder"), + accessLevel: folderAccessLevelSchema.describe("The access level of the folder"),Also applies to: 513-515
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/web/app/api/tags/[id]/route.ts(2 hunks)apps/web/app/api/tags/route.ts(2 hunks)apps/web/lib/openapi/index.ts(2 hunks)apps/web/lib/openapi/tags/create-tag.ts(2 hunks)apps/web/lib/openapi/tags/get-tags.ts(2 hunks)apps/web/lib/openapi/tags/update-tag.ts(2 hunks)apps/web/lib/tinybird/record-link.ts(1 hunks)apps/web/lib/zod/schemas/analytics-response.ts(2 hunks)apps/web/lib/zod/schemas/links.ts(2 hunks)apps/web/lib/zod/schemas/tags.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-17T08:18:19.278Z
Learnt from: devkiran
PR: dubinc/dub#0
File: :0-0
Timestamp: 2025-10-17T08:18:19.278Z
Learning: In the apps/web codebase, `@/lib/zod` should only be used for places that need OpenAPI extended zod schema. All other places should import from the standard `zod` package directly using `import { z } from "zod"`.
Applied to files:
apps/web/lib/openapi/index.ts
🧬 Code graph analysis (7)
apps/web/lib/openapi/tags/create-tag.ts (1)
apps/web/lib/zod/schemas/tags.ts (1)
LinkTagSchema(88-96)
apps/web/lib/openapi/tags/get-tags.ts (1)
apps/web/lib/zod/schemas/tags.ts (1)
LinkTagSchema(88-96)
apps/web/lib/zod/schemas/links.ts (1)
apps/web/lib/zod/schemas/tags.ts (1)
LinkTagSchema(88-96)
apps/web/app/api/tags/[id]/route.ts (1)
apps/web/lib/zod/schemas/tags.ts (1)
LinkTagSchema(88-96)
apps/web/app/api/tags/route.ts (1)
apps/web/lib/zod/schemas/tags.ts (1)
LinkTagSchema(88-96)
apps/web/lib/zod/schemas/analytics-response.ts (1)
apps/web/lib/zod/schemas/tags.ts (1)
LinkTagSchema(88-96)
apps/web/lib/openapi/tags/update-tag.ts (1)
apps/web/lib/zod/schemas/tags.ts (1)
LinkTagSchema(88-96)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (12)
apps/web/lib/tinybird/record-link.ts (1)
42-46: LGTM! Simplified schema usage.The change removes a redundant workspace_id extension since dubLinksMetadataSchema already defines this field with the appropriate transform (lines 29-32). This simplification maintains the same functionality with cleaner code.
apps/web/lib/openapi/tags/create-tag.ts (1)
1-30: LGTM! Consistent schema rename.The update from TagSchema to LinkTagSchema aligns with the project-wide rename to clarify that these tags are specifically for links. The change is applied consistently in both the import and the response schema definition.
apps/web/app/api/tags/route.ts (1)
1-119: LGTM! Route handler updated consistently.The TagSchema → LinkTagSchema rename is correctly applied in both the import and the response parsing. This maintains type safety while aligning with the new schema naming convention.
apps/web/lib/openapi/tags/get-tags.ts (1)
1-27: LGTM! Array response schema updated correctly.The change from
z.array(TagSchema)toz.array(LinkTagSchema)maintains consistency with the schema rename while correctly representing the list response type.apps/web/app/api/tags/[id]/route.ts (1)
1-118: LGTM! Update endpoint schema migration complete.The PATCH handler correctly uses LinkTagSchema for response validation, maintaining consistency with the broader schema rename across the API surface.
apps/web/lib/zod/schemas/links.ts (1)
1-832: LGTM! Link schema tag field updated.The tags field correctly references LinkTagSchema.array(), maintaining the link-to-tags relationship while aligning with the schema rename. The change is isolated to the import and field definition with no impact on the rest of the comprehensive link schema.
apps/web/lib/zod/schemas/tags.ts (1)
86-96: LGTM! Improved schema naming convention.The rename from TagSchema to LinkTagSchema with the corresponding OpenAPI title update clarifies the domain model, making it explicit that these tags are specifically for link categorization. The schema structure remains unchanged, maintaining backward compatibility at the data level.
apps/web/lib/openapi/index.ts (1)
1-83: LGTM! OpenAPI document schema updated.The OpenAPI components schema correctly references LinkTagSchema, ensuring generated API documentation and client SDKs will use the new "LinkTag" type name. This completes the systematic rename across the entire OpenAPI surface.
apps/web/lib/openapi/tags/update-tag.ts (1)
3-3: LinkTagSchema migration is complete and consistent throughout the codebase.The verification confirms that the schema rename from
TagSchematoLinkTagSchemahas been properly applied everywhere—no remnantTagSchemareferences or old OpenAPI component refs remain. All imports and usages are updated consistently across OpenAPI definitions, API routes, and schema files.Note: This change will update the OpenAPI component name from "Tag" to "LinkTag", which is a breaking change for external SDK consumers. If backwards compatibility is needed, consider documenting this as a breaking change or adding a compatibility alias in your OpenAPI documentation.
apps/web/lib/zod/schemas/analytics-response.ts (3)
508-525: No changes required—folder existence and accessLevel invariants are already handled correctly.The handler at lines 258-282 in
get-analytics.tsexplicitly filters out entries where folders are missing or deleted:if (!folder) return null(line 275) followed by.filter((d) => d !== null)(line 282). Only complete folder objects reach the schema validation. The Prisma query always selectsaccessLevel, and sinceFolderAccessLevelis defined as"write" | "read"from constants, it is always present when a folder exists. The schema correctly marksfolderas required, not optional, because incomplete data is filtered upstream.
494-506: No changes needed—the schema is already correct.The review comment raises a valid concern about deleted tags, but the code already handles it. In
get-analytics.ts(lines 238–260), thetop_link_tagshandler:
- Fetches all tags by ID in a batch query
- Maps over response items and explicitly checks
if (!tag) return null;- Filters out nulls with
.filter((d) => d !== null);Only records with found tags reach schema validation. Additionally, the Prisma schema ensures that when a tag is deleted, its associated
LinkTagentries cascade-delete, so orphaned analytics records won't exist. Thetagfield in the schema can remain non-nullable because the runtime logic guarantees it's always present in the validated response.Likely an incorrect or invalid review comment.
493-493: No breaking changes detected — this appears to be a new schema ref, not a rename.Analysis shows:
"AnalyticsTopPartners"is new to the codebase (no prior git history for this ref name)- Naming follows the established pattern:
topPartners→AnalyticsTopPartners, consistent withtop_link_tags→AnalyticsTopLinkTags- No old
"AnalyticsPartners"ref exists as a schema (only found as a React component name)- No generated SDK directories or downstream OpenAPI tooling found in the repository
- The ref change is a new addition, not a replacement affecting downstream consumers
The original review concern about downstream breaking changes appears to be unfounded—this is a new schema reference following established conventions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/lib/zod/schemas/analytics-response.ts (1)
494-525: Well-structured analytics response schemas.The new
top_link_tagsandtop_link_foldersschemas correctly:
- Follow the established pattern from
top_partners- Include proper metric fields with sensible defaults
- Use appropriate OpenAPI references
- Handle the folder's
accessLevelwithnativeEnum(FolderAccessLevel)One minor observation:
top_link_tagsreferencesLinkTagSchemawhiletop_link_foldersdefines an inline object. If the folder structure is reused elsewhere, consider extracting aFolderSchemafor consistency (though the current approach is valid).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/web/app/api/tags/[id]/route.ts(2 hunks)apps/web/app/api/tags/route.ts(2 hunks)apps/web/lib/analytics/constants.ts(1 hunks)apps/web/lib/analytics/get-analytics.ts(2 hunks)apps/web/lib/openapi/index.ts(2 hunks)apps/web/lib/openapi/tags/create-tag.ts(2 hunks)apps/web/lib/openapi/tags/get-tags.ts(2 hunks)apps/web/lib/openapi/tags/update-tag.ts(2 hunks)apps/web/lib/tinybird/record-link.ts(1 hunks)apps/web/lib/zod/schemas/analytics-response.ts(2 hunks)apps/web/lib/zod/schemas/links.ts(2 hunks)apps/web/lib/zod/schemas/tags.ts(1 hunks)apps/web/ui/analytics/use-analytics-filters.tsx(4 hunks)apps/web/ui/analytics/utils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-16T19:21:23.506Z
Learnt from: TWilson023
PR: dubinc/dub#2519
File: apps/web/ui/analytics/utils.ts:35-37
Timestamp: 2025-06-16T19:21:23.506Z
Learning: In the `useAnalyticsFilterOption` function in `apps/web/ui/analytics/utils.ts`, the pattern `options?.context ?? useContext(AnalyticsContext)` is intentionally designed as a complete replacement strategy, not a merge. When `options.context` is provided, it should contain all required fields (`baseApiPath`, `queryString`, `selectedTab`, `requiresUpgrade`) and completely replace the React context, not be merged with it. This is used for dependency injection or testing scenarios.
Applied to files:
apps/web/ui/analytics/use-analytics-filters.tsx
🔇 Additional comments (21)
apps/web/lib/tinybird/record-link.ts (1)
42-46: LGTM! Schema simplification improves consistency.Using
dubLinksMetadataSchemadirectly removes redundant schema extension. The base schema (lines 29-32) already handlesworkspace_idnullish-to-empty-string transformation, so this change maintains identical behavior while improving code clarity. BothrecordLinkTBandrecordLinkTBOldnow use the same schema definition consistently.apps/web/ui/analytics/utils.ts (1)
55-56: LGTM! New groupBy filter mappings integrated correctly.The mappings for
top_link_tags→"tagIds"andtop_link_folders→"folderId"follow the established pattern and correctly identify the filter parameters to omit when fetching all available options for these groupBy dimensions.apps/web/lib/zod/schemas/links.ts (2)
10-10: LGTM! Schema import updated consistently.The import change from
TagSchematoLinkTagSchemaaligns with the project-wide schema rename.
652-652: LGTM! Tag array type updated correctly.The
tagsfield now correctly usesLinkTagSchema.array(), maintaining type consistency with the renamed schema.apps/web/app/api/tags/[id]/route.ts (1)
4-4: LGTM! API route updated with renamed schema.The import and runtime parsing correctly use
LinkTagSchema, ensuring the PATCH endpoint response validates against the updated schema definition.Also applies to: 40-40
apps/web/lib/zod/schemas/tags.ts (1)
88-96: LGTM! Schema renamed consistently.The export rename from
TagSchematoLinkTagSchemaand the corresponding OpenAPI title update maintain consistency. All consumers within this PR have been updated accordingly.apps/web/lib/openapi/tags/create-tag.ts (1)
2-2: LGTM! OpenAPI definition updated correctly.The create tag endpoint's 201 response schema now correctly references
LinkTagSchema, maintaining consistency with the renamed schema.Also applies to: 22-22
apps/web/app/api/tags/route.ts (1)
5-5: LGTM! POST endpoint updated with renamed schema.The import and response parsing correctly use
LinkTagSchema, ensuring the created tag validates against the updated schema definition.Also applies to: 111-111
apps/web/lib/analytics/constants.ts (1)
132-133: LGTM! New analytics endpoints added correctly.The
top_link_tagsandtop_link_foldersendpoints are properly registered inVALID_ANALYTICS_ENDPOINTS, enabling analytics grouping by link tags and folders. The placement aftertop_linksandtop_urlsis logical and maintains consistency.apps/web/lib/openapi/tags/get-tags.ts (1)
3-3: LGTM! OpenAPI tags list endpoint updated correctly.The import and 200 response schema correctly use
LinkTagSchema, ensuring the tags list response validates against the updated schema definition.Also applies to: 19-19
apps/web/lib/openapi/index.ts (1)
4-4: LGTM: Schema rename is consistent.The rename from
TagSchematoLinkTagSchemaaligns with the broader refactoring across the codebase and maintains API consistency.Also applies to: 65-65
apps/web/lib/openapi/tags/update-tag.ts (1)
3-3: LGTM: OpenAPI definition updated consistently.The update to
LinkTagSchemamaintains consistency with the broader schema refactoring without altering the API contract.Also applies to: 29-29
apps/web/lib/analytics/get-analytics.ts (3)
118-120: LGTM: Pipe routing correctly extended.The addition of
"top_link_tags"and"top_link_folders"to route to"v3_group_by_link_metadata"properly integrates these new groupBy options into the analytics pipeline.
238-257: LGTM: Well-structured tag analytics branch.The
top_link_tagsimplementation correctly:
- Fetches Tag records by ID from the Tinybird response
- Handles missing tags gracefully with null checks
- Parses results using the appropriate schema
- Follows the established pattern from
top_partners
258-282: LGTM: Folder analytics branch implemented correctly.The
top_link_foldersimplementation:
- Correctly selects fields (
id,name,accessLevel) that match theanalyticsResponse.top_link_folders.folderschema- Handles missing folders gracefully
- Maintains consistency with the existing analytics patterns
apps/web/lib/zod/schemas/analytics-response.ts (2)
4-5: LGTM: Imports are correct.The imports properly bring in
FolderAccessLevelfrom Prisma andLinkTagSchemafor the new analytics response structures.
493-493: LGTM: Naming consistency improvement.Updating the OpenAPI reference to
AnalyticsTopPartnersaligns with the naming convention for the newAnalyticsTopLinkTagsandAnalyticsTopLinkFoldersschemas.apps/web/ui/analytics/use-analytics-filters.tsx (4)
231-240: LGTM: Filter data sources updated to use link-based analytics.The switch from direct tag/folder queries to
top_link_tagsandtop_link_foldersanalytics endpoints enables the filters to display analytics metrics (clicks, leads, sales) alongside each option, improving the user experience.
465-488: LGTM: Folder filter correctly integrated with analytics.The folder filter implementation:
- Properly destructures folder data from
linkFolders- Stores the complete folder object in
option.datafor use ingetOptionIcon- Displays analytics metrics in the filter dropdown via
getFilterOptionTotal- Maintains the existing folder icon rendering pattern
496-511: LGTM: Tag filter efficiently implemented.The tag filter implementation:
- Correctly maps
linkTagsdata with analytics metrics- Pragmatically stores only the
colorinoption.data(sufficient forTagBadgerendering)- Displays analytics metrics alongside each tag option
- Uses the
multiple: truesetting appropriately for multi-select tags
825-826: The omitGroupByFilterKey mapping is correctly implemented.Verified in
apps/web/ui/analytics/utils.tslines 55-56:
top_link_tags→tagIdstop_link_folders→folderIdThe useMemo dependency updates and filter endpoint implementations are consistent and correct.
steven-tey
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/lib/analytics/constants.ts (1)
159-165: <!-- [scratchpad_start]
[task verify_review_comment_1/1]
[observations]
- Schema definition found: analytics-response.ts line 491 defines
top_link_tagswith fieldtagId: z.string()(SINGULAR)- SINGULAR_ANALYTICS_ENDPOINTS line 160:
top_link_tags: "tagIds"(PLURAL) - MISMATCH CONFIRMED- Usage in get-analytics.ts line 296:
[SINGULAR_ANALYTICS_ENDPOINTS[groupBy]]: item.groupByField- generic dynamic mapping- Usage in get-analytics.ts line 258: hardcoded
tagId: item.groupByField- specific handling- Review comment is CORRECT: mapping uses plural "tagIds" but schema expects singular "tagId"
[analysis]- Schema defines singular field name, not plural
- SINGULAR_ANALYTICS_ENDPOINTS has wrong value for top_link_tags
- Line 296 usage would fail schema validation if groupBy === "top_link_tags" because it would try to set tagIds but schema expects tagId
- Line 258 bypasses the mapping by hardcoding tagId, so specific top_link_tags path works
- But generic fallback paths using SINGULAR_ANALYTICS_ENDPOINTS would fail
[done]- Verification complete: Review comment is accurate about the mismatch
- The fix requested (change "tagIds" to "tagId") is correct
</scratchpad_end] -->Fix singular key for top_link_tags
The review comment is correct. The analytics-response schema defines
top_link_tagswith a singular field name:// apps/web/lib/zod/schemas/analytics-response.ts line 491 top_link_tags: z.object({ tagId: z.string().describe("The ID of the tag"), tag: LinkTagSchema, ... })But
SINGULAR_ANALYTICS_ENDPOINTSmaps it to plural"tagIds". This causes a schema validation failure when the dynamic mapping atget-analytics.ts:296is used:[SINGULAR_ANALYTICS_ENDPOINTS[groupBy]]: item.groupByField, // would set tagIds, but schema expects tagIdUpdate the mapping in
apps/web/lib/analytics/constants.ts:160from"tagIds"to"tagId".
🧹 Nitpick comments (1)
apps/web/lib/analytics/get-analytics.ts (1)
244-262: Select minimal tag fields to reduce query/loadfindMany without select pulls full Tag rows. Since analyticsResponse expects LinkTagSchema, consider selecting only required columns (e.g., id, name, color) to cut payload and CPU.
Apply this diff (adjust fields if LinkTagSchema needs more):
- const tags = await prisma.tag.findMany({ - where: { - id: { - in: response.data.map((item) => item.groupByField), - }, - }, - }); + const tags = await prisma.tag.findMany({ + where: { + id: { in: response.data.map((item) => item.groupByField) }, + }, + select: { + id: true, + name: true, + color: true, + }, + });Double-check LinkTagSchema fields to ensure the select set is sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/web/app/api/tags/[id]/route.ts(2 hunks)apps/web/app/api/tags/route.ts(2 hunks)apps/web/lib/analytics/constants.ts(2 hunks)apps/web/lib/analytics/get-analytics.ts(2 hunks)apps/web/lib/openapi/index.ts(2 hunks)apps/web/lib/openapi/tags/create-tag.ts(2 hunks)apps/web/lib/openapi/tags/get-tags.ts(2 hunks)apps/web/lib/openapi/tags/update-tag.ts(2 hunks)apps/web/lib/tinybird/record-link.ts(1 hunks)apps/web/lib/zod/schemas/analytics-response.ts(3 hunks)apps/web/lib/zod/schemas/links.ts(2 hunks)apps/web/lib/zod/schemas/tags.ts(1 hunks)apps/web/ui/analytics/use-analytics-filters.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-17T08:18:19.278Z
Learnt from: devkiran
PR: dubinc/dub#0
File: :0-0
Timestamp: 2025-10-17T08:18:19.278Z
Learning: In the apps/web codebase, `@/lib/zod` should only be used for places that need OpenAPI extended zod schema. All other places should import from the standard `zod` package directly using `import { z } from "zod"`.
Applied to files:
apps/web/lib/openapi/index.ts
📚 Learning: 2025-06-16T19:21:23.506Z
Learnt from: TWilson023
PR: dubinc/dub#2519
File: apps/web/ui/analytics/utils.ts:35-37
Timestamp: 2025-06-16T19:21:23.506Z
Learning: In the `useAnalyticsFilterOption` function in `apps/web/ui/analytics/utils.ts`, the pattern `options?.context ?? useContext(AnalyticsContext)` is intentionally designed as a complete replacement strategy, not a merge. When `options.context` is provided, it should contain all required fields (`baseApiPath`, `queryString`, `selectedTab`, `requiresUpgrade`) and completely replace the React context, not be merged with it. This is used for dependency injection or testing scenarios.
Applied to files:
apps/web/ui/analytics/use-analytics-filters.tsx
🔇 Additional comments (21)
apps/web/lib/tinybird/record-link.ts (1)
42-46: LGTM! Clean simplification.Removing the redundant
workspace_idoverride is correct sincedubLinksMetadataSchemaalready defines this field with the same nullish transform (lines 29-32). This simplifies the schema definition without changing behavior.apps/web/lib/zod/schemas/tags.ts (1)
88-96: LGTM! Consistent schema rename.The rename from
TagSchematoLinkTagSchemaimproves clarity and aligns with the schema's purpose for link-specific tags. The OpenAPI title update maintains consistency with the export name.apps/web/lib/openapi/tags/get-tags.ts (1)
1-27: LGTM! Import and usage updated correctly.The import and response schema are correctly updated to use
LinkTagSchema, maintaining consistency with the schema rename.apps/web/app/api/tags/route.ts (1)
1-119: LGTM! API route updated consistently.The import and response parsing are correctly updated to use
LinkTagSchema, ensuring the API returns data validated against the renamed schema.apps/web/lib/openapi/tags/create-tag.ts (1)
1-30: LGTM! OpenAPI definition updated correctly.The import and response schema are correctly updated to use
LinkTagSchemafor the create tag operation.apps/web/lib/zod/schemas/links.ts (2)
10-10: LGTM! Import updated correctly.The import is correctly updated to use
LinkTagSchemafrom the tags schema module.
652-654: LGTM! Link tags field updated correctly.The
tagsfield type is correctly updated to useLinkTagSchema.array(), maintaining the nullable constraint and description.apps/web/lib/openapi/index.ts (1)
1-83: LGTM! OpenAPI document generation updated correctly.The import and component schema reference are correctly updated to use
LinkTagSchema, ensuring the generated OpenAPI specification reflects the schema rename.apps/web/app/api/tags/[id]/route.ts (1)
1-55: LGTM! API route updated consistently.The import and response parsing in the PATCH handler are correctly updated to use
LinkTagSchema, ensuring tag update responses are validated against the renamed schema.apps/web/lib/openapi/tags/update-tag.ts (1)
3-3: Schema rename alignment looks goodImporting and returning LinkTagSchema keeps OpenAPI in sync with the Tag → LinkTag rename. No issues found.
Also applies to: 29-30
apps/web/lib/analytics/constants.ts (1)
129-135: Expose new groupBy endpointsAdding top_folders, top_link_tags, and top_domains to VALID_ANALYTICS_ENDPOINTS is correct and aligns with the new views.
apps/web/lib/analytics/get-analytics.ts (2)
118-126: Correct pipe routing for metadata group-bysRouting top_folders, top_link_tags, top_domains, and top_partners to v3_group_by_link_metadata matches the enrichment strategy. Looks good.
Please confirm Tinybird pipe v3_group_by_link_metadata accepts these groupBy values to avoid 400s.
264-287: Folder enrichment matches schema and is efficientSelecting id, name, accessLevel aligns with analyticsResponse.top_folders. Solid.
apps/web/lib/zod/schemas/analytics-response.ts (3)
4-6: Imports are appropriate for new schema fieldsFolderAccessLevel and LinkTagSchema imports are needed and correct.
470-513: New analytics schemas read clearly and match use-casestop_folders, top_link_tags, and top_domains structures look consistent with the data returned from get-analytics enrichment.
Ensure LinkTagSchema is the public tag shape you intend to expose via OpenAPI.
537-537: OpenAPI ref renametop_partners ref updated to AnalyticsTopPartners — consistent with naming.
apps/web/ui/analytics/use-analytics-filters.tsx (5)
198-212: Good lazy-loading via isRequested for new sourcesFetching top_folders, top_link_tags, and top_domains only when requested keeps the UI snappy.
336-336: Stable deps for AI suggestionsDependency list narrowed appropriately; no unnecessary re-computes.
434-457: Folder option rendering aligns with enriched dataUsing data.folder for icons/labels matches analyticsResponse.top_folders. LGTM.
465-479: Tag options wired to link-based dataMapping linkTags → TagBadge with color is correct and user-friendly.
495-499: Domain options from top_domainsConsistent with constants/get-analytics. Looks good.
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/tests/utils/verify-commission.ts (1)
35-36: Comment says "if necessary" but delay is unconditional.This hard-coded 2-second delay compounds with the 1.5s delay above (3.5s total artificial wait), slowing all tests that verify commissions. The comment suggests conditional logic but the delay always executes.
Apply a similar polling pattern here, or make the delay truly conditional:
- // Small delay if necessary for async commission processing - await new Promise((resolve) => setTimeout(resolve, 2000)); + // Poll for commission to be processed + const maxWait = 20; // 2 seconds total, 100ms intervals + for (let i = 0; i < maxWait; i++) { + const { data: check } = await http.get<CommissionResponse[]>({ + path: "/commissions", + query: { customerId }, + }); + if (check.length > 0) break; + await new Promise((resolve) => setTimeout(resolve, 100)); + }This exits early when data is ready, avoiding unnecessary waits.
🧹 Nitpick comments (2)
packages/tinybird/pipes/v2_top_programs.pipe (1)
59-104: Consider using a materialized view for consistency.The
top_programs_clicksandtop_programs_salesnodes now both use materialized views (dub_click_events_mvanddub_sale_events_mv), buttop_programs_leadsstill queriesdub_lead_eventsdirectly. For consistency and performance, consider whetherdub_lead_events_mvshould also be used here.If a materialized view for lead events exists or should be created, apply this change:
- FROM dub_lead_events AS le + FROM dub_lead_events_mv AS lepackages/tinybird/pipes/v3_group_by_link_metadata.pipe (1)
246-250: Minor: Inconsistent trailing whitespace.Line 248 has a trailing space after
group_by_link_metadata_saleswhile line 249 does not. Consider standardizing for consistency.Apply this diff to remove the trailing space:
- {% elif eventType == 'sales' %}group_by_link_metadata_sales + {% elif eventType == 'sales' %}group_by_link_metadata_sales {% elif eventType == 'composite' %}group_by_link_metadata_composite
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/tests/tracks/track-sale.test.ts(0 hunks)apps/web/tests/utils/verify-commission.ts(1 hunks)packages/tinybird/pipes/dub_lead_events_pipe.pipe(1 hunks)packages/tinybird/pipes/dub_sale_events_pipe.pipe(1 hunks)packages/tinybird/pipes/v2_top_programs.pipe(1 hunks)packages/tinybird/pipes/v3_events.pipe(1 hunks)packages/tinybird/pipes/v3_group_by_link_metadata.pipe(3 hunks)packages/tinybird/pipes/v3_timeseries.pipe(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/tests/tracks/track-sale.test.ts
🔇 Additional comments (8)
packages/tinybird/pipes/v2_top_programs.pipe (1)
112-112: Materialized view change is verified and safe.The switch from
dub_sale_eventstodub_sale_events_mvis a valid optimization. The materialized view is properly defined inpackages/tinybird/datasources/dub_sale_events_mv.datasourcewith a comprehensive schema that includes all required columns (link_id,amount,workspace_id, etc.). The MV is created viadub_sale_events_pipewhich reads from the base table with TYPE materialized, and this pattern is already proven and widely used across 11+ other pipes in the codebase.packages/tinybird/pipes/v3_timeseries.pipe (1)
349-349: LGTM! Simplified join logic.The consolidation to always join
timeseries_sales_dataremoves conditional branching and aligns with the PR's overall simplification of the Tinybird pipeline architecture.packages/tinybird/pipes/v3_events.pipe (1)
265-270: LGTM! Streamlined endpoint routing.The simplified conditional logic is clearer and maintains correct routing for all event types while removing unnecessary nested conditionals.
packages/tinybird/pipes/v3_group_by_link_metadata.pipe (3)
8-20: LGTM! Well-structured groupBy routing.The
arrayJoin(tag_ids)approach fortop_link_tagscorrectly handles the multi-valued nature of tags, while themultiIfcleanly routes other groupBy options. The default fallback topartner_idprovides a safe fallback for unexpected values.
33-43: LGTM! Consistent filtering logic.The
multiIfconditions correctly filter out empty values for each groupBy type, ensuring only valid grouping keys are included. The logic mirrors the groupBy selection in lines 8-20.
228-237: LGTM! Clean composite aggregation.The new node properly combines metrics from all three event types using LEFT JOINs, enabling comprehensive composite analytics views.
packages/tinybird/pipes/dub_lead_events_pipe.pipe (1)
10-14: LGTM! Consistent empty string handling across pipes.The same
nullIfpattern applied here matches the approach indub_sale_events_pipe.pipe, ensuring consistent empty string treatment across both lead and sale event pipes. This uniformity simplifies maintenance and ensures predictable behavior when grouping by folders, domains, or link tags.packages/tinybird/pipes/dub_sale_events_pipe.pipe (1)
9-14: Verify backward compatibility of schema changes with historical data.Tinybird automatically handles materialized view updates when pipe definitions change—new data ingestion will use the updated
nullIf/coalescelogic. However, historical records in the MV will not be retransformed. Sinceworkspace_idis used inENGINE_SORTING_KEYand these fields are critical for groupBy operations in downstream pipes (e.g.,v3_group_by_link_metadata,v3_group_by), verify that:
- Existing dashboards/queries don't break due to mixed old/new transformation logic
- No queries rely on consistent historical values for
workspace_id,domain, orkey- If data consistency is critical, a manual MV rebuild via Tinybird CLI may be needed
Top folders:
Top link tags:
Top domains:
Summary by CodeRabbit
New Features
Improvements
Other