Conversation
mujibishola
commented
Feb 7, 2026
- Title: feat(dashboard): improve Filter Bar auto-apply and progress overlay
- Summary:
- Batch dataMask updates, avoid unchanged dispatches
- Config-driven auto-apply debounce (keep overlay min 400ms)
- Hide Apply when auto-apply flag enabled
- Fix TypeError in getRelatedCharts and highlight styles
- Add unit test for hidden Apply
…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
…\n- Batch dataMask updates and avoid unchanged dispatches\n- Config-driven auto-apply debounce; keep overlay min 400ms\n- Hide Apply button when auto-apply flag is enabled\n- Add tests for Apply hidden in auto-apply mode\n- Fix TypeError in getRelatedCharts and highlight styles (safe guards)
|
Bito Automatic Review Skipped - Large PR |
| const visibleActions = { | ||
| // Split actions: controlled by global showSplitInSliceHeader flag | ||
| dropdown: !showSplitInSliceHeader | ||
| ? splitActions.filter(action => { | ||
| // Check selection-based visibility | ||
| const selectionVisible = action.visibilityCondition === 'all' || | ||
| action.visibilityCondition === 'selected' || | ||
| (action.visibilityCondition === 'unselected' && !hasSelection) || | ||
| !action.visibilityCondition; | ||
|
|
||
| // Check RLS visibility conditions (ALL must pass) | ||
| const rlsVisible = evaluateRlsVisibilityConditions(action.rlsVisibilityConditions); | ||
|
|
||
| return selectionVisible && rlsVisible; | ||
| }) | ||
| : [], | ||
|
|
||
| // Non-split actions: controlled by individual showInSliceHeader property | ||
| buttons: nonSplitActions.filter(action => { | ||
| // Check if should show in slice header | ||
| if (action.showInSliceHeader) return false; | ||
|
|
||
| // Check selection-based visibility | ||
| const selectionVisible = action.visibilityCondition === 'all' || | ||
| action.visibilityCondition === 'selected' || | ||
| (action.visibilityCondition === 'unselected' && !hasSelection) || | ||
| !action.visibilityCondition; |
There was a problem hiding this comment.
Suggestion: The selection-based visibility logic treats the "selected" visibility condition the same as "all", so actions configured to show only when rows are selected will appear even when there is no selection, which can lead to actions being available in invalid contexts. Adjust the visibility check so that "selected" actions are only visible when there is at least one selected row, while keeping "all" and "unselected" behavior unchanged. [logic error]
Severity Level: Major ⚠️
- ⚠️ Selected-only bulk actions visible with no selected rows.
- ⚠️ Users can trigger actions expecting a non-empty selection.| const visibleActions = { | |
| // Split actions: controlled by global showSplitInSliceHeader flag | |
| dropdown: !showSplitInSliceHeader | |
| ? splitActions.filter(action => { | |
| // Check selection-based visibility | |
| const selectionVisible = action.visibilityCondition === 'all' || | |
| action.visibilityCondition === 'selected' || | |
| (action.visibilityCondition === 'unselected' && !hasSelection) || | |
| !action.visibilityCondition; | |
| // Check RLS visibility conditions (ALL must pass) | |
| const rlsVisible = evaluateRlsVisibilityConditions(action.rlsVisibilityConditions); | |
| return selectionVisible && rlsVisible; | |
| }) | |
| : [], | |
| // Non-split actions: controlled by individual showInSliceHeader property | |
| buttons: nonSplitActions.filter(action => { | |
| // Check if should show in slice header | |
| if (action.showInSliceHeader) return false; | |
| // Check selection-based visibility | |
| const selectionVisible = action.visibilityCondition === 'all' || | |
| action.visibilityCondition === 'selected' || | |
| (action.visibilityCondition === 'unselected' && !hasSelection) || | |
| !action.visibilityCondition; | |
| const isSelectionVisible = (visibilityCondition?: string) => { | |
| if (!visibilityCondition || visibilityCondition === 'all') { | |
| return true; | |
| } | |
| if (visibilityCondition === 'selected') { | |
| return hasSelection; | |
| } | |
| if (visibilityCondition === 'unselected') { | |
| return !hasSelection; | |
| } | |
| return true; | |
| }; | |
| const visibleActions = { | |
| // Split actions: controlled by global showSplitInSliceHeader flag | |
| dropdown: !showSplitInSliceHeader | |
| ? splitActions.filter(action => { | |
| // Check selection-based visibility | |
| const selectionVisible = isSelectionVisible(action.visibilityCondition); | |
| // Check RLS visibility conditions (ALL must pass) | |
| const rlsVisible = evaluateRlsVisibilityConditions(action.rlsVisibilityConditions); | |
| return selectionVisible && rlsVisible; | |
| }) | |
| : [], | |
| // Non-split actions: controlled by individual showInSliceHeader property | |
| buttons: nonSplitActions.filter(action => { | |
| // Check if should show in slice header | |
| if (action.showInSliceHeader) return false; | |
| // Check selection-based visibility | |
| const selectionVisible = isSelectionVisible(action.visibilityCondition); |
Steps of Reproduction ✅
1. Render the `BulkActions` component from
`superset-frontend/plugins/plugin-chart-remita-table/src/BulkActions.tsx:149` with
`initialSelectedRows` as an empty `Map()` (no rows selected) and a `BulkAction` in
`actions.nonSplit` or `actions.split` whose `visibilityCondition` is set to `'selected'`.
2. During render, `BulkActions` computes `hasSelection` at `BulkActions.tsx:161` as
`selectedRows?.size > 0`, which evaluates to `false` for an empty `Map()`.
3. The visibility filtering logic at `BulkActions.tsx:170-205` sets `selectionVisible` to
`true` for this action because the condition `action.visibilityCondition === 'selected'`
is used without checking `hasSelection`, so the `'selected'` branch ignores the actual
selection state.
4. As a result, the action configured to show only when rows are selected is still
included in `visibleActions.dropdown`/`visibleActions.buttons` and rendered as a clickable
button in the header even when no rows are selected, exposing an action in an invalid
context. This behavior follows directly from the code and does not depend on any external
callers.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/BulkActions.tsx
**Line:** 173:199
**Comment:**
*Logic Error: The selection-based visibility logic treats the "selected" visibility condition the same as "all", so actions configured to show only when rows are selected will appear even when there is no selection, which can lead to actions being available in invalid contexts. Adjust the visibility check so that "selected" actions are only visible when there is at least one selected row, while keeping "all" and "unselected" behavior unchanged.
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.| const containerWidthMemo = useMemo(() => { | ||
| try { | ||
| return Number(initialWidth) || (wrapperRef.current?.clientWidth || 0); | ||
| } catch { return 0; } | ||
| }, [initialWidth]); |
There was a problem hiding this comment.
Suggestion: The computed containerWidthMemo only re-computes when initialWidth changes and ignores changes to wrapperRef.current, so when initialWidth is a non-numeric value like '100%' it will stay at 0 permanently, breaking any header/cell logic that depends on the actual container width (e.g., column resize). [logic error]
Severity Level: Major ⚠️
- ⚠️ Remita table chart column resize uses container width 0.
- ⚠️ Sticky header layout may misalign due to zero width.| const containerWidthMemo = useMemo(() => { | |
| try { | |
| return Number(initialWidth) || (wrapperRef.current?.clientWidth || 0); | |
| } catch { return 0; } | |
| }, [initialWidth]); | |
| const containerWidthMemo = (() => { | |
| try { | |
| return Number(initialWidth) || (wrapperRef.current?.clientWidth || 0); | |
| } catch { | |
| return 0; | |
| } | |
| })(); |
Steps of Reproduction ✅
1. Render the `DataTable` component from
`superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/DataTable.tsx` without
passing a numeric `width` prop (or omit `width` entirely). Due to the function signature
at lines ~90–115 (`width: initialWidth = '100%'`), `initialWidth` will be the string
`'100%'`.
2. On the initial render, before React assigns `ref`, the memoized `containerWidthMemo` is
computed at lines 248–253: `const containerWidthMemo = useMemo(() => { ... },
[initialWidth]);`. At this moment `wrapperRef` (created at lines ~200–207 and attached to
`<div ref={wrapperRef} ...>` at the bottom JSX) still has `wrapperRef.current === null`.
3. Inside the memo callback (lines 249–252), `Number(initialWidth)` with `initialWidth ===
'100%'` evaluates to `NaN` (falsy), so the expression falls back to
`(wrapperRef.current?.clientWidth || 0)`. Because `wrapperRef.current` is `null` during
this render, this expression evaluates to `0`, so `containerWidthMemo` is set to `0`.
Since the `useMemo` dependency array is `[initialWidth]` and `initialWidth` remains
`'100%'`, `containerWidthMemo` will never recompute, even after the ref is attached and
the container has a non-zero `clientWidth`.
4. When the table header is rendered, at lines ~531–568 in `renderTable`, each column
header is rendered via `column.render('Header', { ..., enableColumnResize, setStickyState,
containerWidth: containerWidthMemo, ... })`. Any header implementation that relies on the
`containerWidth` prop (e.g., for column resize or width calculations) will always receive
`0`, causing width-dependent behaviors (such as proportional column sizing or resize
handles) to be calculated against a zero-width container in every Remita table instance
that uses the default `'100%'` width or a non-numeric string width.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:** 249:253
**Comment:**
*Logic Error: The computed `containerWidthMemo` only re-computes when `initialWidth` changes and ignores changes to `wrapperRef.current`, so when `initialWidth` is a non-numeric value like '100%' it will stay at 0 permanently, breaking any header/cell logic that depends on the actual container width (e.g., column resize).
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.| ).pop() as TrWithTh; | ||
| return headerRows.props.children.length; |
There was a problem hiding this comment.
Suggestion: If the table header has no rows, Children.toArray(...).pop() returns undefined and accessing .props.children.length will throw, so columnCount computation needs to handle an empty header safely. [possible bug]
Severity Level: Major ⚠️
- ❌ Sticky header crashes when `<thead>` has zero rows.
- ⚠️ Remita table chart unusable for misconfigured empty headers.| ).pop() as TrWithTh; | |
| return headerRows.props.children.length; | |
| ).pop() as TrWithTh | undefined; | |
| return headerRows?.props.children.length ?? 0; |
Steps of Reproduction ✅
1. A React table instance uses `useSticky` (exported at `useSticky.tsx:419-450`), which
registers `useInstance` with react-table hooks so that the component tree eventually calls
`wrapStickyTable` and renders `<StickyWrap>` with the provided `table` element.
2. The `table` element passed into `<StickyWrap>` (`useSticky.tsx:120-127`) has a
`<thead>` child (`thead` is set in the `Children.forEach` loop at
`useSticky.tsx:135-145`), but that `<thead>` renders with no header rows (no `<tr>`
children) in `thead.props.children`.
3. During render of `<StickyWrap>`, the memoized `columnCount` computation at
`useSticky.tsx:152-157` executes: `Children.toArray(thead?.props.children)` returns an
empty array, and `.pop()` at line 154 produces `undefined` for `headerRows`.
4. The next line (`headerRows.props.children.length`) attempts to read `props` from
`undefined`, throwing a runtime error (`TypeError: Cannot read properties of undefined
(reading 'props')`) which prevents the sticky header from rendering. In the current
DataTable/react-table configuration, header rows are always produced from column
definitions, so having a `<thead>` with no rows is an invalid/misconfigured state, making
this crash unlikely in normal Superset dashboards.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/hooks/useSticky.tsx
**Line:** 155:156
**Comment:**
*Possible Bug: If the table header has no rows, `Children.toArray(...).pop()` returns `undefined` and accessing `.props.children.length` will throw, so `columnCount` computation needs to handle an empty header safely.
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.| const [hasVerticalScroll, hasHorizontalScroll] = needScrollBar({ | ||
| width: maxWidth, | ||
| height: maxHeight - theadHeight - tfootHeight, | ||
| innerHeight: fullTableHeight, | ||
| innerWidth: widths.reduce(sum), |
There was a problem hiding this comment.
Suggestion: Reducing widths without an initial value will throw a runtime error when there are no header cells (empty ths), so the scroll size computation will crash instead of treating the total width as zero. [possible bug]
Severity Level: Major ⚠️
- ❌ Sticky header measurement crashes for malformed header DOM.
- ⚠️ Remita table chart fails when header has zero cells.| const [hasVerticalScroll, hasHorizontalScroll] = needScrollBar({ | |
| width: maxWidth, | |
| height: maxHeight - theadHeight - tfootHeight, | |
| innerHeight: fullTableHeight, | |
| innerWidth: widths.reduce(sum), | |
| const totalWidth = widths.reduce(sum, 0); | |
| const [hasVerticalScroll, hasHorizontalScroll] = needScrollBar({ | |
| width: maxWidth, | |
| height: maxHeight - theadHeight - tfootHeight, | |
| innerHeight: fullTableHeight, | |
| innerWidth: totalWidth, |
Steps of Reproduction ✅
1. A React table using the sticky header hook mounts and renders `<StickyWrap>` in
`useSticky.tsx:114-157` via `wrapStickyTable` returned from `useInstance`
(`useSticky.tsx:360-417`), with a `<thead>` that contains at least one `<tr>` but no
`<th>` cells in the final DOM (so `ths.length === 0` in the effect).
2. On mount, the `useLayoutEffect` in `StickyWrap` (`useSticky.tsx:173-228`) runs and
`theadRef.current` points at the rendered `<thead>`;
`bodyThead.childNodes[bodyThead.childNodes.length - 1].childNodes` (lines 190-191) returns
an empty `NodeListOf<HTMLTableHeaderCellElement>`.
3. The code at `useSticky.tsx:192-194` builds `widths = Array.from(ths).map(...)`,
resulting in `widths` being an empty array (`[]`) because there are no header cells.
4. At `useSticky.tsx:195-201`, `widths.reduce(sum)` is executed without an initial value,
which throws `TypeError: Reduce of empty array with no initial value` at runtime, breaking
sticky table measurement and preventing the Remita table chart from rendering correctly.
In normal Superset table usage, headers are always populated via react-table column
definitions, so constructing a header row with zero cells is an invalid/misconfigured
usage and makes this issue a low-occurrence edge case.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/hooks/useSticky.tsx
**Line:** 195:199
**Comment:**
*Possible Bug: Reducing `widths` without an initial value will throw a runtime error when there are no header cells (empty `ths`), so the scroll size computation will crash instead of treating the total width as zero.
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.| HeaderProps, | ||
| TableFooterProps, | ||
| } from 'react-table'; | ||
| import { DragEvent } from 'react'; |
There was a problem hiding this comment.
Suggestion: The types CSSProperties and MouseEventHandler are referenced in the declaration of the toggle props but are never imported, which will cause TypeScript to report them as unknown types; they should be explicitly imported from react alongside DragEvent. [type error]
Severity Level: Critical 🚨
- ❌ Frontend TypeScript build fails on missing React type names.
- ❌ CI type-check step blocks merging this plugin change.| import { DragEvent } from 'react'; | |
| import { CSSProperties, DragEvent, MouseEventHandler } from 'react'; |
Steps of Reproduction ✅
1. Open
`superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/types/react-table.d.ts`.
2. Observe that `TableSortByToggleProps` is declared at lines ~83-88 using `CSSProperties`
and `MouseEventHandler` types without qualification: `style?: CSSProperties;` and
`onClick?: MouseEventHandler;`.
3. Note that this file only imports `DragEvent` from React at line 45 and does not import
`CSSProperties` or `MouseEventHandler`.
4. Run the TypeScript compiler for the frontend workspace (which loads all `.d.ts` files
under `superset-frontend` including this one); the compiler reports `Cannot find name
'CSSProperties'` and `Cannot find name 'MouseEventHandler'` for the usages in
`TableSortByToggleProps`, because these types are not in the global namespace and are not
imported in this declaration file.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/types/react-table.d.ts
**Line:** 45:45
**Comment:**
*Type Error: The types `CSSProperties` and `MouseEventHandler` are referenced in the declaration of the toggle props but are never imported, which will cause TypeScript to report them as unknown types; they should be explicitly imported from `react` alongside `DragEvent`.
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.| const hasVerticalScroll = innerHeight > height; | ||
| const hasHorizontalScroll = | ||
| innerWidth > width - (hasVerticalScroll ? scrollBarSize : 0); |
There was a problem hiding this comment.
Suggestion: The vertical scrollbar calculation does not account for the height reduction caused by a horizontal scrollbar, so in cases where content width alone forces a horizontal scrollbar, the effective height shrinks but hasVerticalScroll remains false, leading to an incorrect result (horizontal-only when both scrollbars will actually appear). Re-evaluating the vertical scrollbar when a horizontal scrollbar is needed fixes this logic error. [logic error]
Severity Level: Major ⚠️
- ❌ Remita table DataTable may mis-detect vertical scrolling needs.
- ⚠️ Layout logic relying on needScrollBar receives incorrect flags.| const hasVerticalScroll = innerHeight > height; | |
| const hasHorizontalScroll = | |
| innerWidth > width - (hasVerticalScroll ? scrollBarSize : 0); | |
| let hasVerticalScroll = innerHeight > height; | |
| let hasHorizontalScroll = | |
| innerWidth > width - (hasVerticalScroll ? scrollBarSize : 0); | |
| if (hasHorizontalScroll && !hasVerticalScroll) { | |
| hasVerticalScroll = innerHeight > height - scrollBarSize; | |
| } | |
Steps of Reproduction ✅
1. Open
`superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/utils/needScrollBar.ts`
and locate the default-exported function `needScrollBar` at lines 24–38, which returns a
tuple `[hasVerticalScroll, hasHorizontalScroll]` based on `width`, `height`, `innerWidth`,
`innerHeight`, and `scrollBarSize`.
2. From any caller (or a unit test), invoke `needScrollBar` with parameters that force
only horizontal scroll in the current logic but would force both scrollbars in a real
container, e.g.:
- `width = 100`, `height = 100`
- `innerWidth = 120` (content wider than container)
- `innerHeight = 100` (content exactly equal to container height)
- `scrollBarSize = 15`
Example call:
`needScrollBar({ width: 100, height: 100, innerWidth: 120, innerHeight: 100,
scrollBarSize: 15 });`
3. Observe the current implementation at lines 35–38:
- `hasVerticalScroll = innerHeight > height` → `100 > 100` → `false`
- `hasHorizontalScroll = innerWidth > width - (hasVerticalScroll ? scrollBarSize : 0)`
→ `120 > 100 - 0` → `true`
- Function returns `[false, true]`, i.e. "no vertical scroll, horizontal scroll
present".
4. In a real browser layout, a horizontal scrollbar of `scrollBarSize = 15` reduces the
effective visible height from 100px to 85px, so the same `innerHeight = 100` would require
a vertical scrollbar (since `100 > 85`). Any DataTable code using the `[false, true]`
result from `needScrollBar` (file `needScrollBar.ts`) to size or style its scrollable
container will treat the container as horizontally scrollable only, while the actual DOM
will show both scrollbars, leading to incorrect layout assumptions (e.g., misalignment or
unexpected overflow).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/utils/needScrollBar.ts
**Line:** 35:37
**Comment:**
*Logic Error: The vertical scrollbar calculation does not account for the height reduction caused by a horizontal scrollbar, so in cases where content width alone forces a horizontal scrollbar, the effective height shrinks but `hasVerticalScroll` remains false, leading to an incorrect result (horizontal-only when both scrollbars will actually appear). Re-evaluating the vertical scrollbar when a horizontal scrollbar is needed fixes this logic error.
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 (!valueA || typeof valueA !== 'string') { | ||
| return -1; | ||
| } | ||
| if (!valueB || typeof valueB !== 'string') { |
There was a problem hiding this comment.
Suggestion: The use of a generic falsy check (!valueA / !valueB) treats empty strings and zero as "missing" values and always sorts them to the beginning or end, which is inconsistent with how valid values should be ordered and can surprise callers relying on proper string sorting. Replace the falsy checks with explicit null/undefined checks so that valid but falsy values like empty strings are still compared normally. [logic error]
Severity Level: Major ⚠️
- ⚠️ Remita table columns mis-handle empty-string ordering when sorted.
- ⚠️ Inconsistent behavior between null and empty string cell values.| if (!valueA || typeof valueA !== 'string') { | |
| return -1; | |
| } | |
| if (!valueB || typeof valueB !== 'string') { | |
| if (valueA == null || typeof valueA !== 'string') { | |
| return -1; | |
| } | |
| if (valueB == null || typeof valueB !== 'string') { |
Steps of Reproduction ✅
1. Import the comparator from
`superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/utils/sortAlphanumericCaseInsensitive.ts`.
2. Construct two `Row`-like objects where `rowA.values['col'] === ''` (empty string) and
`rowB.values['col'] === 'A'`, matching the `Row<D>` shape expected at lines 22–28.
3. Call `sortAlphanumericCaseInsensitive(rowA, rowB, 'col')` from this file (lines 22–36);
in the body, at line 30, `!valueA` evaluates to `true` for the empty string, so the
function returns `-1` without reaching `localeCompare`.
4. Observe that an empty string, which is a valid string value, is always treated as
"missing" and forced to the beginning of the sort order instead of being compared
lexicographically, which can be seen directly in this function without needing other
callers; non-string falsy values (like `0`) are already filtered by the `typeof !==
'string'` check so the main real-world effect is on empty strings.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/utils/sortAlphanumericCaseInsensitive.ts
**Line:** 30:33
**Comment:**
*Logic Error: The use of a generic falsy check (`!valueA` / `!valueB`) treats empty strings and zero as "missing" values and always sorts them to the beginning or end, which is inconsistent with how valid values should be ordered and can surprise callers relying on proper string sorting. Replace the falsy checks with explicit null/undefined checks so that valid but falsy values like empty strings are still compared normally.
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.| onChange(newValue); | ||
| }; | ||
|
|
||
| return [value, setBoth] as [typeof value, typeof setValue]; |
There was a problem hiding this comment.
Suggestion: The hook's returned setter is typed as React's setState (accepting both a value and an updater function) via typeof setValue, but the actual implementation only supports direct values and will pass any function argument through to the debounced callback, causing the callback to receive a function instead of the updated value and leading to subtle runtime logic errors when consumers use functional updates. [type error]
Severity Level: Major ⚠️
- ⚠️ Remita table hook misbehaves if used with functional updates.
- ⚠️ Setter typing misleads developers, enabling subtle future runtime bugs.| return [value, setBoth] as [typeof value, typeof setValue]; | |
| return [value, setBoth] as [T, (newValue: T) => void]; |
Steps of Reproduction ✅
1. In any React component, import `useAsyncState` from
`superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/utils/useAsyncState.ts:28`
and declare `const [value, setValue] = useAsyncState<number, (v: number) => void>(0,
callbackFn);`.
2. Rely on the inferred setter type from the hook's return at `useAsyncState.ts:50`, which
is `typeof setValue` from React, and call `setValue(prev => prev + 1)` using a functional
update.
3. At runtime, `setBoth` (defined at `useAsyncState.ts:45`) receives the function `prev =>
prev + 1` as `newValue` (since its signature is `(newValue: T)`), stores that function
into React state via `setValue(newValue)`, and passes the same function to the debounced
`onChange(newValue)` callback.
4. The consumer callback `callbackFn` (passed into `useAsyncState` at
`useAsyncState.ts:30`) is now invoked with the function object instead of the expected
numeric value, causing incorrect logic or type errors in any real usage that assumes
`newValue` is a number; this mismatch is only possible because the returned setter type
incorrectly advertises functional updates despite the implementation not supporting them.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/DataTable/utils/useAsyncState.ts
**Line:** 50:50
**Comment:**
*Type Error: The hook's returned setter is typed as React's `setState` (accepting both a value and an updater function) via `typeof setValue`, but the actual implementation only supports direct values and will pass any function argument through to the debounced callback, causing the callback to receive a function instead of the updated value and leading to subtle runtime logic errors when consumers use functional updates.
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.| color: #1a1a1a; /* near black for column menu icon */ | ||
| cursor: pointer; | ||
| line-height: 1; | ||
| user-select: none; | ||
| } | ||
| .dt-ellipsis-button:hover, | ||
| .dt-ellipsis-button:focus { | ||
| background: ${theme.colorBgLayout}; | ||
| border-color: ${theme.colorBorderSecondary}; | ||
| color: #000000; /* black on hover/focus */ | ||
| outline: none; | ||
| } | ||
| .dt-ellipsis-button svg { | ||
| fill-opacity: 1; /* override th svg opacity */ | ||
| } | ||
| .dt-tag-icon { | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| cursor: pointer; | ||
| margin-left: ${theme.marginXXS}px; | ||
| color: inherit; | ||
| } | ||
| .dt-tag-sep { | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| margin: 0 ${theme.marginXXS}px; | ||
| color: ${theme.colorTextTertiary}; | ||
| user-select: none; | ||
| } | ||
|
|
||
| /* Advanced filter popover in header */ | ||
| .dt-filter-icon { | ||
| margin-left: 0; | ||
| color: #1a1a1a; /* near black for advanced filter icon */ | ||
| cursor: pointer; | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| padding: 2px 4px; | ||
| min-width: 12px; | ||
| min-height: 12px; | ||
| border-radius: ${theme.borderRadius}px; | ||
| background: transparent; | ||
| border: 1px solid transparent; | ||
| } | ||
| .dt-filter-icon:hover, | ||
| .dt-filter-icon:focus { | ||
| background: ${theme.colorBgLayout}; | ||
| border-color: ${theme.colorBorderSecondary}; | ||
| color: #000000; /* black on hover/focus */ | ||
| outline: none; | ||
| } | ||
| .dt-filter-icon.active { | ||
| color: #1a1a1a; /* keep near black when active */ | ||
| } | ||
| .dt-filter-icon.active:hover, | ||
| .dt-filter-icon.active:focus { | ||
| color: #000000; /* black on hover when active */ |
There was a problem hiding this comment.
Suggestion: Icon styles for the ellipsis and advanced filter controls hardcode black hex colors instead of using theme tokens, which will make these icons effectively invisible or very low contrast in dark themes or customized themes, breaking readability and theming consistency. Replacing the hardcoded #1a1a1a/#000000 with ${theme.colorText} (or another appropriate theme token) keeps the icons legible across all theme variants. [logic error]
Severity Level: Major ⚠️
- ⚠️ Column menu ellipsis icons low-contrast in dark themes.
- ⚠️ Advanced filter icons hard to see on dark backgrounds.
- ⚠️ Theming consistency broken for this table plugin UI.| color: #1a1a1a; /* near black for column menu icon */ | |
| cursor: pointer; | |
| line-height: 1; | |
| user-select: none; | |
| } | |
| .dt-ellipsis-button:hover, | |
| .dt-ellipsis-button:focus { | |
| background: ${theme.colorBgLayout}; | |
| border-color: ${theme.colorBorderSecondary}; | |
| color: #000000; /* black on hover/focus */ | |
| outline: none; | |
| } | |
| .dt-ellipsis-button svg { | |
| fill-opacity: 1; /* override th svg opacity */ | |
| } | |
| .dt-tag-icon { | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| cursor: pointer; | |
| margin-left: ${theme.marginXXS}px; | |
| color: inherit; | |
| } | |
| .dt-tag-sep { | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| margin: 0 ${theme.marginXXS}px; | |
| color: ${theme.colorTextTertiary}; | |
| user-select: none; | |
| } | |
| /* Advanced filter popover in header */ | |
| .dt-filter-icon { | |
| margin-left: 0; | |
| color: #1a1a1a; /* near black for advanced filter icon */ | |
| cursor: pointer; | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| padding: 2px 4px; | |
| min-width: 12px; | |
| min-height: 12px; | |
| border-radius: ${theme.borderRadius}px; | |
| background: transparent; | |
| border: 1px solid transparent; | |
| } | |
| .dt-filter-icon:hover, | |
| .dt-filter-icon:focus { | |
| background: ${theme.colorBgLayout}; | |
| border-color: ${theme.colorBorderSecondary}; | |
| color: #000000; /* black on hover/focus */ | |
| outline: none; | |
| } | |
| .dt-filter-icon.active { | |
| color: #1a1a1a; /* keep near black when active */ | |
| } | |
| .dt-filter-icon.active:hover, | |
| .dt-filter-icon.active:focus { | |
| color: #000000; /* black on hover when active */ | |
| color: ${theme.colorText}; | |
| cursor: pointer; | |
| line-height: 1; | |
| user-select: none; | |
| } | |
| .dt-ellipsis-button:hover, | |
| .dt-ellipsis-button:focus { | |
| background: ${theme.colorBgLayout}; | |
| border-color: ${theme.colorBorderSecondary}; | |
| color: ${theme.colorText}; | |
| outline: none; | |
| } | |
| .dt-ellipsis-button svg { | |
| fill-opacity: 1; /* override th svg opacity */ | |
| } | |
| .dt-tag-icon { | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| cursor: pointer; | |
| margin-left: ${theme.marginXXS}px; | |
| color: inherit; | |
| } | |
| .dt-tag-sep { | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| margin: 0 ${theme.marginXXS}px; | |
| color: ${theme.colorTextTertiary}; | |
| user-select: none; | |
| } | |
| /* Advanced filter popover in header */ | |
| .dt-filter-icon { | |
| margin-left: 0; | |
| color: ${theme.colorText}; | |
| cursor: pointer; | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| padding: 2px 4px; | |
| min-width: 12px; | |
| min-height: 12px; | |
| border-radius: ${theme.borderRadius}px; | |
| background: transparent; | |
| border: 1px solid transparent; | |
| } | |
| .dt-filter-icon:hover, | |
| .dt-filter-icon:focus { | |
| background: ${theme.colorBgLayout}; | |
| border-color: ${theme.colorBorderSecondary}; | |
| color: ${theme.colorText}; | |
| outline: none; | |
| } | |
| .dt-filter-icon.active { | |
| color: ${theme.colorText}; | |
| } | |
| .dt-filter-icon.active:hover, | |
| .dt-filter-icon.active:focus { | |
| color: ${theme.colorText}; |
Steps of Reproduction ✅
1. Start Superset with this PR code and enable a dark theme so `theme.colorBgBase` and
`theme.colorBgLayout` are dark (see styled wrapper in
`superset-frontend/plugins/plugin-chart-remita-table/src/Styles.tsx:1-23`).
2. Open any dashboard that uses the Remita table plugin so its root `<div>` styled by
`Styles.tsx` is rendered and header ellipsis buttons get the `.dt-ellipsis-button` class
defined at `Styles.tsx:331-347`.
3. Observe the ellipsis / column-menu icon color is hardcoded to `#1a1a1a`/`#000000` while
the background is dark (`theme.colorBgBase`); contrast is much lower than the theme's
`colorText`, making the control difficult to see in dark theme.
4. Open a column with an advanced filter and note the filter icon using `.dt-filter-icon`
from `Styles.tsx:374-405` has the same hardcoded near-black colors, again ignoring
`${theme.colorText}` and leading to inconsistent, low-contrast controls compared to other
themed icons.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-remita-table/src/Styles.tsx
**Line:** 342:401
**Comment:**
*Logic Error: Icon styles for the ellipsis and advanced filter controls hardcode black hex colors instead of using theme tokens, which will make these icons effectively invisible or very low contrast in dark themes or customized themes, breaking readability and theming consistency. Replacing the hardcoded `#1a1a1a`/`#000000` with `${theme.colorText}` (or another appropriate theme token) keeps the icons legible across all theme variants.
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.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |