codeant-ai-for-open-source[bot] commented on code in PR #38851:
URL: https://github.com/apache/superset/pull/38851#discussion_r2990012528
##########
superset-frontend/src/pages/Chart/index.tsx:
##########
@@ -137,7 +137,9 @@ export default function ExplorePage() {
const saveAction = getUrlParam(
URL_PARAMS.saveAction,
) as SaveActionType | null;
Review Comment:
**Suggestion:** `saveAction` is still read from `window.location.search` via
`getUrlParam` without passing `location.search`. In Safari, this can remain
stale right after navigation, so `save_action` may be missed and the
initialization branch can run with incorrect behavior. Parse `saveAction` from
React Router's `location.search` the same way as dashboard context params.
[race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Explore overwrite navigation may skip data refetch.
- ⚠️ Safari can miss save_action on route transitions.
```
</details>
```suggestion
location.search,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `ExplorePage` and inspect `useEffect` in
`superset-frontend/src/pages/Chart/index.tsx:135-144`; `saveAction` is read
at `:137-139`
via `getUrlParam(URL_PARAMS.saveAction)` without passing `location.search`.
2. Trace `getUrlParam` implementation in
`superset-frontend/src/utils/urlUtils.ts:63-66`;
when no `search` is provided it reads `window.location.search`.
3. Verify stale-search behavior is explicitly reproduced in
`superset-frontend/src/utils/urlUtils.test.ts:168-184`; with stale
`window.location.search`, `getUrlParam(...)` returns `null` unless explicit
search
override is passed.
4. Trigger a real route-update path with `save_action=overwrite` shown in
`superset-frontend/src/pages/Chart/Chart.test.tsx:263` and click at `:291`;
after first
load `isExploreInitialized.current` is set true (`index.tsx:238`), so branch
guard at
`index.tsx:144` depends on `saveAction`.
5. If Safari returns stale `window.location.search`, `saveAction` becomes
null, `if
(!isExploreInitialized.current || !!saveAction)` fails (`index.tsx:144`), and
refetch/overwrite merge path is skipped; overwrite-specific behavior exists
at
`superset-frontend/src/explore/controlUtils/getFormDataWithDashboardContext.ts:278-286`.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/Chart/index.tsx
**Line:** 139:139
**Comment:**
*Race Condition: `saveAction` is still read from
`window.location.search` via `getUrlParam` without passing `location.search`.
In Safari, this can remain stale right after navigation, so `save_action` may
be missed and the initialization branch can run with incorrect behavior. Parse
`saveAction` from React Router's `location.search` the same way as dashboard
context params.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38851&comment_hash=caa8477f2df56ef2c0c6dc3976b94f2afa77dadcef01a0562a0f1132ff092f7e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38851&comment_hash=caa8477f2df56ef2c0c6dc3976b94f2afa77dadcef01a0562a0f1132ff092f7e&reaction=dislike'>👎</a>
##########
superset-frontend/src/utils/urlUtils.ts:
##########
@@ -38,22 +38,32 @@ export type UrlParamType = 'string' | 'number' | 'boolean'
| 'object' | 'rison';
export type UrlParam = (typeof URL_PARAMS)[keyof typeof URL_PARAMS];
export function getUrlParam(
param: UrlParam & { type: 'string' },
+ search?: string,
): string | null;
export function getUrlParam(
param: UrlParam & { type: 'number' },
+ search?: string,
): number | null;
export function getUrlParam(
param: UrlParam & { type: 'boolean' },
+ search?: string,
): boolean | null;
export function getUrlParam(
param: UrlParam & { type: 'object' },
+ search?: string,
+): object | null;
+export function getUrlParam(
+ param: UrlParam & { type: 'rison' },
+ search?: string,
): object | null;
Review Comment:
**Suggestion:** The overload for the `rison` URL param now declares `object
| null`, but the implementation still returns a raw string when `rison.decode`
fails. This creates an incorrect API contract: callers are now encouraged by
typing to treat the value as an object, which can cause runtime failures when
malformed `native_filters` values are present. Update the overload to include
`string` so type expectations match runtime behavior. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Malformed `native_filters` returns string, not object.
- ⚠️ Dashboard filter preselection can silently misbehave.
- ⚠️ Type contract misleads callers into unsafe assumptions.
```
</details>
```suggestion
): string | object | null;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard URL containing malformed `native_filters` (defined as
`type: 'rison'`
at `superset-frontend/src/constants.ts:38-41`), e.g. invalid Rison payload.
2. Dashboard hydration path calls `getUrlParam(URL_PARAMS.nativeFilters)`
from
`superset-frontend/src/dashboard/actions/hydrate.ts:370`.
3. `getUrlParam()` enters the `rison` branch at
`superset-frontend/src/utils/urlUtils.ts:92`; when `rison.decode` fails
(`:97`), catch
block returns raw `urlParam` string (`:99`).
4. Current overload for `type: 'rison'` declares `object | null` at
`superset-frontend/src/utils/urlUtils.ts:55-58`, so TypeScript callers are
told result is
object while runtime may be string.
5. Downstream code treats this as structured filters (e.g.
`preselectNativeFilters`
consumed in
`superset-frontend/src/dashboard/components/nativeFilters/FilterBar/state.ts:39-52`),
causing silent mismatch/no preselection instead of type-safe handling.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/utils/urlUtils.ts
**Line:** 58:58
**Comment:**
*Type Error: The overload for the `rison` URL param now declares
`object | null`, but the implementation still returns a raw string when
`rison.decode` fails. This creates an incorrect API contract: callers are now
encouraged by typing to treat the value as an object, which can cause runtime
failures when malformed `native_filters` values are present. Update the
overload to include `string` so type expectations match runtime behavior.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38851&comment_hash=8d7f3da53b7674e2d2b41dfcb7c82c15d7d9b00a5e5f7b31f1bb52ee4c1a3c72&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38851&comment_hash=8d7f3da53b7674e2d2b41dfcb7c82c15d7d9b00a5e5f7b31f1bb52ee4c1a3c72&reaction=dislike'>👎</a>
--
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]
