codeant-ai-for-open-source[bot] commented on code in PR #41957:
URL: https://github.com/apache/superset/pull/41957#discussion_r3563936222
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1073,7 +1073,9 @@ export function TableRenderer(props: TableRendererProps) {
true,
)}
>
- {t('Subtotal')}
+ {t('Subvalue (%(aggregatorName)s)', {
+ aggregatorName: t(aggregatorName),
+ })}
Review Comment:
**Suggestion:** The new label text uses `Subvalue`, which contradicts the
subtotal terminology used by the surrounding code (`pvtSubtotalLabel`) and the
feature intent (subtotal labels). This introduces incorrect user-facing
wording; use `Subtotal (%(aggregatorName)s)` instead. [inconsistent naming]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Suggestion targets wording choice, not functional bug.
- ⚠️ Pivot table behavior unaffected; labels already consistent.
- ⚠️ CSS class name needn’t match human-readable text.
```
</details>
<details>
<summary><b>Steps of Reproduction âś… </b></summary>
```mdx
1. The column subtotal header cell is rendered in `renderColHeaderRow` at
`TableRenderers.tsx:860-1112`; when `attrIdx === colKey.length`, the code at
`1060-1080`
creates a `<th>` with class `pvtSubtotalLabel` and the label `{t('Subvalue
(%(aggregatorName)s)', { aggregatorName: t(aggregatorName) })}` (lines
`1076-1078`).
2. The same `Subvalue` label pattern is used for row subtotals in
`renderTableRow` at
`TableRenderers.tsx:1322-1342`, so subtotal labels are consistent between
row and column
headers within this component.
3. A repository-wide search for `"Subtotal"` in the frontend (`Grep` over
`superset-frontend/**/*.ts*`) shows no other user-facing strings like
`"Subtotal
(%(aggregatorName)s)"`; the only uses of “subtotal” are internal types and
flags such as
`SubtotalOptions` and style selectors `.pvtSubtotalLabel` in
`Styles.ts:69-107`, which
just control visual formatting, not text.
4. Given there is no conflicting `"Subtotal"` label elsewhere in this
visualization and
the PR description’s examples explicitly use `Subvalue (Sum)` / `Subvalue
(Average)` as
the intended copy, this wording appears deliberate rather than a bug;
changing it to
`"Subtotal"` is a product copy decision, not a functional defect.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=912552b42819458da37e799958742ab5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=912552b42819458da37e799958742ab5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx
**Line:** 1076:1078
**Comment:**
*Inconsistent Naming: The new label text uses `Subvalue`, which
contradicts the subtotal terminology used by the surrounding code
(`pvtSubtotalLabel`) and the feature intent (subtotal labels). This introduces
incorrect user-facing wording; use `Subtotal (%(aggregatorName)s)` 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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41957&comment_hash=961cb831f9fdb8a6050d4efd49ca53b6424ff0db879052a7174472959d1c916d&reaction=like'>đź‘Ť</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41957&comment_hash=961cb831f9fdb8a6050d4efd49ca53b6424ff0db879052a7174472959d1c916d&reaction=dislike'>đź‘Ž</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1333,7 +1335,9 @@ export function TableRenderer(props: TableRendererProps) {
true,
)}
>
- {t('Subtotal')}
+ {t('Subvalue (%(aggregatorName)s)', {
+ aggregatorName: t(aggregatorName),
+ })}
Review Comment:
**Suggestion:** This new use of `aggregatorName` is inside a memoized
row-render callback, but that callback’s dependency list does not include
`aggregatorName`. When users switch aggregation (Sum/Average/Count), this label
can keep showing the previous aggregator due to a stale closure. Add
`aggregatorName` to that `useCallback` dependency array so subtotal labels
update correctly. [stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Row subtotal headers show outdated aggregation name after change.
- ⚠️ Users can misread subtotals when aggregator changes.
- ⚠️ Any UI tests on subtotal labels may become flaky.
```
</details>
<details>
<summary><b>Steps of Reproduction âś… </b></summary>
```mdx
1. In `TableRenderers.tsx:327-341`, `TableRenderer` destructures
`aggregatorName` from
props and passes it into several memoized callbacks; row rendering is
handled by
`renderTableRow` defined at `TableRenderers.tsx:1207-1410`.
2. Inside `renderTableRow`, the row subtotal header cell is rendered when
`rowKey.length <
settingsRowAttrs.length` at `TableRenderers.tsx:1322-1342`; the label uses
the new code at
`1338-1340`: `t('Subvalue (%(aggregatorName)s)', { aggregatorName:
t(aggregatorName) })`,
so this callback closes over `aggregatorName`.
3. The `renderTableRow` function itself is memoized with `useCallback` at
`TableRenderers.tsx:1207-1418`, but its dependency array at `1410-1417` is
`[tableOptions,
onContextMenu, toggleRowKey, clickHeaderHandler, rows, collapsedRows]` and
does not
include `aggregatorName`, meaning the callback instance (and its closed-over
`aggregatorName` value) is reused across renders as long as those
dependencies do not
change.
4. When the aggregator is changed from, for example, `"Count"` to `"Sum"`
(as exercised in
tests by `aggregatorName: 'Count'` and `aggregatorName: 'Sum'` in
`buildDefaultProps` at
`test/react-pivottable/tableRenders.test.tsx:62-73` and `211-215`, or via the
`aggregateFunction` control in Storybook at
`src/stories/PivotTable.stories.tsx:31-51`),
`TableRenderer` re-renders with a new `aggregatorName` prop but
`renderTableRow` is not
recreated (its deps are unchanged), so row subtotal header cells still
display the stale
label like `Subvalue (Count)` instead of the updated `Subvalue (Sum)`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dce6b2f901994bd493d138dabdc32d11&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=dce6b2f901994bd493d138dabdc32d11&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx
**Line:** 1338:1340
**Comment:**
*Stale Reference: This new use of `aggregatorName` is inside a memoized
row-render callback, but that callback’s dependency list does not include
`aggregatorName`. When users switch aggregation (Sum/Average/Count), this label
can keep showing the previous aggregator due to a stale closure. Add
`aggregatorName` to that `useCallback` dependency array so subtotal labels
update correctly.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41957&comment_hash=683995f77603cd8331bb0544adee130e7389ff89f97a5d29293c674589c10efd&reaction=like'>đź‘Ť</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41957&comment_hash=683995f77603cd8331bb0544adee130e7389ff89f97a5d29293c674589c10efd&reaction=dislike'>đź‘Ž</a>
--
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]
