Skip to content

Release: v0.6.3#92

Merged
yimsk merged 2 commits intomainfrom
develop
Jan 3, 2026
Merged

Release: v0.6.3#92
yimsk merged 2 commits intomainfrom
develop

Conversation

@yimsk
Copy link
Copy Markdown
Contributor

@yimsk yimsk commented Jan 3, 2026

Changes since v0.6.2

New Features

Refactoring

Testing

  • 25 new style helper tests
  • All tests passing ✅

Closes #29, #87, #88, #89

yimsk added 2 commits January 3, 2026 22:20
* feat: native CloudWatch Logs viewer (issue #29)

Replace external 'aws logs tail' dependency with internal TUI log viewer.

- Add LogView with FilterLogEvents polling (3s interval)
- Support both log-groups and log-streams
- Space to pause/resume, g/G for top/bottom, c to clear
- Remove AWS CLI and less dependencies

* refactor: use Navigation instead of Action for log viewing

Move log viewing from Action menu to Navigation shortcut:
- Press 't' directly from log-groups/log-streams to open LogView
- Remove ActionTypeView handling from action_menu
- Cleaner UX: no action menu step needed

* test: add LogView tests + fix data race in fetchLogs

* fix: add ready flag to LogView, remove ActionTypeView dead code

- Add ready flag to LogView to prevent uninitialized viewport access
- Remove ActionTypeView constant and related IsAllowedInReadOnly case
- Remove obsolete ActionTypeView test cases
- Remove beads issue tracking setup (not using)

* fix: align LogView with codebase patterns (error handling, AWS helpers)

- LogView: use apperrors.Wrap instead of fmt.Errorf
- LogView: use appaws.* helpers instead of raw aws.*
- LogStreamDAO.Delete: add IsNotFound check for idempotency
- helpers.go: add Int64Ptr for consistency with Int32Ptr

* fix: LogView SetSize/ViewString patterns for consistency with other views

* fix: add context timeout and throttling backoff to LogView

- Add 10s timeout for FilterLogEvents API calls
- Add exponential backoff on throttling (up to 30s)
- Reset poll interval on success
- Add godoc for LogView struct

* fix: clear error state on successful log fetch

* chore: ignore .gtrconfig

* fix: add context cancellation check and extract magic numbers

- Check parent context before starting fetch to prevent resource leaks
- Extract buffer sizes and fetch limit to named constants

* fix: stop polling on non-throttling errors

Only continue polling with backoff on throttling errors.
Other errors (access denied, etc.) stop polling to avoid spamming.

* fix: add context check to initClient and throttle indicator

- Check context cancellation before initializing AWS client
- Show throttle status with backoff interval in StatusLine

* fix: unwrap resource before type assertion in createLogView

Fixes multi-region mode where resources are wrapped with RegionalDAOWrapper.
Without unwrapping, type assertions for logGroupProvider/logStreamProvider fail.

* feat: add 'p' key to load older logs and fix old stream viewing

- Add 'p' key to load 1 hour of older logs (prepends to buffer)
- For log streams, use lastEventTimestamp as starting point
- Track oldestEventTime to enable backward navigation
- Fixes issue where old streams showed 'no log events'

* refactor: improve LogView consistency and add configurable timeout

- Use ui.NewSpinner() for consistent spinner initialization
- Extract doFetchLogs() to eliminate code duplication (75% reduction)
- Add LogFetchTimeout to config system (default: 10s)
- Add IsNotFound/IsAccessDenied error classification for better UX
- Replace errors.New() with apperrors.Wrap() for consistency
- Document viewportHeaderOffset magic number

* refactor: remove dead code and restore doc comments

- Remove unused Action.Target field
- Remove unused LogView.width/height fields
- Add LogFetch to DefaultFileConfig for consistency
- Restore doc comments for Navigation, ViewTypeLogView, HandleKey

* fix: add UnwrapResource for multi-region support in cloudwatch resources

* refactor: align LogView with other viewport-based views

- Add width/height fields for consistency with DetailView, DiffView, etc.
- Remove HasActiveInput() since views without filter don't implement it
…88 #89) (#91)

* refactor: extract ViewportState, add common styles, remove wrapper (#87 #88 #89)

- Add ViewportState type to eliminate duplicated viewport init pattern (6 views)
- Add TitleStyle, SelectedStyle, TableHeaderStyle to theme.go
- Remove unnecessary getCloudWatchLogsClient wrapper in log-streams

* refactor: use theme style helpers across views (#88)

* refactor: extend theme style helpers to app and render packages (#88)

* refactor: apply style helpers and Go 1.21+/1.22+ idioms (#88)

- service_browser: use TableHeaderStyle() and SelectedStyle()
- use max()/min() builtins instead of if statements
- use range over int for loop modernization

* refactor: complete style helpers and Go 1.21+ idioms (#88)

- Add 6 new theme helpers: SectionStyle, HighlightStyle, BoldSuccessStyle,
  BoldDangerStyle, BoldWarningStyle, BoldPendingStyle
- Apply helpers across 8 view files for consistency
- Fix hardcoded #ff0000 color in exec_with_header.go
- Use max()/min() builtins in dashboard_view_panels.go
- Use range-over-int in dashboard_view.go and dashboard_view_panels.go

* refactor: remove dead code and expand style helper usage (#88)

- Remove unused width/height fields from 5 view structs
- Apply DimStyle() across 11 locations
- Apply SuccessStyle/WarningStyle/DangerStyle where applicable
- Update tests to match field removal

* refactor: add MutedStyle, LoadingMessage const, cmp.Or, remove dead code (#88)

* refactor: add AccentStyle, slices.Collect, remove dead fields (#88)

* refactor: add TextStyle/SecondaryStyle/BorderStyle/PrimaryStyle, remove dead code (#88)

* refactor: add InfoStyle/PendingStyle/TextBrightStyle, extend helpers to render/action (#88)

* refactor: remove render style wrappers, add BoxStyle/InputStyle, consolidate truncate (#88)

* refactor: add InputFieldStyle, cache tag_search_view styles (#88)
@claude
Copy link
Copy Markdown

claude Bot commented Jan 3, 2026

Pull Request Review: v0.6.3 Release

I've completed a comprehensive review of this release PR. Overall, this is an excellent release with high-quality implementation, thoughtful refactoring, and strong test coverage. Here's my detailed feedback:


✅ Strengths

1. Native CloudWatch Logs Viewer (#86)

The new LogView implementation is exceptionally well-designed:

  • Clean Architecture: Proper separation of concerns with doFetchLogs() eliminating ~75% code duplication
  • Excellent Error Handling: Proper classification of errors (NotFound, AccessDenied, Throttling) with appropriate user feedback
  • Smart Throttling: Exponential backoff (up to 30s) prevents API quota exhaustion
  • Resource Management: Context cancellation checks prevent resource leaks
  • User Experience:
    • Real-time streaming with pause/resume
    • Load older logs with 'p' key
    • Clear buffer management (max 1000 lines)
    • Helpful status indicators (STREAMING/PAUSED/THROTTLED)

Code Quality: internal/view/log_view.go:144-229 demonstrates excellent defensive programming patterns.

2. Style Helpers Consolidation (#91)

Outstanding refactoring work:

  • 25 unified style helpers in ui package with comprehensive test coverage (168 test lines)
  • Consistency: Eliminated hardcoded colors across 20+ files
  • Maintainability: Single source of truth for theming in internal/ui/theme.go
  • Type Safety: All helpers return lipgloss.Style for compile-time safety

3. ViewportState Abstraction

Smart DRY refactoring:

  • Before: 6 views duplicated viewport initialization logic
  • After: Centralized in ViewportState type (internal/view/viewport_helper.go)
  • Clean API: Simple SetSize() method handles lazy initialization

4. Modern Go Idioms

Great use of Go 1.21+/1.22+ features:

  • max(), min() builtins replace verbose if statements
  • cmp.Or() for default value selection
  • Range-over-int for cleaner loops
  • Shows commitment to modern best practices

5. Test Coverage

Comprehensive testing:

  • 286 lines of LogView tests covering all edge cases
  • 168 lines of theme helper tests
  • All 25 style helpers have test coverage
  • Tests cover error states, buffer trimming, pause/resume, viewport operations

🔍 Code Quality Observations

Excellent Patterns Found

  1. UnwrapResource Pattern (internal/dao/dao.go:182-190):

    func UnwrapResource(res Resource) Resource {
        if pr, ok := res.(*ProfiledResource); ok {
            return pr.Resource
        }
        if rr, ok := res.(*RegionalResource); ok {
            return rr.Resource
        }
        return res
    }
    • Critical fix for multi-region support
    • Applied consistently across 8 render files
    • Properly tested with multiple wrapping levels
  2. Configuration Integration (internal/config/file.go:44):

    • New LogFetchTimeout properly integrated into config system
    • Sensible default (10s) with user configurability
    • Documented in README.md
  3. Dead Code Removal:

    • Removed unused Action.Target field
    • Removed ActionTypeView (replaced with proper Navigation)
    • Cleaned up obsolete action menu handling
    • -660 lines total (shows discipline in cleanup)

🎯 Minor Suggestions (Non-blocking)

1. LogView Context Handling

Location: internal/view/log_view.go:127-131

The fetchLogsCmd() captures startTime at function call time:

func (v *LogView) fetchLogsCmd() tea.Cmd {
    startTime := v.lastEventTime
    return func() tea.Msg {
        return v.doFetchLogs(startTime, 0, false)
    }
}

Observation: This is actually correct! It prevents race conditions by capturing the timestamp when the command is created rather than when it executes. Good defensive programming.

2. Error Message Consistency

Location: internal/view/log_view.go:149-152

Using fmt.Errorf() then wrapping with apperrors.Wrap():

err: apperrors.Wrap(fmt.Errorf("CloudWatch Logs client not initialized"), "fetch logs")

Suggestion: Consider apperrors.New() if it exists, or just:

err: apperrors.Wrap(errors.New("CloudWatch Logs client not initialized"), "fetch logs")

This is very minor - current approach works fine.

3. Magic Number Documentation

Location: internal/view/log_view.go:29

Great job documenting the magic number:

viewportHeaderOffset   = 4 // header(1) + status(2) + spacing(1)

This is exactly what code comments should look like - clear rationale for the value.


🔒 Security Review

No security concerns identified:

✅ Context timeouts prevent indefinite hangs
✅ Buffer limits prevent memory exhaustion
✅ AWS credentials handled via SDK (not exposed)
✅ No user input used in API calls without validation
✅ Error messages don't leak sensitive information


📊 Performance Considerations

Excellent performance characteristics:

  1. Buffer Management: Hard limit at 1000 entries prevents unbounded memory growth
  2. Polling Interval: 3s default with exponential backoff on throttling
  3. Lazy Initialization: ViewportState only creates viewport when needed
  4. API Efficiency: Proper use of StartTime filtering to avoid duplicate fetches

No performance issues identified.


🧪 Test Coverage Assessment

Rating: Excellent

  • LogView: 286 test lines covering 11 test cases
    • Initialization, error handling, buffer trimming, pause/resume, navigation
  • Theme helpers: 168 test lines covering all 25 helpers
  • Existing tests updated to reflect field removals (dead code cleanup)

All critical paths are tested.


📝 Documentation

Good documentation practices:

✅ README.md updated with new log_fetch timeout config
✅ Godoc comments added for LogView struct
✅ Magic numbers explained with inline comments
✅ PR description provides excellent changelog


🎉 Final Assessment

This is a production-ready release with:

  • ✅ High code quality
  • ✅ Comprehensive test coverage
  • ✅ No security concerns
  • ✅ Good performance characteristics
  • ✅ Excellent refactoring discipline
  • ✅ Modern Go idioms
  • ✅ Clean architecture

Recommendation: APPROVE AND MERGE 🚀

The native CloudWatch Logs viewer is a significant UX improvement (removes AWS CLI dependency), and the style helpers refactoring will make future development much cleaner.

Great work! This release sets a high bar for code quality.


📋 Nitpicks (Optional)

If you want to be extra thorough:

  1. Consider adding a test for ViewportState in viewport_helper.go (it's simple but untested)
  2. The .gtrconfig gitignore entry - what tool is this for? (Just curious)

Neither is blocking - ship it! 🎊

@yimsk yimsk merged commit 4398d8c into main Jan 3, 2026
9 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.

Feature Proposal: Internal Log Streamer

1 participant