Draft
Conversation
Contributor
mujibishola
commented
Feb 7, 2026
mujibishola
commented
- Title: feat(ui): add BrandedLoader flag and branded spinner support
- Add FeatureFlag.BrandedLoader
- Loading uses image prop → theme.brandSpinnerSvg → theme.brandSpinnerUrl → default
- Add BRAND_SPINNER_URL/BRAND_SPINNER_SVG and inject into theme bootstrap
- Tests updated; dev config includes example
…de control; security hardening (action origin allowlist, postMessage restriction); export filename improvements; CSV streaming chunk size config; high-security flag to strip extras.where; docs + tests; add Remita examples verify CLI
…y with core (only when total pages > 1); docs update
feat(table, remita): server search contains default; security + export improvements; configs and docs
… FeatureFlag.BrandedLoader and gate branded spinner logic\n- Loading prefers image prop, then theme.brandSpinnerSvg/Url when flag on\n- Add config keys BRAND_SPINNER_URL/BRAND_SPINNER_SVG and inject into theme\n- Update dev config to demonstrate branded loader (docker/pythonpath_dev)\n- Add/adjust unit tests for branded spinner behavior
Contributor
|
Bito Automatic Review Skipped - Large PR |
github-actions
bot
added
api
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const sp = u.searchParams; | ||
| Object.entries(extraParams || {}).forEach(([k, v]) => { | ||
| if (v === undefined || v === null) return; | ||
| sp.set(k, String(v)); |
Contributor
There was a problem hiding this comment.
Suggestion: The row-number column unconditionally dereferences serverPaginationData.currentPage and .pageSize, which will throw at render time when serverPaginationData is undefined (e.g., in client-side pagination mode or when the prop is omitted). [null pointer]
Severity Level: Critical 🚨
- ❌ Remita table chart crashes when serverPaginationData is undefined.
- ❌ Selection/row-number column unusable without server pagination.
- ⚠️ Affects all default uses with client-side pagination.
Suggested change
| sp.set(k, String(v)); | |
| // Default to per-page index when server pagination is not in use or data is missing | |
| let rowNumber = row.index + 1; | |
| if (serverPagination && serverPaginationData) { | |
| const currentPage = (serverPaginationData as any).currentPage || 0; // Get current page index (0-based) | |
| const size = (serverPaginationData as any).pageSize || 10; // Get current page size | |
| rowNumber = currentPage * size + row.index + 1; // Calculate global row number | |
| } |
Steps of Reproduction ✅
1. Render the Remita table chart using `TableChart` from
`superset-frontend/plugins/plugin-chart-remita-table/src/TableChart.tsx` (this is the
default React component exported at the bottom of the file) with `selection_enabled` left
at its default `true` (see props destructuring around lines 200–230 where
`selection_enabled = true` is set).
2. Provide props such that `serverPagination` is `false` or omitted (its default is
`false` in the same destructuring) and do not pass a `serverPaginationData` object (it is
not given a default value and may be `undefined` when server-side pagination is not used).
3. During render, `columnsWithSelection` is built (see its definition later in the file)
and adds the selection/row-number column whose `Cell` implementation starts at
approximately line 870; React calls this `Cell` for each visible row.
4. Inside that `Cell`, the code `const currentPage = serverPaginationData.currentPage ||
0;` and `const pageSize = serverPaginationData.pageSize || 10;` (lines 872–873) executes;
because `serverPaginationData` is `undefined` when server pagination is disabled, this
dereference throws `TypeError: Cannot read properties of undefined (reading
'currentPage')`, preventing the table from rendering whenever the selection/row-number
column is present. This is inconsistent with the rest of the component, which guards
`serverPaginationData` usage with optional chaining or `(serverPaginationData as any) ||
{}`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/TableChart.tsx
**Line:** 872:872
**Comment:**
*Null Pointer: The row-number column unconditionally dereferences `serverPaginationData.currentPage` and `.pageSize`, which will throw at render time when `serverPaginationData` is undefined (e.g., in client-side pagination mode or when the prop is omitted).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+195
to
+207
| const normalizeDateInput = (s: any) => { | ||
| try { | ||
| const str = String(s || '').trim(); | ||
| if (!str) return ''; | ||
| const m = str.match(/^(\d{4}-\d{2}-\d{2})(?:[T\s](\d{2}):(\d{2})(?::(\d{2}))?)?/); | ||
| if (!m) return ''; | ||
| const date = m[1]; | ||
| const hh = m[2] ?? '00'; | ||
| const mm = m[3] ?? '00'; | ||
| const ss = m[4] ?? '00'; | ||
| return `${date} ${hh}:${mm}:${ss}`; | ||
| } catch { return ''; } | ||
| }; |
Contributor
There was a problem hiding this comment.
Suggestion: Inside the filter panel, normalizeDateInput is declared as a const arrow function but is called earlier in the same function body, which will throw a runtime ReferenceError due to the temporal dead zone; converting it to a function declaration (which is hoisted) fixes this. [logic error]
Severity Level: Critical 🚨
- ❌ Temporal advanced filters crash when opening filter dropdown.
- ❌ Affected chart header column React subtree fails rendering.
- ⚠️ Users cannot filter by date on Remita table charts.
Suggested change
| const normalizeDateInput = (s: any) => { | |
| try { | |
| const str = String(s || '').trim(); | |
| if (!str) return ''; | |
| const m = str.match(/^(\d{4}-\d{2}-\d{2})(?:[T\s](\d{2}):(\d{2})(?::(\d{2}))?)?/); | |
| if (!m) return ''; | |
| const date = m[1]; | |
| const hh = m[2] ?? '00'; | |
| const mm = m[3] ?? '00'; | |
| const ss = m[4] ?? '00'; | |
| return `${date} ${hh}:${mm}:${ss}`; | |
| } catch { return ''; } | |
| }; | |
| function normalizeDateInput(s: any) { | |
| try { | |
| const str = String(s || '').trim(); | |
| if (!str) return ''; | |
| const m = str.match(/^(\d{4}-\d{2}-\d{2})(?:[T\s](\d{2}):(\d{2})(?::(\d{2}))?)?/); | |
| if (!m) return ''; | |
| const date = m[1]; | |
| const hh = m[2] ?? '00'; | |
| const mm = m[3] ?? '00'; | |
| const ss = m[4] ?? '00'; | |
| return `${date} ${hh}:${mm}:${ss}`; | |
| } catch { return ''; } | |
| } |
Steps of Reproduction ✅
1. Load a dashboard using the Remita table chart so that `HeaderCell` from
`superset-frontend/plugins/plugin-chart-remita-table/src/TableChart/components/HeaderCell.tsx`
is rendered for each column.
2. Ensure a temporal column is present where `isTemporal` is passed as `true` and
`enableAdvancedColumnFilters` is enabled for that column (these props are destructured at
the top of `HeaderCell`).
3. Click the filter icon for that temporal column to open the advanced filter dropdown.
This triggers the `Dropdown` with `dropdownRender` that calls `renderFilterPanel()` (in
the same file, around lines 120–220 in the final state).
4. Inside `renderFilterPanel`, the code at line `179 const s1 = isTemporal ?
normalizeDateInput(v1) : '';` is executed before the `const normalizeDateInput = (s: any)
=> { ... }` definition at lines 195–207. Because `normalizeDateInput` is a `const` in the
same scope and is still in the ECMAScript temporal dead zone, accessing it throws a
`ReferenceError`, causing the advanced filter panel render to fail for temporal columns.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/TableChart/components/HeaderCell.tsx
**Line:** 195:207
**Comment:**
*Logic Error: Inside the filter panel, `normalizeDateInput` is declared as a `const` arrow function but is called earlier in the same function body, which will throw a runtime ReferenceError due to the temporal dead zone; converting it to a function declaration (which is hoisted) fixes this.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| value: DataRecordValue, | ||
| formatter: TimeFormatter, | ||
| ): DateWithFormatter { | ||
| // Get or create the nested map for this formatter |
Contributor
There was a problem hiding this comment.
Suggestion: When a temporal column has no time formatter configured, formatter can be falsy and is still passed into dateFormatterCache.get and .set, which will throw TypeError: Invalid value used as weak map key at runtime because WeakMap keys must be non-null objects. Add a guard to fall back to a safe formatter (e.g., String) and skip caching when no formatter is available. [logic error]
Severity Level: Critical 🚨
- ❌ Remita Table chart crashes for some temporal column setups.
- ❌ Affected dashboards show JS error instead of table.
Suggested change
| // Get or create the nested map for this formatter | |
| // If no formatter is provided, fall back to plain string formatting without caching | |
| if (!formatter) { | |
| return new DateWithFormatter(value, { | |
| formatter: String as unknown as TimeFormatter, | |
| }); | |
| } | |
Steps of Reproduction ✅
1. Render any Remita Table chart (which uses `transformProps`) with a temporal column in
the query result; Superset passes `queriesData[0].coltypes` including
`GenericDataType.Temporal` for that column to `processColumns` in
`superset-frontend/plugins/plugin-chart-remita-table/src/transformProps.ts`.
2. Ensure for that temporal column that `rawFormData.table_timestamp_format`,
`datasource.columnFormats[key]`, and `column_config[key].d3TimeFormat` are all undefined,
so in `processColumns` (same file, around the formatter construction) `isTime` is true but
no `timeFormat` is chosen and `formatter` remains `undefined` on the resulting
`DataColumnMeta`.
3. `transformProps` calls `processColumns(chartProps)` and then
`processDataRecords(baseQuery?.data, columns)`; inside `processDataRecords` (same file)
temporal columns are collected into `timeColumns`, and for each such column it calls
`getCachedDateWithFormatter(x[key], formatter as TimeFormatter)`.
4. For the temporal column whose `formatter` is `undefined`, `getCachedDateWithFormatter`
(lines 108–145 in this file) executes `dateFormatterCache.get(formatter)` and
`dateFormatterCache.set(formatter, valueCache)` with `formatter === undefined`, causing
`TypeError: Invalid value used as weak map key` at runtime and preventing the Remita Table
chart from rendering.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/transformProps.ts
**Line:** 112:112
**Comment:**
*Logic Error: When a temporal column has no time formatter configured, `formatter` can be falsy and is still passed into `dateFormatterCache.get` and `.set`, which will throw `TypeError: Invalid value used as weak map key` at runtime because WeakMap keys must be non-null objects. Add a guard to fall back to a safe formatter (e.g., `String`) and skip caching when no formatter is available.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+627
to
+635
| ? ColorSchemeEnum.Red | ||
| : ColorSchemeEnum.Green; | ||
| const backgroundColor = | ||
| colorOption === ColorSchemeEnum.Green | ||
| ? `rgba(${isPositive ? '0,150,0' : '150,0,0'},0.2)` | ||
| : `rgba(${isPositive ? '150,0,0' : '0,150,0'},0.2)`; | ||
|
|
||
| return { arrow, arrowColor, backgroundColor }; | ||
| }; |
Contributor
There was a problem hiding this comment.
Suggestion: Basic comparison color formatters are computed whenever comparison_color_enabled is true, even if no time comparison is configured, causing metrics to be compared against a non-existent comparison column (effectively zero) and producing misleading arrows/backgrounds; restrict these computations to when time comparison is actually in use. [logic error]
Severity Level: Major ⚠️
- ⚠️ Misleading up/down arrows without any time comparison enabled.
- ⚠️ Comparison coloring misrepresents metric changes versus nonexistent baseline.
Suggested change
| ? ColorSchemeEnum.Red | |
| : ColorSchemeEnum.Green; | |
| const backgroundColor = | |
| colorOption === ColorSchemeEnum.Green | |
| ? `rgba(${isPositive ? '0,150,0' : '150,0,0'},0.2)` | |
| : `rgba(${isPositive ? '150,0,0' : '0,150,0'},0.2)`; | |
| return { arrow, arrowColor, backgroundColor }; | |
| }; | |
| comparisonColorEnabled && isUsingTimeComparison | |
| ? getBasicColorFormatter(baseQuery?.data, columns) | |
| : undefined; | |
| const columnColorFormatters = | |
| getColorFormatters(conditionalFormatting, passedData) ?? | |
| defaultColorFormatters; | |
| const basicColorColumnFormatters = | |
| comparisonColorEnabled && isUsingTimeComparison | |
| ? getBasicColorFormatterForColumn( | |
| baseQuery?.data, | |
| columns, | |
| conditionalFormatting, | |
| ) | |
| : undefined; |
Steps of Reproduction ✅
1. Configure a Remita Table chart where `formData.comparison_color_enabled` is true but no
time comparison is active, e.g., `formData.time_compare` is empty or
`formData.comparison_type` is not `ComparisonType.Values`, so `isUsingTimeComparison` is
false in `transformProps`
(`superset-frontend/plugins/plugin-chart-remita-table/src/transformProps.ts`).
2. In `transformProps`, the time-comparison setup logic leaves `timeOffsets` as an empty
array and `comparisonSuffix` as an empty string when `isUsingTimeComparison` is false,
while `passedData` remains the base query data.
3. Despite there being no comparison data, the current code unconditionally computes
`basicColorFormatters = comparisonColorEnabled && getBasicColorFormatter(baseQuery?.data,
columns);`, so `getBasicColorFormatter` executes with `timeOffsets` empty.
4. Inside `getBasicColorFormatter` (same file), the comparison value is read from keys
like ``${origCol.key}__${ensureIsArray(timeOffsets)[0]}``; with `timeOffsets` empty this
becomes `origCol.key__undefined`, which is not present in `originalData`, so it defaults
to 0. Metrics are therefore always compared against 0, producing arrows and background
colors that imply 100% changes even though no time comparison was configured, misleading
users.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/transformProps.ts
**Line:** 627:635
**Comment:**
*Logic Error: Basic comparison color formatters are computed whenever `comparison_color_enabled` is true, even if no time comparison is configured, causing metrics to be compared against a non-existent comparison column (effectively zero) and producing misleading arrows/backgrounds; restrict these computations to when time comparison is actually in use.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| if (selectedColumns) { | ||
| selectedColumns.forEach(col => { | ||
| if (col?.column?.includes(origCol.key)) { | ||
| const { arrow, arrowColor, backgroundColor } = |
Contributor
There was a problem hiding this comment.
Suggestion: filterState is destructured directly from props and may be undefined, so accessing filterState.filters in the returned props will throw a runtime error when no filter state is provided; instead, use optional chaining and default to an empty object. [null pointer]
Severity Level: Critical 🚨
- ❌ Remita Table chart can crash when no filterState provided.
- ⚠️ Storybook or tests without filters may intermittently fail.
Suggested change
| const { arrow, arrowColor, backgroundColor } = | |
| filters: filterState?.filters || {}, |
Steps of Reproduction ✅
1. Use the Remita Table chart via `SuperChart`, for example through the Storybook helper
in
`superset-frontend/packages/superset-ui-demo/storybook/shared/components/createQuery.story.tsx`,
which renders `<SuperChart>` without explicitly providing a `filterState` prop.
2. `SuperChart` constructs `chartProps` and forwards them to the Remita Table
`transformProps` function at
`superset-frontend/plugins/plugin-chart-remita-table/src/transformProps.ts`, where
`filterState` is destructured from `chartProps` (`const { filterState, ... } = chartProps
as any;`). In scenarios without native filters, `filterState` can be `undefined`.
3. Within `transformProps`, most usages of `filterState` already guard for undefined
(e.g., `filterState?.filters` when building `nativeFilters` and `dashboardQueryParams`),
indicating it is expected to be optional.
4. At the end of `transformProps` (around line 673), the returned props object includes
`filters: filterState.filters,` which unconditionally reads `.filters` on `filterState`.
When `filterState` is `undefined`, this throws `TypeError: Cannot read properties of
undefined (reading 'filters')`, causing the Remita Table chart render to fail.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/transformProps.ts
**Line:** 673:673
**Comment:**
*Null Pointer: `filterState` is destructured directly from props and may be undefined, so accessing `filterState.filters` in the returned props will throw a runtime error when no filter state is provided; instead, use optional chaining and default to an empty object.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| ), | ||
| ], | ||
| validationDependancies: ['server_pagination'], | ||
| default: 10000, |
Contributor
There was a problem hiding this comment.
Suggestion: The row limit control's validation dependency key is misspelled (validationDependancies instead of validationDependencies), so changing the server pagination flag will not re-trigger row limit validation, leaving stale validation state and potentially allowing invalid combinations of settings to slip through. [logic error]
Severity Level: Major ⚠️
- ⚠️ Remita Table row limit not revalidated on pagination toggle.
- ⚠️ Invalid row_limit/server_pagination combos can be saved silently.
- ⚠️ Behavior diverges from standard Table chart UX/validation.
Suggested change
| default: 10000, | |
| validationDependencies: ['server_pagination'], |
Steps of Reproduction ✅
1. Note the correct pattern for validation dependencies in the existing Table chart: in
`superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx` the `row_limit`
control config uses `validationDependencies: ['server_pagination']` together with
`validateServerPagination(...)` (see around lines 211–233 in the provided context). This
is the API expected by `ControlPanelsContainer` in `@superset-ui/chart-controls`.
2. In this PR's new Remita Table plugin control panel, open
`superset-frontend/plugins/plugin-chart-remita-table/src/controlPanel.tsx` and locate the
`row_limit` control inside the "Query" section (lines 388–421 in the diff). The config
uses the same validator stack, including `validateServerPagination(...)`, but the
dependency key is spelled `validationDependancies` instead of `validationDependencies`.
3. When the Explore UI renders this control panel (same mechanism as the existing Table
plugin, which imports its `config` from `plugin-chart-table/src/controlPanel.tsx`), the
form engine from `@superset-ui/chart-controls` will read `validators` and (for the Table
chart) `validationDependencies`. Because `validationDependancies` is not a recognized key,
the Remita Table `row_limit` field is only revalidated when its own value changes, not
when the `server_pagination` control changes.
4. In a running Superset instance with this PR, go to Explore, choose a dataset, and
select the Remita Table visualization (which uses `config` from
`plugin-chart-remita-table/src/controlPanel.tsx`). Set an extreme `row_limit` value (e.g.,
via JSON config or previous saved state) and then toggle the "Server pagination" checkbox
on and off. Observe that, unlike the built-in Table chart, changing `server_pagination`
does not trigger revalidation or error updates on `row_limit`, allowing combinations that
`validateServerPagination` is meant to prevent. This behavioral discrepancy is caused
purely by the misspelled `validationDependancies` key.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/controlPanel.tsx
**Line:** 413:413
**Comment:**
*Logic Error: The row limit control's validation dependency key is misspelled (`validationDependancies` instead of `validationDependencies`), so changing the server pagination flag will not re-trigger row limit validation, leaving stale validation state and potentially allowing invalid combinations of settings to slip through.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+250
to
+252
| try { | ||
| return Number(initialWidth) || (wrapperRef.current?.clientWidth || 0); | ||
| } catch { return 0; } |
Contributor
There was a problem hiding this comment.
Suggestion: The memoized containerWidthMemo never updates when initialWidth is a non-numeric value like the default '100%', because the dependency list ignores wrapperRef, so it will stay 0 and break any sizing logic that relies on the container width; include the ref (or its current value) in the dependencies and handle NaN from Number(initialWidth) explicitly so the memo recomputes with the actual client width after mount. [logic error]
Severity Level: Major ⚠️
- ⚠️ Header renderers receive containerWidth 0 with percentage widths.
- ⚠️ Column resize or layout logic may miscalculate table widths.
Suggested change
| try { | |
| return Number(initialWidth) || (wrapperRef.current?.clientWidth || 0); | |
| } catch { return 0; } | |
| const numericWidth = | |
| typeof initialWidth === 'number' ? initialWidth : Number(initialWidth); | |
| if (Number.isFinite(numericWidth) && numericWidth > 0) { | |
| return numericWidth; | |
| } | |
| return wrapperRef.current?.clientWidth || 0; | |
| } catch { | |
| return 0; | |
| } | |
| }, [initialWidth, wrapperRef.current]); |
Steps of Reproduction ✅
1. Render the `DataTable` component from
`superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/DataTable.tsx` without
a `width` prop, so it uses the default `width: initialWidth = '100%'` defined in the
function signature near the top of the file.
2. On the initial render, React executes the `useMemo` at lines 247–252 to compute
`containerWidthMemo`; `Number(initialWidth)` evaluates to `NaN` for `'100%'` and
`wrapperRef.current` is still `null` because the `ref={wrapperRef}` on the root `<div>`
(at the bottom of the file) has not been attached yet, so the memoized value becomes `0`.
3. Because the `useMemo` dependency array only contains `initialWidth`, which remains the
string `'100%'` for the lifetime of the component in this usage, `containerWidthMemo` is
never recomputed after mount and stays `0` even though `wrapperRef.current?.clientWidth`
is now a non-zero DOM width.
4. Later in the same render cycle, header cells are rendered via `column.render('Header',
{ ..., containerWidth: containerWidthMemo })` in the header mapping (around lines
553–566); any header implementation that uses the injected `containerWidth` to compute
column widths or resize handles will receive `0` instead of the actual container width,
leading to incorrect sizing behavior for those headers.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/DataTable.tsx
**Line:** 250:252
**Comment:**
*Logic Error: The memoized `containerWidthMemo` never updates when `initialWidth` is a non-numeric value like the default `'100%'`, because the dependency list ignores `wrapperRef`, so it will stay `0` and break any sizing logic that relies on the container width; include the ref (or its current value) in the dependencies and handle NaN from `Number(initialWidth)` explicitly so the memo recomputes with the actual client width after mount.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+410
to
+412
| const noResults = | ||
| typeof noResultsText === 'function' | ||
| ? noResultsText(filterValue as string) |
Contributor
There was a problem hiding this comment.
Suggestion: When column-specific search is enabled, filterValue can be an object { text, column }, but noResultsText is called with filterValue as string, causing it to receive "[object Object]" instead of the actual search text; normalize filterValue to a string by extracting its text field before passing it to noResultsText. [logic error]
Severity Level: Major ⚠️
- ⚠️ Custom no-results messages show "[object Object]" search text.
- ⚠️ Users see confusing feedback when filters return zero rows.
Suggested change
| const noResults = | |
| typeof noResultsText === 'function' | |
| ? noResultsText(filterValue as string) | |
| const normalizedFilterValue = | |
| typeof filterValue === 'object' && | |
| filterValue !== null && | |
| (filterValue as any).text !== undefined | |
| ? String((filterValue as any).text) | |
| : String(filterValue ?? ''); | |
| const noResults = | |
| typeof noResultsText === 'function' | |
| ? noResultsText(normalizedFilterValue) |
Steps of Reproduction ✅
1. Use the `DataTable` component (file
`superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/DataTable.tsx`) with
`showSearchColumnSelector` set to `true`, a non-empty `searchOptions` array, and a custom
`noResults` function prop so that `noResultsText` is a function (definition of `noResults`
at lines 410–413).
2. In this configuration, when the user selects a search column and types into the global
search, the `SearchSelectDropdown` `onChange` handler (around lines 334–352) and the
`GlobalFilter` `setGlobalFilter` wrapper both call `setGlobalFilter` with an object `{
text, column }`, so React Table's `globalFilter` state (`filterValue` in the `useTable`
state destructure) becomes that object.
3. When filtering results in zero rows, `DataTable` computes `noResults` at lines 410–413;
because `noResultsText` is a function, the current code calls `noResultsText(filterValue
as string)`.
4. Since `filterValue` is an object `{ text, column }`, the cast to string yields the
literal string `"[object Object]"`, so the custom `noResults` renderer receives `"[object
Object]"` instead of the actual search text (for example, `"foo"`), producing an incorrect
or confusing no-results message.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/DataTable.tsx
**Line:** 410:412
**Comment:**
*Logic Error: When column-specific search is enabled, `filterValue` can be an object `{ text, column }`, but `noResultsText` is called with `filterValue as string`, causing it to receive `"[object Object]"` instead of the actual search text; normalize `filterValue` to a string by extracting its `text` field before passing it to `noResultsText`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+593
to
+594
| data-action-id={ | ||
| effectiveTableActionsIdColumn >= 0 |
Contributor
There was a problem hiding this comment.
Suggestion: The row data-action-id attribute is computed via row.values[effectiveTableActionsIdColumn], but effectiveTableActionsIdColumn is a numeric index derived from data keys while row.values is keyed by column IDs, so this will typically be undefined or incorrect; index into row.values using the configured column name/ID instead. [logic error]
Severity Level: Critical 🚨
- ⚠️ data-action-id attribute on rows is usually undefined.
- ⚠️ Row-level table actions cannot reliably identify clicked rows.
Suggested change
| data-action-id={ | |
| effectiveTableActionsIdColumn >= 0 | |
| tableActionsIdColumn | |
| ? (row.values as any)[tableActionsIdColumn] |
Steps of Reproduction ✅
1. Render `DataTable` from
`superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/DataTable.tsx` with row
objects that include an identifier field such as `"id"`, and pass
`tableActionsIdColumn="id"` so that row-level actions can target a specific row (prop
defined near the top of the `DataTableProps` interface).
2. Inside `DataTable`, `columnNames` is computed from `Object.keys(data?.[0] || {})`, and
`getColumnIdByName` at lines 500–512 derives `effectiveTableActionsIdColumn` as the
numeric index of `"id"` within that `columnNames` array.
3. When rendering table rows in the `<tbody>` mapping around lines 584–614, each `<tr>`
sets `data-action-id` via the code at lines 592–596:
`row.values[effectiveTableActionsIdColumn]` if `effectiveTableActionsIdColumn >= 0`.
4. React Table's `row.values` is an object keyed by column IDs (typically strings like
`"id"`, `"name"`, etc.), not by numeric indices; when a number is used as a key,
JavaScript coerces it to `"0"`, `"1"`, etc., so
`row.values[effectiveTableActionsIdColumn]` is `undefined` unless there happens to be a
column whose `id` is `"0"`, `"1"`, etc.
5. As a result, even though `tableActionsIdColumn` is correctly configured, the rendered
`<tr>` elements typically have `data-action-id` set to `undefined`, and any consumer that
uses `event.currentTarget.dataset.actionId` to route clicks to `onTableActionClick` cannot
determine which row's action was triggered.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/DataTable.tsx
**Line:** 593:594
**Comment:**
*Logic Error: The row `data-action-id` attribute is computed via `row.values[effectiveTableActionsIdColumn]`, but `effectiveTableActionsIdColumn` is a numeric index derived from data keys while `row.values` is keyed by column IDs, so this will typically be `undefined` or incorrect; index into `row.values` using the configured column name/ID instead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Member
|
Going to convert this to a draft until there is more in the description, conflicts are fixed and pr title is correct. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.