Fix: prevent RangeError: Invalid time value in date filter serialization#719
Fix: prevent RangeError: Invalid time value in date filter serialization#719paanSinghCoder wants to merge 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe change enhances date value handling in the filter operations utility by adding explicit validity checking and exception handling for date-to-ISO string conversion. Rather than using inline conditionals, the code now validates the dayjs instance before conversion and catches potential runtime errors. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/raystack/components/data-table/utils/filter-operations.tsx (1)
170-182: Please add a regression test for this exact failure mode.Add a unit test around
getFilterValue/handleStringBasedTypesforFilterType.datewhere ISO serialization throws, and assertstringValuebecomes''instead of throwing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-table/utils/filter-operations.tsx` around lines 170 - 182, Add a regression unit test for getFilterValue/handleStringBasedTypes covering FilterType.date that simulates dayjs(value).toISOString() throwing and asserts the returned object has stringValue === ''. In the test, mock or stub the dayjs instance used by handleStringBasedTypes (or the toISOString method) to throw when called for a date filter, then call getFilterValue (or handleStringBasedTypes directly) with FilterType.date and the test value, and assert the function returns { value, stringValue: '' } and does not propagate an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/raystack/components/data-table/utils/filter-operations.tsx`:
- Around line 170-182: Add a regression unit test for
getFilterValue/handleStringBasedTypes covering FilterType.date that simulates
dayjs(value).toISOString() throwing and asserts the returned object has
stringValue === ''. In the test, mock or stub the dayjs instance used by
handleStringBasedTypes (or the toISOString method) to throw when called for a
date filter, then call getFilterValue (or handleStringBasedTypes directly) with
FilterType.date and the test value, and assert the function returns { value,
stringValue: '' } and does not propagate an error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06dd4e0a-e1e5-429f-bcf8-b3f4630d27a7
📒 Files selected for processing (1)
packages/raystack/components/data-table/utils/filter-operations.tsx
1 participant
Description
Problem
When using the DataTable date filter (e.g. "Created On"), opening the date picker and clicking outside could crash the page with
RangeError: Invalid timevalue fromDate.toISOString().Root cause
dayjs.isValid()andDate.toISOString()can disagree: dayjs may treat a value as valid even when the underlying Date is outside ECMAScript’s representable range or otherwise invalid, causingtoISOString()to throw.Solution
Wrap
dateValue.toISOString()in atry/catchinhandleStringBasedTypes(filter-operations). On failure, use an empty string instead of letting the error propagate.Type of Change
How Has This Been Tested?
[Describe the tests that you ran to verify your changes]
Checklist:
Screenshots (if appropriate):
[Add screenshots here]
Related Issues
[Link any related issues here using #issue-number]
Summary by CodeRabbit