fix(ts): preserve EventType literal inference under TS 5.4 #3080
Conversation
|
@Hritwik001 is attempting to deploy a commit to the Dub Team on Vercel. A member of the Team first needs to authorize it. |
|
|
WalkthroughAdds explicit event type handling and runtime validation to the analytics module. Introduces Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 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/lib/analytics/get-events.ts (1)
97-101: Critical: Type assertion masks runtime lookup failure.The type assertion
eventType as EventTypeis incorrect because:
eventTypeis plural:"clicks" | "leads" | "sales"(based on object keys)EventTypeis singular:"click" | "lead" | "sale"(from line 39)- Indexing
{clicks: ..., leads: ..., sales: ...}with singular values fails- Always falls back to
clickEventSchemaTBEndpoint, breaking lead/sale eventsRemove the assertion and use the plural form that matches the object keys:
data: { clicks: clickEventSchemaTBEndpoint, leads: leadEventSchemaTBEndpoint, sales: saleEventSchemaTBEndpoint, - }[eventType as EventType] ?? clickEventSchemaTBEndpoint, + }[eventType] ?? clickEventSchemaTBEndpoint,Alternatively, if you need to change to singular forms, update the object keys to match:
data: { - clicks: clickEventSchemaTBEndpoint, - leads: leadEventSchemaTBEndpoint, - sales: saleEventSchemaTBEndpoint, + click: clickEventSchemaTBEndpoint, + lead: leadEventSchemaTBEndpoint, + sale: saleEventSchemaTBEndpoint, }[eventType as EventType] ?? clickEventSchemaTBEndpoint,But this requires verifying that
EventsFilters.eventuses singular forms throughout the entire call chain.
🧹 Nitpick comments (1)
apps/web/lib/analytics/get-events.ts (1)
28-46: Consider simpler comment style.The emoji-decorated comments and box-drawing section headers are inconsistent with the rest of the codebase, which uses plain
//comments. Consider using standard JSDoc or simple inline comments to match existing conventions.Also applies to: 201-203
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/lib/analytics/get-events.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-06T07:59:03.120Z
Learnt from: devkiran
Repo: dubinc/dub PR: 2177
File: apps/web/lib/api/links/bulk-create-links.ts:66-84
Timestamp: 2025-06-06T07:59:03.120Z
Learning: In apps/web/lib/api/links/bulk-create-links.ts, the team accepts the risk of potential undefined results from links.find() operations when building invalidLinks arrays, because existing links are fetched from the database based on the input links, so matches are expected to always exist.
Applied to files:
apps/web/lib/analytics/get-events.ts
🧬 Code graph analysis (1)
apps/web/lib/analytics/get-events.ts (3)
apps/web/lib/analytics/constants.ts (1)
EVENT_TYPES(173-173)apps/web/lib/analytics/types.ts (1)
EventType(31-31)apps/web/lib/zod/schemas/clicks.ts (1)
clickEventSchemaTBEndpoint(38-58)
🔇 Additional comments (1)
apps/web/lib/analytics/get-events.ts (1)
196-196: LGTM: Type-safe null filtering.The type predicate correctly narrows the array type by removing null values, ensuring downstream code receives a non-nullable array.
| export const EVENT_TYPES = [ | ||
| "click", | ||
| "lead", | ||
| "sale", | ||
| ] as const; | ||
|
|
||
| /** 🔹 Keeps EventType as a precise literal union */ | ||
| export type EventType = (typeof EVENT_TYPES)[number]; | ||
|
|
||
| /** 🔹 Zod enum for runtime validation & compile-time safety */ | ||
| export const EventTypeSchema = z.enum(EVENT_TYPES); |
There was a problem hiding this comment.
Critical: EVENT_TYPES values conflict with existing codebase and cause runtime mismatch.
This introduces duplicate exports with incompatible values:
- This PR:
EVENT_TYPES = ["click", "lead", "sale"](singular) - Existing (
apps/web/lib/analytics/constants.ts):EVENT_TYPES = ["clicks", "leads", "sales"](plural) - Existing (
apps/web/lib/analytics/types.ts):EventTypealready exported
The singular values don't match the object mapping keys at lines 98-100 (clicks, leads, sales), which will cause the lookup at line 101 to fail and always fall back to clickEventSchemaTBEndpoint.
Fix: Use the existing exports instead of creating duplicates:
-/* -------------------------------------------------------------------------- */
-/* ✅ Event Type Handling */
-/* -------------------------------------------------------------------------- */
-
-export const EVENT_TYPES = [
- "click",
- "lead",
- "sale",
-] as const;
-
-/** 🔹 Keeps EventType as a precise literal union */
-export type EventType = (typeof EVENT_TYPES)[number];
-
-/** 🔹 Zod enum for runtime validation & compile-time safety */
-export const EventTypeSchema = z.enum(EVENT_TYPES);
+import { EVENT_TYPES } from "./constants";
+import type { EventType } from "./types";If TS 5.4 literal narrowing is still an issue with the plural forms, the fix should update the existing constants/types files rather than creating conflicting duplicates.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/lib/analytics/get-events.ts around lines 32 to 42, the new
EVENT_TYPES array uses singular values and duplicates existing exports which
causes runtime lookup failures; remove the duplicate definition and import the
canonical EVENT_TYPES and EventType from apps/web/lib/analytics/constants.ts and
apps/web/lib/analytics/types.ts (or update those original files if you truly
need to change the plural -> singular shape), then reuse the imported constant
for the z.enum schema so keys match the existing mapping and the runtime lookup
no longer falls back to the wrong schema.
There was a problem hiding this comment.
Thanks! The fix reuses the existing EVENT_TYPES tuple and EventType type — no new duplicates introduced.
Let me know if you’d prefer the literal tuple to live inside the shared constants file instead 🙏
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current contents of constants.ts and types.ts
echo "=== constants.ts ==="
cat apps/web/lib/analytics/constants.ts
echo ""
echo "=== types.ts ==="
cat apps/web/lib/analytics/types.ts
echo ""
echo "=== All files changed in this PR ==="
git diff --name-only origin/main...HEAD
Length of output: 6011
🏁 Script executed:
#!/bin/bash
# Check the eventsQuerySchema to see what event values are actually used
echo "=== Finding eventsQuerySchema definition ==="
rg -n "eventsQuerySchema\s*=" apps/web/lib/zod/schemas/analytics.ts -A 20
echo ""
echo "=== Check how event parameter is used in the codebase ==="
rg -n 'event:\s*"(click|lead|sale|clicks|leads|sales)"' apps/web --type ts -C 2 | head -50
echo ""
echo "=== Check Tinybird endpoint parameter ==="
rg -n 'eventType.*clicks|leads|sales' apps/web/lib/analytics/get-events.ts -C 3
Length of output: 4705
@Hritwik001, I appreciate your response, but I need to clarify what I'm seeing in the current state of the code:
Confirmed Duplicate Exports
In apps/web/lib/analytics/constants.ts (line 173):
export const EVENT_TYPES = ["clicks", "leads", "sales"] as const; // PLURALIn apps/web/lib/analytics/types.ts (line 31):
export type EventType = (typeof EVENT_TYPES)[number]; // imports from constantsIn apps/web/lib/analytics/get-events.ts (this PR, lines 32-42):
export const EVENT_TYPES = [
"click", // SINGULAR
"lead",
"sale",
] as const;
export type EventType = (typeof EVENT_TYPES)[number]; // duplicate type export
export const EventTypeSchema = z.enum(EVENT_TYPES);Runtime Mismatch Problem
The singular values in your new EVENT_TYPES won't match the object mapping at lines 98-101 in the same file:
{
clicks: clickEventSchemaTBEndpoint, // plural key
leads: leadEventSchemaTBEndpoint, // plural key
sales: saleEventSchemaTBEndpoint, // plural key
}[eventType as EventType] // will fail lookup with singular valuesAdditionally, eventsQuerySchema expects plural values ("clicks", "leads", "sales") since it references the original EVENT_TYPES from constants.
Questions
- Did you update the other files (
constants.tsandtypes.ts) in commits not yet pushed? - Should the object mapping keys also change from plural to singular?
- Would you prefer to centralize the literal tuple in
constants.tsinstead of creating it inget-events.ts?
The fix should either:
- Option A: Import and reuse the existing plural
EVENT_TYPESfromconstants.ts - Option B: Update
constants.tsto use singular values and ensure all object mappings use singular keys consistently
Could you clarify which approach you've taken or prefer? 🙏
🧬 Code Graph Analysis Results
[filename: apps/web/lib/analytics/get-events.ts]
Line range 18-26:
export const EVENT_TYPES = [
"click",
"lead",
"sale",
] as const;
/** 🔹 Keeps EventType as a precise literal union */
export type EventType = (typeof EVENT_TYPES)[number];
/** 🔹 Zod enum for runtime validation & compile-time safety */
export const EventTypeSchema = z.enum(EVENT_TYPES);
[filename: apps/web/lib/analytics/constants.ts]
Line range 173-173:
export const EVENT_TYPES = ["clicks", "leads", "sales"] as const;
[filename: apps/web/lib/analytics/types.ts]
Line range 31-31:
export type EventType = (typeof EVENT_TYPES)[number];
🧠 Learnings used
Learnt from: devkiran
Repo: dubinc/dub PR: 2637
File: apps/web/app/(ee)/api/singular/webhook/route.ts:0-0
Timestamp: 2025-07-17T06:41:45.620Z
Learning: In the Singular integration (apps/web/app/(ee)/api/singular/webhook/route.ts), the event names in the singularToDubEvent object have intentionally different casing: "Copy GAID" and "copy IDFA". This casing difference is valid and should not be changed, as these are the correct event names expected from Singular.
|
Hi 👋 |
2 participants
Context
Under TypeScript 5.4, chained Zod transforms can widen the \EventType\ literal union to \string.
Fix
Scope
Notes
Summary by CodeRabbit
Refactor
Fixes #3059