CloudWatch Logs filter (v0.9.1)

[yimsk]

Summary

Client-side substring filter for CloudWatch Logs tail view

Changes

• Filter activation: / key, real-time filtering, Esc/Enter
• Visual indicators: 🔍 filter display, (n/m lines) count
• Two-stage clear: c clears filter → buffer
• Comprehensive test coverage (7 new tests)

Implementation Details

• Client-side case-insensitive substring match
• Width/height tracking for viewport recalc
• Min width check (10 chars) for narrow terminals
• Long filter truncation (>20 chars) in status line

Testing

• ✅ All existing tests pass
• ✅ 7 new filter tests added
• ✅ Performance verified (1000 logs = no lag)

Review

• PR Add filter to CloudWatch Logs tail view #121 merged to develop
• All review feedback addressed

Closes #120

🤖 Generated with Claude Code

[claude]

PR Review: CloudWatch Logs Filter (v0.9.1)

Summary

This PR adds client-side substring filtering to the CloudWatch Logs tail view. Overall, this is a well-implemented feature with excellent test coverage. The code follows the repository's patterns and integrates cleanly with the existing TUI framework.

✅ Strengths

1. Excellent Test Coverage

• 7 comprehensive tests covering activation, matching, clearing, counting, and edge cases
• Unicode/emoji handling explicitly tested (great attention to detail!)
• Tests follow existing patterns in the codebase
• All tests are well-named and have clear assertions

2. Consistent Architecture

• Follows the same patterns as ResourceBrowser for filter input
• Proper use of Bubbletea's textinput component
• Consistent with the codebase's style and conventions
• HasActiveInput() method properly implemented for input handling coordination

3. User Experience

• Real-time filtering (updates as user types) - excellent UX
• Visual feedback with 🔍 indicator and filtered count display
• Two-stage clear behavior (filter → buffer) is intuitive
• Proper truncation of long filters in status line

4. Code Quality

• Well-organized constants with clear documentation
• No code duplication
• Proper separation of concerns (matchesFilter, getDisplayedCount, handleFilterInput)
• Clean variable names and structure

🔍 Observations & Suggestions

1. Performance Consideration (Minor)

Location: log_view.go:397-413, log_view.go:501-509

The filter is applied twice per render cycle:

• Once in updateViewportContent() - iterates through all logs to build content
• Once in getDisplayedCount() - iterates through all logs again to count matches

Suggestion: Consider caching the filtered count when updating viewport content:

func (v *LogView) updateViewportContent() {
    var sb strings.Builder
    v.displayedCount = 0  // Add field to struct
    
    for _, entry := range v.logs {
        if !v.matchesFilter(entry) {
            continue
        }
        v.displayedCount++
        ts := v.styles.timestamp.Render(entry.timestamp.Format("15:04:05.000"))
        msg := v.styles.message.Render(entry.message)
        sb.WriteString(fmt.Sprintf("%s %s\n", ts, msg))
    }
    v.vp.Model.SetContent(sb.String())
}

func (v *LogView) getDisplayedCount() int {
    if v.filterText == "" {
        return len(v.logs)
    }
    return v.displayedCount
}

Impact: Low priority - only matters with >10k logs, but worth considering for consistency with the "no lag" performance claim.

2. Case-Insensitive Filter Allocation (Minor)

Location: log_view.go:392-400

The filter creates a new lowercase string for each log entry:

filter := strings.ToLower(v.filterText)
msg := strings.ToLower(entry.message)

Suggestion: Pre-compute the lowercase filter once and store it:

// Add to struct
filterTextLower string

// In handleFilterInput and filter clear
v.filterText = v.filterInput.Value()
v.filterTextLower = strings.ToLower(v.filterText)

// In matchesFilter
func (v *LogView) matchesFilter(entry logEntry) bool {
    if v.filterText == "" {
        return true
    }
    msg := strings.ToLower(entry.message)
    return strings.Contains(msg, v.filterTextLower)
}

Impact: Micro-optimization, reduces allocations from O(n) to O(1) per filter update.

3. Test Coverage Gap (Informational)

Location: log_view_test.go

Missing test cases (not critical, but would be nice to have):

• Filter behavior during real-time log streaming (new logs arriving while filter is active)
• Filter interaction with pause/resume state
• Filter persistence across viewport scrolling (g/G keys)
• Edge case: empty logs array with active filter

4. Viewport Height Recalculation (Minor Logic Question)

Location: log_view.go:329-351

When clearing the filter with c, the code calls:

v.SetSize(v.width, v.height) // Recalculate viewport height

Question: Is this necessary? The viewport height should auto-adjust on the next render cycle when ViewString() is called. This might be premature optimization.

Suggestion: Consider removing unless there's a specific visual glitch this fixes. If kept, add a comment explaining why immediate recalculation is needed.

5. Status Line Filter Display Truncation (Potential UX Issue)

Location: log_view.go:546-551

The truncation logic uses rune slicing but doesn't account for zero-width characters or combining marks:

filterDisplay := v.filterText
runes := []rune(filterDisplay)
if len(runes) > maxFilterDisplayLength {
    filterDisplay = string(runes[:maxFilterDisplayLength-3]) + "..."
}

Suggestion: While the Unicode test passes, consider using a library like uniseg for proper grapheme cluster handling if you encounter issues with complex Unicode (e.g., emoji with skin tone modifiers, combining diacritics).

Impact: Very low - current implementation is fine for 99% of use cases.

🔒 Security

✅ No security concerns identified:

• Filter is client-side only, no API calls
• No user input passed to external commands
• No injection vulnerabilities

🎯 Best Practices Alignment

Follows Repository Conventions:

• ✅ Consistent with ResourceBrowser filter implementation
• ✅ Proper use of Bubbletea message handling
• ✅ Style caching via struct fields (good for performance)
• ✅ Proper use of ui package for styling
• ✅ Follows the established key binding patterns

Code Style:

• ✅ Proper Go formatting
• ✅ Clear variable names
• ✅ Appropriate use of comments for constants
• ✅ No unnecessary complexity

📊 Test Results

Based on the PR description:

• ✅ All existing tests pass
• ✅ 7 new tests added and passing
• ✅ Performance verified (1000 logs with no lag)

🎉 Conclusion

Overall Assessment: LGTM with minor suggestions

This is a solid implementation that adds real value to the CloudWatch Logs experience. The code quality is high, test coverage is excellent, and it follows all the repository's established patterns.

Recommendation: ✅ Approve

The suggestions above are minor optimizations that can be addressed in future PRs if needed. None of them are blockers for merging.

Priority of Suggestions:

1. 🟡 Performance: Cached filtered count (nice to have)
2. 🟡 Performance: Pre-compute lowercase filter (micro-optimization)
3. 🟢 Tests: Additional edge cases (future enhancement)
4. 🟢 Logic: Viewport recalculation necessity (clarification)
5. 🟢 UX: Advanced Unicode handling (edge case)

Great work! 🚀

---

Review generated by Claude Code