Advanced Analytics Filters#3436
Conversation
… a multi-value item
Advanced Analytics Filters
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
📝 WalkthroughWalkthroughReplaces simple query parsing with backward-compatible parsers and operator-aware multi-value filters; adds filter helper utilities, threads normalized filters into analytics/event pipelines and Tinybird pipes, updates UI filter types/components and hooks for operator/multi-value flows, converts boolean root filters to structured ParsedFilter objects, and adds tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "API Route"
participant Parser as "parseAnalyticsQuery / parseEventsQuery"
participant Helpers as "filter-helpers"
participant Service as "get-analytics / get-events"
participant Tinybird
Client->>API: GET /analytics or /events?...
API->>Parser: parseAnalyticsQuery(searchParams)
Parser-->>API: parsedParams (domainFilter, folderIdFilter, filters...)
API->>Helpers: getFirstFilterValue / buildAdvancedFilters / extractWorkspaceLinkFilters
Helpers-->>API: domain, folderId, advancedFilters, tinybirdParams
API->>Service: call get-analytics/get-events with tinybirdParams (JSON(filters))
Service->>Tinybird: POST pipe with merged metadata + filters
Tinybird-->>Service: results
Service-->>API: formatted response
API-->>Client: 200 JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@pepeladeira is attempting to deploy a commit to the Dub Team on Vercel. A member of the Team first needs to authorize it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/lib/zod/schemas/analytics.ts (1)
76-89:⚠️ Potential issue | 🟡 MinorChange
deprecated: falsetodeprecated: truefor consistency.The
linkIdfield is functionally deprecated — theparseAnalyticsQueryfunction explicitly converts it tolinkIdsfor backward compatibility, and its own documentation (lines 415–416) lists it as a deprecated singular field alongsidetagId. However, the metadata marks it asdeprecated: falseon line 82, which contradicts this design. To match the pattern used fortagId(line 356:.meta({ deprecated: true })) and accurately reflect the field's status in API tooling and documentation, this should bedeprecated: true.packages/tinybird/pipes/v3_events.pipe (1)
491-512:⚠️ Potential issue | 🔴 Critical
sale_eventsnode is missing field-based advanced filter handling fromJSON(filters).The
click_eventsandlead_eventsnodes in this file both include the{% elif item.get('field', '') %}branch to handle field-based filters (country, city, device, etc. with IN/NOT IN). Thesale_eventsnode only handles themetadata.operand filters (lines 493-509) and has nofield-based branch.This means advanced filters like
country NOT IN ['US']will be silently ignored for sale events on the/api/eventsendpoint, while working correctly for clicks and leads. Compare withv3_count.pipecount_sales(lines 495-589) which correctly includes this branch.Additionally, the
saleTypefilter fromJSON(filters)won't be applied here — line 515 only handles the legacy scalarsaleTypeparam.
🤖 Fix all issues with AI agents
In `@apps/web/app/api/analytics/export/route.ts`:
- Around line 25-31: The duplicated extraction of scalar values from filter
objects (domainFilter and folderIdFilter) should be centralized: update
parseAnalyticsQuery (and parseEventsQuery) to return domain and folderId as
strings alongside the existing filter objects, or create a small shared helper
(e.g., extractFilterValue(filter): string | undefined) and replace the repeated
logic in route handlers; ensure the helper/parse function handles plain-string
inputs (not only objects with a values array) so it returns the string value
instead of falling through to undefined, and update references in
apps/web/app/api/analytics/export/route.ts (domainFilter, folderIdFilter) and
the corresponding events/dashboard/admin routes to use the new returned
domain/folderId symbols.
In `@apps/web/lib/analytics/get-events.ts`:
- Around line 131-133: The code passes regionForPipe (which can be a
ParsedFilter object) directly to the pipe; update the spread that builds the
pipe params in get-events.ts to only include region when regionForPipe is a
string—mirroring the existing guards for triggerForPipe and countryForPipe—so
change the region spread to use a typeof check (e.g., typeof regionForPipe ===
"string" ? { region: regionForPipe } : {}) and ensure this logic corresponds to
the prepareFiltersForPipe output and the pipe’s Zod schema expectations.
In `@apps/web/lib/stripe/index.ts`:
- Around line 4-12: The exported stripe variable is now Stripe | null but call
sites (e.g., createPaymentIntent, webhook handlers, billing routes) call stripe
methods directly; add null checks or enforce runtime assertion. Fix by either
(A) guarding every use: check if stripe is null at the start of functions like
createPaymentIntent and return a 500/error response (or throw) with a clear
message when STRIPE_SECRET_KEY is missing before calling
stripe.paymentMethods.list() or other methods, or (B) replace the nullable
export with a runtime assertion/wrapper that throws on import if
process.env.STRIPE_SECRET_KEY is unset so callers can assume non-null; update
documentation/comments accordingly and apply the chosen pattern consistently
across all files that reference the stripe export.
In `@apps/web/lib/zod/schemas/analytics.ts`:
- Around line 184-229: In the os transform (the zod schema block using
parseFilterValue), extend the special-casing logic in the values mapping to
handle macOS like iOS; i.e., for each val check val.toLowerCase() and return
"iOS" when "ios", return "macOS" when "macos", otherwise fall back to
capitalize(val), keeping the existing .filter(Boolean) and cast—this change
should be made in the values: parsed.values.map(...) inside the os transform.
In `@apps/web/ui/analytics/use-analytics-filters.tsx`:
- Around line 930-944: The code can set the literal string "undefined" into
query params when parseFilterParam returns undefined; update the removal logic
in use-analytics-filters.tsx to first guard parsed and newValues: if parsed is
falsy, return early (or call queryParams({ del: key, scroll: false }) if you
want to remove the param) so you never build newParam from undefined, and
additionally ensure newValues is a defined array before checking length and
constructing newParam (references: parseFilterParam, newValues, newParam,
queryParams).
- Around line 987-992: The onRemoveFilter callback currently calls queryParams({
del: key }) which fails when key is "link" because the URL uses "domain" and
"key" (and sometimes "url"); update onRemoveFilter (used where activeFilter.key
can be "link") to mirror the logic in onRemove: when key === "link" call
queryParams({ del: ["domain","key","url"], scroll: false }), otherwise call
queryParams({ del: key, scroll: false }). Ensure you update the onRemoveFilter
function reference so it deletes the correct param names for the "link"
activeFilter.
In `@packages/tinybird/pipes/v3_count.pipe`:
- Around line 28-51: The template currently applies folder_id twice: the legacy
scalar branch (elif defined(folderId) -> AND folder_id = {{ folderId }}) and the
later operator-aware branch (checks folderIdOperator and emits IN/NOT IN), which
creates contradictory clauses when folderIdOperator == 'NOT IN'; remove the
legacy scalar branch that emits "AND folder_id = {{ folderId }}" so the
operator-aware block (using folderId, folderIdOperator and
Array(folderId,'String')) is the single source of truth. Make this same change
in the other affected templates (v3_events.pipe, v3_group_by.pipe,
v3_group_by_link_metadata.pipe) to avoid duplicate/conflicting folder_id
filters.
In `@packages/tinybird/pipes/v3_timeseries.pipe`:
- Around line 101-124: The template emits conflicting folder filters: when
folderId is set the earlier elif branch creates "AND folder_id = {{ folderId }}"
while later logic also emits folder_id IN/NOT IN based on folderIdOperator,
causing contradictions (e.g., folder_id = X AND folder_id NOT IN (X)). Remove
the redundant scalar equality or guard it so it only runs when folderIdOperator
is not defined—i.e., change the block that emits "AND folder_id = {{ folderId
}}" to execute only if folderIdOperator is not defined (check
String(folderIdOperator) or use {% if not defined(folderIdOperator) %}) and keep
the later folder_id IN/NOT IN logic using folderIdOperator, referencing
folderId, folderIds, folderIdOperator and folder_id.
In `@packages/ui/src/filter/filter-list.tsx`:
- Around line 620-632: The forEach callback is implicitly returning the result
of onRemove which triggers a static-analysis warning; update the onClick handler
(the branch that calls values.forEach) to use an explicit block callback so the
intent is clear: replace values.forEach(value => onRemove(filterKey, value))
with a block form that calls onRemove(filterKey, value) inside braces (e.g.,
values.forEach(value => { onRemove(filterKey, value); })), leaving the
onRemoveFilter branch unchanged.
In `@packages/ui/src/filter/types.ts`:
- Around line 71-98: The normalizeActiveFilter function currently treats objects
with an explicit operator key set to a falsy value (e.g., { key, values,
operator: undefined }) as missing operator and falls through to the empty-values
default; update the first branch in normalizeActiveFilter to detect when the
operator key exists (use "'operator' in filter") and values is an array
(Array.isArray(filter.values)), and in that case return an ActiveFilter using
the provided values and a safe operator (use the existing filter.operator if
truthy, otherwise default to 'IS'); reference normalizeActiveFilter,
ActiveFilterInput, and LegacyActiveFilterPlural when making this change so that
objects with operator: undefined preserve their values and get a sensible
operator fallback.
🧹 Nitpick comments (26)
apps/web/scripts/dub-wrapped.ts (1)
64-64: Consider usingparseFilterValue("false")instead of a hardcoded filter object.This literal object is repeated across multiple scripts and the cron utility. Since
parseFilterValuefrom@dub/utilsproduces the exact same structure, using it would be DRYer and less prone to drift if the filter shape evolves.- root: { values: ["false"], operator: "IS", sqlOperator: "IN" }, + root: parseFilterValue("false")!,apps/web/scripts/get-top-links-for-workspace.ts (1)
42-42: Same hardcoded filter object — same refactor opportunity as indub-wrapped.ts.apps/web/scripts/download-top-links.ts (1)
14-14: Same hardcoded filter object — same refactor opportunity as indub-wrapped.ts.apps/web/app/(ee)/api/cron/usage/utils.ts (1)
125-125: Same hardcoded filter object — same refactor opportunity as indub-wrapped.ts.packages/utils/src/functions/parse-filter-value.ts (1)
63-65:buildFilterValuewith emptyvaluesarray would produce"-"or""— consider a guard.If a manually constructed
ParsedFilterwith an emptyvaluesarray is passed,buildFilterValuereturns"-"(for NOT operators) or""(for IS operators), both of which would round-trip incorrectly throughparseFilterValue. SinceparseFilterValuenever produces empty arrays, this is low-risk but could bite future callers.apps/web/lib/analytics/types.ts (1)
16-16: Remove the unusedOverridetype utility.The
Overridetype at line 16 is not used anywhere in the codebase.AnalyticsFiltersno longer references it and instead uses inline type composition. This is dead code that should be deleted.apps/web/tests/analytics/normalize-filter.test.ts (1)
132-143: Consider adding a test foroperator: undefinededge case.The edge cases section tests
{ key: "country" }(no operator, no values) but doesn't cover{ key: "country", values: ["US"], operator: undefined }, which would currently drop the values silently (falls through to the default branch).Suggested test
+ test("handles filter with explicit undefined operator", () => { + const input = { + key: "country", + values: ["US", "BR"], + operator: undefined, + } as any; + const result = normalizeActiveFilter(input); + expect(result).toEqual({ + key: "country", + operator: "IS_ONE_OF", + values: ["US", "BR"], + }); + });packages/tinybird/pipes/v3_timeseries.pipe (2)
177-276: Massive filter-block duplication across nodes — consider extracting a shared include.The same ~100-line filter template is copy-pasted verbatim in
timeseries_clicks_data,timeseries_leads_data, andtimeseries_sales_data(with minorse.prefix differences in sales). This makes maintenance error-prone — any field added or bug fixed must be replicated in all three (or more, given 5 pipes are modified per the PR summary).Tinybird supports
INCLUDEdirectives for shared SQL fragments. Consider extracting the filter block into a shared include to keep the pipes DRY.
297-455: Leads node still has legacy single-value filters alongside new JSON filters — potential double-filtering.
timeseries_leads_dataretains legacy scalar filters (e.g.,AND country = {{ country }}at Line 328) while also applying the new JSONfiltersblock (Lines 354-453). If both the legacycountryparameter and acountryentry in thefiltersJSON are provided simultaneously, both WHERE conditions apply, which is likely redundant but could cause confusion.The clicks node was already migrated to JSON-only filtering. Consider whether the leads and sales nodes should follow suit to avoid this ambiguity, or document clearly that legacy params and JSON filters should not overlap.
apps/web/lib/zod/schemas/analytics.ts (2)
417-432: DRY:parseAnalyticsQueryandparseEventsQueryshare identical backward-compat logic.Both functions perform the same
linkId → linkIdsandtagId → tagIdsconversions. Consider extracting the shared migration into a helper to avoid drift.Suggested refactor
+function applyBackwardCompat(data: Record<string, any>) { + if (data.linkId && !data.linkIds) { + data.linkIds = [data.linkId]; + } + if (data.tagId && !data.tagIds) { + data.tagIds = { operator: "IS" as const, sqlOperator: "IN" as const, values: [data.tagId] }; + } + return data; +} + export function parseAnalyticsQuery(searchParams: unknown) { const data = analyticsQuerySchema.parse(searchParams); - if (data.linkId && !data.linkIds) { - data.linkIds = [data.linkId]; - } - if (data.tagId && !data.tagIds) { - data.tagIds = { operator: "IS" as const, sqlOperator: "IN" as const, values: [data.tagId] }; - } - return data; + return applyBackwardCompat(data); } export function parseEventsQuery(searchParams: unknown) { const data = eventsQuerySchema.parse(searchParams); - if (data.linkId && !data.linkIds) { - data.linkIds = [data.linkId]; - } - if (data.tagId && !data.tagIds) { - data.tagIds = { operator: "IS" as const, sqlOperator: "IN" as const, values: [data.tagId] }; - } - return data; + return applyBackwardCompat(data); }Also applies to: 543-557
434-495:folderIdandfolderIdsserve different purposes but have confusing names.
folderId(line 456) is meant for operator-based filtering with a correspondingfolderIdOperatorfield (line 473), whilefolderIds(line 451) is a simpler multi-value field. Both acceptstring | string[]and transform identically, making the distinction unclear. Rename one or add explicit inline documentation explaining when each should be used—folderIdfor advanced filtering with IN/NOT IN operators,folderIdsfor simple multi-value queries.apps/web/app/api/analytics/route.ts (1)
51-58: Repeated domain/folderId extraction pattern — consider a shared helper.This object-check-and-extract pattern is duplicated in
dashboard/route.ts(and likelyexport/route.ts). A small utility would reduce boilerplate and prevent drift:function extractFirstValue(filter: ParsedFilter | undefined): string | undefined { return filter?.values[0]; }Also, when a multi-value domain filter is provided (e.g.,
domain=dub.co,google.com), onlyvalues[0]is validated viagetDomainOrThrowon line 84. The remaining domains skip the ownership check. This isn't a data-leak risk since analytics are workspace-scoped, but it means invalid domains silently pass validation.apps/web/app/api/analytics/dashboard/route.ts (1)
147-160:parsedParamsnow containsParsedFilterobjects — verify cache key stability and log readability.
JSON.stringify(parsedParams)will serializeParsedFilterobjects (withoperator,sqlOperator,valuesfields) deterministically for identical inputs, so cache correctness is maintained. However, the cache key is now longer/more complex, and the console logs on lines 152-159 will output the full nested structure. Just flagging for awareness.apps/web/tests/analytics/index.test.ts (3)
62-79: Backward compatibility test is identical to the single country filter test.The "backward compatibility" test (lines 212–229) sends
country: "US"withgroupBy: "count"and asserts status 200 +clicksproperty — exactly the same as the "single country filter" test (lines 63–79). It doesn't exercise any distinct legacy format. Consider testing the old single-value format in a way that actually differs (e.g., different parameter shape, deprecated query key, or at least a differentiating comment explaining what "old format" means here).Also applies to: 212-229
193-210: Timeseries test lacks meaningful assertions beyond status code.The comment says "Timeseries should only include US data," but there's no actual assertion validating that. If data comes back containing non-US entries, this test would still pass. Consider validating the timeseries array elements or at least the schema shape (e.g., each entry has a
starttimestamp andclicksfield).
95-101: Empty result sets cause allforEachassertions to pass vacuously.If the test link has no click data matching these filters, every
data.forEach(...)block passes without checking anything. Consider addingexpect(data.length).toBeGreaterThan(0)guards (or at minimum acknowledging this in the test) for the tests that rely on inspecting individual items.Also applies to: 118-124, 140-147, 164-170
apps/web/tests/analytics/advanced-filters.test.ts (1)
274-290: Test name "handles all supported fields" only covers 5 of the supported fields.The PR mentions 11 filter types (country, city, device, browser, os, region, referer, referer_url, continent, trigger, url). Consider either renaming this test to "handles multiple supported fields" or expanding it to cover all entries in
SUPPORTED_FIELDSto match the test name.apps/web/ui/analytics/toggle.tsx (1)
86-86:isAdvancedFilteris unconditionallytrue— consider making it configurable.Both
Filter.SelectandFilter.ListreceiveisAdvancedFilter={true}with no way to disable advanced filtering. If this is the intended final state for all analytics views, this is fine. However, if other consumers ofAnalyticsToggle(e.g., partner pages, dashboards) might not want advanced filter UI, consider making it a prop.Also applies to: 297-297
apps/web/tests/events/index.test.ts (1)
70-188: Advanced filter tests for events are weaker than their analytics counterparts.All advanced filter tests only assert
status === 200andArray.isArray(data). Unlike the analyticsindex.test.tswhich validates returnedcountry/devicevalues, these tests don't verify the filters actually affected the response. At minimum, consider schema-validating the response withclickEventResponseSchemaand, where possible, asserting on the filtered field values.apps/web/lib/analytics/get-analytics.ts (2)
144-171:tinybirdParamstyped asanydefeats type safety in a critical path.This object assembles all parameters sent to the Tinybird analytics pipe. Typing it as
anymeans typos in property names, wrong value types, or missing required fields won't be caught at compile time. Consider usingz.input<typeof analyticsFilterTB>or a dedicated interface to get type checking here.Suggested approach
- const tinybirdParams: any = { + const tinybirdParams: z.input<typeof analyticsFilterTB> = {If
analyticsFilterTBdoesn't perfectly match the runtime shape, a partial/intersection type could bridge the gap while still catching obvious mistakes.
98-109: Multiple non-null assertions ongroupBythroughout the function.
groupBy!is used at lines 99, 107, 343, and 348, andevent!at line 179. IfgetAnalyticscan legitimately be called withoutgroupByorevent, these will throw at runtime. Consider adding an early guard or narrowing the type at the top of the function.Suggested guard
export const getAnalytics = async (params: AnalyticsFilters) => { let { event, groupBy, ... } = params; + + if (!groupBy) throw new Error("groupBy is required"); + if (!event) throw new Error("event is required");packages/tinybird/pipes/v3_count.pipe (1)
119-218: ~100-line filter block is copy-pasted verbatim across nodes and files.The
JSON(filters)template block (lines 119-218) is duplicated identically incount_clicks,count_leads, andcount_saleswithin this file, and again acrossv3_events.pipe,v3_group_by.pipe, andv3_group_by_link_metadata.pipe. This amounts to roughly 10+ copies of the same ~100-line block.While Tinybird's templating language may not support shared includes, consider whether Tinybird's shared data sources/pipes or a code-generation step could reduce this surface. Each future filter type addition requires updating all copies in lockstep, which is error-prone.
apps/web/lib/analytics/get-events.ts (1)
120-146: Consider adding a proper type fortinybirdParamsinstead ofany.Using
anydisables all type checking for the pipe parameters. If a field name is misspelled or a value has the wrong shape (e.g., passing aParsedFilterobject where a string is expected), it will silently pass compilation.apps/web/ui/analytics/use-analytics-filters.tsx (3)
121-123: UnnecessaryuseCallbackwrapper around a pure imported function.
parseFilterValueis a stable module-level import — wrapping it inuseCallbackwith an empty dependency array adds indirection without any memoization benefit. Consider usingparseFilterValuedirectly at call sites.
156-172: Long ternary chain on Line 165 is hard to read and maintain.The chained ternaries to recover destructured variables are error-prone. A lookup map would be clearer and easier to extend.
♻️ Proposed refactor
+ const destructuredParams: Record<string, string | undefined> = { + domain, + tagIds, + root, + folderId, + }; + VALID_ANALYTICS_FILTERS.forEach((filter) => { if (["key", "tagId", "customerId"].includes(filter)) return; if (["interval", "start", "end", "qr"].includes(filter)) return; if (filter === "domain" && domain && key) return; - const value = params[filter] || (filter === "domain" ? domain : filter === "tagIds" ? tagIds : filter === "root" ? root : filter === "folderId" ? folderId : undefined); + const value = params[filter] || destructuredParams[filter]; if (value) {
175-181:partnerPagein the dependency array appears unused inside thisuseMemo.It doesn't seem to be referenced in the
activeFilterscomputation (lines 125–174). This won't cause bugs but triggers unnecessary recalculations if the prop changes.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tinybird/pipes/v3_events.pipe (1)
488-515:⚠️ Potential issue | 🔴 CriticalCritical:
sale_eventsnode is missing field-based JSON filter handling — advanced filters silently ignored for sales.The
sale_eventssubquery (lines 488–509) only processes metadata operand filters. It does not include the{% elif item.get('field', '') %}branch thatclick_events(line 139) andlead_events(line 303) have. This means field-based advanced filters (country IN/NOT IN,city,device,browser,utm_*,domain,tagIds,folderId,root, etc.) are silently dropped for sale event queries.Additionally, line 512 only handles the legacy scalar
saleTypeparam ({% if defined(saleType) %}). There is no JSONfilters-basedsaleTypehandling in the outer WHERE, unlikev3_count.pipe(lines 593–603) andv3_group_by.pipe(lines 755–765).This will return unfiltered sale events when advanced filters are active.
🐛 Proposed fix: Add field-based filter branch + saleType handling
Add the field-based filter branch inside the sale_events subquery (after the metadata
{% end %}at line 507), and add saleType JSON filter handling in the outer WHERE at line 512. The field-based block should mirror what's inclick_events(lines 139–226), adapted withse.column prefixes. The outer WHERE should add:) AS t - WHERE true {% if defined(saleType) %} AND t.sale_type = {{ String(saleType) }} {% end %} + WHERE true + {% if defined(saleType) %} AND t.sale_type = {{ String(saleType) }} {% end %} + {% if defined(filters) %} + {% for item in JSON(filters, '[]') %} + {% if item.get('field', '') == 'saleType' %} + {% set operator = item.get('operator', 'IN') %} + {% set values = item.get('values', []) %} + {% if operator == 'IN' %} AND t.sale_type IN {{ Array(values, 'String') }} + {% elif operator == 'NOT IN' %} AND t.sale_type NOT IN {{ Array(values, 'String') }} + {% end %} + {% end %} + {% end %} + {% end %}And the full field-based filter block (country, city, continent, device, browser, os, trigger, referer, refererUrl, url, utm_*, domain, tagIds, folderId, root, saleType) needs to be added inside the subquery's
{% for item in JSON(filters, '[]') %}loop, after the metadata handling.
🧹 Nitpick comments (2)
packages/tinybird/pipes/v3_events.pipe (1)
463-483: Inconsistent UTM LIKE pattern insale_eventsvs. other nodes in this file.The legacy UTM patterns in
sale_events(lines 464–483) use a different format—e.g.,concat('%utm_source=', ...)without the separated leading'%', 'utm_source=...'pattern used inclick_events(line 103) andlead_events(line 267). These lines weren't modified in this PR, but since the other nodes were updated to useconcat('%', 'utm_source=', ...), consider aligningsale_eventsfor consistency.packages/tinybird/pipes/v3_group_by.pipe (1)
161-277: Extract the duplicated ~110-line filter block into a reusable.inclfile using Tinybird'sINCLUDEdirective.The identical JSON filter template (metadata + 15-field dispatch with IN/NOT IN logic) is copy-pasted 14 times across 5 pipe files: 3 occurrences each in
v3_group_by.pipe(nodesgroup_by_clicks,group_by_leads,group_by_sales), and 11 occurrences spread acrossv3_count.pipe,v3_timeseries.pipe,v3_events.pipe, andv3_group_by_link_metadata.pipe. Tinybird's templating system supports.inclincludes andINCLUDE <path>.incl <variables>directives, making reuse of this block straightforward. With minor variations in table prefixes (se.in v3_count) and field filtering logic (commented out in v3_events and v3_group_by_link_metadata), a single parameterized include would reduce maintenance burden and eliminate divergence risk across these blocks.
…lback return value.
…} falls through to empty-values default.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@apps/web/ui/analytics/use-analytics-filters.tsx`:
- Around line 974-990: onToggleOperator currently updates the operator param but
doesn’t clear pagination, which can leave users on a stale page; modify the
queryParams call inside onToggleOperator (the function referencing
searchParamsObj and queryParams) to remove the page param like onSelect/onRemove
do (include del: "page" alongside set: { [key]: newParam } and keep scroll:
false) so toggling operators resets pagination.
In `@packages/tinybird/pipes/v3_events.pipe`:
- Around line 585-588: The inner subquery currently filters on se.sale_type (the
raw dub_sale_events_mv column) for the saleType branch, but the meaningful
computed sale_type comes from the first_sale_by_link_customer join and is
already filtered by the outer WHERE on t.sale_type; remove the inner
se.sale_type filter (in the saleType branch) or replace it with a harmless no-op
comment like the other no-op cases (domain/tagIds/root) so the inner query does
not reference the wrong column; locate the saleType branch in the template (the
block checking {% elif field == 'saleType' %}) and make this change so only
t.sale_type is used for saleType filtering.
- Around line 130-137: The template uses JSONExtractString for numeric
comparisons in the operator branches ('greaterThan', 'lessThan',
'greaterThanOrEqual', 'lessThanOrEqual') which yields string comparisons; change
these to extract numeric types (e.g., JSONExtractFloat64 or JSONExtractInt) or
cast the extracted value to a numeric type so comparisons are numeric, and
propagate the same change to the equivalent templates in v3_group_by.pipe and
v3_count.pipe to avoid lexicographic comparison bugs.
- Around line 122-138: The template interpolates user-controlled metadataKey and
value via {% set %} and {{ metadataKey }} / {{ value }}, allowing possible SQL
injection; update the code that builds filters (the item.get('operand'),
metadataKey, and value handling) to validate and sanitize inputs: enforce a
whitelist of allowed metadata keys (check the extracted metadataKey against it),
cast/validate value types (numbers/dates/strings) and escape or pass value as a
Tinybird parameter rather than direct interpolation, and remove/normalize any
characters that could break SQL syntax (quotes, parentheses, comments). Also
adjust the template generation logic that emits JSONExtractString(metadata, {{
metadataKey }}) to use only validated keys (or map keys to safe identifiers) and
ensure comparisons use parameter placeholders for value instead of injecting raw
{{ value }}.
In `@packages/ui/src/filter/filter-list.tsx`:
- Around line 41-60: pluralize currently lowercases input and builds plurals,
causing "OS" -> "oss"; update the irregularPlurals map in the pluralize function
to include 'os': 'OS' (i.e., add the key 'os' with value 'OS') so
pluralize("OS", n>1) returns "OS" rather than "oss"; keep using lowerWord as the
lookup key and return the mapped value (preserving the capitalized
abbreviation).
- Around line 545-555: The Command.Item currently has an empty onSelect and
performs selection only in the onPointerUp handler, which prevents keyboard
(Enter) selection; update Command.Item so that onSelect invokes
toggleValue(option.value) (or calls a helper that does) so selection works for
both keyboard and pointer, remove the empty onSelect stub, and keep or simplify
pointer handlers (e.g., still preventDefault in onPointerDown but do not perform
toggle logic there); reference Command.Item's onSelect, onPointerUp, toggleValue
and option.value to locate and move the toggle logic into onSelect.
🧹 Nitpick comments (8)
apps/web/lib/analytics/get-events.ts (3)
120-146: Consider replacinganywith a proper type fortinybirdParams.Using
anyhere bypasses TypeScript's ability to catch mismatches between what's built and what the pipe expects. ARecord<string, unknown>or a dedicated interface would catch typos and mismatches at compile time.
173-176: DefensivetestVariantsnormalization is fine, but mutates the returned object.
transformLinkreturns a fresh object, so mutatingtransformedLinkis safe here. IftransformLinkever caches or reuses objects, this could be a subtle bug. The check!Array.isArray(...)covers cases wheretestVariantsis a non-array truthy value (e.g., an object) — consider whethernullcoercion is the right default vs. filtering it out entirely, depending on the response schema expectation.
147-148: Trailing whitespace on line 147.Line 147 has trailing whitespace after the closing brace/semicolon on line 146. Very minor nit.
apps/web/ui/analytics/use-analytics-filters.tsx (2)
121-123: UnnecessaryuseCallbackwrapper.
parseFilterParamis a pure one-liner with an empty dependency array. This stable identity doesn't requireuseCallback—you can just callparseFilterValuedirectly at each call site, or declare a plainconstoutside the component.
165-165: Long chained ternary is hard to follow.This expression chains 5 ternaries to resolve the value for a filter key. Consider extracting it into a small helper for readability.
Suggested refactor
- const value = params[filter] || (filter === "domain" ? domain : filter === "tagIds" ? tagIds : filter === "root" ? root : filter === "folderId" ? folderId : undefined); + const specialParams: Record<string, string | undefined> = { domain, tagIds, root, folderId }; + const value = params[filter] || specialParams[filter];packages/ui/src/filter/filter-list.tsx (1)
110-125: Option lookup is duplicated betweendisplayLabelandOptionDisplay.Lines 113–124 (for
displayLabel) and Lines 136–142 (insideOptionDisplay) perform the same option-finding logic with identical case-insensitive string matching. Consider extracting a shared helper likefindOption(filter, value)to reduce duplication.packages/ui/src/filter/types.ts (1)
9-9: Consider collapsing to justISandIS_NOToperators.
FilterOperatordefines four variants (IS,IS_NOT,IS_ONE_OF,IS_NOT_ONE_OF), but all the actual behavior is determined by value count, not the operator type:
- UI renders them identically:
IS/IS_ONE_OFboth display as "is",IS_NOT/IS_NOT_ONE_OFboth display as "is not"- SQL layer maps them identically: both
ISvariants becomeIN, bothIS_NOTvariants becomeNOT IN- URL encoding only checks negation (via
-prefix), not the operator variant itselfparseFilterValue()andnormalizeActiveFilter()both assign the variant based solely onvalue.length > 1Since no UI, SQL, or URL logic actually differentiates between the paired variants, maintaining four operators adds complexity without functional benefit. Consider reducing to just
ISandIS_NOT, with the value array's length implying multiplicity.packages/tinybird/pipes/v3_events.pipe (1)
120-226: ~270 lines of filter template logic duplicated across three nodes.The JSON field-based filter block is nearly identical across
click_events,lead_events, andsale_events, differing only in table alias prefixes and a couple of field inclusions. Tinybird supportsINCLUDEdirectives for shared template fragments — extracting a reusable macro would significantly reduce maintenance burden and divergence risk (e.g., thetriggerfield is inclick_eventsbut missing from the other two nodes, andsaleTypeis handled inconsistently).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tinybird/pipes/v3_events.pipe (1)
488-509:⚠️ Potential issue | 🔴 Critical
sale_eventsis missing the entire field-based JSON filter branch.Both
click_events(lines 139–223) andlead_events(lines 303–383) handle{% elif item.get('field', '') %}inside the{% for item in JSON(filters, '[]') %}loop, applying IN/NOT IN filters for country, city, device, browser, os, UTMs, etc. Thesale_eventsnode only processesmetadata.*operands and silently drops all field-based filters.This means advanced filters like "country IS NOT US" will have no effect on sale events — the conditions are never emitted into the SQL.
Additionally,
saleTypefield-based filters need special handling:sale_typeis a computed column from thefirst_sale_by_link_customerjoin, so it must be filtered in the outer WHERE (line 512), not inside the subquery.Proposed fix — add field-based branch inside the subquery and saleType in the outer query
Add the field-based filter block after line 506 (inside the subquery, after the metadata block's
{% end %}), mirroring the pattern fromclick_events/lead_eventsbut withse.column prefixes, and excludingsaleType(handled separately):{% elif operator == 'lessThanOrEqual' %} AND JSONExtractString(se.metadata, {{ metadataKey }}) <= {{ value }} {% end %} + {% elif item.get('field', '') %} + {% set field = item.get('field', '') %} + {% set operator = item.get('operator', 'IN') %} + {% set values = item.get('values', []) %} + {% if field == 'country' %} + {% if operator == 'IN' %} AND se.country IN {{ Array(values, 'String') }} + {% elif operator == 'NOT IN' %} AND se.country NOT IN {{ Array(values, 'String') }} + {% end %} + {% elif field == 'city' %} + {% if operator == 'IN' %} AND se.city IN {{ Array(values, 'String') }} + {% elif operator == 'NOT IN' %} AND se.city NOT IN {{ Array(values, 'String') }} + {% end %} + {% elif field == 'continent' %} + {% if operator == 'IN' %} AND se.continent IN {{ Array(values, 'String') }} + {% elif operator == 'NOT IN' %} AND se.continent NOT IN {{ Array(values, 'String') }} + {% end %} + {% elif field == 'device' %} + {% if operator == 'IN' %} AND se.device IN {{ Array(values, 'String') }} + {% elif operator == 'NOT IN' %} AND se.device NOT IN {{ Array(values, 'String') }} + {% end %} + {% elif field == 'browser' %} + {% if operator == 'IN' %} AND se.browser IN {{ Array(values, 'String') }} + {% elif operator == 'NOT IN' %} AND se.browser NOT IN {{ Array(values, 'String') }} + {% end %} + {% elif field == 'os' %} + {% if operator == 'IN' %} AND se.os IN {{ Array(values, 'String') }} + {% elif operator == 'NOT IN' %} AND se.os NOT IN {{ Array(values, 'String') }} + {% end %} + {% elif field == 'referer' %} + {% if operator == 'IN' %} AND se.referer IN {{ Array(values, 'String') }} + {% elif operator == 'NOT IN' %} AND se.referer NOT IN {{ Array(values, 'String') }} + {% end %} + {% elif field == 'refererUrl' %} + {% if operator == 'IN' %} AND splitByString('?', se.referer_url)[1] IN {{ Array(values, 'String') }} + {% elif operator == 'NOT IN' %} AND splitByString('?', se.referer_url)[1] NOT IN {{ Array(values, 'String') }} + {% end %} + {% elif field == 'url' %} + {% if operator == 'IN' %} AND splitByString('?', se.url)[1] IN {{ Array(values, 'String') }} + {% elif operator == 'NOT IN' %} AND splitByString('?', se.url)[1] NOT IN {{ Array(values, 'String') }} + {% end %} + {% elif field == 'utm_source' %} + {% if operator == 'IN' %} + AND (0{% for val in values %} OR se.url LIKE concat('%', 'utm_source=', encodeURLFormComponent({{ String(val) }}), '%'){% end %}) + {% elif operator == 'NOT IN' %} + AND NOT (0{% for val in values %} OR se.url LIKE concat('%', 'utm_source=', encodeURLFormComponent({{ String(val) }}), '%'){% end %}) + {% end %} + {% elif field == 'utm_medium' %} + {% if operator == 'IN' %} + AND (0{% for val in values %} OR se.url LIKE concat('%', 'utm_medium=', encodeURLFormComponent({{ String(val) }}), '%'){% end %}) + {% elif operator == 'NOT IN' %} + AND NOT (0{% for val in values %} OR se.url LIKE concat('%', 'utm_medium=', encodeURLFormComponent({{ String(val) }}), '%'){% end %}) + {% end %} + {% elif field == 'utm_campaign' %} + {% if operator == 'IN' %} + AND (0{% for val in values %} OR se.url LIKE concat('%', 'utm_campaign=', encodeURLFormComponent({{ String(val) }}), '%'){% end %}) + {% elif operator == 'NOT IN' %} + AND NOT (0{% for val in values %} OR se.url LIKE concat('%', 'utm_campaign=', encodeURLFormComponent({{ String(val) }}), '%'){% end %}) + {% end %} + {% elif field == 'utm_term' %} + {% if operator == 'IN' %} + AND (0{% for val in values %} OR se.url LIKE concat('%', 'utm_term=', encodeURLFormComponent({{ String(val) }}), '%'){% end %}) + {% elif operator == 'NOT IN' %} + AND NOT (0{% for val in values %} OR se.url LIKE concat('%', 'utm_term=', encodeURLFormComponent({{ String(val) }}), '%'){% end %}) + {% end %} + {% elif field == 'utm_content' %} + {% if operator == 'IN' %} + AND (0{% for val in values %} OR se.url LIKE concat('%', 'utm_content=', encodeURLFormComponent({{ String(val) }}), '%'){% end %}) + {% elif operator == 'NOT IN' %} + AND NOT (0{% for val in values %} OR se.url LIKE concat('%', 'utm_content=', encodeURLFormComponent({{ String(val) }}), '%'){% end %}) + {% end %} + {% elif field == 'domain' %} + {# domain is already filtered at workspace_links node level #} + {% elif field == 'tagIds' %} + {# tagIds is already filtered at workspace_links node level #} + {% elif field == 'folderId' %} + {# folderId is already filtered at workspace_links node level #} + {% elif field == 'root' %} + {# root is already filtered at workspace_links node level #} + {% elif field == 'saleType' %} + {# saleType is filtered in the outer query where the computed sale_type is available #} {% end %} {% end %} {% end %} {% end %} ) AS t - WHERE true {% if defined(saleType) %} AND t.sale_type = {{ String(saleType) }} {% end %} + WHERE true + {% if defined(saleType) %} AND t.sale_type = {{ String(saleType) }} {% end %} + {% if defined(filters) %} + {% for item in JSON(filters, '[]') %} + {% if item.get('field', '') == 'saleType' %} + {% set operator = item.get('operator', 'IN') %} + {% set values = item.get('values', []) %} + {% if operator == 'IN' %} AND t.sale_type IN {{ Array(values, 'String') }} + {% elif operator == 'NOT IN' %} AND t.sale_type NOT IN {{ Array(values, 'String') }} + {% end %} + {% end %} + {% end %} + {% end %}#!/bin/bash # Verify that click_events and lead_events have the field-based branch but sale_events does not echo "=== click_events field-based branch ===" rg -n "item.get\('field'" packages/tinybird/pipes/v3_events.pipe echo "" echo "=== sale_events filters section (lines 488-509) ===" sed -n '488,515p' packages/tinybird/pipes/v3_events.pipe
🧹 Nitpick comments (1)
packages/tinybird/pipes/v3_events.pipe (1)
120-226: ~170 lines of filter logic are duplicated across event nodes.The field-based and metadata-based JSON filter blocks in
click_eventsandlead_eventsare nearly identical, and the same block needs to be added tosale_events. This triples the maintenance surface for any filter change.If Tinybird supports shared include files or macros, consider extracting the filter template into a reusable fragment. If not, a code-generation approach (e.g., a script that produces the pipe file from a single template) would help prevent the kind of drift that caused the
sale_eventsomission above.#!/bin/bash # Check if Tinybird supports include files or shared templates rg -rn "INCLUDE\|include\|import\|macro" packages/tinybird/ --type-add 'pipe:*.pipe' --type pipe | head -20Also applies to: 284-386
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)
packages/tinybird/pipes/v3_group_by.pipe (1)
645-652:⚠️ Potential issue | 🔴 CriticalMetadata numeric comparisons in
group_by_salesare missingtoFloat64OrNull— lexicographic instead of numeric.The
group_by_clicks(Line 172) andgroup_by_leads(Line 391) nodes correctly wrap metadata comparisons withtoFloat64OrNull(JSONExtractString(...)), butgroup_by_salesstill uses bareJSONExtractString(...)forgreaterThan,lessThan,greaterThanOrEqual, andlessThanOrEqual. This causes string-based comparison where'10' < '9'is true, producing incorrect results.🐛 Proposed fix
{% elif operator == 'greaterThan' %} - AND JSONExtractString(se.metadata, {{ metadataKey }}) > {{ value }} + AND toFloat64OrNull(JSONExtractString(se.metadata, {{ metadataKey }})) > toFloat64OrNull({{ value }}) {% elif operator == 'lessThan' %} - AND JSONExtractString(se.metadata, {{ metadataKey }}) < {{ value }} + AND toFloat64OrNull(JSONExtractString(se.metadata, {{ metadataKey }})) < toFloat64OrNull({{ value }}) {% elif operator == 'greaterThanOrEqual' %} - AND JSONExtractString(se.metadata, {{ metadataKey }}) >= {{ value }} + AND toFloat64OrNull(JSONExtractString(se.metadata, {{ metadataKey }})) >= toFloat64OrNull({{ value }}) {% elif operator == 'lessThanOrEqual' %} - AND JSONExtractString(se.metadata, {{ metadataKey }}) <= {{ value }} + AND toFloat64OrNull(JSONExtractString(se.metadata, {{ metadataKey }})) <= toFloat64OrNull({{ value }})packages/tinybird/pipes/v3_count.pipe (1)
483-490:⚠️ Potential issue | 🔴 CriticalSame missing
toFloat64OrNullissue incount_salesmetadata comparisons.Same bug as
group_by_salesin v3_group_by.pipe: the numeric operators (greaterThan,lessThan, etc.) use bareJSONExtractStringcomparisons, producing lexicographic results instead of numeric. Thecount_leadsnode (Line 277) in this same file does it correctly.🐛 Proposed fix
{% elif operator == 'greaterThan' %} - AND JSONExtractString(se.metadata, {{ metadataKey }}) > {{ value }} + AND toFloat64OrNull(JSONExtractString(se.metadata, {{ metadataKey }})) > toFloat64OrNull({{ value }}) {% elif operator == 'lessThan' %} - AND JSONExtractString(se.metadata, {{ metadataKey }}) < {{ value }} + AND toFloat64OrNull(JSONExtractString(se.metadata, {{ metadataKey }})) < toFloat64OrNull({{ value }}) {% elif operator == 'greaterThanOrEqual' %} - AND JSONExtractString(se.metadata, {{ metadataKey }}) >= {{ value }} + AND toFloat64OrNull(JSONExtractString(se.metadata, {{ metadataKey }})) >= toFloat64OrNull({{ value }}) {% elif operator == 'lessThanOrEqual' %} - AND JSONExtractString(se.metadata, {{ metadataKey }}) <= {{ value }} + AND toFloat64OrNull(JSONExtractString(se.metadata, {{ metadataKey }})) <= toFloat64OrNull({{ value }})
🤖 Fix all issues with AI agents
In `@apps/web/ui/analytics/use-analytics-filters.tsx`:
- Around line 156-172: The root filter options currently use boolean values
while parseFilterValue/parseFilterParam returns strings, causing mismatched
comparisons; update the root filter options to use string values
("true"/"false") instead of booleans (or alternatively coerce parsed values to
booleans) so values.includes(option.value) can match. Locate the root filter
options (the array that defines option objects for the "root" filter) and change
option.value from true/false to "true"/"false" (or add a normalize step after
parseFilterParam to convert parsed.values items to booleans when filter ===
"root"); ensure parseFilterParam/parseFilterValue behavior and the filters push
(inside the loop over VALID_ANALYTICS_FILTERS using parseFilterParam and
filters.push) remain consistent with the chosen string/boolean normalization.
In `@packages/tinybird/pipes/v3_events.pipe`:
- Around line 213-222: v3_events.pipe currently skips JSON filters for
domain/tagIds/folderId/root (relying on extractWorkspaceLinkFilters() and
separate params), which is inconsistent with v3_count.pipe and v3_group_by.pipe;
update v3_events.pipe to detect these fields in JSON filters and emit the same
inline subqueries (link_id IN (SELECT link_id FROM workspace_links WHERE ...))
as v3_count/v3_group_by so behavior matches, referencing the same filter
structure used in those pipes and ensuring compatibility with
buildAdvancedFilters() and SUPPORTED_FIELDS (keep domainParam/tagIdsParam
handling intact but add JSON-path handling to avoid silent drops if backend
changes).
🧹 Nitpick comments (4)
packages/ui/src/filter/filter-list.tsx (2)
360-386:displayValuesprop is declared but unused insideOperatorFilterPill.The
displayValuesprop (line 379) is accepted but never read within the component body — it's only used via theOptionDisplayclosure defined in the parent. You can remove it from the props interface.Proposed fix
function OperatorFilterPill({ filterKey, filter, values, operator, displayLabel, - displayValues, OptionDisplay, onRemove, ...And at the call site (line 235):
<OperatorFilterPill key={key} filterKey={key} filter={filter} values={values} operator={operator} displayLabel={displayLabel} - displayValues={displayValues} OptionDisplay={OptionDisplay}
264-358: Remove unusedFilterComboboxfunction (lines 264–358).This function is neither called anywhere in the codebase nor exported. It appears to have been superseded by the
Command-based dropdown implementation insideOperatorFilterPill. Removing it will reduce maintenance surface.apps/web/ui/analytics/use-analytics-filters.tsx (2)
121-123: UnnecessaryuseCallbackwrapper around a pure function.
parseFilterValueis a stable import with no dependencies — wrapping it inuseCallback([], [])adds indirection without benefit. You could useparseFilterValuedirectly and dropparseFilterParamfrom dependency arrays.
165-165: Long ternary chain is hard to read.This line packs multiple fallbacks into a single expression. A small lookup or early-continue pattern would be clearer.
Suggested refactor
- const value = params[filter] || (filter === "domain" ? domain : filter === "tagIds" ? tagIds : filter === "root" ? root : filter === "folderId" ? folderId : undefined); + const specialParams: Record<string, string | undefined> = { + domain, + tagIds, + root, + folderId, + }; + const value = params[filter] || specialParams[filter];
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
pepeladeira
merged commit c6ae269
into
dubinc:feat/advanced-filters
3 participants
Adds support for advanced filter operators in analytics:
IS,IS NOT.Changes
Frontend:
?country=US,BRor?country=-USfor negationBackend:
@dub/utilsfor frontend/backend consistencyTinybird:
Summary by CodeRabbit
New Features
Improvements
Tests
Chores