aminghadersohi commented on PR #41184: URL: https://github.com/apache/superset/pull/41184#issuecomment-5039967241
Following up on the earlier review: CI is currently red on this head (`9a30e5003e88`) in a way that looks like a real regression, not a flake. `sharded-jest-tests (6)` fails 4 assertions in `plugin-chart-pivot-table/test/TableRenderer.test.tsx` (all about the `pvtRowLabelLast` class that marks the last data row before a totals row): `dataRows[dataRows.length - 1]` comes back `undefined`, and the "spanning" row-label header cell isn't found where expected. That test file isn't part of this PR's own diff — it's an existing test that predates the rebase this branch picked up — and it was passing at the SHA this review was originally done against, so something in this PR's rewrite of the row/rollup logic is the likely cause rather than any of the small follow-up commits since (those only touch a control description, an unrelated table-chart test import, and a docstring). My guess, not yet confirmed: `processRecord()`'s rewrite of how rows/subtotals get keyed and slotted (`react-pivottable/utilities.ts`) changed `rowKeys`/`visibleRowCount` in a way that breaks the last-row detection in `TableRenderers.tsx`'s `pvtRowLabelLast` logic. Worth a look before merge — this is separate from the total-value correctness reviewed earlier, which still looks solid. -- 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]
