EnxDev commented on PR #42088:
URL: https://github.com/apache/superset/pull/42088#issuecomment-5339755374
## EnxDev's Review Agent โ apache/superset#42088 ยท HEAD 0d975a4
**request changes** โ all six of @sadpandajoe's 2026-08-12 findings
reproduce at this HEAD (last commit 2026-08-07), and one of them is a
regression the HEAD commit itself introduced.
Context: the three findings from 2026-08-06 (`semantic_view` coercion,
`getCellStyle` source-vs-target, `isEqualColumns` currency inputs) are
**genuinely fixed** โ verified in the code, not just claimed. The 2026-08-12
batch has had no commits since. I re-derived each one independently below
rather than restating it.
### ๐ด Functional
-
**`superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts:247-250,435-436`**
ยท _High_ โ Introduced by `0d975a4` ("Fix memoization"). `ColorFormatters`
entries carry the actual color logic in `getColorFromValue`, a function, and
`JSON.stringify` drops it โ the serialized form is only
`column`/`toAllRow`/`toTextColor`/`columnFormatting`/`objectFormatting`.
Editing a conditional-formatting rule's color scheme, operator, or threshold
without changing its column or scope therefore yields an identical dep string,
`getCommonColProps` is not recreated, and `colDefs` (deps `[stringifiedCols,
getCommonColProps]`, line 479) keeps the old closures โ the table keeps
rendering the **previous** colors until something else rebuilds the columns.
Signature over the serializable source rules (`conditional_formatting` config)
instead of the built formatters? **regression test:** render with one rule,
change only `targetValue`, assert the cell's `backgroundColor` changes
.
- **`superset/migrations/shared/migrate_viz/processors.py:661,886,904`** ยท
_High_ โ `_get_table_chart_time_offsets` has no `isTimeComparison` gate.
`buildQuery.ts:111,119` gates both chart-level shift branches on
`isTimeComparison(formData, baseQueryObject)` โ which requires
`comparison_type` โ `ComparisonType` **and** a non-empty metric-offset map โ
and leaves only the `extra_form_data` override ungated. The migration reads
`time_compare` unconditionally, so a chart carrying stale `time_compare`
(comparison_type cleared, or raw mode where there are no metrics) gets
`time_offsets` persisted into `query_context` that neither runtime chart would
ever send. Raw mode is internally inconsistent about it too: `post_processing:
[]` but `time_offsets` populated. Pass the base query object in and apply the
same gate, leaving the `extra_form_data` override ungated? **regression test:**
migrate a chart with `time_compare` set and no `comparison_type`, assert
`query_context["queries"
][0]["time_offsets"] == []` โ nothing asserts `time_offsets` today
(`SOURCE_FORM_DATA` has neither key).
-
**`superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:498`**
ยท _High_ โ The snapshot keys columns by `column.getColId()`, but for
time-comparison main columns `useColDefs.ts:282-284` strips `Main ` from the
colId while the row keeps the original key. `formatValue.ts`'s `valueGetter` is
the proof: it rebuilds `` `Main ${params.column.getColId()}` `` to read the
data. Since line 521 pushes raw `node.data`, Export Current View emits a
**blank main-metric column**. Retain the data key separately from the grid
column id in the snapshot? **regression test:** time-comparison table โ sort โ
export current view โ assert the main metric column has values.
-
**`superset-frontend/plugins/plugin-chart-ag-grid-table/src/stateConversion.ts:360`**
ยท _High_ โ Returning `{}` when `!serverPagination` fixes the requery loop, but
the dashboard export path builds `exportOwnState` through the same converter
(`src/dashboard/components/gridComponents/Chart/Chart.tsx:568-575`), and
dashboards never consume `clientView` โ only Explore's
`useExploreAdditionalActionsMenu` does. Exporting a client-paginated V2 table
from a dashboard after sorting or filtering now sends the **default** query.
The requery suppression and the state the download needs are two different
concerns; gate at the ownState diff/fold site rather than inside the converter?
**regression test:** dashboard client-mode table โ sort โ export CSV โ assert
the request carries `sortBy`.
-
**`superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:528`**
ยท _Medium_ โ A client filter matching zero rows produces `{rows: [], count:
0}`. The consumer gates on `ownState?.clientView?.rows?.length`
(`useExploreAdditionalActionsMenu/index.tsx:970`), so `0` is falsy and it falls
back to a backend query that knows nothing about the grid filter โ the user
looking at an empty table exports the **entire unfiltered dataset**. Shared
with V1's `clientView` contract rather than newly introduced, but newly
reachable for V2. Handle an empty snapshot on the client path (headers-only
file)? **regression test:** filter to zero rows, export current view, assert no
backend export request is issued.
-
**`superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:390`**
ยท _Medium_ โ The init-skip guard sits *inside* the debounced body, so the
initial `onStateUpdated` and any user sort/filter/column-move within
`SLOW_DEBOUNCE` coalesce into one invocation that takes the
`!hasCapturedInitialGridStateRef.current` branch and returns โ the first user
change is never persisted to chart state. `handleModelUpdated` (line 488) has
the same shape. Mark initialization before debouncing, or track the init
signature outside the debounced closure? **regression test:** fire the init
state event, then a sort inside the debounce window, assert
`onColumnStateChange` fires.
### ๐ก Should-fix
- **`superset/migrations/shared/migrate_viz/base.py:168-169`** โ
`form_data_bak` is deep-copied *after* the synthesized `datasource` is
injected, so `downgrade_slice` restores params carrying a `datasource` key the
original never had. Not a clean revert.
`test_migration_without_datasource_key_in_params` only asserts the upgrade, so
nothing catches it. Take the backup before injecting. Benign at runtime
(`Slice.form_data` synthesizes the same value), but this also changes downgrade
output for the already-shipped processors that share this base.
- **Patch coverage 58.9%, `processors.py` at 12.79% / 75 lines missing** โ
the `time_offsets` gap above sits squarely in that untested region. The
`_build_query` paths deserve assertions on the emitted `query_context`, not
just on `params`.
### ๐ต Nits
- `superset/migrations/shared/migrate_viz/base.py:244,272` โ the `unchanged`
sentinel is dead code: every branch assigns `new_query_context`, so the guard
is always true and the comment describes a "leave untouched" branch that
doesn't exist.
-
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:655`
โ `<div style={gridFlexStyles}>` inline style prop; move to an Emotion
`css`/styled block.
-
`superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx:398`
โ the `if (!visible)` guard assumes `visible` has flushed between AG Grid's
two `contextmenu` events. If both land in one task the guard reads stale
`false` and the menu still toggles closed. A test asserting two rapid `open()`
calls leave it open would pin the assumption down.
### ๐ Praise
- The three 2026-08-06 findings are fixed properly, with real assertions
rather than smoke tests โ `DATASOURCE_TYPE_MAP` +
`test_migration_preserves_semantic_view_datasource_type` asserting `{"id": 7,
"type": "semantic_view"}` is exactly the right guard.
- `upgrade_slice`/`downgrade_slice` deferring every slice mutation until all
steps succeed removes a real half-migrated-slice failure mode, and the "why no
DB migration" rationale is the right call for an `IN DEVELOPMENT` viz type.
<!-- enxdev-review-agent:0d975a4 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 0d975a4._
--
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]
