codeant-ai-for-open-source[bot] commented on code in PR #41285:
URL: https://github.com/apache/superset/pull/41285#discussion_r3474592407
##########
superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx:
##########
@@ -98,6 +101,101 @@ const AddTabIconWrapper = styled.span`
// Get the user's OS
const userOS = detectOS();
+const newTabTooltip =
+ userOS === 'Windows' ? t('New tab (Ctrl + q)') : t('New tab (Ctrl + t)');
+
+const PlusIcon = (
+ <AddTabIconWrapper>
+ <Icons.PlusOutlined iconSize="l" data-test="add-tab-icon" />
+ </AddTabIconWrapper>
+);
+
+function NewTabButton({ onAddSqlEditor }: { onAddSqlEditor: () => void }) {
+ const [open, setOpen] = useState(false);
+
+ const dropdownItems = useMemo<MenuItemType[]>(() => {
+ if (!open) return [];
+ const primaryItems =
+ menus.getMenu(ViewLocations.sqllab.newTab)?.primary ?? [];
+ return [
+ {
+ key: 'sql-editor',
+ label: t('SQL Editor'),
+ icon: <Icons.TableOutlined iconSize="m" />,
+ onClick: () => {
+ setOpen(false);
+ onAddSqlEditor();
+ },
+ },
+ ...primaryItems.map(item => {
+ const command = commands.getCommand(item.command);
+ const Icon = command?.icon
+ ? ((Icons as Record<string, typeof Icons.FileOutlined>)[
+ command.icon
+ ] ?? Icons.FileOutlined)
+ : Icons.FileOutlined;
+ return {
+ key: command?.id ?? item.command,
+ label: command?.title ?? item.command,
+ icon: <Icon iconSize="m" />,
+ onClick: () => {
+ setOpen(false);
+ commands.executeCommand(item.command);
+ },
+ } as MenuItemType;
+ }),
+ ];
+ }, [open, onAddSqlEditor]);
+
+ const activate = () => {
+ const primaryItems =
+ menus.getMenu(ViewLocations.sqllab.newTab)?.primary ?? [];
+ if (primaryItems.length === 0) {
+ onAddSqlEditor();
+ } else {
+ setOpen(prev => !prev);
+ }
+ };
+
+ const handleClick = (e: React.MouseEvent) => {
+ // Antd's Tabs wraps addIcon in its own <button onClick={() =>
onEdit('add')}>.
+ // Stop propagation so antd doesn't also call newQueryEditor() while we
handle it.
+ e.stopPropagation();
+ activate();
+ };
+
+ const handleKeyDown = (e: React.KeyboardEvent) => {
+ // The same wrapper button activates on Enter/Space; intercept those keys
so
+ // keyboard users reach the extension new-tab dropdown rather than antd's
+ // default add-tab path.
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ e.stopPropagation();
+ activate();
+ }
+ };
+
+ return (
+ <Tooltip id="add-tab" placement="left" title={newTabTooltip}>
+ <Dropdown
+ open={open}
+ onOpenChange={setOpen}
+ menu={{ items: dropdownItems }}
+ trigger={[]}
+ >
+ <span
+ role="button"
+ tabIndex={0}
+ onClick={handleClick}
+ onKeyDown={handleKeyDown}
+ >
Review Comment:
**Suggestion:** The dropdown is controlled entirely by handlers on an inner
`<span>` while the AntD tabs add button still owns the outer clickable button.
Because `trigger` is disabled and propagation is only stopped from the inner
element, clicks on button padding (or keyboard activation on the outer button)
can still go through AntD's default `onEdit('add')` path and create a plain SQL
tab instead of opening the extension new-tab menu. Wire the behavior at the
actual add-button level (or guard the add path when extension tab types exist)
so every add-button activation consistently uses the new-tab menu flow. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Extension tab types bypassed when clicking add-button padding.
- ⚠️ Keyboard users cannot access extension new-tab dropdown.
- ⚠️ SQL Lab + button behavior inconsistent once extensions register.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Load SQL Lab, which renders `TabbedSqlEditors` inside the main app layout
(`superset-frontend/src/SqlLab/components/App/index.tsx:219-228` renders
`<TabbedSqlEditors />`).
2. Enable an extension that contributes new-tab commands so that
`menus.getMenu(ViewLocations.sqllab.newTab)?.primary` returns one or more
items when
`NewTabButton` builds `dropdownItems`
(`superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx:116-123,119-120`),
as
described by the `ViewLocations.sqllab.newTab` comment in
`contributions.ts:53-56`.
3. In this state, the tabs add button is rendered by AntD as an outer
`<button
class="ant-tabs-nav-add" onClick={() => onEdit('add')}>` which wraps the
`addIcon`
content; `TabbedSqlEditors` passes `<NewTabButton ...>` as `addIcon` at
`index.tsx:76-78`,
and `NewTabButton` renders the `<Dropdown>` and inner `<span role="button"
...>` with
handlers at `index.tsx:178-191`. Because the `onClick`/`onKeyDown` handlers
and
`e.stopPropagation()` are attached only to this inner `<span>`
(`index.tsx:186-191,160-165`), a pointer click on the outer button's padding
area (where
the event target is the `<button>`, not the inner `<span>`) bypasses
`handleClick`, so the
click is not stopped and AntD's `onEdit('add')` fires, calling
`TabbedSqlEditors.handleEdit('add')` and `this.newQueryEditor()` at
`index.tsx:248-258` to
create a plain SQL tab instead of opening the extension dropdown.
4. For keyboard users, focus lands on the outer `ant-tabs-nav-add`
`<button>` provided by
AntD, whose Enter/Space activation synthesizes a click on the button; the
inner `<span
tabIndex={0} onKeyDown={handleKeyDown}>` at `index.tsx:186-191` never
receives the keydown
event, so the dropdown is not toggled and `handleEdit('add')` is invoked
instead, again
creating a default SQL tab and bypassing any contributed tab-type commands
even though
extension `newTab` items are present.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1015b93cf9474bd4a78d65bc189b3823&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=1015b93cf9474bd4a78d65bc189b3823&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/SqlLab/components/TabbedSqlEditors/index.tsx
**Line:** 180:191
**Comment:**
*Logic Error: The dropdown is controlled entirely by handlers on an
inner `<span>` while the AntD tabs add button still owns the outer clickable
button. Because `trigger` is disabled and propagation is only stopped from the
inner element, clicks on button padding (or keyboard activation on the outer
button) can still go through AntD's default `onEdit('add')` path and create a
plain SQL tab instead of opening the extension new-tab menu. Wire the behavior
at the actual add-button level (or guard the add path when extension tab types
exist) so every add-button activation consistently uses the new-tab menu flow.
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%2F41285&comment_hash=473ef973961c6a31da04035d5f27afc0335e259a404f1ab55f1bb1ec0ac6171d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41285&comment_hash=473ef973961c6a31da04035d5f27afc0335e259a404f1ab55f1bb1ec0ac6171d&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]
