Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (73)
📝 WalkthroughWalkthroughAdds end-to-end client management: protocol types/messages for listing/getting/detaching/switching clients and ClientChanged events; codec, client APIs and resync logic, server client-lifecycle and dispatch updates, CLI commands and interactive attach detachment, tests, scripting/rhai accessors, and documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant MuxClient as MuxClient
participant Server as Server
participant Protocol as Protocol
CLI->>MuxClient: list_clients()
MuxClient->>Protocol: encode ClientRequest::List
Protocol->>Server: send envelope
Server->>Protocol: ClientsResponse
Protocol->>MuxClient: ServerResponse::Clients
MuxClient-->>CLI: return clients
CLI->>MuxClient: switch_client(client_id, target_session)
MuxClient->>Protocol: encode ClientRequest::Switch
Protocol->>Server: send envelope
Server->>Server: update client.current_session_id
Server->>Protocol: ServerEvent::ClientChanged
Protocol->>MuxClient: ServerEvent::ClientChanged
MuxClient->>MuxClient: fetch own client record (Get) and resync target session
MuxClient-->>CLI: acknowledge
CLI->>MuxClient: detach_client(client_id)
MuxClient->>Protocol: encode ClientRequest::Detach
Protocol->>Server: send envelope
Server->>Server: detach client (self or remote)
Server->>Protocol: ServerEvent::ClientChanged
Protocol->>MuxClient: ServerEvent::ClientChanged
MuxClient-->>CLI: acknowledge
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 HTMLHint (1.9.2)docs/config-api-book/print.html[{"file":"/docs/config-api-book/print.html","messages":[{"type":"warning","message":"A <iframe> element must have an accessible name.","raw":"<iframe class="sidebar-iframe-outer" src="toc.html">","evidence":" <iframe class="sidebar-iframe-outer" src="toc.html"></iframe>","line":109,"col":24,"rule":{"id":"frame-title-require","description":"A or <iframe> element must have an accessible name.","link":"https://htmlhint.com/rules/frame-title-require"}},{"type":"warning","message":"The type attribute must be present on elements.","raw":"<button role="menuitem" class="theme" id="mdbook-theme-default_theme">","evidence":" <li role="none"><button role="menuitem" class="theme" id="mdbook-theme-default_theme">Auto","line":129,"col":52,"rule":{"id":"button-type-require","description":"The type attribute of a element must be present with a valid value: "button", "submit", or "reset".","lin ... [truncated 144358 characters] ... on> element must be present with a valid value: "button", "submit", or "reset".","link":"https://htmlhint.com/rules/button-type-require"}},{"type":"warning","message":"The type attribute must be present on elements.","raw":"<button group="session_id" id="link-session_id-Description" class="tablinks active" onclick="openTab(event, 'session_id', 'Description')">","evidence":"<button group="session_id" id="link-session_id-Description" class="tablinks active" onclick="openTab(event, 'session_id', 'Description')">","line":4534,"col":8,"rule":{"id":"button-type-require","description":"The type attribute of a element must be present with a valid value: "button", "submit", or "reset".","link":"https://htmlhint.com/rules/Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Mar 22, 2026
Pajn
force-pushed
the
client-switch
branch
2 times, most recently
from
4617166 to
bc396bd
Compare
Pajn
force-pushed
the
client-switch
branch
2 times, most recently
from
b86c41d to
8dbc2b0
Compare
Pajn
force-pushed
the
session-rename
branch
2 times, most recently
from
0fc0acc to
f7ae9a2
Compare
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
1 similar comment
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
1 similar comment
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
crates/embers-server/src/server.rs (1)
1765-1799:⚠️ Potential issue | 🟠 MajorDon't build
client_record()from the filtered list view.This still hides live connections whenever they have neither a current session nor subscriptions. That means
ClientRequest::Get { client_id: None }can returnNotFoundon a fresh connection, andclear_client_session()can now skip theClientChanged(None)notification entirely once it clearscurrent_session_idon an unsubscribed client. Keeplist_clients()filtered if you want, butclient_record()needs a direct lookup againstself.clientsplus subscription metadata.Possible fix
- async fn client_record(&self, client_id: u64) -> Option<ClientRecord> { - self.list_clients() - .await - .into_iter() - .find(|client| client.id == client_id) - } + async fn client_record(&self, client_id: u64) -> Option<ClientRecord> { + let clients = self.clients.lock().await; + let client = clients.get(&client_id)?; + let subscriptions = self.subscriptions.lock().await; + + let mut subscribed_all_sessions = false; + let mut subscribed_session_ids = Vec::new(); + for subscription in subscriptions.values() { + if subscription.connection_id != client_id { + continue; + } + match subscription.session_id { + Some(session_id) => subscribed_session_ids.push(session_id), + None => subscribed_all_sessions = true, + } + } + + subscribed_session_ids.sort_by_key(|session_id| session_id.0); + subscribed_session_ids.dedup(); + + Some(ClientRecord { + id: client_id, + current_session_id: client.current_session_id, + subscribed_all_sessions, + subscribed_session_ids, + }) + }Also applies to: 1802-1807
🤖 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 1765 - 1799, The current code builds ClientRecord from the filtered iterator in list_clients which hides live clients that have neither current_session_id nor subscriptions; change the approach so client_record (the function that constructs a ClientRecord for a given client) does a direct lookup in self.clients and merges subscription metadata from self.subscriptions instead of relying on the already-filtered view in list_clients; specifically, implement client_record(client_id) to lock self.clients and self.subscriptions, fetch the client by id, compute subscribed_all_sessions and subscribed_session_ids from self.subscriptions, and always return a ClientRecord for an existing client (even if it has no session/subscriptions); keep list_clients() as a filtered enumerator if desired but call the new client_record(client_id) for each client id (and update any other call sites around lines ~1802-1807 to use this direct-lookup client_record).
🤖 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-protocol/src/codec.rs`:
- Around line 2233-2237: The Switch arm currently constructs
ClientRequest::Switch with SessionId(req.session_id()) even when the wire
session id is 0; change it to explicitly reject wire session id 0 by checking
req.session_id() and returning a decode error (or other appropriate failure)
instead of creating SessionId(0). Update the fb::ClientOp::Switch handling in
the decoder (the match arm creating ClientRequest::Switch) to validate
req.session_id() != 0 and fail fast (include a descriptive error message) when
it equals 0 so malformed frames are not accepted.
In `@crates/embers-protocol/src/types.rs`:
- Around line 314-330: The ClientRequest enum uses Option<u64> for client_id but
the codec reserves 0 as a sentinel, so Some(0) collapses to None; change the
type to a non-zero client identifier (e.g., Option<NonZeroU64> or a newtype like
ClientId wrapping NonZeroU64) and update encode/decode and any construction
sites to validate/require non-zero values; update all enum variants that include
client_id (e.g., the ClientRequest variants List/Get/Detach/Switch and the other
occurrences referenced around lines 466–470) so the codec and serializers treat
0 as invalid at the boundary and never allow Some(0) to be representable.
- Around line 675-677: ClientChangedEvent currently only carries the post-change
ClientRecord so ServerEvent::session_id() returns client.current_session_id and
subscribers on the previous session miss the notification; update the event to
include the previous session id (e.g. add an Option<SessionId> field like
`previous_session_id` to ClientChangedEvent) or adjust routing so
ServerEvent::session_id() for ClientChanged returns both session ids or a
special marker, and ensure the publisher in server-side routing checks that new
field (or special-case ClientChanged) when matching subscriptions instead of
relying solely on client.current_session_id.
In `@crates/embers-server/src/server.rs`:
- Around line 1809-1825: The detach_client flow currently triggers the
per-connection shutdown (the `shutdown` sender taken in detach_client) while the
connection's handler (`handle_connection` / its write task / `write_handle`) may
still be flushing the detach response, which can cause the response to be
dropped; change detach handling so graceful self-detach does not immediately
send shutdown: detect when `client_id` equals the current connection's id (or
when Detach was requested by the same connection via `ClientRequest::Detach {
client_id: None }`) and instead either (a) defer calling `shutdown.send(())`
until after the connection's outbound channel has drained and the `write_handle`
has been awaited, or (b) mark the client as detaching and have the supervisor
await the writer normally to flush the response before aborting; update
detach_client to return the shutdown sender (or a detaching flag) to allow the
caller to await writer completion, and adjust `handle_connection`/supervisor
logic to drain outbound messages and await `write_handle` for graceful detach
paths rather than immediately aborting the writer.
---
Duplicate comments:
In `@crates/embers-server/src/server.rs`:
- Around line 1765-1799: The current code builds ClientRecord from the filtered
iterator in list_clients which hides live clients that have neither
current_session_id nor subscriptions; change the approach so client_record (the
function that constructs a ClientRecord for a given client) does a direct lookup
in self.clients and merges subscription metadata from self.subscriptions instead
of relying on the already-filtered view in list_clients; specifically, implement
client_record(client_id) to lock self.clients and self.subscriptions, fetch the
client by id, compute subscribed_all_sessions and subscribed_session_ids from
self.subscriptions, and always return a ClientRecord for an existing client
(even if it has no session/subscriptions); keep list_clients() as a filtered
enumerator if desired but call the new client_record(client_id) for each client
id (and update any other call sites around lines ~1802-1807 to use this
direct-lookup client_record).
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b4324f5-f2e8-4161-bbf1-9dd00304abb4
📒 Files selected for processing (11)
crates/embers-cli/src/interactive.rscrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-client/src/client.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/state.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-server/src/server.rs
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 4 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/embers-server/src/server.rs (2)
1816-1840:⚠️ Potential issue | 🟠 MajorDon't derive
client_record()from the filtered client list.A live connection with no
current_session_idand no subscriptions disappears fromlist_clients(), so this helper can returnNonefor a freshly accepted client. That breaksClientRequest::Get { client_id: None }, andclear_client_session()can also skip theClientChangednotification after it sets the session back toNone.Also applies to: 1853-1858
🤖 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 1816 - 1840, The helper list_clients() currently returns None for a live client that has no current_session_id and no subscriptions, causing freshly accepted connections to vanish (breaking ClientRequest::Get with client_id: None and preventing clear_client_session() from emitting ClientChanged); fix by not filtering out connected clients—always construct a ClientRecord for every entry in self.clients (i.e., call the client_record() logic for each client_id in list_clients()), and only use the subscribed_all_sessions/subscribed_session_ids checks to populate session lists rather than to exclude the client; apply the same change to the duplicate block around the 1853–1858 logic so connected clients with no sessions/subscriptions are still returned and notifications are still emitted.
104-120:⚠️ Potential issue | 🟠 MajorSelf-detach can still drop its own
Okresponse.After queueing the detach response, this path returns immediately and Line 109 still aborts
write_handleunconditionally. Because the oneshot receiver lives in this same task, the send at Line 2039 does not flush anything; the acknowledgement can still be lost before it reaches the socket.Also applies to: 2037-2041
🤖 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 104 - 120, The code unconditionally aborts write_handle after awaiting read_handle which can cancel a self-initiated detach acknowledgement that is delivered via a local oneshot/ack channel; change the teardown so the task waits for the detach ack (or a signal from the read loop) before aborting write_handle: have read_handle return a result/enum or a boolean (e.g., self_detached or detach_ack_received) instead of just Err, check that value before calling write_handle.abort(), and if self-detached await the write_handle (or the oneshot ack) to ensure the detach response is flushed, then call write_runtime.cleanup_connection(connection_id). Ensure you update the code paths that send the detach response to signal the ack receiver the same way the read_handle result now communicates.
🤖 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-protocol/src/codec.rs`:
- Around line 158-199: Tests don't cover the new client codec branches that use
sentinel 0 ↔ Option conversions, so add round-trip encode/decode unit tests
using the codec for the ClientRequest variants and responses: specifically
create and encode/decode ClientRequest::Get(None),
ClientRequest::Switch(Some(client_id), session_id), plus ClientsResponse and
ClientChangedEvent messages to assert that optional client_id/session_id
round-trip correctly (0 maps to None and back). Locate encoding helpers like
encode_client_request and the fb::Envelope creation used for requests, reuse the
existing decode path to parse the envelope and compare the reconstructed
enums/structs to the originals, and add similar tests around the other codec
areas noted to ensure coverage for those branches.
In `@crates/embers-server/src/server.rs`:
- Around line 614-618: The code currently pushes ClientChanged directly into
client.sender inside the set_client_session success path (seen around
ServerResponse::Client construction), which prevents the central broadcast()
from notifying other subscribers; instead, create and return a
ServerEvent::ClientChanged in the tuple returned by the match (so the broadcast
machinery can handle delivery) rather than writing to client.sender directly.
Update the set_client_session success branches that construct
ServerResponse::Client (and the analogous paths at the other mentioned
locations) to remove direct client.sender pushes and instead populate the third
element of the returned tuple with a ServerEvent::ClientChanged (or appropriate
event wrapper) so broadcast() processes it for all subscribers.
---
Duplicate comments:
In `@crates/embers-server/src/server.rs`:
- Around line 1816-1840: The helper list_clients() currently returns None for a
live client that has no current_session_id and no subscriptions, causing freshly
accepted connections to vanish (breaking ClientRequest::Get with client_id: None
and preventing clear_client_session() from emitting ClientChanged); fix by not
filtering out connected clients—always construct a ClientRecord for every entry
in self.clients (i.e., call the client_record() logic for each client_id in
list_clients()), and only use the subscribed_all_sessions/subscribed_session_ids
checks to populate session lists rather than to exclude the client; apply the
same change to the duplicate block around the 1853–1858 logic so connected
clients with no sessions/subscriptions are still returned and notifications are
still emitted.
- Around line 104-120: The code unconditionally aborts write_handle after
awaiting read_handle which can cancel a self-initiated detach acknowledgement
that is delivered via a local oneshot/ack channel; change the teardown so the
task waits for the detach ack (or a signal from the read loop) before aborting
write_handle: have read_handle return a result/enum or a boolean (e.g.,
self_detached or detach_ack_received) instead of just Err, check that value
before calling write_handle.abort(), and if self-detached await the write_handle
(or the oneshot ack) to ensure the detach response is flushed, then call
write_runtime.cleanup_connection(connection_id). Ensure you update the code
paths that send the detach response to signal the ack receiver the same way the
read_handle result now communicates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51b684e9-cb05-4065-b523-074fe76e559f
📒 Files selected for processing (4)
crates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/types.rscrates/embers-server/src/server.rs
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/embers-cli/src/interactive.rs (1)
31-35:⚠️ Potential issue | 🟠 MajorIgnore
ClientChangedevents for other connections.This client subscribes to all sessions, so it receives every
ClientChangedbroadcast.switched_session_id()only checksevent.client.current_session_id, which means another client's switch can retarget this UI. Cache this connection's id withMuxClient::current_client()and gate the session update onevent.client.id == client_id.Suggested fix
pub async fn run( socket_path: PathBuf, target: Option<String>, config_path: Option<PathBuf>, ) -> Result<()> { let mut client = MuxClient::connect(&socket_path).await?; client.subscribe(None).await?; + let client_id = client.current_client().await?.id; let requested_target = target; let mut session_id = ensure_session_ready(&mut client, requested_target.as_deref()).await?; client.switch_current_session(session_id).await?; @@ - if let Some(next_session_id) = switched_session_id(&event) { + if let Some(next_session_id) = switched_session_id(&event, client_id) { ensure_root_window(configured.client_mut(), next_session_id).await?; configured .client_mut() .resync_session(next_session_id) .await?; session_id = next_session_id; } @@ -fn switched_session_id(event: &ServerEvent) -> Option<SessionId> { +fn switched_session_id(event: &ServerEvent, client_id: u64) -> Option<SessionId> { match event { - ServerEvent::ClientChanged(event) => event.client.current_session_id, + ServerEvent::ClientChanged(event) if event.client.id == client_id => { + event.client.current_session_id + } _ => None, } }Also applies to: 119-127, 138-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-cli/src/interactive.rs` around lines 31 - 35, The client currently subscribes to all sessions and reacts to ClientChanged events without verifying the source, so other clients' switches can retarget this UI; after connecting with MuxClient::connect and before using events (e.g., around where client.subscribe, ensure_session_ready, switched_session_id, and client.switch_current_session are used), call client.current_client() to cache this connection's client id and then guard any event-driven session updates by checking event.client.id == cached_client_id (update all places that handle ClientChanged events, including the blocks around switched_session_id and similar handlers at the other occurrences).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-cli/src/lib.rs`:
- Around line 116-123: The CLI variants DetachClient and SwitchClient currently
parse client_id as u64 but ClientRequest::Detach and ClientRequest::Switch
expect Option<NonZeroU64>; change the parsing/validation so client_id is
converted to NonZeroU64 before building the ClientRequest: validate and reject 0
with a clear error, then construct
Some(NonZeroU64::new_unchecked/NonZeroU64::new(...).unwrap()) (use
NonZeroU64::new and handle the Result/Option safely) and pass that into the
Detach/Switch request; update the code paths that build the ClientRequest for
DetachClient and SwitchClient to use the new NonZeroU64 value and ensure
SwitchClient still uses the existing target field.
In `@crates/embers-client/src/client.rs`:
- Around line 255-259: The match arm that returns Ok(()) for
ServerEvent::ClientChanged(_) needs to resync the newly-targeted session before
returning to ensure a snapshot is loaded for downstream hooks; in
ConfiguredClient::process_next_event() replace the early Ok(()) for
ServerEvent::ClientChanged(_) with a call to the session resync routine (e.g.
self.resync_target_session() or the existing method that loads the current
session snapshot) and propagate any error, then return Ok(()) only after the
resync completes; ensure you reference ServerEvent::ClientChanged,
ConfiguredClient::process_next_event, and current_session_id when locating and
updating the code.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 2360-2366: The ServerEvent::ClientChanged mapping currently
reduces the event to only session_id inside EventInfo, dropping event.client.id
and event.previous_session_id; update the client-changed hook metadata to
include the changed client's id and the previous_session_id (e.g., add fields
client_id and previous_session_id to the EventInfo payload produced for
ServerEvent::ClientChanged) so hook subscribers can identify which client
changed and which session was left; locate the match arm handling
ServerEvent::ClientChanged and populate the new metadata fields from
event.client.id and event.previous_session_id accordingly.
---
Outside diff comments:
In `@crates/embers-cli/src/interactive.rs`:
- Around line 31-35: The client currently subscribes to all sessions and reacts
to ClientChanged events without verifying the source, so other clients' switches
can retarget this UI; after connecting with MuxClient::connect and before using
events (e.g., around where client.subscribe, ensure_session_ready,
switched_session_id, and client.switch_current_session are used), call
client.current_client() to cache this connection's client id and then guard any
event-driven session updates by checking event.client.id == cached_client_id
(update all places that handle ClientChanged events, including the blocks around
switched_session_id and similar handlers at the other occurrences).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5d32b6f-376f-4a85-b65c-ed5bbd8522ec
📒 Files selected for processing (11)
crates/embers-cli/src/interactive.rscrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-client/src/client.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/state.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-server/src/server.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/embers-protocol/schema/embers.fbs (1)
454-492:⚠️ Potential issue | 🟠 MajorAppend the new
Envelopefields to the end of the table; don't insert them mid-table.Lines 465-492 insert
client_requestbefore the existing response and event fields. In FlatBuffers, without explicit field IDs (e.g.,(id: N)), field positions determine vtable offsets sequentially. Inserting in the middle renumbers every downstream offset, breaking wire compatibility with binaries built against the old schema—including pre-existing responses and events. Move the new fields to the end ofEnvelope, or pin explicit IDs across the entire table before regenerating bindings.🤖 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 454 - 492, The new field client_request was inserted mid-table in the Envelope FlatBuffer table which renumbers downstream vtable offsets and breaks wire compatibility; fix by moving client_request (and any other newly added request fields) to the end of the Envelope table after all existing response_* and *_event fields (or alternatively assign explicit field IDs for every field), ensuring you reference the Envelope table and keep existing response_* and event field ordering (e.g., ping_response, ok_response, ... client_response, session_created_event, ... client_changed_event) unchanged so bindings remain compatible.
♻️ Duplicate comments (2)
crates/embers-client/src/configured_client.rs (1)
1145-1145:⚠️ Potential issue | 🟠 MajorPreserve the full
ClientChangedpayload in hook metadata.This still collapses
ClientChangeddown tocurrent_session_id. Hook code has no stable way to tell which client changed or which session was left, so it cannot ignore another client’s switch/detach or distinguish a detach from a move. Extend the event payload here to carry at leastevent.client.idandevent.previous_session_id.Also applies to: 2360-2366
🤖 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` at line 1145, The mapping for ServerEvent::ClientChanged currently discards the event payload (returns None/current_session_id) so hook metadata lacks which client changed; update the match arm handling ServerEvent::ClientChanged in configured_client.rs (and the same block around the duplicate at the other location) to preserve and attach the full payload: include at minimum event.client.id and event.previous_session_id in the hook metadata instead of None/current_session_id so hooks can identify the client and whether it was a detach vs move.crates/embers-client/src/client.rs (1)
255-259:⚠️ Potential issue | 🟠 MajorResync the target session before returning
ClientChanged.
ConfiguredClient::process_next_event()dispatches hooks immediately after this method returns. With this branch as a no-op, a switch to an uncached session can reach hook/render code withcurrent_session_idset but no snapshot loaded yet.Suggested fix
- ServerEvent::BufferCreated(_) - | ServerEvent::BufferDetached(_) - | ServerEvent::FocusChanged(_) - | ServerEvent::RenderInvalidated(_) - | ServerEvent::ClientChanged(_) => Ok(()), + ServerEvent::BufferCreated(_) + | ServerEvent::BufferDetached(_) + | ServerEvent::FocusChanged(_) + | ServerEvent::RenderInvalidated(_) => Ok(()), + ServerEvent::ClientChanged(event) => { + if let Some(session_id) = event.client.current_session_id { + self.resync_session(session_id).await?; + } + Ok(()) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/client.rs` around lines 255 - 259, The branch that treats ServerEvent::ClientChanged(_) as a no-op allows hooks/render to run before the newly selected session has its snapshot loaded; update ConfiguredClient::process_next_event() so that when matching ServerEvent::ClientChanged(_) it triggers a resync of the target session (call the existing resync_target_session / resync_target_session_async helper or equivalent on self) and awaits its completion so current_session_id is accompanied by a loaded snapshot before returning Ok(()); this ensures hooks/render see a consistent, fully-resynced session state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-cli/src/interactive.rs`:
- Line 35: The code calls client.switch_current_session(session_id) and treats
every ClientChanged event as if it were for the attached client; fix by
filtering ClientChanged events so only events where event.client.id equals the
attached client's id are handled. Update places that use
switched_session_id()/ClientChanged (including the handlers that call
ensure_root_window() and resync_session()) to check the event.client.id or
compare against client.id before acting, so foreign client switches do not
retarget this UI; apply same filtering to the other occurrences around the
blocks at the referenced locations (the handlers that currently drop
event.client.id).
In `@crates/embers-server/src/server.rs`:
- Around line 608-621: The detach_client branch currently replies Ok as soon as
the shutdown signal is sent, which allows in-flight dispatch_request calls to
complete and continue mutating shared state; update the logic so detach_client
only acknowledges success after the target's handle_connection loop has actually
stopped — either by (a) adding a oneshot acknowledgement channel that
handle_connection sends when its loop exits and await that ack before returning
Ok in detach_client, or (b) mark the connection state as Detaching in your
connection registry (e.g., set a Detaching flag in the connection struct used by
dispatch_request) so dispatch_request will immediately reject new/ongoing
dispatches for that connection and then wait for the handle_connection shutdown
completion before sending the Ok response; adjust the code paths around
detach_client, handle_connection, and dispatch_request accordingly so the
response is sent only after confirmation.
---
Outside diff comments:
In `@crates/embers-protocol/schema/embers.fbs`:
- Around line 454-492: The new field client_request was inserted mid-table in
the Envelope FlatBuffer table which renumbers downstream vtable offsets and
breaks wire compatibility; fix by moving client_request (and any other newly
added request fields) to the end of the Envelope table after all existing
response_* and *_event fields (or alternatively assign explicit field IDs for
every field), ensuring you reference the Envelope table and keep existing
response_* and event field ordering (e.g., ping_response, ok_response, ...
client_response, session_created_event, ... client_changed_event) unchanged so
bindings remain compatible.
---
Duplicate comments:
In `@crates/embers-client/src/client.rs`:
- Around line 255-259: The branch that treats ServerEvent::ClientChanged(_) as a
no-op allows hooks/render to run before the newly selected session has its
snapshot loaded; update ConfiguredClient::process_next_event() so that when
matching ServerEvent::ClientChanged(_) it triggers a resync of the target
session (call the existing resync_target_session / resync_target_session_async
helper or equivalent on self) and awaits its completion so current_session_id is
accompanied by a loaded snapshot before returning Ok(()); this ensures
hooks/render see a consistent, fully-resynced session state.
In `@crates/embers-client/src/configured_client.rs`:
- Line 1145: The mapping for ServerEvent::ClientChanged currently discards the
event payload (returns None/current_session_id) so hook metadata lacks which
client changed; update the match arm handling ServerEvent::ClientChanged in
configured_client.rs (and the same block around the duplicate at the other
location) to preserve and attach the full payload: include at minimum
event.client.id and event.previous_session_id in the hook metadata instead of
None/current_session_id so hooks can identify the client and whether it was a
detach vs move.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 88ca01b3-72cd-41e3-a1ed-16c73a2878ad
📒 Files selected for processing (11)
crates/embers-cli/src/interactive.rscrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-client/src/client.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/state.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-server/src/server.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
crates/embers-cli/src/interactive.rs (1)
35-35:⚠️ Potential issue | 🟠 MajorHandle only this client's
ClientChangedevents.This path still treats every
ClientChangedas local. Once another live client switches sessions, this UI can also callensure_root_window()/resync_session()and retarget itself. Capture the attached client id from the initialswitch_current_session()response (orcurrent_client()) and gateswitched_session_id()onevent.client.id.🛠️ Suggested fix
- client.switch_current_session(session_id).await?; + let current_client_id = client.switch_current_session(session_id).await?.id; @@ - if let Some(next_session_id) = switched_session_id(&event) { + if let Some(next_session_id) = switched_session_id(&event, current_client_id) { ensure_root_window(configured.client_mut(), next_session_id).await?; configured .client_mut() .resync_session(next_session_id) .await?; session_id = next_session_id; } @@ -fn switched_session_id(event: &ServerEvent) -> Option<SessionId> { +fn switched_session_id(event: &ServerEvent, current_client_id: u64) -> Option<SessionId> { match event { - ServerEvent::ClientChanged(event) => event.client.current_session_id, + ServerEvent::ClientChanged(event) if event.client.id == current_client_id => { + event.client.current_session_id + } _ => None, } }Also applies to: 119-127, 138-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-cli/src/interactive.rs` at line 35, After calling client.switch_current_session(session_id).await? capture this client's id (from the call's response or by calling client.current_client()) and store it (e.g., local variable current_client_id); then update any handlers that react to ClientChanged events (places invoking ensure_root_window(), resync_session(), or calling switched_session_id()) to first check that event.client.id == current_client_id and only proceed when they match so you ignore ClientChanged events from other live clients. Also apply the same guard to the other spots noted (the blocks around lines where ensure_root_window/resync_session are called).crates/embers-client/src/client.rs (1)
255-259:⚠️ Potential issue | 🟠 MajorResync the destination session before returning
ClientChanged.
ConfiguredClient::process_next_event()dispatches hooks immediately after this call. WithServerEvent::ClientChanged(_)as a no-op here, a switch to an uncached session reaches hook/render code withcurrent_session_idset but no snapshot loaded yet.🛠️ Suggested fix
- ServerEvent::BufferCreated(_) - | ServerEvent::BufferDetached(_) - | ServerEvent::FocusChanged(_) - | ServerEvent::RenderInvalidated(_) - | ServerEvent::ClientChanged(_) => Ok(()), + ServerEvent::BufferCreated(_) + | ServerEvent::BufferDetached(_) + | ServerEvent::FocusChanged(_) + | ServerEvent::RenderInvalidated(_) => Ok(()), + ServerEvent::ClientChanged(event) => { + if let Some(session_id) = event.client.current_session_id { + self.resync_session(session_id).await?; + } + Ok(()) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/client.rs` around lines 255 - 259, The match arm treating ServerEvent::ClientChanged(_) as a no-op causes hooks to run before the destination session snapshot is loaded; update the ServerEvent::ClientChanged(_) branch in ConfiguredClient::process_next_event() to resync the destination session (e.g. call the existing resync_destination_session / ensure_session_snapshot_loaded method on self — using .await if the helper is async) before returning Ok(()), so current_session_id and its snapshot are consistent for subsequent hook/render code.crates/embers-client/src/configured_client.rs (1)
2360-2366:⚠️ Potential issue | 🟠 MajorPreserve the full
ClientChangedpayload in hook metadata.This collapses a richer event down to only
current_session_id. On detach,session_idbecomesNone, so hook code loses the session that was left; it also cannot identify which client changed. Carryevent.client.idandevent.previous_session_idthroughEventInfoinstead of reducing the event here.🤖 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 2360 - 2366, The match arm for ServerEvent::ClientChanged is collapsing the payload to only current_session_id; update EventInfo to preserve the full ClientChanged metadata by adding fields (e.g., client_id: Option<ClientId>, previous_session_id: Option<SessionId>) and populate them in the ServerEvent::ClientChanged arm: keep session_id = event.client.current_session_id, set client_id = Some(event.client.id) and previous_session_id = event.previous_session_id (or wrap in Option as appropriate). Also update any consumers of EventInfo to handle the new fields. Ensure you use the existing identifiers ServerEvent::ClientChanged, EventInfo, event.client.current_session_id, event.client.id, and event.previous_session_id so the payload is not lost on detach.
🤖 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-server/src/server.rs`:
- Around line 1877-1882: The current client_record method is inefficient because
it calls list_clients() to build all ClientRecord instances then searches for
one; change client_record to perform a direct lookup that only builds the
requested ClientRecord (e.g., implement a helper like get_client_record_by_id or
a variant of list_clients that accepts an id) so you query the underlying client
store/state for client_id, construct and return a single ClientRecord, and avoid
allocating the full Vec; update client_record to call that new direct-lookup
helper and keep the signature async fn client_record(&self, client_id: u64) ->
Option<ClientRecord>.
---
Duplicate comments:
In `@crates/embers-cli/src/interactive.rs`:
- Line 35: After calling client.switch_current_session(session_id).await?
capture this client's id (from the call's response or by calling
client.current_client()) and store it (e.g., local variable current_client_id);
then update any handlers that react to ClientChanged events (places invoking
ensure_root_window(), resync_session(), or calling switched_session_id()) to
first check that event.client.id == current_client_id and only proceed when they
match so you ignore ClientChanged events from other live clients. Also apply the
same guard to the other spots noted (the blocks around lines where
ensure_root_window/resync_session are called).
In `@crates/embers-client/src/client.rs`:
- Around line 255-259: The match arm treating ServerEvent::ClientChanged(_) as a
no-op causes hooks to run before the destination session snapshot is loaded;
update the ServerEvent::ClientChanged(_) branch in
ConfiguredClient::process_next_event() to resync the destination session (e.g.
call the existing resync_destination_session / ensure_session_snapshot_loaded
method on self — using .await if the helper is async) before returning Ok(()),
so current_session_id and its snapshot are consistent for subsequent hook/render
code.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 2360-2366: The match arm for ServerEvent::ClientChanged is
collapsing the payload to only current_session_id; update EventInfo to preserve
the full ClientChanged metadata by adding fields (e.g., client_id:
Option<ClientId>, previous_session_id: Option<SessionId>) and populate them in
the ServerEvent::ClientChanged arm: keep session_id =
event.client.current_session_id, set client_id = Some(event.client.id) and
previous_session_id = event.previous_session_id (or wrap in Option as
appropriate). Also update any consumers of EventInfo to handle the new fields.
Ensure you use the existing identifiers ServerEvent::ClientChanged, EventInfo,
event.client.current_session_id, event.client.id, and event.previous_session_id
so the payload is not lost on detach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: df14ff9e-1530-434d-8dfd-03fb6accbd76
📒 Files selected for processing (11)
crates/embers-cli/src/interactive.rscrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-client/src/client.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/state.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-server/src/server.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/embers-client/src/configured_client.rs (1)
2298-2389: 🧹 Nitpick | 🔵 TrivialFactor out the shared
EventInfodefaults.Adding the two optional fields already forced edits in every arm here. A small helper/base constructor would make the next metadata addition much harder to miss.
♻️ One way to tighten this up
+fn base_event_info(name: &str) -> EventInfo { + EventInfo { + name: name.to_owned(), + session_id: None, + previous_session_id: None, + client_id: None, + buffer_id: None, + node_id: None, + floating_id: None, + } +} + fn event_info(name: &str, event: &ServerEvent) -> EventInfo { match event { - ServerEvent::SessionCreated(event) => EventInfo { - name: name.to_owned(), - session_id: Some(event.session.id), - previous_session_id: None, - client_id: None, - buffer_id: None, - node_id: None, - floating_id: None, - }, + ServerEvent::SessionCreated(event) => EventInfo { + session_id: Some(event.session.id), + ..base_event_info(name) + }, // apply the same pattern to the remaining arms } }🤖 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 2298 - 2389, The event_info function repeats identical default fields for every ServerEvent arm—factor those defaults into a single base constructor (either implement Default for EventInfo or add a helper fn like base_event_info(name: &str) -> EventInfo) and then in event_info create the base (e.g., let mut info = base_event_info(name) or use struct update syntax ..base) and override only the differing fields for each ServerEvent variant (e.g., SessionCreated, BufferCreated, ClientChanged, FocusChanged, etc.) so you remove duplicate field listings and make future additions less error-prone.crates/embers-cli/src/interactive.rs (1)
121-145:⚠️ Potential issue | 🟠 MajorHandle attached-client detach explicitly instead of folding it into
None.For the attached client,
ClientChanged { current_session_id: None }means this UI was detached, not “ignore this event”. Because the server broadcastsClientChangedback to the originating connection too, this branch will see that case; today it leavessession_idunchanged, so the next render/input pass keeps operating on the old session. Return a distinct detached state here and handle it explicitly in the loop before continuing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-cli/src/interactive.rs` around lines 121 - 145, The code currently treats ClientChanged { current_session_id: None } the same as unrelated events; change switched_session_id to return a three-way result (e.g. an enum SwitchedSession { Ignore, Detached, Switched(SessionId) } or Option<Option<SessionId>>) so we can distinguish "detached" from "no-op" and "switched". In the main loop, match that new return: on Switched(id) keep the existing behavior (call ensure_root_window(...) and resync_session(id) and set session_id = id), on Detached explicitly clear the UI session state (make session_id an Option<SessionId> and set it to None or otherwise stop using the previous SessionId) and skip ensure_root_window/resync, and on Ignore continue as before; update any uses of session_id accordingly so we don't keep operating on the old session after a detach.crates/embers-protocol/schema/embers.fbs (1)
479-492:⚠️ Potential issue | 🟠 MajorAppend the new
Envelopefields instead of inserting them mid-table.
clients_responseandclient_responseare inserted before the existing event fields. Since Envelope has no explicitid:attributes on its fields, field order determines the vtable offsets on the wire. Inserting these new fields in the middle renumberssession_created_eventthroughsession_renamed_event, breaking backward compatibility in mixed-version deployments. Move the new fields to the end ofEnvelope, or add explicitid:attributes to all fields.🤖 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 479 - 492, The new fields clients_response and client_response were inserted before existing event fields in the Envelope table, which changes field ordering and breaks FlatBuffers vtable compatibility; fix by appending clients_response and client_response to the end of the Envelope field list (after client_request) OR alternatively add explicit id: N attributes to every field in the Envelope declaration so existing vtable offsets stay stable; update the Envelope declaration referencing the field names (clients_response, client_response, session_created_event, session_closed_event, ..., session_renamed_event, client_changed_event, client_request) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-cli/tests/interactive.rs`:
- Around line 143-152: The helper first_client_id currently takes the first
output line which is order-dependent; update first_client_id(output: &str) to
search the lines for the specific attached client row (e.g., find a line where
the status column contains "attached" or otherwise matches the test's client
identifier) instead of using lines().next(), then split that found line on '\t'
and parse the first column as u64; keep using the same function name so callers
continue to work.
In `@crates/embers-server/src/server.rs`:
- Around line 727-733: The session-close path drops the shared state lock before
calling clear_client_session(), allowing a race where a concurrent List/Get can
see a client with current_session_id referencing a removed session; also
subscriptions for that session are never pruned. Fix by performing the
client->session binding removal and any client metadata updates inside the same
critical section that holds state->clients (i.e., don't drop state before
calling the clearing logic), and extend clear_client_session() (and the
session-close code paths that call it, including the other session-termination
branches) to also remove/retire any entries in self.subscriptions for that
session_id; ensure you emit SessionClosed and ClientChanged notifications first
and then retire session-scoped subscriptions only after those notifications are
delivered so list_clients() and subscribers cannot observe stale session ids.
---
Outside diff comments:
In `@crates/embers-cli/src/interactive.rs`:
- Around line 121-145: The code currently treats ClientChanged {
current_session_id: None } the same as unrelated events; change
switched_session_id to return a three-way result (e.g. an enum SwitchedSession {
Ignore, Detached, Switched(SessionId) } or Option<Option<SessionId>>) so we can
distinguish "detached" from "no-op" and "switched". In the main loop, match that
new return: on Switched(id) keep the existing behavior (call
ensure_root_window(...) and resync_session(id) and set session_id = id), on
Detached explicitly clear the UI session state (make session_id an
Option<SessionId> and set it to None or otherwise stop using the previous
SessionId) and skip ensure_root_window/resync, and on Ignore continue as before;
update any uses of session_id accordingly so we don't keep operating on the old
session after a detach.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 2298-2389: The event_info function repeats identical default
fields for every ServerEvent arm—factor those defaults into a single base
constructor (either implement Default for EventInfo or add a helper fn like
base_event_info(name: &str) -> EventInfo) and then in event_info create the base
(e.g., let mut info = base_event_info(name) or use struct update syntax ..base)
and override only the differing fields for each ServerEvent variant (e.g.,
SessionCreated, BufferCreated, ClientChanged, FocusChanged, etc.) so you remove
duplicate field listings and make future additions less error-prone.
In `@crates/embers-protocol/schema/embers.fbs`:
- Around line 479-492: The new fields clients_response and client_response were
inserted before existing event fields in the Envelope table, which changes field
ordering and breaks FlatBuffers vtable compatibility; fix by appending
clients_response and client_response to the end of the Envelope field list
(after client_request) OR alternatively add explicit id: N attributes to every
field in the Envelope declaration so existing vtable offsets stay stable; update
the Envelope declaration referencing the field names (clients_response,
client_response, session_created_event, session_closed_event, ...,
session_renamed_event, client_changed_event, client_request) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8537d87a-b2fa-4e15-9915-dd577c2d4730
📒 Files selected for processing (17)
crates/embers-cli/src/interactive.rscrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-client/src/client.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/scripting/context.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/src/state.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/reducer.rscrates/embers-client/tests/repo_config.rscrates/embers-client/tests/script_actions.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-server/src/server.rs
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
crates/embers-cli/src/interactive.rs (1)
54-69:⚠️ Potential issue | 🔴 CriticalDon’t auto-reattach when the cached session disappears.
SessionRequest::CloseemitsSessionClosedbeforeClientChanged(crates/embers-server/src/server.rs:722-741). After the first event, this block sees the session missing from the cache and callsensure_session_ready()/switch_current_session()before the queued detach event is processed. That turns a detach-on-close into an implicit reattach, and when the closed session was the last one it can even create a new default session unexpectedly. Clear local state here and let the matchingClientChangedevent decide whether this client is detached or switched elsewhere.🛠️ Proposed fix
- if let Some(active_session_id) = session_id - && !configured - .client() - .state() - .sessions - .contains_key(&active_session_id) - { - let next_session_id = - ensure_session_ready(configured.client_mut(), requested_target.as_deref()) - .await?; - configured - .client() - .switch_current_session(next_session_id) - .await?; - session_id = Some(next_session_id); - } + if let Some(active_session_id) = session_id + && !configured + .client() + .state() + .sessions + .contains_key(&active_session_id) + { + // Wait for the corresponding ClientChanged event to tell us whether this client + // was detached or moved to a different session. + session_id = None; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-cli/src/interactive.rs` around lines 54 - 69, The code currently auto-reattaches when a cached session is missing by calling ensure_session_ready(...) and configured.client().switch_current_session(...); instead, when session_id is Some but not found in configured.client().state().sessions, clear the local session state (set session_id = None and avoid calling ensure_session_ready or switch_current_session) so we don’t implicitly reattach; let the upcoming ClientChanged event handle detach/reattach decisions. Update the block that checks session presence (the session_id variable, configured.client().state().sessions.contains_key(...), ensure_session_ready, and configured.client().switch_current_session) to simply clear local session_id and return/continue.crates/embers-server/src/server.rs (1)
1925-1930: 🧹 Nitpick | 🔵 TrivialMinor inefficiency:
client_recorditerates all clients to find one.This iterates
list_clients()and searches for a specific client, which is O(n) per lookup. For large client counts, consider a direct lookup.♻️ Optional optimization for direct client lookup
async fn client_record(&self, client_id: u64) -> Option<ClientRecord> { - self.list_clients() - .await - .into_iter() - .find(|client| client.id == client_id) + let clients = self.clients.lock().await; + let client = clients.get(&client_id)?; + let subscriptions = self.subscriptions.lock().await; + let mut subscribed_all_sessions = false; + let mut subscribed_session_ids = Vec::new(); + for subscription in subscriptions.values() { + if subscription.connection_id != client_id { + continue; + } + match subscription.session_id { + Some(session_id) => subscribed_session_ids.push(session_id), + None => subscribed_all_sessions = true, + } + } + subscribed_session_ids.sort_by_key(|session_id| session_id.0); + subscribed_session_ids.dedup(); + Some(ClientRecord { + id: client_id, + current_session_id: client.current_session_id, + subscribed_all_sessions, + subscribed_session_ids, + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 1925 - 1930, client_record currently calls list_clients().await and scans for the id (O(n)); change it to perform a direct lookup instead — e.g., call an existing direct getter (like get_client(client_id).await) or access the clients HashMap/index (e.g., self.clients.get(&client_id).cloned()) so the lookup is O(1); update the client_record implementation to use that direct lookup path (referencing the client_record function and the direct getter or clients collection).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-client/src/client.rs`:
- Around line 255-259: The code unconditionally calls
self.resync_session(event.client.current_session_id) for every
ServerEvent::ClientChanged; change it to only resync when the event is for this
client by comparing event.client.id with this connection's client id (e.g.
self.id or self.client_id) and only then, if let Some(session_id) =
event.client.current_session_id, call self.resync_session(session_id).await?;
keep existing error handling and return Ok(()) for other-client events.
In `@crates/embers-client/src/configured_client.rs`:
- Around line 1145-1148: The ServerEvent::ClientChanged handling currently falls
back to event.previous_session_id which causes detached client_changed hooks to
see a non-empty ctx.current_session(); change the mapping to use only
event.client.current_session_id (remove the .or(event.previous_session_id)
fallback) so that detached hooks get no current session; reference the
ServerEvent::ClientChanged branch and the event.client.current_session_id /
event.previous_session_id fields when making the edit.
In `@crates/embers-protocol/src/codec.rs`:
- Around line 2762-2774: decode_client_record currently accepts zero-valued ids
(record.id() == 0 and subscribed_session_ids entries of 0) which are invalid
wire sentinels; update decode_client_record to reject any zero id by returning a
ProtocolError when record.id() == 0 or when any value in
subscribed_session_ids_fb.iter() == 0, and ensure current_session_id keeps the
existing optional handling (only nonzero becomes Some(SessionId)). Specifically,
check record.id() before constructing ClientRecord and iterate
subscribed_session_ids_fb to validate nonzero values (using SessionId as the
constructor) and return an appropriate ProtocolError if any zero is found so
malformed frames cannot produce impossible identifiers.
In `@crates/embers-protocol/src/types.rs`:
- Around line 711-726: The server duplicates ClientChanged routing logic instead
of reusing the helper: update the server's broadcast() implementation to call
Event::all_session_ids() (and remove its special-case matching for
ClientChanged) so routing uses a single source of truth; specifically, replace
the direct match on ClientChanged inside broadcast() with a call to the
message's all_session_ids() method and use the returned Vec<SessionId> for
routing, or alternatively add a brief doc comment on Event::all_session_ids()
explaining that broadcast() intentionally bypasses it if you prefer to keep the
current behaviour.
In `@docs/config-api/event-info.md`:
- Line 38: Remove trailing whitespace characters in the markdown file around the
HTML button element to satisfy markdownlint MD009: edit the line containing the
button tag with attributes ("<button group="client_id"
id="link-client_id-Description" class="tablinks active"") and the other
affected line near the later occurrence (around line 132) to ensure there are no
trailing spaces at the end of the lines; save the file so both instances no
longer end with extra spaces.
---
Duplicate comments:
In `@crates/embers-cli/src/interactive.rs`:
- Around line 54-69: The code currently auto-reattaches when a cached session is
missing by calling ensure_session_ready(...) and
configured.client().switch_current_session(...); instead, when session_id is
Some but not found in configured.client().state().sessions, clear the local
session state (set session_id = None and avoid calling ensure_session_ready or
switch_current_session) so we don’t implicitly reattach; let the upcoming
ClientChanged event handle detach/reattach decisions. Update the block that
checks session presence (the session_id variable,
configured.client().state().sessions.contains_key(...), ensure_session_ready,
and configured.client().switch_current_session) to simply clear local session_id
and return/continue.
In `@crates/embers-server/src/server.rs`:
- Around line 1925-1930: client_record currently calls list_clients().await and
scans for the id (O(n)); change it to perform a direct lookup instead — e.g.,
call an existing direct getter (like get_client(client_id).await) or access the
clients HashMap/index (e.g., self.clients.get(&client_id).cloned()) so the
lookup is O(1); update the client_record implementation to use that direct
lookup path (referencing the client_record function and the direct getter or
clients collection).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: efe8c334-b397-4499-b041-aa0cc90ccab7
📒 Files selected for processing (51)
crates/embers-cli/src/interactive.rscrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-client/src/client.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/scripting/context.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/src/state.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/reducer.rscrates/embers-client/tests/repo_config.rscrates/embers-client/tests/script_actions.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-server/src/server.rscrates/embers-server/tests/client_sessions.rscrates/embers-server/tests/integration.rsdocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-256e957a.jsdocs/config-api-book/searchindex-72423136.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/defs/runtime.rhaidocs/config-api/event-info.md
Comment on lines
+711
to
726
| pub fn all_session_ids(&self) -> Vec<SessionId> { | ||
| match self { | ||
| Self::ClientChanged(event) => { | ||
| let mut ids = Vec::new(); | ||
| if let Some(prev) = event.previous_session_id { | ||
| ids.push(prev); | ||
| } | ||
| if let Some(curr) = event.client.current_session_id { | ||
| ids.push(curr); | ||
| } | ||
| ids | ||
| } | ||
| _ => self.session_id().into_iter().collect(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The all_session_ids() method is defined but not used by the server's broadcast logic.
The server's broadcast() method (in server.rs lines 1854-1868) special-cases ClientChanged events directly rather than using this method. While this works correctly, it creates a maintenance burden where the routing logic is duplicated.
Consider either:
- Using
all_session_ids()in the broadcast logic for consistency - Documenting that this method exists for external consumers but the server uses direct matching
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-protocol/src/types.rs` around lines 711 - 726, The server
duplicates ClientChanged routing logic instead of reusing the helper: update the
server's broadcast() implementation to call Event::all_session_ids() (and remove
its special-case matching for ClientChanged) so routing uses a single source of
truth; specifically, replace the direct match on ClientChanged inside
broadcast() with a call to the message's all_session_ids() method and use the
returned Vec<SessionId> for routing, or alternatively add a brief doc comment on
Event::all_session_ids() explaining that broadcast() intentionally bypasses it
if you prefer to keep the current behaviour.
|
|
||
| <div> | ||
| <div class="tab"> | ||
| <button group="client_id" id="link-client_id-Description" class="tablinks active" |
There was a problem hiding this comment.
Remove trailing spaces to satisfy markdownlint MD009.
Static analysis flags trailing whitespace at Line 38 and Line 132.
🧹 Proposed lint fix
-<button group="client_id" id="link-client_id-Description" class="tablinks active"
+<button group="client_id" id="link-client_id-Description" class="tablinks active"
@@
-<button group="previous_session_id" id="link-previous_session_id-Description" class="tablinks active"
+<button group="previous_session_id" id="link-previous_session_id-Description" class="tablinks active"Also applies to: 132-132
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 38-38: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/config-api/event-info.md` at line 38, Remove trailing whitespace
characters in the markdown file around the HTML button element to satisfy
markdownlint MD009: edit the line containing the button tag with attributes
("<button group="client_id" id="link-client_id-Description" class="tablinks
active"") and the other affected line near the later occurrence (around line
132) to ensure there are no trailing spaces at the end of the lines; save the
file so both instances no longer end with extra spaces.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/embers-cli/tests/interactive.rs (1)
143-158:⚠️ Potential issue | 🟡 MinorAssert uniqueness instead of picking the first attached client.
This helper is still order-dependent if another attached client appears, because it stops at the first non-
-row. That can still makeclient_commands_can_switch_and_detach_a_live_attached_clienttarget the wrong client id.🛠️ Suggested hardening
fn first_client_id(output: &str) -> u64 { - output + let ids = output .lines() - .find_map(|line| { + .filter_map(|line| { let mut columns = line.split('\t'); let client_id = columns.next()?; let current_session = columns.next()?; - if current_session == "-" { - return None; - } - Some(client_id) + (current_session != "-").then_some(client_id) }) - .expect("attached client row present") - .parse::<u64>() - .expect("client id parses") + .collect::<Vec<_>>(); + + assert_eq!(ids.len(), 1, "expected exactly one attached client:\n{output}"); + ids[0].parse::<u64>().expect("client id parses") }🤖 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 143 - 158, The helper first_client_id is order-dependent because it returns the first non-"-" row; modify first_client_id to collect all client_id candidates from lines where current_session != "-" (using the same parsing logic), assert that exactly one attached client exists (panic with a clear message otherwise), parse that single client_id to u64 and return it; reference first_client_id so tests like client_commands_can_switch_and_detach_a_live_attached_client reliably target the unique attached client.
🤖 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 138-145: The extra resync is redundant: remove the call to
configured.client_mut().resync_session(next_session_id).await? inside the
SwitchedSession::Switched branch (the block handling switched_session_id(&event,
attached_client_id)) because ConfiguredClient::process_next_event() already
resyncs the target session and ensure_root_window(...) also begins with a
resync; rely on those and keep ensure_root_window(configured.client_mut(),
next_session_id).await? only, eliminating the duplicate
resync_session(next_session_id) invocation.
---
Duplicate comments:
In `@crates/embers-cli/tests/interactive.rs`:
- Around line 143-158: The helper first_client_id is order-dependent because it
returns the first non-"-" row; modify first_client_id to collect all client_id
candidates from lines where current_session != "-" (using the same parsing
logic), assert that exactly one attached client exists (panic with a clear
message otherwise), parse that single client_id to u64 and return it; reference
first_client_id so tests like
client_commands_can_switch_and_detach_a_live_attached_client reliably target the
unique attached client.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2e1ba74c-c882-40f5-b85e-13b87f38026c
📒 Files selected for processing (73)
crates/embers-cli/src/interactive.rscrates/embers-cli/src/lib.rscrates/embers-cli/tests/interactive.rscrates/embers-client/src/client.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/scripting/context.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/src/state.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/reducer.rscrates/embers-client/tests/repo_config.rscrates/embers-client/tests/script_actions.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/lib.rscrates/embers-protocol/src/types.rscrates/embers-server/src/server.rscrates/embers-server/tests/client_sessions.rscrates/embers-server/tests/integration.rsdocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-256e957a.jsdocs/config-api-book/searchindex-72423136.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/action.mddocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/floating-ref.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/tree.mddocs/config-api/ui.md
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Line 1076: Run rustfmt on the repository to fix CI formatting: execute cargo
fmt --all (or rustfmt via your editor) and commit the resulting changes so the
formatting differences in crates/embers-cli/src/interactive.rs are resolved;
ensure you re-run cargo fmt before pushing so the diff around the interactive.rs
file (e.g., the function/block ending near the closing brace) no longer fails
CI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 579a55b7-54f3-48df-8c14-e484713d0c62
📒 Files selected for processing (1)
crates/embers-cli/src/interactive.rs
| assert!(!without_mouse_exit.contains(TERMINAL_DISABLE_MOUSE_SEQUENCE)); | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Fix formatting to pass CI.
The pipeline reports a formatting diff detected by cargo fmt. Run cargo fmt --all to apply the required formatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-cli/src/interactive.rs` at line 1076, Run rustfmt on the
repository to fix CI formatting: execute cargo fmt --all (or rustfmt via your
editor) and commit the resulting changes so the formatting differences in
crates/embers-cli/src/interactive.rs are resolved; ensure you re-run cargo fmt
before pushing so the diff around the interactive.rs file (e.g., the
function/block ending near the closing brace) no longer fails CI.
Co-authored-by: CodeRabbit <noreply@coderabbit.ai> fix: apply CodeRabbit auto-fixes Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
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
Behavior Improvements
Tests
Documentation
Stack
api-docs Add api docs for rhai config #2(merged)persistance Persist workspace state across clean restarts #3(merged)buffer-cli Add native detached buffer CLI workflows #9(merged)session-rename Add native session rename support #5(merged)