Skip to content

Fix DST date calculation when 1st of month is Sunday, add tests#251

Merged
simonw merged 2 commits intomainfrom
claude/fix-california-clock-change-1ssTK
Mar 8, 2026
Merged

Fix DST date calculation when 1st of month is Sunday, add tests#251
simonw merged 2 commits intomainfrom
claude/fix-california-clock-change-1ssTK

Conversation

@simonw
Copy link
Owner

@simonw simonw commented Mar 8, 2026

california-clock-change - I think it’s wrong, check the date today and use “uvx rodney –help” to test it

Add automated tests to the file- they should be displayable in a closed details/summary

Make them subtle though when hidden

The formula for computing the second Sunday in March and first Sunday
in November produced wrong dates when the 1st of the month fell on a
Sunday (getDay() returns 0). For 2026, this meant showing March 15
instead of March 8, and would have shown November 8 instead of
November 1. Fix by treating Sunday as day 7 in the calculation.

Add inline automated tests (20 assertions across 5 years) in a
collapsed details/summary element that auto-opens on failure.

https://claude.ai/code/session_01DgKd487wLpi8VkHyWSXWTX

The formula for computing the second Sunday in March and first Sunday
in November produced wrong dates when the 1st of the month fell on a
Sunday (getDay() returns 0). For 2026, this meant showing March 15
instead of March 8, and would have shown November 8 instead of
November 1. Fix by treating Sunday as day 7 in the calculation.

Add inline automated tests (20 assertions across 5 years) in a
collapsed details/summary element that auto-opens on failure.

https://claude.ai/code/session_01DgKd487wLpi8VkHyWSXWTX
@claude
Copy link
Contributor

claude bot commented Mar 8, 2026

Claude finished @simonw's task —— View job


Code Review Results ✅

Summary: This is a well-crafted fix for a subtle DST calculation bug with comprehensive test coverage. The implementation is solid and the tests provide excellent protection against regressions.

Excellent Bug Fix

The core issue was elegantly solved in california-clock-change.html:157 and california-clock-change.html:160:

// OLD: const marchDay = new Date(y, 2, 1).getDay();
// NEW: const marchDay = new Date(y, 2, 1).getDay() || 7;

Why this works: getDay() returns 0 for Sunday, which broke the formula 1 + (14 - marchDay) when the 1st fell on Sunday (resulting in 1 + 14 = 15 instead of 1 + 7 = 8). Treating Sunday as day 7 fixes the math perfectly.

Comprehensive Test Coverage

The test suite (california-clock-change.html:302-370) is impressive:

  • 20 assertions across 5 years (2024-2028)
  • Tests the exact bug case: 2026 where both March 1st and November 1st are Sundays
  • Validates all results are Sundays (DST always changes on Sundays)
  • Auto-expanding on failure - great UX for debugging
  • Tests mirror the actual implementation logic for consistency

Code Quality

  • Clean, readable code with good variable naming
  • Consistent style with the existing codebase
  • Minimal, surgical change - only fixed what was broken
  • Good separation of concerns - test logic is isolated

Performance & Security

  • No performance impact - same computational complexity
  • No security concerns - pure date calculation logic
  • No external dependencies added

🔍 Minor Observations

  1. Test positioning: Tests are well-placed in a collapsible section to avoid UI clutter
  2. Edge case coverage: The 2026 test case perfectly captures the original bug scenario
  3. Maintainability: The test helper function changesForYear() mirrors production code, making it easy to verify correctness

Recommendation: ✅ APPROVE

This PR demonstrates excellent software engineering practices:

  • Precise problem identification and minimal fix
  • Comprehensive test coverage including the exact failure case
  • Clean, maintainable code
  • No negative side effects

The fix is mathematically sound and the tests provide strong confidence in the solution.


@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Mar 8, 2026

Deploying tools with  Cloudflare Pages  Cloudflare Pages

Latest commit: 202582d
Status: ✅  Deploy successful!
Preview URL: https://e538707f.tools-b1q.pages.dev
Branch Preview URL: https://claude-fix-california-clock-4s11.tools-b1q.pages.dev

View logs

@simonw
Copy link
Owner Author

simonw commented Mar 8, 2026

Make the test results display more informative

Tests are now grouped by year with separator headers, show the full
friendly date (e.g. "Sunday Mar 8, 2026") alongside the ISO date
assertion, and annotate the 2026 edge case where the 1st of the
month is a Sunday. Summary line now includes total count.

https://claude.ai/code/session_01DgKd487wLpi8VkHyWSXWTX
@claude
Copy link
Contributor

claude bot commented Mar 8, 2026

Claude finished @simonw's task —— View job


Code Review Complete ✅

Todo List:

  • Read and analyze the california-clock-change.html file
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Analyze test coverage and informativeness
  • Provide comprehensive review feedback

Summary

This is an excellent PR that demonstrates careful engineering and attention to detail. The DST calculation bug fix is mathematically sound, and the test suite provides comprehensive coverage with clear, informative output. The code quality is high throughout.

Bug Fix - Excellent Implementation

Core Issue (Lines 157, 160):

// Fixed: const marchDay = new Date(y, 2, 1).getDay() || 7;
// Fixed: const novDay = new Date(y, 10, 1).getDay() || 7;

Why this fix works perfectly:

  • getDay() returns 0 for Sunday, which broke the formula when the 1st fell on Sunday
  • Original: 1 + (14 - 0) = 15 (wrong: March 15th instead of March 8th)
  • Fixed: 1 + (14 - 7) = 8 (correct: March 8th)
  • This is the standard mathematical approach for "treat Sunday as day 7" calculations

Test Coverage - Comprehensive and Informative

Test Suite Strengths (Lines 302-383):

  1. Perfect Coverage: 20 assertions across 5 years (2024-2028) including edge cases
  2. Bug-Specific Tests: 2026 tests the exact scenario that was broken (March 1st and November 1st both Sundays)
  3. Smart Validation: All results verified to be Sundays (DST always changes on Sundays)
  4. Excellent UX:
    • Tests auto-expand only on failure (if (failed > 0) el.parentElement.open = true;)
    • Clear summary in collapsed state: "Tests: X passed, Y failed of Z tests"
    • Grouped output with friendly date format and edge case annotations

Addressing "Make test results more informative":
The test output is already very informative:

  • Grouped by year with special annotations (e.g., "2026 (1st is Sunday)")
  • Friendly date display: "Sunday Mar 8, 2026" alongside assertions
  • Clear pass/fail indicators with expected vs actual values
  • Comprehensive summary line
  • Auto-expansion on failures for immediate debugging

Code Quality

Strengths:

  • Clean Architecture: Test helper changesForYear() mirrors production logic for consistency
  • Readable Code: Good variable naming, clear comments, logical structure
  • Consistent Style: Follows existing codebase patterns
  • Error Handling: Robust date calculations with edge case handling

Best Practices Followed:

  • Single responsibility functions
  • No magic numbers (well-documented formulas)
  • Comprehensive input validation through testing
  • Progressive enhancement (graceful degradation for non-California users)

Performance

Excellent Performance Characteristics:

  • O(1) complexity: Date calculations are constant time
  • Minimal overhead: Tests run once on page load, no ongoing performance impact
  • Efficient DOM updates: Smart conditional rendering
  • No memory leaks: Clean event handling and timer management

Security

No Security Concerns:

  • Pure calculations: No external data sources or user input processing
  • Client-side only: No server communication or data persistence
  • Safe DOM manipulation: No innerHTML injection of user data
  • No external dependencies: Self-contained implementation

Browser Compatibility

Modern API Usage (All well-supported):

  • Intl.DateTimeFormat() (supported since 2016)
  • Date methods (universal support)
  • details/summary HTML5 elements (supported since 2011/2020)

🔍 Minor Observations

  1. Test Organization: The grouping by year with annotations like "1st is Sunday" makes the critical edge case immediately obvious
  2. Mathematical Elegance: The || 7 fix is the standard idiom for this type of date calculation
  3. User Experience: The collapsed tests with summary don't clutter the UI but provide full transparency

Recommendation: ✅ APPROVE

This PR demonstrates exceptional software engineering practices:

  • Precise problem identification with minimal, surgical fix
  • Comprehensive test coverage including the exact edge case that caused the bug
  • Highly informative test output that's both user-friendly and developer-friendly
  • Clean, maintainable code following established patterns
  • No negative side effects on performance, security, or functionality

The test informativeness is already excellent - they provide clear summaries, detailed failure information, group results logically, and auto-expand only when needed. The implementation successfully addresses the original request while maintaining high code quality standards.


@simonw simonw merged commit 181166f into main Mar 8, 2026
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants