codeant-ai-for-open-source[bot] commented on code in PR #41912:
URL: https://github.com/apache/superset/pull/41912#discussion_r3562080891
##########
superset-frontend/src/features/home/SavedQueries.tsx:
##########
@@ -303,7 +304,10 @@ export const SavedQueries = ({
{queries.length > 0 ? (
<CardContainer showThumbnails={showThumbnails}>
{queries.map(q => (
- <CardStyles key={q.id}>
+ <CardStyles
+ key={q.id}
+ onClick={() => history.push(`/sqllab?savedQueryId=${q.id}`)}
+ >
Review Comment:
**Suggestion:** Adding a wrapper-level click navigation creates a
double-navigation path for cards that still render `ListViewCard`’s fallback
anchor (`cover={false}` when thumbnails are enabled and SQL is empty): the
anchor default navigation and the wrapper `history.push` both fire on the same
click. This can produce duplicate history entries and breaks expected
modified-click/link behavior; use a single navigation mechanism per card (or
ignore wrapper clicks originating from inner links). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Thumbnail click can trigger SPA and full page navigation.
- ⚠️ Modified clicks on thumbnail also navigate current tab unexpectedly.
- ⚠️ Saved queries with empty sql render inconsistent card behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Home page mounts the SavedQueries section via SavedQueries in file
`superset-frontend/src/pages/Home/index.tsx` lines 158-172, passing
`showThumbnails={checked}`, `mine={queryData}`, and
`featureFlag={isThumbnailsEnabled}` to
SavedQueries.
2. SavedQueries renders saved query cards in
`superset-frontend/src/features/home/SavedQueries.tsx` lines 304-343,
wrapping each
ListViewCard in CardStyles with an onClick handler at lines 307-310 that
calls
`history.push('/sqllab?savedQueryId=' + q.id)` when the card wrapper is
clicked.
3. For each query, the ListViewCard `cover` prop is computed at lines
319-342 in
SavedQueries: when `showThumbnails` is true and the query has no SQL (q.sql
is null or
empty) the expression evaluates to `false`, so ListViewCard receives
`cover={false}` while
still receiving a non undefined `url` prop.
4. In ListViewCard implementation at
`superset-frontend/packages/superset-ui-core/src/components/ListViewCard/index.tsx`
lines
147-195, when `cover` is falsy and `url` is set, the component renders its
fallback cover,
which includes AnchorLink at lines 143-145 and 175-184 wrapping the
thumbnail image in a
native anchor element with `href={sanitizeUrl(url)}`; a click on this image
now triggers
both the outer CardStyles onClick (SavedQueries.tsx 307-310, calling
`history.push`) and
the inner anchor default navigation (full page navigation to `url`), so for
saved queries
whose `sql` attribute is null or empty (allowed by SavedQuery model `sql:
str | None` in
`superset-core/src/superset_core/queries/models.py` lines 56-71), a user
with thumbnails
toggled on (Home/index.tsx lines 69-79) can produce double navigation and
broken modified
click behavior (for example, ctrl or cmd click on the thumbnail navigates
the current tab
via the wrapper handler while also opening a new tab via the anchor).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=33fb84531f0d4e19ba683ac8afba22e2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=33fb84531f0d4e19ba683ac8afba22e2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/home/SavedQueries.tsx
**Line:** 307:310
**Comment:**
*Logic Error: Adding a wrapper-level click navigation creates a
double-navigation path for cards that still render `ListViewCard`’s fallback
anchor (`cover={false}` when thumbnails are enabled and SQL is empty): the
anchor default navigation and the wrapper `history.push` both fire on the same
click. This can produce duplicate history entries and breaks expected
modified-click/link behavior; use a single navigation mechanism per card (or
ignore wrapper clicks originating from inner links).
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41912&comment_hash=e07e52bdcee413c03edfb0742ea3e9d3caf7cb52c0bbb0095f9e8d2efefb60b8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41912&comment_hash=e07e52bdcee413c03edfb0742ea3e9d3caf7cb52c0bbb0095f9e8d2efefb60b8&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]
