bito-code-review[bot] commented on code in PR #39658:
URL: https://github.com/apache/superset/pull/39658#discussion_r3891731614
##########
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:
<!-- Bito Reply -->
The implementation correctly addresses the concern by ensuring that the
fallback value is rendered as plain text, which prevents it from bypassing the
existing sanitization guards. The addition of the `try-catch` block with
logging provides visibility into formatter failures, and the unit tests verify
the fallback behavior for both formatter paths.
**superset-frontend/plugins/plugin-chart-table/src/utils/formatValue.ts**
```
} catch (e) {
logging.warn('formatValue formatter failed', e);
return [false, String(value)];
}
```
##########
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:
<!-- Bito Reply -->
The reviewer's suggestion is correct. Calling `gotoPage()` directly in the
render body triggers a state update during the render phase, which violates
React's rules and causes warnings. Wrapping the call in a `useEffect` hook is
the standard pattern in this codebase to ensure state updates occur after the
render cycle.
**superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx**
```
useEffect(() => {
if (data.length > 0 && pageCount > 0 && pageIndex >= pageCount) {
gotoPage(pageCount - 1);
} else if (pageCount === 0 && pageIndex !== 0) {
gotoPage(0);
}
}, [data.length, pageCount, pageIndex, gotoPage]);
```
--
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]
