SCAL-321380: Add converter from EmbedEvent.FilterChanged payload to HostEvent.UpdateFilters - #586
SCAL-321380: Add converter from EmbedEvent.FilterChanged payload to HostEvent.UpdateFilters#586sastaachar wants to merge 8 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a utility function convertFilterChangedToUpdateFiltersPayload along with comprehensive unit tests and type exports to convert the payload of EmbedEvent.FilterChanged into the format expected by HostEvent.UpdateFilters. It also updates the UpdateFilters contract to support optional properties and broader value types. The feedback recommends using loose inequality checks (!= null) instead of strict inequality checks (!== undefined) when validating optional properties like epoch, number, and includeCurrentPeriod to safely handle potential null values in the incoming JSON payloads.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const DATE_FILTER_VALUE_EXTRACTORS: Record<string, DateFilterValueExtractor> = { | ||
| EXACT_DATE: (dateFilter) => ( | ||
| dateFilter.epoch !== undefined ? [Number(dateFilter.epoch)] : [] | ||
| ), | ||
| EXACT_DATE_RANGE: (dateFilter) => { | ||
| const { lowEpoch, highEpoch } = dateFilter.dateRange ?? {}; | ||
| return lowEpoch !== undefined && highEpoch !== undefined | ||
| ? [Number(lowEpoch), Number(highEpoch)] | ||
| : []; | ||
| }, | ||
| MONTH_YEAR: (dateFilter) => ( | ||
| dateFilter.monthName && dateFilter.yearName | ||
| ? [dateFilter.monthName, dateFilter.yearName] | ||
| : [] | ||
| ), | ||
| QUARTER_YEAR: (dateFilter) => ( | ||
| dateFilter.quarterName && dateFilter.yearName | ||
| ? [dateFilter.quarterName, dateFilter.yearName] | ||
| : [] | ||
| ), | ||
| YEAR_ONLY: (dateFilter) => (dateFilter.yearName ? [dateFilter.yearName] : []), | ||
| LAST_N_PERIOD: (dateFilter) => ( | ||
| dateFilter.number !== undefined ? [dateFilter.number] : [] | ||
| ), | ||
| NEXT_N_PERIOD: (dateFilter) => ( | ||
| dateFilter.number !== undefined ? [dateFilter.number] : [] | ||
| ), | ||
| }; |
There was a problem hiding this comment.
Using !== undefined to check for optional properties can lead to bugs if the incoming payload contains null values (which is common in JSON payloads from APIs for unset fields). For example, if epoch is null, dateFilter.epoch !== undefined evaluates to true, and Number(null) evaluates to 0, resulting in an incorrect epoch value of 0 (1970-01-01) instead of ignoring the filter. Using != null is a safer and more robust way to check that a value is neither null nor undefined.
const DATE_FILTER_VALUE_EXTRACTORS: Record<string, DateFilterValueExtractor> = {
EXACT_DATE: (dateFilter) => (
dateFilter.epoch != null ? [Number(dateFilter.epoch)] : []
),
EXACT_DATE_RANGE: (dateFilter) => {
const { lowEpoch, highEpoch } = dateFilter.dateRange ?? {};
return lowEpoch != null && highEpoch != null
? [Number(lowEpoch), Number(highEpoch)]
: [];
},
MONTH_YEAR: (dateFilter) => (
dateFilter.monthName && dateFilter.yearName
? [dateFilter.monthName, dateFilter.yearName]
: []
),
QUARTER_YEAR: (dateFilter) => (
dateFilter.quarterName && dateFilter.yearName
? [dateFilter.quarterName, dateFilter.yearName]
: []
),
YEAR_ONLY: (dateFilter) => (dateFilter.yearName ? [dateFilter.yearName] : []),
LAST_N_PERIOD: (dateFilter) => (
dateFilter.number != null ? [dateFilter.number] : []
),
NEXT_N_PERIOD: (dateFilter) => (
dateFilter.number != null ? [dateFilter.number] : []
),
};| if (dateFilter.includeCurrentPeriod !== undefined) { | ||
| param.includeCurrentPeriod = dateFilter.includeCurrentPeriod; | ||
| } |
There was a problem hiding this comment.
Similarly, use != null to protect against null values in the payload, which would otherwise be assigned to param.includeCurrentPeriod and violate the expected boolean | undefined type.
| if (dateFilter.includeCurrentPeriod !== undefined) { | |
| param.includeCurrentPeriod = dateFilter.includeCurrentPeriod; | |
| } | |
| if (dateFilter.includeCurrentPeriod != null) { | |
| param.includeCurrentPeriod = dateFilter.includeCurrentPeriod; | |
| } |
…filters Address Fable review of #586: - Convert every filterContent/dateFilterContent entry per column instead of only the first, so compound conditions on a single column are no longer silently dropped. - Skip (rather than emit with empty values) date filters missing a required field (e.g. EXACT_DATE without epoch) or with an unrecognized type, so a partial payload can't clear/corrupt a filter on re-apply. - Export the nested Liveboard filter types for consumer typing. - Document the columnName/operator aliases and converter in the HostEvent.UpdateFilters JSDoc. - Add tests for the above plus isValidUpdateFiltersPayload integration. SCAL-321380
Fable reviewRan a review pass with the Fable model. Findings and resolution:
Added 7 new test cases covering the fixes (multiple conditions per column, multiple filters per group, falsy values like 🤖 Review performed with Claude Code using the Fable model. |
FilterChanged returns liveboard filter state in a different shape than HostEvent.UpdateFilters expects, forcing consumers to hand-parse and reformat the payload (especially for date filters) to reapply it later. Adds convertFilterChangedToUpdateFiltersPayload() to bridge the two formats, and widens the UpdateFilters request type to match the fields the host app actually accepts (columnName/operator aliases, datePeriod, negate, includeCurrentPeriod). SCAL-321380
…filters Address Fable review of #586: - Convert every filterContent/dateFilterContent entry per column instead of only the first, so compound conditions on a single column are no longer silently dropped. - Skip (rather than emit with empty values) date filters missing a required field (e.g. EXACT_DATE without epoch) or with an unrecognized type, so a partial payload can't clear/corrupt a filter on re-apply. - Export the nested Liveboard filter types for consumer typing. - Document the columnName/operator aliases and converter in the HostEvent.UpdateFilters JSDoc. - Add tests for the above plus isValidUpdateFiltersPayload integration. SCAL-321380
The FilterChanged payload arrives as JSON from the embedded app, so an absent field can be null rather than omitted. The converter checked only for undefined, so `epoch: null` coerced to `Number(null)` === 0 and produced a valid-looking filter pinned to the Unix epoch; `number: null` likewise produced a zero-length period. Non-numeric strings had the same problem via NaN. Route epoch/count fields through a toFiniteNumber() helper that rejects null, empty string and non-finite values, and skip the filter when a required field is missing, matching the existing behaviour for absent fields. Genuine zeroes (epoch 0, includeCurrentPeriod false) are kept. Also rename the converter's payload interfaces to a FilterChanged* family: main has since added its own LiveboardFilter to the host event contracts, and both were re-exported from the package root. Addresses the review feedback on PR #586. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5cb4d7e to
4a673b8
Compare
Rebased onto current main, plus the outstanding review fixThis branch was 104 commits behind main, so I've rebased it. Three things 1.
|
sastaachar
changed the title
commit: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const runtimeFilterParams: UpdateFiltersFilterParam[] = ( | ||
| filterChangedPayload?.runtimeFilters ?? [] | ||
| ).map((runtimeFilter) => ({ | ||
| columnName: runtimeFilter.columnName, | ||
| operator: runtimeFilter.operator, | ||
| values: runtimeFilter.values, | ||
| })); |
There was a problem hiding this comment.
Correctness (plausible): the runtime-filter branch copies columnName/operator/values straight through with no null-safety, unlike every other path in this file.
The whole point of 4a673b8b (this same PR) was that the FilterChanged payload is JSON from the embedded app, so an absent field can arrive as null rather than being omitted — and that a naive pass-through silently produces a corrupt-but-valid-looking filter. That fix was applied to liveboardFilters/dateFilter fields (toFiniteNumber, the isNil checks), but runtimeFilters here gets none of it. If the host ever emits a runtimeFilters entry with columnName: null (same JSON-round-trip mechanism as the rest of the payload), it flows straight into the output filters array. isValidUpdateFiltersPayload's hasColumn check (typeof f.columnName === 'string') would then fail for that one entry, and since it checks payload.filters.every(isValidFilter), the entire trigger(HostEvent.UpdateFilters, ...) call throws — not just the bad entry.
RuntimeFilter's fields are typed as required, so this may be a non-issue if the host always populates them fully for this event (unlike the liveboard-filter payload). But there's no test covering a null/missing field here, whereas the equivalent liveboard-filter and date-filter cases are extensively tested (see the null and non-numeric values in the payload describe block). Worth confirming with the host payload's actual contract, or applying the same defensive filtering used elsewhere.
Review verdict: CleanNo correctness, security, or public-API-breaking issues found in the diff. What was reviewed:
Style guideNo documentation issues found in the changed lines. |
…through The runtimeFilters branch copied columnName/operator/values straight through while every other path in the converter skips entries it can't faithfully reconstruct. That asymmetry has a blast radius: isValidUpdateFiltersPayload rejects the entire filters array if any one entry is missing a column, operator or values array, so a single malformed runtime filter would make trigger(HostEvent.UpdateFilters) throw and take every correctly converted filter down with it. Skip such entries, matching the rest of the file. A runtime filter with an empty values array is still valid and is kept. Raised by the automated review on PR #586. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Good catch on the It was a real asymmetry: every other path in the converter skips entries it
9 new tests, including the one that matters: a bad runtime filter alongside good |
| export interface FilterChangedFilterGroup { | ||
| columnInfo?: { | ||
| name?: string; | ||
| }; | ||
| filters?: FilterChangedFilter[]; |
There was a problem hiding this comment.
Correctness: applicability is dropped during conversion, silently changing where a re-applied filter scopes to.
src/types.ts's docs for EmbedEvent.FilterChanged (~line 3386) and HostEvent.GetFilters (~line 5728) both state that a Liveboard filter entry carries an optional applicability ({ level: 'LIVEBOARD' | 'TAB' | 'GROUP', targetId? }) since SDK 1.53.0. HostEvent.UpdateFilters's FilterUpdate (contracts.ts) already accepts applicability on each filter.
FilterChangedFilterGroup/FilterChangedFilter here have no applicability field, and neither convertFilterGroupToParams (line 227) nor convertFilterToParams (line 200) read or propagate one onto UpdateFiltersFilterParam.
Concrete failure: a filter scoped to a specific tab (applicability: { level: 'TAB', targetId: 'abc' }) is captured via convertFilterChangedToUpdateFiltersPayload(payload) and later replayed with liveboardEmbed.trigger(HostEvent.UpdateFilters, savedFilters). The scoping is silently lost — the filter re-applies at the Liveboard level instead of the original tab, with no error to indicate the state wasn't faithfully restored. That directly undermines the stated purpose of this utility ("capture and re-apply later").
| /** | ||
| * Name of the column to filter on. `columnName` is an accepted alias, so a | ||
| * payload produced by `convertFilterChangedToUpdateFiltersPayload` can be | ||
| * passed straight through. | ||
| */ | ||
| column?: string; | ||
| columnName?: string; | ||
| /** | ||
| * Operator to apply. `operator` is an accepted alias. | ||
| */ | ||
| oper?: string; | ||
| operator?: string; |
There was a problem hiding this comment.
Doc comments here are published to developers.thoughtspot.com, and TypeDoc attaches a leading comment only to the declaration immediately below it — so columnName and operator (the new alias properties) will render with no description at all, while column/oper get one. Suggest giving the aliases their own (even one-line) doc comments.
| /** | |
| * Name of the column to filter on. `columnName` is an accepted alias, so a | |
| * payload produced by `convertFilterChangedToUpdateFiltersPayload` can be | |
| * passed straight through. | |
| */ | |
| column?: string; | |
| columnName?: string; | |
| /** | |
| * Operator to apply. `operator` is an accepted alias. | |
| */ | |
| oper?: string; | |
| operator?: string; | |
| /** | |
| * Name of the column to filter on. `columnName` is an accepted alias, so a | |
| * payload produced by `convertFilterChangedToUpdateFiltersPayload` can be | |
| * passed straight through. | |
| */ | |
| column?: string; | |
| /** | |
| * Alias for `column`. | |
| */ | |
| columnName?: string; | |
| /** | |
| * Operator to apply. `operator` is an accepted alias. | |
| */ | |
| oper?: string; | |
| /** | |
| * Alias for `oper`. | |
| */ | |
| operator?: string; |
| * } | ||
| * }); | ||
| * ``` | ||
| * `columnName` and `operator` are also accepted as aliases for `column` |
There was a problem hiding this comment.
Tag-ordering (style guide rule 1): this new paragraph about the columnName/operator aliases and convertFilterChangedToUpdateFiltersPayload is inserted after all the @example blocks but before @version. Per the canonical order, free-text description belongs with the short description at the top of the comment, not sandwiched between @example and @version. Consider moving it up to the main description instead.
liveboardFilters entries carry an optional applicability ({ level,
targetId }) scoping a filter to a LIVEBOARD, TAB or GROUP, added in SDK
1.53.0 / 26.10.0.cl, and HostEvent.UpdateFilters accepts the same field
per filter. The converter dropped it, so a filter scoped to one tab was
replayed with no scope at all - which UpdateFilters treats as LIVEBOARD
level. Restoring saved filter state would silently widen a tab filter
across the whole Liveboard and change what every other tab shows.
Carry the group's applicability onto every param it produces. When a
scope is present but unusable - unknown level, or TAB/GROUP without a
targetId - skip the filter instead: widening is the worse failure, and
isValidUpdateFiltersPayload would reject the entire filters array over
one malformed entry, taking every other filter with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
isValidUpdateFiltersPayload accepts columnName/operator as aliases for column/oper, but handleUpdateFiltersEvent forwards the payload to the embedded app verbatim and only column/oper are known to be understood there. A payload using the aliases therefore passed validation and was then silently ignored downstream - the worst of both, since it looked accepted. Canonicalise each filter to column/oper after validation, so either spelling works end to end. The canonical spelling wins when both are present. Also narrow the FilterUpdate contract to what is actually known to be supported: columnName/operator are declared and column/oper marked @deprecated in favour of them, matching RuntimeFilter and the FilterChanged payload, and values widens to (string | number | boolean | bigint)[] to match RuntimeFilter.values. datePeriod, negate and includeCurrentPeriod are deliberately NOT declared - the converter emits them, but nothing verifies the embedded app honours them, and declaring a field in a public contract asserts support we do not have evidence for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ten public exports for one function was too much surface for a published package. FilterChangedFilterGroup, FilterChangedFilter, FilterChangedFilterContent, FilterChangedFilterContentValue, FilterChangedDateFilterContent and FilterChangedDateFilterValue describe the internal structure of a payload callers receive and pass straight through - naming them is not something a consumer needs to do. They stay exported from the module for internal use and tests, just not re-exported from the package root. What remains public is the function plus the three types a caller actually touches: FilterChangedPayload in, UpdateFiltersPayload out, and UpdateFiltersFilterParam for iterating it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Canonicalise" was jargon for a plain operation, and British spelling in a codebase that uses American. The names now say what happens: canonicaliseFilter -> resolveFilterAliases canonicaliseUpdateFiltersPayload -> resolveUpdateFiltersAliases Doc comments reworded to match. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
EmbedEvent.FilterChangedreturns Liveboard filter state in a shape that's incompatible with whatHostEvent.UpdateFiltersexpects, forcing consumers to hand-parse and reformat the payload (especially tricky for date filters) in order to reapply it later.convertFilterChangedToUpdateFiltersPayload(), a converter utility that bridges the two payload formats, covering liveboard filters (attribute/measure + all date filter types:EXACT_DATE,EXACT_DATE_RANGE,MONTH_YEAR,QUARTER_YEAR,YEAR_ONLY,LAST_N_PERIOD/NEXT_N_PERIOD, period-only types) and runtime filters.UpdateFiltersrequest type incontracts.tsto includecolumnName/operatoraliases,datePeriod,negate, andincludeCurrentPeriod, matching what the host app already accepts but wasn't reflected in the SDK's types.Fixes SCAL-321380.
Test plan
tsc --noEmitpasses with no errorseslintpasses on all new/changed filesjest -c jest.config.sdk.js src/utils/filterConverter.spec.ts— 17 new tests passing (~96% line coverage)jest -c jest.config.sdk.js src/embed/hostEventClient— no regressions (75 existing tests passing)🤖 Generated with Claude Code