codeant-ai-for-open-source[bot] commented on code in PR #41352:
URL: https://github.com/apache/superset/pull/41352#discussion_r3543571114
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -227,14 +227,27 @@ const SaveModal = ({
const history = useHistory();
const theme = useTheme();
+ const isCurrentUserOwner = useCallback((): boolean => {
+ const userId = user?.userId;
+ if (userId === undefined) {
+ return false;
+ }
+ // Owners can arrive either as plain ids (number) or as objects depending
on
+ // where the slice was hydrated from (bootstrap vs. SLICE_UPDATED reducer),
+ // so normalize to the numeric id before comparing.
Review Comment:
**Suggestion:** The added comment claims object-shaped owners come from the
`SLICE_UPDATED` reducer path, but `exploreReducer` normalizes `SLICE_UPDATED`
owners to numeric IDs before storing them in `slice.owners`; this is a factual
mismatch with current reducer behavior and should be corrected to avoid future
debugging mistakes. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
โ ๏ธ SaveModal comment misstates SLICE_UPDATED owners normalization behavior.
โ ๏ธ Debugging overwrite issues may follow misleading reducer path.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Open `superset-frontend/src/explore/components/SaveModal.tsx` and find the
`isCurrentUserOwner` helper; its comment at diff lines 235-237 states that
owners can be
plain IDs or objects depending on where the slice was hydrated from,
explicitly citing the
bootstrap versus `SLICE_UPDATED` reducer paths.
2. Open `superset-frontend/src/explore/reducers/exploreReducer.ts` and
inspect the
`SliceUpdatedAction` type at lines 165-169 and associated `OwnerItem`
definition at lines
160-163, which allow `owners` in the action payload to be either numbers or
objects with
`value` and `label`.
3. In the same reducer file, examine the `[actions.SLICE_UPDATED]` handler
at lines
615-619 (from Grep output); the code maps `typedAction.slice.owners` through
`getOwnerId`
and assigns `owners: typedAction.slice.owners ?
typedAction.slice.owners.map(getOwnerId) :
null`, ensuring that `state.slice.owners` is stored as a numeric ID array
rather than
object-shaped data after the reducer runs.
4. From this, verify that while the action payload may contain mixed shapes,
the persisted
`slice.owners` in explore state is normalized to numbers in the
`SLICE_UPDATED` path, so
the SaveModal comment misattributes object-shaped owners to the reducer
state and can
misdirect future debugging or refactors that rely on that description.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=113fff3593a3458e89c7ba171ff9f122&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=113fff3593a3458e89c7ba171ff9f122&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/explore/components/SaveModal.tsx
**Line:** 235:237
**Comment:**
*Comment Mismatch: The added comment claims object-shaped owners come
from the `SLICE_UPDATED` reducer path, but `exploreReducer` normalizes
`SLICE_UPDATED` owners to numeric IDs before storing them in `slice.owners`;
this is a factual mismatch with current reducer behavior and should be
corrected to avoid future debugging mistakes.
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%2F41352&comment_hash=659b835283e374dc338170b49368aae3f657192de28e39e5565da5ea63e896de&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41352&comment_hash=659b835283e374dc338170b49368aae3f657192de28e39e5565da5ea63e896de&reaction=dislike'>๐</a>
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -350,6 +350,27 @@ test('enables overwrite option for admin non-owner', () =>
{
expect(getByRole('radio', { name: 'Save (Overwrite)' })).toBeEnabled();
});
+test('enables overwrite option for owner when owners are objects', () => {
+ // The Slice type declares `owners: { id: number }[]`, and the explore
+ // bootstrap can deliver owners in object form. A chart owner must still be
+ // able to overwrite their own chart in that case.
Review Comment:
**Suggestion:** The new test comment states that the `Slice` type declares
`owners` as `{ id: number }[]`, but the actual `Slice` type in
`src/types/Chart.ts` declares `owners?: number[]`; this inaccurate comment will
mislead future maintainers about the real contract and why this case exists.
Update the comment to describe the true source of object-shaped owners without
claiming the `Slice` type itself is object-based. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
โ ๏ธ Tests misdocument Slice owners contract, confusing maintainers.
โ ๏ธ Future fixes may target wrong owner data source.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Open `superset-frontend/src/explore/components/SaveModal.test.tsx` and
inspect the new
test `enables overwrite option for owner when owners are objects`; the
inline comment at
diff lines 354-356 states that the Slice type declares `owners: { id: number
}[]`.
2. Open `superset-frontend/src/types/Chart.ts` and inspect the `Slice` type
definition at
lines 65-76 (from Grep output), where `owners` is declared as `owners?:
number[]`,
indicating a numeric ID array rather than an array of `{ id: number }`
objects.
3. Compare the two definitions and verify that the comment in the test file
misdocuments
the actual `Slice.owners` contract, attributing an object-shaped type to the
Slice
definition itself instead of to specific hydration paths.
4. Consider a maintainer debugging owner handling based on this test; they
will be led to
believe the core type is object-based and may incorrectly adjust type
definitions or
normalization logic, demonstrating the practical impact of this misleading
comment.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c7a84019464e49c7816d0fdb3ec6ebba&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=c7a84019464e49c7816d0fdb3ec6ebba&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/explore/components/SaveModal.test.tsx
**Line:** 354:356
**Comment:**
*Comment Mismatch: The new test comment states that the `Slice` type
declares `owners` as `{ id: number }[]`, but the actual `Slice` type in
`src/types/Chart.ts` declares `owners?: number[]`; this inaccurate comment will
mislead future maintainers about the real contract and why this case exists.
Update the comment to describe the true source of object-shaped owners without
claiming the `Slice` type itself is object-based.
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%2F41352&comment_hash=e752fd368a0513ab9a6128baab9d1a100bc1d2051f099124c51245fe904c5114&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41352&comment_hash=e752fd368a0513ab9a6128baab9d1a100bc1d2051f099124c51245fe904c5114&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]
