refactor: fix version comparison bug and extract DRY helpers#71
refactor: fix version comparison bug and extract DRY helpers#71
Conversation
- Replace lexicographic string comparison with compareVersions() that splits on "." and compares each segment as integers - Fixes bug where "0.9.0" > "0.10.0" was true (string comparison) - Add 3 test cases for multi-digit version segments
Filter DRY cleanup: - Add filter.go with generic applyFilter[T Filterable] and handleFilterKey - Delete 7 duplicate applyXxxFilter() functions across screens - Collapse 8 filter-input handlers from 18 to 7 lines each - Net reduction: ~180 lines of duplicated filter logic Update() extraction: - Extract 7 per-domain handleXxxMsg() to respective screen files - Update() reduced from 374 to 119 lines (68% reduction) - app.go reduced from 1,076 to 816 lines Also adds /refactor-review skill and .claude/reports/ to .gitignore
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR addresses critical version comparison bugs and successfully extracts duplicate code into reusable helpers. The refactoring is well-structured with one critical issue requiring attention.
Critical Issue Found
Version Comparison Logic Error - The compareVersions function silently ignores errors from fmt.Sscanf, which could cause incorrect version comparisons when version strings contain non-numeric characters.
Refactoring Assessment
The DRY extraction and message handler refactoring appear solid:
- ✅ New
filter.gowith genericapplyFilter[T]andhandleFilterKeyeliminates ~180 lines of duplication - ✅ Domain message handlers properly extracted to screen-specific files
- ✅
Update()method reduced from 374 → 119 lines - ✅ Tests added for multi-digit version segments (0.9.0 vs 0.10.0)
Next Steps
Address the version comparison error handling before merging.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
internal/update/update.go
Outdated
| if i < len(aParts) { | ||
| fmt.Sscanf(aParts[i], "%d", &ai) | ||
| } | ||
| if i < len(bParts) { | ||
| fmt.Sscanf(bParts[i], "%d", &bi) | ||
| } |
There was a problem hiding this comment.
🛑 Logic Error: fmt.Sscanf silently ignores errors, treating invalid version segments as 0. This causes incorrect version comparisons when segments contain non-numeric characters (e.g., "1.2.beta" vs "1.2.0" would treat "beta" as 0).
| if i < len(aParts) { | |
| fmt.Sscanf(aParts[i], "%d", &ai) | |
| } | |
| if i < len(bParts) { | |
| fmt.Sscanf(bParts[i], "%d", &bi) | |
| } | |
| if i < len(aParts) { | |
| n, err := fmt.Sscanf(aParts[i], "%d", &ai) | |
| if err != nil || n != 1 { | |
| return 0 // treat malformed versions as equal | |
| } | |
| } | |
| if i < len(bParts) { | |
| n, err := fmt.Sscanf(bParts[i], "%d", &bi) | |
| if err != nil || n != 1 { | |
| return 0 // treat malformed versions as equal | |
| } | |
| } |
- Extract parseVersionSegment helper with proper Sscanf error check - Non-numeric segments explicitly return 0 instead of silently ignoring - Add test case for non-numeric version segments (e.g., "0.1.0-beta")
|
/q review |
There was a problem hiding this comment.
Review Summary
This refactoring PR successfully addresses the core priorities from the senior review report:
✅ Priority 1 — Version Comparison Bug Fix
The replacement of lexicographic string comparison with proper numeric segment-by-segment comparison is well-implemented. The new compareVersions() and parseVersionSegment() functions correctly handle multi-digit version components, and test coverage includes edge cases like "0.9.0" vs "0.10.0".
The extraction of applyFilter[T Filterable]() and handleFilterKey() successfully eliminates duplication. However, there is one critical bug in the case-insensitive filtering logic that must be fixed before merge.
✅ Priority 3 — Extract Update() Handlers
The extraction of domain-specific message handlers is clean and reduces the main Update() function significantly.
Merge Decision: One blocking issue identified that prevents correct filtering functionality. Please address the critical bug in the case-insensitive filter implementation.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
internal/app/filter.go
Outdated
| q := strings.ToLower(query) | ||
| var result []T | ||
| for _, item := range items { | ||
| if strings.Contains(item.FilterText(), q) { |
There was a problem hiding this comment.
🛑 Logic Error: Case-insensitive filtering is incomplete. While the query is converted to lowercase (line 16), the item's FilterText() output is not. This causes "ABC" to not match query "abc" when it should.
| if strings.Contains(item.FilterText(), q) { | |
| if strings.Contains(strings.ToLower(item.FilterText()), q) { |
- Apply strings.ToLower to FilterText() output in applyFilter - Defensive: all current FilterText() already return lowercase, but applyFilter should not rely on that convention
|
/q review |
There was a problem hiding this comment.
Review Summary
This refactoring PR successfully addresses the stated priorities without introducing any blocking defects. The changes are well-structured and maintain existing functionality.
Key Changes Validated
Version Comparison Bug Fix — The replacement of lexicographic string comparison (l > c) with segment-based integer comparison via compareVersions() correctly fixes the reported bug where "0.9.0" > "0.10.0" would evaluate incorrectly. The implementation properly handles multi-digit version segments, and test coverage validates the fix.
Generic Filter Helpers — The extraction of applyFilter[T Filterable] and handleFilterKey into filter.go eliminates code duplication while preserving functionality. The generic implementation correctly handles case-insensitive filtering and keyboard input.
Message Handler Extraction — The refactoring of domain message handlers into screen-specific files (e.g., handleEC2VPCMsg, handleSecretMsg) reduces app.go complexity without changing behavior. The handler dispatch pattern using a function slice is clean and maintainable.
Testing
All existing tests pass with no behavior changes, confirming this is a safe refactoring that improves code quality without introducing regressions.
Status: Approved — No blocking issues found.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
2 participants
Summary
Address the top 3 priorities from the
/senior-reviewcode quality report:Priority 1 — Fix version comparison bug
IsNewer()used lexicographic string comparison (l > c), causing"0.9.0" > "0.10.0"to be truecompareVersions()that splits on.and compares segments as integersPriority 2 — Extract generic filter helpers
filter.gowithapplyFilter[T Filterable]()andhandleFilterKey()applyXxxFilter()functions across screensPriority 3 (partial) — Extract Update() message handlers
handleXxxMsg()functions to respective screen filesUpdate()reduced from 374 → 119 lines (68% reduction)app.goreduced from 1,076 → 816 linesAlso includes:
/refactor-reviewskill for running refactorings from review reports.claude/reports/added to.gitignoreValidation
make test— all tests pass (no behavior changes)make build— compiles cleanly