codeant-ai-for-open-source[bot] commented on code in PR #42601:
URL: https://github.com/apache/superset/pull/42601#discussion_r3696261882
##########
superset-frontend/src/utils/downloadAsPivotExcel.ts:
##########
@@ -17,12 +17,77 @@
* under the License.
*/
import { utils, writeFile } from 'xlsx';
+import type { WorkSheet } from 'xlsx';
+
+// ISO 8601 date (and optional time) form, e.g. "2024-01-01" or
+// "2024-01-01 00:00:00". This layout is unambiguous under any locale
+// (unlike "1/2/2024", which means different dates depending on the
+// reader), so it's safe to restore as a native Excel date.
+const ISO_DATE_RE = /^(\d{4})-(\d{2})-(\d{2})(?:[
T](\d{2}):(\d{2}):(\d{2}))?$/;
+
+// `raw: true` (used below) keeps every table cell as text, so ordinary
+// numbers and dates lose their native Excel type along with the
+// locale-formatted values. A cell's text is only restored to a real number
+// or date when it is unambiguous under any locale: a plain number that
+// round-trips losslessly through Number() (e.g. "42" or "-3.5"), or an
+// ISO 8601 date/datetime string. Restoring those can't reintroduce the
+// misparsing raw: true guards against. Anything else (grouped thousands,
+// percent suffixes, trailing zero padding, other D3_FORMAT output, etc.)
+// stays as text, exactly as rendered.
+function restoreUnambiguousNumbers(sheet: WorkSheet): void {
+ Object.keys(sheet).forEach(cellRef => {
+ if (cellRef.startsWith('!')) {
+ return;
+ }
+ const cell = sheet[cellRef];
+ if (!cell || cell.t !== 's' || typeof cell.v !== 'string') {
+ return;
+ }
+ const isoMatch = cell.v.match(ISO_DATE_RE);
+ if (isoMatch) {
+ const [y, mo, d, h, mi, s] = isoMatch
+ .slice(1)
+ .map((part: string | undefined) => Number(part ?? 0));
+ const date = new Date(y, mo - 1, d, h, mi, s);
Review Comment:
**Suggestion:** The conversion scans every worksheet cell, including pivot
headers and row-label cells, and turns any ISO-shaped string into a native
date. Pivot rendering does not distinguish dimension labels from date values,
so a legitimate categorical value such as `2024-01-01` will be exported as a
date cell and may display differently or lose its original text semantics in
Excel. Restrict date restoration to cells known to represent date metrics, or
leave rendered table text unchanged. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ ISO-shaped pivot dimension labels lose text semantics.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e07684948ddd4be7859dc1cb6fbf81c5&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=e07684948ddd4be7859dc1cb6fbf81c5&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/src/utils/downloadAsPivotExcel.ts
**Line:** 46:51
**Comment:**
*Logic Error: The conversion scans every worksheet cell, including
pivot headers and row-label cells, and turns any ISO-shaped string into a
native date. Pivot rendering does not distinguish dimension labels from date
values, so a legitimate categorical value such as `2024-01-01` will be exported
as a date cell and may display differently or lose its original text semantics
in Excel. Restrict date restoration to cells known to represent date metrics,
or leave rendered table text 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.
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%2F42601&comment_hash=829382583f6ca65754120a4475c549b897acaf4f4f7f2f46eaba09cf9ed8121c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42601&comment_hash=829382583f6ca65754120a4475c549b897acaf4f4f7f2f46eaba09cf9ed8121c&reaction=dislike'>👎</a>
##########
superset-frontend/src/utils/downloadAsPivotExcel.ts:
##########
@@ -17,12 +17,77 @@
* under the License.
*/
import { utils, writeFile } from 'xlsx';
+import type { WorkSheet } from 'xlsx';
+
+// ISO 8601 date (and optional time) form, e.g. "2024-01-01" or
+// "2024-01-01 00:00:00". This layout is unambiguous under any locale
+// (unlike "1/2/2024", which means different dates depending on the
+// reader), so it's safe to restore as a native Excel date.
+const ISO_DATE_RE = /^(\d{4})-(\d{2})-(\d{2})(?:[
T](\d{2}):(\d{2}):(\d{2}))?$/;
+
+// `raw: true` (used below) keeps every table cell as text, so ordinary
+// numbers and dates lose their native Excel type along with the
+// locale-formatted values. A cell's text is only restored to a real number
+// or date when it is unambiguous under any locale: a plain number that
+// round-trips losslessly through Number() (e.g. "42" or "-3.5"), or an
+// ISO 8601 date/datetime string. Restoring those can't reintroduce the
+// misparsing raw: true guards against. Anything else (grouped thousands,
+// percent suffixes, trailing zero padding, other D3_FORMAT output, etc.)
+// stays as text, exactly as rendered.
+function restoreUnambiguousNumbers(sheet: WorkSheet): void {
+ Object.keys(sheet).forEach(cellRef => {
+ if (cellRef.startsWith('!')) {
+ return;
+ }
+ const cell = sheet[cellRef];
+ if (!cell || cell.t !== 's' || typeof cell.v !== 'string') {
+ return;
+ }
+ const isoMatch = cell.v.match(ISO_DATE_RE);
+ if (isoMatch) {
+ const [y, mo, d, h, mi, s] = isoMatch
+ .slice(1)
+ .map((part: string | undefined) => Number(part ?? 0));
+ const date = new Date(y, mo - 1, d, h, mi, s);
+ // The Date constructor rolls invalid components over into the next
+ // unit (e.g. day 40 becomes the 10th of the following month, minute
+ // 60 becomes the top of the next hour) instead of rejecting them, so
+ // confirm every part - date and time - round-trips before trusting
+ // the result.
+ const isValid =
+ date.getFullYear() === y &&
+ date.getMonth() === mo - 1 &&
+ date.getDate() === d &&
+ date.getHours() === h &&
+ date.getMinutes() === mi &&
+ date.getSeconds() === s;
+ if (isValid) {
+ cell.t = 'd';
+ cell.v = date;
+ return;
+ }
+ }
+ const value = Number(cell.v);
+ if (cell.v !== '' && Number.isFinite(value) && String(value) === cell.v) {
+ cell.t = 'n';
+ cell.v = value;
+ }
Review Comment:
**Suggestion:** This applies numeric type restoration to every string cell,
including categorical dimension values and row labels. A label such as `42` is
rendered by the pivot table as text but is changed to an Excel numeric cell
here, altering the cell's type and semantics even though it is not a metric
value. The export needs metadata or positional filtering so only cells
representing numeric measures are restored. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Numeric categorical labels change worksheet cell semantics.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4271be914ec64b54a401d554506c5fd9&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=4271be914ec64b54a401d554506c5fd9&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/src/utils/downloadAsPivotExcel.ts
**Line:** 70:74
**Comment:**
*Type Error: This applies numeric type restoration to every string
cell, including categorical dimension values and row labels. A label such as
`42` is rendered by the pivot table as text but is changed to an Excel numeric
cell here, altering the cell's type and semantics even though it is not a
metric value. The export needs metadata or positional filtering so only cells
representing numeric measures are restored.
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%2F42601&comment_hash=6010d64ed7452693e7119230fdf0cc18539712c1031e2b6b93fbbbbf6916c19c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42601&comment_hash=6010d64ed7452693e7119230fdf0cc18539712c1031e2b6b93fbbbbf6916c19c&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]
