Copilot commented on code in PR #43184:
URL: https://github.com/apache/superset/pull/43184#discussion_r3797536512
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:
##########
@@ -103,6 +103,41 @@ const sortTypes = {
alphanumeric: sortAlphanumericCaseInsensitive,
};
+// Prefer a stable identifier from original row data; otherwise use a
deterministic
+// concatenation of visible values (keys sorted so column order changes are
detected).
+function stableRowKey<D extends object>(r: Row<D>): string {
+ const orig = r.original as Record<string, unknown> | undefined;
+ if (orig) {
+ const idLike = orig.id ?? orig.ID ?? orig.key ?? orig.uuid;
+ if (idLike != null) return String(idLike);
+ }
+
+ // Fallback: derive from row.values, but make it stable against column order
changes.
+ const v = r.values as Record<string, unknown>;
+ const keys = Object.keys(v).sort(); // detect column order changes
+ return keys.map(k => String(v[k] ?? '')).join('|');
Review Comment:
The comments around the fallback key generation are contradictory: sorting
`Object.keys(v)` makes the key stable against column-order changes, not a way
to “detect” column order changes. This is misleading for future readers and
suggests the opposite behavior.
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:
##########
@@ -289,6 +324,48 @@ export default typedMemo(function DataTable<D extends
object>({
onFilteredDataChange(rowsRef.current, searchText);
}, [filterValue, onFilteredDataChange, rowSignature]);
+ // Emit filtered rows to parent in client-side mode (debounced via RAF).
+ // These hooks must stay above the "no columns" early return below, otherwise
+ // the hook count changes when the column count crosses zero (see #42978).
+ const isMountedRef = useRef(true);
+ useEffect(() => {
+ isMountedRef.current = true;
+ return () => {
+ isMountedRef.current = false;
+ };
+ }, []);
+
+ const rafRef = useRef<number | null>(null);
+ const lastSigRef = useRef<string>('');
+
+ useEffect(() => {
+ if (serverPagination || typeof onFilteredRowsChange !== 'function') {
+ return;
+ }
+
+ const sig = signatureOfRows(rows);
+
+ if (sig !== lastSigRef.current) {
+ lastSigRef.current = sig;
+ if (rafRef.current != null) {
+ cancelAnimationFrame(rafRef.current);
+ }
+ rafRef.current = requestAnimationFrame(() => {
+ if (isMountedRef.current) {
+ // Only emit originals when the signature truly changed
+ onFilteredRowsChange(rows.map(r => r.original as D));
+ }
+ });
+ }
+
+ return () => {
+ if (rafRef.current != null) {
+ cancelAnimationFrame(rafRef.current);
+ rafRef.current = null;
+ }
+ };
+ }, [rows, serverPagination, onFilteredRowsChange]);
Review Comment:
Because the `onFilteredRowsChange` effect is now above the `columns.length
=== 0` early return, it can emit filtered rows while the component is rendering
the “no columns” placeholder (previously it never ran in that state). If you
want to preserve the prior callback semantics and avoid scheduling RAFs when
there are no visible columns, add a `columns.length` guard inside the effect
and include it in the dependency list.
--
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]
