EnxDev commented on PR #39461: URL: https://github.com/apache/superset/pull/39461#issuecomment-4810969770
## EnxDev's Review Agent — apache/superset#39461 · HEAD 7262a36 **request changes** — the class→function ports are mostly faithful and the earlier bot/reviewer findings are addressed, but a metrics-table sort regression and the AnnotationLayerControl mount path are genuine silent regressions, and two non-refactor UI changes slipped into a "lint cleanup" PR. _Supersedes my [prior review](https://github.com/apache/superset/pull/39461#issuecomment-4809476173) (HEAD 80c8552); re-reviewed against the new commits._ Refactor PR → reviewed for behavioral equivalence, not style. Confirmed the author's earlier fixes are present in this HEAD (SaveModal `save_action` history state + `OUT_OF_TAB` hash guard + dashboard-clear null guard; CopyToClipboard `isValidElement`; TabbedSqlEditors `fetchedResultsKeyRef`; AdhocFilterControl `setPartitionColumn(null)` reset) — not re-reported. The 36-component conversion is otherwise careful: lifecycle ports, cleanup timing, and removed-wrapper prop injection all check out across SqlLab, the metric/filter controls, and ChartCreation/RightMenu. ### 🔴 Functional - **`Datasource/components/DatasourceEditor/DatasourceEditor.tsx` (`sortMetrics` ~L2877 + `renderMetricCollection`) → `CollectionTable/index.tsx:1101`** · _High_ — The metrics table loses the user's column sort on any metrics re-render (e.g. typing in "Search metrics by key or label"). On master `sortMetrics` sorted in place and returned the stable `datasource.metrics` reference, so CollectionTable's `componentDidUpdate` (`collection !== prevProps.collection`) never rebuilt and the sort persisted. The PR changes it to `[...metrics].sort()` (new array every call) and CollectionTable's new `useEffect([propsCollection])` calls `setCollectionArray(...)` in default id-order **without re-applying `sortColumn`/`sort`** → rows jump back to default order while the sort arrow still shows the old column. Fix: pass the already-memoized top-level `sortedMetrics` (`useMemo([datasource.metrics, sortMetrics])`) into the metrics CollectionTable instead of recomputing inside `renderMetricCol lection`, and/or re-apply the active sort in the sync effect. **regression test:** sort metrics by Metric Key, type into the metric search box, assert row order is unchanged. - **`explore/components/controls/AnnotationLayerControl/index.tsx` (~L95–122, the `componentDidUpdate`→sync `useEffect`)** · _Medium_ — `validationErrors` is destructured with **no default** (unlike `annotationError = {}` beside it), and the validation-sync effect now runs on **mount** with **no first-render / prev-props guard** — the old `componentDidUpdate` did neither (skipped mount; had a `prevProps` reference guard). When `validationErrors` is `undefined` at mount (control state not yet validated — `controlUtils.getControlState` returns state without it on the pre-init pass), the branch `!Object.keys(annotationError).length && validationErrors.length` throws `Cannot read properties of undefined (reading 'length')`, caught by Control.tsx's ErrorBoundary → the annotation control renders its error fallback. Even without the throw, it fires an extra `setControlValue` dispatch on mount the class never did. Fix: default `validationErrors = []` and add a mounted-ref gua rd so the sync only runs after first render. **regression test:** render with `validationErrors={undefined}` and `annotationError={}`; assert it doesn't throw and dispatches no `setControlValue` on mount, then update `annotationError` and assert exactly one dispatch. - **`Datasource/components/DatasourceEditor/DatasourceEditor.tsx:3967,4015` — duplicate "Column Settings" heading** · _High_ — Both the Columns and Calculated-columns tabs now render `{renderDefaultColumnSettings()}` (which already renders a **"Default Column Settings"** heading) *and* a new `<DefaultColumnSettingsTitle>{t('Column Settings')}</DefaultColumnSettingsTitle>`. `t('Column Settings')` does not exist on master — verified — so this stacks a second heading and adds a new translatable string. Not a class→FC change. Fix: remove the added heading (or, if intentional, split it out of the lint-cleanup PR). ### 🟡 Should-fix - **`Datasource/components/CollectionTable/index.tsx:1790`** — the sticky-table branch gains a hard `height: 350px`; master's sticky branch is `overflow: auto` only. This forces a fixed 350px scroll viewport on every sticky table (Columns and Calculated-columns). Unrelated to the conversion → drop it or justify separately. - **`Datasource/components/DatasourceEditor/DatasourceEditor.tsx` (`handleFoldersChange` ~L2407)** — master stored the `folders` argument as-is (`setState({ folders, folderCount })` → `onDatasourceChange`). The new handler filters out the default Columns/Metrics folder UUIDs and any childless folders before `setFolders`/`setFolderCount`/`setDatasource`, changing the persisted `datasource.folders` and the live folder badge count vs the class. If intentional, move it out of the refactor; otherwise persist the payload as-is. - **`explore/components/controls/TextControl/index.tsx` (ControlHeader render ~L133)** — old forwarded `<ControlHeader {...this.props} />`; new forwards only 6 explicit props, dropping `warning`/`danger`/`tooltipOnClick`/`rightNode`/`leftNode`/`onClick`. Latent today (no static config sets them), but the contract is narrowed — a TextControl that later gains a `warning`/`danger` will silently stop rendering it. Forward those like `SelectControl` does. - **`explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx`** — `getDerivedStateFromProps` (synchronous, pre-render) became a post-commit `useEffect([propsAdhocMetric])` and the render now reads `state.adhocMetric` where master read `props.adhocMetric`. When the `adhocMetric` prop changes identity in the controlled path (`DndMetricSelect`, popover open), the title/`labelModified` reset lands one commit late (brief stale frame + extra render). Use `useLayoutEffect`, or derive the rendered metric from `propsAdhocMetric`. - **`FixedOrMetricControl/index.tsx:202` & `MetricControl/MetricsControl.tsx:348`** — both wrap the export in `memo()` with a comment claiming they "preserve" a prior PureComponent, but on master `FixedOrMetricControl` was a plain `Component` and `MetricsControl` a plain function (no memo). The PR *adds* shallow-equal re-render skipping that didn't exist — it can swallow a re-render the originals always did on an in-place prop mutation (e.g. `datasource.metrics` mutated without a new reference). Drop `memo()` to match master, or keep it intentionally and fix the misleading comment. - **`explore/components/controls/TextControl/index.tsx` (unmount effect ~L108)** — new cancels the pending debounced `onChange` on unmount (`cancel()`); the old class had no unmount handler, so the last keystroke still committed, and sibling `TextAreaControl` uses `flush()`. An edit made <250ms before unmount (type, then close a popover / switch viz) is now dropped. Use `flush()` instead of `cancel()`. ### Open reviewer threads — still worth closing - @sadpandajoe asked for regression tests on the six bot-fixed behaviors (SaveModal `save_action`/`OUT_OF_TAB`/dashboard-clear, AdhocFilterControl partition reset, TabbedSqlEditors resultsKey, CopyToClipboard non-element) and flagged `SaveModal.test.tsx`'s `expect(true).toBe(true)` placeholder — none of those tests are in this diff yet. The fixes above need the same: each currently lands without a guarding test. - @Copilot: `DatasourceControl.test.tsx` module-scope `SupersetClient.get` spy is no longer restored in `afterEach` — add `jest.restoreAllMocks()` to avoid cross-file mock leakage. <!-- enxdev-review-agent:7262a36 --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 7262a36._ -- 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]
