codeant-ai-for-open-source[bot] commented on code in PR #37681:
URL: https://github.com/apache/superset/pull/37681#discussion_r2766214141
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -144,11 +144,11 @@ const publishDataMask = debounce(
// it when necessary. We strip any prefix so that history.replace adds
it back and doesn't
// double it up.
const appRoot = applicationRoot();
- let replacement_pathname = window.location.pathname;
- if (appRoot !== '/' && replacement_pathname.startsWith(appRoot)) {
- replacement_pathname = replacement_pathname.substring(appRoot.length);
+ let replacementPathname = window.location.pathname;
+ if (appRoot !== '/' && replacementPathname.startsWith(appRoot)) {
+ replacementPathname = replacementPathname.substring(appRoot.length);
}
- history.location.pathname = replacement_pathname;
+ history.location.pathname = replacementPathname;
history.replace({
Review Comment:
**Suggestion:** The code mutates `history.location.pathname` directly before
calling `history.replace`, but in React Router the `location` object is treated
as immutable and may be frozen or non‑writable, which can cause a runtime
TypeError in strict mode and is an unsupported pattern; instead, pass the
corrected `pathname` directly to `history.replace` and avoid mutating
`history.location`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Persisting native filters to URL fails on dashboard pages.
- ❌ Uncaught TypeError breaks publishDataMask execution.
- ⚠️ Filters may not be saved for users with appRoot set.
- ⚠️ Tests that expect history.replace may intermittently fail.
```
</details>
```suggestion
if (appRoot !== '/' && appRoot &&
replacementPathname.startsWith(appRoot)) {
replacementPathname = replacementPathname.substring(appRoot.length);
}
history.replace({
pathname: replacementPathname,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the file src/dashboard/components/nativeFilters/FilterBar/index.tsx
and locate the
publishDataMask function defined starting at line 134. Note the guard that
checks the
current page: the if
(window.location.pathname.includes('/superset/dashboard')) check at
line 141 is what enables the code path that updates the history.
2. Run the application and navigate to a dashboard whose URL includes the
application root
prefix (for example, the URL starts with /superset/dashboard).
applicationRoot() is read
at the code on line 146 (const appRoot = applicationRoot()); ensure
applicationRoot()
returns a non-root value (e.g., '/superset') so the substring branch is
exercised.
3. Trigger publishDataMask to run by causing the effect that calls it to
fire: in the same
file the useEffect that calls publishDataMask(history, dashboardId,
updateKey,
dataMaskApplied, tabId) executes when dataMaskAppliedText or
dashboardId/updateKey/tabId
changes (see the useEffect that invokes publishDataMask in this component).
This happens
in normal usage when filter state is persisted (e.g., updating filters or
loading a
dashboard with user.userId present).
4. When publishDataMask runs, it computes replacementPathname and then
executes the
mutation at history.location.pathname = replacementPathname (line 151 in the
publishDataMask function). If the history.location object provided by
react-router is
non-writable/frozen (some React Router history implementations expose an
immutable
Location), this assignment throws a runtime TypeError (e.g., "Cannot assign
to read only
property 'pathname' of object '#<Location>'"), causing publishDataMask to
fail and
preventing history.replace from correctly executing. Observe the uncaught
TypeError in the
browser console and the failure to update the URL / persist filter state.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx
**Line:** 148:152
**Comment:**
*Logic Error: The code mutates `history.location.pathname` directly
before calling `history.replace`, but in React Router the `location` object is
treated as immutable and may be frozen or non‑writable, which can cause a
runtime TypeError in strict mode and is an unsupported pattern; instead, pass
the corrected `pathname` directly to `history.replace` and avoid mutating
`history.location`.
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.
```
</details>
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts:
##########
@@ -64,7 +63,9 @@ export const checkIsApplyDisabled = (
const dataSelectedValues = Object.values(dataMaskSelected);
const dataAppliedValues = Object.values(dataMaskApplied);
- const hasMissingRequiredFilter = filters.some(filter =>
+ // Check required values for ALL filters (including out-of-scope) if provided
+ const filtersToValidateRequired = allFilters ?? filtersInScope;
Review Comment:
**Suggestion:** Using `allFilters ?? filtersInScope` means that when
`allFilters` is accidentally passed as an empty array, required-filter
validation is skipped entirely instead of falling back to in-scope filters,
which can leave required filters unvalidated and allow unfiltered data through.
[possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Required filters skipped when allFilters is [].
- ⚠️ Apply validation falls back incorrectly.
- ⚠️ Potential unfiltered chart data exposure.
```
</details>
```suggestion
const filtersToValidateRequired =
allFilters && allFilters.length ? allFilters : filtersInScope;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts`
and
locate the all-filters fallback at lines 66-73.
2. From the UI path that calls `checkIsApplyDisabled`, ensure an explicit
call passes
`allFilters` as an empty array (e.g., `checkIsApplyDisabled(dataMaskSelected,
dataMaskApplied, filtersInScope, [])`). This is a realistic caller pattern
when code
gathers all filters but results in an empty array for certain dashboard
states.
3. Execution reaches `utils.ts:66` and evaluates `const
filtersToValidateRequired =
allFilters ?? filtersInScope;`. Because `allFilters` is an empty array
(truthy), the code
selects the empty array and `filtersToValidateRequired.some(...)` runs on an
empty array
and returns false, skipping required-filter checks.
4. Observe that required filters in `filtersInScope` are not validated and
Apply may be
allowed despite missing required values.
Note: this reproduces directly from the fallback expression behavior in
`utils.ts:66-73`
and demonstrates a realistic callsite passing an explicit empty array.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts
**Line:** 67:67
**Comment:**
*Possible Bug: Using `allFilters ?? filtersInScope` means that when
`allFilters` is accidentally passed as an empty array, required-filter
validation is skipped entirely instead of falling back to in-scope filters,
which can leave required filters unvalidated and allow unfiltered data through.
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.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
