fix: timezone shift for todo due dates parsed from date-only strings#347
Conversation
📝 WalkthroughWalkthroughAdds parseLocalDate() to parse YYYY-MM-DD as local-time Date objects, uses it in todo formatting, overdue checks, and inline calendar code, exposes it on window for tests, and declares it as a readonly global in ESLint; tests validate parsing behavior. ChangesLocal-timezone date parsing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@eslint.config.js`:
- Line 136: The new test-exposed global parseLocalDate was added only to
testGlobals; add the same identifier to srcGlobals so both global contract
objects include parseLocalDate. Update the eslint.config.js configuration by
declaring parseLocalDate: 'readonly' inside the srcGlobals object (matching the
entry in testGlobals) to keep srcGlobals and testGlobals consistent for window.*
symbols.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20bbca5f-12c1-4f04-89df-fb23c8f7ad39
📒 Files selected for processing (3)
eslint.config.jssrc/features/todo.jstests/todo.test.js
|
Addressed CodeRabbit finding: added parseLocalDate to srcGlobals in �slint.config.js to keep srcGlobals and estGlobals consistent for window-exposed symbols. Lint clean and all 433 tests pass. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/todo.test.js (1)
349-364: ⚡ Quick winAdd a regression test for the Date-object path used by due-date updates.
These new tests validate string parsing, but they miss the runtime path where a
Dateinstance flows into formatting through due-date update feedback. A small test here would prevent this regression from returning.Suggested test addition
it('parseLocalDate pads single-digit month and day', () => { const d = parseLocalDate('2025-01-05'); expect(d.getFullYear()).toBe(2025); expect(d.getMonth()).toBe(0); expect(d.getDate()).toBe(5); }); + + it('showDateUpdateFeedback accepts Date input from inline picker flow', () => { + const dueDateElement = document.createElement('div'); + dueDateElement.innerHTML = '<span class="due-date-text"></span>'; + expect(() => showDateUpdateFeedback(dueDateElement, null, new Date(2025, 5, 15))).not.toThrow(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/todo.test.js` around lines 349 - 364, Add a regression test that exercises the Date-object path used by due-date updates: create a real Date instance (not a string) and pass it into the same formatting/feedback path that the due-date update uses (the code surrounding parseLocalDate and the due-date update formatter), then assert the resulting formatted/updated value matches the expected local date components and string format; ensure the test references parseLocalDate and the due-date update/formatter function used by the feedback path so the Date-instance branch is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/todo.js`:
- Around line 122-125: formatDate currently assumes its input is a string and
passes it to parseLocalDate, but showDateUpdateFeedback sometimes calls
formatDate with a Date object causing parseLocalDate to throw; update formatDate
to detect Date objects (e.g. using instanceof Date) and convert them to the
expected string form before calling parseLocalDate (for example by converting to
an ISO date string or the same YYYY-MM-DD form parseLocalDate expects), then
proceed with the existing logic; reference: function formatDate and
parseLocalDate and the call site showDateUpdateFeedback.
---
Nitpick comments:
In `@tests/todo.test.js`:
- Around line 349-364: Add a regression test that exercises the Date-object path
used by due-date updates: create a real Date instance (not a string) and pass it
into the same formatting/feedback path that the due-date update uses (the code
surrounding parseLocalDate and the due-date update formatter), then assert the
resulting formatted/updated value matches the expected local date components and
string format; ensure the test references parseLocalDate and the due-date
update/formatter function used by the feedback path so the Date-instance branch
is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cd78091-af11-4b40-b222-a1e8c31e3bcd
📒 Files selected for processing (3)
eslint.config.jssrc/features/todo.jstests/todo.test.js
| function formatDate(dateString) { | ||
| if (!dateString) return ''; | ||
| const date = new Date(dateString); | ||
| const date = parseLocalDate(dateString); | ||
| const currentLang = window.i18n ? window.i18n.currentLanguage() : 'en'; |
There was a problem hiding this comment.
formatDate now crashes on the due-date update path.
Line 124 assumes string input, but showDateUpdateFeedback passes a Date object (Line 1133). That causes parseLocalDate to call .split on a non-string and throws when users set/tap due dates.
Suggested fix
-function formatDate(dateString) {
- if (!dateString) return '';
- const date = parseLocalDate(dateString);
+function formatDate(dateInput) {
+ if (!dateInput) return '';
+ const date = dateInput instanceof Date ? new Date(dateInput) : parseLocalDate(dateInput);
+ if (Number.isNaN(date.getTime())) return '';
const currentLang = window.i18n ? window.i18n.currentLanguage() : 'en';
const locale = currentLang === 'zh' ? 'zh-CN' : 'en-US';
return date.toLocaleDateString(locale, {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function formatDate(dateString) { | |
| if (!dateString) return ''; | |
| const date = new Date(dateString); | |
| const date = parseLocalDate(dateString); | |
| const currentLang = window.i18n ? window.i18n.currentLanguage() : 'en'; | |
| function formatDate(dateInput) { | |
| if (!dateInput) return ''; | |
| const date = dateInput instanceof Date ? new Date(dateInput) : parseLocalDate(dateInput); | |
| if (Number.isNaN(date.getTime())) return ''; | |
| const currentLang = window.i18n ? window.i18n.currentLanguage() : 'en'; | |
| const locale = currentLang === 'zh' ? 'zh-CN' : 'en-US'; | |
| return date.toLocaleDateString(locale, { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/todo.js` around lines 122 - 125, formatDate currently assumes
its input is a string and passes it to parseLocalDate, but
showDateUpdateFeedback sometimes calls formatDate with a Date object causing
parseLocalDate to throw; update formatDate to detect Date objects (e.g. using
instanceof Date) and convert them to the expected string form before calling
parseLocalDate (for example by converting to an ISO date string or the same
YYYY-MM-DD form parseLocalDate expects), then proceed with the existing logic;
reference: function formatDate and parseLocalDate and the call site
showDateUpdateFeedback.
|
Both CodeRabbit review findings are false positives — no code changes needed. Lint is clean, 433 tests pass. Finding 1 (Actionable): formatDate receiving a Date objectCodeRabbit claimed
Finding 2 (Nitpick): Regression test for Date-object pathSince Both findings can be dismissed. The PR is ready to merge as-is. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify the data flow claims about showDateUpdateFeedback and formatDate in todo.js
echo "=== Lines around 1060-1110 (formatDateISO + showDateUpdateFeedback call sites) ==="
sed -n '1060,1115p' src/features/todo.js
echo ""
echo "=== All occurrences of showDateUpdateFeedback ==="
rg -n 'showDateUpdateFeedback' src/features/todo.js
echo ""
echo "=== All occurrences of formatDate (non-ISO) ==="
rg -n '\bformatDate\b' src/features/todo.js
echo ""
echo "=== formatDate function definition ==="
rg -n 'function formatDate' src/features/todo.js
echo ""
echo "=== Lines around formatDate definition ==="
grep -n 'function formatDate' src/features/todo.js | head -5Length of output: 3506
Finding 1: Line 1073 confirms Finding 2: Since The PR looks correct and is ready to merge. (ˆ · ˆ) 🐇✨ |
2 participants
Summary
Todo due dates stored as
YYYY-MM-DDwere parsed withnew Date(dateString), which interprets date-only strings as UTC midnight. In non-UTC timezones, this caused the date to display and evaluate as the previous day (e.g.2026-06-15displayed as2026-06-14in UTC-5). Fixes #258.What Changed
parseLocalDate()helper insrc/features/todo.jsthat splits aYYYY-MM-DDstring and constructs aDateusing local time components (new Date(year, month - 1, day)), avoiding the UTC shift.parseLocalDate():formatDate(),isOverdue(),createCalendarHtml(),setupInlineCalendarHandlers()(×2),refreshInlineDatePickers().parseLocalDateonwindowfor test access.parseLocalDatetoeslint.config.jsglobals.tests/todo.test.jsverifying local-time construction.Verification
npm run lint— cleannpm test— 433 tests pass (16 test files)Notes
createdAt/completedAtISO timestamps were left untouched — they already include time components and parse correctly.background/service-worker.jswas left untouched — it already appendsT23:59:59to date strings.YYYY-MM-DDformat.Summary by CodeRabbit
Bug Fixes
Tests
Chores