Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTwo new CLI subcommands ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Server
User->>CLI: list-buffers --detached
CLI->>Server: ClientMessage::Buffer(BufferRequest::List { detached_only: true })
Server-->>CLI: ServerResponse::Buffers { records }
CLI->>CLI: format_buffers(records)
CLI-->>User: Tab-delimited buffer output
User->>CLI: attach-buffer 42 -t pane123
CLI->>Server: ClientMessage::Node(NodeRequest::MoveBufferToNode { buffer_id: 42, target_leaf_node_id: pane123 })
Server-->>CLI: Success
CLI-->>User: Empty response (success)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 Tip You can disable the changed files summary in the walkthrough.Disable the |
This was referenced Mar 23, 2026
Pajn
force-pushed
the
buffer-cli
branch
2 times, most recently
from
c708b29 to
48286b0
Compare
Pajn
force-pushed
the
api-docs
branch
2 times, most recently
from
93fb341 to
a92217c
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-cli/src/lib.rs`:
- Around line 97-100: The two boolean flags attached and detached should be made
mutually exclusive at parse time so BufferRequest::List cannot receive
contradictory attached_only and detached_only values; update the clap derive on
the struct that defines attached and detached (the fields currently annotated
with #[arg(long)]) to declare a mutual conflict (e.g., add conflicts_with =
"detached" to the attached field and conflicts_with = "attached" to the detached
field, or define an ArgGroup with multiple = false) so the parser rejects both
true simultaneously before constructing BufferRequest::List.
- Around line 285-294: The AttachBuffer branch currently sends
NodeRequest::MoveBufferToNode and awaits but doesn't validate the response;
modify the code in the Command::AttachBuffer handler (around
connection.resolve_pane and the request via
ClientMessage::Node/NodeRequest::MoveBufferToNode) to capture the awaited
response and call expect_session_snapshot() on it (mirroring the pattern used in
SplitWindow) to ensure the server returned a SessionSnapshot and fail fast on
protocol drift.
In `@crates/embers-cli/tests/panes.rs`:
- Around line 172-177: The test is brittle because it asserts the buffer state
is exactly "running" immediately after create/attach; instead update the
assertion in the panes.rs test (around the use of run_cli, stdout, listed and
detached_buffer_id) to accept either "running" or "created" (the transient
state) — e.g., parse the status field from the tab-separated line for the buffer
id and assert it is one of ["created","running"] so the test is not
timing-flaky.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6eac6ac0-16df-4030-a570-750c21baddc1
📒 Files selected for processing (2)
crates/embers-cli/src/lib.rscrates/embers-cli/tests/panes.rs
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/embers-cli/tests/panes.rs (1)
210-217:⚠️ Potential issue | 🟠 MajorAvoid strict
runningstate checks immediately after attach/list transitions.Line 215 and Line 242 still hard-code
running, which can reintroduce timing flakes (createdis transiently valid).Suggested fix
let listed = run_cli(&server, ["list-buffers", "--attached"]); assert!( stdout(&listed) .lines() - .any(|line| line.starts_with(&format!( - "{detached_buffer_id}\trunning\tattached:{target_pane_id}\t" - ))) + .filter(|line| line.starts_with(&format!("{detached_buffer_id}\t"))) + .any(|line| { + let fields: Vec<&str> = line.split('\t').collect(); + fields + .get(1) + .is_some_and(|status| status == &"created" || status == &"running") + && fields.get(2).is_some_and(|attachment| { + *attachment == format!("attached:{target_pane_id}") + }) + }) ); @@ let detached_again = run_cli(&server, ["list-buffers", "--detached"]); assert!( stdout(&detached_again) .lines() - .any(|line| line.starts_with(&format!("{previous_buffer_id}\trunning\tdetached\t"))) + .filter(|line| line.starts_with(&format!("{previous_buffer_id}\t"))) + .any(|line| { + let fields: Vec<&str> = line.split('\t').collect(); + fields + .get(1) + .is_some_and(|status| status == &"created" || status == &"running") + && fields.get(2) == Some(&"detached") + }) );Also applies to: 238-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-cli/tests/panes.rs` around lines 210 - 217, The test is asserting a strict "running" state immediately after attach/list transitions which can be transiently "created"; update the assertions in the panes.rs test where run_cli(&server, ["list-buffers", "--attached"]) is used (and the similar assertion later) to accept either "created" or "running" rather than only "running": locate the checks that build the expected string with {detached_buffer_id}\trunning\tattached:{target_pane_id} and change the match logic (using e.g. a predicate that checks .any(|line| line.starts_with(&format!("{detached_buffer_id}\t")) && (line.contains("\trunning\tattached:") || line.contains("\tcreated\tattached:")) ) ) so both transient states pass; apply the same change to the duplicate assertion later in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/embers-cli/tests/panes.rs`:
- Around line 210-217: The test is asserting a strict "running" state
immediately after attach/list transitions which can be transiently "created";
update the assertions in the panes.rs test where run_cli(&server,
["list-buffers", "--attached"]) is used (and the similar assertion later) to
accept either "created" or "running" rather than only "running": locate the
checks that build the expected string with
{detached_buffer_id}\trunning\tattached:{target_pane_id} and change the match
logic (using e.g. a predicate that checks .any(|line|
line.starts_with(&format!("{detached_buffer_id}\t")) &&
(line.contains("\trunning\tattached:") || line.contains("\tcreated\tattached:"))
) ) so both transient states pass; apply the same change to the duplicate
assertion later in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 795d1b36-b468-410a-903e-c92c60123678
📒 Files selected for processing (2)
crates/embers-cli/src/lib.rscrates/embers-cli/tests/panes.rs
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.
Stack
Summary by CodeRabbit
New Features
list-bufferscommand to display buffers, with mutually exclusive flags to filter attached or detached buffers and readable tab-separated output.attach-buffercommand to move a buffer to a specified target pane.Tests