Skip to content

Commit f2f1870

Browse files
authored
fix dates rendering in table (#7361)
## 📝 Summary <!-- Provide a concise summary of what this pull request is addressing. If this PR fixes any issues, list them here by number (e.g., Fixes #123). --> Fixes #7333 . Some values were not valid dates like 'NaT', 'NaN'. ## 🔍 Description of Changes <!-- Detail the specific changes made in this pull request. Explain the problem addressed and how it was resolved. If applicable, provide before and after comparisons, screenshots, or any relevant details to help reviewers understand the changes easily. --> ## 📋 Checklist - [x] I have read the [contributor guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md). - [ ] For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on [Discord](https://marimo.io/discord?ref=pr), or the community [discussions](https://github.com/marimo-team/marimo/discussions) (Please provide a link if applicable). - [x] I have added tests for the changes made. - [x] I have run the code and verified that it works as expected.
1 parent 7c8070a commit f2f1870

File tree

2 files changed

+182
-2
lines changed

2 files changed

+182
-2
lines changed

frontend/src/components/data-table/__tests__/columns.test.tsx

Lines changed: 165 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,16 @@
11
/* Copyright 2024 Marimo. All rights reserved. */
22

3+
import type { Column } from "@tanstack/react-table";
34
import { render } from "@testing-library/react";
45
import { I18nProvider } from "react-aria";
56
import { describe, expect, it, test } from "vitest";
67
import { TooltipProvider } from "@/components/ui/tooltip";
7-
import { generateColumns, inferFieldTypes, LocaleNumber } from "../columns";
8+
import {
9+
generateColumns,
10+
inferFieldTypes,
11+
LocaleNumber,
12+
renderCellValue,
13+
} from "../columns";
814
import { getMimeValues, isMimeValue, MimeCell } from "../mime-cell";
915
import type { FieldTypesWithExternalType } from "../types";
1016
import { uniformSample } from "../uniformSample";
@@ -519,3 +525,161 @@ describe("LocaleNumber", () => {
519525
expect(container.textContent).toMatchInlineSnapshot(`"10,000,000,000"`);
520526
});
521527
});
528+
529+
describe("renderCellValue with datetime values", () => {
530+
const createMockColumn = () =>
531+
({
532+
id: "created",
533+
columnDef: {
534+
meta: {
535+
dataType: "datetime" as const,
536+
dtype: "datetime64[ns]",
537+
},
538+
},
539+
getColumnFormatting: () => undefined,
540+
getColumnWrapping: () => undefined,
541+
applyColumnFormatting: (value: unknown) => value,
542+
}) as unknown as Column<unknown>;
543+
544+
it("should handle null, empty string, and 'null' string datetime values without throwing RangeError", () => {
545+
const mockColumn = createMockColumn();
546+
547+
// Test null, empty string, and "null" string (as they might come from SQL)
548+
const nullishValues = [null, "", "null"];
549+
550+
nullishValues.forEach((value) => {
551+
const result = renderCellValue({
552+
column: mockColumn,
553+
renderValue: () => value,
554+
getValue: () => value,
555+
selectCell: undefined,
556+
cellStyles: "",
557+
});
558+
559+
expect(result).toBeDefined();
560+
// Should not throw RangeError when rendering
561+
expect(() => {
562+
render(
563+
<I18nProvider locale="en-US">
564+
<TooltipProvider>{result}</TooltipProvider>
565+
</I18nProvider>,
566+
);
567+
}).not.toThrow();
568+
});
569+
});
570+
571+
it("should handle invalid date strings without throwing RangeError", () => {
572+
const mockColumn = createMockColumn();
573+
574+
const invalidDates = [
575+
"invalid-date",
576+
"2024-13-45", // Invalid month/day
577+
"not-a-date",
578+
"2024-06-14 12:34:20.665332",
579+
];
580+
581+
invalidDates.forEach((invalidDate) => {
582+
const result = renderCellValue({
583+
column: mockColumn,
584+
renderValue: () => invalidDate,
585+
getValue: () => invalidDate,
586+
selectCell: undefined,
587+
cellStyles: "",
588+
});
589+
expect(result).toBeDefined();
590+
// Should not throw RangeError when rendering
591+
expect(() => {
592+
render(
593+
<I18nProvider locale="en-US">
594+
<TooltipProvider>{result}</TooltipProvider>
595+
</I18nProvider>,
596+
);
597+
}).not.toThrow();
598+
});
599+
});
600+
601+
it("should still render valid datetime strings correctly", () => {
602+
const mockColumn = createMockColumn();
603+
604+
const validDates = [
605+
"2024-06-14T12:34:20Z",
606+
"2024-06-14 12:34:20",
607+
"2024-06-14",
608+
];
609+
610+
validDates.forEach((validDate) => {
611+
const result = renderCellValue({
612+
column: mockColumn,
613+
renderValue: () => validDate,
614+
getValue: () => validDate,
615+
selectCell: undefined,
616+
cellStyles: "",
617+
});
618+
expect(result).toBeDefined();
619+
// Should render as a date component, not as plain string
620+
expect(result).not.toBeNull();
621+
// Should not throw when rendering
622+
expect(() => {
623+
render(
624+
<I18nProvider locale="en-US">
625+
<TooltipProvider>{result}</TooltipProvider>
626+
</I18nProvider>,
627+
);
628+
}).not.toThrow();
629+
});
630+
});
631+
632+
it("should handle invalid Date instances without throwing RangeError", () => {
633+
const mockColumn = createMockColumn();
634+
635+
const invalidDate = new Date("invalid");
636+
637+
const result = renderCellValue({
638+
column: mockColumn,
639+
renderValue: () => invalidDate,
640+
getValue: () => invalidDate,
641+
selectCell: undefined,
642+
cellStyles: "",
643+
});
644+
expect(result).toBeDefined();
645+
// Should not throw RangeError when rendering
646+
expect(() => {
647+
render(
648+
<I18nProvider locale="en-US">
649+
<TooltipProvider>{result}</TooltipProvider>
650+
</I18nProvider>,
651+
);
652+
}).not.toThrow();
653+
});
654+
655+
it("should handle mixed valid and null datetime values in a column", () => {
656+
const mockColumn = createMockColumn();
657+
658+
const values = [
659+
"2024-06-14T12:34:20Z", // Valid
660+
null,
661+
"2024-06-15T12:34:20Z", // Valid
662+
"",
663+
"2024-06-16T12:34:20Z", // Valid
664+
];
665+
666+
values.forEach((value) => {
667+
const result = renderCellValue({
668+
column: mockColumn,
669+
renderValue: () => value,
670+
getValue: () => value,
671+
selectCell: undefined,
672+
cellStyles: "",
673+
});
674+
expect(result).toBeDefined();
675+
// Should not throw RangeError when rendering
676+
expect(() => {
677+
render(
678+
<I18nProvider locale="en-US">
679+
<TooltipProvider>{result}</TooltipProvider>
680+
</I18nProvider>,
681+
);
682+
}).not.toThrow();
683+
});
684+
});
685+
});

frontend/src/components/data-table/columns.tsx

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
import { PopoverClose } from "@radix-ui/react-popover";
55
import type { Column, ColumnDef } from "@tanstack/react-table";
6-
import { formatDate } from "date-fns";
6+
import { formatDate, isValid } from "date-fns";
77
import { useLocale, useNumberFormatter } from "react-aria";
88
import { WithLocale } from "@/core/i18n/with-locale";
99
import type { DataType } from "@/core/kernel/messages";
@@ -490,6 +490,14 @@ export function renderCellValue<TData, TValue>({
490490

491491
if (dataType === "datetime" && typeof value === "string") {
492492
try {
493+
if (!isValid(value)) {
494+
return (
495+
<div onClick={selectCell} className={cellStyles}>
496+
{value}
497+
</div>
498+
);
499+
}
500+
493501
const date = new Date(value);
494502
const format = getDateFormat(value);
495503
return (
@@ -505,6 +513,14 @@ export function renderCellValue<TData, TValue>({
505513
}
506514

507515
if (value instanceof Date) {
516+
if (!isValid(value)) {
517+
return (
518+
<div onClick={selectCell} className={cellStyles}>
519+
{value.toString()}
520+
</div>
521+
);
522+
}
523+
508524
// e.g. 2010-10-07 17:15:00
509525
return (
510526
<WithLocale>

0 commit comments

Comments
 (0)