Skip to content

[WC-3334] Fix DG2 export type bugs#2182

Open
samuelreichert wants to merge 10 commits intomainfrom
fix/datagrid-export-type-bugs
Open

[WC-3334] Fix DG2 export type bugs#2182
samuelreichert wants to merge 10 commits intomainfrom
fix/datagrid-export-type-bugs

Conversation

@samuelreichert
Copy link
Copy Markdown
Contributor

Pull request type

Refactoring (e.g. file rename, variable rename, etc.)
Bug fix (non-breaking change which fixes an issue)

Description

Fix multiple data export bugs in DataGrid2's Excel export feature (WC-3334).

Problems

  1. customContent columns ignore exportType — exportValue always exports as text regardless of exportType setting, making numeric values unusable for calculations in Excel
  2. Date exports leak time components — Date-only formats (e.g., dd-MMM-yyyy) carry a hidden time component in the cell value, affecting sorting and calculations
  3. Boolean exports show TRUE/FALSE — Excel boolean cells always render as TRUE/FALSE regardless of the w field; DataGrid2 displays Yes/No in the UI
  4. Long numbers lose precision — Big.toNumber() loses precision for values >15 significant digits, causing scientific notation in Excel

Changes

  • Extracted cell-creation logic from DSExportRequest.ts into standalone cell-readers.ts module (pure functions, zero behavior change)
  • customContent reader now branches on exportType: parses to Number() for "number", new Date() for "date", with graceful string
    fallback on parse failure
  • Added hasTimeComponent(format) / stripTime(date) helpers — when format has no h/s tokens, date values are normalized to midnight UTC
  • excelBoolean now returns { t: "s", v: "Yes" | "No" } instead of { t: "b" } to match grid display
  • Big values with >15 significant digits export as strings via toFixed() to preserve all digits
  • 28 new unit tests covering all reader paths, edge cases, and fallback behavior

@samuelreichert samuelreichert marked this pull request as ready for review April 20, 2026 14:06
@samuelreichert samuelreichert requested a review from a team as a code owner April 20, 2026 14:06
@samuelreichert samuelreichert force-pushed the fix/datagrid-export-type-bugs branch 2 times, most recently from b804adb to 05d8784 Compare April 24, 2026 13:57
@samuelreichert samuelreichert force-pushed the fix/datagrid-export-type-bugs branch from 05d8784 to bc6c7a0 Compare April 30, 2026 07:52
@samuelreichert samuelreichert requested a review from iobuhov May 1, 2026 12:36
@samuelreichert samuelreichert force-pushed the fix/datagrid-export-type-bugs branch 2 times, most recently from 4e940c2 to 08b3fd2 Compare May 5, 2026 07:42
samuelreichert and others added 10 commits May 7, 2026 16:02
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Documents current behavior of attribute, dynamicText, and customContent
readers before bug-fix changes are applied.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…number

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…digits

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove "b" from ExcelCell.t union and boolean from ExcelCell.v
since excelBoolean now returns string cells. Fix misleading test
name for undefined dynamicText case.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@samuelreichert samuelreichert force-pushed the fix/datagrid-export-type-bugs branch from 08b3fd2 to 66ebfc8 Compare May 7, 2026 14:02
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2026

AI Code Review

🔶 Changes requested — one or more medium-severity items must be addressed


What was reviewed

File Change
packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts New module: extracted cell-reader logic from DSExportRequest, added customContent type-branching, boolean→Yes/No, date time-stripping, Big precision guard
packages/pluggableWidgets/datagrid-web/src/features/data-export/DSExportRequest.ts Removed inline reader logic, imports readChunk from new module
packages/pluggableWidgets/datagrid-web/src/features/data-export/__tests__/cell-readers.spec.ts New test file: 28 tests covering reader paths, fallbacks, precision, and time-stripping
packages/pluggableWidgets/datagrid-web/CHANGELOG.md Added four ### Fixed entries under [Unreleased]

Skipped (out of scope): dist/, pnpm-lock.yaml


Findings

🔶 Medium — excelDate type signature allows {t:"d", v: string} — invalid SheetJS cell

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts line 71
Problem: excelDate accepts value: string | Date and format?: string. When a caller passes a string value with a non-undefined format, the function returns { t: "d", v: "<string>" } — an invalid SheetJS cell (SheetJS requires t:"d" cells to have a Date as v). The current callers happen not to hit this combination, but the type signature doesn't prevent it. A future caller could easily produce a corrupt export.
Fix: Overload or split the function so t:"d" is only produced when v is a Date:

export function excelDate(value: string): ExcelCell;
export function excelDate(value: Date, format: string): ExcelCell;
export function excelDate(value: string | Date, format?: string): ExcelCell {
    return {
        t: format === undefined || typeof value === "string" ? "s" : "d",
        v: value,
        z: format
    };
}

🔶 Medium — dynamicText reader computes format but never uses it for numeric/date coercion

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts line 142
Problem: The dynamicText reader calls getCellFormat(...) and passes format to excelString. However, excelString only uses format to set the z field — there is no coercion to t:"n" or t:"d" regardless of exportType. This means a dynamicText column with exportType: "number" still exports as a string cell, which is the bug described in problem #1 of the PR for customContent. The same silent no-op exists here but is untested and undocumented.
Fix: Either apply the same number/date branching as customContent, or explicitly document and test that dynamicText always exports as string (and why). If the intent is string-only, remove the getCellFormat call to avoid confusion.


⚠️ Low — hasTimeComponent regex misses minute-only formats like "mm:ss"

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts line 86
Note: /[hs]/i detects h (hours) and s (seconds). A format like "mm:ss" (minutes and seconds, no hours) would match s and correctly preserve time — that path is fine. However, a format of "HH:mm" (hours and minutes, locale variants) would also match H via the case-insensitive flag — also correct. The one edge case not covered by a test is "ss" alone (elapsed seconds format) — worth adding a test case for documentation. No code change required if the current behavior is intentional.


⚠️ Low — excelString has redundant ?? undefined

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/cell-readers.ts line 67
Note: z: format ?? undefined — since format is already string | undefined, this is a no-op. Simplify to z: format.


⚠️ Low — Plain number type path in attribute reader is untested

File: packages/pluggableWidgets/datagrid-web/src/features/data-export/__tests__/cell-readers.spec.ts
Note: The attribute reader at line 131 has a branch for typeof value === "number" (native JS number, not Big). All tests use new Big(...). Since Mendix's ListAttributeValue for numeric types always returns Big, this branch is likely dead code — if so, remove it. If it can be reached, add a test with a plain number builder or a comment explaining when it applies.


Positives

  • Clean extraction into a pure-function module (cell-readers.ts) with zero side effects — easy to test and reason about.
  • The countSignificantDigits guard for Big precision handles leading zeros, decimals, and negative values correctly.
  • stripTime uses Date.UTC consistently, avoiding local-timezone contamination in exported date values.
  • Boolean fix (Yes/No string cell instead of {t:"b"}) matches grid display and avoids the Excel TRUE/FALSE rendering issue cleanly.
  • Test structure uses the column() factory helper from test-utils correctly, keeping tests concise and avoiding repetition.
  • CHANGELOG entries are clear, user-facing, and cover all four fixed bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants