feat(dashboard): add Rison-encoded URL filter support#39795
Conversation
Adds ?f=(...) URL parameter support for hydrating dashboard filters from human-readable Rison syntax. Matches URL filters to native filters by column name (case-insensitive, handles spaces) and auto-applies them by populating both filterState.value and extraFormData. Unmatched filters render as removable chips in a "URL Filters" section above cross-filters. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
dosubot
Bot
added
change:frontend
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39795 +/- ##
==========================================
- Coverage 64.36% 63.78% -0.59%
==========================================
Files 2569 2589 +20
Lines 134626 136992 +2366
Branches 31227 31587 +360
==========================================
+ Hits 86656 87375 +719
- Misses 46471 48101 +1630
- Partials 1499 1516 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const activeUrlFilters = useMemo(() => getUrlFilterIndicators(), []); | ||
|
|
||
| const handleRemoveUrlFilter = useCallback( | ||
| (filterToRemove: UrlFilterIndicator) => { | ||
| const risonParam = getRisonFilterParam(); | ||
| if (!risonParam) return; | ||
|
|
||
| const currentFilters = parseRisonFilters(risonParam); | ||
| const remaining = currentFilters.filter( | ||
| f => f.subject !== filterToRemove.filter.subject, | ||
| ); | ||
| updateUrlWithUnmatchedFilters(remaining); | ||
| }, | ||
| [], |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
In the horizontal filter bar, URL filters are read once into a memoized value with an empty dependency array and not stored in mutable state, so clicking a URL filter chip's close icon only mutates the URL and does not update the rendered list of chips.
Suggestion: Track URL filter indicators in React state (or derive them from a location/search source that updates on URL changes) and update that state on chip removal so the horizontal chip list reflects user actions immediately.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx
**Line:** 130:143
**Comment:**
*HIGH: In the horizontal filter bar, URL filters are read once into a memoized value with an empty dependency array and not stored in mutable state, so clicking a URL filter chip's close icon only mutates the URL and does not update the rendered list of chips.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| @@ -94,9 +127,47 @@ const HorizontalFilterBar: FC<HorizontalBarProps> = ({ | |||
| [chartIds, chartLayoutItems, dataMask, verboseMaps], | |||
| ); | |||
|
|
|||
| const activeUrlFilters = useMemo(() => getUrlFilterIndicators(), []); | |||
There was a problem hiding this comment.
Suggestion: URL filter indicators are memoized with an empty dependency array, so they never refresh after URL updates (including chip removal). This leaves stale chips visible and keeps filter-bar state incorrect until a full remount. Recompute from URL changes or keep local state that updates on remove. [possible bug]
Severity Level: Major ⚠️
- ❌ Horizontal URL chips do not disappear when closed.
- ⚠️ Filter bar reports filters present after URL cleared.Steps of Reproduction ✅
1. Load any dashboard page that uses the native filter bar (`FilterBar` at
`superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:105-117`)
with `orientation={FilterBarOrientation.Horizontal}`, which causes it to render the
`Horizontal` component from `./Horizontal` (`index.tsx:52-55`), i.e. `HorizontalFilterBar`
in `Horizontal.tsx:101-207`.
2. Open the dashboard URL with an unmatched Rison filter so that there will be URL-filter
chips, for example `?f=(unknown_col:foo)`. `DashboardPage`
(`superset-frontend/src/dashboard/containers/DashboardPage.tsx:200-61`) reads `f` via
`getRisonFilterParam` and `parseRisonFilters`, and unmatched filters are exposed via
`getUrlFilterIndicators` (`UrlFilters/selectors.ts:47-59`).
3. Observe that `HorizontalFilterBar` reads the URL filters once into `activeUrlFilters`
using `useMemo(() => getUrlFilterIndicators(), [])` at `Horizontal.tsx:130`, and renders
chips via `<UrlFilterTag ... />` inside `urlFiltersComponent` (`Horizontal.tsx:146-165`).
4. Click the close icon on a URL filter chip in the horizontal bar: `UrlFilterTag` calls
its `onRemove` prop (`UrlFilters/UrlFilterTag.tsx:71-72`), which invokes
`handleRemoveUrlFilter` (`Horizontal.tsx:132-142`) to update the URL via
`updateUrlWithUnmatchedFilters`, but this handler does not update any React state and
`activeUrlFilters` is memoized with an empty dependency list, so `HorizontalFilterBar`
does not recompute the indicators; the chip remains visible and `hasFilters`
(`Horizontal.tsx:167-171`) still treats URL filters as present until the component is
remounted.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx
**Line:** 130:130
**Comment:**
*Possible Bug: URL filter indicators are memoized with an empty dependency array, so they never refresh after URL updates (including chip removal). This leaves stale chips visible and keeps filter-bar state incorrect until a full remount. Recompute from URL changes or keep local state that updates on remove.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const remaining = currentFilters.filter( | ||
| f => f.subject !== filterToRemove.filter.subject, | ||
| ); |
There was a problem hiding this comment.
Suggestion: Removing a chip currently filters by subject only, so if multiple URL filters target the same column, closing one removes all of them. Match on full filter identity (subject + operator + comparator) to remove only the selected chip. [logic error]
Severity Level: Major ⚠️
- ❌ OR URL filters all removed when one chip dismissed.
- ⚠️ Users cannot selectively remove same-column URL conditions.Steps of Reproduction ✅
1. Open a dashboard as rendered by `DashboardPage`
(`superset-frontend/src/dashboard/containers/DashboardPage.tsx:200-61`) and ensure the
native filter bar is horizontal so it uses `HorizontalFilterBar`
(`FilterBar/index.tsx:52-55` -> `Horizontal.tsx:101-207`).
2. Navigate to the dashboard with an OR Rison filter that produces multiple conditions on
the same subject, for example `?f=(OR:!((country:USA),(country:Canada)))`.
`parseRisonFilters` (`dashboard/util/risonFilters.ts:42-88`) parses this into two
`RisonFilter` objects, both with `subject: 'country'` and different `comparator` values.
3. `getUrlFilterIndicators` (`UrlFilters/selectors.ts:47-59`) converts these into two
`UrlFilterIndicator` entries, both having `subject: 'country'` but different `value`
strings; `HorizontalFilterBar` renders two chips via `activeUrlFilters.map(filter =>
<UrlFilterTag ... />)` in `Horizontal.tsx:155-161`.
4. Click the close icon on just one of the `country` URL filter chips in the horizontal
bar: `UrlFilterTag` forwards to `handleRemoveUrlFilter` (`Horizontal.tsx:132-142`), which
re-parses the current URL (`parseRisonFilters`) and computes `remaining =
currentFilters.filter(f => f.subject !== filterToRemove.filter.subject)` at
`Horizontal.tsx:138-140`; because both filters share the same `subject`, `remaining`
becomes empty and `updateUrlWithUnmatchedFilters(remaining)` removes all `country` URL
filters from the URL in one action, rather than only the selected chip.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx
**Line:** 138:140
**Comment:**
*Logic Error: Removing a chip currently filters by `subject` only, so if multiple URL filters target the same column, closing one removes all of them. Match on full filter identity (subject + operator + comparator) to remove only the selected chip.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| </UrlFilterTitle> | ||
| {activeUrlFilters.map(filter => ( | ||
| <UrlFilterTag | ||
| key={filter.subject} |
There was a problem hiding this comment.
Suggestion: Using only subject as the React key causes key collisions when multiple URL filters share the same column name, which can produce incorrect chip rendering/removal behavior. Use a unique key that includes operator/comparator (or a stable hash). [possible bug]
Severity Level: Major ⚠️
- ⚠️ React receives duplicate keys for multiple URL filter chips.
- ⚠️ Vertical URL chips may render or update inconsistently.Steps of Reproduction ✅
1. Open a dashboard with the native filter bar visible; for horizontal orientation,
`FilterBar` renders `HorizontalFilterBar` (`FilterBar/index.tsx:52-55`,
`Horizontal.tsx:101-207`), and for vertical orientation, URL filters are rendered via
`UrlFiltersVertical` (`UrlFilters/Vertical.tsx:24-31`) and `UrlFiltersVerticalCollapse`
(`UrlFilters/VerticalCollapse.tsx:33-41`).
2. Hit the dashboard with a Rison OR filter that creates multiple URL conditions on the
same subject, e.g. `?f=(OR:!((country:USA),(country:Canada)))`. `parseRisonFilters`
(`dashboard/util/risonFilters.ts:42-88`) returns multiple `RisonFilter` objects with
identical `subject` values, and `getUrlFilterIndicators` (`UrlFilters/selectors.ts:47-59`)
turns each into a `UrlFilterIndicator` with the same `subject` but different `value`.
3. In the horizontal bar, `urlFiltersComponent` maps these indicators to `<UrlFilterTag>`
components using `key={filter.subject}` (`Horizontal.tsx:155-161`), and the vertical
collapse does the same in its `filterIndicators` memo
(`UrlFilters/VerticalCollapse.tsx:130-138`). Since multiple list items share the same
`subject`, React receives duplicate keys for different children.
4. With duplicate keys present, React's reconciliation can mix up items when URL filters
change (for example after removal in `UrlFilters/VerticalCollapse.handleRemoveFilter` at
`VerticalCollapse.tsx:46-59`, which updates `urlFilters` state), leading to console key
warnings and potential mismatches where the remaining chip DOM is reused incorrectly; this
is particularly observable in the vertical URL filter section, which actually updates
local `urlFilters` state and relies on React correctly reconciling the chip list.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx
**Line:** 157:157
**Comment:**
*Possible Bug: Using only `subject` as the React key causes key collisions when multiple URL filters share the same column name, which can produce incorrect chip rendering/removal behavior. Use a unique key that includes operator/comparator (or a stable hash).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let searchString = newParams.toString(); | ||
| if (risonFilterValue) { | ||
| const separator = searchString ? '&' : ''; | ||
| searchString = `${searchString}${separator}f=${risonFilterValue}`; | ||
| } |
There was a problem hiding this comment.
Suggestion: The f parameter is appended as raw text to the query string, bypassing URL encoding. If the filter value contains reserved characters (like &), the URL becomes malformed and can split into unintended query params. Preserve raw display separately but encode the value when constructing search. [logic error]
Severity Level: Major ⚠️
- ❌ Rison `f` URL param corrupted when value contains `&`.
- ⚠️ Dashboard URL filters mis-parse on reload for such values.
- ⚠️ Shared dashboard links can lose parts of filter state.Steps of Reproduction ✅
1. Open a dashboard route `/superset/dashboard/<id>/` with a Rison filter whose comparator
contains an encoded `&`, for example: `?f=(company:'AT%26T')`. This hits `DashboardPage`
in `superset-frontend/src/dashboard/containers/DashboardPage.tsx` and mounts `FilterBar`
from `superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx`.
2. After the dashboard loads, change any native filter so that `dataMaskApplied` updates.
`FilterBar`'s `useEffect` at `index.tsx:28-34` detects the change and calls
`publishDataMask(history, dashboardId, updateKey, dataMaskApplied, tabId)` defined at
`index.tsx:18-25`.
3. Inside `publishDataMask` (`index.tsx:26-43`), the code builds `previousParams = new
URLSearchParams(search)` from `history.location.search`. In the `previousParams.forEach`
loop at `index.tsx:33-41`, when `key === 'f'` it sets `risonFilterValue = value`. For the
example URL, `URLSearchParams` decodes `%26` to `&`, so `risonFilterValue` becomes
`(company:'AT&T')`.
4. Later in `publishDataMask` at `index.tsx:79-84`, the code reconstructs the search
string: `let searchString = newParams.toString();` then appends the raw Rison value with
``searchString = `${searchString}${separator}f=${risonFilterValue}`;`` (lines 159-163 in
the PR hunk). This produces a query like `...?native_filters_key=…&f=(company:'AT&T')`,
where the `&` inside the value is now an unencoded delimiter. On a subsequent load,
`getRisonFilterParam()` in `superset-frontend/src/dashboard/util/risonFilters.ts:23-25`
parses `window.location.search` with `new URLSearchParams`, which truncates `f` at
`(company:'AT` and treats `T')` as a separate param. As a result, `parseRisonFilters()`
(used in `DashboardPage.tsx:18-23` and `UrlFilters/selectors.ts:14-19`) receives a
corrupted Rison string and the URL filter no longer represents the original value.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx
**Line:** 159:163
**Comment:**
*Logic Error: The `f` parameter is appended as raw text to the query string, bypassing URL encoding. If the filter value contains reserved characters (like `&`), the URL becomes malformed and can split into unintended query params. Preserve raw display separately but encode the value when constructing `search`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const risonDataMask = { | ||
| __rison_filters__: { | ||
| filterState: { value: unmatchedAdhocFilters }, | ||
| ownState: {}, | ||
| }, | ||
| }; | ||
|
|
||
| dataMask = { ...dataMask, ...risonDataMask }; |
There was a problem hiding this comment.
Suggestion: The fallback path for unmatched URL filters stores them only in filterState.value, but dashboard query building reads filters from extraFormData. As a result, unmatched filters are displayed but never actually applied to chart queries. Populate extraFormData.filters for __rison_filters__ (and keep filterState only for UI) so fallback filters execute. [logic error]
Severity Level: Major ⚠️
- ❌ Rison URL filters on unknown columns never filter charts.
- ⚠️ Dashboard URL filtering inconsistent with documented unmatched-filter behavior.Steps of Reproduction ✅
1. Open a dashboard through `DashboardPage`
(`superset-frontend/src/dashboard/containers/DashboardPage.tsx`) with an unmatched Rison
URL filter, e.g. `?f=(unknown_col:foo)`. This triggers `getDataMaskApplied` in the
`useEffect` starting around line 31, which parses the URL filter via
`getRisonFilterParam()` and `parseRisonFilters()` (lines 58–62).
2. Because `unknown_col` does not match any native filter,
`injectRisonFiltersIntelligently()` returns it in `unmatchedFilters`. The fallback block
at lines 80–94 runs: `unmatchedAdhocFilters = risonToAdhocFilters(unmatchedFilters)` is
created, and `risonDataMask` is built at lines 235–241 with `__rison_filters__` containing
only `filterState: { value: unmatchedAdhocFilters }` and `ownState: {}`, but **no
`extraFormData` field**.
3. Still in `getDataMaskApplied`, this `risonDataMask` object is merged into `dataMask`
(line 242) and passed to `hydrateDashboard` in the dispatch at lines 117–125. The dataMask
reducer (`superset-frontend/src/dataMask/reducer.ts`, case `HYDRATE_DASHBOARD` around
lines 199–231) uses this `dataMask` to initialize Redux `state.dataMask`, so
`__rison_filters__` exists in state with only `filterState.value`, no `extraFormData`.
4. When a chart builds its query with dashboard context, `getFormDataWithExtraFilters`
(`superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts`) is used
(see import/use in `src/pages/Chart/index.tsx` lines 36 and 110–116). Inside
`getFormDataWithExtraFilters`, helper `createFilterDataMapping` (lines 104–117) calls
`getExtraFormData(dataMask, [filterId])` and **only inspects `extraFormData.filters`**
from each `DataMask` entry to populate query-level filters. Since `__rison_filters__` was
created in `DashboardPage.tsx` with no `extraFormData`, `getExtraFormData` returns no
filters for it, so the unmatched Rison filters never reach the chart query payload and are
not applied to any chart.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/containers/DashboardPage.tsx
**Line:** 235:242
**Comment:**
*Logic Error: The fallback path for unmatched URL filters stores them only in `filterState.value`, but dashboard query building reads filters from `extraFormData`. As a result, unmatched filters are displayed but never actually applied to chart queries. Populate `extraFormData.filters` for `__rison_filters__` (and keep `filterState` only for UI) so fallback filters execute.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if (parsedObj.OR && Array.isArray(parsedObj.OR)) { | ||
| (parsedObj.OR as Record<string, unknown>[]).forEach(condition => { | ||
| if (typeof condition === 'object') { | ||
| Object.entries(condition).forEach(([key, value]) => { | ||
| filters.push(parseFilterCondition(key, value)); | ||
| }); | ||
| } | ||
| }); | ||
| return filters; | ||
| } |
There was a problem hiding this comment.
Suggestion: OR expressions are flattened into multiple simple filters, which changes OR semantics into AND behavior downstream. This silently alters query meaning. Preserve OR as a grouped expression (for example SQL expression or explicit boolean grouping) instead of emitting independent filters. [logic error]
Severity Level: Major ⚠️
- ❌ OR URL filters execute as AND, narrowing results.
- ⚠️ Shared dashboard links with OR filters misrepresent intended data.Steps of Reproduction ✅
1. Open any dashboard in the frontend (entry point
`superset-frontend/src/dashboard/containers/DashboardPage.tsx:208-261`) with a URL
containing an OR Rison filter, e.g. `?f=(OR:!((status:active),(priority:high)))` (syntax
validated by backend tests in
`superset/tests/unit_tests/utils/test_rison_filters.py:76-84`).
2. In `DashboardPage.tsx:208-224`, `getRisonFilterParam()` and `parseRisonFilters()` are
called; `parseRisonFilters` in
`superset-frontend/src/dashboard/util/risonFilters.ts:42-63` sees `parsedObj.OR` and, in
the OR block at lines 54-63, flattens the OR list into multiple `RisonFilter` entries via
`filters.push(parseFilterCondition(key, value))` without preserving any grouping
information.
3. The resulting flat `RisonFilter[]` is passed to `injectRisonFiltersIntelligently`
(`risonFilters.ts:50-91`) and, for unmatched filters, to `risonToAdhocFilters` at
`risonFilters.ts:171-183`, which converts each into a separate `QueryObjectFilterClause`
with `expressionType: 'SIMPLE'` and `clause: 'WHERE'`, later stored in the
`__rison_filters__` data mask in `DashboardPage.tsx:30-43`.
4. On the backend, multiple adhoc filters are treated as ANDed conditions (see
`test_multiple_filters_and` in
`superset/tests/unit_tests/utils/test_rison_filters.py:35-44`), whereas a true OR is
represented as a single SQL expression built by `_handle_or_operator` in
`superset/utils/rison_filters.py:157-177`. Because the frontend flattened OR into multiple
simple filters, the effective condition becomes `status = 'active' AND priority = 'high'`
instead of `status = 'active' OR priority = 'high'`, silently changing query semantics for
URL-based OR filters.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/util/risonFilters.ts
**Line:** 54:63
**Comment:**
*Logic Error: OR expressions are flattened into multiple simple filters, which changes OR semantics into AND behavior downstream. This silently alters query meaning. Preserve OR as a grouped expression (for example SQL expression or explicit boolean grouping) instead of emitting independent filters.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const operatorMap: Record<string, string> = { | ||
| '>': 'gt', | ||
| '>=': 'gte', | ||
| '<': 'lt', | ||
| '<=': 'lte', | ||
| BETWEEN: 'between', | ||
| LIKE: 'like', | ||
| }; | ||
|
|
||
| const risonOp = operatorMap[filter.operator] || filter.operator; | ||
| risonObject[filter.subject] = { [risonOp]: filter.comparator }; | ||
| } |
There was a problem hiding this comment.
Suggestion: Serialization does not map != and NOT IN, so negated filters are encoded with raw operator tokens that do not round-trip through the parser. After URL updates/removals, NOT filters can be corrupted or lost. Add explicit operator mapping for negation operators. [logic error]
Severity Level: Critical 🚨
- ❌ Unmatched NOT URL filters vanish on reload or after cleanup.
- ⚠️ Shared links with NOT filters show inconsistent, confusing behavior.Steps of Reproduction ✅
1. Create a dashboard URL that combines a native-matchable filter and an unmatched NOT
filter, for example `?f=(country:USA,NOT:(region:!(\"North America\",\"Europe\")))`, where
`country` has a native filter configured but `region` does not (frontend entrypoint
`DashboardPage.tsx:208-224`, backend `RisonFilterParser` tests for NOT in
`superset/tests/unit_tests/utils/test_rison_filters.py:56-73`).
2. On initial load, `DashboardPage.tsx:208-224` calls `parseRisonFilters()`. In
`risonFilters.ts:65-79`, the NOT branch maps `region` to a `RisonFilter` with `operator:
'NOT IN'` (or `'!='` for scalar) by mutating the output of `parseFilterCondition`. The
unmatched NOT filter is included in `injectionResult.unmatchedFilters` and converted to
adhoc format by `risonToAdhocFilters` at `risonFilters.ts:171-183`, so the first request
correctly sends a `NOT IN` or `!=` condition to the backend.
3. Because `country` was matched to a native filter while `region` was not,
`DashboardPage.tsx:46-55` calls
`updateUrlWithUnmatchedFilters(injectionResult.unmatchedFilters)`.
`updateUrlWithUnmatchedFilters` uses `risonFiltersToString` (`risonFilters.ts:250-277`),
whose `operatorMap` at lines 266-273 does not map `'!='` or `'NOT IN'`, so for the NOT
filter it falls into the `else` branch and emits an object like `{ 'NOT IN': [...] }` or
`{ '!=': 'USA' }` under `region` in the Rison string instead of the original `NOT:(...)`
form.
4. On a subsequent reload or when the UI builds URL filter chips, `getUrlFilterIndicators`
in
`superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/selectors.ts:8-20`
calls `parseRisonFilters` on the mutated `f` parameter. For the NOT filter,
`parseFilterCondition` in `risonFilters.ts:98-148` receives `value = { 'NOT IN': [...] }`
or `{'!=': 'USA'}`; since `'NOT IN'` and `'!='` are not recognized operator keys, it takes
the default branch and produces a filter with `operator: '=='` and `comparator` equal to
the whole operator object. Downstream, the backend parser
(`superset/utils/rison_filters.py:134-155`) likewise does not recognize these raw operator
tokens, so `_parse_operator_dict` returns `None` and `_create_filter` bails out, causing
the NOT filter to be effectively lost or corrupted when encoded, parsed again, or removed
via the URL Filters UI.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/util/risonFilters.ts
**Line:** 266:277
**Comment:**
*Logic Error: Serialization does not map `!=` and `NOT IN`, so negated filters are encoded with raw operator tokens that do not round-trip through the parser. After URL updates/removals, NOT filters can be corrupted or lost. Add explicit operator mapping for negation operators.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| import UrlFiltersVerticalCollapse from './VerticalCollapse'; | ||
|
|
||
| const UrlFiltersVertical = () => { | ||
| const urlFilters = useMemo(() => getUrlFilterIndicators(), []); |
There was a problem hiding this comment.
Suggestion: urlFilters is memoized with an empty dependency array, so it is computed only once and never updates when ?f= changes. This causes stale chips (for example after the dashboard cleanup rewrites URL filters asynchronously, or after other URL updates) and the UI can keep showing filters that are no longer in the URL. Recompute from current location/search params instead of freezing the value at mount. [logic error]
Severity Level: Major ⚠️
- ⚠️ Vertical filter bar shows filters removed from URL.
- ⚠️ URL Filters section misrepresents which URL filters remain.
- ⚠️ Users may see chips for already-matched native filters.Steps of Reproduction ✅
1. Open a dashboard via `DashboardPage`
(`superset-frontend/src/dashboard/containers/DashboardPage.tsx`) with a URL like
`?f=(country:USA,msrp:(between:!(35,200)))`, ensuring there is a native filter on
`country` as described in the PR.
2. On initial render, `VerticalFilterBar`
(`superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx:260-58`)
mounts and renders `<UrlFiltersVertical />` at line 46, which calls `UrlFiltersVertical`
(`UrlFilters/Vertical.tsx:24-32`). Inside `UrlFiltersVertical`, `useMemo(() =>
getUrlFilterIndicators(), [])` at line 25 calls `getUrlFilterIndicators`
(`UrlFilters/selectors.ts:47-59`), which reads the current `f` value via
`getRisonFilterParam()` (`dashboard/util/risonFilters.ts:242-245`) and produces indicators
for *all* Rison filters currently in the URL.
3. After the first paint, `DashboardPage` runs `getDataMaskApplied` in a `useEffect`
(`DashboardPage.tsx:28-81` in the excerpt), which parses the same Rison filters with
`parseRisonFilters` and injects matching ones into native filters. It then computes
`matchedCount` and, if any filters matched, schedules
`updateUrlWithUnmatchedFilters(injectionResult.unmatchedFilters)` via `setTimeout` at
lines 248-253, which updates `window.location` using `updateUrlWithUnmatchedFilters`
(`dashboard/util/risonFilters.ts:291-304`) and `window.history.replaceState`.
4. When the URL is rewritten (removing matched filters from `?f=` or trimming it to only
unmatched filters), React components that computed URL filters via `useMemo(() =>
getUrlFilterIndicators(), [])` do *not* re-run this computation because their dependency
arrays are empty and no props/state changed. As a result, `UrlFiltersVertical` continues
to pass the stale `initialFilters` array to `UrlFiltersVerticalCollapse`
(`UrlFilters/VerticalCollapse.tsx:33-40`), so the "URL Filters" section in the vertical
filter bar still shows chips for filters that have been removed from the `f` parameter
and/or successfully injected into native filters, demonstrating the stale-chip behavior
caused by freezing `urlFilters` at mount.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/Vertical.tsx
**Line:** 25:25
**Comment:**
*Logic Error: `urlFilters` is memoized with an empty dependency array, so it is computed only once and never updates when `?f=` changes. This causes stale chips (for example after the dashboard cleanup rewrites URL filters asynchronously, or after other URL updates) and the UI can keep showing filters that are no longer in the URL. Recompute from current location/search params instead of freezing the value at mount.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const currentFilters = parseRisonFilters(risonParam); | ||
| const remaining = currentFilters.filter( | ||
| f => f.subject !== filterToRemove.filter.subject, | ||
| ); | ||
|
|
||
| updateUrlWithUnmatchedFilters(remaining); |
There was a problem hiding this comment.
Suggestion: This removal path reparses the URL into flattened filters and rewrites it via updateUrlWithUnmatchedFilters, which does not preserve original logical grouping like OR/NOT. As a result, removing one chip can silently change semantics of the remaining URL filter expression. Preserve the original AST/structure when deleting a single condition instead of round-tripping through flattened filters. [logic error]
Severity Level: Critical 🚨
- ❌ NOT URL filters can corrupt remaining condition semantics.
- ⚠️ URL filter chips may display nonsensical "[object Object]" values.Steps of Reproduction ✅
1. Open a dashboard with this PR and navigate to it using a URL that contains a NOT
expression with multiple fields, for example `?f=(NOT:(status:active,priority:high))`.
`parseRisonFilters` at `superset-frontend/src/dashboard/util/risonFilters.ts:42-88`
detects the `NOT` branch at lines 65-78, iterates each inner key/value, and for each one
calls `parseFilterCondition` (lines 98-166) and then changes its operator from `==`/`IN`
to `!=` or `NOT IN` (lines 70-74), producing two `RisonFilter` objects: `status !=
'active'` and `priority != 'high'`.
2. `getUrlFilterIndicators` at `UrlFilters/selectors.ts:28-40` maps those `RisonFilter`
objects to `UrlFilterIndicator`s, and both the horizontal filter bar
(`Horizontal.tsx:130-165`) and vertical URL filter section (`UrlFilters/Vertical.tsx:5-12`
and `UrlFilters/VerticalCollapse.tsx:33-41`) display two URL filter chips via
`UrlFilterTag` (`UrlFilters/UrlFilterTag.tsx:14-42`).
3. In the vertical URL filter section, click the close icon on just the `status` chip.
`UrlFilterTag` calls `onRemove(filter)` (`UrlFilterTag.tsx:32-34`), which invokes
`handleRemoveFilter` in `UrlFilters/VerticalCollapse.tsx:46-60`. That function re-parses
the current `f` value via `parseRisonFilters` (line 51), filters out any `RisonFilter`
whose `subject` matches `filterToRemove.filter.subject` (lines 52-54), leaving a single
remaining filter `priority != 'high'`, and then calls
`updateUrlWithUnmatchedFilters(remaining)` at line 56.
4. `updateUrlWithUnmatchedFilters` in `dashboard/util/risonFilters.ts:32-47` uses
`risonFiltersToString` (lines 248-260) to encode `remaining` back to Rison. Because the
remaining filter has operator `!=`, the encoder writes it as `(priority:(!=:high))` (line
16). On the next parse (for example after a page reload or another removal),
`parseRisonFilters` calls `parseFilterCondition` for value `{ '!=': 'high' }`. Since
`'!='` is not one of the handled operators in the `switch` at lines 105-141, the default
branch returns `operator: '=='` and `comparator: value` (the whole object), causing
`getUrlFilterIndicators` to format the comparator as `String(comparator)` at
selectors.ts:14-25, which renders `"[object Object]"` in the chip. The remaining filter's
semantics have now changed from `priority != 'high'` to a broken equality comparison,
illustrating that round-tripping through the flattened `RisonFilter[]` representation and
`updateUrlWithUnmatchedFilters` does not preserve the original NOT semantics when a single
chip is removed.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/VerticalCollapse.tsx
**Line:** 51:56
**Comment:**
*Logic Error: This removal path reparses the URL into flattened filters and rewrites it via `updateUrlWithUnmatchedFilters`, which does not preserve original logical grouping like `OR`/`NOT`. As a result, removing one chip can silently change semantics of the remaining URL filter expression. Preserve the original AST/structure when deleting a single condition instead of round-tripping through flattened filters.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| <UrlFilterTag | ||
| key={filter.subject} | ||
| filter={filter} | ||
| orientation={FilterBarOrientation.Vertical} | ||
| onRemove={handleRemoveFilter} | ||
| /> |
There was a problem hiding this comment.
Suggestion: Using only subject as the React key is not unique when multiple URL filters target the same column (for example OR conditions on the same field). Duplicate keys cause React reconciliation issues, leading to incorrect tag reuse/removal behavior. Use a key that uniquely identifies each filter instance (e.g., include operator/value or a stable index). [possible bug]
Severity Level: Major ⚠️
- ⚠️ Horizontal URL filter bar shows React duplicate-key warnings.
- ⚠️ Vertical URL filter chips misidentify repeated same-column filters.Steps of Reproduction ✅
1. Start Superset with this PR and open any dashboard so that `DashboardPage` runs its
data-mask hydration effect at
`superset-frontend/src/dashboard/containers/DashboardPage.tsx:200-61`.
2. Navigate to the dashboard with a URL like `?f=(OR:!((country:USA),(country:Canada)))`,
which is valid Rison; `parseRisonFilters` at
`superset-frontend/src/dashboard/util/risonFilters.ts:42-88` sees the `OR` key (lines
53-62) and flattens it into two `RisonFilter` objects, both with `subject === 'country'`
and different `comparator` values.
3. `getUrlFilterIndicators` at
`superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/selectors.ts:28-40`
converts these `RisonFilter` objects into two `UrlFilterIndicator` entries with the same
`subject` field; `UrlFiltersVertical` at `UrlFilters/Vertical.tsx:5-12` and
`HorizontalFilterBar` at `Horizontal.tsx:130-165` read these indicators to render URL
filter chips.
4. In the vertical filter bar, `UrlFiltersVerticalCollapse` at
`UrlFilters/VerticalCollapse.tsx:130-139` maps `urlFilters` to `<UrlFilterTag
key={filter.subject} ... />`, so both "country" filters render with the same React key
(`"country"`). This triggers React's "Encountered two children with the same key" warning
in the console and risks incorrect child reuse if the list ever changes, because React
cannot distinguish between the two chips that share the same `subject`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/VerticalCollapse.tsx
**Line:** 133:138
**Comment:**
*Possible Bug: Using only `subject` as the React key is not unique when multiple URL filters target the same column (for example OR conditions on the same field). Duplicate keys cause React reconciliation issues, leading to incorrect tag reuse/removal behavior. Use a key that uniquely identifies each filter instance (e.g., include operator/value or a stable index).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| if negate and operator == "==": | ||
| operator = "!=" | ||
| elif negate and operator == "IN": | ||
| operator = "NOT IN" | ||
| filter_dict["operator"] = operator | ||
| filter_dict["comparator"] = comparator |
There was a problem hiding this comment.
Suggestion: NOT only inverts == and IN, so NOT on other operators (like >, <, LIKE, BETWEEN) is emitted unchanged and produces the opposite of expected logic. Add full operator inversion mapping (e.g., >→<=, LIKE→NOT LIKE, BETWEEN→NOT BETWEEN) when negate=True. [logic error]
Severity Level: Major ⚠️
- ❌ NOT with range/LIKE yields un-negated conditions.
- ⚠️ Dashboards show opposite results for NOT range filters.Steps of Reproduction ✅
1. Add a test similar to `test_not_operator` in
`superset/tests/unit_tests/utils/test_rison_filters.py:56-64`, but using a comparison:
`result = RisonFilterParser().parse("(NOT:(sales:(gt:100000)))")`.
2. `parse()` at `superset/utils/rison_filters.py:59-77` Rison-decodes this as `{"NOT":
{"sales": {"gt": 100000}}}` and passes it to `_convert_to_adhoc_filters()` at lines
84-102.
3. `_convert_to_adhoc_filters()` sees key `"NOT"` and calls `_handle_not_operator()` at
`superset/utils/rison_filters.py:203-211`, which for column `"sales"` and value `{"gt":
100000}` calls `_create_filter("sales", {"gt": 100000}, negate=True)` at lines 104-132.
4. `_parse_operator_dict()` at `superset/utils/rison_filters.py:134-149` returns `(">",
100000)`, but `_create_filter()` only inverts `"=="` and `"IN"` (lines 120-123). Because
`"operator" == ">"`, the negation block is skipped and `filter_dict["operator"]` remains
`">"` at lines 124-125, so the resulting adhoc filter expresses `sales > 100000` instead
of the logical negation (which should be `<= 100000` or equivalent). Any future use of NOT
with `>`, `<`, `LIKE`, `BETWEEN`, etc. via this parser will silently produce incorrect
filter logic.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/rison_filters.py
**Line:** 120:125
**Comment:**
*Logic Error: `NOT` only inverts `==` and `IN`, so `NOT` on other operators (like `>`, `<`, `LIKE`, `BETWEEN`) is emitted unchanged and produces the opposite of expected logic. Add full operator inversion mapping (e.g., `>`→`<=`, `LIKE`→`NOT LIKE`, `BETWEEN`→`NOT BETWEEN`) when `negate=True`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| return [ | ||
| { | ||
| "expressionType": "SQL", | ||
| "clause": "WHERE", | ||
| "sqlExpression": f"({' OR '.join(sql_parts)})", | ||
| } | ||
| ] |
There was a problem hiding this comment.
Suggestion: The OR path builds a raw SQL filter string from URL input and stores it as expressionType: "SQL", which bypasses structured column/operator handling. Because both column names and values come directly from the request, this allows user-controlled SQL fragments to be injected into the WHERE clause. Build OR conditions as structured SIMPLE filters (or validated SQLAlchemy expressions) instead of concatenating SQL strings. [security]
Severity Level: Critical 🚨
- ❌ Potential SQL injection via future URL `f` filters.
- ⚠️ Arbitrary WHERE clauses from untrusted request parameters.Steps of Reproduction ✅
1. In a Python shell or test, instantiate `RisonFilterParser` from
`superset/utils/rison_filters.py:33` and call `parse("(OR:!((status:active),(priority:'a'
OR 1=1)))")` or similar crafted Rison string.
2. `parse()` at `superset/utils/rison_filters.py:59-77` calls `prison.loads()` on the
user-controlled string and then `_convert_to_adhoc_filters()`, which sees the `"OR"` key
and calls `_handle_or_operator()` at `superset/utils/rison_filters.py:157-180`.
3. `_handle_or_operator()` iterates user-controlled dicts, calls `_build_sql_condition()`
at `superset/utils/rison_filters.py:182-201`, and appends the returned SQL fragments
(built from user-supplied column names and values) into `sql_parts` (lines 161-169).
4. When `sql_parts` is non-empty, `_handle_or_operator()` returns a single adhoc filter
with `expressionType: "SQL"` and `sqlExpression: f"({' OR '.join(sql_parts)})"` at
`superset/utils/rison_filters.py:172-176`. If this result is added to
`form_data["adhoc_filters"]` and processed via `split_adhoc_filters_into_base_filters()`
in `superset/utils/core.py:1390-38`, the raw `sqlExpression` is passed to
`sanitize_clause()` in `superset/sql/parse.py:1548-17` and then embedded directly into the
WHERE clause, allowing arbitrary boolean SQL from the URL to affect query results. Current
code doesn't restrict where `RisonFilterParser.parse()` may be used, so any future
integration that merges its output into `adhoc_filters` will expose this behavior.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/rison_filters.py
**Line:** 172:178
**Comment:**
*Security: The OR path builds a raw SQL filter string from URL input and stores it as `expressionType: "SQL"`, which bypasses structured column/operator handling. Because both column names and values come directly from the request, this allows user-controlled SQL fragments to be injected into the WHERE clause. Build OR conditions as structured SIMPLE filters (or validated SQLAlchemy expressions) instead of concatenating SQL strings.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| values_str = ", ".join( | ||
| [f"'{v}'" if isinstance(v, str) else str(v) for v in value] | ||
| ) | ||
| return f"{column} IN ({values_str})" |
There was a problem hiding this comment.
Suggestion: String values are wrapped in single quotes without escaping embedded quotes, so legitimate values like O'Reilly produce invalid SQL and break OR filter parsing/execution. Use proper SQL literal escaping or parameterized expression construction instead of manual quoting. [possible bug]
Severity Level: Major ⚠️
- ❌ OR URL filters with apostrophes break query execution.
- ⚠️ Common names like O'Reilly cause dashboard filter failures.Steps of Reproduction ✅
1. In a Python test, instantiate `RisonFilterParser` and call its internal
`_build_sql_condition()` directly with a list value containing an apostrophe, e.g.
`_build_sql_condition("author", ["O'Reilly"])` in
`superset/utils/rison_filters.py:182-201`.
2. `_build_sql_condition()` detects `value` as a list and builds `values_str` at lines
184-186 by naïvely wrapping each string in single quotes: `", ".join([f"'{v}'" for v in
value])`, producing `"('O'Reilly')"` with an unescaped inner quote.
3. It returns the SQL fragment `"author IN ('O'Reilly')"` at line 187, which is
syntactically invalid SQL because the embedded single quote prematurely terminates the
literal.
4. When `_handle_or_operator()` at `superset/utils/rison_filters.py:157-180` uses
`_build_sql_condition()` to assemble `sqlExpression` for an OR filter and that adhoc
filter is processed by `split_adhoc_filters_into_base_filters()` in
`superset/utils/core.py:1388-1507`, `sanitize_clause()` in `superset/sql/parse.py:1548-17`
will attempt to parse this invalid clause and raise a `QueryClauseValidationException`,
causing chart queries using such URL OR filters to fail once this parser is integrated
into the adhoc filter pipeline.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/rison_filters.py
**Line:** 184:187
**Comment:**
*Possible Bug: String values are wrapped in single quotes without escaping embedded quotes, so legitimate values like `O'Reilly` produce invalid SQL and break OR filter parsing/execution. Use proper SQL literal escaping or parameterized expression construction instead of manual quoting.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #97a860
Actionable Suggestions - 3
-
superset-frontend/src/dashboard/util/risonFilters.ts - 1
- Type assertion bug · Line 142-148
-
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx - 1
- State update bug · Line 130-130
-
superset/utils/rison_filters.py - 1
- Blind exception catch should be specific · Line 75-81
Additional Suggestions - 1
-
superset-frontend/src/dashboard/containers/DashboardPage.tsx - 1
-
Magic Numbers in setTimeout · Line 249-257Define named constants `URL_UPDATE_DELAY` and `PRETTIFY_DELAY` (set to 100 and 150, respectively) after the import section, then use these constants in the `setTimeout` calls to replace the magic numbers.
-
Review Details
-
Files reviewed - 12 · Commit Range:
16e8345..16e8345- superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx
- superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/UrlFilterTag.tsx
- superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/Vertical.tsx
- superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/VerticalCollapse.tsx
- superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/selectors.ts
- superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx
- superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx
- superset-frontend/src/dashboard/containers/DashboardPage.tsx
- superset-frontend/src/dashboard/util/risonFilters.test.ts
- superset-frontend/src/dashboard/util/risonFilters.ts
- superset/utils/rison_filters.py
- tests/unit_tests/utils/test_rison_filters.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| default: | ||
| return { | ||
| subject: key, | ||
| operator: '==', | ||
| comparator: value as string | number, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The default case in parseFilterCondition incorrectly casts the object value to string | number for comparator, but value is an object here. This could cause runtime type mismatches if an unrecognized operator is encountered. Change to throw an error instead.
Code suggestion
Check the AI-generated fix before applying
| default: | |
| return { | |
| subject: key, | |
| operator: '==', | |
| comparator: value as string | number, | |
| }; | |
| } | |
| default: | |
| throw new Error(`Unknown operator: ${operator}`); | |
| } |
Code Review Run #97a860
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| [chartIds, chartLayoutItems, dataMask, verboseMaps], | ||
| ); | ||
|
|
||
| const activeUrlFilters = useMemo(() => getUrlFilterIndicators(), []); |
There was a problem hiding this comment.
The useMemo for activeUrlFilters has empty dependencies, so it won't update when filters are removed, leaving stale UI. Change to useState and sync state on removal.
Code Review Run #97a860
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| try: | ||
| filters_obj = prison.loads(filter_string) | ||
| return self._convert_to_adhoc_filters(filters_obj) | ||
| except Exception: | ||
| logger.warning( | ||
| "Failed to parse Rison filters: %s", filter_string, exc_info=True | ||
| ) |
There was a problem hiding this comment.
Avoid catching bare Exception. Catch specific exception types like ValueError or prison.ParsingError instead.
Code suggestion
Check the AI-generated fix before applying
| try: | |
| filters_obj = prison.loads(filter_string) | |
| return self._convert_to_adhoc_filters(filters_obj) | |
| except Exception: | |
| logger.warning( | |
| "Failed to parse Rison filters: %s", filter_string, exc_info=True | |
| ) | |
| try: | |
| filters_obj = prison.loads(filter_string) | |
| return self._convert_to_adhoc_filters(filters_obj) | |
| except (ValueError, TypeError): | |
| logger.warning( | |
| "Failed to parse Rison filters: %s", filter_string, exc_info=True | |
| ) |
Code Review Run #97a860
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
- Move parseFilterCondition above parseRisonFilters to fix oxlint no-use-before-define - Apply prettier to risonFilters.ts and DashboardPage.tsx - Apply ruff-format to test_rison_filters.py Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #e71b1bActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
- Cast through `unknown` for the default-case comparator since `value` is narrowed to `object` in that branch - Guard against `Partial<Filter>['id']` being optional when indexing `updatedDataMask`; fall back to the matched key Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review Agent Run #e64a1d
Actionable Suggestions - 1
-
superset-frontend/src/dashboard/util/risonFilters.ts - 1
- CWE-1321: Object injection sink in updatedDataMask property access · Line 482-483
Review Details
-
Files reviewed - 1 · Commit Range:
4d001c7..2aa44d8- superset-frontend/src/dashboard/util/risonFilters.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| updatedDataMask[filterId] = { | ||
| ...updatedDataMask[filterId], |
There was a problem hiding this comment.
Lines 482-483 use dynamic property access on updatedDataMask[filterId]. The PR fix properly validates filterId before use, mitigating the object injection risk (CWE-1321).
Code Review Run #e64a1d
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for Rison-encoded ?f=(...) URL filters on dashboards, including auto-applying to matching native filters and displaying unmatched filters as removable “URL Filters” chips.
Changes:
- Added a Python Rison filter parser and merge helper for backend adhoc filter injection.
- Added frontend Rison parsing, intelligent native filter injection, and URL update utilities with unit tests.
- Added UI components to display/remove unmatched URL filters in both vertical and horizontal filter bars.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_tests/utils/test_rison_filters.py | Adds unit tests for the backend Rison filter parser. |
| superset/utils/rison_filters.py | Implements backend Rison parsing and merge into form_data. |
| superset-frontend/src/dashboard/util/risonFilters.ts | Implements frontend Rison parsing, native filter injection, URL cleanup, and helpers. |
| superset-frontend/src/dashboard/util/risonFilters.test.ts | Adds unit tests for frontend Rison parsing/injection utilities. |
| superset-frontend/src/dashboard/containers/DashboardPage.tsx | Hydrates dashboard state with URL Rison filters and cleans up the URL. |
| superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx | Preserves f query param while updating other filter state URL params. |
| superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx | Renders vertical “URL Filters” section above cross filters. |
| superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx | Adds horizontal “URL Filters” chips section. |
| superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/selectors.ts | Selects/parses URL filters into display-friendly “indicator” objects. |
| superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/Vertical.tsx | Adds vertical container component for URL filter chips section. |
| superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/VerticalCollapse.tsx | Adds collapsible vertical “URL Filters” section with removable chips. |
| superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/UrlFilterTag.tsx | Adds the URL filter chip UI. |
| def _build_sql_condition(self, column: str, value: Any) -> Optional[str]: | ||
| if isinstance(value, list): | ||
| values_str = ", ".join( | ||
| [f"'{v}'" if isinstance(v, str) else str(v) for v in value] | ||
| ) | ||
| return f"{column} IN ({values_str})" | ||
|
|
||
| if isinstance(value, dict): | ||
| operator_info = self._parse_operator_dict(value) | ||
| if operator_info: | ||
| op, comp = operator_info | ||
| if op == "BETWEEN" and isinstance(comp, list): | ||
| return f"{column} BETWEEN '{comp[0]}' AND '{comp[1]}'" | ||
| if op == "LIKE": | ||
| return f"{column} LIKE '{comp}'" | ||
| comp_str = f"'{comp}'" if isinstance(comp, str) else str(comp) | ||
| return f"{column} {op} {comp_str}" | ||
|
|
||
| val_str = f"'{value}'" if isinstance(value, str) else str(value) | ||
| return f"{column} = {val_str}" |
| "eq": "==", | ||
| } | ||
|
|
||
| def parse(self, filter_string: Optional[str] = None) -> list[dict[str, Any]]: |
| List of adhoc_filter dictionaries | ||
| """ | ||
| if filter_string is None: | ||
| filter_string = request.args.get("f") |
| export function risonFiltersToString(filters: RisonFilter[]): string { | ||
| if (filters.length === 0) { | ||
| return ''; | ||
| } | ||
|
|
||
| const risonObject: Record< | ||
| string, | ||
| string | number | boolean | (string | number)[] | Record<string, unknown> | ||
| > = {}; | ||
|
|
||
| filters.forEach(filter => { | ||
| if (filter.operator === 'IN' && Array.isArray(filter.comparator)) { | ||
| risonObject[filter.subject] = filter.comparator; | ||
| } else if (filter.operator === '==') { | ||
| risonObject[filter.subject] = filter.comparator; | ||
| } else { | ||
| const operatorMap: Record<string, string> = { | ||
| '>': 'gt', | ||
| '>=': 'gte', | ||
| '<': 'lt', | ||
| '<=': 'lte', | ||
| BETWEEN: 'between', | ||
| LIKE: 'like', | ||
| }; | ||
|
|
||
| const risonOp = operatorMap[filter.operator] || filter.operator; | ||
| risonObject[filter.subject] = { [risonOp]: filter.comparator }; | ||
| } | ||
| }); | ||
|
|
||
| try { | ||
| return rison.encode(risonObject); |
| export function risonFiltersToString(filters: RisonFilter[]): string { | ||
| if (filters.length === 0) { | ||
| return ''; | ||
| } | ||
|
|
||
| const risonObject: Record< | ||
| string, | ||
| string | number | boolean | (string | number)[] | Record<string, unknown> | ||
| > = {}; | ||
|
|
||
| filters.forEach(filter => { | ||
| if (filter.operator === 'IN' && Array.isArray(filter.comparator)) { | ||
| risonObject[filter.subject] = filter.comparator; | ||
| } else if (filter.operator === '==') { | ||
| risonObject[filter.subject] = filter.comparator; | ||
| } else { | ||
| const operatorMap: Record<string, string> = { | ||
| '>': 'gt', | ||
| '>=': 'gte', | ||
| '<': 'lt', | ||
| '<=': 'lte', | ||
| BETWEEN: 'between', | ||
| LIKE: 'like', | ||
| }; | ||
|
|
||
| const risonOp = operatorMap[filter.operator] || filter.operator; | ||
| risonObject[filter.subject] = { [risonOp]: filter.comparator }; | ||
| } | ||
| }); | ||
|
|
||
| try { | ||
| return rison.encode(risonObject); |
| const activeUrlFilters = useMemo(() => getUrlFilterIndicators(), []); | ||
|
|
||
| const handleRemoveUrlFilter = useCallback( | ||
| (filterToRemove: UrlFilterIndicator) => { | ||
| const risonParam = getRisonFilterParam(); | ||
| if (!risonParam) return; | ||
|
|
||
| const currentFilters = parseRisonFilters(risonParam); | ||
| const remaining = currentFilters.filter( | ||
| f => f.subject !== filterToRemove.filter.subject, | ||
| ); | ||
| updateUrlWithUnmatchedFilters(remaining); | ||
| }, | ||
| [], | ||
| ); |
| const previousParams = new URLSearchParams(search); | ||
| const newParams = new URLSearchParams(); | ||
| let dataMaskKey: string | null; | ||
| let risonFilterValue: string | null = null; | ||
|
|
||
| previousParams.forEach((value, key) => { | ||
| if (!EXCLUDED_URL_PARAMS.includes(key)) { | ||
| newParams.append(key, value); | ||
| if (key === 'f') { | ||
| // Preserve the original Rison filter value to avoid encoding | ||
| risonFilterValue = value; | ||
| } else { | ||
| newParams.append(key, value); | ||
| } | ||
| } | ||
| }); |
| // Manually reconstruct the search string to preserve Rison filter encoding | ||
| let searchString = newParams.toString(); | ||
| if (risonFilterValue) { | ||
| const separator = searchString ? '&' : ''; | ||
| searchString = `${searchString}${separator}f=${risonFilterValue}`; | ||
| } | ||
|
|
||
| history.replace({ | ||
| pathname: replacementPathname, | ||
| search: newParams.toString(), | ||
| search: searchString, | ||
| }); |
| const currentFilters = parseRisonFilters(risonParam); | ||
| const remaining = currentFilters.filter( | ||
| f => f.subject !== filterToRemove.filter.subject, | ||
| ); | ||
|
|
||
| updateUrlWithUnmatchedFilters(remaining); | ||
| setUrlFilters(prev => | ||
| prev.filter(f => f.subject !== filterToRemove.subject), | ||
| ); |
| const filterIndicators = useMemo( | ||
| () => | ||
| urlFilters.map(filter => ( | ||
| <UrlFilterTag | ||
| key={filter.subject} | ||
| filter={filter} | ||
| orientation={FilterBarOrientation.Vertical} | ||
| onRemove={handleRemoveFilter} | ||
| /> | ||
| )), | ||
| [urlFilters, handleRemoveFilter], | ||
| ); |
eschutho
left a comment
eschutho
left a comment
There was a problem hiding this comment.
Posting on Elizabeth's behalf — this is her PR reviewer agent. Forward any pushback to her and she'll loop me back in.
Three functional gaps below are the main things worth addressing before merge — the unmatched-filter wiring, the stale chip state, and the URL cleanup sequencing. The Python parser SQL issue is lower priority since the parser isn't connected yet, but worth flagging now. All line numbers verified against HEAD 2aa44d83.
Functional — worth addressing before merge
DashboardPage.tsx:634–641
The unmatched-filter path stores adhoc filters under the key __rison_filters__ in the data mask, but that key doesn't correspond to any native filter ID and nothing downstream reads it. The filters are computed, stored in dataMask, and silently dropped — the "unmatched filters apply as adhoc" behavior described in the PR summary doesn't actually happen.
WDYT — should this wire into the existing adhoc filter Redux path, or is the intent that unmatched filters only show as chips without filtering charts?
Horizontal.tsx:72, Vertical.tsx:267
Both components compute the chip list via useMemo(() => getUrlFilterIndicators(), []). getUrlFilterIndicators reads window.location.search, and with an empty dep array it only runs at mount. When the user clicks ✕ and updateUrlWithUnmatchedFilters changes the URL, the useMemo doesn't re-run — the chip stays rendered even though the filter is gone.
Could we drive the chip list reactively instead — either a useState updated by the remove handler, or a useEffect subscribed to popstate?
DashboardPage.tsx:646–657, FilterBar/index.tsx:550–573
Two issues combine here. First, the two setTimeout calls at 100ms and 150ms that mutate the URL aren't a safe ordering guarantee. Second, and more importantly, publishDataMask unconditionally re-appends the original Rison value on every filter interaction — so after the 100ms cleanup removes matched filters from f=, the next user interaction with any filter brings the original param back. Matched filters re-appear in the URL.
Could we combine the URL cleanup into one operation and have publishDataMask only preserve f= when there are still unmatched filters remaining, rather than always?
Other suggestions
rison_filters.py:1678–1696
_build_sql_condition (called by _handle_or_operator) interpolates user-controlled URL values directly into SQL strings — column is a URL key and values_str only wraps strings in single quotes (easy to escape). Since merge_rison_filters isn't wired to an endpoint yet this isn't an active risk, but would it be worth replacing the string interpolation with SQLAlchemy's parameterized filter construction before this gets connected?
DashboardPage.tsx:610–617
The code casts dashboard?.metadata?.native_filter_configuration to a loose inline type to build the nativeFilters lookup. state.nativeFilters.filters — already typed as PartialFilters — is available from the Redux selector used elsewhere in this component. Small suggestion: could we use that directly and avoid the cast?
A few small nits
-
VerticalCollapse.tsx:346–410—sectionContainerStyle,sectionHeaderStyle, etc. are wrapped inuseCallback(fn, [])but they're pure functions with no captured state. Happy to keep as-is, but they could be module-level constants. -
UrlFilterTag.tsx:224—editableprop onStyledTag; antd'sTagwitheditableenables inline text editing, which is probably unintentional for read-only URL filter chips. -
UrlFilters/selectors.ts— no Redux selectors in this file;urlFilterUtils.tsmight be a clearer name, but totally optional.
Praise
The native filter matching in risonFilters.ts:1322–1346 is solid — case-insensitive, whitespace-trimmed comparison across all filter targets, and the test suite covers it thoroughly including the space-in-column-name edge case. That's the kind of logic that usually has silent edge-case bugs; the implementation is clean.
SUMMARY
Adds support for
?f=(...)Rison-encoded URL filters on dashboards. Filters are matched to existing native filters by column name (case-insensitive, whitespace-tolerant) and auto-applied by populating bothfilterState.valueandextraFormData. Unmatched filters render as removable chips in a new "URL Filters" section above cross-filters in both vertical and horizontal filter bars. Includes a Python parser (superset/utils/rison_filters.py) for backend use, with unit tests covering equality, IN, NOT, OR, BETWEEN, LIKE, and comparison operators.building on top of: #34607
TESTING INSTRUCTIONS
Open a dashboard with a native filter on column
countryand try these URLs:?f=(country:USA)— auto-applies, hydrates the native filter?f=('Country Code':USA)— matches column names with spaces?f=(country:!(USA,Canada))— multi-value IN?f=(msrp:(between:!(35,200)))— numeric range, populatesextraFormData?f=(unknown_col:foo)— unmatched filter shows as removable chip in "URL Filters" sectionRun
pytest tests/unit_tests/utils/test_rison_filters.pyandnpm run test -- risonFilters.test.ts.ADDITIONAL INFORMATION