Fix/date invalid input 107#275
Conversation
utksh1
left a comment
There was a problem hiding this comment.
Requesting changes. The formatting-hygiene check is failing, and the date parsing change has indentation/comment style issues. Please run the formatter/checks and keep the invalid-date tests focused on the issue behavior.
|
Hi @utksh1 ! I've cleaned up the indentation, fixed the inline comment formatting, and narrowed down the test scope to focus directly on the issue behavior. All CI checks are passing and it's ready for another review whenever you're free. Thanks! |
|
Thanks for following up. Clarifying the change request so it is actionable: Why this is blocked: What to do next:
|
utksh1
left a comment
There was a problem hiding this comment.
Re-reviewed after your update, but changes are still required. The formatter/checks are green, but the diff still has indentation/style regressions in date.ts and the new Issue #107 test block is outside the main date utility describe structure with inconsistent indentation. Please clean the formatting manually, keep the test inside the relevant parse/format describe block, and avoid broad whitespace churn.
utksh1
left a comment
There was a problem hiding this comment.
Re-reviewed the latest commits. This still needs work before merge: the branch is now conflicting with current main, and the diff still has style regressions in date.ts/date.test.ts, including mis-indented comments inside parseDateSafe, semicolon/style churn inconsistent with the surrounding file, trailing whitespace at the end of date.test.ts, and broad indentation churn outside the focused Issue #107 tests. Please rebase against current main, keep the change narrowly scoped to invalid-date validation, follow the existing no-semicolon style in this file, and keep the Issue #107 assertions inside the existing formatLocaleDate describe block without reformatting unrelated tests.
|
Hey @utksh1 ! Sorry for the delay but I have manually tried fixing up the formatting in date.ts/date.test.ts to meet the requested standard. Kindly review them again. Also, I noticed that even after fetching the latest changes, there are now merge conflicts in the files. Could you please clarify if there was a specific oversight on my part regarding the implementation, or is this just a result of recent changes to the base branch? Once I'm clear on the preferred logic, I'll rectify it immediately! |
utksh1
left a comment
There was a problem hiding this comment.
Thanks for pushing an update. This still cannot be reviewed for merge because the branch has merge conflicts with main (mergeStateStatus: DIRTY) and GitHub is not running checks on the current head.
Please rebase or merge the latest main, resolve the conflicts in the date utility/test files, and let CI run again. After that I can re-review the actual behavior change.
f503c0c to
1ac6cf0
Compare
1ac6cf0 to
3e9db9c
Compare
|
Hey @utksh1 the branch is ready for review. I've resolved the merge conflicts, cleaned up the formatting, and all CI checks are passing green. Kindly confirm. |
utksh1
left a comment
There was a problem hiding this comment.
Re-reviewed the latest push. Still needs cleanup before merge: the date utility diff changes broad file style/formatting, leaves inconsistent formatting/no-newline churn, and the year filter should be implemented as a narrow validation change with focused tests only. Please rebase and keep the diff minimal around issue #107.
|
Closing due to unresolved review feedback. |
Description
This PR hardens the date parsing logic in src/utils/date.ts to prevent "Invalid Date" errors and unrealistic date overflows (e.g., year 99,999).
The parseDateSafe function was updated to include a semantic validation layer. While JavaScript's Date constructor can technically parse extreme numeric strings or overflow invalid months into the following year, this fix ensures that only dates within a realistic range (1900–2100) are treated as valid. If a date falls outside this range or is syntactically incorrect, the function now correctly returns null, allowing the UI to display a consistent 'N/A' fallback.
Related Issues
Closes #107
Type of Change
How Has This Been Tested?
Performed unit testing using Vitest to verify the fix against multiple edge cases -
Result: All 91 tests passed successfully
Checklist