EnxDev commented on PR #41285: URL: https://github.com/apache/superset/pull/41285#issuecomment-5339741076
## EnxDev's Review Agent โ apache/superset#41285 ยท HEAD fee0981 **comment** โ one real UX bug in the new `+` dropdown; everything else is clean and CI is green apart from the `hold!` gate. Supersedes my [earlier review at 5c38df7](https://github.com/apache/superset/pull/41285#pullrequestreview-4572665212). Re-verified against this HEAD: `lint-frontend` is green now, `NewTabButton` has four tests, the command-not-registered filter and the `isActive` gate are in, and the a11y concern about the nested interactive `<span>` is gone with the capture-listener rewrite. `check-hold-label` is the only red check โ that's the `hold!` label, not the code. ### ๐ด Functional - **`SqlLab/components/TabbedSqlEditors/index.tsx:206`** ยท _High_ โ `trigger={[]}` opts the Dropdown out of rc-trigger's own dismissal. antd passes `trigger` straight through as `action` (`antd/lib/dropdown/dropdown.js:135,228`), rc-dropdown forwards it (`@rc-component/dropdown/lib/Dropdown.js:122-124`), and rc-trigger derives `clickToHide` from it โ with an empty array `clickToHide` is `false`, so `useWinClick` never registers the outside `mousedown` listener. Once an extension contributes a `newTab` item and the user opens the dropdown, it stays open until they re-click `+` or pick an item: clicking into the editor, switching tabs, or clicking anywhere else leaves it floating over the UI. Keeping `trigger={[]}`, add an explicit window `mousedown` + `Escape` dismiss while `open` (what `useWinClick` would have done). `trigger={['click']}` restores outside-click for free but makes a second `+` click reopen rather than close, since rc-trigger's mousedown closes it just before y our capture handler toggles it back. **regression test:** open the dropdown with a contributed item, `fireEvent.mouseDown(document.body)`, assert the menu is gone. ### ๐ก Should-fix - **`SqlLab/components/SqlEditor/index.tsx:325-337`** โ the `isActive` gate (the fix for Copilot's stale-pending-key finding) has no test; both new SqlEditor tests mount the active tab. Add one that mounts a non-active editor with `PENDING_NORTH_PANE_VIEW_KEY` set and asserts it neither consumes the key nor swaps to the view โ otherwise the regression it guards against comes back silently. - **`SqlLab/components/TabbedSqlEditors/index.tsx:176-198`** โ the listener moved to capture-phase click specifically so Enter/Space reach the dropdown, but all four new tests use `fireEvent.click`, so the keyboard path is unguarded. `userEvent.keyboard('{Enter}')` on the focused add button would cover it. - **`SqlLab/components/TabbedSqlEditors/index.tsx:101`** โ `newTabTooltip` moved from render-time to module scope, so `t()` now runs at module load. That's the pattern `TranslatorSingleton.warnPreConfigure` calls out ("called before configure() โ the result is the fallback language"); it only resolves today because the HTML template pre-stashes `__SUPERSET_LANGUAGE_PACK__` on `window`. Anywhere that pack isn't there first the tooltip silently falls back to English. Move it back into the component or make it a function. ### ๐ต Nits - `SqlLab/components/TabbedSqlEditors/index.tsx:304` โ `onTabClicked` still calls `Logger.markTimeOrigin()` immediately before `newQueryEditor()`, which now marks again at `:263`. Drop the one at `:304` to match the "centralized here" comment. - `extensions/ExtensionsLoader.ts:117` + `extensions/ExtensionsStartup.tsx:85` โ both log `'Error setting up extensions:'`. Now that the loader rethrows, a failed load is always logged twice. - `SqlLab/components/SqlEditor/index.tsx:341-345` โ when `tabViewId` arrives after backend sync, `northPaneStorageId` flips from `id` and the old `id`-keyed entry is left orphaned in `localStorage`. Harmless (I confirmed reload still restores, since hydrated editors carry the backend id as `queryEditor.id`), just untidy. ### ๐ Praise - `SqlLab/components/SqlEditor/index.tsx:311-355` โ driving northPane off the reactive `useViews()` hook, guarding the raw `localStorage` access, and syncing via the `storage` event is a tidy design; async-registered extension views appear without blocking mount. - Good turnaround on the last round โ the `isActive` gate, the unregistered-command filter, and the `markTimeOrigin` centralization all landed as real fixes rather than assertions. <!-- enxdev-review-agent:fee0981 --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD fee0981._ -- 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]
