Refine list column width calculations #40

satococoa · 2025-11-01T14:27:33Z

Summary

reuse shared constants for spacing and head width in the list command
drop redundant MaxPathWidth and path width clamps
derive total spacing via columnSpacingSlots to satisfy linting

Testing

go test ./cmd/wtp
go tool task dev

Summary by CodeRabbit

Refactor
- Reorganized internal width and spacing calculations for list output formatting. No changes to user-visible behavior or command output.

coderabbitai · 2025-11-01T14:27:42Z

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

Refactors width and spacing calculations in the list rendering logic by introducing columnSpacingSlots and computed spacingTotal, removing hard-coded constants (minPathWidth, headWidth, spacing), and adjusting path width clamping behavior in derivePathWidth.

Changes

Cohort / File(s)	Change Summary
List rendering width refactor `cmd/wtp/list.go`	Replaces hard-coded spacing/width constants with `columnSpacingSlots` and computed `spacingTotal` (columnSpacing × columnSpacingSlots). Removes local constants from `clampBranchAndStatusWidths` and internal max path width fallback from `listCommandWithCommandExecutor`. Updates `derivePathWidth` to remove unconditional pathWidth enforcement, deferring width clamping to downstream logic.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Verify the spacingTotal calculation and its impact on maxAvailableForBranch logic
Confirm that removing the pathWidth minimum enforcement doesn't introduce edge cases with narrow terminals
Ensure the removal of the internal max path width fallback doesn't break behavior when terminal width is unavailable

Possibly related PRs

feat: add width controls for list command output #39: Introduces the original list rendering width calculation logic that is being refactored in this PR, directly related through shared functions and constants.

Poem

A rabbit hops through columns neat, 🐰
With spacing slots keeping widths sweet,
No hard-coded bounds to constrain,
Just flowing math, again, again—
List rendering dances light on its feet! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "Refine list column width calculations" accurately summarizes the main objective of the changeset. The title directly reflects the primary changes: introducing new constants (columnSpacingSlots, spacingTotal), removing hard-coded values, and updating the underlying width calculation logic in the list command. The word "refine" appropriately conveys that these are improvements to existing calculations, and the phrase is specific enough to communicate the scope clearly without being overly verbose or generic. A teammate reviewing the commit history would immediately understand that this PR involves improvements to how column widths are computed in the list command.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch refactor/list-width-constants

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9fcab14 and b541ba4.

📒 Files selected for processing (1)

cmd/wtp/list.go (2 hunks)

🔇 Additional comments (1)

cmd/wtp/list.go (1)

396-397: LGTM! Clear spacing calculation.

The computed spacingTotal approach is more maintainable than hard-coded values and remains consistent with the explicit spacing in derivePathWidth (lines 410-411).

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull Request Overview

This PR introduces repository-specific worktree isolation by changing the default base directory from ../worktrees to ../worktrees/${WTP_REPO_BASENAME}, where ${WTP_REPO_BASENAME} expands to the repository's directory name. This prevents conflicts when multiple repositories use the same parent directory.

Key changes:

Added expandBaseDirVariables() function to expand ${WTP_REPO_BASENAME} placeholder in base directory paths
Updated default base_dir constant to include the placeholder
Added legacy worktree layout detection and migration warnings for existing worktrees
Refactored test utilities to improve test reliability across different environments (replaced permission-based test failures with file/symlink-based approaches)

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
internal/config/config.go	Added placeholder expansion logic and updated default base directory constant
internal/config/config_test.go	Added tests for variable expansion and updated existing tests to use new default
cmd/wtp/list.go	Added legacy layout warnings and minor refactoring to extract root command
cmd/wtp/remove.go	Added legacy layout warnings, refactored into smaller functions, and simplified base directory resolution
cmd/wtp/cd.go	Added legacy layout warnings and simplified base directory resolution
cmd/wtp/legacy_warning.go	New file implementing legacy worktree layout detection and migration suggestions
cmd/wtp/legacy_warning_test.go	Tests for legacy layout detection logic
cmd/wtp/worktree_managed.go	Optimized path conversion to avoid redundant absolute path conversions
cmd/wtp/worktree_managed_test.go	Tests for worktree management logic with new default layout
cmd/wtp/init.go	Updated default config template and added test hook for file writing
test/e2e/worktree_test.go	Updated e2e test to verify new worktree path structure
test/e2e/hook_streaming_test.go	Updated test path expectations for new layout
internal/hooks/executor_test.go	Improved test reliability by using files/symlinks instead of permissions
cmd/wtp/remove_test.go	Updated mock paths to reflect new layout and added ErrWriter discard
cmd/wtp/list_test.go	Updated mock paths and added ErrWriter discard to suppress warnings in tests
cmd/wtp/cd_test.go	Updated mock paths to reflect new layout
cmd/wtp/init_test.go	Updated test expectations and added write file mock
docs/architecture.md	Updated documentation to reflect new default path structure
README.md	Updated examples and added breaking change notice

Comments suppressed due to low confidence (1)

cmd/wtp/legacy_warning.go:1

Same issue as in config.go - filepath.Base() is called on repoRoot without cleaning the path first. Since repoRoot is cleaned on line 75, this is safe in detectLegacyWorktreeMigrations(), but the same pattern should be applied consistently or the function should document that it expects a clean path.

package main

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-01T14:29:23Z

internal/config/config.go

+		return baseDir
+	}
+
+	repoBaseName := filepath.Base(repoRoot)


The function doesn't handle trailing slashes in repoRoot properly. filepath.Base(\"/home/user/project/\") returns \"project\" on Unix but the behavior can be inconsistent. While there's a test case for this scenario (line 54-58 in config_test.go), the actual expansion function should clean the path first to ensure consistent behavior. Consider adding repoRoot = filepath.Clean(repoRoot) before calling filepath.Base().

Suggested change

repoBaseName := filepath.Base(repoRoot)

cleanRepoRoot := filepath.Clean(repoRoot)

repoBaseName := filepath.Base(cleanRepoRoot)

Copilot · 2025-11-01T14:29:23Z

cmd/wtp/list.go

 	if statusWidth < statusHeaderWidth {
 		statusWidth = statusHeaderWidth
 	}



[nitpick] The calculation uses columnSpacingSlots (value 3) but it's not immediately clear why there are 3 spacing slots. Consider adding a comment explaining the layout: one spacing between PATH-BRANCH, one between BRANCH-STATUS, and one between STATUS-HEAD.

Suggested change

// There are three spacing slots between the four columns: one between PATH-BRANCH,

// one between BRANCH-STATUS, and one between STATUS-HEAD.

Copilot · 2025-11-01T14:29:23Z

cmd/wtp/init_test.go

+	originalWriteFile := writeFile
+	writeFile = func(string, []byte, os.FileMode) error {
+		return assert.AnError
+	}
+	defer func() { writeFile = originalWriteFile }()


[nitpick] The mock function for writeFile is created but cmd.Action is called outside the test setup context where it's clear that a config file doesn't already exist. The test at line 207 should verify that the error message is meaningful, not just that an error occurred. Consider asserting the specific error type or message to ensure the failure path is correctly handled.

Copilot AI review requested due to automatic review settings November 1, 2025 14:27

Copilot AI reviewed Nov 1, 2025

View reviewed changes

refactor: reuse shared list layout constants

b541ba4

satococoa force-pushed the refactor/list-width-constants branch from 57b0351 to b541ba4 Compare November 1, 2025 14:32

satococoa merged commit c6e3c39 into main Nov 1, 2025
7 checks passed

satococoa deleted the refactor/list-width-constants branch November 1, 2025 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refine list column width calculations #40

Refine list column width calculations #40

Uh oh!

satococoa commented Nov 1, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 1, 2025 •

edited

Loading

Other AI code review bot(s) detected

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 1, 2025

Uh oh!

Copilot AI Nov 1, 2025

Uh oh!

Copilot AI Nov 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	repoBaseName := filepath.Base(repoRoot)
	cleanRepoRoot := filepath.Clean(repoRoot)
	repoBaseName := filepath.Base(cleanRepoRoot)



	// There are three spacing slots between the four columns: one between PATH-BRANCH,
	// one between BRANCH-STATUS, and one between STATUS-HEAD.

Refine list column width calculations #40

Refine list column width calculations #40

Uh oh!

Conversation

satococoa commented Nov 1, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Other AI code review bot(s) detected

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

satococoa commented Nov 1, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 1, 2025 •

edited

Loading