EnxDev commented on PR #43184:
URL: https://github.com/apache/superset/pull/43184#issuecomment-5339658356
## EnxDev's Review Agent โ apache/superset#43184 ยท HEAD 8de426a
comment โ the hoist is a genuine verbatim move and fixes a real crash; one
unintended emission in the zero-column state, and the new tests don't guard the
emit path.
Verified the fix: every hook now sits above the `columns.length === 0`
return at `DataTable.tsx:421` (last one is the sortBy effect at `:381`), and
the hoisted block is byte-equivalent to the deleted one apart from a comment
reflow and dropping the `as any` casts. No hook bodies, dependency arrays or
emit conditions changed. CI is green.
### ๐ก Should-fix
-
**`superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:341`**
โ the emit effect now runs while the component renders the "no columns"
placeholder, which it never did before. This is reachable outside the crash
path: hiding every column (`config.visible === false`) empties
`visibleColumnsMeta`, so `TableChart.tsx:1432` passes `columns=[]` while `data`
is unchanged โ and react-table builds `rows` from `data`, not from `columns`,
so `rows` is still every data row with `values: {}`. `onFilteredRowsChange`
therefore fires with the full unfiltered dataset and `TableChart` writes it
into `clientView` ownState. Export is not corrupted
(`useExploreAdditionalActionsMenu` requires `clientView.columns.length`, and
`exportColumns` is `[]` here), so the net effect is a redundant dataMask write
carrying a full copy of the result set. Add `columns.length === 0` to the
effect's early return and to its dependency list โ that keeps the semantics
identical to master and m
akes the move a pure move. Copilot asked for the same guard at line 367.
-
**`superset-frontend/plugins/plugin-chart-table/test/DataTable/DataTable.test.tsx`**
โ both tests assert only `not.toThrow()`; nothing exercises
`onFilteredRowsChange`, and codecov's one uncovered line is the RAF callback.
What the hoist actually changes is *when* that effect runs, so that's the
behavior needing a regression guard: stub `requestAnimationFrame`, cross the
boundary, and assert what the callback receives when `columns` is `[]` and
`data` is not.
### ๐ต Nits
- `DataTable.tsx:117` โ `// detect column order changes` contradicts the
block comment three lines above it: sorting the keys makes the fallback key
*stable against* a column reorder, it does not detect one. The line moved into
the diff, so worth correcting while it's here.
- The description's testing section reports "2 failed before, 5 passed
after", but the file ships 2 tests and no other file matches
`plugins/plugin-chart-table/test/DataTable`. Worth correcting so the numbers
can be reproduced.
### Open bot threads
- bito-code-review at line 367 ("Removing this effect breaks
`clientView.rows`") is a false positive โ the effect is moved, not removed.
- Both codeant threads (lines 112 and 349) describe pre-existing behavior in
a block this PR moves verbatim, so neither is a regression from this change.
The first one is real and worth its own issue: `stableRowKey` returns the
id-like field alone, so a result set carrying an `id`/`ID`/`key`/`uuid` column
whose other values change between queries produces the same signature, the emit
is suppressed, and "CSV (Current View)" hands back the stale snapshot.
### ๐ Praise
- Scoped exactly to the bug: the diff is a move plus a module-scope lift,
both boundary directions are covered by tests, and #42978 documents the
reachable path and the oxlint-warn-only reason CI never caught it.
<!-- enxdev-review-agent:8de426a -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 8de426a._
--
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]
