EnxDev commented on PR #39461:
URL: https://github.com/apache/superset/pull/39461#issuecomment-4809476173
## EnxDev's Review Agent โ apache/superset#39461 ยท HEAD 80c8552
**request changes** โ a "convert to function components" chore that silently
drops several real behaviors (semantic-view editing, metric compatibility
gating, table-change column sync, custom-label preservation). The pure
mechanics (state seeding, drag/resize refs, effect cleanup) are otherwise sound.
### ๐ด Functional
- **`DatasourceControl/index.tsx:9552` +
`controls/FilterControl/AdhocFilterEditPopover/index.tsx:~10779`** โ the
refactor deletes `semantic_view` handling: the `SemanticViewEditModal` import +
render are gone (editing a semantic view now opens the plain
`DatasourceModal`), and the Custom SQL tab is now rendered for `semantic_view`
datasources where it was previously hidden. `SemanticViewEditModal.tsx` still
exists in master, so this is a real regression (looks like a stale-base
artifact). Restore the `datasource.type === 'semantic_view'` branches.
- **`controls/MetricControl/AdhocMetricEditPopover/index.tsx:~12005`** โ the
saved-metric `Select` lost both its alphabetical `localeCompare` sort and the
`disabled` gate for incompatible metrics; the `AdhocMetricEditPopoverWithRedux`
wrapper that injected `state.explore.compatibleMetrics` was deleted.
Incompatible saved metrics are now selectable and the list is unsorted. Re-add
the sort and re-inject `compatibleMetrics` (e.g. `useSelector` in the now-FC).
(test: incompatible saved metric option is `disabled`)
- **`controls/MetricControl/AdhocMetricPopoverTrigger.tsx:~12557`** โ the
`GET_CURRENT_LABEL` guard changed from `savedMetricLabel` to `currentLabel` (=
`savedMetricLabel || adhocMetricLabel`). For an adhoc (non-saved) metric with a
user-set custom label, the guard is now truthy and overwrites `title` with
`hasCustomLabel:false`, wiping the custom label on column/aggregate change.
Guard on `savedMetricLabel`, not `currentLabel`. (test: custom-labeled adhoc
metric keeps its label after editing its column)
-
**`components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:~2343`**
โ old `onDatasourcePropChange('table_name', โฆ)` ran `syncMetadata()`; the new
handler only does setState + validate, so changing the physical table (live
call sites `:3436`/`:3643`) no longer refreshes columns from the new table โ
sync is now reachable only via the manual button. Re-add the `table_name` โ
`syncMetadata()` path.
### ๐ก Should-fix
- **`AdhocFilterControl/index.tsx:10103`,
`FixedOrMetricControl/index.tsx:11203`, `SpatialControl.tsx:13970`** โ
`<ControlHeader {...this.props}>` was narrowed to only
`label`/`name`/`description`, dropping `validationErrors`, `hovered`,
`warning`, `renderTrigger` โ validation styling and the description/warning
tooltips are lost. Spread `{...restProps}` as
`TextAreaControl`/`ViewportControl` correctly do in this same PR.
- **`AdhocMetricEditPopover/index.tsx:~11725` (`refreshAceEditor`)** โ
`aceEditorRef.current.editor?.resize?.()` replaced old
`editorRef.current?.resize()`. `editors.EditorHandle` exposes `resize()`
directly, so the new nested `.editor?.resize?.()` silently no-ops and the
editor won't resize on tab switch. Call `aceEditorRef.current?.resize?.()`.
- **`AnnotationLayerControl/index.tsx:~8091`** โ the validation-sync that
lived in `componentDidUpdate` is now a `useEffect` that also fires on mount, so
it can dispatch `setControlValue` at mount the class never did. Skip the first
run with a mounted ref.
- **`AdhocFilterControl/index.tsx:~217`** โ the partition-metadata fetch
effect is keyed on `[datasource]` (object identity); a fresh `datasource` ref
each render re-issues `/table_metadata/extra/`. Key on stable primitives (db
id, catalog, schema, table).
- **`SaveModal.tsx:~335`** โ dashboard-init `componentDidMount` became a
`useEffect` with reactive deps (`user`, `addDangerToast`, `metadata`); if any
changes identity while the modal is open it re-runs init and can overwrite a
dashboard/tab the user just picked. Run once (`[]` + eslint-disable, or a
`hasInitialized` ref).
- **`TextControl/index.tsx:~14595`** โ new unmount cleanup `cancel()`s the
pending debounced `onChange` (the class had no unmount handler); a final edit
within the debounce window is now dropped on unmount. Use `flush()` to preserve
the last keystroke.
### ๐ต Nits
- `DatasourceEditor.tsx:~1126` โ `handleFoldersChange` now filters out
default + empty folders (new logic, new `DEFAULT_*_FOLDER_UUID` imports), not a
mechanical conversion; confirm intended.
- `SpatialControl.tsx:~13734` โ the mount/sync effect lists `onChange` in
its deps, re-dispatching `SET_FIELD_VALUE` on every parent `onChange` identity
change; hold it in a ref or fire only when the computed value changes.
- `TextAreaControl.tsx:~14306` โ plain-textarea path now
`defaultValue={initialValue ?? value}` (was `initialValue` only) and an empty
`tooltipOptions` ({}) no longer wraps the editor in `<Tooltip>` (was always
wrapped); both are silent behavior changes โ verify.
### ๐ Praise
- `CollectionTable`, `AdhocFilterEditPopover`, and `AdhocMetricEditPopover`
drag/resize: instance vars โ `useRef` and listeners โ `useEffect` cleanup are
done correctly โ live drag-start values, no leaked `mousemove`/`mouseup`,
stable `onResize` identity across a drag.
<!-- enxdev-review-agent:80c8552 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 80c8552._
--
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]
