bito-code-review[bot] commented on code in PR #39658:
URL: https://github.com/apache/superset/pull/39658#discussion_r3881276303
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:
##########
@@ -243,6 +243,13 @@ export default typedMemo(function DataTable<D extends
object>({
},
...tableHooks,
);
+ // Clamp pageIndex when filtered data shrinks below current view (#31403)
+ if (data.length > 0 && pageCount > 0 && pageIndex >= pageCount) {
+ gotoPage(pageCount - 1);
+ } else if (pageCount === 0 && pageIndex !== 0) {
+ gotoPage(0);
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Side effect during render</b></div>
<div id="fix">
`gotoPage()` is called directly in the render body of `DataTable`. In
react-table v7, `gotoPage` triggers a `setState` on the table's internal state,
which violates React's rules and produces the "Cannot update a component while
rendering a different component" warning (introduced in React 16.13). The
established pattern in this codebase wraps `gotoPage` calls in `useEffect` —
see `ListView.tsx` line 359 and `TableView.tsx` line 196. `useEffect` is
already imported in this file (line 28).
</div>
</div>
<small><i>Code Review Run #087cc3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-table/src/utils/formatValue.ts:
##########
@@ -50,11 +50,15 @@ function formatValue(
return [false, 'N/A'];
}
if (formatter) {
- // If formatter is a CurrencyFormatter, pass row context for AUTO mode
- if (formatter instanceof CurrencyFormatter) {
- return [false, formatter(value as number, rowData, currencyColumn)];
+ try {
+ // If formatter is a CurrencyFormatter, pass row context for AUTO mode
+ if (formatter instanceof CurrencyFormatter) {
+ return [false, formatter(value as number, rowData, currencyColumn)];
+ }
+ return [false, formatter(value as number)];
+ } catch (e) {
+ return [false, String(value)];
}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-209: Silent catch swallows formatter errors</b></div>
<div id="fix">
The new `catch (e)` block silently swallows the formatter exception and
returns `String(value)` without going through the
`isProbablyHTML`/`sanitizeHtml` path used elsewhere. Any formatter error
becomes invisible, and an HTML-bearing value that trips the formatter is now
rendered unsanitized. At minimum log `e` (e.g., `console.error('formatValue
formatter failed', e)`); ideally also sanitize HTML-shaped values in the
fallback to avoid bypassing the existing XSS guard.
([CWE-209](https://cwe.mitre.org/data/definitions/209.html))
</div>
</div>
<small><i>Code Review Run #087cc3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
