codeant-ai-for-open-source[bot] commented on code in PR #38145:
URL: https://github.com/apache/superset/pull/38145#discussion_r2835635130
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -384,89 +384,87 @@ const processComparisonColumns = (
props: TableChartProps,
comparisonSuffix: string,
) =>
- columns
- .map(col => {
- const {
- datasource: { columnFormats, currencyFormats },
- rawFormData: { column_config: columnConfig = {} },
- } = props;
- const savedFormat = columnFormats?.[col.key];
- const savedCurrency = currencyFormats?.[col.key];
- const originalLabel = col.label;
- if (
- (col.isMetric || col.isPercentMetric) &&
- !col.key.includes(comparisonSuffix) &&
- col.isNumeric
- ) {
- return [
- {
- ...col,
- originalLabel,
- label: t('Main'),
- key: `${t('Main')} ${col.key}`,
- config: getComparisonColConfig(t('Main'), col.key, columnConfig),
- formatter: getComparisonColFormatter(
- t('Main'),
- col,
- columnConfig,
- savedFormat,
- savedCurrency,
- ),
- },
- {
- ...col,
- originalLabel,
- label: `#`,
- key: `# ${col.key}`,
- config: getComparisonColConfig(`#`, col.key, columnConfig),
- formatter: getComparisonColFormatter(
- `#`,
- col,
- columnConfig,
- savedFormat,
- savedCurrency,
- ),
- },
- {
- ...col,
- originalLabel,
- label: `△`,
- key: `△ ${col.key}`,
- config: getComparisonColConfig(`△`, col.key, columnConfig),
- formatter: getComparisonColFormatter(
- `△`,
- col,
- columnConfig,
- savedFormat,
- savedCurrency,
- ),
- },
- {
- ...col,
- originalLabel,
- label: `%`,
- key: `% ${col.key}`,
- config: getComparisonColConfig(`%`, col.key, columnConfig),
- formatter: getComparisonColFormatter(
- `%`,
- col,
- columnConfig,
- savedFormat,
- savedCurrency,
- ),
- },
- ];
- }
- if (
- !col.isMetric &&
- !col.isPercentMetric &&
- !col.key.includes(comparisonSuffix)
- ) {
- return [col];
- }
- return [];
- })
- .flat();
+ columns.flatMap(col => {
+ const {
+ datasource: { columnFormats, currencyFormats },
+ rawFormData: { column_config: columnConfig = {} },
+ } = props;
+ const savedFormat = columnFormats?.[col.key];
+ const savedCurrency = currencyFormats?.[col.key];
+ const originalLabel = col.label;
+ if (
+ (col.isMetric || col.isPercentMetric) &&
+ !col.key.includes(comparisonSuffix) &&
+ col.isNumeric
+ ) {
+ return [
+ {
+ ...col,
+ originalLabel,
+ label: t('Main'),
+ key: `${t('Main')} ${col.key}`,
Review Comment:
**Suggestion:** The comparison column metadata uses a translated label
(`t('Main')`) as part of the column `key`, while the comparison data records
and totals are built with a hard-coded `"Main "` prefix; in non-English locales
this mismatch means the table will look for fields like "TraduçãoDoMain
<metric>" that do not exist in the row data, resulting in empty cells and
broken totals for time comparison columns. To fix this, keep the user-facing
`label` translated but use a stable, non-translated `"Main "` prefix for the
`key` so it matches the field names produced in `processComparisonDataRecords`
and `processComparisonTotals`. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Main time-comparison metric column renders empty in non-English UI.
- ❌ Summary totals for main comparison metrics show as blank.
- ⚠️ Users can misinterpret comparison tables as having missing data.
```
</details>
```suggestion
key: `Main ${col.key}`,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run Superset with the UI locale set to a non-English language where
`t('Main')` is not
equal to the literal string `"Main"` (frontend translation function from
`@apache-superset/core` is used at
`superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:20` and
`superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:57`).
2. In Explore, create or open a Table chart that has at least one numeric
metric (so
`isMetric` is true) and enable time comparison with `Values` comparison type
(this sets
`time_compare` and `comparison_type`, making `isUsingTimeComparison` true in
`transformProps` at `transformProps.ts:515-538`).
3. Observe that `transformProps` builds comparison row data with hard-coded
keys using the
English prefix `Main` in `processComparisonDataRecords`
(`transformedItem['Main ' +
origCol.key]` at `transformProps.ts:179`) and builds totals with the same
hard-coded key
pattern in `processComparisonTotals` (`transformedTotals['Main ' + key]` at
`transformProps.ts:131-145`).
4. At the same time, `processComparisonColumns` constructs column metadata
with keys based
on the translated label `${t('Main')} ${col.key}` at
`transformProps.ts:404-405`, so for a
French locale this becomes e.g. `'Principal my_metric'`, while the data
objects still use
`'Main my_metric'`. When `TableChart` renders cells, it accesses values by
`datum[key]` in
the accessor at
`superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:917-918`
and totals by `displayedTotals[key]` in the footer at
`TableChart.tsx:1231-1233`, so the
mismatch between translated keys (e.g. `'Principal my_metric'`) and
hard-coded data keys
(`'Main my_metric'`) causes the main comparison column's cells and totals to
be
`undefined`/empty in non-English locales.
```
</details>
<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-table/src/transformProps.ts
**Line:** 405:405
**Comment:**
*Logic Error: The comparison column metadata uses a translated label
(`t('Main')`) as part of the column `key`, while the comparison data records
and totals are built with a hard-coded `"Main "` prefix; in non-English locales
this mismatch means the table will look for fields like "TraduçãoDoMain
<metric>" that do not exist in the row data, resulting in empty cells and
broken totals for time comparison columns. To fix this, keep the user-facing
`label` translated but use a stable, non-translated `"Main "` prefix for the
`key` so it matches the field names produced in `processComparisonDataRecords`
and `processComparisonTotals`.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38145&comment_hash=cdb872b9a48cdada7c759acb12a79d60775fd9d27505ab713af41cbe6c22924c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38145&comment_hash=cdb872b9a48cdada7c759acb12a79d60775fd9d27505ab713af41cbe6c22924c&reaction=dislike'>👎</a>
##########
superset-frontend/src/SqlLab/reducers/getInitialState.ts:
##########
@@ -242,7 +242,7 @@ export default function getInitialState({
}
});
}
- lastUpdatedActiveTab = tabHistory.slice(tabHistory.length - 1)[0] ||
'';
+ lastUpdatedActiveTab = tabHistory.slice(-1)[0] || '';
Review Comment:
**Suggestion:** The new assignment of the last-synced active tab id to the
last entry in the merged tab history overwrites the backend-sourced value, so
`lastUpdatedActiveTab` can point to a tab that has never been persisted; this
breaks the contract used by `EditorAutoSync` and `persistSqlLabStateEnhancer`,
which rely on detecting when the current active tab differs from
`lastUpdatedActiveTab` to push state to the backend, meaning the active tab may
never be synced after localStorage migration. Removing this reassignment
preserves the backend-sourced value and keeps the sync logic working as
intended. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Backend active tab may never update after localStorage migration.
- ⚠️ EditorAutoSync skips syncing active tab on first load.
- ⚠️ Subsequent sessions may open outdated active tab from backend.
```
</details>
```suggestion
// Preserve lastUpdatedActiveTab from the backend-sourced initial
state.
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable the `SqllabBackendPersistence` feature flag so that SQL Lab uses
backend
persistence, as checked in
`superset-frontend/src/SqlLab/middlewares/persistSqlLabStateEnhancer.ts:79-80`
with
`isFeatureEnabled(FeatureFlag.SqllabBackendPersistence)`.
2. In an older session (before this PR's change), use SQL Lab so that state
is saved into
browser localStorage under the `redux` key via
`persistSqlLabStateEnhancer`'s slicer at
`superset-frontend/src/SqlLab/middlewares/persistSqlLabStateEnhancer.ts:71-123`,
which
persists `sqlLab.tabHistory` and other fields.
3. Restart the frontend so a fresh Redux store is created, keeping the
`redux`
localStorage entry; when the app initializes,
`superset-frontend/src/views/store.ts:157`
calls `getInitialState(bootstrapData)` from
`superset-frontend/src/SqlLab/reducers/getInitialState.ts` to build the
initial `sqlLab`
slice, and the SQL Lab page mounts `EditorAutoSync`
(`superset-frontend/src/pages/SqlLab/index.tsx:78`).
4. During `getInitialState` execution, backend bootstrap data defines
`active_tab` (used
to set `tabHistory` and `lastUpdatedActiveTab` at lines 117-118), and then
the
localStorage state is merged: `localStorageData` is parsed at lines 162-167,
`sqlLab.tabHistory` from localStorage is pushed into the in-memory
`tabHistory` at lines
230-235, and finally `lastUpdatedActiveTab` is overwritten with the last
merged history
entry via `lastUpdatedActiveTab = tabHistory.slice(-1)[0] || '';` at line
245.
5. After this initialization, the Redux state has `sqlLab.tabHistory` ending
with the last
locally stored tab id (e.g. `'local-tab-id'`), and
`sqlLab.lastUpdatedActiveTab` equal to
that same id due to line 245, even though the backend's notion of the active
tab (the
`active_tab` passed in bootstrap) may differ and has never been updated to
this new value.
6. `EditorAutoSync` reads `currentQueryEditorId` as
`state.sqlLab.tabHistory.slice(-1)[0]
|| ''` and `lastUpdatedActiveTab` as `state.sqlLab.lastUpdatedActiveTab` at
`superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx:92-97`; in
`syncCurrentQueryEditor` (lines 127-140), the condition
`currentQueryEditorId &&
currentQueryEditorId !== lastUpdatedActiveTab &&
!queryEditors.find(...).inLocalStorage`
evaluates to false because `currentQueryEditorId === lastUpdatedActiveTab`,
so
`updateCurrentSqlEditor` is never called and `setLastUpdatedActiveTab` is
never
dispatched.
7. Because `lastUpdatedActiveTab` was pre-set to the merged tabHistory's
last entry
instead of the backend's `active_tab`, both `EditorAutoSync` and
`persistSqlLabStateEnhancer`'s `hasUnsavedActiveTabState` check at
`persistSqlLabStateEnhancer.ts:95-96` incorrectly conclude there is no
unsynced active-tab
change, so the active tab chosen from localStorage after migration may never
be reported
back to the backend unless the user later switches to yet another tab, which
breaks the
intended "detect change and sync" contract during localStorage migration.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/reducers/getInitialState.ts
**Line:** 245:245
**Comment:**
*Logic Error: The new assignment of the last-synced active tab id to
the last entry in the merged tab history overwrites the backend-sourced value,
so `lastUpdatedActiveTab` can point to a tab that has never been persisted;
this breaks the contract used by `EditorAutoSync` and
`persistSqlLabStateEnhancer`, which rely on detecting when the current active
tab differs from `lastUpdatedActiveTab` to push state to the backend, meaning
the active tab may never be synced after localStorage migration. Removing this
reassignment preserves the backend-sourced value and keeps the sync logic
working as intended.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38145&comment_hash=059559291ef8727a000ad6412e5de9d2d08ca8bb71b25ab91fa105a9ceda79bf&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38145&comment_hash=059559291ef8727a000ad6412e5de9d2d08ca8bb71b25ab91fa105a9ceda79bf&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]
