feat: add width controls for list command output #39
Conversation
- Add --compact flag to minimize column widths - Add --max-path-width flag to cap PATH column width (default: 56) - Support WTP_LIST_MAX_PATH environment variable - Auto-enable compact mode on non-TTY output (pipes, redirects) - Auto-enable compact mode on super-wide terminals (≥160 columns) - Improve readability on very wide terminals by preventing excessive padding Closes #29
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds width-aware rendering to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI
participant Cmd as listCommandWithCommandExecutor
participant Resolver as resolveListDisplayOptions
participant Renderer as displayWorktreesRelative
CLI->>Cmd: run `wtp list` (flags/env)
Cmd->>Resolver: derive options (flags, WTP_LIST_MAX_PATH, detect TTY & term width)
Resolver->>Resolver: parse values, detect terminal width
Resolver-->>Cmd: return listDisplayOptions + termWidth
Cmd->>Renderer: render worktrees with termWidth + opts
Renderer->>Renderer: collectListDisplayData()
Renderer->>Renderer: computeListColumnWidths() and clamp widths
Renderer->>Renderer: format rows (truncate PATH/BRANCH/STATUS)
Renderer-->>Cmd: formatted output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR adds display customization options to the list command, allowing users to control the width of the PATH column and enable compact mode for better readability on various terminal sizes.
- Introduces
--compactand--max-path-widthflags for the list command - Implements automatic compact mode for non-TTY output and super-wide terminals (>= 160 columns)
- Adds support for
WTP_LIST_MAX_PATHenvironment variable to set default max path width
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/wtp/list.go | Adds display options handling, path width calculation logic, and new command flags for compact mode and max path width |
| cmd/wtp/list_test.go | Updates all test calls to include display options and adds comprehensive tests for new display behaviors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/wtp/list.go
Outdated
| if pathWidth < minCompactWidth { | ||
| pathWidth = minCompactWidth | ||
| } |
There was a problem hiding this comment.
Lines 382-387 are redundant. After line 383 sets pathWidth = minCompactWidth, line 385 checks if pathWidth < minCompactWidth, which can never be true. Lines 385-387 should be removed.
| if pathWidth < minCompactWidth { | |
| pathWidth = minCompactWidth | |
| } |
cmd/wtp/list.go
Outdated
| termWidth = 80 | ||
| } | ||
| // If branch column is too wide, limit it as well | ||
| maxAvailableForBranch := termWidth - minPathWidth - spacing - maxStatusLen - spacing - spacing - headWidth |
There was a problem hiding this comment.
The spacing constant is subtracted three times but written as spacing - spacing - spacing. This should be simplified to (spacing * 3) or (3 * spacing) for clarity and to avoid confusion.
| maxAvailableForBranch := termWidth - minPathWidth - spacing - maxStatusLen - spacing - spacing - headWidth | |
| maxAvailableForBranch := termWidth - minPathWidth - (3 * spacing) - maxStatusLen - headWidth |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/wtp/list.go (1)
345-412: Consider simplifying the width calculation logic.The path width calculation has multiple conditional branches with overlapping bounds checks that make the logic difficult to follow. For example:
- Lines 382-387: Sets
pathWidthtominCompactWidth, then immediately checks if it's less thanminCompactWidth(redundant)- Lines 404-412: Performs additional bounds checking that overlaps with earlier constraints
- Overall flow: calculate → cap → apply mode logic → re-cap → final bounds
While functionally correct, consolidating the bounds checking and reducing branching would improve maintainability. Consider calculating the constraints (min, max, desired) first, then applying them in a single pass.
Example refactoring approach:
// Calculate constraints minWidth := max(pathHeaderWidth, 1) maxWidth := min(availableForPath, opts.MaxPathWidth) // Calculate desired width based on mode var desiredWidth int if opts.Compact { desiredWidth = max(minWidth, maxPathLen) } else { desiredWidth = max(minPathWidth, maxPathLen+2) } // Apply constraints pathWidth := clamp(desiredWidth, minWidth, maxWidth)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/wtp/list.go(8 hunks)cmd/wtp/list_test.go(18 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/wtp/list_test.go (2)
internal/command/types.go (2)
Result(11-15)Command(4-8)internal/config/config.go (2)
Config(12-16)Defaults(19-21)
🪛 GitHub Check: Lint
cmd/wtp/list_test.go
[failure] 141-141:
The line is 124 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 210-210:
The line is 124 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 255-255:
The line is 122 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 277-277:
The line is 122 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 338-338:
The line is 124 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 406-406:
The line is 124 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 456-456:
The line is 122 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 487-487:
The line is 122 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 732-732:
The line is 124 characters long, which exceeds the maximum of 120 characters. (lll)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL analysis (go)
🔇 Additional comments (5)
cmd/wtp/list_test.go (2)
18-40: LGTM! Well-designed test helpers.The
defaultListDisplayOptionsForTests()andextractPathColumnWidth()helpers provide consistent test setup and enable verification of the new width control behavior. The approach of extracting the path column width from formatted output is appropriate for validating display formatting.
841-1000: LGTM! Comprehensive test coverage for new display features.The five new tests thoroughly validate the width control functionality:
- Path column capping at the default width
- Auto-compact activation for super-wide terminals (≥160 columns)
- Auto-compact activation for non-TTY output
- Explicit
--compactflag behavior- Custom
--max-path-widthconfigurationThe tests appropriately mock terminal width and verify the resulting column widths.
cmd/wtp/list.go (3)
29-32: LGTM! Clean API design for width controls.The new constants, CLI flags, and
listDisplayOptionsstruct provide a clear and well-documented interface for controlling list output formatting. The default values (max path width of 56, super-wide threshold of 160) are reasonable for typical terminal usage.Also applies to: 64-73, 441-445
447-472: LGTM! Robust option resolution with proper precedence.The
resolveListDisplayOptionsfunction correctly:
- Prioritizes CLI flags over environment variables (
WTP_LIST_MAX_PATH)- Validates that
MaxPathWidthis positive- Detects TTY status for auto-compact behavior
- Handles edge cases like invalid environment variable values
The precedence order (CLI flag → env var → default) follows standard CLI conventions.
154-165: LGTM! Appropriate auto-compact behavior.The auto-compact logic correctly:
- Enables compact mode automatically for non-TTY output (redirected/piped)
- Enables compact mode for super-wide terminals (≥160 columns)
- Respects explicit user choice (only auto-enables when
--compactflag wasn't set)Minor note: Lines 155-157 perform redundant validation already done in
resolveListDisplayOptions(lines 456-458), but this defensive check is harmless.
- Extract helper functions to satisfy gocyclo lint rules - Split displayWorktreesRelative into smaller focused functions - Add named return values for clarity - Format long lines to meet lll (120 char) requirement - Fix clampCompactPathWidth to respect currentWidth properly
- Replace manual if comparisons with built-in min/max functions - Simplify clampCompactPathWidth and clampExpandedPathWidth - More readable and idiomatic Go 1.21+ code
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/wtp/list.go (1)
402-402: Simplify spacing arithmetic for clarity.The spacing constant is subtracted three times in a non-contiguous pattern. Grouping the spacing terms makes the intent clearer.
Apply this diff:
- maxAvailableForBranch := termWidth - minPathWidth - spacing - statusWidth - spacing - spacing - headWidth + maxAvailableForBranch := termWidth - minPathWidth - (3 * spacing) - statusWidth - headWidth
🧹 Nitpick comments (1)
cmd/wtp/list.go (1)
421-421: Simplify spacing arithmetic for clarity.Similar to line 402, the spacing is subtracted three times non-contiguously. Grouping improves readability.
Apply this diff:
- availableForPath := max(termWidth-spacing-branchWidth-spacing-statusWidth-spacing-headWidth, pathHeaderWidth) + availableForPath := max(termWidth-(3*spacing)-branchWidth-statusWidth-headWidth, pathHeaderWidth)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/wtp/list.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/wtp/list.go (1)
cmd/wtp/list_test.go (3)
TestListCommand_RelativePathDisplay(582-718)TestListCommand_LongPaths(325-389)TestListCommand_TerminalWidthTruncation(720-814)
🔇 Additional comments (6)
cmd/wtp/list.go (6)
9-9: LGTM! Constants and import are appropriate.The
strconvimport and display constants are well-chosen for the width control feature.Also applies to: 29-33
65-74: LGTM! CLI flags are well-defined.The new flags provide clear control over display formatting with appropriate defaults and help text.
113-114: LGTM! Display options integration is correct.The option resolution and auto-compact logic work as intended, providing adaptive behavior for different terminal widths and output types.
Also applies to: 121-121, 126-126, 155-166
276-310: LGTM! Display refactor is well-structured.The function cleanly separates data collection, width computation, and rendering with appropriate width enforcement via truncation.
312-368: LGTM! Data collection is clean and efficient.The data structures and collection logic properly track display content and metrics needed for width computation.
454-485: LGTM! Options resolution is robust and well-designed.The function properly prioritizes CLI flags over environment variables, validates inputs, and safely detects TTY status.
- Move minPathWidth, columnSpacing to package constants - Remove duplicate const blocks in helper functions - Single source of truth for layout dimensions
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
- Move MaxPathWidth application after mode-specific width adjustment - Ensures user-specified width limit is respected - Fixes --max-path-width flag not taking effect
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/wtp/list.go (1)
385-389: Duplicate constants still present.Despite being marked as addressed in a previous review, these local constants duplicate file-level definitions and create inconsistent naming:
minPathWidth(line 386) duplicates line 33spacing(line 388) duplicatescolumnSpacing(line 34) with different namingheadWidth(line 387) is redundant withheadDisplayLength(line 25)The function
derivePathWidthalready uses the file-level constants correctly (lines 417-418). This function should do the same.Apply this diff to remove the local const block and use file-level constants:
func clampBranchAndStatusWidths( maxBranchLen, maxStatusLen, termWidth int, ) (branchWidth, statusWidth int) { - const ( - minPathWidth = 20 - headWidth = headDisplayLength - spacing = 3 - ) - branchWidth = maxBranchLen statusWidth = maxStatusLen branchHeaderWidth := len("BRANCH") statusHeaderWidth := len("STATUS") if branchWidth < branchHeaderWidth { branchWidth = branchHeaderWidth } if statusWidth < statusHeaderWidth { statusWidth = statusHeaderWidth } - maxAvailableForBranch := termWidth - minPathWidth - spacing - statusWidth - spacing - spacing - headWidth + maxAvailableForBranch := termWidth - minPathWidth - columnSpacing - statusWidth - columnSpacing - columnSpacing - headDisplayLength if branchWidth > maxAvailableForBranch { branchWidth = maxAvailableForBranch if branchWidth < branchHeaderWidth { branchWidth = branchHeaderWidth } } return branchWidth, statusWidth }
🧹 Nitpick comments (2)
cmd/wtp/list.go (2)
158-160: Consider removing redundant check.The
resolveListDisplayOptionsfunction (lines 467-469) already ensuresopts.MaxPathWidthis never <= 0, so this check is redundant.Apply this diff to remove the redundant check:
- if opts.MaxPathWidth <= 0 { - opts.MaxPathWidth = defaultMaxPathWidth - }
433-434: Consider removing redundant boundary check.Line 433 already ensures
pathWidth >= pathHeaderWidth(which is 4), so the subsequentmax(pathWidth, 1)on line 434 is redundant.Apply this diff to simplify:
pathWidth = max(min(pathWidth, availableForPath), pathHeaderWidth) - pathWidth = max(pathWidth, 1) return pathWidth
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/wtp/list.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/wtp/list.go (2)
internal/config/config.go (1)
Config(12-16)internal/git/worktree.go (1)
Worktree(14-19)
🔇 Additional comments (7)
cmd/wtp/list.go (7)
67-76: LGTM! Well-structured flag definitions.The new flags are clearly defined with appropriate defaults, aliases, and usage descriptions. The integration with the existing flag structure is clean.
115-123: Clean integration of display options.The display options are resolved early and propagated cleanly through the command execution flow.
161-167: Auto-enable compact mode logic is correct.The logic correctly auto-enables compact mode for non-TTY output and super-wide terminals (≥160 columns) as specified in the PR objectives.
280-312: Width-aware rendering is well-implemented.The rendering logic correctly:
- Collects display data and metrics
- Computes appropriate column widths based on terminal size and options
- Applies truncation to fit content within computed widths
- Maintains alignment across all rows
314-370: Data collection logic is sound.The new data structures and collection function provide a clean separation between data gathering and rendering. The metrics tracking correctly captures maximum column widths needed for proper formatting.
415-437: Width derivation logic is well-structured.The function correctly:
- Calculates available space
- Applies appropriate clamping based on compact vs expanded mode
- Enforces the MaxPathWidth cap
- Ensures bounds are respected
The separation between compact and expanded clamping logic provides clear, maintainable behavior.
458-483: Display options resolution is robust.The function correctly:
- Prioritizes explicit CLI flags over environment variables
- Safely parses environment variable with error handling
- Ensures MaxPathWidth is always positive
- Properly detects TTY status for adaptive behavior
The logic handles all edge cases appropriately.
Summary
Fixes #29 - Improves readability of
wtp listoutput on very wide terminals by adding width control options.Changes
--compactflag (-c) to minimize column widths for narrow or redirected output--max-path-widthflag to cap PATH column width (default: 56 characters)WTP_LIST_MAX_PATHenvironment variable for customizing the default capTesting
TestListCommand_PathColumnCappedOnWideTerminalsTestListCommand_AutoCompactForSuperWideTerminalsTestListCommand_AutoCompactForNonTTYTestListCommand_CompactFlagTestListCommand_CustomMaxPathWidthExamples
Before (very wide terminal)
After (same wide terminal, automatic compact)
With custom width cap
Backward Compatibility
Summary by CodeRabbit
New Features
Improvements
Tests