Two fixes from @sadpandajoe's review.
1. **Double-fetch.** `fetchAppliedChart` synchronously sets both `value`
and `slice` from one API response. The value-change watcher then saw
`value` changed and called `fetchSliceData(value.value)` — re-resolving
the same chart and overwriting the slice we just set.
Fix: gate the watcher's `fetchSliceData` on `!slice`. When
`fetchAppliedChart` populated slice in lockstep, the gate skips. When
the user selects a different chart from the dropdown
(`handleSelectValue`), `slice` is now cleared to null first, so the
watcher fires and fetches correctly.
2. **Dead `useCallback`.** `renderChartHeader` (empty deps) only built
JSX from its arguments and was called inline as `renderChartHeader(…)`
— neither the produced node nor the function identity was observed by
a memoized consumer, so the useCallback was overhead with no benefit.
Inline as a plain helper named `buildChartHeader`.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Master's class version passed the full <ControlHeader> as modalTitle via
`as any` to bypass ModalTrigger's `modalTitle?: string` typing. The FC
conversion narrowed that to `String(label || '')`, which dropped the
description tooltip, validation badges, and warning icons from the
"Edit X in modal" header.
- Widen `ModalTrigger.modalTitle` to `ReactNode` (the inner `Modal`'s
`title` already accepts ReactNode; only `name` needs to stay a string
for data-test/telemetry, so we pass `name` only when modalTitle is a
string).
- Replace TextAreaControl's 12-prop destructure-and-cast of restProps
with `<ControlHeader name={name} {...restProps} />`, matching the
spread pattern used by ViewportControl elsewhere in this PR.
- Pass `controlHeader` (now equivalent to master's behavior) back to
ModalTrigger.
Also: restore the SupersetClient.get spy in DatasourceControl.test.tsx's
afterAll so the module-scope spy doesn't leak into other test files in
the same Jest worker (per the Copilot bot suggestion).
Per @sadpandajoe's two review comments on TextAreaControl and the
Copilot bot's mock-restoration note on DatasourceControl.test.tsx.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When copyNode is not a valid React element, CopyToClipboard wraps it in a
span with role="button" but the previous implementation had no onKeyDown
handler and tabIndex was undefined (so the span wasn't even focusable).
Non-mouse users couldn't trigger the copy.
- Add onKeyDown that triggers onClick on Enter or Space (with
preventDefault to suppress space-scroll).
- Default tabIndex to 0 when enabled so the span is in tab order.
- Add a regression test that focuses the wrapper and asserts Enter
triggers onCopyEnd.
Per the Copilot bot suggestion on the FC conversion PR.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Each of these was a class extending PureComponent before the FC
conversion in this PR; without an explicit memo() the components no
longer skip re-render on shallow-equal props. They're rendered many
times across the explore control panel, so the regression matters.
- SelectControl
- AnnotationLayerControl (wrapped inside the connect HOC)
- AdhocFilterPopoverTrigger
- FixedOrMetricControl
- MetricsControl
Per @sadpandajoe's review comment on this PR.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two changes in response to review feedback on the FC conversion:
1. Drop three `expect(true).toBe(true)` placeholder tests in
`SaveModal.test.tsx` (`dispatches removeChartState ... - placeholder`,
`onDashboardChange triggers tabs load ... - placeholder`,
`onTabChange correctly updates selectedTab - placeholder`). They were
left behind from the class-component version and assert nothing —
net-negative as regression guards. They'll be re-added in a follow-up
once the FC-shaped versions are wired up.
2. Add a regression test in `CopyToClipboard.test.tsx` that renders the
component with a string `copyNode` and with a number `copyNode`,
covering the `cloneElement`-on-non-element crash that prompted the
`isValidElement` guard in `src/components/CopyToClipboard/index.tsx`.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CollectionControl was converted to a function component in #38563 (React 18)
and further refined in #39862 (stable keyless-item ids via WeakMap). This
PR's own conversion predated both and, if kept, would re-introduce the
removed react-sortable-hoc dependency and lose the keyless-item id fix.
Restore master's version so the React 18 + #39862 work is preserved.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The function-component SaveModal typed onDashboardChange as taking
`{ label; value } | null`, which TS2322-rejected against AsyncSelect's
onChange signature — the antd callback supplies the broader
`SelectValue` (which includes `undefined`).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
userEvent.click is asynchronous in @testing-library/user-event v14+;
unawaited calls left the downstream getByTestId assertion to race the
modal render. Awaits both clicks in the Edit-dataset test so the modal
is guaranteed open before the assertion runs.
Addresses codeant-ai review on PR #39461.
partitionColumn was only set when table metadata returned exactly one
partition key, but never reset when switching to a datasource with zero
or 2+ partition keys. Stale state leaked across datasource changes and
the 'latest partition' operator appeared incorrectly. Reset to null at
the top of the effect so only single-partition-key datasources end up
with a value.
Addresses codeant-ai review on PR #39461.
Two codeant-ai findings on the function-component SaveModal:
1. onDashboardChange was typed as receiving a non-null option, but the
Select allows clearing. Clearing sent null and crashed on
newDashboard.value access. Widens the type to `... | null`, sets
dashboard to undefined on clear, and guards the numeric-value branch.
2. The redirect-to-dashboard URL appended `#${selectedTab.value}` for
any truthy value, including the synthetic OUT_OF_TAB sentinel used to
represent "no tab". Skip appending the hash for OUT_OF_TAB, matching
the existing guard used when building selectedTabId.
Addresses codeant-ai review on PR #39461.
Replaces the one-shot hasRunInitialEffect mount guard in TabbedSqlEditors
with a fetchedResultsKeyRef that records the last persisted resultsKey
fetched. The old guard permanently blocked the effect once it ran, so when
activeQueryEditor was unavailable on first render (common when tabHistory
hydrates asynchronously), persisted results were never fetched. Tracking
resultsKey lets the effect retry when it resolves and dedupes repeats.
Addresses codeant-ai review on PR #39461.
copyNode is typed ReactNode but was passed unconditionally to
cloneElement. Non-element values (strings, numbers) would crash at
runtime. Guard with isValidElement and wrap non-elements in a styled
span.
The class component passed { saveAction: this.state.action } as the
second arg to history.replace after save. The function component
conversion dropped that argument. ExploreViewContainer reads
history.location.state.saveAction to trigger chart reload, which
puts the chart in 'loading' status and disables query-save-button.
Restores the argument to fix the `Cross-referenced dashboards` cypress
test which asserts the button becomes disabled after save.
