Skip to content

Conversation

@jlaneve
Copy link
Owner

@jlaneve jlaneve commented Jul 15, 2025

🎯 Overview

This PR adds a comprehensive diff viewing mode to the CWT TUI, along with mouse scroll support and automatic binary detection for command execution.

✨ Key Features

πŸ“‹ Diff Mode

  • Access: Press v on any session with changes
  • Navigation: Scroll with ↑↓/jk, PgUp/PgDn, or mouse wheel
  • Views: Unified diff with syntax highlighting and line numbers
  • Controls: Toggle cached/working (c), refresh (r), exit (Esc/q)

πŸ–±οΈ Mouse Support

  • Mouse wheel scrolling in diff mode
  • Mouse wheel navigation in main session list
  • Natural scrolling experience

πŸ”§ Self-Detection

  • Auto-detects go run cmd/cwt/main.go vs built binary
  • Switch/merge/publish commands work in both contexts
  • No more "cwt command not found" when running locally

πŸ› οΈ Technical Implementation

Core Components

  • DiffMode state: Manages scroll position and diff data
  • Git diff parsing: Structured DiffLine types with syntax highlighting
  • Mouse events: Bubble Tea mouse wheel support
  • Command utils: Shared ExecuteCWTCommand() with detection logic

Files Changed

  • internal/tui/model.go: Diff mode state and event handling
  • internal/tui/view.go: Diff rendering with lipgloss styling
  • internal/tui/commands.go: Git diff parsing and data loading
  • internal/utils/command.go: Self-detection utility (new)

πŸ§ͺ Testing

Tested with both execution methods:

  • βœ… go run cmd/cwt/main.go (development)
  • βœ… ./cwt (built binary)

πŸ“Έ Demo

The diff mode provides:

  • Syntax highlighted additions (green background)
  • Syntax highlighted deletions (red background)
  • Context lines and line numbers
  • File headers and hunk headers
  • Scroll indicators for long diffs

πŸ”— Related Issues

Fixes the issue where TUI commands (switch/merge/publish) failed when running with go run instead of a built binary.


Ready for review! The diff mode makes it much easier to review changes before deciding to merge or publish sessions. πŸš€

Add comprehensive diff viewing capabilities to the CWT TUI:

## Diff Mode Features
- Press 'v' on sessions with changes to view unified diff
- Syntax highlighting for additions, deletions, and context
- Line numbers for easy reference
- Scroll with ↑↓/jk, PgUp/PgDn, or mouse wheel
- Toggle between working tree and staged changes with 'c'
- Refresh diff with 'r', exit with Esc/q

## Mouse Support
- Enable mouse wheel scrolling in diff mode
- Mouse wheel navigation in main session list
- Natural scrolling experience for reviewing long diffs

## Self-Detection for Commands
- Auto-detect whether running from source or built binary
- Switch/merge/publish commands work with both:
  - go run cmd/cwt/main.go (development)
  - ./cwt or cwt (built binary)
- Created shared utils.ExecuteCWTCommand() for consistency

## Technical Implementation
- New DiffMode state with scroll management
- Git diff parsing with structured DiffLine types
- Lipgloss styling for syntax highlighting
- Mouse event handling in Bubble Tea
- Robust executable detection logic

Fixes the issue where TUI commands failed when running locally
with 'go run' instead of built binary.

πŸ€– Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link
Contributor

claude bot commented Jul 15, 2025

Claude finished @jlaneve's task β€”β€” View job


πŸ” Code Review: TUI Diff Mode & Self-Detection

I've thoroughly analyzed the implementation and have several observations across code quality, potential issues, performance, security, and testing:

βœ… Strengths

Code Quality & Design

  • Well-structured diff parsing: parseDiffOutput() properly handles all git diff line types with clear state tracking
  • Clean separation of concerns: Diff functionality properly separated across commands, model, and view layers
  • Consistent error handling: Good use of error wrapping pattern throughout (fmt.Errorf("context: %w", err))
  • Intuitive UX: Mouse wheel support and keyboard navigation provide excellent user experience

Self-Detection Implementation

  • Robust detection logic: GetCWTCommand() covers multiple execution contexts (go run, built binary, PATH lookup)
  • Fallback strategy: Progressive fallbacks ensure the utility works in various environments

⚠️ Potential Issues & Improvements

Bug Risks

1. Race Condition in File Watching (internal/tui/commands.go:70-144)

// Potential issue: goroutines writing to unbuffered channel
select {
case eventChan <- sessionStateChangedMsg{}:
default: // Channel full, skip this event
}
  • Issue: Multiple goroutines could access eventChan concurrently without proper synchronization
  • Recommendation: Consider using proper channel synchronization or atomic operations for channel state

2. Directory Change Side Effects (internal/tui/commands.go:424-438)

originalDir, err := os.Getwd()
defer os.Chdir(originalDir)
if err := os.Chdir(worktreePath); err != nil {
    return diffErrorMsg{err: fmt.Errorf("failed to change to worktree directory: %w", err)}
}
  • Issue: If defer doesn't execute due to panic, working directory won't be restored
  • Recommendation: Use absolute paths instead of changing directories, or ensure panic recovery

3. Command Injection Vulnerability (internal/utils/command.go:56)

cmd := exec.Command(baseCmd[0], fullArgs...)
  • Issue: While baseCmd is controlled, args could potentially contain malicious input
  • Status: Low risk since args come from internal TUI, but worth validating

Performance Considerations

1. Memory Usage in Diff Parsing (internal/tui/commands.go:460-535)

  • Large diffs create many DiffLine structs simultaneously
  • Recommendation: Consider streaming/pagination for very large diffs

2. Git Command Execution (internal/tui/commands.go:441-456)

  • Synchronous git commands block UI thread
  • Recommendation: Already using Bubble Tea's async command pattern correctly βœ…

Code Quality Improvements

1. Magic Numbers (internal/tui/model.go:24)

ScrollAmount = 10 // Number of lines to scroll in diff view
  • Suggestion: Consider making scroll amount configurable or adaptive to terminal height

2. Error Message Truncation Logic (internal/tui/view.go:502-554)

  • Complex word-wrapping logic could be extracted to a utility function
  • Improvement: Consider using a dedicated text-wrapping library

3. Debug Logging Pattern (internal/tui/model.go:27-34)

logFile, err := os.OpenFile("cwt-tui-debug.log", os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0666)
  • Security concern: File permissions 0666 are overly permissive
  • Recommendation: Use 0644 for debug logs

πŸ”’ Security Assessment

Low Risk Issues

  1. File permissions: Debug log file created with 0666 permissions
  2. Path handling: Generally safe due to controlled internal usage
  3. Command execution: Limited to controlled CWT commands

Recommendations

  • Tighten debug log file permissions to 0644
  • Add input validation for any user-provided paths (though current implementation looks safe)

πŸ§ͺ Test Coverage Assessment

Missing Test Coverage

The PR adds significant functionality without corresponding tests:

  1. Diff parsing logic (parseDiffOutput()) - Critical functionality that should have unit tests
  2. Command detection (GetCWTCommand()) - Complex logic with multiple code paths
  3. Mouse event handling - User interaction that should be tested
  4. Error scenarios - File system errors, git command failures

Recommended Tests

// Example test cases needed:
func TestParseDiffOutput(t *testing.T) {
    // Test various git diff outputs
    // Test edge cases (empty diff, binary files, etc.)
}

func TestGetCWTCommand(t *testing.T) {
    // Test go run detection
    // Test binary detection
    // Test fallback scenarios
}

🎯 Specific Recommendations

High Priority

  1. Fix directory change pattern in loadDiffData() - use absolute paths
  2. Add unit tests for diff parsing and command detection
  3. Tighten file permissions for debug logs

Medium Priority

  1. Extract text wrapping utility from view rendering
  2. Add input validation for git diff target parameter
  3. Consider memory optimization for large diff handling

Low Priority

  1. Make scroll amount configurable
  2. Add more detailed error messages for git command failures

πŸ† Overall Assessment

This is a well-implemented feature that significantly enhances the TUI experience. The code follows good Go practices and integrates well with the existing architecture. The main concerns are around test coverage and a few reliability edge cases.

Rating: 8/10 - Excellent feature implementation with room for defensive improvements.

The diff view functionality is intuitive and the self-detection solves a real usability problem. With the recommended fixes for directory handling and additional test coverage, this would be production-ready.


jlaneve and others added 3 commits July 15, 2025 00:23
- Fix code formatting issues identified by CI
- Add missing newline at end of command.go
- All files now pass gofmt and go vet checks

πŸ€– Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Fix final gofmt formatting issue
- Ensure proper file ending without extra blank lines

πŸ€– Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
…andling

- Fix cross-platform temp directory detection in utils/command.go
- Add git repository validation before changing directories
- Change diff target from 'main' to 'origin/main' for remote comparison
- Add ScrollAmount constant to replace magic numbers
- Improve error messages by capturing stderr in ExecuteCWTCommand

πŸ€– Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@jlaneve jlaneve merged commit 605986e into main Jul 15, 2025
4 checks passed
@jlaneve jlaneve deleted the feature/tui-diff-mode branch July 15, 2025 04:44
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