codeant-ai-for-open-source[bot] commented on code in PR #41552:
URL: https://github.com/apache/superset/pull/41552#discussion_r3536748851
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx:
##########
@@ -281,11 +281,20 @@ const ColumnSelectPopover = ({
[columnMap, metricMap, onSimpleColumnChange, onSimpleMetricChange],
);
- const defaultActiveTabKey = initialAdhocColumn
- ? 'sqlExpression'
- : selectedCalculatedColumn
- ? 'saved'
- : 'simple';
+ // Pick the most natural starting tab, but never one that's disabled โ
+ // semantic-view contexts disable Simple and Custom SQL, leaving Saved.
Review Comment:
**Suggestion:** The new comment above default-tab selection contradicts the
implemented behavior: semantic views in this PR only disable `sqlExpression`,
not `simple`. Update the comment to match actual logic, otherwise future
changes may incorrectly disable the Simple tab and make semantic-view
dimensions unreachable. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Semantic-view tab behavior misdocumented in column popover.
- โ ๏ธ Future refactors risk disabling Simple tab incorrectly.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Open `DndColumnSelect.tsx` at
`superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx`
and inspect lines 56-71, where the comment explains semantic-view behavior
and
`effectiveDisabledTabs` is computed (only `sqlExpression` is added when
`datasourceType
=== 'semantic_view'`).
2. Note in the same file (`DndColumnSelect.tsx` lines 62-69) that the
comment explicitly
states: \"Semantic-view dimensions are pre-defined items... the Custom SQL
tab... is
disabled. The Simple tab stays enabled\", confirming Simple must remain
enabled for
semantic views.
3. Open `ColumnSelectPopover.tsx` at
`superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx`
and inspect the new comment at lines 185-187 (PR hunk lines 284-285):
\"semantic-view
contexts disable Simple and Custom SQL, leaving Saved.\"
4. Compare this comment with the actual behavior: in semantic-view contexts
`disabledTabs`
contains only `sqlExpression` (from `effectiveDisabledTabs`), so Simple is
not disabled;
the comment therefore documents the opposite of the real behavior, creating
a misleading
guide for future changes.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3628b63dc7364720baf1ab8045be1cfd&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=3628b63dc7364720baf1ab8045be1cfd&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/controls/DndColumnSelectControl/ColumnSelectPopover.tsx
**Line:** 284:285
**Comment:**
*Comment Mismatch: The new comment above default-tab selection
contradicts the implemented behavior: semantic views in this PR only disable
`sqlExpression`, not `simple`. Update the comment to match actual logic,
otherwise future changes may incorrectly disable the Simple tab and make
semantic-view dimensions unreachable.
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%2F41552&comment_hash=f27695f3f03e1ef181fbb9ece18662c7db567b4dccd91ab863aa3c3770f11c96&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41552&comment_hash=f27695f3f03e1ef181fbb9ece18662c7db567b4dccd91ab863aa3c3770f11c96&reaction=dislike'>๐</a>
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx:
##########
@@ -501,8 +510,15 @@ const ColumnSelectPopover = ({
},
]),
{
+ // The Simple tab is rendered even when disabled (mirroring how
+ // Custom SQL behaves below) so users see all the options that
+ // could exist in another context and understand why they're
+ // unavailable here. Semantic-view contexts disable it because
+ // dimensions there are pre-defined items in the semantic model,
+ // not physical columns to wrap.
Review Comment:
**Suggestion:** The inline comment on the Simple tab says semantic-view
contexts disable Simple, but this PRโs behavior and tests require Simple to
remain enabled for semantic-view dimensions. Correct this comment to avoid
documenting the inverse of the intended behavior. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Simple tab comment contradicts semantic-view implementation in
DndColumnSelect.
- โ ๏ธ Misleading docs complicate maintenance of explore column selector.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. In `DndColumnSelect.tsx`
(`superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx`),
inspect lines 56-71 where semantic-view behavior is documented: the comment
states that
for semantic views the Custom SQL tab is disabled while Simple remains
enabled, and
`effectiveDisabledTabs` only adds `'sqlExpression'` when `datasourceType ===
'semantic_view'`.
2. Confirm that `effectiveDisabledTabs` is passed as `disabledTabs` into
`ColumnSelectPopoverTrigger` (lines 135-149) and then into
`ColumnSelectPopover` (Trigger
file `ColumnSelectPopoverTrigger.tsx` lines 116-134), so semantic-view
contexts see
`disabledTabs.has('sqlExpression') === true` and `disabledTabs.has('simple')
=== false`.
3. Open `ColumnSelectPopover.tsx`
(`superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx`)
and inspect the Simple tab definition around lines 410-422 (PR hunk lines
513-518): the
comment says \"Semantic-view contexts disable it because dimensions there
are pre-defined
items in the semantic model, not physical columns to wrap.\"
4. Compare this Simple-tab comment with the upstream `DndColumnSelect`
logic: Simple is
deliberately kept enabled for semantic-view dimensions, so the comment
describes the
inverse of actual behavior; this mismatch can mislead maintainers about
which tabs are
supposed to be disabled in semantic-view explore flows.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=94b6bcd9fdbc4b07b3d9a4f48c690801&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=94b6bcd9fdbc4b07b3d9a4f48c690801&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/controls/DndColumnSelectControl/ColumnSelectPopover.tsx
**Line:** 513:518
**Comment:**
*Comment Mismatch: The inline comment on the Simple tab says
semantic-view contexts disable Simple, but this PRโs behavior and tests require
Simple to remain enabled for semantic-view dimensions. Correct this comment to
avoid documenting the inverse of the intended 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.
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%2F41552&comment_hash=2de6696cf07ad4557b245a1325ab24a7a172b4f879bcbae53353d9944d58d925&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41552&comment_hash=2de6696cf07ad4557b245a1325ab24a7a172b4f879bcbae53353d9944d58d925&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]
