fix: PR comments #70

noe.trevino · 2026-02-14T04:06:18-06:00

noe.trevino commented

2026-02-14 04:06:18 -06:00

No description provided.

github-actions[bot] commented

2026-02-14 04:07:03 -06:00

(Migrated from github.com)

PR Summary (auto-generated)

Refactored month navigation in date picker state by extracting duplicated logic from NextMonth() and PrevMonth() into a single navigateMonth(delta int) helper function
Simplified year/month wrapping logic by leveraging Go's time.Date() automatic normalization instead of manual boundary checks (month 13 → January next year, month 0 → December previous year)
Maintained sticky cursor navigation behavior where cursorWant preserves the user's desired day position (e.g., day 31) when navigating through months with fewer days
Added comprehensive test suite covering positive/negative deltas, day clamping across different month lengths, leap year handling, year boundary crossings, and cursor preservation across multiple month transitions
Reduced code duplication by ~30 lines while preserving identical behavior and improving maintainability

**PR Summary** _(auto-generated)_ - Refactored month navigation in date picker state by extracting duplicated logic from `NextMonth()` and `PrevMonth()` into a single `navigateMonth(delta int)` helper function - Simplified year/month wrapping logic by leveraging Go's `time.Date()` automatic normalization instead of manual boundary checks (month 13 → January next year, month 0 → December previous year) - Maintained sticky cursor navigation behavior where `cursorWant` preserves the user's desired day position (e.g., day 31) when navigating through months with fewer days - Added comprehensive test suite covering positive/negative deltas, day clamping across different month lengths, leap year handling, year boundary crossings, and cursor preservation across multiple month transitions - Reduced code duplication by ~30 lines while preserving identical behavior and improving maintainability

claude[bot] (Migrated from github.com) reviewed

2026-02-14 04:07:06 -06:00

internal/tui/state/date_picker_state_test.go

					
				@ -860,0 +930,4 @@

								cursorWant:   tt.startDay,

							}

							state.navigateMonth(tt.delta)

Testing a private method (navigateMonth) is a code smell. Tests should verify behavior through public APIs (NextMonth(), PrevMonth()). Private methods are implementation details that should be covered indirectly through their callers.

This creates brittle tests that break when refactoring internals, and makes it harder to change the implementation later.

Testing a private method (`navigateMonth`) is a code smell. Tests should verify behavior through public APIs (`NextMonth()`, `PrevMonth()`). Private methods are implementation details that should be covered indirectly through their callers. This creates brittle tests that break when refactoring internals, and makes it harder to change the implementation later.

claude[bot] (Migrated from github.com) reviewed

2026-02-14 04:07:11 -06:00

internal/tui/state/date_picker_state_test.go

					
				@ -860,0 +946,4 @@

							}

						})

					}

				}

This test function duplicates the exact same test structure and logic as TestNavigateMonth_NegativeDelta, TestNavigateMonth_DayClamping, TestNavigateMonth_PreservesCursorWant, TestNavigateMonth_YearBoundaries, and TestNavigateMonth_ZeroDelta.

DRY violation: The test execution logic (lines 924-948) is copy-pasted 6 times. Extract to a helper:

func runNavigateMonthTests(t *testing.T, tests []struct{...}) {
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            state := &DatePickerState{...}
            state.navigateMonth(tt.delta)
            // assertions...
        })
    }
}

Then each test function just defines its cases and calls the helper.

This test function duplicates the exact same test structure and logic as `TestNavigateMonth_NegativeDelta`, `TestNavigateMonth_DayClamping`, `TestNavigateMonth_PreservesCursorWant`, `TestNavigateMonth_YearBoundaries`, and `TestNavigateMonth_ZeroDelta`. DRY violation: The test execution logic (lines 924-948) is copy-pasted 6 times. Extract to a helper: ```go func runNavigateMonthTests(t *testing.T, tests []struct{...}) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { state := &DatePickerState{...} state.navigateMonth(tt.delta) // assertions... }) } } ``` Then each test function just defines its cases and calls the helper.

claude[bot] (Migrated from github.com) reviewed

2026-02-14 04:07:13 -06:00

internal/tui/state/date_picker_state_test.go

					
				@ -860,0 +868,4 @@

						expectedYear  int

						expectedMonth time.Month

						expectedDay   int

					}{

The test struct is duplicated across 5 different test functions with identical fields. This violates DRY—define it once at package level:

type navigateMonthTestCase struct {
    name          string
    startYear     int
    startMonth    time.Month
    startDay      int
    delta         int
    expectedYear  int
    expectedMonth time.Month
    expectedDay   int
}

The test struct is duplicated across 5 different test functions with identical fields. This violates DRY—define it once at package level: ```go type navigateMonthTestCase struct { name string startYear int startMonth time.Month startDay int delta int expectedYear int expectedMonth time.Month expectedDay int } ```

claude[bot] (Migrated from github.com) reviewed

2026-02-14 04:07:16 -06:00

internal/tui/state/date_picker_state_test.go

					
				@ -860,0 +943,4 @@

							}

							if state.cursorWant != tt.startDay {

								t.Errorf("cursorWant should remain %d, got %d", tt.startDay, state.cursorWant)

							}

Assertion logic duplicated across all 6 TestNavigateMonth_* functions. These 12 lines are copy-pasted repeatedly—extract to a helper:

func assertNavigateMonthResult(t *testing.T, state *DatePickerState, expected navigateMonthTestCase) {
    t.Helper()
    if state.CurrentYear != expected.expectedYear {
        t.Errorf("expected year %d, got %d", expected.expectedYear, state.CurrentYear)
    }
    // ... rest of assertions
}

Assertion logic duplicated across all 6 `TestNavigateMonth_*` functions. These 12 lines are copy-pasted repeatedly—extract to a helper: ```go func assertNavigateMonthResult(t *testing.T, state *DatePickerState, expected navigateMonthTestCase) { t.Helper() if state.CurrentYear != expected.expectedYear { t.Errorf("expected year %d, got %d", expected.expectedYear, state.CurrentYear) } // ... rest of assertions } ```

claude[bot] (Migrated from github.com) reviewed

2026-02-14 04:07:19 -06:00

internal/tui/state/date_picker_state_test.go

					
				@ -860,0 +878,4 @@

							expectedYear:  2024,

							expectedMonth: time.February,

							expectedDay:   15,

						},

Test case duplication: This "forward 1 month" test is semantically equivalent to existing tests like TestNextMonth_BasicForward (which likely exists for the public API). These tests through the private method add no value beyond what testing NextMonth() already provides.

Test case duplication: This "forward 1 month" test is semantically equivalent to existing tests like `TestNextMonth_BasicForward` (which likely exists for the public API). These tests through the private method add no value beyond what testing `NextMonth()` already provides.

claude[bot] (Migrated from github.com) reviewed

2026-02-14 04:07:22 -06:00

internal/tui/state/date_picker_state_test.go

					
				@ -860,0 +908,4 @@

							expectedYear:  2026,

							expectedMonth: time.March,

							expectedDay:   10,

						},

These cases test navigateMonth with deltas >1 (3, 12, 24 months). However, the public API (NextMonth/PrevMonth) only uses delta=±1. Testing with arbitrary deltas tests functionality that's never actually used.

If multi-month navigation is needed, expose it through the public API first. Otherwise, remove these cases.

These cases test `navigateMonth` with deltas >1 (3, 12, 24 months). However, the public API (`NextMonth`/`PrevMonth`) only uses delta=±1. Testing with arbitrary deltas tests functionality that's never actually used. If multi-month navigation is needed, expose it through the public API first. Otherwise, remove these cases.

Sign in to join this conversation.

No reviewers