Improve status command readability - #394
Merged
Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbitStyle
WalkthroughThe status command's human output rendering is refactored to use a shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/status_tests.rs`:
- Around line 683-703: The test is currently constructing a ChildNotSymlink
issue with populated actual/expected fields that production never sets; update
the fixture to match the real issue shape by calling status_issue for
StatusIssueKind::ChildNotSymlink with None for both expected and actual (i.e.,
change the Some("symlink"), Some("file") args to None, None) and adjust
assertions accordingly to not require " actual:" or " expected:" lines—still
assert the "✗ Drift:" and "exists but is not a symlink" via render_status_entry
and plain_formatter.
In `@src/commands/status.rs`:
- Around line 338-364: The MissingExpectedSource branch for StatusIssueKind only
prints entry.destination for "symlink-contents" and never surfaces the missing
source path stored in issue.path; update that branch to include the missing
source directory (use issue.path or issue.path.as_deref()) in the formatted
message so users see the actual missing source, keeping the same
formatter.format_label usage and consistent with how issue.actual is used
elsewhere (references: StatusIssueKind::MissingExpectedSource,
entry.destination, issue.actual, issue.path, formatter.format_label, and
issue_detail_lines).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb1958c6-d00f-4409-b55c-0981ea11d504
📒 Files selected for processing (2)
src/commands/status.rssrc/commands/status_tests.rs
Comment on lines
+683
to
+703
| fn test_render_child_not_symlink_shows_actual_detail() { | ||
| let entry = status_entry( | ||
| "/tmp/.claude/commands", | ||
| "symlink-contents", | ||
| crate::commands::status::DestinationKind::Directory, | ||
| None, | ||
| Some("/tmp/.agents/commands"), | ||
| vec![status_issue( | ||
| crate::commands::status::StatusIssueKind::ChildNotSymlink, | ||
| "/tmp/.claude/commands/review.md", | ||
| Some("symlink"), | ||
| Some("file"), | ||
| )], | ||
| ); | ||
|
|
||
| let rendered = render_status_entry(&entry, &plain_formatter()); | ||
|
|
||
| assert!(rendered[0].contains("✗ Drift:")); | ||
| assert!(rendered[0].contains("exists but is not a symlink")); | ||
| assert!(rendered.iter().any(|line| line == " actual: file")); | ||
| assert!(rendered.iter().any(|line| line == " expected: symlink")); |
Contributor
There was a problem hiding this comment.
Make this ChildNotSymlink test use the real issue shape.
collect_status_entries() does not currently populate actual: Some("file") or expected: Some("symlink") for ChildNotSymlink, so this fixture is asserting a branch that production never reaches. As written, the test can stay green while shipped output still omits the actual: detail for this case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/status_tests.rs` around lines 683 - 703, The test is currently
constructing a ChildNotSymlink issue with populated actual/expected fields that
production never sets; update the fixture to match the real issue shape by
calling status_issue for StatusIssueKind::ChildNotSymlink with None for both
expected and actual (i.e., change the Some("symlink"), Some("file") args to
None, None) and adjust assertions accordingly to not require " actual:" or "
expected:" lines—still assert the "✗ Drift:" and "exists but is not a symlink"
via render_status_entry and plain_formatter.
Comment on lines
338
to
+364
| StatusIssueKind::MissingExpectedSource => { | ||
| if entry.sync_type.as_str() == "symlink-contents" { | ||
| format!( | ||
| "{} Missing source container directory: {}", | ||
| "!".yellow(), | ||
| vec![format!( | ||
| "{}: {}", | ||
| formatter.format_label( | ||
| "!", | ||
| "Missing source container directory", | ||
| LabelKind::Warning | ||
| ), | ||
| entry.destination | ||
| ) | ||
| )] | ||
| } else { | ||
| format!( | ||
| "{} Link points to missing source: {}", | ||
| "!".yellow(), | ||
| vec![format!( | ||
| "{}: {}", | ||
| formatter.format_label( | ||
| "!", | ||
| "Link points to missing source", | ||
| LabelKind::Warning | ||
| ), | ||
| issue.actual.as_deref().unwrap_or(&entry.destination) | ||
| ) | ||
| )] | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| lines.extend(issue_detail_lines(issue, formatter)); | ||
| lines |
Contributor
There was a problem hiding this comment.
Expose the missing source path for symlink-contents source-missing errors.
This branch only prints entry.destination, and the follow-up detail lines only surface actual, so the missing source directory stored in issue.path never appears in human output. That makes this case much less actionable than the other structured issue renderers.
Suggested fix
StatusIssueKind::MissingExpectedSource => {
if entry.sync_type.as_str() == "symlink-contents" {
- vec![format!(
- "{}: {}",
- formatter.format_label(
- "!",
- "Missing source container directory",
- LabelKind::Warning
- ),
- entry.destination
- )]
+ vec![
+ format!(
+ "{}: {}",
+ formatter.format_label(
+ "!",
+ "Missing source container directory",
+ LabelKind::Warning
+ ),
+ entry.destination
+ ),
+ format!(
+ " {}",
+ formatter.format_key_value("source", &issue.path)
+ ),
+ ]
} else {
vec
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #386
Verification
cargo test commands::status_testscargo test commands::statuscargo test output::testscargo testcargo fmt --checkcargo run -- status --help