Conversation
This was referenced Mar 22, 2026
📝 WalkthroughWalkthroughAdds CLI buffer/node commands, protocol types for buffer location/history and node operations, server state and APIs for zoom/helper buffers and node reparenting, client event/presentation changes for zoom and history, extensive test and doc updates, plus test harness/PTY tooling and config-watching integration. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as embers CLI
participant Client as ConfiguredClient
participant Protocol as ProtocolClient
participant Server as MuxServer
participant State as ServerState
CLI->>Protocol: ClientMessage::Buffer(OpenHistory / Reveal / Inspect)
Protocol->>Server: dispatch validated Buffer/Node request
Server->>State: resolve buffer/node, create helper buffer or mutate layout
State-->>Server: BufferLocation / BufferWithLocation / layout events
Server->>Protocol: ServerResponse::BufferWithLocation / BufferLocation / Ok + events
Protocol->>Client: deliver response + events
Client->>Client: apply buffer record, update snapshot
Client->>CLI: formatted output (show/history/location)
sequenceDiagram
participant CLI as embers CLI
participant Protocol as ProtocolClient
participant Server as MuxServer
participant State as ServerState
CLI->>Protocol: ClientMessage::Node(Zoom / Swap / Break / Join / Move)
Protocol->>Server: validated NodeRequest
Server->>State: perform node mutation (zoom/unzoom/swap/break/join/move)
State-->>Server: NodeChanged (+ optional FloatingChanged / FocusChanged)
Server->>Protocol: ServerResponse::Ok and event list
Protocol->>Client: events
Client->>Client: update presentation (project zoomed subtree / re-render)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
|
This was referenced Mar 22, 2026
Pajn
force-pushed
the
pty-alive
branch
2 times, most recently
from
80fbf4e to
991111a
Compare
Pajn
force-pushed
the
pty-alive
branch
4 times, most recently
from
85a63c3 to
8762d2d
Compare
Pajn
force-pushed
the
pty-alive
branch
2 times, most recently
from
2d2abf7 to
ad3545d
Compare
Add the terminal runtime contract note and phase-one lifecycle coverage. Fix runtime cleanup so poller threads are joined and keep exited runtime handles available for post-exit capture while rejecting live operations after exit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Document the PTY-routing-to-backend pipeline, remove the unused duplicate server-side surface, and add backend tests that lock down the raw routing seam and alternate-screen visibility semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Expand the backend parser suite with deterministic byte-stream fixtures for carriage return overwrite, auto-wrap, erase-in-line, clear-screen, and mode disable handling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add live PTY coverage for scripted input, config reload, buffer continuity, hidden bell visibility, and fullscreen transitions. Stabilize the interactive client by watching config files off the main loop and by separating event waiting from event handling so request/response resync work cannot be cancelled mid-flight. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (16)
crates/embers-client/src/presentation.rs (1)
95-132:⚠️ Potential issue | 🟠 MajorZoomed floating roots still lose floating context.
At Line 95, the zoom branch projects with
floating_id: Noneand bypasses floating frame projection. Whenzoomed_node_idpoints to a floating root,focused_floating_id()can fall back toNone, breaking floating-targeted actions.🔧 Suggested fix
- if let Some(zoomed_node_id) = session.zoomed_node_id - && state.nodes.contains_key(&zoomed_node_id) - { - projector.project_node(zoomed_node_id, root_bounds, None, false, Vec::new())?; + if let Some(zoomed_node_id) = session.zoomed_node_id + && state.nodes.contains_key(&zoomed_node_id) + { + if let Some(window) = state + .floating + .values() + .find(|window| window.root_node_id == zoomed_node_id) + { + let rect = clip_rect(geometry_rect(window.geometry), root_bounds); + if rect.size.width > 0 && rect.size.height > 0 { + let content_rect = inset_border(rect); + projector.projection.floating.push(FloatingFrame { + floating_id: window.id, + rect, + content_rect, + title: window.title.clone(), + focused: window.focused, + }); + projector.project_node( + window.root_node_id, + content_rect, + Some(window.id), + false, + Vec::new(), + )?; + } + } else { + projector.project_node(zoomed_node_id, root_bounds, None, false, Vec::new())?; + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/presentation.rs` around lines 95 - 132, The zoom branch currently projects the zoomed node with floating_id None, which loses floating context when session.zoomed_node_id refers to a floating window root; update the zoom-handling path in presentation.rs to detect if session.zoomed_node_id belongs to a floating window (check state.floating by node id), and if so construct and push the corresponding FloatingFrame (rect, content_rect, title, focused, floating_id = window.id) and call projector.project_node with Some(window.id) and the content_rect instead of None; otherwise keep the existing non-floating behavior (project with root_bounds and floating_id None).crates/embers-server/tests/model_state.rs (2)
361-367:⚠️ Potential issue | 🟠 MajorAssert the sibling order after each mutation.
Both calls can succeed as no-ops and this test still passes, because
state.validate()only checks invariants. Capture the split children afterswap_sibling_nodesand again aftermove_node_beforeso the test proves the order changed.Suggested assertion shape
state .swap_sibling_nodes(first_child, second_child) .expect("swap siblings"); + let after_swap = match state + .node(root_tab_child(&state, session_id, 0)) + .expect("root split") + { + Node::Split(split) => split.children.clone(), + other => panic!("expected split, got {other:?}"), + }; + assert_eq!(after_swap, vec![second_child, first_child]); + state .move_node_before(first_child, second_child) .expect("move sibling before"); + let after_move = match state + .node(root_tab_child(&state, session_id, 0)) + .expect("root split") + { + Node::Split(split) => split.children.clone(), + other => panic!("expected split, got {other:?}"), + }; + assert_eq!(after_move, vec![first_child, second_child]); state.validate().expect("state remains valid");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/tests/model_state.rs` around lines 361 - 367, After calling state.swap_sibling_nodes(first_child, second_child) and state.move_node_before(first_child, second_child), capture the parent's child list (e.g., call whatever test helper or API returns the split children) immediately after each mutation and assert the relative order of first_child and second_child changed as expected; specifically, after swap_sibling_nodes assert their positions are reversed compared to the original, and after move_node_before assert the final ordering matches the intended move, then still call state.validate() as before. Use the existing identifiers state, first_child, and second_child to locate where to insert these assertions.
404-409:⚠️ Potential issue | 🟠 MajorThe final
Node::Tabsassertion is vacuous.
session_root()is already aNode::Tabsin this fixture, so this passes even ifjoin_buffer_at_node(..., TabAfter)never wraps the target node. Assert on the child under the root tab (or the inserted tabs node) instead.Suggested assertion shape
- let root = session_root(&state, session_id); - assert!(matches!(state.node(root).expect("root"), Node::Tabs(_))); + let joined = root_tab_child(&state, session_id, 0); + match state.node(joined).expect("joined node") { + Node::Tabs(tabs) => assert_eq!(tabs.tabs.len(), 2), + other => panic!("expected tabs, got {other:?}"), + } state.validate().expect("state remains valid");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/tests/model_state.rs` around lines 404 - 409, The test's final assert is vacuous because session_root(&state, session_id) is already Node::Tabs; instead verify that join_buffer_at_node actually inserted a Tabs wrapper or added a new child: after calling state.join_buffer_at_node(root, detached_again, NodeJoinPlacement::TabAfter) fetch the (possibly updated) root via session_root(&state, session_id) or the returned node id, then inspect its children by using state.node(root) and assert that the expected child node (e.g., the newly created tabs node or that one of root's children matches detached_again wrapped as a Tab) is present — replace the bare assert!(matches!(state.node(root).expect("root"), Node::Tabs(_))) with an assertion that checks the specific child node id or that a Tabs node contains the buffer id detached_again.crates/embers-protocol/tests/family_round_trip.rs (1)
542-548:⚠️ Potential issue | 🟡 MinorAdd a helper-buffer round-trip fixture here.
sample_buffer_record()still only emitsBufferRecordKind::Ptywithhelper_source_buffer_id == Noneandhelper_scope == None, so the new helper-buffer path never round-trips in this suite. Add one helper record so regressions in that wire shape fail here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/tests/family_round_trip.rs` around lines 542 - 548, Update the test fixtures so a helper-buffer wire shape is exercised: modify sample_buffer_record() (in the family_round_trip.rs test file) to also return or include a BufferRecord whose kind is the helper variant (e.g., BufferRecordKind::Helper or the appropriate helper enum value) with helper_source_buffer_id set to Some(<any test id>) and helper_scope set to Some(<appropriate scope value>), instead of always emitting only BufferRecordKind::Pty with helper_source_buffer_id == None and helper_scope == None; ensure the new helper record is included in the round-trip vector used by the tests so the helper-buffer path is validated.crates/embers-server/src/persist.rs (1)
24-25:⚠️ Potential issue | 🟠 MajorBump the workspace format version for these new persisted fields.
zoomed_nodeandPersistedBufferKindchange the on-disk schema, but the workspace is still written as format1and only version0is migrated. That leaves version1ambiguous and prevents readers from distinguishing pre- and post-helper-buffer workspaces.Minimal fix direction
-pub const CURRENT_FORMAT_VERSION: u32 = 1; +pub const CURRENT_FORMAT_VERSION: u32 = 2;- LEGACY_FORMAT_VERSION => { + LEGACY_FORMAT_VERSION | 1 => { workspace.format_version = Some(CURRENT_FORMAT_VERSION); Ok(workspace) }Also applies to: 283-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/persist.rs` around lines 24 - 25, The workspace format constant CURRENT_FORMAT_VERSION must be bumped to reflect added persisted fields (zoomed_node and PersistedBufferKind) so readers can distinguish pre- and post-helper-buffer workspaces; update the value from 1 to 2 and add/adjust migration logic (the code paths that currently only migrate version 0) to handle migrating on-disk data from version 1 to 2, ensuring any deserialization/upgrade code for PersistedBufferKind and zoomed_node is applied during migration so old workspaces are upgraded to the new schema.docs/config-api/action.md (1)
8-10:⚠️ Potential issue | 🟠 MajorSpecify valid string literals for new action parameters.
Line 9, Line 635, and Line 914 still leave accepted values undocumented for
destination,scope, andplacement, which makes configs error-prone.📝 Suggested doc patch
fn break_current_node(_: ActionApi, destination: String) -> Action @@ Break the current node into a new tab or floating window. +- `destination`: `"tab"` | `"floating"` fn join_buffer_here(_: ActionApi, buffer_id: int, placement: String) -> Action @@ Join a buffer at the current node. +- `placement`: `"left"` | `"right"` | `"up"` | `"down"` | `"tab-before"` | `"tab-after"` fn open_buffer_history(_: ActionApi, buffer_id: int, scope: String, placement: String) -> Action @@ Open the history of a buffer in a new view. +- `scope`: `"full"` | `"visible"` +- `placement`: `"left"` | `"right"` | `"up"` | `"down"` | `"tab-before"` | `"tab-after"`Also applies to: 21-22, 635-636, 647-648, 914-915, 926-927
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/config-api/action.md` around lines 8 - 10, The docs leave accepted string literals undocumented for new action parameters (e.g., break_current_node(destination: String), and other actions using parameters named destination, scope, placement); update the action.md entries for these functions to enumerate the valid literal values for each parameter, include the exact allowed options (e.g., "node", "edge", "global" or whatever your runtime accepts), and add a short example usage for each parameter so readers know which strings to pass; make sure to update every occurrence referencing destination, scope, and placement (including the break_current_node signature and the other two action signatures) so the documentation consistently lists allowed literals and an example for each.docs/config-api/registration-action.md (1)
8-10:⚠️ Potential issue | 🟡 MinorDocument accepted literals for
destination,scope, andplacement.Line 9, Line 635, and Line 914 expose string parameters without listing valid values, so users still have to guess supported inputs.
📝 Suggested doc patch
fn break_current_node(_: ActionApi, destination: String) -> Action @@ Break the current node into a new tab or floating window. +- `destination` accepted values: + - `"tab"`: move/break into a new tab + - `"floating"`: move/break into a floating window fn join_buffer_here(_: ActionApi, buffer_id: int, placement: String) -> Action @@ Join a buffer at the current node. +- `placement` accepted values: + - `"left" | "right" | "up" | "down" | "tab-before" | "tab-after"` fn open_buffer_history(_: ActionApi, buffer_id: int, scope: String, placement: String) -> Action @@ Open the history of a buffer in a new view. +- `scope` accepted values: + - `"full"`: full capture history + - `"visible"`: currently visible history/surface +- `placement` accepted values: + - `"left" | "right" | "up" | "down" | "tab-before" | "tab-after"`Also applies to: 21-22, 635-636, 647-648, 914-915, 926-927
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/config-api/registration-action.md` around lines 8 - 10, Update the API docs to enumerate the allowed literal values for the string parameters used across the registration-action docs—specifically for destination in break_current_node (fn break_current_node(_: ActionApi, destination: String) -> Action) and for the scope and placement parameters used elsewhere—by replacing the vague "String" type with a short list or table of accepted values (e.g., exact token names or enum variants), include brief descriptions for each literal and any precedence/compatibility rules, and ensure the same explicit lists are added for the other occurrences of these parameters so consumers don’t have to guess valid inputs.crates/embers-cli/tests/interactive.rs (1)
49-77: 🧹 Nitpick | 🔵 TrivialPID verification before kill would improve safety.
The
kill_orphaned_serverfunction reads a PID from the socket's pid file and sends signals without verifying the process is actually the__servebinary. In edge cases (rapid PID recycling, stale pid file), this could signal an unrelated process.Consider verifying the process cmdline contains
__serveor the expected binary name before signaling, especially beforeSIGKILL.♻️ Suggested verification approach
fn kill_orphaned_server(socket_path: &Path) { let pid_path = socket_path.with_extension("pid"); // Try to read and kill the PID from the pid file if let Ok(pid_str) = fs::read_to_string(&pid_path) && let Ok(pid) = pid_str.trim().parse::<i32>() && pid > 0 { + // Verify the process is actually our server before killing + let cmdline_path = format!("/proc/{pid}/cmdline"); + if let Ok(cmdline) = fs::read_to_string(&cmdline_path) { + if !cmdline.contains("__serve") && !cmdline.contains("embers") { + // PID was recycled, just clean up the stale pid file + let _ = fs::remove_file(&pid_path); + return; + } + } // SAFETY: pid comes from our own pid file unsafe { libc::kill(pid, libc::SIGTERM) }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-cli/tests/interactive.rs` around lines 49 - 77, In kill_orphaned_server, before sending SIGTERM or SIGKILL to the PID parsed from pid_path, verify that the PID actually corresponds to our __serve binary by reading the process command line (e.g., /proc/<pid>/cmdline on Linux or using platform-appropriate APIs) and checking it contains "__serve" (or the expected executable name); only proceed to call libc::kill for that pid if the check passes, and otherwise skip signaling and still remove or log the stale pid file. Ensure both the graceful (SIGTERM) and force (SIGKILL) attempts perform this verification and handle I/O/parse errors cleanly so we don't accidentally kill unrelated processes.docs/config-api/defs/registration.rhai (1)
85-86:⚠️ Potential issue | 🟡 MinorDocument the accepted string literals for these parameters.
The
destination,scope, andplacementparameters accept closed sets of string values, but the doc comments only show the genericstringtype. Users must guess or encounter runtime errors to discover valid inputs.Based on the runtime implementation:
destination:"tab"|"floating"scope:"visible"|"full"placement(foropen_buffer_history):"floating"|"tab"placement(forjoin_buffer_here):"tab-after"|"tab-before"|"left"|"right"|"up"|"down"Also applies to: 175-176, 222-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/config-api/defs/registration.rhai` around lines 85 - 86, Update the doc signatures to document the accepted closed string literal sets instead of using generic string types: change the parameter types for break_current_node (destination) to "tab" | "floating", for the functions that accept scope to "visible" | "full", for open_buffer_history's placement to "floating" | "tab", and for join_buffer_here's placement to "tab-after" | "tab-before" | "left" | "right" | "up" | "down"; locate these by the function names break_current_node, open_buffer_history, join_buffer_here and any other defs with destination/scope/placement parameters and replace their parameter types in the doc signatures accordingly so the accepted values are explicit.docs/config-api/defs/runtime.rhai (1)
134-135:⚠️ Potential issue | 🟡 MinorDocument the accepted string literals for these parameters.
Same issue as
registration.rhai— thedestination,scope, andplacementparameters should enumerate their valid string values in the doc comments so users can reference the API docs directly.Also applies to: 224-225, 271-272
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/config-api/defs/runtime.rhai` around lines 134 - 135, Update the doc comment for fn break_current_node(_: ActionApi, destination: string) -> Action to list the accepted destination string literals (e.g., "tab", "window", "floating" or whatever the API supports), and similarly update the doc comments for every other runtime.rhai function that takes a destination, scope, or placement string parameter to enumerate the valid string values the API accepts; locate these by searching for functions with parameters named destination, scope, or placement and add a concise list of allowed literal values to each doc comment so consumers can reference the exact options in the API docs.crates/embers-protocol/src/codec.rs (1)
617-648:⚠️ Potential issue | 🟠 MajorReject
client_id == 0for reveal/history.
0is the wire sentinel forNonehere, soSome(0)is silently rewritten to the implicit client and decodes back asNone. That makes these requests non-round-trippable and can reveal/open history in the wrong client instead of failing fast. Make this field non-zero end-to-end, or rejectSome(0)during encoding.Also applies to: 2530-2540
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/src/codec.rs` around lines 617 - 648, The encoder currently maps Some(0) client IDs to the wire sentinel 0 which is interpreted as None on decode; update the BufferRequest encoding for BufferRequest::Reveal and BufferRequest::OpenHistory (and the same logic used at the other similar site) to reject client_id == 0 instead of silently writing 0: detect when client_id.is_some() && client_id.unwrap() == 0 and return an encoding error (or map to an explicit non-zero value if your protocol requires non-zero ids end-to-end), ensuring the encoder fails fast rather than emitting the sentinel 0 so round-trip decoding preserves Some(client_id).crates/embers-cli/src/lib.rs (2)
1649-1660:⚠️ Potential issue | 🟡 MinorKeep
buffer showvalues to one tab delimiter.
format_buffer_detailsemitskey\tvalue, butformat_buffer_location_valueinjects extra tabs for attached buffers. That turns thelocationrow into multiple columns and breaks split-once parsers. Use a tab-free inline formatter here and keep the tabular formatter forbuffer reveal/buffer history.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-cli/src/lib.rs` around lines 1649 - 1660, format_buffer_details currently calls format_buffer_location_value which emits extra tabs for attached buffers and breaks single-tab split-once parsing; replace that call with a tab-free inline formatter (e.g., implement or call a new function like format_buffer_location_inline or inline the logic) so the "location" row is emitted as "location\t<no-tabs-value>" only; keep the existing tab-containing formatter (format_buffer_location_value) for buffer reveal / buffer history outputs but ensure format_buffer_details uses the new tab-free version so parsers receive exactly one tab delimiter per row.
479-501:⚠️ Potential issue | 🟡 Minor
buffer showstill mixes two different snapshots.It awaits
BufferRequest::GetandBufferRequest::GetLocationseparately. A concurrent move, detach, or kill between them can combine stale metadata with a newer location, or make the second lookup fail after the first succeeded. If this output is meant to be reliable for scripts, it needs a single snapshot/atomic response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-cli/src/lib.rs` around lines 479 - 501, The current BufferCommand::Show implementation issues two separate requests (ClientMessage::Buffer with BufferRequest::Get and BufferRequest::GetLocation) via connection.request, which can race and mix snapshots; replace this with a single atomic request/response pair so both buffer metadata and location come from the same snapshot. Add/use a single BufferRequest variant (e.g., GetWithLocation or Inspect) that returns a combined ServerResponse (e.g., ServerResponse::BufferWithLocation), update the client code here to call connection.request once with that combined request, match the single response (instead of separate Get/GetLocation handling and expect_buffer_location) and then pass the returned buffer+location to format_buffer_details. Ensure the server side populates the combined response atomically so scripts see a consistent snapshot.crates/embers-server/src/state.rs (2)
1371-1445:⚠️ Potential issue | 🟠 MajorResolve the join target before tearing down the source view.
Closing
source_viewfirst can normalize awaynode_idwhen the buffer already lives at, or somewhere under, the target subtree, and thetabs_id == node_idbranch also producesinsert_index = 1forTabAfteron an empty tabs container. Both cases discover the invalid target too late; if the buffer was attached, the error path has already torn down its original view and leaves it detached. Compute/clamp the final target first and short-circuit the already-attached case before mutating the tree.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/state.rs` around lines 1371 - 1445, Compute and validate the final join target before mutating the tree: determine target_session via self.node_session_id(node_id) and inspect self.buffer(buffer_id)?.attachment to see if the buffer is already attached to source_view that is the same as node_id or within the target subtree, and if so short‑circuit and return early; only after confirming the join target is valid should you call self.close_node(source_view), then call self.create_buffer_view to make new_view and proceed with wrap_node_in_split, wrap_node_in_tabs, and add_tab_sibling_at; ensure on error you still call self.discard_buffer_view(new_view) as before.
1342-1350: 🧹 Nitpick | 🔵 TrivialAvoid a fixed 100x26 geometry for floated breakouts.
This still bakes one terminal size into the server state. On smaller viewports the breakout window can open clipped or offscreen; please derive a default from the current terminal/session size or make it configurable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/state.rs` around lines 1342 - 1350, The code currently uses a hardcoded FloatGeometry::new(10, 3, 100, 26) in the to_floating branch which bakes a 100x26 terminal size; change create_floating_window_with_options call to compute a default geometry from the current session/terminal size (or read a configurable default) instead of fixed values—retrieve the session/terminal dimensions available in this module (use the session_id to get the session/terminal size API you have in state), clamp the floating width/height to reasonable min/max (or a percentage of the terminal), and pass those computed x,y,width,height into FloatGeometry::new(...) (or expose a config option that overrides the computed defaults) so create_floating_window_with_options(session_id, node_id, FloatGeometry::new(...), Some(title), true, true) uses adaptive sizes rather than 100x26.crates/embers-client/src/configured_client.rs (1)
933-942:⚠️ Potential issue | 🟠 MajorRefresh the queued action context after
RevealBuffer.This request can retarget the attached client to another session, but
execute_actions()keeps using the originalsession_idand presentation for the rest of the queue. Any chained action after Line 941 still runs against the pre-reveal session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/configured_client.rs` around lines 933 - 942, In the Action::RevealBuffer branch, after awaiting self.client.resync_all_sessions(), refresh the queued-action context (the local session_id and presentation values used by execute_actions()) so subsequent actions use the potentially new session; specifically, when handling RevealBuffer in execute_actions(), recompute or fetch the current session/presentation from self.client (replace the stale session_id/presentation variables) immediately after self.client.resync_all_sessions() returns so the rest of the queue runs against the updated session.
🤖 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/interactive.rs`:
- Around line 124-127: The ConfigChanged branch currently calls
configured.reload_config_if_changed()? which makes reload errors fatal; instead,
call configured.reload_config_if_changed() and match on the Result so failures
are handled non‑fatally: on Ok, continue to call
terminal.write_bytes(&drain_terminal_output(&mut configured))? and set dirty =
true; on Err(e), render/log a non‑fatal error (e.g. via terminal.write_bytes
with an error message or a logger) and do not replace the active config or set
dirty, preserving the last known good bindings. Keep references to
TerminalEvent::ConfigChanged, configured.reload_config_if_changed(),
drain_terminal_output(&mut configured), terminal.write_bytes(...), and the dirty
flag to locate and update the logic.
In `@crates/embers-cli/tests/panes.rs`:
- Around line 376-378: The test
node_commands_cover_zoom_swap_break_join_and_reorder is missing the integration
test serialization guard; add an awaitable test lock acquisition at the start of
the async test (e.g., call acquire_test_lock().await and bind to a _lock
variable to hold the guard) before starting the TestServer so this test is
serialized with the other integration tests (same pattern used by
buffer_show_and_history_open_helper_buffers).
In `@crates/embers-client/src/configured_client.rs`:
- Around line 348-355: Compute the event's session id before mutating state by
calling self.event_session_id(event) prior to invoking
self.client.handle_event(event).await; store that pre-event session (used for
BufferDetached which has only a buffer_id) and pass it into the EventInfo you
dispatch when the event itself does not carry a session/presentation context.
Update handle_event to use the pre-computed session for the detach-case (and
similarly in the other dispatch path around the block at lines 367-372) so hooks
receive the original session/presentation context instead of None.
In `@crates/embers-client/tests/e2e.rs`:
- Around line 659-677: The test currently sends a bell byte `\a` followed by a
newline which allows the activity tracker to see a later `Activity` event;
update the sends in the block that uses connection.request with
ClientMessage::Input (and the analogous block around lines 890-909) so the
terminal byte of the write is the bell (either remove the trailing `\n`/`\r`
after `\a` or perform the bell as a separate request), then keep the assertions
that call wait_for_capture_contains(buffer_a.id, "detached-bell", ...) and
wait_for_buffer_activity(..., ActivityState::Bell, ...) to ensure the last event
is deterministically `Bell`.
- Around line 814-849: The fixed 10-iteration loop that polls for hidden tab
activity (the for _ in 0..10 { ... } loop updating saw_hidden_activity via
first_client.resync_all_sessions(), refresh_all_snapshots(),
PresentationModel::project, and checking tabs.node_id == fixture.nested_tabs_id
and tab.activity == ActivityState::Activity) should be replaced with a
deadline-based polling loop: compute a deadline (Instant::now() + desired
Duration), then loop while Instant::now() < deadline performing the same
resync_all_sessions() and refresh_all_snapshots() calls, build the
PresentationModel and check the tabs condition each iteration, breaking and
setting saw_hidden_activity when observed; sleep briefly between iterations
(tokio::time::sleep). This mirrors the other deadline-based polls in the file
and avoids flakiness from a fixed 10-pass budget.
In `@crates/embers-protocol/schema/embers.fbs`:
- Around line 203-210: The schema currently relies on implicit FlatBuffers field
IDs which can break wire compatibility if fields are later reordered; update the
affected tables (e.g., BufferRequest and BufferRecord) to assign explicit id:
values to each field such as client_id, attached_only, detached_only, force,
start_line, line_count, history_scope, history_placement and any newly added
fields like kind so that each field has a stable numeric id; pick sequential,
unique ids per table and add the id: N annotations to the field declarations to
ensure future-safe insertions and reorderings.
In `@crates/embers-test-support/src/pty.rs`:
- Around line 111-115: The error construction uses last_error.unwrap() which can
panic; replace the unwrap with a safe fallback by formatting the
Option<io::Error>/Result value via last_error.map(|e|
e.to_string()).unwrap_or_else(|| "unknown error".to_string()) (or similar) so
the MuxError::pty message becomes: "failed to openpty after {} attempts: {}",
Self::MAX_RETRIES + 1, <safe string from last_error>; update the expression
around MuxError::pty in function/opening code that sets last_error so it never
calls unwrap() directly.
In `@docs/activity-bell-policy.md`:
- Line 1: The document currently starts with a level-2 heading "## Activity and
bell policy"; change it to a top-level heading by replacing "## Activity and
bell policy" with "# Activity and bell policy" if the file is intended to be
standalone (leave as-is only if it's purposely a subsection for embedding).
---
Duplicate comments:
In `@crates/embers-cli/src/lib.rs`:
- Around line 1649-1660: format_buffer_details currently calls
format_buffer_location_value which emits extra tabs for attached buffers and
breaks single-tab split-once parsing; replace that call with a tab-free inline
formatter (e.g., implement or call a new function like
format_buffer_location_inline or inline the logic) so the "location" row is
emitted as "location\t<no-tabs-value>" only; keep the existing tab-containing
formatter (format_buffer_location_value) for buffer reveal / buffer history
outputs but ensure format_buffer_details uses the new tab-free version so
parsers receive exactly one tab delimiter per row.
- Around line 479-501: The current BufferCommand::Show implementation issues two
separate requests (ClientMessage::Buffer with BufferRequest::Get and
BufferRequest::GetLocation) via connection.request, which can race and mix
snapshots; replace this with a single atomic request/response pair so both
buffer metadata and location come from the same snapshot. Add/use a single
BufferRequest variant (e.g., GetWithLocation or Inspect) that returns a combined
ServerResponse (e.g., ServerResponse::BufferWithLocation), update the client
code here to call connection.request once with that combined request, match the
single response (instead of separate Get/GetLocation handling and
expect_buffer_location) and then pass the returned buffer+location to
format_buffer_details. Ensure the server side populates the combined response
atomically so scripts see a consistent snapshot.
In `@crates/embers-cli/tests/interactive.rs`:
- Around line 49-77: In kill_orphaned_server, before sending SIGTERM or SIGKILL
to the PID parsed from pid_path, verify that the PID actually corresponds to our
__serve binary by reading the process command line (e.g., /proc/<pid>/cmdline on
Linux or using platform-appropriate APIs) and checking it contains "__serve" (or
the expected executable name); only proceed to call libc::kill for that pid if
the check passes, and otherwise skip signaling and still remove or log the stale
pid file. Ensure both the graceful (SIGTERM) and force (SIGKILL) attempts
perform this verification and handle I/O/parse errors cleanly so we don't
accidentally kill unrelated processes.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 933-942: In the Action::RevealBuffer branch, after awaiting
self.client.resync_all_sessions(), refresh the queued-action context (the local
session_id and presentation values used by execute_actions()) so subsequent
actions use the potentially new session; specifically, when handling
RevealBuffer in execute_actions(), recompute or fetch the current
session/presentation from self.client (replace the stale session_id/presentation
variables) immediately after self.client.resync_all_sessions() returns so the
rest of the queue runs against the updated session.
In `@crates/embers-client/src/presentation.rs`:
- Around line 95-132: The zoom branch currently projects the zoomed node with
floating_id None, which loses floating context when session.zoomed_node_id
refers to a floating window root; update the zoom-handling path in
presentation.rs to detect if session.zoomed_node_id belongs to a floating window
(check state.floating by node id), and if so construct and push the
corresponding FloatingFrame (rect, content_rect, title, focused, floating_id =
window.id) and call projector.project_node with Some(window.id) and the
content_rect instead of None; otherwise keep the existing non-floating behavior
(project with root_bounds and floating_id None).
In `@crates/embers-protocol/src/codec.rs`:
- Around line 617-648: The encoder currently maps Some(0) client IDs to the wire
sentinel 0 which is interpreted as None on decode; update the BufferRequest
encoding for BufferRequest::Reveal and BufferRequest::OpenHistory (and the same
logic used at the other similar site) to reject client_id == 0 instead of
silently writing 0: detect when client_id.is_some() && client_id.unwrap() == 0
and return an encoding error (or map to an explicit non-zero value if your
protocol requires non-zero ids end-to-end), ensuring the encoder fails fast
rather than emitting the sentinel 0 so round-trip decoding preserves
Some(client_id).
In `@crates/embers-protocol/tests/family_round_trip.rs`:
- Around line 542-548: Update the test fixtures so a helper-buffer wire shape is
exercised: modify sample_buffer_record() (in the family_round_trip.rs test file)
to also return or include a BufferRecord whose kind is the helper variant (e.g.,
BufferRecordKind::Helper or the appropriate helper enum value) with
helper_source_buffer_id set to Some(<any test id>) and helper_scope set to
Some(<appropriate scope value>), instead of always emitting only
BufferRecordKind::Pty with helper_source_buffer_id == None and helper_scope ==
None; ensure the new helper record is included in the round-trip vector used by
the tests so the helper-buffer path is validated.
In `@crates/embers-server/src/persist.rs`:
- Around line 24-25: The workspace format constant CURRENT_FORMAT_VERSION must
be bumped to reflect added persisted fields (zoomed_node and
PersistedBufferKind) so readers can distinguish pre- and post-helper-buffer
workspaces; update the value from 1 to 2 and add/adjust migration logic (the
code paths that currently only migrate version 0) to handle migrating on-disk
data from version 1 to 2, ensuring any deserialization/upgrade code for
PersistedBufferKind and zoomed_node is applied during migration so old
workspaces are upgraded to the new schema.
In `@crates/embers-server/src/state.rs`:
- Around line 1371-1445: Compute and validate the final join target before
mutating the tree: determine target_session via self.node_session_id(node_id)
and inspect self.buffer(buffer_id)?.attachment to see if the buffer is already
attached to source_view that is the same as node_id or within the target
subtree, and if so short‑circuit and return early; only after confirming the
join target is valid should you call self.close_node(source_view), then call
self.create_buffer_view to make new_view and proceed with wrap_node_in_split,
wrap_node_in_tabs, and add_tab_sibling_at; ensure on error you still call
self.discard_buffer_view(new_view) as before.
- Around line 1342-1350: The code currently uses a hardcoded
FloatGeometry::new(10, 3, 100, 26) in the to_floating branch which bakes a
100x26 terminal size; change create_floating_window_with_options call to compute
a default geometry from the current session/terminal size (or read a
configurable default) instead of fixed values—retrieve the session/terminal
dimensions available in this module (use the session_id to get the
session/terminal size API you have in state), clamp the floating width/height to
reasonable min/max (or a percentage of the terminal), and pass those computed
x,y,width,height into FloatGeometry::new(...) (or expose a config option that
overrides the computed defaults) so
create_floating_window_with_options(session_id, node_id,
FloatGeometry::new(...), Some(title), true, true) uses adaptive sizes rather
than 100x26.
In `@crates/embers-server/tests/model_state.rs`:
- Around line 361-367: After calling state.swap_sibling_nodes(first_child,
second_child) and state.move_node_before(first_child, second_child), capture the
parent's child list (e.g., call whatever test helper or API returns the split
children) immediately after each mutation and assert the relative order of
first_child and second_child changed as expected; specifically, after
swap_sibling_nodes assert their positions are reversed compared to the original,
and after move_node_before assert the final ordering matches the intended move,
then still call state.validate() as before. Use the existing identifiers state,
first_child, and second_child to locate where to insert these assertions.
- Around line 404-409: The test's final assert is vacuous because
session_root(&state, session_id) is already Node::Tabs; instead verify that
join_buffer_at_node actually inserted a Tabs wrapper or added a new child: after
calling state.join_buffer_at_node(root, detached_again,
NodeJoinPlacement::TabAfter) fetch the (possibly updated) root via
session_root(&state, session_id) or the returned node id, then inspect its
children by using state.node(root) and assert that the expected child node
(e.g., the newly created tabs node or that one of root's children matches
detached_again wrapped as a Tab) is present — replace the bare
assert!(matches!(state.node(root).expect("root"), Node::Tabs(_))) with an
assertion that checks the specific child node id or that a Tabs node contains
the buffer id detached_again.
In `@docs/config-api/action.md`:
- Around line 8-10: The docs leave accepted string literals undocumented for new
action parameters (e.g., break_current_node(destination: String), and other
actions using parameters named destination, scope, placement); update the
action.md entries for these functions to enumerate the valid literal values for
each parameter, include the exact allowed options (e.g., "node", "edge",
"global" or whatever your runtime accepts), and add a short example usage for
each parameter so readers know which strings to pass; make sure to update every
occurrence referencing destination, scope, and placement (including the
break_current_node signature and the other two action signatures) so the
documentation consistently lists allowed literals and an example for each.
In `@docs/config-api/defs/registration.rhai`:
- Around line 85-86: Update the doc signatures to document the accepted closed
string literal sets instead of using generic string types: change the parameter
types for break_current_node (destination) to "tab" | "floating", for the
functions that accept scope to "visible" | "full", for open_buffer_history's
placement to "floating" | "tab", and for join_buffer_here's placement to
"tab-after" | "tab-before" | "left" | "right" | "up" | "down"; locate these by
the function names break_current_node, open_buffer_history, join_buffer_here and
any other defs with destination/scope/placement parameters and replace their
parameter types in the doc signatures accordingly so the accepted values are
explicit.
In `@docs/config-api/defs/runtime.rhai`:
- Around line 134-135: Update the doc comment for fn break_current_node(_:
ActionApi, destination: string) -> Action to list the accepted destination
string literals (e.g., "tab", "window", "floating" or whatever the API
supports), and similarly update the doc comments for every other runtime.rhai
function that takes a destination, scope, or placement string parameter to
enumerate the valid string values the API accepts; locate these by searching for
functions with parameters named destination, scope, or placement and add a
concise list of allowed literal values to each doc comment so consumers can
reference the exact options in the API docs.
In `@docs/config-api/registration-action.md`:
- Around line 8-10: Update the API docs to enumerate the allowed literal values
for the string parameters used across the registration-action docs—specifically
for destination in break_current_node (fn break_current_node(_: ActionApi,
destination: String) -> Action) and for the scope and placement parameters used
elsewhere—by replacing the vague "String" type with a short list or table of
accepted values (e.g., exact token names or enum variants), include brief
descriptions for each literal and any precedence/compatibility rules, and ensure
the same explicit lists are added for the other occurrences of these parameters
so consumers don’t have to guess valid inputs.
🪄 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: 80128fe6-a925-4d7d-a6ea-09eab12c0a26
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (97)
crates/embers-cli/src/interactive.rscrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-cli/tests/support/mod.rscrates/embers-client/Cargo.tomlcrates/embers-client/src/client.rscrates/embers-client/src/config/loader.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/grid.rscrates/embers-client/src/input/keymap.rscrates/embers-client/src/presentation.rscrates/embers-client/src/renderer.rscrates/embers-client/src/scripting/model.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/src/scripting/types.rscrates/embers-client/src/state.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/reducer.rscrates/embers-client/tests/script_actions.rscrates/embers-client/tests/support/mod.rscrates/embers-core/Cargo.tomlcrates/embers-protocol/Cargo.tomlcrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-server/Cargo.tomlcrates/embers-server/src/buffer_runtime.rscrates/embers-server/src/model.rscrates/embers-server/src/persist.rscrates/embers-server/src/protocol.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/src/terminal_backend.rscrates/embers-server/tests/buffer_lifecycle.rscrates/embers-server/tests/model_state.rscrates/embers-test-support/Cargo.tomlcrates/embers-test-support/src/lib.rscrates/embers-test-support/src/pty.rscrates/embers-test-support/src/server.rscrates/embers-test-support/tests/buffer_runtime.rscrates/embers-test-support/tests/detach_move.rscrates/embers-test-support/tests/floating_windows.rscrates/embers-test-support/tests/integration.rscrates/embers-test-support/tests/nested_tabs.rscrates/embers-test-support/tests/protocol_server.rscrates/embers-test-support/tests/pty_smoke.rscrates/embers-test-support/tests/server_harness.rscrates/embers-test-support/tests/session_root_tabs.rscrates/embers-test-support/tests/split_layout.rscrates/embers-test-support/tests/support.rsdocs/activity-bell-policy.mddocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-256e957a.jsdocs/config-api-book/searchindex-bd58f06f.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/action.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/registration-action.mddocs/input-routing-policy.mddocs/input-routing.mddocs/render-source-contract.mddocs/terminal-backend-boundary.mddocs/terminal-capture-model.mddocs/terminal-runtime.mddocs/terminal-test-matrix.md
Comment on lines
+124
to
+127
| Ok(TerminalEvent::ConfigChanged) => { | ||
| let _ = configured.reload_config_if_changed()?; | ||
| terminal.write_bytes(&drain_terminal_output(&mut configured))?; | ||
| dirty = true; |
There was a problem hiding this comment.
Don’t tear down the interactive client on a bad config reload.
reload_config_if_changed()? makes any read/parse failure fatal here. A transient write or syntax error while the user is editing the config will exit the TUI instead of keeping the last known good bindings. Treat reload failures as non-fatal and surface them in the UI/log while retaining the current config.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-cli/src/interactive.rs` around lines 124 - 127, The
ConfigChanged branch currently calls configured.reload_config_if_changed()?
which makes reload errors fatal; instead, call
configured.reload_config_if_changed() and match on the Result so failures are
handled non‑fatally: on Ok, continue to call
terminal.write_bytes(&drain_terminal_output(&mut configured))? and set dirty =
true; on Err(e), render/log a non‑fatal error (e.g. via terminal.write_bytes
with an error message or a logger) and do not replace the active config or set
dirty, preserving the last known good bindings. Keep references to
TerminalEvent::ConfigChanged, configured.reload_config_if_changed(),
drain_terminal_output(&mut configured), terminal.write_bytes(...), and the dirty
flag to locate and update the logic.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/embers-client/tests/e2e.rs (1)
912-939:⚠️ Potential issue | 🟡 MinorPoll for the hidden bell state before asserting it in the projected tab bar.
wait_for_buffer_activity(..., ActivityState::Bell)only proves the server-side buffer record has flipped. The analogous hidden-activity test already needs a deadline-based poll for client projection, but this path does a single resync and asserts immediately, so it can still flap when the tab-bar state lags behind.⏱️ Suggested change
let mut client = MuxClient::connect(server.socket_path()) .await .expect("client connects"); - client - .resync_all_sessions() - .await - .expect("client resyncs sessions"); - refresh_all_snapshots(&mut client).await; - let session_id = session_id_by_name(&client, "alpha"); - let model = PresentationModel::project( - client.state(), - session_id, - Size { - width: 80, - height: 24, - }, - ) - .expect("projection succeeds"); - let tabs = model - .tab_bars - .iter() - .find(|tabs| tabs.node_id == fixture.nested_tabs_id) - .expect("nested tabs frame exists"); - assert!( - tabs.tabs - .iter() - .any(|tab| tab.title == "bg" && tab.activity == ActivityState::Bell) - ); + let deadline = Instant::now() + Duration::from_secs(2); + let mut saw_hidden_bell = false; + while Instant::now() < deadline { + client + .resync_all_sessions() + .await + .expect("client resyncs sessions"); + refresh_all_snapshots(&mut client).await; + let session_id = session_id_by_name(&client, "alpha"); + let model = PresentationModel::project( + client.state(), + session_id, + Size { + width: 80, + height: 24, + }, + ) + .expect("projection succeeds"); + let tabs = model + .tab_bars + .iter() + .find(|tabs| tabs.node_id == fixture.nested_tabs_id) + .expect("nested tabs frame exists"); + if tabs + .tabs + .iter() + .any(|tab| tab.title == "bg" && tab.activity == ActivityState::Bell) + { + saw_hidden_bell = true; + break; + } + tokio::time::sleep(Duration::from_millis(50)).await; + } + assert!(saw_hidden_bell, "hidden tab bell should propagate before reveal");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/tests/e2e.rs` around lines 912 - 939, The test currently does a single resync and immediately asserts the projected tab bar shows ActivityState::Bell, which can race; change the assertion to poll the client-side projection with a short deadline instead of a single check: after refresh_all_snapshots(&mut client) and obtaining model via PresentationModel::project(client.state(), session_id, ...), loop until timeout (e.g., deadline + small sleep) re-projecting or calling whatever helper that reads tab_bars from client.state() and checking that tab_bars.iter().find(|t| t.node_id == fixture.nested_tabs_id).tabs.iter().any(|tab| tab.title == "bg" && tab.activity == ActivityState::Bell) returns true, failing only if the deadline elapses; reference MuxClient::connect, resync_all_sessions, refresh_all_snapshots, session_id_by_name, PresentationModel::project, tab_bars, nested_tabs_id, and ActivityState::Bell when locating the code to modify.crates/embers-client/src/scripting/runtime.rs (1)
2201-2210:⚠️ Potential issue | 🟡 MinorDocument the new
blinkui.segmentoption.
parse_segment_optionsnow acceptsblink, but the exportedsegment(..., options)docs still omit it. That leaves the public scripting API and generated config docs out of sync; update the inline docs anddocs/config-api/defs/runtime.rhaitogether.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/scripting/runtime.rs` around lines 2201 - 2210, Update the public docs to include the newly accepted blink option for ui.segment: add documentation for the blink boolean option in the inline docs for the segment(..., options) function and update the generated config API definition file docs/config-api/defs/runtime.rhai to list blink alongside fg, bg, bold, italic, underline, dim in the segment options; reference the parse_segment_options function (which now parses "blink") to ensure the docs match the runtime behavior and describe blink as a boolean that enables blinking text for that segment.
♻️ Duplicate comments (2)
crates/embers-cli/tests/interactive.rs (1)
33-47:⚠️ Potential issue | 🟠 MajorOnly let
SpawnedEmbersclean up servers it actually started.
spawn_embersreturns the drop guard even for plainattachclients, so those tests can still signal and remove the pid file for the sharedTestServersocket. That recreates the teardown race the earlier review called out.🔧 Suggested fix
struct SpawnedEmbers { socket_path: PathBuf, + owns_server: bool, } impl SpawnedEmbers { - fn new(socket_path: PathBuf) -> Self { - Self { socket_path } + fn new(socket_path: PathBuf, owns_server: bool) -> Self { + Self { + socket_path, + owns_server, + } } } impl Drop for SpawnedEmbers { fn drop(&mut self) { + if !self.owns_server { + return; + } // Kill any orphaned __serve process for our socket kill_orphaned_server(&self.socket_path); } } @@ fn spawn_embers(args: &[&str], socket_path: PathBuf) -> (SpawnedEmbers, PtyHarness) { + let owns_server = args.first().copied() != Some("attach"); let binary = cargo_bin_path("embers"); @@ - (SpawnedEmbers::new(socket_path), harness) + (SpawnedEmbers::new(socket_path, owns_server), harness) }Also applies to: 100-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-cli/tests/interactive.rs` around lines 33 - 47, SpawnedEmbers' Drop unconditionally kills the orphaned server even for attach-only clients; add an ownership flag (e.g., started_server: bool) to the SpawnedEmbers struct, set it to true only in spawn_embers when you actually start a TestServer, and update new()/constructors accordingly; then change the Drop impl to call kill_orphaned_server(&self.socket_path) only when started_server is true; update any code paths that return the guard for attach clients to leave started_server=false so shared TestServer sockets are not cleaned up.crates/embers-protocol/schema/embers.fbs (1)
201-218:⚠️ Potential issue | 🟠 MajorKeep the previous FlatBuffers field numbers stable.
These edits don't just extend the schema; they appear to renumber existing fields.
BufferRequestandBufferRecordadd explicitid:values after inserting new fields ahead of older ones,NodeRequestinserts fields beforesizes/child_node_idswhile still using implicit IDs, andEnvelopeinsertsbuffer_location_responsebefore the existing event fields. Mixed-version client/server pairs will decode the wrong payloads unless the old numeric IDs are preserved for all pre-existing fields, or the protocol is explicitly version-bumped/gated.In FlatBuffers, if an existing table first used implicit field order and later adds explicit `(id: N)` annotations, do those IDs need to preserve the previous field numbers to stay wire-compatible? What happens if new fields are inserted before existing ones or existing fields are reordered?Also applies to: 220-243, 303-325, 527-568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/schema/embers.fbs` around lines 201 - 218, You renamed/reordered fields which changes their FlatBuffers numeric IDs; restore wire-compatibility by preserving the original numeric IDs for every existing field in tables like BufferRequest and BufferRecord (keep prior implicit ordering numbers when adding explicit (id: N) annotations), do not insert new fields before existing ones in NodeRequest (leave sizes and child_node_ids’ IDs unchanged) and add buffer_location_response to Envelope using a new unused id placed after existing event fields; alternatively, if you intend an incompatible change, explicitly bump/gate the protocol version everywhere instead of renumbering—update all added fields to use explicit unique ids and ensure no existing id is reused or reordered.
🤖 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/tests/interactive.rs`:
- Around line 825-835: Replace the fixed tokio::time::sleep call with an
observable wait that polls until the new binding is active: after writing to
config_path, repeatedly attempt to trigger the binding with
harness.write_all("\x07") and call harness.read_until_contains("after-reload",
short_timeout) in a loop (backed by a total deadline like a few seconds using
IO_TIMEOUT) until it succeeds or the deadline elapses, failing the test on
timeout; this ensures the test waits for the watcher/reload to complete instead
of relying on a fixed 700ms sleep.
In `@crates/embers-cli/tests/panes.rs`:
- Around line 309-314: The test currently hardcodes "session:1" when asserting
buffer location; instead extract the actual session id from the test
snapshot/state and use that variable in the expected string. Locate where the
test constructs or obtains its snapshot/state (the same setup that produced
buffer_id) and derive a session_id (e.g., snapshot.sessions[0].id or similar),
then replace the literal "1" in the assertion on
shown_stdout.contains(&format!("location\tsession:1 node:{leaf}")) with that
session_id variable so the expectation uses the actual allocated session id.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 333-340: ConfiguredClient::process_next_event_timeout currently
double-applies events because MuxClient::process_next_event_timeout already
calls handle_event; remove the extra self.handle_event(&event).await? and
instead either (A) replace the call to
self.client.process_next_event_timeout(timeout).await? with a timeout-wrapped
fetch-only call to self.client.next_event().await (e.g., use
tokio::time::timeout(timeout, self.client.next_event()).await and map the Ok/Err
appropriately) or (B) add a fetch-only timeout method on MuxClient (e.g.,
fetch_next_event_timeout) that does not call handle_event, then call that from
ConfiguredClient::process_next_event_timeout; ensure the chosen path returns the
fetched ServerEvent without applying handle_event again.
In `@crates/embers-client/src/scripting/documentation.rs`:
- Around line 322-348: The hardcoded replacements in rewrite_signature_literals
are brittle and need an explanatory comment: add a concise comment above the
rewrite_signature_literals function describing that these .replace calls map
Rhai-generated function signatures (e.g. the break_current_node,
join_buffer_here, and open_buffer_history signatures) into more specific literal
union types for documentation purposes, note that upstream Rhai signature
changes will break the string matches, and document that this is a
post-processing step for docs generation and should be updated if Rhai
signatures change; reference the exact patterns being rewritten (the two
variants with and without trailing semicolons) so future maintainers know what
to update.
In `@crates/embers-protocol/src/codec.rs`:
- Around line 2968-2989: The decoder currently accepts a
BufferWithLocationResponse even if the buffer record's id doesn't match the
location.buffer_id, producing an inconsistent BufferWithLocationResponse; inside
the fb::MessageKind::BufferWithLocationResponse match arm (around
decode_buffer_record, BufferWithLocationResponse construction and use of
location.buffer_id), after calling decode_buffer_record(buffer) compare the
decoded buffer's id with BufferId(location.buffer_id()) and return a
decoding/protocol error if they differ so mismatched payloads fail fast instead
of producing an inconsistent ServerResponse.
In `@crates/embers-protocol/src/types.rs`:
- Around line 135-146: Change the client_id fields in the Reveal and OpenHistory
enum variants to use Option<NonZeroU64> instead of Option<u64> so they cannot
hold Some(0); update the type for Reveal { client_id } and OpenHistory {
client_id } to Option<NonZeroU64> and add the required import
(std::num::NonZeroU64) where types are declared, ensuring any
construction/deserialization sites that create these variants convert/validate
u64 -> Option<NonZeroU64> (e.g., via NonZeroU64::new) consistent with how
ClientRequest is modeled.
In `@crates/embers-protocol/tests/family_round_trip.rs`:
- Around line 101-112: Add tests to cover the two missing client_id branches for
Buffer requests: add a ClientMessage::Buffer(BufferRequest::Reveal { request_id:
RequestId(...), buffer_id: BufferId(...), client_id: None }) case and a
ClientMessage::Buffer(BufferRequest::OpenHistory { request_id: RequestId(...),
buffer_id: BufferId(...), scope: BufferHistoryScope::..., placement:
BufferHistoryPlacement::..., client_id: Some(nonzero) }) case so the suite
exercises Reveal with client_id None and OpenHistory with client_id
Some(nonzero); place these alongside the existing examples (they should mirror
the existing RequestId/BufferId patterns near the ClientMessage::Buffer entries)
to ensure both accepted branches are tested and prevent regressions.
In `@crates/embers-server/src/persist.rs`:
- Around line 665-680: The test constructs a Rust PersistedWorkspace directly
which doesn't exercise real v1 deserialization; change the test to deserialize
an actual v1 JSON fixture (or a JSON string literal) that sets format_version to
FIRST_VERSIONED_FORMAT_VERSION and omits the v1-only fields
PersistedSession.zoomed_node and PersistedBuffer.kind, using
serde_json::from_str to produce a PersistedWorkspace and then call
load_current_workspace on that value; keep the existing assertions against
CURRENT_FORMAT_VERSION to ensure the true migration path (refer to
load_current_workspace, PersistedWorkspace, PersistedSession.zoomed_node,
PersistedBuffer.kind, FIRST_VERSIONED_FORMAT_VERSION, and
CURRENT_FORMAT_VERSION).
In `@crates/embers-server/src/server.rs`:
- Around line 2031-2038: The code currently hardcodes the floating history
window size via embers_core::FloatGeometry::new(10, 3, 100, 26) inside the
BufferHistoryPlacement::Floating branch; update
state.create_floating_from_buffer usage to derive geometry from a configurable
source instead of fixed literals—e.g., read a user-configurable default or
session/buffer-stored geometry and pass that FloatGeometry into
create_floating_from_buffer (keep the same arguments target_session_id,
helper_buffer_id, helper_title), or fall back to named constants for the
defaults so they can be tuned later; ensure the new code constructs the
FloatGeometry from the chosen config before calling
state.create_floating_from_buffer.
In `@crates/embers-server/src/state.rs`:
- Around line 1622-1623: close_tab() and close_floating() can delete subtrees
while leaving session.zoomed_node pointing at removed nodes; to fix, invoke
self.normalize_zoomed_node(session_id)? before the code path that removes the
subtree (and keep the subsequent self.heal_focus(session_id) call) so the
zoomed_node is cleared/normalized and validate() invariant holds; update both
close_tab and close_floating to call normalize_zoomed_node(session_id)? at the
start of their subtree-removal flow.
In `@docs/activity-bell-policy.md`:
- Line 5: The document skips heading level (H1 -> H3); update the section
headings like "### What counts as activity" to use H2 ("## What counts as
activity") and similarly change other `###` headings under the top-level H1 to
`##` so the document has a proper H1 -> H2 hierarchy; locate headings in the
file that start with "###" and convert them to "##" to restore correct
structure.
---
Outside diff comments:
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 2201-2210: Update the public docs to include the newly accepted
blink option for ui.segment: add documentation for the blink boolean option in
the inline docs for the segment(..., options) function and update the generated
config API definition file docs/config-api/defs/runtime.rhai to list blink
alongside fg, bg, bold, italic, underline, dim in the segment options; reference
the parse_segment_options function (which now parses "blink") to ensure the docs
match the runtime behavior and describe blink as a boolean that enables blinking
text for that segment.
In `@crates/embers-client/tests/e2e.rs`:
- Around line 912-939: The test currently does a single resync and immediately
asserts the projected tab bar shows ActivityState::Bell, which can race; change
the assertion to poll the client-side projection with a short deadline instead
of a single check: after refresh_all_snapshots(&mut client) and obtaining model
via PresentationModel::project(client.state(), session_id, ...), loop until
timeout (e.g., deadline + small sleep) re-projecting or calling whatever helper
that reads tab_bars from client.state() and checking that
tab_bars.iter().find(|t| t.node_id ==
fixture.nested_tabs_id).tabs.iter().any(|tab| tab.title == "bg" && tab.activity
== ActivityState::Bell) returns true, failing only if the deadline elapses;
reference MuxClient::connect, resync_all_sessions, refresh_all_snapshots,
session_id_by_name, PresentationModel::project, tab_bars, nested_tabs_id, and
ActivityState::Bell when locating the code to modify.
---
Duplicate comments:
In `@crates/embers-cli/tests/interactive.rs`:
- Around line 33-47: SpawnedEmbers' Drop unconditionally kills the orphaned
server even for attach-only clients; add an ownership flag (e.g.,
started_server: bool) to the SpawnedEmbers struct, set it to true only in
spawn_embers when you actually start a TestServer, and update new()/constructors
accordingly; then change the Drop impl to call
kill_orphaned_server(&self.socket_path) only when started_server is true; update
any code paths that return the guard for attach clients to leave
started_server=false so shared TestServer sockets are not cleaned up.
In `@crates/embers-protocol/schema/embers.fbs`:
- Around line 201-218: You renamed/reordered fields which changes their
FlatBuffers numeric IDs; restore wire-compatibility by preserving the original
numeric IDs for every existing field in tables like BufferRequest and
BufferRecord (keep prior implicit ordering numbers when adding explicit (id: N)
annotations), do not insert new fields before existing ones in NodeRequest
(leave sizes and child_node_ids’ IDs unchanged) and add buffer_location_response
to Envelope using a new unused id placed after existing event fields;
alternatively, if you intend an incompatible change, explicitly bump/gate the
protocol version everywhere instead of renumbering—update all added fields to
use explicit unique ids and ensure no existing id is reused or reordered.
🪄 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: f20d9460-ef15-4403-b4a0-2c521c78ed30
📒 Files selected for processing (74)
crates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/presentation.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/presentation.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-server/src/persist.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/model_state.rscrates/embers-test-support/src/pty.rsdocs/activity-bell-policy.mddocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-d719c517.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/action.mddocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/floating-ref.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/tree.mddocs/config-api/ui.md
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/embers-protocol/schema/embers.fbs (1)
190-199: 🧹 Nitpick | 🔵 TrivialConsider adding explicit field IDs to remaining request tables for consistency.
SessionRequest,FloatingRequest, andInputRequeststill use implicit field IDs whileBufferRequest,NodeRequest,BufferRecord, andEnvelopenow have explicit IDs. If these tables are expected to evolve, adding explicit IDs now would prevent future wire compatibility issues.This is a minor consistency concern - the tables with explicit IDs are the ones that changed in this PR, which is reasonable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/schema/embers.fbs` around lines 190 - 199, Add explicit field IDs to the remaining request tables to match the style used elsewhere: update the SessionRequest, FloatingRequest, and InputRequest table definitions so every field has an explicit numeric ID (e.g., name:Type = default; -> name:Type (id: N);) keeping existing field names and defaults, preserving types and order, and choosing unique incremental IDs that don't collide with other tables to ensure future wire compatibility and consistency with BufferRequest, NodeRequest, BufferRecord, and Envelope.
♻️ Duplicate comments (2)
crates/embers-server/src/state.rs (1)
624-656:⚠️ Potential issue | 🟠 MajorDefault floating geometry still uses pane-local dimensions.
last_render_sizehere is the anchor leaf’s render box, not the session viewport. As soon as the focused leaf is a narrow split or an already-focused floating, this centers the newFloatGeometryin the wrong coordinate space, so the window opens undersized and offset. The default needs client/session dimensions, notBufferViewNode.view.last_render_size.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/state.rs` around lines 624 - 656, default_floating_geometry is using the anchor leaf's BufferViewNode.view.last_render_size (pane-local) instead of the session/client viewport, causing mis-sized/offset floats; change it to obtain the session-level PtySize (the session/client viewport/terminal size) from the Session (e.g., session.viewport_size or session.pty_size) and use that for computing width/height and centering in FloatGeometry; only fall back to anchor_leaf.view.last_render_size if the session-level size is unavailable; update references in default_floating_geometry to use the session size instead of Node::BufferView.view.last_render_size.crates/embers-server/src/server.rs (1)
1900-1943:⚠️ Potential issue | 🟠 MajorValidate the target client before mutating reveal/history state.
reveal_buffer()still changes focus beforeresolve_reveal_client_id()/set_client_session()can fail, andopen_history_buffer()still creates/attaches the helper before that validation runs when the source buffer is already attached. A badclient_idcan therefore return an error after the request has already mutated server state.🔧 Safer ordering
async fn reveal_buffer(...) -> Result<(embers_protocol::BufferLocation, Vec<ServerEvent>)> { + let target_client_id = if requested_client_id.is_some() || self.has_attached_client().await { + Some(self.resolve_reveal_client_id(connection_id, requested_client_id).await?) + } else { + None + }; + let (location, mut events) = { // state mutations }; - if let Some(session_id) = location.session_id - && (requested_client_id.is_some() || self.has_attached_client().await) - { - let client_id = self - .resolve_reveal_client_id(connection_id, requested_client_id) - .await?; + if let (Some(session_id), Some(client_id)) = (location.session_id, target_client_id) { let (_, event) = self.set_client_session(client_id, Some(session_id)).await?; events.push(event); } }Apply the same pre-validation in
open_history_buffer()beforecreate_helper_buffer()and the subsequent attach calls.Also applies to: 1948-2055
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 1900 - 1943, reveal_buffer currently mutates focus state before validating the target client, so failures in resolve_reveal_client_id or set_client_session can leave the server mutated; change the ordering in reveal_buffer to resolve and validate the client first (call resolve_reveal_client_id(connection_id, requested_client_id).await? and set_client_session(client_id, Some(session_id)).await? to ensure success) and only then perform focus changes and push ServerEvent(s); apply the same pre-validation pattern in open_history_buffer by resolving/validating the client before calling create_helper_buffer() and any attach operations so no state is changed if client resolution/attachment fails (refer to functions reveal_buffer, resolve_reveal_client_id, set_client_session, open_history_buffer, create_helper_buffer).
🤖 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-client/src/configured_client.rs`:
- Around line 525-576: The handlers for Action::RevealBuffer and
Action::OpenBufferHistory currently abort early when current_viewport is None
(the `continue`), skipping the protocol request; instead always perform the
client.request_message / buffer_location_from_response flow and update
current_session_id and call self.client.resync_all_sessions().await, but only
call self.set_active_view(session_id, viewport) when current_viewport is
Some(viewport); remove the early `continue`, branch the set_active_view call on
the Option while keeping the request, session resolution (session_id),
current_session_id assignment, and resync logic intact (referencing
Action::RevealBuffer, Action::OpenBufferHistory, current_viewport,
buffer_location_from_response, request_message/BufferRequest, set_active_view,
current_session_id, resync_all_sessions).
In `@crates/embers-client/src/scripting/documentation.rs`:
- Around line 310-319: Add unit tests to cover the doc-rewrite helpers so format
drift is caught: write focused tests for rewrite_generated_markdown and
rewrite_signature_literals that feed representative input strings containing the
signature literal patterns and tab/button markup (e.g., missing button type
attribute, various signature literal escapes) and assert the exact expected
output after rewriting; include tests that exercise rewrite_generated_defs
behavior by creating a temporary file, calling rewrite_generated_defs (or
calling the underlying rewrite_signature_literals on read content) and verifying
the file content was updated as expected. Ensure tests reference the functions
rewrite_generated_markdown, rewrite_signature_literals, and
rewrite_generated_defs and cover positive and edge cases (no-op input,
already-correct input, and inputs needing transformation).
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 2006-2009: The overload-local doc explaining `options`/`blink` is
not applied to the public `segment` docs; update the canonical `segment`
overload (the primary doc comment for the `segment` function/constructor in
runtime.rs) to include the shared `options` and `blink` description so it
appears in generated docs, or alter the doc-generation step to merge overload
comments for `segment` so the `blink` explanation is preserved in docs; locate
the `segment` function/impl and move or duplicate the `blink`/`options`
paragraph into the canonical overload's doc comment (or adjust the
overload-merging logic) so the public `segment` API documentation includes the
`blink` detail.
In `@crates/embers-client/tests/presentation.rs`:
- Around line 147-165: The checks asserting all leaves/dividers have the
FLOATING_ID are vacuous if there are zero items; first collect or check that the
floating subtree actually contains at least one leaf and at least one divider,
then assert their floating_id values; specifically, for symbols presentation,
presentation.leaves, presentation.dividers and FLOATING_ID, ensure you assert
the matching-leaves collection is non-empty and the matching-dividers collection
is non-empty (e.g., collect/filter by floating_id or by matching floating frame)
before applying the existing all(...) assertions so the test fails if the
subtree is missing.
In `@crates/embers-protocol/src/codec.rs`:
- Around line 1770-1796: The encoder currently writes r.location.buffer_id
verbatim in ServerResponse::BufferWithLocation, which can create frames the
decoder will reject; update the ServerResponse::BufferWithLocation branch so the
buffer_id used for fb::BufferLocation::create is derived from r.buffer.id (e.g.
use r.buffer.id.into()) or validate that r.location.buffer_id == r.buffer.id and
return/propagate an error if they differ; modify the fb::BufferLocation::create
call in the ServerResponse::BufferWithLocation arm (and related construction
around encode_buffer_record / fb::BufferWithLocationResponse) to enforce this
invariant.
- Around line 2964-2993: Validate and reject a zero location buffer id when
decoding BufferLocation: in the BufferWithLocationResponse branch where you
build BufferLocation from
location.buffer_id()/session_id()/node_id()/floating_id (and the similar decoder
code paths around the other BufferWithLocation handling), add a check that
location.buffer_id() != 0 and return a ProtocolError::InvalidMessageOwned if
it's zero; ensure you reference the constructed BufferLocation and
RequestId/BufferWithLocationResponse code paths (the block that calls
required(...).buffer(), required(...).location(), decode_buffer_record(...), and
compares buffer.id to BufferId(location.buffer_id())) so malformed frames with
location.buffer_id == 0 are rejected instead of producing BufferId(0).
In `@crates/embers-server/src/server.rs`:
- Around line 2150-2171: The code clones the entire helper scrollback
(helper.lines.clone()) while holding the state mutex, causing heavy allocations
and long locks; instead, compute the requested start/end indices while holding
the lock, take a slice of helper.lines[start..end] and clone only that slice (or
collect into a Vec) before releasing the mutex; adjust the logic around
state.lock().await, BufferKind::Helper, buffer_id and the
ScrollbackSliceResponse construction so total_lines is computed from
helper.lines.len() under the lock but lines contains only the sliced clone of
helper.lines[start..end].
- Around line 1128-1150: The code currently pushes ServerEvent::NodeChanged for
node-backed locations then appends reveal_events which may already contain a
NodeChanged (from reveal_buffer()), causing duplicate invalidations; update the
block handling session_id so that when location.floating_id.is_none() you only
push NodeChanged if reveal_events does not already contain a
ServerEvent::NodeChanged for the same session_id (inspect reveal_events for
ServerEvent::NodeChanged(session_id) before pushing), or remove the preemptive
push entirely and rely on reveal_events from reveal_buffer() to emit NodeChanged
when appropriate; look for the variables/events named location, reveal_events,
ServerEvent::NodeChanged, FloatingChangedEvent, NodeChangedEvent and adjust that
logic accordingly.
In `@crates/embers-server/src/state.rs`:
- Around line 1380-1400: The break_node implementation can reorder tabs when
node_id was already a direct child of a Node::Tabs because
detach_node_from_owner() removes it and nearest_tabs_ancestor(old_parent)
returns the same tabs container, then add_tab_sibling() appends to the end; make
break_node idempotent by detecting this case and skipping re-insertion: after
obtaining old_parent from detach_node_from_owner(node_id) and before calling
nearest_tabs_ancestor/add_tab_sibling, check whether to_floating is false and
old_parent is a Tabs container and that the original index (or sibling relation)
indicates the node was already inside that tabs container; if so, either
re-insert at the original index (preserving order) or simply return Ok(()) to
avoid changing order. Update break_node to use the existing symbols break_node,
detach_node_from_owner, nearest_tabs_ancestor, add_tab_sibling, and
ensure_root_tabs_container to implement this guard.
In `@crates/embers-test-support/src/pty.rs`:
- Around line 15-25: is_pty_available currently does a single openpty probe;
change it to call the same retry/backoff helper used by spawn() so transient
openpty failures are retried rather than treated as permanently unavailable.
Replace the direct NativePtySystem::default().openpty(...) call in
is_pty_available (and the similar probe at lines 46-63) with the existing
retry/backoff wrapper used by spawn() so the helper executes the openpty attempt
with identical retry policy and returns true on a successful attempt and false
after retries exhaust. Ensure you reference the same openpty invocation
(NativePtySystem::default().openpty(...)/PortableSize) inside that retry helper
so behavior is identical.
- Around line 73-84: The PTY setup currently spawns the child via
pair.slave.spawn_command(command_builder) before completing master-side setup,
which can leak a Child if try_clone_reader() or take_writer() fail; reorder the
calls so you first call pair.master.try_clone_reader().map_err(|e|
MuxError::pty(e.to_string())) and pair.master.take_writer().map_err(|e|
MuxError::pty(e.to_string())) to obtain reader and writer, and only then call
pair.slave.spawn_command(command_builder).map_err(|e|
MuxError::pty(e.to_string())) to produce the child, preserving the same error
mapping and variables (reader, writer, child) used later.
---
Outside diff comments:
In `@crates/embers-protocol/schema/embers.fbs`:
- Around line 190-199: Add explicit field IDs to the remaining request tables to
match the style used elsewhere: update the SessionRequest, FloatingRequest, and
InputRequest table definitions so every field has an explicit numeric ID (e.g.,
name:Type = default; -> name:Type (id: N);) keeping existing field names and
defaults, preserving types and order, and choosing unique incremental IDs that
don't collide with other tables to ensure future wire compatibility and
consistency with BufferRequest, NodeRequest, BufferRecord, and Envelope.
---
Duplicate comments:
In `@crates/embers-server/src/server.rs`:
- Around line 1900-1943: reveal_buffer currently mutates focus state before
validating the target client, so failures in resolve_reveal_client_id or
set_client_session can leave the server mutated; change the ordering in
reveal_buffer to resolve and validate the client first (call
resolve_reveal_client_id(connection_id, requested_client_id).await? and
set_client_session(client_id, Some(session_id)).await? to ensure success) and
only then perform focus changes and push ServerEvent(s); apply the same
pre-validation pattern in open_history_buffer by resolving/validating the client
before calling create_helper_buffer() and any attach operations so no state is
changed if client resolution/attachment fails (refer to functions reveal_buffer,
resolve_reveal_client_id, set_client_session, open_history_buffer,
create_helper_buffer).
In `@crates/embers-server/src/state.rs`:
- Around line 624-656: default_floating_geometry is using the anchor leaf's
BufferViewNode.view.last_render_size (pane-local) instead of the session/client
viewport, causing mis-sized/offset floats; change it to obtain the session-level
PtySize (the session/client viewport/terminal size) from the Session (e.g.,
session.viewport_size or session.pty_size) and use that for computing
width/height and centering in FloatGeometry; only fall back to
anchor_leaf.view.last_render_size if the session-level size is unavailable;
update references in default_floating_geometry to use the session size instead
of Node::BufferView.view.last_render_size.
🪄 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: 6fab7c28-defa-4b68-820b-d086a54e6415
📒 Files selected for processing (74)
crates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/presentation.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/presentation.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-server/src/persist.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/model_state.rscrates/embers-test-support/src/pty.rsdocs/activity-bell-policy.mddocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-d719c517.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/action.mddocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/floating-ref.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/tree.mddocs/config-api/ui.md
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/embers-protocol/src/codec.rs (1)
2554-2595:⚠️ Potential issue | 🟠 MajorReject zero in the new required-ID decoders.
These branches still turn the wire sentinel
0intoBufferId(0),NodeId(0), orSessionId(0)for required fields. MalformedInspect/GetLocation/Reveal/OpenHistoryandZoom/Unzoom/ToggleZoom/SwapSiblings/BreakNode/JoinBufferAtNode/MoveNodeBefore/MoveNodeAfterframes should fail at the codec boundary instead of leaking bogus ids downstream. A smalldecode_required_*helper would keep the new variants consistent.Also applies to: 2728-2765
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/src/codec.rs` around lines 2554 - 2595, Implement and use helper decoders that reject the wire sentinel 0 for required IDs instead of constructing zero-valued IDs; e.g. add decode_required_buffer_id(buffer_id: u64) -> Result<BufferId, _> which returns an Err for 0 and Ok(BufferId(id)) otherwise, and a decode_required_nonzero_u64(id: u64) -> Result<NonZeroU64, _> that returns Err on 0 (or map directly to NonZeroU64::new(id).ok_or(...)). Replace places that currently do BufferId(req.buffer_id()) or NonZeroU64::new(req.client_id()) inside the BufferRequest branches (Inspect, GetLocation, Reveal, OpenHistory, Capture/ CaptureVisible/ScrollbackSlice if applicable) and the node/session-related branches (Zoom, Unzoom, ToggleZoom, SwapSiblings, BreakNode, JoinBufferAtNode, MoveNodeBefore, MoveNodeAfter, etc.) to call these helpers and propagate the error so malformed frames with id==0 fail at the codec boundary.crates/embers-cli/tests/interactive.rs (1)
643-664: 🧹 Nitpick | 🔵 TrivialAssert the scrollback indicator or rename the test.
This test only proves that
PageUpreveals older output. It never checks the indicator named in the test title, so an indicator regression would still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-cli/tests/interactive.rs` around lines 643 - 664, The test page_up_enters_local_scrollback_and_shows_indicator currently only verifies PageUp reveals older output via page_up_until_visible but never asserts the scrollback indicator; either rename the test to remove "and_shows_indicator" or add an explicit assertion after page_up_until_visible that the indicator is rendered (e.g., call harness.read_until_contains with the expected indicator string and STARTUP_TIMEOUT or otherwise probe the rendered output for the indicator token using the existing harness). Locate the test function page_up_enters_local_scrollback_and_shows_indicator and update it to perform the indicator check (or rename the function) accordingly.
♻️ Duplicate comments (2)
crates/embers-server/src/server.rs (2)
2066-2103:⚠️ Potential issue | 🟠 MajorClean up the helper buffer when history placement fails.
create_helper_buffer()happens before the fallible placement call. Ifadd_root_tab_from_buffer()orcreate_floating_from_buffer()fails here, the request exits with a brand-new detached helper buffer still in state;switch_reveal_client()has also already run, so the operation is currently non-atomic.♻️ Roll back the helper buffer on the error path
let helper_buffer_id = { let mut state = self.state.lock().await; let helper_buffer_id = state.create_helper_buffer( helper_title.clone(), source_buffer_id, helper_scope, source_cwd, lines, ); - match placement { + let attach_result: Result<()> = (|| match placement { BufferHistoryPlacement::Tab => { state.add_root_tab_from_buffer( target_session_id, helper_title, helper_buffer_id, - )?; + ) } BufferHistoryPlacement::Floating => { let geometry = state.default_floating_geometry(target_session_id)?; state.create_floating_from_buffer( target_session_id, helper_buffer_id, geometry, Some(helper_title), - )?; + ) } - } + })(); + if let Err(error) = attach_result { + let _ = state.remove_buffer(helper_buffer_id); + return Err(error); + } helper_buffer_id };I would also defer
switch_reveal_client()until after the helper has been attached and focused successfully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 2066 - 2103, Move the call to switch_reveal_client so it runs only after the helper buffer is successfully attached and focused, and make the helper-buffer creation atomic by cleaning it up on placement failure: after calling state.create_helper_buffer (helper_buffer_id) attempt the placement with add_root_tab_from_buffer or create_floating_from_buffer (and default_floating_geometry) inside a block that captures any error; on error call state.delete_buffer or the appropriate removal API for helper_buffer_id to rollback the created buffer before returning the error, and only call switch_reveal_client(target_client_id, target_session_id) once placement succeeds. Ensure you reference helper_buffer_id, create_helper_buffer, add_root_tab_from_buffer, create_floating_from_buffer, default_floating_geometry and switch_reveal_client when editing the flow.
1980-1985:⚠️ Potential issue | 🟠 Major
Revealcan fail after already switching the client.
switch_reveal_client()mutatescurrent_session_idbeforefocus_revealed_buffer()does its second state lookup. If another request detaches, closes, or moves the buffer between those awaits, this path returns an error with the client already moved to a different session. Please make the client/session update atomic with the reveal, or restore the previous client session on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 1980 - 1985, The code calls switch_reveal_client(client_id, session_id).await? before focus_revealed_buffer(buffer_id).await?, which can leave current_session_id mutated if focus_revealed_buffer fails; either make the update atomic or roll it back on failure: capture the previous client/session state before calling switch_reveal_client (or implement a new transactional helper like switch_and_focus_revealed_buffer that performs both operations under the same lock), then call focus_revealed_buffer, and if that call errors restore the previous session via switch_reveal_client (or abort the transactional helper) so the client/session update is not left half-applied; use the existing function names resolve_optional_reveal_client_id, switch_reveal_client, and focus_revealed_buffer to locate and implement the change.
🤖 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/tests/panes.rs`:
- Around line 475-486: After calling run_cli to break the node into a floating
pane and collecting session_snapshot_by_name("alpha"), add an assertion that the
floating pane's root equals second_pane_id (not just that there is one floating
pane). Locate the snapshot usage (session_snapshot_by_name and the
snapshot.floating collection) and assert the floating root id matches
second_pane_id (e.g., check snapshot.floating[0].root or the appropriate root/id
field on the first floating entry) so the test fails if the wrong pane was
floated.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 575-579: The match arm currently aborts handling when
current_viewport is None by zipping current_session_id and current_viewport and
continuing, which drops explicit-id actions; change the logic so
current_session_id is checked independently from current_viewport and only
require/unpack current_viewport for actions that need a PresentationModel.
Concretely, use current_session_id.as_ref()/map or if-let to get session_id when
present, then match on the action variants (e.g., UnzoomNode { session_id:
Some(_) }, MoveNodeBefore, MoveNodeAfter) and handle those without requiring
current_viewport, while for variants that need viewport keep the existing
current_viewport extraction and continue only when it’s missing; update the
match arm around current_session_id/current_viewport rather than unconditionally
zipping them.
- Around line 352-355: The current logic calls event_session_id() before
self.client.handle_event(event) for all events which prevents newly
created/attached buffers from finding their attachment node; change handle_event
so you only precompute the fallback session context for detach events (e.g.,
when matches ServerEvent::BufferDetached) by calling event_session_id() before
self.client.handle_event(event), but for other events (BufferCreated/attachment
cases) call self.client.handle_event(event).await? first and then compute
event_session_id() afterward so buffer_created hooks get the up-to-date
session/presentation context; adjust uses of the session_id variable accordingly
in handle_event.
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 1305-1428: Extract the repeated string-to-enum parsing in
open_buffer_history (for BufferHistoryScope and BufferHistoryPlacement),
break_current_node (NodeBreakDestination), and join_buffer_here
(NodeJoinPlacement) into small helper functions (e.g.,
parse_buffer_history_scope, parse_buffer_history_placement,
parse_node_break_destination, parse_node_join_placement) placed alongside the
existing parse_split_direction/parse_notify_level helpers; each helper should
accept &str and a call position (or return a RhaiResult with runtime_error_at on
invalid input) and return the appropriate enum, then replace the match blocks in
open_buffer_history, break_current_node, and join_buffer_here to call those
helpers and propagate errors using parse_buffer_id/parse_node_id as currently
done.
In `@crates/embers-protocol/src/codec.rs`:
- Around line 1897-1905: The ServerResponse::BufferLocation serialization branch
currently passes r.location.buffer_id through to fb::BufferLocation::create even
though the decoder rejects BufferLocationResponse with buffer_id == 0; modify
the ServerResponse::BufferLocation arm (the code calling
fb::BufferLocation::create with BufferLocationArgs) to validate
r.location.buffer_id != 0 before creating the FlatBuffer, and if it is 0 either
return an error/result indicating invalid input or skip/omit encoding that frame
so you never emit a BufferLocation with buffer_id == 0; update any callers of
this encoder path to handle the error/skip case accordingly.
In `@crates/embers-server/src/state.rs`:
- Around line 1424-1429: The code in join_buffer_at_node currently returns
Ok(()) when BufferAttachment::Attached(source_view) is already under node_id,
which silently treats the placement as successful; instead, detect that
source_view == node_id or node_is_descendant_of(source_view, node_id)? and
return a descriptive error (e.g., Err(BufferError::AlreadyContained or a
suitable domain error) rather than Ok(())), so callers know the buffer was not
moved; update the branch that now does "if source_view == node_id ||
self.node_is_descendant_of(...)" to return an error referencing source_view and
node_id (using the existing BufferAttachment::Attached, buffer(buffer_id)?,
node_is_descendant_of, and join_buffer_at_node identifiers).
In `@crates/embers-test-support/src/pty.rs`:
- Around line 38-50: The retry loop in the openpty call (inside the function
that iterates for attempt in 0..=Self::MAX_RETRIES and uses Self::RETRY_DELAY)
should add a small random jitter to the sleep so concurrent retries don’t retry
in lockstep; implement this by generating a per-attempt random jitter (e.g.
using thread_rng().gen_range(0..JITTER_MS) or similar) and add it to the
computed backoff before calling thread::sleep, introduce a small JITTER_MS const
(or compute as a fraction of RETRY_DELAY) and ensure you use Duration for the
combined delay, keeping last_error and the existing retry/count logic unchanged.
---
Outside diff comments:
In `@crates/embers-cli/tests/interactive.rs`:
- Around line 643-664: The test
page_up_enters_local_scrollback_and_shows_indicator currently only verifies
PageUp reveals older output via page_up_until_visible but never asserts the
scrollback indicator; either rename the test to remove "and_shows_indicator" or
add an explicit assertion after page_up_until_visible that the indicator is
rendered (e.g., call harness.read_until_contains with the expected indicator
string and STARTUP_TIMEOUT or otherwise probe the rendered output for the
indicator token using the existing harness). Locate the test function
page_up_enters_local_scrollback_and_shows_indicator and update it to perform the
indicator check (or rename the function) accordingly.
In `@crates/embers-protocol/src/codec.rs`:
- Around line 2554-2595: Implement and use helper decoders that reject the wire
sentinel 0 for required IDs instead of constructing zero-valued IDs; e.g. add
decode_required_buffer_id(buffer_id: u64) -> Result<BufferId, _> which returns
an Err for 0 and Ok(BufferId(id)) otherwise, and a
decode_required_nonzero_u64(id: u64) -> Result<NonZeroU64, _> that returns Err
on 0 (or map directly to NonZeroU64::new(id).ok_or(...)). Replace places that
currently do BufferId(req.buffer_id()) or NonZeroU64::new(req.client_id())
inside the BufferRequest branches (Inspect, GetLocation, Reveal, OpenHistory,
Capture/ CaptureVisible/ScrollbackSlice if applicable) and the
node/session-related branches (Zoom, Unzoom, ToggleZoom, SwapSiblings,
BreakNode, JoinBufferAtNode, MoveNodeBefore, MoveNodeAfter, etc.) to call these
helpers and propagate the error so malformed frames with id==0 fail at the codec
boundary.
---
Duplicate comments:
In `@crates/embers-server/src/server.rs`:
- Around line 2066-2103: Move the call to switch_reveal_client so it runs only
after the helper buffer is successfully attached and focused, and make the
helper-buffer creation atomic by cleaning it up on placement failure: after
calling state.create_helper_buffer (helper_buffer_id) attempt the placement with
add_root_tab_from_buffer or create_floating_from_buffer (and
default_floating_geometry) inside a block that captures any error; on error call
state.delete_buffer or the appropriate removal API for helper_buffer_id to
rollback the created buffer before returning the error, and only call
switch_reveal_client(target_client_id, target_session_id) once placement
succeeds. Ensure you reference helper_buffer_id, create_helper_buffer,
add_root_tab_from_buffer, create_floating_from_buffer, default_floating_geometry
and switch_reveal_client when editing the flow.
- Around line 1980-1985: The code calls switch_reveal_client(client_id,
session_id).await? before focus_revealed_buffer(buffer_id).await?, which can
leave current_session_id mutated if focus_revealed_buffer fails; either make the
update atomic or roll it back on failure: capture the previous client/session
state before calling switch_reveal_client (or implement a new transactional
helper like switch_and_focus_revealed_buffer that performs both operations under
the same lock), then call focus_revealed_buffer, and if that call errors restore
the previous session via switch_reveal_client (or abort the transactional
helper) so the client/session update is not left half-applied; use the existing
function names resolve_optional_reveal_client_id, switch_reveal_client, and
focus_revealed_buffer to locate and implement the change.
🪄 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: c19a67d8-66a3-4d62-b0e6-f61b73f7b8d6
📒 Files selected for processing (74)
crates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/presentation.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/presentation.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-server/src/persist.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/model_state.rscrates/embers-test-support/src/pty.rsdocs/activity-bell-policy.mddocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-deaec7e4.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/action.mddocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/floating-ref.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/tree.mddocs/config-api/ui.md
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/embers-protocol/src/codec.rs (1)
250-257:⚠️ Potential issue | 🟠 MajorValidate required ids before serializing these new request variants.
The decoder now treats zero-valued required ids as protocol errors, but these newly added buffer/node request arms still write raw
.into()values.Inspect,GetLocation,Focus,MoveBufferToNode,Zoom,SwapSiblings,MoveNodeAfter, etc. can therefore still emit frames this codec will reject on the other side if a caller ever constructsBufferId(0),NodeId(0), orSessionId(0). Fail fast on encode for the required ids instead of relying on the remote decoder to catch them.Also applies to: 555-724, 1086-1430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/src/codec.rs` around lines 250 - 257, The Buffer/Node arms in encode_client_message (and the helper functions encode_buffer_request / encode_node_request) must validate required id types (BufferId, NodeId, SessionId) are non-zero before serializing and return an Err(ProtocolError) instead of writing `.into()` values; locate request variants that carry ids (Inspect, GetLocation, Focus, MoveBufferToNode, Zoom, SwapSiblings, MoveNodeAfter, etc.) and add an early check that the id.0 != 0 (or equivalent) and return a protocol error when zero, so the encoder fails fast rather than emitting frames the decoder will reject.crates/embers-protocol/src/types.rs (1)
499-517: 🛠️ Refactor suggestion | 🟠 MajorMake helper buffers a distinct wire shape.
kind,read_only,helper_source_buffer_id, andhelper_scopeadd a second record shape, butBufferRecordstill always exposes PTY-centric fields likecommand,pid,state, andenv. That lets the protocol encode contradictory records (kind: Helperwith process metadata, orkind: Ptywith helper metadata) and forces every consumer to re-implement the invariants. Splitting this into kind-specific payloads would make the API self-validating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/src/types.rs` around lines 499 - 517, The current BufferRecord exposes PTY-specific fields alongside helper-specific fields which allows invalid combinations; refactor BufferRecord into a sum type with distinct variants (e.g. enum BufferRecord { Pty { id: BufferId, title: String, command: Vec<String>, cwd: Option<String>, state: BufferRecordState, pid: Option<u32>, attachment_node_id: Option<NodeId>, read_only: bool }, Helper { id: BufferId, title: String, attachment_node_id: Option<NodeId>, helper_source_buffer_id: BufferId, helper_scope: BufferHistoryScope } }) so each variant only contains its relevant fields and remove the old BufferRecordKind and the mutual fields (or keep a helper enum only if needed for legacy). Update all uses of BufferRecord, BufferRecordKind, and field accesses (e.g. places reading command, pid, state, read_only, helper_source_buffer_id, helper_scope) to pattern-match the new variants and adjust serialization/derive attributes accordingly (retain BufferId, BufferRecordState, NodeId, BufferHistoryScope types).
♻️ Duplicate comments (1)
crates/embers-client/src/scripting/runtime.rs (1)
2648-2655:⚠️ Potential issue | 🟡 MinorExpand the literal-parser regression cases.
This still exercises only one accepted literal per parser and never hits the invalid branches, so the remaining accepted values and the
runtime_error_at(...)paths can drift unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/scripting/runtime.rs` around lines 2648 - 2655, The test parse_history_and_node_enums_accept_known_literals only asserts one positive literal per parser; expand it to cover every accepted literal for parse_buffer_history_scope, parse_buffer_history_placement, parse_node_break_destination, and parse_node_join_placement (call each parser with all valid string variants) and add negative cases that pass invalid strings to assert Err results so the runtime_error_at/... error paths are exercised; reuse Position::NONE and explicit invalid literals (e.g., empty string, "invalid", or misspelled variants) to ensure both success and failure branches are covered for each parser.
🤖 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/tests/panes.rs`:
- Around line 361-372: The test currently only checks for
ServerResponse::Error(_) which lets unrelated errors slip through; update the
assertion after sending ClientMessage::Input(InputRequest::Send) (the send
variable) to assert the specific error variant representing a read-only buffer
rejection (replace the wildcard with the concrete ServerError variant used by
the codebase, e.g., ServerError::ReadOnlyBuffer or
ServerError::WriteToReadOnlyBuffer) so the test ties explicitly to the read-only
contract rather than any error.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 1197-1246: Remove the stale duplicate Action handlers from the
match inside execute_action(): prune the arms handling FocusBuffer,
RevealBuffer, OpenBufferHistory, ZoomNode (if duplicated by queue-aware path),
and UnzoomNode so that execution funnels to the existing queue-aware
implementations in execute_actions()/execute_without_presentation(); locate the
duplicate arms (they call self.focus_buffer, client.request_message with
ClientMessage::Buffer/Node variants and client.resync_all_sessions()) and delete
them, ensuring execute_action() delegates to the central queue-aware path and no
duplicate logic remains.
- Around line 613-618: The Action::FocusBuffer branch currently returns Ok(None)
when current_session_id is missing, silently dropping the action and possibly
calling focus_buffer with the wrong session; change it to resolve the correct
session from buffer_id first (e.g., look up the session/attachment for
buffer_id) and use that session id when calling self.focus_buffer(session_id,
buffer_id). If no session can be resolved for buffer_id, surface a real error
(return Err or a descriptive error variant) instead of returning Ok(None);
ensure you update the Action::FocusBuffer handling to consult the buffer→session
mapping before relying on current_session_id.
In `@crates/embers-client/src/presentation.rs`:
- Around line 95-123: The code only treats a zoomed node as inside a floating
when zoomed_node_id == window.root_node_id; update the logic to detect floating
ancestry: when session.zoomed_node_id is Some(id) and state.nodes contains id,
search state.floating for a window whose root_node_id is an ancestor of id (not
just equal) and treat that window as the containing floating. When found, push a
FloatingFrame with window.id and call projector.project_node(id, content_rect,
Some(window.id), ...) so focused_floating_id() and floating-targeted actions
work; if none found fall back to projector.project_node(..., None, ...). Also
add a regression test that zooms a child node inside a floating split/tabs tree
to assert focused_floating_id() and floating actions still target the floating.
In `@crates/embers-client/tests/e2e.rs`:
- Line 2: The test code mixes std::time::Instant with tokio::time::Instant;
update the import and usages to use tokio's time types consistently. Replace the
top-level import use std::time::{Duration, Instant}; with use
tokio::time::{Duration, Instant}; and change any direct std::time::Instant uses
in tests (e.g., the Instant used in the test around wait_for_visible_snapshot
and wait_for_buffer_activity and the Instant at the test site that currently
uses std::time::Instant) to tokio::time::Instant so deadline comparisons all use
the same clock.
In `@crates/embers-protocol/src/codec.rs`:
- Around line 1669-1675: The branch that builds a BufferWithLocationResponse
accepts a BufferRecord with BufferId(0) and therefore can serialize a message
that decode_server_envelope will later reject; enforce the same non-zero
invariant before building the FlatBuffer by validating
response.location.buffer_id (or the source r.buffer.id) is non-zero and
returning ProtocolError::InvalidMessageOwned with the same message when it is
zero. Update the branch matching
ServerEnvelope::Response(ServerResponse::BufferLocation(response)) and the
equivalent code around the other occurrence (near the region mentioned for
1822-1848) to perform this check prior to constructing the
BufferWithLocationResponse so zero IDs are rejected early.
In `@crates/embers-protocol/src/types.rs`:
- Around line 180-186: The BufferWithLocationResponse currently duplicates
identity/attachment via buffer.id vs location.buffer_id and
buffer.attachment_node_id vs location.node_id; change the types so there is a
single canonical source of truth: either remove buffer_id and node_id from
BufferLocation (keep only session_id and floating_id) and let
BufferWithLocationResponse use buffer.id and buffer.attachment_node_id as the
canonical values, or refactor to a single Attachment enum used by
BufferWithLocationResponse that encodes node vs session vs floating. Update the
BufferLocation and BufferWithLocationResponse definitions (and any
serializers/deserializers) so only one authoritative owner of buffer
identity/attachment exists (refer to BufferLocation, BufferWithLocationResponse,
buffer.id, location.buffer_id, buffer.attachment_node_id, location.node_id).
In `@crates/embers-protocol/tests/family_round_trip.rs`:
- Around line 328-348: Add a test fixture that exercises a fully detached
BufferLocation (no session_id, no node_id, no floating_id) so the None/zero
codec path is covered: insert another
ServerEnvelope::Response(ServerResponse::BufferLocation(BufferLocationResponse {
request_id: RequestId(<new>), location: BufferLocation { buffer_id:
BufferId(<new>), session_id: None, node_id: None, floating_id: None } }))
alongside the existing BufferLocation/BufferWithLocation entries to validate
round-trip encoding/decoding for detached buffers.
In `@crates/embers-server/src/persist.rs`:
- Around line 73-84: The two separate derive attributes on the
PersistedBufferKind enum should be consolidated into a single #[derive(...)] to
match project style; remove the duplicated #[derive(Default)] and combine all
traits (Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Default) into one
derive placed alongside the existing serde attribute (keep #[serde(tag = "kind",
rename_all = "snake_case")] and the enum name PersistedBufferKind intact) so the
enum uses a single consolidated derive attribute for readability and
consistency.
In `@crates/embers-server/src/server.rs`:
- Around line 2022-2030: The double reverse + take + collect pattern on
helper.lines is unnecessary; instead, consume helper.lines with into_iter(),
skip the first helper.lines.len().saturating_sub(count) elements, then collect
the remainder into a Vec to get the last `count` lines. Replace the chain
starting at
helper.lines.into_iter().rev().take(count).collect::<Vec<_>>().into_iter().rev().collect()
with an into_iter().skip(helper.lines.len().saturating_sub(count)).collect()
approach so it avoids the intermediate Vec and extra reversals.
In `@crates/embers-server/src/state.rs`:
- Around line 1291-1310: When enabling zoom in zoom_node (and in
toggle_zoom_node when setting zoomed_node to Some), ensure the session's
focused_leaf and the per-leaf view.focused flag are moved into the zoomed
subtree: check if current session.focused_leaf is a descendant of node_id (use
whatever descendant-test helper exists), and if not set session.focused_leaf =
resolve_focus_candidate(node_id); also clear the previous leaf's view.focused
and set view.focused = true for the new focused_leaf via
session_mut(session_id)?.views[...] or the per-leaf view accessor. Update these
changes in the same places you set session.zoomed_node (functions: zoom_node and
toggle_zoom_node) so the session always has a focused leaf inside the zoomed
subtree.
- Around line 1467-1478: The insertion index calculation for Node::Tabs can
produce active+1 which is out of range for empty tabs; update the insert_index
logic (the block using self.node(tabs_id) and reading tabs.active) to clamp the
computed index into 0..=tabs.tabs.len() instead of returning active or active+1
directly. Specifically, for NodeJoinPlacement::TabBefore use min(active,
tabs.tabs.len()) and for NodeJoinPlacement::TabAfter use min(active + 1,
tabs.tabs.len()) (or handle empty tabs by returning 0), so add_tab_sibling_at()
will receive a valid index.
In `@crates/embers-server/tests/model_state.rs`:
- Around line 488-509: The test currently only asserts that detached_again_view
appears in the Tabs root after calling state.join_buffer_at_node(...,
NodeJoinPlacement::TabAfter); update the assertion to verify that the tabs list
contains both the original target child (the subtree returned by
root_tab_child(session_id, 0) or session_root child) and detached_again_view in
the correct order (target_root followed by detached_again_view). Locate the test
section using join_buffer_at_node, NodeJoinPlacement::TabAfter,
root_tab_child/session_root, attached_view and Node::Tabs/root_tabs.tabs and
replace the single any(...) check with an ordered check that the tabs vector
contains the two expected child IDs in sequence.
In `@crates/embers-test-support/src/pty.rs`:
- Around line 90-95: thread::spawn() can panic on thread creation and if that
happens the already-started child (created via pair.slave.spawn_command and
bound to the variable child) will be dropped but not reaped, leaving an orphaned
subprocess; replace thread::spawn with std::thread::Builder::spawn() so you can
handle spawn errors, and if Builder::spawn returns Err then call child.kill()
and child.wait() (or otherwise reap the process) before returning an Err (e.g.
MuxError::pty(...)); ensure the created reader thread assignment (previously
reader_join) remains set only on successful spawn and propagate the spawn error
instead of letting the child leak so PtyHarness owns a properly-cleaned child or
returns a failure.
---
Outside diff comments:
In `@crates/embers-protocol/src/codec.rs`:
- Around line 250-257: The Buffer/Node arms in encode_client_message (and the
helper functions encode_buffer_request / encode_node_request) must validate
required id types (BufferId, NodeId, SessionId) are non-zero before serializing
and return an Err(ProtocolError) instead of writing `.into()` values; locate
request variants that carry ids (Inspect, GetLocation, Focus, MoveBufferToNode,
Zoom, SwapSiblings, MoveNodeAfter, etc.) and add an early check that the id.0 !=
0 (or equivalent) and return a protocol error when zero, so the encoder fails
fast rather than emitting frames the decoder will reject.
In `@crates/embers-protocol/src/types.rs`:
- Around line 499-517: The current BufferRecord exposes PTY-specific fields
alongside helper-specific fields which allows invalid combinations; refactor
BufferRecord into a sum type with distinct variants (e.g. enum BufferRecord {
Pty { id: BufferId, title: String, command: Vec<String>, cwd: Option<String>,
state: BufferRecordState, pid: Option<u32>, attachment_node_id: Option<NodeId>,
read_only: bool }, Helper { id: BufferId, title: String, attachment_node_id:
Option<NodeId>, helper_source_buffer_id: BufferId, helper_scope:
BufferHistoryScope } }) so each variant only contains its relevant fields and
remove the old BufferRecordKind and the mutual fields (or keep a helper enum
only if needed for legacy). Update all uses of BufferRecord, BufferRecordKind,
and field accesses (e.g. places reading command, pid, state, read_only,
helper_source_buffer_id, helper_scope) to pattern-match the new variants and
adjust serialization/derive attributes accordingly (retain BufferId,
BufferRecordState, NodeId, BufferHistoryScope types).
---
Duplicate comments:
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 2648-2655: The test
parse_history_and_node_enums_accept_known_literals only asserts one positive
literal per parser; expand it to cover every accepted literal for
parse_buffer_history_scope, parse_buffer_history_placement,
parse_node_break_destination, and parse_node_join_placement (call each parser
with all valid string variants) and add negative cases that pass invalid strings
to assert Err results so the runtime_error_at/... error paths are exercised;
reuse Position::NONE and explicit invalid literals (e.g., empty string,
"invalid", or misspelled variants) to ensure both success and failure branches
are covered for each parser.
🪄 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: edba3590-992e-41d5-93ee-468cb404cc32
📒 Files selected for processing (75)
crates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/presentation.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/presentation.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-server/src/persist.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/model_state.rscrates/embers-test-support/src/pty.rsdocs/activity-bell-policy.mddocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-deaec7e4.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/action.mddocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/floating-ref.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/tree.mddocs/config-api/ui.md
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/embers-protocol/src/codec.rs (1)
416-429:⚠️ Potential issue | 🟠 MajorValidate zero IDs in the remaining request encoders.
The new encode-time validation only covers
SessionRequest,BufferRequest, andNodeRequest.ClientRequest::Switch,FloatingRequest::{Create,Close,Move,Focus}, andInputRequest::{Send,Resize}still serialize0into fields thatdecode_client_messagenow rejects, so those messages are still not round-trippable. Please mirror the newvalidate_*_requestpattern for these request families and add encode-side regression tests for them.Also applies to: 457-501, 1645-1790
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/src/codec.rs` around lines 416 - 429, The encode path is missing zero-ID validation for several request variants so they still serialize invalid 0 IDs; add encode-time validators mirroring the existing validate_session_request/validate_buffer_request/validate_node_request pattern and call them before serialization in encode_client_request, encode_floating_request, and encode_input_request (specifically ensure ClientRequest::Switch, FloatingRequest::{Create,Close,Move,Focus}, and InputRequest::{Send,Resize} run validation). Implement new functions like validate_client_request, validate_floating_request, validate_input_request that check non-zero IDs for the offending fields and return a ProtocolError if invalid, invoke these from encode_client_message (or inside the specific encode_*_request helpers) prior to building the FlatBuffer, and add regression tests that attempt to encode/decode those variants with ID == 0 to assert encode fails (and valid non-zero IDs round-trip).crates/embers-client/src/configured_client.rs (1)
352-372:⚠️ Potential issue | 🟠 MajorEmit
buffer_bellonly on the transition intoBell.This currently keys off the post-event activity level only. Once a hidden buffer reaches
ActivityState::Bell, any laterRenderInvalidatedfor that buffer will retriggerbuffer_bellhooks even if no new bell arrived, which can spam notifications and duplicate hook side effects.💡 Minimal fix
pub async fn handle_event(&mut self, event: &ServerEvent) -> Result<()> { let detached_session_id = matches!(event, ServerEvent::BufferDetached(_)) .then(|| self.event_session_id(event)) .flatten(); + let was_bell = if let ServerEvent::RenderInvalidated(render) = event { + self.client + .state() + .buffers + .get(&render.buffer_id) + .is_some_and(|buffer| buffer.activity == ActivityState::Bell) + } else { + false + }; self.client.handle_event(event).await?; if let ServerEvent::RenderInvalidated(event) = event { self.client.refresh_buffer_snapshot(event.buffer_id).await?; } @@ if let ServerEvent::RenderInvalidated(render) = event && self .client .state() .buffers .get(&render.buffer_id) - .is_some_and(|buffer| buffer.activity == ActivityState::Bell) + .is_some_and(|buffer| !was_bell && buffer.activity == ActivityState::Bell) { event_names.push("buffer_bell".to_owned()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/configured_client.rs` around lines 352 - 372, The code currently checks the post-event buffer activity (via client.state().buffers) and emits "buffer_bell" for any RenderInvalidated when activity == ActivityState::Bell, causing repeat notifications; to fix, capture the buffer's previous activity before applying the event (call client.state().buffers.get(&render.buffer_id) before client.handle_event in handle_event), then after handling check that previous activity != ActivityState::Bell and current activity == ActivityState::Bell and only then push "buffer_bell" into event_names; reference ServerEvent::RenderInvalidated, handle_event, client.state().buffers, event_names, and ActivityState::Bell when making the change.
♻️ Duplicate comments (2)
crates/embers-server/src/persist.rs (1)
73-83: 🧹 Nitpick | 🔵 TrivialConsolidate derive attributes for consistency.
The split
#[derive(...)]attributes work but are unconventional compared to other enums in this file (e.g.,PersistedBufferState). Consider consolidating for readability.✨ Suggested consolidation
-#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Default)] -#[serde(tag = "kind", rename_all = "snake_case")] +#[derive(Clone, Debug, Default, Serialize, Deserialize, PartialEq, Eq)] +#[serde(tag = "kind", rename_all = "snake_case")] pub enum PersistedBufferKind {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/persist.rs` around lines 73 - 83, The PersistedBufferKind enum currently uses split derive attributes; consolidate them into a single #[derive(...)] containing all traits (Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Default) so the attribute style matches other enums like PersistedBufferState and keep the #[serde(tag = "kind", rename_all = "snake_case")] serde attribute on the enum (PersistedBufferKind) as before.crates/embers-protocol/src/types.rs (1)
180-186:⚠️ Potential issue | 🟠 MajorAvoid two authoritative copies of buffer identity/location here.
When
BufferLocationis embedded inBufferWithLocationResponse, the payload can disagree onbuffer.idvslocation.buffer_idandbuffer.attachment_node_idvslocation.node_id. That leaves clients without a canonical value to trust. Prefer either trimminglocationdown to only the extra context needed in this response, or using a single shared attachment/location payload.Also applies to: 671-675
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/src/types.rs` around lines 180 - 186, BufferLocation currently duplicates authoritative identity fields that also exist on Buffer (causing potential disagreement when embedded in BufferWithLocationResponse); choose a single canonical source and remove duplicates: either (A) shrink BufferLocation to only contextual fields (e.g., drop buffer_id and node_id from BufferLocation and keep session_id/floating_id), or (B) collapse location into a single shared struct used by Buffer and BufferWithLocationResponse so only buffer.id and buffer.attachment_node_id are authoritative. Update the BufferLocation definition and all places referencing it (notably BufferWithLocationResponse and the other occurrence around the 671-675 region) and adjust serialization/constructors and tests to use the chosen canonical fields so clients have one trusted source of truth.
🤖 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 489-505: The response from the server for BufferCommand::Reveal
must be validated to ensure the returned location belongs to the requested
buffer: after calling expect_buffer_location (and similarly in the code path
referenced by expect_buffer_with_location at lines around 1581-1593), compare
location.buffer_id with the BufferId constructed from the requested buffer_id
and return a MuxError::conflict (or appropriate error) if they differ, instead
of proceeding to format_buffer_location_line; this ensures reveal/show never
prints another buffer’s state if the server/codec returned a mismatched buffer
id.
- Around line 525-621: The node-command branches currently treat any non-error
reply as success; change them to validate the server response is an Ok response.
Add a helper like expect_ok(response: ServerResponse, operation: &str) ->
Result<()> and after each
connection.request(ClientMessage::Node(NodeRequest::...)).await? capture the
ServerResponse, then call expect_ok(response, "Node::<operation>") (referencing
operations by NodeCommand::Zoom, NodeCommand::Unzoom, NodeCommand::ToggleZoom,
NodeCommand::Swap -> NodeRequest::SwapSiblings, NodeRequest::BreakNode,
NodeRequest::JoinBufferAtNode, NodeRequest::MoveNodeBefore,
NodeRequest::MoveNodeAfter) so any unexpected ServerResponse is converted into a
protocol error instead of silently succeeding.
In `@crates/embers-cli/tests/interactive.rs`:
- Around line 57-70: Modify try_signal_server to return a boolean (e.g.,
matched_and_signaled: bool) indicating whether it actually found the pid file
and sent a signal; then update kill_orphaned_server to call try_signal_server
and only attempt to remove the pid file when that call returned true and you
have verified the PID no longer resolves to the expected __serve process (or the
pid file was already stale/missing). Concretely: change try_signal_server
signature to return bool, call it twice from kill_orphaned_server (SIGTERM then
SIGKILL), and after a positive result poll/inspect the PID (by checking /proc or
using platform-appropriate process lookup and command name) to confirm the
process is gone before calling fs::remove_file(&pid_path); do not delete the pid
file if try_signal_server returned false.
In `@crates/embers-cli/tests/panes.rs`:
- Around line 523-530: The current assertion only checks for any node with
multiple tabs; change it to specifically verify that the buffer identified by
detached_buffer_id is now attached under a tabs container: after calling
session_snapshot_by_name(&mut connection, "alpha"), iterate snapshot.nodes and
assert there exists a node whose tabs.is_some_and(|tabs|
tabs.tabs.iter().any(|t| t.buffer_id == detached_buffer_id)) so the test fails
when the detached buffer wasn't attached by the node join-buffer operation.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 528-577: Action::FocusBuffer currently switches to a buffer but
does not update current_session_id, causing subsequent focus-relative actions to
target the wrong session; mirror the pattern used in Action::RevealBuffer and
Action::OpenBufferHistory by extracting the session_id from the response via
buffer_location_from_response (as done there), set current_session_id =
Some(session_id), call self.set_active_view(session_id, viewport) if
current_viewport is Some(...), and then call
self.client.resync_all_sessions().await; apply the same fix to the other
FocusBuffer handling block referenced (the second occurrence around the later
lines).
In `@crates/embers-client/src/scripting/documentation.rs`:
- Around line 210-218: The calls to rewrite_generated_defs are applying
signature-literal rewrites unsuitable for Rhai definition files; remove the
rewrite_generated_defs(...) invocations around the generated .rhai outputs
(specifically the calls using registration_defs_path and runtime_defs_path after
build_runtime_docs_engine()/runtime_engine.definitions_with_scope) so generated
definition files are written with plain type names (string/int) instead of
string-literal unions; keep any rewrite_generated_defs usage only on the
Markdown/rendered documentation path (the separate flow around the documentation
generation later in the file).
In `@crates/embers-client/tests/configured_client.rs`:
- Around line 134-149: The helper push_send_input_refresh_responses queues bare
responses on FakeTransport which allows tests that only assert the first
InputRequest::Send to pass even if later requests (e.g., CaptureVisible /
SessionRequest::Get) are wrong; update the helper or callers to be
request-aware: either switch to using ScriptedTransport and register expected
request types in sequence (InputRequest::Send, CaptureVisible,
SessionRequest::Get) with their matching responses inside
push_send_input_refresh_responses, or leave FakeTransport but update the tests
to assert the full transport.requests() sequence after the action so the
CaptureVisible and SessionSnapshot responses are tied to the correct incoming
requests (refer to push_send_input_refresh_responses,
FakeTransport/ScriptedTransport, transport.requests(), InputRequest::Send,
CaptureVisible, SessionSnapshot/SessionRequest::Get).
In `@crates/embers-client/tests/e2e.rs`:
- Around line 986-990: The fullscreen fixture's alternate-screen dwell time is
too short and causes CI races; update the test calls that create the buffer with
fullscreen_fixture_command (e.g., the create_buffer_with_command invocation
using "fullscreen" and the other instance around lines 1053-1060) to use a
longer duration string (for example "3.0" or "4.0" instead of "1.0"/"1.2") so
the server keeps the alternate-screen state long enough for the client
assertions to run reliably.
In `@crates/embers-protocol/tests/family_round_trip.rs`:
- Around line 328-357: Add a detached BufferWithLocation round-trip case: create
another
ServerEnvelope::Response(ServerResponse::BufferWithLocation(BufferWithLocationResponse
{ ... })) entry mirroring the existing BufferWithLocation case but set
location.session_id = None, location.node_id = None, location.floating_id = None
and choose a unique RequestId and appropriate buffers[index].clone() (e.g., next
unused RequestId and a buffer from the buffers array) so the test exercises the
detached path for BufferWithLocation.
In `@crates/embers-server/src/server.rs`:
- Around line 2155-2172: The helper branch currently returns all helper.lines
and sets viewport_top_line to 0; instead compute the viewport height from
buffer.pty_size.rows, calculate the start offset so the viewport contains at
most that many lines from helper.lines (respecting full history by keeping
total_lines equal to helper.lines.len()), slice helper.lines to that range for
the VisibleSnapshotResponse.lines field, and set
VisibleSnapshotResponse.viewport_top_line to the start offset (convert
sizes/indices safely with u64/usize conversions like elsewhere). Ensure
sequence, size, title, cwd, and total_lines semantics remain the same while only
returning the viewport slice.
In `@crates/embers-server/src/state.rs`:
- Around line 1291-1314: The methods zoom_node and toggle_zoom_node currently
set session.zoomed_node before calling refocus_zoomed_subtree, which can leave
the session in a changed state if refocus fails; change both so
refocus_zoomed_subtree(session_id, node_id) is called first and only on Ok(())
assign session_mut(session_id)?.zoomed_node = Some(node_id) (for zoom_node and
the Some branch of toggle_zoom_node); for toggle when next is None keep behavior
of clearing zoomed_node immediately (or leave as-is) but ensure any refocus
attempt happens before persisting the new zoomed_node state so failures do not
persist partial changes.
- Around line 1457-1511: The TabBefore/TabAfter path may call
wrap_node_in_tabs(node_id, ...) which creates a synthetic Tabs node but on later
failure only discard_buffer_view(new_view) is called, leaving the wrapper in
place; update the flow so you detect when wrap_node_in_tabs created a new tabs
container (capture that fact or the returned tabs_id) and on any error path you
remove that synthetic tabs wrapper as part of rollback (after discarding
new_view). Concretely: in the NodeJoinPlacement::TabBefore|TabAfter branch
capture whether tabs_id came from wrap_node_in_tabs (or compare
node_parent(node_id) vs tabs_id), propagate that info out with result, and in
the error handling call the appropriate removal/unwrap routine for that
synthetic tabs node (the counterpart to wrap_node_in_tabs) before returning the
Err from the operation.
In `@docs/activity-bell-policy.md`:
- Around line 17-19: The section title "## Reset on reveal or focus" is broader
than the body which only documents the focused-leaf transition; either change
the heading to "Reset on focus" (or "Reset on focus (focused-leaf)") or add a
sentence describing the reveal-only behavior (e.g., what "reveal" means, whether
activity is cleared, or explicitly that reveal without focus does not reset
activity), and update the paragraph text accordingly so the heading and content
match; modify the markdown heading and the paragraph that mentions "focused
leaf" to reflect the chosen option.
---
Outside diff comments:
In `@crates/embers-client/src/configured_client.rs`:
- Around line 352-372: The code currently checks the post-event buffer activity
(via client.state().buffers) and emits "buffer_bell" for any RenderInvalidated
when activity == ActivityState::Bell, causing repeat notifications; to fix,
capture the buffer's previous activity before applying the event (call
client.state().buffers.get(&render.buffer_id) before client.handle_event in
handle_event), then after handling check that previous activity !=
ActivityState::Bell and current activity == ActivityState::Bell and only then
push "buffer_bell" into event_names; reference ServerEvent::RenderInvalidated,
handle_event, client.state().buffers, event_names, and ActivityState::Bell when
making the change.
In `@crates/embers-protocol/src/codec.rs`:
- Around line 416-429: The encode path is missing zero-ID validation for several
request variants so they still serialize invalid 0 IDs; add encode-time
validators mirroring the existing
validate_session_request/validate_buffer_request/validate_node_request pattern
and call them before serialization in encode_client_request,
encode_floating_request, and encode_input_request (specifically ensure
ClientRequest::Switch, FloatingRequest::{Create,Close,Move,Focus}, and
InputRequest::{Send,Resize} run validation). Implement new functions like
validate_client_request, validate_floating_request, validate_input_request that
check non-zero IDs for the offending fields and return a ProtocolError if
invalid, invoke these from encode_client_message (or inside the specific
encode_*_request helpers) prior to building the FlatBuffer, and add regression
tests that attempt to encode/decode those variants with ID == 0 to assert encode
fails (and valid non-zero IDs round-trip).
---
Duplicate comments:
In `@crates/embers-protocol/src/types.rs`:
- Around line 180-186: BufferLocation currently duplicates authoritative
identity fields that also exist on Buffer (causing potential disagreement when
embedded in BufferWithLocationResponse); choose a single canonical source and
remove duplicates: either (A) shrink BufferLocation to only contextual fields
(e.g., drop buffer_id and node_id from BufferLocation and keep
session_id/floating_id), or (B) collapse location into a single shared struct
used by Buffer and BufferWithLocationResponse so only buffer.id and
buffer.attachment_node_id are authoritative. Update the BufferLocation
definition and all places referencing it (notably BufferWithLocationResponse and
the other occurrence around the 671-675 region) and adjust
serialization/constructors and tests to use the chosen canonical fields so
clients have one trusted source of truth.
In `@crates/embers-server/src/persist.rs`:
- Around line 73-83: The PersistedBufferKind enum currently uses split derive
attributes; consolidate them into a single #[derive(...)] containing all traits
(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Default) so the attribute
style matches other enums like PersistedBufferState and keep the #[serde(tag =
"kind", rename_all = "snake_case")] serde attribute on the enum
(PersistedBufferKind) as before.
🪄 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: 7e033a56-a1f8-443c-a813-6aa26004b35a
📒 Files selected for processing (75)
crates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/presentation.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/presentation.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-server/src/persist.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/model_state.rscrates/embers-test-support/src/pty.rsdocs/activity-bell-policy.mddocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-deaec7e4.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/action.mddocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/floating-ref.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/tree.mddocs/config-api/ui.md
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/embers-server/src/state.rs (1)
688-707:⚠️ Potential issue | 🔴 CriticalRollback the floating insert when initial focus fails.
This method mutates
self.floatingandsession.floatingbefore callingfocus_floating(). If the root subtree has no focusable leaf, it returnsErrwith the floating already persisted. The caller-side cleanup increate_floating_from_buffer_with_options()only discards the buffer view, so the leaked floating/session entry survives.🩹 Minimal rollback
self.floating.insert( floating_id, FloatingWindow { id: floating_id, session_id, @@ }, ); self.session_mut(session_id)?.floating.push(floating_id); - if focus { - self.focus_floating(floating_id)?; + if focus { + if let Err(error) = self.focus_floating(floating_id) { + let _ = self.remove_floating_window(floating_id); + return Err(error); + } } Ok(floating_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/state.rs` around lines 688 - 707, The code inserts a new FloatingWindow into self.floating and pushes the id into session.floating before calling focus_floating(), which can return Err and leak the floating/session entry; change the flow so that if focus_floating(floating_id) returns Err you remove the inserted entry from self.floating and also remove the floating_id from the session's floating Vec (the same session_mut(session_id)?.floating that was pushed), then propagate the error; perform this cleanup only on the focus error path to fully rollback the partially persisted FloatingWindow created by floating_ids.next() / FloatingWindow and avoid leaving stale entries when focus_floating fails.
♻️ Duplicate comments (5)
crates/embers-cli/tests/panes.rs (1)
523-538:⚠️ Potential issue | 🟡 MinorTie the
join-bufferassertion tofirst_pane_id.The session already has a root tabs container, so this still passes if
node join-bufferpromotesdetached_buffer_idto a new window tab instead of tabbing the requested pane. Assert that the tabs container containing the detached buffer also containsfirst_pane_id.🔧 Tighten the assertion
let snapshot = session_snapshot_by_name(&mut connection, "alpha").await; + let joined_node_id = snapshot + .buffers + .iter() + .find(|buffer| buffer.id == detached_buffer_id) + .and_then(|buffer| buffer.attachment_node_id) + .expect("joined buffer is attached"); assert!( snapshot.nodes.iter().any(|node| { node.tabs.as_ref().is_some_and(|tabs| { - tabs.tabs.iter().any(|tab| { - snapshot - .nodes - .iter() - .find(|candidate| candidate.id == tab.child_id) - .and_then(|candidate| candidate.buffer_view.as_ref()) - .is_some_and(|view| view.buffer_id == detached_buffer_id) - }) + let child_ids = tabs.tabs.iter().map(|tab| tab.child_id).collect::<Vec<_>>(); + child_ids.contains(&embers_core::NodeId(first_pane_id)) + && child_ids.contains(&joined_node_id) }) }), - "join-buffer attached the detached buffer under a tabs container" + "join-buffer attached the detached buffer alongside the requested pane" );🤖 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 523 - 538, The current assertion checks only that some tabs container contains the detached_buffer_id; update the assertion in the test (the block using session_snapshot_by_name, snapshot, detached_buffer_id) to also require that the same tabs container contains first_pane_id. Concretely, when iterating snapshot.nodes and drilling into node.tabs -> tabs.tabs -> tab.child_id to find the candidate node with buffer_view.buffer_id == detached_buffer_id, additionally verify that that candidate's parent tabs container (the same tabs.tabs collection) has any tab whose child_id == first_pane_id; adjust the closure logic to check both conditions so the test fails if the detached buffer was promoted to a new window instead of being tabbed with first_pane_id.crates/embers-server/src/server.rs (2)
1980-1986:⚠️ Potential issue | 🟠 Major
RevealandOpenHistorycan still fail after mutating state.
resolve_optional_reveal_client_id()only proves the client existed earlier. Another connection can detach that client, or close the target session, beforeswitch_reveal_client()runs, so these paths can return an error after focus has already moved or after the helper buffer/floating was created. Make the client switch part of the same critical section, or roll back the earlier mutations on failure.Also applies to: 2066-2104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 1980 - 1986, The code calls resolve_optional_reveal_client_id(), then mutates state via focus_revealed_buffer(), then calls switch_reveal_client() which can fail and leave the earlier mutation in-place; make the client switch atomic with the focus or ensure rollback on failure. Either (a) move the switch_reveal_client(client_id, session_id) call inside the same critical section/lock as focus_revealed_buffer(buffer_id) so both succeed or fail together, or (b) if keeping separate calls, implement rollback logic after switch_reveal_client fails to undo what focus_revealed_buffer did (remove created helper buffer/floating, restore previous focus and events) and surface the original error. Apply the same fix to the analogous block handling OpenHistory (lines ~2066-2104) so reveal/open-history state cannot be left inconsistent.
1999-2008:⚠️ Potential issue | 🟠 MajorAvoid cloning the whole helper scrollback for visible-history opens.
state.buffer(source_buffer_id)?.clone()copies every helper line under the global state mutex before theVisiblebranch trims it down. Large helper histories makeopen-history --scope visibleO(total_history) and lengthen lock hold time for unrelated requests. Clone only the tail slice you actually need from&buffer.kind.Also applies to: 2015-2024
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 1999 - 2008, The code currently clones the entire buffer with state.buffer(source_buffer_id)?.clone(), which copies full helper scrollback while holding the state mutex; instead, inside the locked section acquire the minimal pieces (buffer.title.clone(), buffer.cwd.clone(), buffer_location(&state, source_buffer_id)?) and for buffer.kind do not clone the whole kind: inspect &buffer.kind to compute the tail slice needed for the Visible branch (e.g. determine start = len.saturating_sub(visible_limit) and clone only buffer.kind.helper_lines[start..].to_vec() or construct a trimmed Kind variant), then assign that trimmed kind to source_kind and release the lock — this keeps mutex hold time short and avoids copying full helper history unnecessarily (refer to state.lock().await, state.buffer, buffer.kind, source_buffer_id and the Visible branch handling of visible-history).crates/embers-protocol/src/codec.rs (1)
155-166:⚠️ Potential issue | 🟠 MajorOptional IDs that use the wire
0sentinel still bypass validation.
SessionRequest::AddRootTab,BufferRequest::List, andNodeRequest::AddTabcan still carrySome(BufferId(0)),Some(NodeId(0)), orSome(SessionId(0)). Those encode as0and decode back asNone, so an invalid request silently changes meaning instead of failing fast.♻️ Suggested fix
+fn validate_optional_session_id( + session_id: Option<SessionId>, + field: &'static str, +) -> Result<(), ProtocolError> { + if let Some(session_id) = session_id { + validate_required_session_id(session_id, field)?; + } + Ok(()) +} + fn validate_session_request(req: &SessionRequest) -> Result<(), ProtocolError> { match req { SessionRequest::Create { .. } | SessionRequest::List { .. } => Ok(()), SessionRequest::Get { session_id, .. } | SessionRequest::Close { session_id, .. } | SessionRequest::Rename { session_id, .. } - | SessionRequest::AddRootTab { session_id, .. } | SessionRequest::SelectRootTab { session_id, .. } | SessionRequest::RenameRootTab { session_id, .. } | SessionRequest::CloseRootTab { session_id, .. } => { validate_required_session_id(*session_id, "session_request.session_id") } + SessionRequest::AddRootTab { + session_id, + buffer_id, + child_node_id, + .. + } => { + validate_required_session_id(*session_id, "session_request.session_id")?; + validate_optional_buffer_id(*buffer_id, "session_request.buffer_id")?; + validate_optional_node_id(*child_node_id, "session_request.child_node_id") + } } } fn validate_buffer_request(req: &BufferRequest) -> Result<(), ProtocolError> { match req { - BufferRequest::Create { .. } | BufferRequest::List { .. } => Ok(()), + BufferRequest::Create { .. } => Ok(()), + BufferRequest::List { session_id, .. } => { + validate_optional_session_id(*session_id, "buffer_request.session_id") + } BufferRequest::Get { buffer_id, .. } | BufferRequest::Inspect { buffer_id, .. } | BufferRequest::Detach { buffer_id, .. } | BufferRequest::Kill { buffer_id, .. } | BufferRequest::Capture { buffer_id, .. } @@ - NodeRequest::AddTab { tabs_node_id, .. } | NodeRequest::SelectTab { tabs_node_id, .. } => { + NodeRequest::AddTab { + tabs_node_id, + buffer_id, + child_node_id, + .. + } => { validate_required_node_id(*tabs_node_id, "node_request.tabs_node_id") + ?; + validate_optional_buffer_id(*buffer_id, "node_request.buffer_id")?; + validate_optional_node_id(*child_node_id, "node_request.child_node_id") + } + NodeRequest::SelectTab { tabs_node_id, .. } => { + validate_required_node_id(*tabs_node_id, "node_request.tabs_node_id") }Also applies to: 170-185, 188-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/src/codec.rs` around lines 155 - 166, Some request variants silently accept Some(Id(0)) which encodes/decodes as 0→None and changes meaning; update validation to reject explicit sentinel IDs. In validate_session_request add checks for variants that carry Option<SessionId> (e.g. SessionRequest::AddRootTab, SelectRootTab, RenameRootTab, CloseRootTab) and return ProtocolError if the Option is Some(session_id) and session_id is the sentinel zero; likewise add analogous checks in the BufferRequest and NodeRequest validators (e.g. BufferRequest::List, NodeRequest::AddTab) to reject Some(BufferId(0)) and Some(NodeId(0)). Use the existing helper validators (or add similar helpers like validate_non_sentinel_optional_session_id / validate_non_sentinel_optional_buffer_id / validate_non_sentinel_optional_node_id) and call them from the respective match arms so any explicit Some(Id(0)) fails rather than being treated as None.crates/embers-client/src/configured_client.rs (1)
639-818: 🧹 Nitpick | 🔵 TrivialConsolidate the explicit-id action handlers.
execute_without_presentation()now consumes the explicit forms ofCloseFloating,CloseView,KillBuffer,DetachBuffer,MoveBufferToNode,ToggleZoomNode, sibling reorders, and similar actions, so the matchingexecute_action()arms are effectively dead. Keeping both paths alive is going to drift again.Also applies to: 1050-1337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/configured_client.rs` around lines 639 - 818, execute_without_presentation currently matches the explicit-id variants (e.g., Action::CloseFloating, CloseView, KillBuffer, DetachBuffer, MoveBufferToNode, ToggleZoomNode, SwapSiblingNodes, MoveNodeBefore, MoveNodeAfter, BreakNode, JoinBufferAtNode, ZoomNode, UnzoomNode) which duplicates logic already handled by execute_action; remove the duplicated explicit arms and consolidate by either delegating to execute_action for these variants or by replacing them with the generic passthrough arm that forwards the Action to the same central handler, ensuring only one canonical place implements the request_message + resync_all_sessions workflow (refer to execute_without_presentation and execute_action to pick the single implementation to keep).
🤖 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 1621-1633: ensure_matching_buffer_id currently returns
MuxError::conflict for a server/codec integrity failure; change it to return the
protocol-level error variant (e.g., MuxError::protocol or the crate's
protocol/codec error constructor) so response ID mismatches are classified as
protocol errors, and keep the existing descriptive message with the operation,
requested_buffer_id and actual_buffer_id to aid diagnostics.
- Around line 521-531: The branch handling the server response to
BufferRequest::OpenHistory should not treat a detached location as success:
after calling expect_buffer_location (the variable `location` returned from the
request to OpenHistory via connection.request and BufferRequest::OpenHistory),
validate that the returned location is not a detached location (e.g., match
against the Location/BufferLocation enum variant for Detached); if it is
detached, return an Err (propagate a failure) instead of calling
format_buffer_location_line and returning Ok; otherwise proceed to
format_buffer_location_line(&location) as before. Ensure you reference the
existing expect_buffer_location and format_buffer_location_line symbols and
return an appropriate error when detecting a detached placement.
In `@crates/embers-cli/tests/interactive.rs`:
- Around line 271-298: split_child_order currently panics when it doesn't find a
split containing both panes; change split_child_order to return Option<[u64; 2]>
(i.e., return None instead of unwrap_or_else panic) so callers can poll without
aborting, then update the initial call in
live_pty_client_preserves_buffers_across_layout_and_attachment_changes to unwrap
the Option once before entering the polling path and have
wait_for_split_child_order (and the other call site around the second hunk) use
the new Option-returning split_child_order to retry until the deadline instead
of panicking.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 538-559: The FocusBuffer branch currently ignores the
server-returned location.node_id and calls focus_buffer which re-reads
attachment_node_id from local state; change it to use the authoritative node
from buffer_location_from_response (i.e., pass location.node_id into
focus_buffer or an equivalent method) instead of relying on self.client.state(),
or alternatively call self.client.resync_all_sessions().await before
focus_buffer so local state reflects the server location; update the
Action::FocusBuffer handling to use the returned location.node_id (from
buffer_location_from_response) when focusing the buffer to avoid races.
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 2481-2487: The parse helpers (e.g., parse_buffer_history_scope,
and the similar functions around it) currently return generic errors like
"invalid scope"/"invalid placement" that drop the offending literal; update each
to include the rejected value in the runtime_error_at message so errors are
actionable—e.g., construct the error text with the value (such as
format!("invalid scope: {}", value)) and pass that into
runtime_error_at(position) so runtime_error_at receives the descriptive message;
apply the same change to the other literal-parsing helpers mentioned (the
functions spanning the other ranges) so they all surface the bad literal.
In `@crates/embers-protocol/tests/family_round_trip.rs`:
- Around line 217-221: The test currently only round-trips ClientMessage::Node
with NodeRequest::BreakNode using NodeBreakDestination::Floating and
NodeJoinPlacement::TabAfter; add additional test fixtures in the same
family_round_trip suite to exercise the other enum variants (e.g.,
NodeBreakDestination::Inline/Other variants and all NodeJoinPlacement variants
like Tab, TabBefore, TabAfter, and any NonTab variants) so each arm of the wire
families is covered; create additional ClientMessage::Node entries mirroring the
existing structure but swapping destination and join_placement values to include
every variant and ensure round-trip coverage for NodeRequest::BreakNode and
NodeRequest::JoinNode paths referenced in the file.
In `@crates/embers-server/src/persist.rs`:
- Around line 284-286: During the legacy/v1 migration branch
(LEGACY_FORMAT_VERSION | FIRST_VERSIONED_FORMAT_VERSION) you only set
workspace.format_version = Some(CURRENT_FORMAT_VERSION) which leaves
PersistedSession.zoomed_node as None even though
PersistedNode::BufferView.zoomed existed in the old schema; update the migration
logic to iterate sessions and backfill each PersistedSession.zoomed_node by
locating the node id in workspace.nodes whose BufferView.zoomed == true (or the
first matching node) and assign that id to the session’s zoomed_node before
returning workspace, ensuring zoom state is preserved after migration.
In `@crates/embers-server/src/server.rs`:
- Around line 1595-1609: The BreakNode branch
(embers_protocol::NodeRequest::BreakNode) can create a floating window but
currently returns layout_ok_response(&state, request_id, session_id) which only
emits NodeChanged and FocusChanged; update the success path so that when
destination is embers_protocol::NodeBreakDestination::Floating you also emit a
FloatingChanged event for that session. Either extend layout_ok_response to
accept a flag (or extra response) to include FloatingChanged, or add a separate
call that constructs and returns the FloatingChanged response alongside the
existing NodeChanged/FocusChanged responses after state.break_node(...)
succeeds.
In `@crates/embers-server/tests/model_state.rs`:
- Around line 350-365: The test currently only checks that focus changed after
calling zoom_node(leaf_id); add an assertion that the session's zoom target was
persisted by verifying session.zoomed_node equals Some(leaf_id). After calling
state.zoom_node(leaf_id) and before validate(), fetch the session via
state.session(session_id).expect("session") and assert that its zoomed_node
field is Some(leaf_id) to ensure zoom_node actually sets the
session.zoomed_node.
---
Outside diff comments:
In `@crates/embers-server/src/state.rs`:
- Around line 688-707: The code inserts a new FloatingWindow into self.floating
and pushes the id into session.floating before calling focus_floating(), which
can return Err and leak the floating/session entry; change the flow so that if
focus_floating(floating_id) returns Err you remove the inserted entry from
self.floating and also remove the floating_id from the session's floating Vec
(the same session_mut(session_id)?.floating that was pushed), then propagate the
error; perform this cleanup only on the focus error path to fully rollback the
partially persisted FloatingWindow created by floating_ids.next() /
FloatingWindow and avoid leaving stale entries when focus_floating fails.
---
Duplicate comments:
In `@crates/embers-cli/tests/panes.rs`:
- Around line 523-538: The current assertion checks only that some tabs
container contains the detached_buffer_id; update the assertion in the test (the
block using session_snapshot_by_name, snapshot, detached_buffer_id) to also
require that the same tabs container contains first_pane_id. Concretely, when
iterating snapshot.nodes and drilling into node.tabs -> tabs.tabs ->
tab.child_id to find the candidate node with buffer_view.buffer_id ==
detached_buffer_id, additionally verify that that candidate's parent tabs
container (the same tabs.tabs collection) has any tab whose child_id ==
first_pane_id; adjust the closure logic to check both conditions so the test
fails if the detached buffer was promoted to a new window instead of being
tabbed with first_pane_id.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 639-818: execute_without_presentation currently matches the
explicit-id variants (e.g., Action::CloseFloating, CloseView, KillBuffer,
DetachBuffer, MoveBufferToNode, ToggleZoomNode, SwapSiblingNodes,
MoveNodeBefore, MoveNodeAfter, BreakNode, JoinBufferAtNode, ZoomNode,
UnzoomNode) which duplicates logic already handled by execute_action; remove the
duplicated explicit arms and consolidate by either delegating to execute_action
for these variants or by replacing them with the generic passthrough arm that
forwards the Action to the same central handler, ensuring only one canonical
place implements the request_message + resync_all_sessions workflow (refer to
execute_without_presentation and execute_action to pick the single
implementation to keep).
In `@crates/embers-protocol/src/codec.rs`:
- Around line 155-166: Some request variants silently accept Some(Id(0)) which
encodes/decodes as 0→None and changes meaning; update validation to reject
explicit sentinel IDs. In validate_session_request add checks for variants that
carry Option<SessionId> (e.g. SessionRequest::AddRootTab, SelectRootTab,
RenameRootTab, CloseRootTab) and return ProtocolError if the Option is
Some(session_id) and session_id is the sentinel zero; likewise add analogous
checks in the BufferRequest and NodeRequest validators (e.g.
BufferRequest::List, NodeRequest::AddTab) to reject Some(BufferId(0)) and
Some(NodeId(0)). Use the existing helper validators (or add similar helpers like
validate_non_sentinel_optional_session_id /
validate_non_sentinel_optional_buffer_id /
validate_non_sentinel_optional_node_id) and call them from the respective match
arms so any explicit Some(Id(0)) fails rather than being treated as None.
In `@crates/embers-server/src/server.rs`:
- Around line 1980-1986: The code calls resolve_optional_reveal_client_id(),
then mutates state via focus_revealed_buffer(), then calls
switch_reveal_client() which can fail and leave the earlier mutation in-place;
make the client switch atomic with the focus or ensure rollback on failure.
Either (a) move the switch_reveal_client(client_id, session_id) call inside the
same critical section/lock as focus_revealed_buffer(buffer_id) so both succeed
or fail together, or (b) if keeping separate calls, implement rollback logic
after switch_reveal_client fails to undo what focus_revealed_buffer did (remove
created helper buffer/floating, restore previous focus and events) and surface
the original error. Apply the same fix to the analogous block handling
OpenHistory (lines ~2066-2104) so reveal/open-history state cannot be left
inconsistent.
- Around line 1999-2008: The code currently clones the entire buffer with
state.buffer(source_buffer_id)?.clone(), which copies full helper scrollback
while holding the state mutex; instead, inside the locked section acquire the
minimal pieces (buffer.title.clone(), buffer.cwd.clone(),
buffer_location(&state, source_buffer_id)?) and for buffer.kind do not clone the
whole kind: inspect &buffer.kind to compute the tail slice needed for the
Visible branch (e.g. determine start = len.saturating_sub(visible_limit) and
clone only buffer.kind.helper_lines[start..].to_vec() or construct a trimmed
Kind variant), then assign that trimmed kind to source_kind and release the lock
— this keeps mutex hold time short and avoids copying full helper history
unnecessarily (refer to state.lock().await, state.buffer, buffer.kind,
source_buffer_id and the Visible branch handling of visible-history).
🪄 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: 36a86a65-93dd-4731-aaf4-6c72fa467b90
📒 Files selected for processing (75)
crates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/presentation.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/presentation.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-server/src/persist.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/model_state.rscrates/embers-test-support/src/pty.rsdocs/activity-bell-policy.mddocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-deaec7e4.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/action.mddocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/floating-ref.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/tree.mddocs/config-api/ui.md
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/embers-test-support/src/pty.rs (2)
205-212:⚠️ Potential issue | 🟡 MinorAdd
wait()afterkill()to reap child process.The
Dropimpl callskill()but notwait(). Perportable_ptybehavior,kill()terminates the child but doesn't reap it—the process remains a zombie untilwait()is called. In test suites spawning many PTY harnesses, this can accumulate zombie processes until the test process exits.🛡️ Suggested fix
impl Drop for PtyHarness { fn drop(&mut self) { let _ = self.child.kill(); + let _ = self.child.wait(); if let Some(join) = self.reader_join.take() { let _ = join.join(); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-test-support/src/pty.rs` around lines 205 - 212, The Drop implementation for PtyHarness currently calls self.child.kill() but never reaps the child; update PtyHarness::drop to call self.child.wait() after kill() (e.g., let _ = self.child.kill(); let _ = self.child.wait();) so the terminated process is reaped; keep both calls' results ignored or logged as appropriate and preserve the existing reader_join.take() join behavior.
200-200: 🧹 Nitpick | 🔵 TrivialRemove dead code.
let _ = &self.master;is a no-op—it creates an immutable reference and immediately discards it. If the intent was to ensuremasteroutlives some operation, it's unnecessary sinceselfholds ownership. If it was meant to suppress an unused-field warning, consider adding#[allow(dead_code)]to the field instead.♻️ Suggested removal
let _ = join.join(); } - let _ = &self.master; Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-test-support/src/pty.rs` at line 200, The statement `let _ = &self.master;` is a no-op and should be removed; delete that line from the function to eliminate dead code, or if the aim was to silence an unused-field warning for the `master` field on the struct, instead add `#[allow(dead_code)]` to the `master` field declaration (or use the field in a meaningful way) so the compiler warning is handled correctly.crates/embers-protocol/tests/family_round_trip.rs (1)
488-498: 🧹 Nitpick | 🔵 TrivialCover the non-zoomed
SessionRecordpath too.Every
SessionRecordin this suite now serializeszoomed_node_id: Some(NodeId(24)). Add one session or snapshot fixture withzoomed_node_id: Noneso the default non-zoomed wire shape is round-tripped as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/tests/family_round_trip.rs` around lines 488 - 498, Add a test fixture that exercises the non-zoomed path by creating a SessionSnapshot / SessionRecord where zoomed_node_id is None (e.g., duplicate or new helper similar to sample_snapshot but set zoomed_node_id: None) so the wire format without a zoom field is round-tripped; update any relevant tests in family_round_trip.rs to use this new snapshot (referencing sample_snapshot, SessionSnapshot, SessionRecord, and zoomed_node_id) to ensure both Some(NodeId(...)) and None cases are covered.crates/embers-protocol/src/codec.rs (1)
1051-1099: 🧹 Nitpick | 🔵 TrivialReplace the positional
EncodedNodeRequesttuple with named fields.At this size, adding
break_destination,join_placement, and the sibling-id slots makes the tuple easy to mis-wire silently. A small struct or directfb::NodeRequestArgsconstruction per match arm would be much safer to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/src/codec.rs` around lines 1051 - 1099, The large positional EncodedNodeRequest tuple is error-prone; replace it with a named-struct-style binding (or directly construct fb::NodeRequestArgs per match arm) so fields like break_destination, join_placement, sibling_node_id, etc. are explicit rather than positional; update the match req arms to destructure into the new struct or to call fb::NodeRequestArgs { ... } with explicit field names and adjust downstream uses (op, session_id, title_str, sizes_vec, child_node_ids_vec, titles_vec, insert_before, first_node_id, second_node_id, sibling_node_id) accordingly to remove silent mis-wiring.
♻️ Duplicate comments (4)
crates/embers-server/src/persist.rs (1)
284-297:⚠️ Potential issue | 🟠 MajorReject ambiguous legacy zoom-state migrations.
Line 287 uses
find_map, so a malformed v0/v1 workspace with twozoomed: truebuffer views in the same session is restored arbitrarily based on node order. That makes migration nondeterministic and hides real data corruption.🛠 Minimal fix direction
LEGACY_FORMAT_VERSION | FIRST_VERSIONED_FORMAT_VERSION => { - for session in &mut workspace.sessions { - if session.zoomed_node.is_none() { - session.zoomed_node = workspace.nodes.iter().find_map(|node| match node { - PersistedNode::BufferView { - id, - session_id, - zoomed: true, - .. - } if *session_id == session.id => Some(*id), - _ => None, - }); - } - } + let mut zoomed_by_session = BTreeMap::new(); + for node in &workspace.nodes { + if let PersistedNode::BufferView { + id, + session_id, + zoomed: true, + .. + } = node + { + if zoomed_by_session.insert(*session_id, *id).is_some() { + return Err(MuxError::internal(format!( + "multiple zoomed nodes found for session {session_id} in migrated workspace" + ))); + } + } + } + for session in &mut workspace.sessions { + if session.zoomed_node.is_none() { + session.zoomed_node = zoomed_by_session.get(&session.id).copied(); + } + } workspace.format_version = Some(CURRENT_FORMAT_VERSION); Ok(workspace) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/persist.rs` around lines 284 - 297, The migration currently uses find_map over workspace.nodes which silently picks one of multiple BufferView nodes with zoomed: true per session; update the loop that sets session.zoomed_node to first collect all matching nodes by iterating workspace.nodes and filtering PersistedNode::BufferView where zoomed: true and session_id == session.id (using the same pattern match as in the diff), then if the filtered matches.len() == 1 set session.zoomed_node to that id, if matches.is_empty() leave None, and if matches.len() > 1 return an Err (or propagate a MigrationError) with context about the duplicate zoomed nodes for that session so the migration fails deterministically rather than picking an arbitrary node; reference symbols: workspace.sessions, session.zoomed_node, PersistedNode::BufferView, zoomed, session_id.crates/embers-client/src/scripting/runtime.rs (1)
2660-2714: 🧹 Nitpick | 🔵 TrivialThese tests still stop at the helper parsers.
They verify literal parsing, but they never invoke the exported Rhai builders. A regression in
open_buffer_history,break_current_node, orjoin_buffer_herewiring those parsed values into the wrongActionwould still pass here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/scripting/runtime.rs` around lines 2660 - 2714, The test only exercises low-level parsers (parse_buffer_history_scope, parse_buffer_history_placement, parse_node_break_destination, parse_node_join_placement) but not the exported Rhai builders that produce Actions; add assertions that call the exported builder functions (open_buffer_history, break_current_node, join_buffer_here or whatever public builder names are in scope) with the same literal inputs and validate the produced Action enum/struct fields (e.g., variant and embedded scope/placement/destination values) to ensure the parsed values are correctly wired into the resulting Actions; update the test parse_history_and_node_enums_accept_known_literals to invoke those builders for both valid and invalid literals and assert Ok(Action) or Err messages match expected strings to catch regressions in the builder wiring.crates/embers-protocol/src/types.rs (1)
180-186: 🛠️ Refactor suggestion | 🟠 MajorKeep attachment data single-sourced in
BufferWithLocationResponse.
buffer.id/buffer.attachment_node_idandlocation.buffer_id/location.node_idcan diverge on the wire, and clients have no canonical value when they do. Prefer shrinkinglocationhere to session/floating context only, or replacing both with a single attachment model.Also applies to: 670-675
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/src/types.rs` around lines 180 - 186, BufferLocation currently contains buffer_id and node_id which can diverge from the buffer's own attachment fields on the wire; update the model so BufferLocation only carries session/floating context (remove buffer_id and node_id) and ensure BufferWithLocationResponse uses the buffer's canonical attachment fields (e.g., buffer.id and buffer.attachment_node_id) instead of duplicating them, or alternatively introduce a single Attachment model used by both the Buffer struct and the location wrapper; update all references to BufferLocation and BufferWithLocationResponse (and any serializers/deserializers) to match the new shape so clients have one canonical source for attachment info.crates/embers-client/src/configured_client.rs (1)
538-559:⚠️ Potential issue | 🟠 MajorDon't preselect tabs for
FocusBufferfrom cached client state.
GetLocationgives you the authoritativenode_id, butfocus_buffer()still walksself.client.state().nodesand emitsSelectTabrequests before the finalFocus. If local layout is stale, those tab selections can target parents that no longer ownnode_idand make the action fail after a successful lookup.NodeRequest::Focusalready activates ancestor tabs on the server, so focusing the returned node directly is safer.💡 Minimal fix
async fn focus_buffer(&mut self, session_id: SessionId, node_id: NodeId) -> Result<()> { - let mut selections = Vec::new(); - let mut child_id = node_id; - let mut parent_id = self - .client - .state() - .nodes - .get(&node_id) - .and_then(|node| node.parent_id); - while let Some(current_parent) = parent_id { - if let Some(tabs) = self - .client - .state() - .nodes - .get(¤t_parent) - .and_then(|node| node.tabs.as_ref()) - && let Some(index) = tabs.tabs.iter().position(|tab| tab.child_id == child_id) - { - selections.push((current_parent, index)); - } - child_id = current_parent; - parent_id = self - .client - .state() - .nodes - .get(¤t_parent) - .and_then(|node| node.parent_id); - } - selections.reverse(); - - for (tabs_node_id, index) in selections { - self.client - .request_message(ClientMessage::Node(NodeRequest::SelectTab { - request_id: self.client.next_request_id(), - tabs_node_id, - index: u32::try_from(index).map_err(|_| { - MuxError::invalid_input("tab index exceeds protocol limits") - })?, - })) - .await?; - } self.focus_node(session_id, node_id).await }Also applies to: 1658-1699
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/configured_client.rs` around lines 538 - 559, The FocusBuffer handler should not preselect ancestor tabs from cached state before issuing the final focus; instead use the authoritative node_id from buffer_location_from_response and issue a direct node focus so server-side NodeRequest::Focus will activate ancestors. Update the Action::FocusBuffer branch to call a variant of focus_buffer (or add a skip-preselect flag) that takes the returned session_id and node_id and avoids walking self.client.state().nodes and emitting SelectTab requests (remove/skip SelectTab emission there), or replace that call with a direct ClientMessage::Node(NodeRequest::Focus{ node_id, .. }) request so the server performs ancestor tab activation.
🤖 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 2229-2263: Add unit tests exercising the CLI parser to cover the
new buffer/node subcommands and flags to detect clap wiring regressions; in the
same test module near buffer_details_location_row_uses_single_tab_delimiter add
tests that call Cli::try_parse_from with representative invocations for the
"buffer" and "node" commands (including flag combinations like --as, --to,
--tab-before, --tab-after and default-enum cases) and assert Ok results or
expected error variants so parser drift is caught early. Ensure you reference
the Cli type and the exact flag names used in the diff when creating these
cases.
- Around line 1713-1715: Replace the space-joined serialization that loses argv
boundaries: instead of lines.push(format!("command\t{}", buffer.command.join("
"))), serialize the argument vector so boundaries are preserved (e.g. use
serde_json::to_string(&buffer.command) or another lossless encoding) and write
lines.push(format!("command\t{}", serialized_args)); ensure the matching
parser/consumer expects the chosen encoding (update parsing of the "command" row
accordingly).
In `@crates/embers-cli/tests/panes.rs`:
- Around line 512-540: The current assertion only checks that the detached
buffer and first pane are co-members of the same tabs container; update the
check to assert their relative ordering: find the matched tabs node (the same
code that locates tabs.tabs), compute the index of the tab whose child_id
corresponds to detached_buffer_id (by resolving candidate.buffer_view.buffer_id)
and the index of the tab whose child_id.0 == first_pane_id, and assert that
detached_index == first_index + 1 to ensure "--as tab-after" placed it
immediately after the first pane. Use the existing snapshot, tabs.tabs,
tab.child_id, and buffer_view.buffer_id accessors to locate indices and fail the
test if ordering is wrong.
In `@crates/embers-client/src/presentation.rs`:
- Around line 98-116: The fallback branch that handles a zoomed node uses
floating_ancestor_for_node(…) and always builds and projects a FloatingFrame
(via projector.projection.floating.push and projector.project_node), which can
re-expose hidden popups; modify that branch to match the normal floating path by
checking the ancestor window's visibility (window.visible) and only push/project
when window.visible is true (i.e., skip the block when !window.visible) so
hidden floatings are not rebuilt or targeted by focused_floating_id().
In `@crates/embers-client/tests/e2e.rs`:
- Around line 155-180: The polling loop in wait_for_visible_snapshot can hang if
capture_visible_buffer() stalls because the deadline is only checked between
iterations; wrap the per-iteration await in a hard timeout using
tokio::time::timeout with the remaining duration (deadline - Instant::now()) so
each call to connection.capture_visible_buffer(buffer_id).await is bounded, and
if the per-iteration timeout elapses treat it as a test timeout (panic or return
error) with the same diagnostic message; apply the same change to the other
similar helper (the one around lines 197-219) so BufferRequest::Get / capture
calls are always bounded by the overall timeout.
In `@crates/embers-protocol/src/codec.rs`:
- Around line 1946-1952: The preflight currently only checks for zero IDs for
ServerEnvelope::Response(ServerResponse::BufferWithLocation(response)) but
doesn't ensure response.buffer.id matches response.location.buffer_id; update
the checks in the encoder/validation logic (the branch handling
BufferWithLocationResponse / ServerResponse::BufferWithLocation) to also reject
when response.buffer.id.0 != response.location.buffer_id.0 (return a
ProtocolError::InvalidMessageOwned with a clear message like
"buffer_with_location_response.buffer.id must equal location.buffer_id"), and
apply the same consistency check in the repeated block around the other
occurrence (the similar check at the 2099–2108 area) so encoding never
serializes mismatched buffer id and location.buffer_id.
In `@crates/embers-protocol/tests/family_round_trip.rs`:
- Around line 375-386: The BufferWithLocation fixture is internally
inconsistent: it uses buffers[2] (which has attachment_node_id: Some(NodeId(24))
) for the buffer field while the BufferLocation is detached; update the
BufferWithLocationResponse so the buffer is a detached BufferRecord
(attachment_node_id = None) instead of buffers[2].clone(); e.g., construct or
clone a BufferRecord for the buffer field with attachment_node_id set to None so
the wire path for detached BufferRecord.attachment_node_id is exercised (target
symbols: ServerEnvelope::Response, ServerResponse::BufferWithLocation,
BufferWithLocationResponse, buffers[2], BufferRecord.attachment_node_id).
In `@crates/embers-server/src/persist.rs`:
- Around line 582-620: Add a unit test that round-trips a Helper buffer through
persisted_buffer_kind -> restored_buffer_kind to ensure the Helper branch is
exercised and its fields don't regress: construct a
BufferKind::Helper(HelperBuffer { source_buffer_id: BufferId(x), scope:
HelperBufferScope::Full (and also a case for Visible), lines: vec![...]}), call
persisted_buffer_kind on it, then call restored_buffer_kind on the result and
assert the final BufferKind equals the original (and similarly assert
persisted_helper_scope/restored_helper_scope preserve scope). Place the test
near existing persist tests so it fails if source_buffer_id, scope, or lines are
changed.
In `@crates/embers-server/src/state.rs`:
- Around line 1393-1405: Add an early idempotency guard in break_node: if
to_floating is true and the node is already the root of a floating window,
return Ok(()) instead of calling detach_node_from_owner() and recreating the
window. Detect this by checking node_parent(node_id)? is None (or whatever
indicates root) and matching the node kind via self.node(node_id)? (e.g.,
matches!(..., Node::FloatingRoot) or the equivalent floating/window enum variant
used in this codebase). Apply the same guard at the other occurrence (around
lines 2122-2124) so break_node(..., true) becomes a no-op for nodes that already
own a floating window, preserving user-adjusted geometry/title.
In `@crates/embers-server/tests/model_state.rs`:
- Around line 726-740: After the failed floating creation, add assertions that
the caller-owned node empty_tabs still exists and is parentless: verify
state.node(empty_tabs) returns Some (node is present) and that
state.parent_of(empty_tabs) (or the equivalent API that returns a node's parent)
is None; keep the existing check floating_id_for_node(empty_tabs) == None to
ensure no floating entry was created. This ensures rollback didn't drop
empty_tabs but detached it back to a parentless state.
---
Outside diff comments:
In `@crates/embers-protocol/src/codec.rs`:
- Around line 1051-1099: The large positional EncodedNodeRequest tuple is
error-prone; replace it with a named-struct-style binding (or directly construct
fb::NodeRequestArgs per match arm) so fields like break_destination,
join_placement, sibling_node_id, etc. are explicit rather than positional;
update the match req arms to destructure into the new struct or to call
fb::NodeRequestArgs { ... } with explicit field names and adjust downstream uses
(op, session_id, title_str, sizes_vec, child_node_ids_vec, titles_vec,
insert_before, first_node_id, second_node_id, sibling_node_id) accordingly to
remove silent mis-wiring.
In `@crates/embers-protocol/tests/family_round_trip.rs`:
- Around line 488-498: Add a test fixture that exercises the non-zoomed path by
creating a SessionSnapshot / SessionRecord where zoomed_node_id is None (e.g.,
duplicate or new helper similar to sample_snapshot but set zoomed_node_id: None)
so the wire format without a zoom field is round-tripped; update any relevant
tests in family_round_trip.rs to use this new snapshot (referencing
sample_snapshot, SessionSnapshot, SessionRecord, and zoomed_node_id) to ensure
both Some(NodeId(...)) and None cases are covered.
In `@crates/embers-test-support/src/pty.rs`:
- Around line 205-212: The Drop implementation for PtyHarness currently calls
self.child.kill() but never reaps the child; update PtyHarness::drop to call
self.child.wait() after kill() (e.g., let _ = self.child.kill(); let _ =
self.child.wait();) so the terminated process is reaped; keep both calls'
results ignored or logged as appropriate and preserve the existing
reader_join.take() join behavior.
- Line 200: The statement `let _ = &self.master;` is a no-op and should be
removed; delete that line from the function to eliminate dead code, or if the
aim was to silence an unused-field warning for the `master` field on the struct,
instead add `#[allow(dead_code)]` to the `master` field declaration (or use the
field in a meaningful way) so the compiler warning is handled correctly.
---
Duplicate comments:
In `@crates/embers-client/src/configured_client.rs`:
- Around line 538-559: The FocusBuffer handler should not preselect ancestor
tabs from cached state before issuing the final focus; instead use the
authoritative node_id from buffer_location_from_response and issue a direct node
focus so server-side NodeRequest::Focus will activate ancestors. Update the
Action::FocusBuffer branch to call a variant of focus_buffer (or add a
skip-preselect flag) that takes the returned session_id and node_id and avoids
walking self.client.state().nodes and emitting SelectTab requests (remove/skip
SelectTab emission there), or replace that call with a direct
ClientMessage::Node(NodeRequest::Focus{ node_id, .. }) request so the server
performs ancestor tab activation.
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 2660-2714: The test only exercises low-level parsers
(parse_buffer_history_scope, parse_buffer_history_placement,
parse_node_break_destination, parse_node_join_placement) but not the exported
Rhai builders that produce Actions; add assertions that call the exported
builder functions (open_buffer_history, break_current_node, join_buffer_here or
whatever public builder names are in scope) with the same literal inputs and
validate the produced Action enum/struct fields (e.g., variant and embedded
scope/placement/destination values) to ensure the parsed values are correctly
wired into the resulting Actions; update the test
parse_history_and_node_enums_accept_known_literals to invoke those builders for
both valid and invalid literals and assert Ok(Action) or Err messages match
expected strings to catch regressions in the builder wiring.
In `@crates/embers-protocol/src/types.rs`:
- Around line 180-186: BufferLocation currently contains buffer_id and node_id
which can diverge from the buffer's own attachment fields on the wire; update
the model so BufferLocation only carries session/floating context (remove
buffer_id and node_id) and ensure BufferWithLocationResponse uses the buffer's
canonical attachment fields (e.g., buffer.id and buffer.attachment_node_id)
instead of duplicating them, or alternatively introduce a single Attachment
model used by both the Buffer struct and the location wrapper; update all
references to BufferLocation and BufferWithLocationResponse (and any
serializers/deserializers) to match the new shape so clients have one canonical
source for attachment info.
In `@crates/embers-server/src/persist.rs`:
- Around line 284-297: The migration currently uses find_map over
workspace.nodes which silently picks one of multiple BufferView nodes with
zoomed: true per session; update the loop that sets session.zoomed_node to first
collect all matching nodes by iterating workspace.nodes and filtering
PersistedNode::BufferView where zoomed: true and session_id == session.id (using
the same pattern match as in the diff), then if the filtered matches.len() == 1
set session.zoomed_node to that id, if matches.is_empty() leave None, and if
matches.len() > 1 return an Err (or propagate a MigrationError) with context
about the duplicate zoomed nodes for that session so the migration fails
deterministically rather than picking an arbitrary node; reference symbols:
workspace.sessions, session.zoomed_node, PersistedNode::BufferView, zoomed,
session_id.
🪄 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: 9fbc3c66-52ed-4411-af0d-e04b7feb87d7
📒 Files selected for processing (75)
crates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/presentation.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/presentation.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-server/src/persist.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/model_state.rscrates/embers-test-support/src/pty.rsdocs/activity-bell-policy.mddocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-deaec7e4.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/action.mddocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/floating-ref.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/tree.mddocs/config-api/ui.md
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
crates/embers-server/tests/model_state.rs (1)
765-781:⚠️ Potential issue | 🟡 MinorStrengthen the rollback assertion on
empty_tabs.
state.node(empty_tabs).is_ok()still passes if the rollback mutates the caller-owned node or leaves broader invariants broken. Assert thatempty_tabsis still aNode::Tabs(_)and finish withstate.validate()so this test fails on structural rollback corruption.🧪 Suggested tightening
- assert!( - state.node(empty_tabs).is_ok(), - "empty tabs node should remain present" - ); + assert!(matches!( + state.node(empty_tabs).expect("empty tabs still exists"), + Node::Tabs(_) + )); assert_eq!( state .node_parent(empty_tabs) .expect("empty tabs parent lookup"), None @@ assert_eq!( state .floating_id_for_node(empty_tabs) .expect("floating lookup"), None ); + state.validate().expect("state remains valid");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/tests/model_state.rs` around lines 765 - 781, The current assertions only check presence of empty_tabs; instead assert its variant is Node::Tabs by pattern-checking state.node(empty_tabs) result to ensure it returns a Node::Tabs(_) value, then keep the existing parent and floating_id_for_node checks and finally call state.validate() to trigger a failure if rollback corrupted broader invariants; reference the node(empty_tabs) call, Node::Tabs variant, floating_id_for_node(empty_tabs), node_parent(empty_tabs), and state.validate() when making these changes.crates/embers-protocol/src/codec.rs (2)
2561-2561:⚠️ Potential issue | 🟠 MajorFail fast on zero-valued optional record ids.
These new fields have the same sentinel bug:
Some(NodeId(0))andSome(BufferId(0))encode as0, and their decoders map0back toNone. That means zoom state or helper-buffer lineage can disappear on round-trip instead of being rejected.Please add validation before serializing these fields—either by making
encode_session_record/encode_buffer_recordfallible, or by preflighting them in the enclosing response/event encoder.Also applies to: 2617-2622
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/src/codec.rs` at line 2561, The serializer is silently collapsing Some(NodeId(0))/Some(BufferId(0)) to 0 which decoders treat as None; add validation to detect zero-valued optional ids before encoding: update encode_session_record and encode_buffer_record (or the enclosing response/event encoder that calls them) to check fields like zoomed_node_id and helper_buffer_id (and any other mapped .map(|n| n.into()).unwrap_or(0) sites such as the ones around lines 2617–2622) and return an error if a Some(id) has an inner value of zero, or else perform the check in the caller (preflight) and fail fast with a clear error instead of encoding 0.
1955-1964:⚠️ Potential issue | 🟠 MajorReject
Some(0)inBufferLocationoptional ids before encoding.The new preflight only guards
buffer_id.session_id,node_id, andfloating_idstill use0as the wire sentinel here, soSome(SessionId(0))/Some(NodeId(0))/Some(FloatingId(0))are silently rewritten toNoneon decode. That makes the new location responses non-round-trippable and can drop location context instead of failing fast.🔧 Suggested fix
+fn validate_optional_floating_id( + floating_id: Option<FloatingId>, + field: &'static str, +) -> Result<(), ProtocolError> { + if let Some(floating_id) = floating_id { + validate_required_floating_id(floating_id, field)?; + } + Ok(()) +} + pub fn encode_server_envelope(envelope: &ServerEnvelope) -> Result<Vec<u8>, ProtocolError> { let mut builder = FlatBufferBuilder::new(); - if let ServerEnvelope::Response(ServerResponse::BufferLocation(response)) = envelope - && response.location.buffer_id.0 == 0 - { - return Err(ProtocolError::InvalidMessageOwned( - "buffer_location_response.location.buffer_id must be non-zero".to_owned(), - )); + if let ServerEnvelope::Response(ServerResponse::BufferLocation(response)) = envelope { + validate_required_buffer_id( + response.location.buffer_id, + "buffer_location_response.location.buffer_id", + )?; + validate_optional_session_id( + response.location.session_id, + "buffer_location_response.location.session_id", + )?; + validate_optional_node_id( + response.location.node_id, + "buffer_location_response.location.node_id", + )?; + validate_optional_floating_id( + response.location.floating_id, + "buffer_location_response.location.floating_id", + )?; } if let ServerEnvelope::Response(ServerResponse::BufferWithLocation(response)) = envelope { + validate_optional_session_id( + response.location.session_id, + "buffer_with_location_response.location.session_id", + )?; + validate_optional_node_id( + response.location.node_id, + "buffer_with_location_response.location.node_id", + )?; + validate_optional_floating_id( + response.location.floating_id, + "buffer_with_location_response.location.floating_id", + )?; validate_buffer_with_location_response(response)?; }Also applies to: 2117-2120, 2243-2246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/src/codec.rs` around lines 1955 - 1964, The preflight currently only rejects a zero buffer_id in ServerResponse::BufferLocation but must also reject sentinel-zero wrapped in Some(...) for other optional ids; update the checks in the ServerEnvelope::Response arms (the BufferLocation and analogous places noted) to return ProtocolError::InvalidMessageOwned when any of response.location.session_id, response.location.node_id, or response.location.floating_id is Some(id) && id.0 == 0 (not just buffer_id), and apply the same validation logic in the other locations referenced (the similar checks around the other diffs and in validate_buffer_with_location_response usage) so Some(0) is rejected before encoding/decoding.crates/embers-server/src/server.rs (2)
1595-1637:⚠️ Potential issue | 🟠 MajorEmit
FloatingChangedwhen these mutations touch a floating owner.
layout_ok_response()only invalidates the node tree.BreakNode --to tabcan remove or retarget an existing floating window, andJoinBufferAtNodecan replace a floating root whennode_idis that root. Other clients can keep a stalesnapshot.floatinguntil some unrelated floating event arrives. Capture the pre-mutation floating id (for example viastate.floating_id_for_node(node_id)) and emitFloatingChangedwhenever one of these paths mutates floating ownership, not just wheninto_floatingistrue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 1595 - 1637, The BreakNode and JoinBufferAtNode handlers currently only call layout_ok_response (and emit FloatingChanged only when into_floating is true), which misses cases where floating ownership changes; before calling state.break_node or state.join_buffer_at_node capture the pre-mutation floating id for node_id (use state.floating_id_for_node(node_id)), perform the mutation, then compute the post-mutation floating id (e.g. via state.session(session_id).and_then(|s| s.focused_floating) or state.floating_id_for_node again) and if they differ push a ServerEvent::FloatingChanged(FloatingChangedEvent { session_id, floating_id: post }) onto the events returned alongside layout_ok_response (or construct the response/events like the BreakNode path does) so any change of floating ownership is emitted regardless of into_floating or node role.
2165-2168:⚠️ Potential issue | 🟠 MajorRollback the attached helper before removing it.
focus_revealed_buffer()can still fail here if the resolved client disappears between validation and focus. By this point the helper is already attached, soremove_buffer(helper_buffer_id)returns a conflict and leaves the new tab/floating behind even though the request failed.💡 Suggested rollback
Err(error) => { let mut state = self.state.lock().await; + let _ = state.detach_buffer(helper_buffer_id); let _ = state.remove_buffer(helper_buffer_id); Err(error) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 2165 - 2168, The failure path after focus_revealed_buffer() should first undo the helper attachment before calling remove_buffer(helper_buffer_id) to avoid a conflict when the client vanished; inside the Err(error) branch after acquiring self.state.lock().await, roll back the prior attach (e.g., call the inverse operation used to attach the helper — detach/remove the helper tab/floating for helper_buffer_id or invoke the state method that undoes the attach) and swallow any errors from that rollback, then call state.remove_buffer(helper_buffer_id) as currently written and return the original error.crates/embers-server/src/state.rs (1)
363-375:⚠️ Potential issue | 🟠 MajorPreserve the source
PtySizeon helper buffers.Helper snapshots later use
buffer.pty_sizeas the viewport. Rebuilding it fromlines.len()and a hardcoded80means full-history helpers advertise their entire scrollback as visible, and helpers opened from wider/narrower panes render with the wrong width. Thread the source buffer'sPtySizethroughcreate_helper_buffer()instead of synthesizing it here.💡 Suggested shape
pub fn create_helper_buffer( &mut self, title: impl Into<String>, source_buffer_id: BufferId, scope: HelperBufferScope, cwd: Option<PathBuf>, + size: PtySize, lines: Vec<String>, ) -> BufferId { let buffer_id = self.buffer_ids.next(); - let rows = u16::try_from(lines.len().max(1)).unwrap_or(u16::MAX); let mut buffer = Buffer::new(buffer_id, title, Vec::new(), cwd, BTreeMap::new()); - buffer.pty_size = PtySize::new(80, rows); + buffer.pty_size = size;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/state.rs` around lines 363 - 375, create_helper_buffer currently synthesizes a PtySize (80 x lines.len()) which misreports viewport; change the function to accept the source buffer's PtySize (e.g. add a parameter named source_pty: PtySize or source_pty_size: PtySize to create_helper_buffer) and set buffer.pty_size = source_pty instead of recreating it from lines and a hardcoded width; update all call sites to pass the source buffer's PtySize (or, if you prefer, read it from self.buffers.get(&source_buffer_id).pty_size at the start of create_helper_buffer) so helper buffers preserve the originating viewport.crates/embers-client/src/configured_client.rs (1)
538-585:⚠️ Potential issue | 🟡 MinorValidate
FocusBuffer/RevealBufferreplies before mutating queue context.These branches trust
location.buffer_idand then update focus/current session from it. If the server or codec crosses IDs, the rest of the queue can run against the wrong buffer/session while reporting success.🛠️ Suggested change
Action::FocusBuffer { buffer_id } => { let location = Self::buffer_location_from_response( self.client .request_message(ClientMessage::Buffer(BufferRequest::GetLocation { request_id: self.client.next_request_id(), buffer_id, })) .await?, "buffer focus", )?; + if location.buffer_id != buffer_id { + return Err(MuxError::protocol(format!( + "buffer focus returned location for {} instead of {}", + location.buffer_id, buffer_id + ))); + } let session_id = location.session_id.ok_or_else(|| { MuxError::conflict(format!( "buffer {buffer_id} is not attached to any session" )) })?; @@ Action::RevealBuffer { buffer_id } => { let location = Self::buffer_location_from_response( self.client .request_message(ClientMessage::Buffer(BufferRequest::Reveal { request_id: self.client.next_request_id(), buffer_id, client_id: None, })) .await?, "buffer reveal", )?; + if location.buffer_id != buffer_id { + return Err(MuxError::protocol(format!( + "buffer reveal returned location for {} instead of {}", + location.buffer_id, buffer_id + ))); + } let session_id = location.session_id.ok_or_else(|| { MuxError::conflict(format!( "buffer {buffer_id} is not attached to any session" )) })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/configured_client.rs` around lines 538 - 585, For Action::FocusBuffer and Action::RevealBuffer branches, validate the server reply (returned via buffer_location_from_response) before mutating queue context: ensure the returned location.buffer_id equals the requested buffer_id and that session_id/node_id are Some (for FocusBuffer) and present for RevealBuffer; if the IDs mismatch or required fields are missing, return an error (e.g., MuxError::protocol/conflict) instead of updating current_session_id, calling focus_node, set_active_view, or resyncing; apply these checks around the existing calls to buffer_location_from_response, and only perform focus_node, set_active_view, current_session_id assignment, and client.resync_all_sessions() after validation succeeds.
🤖 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 1699-1719: The current buffer show output emits title and cwd raw
which can contain tabs/newlines and breaks line-based parsing; update the
emission in the block building lines (the vec! starting with format!("id\t{}",
buffer.id)) to encode title and cwd losslessly (same approach used for command).
Specifically, JSON-serialize buffer.title and buffer.cwd (use
serde_json::to_string) before inserting them into the format!("title\t...") and
format!("cwd\t...") entries so the output is line-safe and reversible; keep the
existing labels and use the same expect/error strategy as used for serializing
buffer.command.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 333-345: The current use of tokio::time::timeout around
self.client.next_event() is unsafe because next_event() uses read_exact() and
cancelling that future can drop the reader mid-read and desynchronize the
transport; stop applying timeout directly to next_event(). Instead, move the
actual read loop into a dedicated background reader (so read_exact() runs in a
long-lived task and never gets dropped) and have next_event() become a channel
receiver (mpsc/oneshot) that the reader task sends ServerEvent through; then in
process_next_event_timeout use tokio::time::timeout on the channel receive (not
on next_event's read) so timeouts don't cancel the in-progress read. Update
symbols: modify the reader that calls read_exact() in framing.rs, adjust
next_event() in the client implementation to consume from the reader-owned mpsc,
and change process_next_event_timeout (the function shown) to timeout the
channel receive and keep calling self.handle_event(&event) as before.
In `@crates/embers-client/src/scripting/documentation.rs`:
- Around line 321-352: The rewrite_signature_literals function currently only
rewrites three signatures; extend it to also replace the Rhai-emitted generic
String/string signatures for notify and split_with with their literal unions.
Specifically, add replacements for "fn notify(_: ActionApi, level: String) ->
Action" and "fn notify(_: ActionApi, level: string) -> Action;" to use level:
\"info\" | \"warn\" | \"error\", and for "fn split_with(_: ActionApi, direction:
String) -> Action" and "fn split_with(_: ActionApi, direction: string) ->
Action;" to use direction: \"h\" | \"horizontal\" | \"v\" | \"vertical\" so both
markdown and defs forms are covered in rewrite_signature_literals.
In `@crates/embers-protocol/src/types.rs`:
- Around line 180-186: BufferLocation currently allows invalid combinations via
independent Option fields (session_id, node_id, floating_id); replace these with
a closed/tagged attachment type to enforce valid states: introduce an enum
(e.g., Attachment or BufferAttachment) with variants like Detached, Session {
session_id: SessionId, node_id: NodeId }, and Floating { floating_id: FloatingId
} and change BufferLocation to hold buffer_id: BufferId and attachment:
Attachment; update derives/traits and any (de)serialization, pattern matches,
and code paths that read session_id/node_id/floating_id to use the new enum
variants (e.g., match on BufferLocation::attachment) so impossible states can no
longer be represented.
In `@crates/embers-test-support/src/pty.rs`:
- Around line 54-60: The jitter calculation in openpty_with_retry uses
SystemTime::now().subsec_nanos() mod RETRY_JITTER_MS which can produce identical
values under concurrent calls; replace this with a proper pseudo-random
generator (e.g., use the fastrand crate or rand::thread_rng().gen_range) to
compute Duration::from_millis(random % Self::RETRY_JITTER_MS.max(1)). Update
Cargo.toml to add the chosen dependency (fastrand or rand) and modify the jitter
assignment in openpty_with_retry to call the random API instead of using
subsec_nanos(), keeping the same max(1) guard.
---
Duplicate comments:
In `@crates/embers-client/src/configured_client.rs`:
- Around line 538-585: For Action::FocusBuffer and Action::RevealBuffer
branches, validate the server reply (returned via buffer_location_from_response)
before mutating queue context: ensure the returned location.buffer_id equals the
requested buffer_id and that session_id/node_id are Some (for FocusBuffer) and
present for RevealBuffer; if the IDs mismatch or required fields are missing,
return an error (e.g., MuxError::protocol/conflict) instead of updating
current_session_id, calling focus_node, set_active_view, or resyncing; apply
these checks around the existing calls to buffer_location_from_response, and
only perform focus_node, set_active_view, current_session_id assignment, and
client.resync_all_sessions() after validation succeeds.
In `@crates/embers-protocol/src/codec.rs`:
- Line 2561: The serializer is silently collapsing
Some(NodeId(0))/Some(BufferId(0)) to 0 which decoders treat as None; add
validation to detect zero-valued optional ids before encoding: update
encode_session_record and encode_buffer_record (or the enclosing response/event
encoder that calls them) to check fields like zoomed_node_id and
helper_buffer_id (and any other mapped .map(|n| n.into()).unwrap_or(0) sites
such as the ones around lines 2617–2622) and return an error if a Some(id) has
an inner value of zero, or else perform the check in the caller (preflight) and
fail fast with a clear error instead of encoding 0.
- Around line 1955-1964: The preflight currently only rejects a zero buffer_id
in ServerResponse::BufferLocation but must also reject sentinel-zero wrapped in
Some(...) for other optional ids; update the checks in the
ServerEnvelope::Response arms (the BufferLocation and analogous places noted) to
return ProtocolError::InvalidMessageOwned when any of
response.location.session_id, response.location.node_id, or
response.location.floating_id is Some(id) && id.0 == 0 (not just buffer_id), and
apply the same validation logic in the other locations referenced (the similar
checks around the other diffs and in validate_buffer_with_location_response
usage) so Some(0) is rejected before encoding/decoding.
In `@crates/embers-server/src/server.rs`:
- Around line 1595-1637: The BreakNode and JoinBufferAtNode handlers currently
only call layout_ok_response (and emit FloatingChanged only when into_floating
is true), which misses cases where floating ownership changes; before calling
state.break_node or state.join_buffer_at_node capture the pre-mutation floating
id for node_id (use state.floating_id_for_node(node_id)), perform the mutation,
then compute the post-mutation floating id (e.g. via
state.session(session_id).and_then(|s| s.focused_floating) or
state.floating_id_for_node again) and if they differ push a
ServerEvent::FloatingChanged(FloatingChangedEvent { session_id, floating_id:
post }) onto the events returned alongside layout_ok_response (or construct the
response/events like the BreakNode path does) so any change of floating
ownership is emitted regardless of into_floating or node role.
- Around line 2165-2168: The failure path after focus_revealed_buffer() should
first undo the helper attachment before calling remove_buffer(helper_buffer_id)
to avoid a conflict when the client vanished; inside the Err(error) branch after
acquiring self.state.lock().await, roll back the prior attach (e.g., call the
inverse operation used to attach the helper — detach/remove the helper
tab/floating for helper_buffer_id or invoke the state method that undoes the
attach) and swallow any errors from that rollback, then call
state.remove_buffer(helper_buffer_id) as currently written and return the
original error.
In `@crates/embers-server/src/state.rs`:
- Around line 363-375: create_helper_buffer currently synthesizes a PtySize (80
x lines.len()) which misreports viewport; change the function to accept the
source buffer's PtySize (e.g. add a parameter named source_pty: PtySize or
source_pty_size: PtySize to create_helper_buffer) and set buffer.pty_size =
source_pty instead of recreating it from lines and a hardcoded width; update all
call sites to pass the source buffer's PtySize (or, if you prefer, read it from
self.buffers.get(&source_buffer_id).pty_size at the start of
create_helper_buffer) so helper buffers preserve the originating viewport.
In `@crates/embers-server/tests/model_state.rs`:
- Around line 765-781: The current assertions only check presence of empty_tabs;
instead assert its variant is Node::Tabs by pattern-checking
state.node(empty_tabs) result to ensure it returns a Node::Tabs(_) value, then
keep the existing parent and floating_id_for_node checks and finally call
state.validate() to trigger a failure if rollback corrupted broader invariants;
reference the node(empty_tabs) call, Node::Tabs variant,
floating_id_for_node(empty_tabs), node_parent(empty_tabs), and state.validate()
when making these changes.
🪄 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: eae60475-b176-404d-a82a-0cefb4bcc44c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (76)
crates/embers-cli/Cargo.tomlcrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/presentation.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/presentation.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-server/src/persist.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/model_state.rscrates/embers-test-support/src/pty.rsdocs/activity-bell-policy.mddocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-deaec7e4.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/action.mddocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/floating-ref.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/tree.mddocs/config-api/ui.md
Comment on lines
+333
to
+345
| pub async fn process_next_event_timeout( | ||
| &mut self, | ||
| timeout: std::time::Duration, | ||
| ) -> Result<Option<ServerEvent>> { | ||
| let Some(event) = tokio::time::timeout(timeout, self.client.next_event()) | ||
| .await | ||
| .ok() | ||
| .transpose()? | ||
| else { | ||
| return Ok(None); | ||
| }; | ||
| self.handle_event(&event).await?; | ||
| Ok(Some(event)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== timeout wrapper in ConfiguredClient ==="
sed -n '333,345p' crates/embers-client/src/configured_client.rs
echo
echo "=== MuxClient event-read entry points ==="
rg -n -C5 '\b(async\s+fn|fn)\s+(next_event|process_next_event_timeout|process_next_event)\b' crates/embers-client
echo
echo "=== Underlying receive/decoder path ==="
rg -n -C5 'read_exact|AsyncReadExt::read_exact|\btrait\s+Transport\b|\b(async\s+fn|fn)\s+(recv|receive|read_message|read_envelope|next_event)\b' crates/embers-client crates/embers-protocolRepository: Pajn/Embers
Length of output: 15101
Fix cancellation safety: tokio::time::timeout() wrapping read_exact()-based next_event() will corrupt the transport stream.
The receive path uses read_exact() in crates/embers-protocol/src/framing.rs (lines 51 and 68) to read frame headers and payloads. When tokio::time::timeout() expires and drops the in-flight future, if read_exact() is mid-read, the stream position becomes inconsistent and subsequent frames cannot be decoded. The transport will be permanently desynchronized.
Either wrap a cancellation-safe receive primitive, or spawn a separate cancellation scope that does not drop the reader mid-operation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-client/src/configured_client.rs` around lines 333 - 345, The
current use of tokio::time::timeout around self.client.next_event() is unsafe
because next_event() uses read_exact() and cancelling that future can drop the
reader mid-read and desynchronize the transport; stop applying timeout directly
to next_event(). Instead, move the actual read loop into a dedicated background
reader (so read_exact() runs in a long-lived task and never gets dropped) and
have next_event() become a channel receiver (mpsc/oneshot) that the reader task
sends ServerEvent through; then in process_next_event_timeout use
tokio::time::timeout on the channel receive (not on next_event's read) so
timeouts don't cancel the in-progress read. Update symbols: modify the reader
that calls read_exact() in framing.rs, adjust next_event() in the client
implementation to consume from the reader-owned mpsc, and change
process_next_event_timeout (the function shown) to timeout the channel receive
and keep calling self.handle_event(&event) as before.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/embers-server/src/state.rs (1)
598-598:⚠️ Potential issue | 🟡 MinorNo-op expression:
active.saturating_sub(0)always returnsactive.This appears to be a typo or copy-paste error. The expression has no effect.
🔧 Proposed fix
- active: active.min(active.saturating_sub(0)), + active: active.min(tabs.len().saturating_sub(1)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/state.rs` at line 598, The expression active.min(active.saturating_sub(0)) is a no-op because active.saturating_sub(0) equals active; replace this with the simpler and correct value (e.g., just active) or, if the intent was to subtract a non-zero amount, use the correct operand instead of 0. Update the assignment that sets the active field (the active: active.min(...) expression) to either active or active.min(active.saturating_sub(<correct_amount>)) as appropriate so the logic is non-redundant and reflects the intended decrement/limit.crates/embers-client/tests/e2e.rs (1)
752-787:⚠️ Potential issue | 🟡 MinorAssert that
buffer_aactually reattaches totarget_leaf.After
MoveBufferToNode, this only proves thatbuffer_bbecomes detached andbuffer_astill accepts input. A detachedbuffer_awould still satisfy both checks, so the reattach path can regress silently.🧪 Suggested assertion tightening
let _ = connection .request(&ClientMessage::Node(NodeRequest::MoveBufferToNode { request_id: new_request_id(), buffer_id: buffer_a.id, target_leaf_node_id: target_leaf, })) .await .expect("reattach detached buffer succeeds"); + let reattached = buffer_record(&mut connection, buffer_a.id).await; + assert_eq!(reattached.attachment_node_id, Some(target_leaf)); @@ assert!(detached.iter().any(|buffer| buffer.id == buffer_b.id)); + assert!(!detached.iter().any(|buffer| buffer.id == buffer_a.id)); assert_ne!(main_leaf, target_leaf);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/tests/e2e.rs` around lines 752 - 787, The test currently only verifies buffer_a accepts input after MoveBufferToNode but doesn't assert it attached to target_leaf; after the MoveBufferToNode request (and the wait_for_capture_contains), call connection.request with ClientMessage::Buffer(BufferRequest::List { request_id: new_request_id(), session_id: None, attached_only: true, detached_only: false }) and assert in the returned BuffersResponse that there is a buffer with id == buffer_a.id and that its node/leaf id equals target_leaf (or otherwise the field that records the node assignment), ensuring buffer_a is actually attached to target_leaf rather than merely alive.crates/embers-protocol/src/codec.rs (1)
3051-3072:⚠️ Potential issue | 🟠 MajorReject invalid tab metadata in
CreateTabs.
req.active()andtitlesare trusted as-is after parsingchild_node_ids. A malformed frame can therefore decode into aNodeRequest::CreateTabswhose active index is out of range or whose titles no longer line up 1:1 with the children. This should be rejected here, and mirrored invalidate_node_requestso local encodes fail early too.🛠️ Minimal guardrails
fb::NodeOp::CreateTabs => { let child_node_ids = required(req.child_node_ids(), "node_request.child_node_ids")? .iter() .map(|node_id| { decode_required_node_id(node_id, "node_request.child_node_ids") }) - .collect::<Result<_, _>>()?; - let titles = required(req.titles(), "node_request.titles")? + .collect::<Result<Vec<_>, _>>()?; + let titles: Vec<String> = required(req.titles(), "node_request.titles")? .iter() .map(|title| title.to_owned()) .collect(); + if titles.len() != child_node_ids.len() { + return Err(ProtocolError::InvalidMessageOwned( + "node_request.titles length must match child_node_ids".to_owned(), + )); + } + let active = usize::try_from(req.active()).map_err(|_| { + ProtocolError::InvalidMessageOwned( + "node_request.active is out of range".to_owned(), + ) + })?; + if active >= child_node_ids.len() { + return Err(ProtocolError::InvalidMessageOwned( + "node_request.active must reference an existing child".to_owned(), + )); + } NodeRequest::CreateTabs { request_id, session_id: decode_required_session_id( req.session_id(), "node_request.session_id",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/src/codec.rs` around lines 3051 - 3072, The CreateTabs decoder (fb::NodeOp::CreateTabs -> NodeRequest::CreateTabs) currently trusts req.titles() and req.active() after parsing child_node_ids, allowing malformed frames where titles.count != child_node_ids.count or active is out-of-range; update the decoder to validate that titles.len() == child_node_ids.len() and that the active index (from req.active()) is within 0..titles.len() (or treat out-of-range as error) and return a decoding error instead of constructing NodeRequest::CreateTabs; also add the same checks in validate_node_request for NodeRequest::CreateTabs so local encoders/constructors fail early. Ensure you keep using the existing helpers (decode_required_node_id, decode_required_session_id) and return the same error type used by other decoding failures.
♻️ Duplicate comments (3)
crates/embers-client/tests/configured_client.rs (1)
1198-1205: 🧹 Nitpick | 🔵 TrivialMake this test request-aware too.
Lines 1198-1205 queue canned
SECOND_BUFFER_IDresponses onFakeTransport, so this test can still go green if the client asks for the wrong buffer before running the hook. Please switch this one toScriptedTransport, or asserttransport.requests()afterprocess_next_event().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/tests/configured_client.rs` around lines 1198 - 1205, The test currently queues fixed SECOND_BUFFER_ID responses with FakeTransport via transport.push_response (ServerResponse::Buffer/VisibleSnapshot), which lets the client pass even if it requested the wrong buffer; change the transport to a request-aware ScriptedTransport (or alternatively, after calling process_next_event() add an assertion on transport.requests() to verify the expected RequestId/buffer request was made) so the test fails if the client asks for a different buffer; update the test setup that constructs FakeTransport to use ScriptedTransport and generate scripted responses matching the expected request sequence (or add an assertion checking transport.requests() contains the expected buffer request after process_next_event()).crates/embers-cli/src/lib.rs (1)
1699-1720:⚠️ Potential issue | 🟡 MinorKeep
buffer showline-safe fortitleandcwdtoo.
commandis JSON-encoded now, buttitleandcwdare still emitted raw. Tabs or newlines in either field will still split rows and break scripting output.🛠️ Suggested fix
let mut lines = vec![ format!("id\t{}", buffer.id), - format!("title\t{}", buffer.title), + format!( + "title\t{}", + serde_json::to_string(&buffer.title).expect("buffer titles serialize to JSON") + ), format!("state\t{}", buffer_state_label(buffer.state)), format!("kind\t{}", buffer_kind_label(buffer.kind)), format!("read_only\t{}", usize::from(buffer.read_only)), format!("location\t{}", format_buffer_location_inline(location)), ]; @@ if let Some(cwd) = &buffer.cwd { - lines.push(format!("cwd\t{cwd}")); + lines.push(format!( + "cwd\t{}", + serde_json::to_string(cwd).expect("buffer cwd serializes to JSON") + )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-cli/src/lib.rs` around lines 1699 - 1720, The current emitter writes title and cwd raw which allows tabs/newlines to break the tab-separated output; change the title and cwd lines to JSON-escape them like command does: replace format!("title\t{}", buffer.title) with serializing buffer.title via serde_json::to_string(&buffer.title).expect("buffer title serialize to JSON") and similarly serialize cwd with serde_json::to_string(cwd).expect("buffer cwd serialize to JSON") before pushing them into lines so tabs/newlines are encoded and scripting output remains line-safe.crates/embers-client/src/configured_client.rs (1)
333-346:⚠️ Potential issue | 🔴 CriticalCancellation safety issue with
tokio::time::timeoutwrappingnext_event().This timeout wrapper is not cancellation-safe. The underlying
next_event()path usesread_exact()for frame headers and payloads. If the timeout expires mid-read, the in-flight future is dropped, leaving the stream position inconsistent. Subsequent reads will interpret partial data as new frames, permanently desynchronizing the transport.Consider one of these approaches:
- Spawn the reader in a separate task that is never cancelled, communicating via a channel
- Use a cancellation-safe buffering layer that atomically reads complete messages
- Add a read-side buffer that can resume partial reads
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/configured_client.rs` around lines 333 - 346, The timeout wrapper around self.client.next_event() in process_next_event_timeout is not cancellation-safe because next_event() uses read_exact; instead spawn a dedicated reader task that runs next_event() (and underlying read_exact) without being cancelled and sends completed ServerEvent values over a channel; change process_next_event_timeout to wait on the receiver with tokio::time::timeout (so only the receiver wait is cancelled), keep using handle_event(&event) after receiving, and ensure the reader task is tied to the ConfiguredClient lifetime (store the sender/receiver on the struct and start the task when the client is created) so partial reads never leave the stream in an inconsistent state.
🤖 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/tests/interactive.rs`:
- Around line 11-19: The local process-only Mutex returned by test_lock() must
be replaced with the shared inter-process lock: remove or stop using the
test_lock() function and call acquire_test_lock() instead wherever you currently
do let _guard = test_lock().lock().await; — update those lines to use let _guard
= acquire_test_lock().lock().await; (or the equivalent await pattern for
acquire_test_lock()) so interactive.rs coordinates with other CLI integration
tests like panes.rs; keep the rest of the test logic intact.
In `@crates/embers-client/src/presentation.rs`:
- Around line 119-120: When projecting the zoom target you currently call
projector.project_node(..., is_root = false) which drops the root flag when
session.zoomed_node_id == session.root_node_id; change the call in that branch
so it preserves is_root = true when the zoomed node equals the session root (so
PresentationModel::root_tabs remains Some for root Tabs), and add a regression
test that creates a session where root_node_id == zoomed_node_id with a Tabs
root to assert root_tabs is preserved while zoomed; update any affected
expectations in the test suite accordingly.
In `@crates/embers-client/tests/e2e.rs`:
- Around line 973-987: After selecting the hidden tab via
connection.request(ClientMessage::Node(NodeRequest::SelectTab { request_id:
new_request_id(), tabs_node_id: fixture.nested_tabs_id, index: 1 })) and waiting
for server-side idle with wait_for_buffer_activity(&mut connection,
fixture.hidden_buffer.id, ActivityState::Idle, ...).await, add an assertion that
the client UI cleared the Bell indicator for fixture.hidden_buffer.id — e.g.
query the client-side buffer/tab state (using the existing test helper that
fetches buffer indicators or tab state) and assert that the Bell flag/indicator
is no longer present for that buffer; place this check immediately after the
wait_for_buffer_activity call to ensure the revealed tab no longer shows a Bell.
- Around line 1161-1206: The test currently derives latest_rendered_line from
client.state().snapshots (same cache Renderer uses), which can be stale; instead
fetch the authoritative snapshot from the server via the existing connection and
use that as the oracle. Replace the lookup that uses
client.state().snapshots.get(&buffer.id) (the code computing
latest_rendered_line) with a call that fetches the server snapshot for buffer.id
via the connection (e.g., use the connection's snapshot/get or similar RPC
method), then compute latest_rendered_line from that server snapshot and use it
in the assert checks so the expectation is based on the server-side state rather
than the client's cached state.
In `@crates/embers-protocol/src/types.rs`:
- Around line 180-186: BufferLocation currently allows inconsistent attachment
states via independent Option fields (session_id, node_id, floating_id); either
replace the struct with a tagged enum representing distinct attachment variants
(e.g., Detached, Session(SessionId), Node(NodeId), Floating(FloatingId),
Composite(...)) or add an explicit constructor/validator on BufferLocation
(e.g., BufferLocation::new or BufferLocation::validate) that enforces allowed
combinations and returns a Result, referencing the BufferLocation type and its
session_id, node_id, and floating_id fields so callers must produce or check a
validated instance before use.
In `@crates/embers-server/src/server.rs`:
- Around line 2268-2291: The returned ScrollbackSliceResponse currently uses the
original requested start_line even though you clamp it to start when building
helper_slice; update the response to use the clamped origin (convert the
computed local start usize into the u64/total_lines coordinate) so the returned
start_line never exceeds total_lines. Locate the helper_slice construction in
the helper match (variables total_lines and start) and ensure the
ScrollbackSliceResponse uses that clamped value instead of the original
start_line.
- Around line 1894-1907: The current code falls back to selecting the last
attached client by iter().rev().find(...) when the caller (connection_id) is
detached; change this so it fails on ambiguity: after locking clients
(clients.lock().await) check if clients.get(&connection_id).is_some_and(|c|
c.current_session_id.is_some()) and return as before, otherwise collect all
client IDs where client.current_session_id.is_some(); if exactly one exists
return that ID, otherwise return MuxError::conflict("no interactive client is
currently attached" or "multiple interactive clients attached" as appropriate).
Replace the iter().rev().find(...) branch with this unambiguous check to avoid
auto-picking the highest connection id.
---
Outside diff comments:
In `@crates/embers-client/tests/e2e.rs`:
- Around line 752-787: The test currently only verifies buffer_a accepts input
after MoveBufferToNode but doesn't assert it attached to target_leaf; after the
MoveBufferToNode request (and the wait_for_capture_contains), call
connection.request with ClientMessage::Buffer(BufferRequest::List { request_id:
new_request_id(), session_id: None, attached_only: true, detached_only: false })
and assert in the returned BuffersResponse that there is a buffer with id ==
buffer_a.id and that its node/leaf id equals target_leaf (or otherwise the field
that records the node assignment), ensuring buffer_a is actually attached to
target_leaf rather than merely alive.
In `@crates/embers-protocol/src/codec.rs`:
- Around line 3051-3072: The CreateTabs decoder (fb::NodeOp::CreateTabs ->
NodeRequest::CreateTabs) currently trusts req.titles() and req.active() after
parsing child_node_ids, allowing malformed frames where titles.count !=
child_node_ids.count or active is out-of-range; update the decoder to validate
that titles.len() == child_node_ids.len() and that the active index (from
req.active()) is within 0..titles.len() (or treat out-of-range as error) and
return a decoding error instead of constructing NodeRequest::CreateTabs; also
add the same checks in validate_node_request for NodeRequest::CreateTabs so
local encoders/constructors fail early. Ensure you keep using the existing
helpers (decode_required_node_id, decode_required_session_id) and return the
same error type used by other decoding failures.
In `@crates/embers-server/src/state.rs`:
- Line 598: The expression active.min(active.saturating_sub(0)) is a no-op
because active.saturating_sub(0) equals active; replace this with the simpler
and correct value (e.g., just active) or, if the intent was to subtract a
non-zero amount, use the correct operand instead of 0. Update the assignment
that sets the active field (the active: active.min(...) expression) to either
active or active.min(active.saturating_sub(<correct_amount>)) as appropriate so
the logic is non-redundant and reflects the intended decrement/limit.
---
Duplicate comments:
In `@crates/embers-cli/src/lib.rs`:
- Around line 1699-1720: The current emitter writes title and cwd raw which
allows tabs/newlines to break the tab-separated output; change the title and cwd
lines to JSON-escape them like command does: replace format!("title\t{}",
buffer.title) with serializing buffer.title via
serde_json::to_string(&buffer.title).expect("buffer title serialize to JSON")
and similarly serialize cwd with serde_json::to_string(cwd).expect("buffer cwd
serialize to JSON") before pushing them into lines so tabs/newlines are encoded
and scripting output remains line-safe.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 333-346: The timeout wrapper around self.client.next_event() in
process_next_event_timeout is not cancellation-safe because next_event() uses
read_exact; instead spawn a dedicated reader task that runs next_event() (and
underlying read_exact) without being cancelled and sends completed ServerEvent
values over a channel; change process_next_event_timeout to wait on the receiver
with tokio::time::timeout (so only the receiver wait is cancelled), keep using
handle_event(&event) after receiving, and ensure the reader task is tied to the
ConfiguredClient lifetime (store the sender/receiver on the struct and start the
task when the client is created) so partial reads never leave the stream in an
inconsistent state.
In `@crates/embers-client/tests/configured_client.rs`:
- Around line 1198-1205: The test currently queues fixed SECOND_BUFFER_ID
responses with FakeTransport via transport.push_response
(ServerResponse::Buffer/VisibleSnapshot), which lets the client pass even if it
requested the wrong buffer; change the transport to a request-aware
ScriptedTransport (or alternatively, after calling process_next_event() add an
assertion on transport.requests() to verify the expected RequestId/buffer
request was made) so the test fails if the client asks for a different buffer;
update the test setup that constructs FakeTransport to use ScriptedTransport and
generate scripted responses matching the expected request sequence (or add an
assertion checking transport.requests() contains the expected buffer request
after process_next_event()).
🪄 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: a5a71bd6-63cf-48e9-9e9b-5b34892c9eb7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (76)
crates/embers-cli/Cargo.tomlcrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/presentation.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/presentation.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-server/src/persist.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/model_state.rscrates/embers-test-support/src/pty.rsdocs/activity-bell-policy.mddocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-deaec7e4.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/action.mddocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/floating-ref.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/tree.mddocs/config-api/ui.md
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/embers-client/src/scripting/runtime.rs (1)
2180-2199: 🧹 Nitpick | 🔵 TrivialAdd a regression test for
blink.
blinkis now part of the publicui.segment(..., options)contract, but I don't see coverage for it in this file's added tests. A smallparse_segment_optionsorui.segment("x", #{ blink: true })assertion would pin the new flag and keep the docs/runtime behavior from drifting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/scripting/runtime.rs` around lines 2180 - 2199, Add a regression test that verifies the new blink flag is parsed and preserved: call parse_segment_options (or exercise ui.segment("x", #{ blink: true })) with an options Map containing "blink": true, then assert the returned StyleSpec has blink == true (and no unknown options remain) so the blink flag is covered by tests and cannot regress; reference parse_segment_options and StyleSpec (or the ui.segment API) when adding the assertion.
♻️ Duplicate comments (10)
crates/embers-protocol/schema/embers.fbs (1)
293-301: 🛠️ Refactor suggestion | 🟠 MajorFinish pinning IDs on the remaining public payloads.
SessionRecordand the newBufferLocation*tables still depend on declaration-order field numbering, while the rest of this PR moves the protocol toward explicitid:assignments. That leaves part of the wire format with different evolution rules. Please pin these tables too so future inserts/reorders stay wire-stable.♻️ Minimal follow-up
table SessionRecord { - id:ulong; - name:string; - root_node_id:ulong; - floating_ids:[ulong]; - focused_leaf_id:ulong = 0; - focused_floating_id:ulong = 0; - zoomed_node_id:ulong = 0; + id:ulong (id: 0); + name:string (id: 1); + root_node_id:ulong (id: 2); + floating_ids:[ulong] (id: 3); + focused_leaf_id:ulong = 0 (id: 4); + focused_floating_id:ulong = 0 (id: 5); + zoomed_node_id:ulong = 0 (id: 6); } table BufferLocation { - buffer_id:ulong; - session_id:ulong = 0; - node_id:ulong = 0; - floating_id:ulong = 0; + buffer_id:ulong (id: 0); + session_id:ulong = 0 (id: 1); + node_id:ulong = 0 (id: 2); + floating_id:ulong = 0 (id: 3); } table BufferLocationResponse { - location:BufferLocation; + location:BufferLocation (id: 0); } table BufferWithLocationResponse { - buffer:BufferRecord; - location:BufferLocation; + buffer:BufferRecord (id: 0); + location:BufferLocation (id: 1); }Also applies to: 426-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-protocol/schema/embers.fbs` around lines 293 - 301, SessionRecord and the new BufferLocation* tables rely on declaration-order field numbering, so pin each field with explicit id: assignments to make the wire format stable; update SessionRecord by adding explicit ids (e.g. id:ulong -> id:1, name:string -> id:2, root_node_id:3, floating_ids:4, focused_leaf_id:5, focused_floating_id:6, zoomed_node_id:7) and make the analogous explicit id: assignments for every field in the BufferLocation* tables so future inserts or reorders won't change on-the-wire numbers.crates/embers-server/tests/model_state.rs (1)
756-780:⚠️ Potential issue | 🟡 MinorFinish the rollback test with
state.validate().The explicit assertions cover the visible rollback state, but they still won't catch a stale parent/focus/floating index that leaves the graph internally inconsistent. Ending the test with
state.validate()keeps the rollback contract fully exercised.🧪 Minimal fix
assert_eq!( state .floating_id_for_node(empty_tabs) .expect("floating lookup"), None ); + state.validate().expect("state remains valid"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/tests/model_state.rs` around lines 756 - 780, Add a final call to state.validate() at the end of the rollback test to assert internal graph consistency; locate the test assertions that check session, node, parent, and floating indices (references: state.session(...), state.node(...), state.node_parent(...), state.floating_id_for_node(...)) and append state.validate() as the last assertion so the rollback contract is fully exercised.crates/embers-client/src/presentation.rs (1)
119-120:⚠️ Potential issue | 🟠 MajorPreserve the root flag when zooming the session root.
When
session.zoomed_node_id == session.root_node_id, Line 120 still projects the subtree withis_root = false. That leavesPresentationModel::root_tabsempty for a zoomed rootTabstree, so root-tab rendering/actions lose their anchor in the zoom path.🩹 Minimal fix
- projector.project_node(zoomed_node_id, root_bounds, None, false, Vec::new())?; + let is_root = zoomed_node_id == session.root_node_id; + projector.project_node(zoomed_node_id, root_bounds, None, is_root, Vec::new())?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/presentation.rs` around lines 119 - 120, The projector call is passing is_root = false even when the zoomed node is the session root, which prevents PresentationModel::root_tabs from being populated; change the call site that currently does projector.project_node(zoomed_node_id, root_bounds, None, false, Vec::new()) so that the is_root argument is true when session.zoomed_node_id == session.root_node_id (e.g., compute a local is_root = zoomed_node_id == root_node_id and pass that boolean) so the zoomed-root path preserves root_tabs.crates/embers-cli/tests/interactive.rs (1)
11-19:⚠️ Potential issue | 🟠 MajorReplace the file-local PTY mutex with the shared CLI test lock.
Lines 16-19 only serialize within this integration-test binary. Rust builds each file under
crates/embers-cli/tests/as a separate binary, sointeractive.rscan still race with sibling PTY tests on socket/server cleanup. Use the sharedacquire_test_lock()helper here instead of a localOnceLock<Mutex<()>>.#!/bin/bash set -euo pipefail echo "Local lock in interactive.rs:" rg -n -C2 'fn test_lock|test_lock\(\)\.lock\(\)\.await' crates/embers-cli/tests/interactive.rs echo echo "Shared lock helper and existing call sites:" rg -n -C2 'acquire_test_lock' crates/embers-test-support crates/embers-cli/testsExpected:
interactive.rsshows the localtest_lock()helper and call sites, while the repository exposesacquire_test_lock()for cross-binary coordination.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-cli/tests/interactive.rs` around lines 11 - 19, The file defines a file-local test_lock() using OnceLock<Mutex<()>> which only serializes within this binary; replace that helper with the shared test lock by removing test_lock() and using the repo-provided acquire_test_lock() helper instead wherever test_lock().lock().await is called (preserve awaiting the returned guard). Update imports to remove OnceLock and local Mutex if unused and import or reference acquire_test_lock() from the embers_test_support helper so the integration test coordinates with sibling PTY tests.crates/embers-server/src/server.rs (2)
2280-2303:⚠️ Potential issue | 🟡 MinorReturn the clamped origin for helper scrollback slices.
startis already clamped to the helper length, but the response still echoes the raw request. Out-of-range requests can reportstart_line > total_lines, which will skew client scroll state.♻️ Minimal fix
- let helper_slice = { + let helper_slice = { let state = self.state.lock().await; match &state.buffer(buffer_id)?.kind { BufferKind::Helper(helper) => { let total_lines = u64::try_from(helper.lines.len()).unwrap_or(u64::MAX); let start = usize::try_from(start_line) .unwrap_or(usize::MAX) .min(helper.lines.len()); let end = start .saturating_add(usize::try_from(line_count).unwrap_or(usize::MAX)) .min(helper.lines.len()); - Some((total_lines, helper.lines[start..end].to_vec())) + Some(( + total_lines, + u64::try_from(start).unwrap_or(u64::MAX), + helper.lines[start..end].to_vec(), + )) } BufferKind::Pty => None, } }; - if let Some((total_lines, lines)) = helper_slice { + if let Some((total_lines, start_line, lines)) = helper_slice { return Ok(ScrollbackSliceResponse { request_id, buffer_id, start_line, total_lines, lines, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 2280 - 2303, The response currently echoes the raw request start_line even when you clamp it to the helper buffer; locate the helper_slice logic (match on BufferKind::Helper in the block that builds helper_slice) and return the clamped origin instead of start_line when constructing ScrollbackSliceResponse: convert the local clamped start (the usize variable computed in that block) to the response type and use that for the response's start_line/origin so total_lines and start align with the actual slice returned.
1925-1938:⚠️ Potential issue | 🟠 MajorFail on ambiguous implicit client selection.
When the caller is detached, this still picks the last attached client.
Reveal/OpenHistorycan therefore switch the wrong user's session instead of forcing an explicitclient_id.♻️ Minimal fix
- clients - .iter() - .rev() - .find(|(_, client)| client.current_session_id.is_some()) - .map(|(client_id, _)| *client_id) - .ok_or_else(|| MuxError::conflict("no interactive client is currently attached")) + let mut attached = clients + .iter() + .filter_map(|(client_id, client)| client.current_session_id.map(|_| *client_id)); + + match (attached.next(), attached.next()) { + (Some(client_id), None) => Ok(client_id), + (None, _) => Err(MuxError::conflict("no interactive client is currently attached")), + (Some(_), Some(_)) => Err(MuxError::conflict( + "multiple interactive clients are attached; specify client_id", + )), + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 1925 - 1938, The code implicitly selects the last attached client when the caller is detached, which can cause wrong session switching; instead, after confirming the caller is not attached (the existing clients.get(...).is_some_and(...) branch), scan self.clients for entries where client.current_session_id.is_some(), and if exactly one match return that client_id, if zero return the existing conflict error, and if more than one return a distinct conflict/error indicating ambiguous implicit client selection and require an explicit client_id; update the block using self.clients.iter().rev().find(...) to perform a count and single-match check so ambiguity is detected (referencing self.clients, connection_id, and client.current_session_id).crates/embers-client/tests/e2e.rs (2)
1165-1171:⚠️ Potential issue | 🟡 MinorUse the server snapshot as the oracle here.
latest_rendered_linecomes fromclient.state().snapshots, which is the same cacheRendererrenders from. If that cache is stale, the assertion goes stale with it and this test can still pass.🧪 Tighten the test
- wait_for_visible_snapshot( + let visible = wait_for_visible_snapshot( &mut connection, buffer.id, Duration::from_secs(3), |snapshot| snapshot.total_lines >= 80 && snapshot.lines.join("\n").contains("burst-80"), ) .await; @@ - let session_id = session_id_by_name(&client, "alpha"); - let presentation = PresentationModel::project( - client.state(), - session_id, - Size { - width: 80, - height: 24, - }, - ) - .expect("projection succeeds"); - let visible_rows = presentation - .focused_leaf() - .expect("focused leaf") - .rect - .size - .height - .saturating_sub(1) as usize; - let latest_rendered_line = client - .state() - .snapshots - .get(&buffer.id) - .expect("burst snapshot") - .lines - .iter() - .take(visible_rows) - .rev() - .find(|line| line.starts_with("burst-")) - .expect("latest rendered burst line"); - assert!(render.contains(latest_rendered_line)); + let latest_visible_line = visible + .lines + .iter() + .rev() + .find(|line| line.starts_with("burst-")) + .expect("latest visible burst line"); + assert!(render.contains(latest_visible_line));Also applies to: 1173-1205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/tests/e2e.rs` around lines 1165 - 1171, The test currently uses the client's cached state (client.state().snapshots / latest_rendered_line) as the oracle which can be stale; instead capture and use the server snapshot returned by wait_for_visible_snapshot (the call that takes &mut connection, buffer.id, Duration::from_secs(3), ...) as the authoritative snapshot for assertions and rendering checks, and replace any uses of client.state().snapshots or latest_rendered_line in this block and the similar section covering lines 1173-1205 with the returned snapshot value so the test validates against the server snapshot the Renderer is supposed to render from.
973-987:⚠️ Potential issue | 🟡 MinorAssert the client clears
Bellafter the tab is revealed.This only waits for server-side
BufferRecord.activityto return toIdle. If the client keeps rendering the tab asBellafter reveal, the test still passes.🧪 Tighten the assertion
wait_for_buffer_activity( &mut connection, fixture.hidden_buffer.id, ActivityState::Idle, Duration::from_secs(3), ) .await; + + client + .resync_all_sessions() + .await + .expect("client resyncs sessions"); + refresh_all_snapshots(&mut client).await; + let model = PresentationModel::project( + client.state(), + session_id, + Size { + width: 80, + height: 24, + }, + ) + .expect("projection succeeds"); + let tabs = model + .tab_bars + .iter() + .find(|tabs| tabs.node_id == fixture.nested_tabs_id) + .expect("nested tabs frame exists"); + assert!( + !tabs + .tabs + .iter() + .any(|tab| tab.title == "bg" && tab.activity == ActivityState::Bell), + "revealed tab should clear bell activity in the client model" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/tests/e2e.rs` around lines 973 - 987, After selecting the hidden tab and waiting for server-side activity to become Idle, also assert the client's rendered state for that buffer no longer has the Bell indicator: after the existing call to wait_for_buffer_activity(&mut connection, fixture.hidden_buffer.id, ActivityState::Idle, ...).await, make an additional request via the same connection (e.g. a ClientMessage query that fetches the client's view of the buffer using fixture.hidden_buffer.id) and assert the returned buffer/render metadata does not have the Bell flag set; place this assertion immediately after wait_for_buffer_activity so the test fails if the client keeps rendering Bell after the tab is revealed.crates/embers-client/src/configured_client.rs (1)
333-345:⚠️ Potential issue | 🔴 CriticalDon't timeout
next_event()by dropping an in-flight frame read.If
next_event()still bottoms out inread_exact(),tokio::time::timeout()can cancel it mid-frame and permanently desynchronize the transport. This reintroduces the same stream-corruption path that was already flagged earlier.#!/bin/bash set -euo pipefail echo "=== timeout wrapper in ConfiguredClient ===" sed -n '333,345p' crates/embers-client/src/configured_client.rs echo echo "=== event-read entry points ===" rg -n -C4 '\b(async\s+fn|fn)\s+(next_event|process_next_event_timeout|process_next_event)\b' crates/embers-client crates/embers-protocol echo echo "=== cancellation-sensitive frame reads ===" rg -n -C2 'read_exact' crates/embers-client crates/embers-protocolcrates/embers-cli/src/lib.rs (1)
516-538:⚠️ Potential issue | 🟡 MinorValidate
--placement floatingagainst the returned attachment.This branch only rejects
Detached. IfOpenHistoryregresses and returns a tab/session attachment for a floating request, the CLI still exits0and reports success for the wrong placement.🛠️ Minimal guard
let location = expect_buffer_location(response, "buffer history")?; + if matches!(placement, HistoryPlacementArg::Floating) + && !matches!(location.attachment, BufferLocationAttachment::Floating { .. }) + { + return Err(MuxError::protocol(format!( + "buffer history requested floating placement for buffer {buffer_id} but server returned {:?}", + location.attachment + ))); + } if matches!(location.attachment, BufferLocationAttachment::Detached) { return Err(MuxError::protocol(format!( "buffer history returned detached helper location for buffer {}",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-cli/src/lib.rs` around lines 516 - 538, The History branch currently only rejects BufferLocationAttachment::Detached but must also verify the returned attachment matches the requested placement; after calling expect_buffer_location and before returning, check the original placement argument (via history_placement(placement) or the raw placement variable) and if the requested placement is floating ensure location.attachment is BufferLocationAttachment::Floating (and similarly validate tab/session cases if applicable); if it does not match, return a MuxError::protocol with a clear message like "buffer history returned unexpected attachment for buffer {buffer_id}: expected Floating" so the CLI fails non-zero instead of reporting success for the wrong placement.
🤖 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-client/src/scripting/documentation.rs`:
- Around line 308-310: rewrite_generated_markdown currently only calls
rewrite_signature_literals and leaves raw <button> tags untyped; update it to
also normalize generated tab buttons to type="button" by post-processing the
content (either inline or via a new helper) after rewrite_signature_literals
returns. Specifically, ensure rewrite_generated_markdown (and/or a new function
referenced there) finds <button ...> tags that lack a type attribute and injects
type="button" so tab controls are not treated as submit buttons; reference
rewrite_generated_markdown and rewrite_signature_literals when making the
change.
In `@crates/embers-protocol/tests/family_round_trip.rs`:
- Around line 372-389: The floating BufferWithLocation fixture is inconsistent:
buffers[1] has attachment_node_id Some(NodeId(23]) but the test pairs it with
BufferLocation::floating(..., NodeId(22), ...). Update the
BufferLocation::floating call in the ServerResponse::BufferWithLocation entry
(BufferWithLocationResponse that references buffers[1]) to use NodeId(23) so the
location matches buffers[1].attachment_node_id and remains a valid output for
buffer_location().
In `@crates/embers-server/src/state.rs`:
- Around line 1442-1449: The join_buffer_at_node() path currently calls
close_node(source_view) while the destination placement can still fail (see
BufferAttachment::Attached check and node_is_descendant_of usage), which can
leave the buffer detached if a later step errors; modify join_buffer_at_node()
to make the operation transactional by deferring teardown of source_view until
the destination commit succeeds (i.e., create new_view and perform all
validations/placements first), and only then call close_node(source_view); if
any later step fails, restore or leave the original attachment intact (or
explicitly reattach the original owner) so the buffer is not lost; apply the
same change to the similar block around lines 1517-1524 where new_view is
created.
---
Outside diff comments:
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 2180-2199: Add a regression test that verifies the new blink flag
is parsed and preserved: call parse_segment_options (or exercise ui.segment("x",
#{ blink: true })) with an options Map containing "blink": true, then assert the
returned StyleSpec has blink == true (and no unknown options remain) so the
blink flag is covered by tests and cannot regress; reference
parse_segment_options and StyleSpec (or the ui.segment API) when adding the
assertion.
---
Duplicate comments:
In `@crates/embers-cli/src/lib.rs`:
- Around line 516-538: The History branch currently only rejects
BufferLocationAttachment::Detached but must also verify the returned attachment
matches the requested placement; after calling expect_buffer_location and before
returning, check the original placement argument (via
history_placement(placement) or the raw placement variable) and if the requested
placement is floating ensure location.attachment is
BufferLocationAttachment::Floating (and similarly validate tab/session cases if
applicable); if it does not match, return a MuxError::protocol with a clear
message like "buffer history returned unexpected attachment for buffer
{buffer_id}: expected Floating" so the CLI fails non-zero instead of reporting
success for the wrong placement.
In `@crates/embers-cli/tests/interactive.rs`:
- Around line 11-19: The file defines a file-local test_lock() using
OnceLock<Mutex<()>> which only serializes within this binary; replace that
helper with the shared test lock by removing test_lock() and using the
repo-provided acquire_test_lock() helper instead wherever
test_lock().lock().await is called (preserve awaiting the returned guard).
Update imports to remove OnceLock and local Mutex if unused and import or
reference acquire_test_lock() from the embers_test_support helper so the
integration test coordinates with sibling PTY tests.
In `@crates/embers-client/src/presentation.rs`:
- Around line 119-120: The projector call is passing is_root = false even when
the zoomed node is the session root, which prevents PresentationModel::root_tabs
from being populated; change the call site that currently does
projector.project_node(zoomed_node_id, root_bounds, None, false, Vec::new()) so
that the is_root argument is true when session.zoomed_node_id ==
session.root_node_id (e.g., compute a local is_root = zoomed_node_id ==
root_node_id and pass that boolean) so the zoomed-root path preserves root_tabs.
In `@crates/embers-client/tests/e2e.rs`:
- Around line 1165-1171: The test currently uses the client's cached state
(client.state().snapshots / latest_rendered_line) as the oracle which can be
stale; instead capture and use the server snapshot returned by
wait_for_visible_snapshot (the call that takes &mut connection, buffer.id,
Duration::from_secs(3), ...) as the authoritative snapshot for assertions and
rendering checks, and replace any uses of client.state().snapshots or
latest_rendered_line in this block and the similar section covering lines
1173-1205 with the returned snapshot value so the test validates against the
server snapshot the Renderer is supposed to render from.
- Around line 973-987: After selecting the hidden tab and waiting for
server-side activity to become Idle, also assert the client's rendered state for
that buffer no longer has the Bell indicator: after the existing call to
wait_for_buffer_activity(&mut connection, fixture.hidden_buffer.id,
ActivityState::Idle, ...).await, make an additional request via the same
connection (e.g. a ClientMessage query that fetches the client's view of the
buffer using fixture.hidden_buffer.id) and assert the returned buffer/render
metadata does not have the Bell flag set; place this assertion immediately after
wait_for_buffer_activity so the test fails if the client keeps rendering Bell
after the tab is revealed.
In `@crates/embers-protocol/schema/embers.fbs`:
- Around line 293-301: SessionRecord and the new BufferLocation* tables rely on
declaration-order field numbering, so pin each field with explicit id:
assignments to make the wire format stable; update SessionRecord by adding
explicit ids (e.g. id:ulong -> id:1, name:string -> id:2, root_node_id:3,
floating_ids:4, focused_leaf_id:5, focused_floating_id:6, zoomed_node_id:7) and
make the analogous explicit id: assignments for every field in the
BufferLocation* tables so future inserts or reorders won't change on-the-wire
numbers.
In `@crates/embers-server/src/server.rs`:
- Around line 2280-2303: The response currently echoes the raw request
start_line even when you clamp it to the helper buffer; locate the helper_slice
logic (match on BufferKind::Helper in the block that builds helper_slice) and
return the clamped origin instead of start_line when constructing
ScrollbackSliceResponse: convert the local clamped start (the usize variable
computed in that block) to the response type and use that for the response's
start_line/origin so total_lines and start align with the actual slice returned.
- Around line 1925-1938: The code implicitly selects the last attached client
when the caller is detached, which can cause wrong session switching; instead,
after confirming the caller is not attached (the existing
clients.get(...).is_some_and(...) branch), scan self.clients for entries where
client.current_session_id.is_some(), and if exactly one match return that
client_id, if zero return the existing conflict error, and if more than one
return a distinct conflict/error indicating ambiguous implicit client selection
and require an explicit client_id; update the block using
self.clients.iter().rev().find(...) to perform a count and single-match check so
ambiguity is detected (referencing self.clients, connection_id, and
client.current_session_id).
In `@crates/embers-server/tests/model_state.rs`:
- Around line 756-780: Add a final call to state.validate() at the end of the
rollback test to assert internal graph consistency; locate the test assertions
that check session, node, parent, and floating indices (references:
state.session(...), state.node(...), state.node_parent(...),
state.floating_id_for_node(...)) and append state.validate() as the last
assertion so the rollback contract is fully exercised.
🪄 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: f5ecc48f-58e9-4db2-9da0-ff66ac29a381
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (62)
Cargo.tomlcrates/embers-cli/Cargo.tomlcrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/presentation.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/presentation.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/client.rscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-server/src/persist.rscrates/embers-server/src/protocol.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/model_state.rscrates/embers-test-support/Cargo.tomlcrates/embers-test-support/src/pty.rsdocs/activity-bell-policy.mddocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-deaec7e4.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/action.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/registration-action.mddocs/config-api/registration-ui.mddocs/config-api/ui.md
Comment on lines
+308
to
+310
| fn rewrite_generated_markdown(content: &str) -> String { | ||
| rewrite_signature_literals(content) | ||
| } |
There was a problem hiding this comment.
Normalize generated tab buttons to type="button".
rewrite_generated_markdown() only fixes signatures right now, so the generated API pages still emit bare <button> tags. That keeps the HTMLHint warnings in this PR and will turn these controls into submit buttons if the content is ever rendered inside a form. This helper is the right place to patch the generated markup once at the source.
🛠️ Suggested fix
fn rewrite_generated_markdown(content: &str) -> String {
- rewrite_signature_literals(content)
+ rewrite_signature_literals(content)
+ .replace("<button group=", "<button type=\"button\" group=")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn rewrite_generated_markdown(content: &str) -> String { | |
| rewrite_signature_literals(content) | |
| } | |
| fn rewrite_generated_markdown(content: &str) -> String { | |
| rewrite_signature_literals(content) | |
| .replace("<button group=", "<button type=\"button\" group=") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-client/src/scripting/documentation.rs` around lines 308 - 310,
rewrite_generated_markdown currently only calls rewrite_signature_literals and
leaves raw <button> tags untyped; update it to also normalize generated tab
buttons to type="button" by post-processing the content (either inline or via a
new helper) after rewrite_signature_literals returns. Specifically, ensure
rewrite_generated_markdown (and/or a new function referenced there) finds
<button ...> tags that lack a type attribute and injects type="button" so tab
controls are not treated as submit buttons; reference rewrite_generated_markdown
and rewrite_signature_literals when making the change.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/embers-server/src/protocol.rs (1)
14-23: 🛠️ Refactor suggestion | 🟠 MajorKeep
zoomed_node_idconsistent with the serialized node set.
session_record()now exportszoomed_node_id, but the snapshot path only validatesroot_node. If a later tree mutation leavessession.zoomed_nodestale, clients get a session record that points at a node missing from the accompanyingnodeslist.🧩 Minimal guard in
session_snapshot()if !nodes.iter().any(|node| node.id == session.root_node) { return Err(MuxError::conflict(format!( "session snapshot for {} is missing its root node {}", session_id, session.root_node ))); } + if let Some(zoomed_node_id) = session.zoomed_node { + if !nodes.iter().any(|node| node.id == zoomed_node_id) { + return Err(MuxError::conflict(format!( + "session snapshot for {} is missing its zoomed node {}", + session_id, zoomed_node_id + ))); + } + } Ok(SessionSnapshot {
♻️ Duplicate comments (4)
crates/embers-client/src/scripting/documentation.rs (1)
308-310:⚠️ Potential issue | 🟡 MinorRe-add button normalization in generated markdown.
rewrite_generated_markdown()still only rewrites signatures, so the newly generated action pages in this diff keep emitting bare<button>tags. The test at Lines 709-716 never includes button markup either, so this regression passes unnoticed.🛠️ Minimal fix
fn rewrite_generated_markdown(content: &str) -> String { - rewrite_signature_literals(content) + rewrite_signature_literals(content) + .replace("<button group=", "<button type=\"button\" group=") }#[test] fn rewrite_generated_markdown_updates_button_markup_and_leaves_correct_input_unchanged() { - let input = "fn break_current_node(_: ActionApi, destination: String) -> Action"; - let expected = - "fn break_current_node(_: ActionApi, destination: \"tab\" | \"floating\") -> Action"; + let input = concat!( + "<button group=\"break_current_node\">", + "\nfn break_current_node(_: ActionApi, destination: String) -> Action", + ); + let expected = concat!( + "<button type=\"button\" group=\"break_current_node\">", + "\nfn break_current_node(_: ActionApi, destination: \"tab\" | \"floating\") -> Action", + );Also applies to: 709-716
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/scripting/documentation.rs` around lines 308 - 310, rewrite_generated_markdown currently only calls rewrite_signature_literals, so generated pages still contain raw <button> tags; update rewrite_generated_markdown to also run the button-normalization pass (e.g., call the existing button-normalizer function — such as rewrite_button_literals, normalize_buttons, or whatever helper reworks <button> markup — and compose it with rewrite_signature_literals) so that both signatures and button tags are rewritten before returning the String.crates/embers-cli/tests/interactive.rs (1)
53-68:⚠️ Potential issue | 🟠 MajorPreserve the matched PID through orphan cleanup.
try_signal_server()throws away the PID it actually matched, andkill_orphaned_server()then re-reads the pid file before waiting/removing it. If that file flips between those steps, this guard can wait on or delete the wrong server's pid file. Return the matched PID fromtry_signal_server()and only remove the pid file when it still points at that same PID.Suggested direction
fn kill_orphaned_server(socket_path: &Path) { let pid_path = socket_path.with_extension("pid"); - let term_sent = try_signal_server(&pid_path, socket_path, libc::SIGTERM); - if term_sent && read_pid(&pid_path).is_some_and(|pid| wait_for_server_exit(pid, socket_path)) { - let _ = fs::remove_file(&pid_path); - return; + if let Some(pid) = try_signal_server(&pid_path, socket_path, libc::SIGTERM) { + if wait_for_server_exit(pid, socket_path) + && match read_pid(&pid_path) { + None => true, + Some(current) => current == pid, + } + { + let _ = fs::remove_file(&pid_path); + return; + } } std::thread::sleep(Duration::from_millis(50)); - let kill_sent = try_signal_server(&pid_path, socket_path, libc::SIGKILL); - if kill_sent && read_pid(&pid_path).is_some_and(|pid| wait_for_server_exit(pid, socket_path)) { - let _ = fs::remove_file(&pid_path); + if let Some(pid) = try_signal_server(&pid_path, socket_path, libc::SIGKILL) { + if wait_for_server_exit(pid, socket_path) + && match read_pid(&pid_path) { + None => true, + Some(current) => current == pid, + } + { + let _ = fs::remove_file(&pid_path); + } } } -fn try_signal_server(pid_path: &Path, socket_path: &Path, signal: i32) -> bool { +fn try_signal_server(pid_path: &Path, socket_path: &Path, signal: i32) -> Option<i32> { let Some(pid) = read_pid(pid_path) else { - return false; + return None; }; if !pid_matches_serve_process(pid, socket_path) { - return false; + return None; } // SAFETY: pid was read from our pid file and verified against the active __serve command line. - unsafe { libc::kill(pid, signal) }; - true + let _ = unsafe { libc::kill(pid, signal) }; + Some(pid) }Also applies to: 71-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-cli/tests/interactive.rs` around lines 53 - 68, try_signal_server currently discards the PID it matched; change it to return the matched PID (e.g., Option<u32>/Option<usize>) and update kill_orphaned_server to use that returned PID instead of re-reading the pid file: call try_signal_server(socket_path) to get matched_pid, then if matched_pid.is_some() and wait_for_server_exit(matched_pid, socket_path) succeeds, remove the pid file only after verifying the file still contains the same matched_pid (read_pid and compare to matched_pid before fs::remove_file). Apply the same change to the other cleanup block that also calls try_signal_server so removal always targets the exact PID that was signaled.crates/embers-client/src/presentation.rs (1)
230-243: 🧹 Nitpick | 🔵 TrivialAdd coverage for a zoomed descendant under a floating root.
floating_ancestor_for_node()is the new behavior here, but the added tests only cover a non-floating target, a hidden floating root, and the session root. A floating split/tabs descendant case can still regress without failing anything; add a test that zooms a child inside a floating subtree and asserts the projection keeps the floating frame andfocused_floating_id().Also applies to: 688-958
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/presentation.rs` around lines 230 - 243, Tests for floating subtree zooming are missing and allow regressions: add a unit/integration test that creates a floating root (FloatingRecord), a descendant split/tab inside that floating frame, then zooms the descendant node and asserts the projection still includes the floating frame and that focused_floating_id() returns the floating root's id; update or add the test near other projection/floating tests referencing floating_ancestor_for_node, focused_floating_id, projection, and FloatingRecord so the new case (zoomed descendant under a floating root) is covered.crates/embers-client/src/configured_client.rs (1)
333-345:⚠️ Potential issue | 🔴 CriticalDon't time out
next_event()directly.Line 337 cancels the in-flight receive when the timeout elapses. If
MuxClient::next_event()still drives aread_exact()-style frame read, that can desynchronize the socket and leave the client permanently broken even though this method returnsOk(None). Move the timeout to a channel receive / background reader, or otherwise make the underlying read cancel-safe.Verify by tracing
next_event()to the framing reads. Expected result: if the receive path still reachesread_exact(), this timeout wrapper is unsafe.#!/bin/bash set -euo pipefail echo "=== ConfiguredClient timeout wrapper ===" sed -n '333,345p' crates/embers-client/src/configured_client.rs echo echo "=== MuxClient event-read entry points ===" rg -n -C4 '\b(async\s+fn|fn)\s+(next_event|process_next_event_timeout|process_next_event)\b' crates/embers-client echo echo "=== Frame decoding reads ===" rg -n -C3 '\bread_exact\s*\(' crates/embers-client crates/embers-protocol🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/configured_client.rs` around lines 333 - 345, process_next_event_timeout currently wraps MuxClient::next_event() in tokio::time::timeout which cancels the in-flight read and can desynchronize the socket if next_event()/MuxClient::next_event() uses read_exact; instead make the timeout apply to a cancellation-safe channel receive by moving socket/frame reads into a dedicated background reader that pushes complete ServerEvent values into an mpsc/unbounded channel, have process_next_event_timeout await the channel recv with tokio::time::timeout, and ensure the background reader handles IO errors and frame parsing (read_exact) and shuts down the channel cleanly; update references to process_next_event_timeout, MuxClient::next_event, and any read_exact usage so the socket read is never directly timeout-cancelled.
🤖 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-client/src/presentation.rs`:
- Around line 850-866: The test currently only checks presentation.floating and
focused_floating_id so it would still pass if project() fell back to the session
root; update the hidden-floating zoom test that calls PresentationModel::project
to also assert that no leaves and no root tabs were projected by adding explicit
checks such as asserting presentation.leaves.is_empty() and asserting the
presentation field that represents root tabs is empty (e.g.,
presentation.root_tabs.is_empty()) so the test fails if project() falls back to
the root.
In `@crates/embers-client/tests/e2e.rs`:
- Around line 828-860: Extract the repeated polling/resync sequence into a
helper (e.g., poll_for_tab_activity) that takes a mutable client reference,
session name (used with session_id_by_name), target tab title, expected
ActivityState, timeout and poll interval; inside the helper call
first_client.resync_all_sessions().await, refresh_all_snapshots(&mut
client).await, build the PresentationModel via
PresentationModel::project(client.state(), session_id, Size{...}), locate the
tab bar with fixture.nested_tabs_id and check tabs.iter().any(|tab| tab.title ==
target && tab.activity == expected); return a bool or Result indicating
success/failure and use that helper to replace the duplicated loops (the blocks
that set saw_hidden_activity and break). Ensure the helper mirrors the current
timeout (Duration::from_secs(2)) and sleep interval (50ms) and update call sites
(including the other ranges noted) to call this helper and assert on its result.
In `@crates/embers-protocol/src/types.rs`:
- Around line 220-223: BufferWithLocationResponse currently allows two
independent BufferId sources (buffer.id and location.buffer_id) which can
disagree; fix by either (A) removing the redundant BufferId and store only
BufferLocationAttachment in BufferWithLocationResponse so there's a single
source of truth, or (B) add a checked constructor/factory (e.g., impl
BufferWithLocationResponse::new(buffer: Buffer, location: BufferLocation) ->
Result<...>) that verifies buffer.id == location.buffer_id and returns an error
if they mismatch; update usages to construct via that checked constructor and
apply the same change to the other occurrence noted (lines ~755-758) to ensure
consistency.
In `@crates/embers-server/src/server.rs`:
- Around line 2054-2063: The detached fast-path returns success before
validating an explicit Reveal target, so a Reveal with client_id: Some(x) can
silently succeed even if x doesn't exist; move or perform client resolution
prior to the Detached check (call or await
resolve_optional_reveal_client_id(connection_id, requested_client_id) before
inspecting BufferLocationAttachment::Detached), or at minimum if
requested_client_id.is_some() then resolve it and propagate NotFound on failure
so explicit targets are validated even for detached buffers; reference
buffer_location, BufferLocationAttachment::Detached,
resolve_optional_reveal_client_id, connection_id, and requested_client_id when
making the change.
- Around line 2179-2188: When focus_revealed_buffer(helper_buffer_id,
target_client_id) returns Err after the helper buffer/container has been
created, the current branch only best-effort detaches/removes the buffer and
ignores results, leaving the container open; instead, look up the helper
buffer's location from state (using helper_buffer_id), call the appropriate
rollback API (close_node or close_floating) to remove the created container,
propagate or log any rollback errors, and only after successful rollback
detach/remove the buffer (use state.detach_buffer and state.remove_buffer as
before) and then return the original error; make these changes inside the
Err(error) arm so rollback is performed deterministically and any rollback
failures are surfaced.
- Around line 2921-2924: The immutable borrow from state.buffers.get(&buffer_id)
conflicts with the mutable borrow required by state.set_buffer_activity(...);
fix by copying the buffer's activity into a local (e.g., let current_activity =
buffer.activity;) before calling set_buffer_activity, then call
state.set_buffer_activity(buffer_id, max_activity(current_activity,
status.activity)); reference symbols: state.buffers.get, buffer.activity,
buffer_id, max_activity, and set_buffer_activity.
In `@crates/embers-server/tests/model_state.rs`:
- Around line 423-467: Add a negative case to
swap_and_reorder_operate_only_on_siblings that attempts to call
swap_sibling_nodes and move_node_before with nodes that are not siblings and
assert they return Err; specifically, create or identify a node under a
different parent (e.g., the third_leaf's node vs a child of the root split) and
call state.swap_sibling_nodes(non_sibling, first_child) and
state.move_node_before(non_sibling, first_child) expecting Err results; ensure
you use the same helper symbols (swap_sibling_nodes, move_node_before,
create_buffer_view/wrap_node_in_split to construct the non-sibling) and leave
the existing happy-path assertions intact.
---
Duplicate comments:
In `@crates/embers-cli/tests/interactive.rs`:
- Around line 53-68: try_signal_server currently discards the PID it matched;
change it to return the matched PID (e.g., Option<u32>/Option<usize>) and update
kill_orphaned_server to use that returned PID instead of re-reading the pid
file: call try_signal_server(socket_path) to get matched_pid, then if
matched_pid.is_some() and wait_for_server_exit(matched_pid, socket_path)
succeeds, remove the pid file only after verifying the file still contains the
same matched_pid (read_pid and compare to matched_pid before fs::remove_file).
Apply the same change to the other cleanup block that also calls
try_signal_server so removal always targets the exact PID that was signaled.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 333-345: process_next_event_timeout currently wraps
MuxClient::next_event() in tokio::time::timeout which cancels the in-flight read
and can desynchronize the socket if next_event()/MuxClient::next_event() uses
read_exact; instead make the timeout apply to a cancellation-safe channel
receive by moving socket/frame reads into a dedicated background reader that
pushes complete ServerEvent values into an mpsc/unbounded channel, have
process_next_event_timeout await the channel recv with tokio::time::timeout, and
ensure the background reader handles IO errors and frame parsing (read_exact)
and shuts down the channel cleanly; update references to
process_next_event_timeout, MuxClient::next_event, and any read_exact usage so
the socket read is never directly timeout-cancelled.
In `@crates/embers-client/src/presentation.rs`:
- Around line 230-243: Tests for floating subtree zooming are missing and allow
regressions: add a unit/integration test that creates a floating root
(FloatingRecord), a descendant split/tab inside that floating frame, then zooms
the descendant node and asserts the projection still includes the floating frame
and that focused_floating_id() returns the floating root's id; update or add the
test near other projection/floating tests referencing
floating_ancestor_for_node, focused_floating_id, projection, and FloatingRecord
so the new case (zoomed descendant under a floating root) is covered.
In `@crates/embers-client/src/scripting/documentation.rs`:
- Around line 308-310: rewrite_generated_markdown currently only calls
rewrite_signature_literals, so generated pages still contain raw <button> tags;
update rewrite_generated_markdown to also run the button-normalization pass
(e.g., call the existing button-normalizer function — such as
rewrite_button_literals, normalize_buttons, or whatever helper reworks <button>
markup — and compose it with rewrite_signature_literals) so that both signatures
and button tags are rewritten before returning the String.
🪄 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: 366929ee-789f-47a8-bd7b-839e6bb83116
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (62)
Cargo.tomlcrates/embers-cli/Cargo.tomlcrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/presentation.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/presentation.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/client.rscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-server/src/persist.rscrates/embers-server/src/protocol.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/model_state.rscrates/embers-test-support/Cargo.tomlcrates/embers-test-support/src/pty.rsdocs/activity-bell-policy.mddocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-deaec7e4.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/action.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/registration-action.mddocs/config-api/registration-ui.mddocs/config-api/ui.md
Comment on lines
+850
to
+866
| let presentation = PresentationModel::project( | ||
| &state, | ||
| SessionId(1), | ||
| Size { | ||
| width: 80, | ||
| height: 24, | ||
| }, | ||
| ) | ||
| .expect("project hidden zoomed floating"); | ||
| assert!(presentation.floating.is_empty()); | ||
| assert!( | ||
| presentation | ||
| .leaves | ||
| .iter() | ||
| .all(|leaf| leaf.floating_id.is_none()) | ||
| ); | ||
| assert_eq!(presentation.focused_floating_id(), None); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make the hidden-floating zoom test prove the fallback never renders root content.
As written, this still passes if project() falls back to the session root: presentation.floating.is_empty() and focused_floating_id() == None both remain true in that failure mode. Add an explicit “no leaves / no root tabs” assertion so the test actually pins the no-projection behavior.
Suggested strengthening
)
.expect("project hidden zoomed floating");
assert!(presentation.floating.is_empty());
- assert!(
- presentation
- .leaves
- .iter()
- .all(|leaf| leaf.floating_id.is_none())
- );
+ assert!(presentation.leaves.is_empty());
+ assert!(presentation.root_tabs.is_none());
assert_eq!(presentation.focused_floating_id(), None);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let presentation = PresentationModel::project( | |
| &state, | |
| SessionId(1), | |
| Size { | |
| width: 80, | |
| height: 24, | |
| }, | |
| ) | |
| .expect("project hidden zoomed floating"); | |
| assert!(presentation.floating.is_empty()); | |
| assert!( | |
| presentation | |
| .leaves | |
| .iter() | |
| .all(|leaf| leaf.floating_id.is_none()) | |
| ); | |
| assert_eq!(presentation.focused_floating_id(), None); | |
| let presentation = PresentationModel::project( | |
| &state, | |
| SessionId(1), | |
| Size { | |
| width: 80, | |
| height: 24, | |
| }, | |
| ) | |
| .expect("project hidden zoomed floating"); | |
| assert!(presentation.floating.is_empty()); | |
| assert!(presentation.leaves.is_empty()); | |
| assert!(presentation.root_tabs.is_none()); | |
| assert_eq!(presentation.focused_floating_id(), None); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-client/src/presentation.rs` around lines 850 - 866, The test
currently only checks presentation.floating and focused_floating_id so it would
still pass if project() fell back to the session root; update the
hidden-floating zoom test that calls PresentationModel::project to also assert
that no leaves and no root tabs were projected by adding explicit checks such as
asserting presentation.leaves.is_empty() and asserting the presentation field
that represents root tabs is empty (e.g., presentation.root_tabs.is_empty()) so
the test fails if project() falls back to the root.
Comment on lines
+828
to
+860
| let mut saw_hidden_activity = false; | ||
| let deadline = Instant::now() + Duration::from_secs(2); | ||
| while Instant::now() < deadline { | ||
| first_client | ||
| .resync_all_sessions() | ||
| .await | ||
| .expect("first client resyncs"); | ||
| refresh_all_snapshots(&mut first_client).await; | ||
| let session_id = session_id_by_name(&first_client, "alpha"); | ||
| let model = PresentationModel::project( | ||
| first_client.state(), | ||
| session_id, | ||
| Size { | ||
| width: 80, | ||
| height: 24, | ||
| }, | ||
| ) | ||
| .expect("projection succeeds"); | ||
| let tabs = model | ||
| .tab_bars | ||
| .iter() | ||
| .find(|tabs| tabs.node_id == fixture.nested_tabs_id) | ||
| .expect("nested tabs frame exists"); | ||
| if tabs | ||
| .tabs | ||
| .iter() | ||
| .any(|tab| tab.title == "bg" && tab.activity == ActivityState::Activity) | ||
| { | ||
| saw_hidden_activity = true; | ||
| break; | ||
| } | ||
| tokio::time::sleep(Duration::from_millis(50)).await; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the tab-activity polling helper.
These loops all repeat the same resync → snapshot refresh → PresentationModel::project → nested-tab scan sequence. Centralizing that logic would make the reconnect/activity tests easier to keep aligned when the polling budget or projection rules change.
Also applies to: 935-967, 988-1020
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-client/tests/e2e.rs` around lines 828 - 860, Extract the
repeated polling/resync sequence into a helper (e.g., poll_for_tab_activity)
that takes a mutable client reference, session name (used with
session_id_by_name), target tab title, expected ActivityState, timeout and poll
interval; inside the helper call first_client.resync_all_sessions().await,
refresh_all_snapshots(&mut client).await, build the PresentationModel via
PresentationModel::project(client.state(), session_id, Size{...}), locate the
tab bar with fixture.nested_tabs_id and check tabs.iter().any(|tab| tab.title ==
target && tab.activity == expected); return a bool or Result indicating
success/failure and use that helper to replace the duplicated loops (the blocks
that set saw_hidden_activity and break). Ensure the helper mirrors the current
timeout (Duration::from_secs(2)) and sleep interval (50ms) and update call sites
(including the other ranges noted) to call this helper and assert on its result.
Comment on lines
+220
to
+223
| pub struct BufferLocation { | ||
| pub buffer_id: BufferId, | ||
| pub attachment: BufferLocationAttachment, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid two independent buffer ids in BufferWithLocationResponse.
buffer.id and location.buffer_id can disagree, so this new response shape admits inconsistent payloads. Prefer storing only BufferLocationAttachment here, or funnel construction through a checked constructor that rejects mismatched ids.
Also applies to: 755-758
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-protocol/src/types.rs` around lines 220 - 223,
BufferWithLocationResponse currently allows two independent BufferId sources
(buffer.id and location.buffer_id) which can disagree; fix by either (A)
removing the redundant BufferId and store only BufferLocationAttachment in
BufferWithLocationResponse so there's a single source of truth, or (B) add a
checked constructor/factory (e.g., impl BufferWithLocationResponse::new(buffer:
Buffer, location: BufferLocation) -> Result<...>) that verifies buffer.id ==
location.buffer_id and returns an error if they mismatch; update usages to
construct via that checked constructor and apply the same change to the other
occurrence noted (lines ~755-758) to ensure consistency.
Comment on lines
+2054
to
+2063
| let location = { | ||
| let state = self.state.lock().await; | ||
| buffer_location(&state, buffer_id)? | ||
| }; | ||
| if matches!(location.attachment, BufferLocationAttachment::Detached) { | ||
| return Ok((location, Vec::new())); | ||
| } | ||
| let client_id = self | ||
| .resolve_optional_reveal_client_id(connection_id, requested_client_id) | ||
| .await?; |
There was a problem hiding this comment.
Validate explicit client_ids before the detached fast-path.
This returns success for detached buffers before any client resolution happens, so Reveal { client_id: Some(x) } silently succeeds even when x does not exist. Explicit targets should still fail with NotFound here instead of being ignored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-server/src/server.rs` around lines 2054 - 2063, The detached
fast-path returns success before validating an explicit Reveal target, so a
Reveal with client_id: Some(x) can silently succeed even if x doesn't exist;
move or perform client resolution prior to the Detached check (call or await
resolve_optional_reveal_client_id(connection_id, requested_client_id) before
inspecting BufferLocationAttachment::Detached), or at minimum if
requested_client_id.is_some() then resolve it and propagate NotFound on failure
so explicit targets are validated even for detached buffers; reference
buffer_location, BufferLocationAttachment::Detached,
resolve_optional_reveal_client_id, connection_id, and requested_client_id when
making the change.
Comment on lines
+2179
to
+2188
| match self | ||
| .focus_revealed_buffer(helper_buffer_id, target_client_id) | ||
| .await | ||
| { | ||
| Ok(result) => Ok(result), | ||
| Err(error) => { | ||
| let mut state = self.state.lock().await; | ||
| let _ = state.detach_buffer(helper_buffer_id); | ||
| let _ = state.remove_buffer(helper_buffer_id); | ||
| Err(error) |
There was a problem hiding this comment.
Rollback doesn't undo the newly created tab/floating.
focus_revealed_buffer() can still fail after the helper has been inserted, e.g. if the target client disconnects between client resolution and this call. This branch only best-effort detaches/removes the helper buffer and ignores both results, which does not remove the container that was just created. Roll back via the helper's location (close_node / close_floating) and surface rollback failures.
🛠 Minimal rollback direction
match self
.focus_revealed_buffer(helper_buffer_id, target_client_id)
.await
{
Ok(result) => Ok(result),
Err(error) => {
let mut state = self.state.lock().await;
- let _ = state.detach_buffer(helper_buffer_id);
- let _ = state.remove_buffer(helper_buffer_id);
+ match buffer_location(&state, helper_buffer_id)
+ .ok()
+ .map(|location| location.attachment)
+ {
+ Some(BufferLocationAttachment::Floating { floating_id, .. }) => {
+ let _ = state.close_floating(floating_id);
+ }
+ Some(BufferLocationAttachment::Session { node_id, .. }) => {
+ let _ = state.close_node(node_id);
+ }
+ _ => {
+ let _ = state.remove_buffer(helper_buffer_id);
+ }
+ }
Err(error)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-server/src/server.rs` around lines 2179 - 2188, When
focus_revealed_buffer(helper_buffer_id, target_client_id) returns Err after the
helper buffer/container has been created, the current branch only best-effort
detaches/removes the buffer and ignores results, leaving the container open;
instead, look up the helper buffer's location from state (using
helper_buffer_id), call the appropriate rollback API (close_node or
close_floating) to remove the created container, propagate or log any rollback
errors, and only after successful rollback detach/remove the buffer (use
state.detach_buffer and state.remove_buffer as before) and then return the
original error; make these changes inside the Err(error) arm so rollback is
performed deterministically and any rollback failures are surfaced.
Comment on lines
+2921
to
+2924
| if let Some(buffer) = state.buffers.get(&buffer_id) { | ||
| let _ = | ||
| state.set_buffer_activity(buffer_id, max_activity(buffer.activity, status.activity)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the borrow site and the mutable setter used there.
sed -n '2918,2925p' crates/embers-server/src/server.rs
rg -n -C2 '\bfn set_buffer_activity\b' cratesRepository: Pajn/Embers
Length of output: 646
🏁 Script executed:
# Find the state struct definition and the buffers field type
rg -n 'struct.*State' crates/embers-server/src/state.rs | head -5
# Look at the buffers field definition
rg -n -A20 'struct.*State\s*\{' crates/embers-server/src/state.rs | head -40Repository: Pajn/Embers
Length of output: 706
🏁 Script executed:
# Also check the exact context around the issue
sed -n '2915,2930p' crates/embers-server/src/server.rsRepository: Pajn/Embers
Length of output: 647
🏁 Script executed:
# Check if the code currently compiles or if there are compilation errors
# First, let's check if there's a Cargo.toml and try to understand the project structure
ls -la crates/embers-server/ | head -20Repository: Pajn/Embers
Length of output: 328
🏁 Script executed:
# Let's also check the exact method definition to understand the receiver type
sed -n '480,490p' crates/embers-server/src/state.rsRepository: Pajn/Embers
Length of output: 318
🏁 Script executed:
# Check git history to see if this code was modified recently or if there are known issues
git log --oneline -20 -- crates/embers-server/src/server.rs 2>/dev/null | head -10
git log --oneline -20 -- crates/embers-server/src/state.rs 2>/dev/null | head -10Repository: Pajn/Embers
Length of output: 96
🏁 Script executed:
# Try to compile the crate to see if there's actually a borrow error
cd crates/embers-server && cargo check 2>&1 | head -100Repository: Pajn/Embers
Length of output: 301
🏁 Script executed:
# Also check if there are any recent changes or if this is already fixed
git show HEAD:crates/embers-server/src/server.rs 2>/dev/null | sed -n '2918,2930p'Repository: Pajn/Embers
Length of output: 527
Copy the current activity before calling the setter.
The immutable borrow from state.buffers.get(&buffer_id) extends through the entire if block, conflicting with the &mut self borrow required by set_buffer_activity(). Extract the activity value before the call:
Fix
- if let Some(buffer) = state.buffers.get(&buffer_id) {
- let _ =
- state.set_buffer_activity(buffer_id, max_activity(buffer.activity, status.activity));
+ if let Some(current_activity) = state.buffers.get(&buffer_id).map(|buffer| buffer.activity) {
+ let _ =
+ state.set_buffer_activity(buffer_id, max_activity(current_activity, status.activity));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-server/src/server.rs` around lines 2921 - 2924, The immutable
borrow from state.buffers.get(&buffer_id) conflicts with the mutable borrow
required by state.set_buffer_activity(...); fix by copying the buffer's activity
into a local (e.g., let current_activity = buffer.activity;) before calling
set_buffer_activity, then call state.set_buffer_activity(buffer_id,
max_activity(current_activity, status.activity)); reference symbols:
state.buffers.get, buffer.activity, buffer_id, max_activity, and
set_buffer_activity.
Comment on lines
+423
to
+467
| #[test] | ||
| fn swap_and_reorder_operate_only_on_siblings() { | ||
| let mut state = ServerState::new(); | ||
| let (session_id, _, leaf_id) = seed_single_leaf_session(&mut state, "alpha"); | ||
| let second = new_buffer(&mut state, "second"); | ||
| let third = new_buffer(&mut state, "third"); | ||
| let split_id = state | ||
| .split_leaf_with_new_buffer(leaf_id, SplitDirection::Vertical, second) | ||
| .expect("split root"); | ||
| let _second_leaf = attached_view(&state, second); | ||
| let third_leaf = state | ||
| .create_buffer_view(session_id, third) | ||
| .expect("third leaf"); | ||
| state | ||
| .wrap_node_in_split(split_id, SplitDirection::Vertical, third_leaf, false) | ||
| .expect("wrap split with third leaf"); | ||
|
|
||
| let root = root_tab_child(&state, session_id, 0); | ||
| let split = match state.node(root).expect("root split") { | ||
| Node::Split(split) => split.clone(), | ||
| other => panic!("expected split, got {other:?}"), | ||
| }; | ||
| let first_child = split.children[0]; | ||
| let second_child = split.children[1]; | ||
|
|
||
| state | ||
| .swap_sibling_nodes(first_child, second_child) | ||
| .expect("swap siblings"); | ||
| let swapped_children = match state.node(root).expect("root split") { | ||
| Node::Split(split) => split.children.clone(), | ||
| other => panic!("expected split after swap, got {other:?}"), | ||
| }; | ||
| assert_eq!(swapped_children[0], second_child); | ||
| assert_eq!(swapped_children[1], first_child); | ||
| state | ||
| .move_node_before(first_child, second_child) | ||
| .expect("move sibling before"); | ||
| let restored_children = match state.node(root).expect("root split") { | ||
| Node::Split(split) => split.children.clone(), | ||
| other => panic!("expected split after reorder, got {other:?}"), | ||
| }; | ||
| assert_eq!(restored_children[0], first_child); | ||
| assert_eq!(restored_children[1], second_child); | ||
| state.validate().expect("state remains valid"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The “only on siblings” guarantee still isn't exercised.
This proves the sibling happy path, but a regression that starts accepting nodes from different parents would still pass. Add one negative case here with a non-sibling node and assert swap_sibling_nodes / move_node_before return an error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-server/tests/model_state.rs` around lines 423 - 467, Add a
negative case to swap_and_reorder_operate_only_on_siblings that attempts to call
swap_sibling_nodes and move_node_before with nodes that are not siblings and
assert they return Err; specifically, create or identify a node under a
different parent (e.g., the third_leaf's node vs a child of the root split) and
call state.swap_sibling_nodes(non_sibling, first_child) and
state.move_node_before(non_sibling, first_child) expecting Err results; ensure
you use the same helper symbols (swap_sibling_nodes, move_node_before,
create_buffer_view/wrap_node_in_split to construct the non-sibling) and leave
the existing happy-path assertions intact.
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 by CodeRabbit
New Features
Improvements
Documentation
Stack
api-docs Add api docs for rhai config #2(merged)persistance Persist workspace state across clean restarts #3(merged)buffer-cli Add native detached buffer CLI workflows #9(merged)session-rename Add native session rename support #5(merged)