Conversation
Add the authoritative session, buffer, node, floating-window, and ServerState model with normalization, focus healing, validation helpers, and structural/property tests that lock in the Phase 1 invariants. 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>
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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…@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 tab bar formatter execution, atomic config reload, and a configured client runtime that applies scripted bindings, event hooks, renderer overrides, and non-fatal runtime notifications. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add reload swap tests, formatter runtime tests, and configured client integration coverage for live bindings, event hooks, formatter fallback, reload, and non-fatal script failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Make embers start or reuse a server, bootstrap a usable interactive client, add attach, and inject EMBERS_SOCKET into server-spawned panes so nested CLI commands target the current server automatically. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add PTY-backed coverage for default embers startup and attach, plus test-helper updates so user-facing CLI tests execute through the embers binary path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (22)
📝 WalkthroughWalkthroughRename workspace crates to embers-*, add a full EmBers stack (CLI, client, protocol, server), introduce a FlatBuffers schema and CI installer for flatc, implement Rhai-based configuration/scripting, add presentation/renderer/input/controller, socket transport and framing, a Tokio Unix-socket server with PTY runtime and Alacritty backend, and many tests/benchmarks. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (embers)"
participant ST as "SocketTransport"
participant PC as "ProtocolClient"
participant Server as "Embers Server"
participant State as "ServerState"
participant Backend as "TerminalBackend"
participant PTY as "PTY/Process"
CLI->>ST: send(ClientMessage)
ST->>PC: write_frame(payload)
PC->>Server: deliver frame / decode message
Server->>State: apply operation (create/split/attach/...)
State-->>Server: events / snapshot updates
Server->>Backend: route input / spawn buffer runtime / resize
Backend->>PTY: write bytes / resize
PTY-->>Backend: stdout/stderr output
Backend->>Server: TerminalSnapshot / damage
Server->>PC: encode_server_envelope(response/event)
PC->>ST: write_frame(response/event)
ST->>CLI: deliver(ServerResponse / ServerEvent)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 45
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/testing.rs (1)
176-183: 🧹 Nitpick | 🔵 TrivialConsider updating test string from "mux" to "embers" for consistency.
The test still uses
"mux"as test data. While functionally correct, updating to"embers"would maintain consistency with the crate rename.📝 Optional: Update test data to match new naming
#[test] fn test_grid_renders_rows() { let mut grid = TestGrid::new(6, 2); - grid.put_str(1, 0, "mux"); + grid.put_str(1, 0, "emb"); grid.put_str(0, 1, "ok"); - assert_eq!(grid.render(), " mux \nok "); + assert_eq!(grid.render(), " emb \nok "); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/testing.rs` around lines 176 - 183, Update the test test_grid_renders_rows to use the new crate name by replacing the string "mux" with "embers" in the calls to TestGrid::put_str, and update the expected rendered string passed to assert_eq!(grid.render(), ...) accordingly so it matches the new content produced by grid.render() after putting "embers" into the grid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 15-28: Extract the repeated flatc install script (the curl loop
that sets asset, the test -n "${asset}" check, unzip/chmod/install steps) into a
single reusable composite GitHub Action (e.g., an install-flatc action) and
replace the inlined blocks in the fmt-lint, test, and pty-smoke jobs with a
uses: reference to that action; inside the composite action, replace the silent
failure test -n "${asset}" with an explicit failure path that echoes a clear
error (including ${FLATBUFFERS_VERSION} and attempted asset names) and exits
non-zero so failures are visible.
In `@crates/embers-cli/src/bin/embers-cli.rs`:
- Line 7: The call init_tracing("info") hardcodes the log level; change it to
accept runtime configuration by reading a CLI flag or environment variable and
passing that value into init_tracing. Update main to parse a --verbose/-v or
--log (or check RUST_LOG/EMBERS_LOG) and normalize into a string (e.g.,
"info"/"debug") with a sensible default, then call init_tracing(log_level);
ensure the parsing code lives alongside existing argument parsing (or add
clap/structopt usage) so init_tracing is no longer given a fixed "info".
- Around line 10-12: The error print currently uses Display and hides the error
chain; update the error reporting in the main async block where run(cli).await
is handled so it prints the full debug/chain information (e.g., replace
eprintln!("{error}") with eprintln!("{error:?}") or use an anyhow-style
formatted chain) — change the eprintln call that logs the Err(error) from
run(cli).await to output the debug representation or formatted chain to surface
cause/context.
In `@crates/embers-cli/src/interactive.rs`:
- Around line 284-287: The stdin locking sequence (let stdin = io::stdin(); let
lock = stdin.lock(); let fd = lock.as_raw_fd(); let _lock = lock;) is subtle
because the FileDesc (fd) is only valid while the lock value is alive; add a
brief in-line comment above this block explaining that we intentionally keep the
lock binding alive to maintain a valid fd, and consider renaming the shadowed
binding from `_lock` to a clearer name like `_stdin_lock` to make the intent
explicit (referencing the stdin, lock, fd, and _lock bindings).
- Around line 281-306: The
thread::Builder::new().name("embers-input").spawn(...) result is currently
discarded; instead match on the spawn Result from spawn and handle Err
explicitly: if spawn returns Err, log the error and/or send a
TerminalEvent::InputError (using tx.send) or return an Err from the surrounding
function so the caller knows input thread failed; on Ok(handle) keep the handle
(or detach it) so the spawn cannot silently fail. Update the code around
thread::Builder::new() / spawn and the tx/TerminalEvent usage to ensure spawn
failures are propagated or reported rather than ignored.
In `@crates/embers-cli/src/lib.rs`:
- Around line 516-532: On the timeout branch where MuxError::timeout is
returned, ensure the spawned child server (variable child) is terminated first
to avoid leaving a stray server running: call child.kill() and await/handle its
termination (e.g., child.kill().await and then child.wait().await or handle Err
from kill) before returning the timeout error; keep the original error return
(MuxError::timeout) but perform the kill/wait cleanup so the server process
cannot later create the socket and race with subsequent invocations (references:
child, MuxError::timeout, server_is_available, socket_path).
- Around line 469-471: Replace the use of shared /tmp for the control socket and
pidfile by resolving a per-user runtime directory (check XDG_RUNTIME_DIR,
fallback to /run/user/{uid}, or create a private dir under /tmp with 0700), and
update default_socket_path() to join that runtime dir instead of "/tmp"; for
pidfile creation (the places noted near the pidfile logic) open the file with
no-follow/create-new semantics (use OpenOptions::create_new(true) and set
O_NOFOLLOW via std::os::unix::fs::OpenOptionsExt::custom_flags or equivalent)
and set restrictive file mode (0600) so attackers cannot pre-create or hijack
the pidfile. Ensure any socket file is created in that private runtime dir and
set permissions to owner-only so other local users cannot bind/impersonate.
In `@crates/embers-cli/tests/interactive.rs`:
- Around line 76-91: The test is racy because the server's socket may vanish
between harness.wait() and spawning the list-sessions CLI; modify the test to
wait/poll for the socket at socket_path to exist (with a short timeout) after
harness.wait() and before invoking cargo_bin("embers"). Specifically, after
harness.wait() (where harness.write_all("\x11") and harness.wait() are used) add
a small polling loop that checks for the socket_path file's existence (or
retries the cargo_bin call until success for a bounded time) so that
arg("list-sessions") runs only after the socket is present; alternatively
replace this sequence with the existing TestServer helper used in
attach_subcommand_connects_to_running_server if that better matches intended
server lifetime semantics.
In `@crates/embers-cli/tests/panes.rs`:
- Around line 97-111: The fixed sleep after calling run_cli("send-keys", ...,
other_pane_id) can cause flaky tests; replace the single
sleep(Duration::from_millis(100)).await with a polling loop that repeatedly
calls run_cli(&server, ["capture-pane", "-t", &other_pane_id.to_string()]) and
checks stdout(&captured).contains("cli-pane") until a deadline (e.g.
Instant::now() + Duration::from_secs(2)) is reached; on success break, and on
timeout panic with a clear message; include a short await sleep between polls
(e.g. 50ms) to avoid busy loop.
In `@crates/embers-client/src/config/loader.rs`:
- Around line 84-102: Replace the panic-causing expect() in the origin arm with
defensive handling: change let path = discovered.path.clone().expect(...) to an
if let Some(path) = discovered.path.clone() { ... } else { return
Err(ConfigError::MissingPath { origin }); } (or return an appropriate
ConfigError variant if you prefer), then proceed to call fs::read_to_string,
source_hash, and construct LoadedConfigSource as before; add the new
ConfigError::MissingPath variant to the ConfigError enum (or reuse an existing
suitable variant) so missing discovered.path is reported as an error instead of
panicking.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 126-145: process_next_event currently blocks dispatching script
actions unless a session_id and viewport exist and calls execute_actions which
reprojections before every action; move to allow event hooks to run without a
live presentation by dispatching actions regardless of session/viewport and
resolving the PresentationModel lazily only in branches that require it.
Specifically, keep the call to
config.active_script().dispatch_event(event_name(&event), context) as-is in
process_next_event, but change the handling of the returned actions so that you
do not require session_id/viewport up-front; instead, iterate the actions and
only call execute_actions or perform presentation-dependent work when an action
type actually needs a PresentationModel (e.g., rendering/reprojection-related
actions), resolving session_id/viewport and creating the PresentationModel
inside those branches; apply the same lazy-resolution change in the analogous
logic around the later block referenced (lines ~207-215) so
Notify/ReloadConfig/EnterMode style actions run even with no live presentation.
In `@crates/embers-client/src/grid.rs`:
- Around line 86-94: The code uses unchecked arithmetic (x + 1 and y + 1) when
calling draw_hline and draw_vline which is inconsistent with the rest of the
module that uses saturating_add; update those calls in the grid drawing code to
use x.saturating_add(1) and y.saturating_add(1) (and right/ bottom if
applicable) when invoking draw_hline and draw_vline so they match the existing
use of saturating_add in draw_hline/draw_vline and preserve the existing width >
2 / height > 2 guards and behavior of put_char.
- Around line 46-54: The loop over text.chars().enumerate() casts offset to u16
which can silently truncate for very large strings; change the cast to a
fallible conversion using u16::try_from(offset).ok() and bail out when it fails,
i.e., before calling x.checked_add; update the block around
text.chars().enumerate() to convert offset with u16::try_from(offset).ok(),
handle None by breaking, then use x.checked_add(offset_u16) and keep the
existing bounds check and self.put_char(x_pos, y, ch) references (offset,
x.checked_add, put_char, self.width).
- Around line 74-75: The current conversion of rect.origin.x/y to u16 (variables
x and y in grid.rs) uses max(0) then as u16 which silently truncates values >
u16::MAX; change the clamp behavior to bound the i32 coordinates to 0..=u16::MAX
before casting: for Rect::origin (Point with i32 fields) clamp each coordinate
to the range [0, u16::MAX as i32] and then cast to u16 so large positive
coordinates are correctly clamped to the grid boundary rather than truncated.
In `@crates/embers-client/src/input/keymap.rs`:
- Around line 40-45: The code currently clones the entire Vec (mode_bindings)
and then clones an individual BindingSpec when matching, which is unnecessary;
change bindings.get(&mode).cloned().unwrap_or_default() to obtain a
reference/slice (e.g., bindings.get(&mode).map(|v| v.as_slice()).unwrap_or(&[]))
so you iterate by reference, and remove the .cloned() after .find(...) so you
use the &BindingSpec<T> directly (or destructure by dereferencing, e.g., use
*binding to access fields). Update references to mode_bindings, BindingSpec<T>,
and the find on pending accordingly to avoid cloning.
In `@crates/embers-client/src/renderer.rs`:
- Around line 139-142: The conversion from i32 to u16 for leaf.rect.origin.x/y
can overflow for large positive values; update the conversions in renderer.rs
(where x/y are computed from leaf.rect.origin.x and .y) to clamp the i32 into
the u16 range before casting (e.g., clamp to 0..=u16::MAX or use try_into() with
handling), then convert to u16 so very large coordinates saturate safely instead
of overflowing.
In `@crates/embers-client/src/scripting/context.rs`:
- Around line 230-235: BufferRef::process_name currently uses
command.rsplit('/') which fails on Windows paths; change the implementation to
use std::path::Path::file_name() on the first command string (e.g., create a
Path from the command, call file_name(), convert to &str) and fall back to the
original command string when file_name() or UTF-8 conversion returns None so you
return the executable name cross-platform.
In `@crates/embers-client/src/scripting/engine.rs`:
- Around line 295-315: The registration currently converts incoming Rhai FnPtr
to a plain function reference via function_ref, losing any bound/curry state;
update set_root_formatter and set_nested_formatter (and the handlers around the
other ranges mentioned) to either store the original FnPtr directly in
registration.root_tab_formatter / registration.nested_tab_formatter or
explicitly validate and reject FnPtrs that carry bound arguments (non-zero-arg)
before accepting them; locate usages where call_fn_with_options(...,
function_name, ()) is used and ensure those call sites either call the stored
FnPtr directly or only accept zero-arg FnPtrs so curried callbacks are not
dropped.
- Around line 30-45: The engine currently only limits AST depth via
set_max_expr_depths but not execution steps, so add a call to
engine.set_max_operations(<reasonable_limit>) right after Engine::new() (before
compile/eval) to cap instruction execution for eval_ast_with_scope and handler
invocations; additionally, when invoking scripts via call_fn_with_options use
the available CallOptions/Options to set per-call operation limits or ensure the
global engine.set_max_operations applies so both eval_ast_with_scope and
call_fn_with_options are protected from infinite or very long loops.
In `@crates/embers-client/src/scripting/error.rs`:
- Around line 79-86: The helper source_path(&LoadedConfigSource) duplicates the
same path-to-string logic used inline in runtime_path and validation_path;
remove the inline duplication by calling source_path(source) from both
runtime_path and validation_path (ensure you pass the same LoadedConfigSource
reference) so all path formatting uses the single source_path implementation and
avoids repeated as_deref()/unwrap_or_else()/display().to_string() logic.
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 162-217: The closures currently cast unsigned IDs to i64 (e.g.,
session.id.0 as i64, buffer.id.0 as i64, node.id.0 as i64, child_id.0 as i64 in
children) which can produce negative values on overflow; replace those direct
casts with a safe conversion using i64::try_from(id.0) and handle the Err case
(e.g., return the ID as a string Dynamic::from(id.0.to_string()) or return
Dynamic::UNIT / an explicit error) so scripts see a correct representation
instead of a wrapped negative number; update each registered fn that returns raw
i64 (session id, buffer id, node id, children mapping, and any other id casts
shown) to perform try_from and map to the chosen fallback.
In `@crates/embers-client/tests/config_loading.rs`:
- Line 12: The test writes leader keys using incorrect notation—replace literal
sequences like set_leader("C-a"), set_leader("C-b"), set_leader("C-c") with
angle-bracket modifier notation set_leader("<C-a>"), set_leader("<C-b>"),
set_leader("<C-c>") so the parser treats them as modifier+key combos; update the
test strings written to config (where fs::write writes the set_leader calls in
tests/config_loading.rs) to use the "<C-...>" form to match the other tests and
intended behavior.
In `@crates/embers-client/tests/configured_client.rs`:
- Around line 18-24: manager_from_source currently creates a TempDir that is
dropped at function return while ConfigManager stores ConfigDiscoveryOptions
pointing at tempdir.path(), causing dangling path issues when calling
ConfigManager::reload(); fix by changing manager_from_source to return a tuple
(ConfigManager, TempDir) so the caller retains the TempDir lifetime, update call
sites to destructure let (config, _tempdir) = manager_from_source(...), and
ensure references to
ConfigDiscoveryOptions::default().with_project_config_dir(tempdir.path()) remain
unchanged inside manager_from_source so the stored path stays valid.
In `@crates/embers-client/tests/controller.rs`:
- Around line 15-22: Extract the repeated Size literal into a test constant and
use it in each PresentationModel::project call: define a constant like
TEST_SIZE: Size = Size { width: 40, height: 14 }; near the top of the tests file
(after imports), then replace all occurrences of Size { width: 40, height: 14 }
used when calling PresentationModel::project (and any other test helpers) with
TEST_SIZE to avoid duplication and improve maintainability.
In `@crates/embers-client/tests/script_engine.rs`:
- Around line 12-122: The test loaded_config_debug_snapshot_is_stable currently
asserts a huge inline Debug string from engine.loaded_config(), which is
brittle; replace the large assert_eq snapshot with either (A) a snapshot
assertion using the insta crate against engine.loaded_config() (add insta to
dev-dependencies and use its assert_debug_snapshot! macro), or (B) replace the
big Debug comparison with focused assertions on specific fields returned by
engine.loaded_config() (e.g., leader, modes, bindings, theme) so only the
important invariants are tested; locate the test function
loaded_config_debug_snapshot_is_stable and the use of engine.loaded_config() to
make the replacement.
In `@crates/embers-client/tests/support/mod.rs`:
- Around line 43-54: The use of a magic sentinel FloatingId(0) in
root_focus_state() is confusing; update the demo_snapshot call to use an
explicit Option to represent "no floating focus" (e.g., pass None or
Some((floating_id, node_id)) as appropriate) or, if demo_snapshot expects a
sentinel, change demo_snapshot's API to accept Option<(FloatingId, NodeId)> and
update callers (including root_focus_state) accordingly; ensure you also remove
the FloatingId(0) sentinel from demo_snapshots()/demo_snapshot and update
session initialization code that manipulates session.focused_floating_id /
session.focused_leaf_id to use Option semantics (or add a clarifying comment
near FloatingId(0) and LEFT_LEAF_ID if you must keep the sentinel).
In `@crates/embers-protocol/schema/embers.fbs`:
- Line 1: The FlatBuffers schema namespace is still declared as "mux.protocol"
while the project and file identifier ("EMBR") were renamed to embers; update
the namespace declaration in the schema by replacing the "namespace
mux.protocol;" declaration with "namespace embers.protocol;" (or the project's
canonical embers namespace) so the FlatBuffer types align with the new embers-*
naming and avoid mismatches with generated code and identifiers.
In `@crates/embers-protocol/src/codec.rs`:
- Around line 1893-1926: The tabs decoding currently uses filter_map in codec.rs
which silently drops TabRecord entries when title() is None; change the tabs
decoding in the closure that builds TabsRecord so missing titles are treated as
errors (or explicitly documented if intentional) by replacing the filter_map
with explicit handling: for each tab from tabs_fb.iter() attempt to read title
with tab.title().ok_or(...) or return None from the outer tabs closure (so the
entire TabsRecord decoding fails) and propagate that error/None instead of
silently skipping entries; reference TabRecord, TabsRecord, NodeId and the tabs
closure in codec.rs when making the change.
In `@crates/embers-protocol/src/framing.rs`:
- Around line 77-95: The current write_frame function always calls
writer.flush(), which harms throughput for bulk writes; update the API so
callers can choose flushing: either add a boolean parameter (e.g., flush: bool)
to write_frame or add a companion function (e.g., write_frame_no_flush) so
write_frame(writer, frame, flush) or write_frame_no_flush(writer, frame) writes
the length/type/request_id/payload but only calls writer.flush() when requested;
adjust callers to use the non-flushing variant for bulk transfers and keep
flushing for latency-sensitive paths. Ensure references to write_frame, the
flush() call, and AsyncWrite usage are updated accordingly.
In `@crates/embers-protocol/src/lib.rs`:
- Line 24: Replace the wildcard re-export `pub use types::*;` with an explicit
allowlist of the public items you intend to expose from types.rs to avoid
accidental API expansion; open types.rs, collect the public types (enums,
structs, traits, constants) that are part of the crate API and replace the
wildcard with a single `pub use types::{Name1, Name2, ...};` re-export listing
those symbols so new items in types.rs are not exported implicitly (ensure the
list includes the currently relied-on symbols and update it deliberately when
API changes are intended).
In `@crates/embers-protocol/src/types.rs`:
- Around line 43-58: The API enum variants SelectRootTab, RenameRootTab,
CloseRootTab (and other similar variants referenced around the other locations)
use usize for index fields (and an active field elsewhere); change those fields
to u32 so the Rust API matches the wire-format uint (u32) and remove any
unchecked casts like `*index as u32` in the encoding/serialization paths; update
the signatures and any callers to accept/produce u32 for index/active to avoid
silent truncation and make the width contract explicit.
In `@crates/embers-server/src/buffer_runtime.rs`:
- Around line 84-94: The spawned reader and wait threads (started with
thread::Builder::new().spawn(...) calling read_loop and wait_loop for
buffer-{buffer_id}-reader and buffer-{buffer_id}-wait) currently discard their
JoinHandle, so panics or clean shutdown cannot be observed; capture the
JoinHandle returned by spawn, store it in the buffer runtime state (e.g., a Vec
or fields on the BufferRuntime/Buffer struct) and ensure you join or abort those
handles during shutdown/Drop to detect panics and ensure clean termination
(handle JoinHandle::join results and log/propagate errors).
- Around line 182-188: The function exit_status_code currently casts
portable_pty::ExitStatus::exit_code() (u32) to i32 using as which can overflow;
update the function to avoid unsafe casts by either returning Option<u32>
instead of Option<i32> (preserving the original u32 value from
ExitStatus::exit_code()) or validate/convert safely (e.g., check whether
exit_code() <= i32::MAX and only then return Some(exit_code as i32), otherwise
return None or use a defined saturating behavior); adjust all call sites that
consume exit_status_code accordingly and keep the function name exit_status_code
and the use of portable_pty::ExitStatus::exit_code() as the reference points for
the change.
In `@crates/embers-server/src/config.rs`:
- Around line 15-18: The code currently uses socket_path.to_string_lossy() which
corrupts non-UTF8 paths; instead preserve the raw OS string: change the code
that inserts into buffer_env to use socket_path.as_os_str().to_owned() (i.e., an
OsString) and adjust buffer_env’s value type and any downstream consumers to
accept std::ffi::OsString (or otherwise propagate OsStr/OsString) so the
EMBERS_SOCKET env is set/forwarded without lossy UTF-8 conversion; update
references to SOCKET_ENV_VAR, buffer_env, and any spawn/env-setting logic to
work with OsString/OsStr.
In `@crates/embers-server/src/protocol.rs`:
- Around line 76-79: The match on SplitDirection is an identity mapping; replace
the redundant match in the code that sets direction (the expression using
split.direction and SplitDirection::Horizontal/Vertical) with a direct
assignment of split.direction (or otherwise propagate the same enum value) so
you no longer pattern-match variants to themselves; update any function/struct
initializers referencing this mapping (the SplitDirection usage) to use the enum
value directly.
- Around line 42-46: The match on ActivityState in the assignment to activity is
an unnecessary identity mapping; replace the whole match block by directly
assigning buffer.activity to activity (i.e., set activity = buffer.activity) so
you remove the redundant match and simplify code that references ActivityState,
ActivityState::Idle, ActivityState::Activity and ActivityState::Bell.
In `@crates/embers-server/src/server.rs`:
- Around line 1198-1221: The broadcast function currently iterates all events
per subscription and only removes a subscription if a send fails, but it
continues sending remaining events after a failure; update broadcast (the async
fn broadcast) so that for each subscription (locked via
self.subscriptions.lock().await) you stop sending further events to that
subscription as soon as any subscription.sender.send(ServerEnvelope::Event(...))
returns Err and make the retain closure return false immediately on that first
failure (i.e., short-circuit the per-subscription event loop), ensuring a failed
send prevents subsequent events in the same batch from being sent to that
subscription.
- Around line 66-73: The write_loop task's JoinHandle is currently discarded so
failures or panics are not logged; capture the handle returned from
tokio::spawn(write_loop(writer, outbound_rx)) (e.g. let write_handle =
tokio::spawn(write_loop(...))) and then spawn an async monitor that awaits that
handle and logs both JoinError (task panicked or cancelled) and any Err returned
by the inner write_loop future, using error! with context (include connection_id
and a descriptive message). Ensure you use the same logging style as the
connection handler (error!(%error, connection_id, "write_loop failed")) so you
have visibility into both panics and runtime errors from write_loop.
In `@crates/embers-server/src/state.rs`:
- Around line 814-816: The public detach_buffer currently only flips
BufferAttachment::Detached which leaves any live BufferViewNode(s) still
pointing at that buffer and causes validate() to fail; change detach_buffer to
also sever the live view by finding any BufferViewNode that references the given
buffer_id and closing or replacing its leaf (e.g., call the view-close or
leaf-replace logic used elsewhere) so no live view node points at a detached
buffer, and move the direct flag-only mutation into a private helper (e.g.,
detach_buffer_raw or similar) used only by teardown paths; update references to
use the public detach_buffer when removing user-visible buffers so validate()
remains consistent.
- Around line 279-283: When constructing nodes, reject children that are
duplicates in the input or already have a parent instead of unconditionally
overwriting them: before calling self.set_parent(*child, Some(node_id)) (and
after self.ensure_node_belongs_to(*child, session_id)), check the child's
current parent (via whatever accessor you have, e.g. a get_parent/parent map)
and if it is Some(...) return an error; also deduplicate the input children (use
a local HashSet to detect repeated child ids and return an error if a child
appears twice). Apply the same checks in both constructor sites (the block using
self.node_ids shown and the other block around lines 309-312) so validate()
won’t later fail due to multi-ownership or duplicate children.
- Around line 343-356: The current checks in the function
(ensure_session_exists, ensure_node_belongs_to, node_parent, is_session_root)
still allow reusing the same subtree as multiple floating roots because a
floating root also has parent == None; add a guard that detects if root_node is
already registered as a floating root (e.g. check the state’s floating_roots
collection or a roots->floating mapping) and return
Err(MuxError::invalid_input("node is already a floating root")) if so; locate
this check alongside the existing parent/is_session_root checks in the same
function and use the same error pattern to reject duplicates.
- Around line 430-455: Validate that tabs_id refers to a Tabs container and
reject self-parenting before mutating the child's parent: first call
node_mut(tabs_id) and ensure it matches Node::Tabs (and that child != tabs_id),
only after those checks proceed to set_parent(child, Some(tabs_id)) and then
push the TabEntry via tabs.tabs.push(TabEntry::new(title, child)) and update
tabs.active; keep the existing calls to node_session_id, ensure_node_belongs_to,
node_parent, is_session_root, and floating_id_by_root but move set_parent to
after the container type/self-parent validation to avoid corrupting the graph.
In `@crates/embers-server/src/terminal_backend.rs`:
- Around line 216-226: The metadata() accessor currently reads bell_pending from
the locked events but never clears it, so ActivityState::Bell becomes permanent;
update metadata() (in terminal_backend.rs) to acquire the events lock, read and
then reset state.bell_pending = false (or call a new clear_bell() helper you
add) before constructing and returning BackendMetadata so that subsequent
metadata() calls will yield ActivityState::Activity unless a new bell is
received; ensure the change references the events mutex, state.bell_pending, and
returns BackendMetadata with ActivityState::Bell or ActivityState::Activity
accordingly.
- Around line 83-96: The send_event implementation on BackendEventProxy
currently ignores errors from self.state.lock() which silently drops events when
the mutex is poisoned; update send_event (EventListener::send_event) to handle
the Err case instead of returning silently—capture the lock error, log a
descriptive message including the event and the error (or propagate/panic via
expect() if losing events is unacceptable) so mutex-poisoning or lock failures
are visible; make the change where self.state.lock() is called in
BackendEventProxy::send_event.
---
Outside diff comments:
In `@crates/embers-client/src/testing.rs`:
- Around line 176-183: Update the test test_grid_renders_rows to use the new
crate name by replacing the string "mux" with "embers" in the calls to
TestGrid::put_str, and update the expected rendered string passed to
assert_eq!(grid.render(), ...) accordingly so it matches the new content
produced by grid.render() after putting "embers" into the grid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 349d76ee-455b-4f7a-981e-9fed9f33cccc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (117)
.github/workflows/ci.ymlCargo.tomlcrates/embers-cli/Cargo.tomlcrates/embers-cli/src/bin/embers-cli.rscrates/embers-cli/src/interactive.rscrates/embers-cli/src/lib.rscrates/embers-cli/src/main.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-cli/tests/ping.rscrates/embers-cli/tests/popups.rscrates/embers-cli/tests/sessions.rscrates/embers-cli/tests/support/mod.rscrates/embers-cli/tests/windows.rscrates/embers-client/Cargo.tomlcrates/embers-client/src/client.rscrates/embers-client/src/config/discover.rscrates/embers-client/src/config/error.rscrates/embers-client/src/config/loader.rscrates/embers-client/src/config/mod.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/controller.rscrates/embers-client/src/grid.rscrates/embers-client/src/input/keymap.rscrates/embers-client/src/input/keyparse.rscrates/embers-client/src/input/mod.rscrates/embers-client/src/input/modes.rscrates/embers-client/src/lib.rscrates/embers-client/src/presentation.rscrates/embers-client/src/renderer.rscrates/embers-client/src/scripting/context.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/error.rscrates/embers-client/src/scripting/harness.rscrates/embers-client/src/scripting/mod.rscrates/embers-client/src/scripting/model.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/src/scripting/types.rscrates/embers-client/src/socket_transport.rscrates/embers-client/src/state.rscrates/embers-client/src/testing.rscrates/embers-client/src/transport.rscrates/embers-client/tests/config_loading.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/controller.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/presentation.rscrates/embers-client/tests/reducer.rscrates/embers-client/tests/renderer.rscrates/embers-client/tests/script_actions.rscrates/embers-client/tests/script_engine.rscrates/embers-client/tests/socket_transport.rscrates/embers-client/tests/support/mod.rscrates/embers-core/Cargo.tomlcrates/embers-core/src/diagnostics.rscrates/embers-core/src/error.rscrates/embers-core/src/geometry.rscrates/embers-core/src/ids.rscrates/embers-core/src/lib.rscrates/embers-core/src/metadata.rscrates/embers-core/src/snapshot.rscrates/embers-protocol/Cargo.tomlcrates/embers-protocol/build.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/client.rscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/framing.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-protocol/tests/ping_round_trip.rscrates/embers-server/Cargo.tomlcrates/embers-server/src/buffer_runtime.rscrates/embers-server/src/config.rscrates/embers-server/src/lib.rscrates/embers-server/src/model.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/buffer_move.rscrates/embers-server/tests/floating_windows.rscrates/embers-server/tests/model_state.rscrates/embers-server/tests/nested_tabs.rscrates/embers-server/tests/ping_server.rscrates/embers-server/tests/session_root_tabs.rscrates/embers-server/tests/split_layout.rscrates/embers-test-support/Cargo.tomlcrates/embers-test-support/src/cli.rscrates/embers-test-support/src/lib.rscrates/embers-test-support/src/protocol.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/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/mux-cli/Cargo.tomlcrates/mux-cli/src/lib.rscrates/mux-client/Cargo.tomlcrates/mux-client/src/lib.rscrates/mux-protocol/schema/embers.fbscrates/mux-protocol/src/client.rscrates/mux-protocol/src/codec.rscrates/mux-protocol/src/framing.rscrates/mux-protocol/src/types.rscrates/mux-protocol/tests/ping_round_trip.rscrates/mux-server/src/config.rscrates/mux-server/src/lib.rscrates/mux-server/src/server.rscrates/mux-test-support/src/protocol.rs
Comment on lines
+1198
to
+1221
| async fn broadcast(&self, events: Vec<ServerEvent>) { | ||
| if events.is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| let mut subscriptions = self.subscriptions.lock().await; | ||
| subscriptions.retain(|_, subscription| { | ||
| for event in &events { | ||
| let event_matches = event.session_id().is_none() | ||
| || subscription.session_id.is_none() | ||
| || subscription.session_id == event.session_id(); | ||
|
|
||
| if event_matches | ||
| && subscription | ||
| .sender | ||
| .send(ServerEnvelope::Event(event.clone())) | ||
| .is_err() | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
| true | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential issue: Subscription may be retained despite partial send failure.
The retain callback sends events to each subscription and returns false (removing the subscription) only if a send fails. However, if an earlier event succeeds but a later event fails, some events were delivered before the subscription was removed. This is likely acceptable (at-most-once semantics), but worth noting.
Additionally, the loop sends all matching events before checking if any failed, which means a failed subscription still processes subsequent events in the current batch before being removed on the next broadcast.
🤖 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 1198 - 1221, The broadcast
function currently iterates all events per subscription and only removes a
subscription if a send fails, but it continues sending remaining events after a
failure; update broadcast (the async fn broadcast) so that for each subscription
(locked via self.subscriptions.lock().await) you stop sending further events to
that subscription as soon as any
subscription.sender.send(ServerEnvelope::Event(...)) returns Err and make the
retain closure return false immediately on that first failure (i.e.,
short-circuit the per-subscription event loop), ensuring a failed send prevents
subsequent events in the same batch from being sent to that subscription.
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (2)
crates/embers-cli/tests/interactive.rs (1)
87-103:⚠️ Potential issue | 🔴 CriticalRace condition causes pipeline failure:
wait_for_socketdoesn't prevent the server from exiting.The pipeline failure shows the socket disappears between the client exit and the
list-sessionscommand. The issue is thatwait_for_socketat line 89 only checks if the socket file exists, but the server may exit immediately after the client quits (if it's configured to exit with zero clients), causing the socket to disappear before or during thelist-sessionscall.The
wait_for_socketcall here is logically inverted—it runs after the socket should already exist (the client was already connected), not before the server starts.Consider one of:
- Use
TestServer(likeattach_subcommand_connects_to_running_server) which has controlled lifetime- Add a server persistence flag if the spawned server should outlive clients
- Remove the post-exit CLI verification if the server is expected to exit with the client
🛠️ Suggested fix using TestServer pattern
If the intent is for the server to persist after the client exits, the test should use the
TestServerpattern that explicitly manages server lifetime:#[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn embers_without_subcommand_starts_server_and_client() { - let tempdir = tempdir().expect("tempdir"); - let socket_path = tempdir.path().join("embers.sock"); - let socket_arg = socket_path.to_string_lossy().into_owned(); - let mut harness = spawn_embers(&["--socket", &socket_arg]); + let server = TestServer::start().await.expect("start server"); + let socket_arg = server.socket_path().to_string_lossy().into_owned(); + let mut harness = spawn_embers(&["attach", "--socket", &socket_arg]); // ... rest of test using managed server lifetime + server.shutdown().await.expect("shutdown server"); }Alternatively, if the test specifically validates that
emberswithout a subcommand starts its own server, the post-exit CLI verification should be removed or the server's exit-on-last-client behavior should be disabled.🤖 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 87 - 103, The test currently races because wait_for_socket is used after the client quits (harness.write_all("\x11") / harness.wait()), so the server may have already shut down and removed the socket before list-sessions runs; fix by ensuring the server lifetime outlives the client: replace the ephemeral spawn+wait_for_socket pattern with the TestServer pattern used in attach_subcommand_connects_to_running_server (create a TestServer that is started before the client connects and kept alive until after the list-sessions assertion), or alternatively remove the post-exit list-sessions verification if server exit-on-last-client is expected, or add/enable a server persistence flag so the socket remains; locate and update the test around wait_for_socket, harness.write_all and the subsequent cargo_bin("embers").arg("list-sessions") call to use TestServer or persistent-server behavior.crates/embers-server/src/state.rs (1)
862-872:⚠️ Potential issue | 🟠 MajorReject detached or hidden leaves before mutating focus.
This still lets callers focus a leaf that is not rooted under the session layout, or one inside an invisible floating window. In both cases
session.focused_leafends up pointing at a non-visible node andvalidate()fails.🐛 Suggested fix
pub fn focus_leaf(&mut self, session_id: SessionId, leaf_id: NodeId) -> Result<()> { self.ensure_leaf_belongs_to(leaf_id, session_id)?; + let root = self.top_root_for_node(leaf_id)?; + if root != self.session(session_id)?.root_node { + let Some(floating_id) = self.floating_id_by_root(root) else { + return Err(MuxError::invalid_input(format!( + "leaf {leaf_id} is not attached to the session layout" + ))); + }; + if !self.floating_window(floating_id)?.visible { + return Err(MuxError::invalid_input(format!( + "leaf {leaf_id} is inside a hidden floating window" + ))); + } + } self.clear_session_focus(session_id)?; self.set_leaf_focus(leaf_id, true)?;🤖 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 862 - 872, Before mutating session state in focus_leaf, reject leaves that are detached from the session layout or are inside an invisible floating window: call the existing membership/visibility checks (e.g., use whatever verifies a node is rooted under the session layout and check floating window visibility returned by floating_id_for_node) before calling clear_session_focus, set_leaf_focus, or changing session.focused_leaf/session.focused_floating; if the node is not rooted under the session layout or its floating owner is hidden, return an error (so validate() won't later fail) instead of mutating state in focus_leaf.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/install-flatc/action.yml:
- Around line 36-38: After installing flatc, remove the temporary extraction
directory to avoid leaving flatbuffers-bin/ in the workspace: update the install
steps that include "unzip -q flatbuffers-bin/flatc.zip -d flatbuffers-bin",
"chmod +x flatbuffers-bin/flatc" and "sudo install flatbuffers-bin/flatc
/usr/local/bin/flatc" to add a cleanup command (e.g., rm -rf flatbuffers-bin or
similar) immediately after the install so the workspace no longer contains the
flatbuffers-bin/ artifact.
In `@crates/embers-cli/src/lib.rs`:
- Around line 253-265: The code creates a buffer via
connection.create_buffer(...) then sends a Session::AddRootTab RPC and if that
RPC fails the buffer is leaked; modify the flow to perform a best-effort
rollback: after calling create_buffer(...) (the buffer variable /
buffer.buffer.id), wrap the subsequent
connection.request(ClientMessage::Session(SessionRequest::AddRootTab { ... }))
and expect_session_snapshot(...) in a match/try block and on any Err for the
request or snapshot call call a cleanup path that sends a buffer deletion (or
appropriate buffer-close) request for the created buffer (using the same
connection and buffer.buffer.id), ensuring any error from the cleanup is logged
but does not mask the original error; apply the same pattern to the similar
flows around the other create_buffer+Session requests referenced (the blocks at
~317-333 and ~439-452) to avoid leaving detached buffers when the follow-up RPC
fails.
- Around line 471-492: The non-interactive branch (Some(command) => ...) must
bootstrap the server before attempting to execute commands; add a call to
ensure_server_process(&socket).await (propagating errors) at the start of that
match arm so the server is started on first-run before calling execute(&socket,
command).await; note this should be added only to the general Some(command)
branch (Serve and Attach are already handled separately), and keep existing
behavior of printing non-empty output.
In `@crates/embers-client/src/grid.rs`:
- Around line 100-108: lines() panics when self.width is zero because chunks(0)
is invalid; add a fast-path in the lines() method that checks if self.width == 0
and returns a Vec of empty strings with length usize::from(self.height) (so
render() still just joins lines()), e.g., in lines() detect zero width and
return vec!["".to_string(); usize::from(self.height)] instead of calling chunks;
keep render() unchanged to rely on lines().
In `@crates/embers-client/src/scripting/context.rs`:
- Around line 38-39: visible_node_ids is built only from nodes with direct
geometry so ancestors (e.g., split containers) remain marked hidden; change
construction of visible_node_ids (the set built from geometry_by_node.keys()) to
also include all ancestor node IDs by walking each visible node's parent chain
(via the node parent field / ctx.nodes() lookup) and inserting each ancestor
into the BTreeSet; apply the same ancestor-propagation fix at the other
occurrence referenced (the block around lines 90-95) so scripts iterating
ctx.nodes() see the full visible/layout path.
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 657-676: The parse_floating_options path currently uses
parse_u16_field for x/y/width/height which funnels coordinates through
parse_amount and rejects 0; change x and y to be parsed by a new or modified
helper that allows non-negative u16 (0 allowed) while keeping width/height
parsed with the existing strict >0 logic. Concretely, add a parse_coordinate (or
add a allow_zero flag to parse_u16_field) that mirrors parse_u16_field's removal
and try_cast steps but calls a variant of parse_amount that accepts 0, then use
that for x and y in parse_floating_options while leaving width/height untouched;
keep FloatingOptions, FloatGeometry::new and parse_optional_string usage the
same.
In `@crates/embers-client/tests/controller.rs`:
- Around line 106-113: The assertion uses a hardcoded embers_core::BufferId(4)
which couples the test to demo_state() internals; replace the literal with a
named fixture constant (e.g., FOCUSED_BUFFER_ID) exported from the test support
module and use that constant in the expected
ClientMessage::Input(InputRequest::Send { ... buffer_id: FOCUSED_BUFFER_ID, ...
}) so the test references the canonical focused buffer value instead of a magic
number; update the support module to expose the constant and adjust this
assertion to use it.
In `@crates/embers-client/tests/support/mod.rs`:
- Around line 56-65: The mapping in demo_snapshot contains a dead sentinel check
for FloatingId(0); remove the if floating_id.0 == 0 branch and simplify the map
to always treat a Some((floating_id, leaf_id)) as (Some(floating_id),
Some(leaf_id)), using unwrap_or((None, Some(FOCUSED_LEAF_ID))) for the None
case; update the demo_snapshot function so it no longer inspects FloatingId.0
and directly returns (Some(floating_id), Some(leaf_id)) when focused_floating is
Some.
In `@crates/embers-protocol/schema/embers.fbs`:
- Around line 351-386: The Envelope table currently uses a manual discriminator
field (kind: MessageKind) plus many optional typed fields (e.g., ping_request,
session_request, ping_response, error_response, etc.), which is error-prone;
replace this pattern with a FlatBuffers union to get type-safe message dispatch:
define a union (e.g., MessageUnion) listing all message types used in Envelope,
remove the manual kind field and the many optional fields from Envelope, add a
single field like message:MessageUnion and a corresponding
message_type:MessageUnion (or let FlatBuffers generate the type), and update any
code that pattern-matches on Envelope.kind to use the union-generated accessors
and switch on the union type; ensure to keep root_type Envelope and
file_identifier "EMBR" unchanged.
In `@crates/embers-protocol/src/codec.rs`:
- Around line 1880-1928: decode_node_record must reject mismatched/missing
payloads instead of producing a NodeRecord with kind X and payload None: check
record.kind() and ensure the corresponding table/vector is present and
well-formed, returning Err(ProtocolError::InvalidMessage) on failure.
Concretely, replace the current optional constructions for buffer_view, split,
and tabs so that when record.kind() == fb::NodeKind::BufferView you require
buffer_view() to exist and parse to a BufferViewRecord (use required(...) where
helpful) or return ProtocolError::InvalidMessage; when record.kind() ==
fb::NodeKind::Split require split() and its child_ids()/sizes()/direction to be
present and valid before creating SplitRecord (do not silently return None), and
when record.kind() == fb::NodeKind::Tabs ensure tabs() and tabs.tabs() parse to
TabsRecord or return ProtocolError::InvalidMessage; use the same error variant
and include context strings as in required(...) to aid debugging.
In `@crates/embers-server/src/server.rs`:
- Around line 71-84: The write monitor currently waits for write_loop
(write_handle) to finish but leaves the read task running; update the spawn
logic so the read task's JoinHandle (read_handle) is kept and aborted when
write_handle completes, and ensure read_loop treats failed outbound.send(...) as
terminal (returning/propagating error instead of continue). Concretely: when
spawning read_loop and write_loop keep both handles, on write_handle completion
call read_handle.abort() (or send a shutdown signal) before calling
cleanup_connection(connection_id). Also modify read_loop to stop/return on any
outbound.send error so failed sends cannot be ignored.
In `@crates/embers-server/src/state.rs`:
- Around line 243-263: create_buffer_view currently inserts the new Node into
self.nodes before calling attach_buffer, leaving an orphaned node if
attach_buffer returns Err; fix this by rolling back the partial creation: after
obtaining node_id and inserting the Node, call attach_buffer(buffer_id, node_id)
and if it returns Err remove the inserted node (self.nodes.remove(&node_id))
before returning the error. Apply the same rollback pattern to the analogous
code paths referenced (the add_tab_from_buffer / add_tab_sibling flow around the
code at the other location) so any node inserted into self.nodes is removed on
error to keep state consistent.
---
Duplicate comments:
In `@crates/embers-cli/tests/interactive.rs`:
- Around line 87-103: The test currently races because wait_for_socket is used
after the client quits (harness.write_all("\x11") / harness.wait()), so the
server may have already shut down and removed the socket before list-sessions
runs; fix by ensuring the server lifetime outlives the client: replace the
ephemeral spawn+wait_for_socket pattern with the TestServer pattern used in
attach_subcommand_connects_to_running_server (create a TestServer that is
started before the client connects and kept alive until after the list-sessions
assertion), or alternatively remove the post-exit list-sessions verification if
server exit-on-last-client is expected, or add/enable a server persistence flag
so the socket remains; locate and update the test around wait_for_socket,
harness.write_all and the subsequent cargo_bin("embers").arg("list-sessions")
call to use TestServer or persistent-server behavior.
In `@crates/embers-server/src/state.rs`:
- Around line 862-872: Before mutating session state in focus_leaf, reject
leaves that are detached from the session layout or are inside an invisible
floating window: call the existing membership/visibility checks (e.g., use
whatever verifies a node is rooted under the session layout and check floating
window visibility returned by floating_id_for_node) before calling
clear_session_focus, set_leaf_focus, or changing
session.focused_leaf/session.focused_floating; if the node is not rooted under
the session layout or its floating owner is hidden, return an error (so
validate() won't later fail) instead of mutating state in focus_leaf.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 748312bf-0c6f-4911-9b36-0b143358b31a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.github/actions/install-flatc/action.yml.github/workflows/ci.ymlCargo.tomlcrates/embers-cli/src/bin/embers-cli.rscrates/embers-cli/src/interactive.rscrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-client/src/config/error.rscrates/embers-client/src/config/loader.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/controller.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/context.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/error.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/src/testing.rscrates/embers-client/tests/config_loading.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/controller.rscrates/embers-client/tests/reducer.rscrates/embers-client/tests/script_engine.rscrates/embers-client/tests/support/mod.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/framing.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-server/src/buffer_runtime.rscrates/embers-server/src/config.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/model_state.rs
Comment on lines
+351
to
+386
| table Envelope { | ||
| request_id:ulong = 0; | ||
| kind:MessageKind = None; | ||
| ping_request:PingRequest; | ||
| session_request:SessionRequest; | ||
| buffer_request:BufferRequest; | ||
| node_request:NodeRequest; | ||
| floating_request:FloatingRequest; | ||
| input_request:InputRequest; | ||
| subscribe_request:SubscribeRequest; | ||
| unsubscribe_request:UnsubscribeRequest; | ||
|
|
||
| ping_response:PingResponse; | ||
| ok_response:OkResponse; | ||
| error_response:ErrorResponse; | ||
| sessions_response:SessionsResponse; | ||
| session_snapshot_response:SessionSnapshotResponse; | ||
| buffers_response:BuffersResponse; | ||
| buffer_response:BufferResponse; | ||
| floating_list_response:FloatingListResponse; | ||
| floating_response:FloatingResponse; | ||
| subscription_ack_response:SubscriptionAckResponse; | ||
| snapshot_response:SnapshotResponse; | ||
|
|
||
| session_created_event:SessionCreatedEvent; | ||
| session_closed_event:SessionClosedEvent; | ||
| buffer_created_event:BufferCreatedEvent; | ||
| buffer_detached_event:BufferDetachedEvent; | ||
| node_changed_event:NodeChangedEvent; | ||
| floating_changed_event:FloatingChangedEvent; | ||
| focus_changed_event:FocusChangedEvent; | ||
| render_invalidated_event:RenderInvalidatedEvent; | ||
| } | ||
|
|
||
| root_type Envelope; | ||
| file_identifier "EMBR"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using FlatBuffers union for type-safe message dispatch.
The Envelope table uses a manual kind discriminator with many optional fields. FlatBuffers supports native union types that provide compile-time type safety and reduce boilerplate. However, this pattern is functional and allows flexibility for partial messages.
🤖 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 351 - 386, The
Envelope table currently uses a manual discriminator field (kind: MessageKind)
plus many optional typed fields (e.g., ping_request, session_request,
ping_response, error_response, etc.), which is error-prone; replace this pattern
with a FlatBuffers union to get type-safe message dispatch: define a union
(e.g., MessageUnion) listing all message types used in Envelope, remove the
manual kind field and the many optional fields from Envelope, add a single field
like message:MessageUnion and a corresponding message_type:MessageUnion (or let
FlatBuffers generate the type), and update any code that pattern-matches on
Envelope.kind to use the union-generated accessors and switch on the union type;
ensure to keep root_type Envelope and file_identifier "EMBR" unchanged.
Comment on lines
+1176
to
+1188
| { | ||
| let mut state = self.state.lock().await; | ||
| if let Some(title) = title | ||
| && let Err(error) = state.set_buffer_title(buffer_id, title) | ||
| { | ||
| debug!(%buffer_id, %error, "failed to apply terminal title update"); | ||
| } | ||
| if let Err(error) = state.set_buffer_activity(buffer_id, activity) { | ||
| debug!(%buffer_id, %error, "failed to apply buffer activity update"); | ||
| } | ||
| } | ||
|
|
||
| self.broadcast(render_events(buffer_id, damage)).await; |
There was a problem hiding this comment.
Render invalidations alone can't sync activity/exit metadata.
These paths mutate server-side activity, state, and exit_code, but the only broadcast is RenderInvalidated. The client handles that by refreshing the snapshot, and SnapshotResponse does not carry those fields, so buffer activity and exit state can never be brought back into sync client-side.
Also applies to: 1191-1215
Track local buffer-view state by node id so scroll/follow behavior can diverge per pane without mutating server layout state. Seed and refresh view state from session snapshots and visible snapshots, preserve scrolled positions across output, and keep alternate-screen panes marked inactive without discarding local state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the char-only render surface with grapheme-aware styled cells, add ANSI row emission for the interactive frontend, and render a real cursor with backend-provided shape metadata. Update renderer helpers to use width-aware truncation, surface bar styles, and keep plain-text render snapshots stable for tests. 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>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (3)
crates/embers-client/tests/repo_config.rs (1)
214-223:⚠️ Potential issue | 🟡 MinorRun rustfmt on this helper before merge.
CI is still reporting a formatting diff in this
LoadedConfigSourceinitializer, socargo fmt --checkwill keep failing until this block is reformatted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/tests/repo_config.rs` around lines 214 - 223, The initializer for LoadedConfigSource inside repository_config_engine is not rustfmt-formatted; run rustfmt (cargo fmt) or manually reformat the block so the struct fields and chained PathBuf::from(...).join(...) call conform to project style, keeping the function name repository_config_engine, the ScriptEngine::load(...) call, LoadedConfigSource { origin: ConfigOrigin::Explicit, path: Some(PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/repository_config.rhai")), source: include_str!("fixtures/repository_config.rhai").to_owned(), source_hash: 0, } unchanged in semantics but with proper spacing/indentation, then re-run cargo fmt --check to ensure CI passes.crates/embers-cli/src/lib.rs (1)
550-563:⚠️ Potential issue | 🔴 CriticalValidate the
/tmpfallback runtime directory before trusting its socket.
/tmp/embers-<uid>is still a predictable shared path, andensure_server_process()probes it before any ownership or symlink check. Another local user can pre-create that directory/socket and have clients connect to the wrong server. Refuse this fallback unless the parent is a real0700directory owned byeffective_uid(), and do that validation beforeserver_is_available()or any other connect path touches the socket.Also applies to: 579-589
🤖 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 550 - 563, The /tmp fallback created by default_runtime_dir() is unsafe; before returning or ever calling server_is_available() or ensure_server_process(), validate that the parent directory (e.g., /tmp) entry for format!("embers-{}", effective_uid()) exists and is owned by effective_uid() and has mode 0o700 (no group/other access); if the ownership/mode check fails, refuse to use that fallback (return an error or fall back to a different non-shared path) and ensure the same validation is performed in any other code path that probes the same fallback socket (e.g., the code referenced around ensure_server_process() / server_is_available()) so you never touch or connect to a socket in a predictable shared directory without verifying owner and 0700 permissions first.crates/embers-client/src/configured_client.rs (1)
832-839:⚠️ Potential issue | 🟠 MajorReject action variants that the live executor still cannot realize.
ReplaceFloatingRoot,WrapNodeInSplit, andWrapNodeInTabscan make it through config loading, but they only fail later with"not supported by the live executor yet"when the user triggers them. That turns a config error into a latent runtime failure. Either implement these variants or reject them during script normalization/load.🤖 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 832 - 839, Detect and reject Action::ReplaceFloatingRoot, Action::WrapNodeInSplit, and Action::WrapNodeInTabs during config/script normalization/load rather than letting them reach the live executor; modify the normalization or loading path that produces Action instances (the code matching Action in configured_client.rs) to return Err(MuxError::invalid_input(...)) with a clear message when any of those variants are encountered, so the config fails fast; update the existing match that currently defers with "not supported by the live executor yet" to emit this early error and add/adjust tests that assert config loading rejects these variants.
🤖 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 39-42: The Release(MouseButton) variant cannot represent SGR-style
button-up events (SGR uses code 3 with no button identity) so the parser
currently drops standard release events; change the MouseEvent/MouseButton
representation to allow unknown/unspecified button on release (e.g.,
Release(Option<MouseButton>) or a distinct ReleaseUnknown variant) and update
the parser logic that handles SGR reports (the mouse parsing function that maps
SGR codes to events) to map SGR code 3 to a release event with None/Unknown
instead of dropping it; also update all usages of Release(MouseButton)
(including the Release handling sites mentioned at the other location) to handle
the Option/Unknown case and preserve click-up/drag-end semantics.
- Around line 593-610: The code enables raw mode via set_terminal_mode(input_fd,
&raw_mode) before performing the fallible write!/flush of
TERMINAL_ENTER_SEQUENCE, so if the write fails the function returns early and
Drop never runs to restore original_mode; change enter() to restore the termios
on any error after set_terminal_mode by wrapping the write/flush in a
match/if-let that on Err calls set_terminal_mode(input_fd, &original_mode) (or
use a short-lived guard that restores on drop) before returning the error,
referencing input_fd, original_mode, raw_mode, set_terminal_mode,
TERMINAL_ENTER_SEQUENCE and the type whose Drop currently restores the mode.
In `@crates/embers-cli/tests/interactive.rs`:
- Around line 171-205: Extract the repeated scrollback setup (building the long
printf string, calling harness.write_all(...), and awaiting the final visible
line) into a small test helper such as populate_scrollback_or_wait(harness: &mut
Harness, lines: usize) or write_long_output_and_wait(harness: &mut Harness,
lines: usize, final_line: &str) and call it from both
page_up_enters_local_scrollback_and_shows_indicator and
local_selection_yank_emits_osc52_clipboard_sequence; move the printf/echo
construction and the read_until_contains("line-N", ...) + sleep into that
helper, return any output needed by the tests, and update the two tests to call
this helper to remove the duplicated block while preserving the same assertions
and timing.
In `@crates/embers-cli/tests/sessions.rs`:
- Around line 15-22: shutdown_spawned_server currently reads the pid file
immediately and can race with socket creation; change it to wait for the pid
file to appear before reading: in async fn shutdown_spawned_server use pid_path
(the socket_path.with_extension("pid")) and poll for existence (e.g., loop with
tokio::time::sleep and a small delay, or use tokio::time::timeout around a retry
loop) until fs::metadata(&pid_path) or read_to_string succeeds or a sensible
timeout elapses, then continue parsing the pid as before; apply the same
wait-before-read pattern to the other occurrence referenced (lines ~119-120).
In `@crates/embers-client/Cargo.toml`:
- Around line 21-24: Remove the redundant tokio entry in the [dev-dependencies]
section of Cargo.toml: since tokio is already declared under [dependencies],
delete the tokio.workspace = true line (the redundant declaration in the
[dev-dependencies] block) so tests inherit the existing tokio dependency and
avoid duplication.
In `@crates/embers-client/src/client.rs`:
- Around line 207-217: resync_for_event currently calls resync_session for
NodeChanged and FloatingChanged but never calls resync_detached_buffers, so
detached-but-not-killed buffers remain stale; update resync_for_event (handling
ServerEvent::NodeChanged, ServerEvent::FloatingChanged, and possibly
ServerEvent::SessionClosed) to await resync_detached_buffers() after
resync_session(...) (or call the full-resync path) so the reducer is told which
buffers are detached vs killed and detached buffer state is cleaned up.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 843-868: resolve_tree_buffer currently returns only a BufferId for
newly created buffers (via create_buffer) which loses ownership info and
prevents callers from rolling back on RPC failure; change resolve_tree_buffer to
return both the BufferId and ownership/handle metadata (e.g., an Option or enum
indicating "newly_spawned" vs "existing") so callers like SplitCurrent,
OpenFloating, InsertTab*, and ReplaceNode can detect when a buffer was created
by this helper and detach/kill it on error; update resolve_tree_buffer's
signature and all call sites to accept the new return shape and implement the
rollback/cleanup path (use the same detach/kill logic pattern as the CLI flows)
while preserving existing behavior for BufferAttach and BufferCurrent cases that
should return no ownership flag.
In `@crates/embers-client/src/controller.rs`:
- Around line 66-73: The match arms for KeyEvent::Ctrl(ch) and KeyEvent::Alt(ch)
drop any unhandled chords instead of forwarding them to the focused buffer;
update the KeyEvent::Ctrl and KeyEvent::Alt branches in controller.rs so that
after handling the explicit navigation/tab cases (NavigationDirection mapping
and tab handling) you have a fallback that converts Ctrl+char into the
corresponding control byte (e.g., ch & 0x1f) and Alt+char into an ESC-prefixed
byte sequence (escape byte + ch) and send those bytes to the same code path that
writes to the focused buffer (reuse the existing send/write-to-focused-buffer
logic used for other KeyEvent variants) so unbound Ctrl/Alt inputs are forwarded
rather than dropped.
In `@crates/embers-client/src/grid.rs`:
- Around line 195-206: The rect drawing functions (fill_rect and
draw_box_styled) currently clamp only origin via clamp_i32_to_u16, which leaves
width/height unchanged and causes negative-origin rectangles to paint
incorrectly; before looping call a rect/bounds intersection routine to clip the
input Rect to the grid viewport (use the grid's width/height as bounds), update
rect.origin and rect.size to the intersected values, and return early if the
intersection is empty; ensure subsequent calls to put_char_styled use the
clipped origin/size (not the original rect) so negative offsets or partially
off-screen rectangles are handled correctly.
In `@crates/embers-client/src/input/keyparse.rs`:
- Around line 5-22: The KeyToken enum is missing common terminal navigation keys
(Home, End, Insert, Delete); update the KeyToken definition (enum KeyToken) to
add Home, End, Insert, and Delete variants, and then update any match arms,
parsers, or conversion functions that construct or consume KeyToken (e.g., key
parsing code and any From/Into implementations) to handle these new variants so
they serialize/compare and behave consistently with existing keys like
Up/Down/Left/Right and PageUp/PageDown.
In `@crates/embers-client/src/presentation.rs`:
- Around line 581-605: The inset_border function currently has a conditional
that duplicates logic; replace both branches by a single return that always
constructs a Rect with origin.x/y incremented by 1 and size.width/height
computed using saturating_sub(2) so it safely handles small dimensions; update
the function inset_border (and its use of Rect, Point, Size) to remove the
conditional and always use saturating_sub for width and height.
- Line 446: The current silent cast of divider_count =
child_count.saturating_sub(1) as u16 can truncate values > u16::MAX; replace the
direct as-cast with a safe conversion using u16::try_from (or clamp/min) so
overflow is handled explicitly (e.g., compute child_count.saturating_sub(1) then
attempt u16::try_from and on Err set to u16::MAX or otherwise clamp to
u16::MAX), and/or add a brief comment documenting the assumption about
child_count bounds; refer to the divider_count and child_count variables when
making this change.
In `@crates/embers-client/src/scripting/context.rs`:
- Around line 45-46: The formatting for the assignment to current_buffer_id is
off; run rustfmt/cargo fmt to reformat this expression (the line using
current_buffer_id.or_else(||
presentation.and_then(PresentationModel::focused_buffer_id))) so it matches the
project's style. Locate the code around the current_buffer_id variable and
presentation/PresentationModel::focused_buffer_id usage and apply cargo fmt (or
adjust spacing/line breaks to match rustfmt) to satisfy the CI formatting check.
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 1548-1557: The which function currently returns the first
candidate where candidate.is_file() is true; change it to also verify the file
is executable on Unix by checking the permission bits (use
std::os::unix::fs::PermissionsExt and test metadata.permissions().mode() & 0o111
!= 0) before returning the PathBuf from which, while keeping the existing
behavior on non-Unix targets (i.e., still require is_file() but skip the
exec-bit check). Update the candidate check in fn which(name: &str) ->
Option<PathBuf> to call candidate.metadata() and validate both is_file() and, on
Unix, the executable bit via the PermissionsExt API.
In `@crates/embers-client/tests/script_engine.rs`:
- Around line 154-159: The assertion block around ScriptEngine::load is not
formatted to rustfmt standards; run cargo fmt (or rustfmt) and reflow the
match/assert so it matches project style (keep the match on
ScriptEngine::load(&source) returning Err(error) assigned to let error, then
call assert!(error.to_string().contains("unknown mode option(s): on_entter")));
ensure spacing/line breaks around the match arm and the assert follow rustfmt,
then re-run cargo fmt --check to confirm; reference: ScriptEngine::load and the
local variable error.
---
Duplicate comments:
In `@crates/embers-cli/src/lib.rs`:
- Around line 550-563: The /tmp fallback created by default_runtime_dir() is
unsafe; before returning or ever calling server_is_available() or
ensure_server_process(), validate that the parent directory (e.g., /tmp) entry
for format!("embers-{}", effective_uid()) exists and is owned by effective_uid()
and has mode 0o700 (no group/other access); if the ownership/mode check fails,
refuse to use that fallback (return an error or fall back to a different
non-shared path) and ensure the same validation is performed in any other code
path that probes the same fallback socket (e.g., the code referenced around
ensure_server_process() / server_is_available()) so you never touch or connect
to a socket in a predictable shared directory without verifying owner and 0700
permissions first.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 832-839: Detect and reject Action::ReplaceFloatingRoot,
Action::WrapNodeInSplit, and Action::WrapNodeInTabs during config/script
normalization/load rather than letting them reach the live executor; modify the
normalization or loading path that produces Action instances (the code matching
Action in configured_client.rs) to return Err(MuxError::invalid_input(...)) with
a clear message when any of those variants are encountered, so the config fails
fast; update the existing match that currently defers with "not supported by the
live executor yet" to emit this early error and add/adjust tests that assert
config loading rejects these variants.
In `@crates/embers-client/tests/repo_config.rs`:
- Around line 214-223: The initializer for LoadedConfigSource inside
repository_config_engine is not rustfmt-formatted; run rustfmt (cargo fmt) or
manually reformat the block so the struct fields and chained
PathBuf::from(...).join(...) call conform to project style, keeping the function
name repository_config_engine, the ScriptEngine::load(...) call,
LoadedConfigSource { origin: ConfigOrigin::Explicit, path:
Some(PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/repository_config.rhai")),
source: include_str!("fixtures/repository_config.rhai").to_owned(), source_hash:
0, } unchanged in semantics but with proper spacing/indentation, then re-run
cargo fmt --check to ensure CI passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1db538b9-9a9a-4800-bb79-1c84520f79ca
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (75)
.github/actions/install-flatc/action.yml.github/workflows/ci.ymlCargo.tomlcrates/embers-cli/Cargo.tomlcrates/embers-cli/src/bin/embers-cli.rscrates/embers-cli/src/interactive.rscrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-cli/tests/sessions.rscrates/embers-cli/tests/windows.rscrates/embers-client/Cargo.tomlcrates/embers-client/src/client.rscrates/embers-client/src/config/error.rscrates/embers-client/src/config/loader.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/controller.rscrates/embers-client/src/grid.rscrates/embers-client/src/input/keymap.rscrates/embers-client/src/input/keyparse.rscrates/embers-client/src/input/mod.rscrates/embers-client/src/input/modes.rscrates/embers-client/src/lib.rscrates/embers-client/src/presentation.rscrates/embers-client/src/renderer.rscrates/embers-client/src/scripting/context.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/error.rscrates/embers-client/src/scripting/harness.rscrates/embers-client/src/scripting/mod.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/src/testing.rscrates/embers-client/tests/config_loading.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/context.rscrates/embers-client/tests/controller.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/fixtures/repository_config.rhaicrates/embers-client/tests/presentation.rscrates/embers-client/tests/reducer.rscrates/embers-client/tests/renderer.rscrates/embers-client/tests/repo_config.rscrates/embers-client/tests/script_actions.rscrates/embers-client/tests/script_engine.rscrates/embers-client/tests/support/mod.rscrates/embers-core/src/lib.rscrates/embers-core/src/snapshot.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/framing.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-server/src/buffer_runtime.rscrates/embers-server/src/config.rscrates/embers-server/src/lib.rscrates/embers-server/src/model.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_move.rscrates/embers-server/tests/model_state.rscrates/embers-server/tests/session_root_tabs.rscrates/embers-server/tests/split_layout.rscrates/embers-test-support/src/protocol.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/nested_tabs.rscrates/embers-test-support/tests/session_root_tabs.rscrates/embers-test-support/tests/split_layout.rs
There was a problem hiding this comment.
Actionable comments posted: 20
♻️ Duplicate comments (2)
crates/embers-client/src/controller.rs (1)
91-109:⚠️ Potential issue | 🟠 MajorPreserve
Alt+digitpassthrough when tab selection doesn't apply.Line 94 short-circuits on
presentation.focused_tabs()?, and Lines 95-97 returnNonefor out-of-range digits. That dropsAlt+1..9entirely instead of forwarding the usual ESC-prefixed bytes to the focused buffer, so these chords stop working in single-tab or non-tab layouts.💡 Possible fix
KeyEvent::Alt(ch) if ('1'..='9').contains(&ch) => { let index = ch.to_digit(10)?.saturating_sub(1); let index_usize = usize::try_from(index).ok()?; - let tabs = presentation.focused_tabs()?; - if index_usize >= tabs.tabs.len() { - return None; + if let Some(tabs) = presentation.focused_tabs() { + if index_usize < tabs.tabs.len() { + return Some(ClientMessage::Node(NodeRequest::SelectTab { + request_id, + tabs_node_id: tabs.node_id, + index, + })); + } } - Some(ClientMessage::Node(NodeRequest::SelectTab { - request_id, - tabs_node_id: tabs.node_id, - index, - })) + let mut encoded = [0; 4]; + let mut bytes = vec![0x1b]; + bytes.extend_from_slice(ch.encode_utf8(&mut encoded).as_bytes()); + input_request(presentation, request_id, bytes) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/controller.rs` around lines 91 - 109, The Alt+digit arm must fall back to forwarding ESC+digit bytes when tab selection is not applicable: instead of early-returning None on presentation.focused_tabs()? failure or on an out-of-range index, attempt to obtain tabs via presentation.focused_tabs(), and if that yields Some(tabs) and the computed index (from ch.to_digit(10)?) is within tabs.tabs.len(), send ClientMessage::Node(NodeRequest::SelectTab { request_id, tabs_node_id: tabs.node_id, index }); otherwise build the ESC-prefixed bytes (as done in the current Alt(ch) fallback) and call input_request(presentation, request_id, bytes) so Alt+1..9 is forwarded to the focused buffer when selection doesn't apply; keep using the same helpers (presentation.focused_tabs(), input_request, KeyEvent::Alt match arm, and NodeRequest::SelectTab) to locate changes.crates/embers-client/src/configured_client.rs (1)
259-260:⚠️ Potential issue | 🟠 MajorMouse forwarding is still one row too high.
Line 259 excludes the first visible row, and Lines 2058-2067 subtract an extra row again when encoding the SGR mouse event. First-row clicks and wheels are dropped, and later rows are forwarded one line too high.
Suggested fix
- let point_in_content = point.y > target_leaf.rect.origin.y; + let point_in_content = point.y >= target_leaf.rect.origin.y;- let origin_y = clamp_origin(leaf.rect.origin.y).saturating_add(1); + let origin_y = clamp_origin(leaf.rect.origin.y); let local_column = event .column .checked_sub(origin_x) .ok_or_else(|| MuxError::invalid_input("mouse event fell outside pane bounds"))?; let local_row = event .row .checked_sub(origin_y) .ok_or_else(|| MuxError::invalid_input("mouse event fell outside pane content"))?;Also applies to: 2058-2067
🤖 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 259 - 260, The bug comes from excluding the first visible row and then subtracting an extra row when encoding SGR mouse events: change the point check to include the first visible row by using >= (replace let point_in_content = point.y > target_leaf.rect.origin.y with >=) so first-row clicks/wheels are not dropped, and in the SGR mouse encoding block (the code that computes the exported row using target_leaf.rect.origin.y / the expression that currently subtracts 1) remove the extra "- 1" adjustment so the forwarded row matches the visual row; adjust any variables that compute exported_row/exported_y accordingly to stop shifting events one line too high.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/install-flatc/action.yml:
- Around line 27-43: The loop currently sets asset to the first successfully
downloaded candidate and only checks checksum after the loop, which prevents
trying subsequent candidates on a checksum mismatch; change the logic inside the
candidates loop so that after a successful curl you compute actual_sha256
(sha256sum flatbuffers-bin/flatc.zip) and compare it to ${{ inputs.sha256 }},
and only set asset and break if the checksum matches; if it doesn't match,
delete or overwrite flatbuffers-bin/flatc.zip and continue the loop to try the
next candidate, leaving the existing post-loop check for an empty asset
unchanged.
In `@crates/embers-cli/src/interactive.rs`:
- Around line 339-346: The match arm for control chords only covers 0x01..=0x1a;
extend it to include 0x00 and 0x1c..=0x1f so Ctrl-@, Ctrl-\, Ctrl-], Ctrl-^ and
Ctrl-_ are emitted by the interactive path: update the match on first in the
function that builds TerminalEvent (the arm currently using 0x01..=0x1a ->
TerminalEvent::Key(KeyEvent::Ctrl(...))) to match 0x00 | 0x01..=0x1a |
0x1c..=0x1f, and compute the corresponding printable control letter by
normalizing the byte to a printable ASCII letter (e.g. (first | 0x40) cast to
char then to_ascii_lowercase()) before constructing KeyEvent::Ctrl; keep
existing handling for 0x1b (read_escape_event), decode_utf8_key and other arms
unchanged.
In `@crates/embers-cli/tests/interactive.rs`:
- Around line 91-95: The test currently computes final_line with
lines.saturating_sub(1), causing an off-by-one and allowing the helper to
continue before the actual final emitted line appears; update the final_line
computation used by harness.read_until_contains to target the true final emitted
line (use format!("line-{}", lines) or otherwise use the raw lines value instead
of saturating_sub(1)) so that harness.read_until_contains waits for the actual
last line before proceeding (affects final_line, lines, and the call to
harness.read_until_contains).
In `@crates/embers-client/src/configured_client.rs`:
- Around line 511-523: The code sets the new mode via
input_state.set_mode(mode.clone()) before calling
active_script().run_enter_hook, so if run_enter_hook returns an error the
input_state remains changed and leave-hook actions are lost; modify the error
path of the run_enter_hook call (the map_err closure) to reset
input_state.set_mode(previous_mode.clone()) (or previous_mode) before returning
the MuxError, ensuring you reference input_state.set_mode, run_enter_hook,
run_leave_hook, and context_for when making the change so the previous mode is
restored on error.
In `@crates/embers-client/src/grid.rs`:
- Around line 253-256: The math that computes right and bottom can overflow when
adding rect.origin (i32) and rect.size (u32) — update the calculations in
clip_rect so the sum is done in a wider signed type (e.g., cast origin and size
to i64), perform the addition there, then clamp/convert the result back to i32
before taking the min with i32::from(self.width)/i32::from(self.height);
alternatively use checked_add/saturating_add on rect.origin.x/y with
i32::from(rect.size.width/height) and on overflow saturate to a safe bound (such
as i32::MAX or self.width/self.height) so right and bottom cannot overflow
(refer to rect.origin.x, rect.origin.y, rect.size.width, rect.size.height,
self.width, self.height and the clip_rect logic).
In `@crates/embers-client/src/presentation.rs`:
- Around line 292-297: The projection recomputes subtree_activity() and
subtree_buffer_count() for each tab item causing repeated full-subtree walks;
fix by computing those values once into lookup maps keyed by NodeId before the
.map that builds TabItem (e.g., create HashMap<NodeId, Activity> and
HashMap<NodeId, usize> using the same traversal logic used by
subtree_activity/subtree_buffer_count), then replace calls to
subtree_activity(self.state, tab.child_id) and subtree_buffer_count(self.state,
tab.child_id) inside the TabItem mapping with map lookups; apply the same
memoization refactor for the other projection range mentioned (around the
354-417 block) so both projections reuse the cached results.
- Around line 151-153: focused_floating_id() currently only returns a FloatingId
when focused_leaf() yields a projected leaf, which misses cases where a floating
frame is focused but produced no leaf; change the function to fall back to the
floating frame focus state: first try self.focused_leaf().and_then(|leaf|
leaf.floating_id), and if that returns None, iterate over your floating frames
collection (e.g., self.floating_frames or similar) to find the frame where
FloatingFrame.focused is true and return its id (FloatingId); update the
function to return that id when present so focused-floating actions still have a
target.
In `@crates/embers-client/src/renderer.rs`:
- Around line 99-117: The left/right/center segments are each computed against
the full row and can overlap; change the custom-bar logic to first compute
left_width = sum(display_width(segment.text) for bar.left) and right_width
similarly, clamp each to the available width, render left at x..left_end
(left_end = x + min(left_width, width)), render right at right_start..end_x
(right_start = end_x.saturating_sub(min(right_width, width))), then compute the
center available span as center_span = right_start.saturating_sub(left_end) and
center_width = sum(display_width(segment.text) for bar.center) clamped to
center_span, position the center centered within that span (center_x = left_end
+ (center_span.saturating_sub(center_width) / 2)) and call render_bar_segments
for left, right, then center using these clamped positions so no section
overwrites another; use the existing symbols bar, render_bar_segments,
display_width, x, end_x, width, left_end, right_start, center_span, center_x.
In `@crates/embers-client/src/scripting/context.rs`:
- Around line 59-60: The current code uses window.visible to determine floating
visibility which is too coarse; when a presentation snapshot exists, derive
visible floating IDs from presentation.floating and use that set as the source
for FloatingRef.visible so ctx.visible_floating() matches rendered UI. Update
the block around geometry_by_node / visible_node_ids so that if
presentation.is_some() you build a visible_floating_ids from
presentation.floating (e.g., collect IDs into a HashSet) and pass that into
whatever constructs FloatingRef.visible (or make ctx.visible_floating() consult
this set), and apply the same change to the other similar block handling visible
floats (the later code referenced in the comment) so both paths prefer
presentation-derived visibility when available.
In `@crates/embers-client/src/scripting/engine.rs`:
- Around line 48-64: The bug is that prepending built-ins into composed_source
shifts user-facing error line numbers; instead compile/evaluate the builtins
separately so errors map to their original sources and only compile/evaluate the
user source against an already-populated scope. Concretely: compile and eval the
builtins string (BUILTIN_CONFIG_SOURCE / builtins) with its own compile/eval
calls and map errors with ScriptError::compile/runtime using the builtins
source, then create registration_scope(), push constants, and compile/eval only
source.source (not composed_source) with engine.compile and
engine.eval_ast_with_scope so ScriptError uses the correct user file positions
(references: composed_source, engine.compile, eval_ast_with_scope,
ScriptError::compile, ScriptError::runtime, registration_scope).
In `@crates/embers-client/src/scripting/harness.rs`:
- Around line 31-47: The function resolve_notation currently panics when
parse_key_sequence yields no keys (last_resolution.expect(...)); instead detect
the empty-case and return an Err(crate::input::KeyParseError) from the function
API. Replace the expect call with logic that returns Ok(the resolution) when
last_resolution is Some(...) and returns an appropriate KeyParseError (e.g., a
new EmptyNotation/EmptySequence variant or an existing variant carrying a
message) when None; update or add the KeyParseError variant in crate::input if
one does not already exist. Ensure references to resolve_notation,
last_resolution, and parse_key_sequence are used so the change is located
correctly.
In `@crates/embers-client/src/scripting/model.rs`:
- Around line 205-208: TabsSpec currently allows an empty tabs Vec or an
out-of-range active index; make those states unrepresentable by adding a
validated constructor and enforcing it across deserialization and creation
sites: implement TabsSpec::try_new(tabs: Vec<TabSpec>, active: usize) ->
Result<TabsSpec, Error> (or impl TryFrom<(Vec<TabSpec>, usize)> for TabsSpec)
that returns Err if tabs.is_empty() or active >= tabs.len(), and wire this into
any places that construct TabsSpec (including custom serde Deserialize impl or
using a deserialize_with helper) so invalid configs fail at load time instead of
during render.
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 122-136: The validator currently only checks top-level Action
variants; update validate_live_actions to recurse into Action::Chain by adding a
match arm for Action::Chain { actions: inner, .. } (or similar) that iterates
inner and calls validate_live_actions (or a helper like
validate_live_actions_recursive) on inner actions, so any nested
ReplaceFloatingRoot, WrapNodeInSplit, WrapNodeInTabs (the existing forbidden
variants) are detected; ensure recursion terminates and reuse existing error
formatting for consistency.
- Around line 1568-1578: In function which, replace the early-return use of
candidate.metadata().ok()? so a missing candidate doesn't return None; instead,
handle metadata().ok() with an if let or match and continue to the next PATH
entry when metadata is None (so the loop keeps scanning), then proceed with the
existing checks (is_file and unix permissions). This targets the
candidate.metadata().ok()? expression in which and ensures the loop continues
rather than exiting on the first missing file.
In `@crates/embers-client/src/state.rs`:
- Around line 128-132: The loop that unconditionally clears
buffer.attachment_node_id for ids in
previous_attached_buffers.difference(¤t_buffer_ids) is removing
attachments that may have been reassigned by another session; change it to first
fetch the buffer (self.buffers.get_mut(buffer_id)) and only set
buffer.attachment_node_id = None if the current attachment equals this session's
node id (the same session identifier used to build previous_attached_buffers,
e.g., session.node_id or current_session_id); otherwise leave the attachment
untouched so a reassignment by another session is preserved.
In `@crates/embers-client/tests/configured_client.rs`:
- Around line 40-54: The helper session_snapshot_from_state currently clones all
global nodes, buffers, and floating entries; change it to produce a
session-scoped snapshot by selecting only nodes whose session_id matches the
provided session_id (filter state.nodes by node.session_id), select floating
records filtered by the same session_id (filter state.floating by
floating.session_id), and then select buffers only for the node IDs included in
that filtered node list (filter state.buffers by buffer.node_id in the chosen
node IDs). Keep using the same SessionSnapshot construction and existing symbols
(session, nodes, buffers, floating, session_id, state.*) but restrict the
collected entries as described.
In `@crates/embers-client/tests/e2e.rs`:
- Around line 409-412: The waits are matching stale markers ("popup-base" /
"moved-buffer") written before the popup close/detach/reattach, so change each
check that uses connection.wait_for_capture_contains(base_buffer, "popup-base",
...) or similar to first emit a fresh uniquely-named marker into the buffer
after the lifecycle transition (e.g., write a new marker string into base_buffer
or moved_buffer) and then call connection.wait_for_capture_contains(...) for
that fresh marker; apply the same pattern to the other two occurrences (the
checks around the popup close/detach/reattach) so each lifecycle step is
validated by a marker written after the step rather than an earlier one.
In `@crates/embers-client/tests/fixtures/repository_config.rhai`:
- Around line 1-2: Remove the user-specific absolute path in the top comment and
replace it with a generic or relative reference (e.g.,
"project/nix-config/home/base/shell/default.nix" or "config/nix/default.nix");
locate the comment that currently contains
"/Users/emma/Development/nix-config/home/base/shell/default.nix" in the
repository_config.rhai fixture and update it to a non-personal, generic path or
remove the path entirely to avoid leaking personal directory structures.
- Around line 22-30: The shell_tree fixture hardcodes "/bin/zsh" which breaks
portability; update the call inside shell_tree (the tree_api.buffer_spawn
invocation) to use an env lookup (e.g., "/usr/bin/env" with "zsh" as the
argument) or make the shell path configurable via an argument or env variable so
systems with zsh elsewhere (like /usr/bin/zsh or Homebrew installs) can find it;
adjust the parameters passed to tree_api.buffer_spawn accordingly and preserve
title and cwd behavior in shell_tree.
In `@crates/embers-client/tests/reducer.rs`:
- Around line 387-396: The test scripts only the initial
SessionRequest::Get/SessionSnapshot exchange but process_next_event() triggers
additional resync work after applying the NodeChanged event, causing the
"scripted transport has no queued exchange" failure; update the transport script
in the test by queuing the full post-resync exchanges (e.g., add the follow-up
ClientMessage and ServerResponse exchanges that fetch the newly active tree
contents after the SessionSnapshot) or modify session_snapshot(1, 1) so the
snapshot topology remains unchanged and no extra round-trip is required; locate
the scripted exchange block containing
ClientMessage::Session(SessionRequest::Get { RequestId(1), SessionId(1) }) and
append the necessary subsequent exchanges that process_next_event() will
request.
---
Duplicate comments:
In `@crates/embers-client/src/configured_client.rs`:
- Around line 259-260: The bug comes from excluding the first visible row and
then subtracting an extra row when encoding SGR mouse events: change the point
check to include the first visible row by using >= (replace let point_in_content
= point.y > target_leaf.rect.origin.y with >=) so first-row clicks/wheels are
not dropped, and in the SGR mouse encoding block (the code that computes the
exported row using target_leaf.rect.origin.y / the expression that currently
subtracts 1) remove the extra "- 1" adjustment so the forwarded row matches the
visual row; adjust any variables that compute exported_row/exported_y
accordingly to stop shifting events one line too high.
In `@crates/embers-client/src/controller.rs`:
- Around line 91-109: The Alt+digit arm must fall back to forwarding ESC+digit
bytes when tab selection is not applicable: instead of early-returning None on
presentation.focused_tabs()? failure or on an out-of-range index, attempt to
obtain tabs via presentation.focused_tabs(), and if that yields Some(tabs) and
the computed index (from ch.to_digit(10)?) is within tabs.tabs.len(), send
ClientMessage::Node(NodeRequest::SelectTab { request_id, tabs_node_id:
tabs.node_id, index }); otherwise build the ESC-prefixed bytes (as done in the
current Alt(ch) fallback) and call input_request(presentation, request_id,
bytes) so Alt+1..9 is forwarded to the focused buffer when selection doesn't
apply; keep using the same helpers (presentation.focused_tabs(), input_request,
KeyEvent::Alt match arm, and NodeRequest::SelectTab) to locate changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97b9f97d-5fc8-403b-8a0c-03a1cda7718d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (76)
.github/actions/install-flatc/action.yml.github/workflows/ci.ymlCargo.tomlcrates/embers-cli/Cargo.tomlcrates/embers-cli/src/bin/embers-cli.rscrates/embers-cli/src/interactive.rscrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-cli/tests/sessions.rscrates/embers-cli/tests/windows.rscrates/embers-client/Cargo.tomlcrates/embers-client/benches/search_yank.rscrates/embers-client/src/client.rscrates/embers-client/src/config/error.rscrates/embers-client/src/config/loader.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/controller.rscrates/embers-client/src/grid.rscrates/embers-client/src/input/keymap.rscrates/embers-client/src/input/keyparse.rscrates/embers-client/src/input/mod.rscrates/embers-client/src/input/modes.rscrates/embers-client/src/lib.rscrates/embers-client/src/presentation.rscrates/embers-client/src/renderer.rscrates/embers-client/src/scripting/context.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/error.rscrates/embers-client/src/scripting/harness.rscrates/embers-client/src/scripting/mod.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/src/testing.rscrates/embers-client/tests/config_loading.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/context.rscrates/embers-client/tests/controller.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/fixtures/repository_config.rhaicrates/embers-client/tests/presentation.rscrates/embers-client/tests/reducer.rscrates/embers-client/tests/renderer.rscrates/embers-client/tests/repo_config.rscrates/embers-client/tests/script_actions.rscrates/embers-client/tests/script_engine.rscrates/embers-client/tests/support/mod.rscrates/embers-core/src/lib.rscrates/embers-core/src/snapshot.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/framing.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/family_round_trip.rscrates/embers-server/src/buffer_runtime.rscrates/embers-server/src/config.rscrates/embers-server/src/lib.rscrates/embers-server/src/model.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_move.rscrates/embers-server/tests/model_state.rscrates/embers-server/tests/session_root_tabs.rscrates/embers-server/tests/split_layout.rscrates/embers-test-support/src/protocol.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/nested_tabs.rscrates/embers-test-support/tests/session_root_tabs.rscrates/embers-test-support/tests/split_layout.rs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/actions/install-flatc/action.yml (1)
43-47: 🧹 Nitpick | 🔵 TrivialRemove redundant checksum re-validation after the loop.
Checksum is already validated before
assetis set, so this second check is duplicate logic. Keeping a single validation path will simplify maintenance.♻️ Optional simplification
- actual_sha256="$(sha256sum flatbuffers-bin/flatc.zip | awk '{print $1}')" - if [[ "${actual_sha256}" != "${{ inputs.sha256 }}" ]]; then - echo "::error::flatc archive checksum mismatch for ${asset}: expected ${{ inputs.sha256 }}, got ${actual_sha256}" - exit 1 - fi - unzip -q flatbuffers-bin/flatc.zip -d flatbuffers-bin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/install-flatc/action.yml around lines 43 - 47, Remove the redundant checksum re-validation block that computes actual_sha256 and compares it to ${{ inputs.sha256 }} for the variable asset; the checksum is already validated earlier in the loop, so delete the lines that set actual_sha256 and the if-statement comparing actual_sha256 to ${{ inputs.sha256 }} (the echo "::error::..." and exit 1) to leave a single validation path.
🤖 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 774-795: The fit_width function uses UnicodeWidthStr::width on an
encoded temporary buffer for each character; replace that per-character sizing
with UnicodeWidthChar::width(ch). Update the loop in fit_width to call
UnicodeWidthChar::width(ch).unwrap_or(1) (or .map(|w| w).unwrap_or(1)) to
compute ch_width, keeping the rest of the logic (used, break, push, padding)
unchanged; this removes the temporary buffer and uses the more direct
UnicodeWidthChar API.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 1322-1338: The scroll delta arithmetic in async fn scroll_view_by
(using node_id, state.scroll_top_line and calling set_view_scroll_top) is
correct but non-obvious; add a short inline comment above the if
delta.is_negative() block that explains the intent: for negative deltas we
subtract the absolute value (scroll up) and for positive deltas we add (scroll
down), and that we use saturating_sub/saturating_add with i128 to avoid
under/overflow before clamping to zero and converting to u64; place the comment
near the current/delta/next computations so future readers understand the
unsigned_abs() usage.
---
Duplicate comments:
In @.github/actions/install-flatc/action.yml:
- Around line 43-47: Remove the redundant checksum re-validation block that
computes actual_sha256 and compares it to ${{ inputs.sha256 }} for the variable
asset; the checksum is already validated earlier in the loop, so delete the
lines that set actual_sha256 and the if-statement comparing actual_sha256 to ${{
inputs.sha256 }} (the echo "::error::..." and exit 1) to leave a single
validation path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac15c47b-f21d-42b3-892f-67347a98e74e
📒 Files selected for processing (28)
.github/actions/install-flatc/action.ymlcrates/embers-cli/src/interactive.rscrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/sessions.rscrates/embers-client/Cargo.tomlcrates/embers-client/benches/search_yank.rscrates/embers-client/src/client.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/controller.rscrates/embers-client/src/grid.rscrates/embers-client/src/input/keyparse.rscrates/embers-client/src/presentation.rscrates/embers-client/src/renderer.rscrates/embers-client/src/scripting/context.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/harness.rscrates/embers-client/src/scripting/model.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/src/state.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/controller.rscrates/embers-client/tests/e2e.rscrates/embers-client/tests/fixtures/repository_config.rhaicrates/embers-client/tests/reducer.rscrates/embers-client/tests/repo_config.rscrates/embers-client/tests/script_actions.rscrates/embers-client/tests/script_engine.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests
Chores
Stack