Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 (11)
📝 WalkthroughWalkthroughAdds session-rename support: new CLI Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI
participant Proto as Protocol
participant Runtime as Server Runtime
participant State as Server State
participant Client as Client State
CLI->>Proto: build SessionRequest::Rename{request_id, session_id, name}
Proto->>Runtime: send ClientMessage::Session(Rename)
Runtime->>State: dispatch_session(SessionRequest::Rename)
State->>State: rename_session(session_id, name)
State-->>Runtime: Ok(())
Runtime->>State: session_snapshot(session_id)
State-->>Runtime: SessionSnapshot{ session_id, name, ... }
Runtime->>Proto: emit ServerEvent::SessionRenamed + Response
Proto->>Client: deliver ServerEvent::SessionRenamed
Client->>Client: apply_event -> update session name / mark dirty
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Mar 22, 2026
Pajn
force-pushed
the
session-rename
branch
2 times, most recently
from
33a6b57 to
09b340f
Compare
Pajn
force-pushed
the
buffer-cli
branch
2 times, most recently
from
ddd4ca3 to
89b7b28
Compare
Pajn
force-pushed
the
session-rename
branch
2 times, most recently
from
0fc0acc to
f7ae9a2
Compare
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-protocol/schema/embers.fbs`:
- Around line 55-59: The SessionOp enum was renumbered causing wire
incompatibility; restore the original numeric discriminants for AddRootTab,
SelectRootTab, RenameRootTab, and CloseRootTab so their existing values remain
unchanged, and instead append the new Rename variant at the end with a new
unused discriminant; locate the enum definition (symbols: SessionOp, AddRootTab,
SelectRootTab, RenameRootTab, CloseRootTab, Rename) and change only the Rename
variant's numeric value to a new unique number, leaving all other discriminants
as they were.
In `@crates/embers-protocol/src/codec.rs`:
- Around line 195-206: The change introduced SessionOp::Rename shifts subsequent
enum discriminants and breaks wire compatibility; update the protocol to avoid
this by either (A) implementing explicit protocol version negotiation in the
encode/decode paths (e.g., add a version header field and handle different enum
layouts in the codec functions) or (B) revert the discriminant change by
appending Rename to the end of the fb::SessionOp enum and adjust all encode
(where SessionRequest::Rename is mapped) and decode logic (where fb::SessionOp
is matched) to use the new terminal discriminant value; reference the
SessionOp::Rename variant, the SessionRequest mapping in the encoder, and the
fb::SessionOp matching in the decoder to locate and update the affected code
paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a4eeabe-99cc-44e6-89ef-57b1046fad30
📒 Files selected for processing (7)
crates/embers-cli/src/lib.rscrates/embers-cli/tests/sessions.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/types.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rs
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-protocol/src/codec.rs`:
- Around line 195-206: Add a focused unit test (e.g., in
crates/embers-protocol/src/codec.rs tests) that verifies a
SessionRequest::Rename encodes to the fb::SessionOp::Rename wire form and
decodes back to the same SessionRequest (round-trip), and a separate test that
constructs the wire Rename payload with a missing name field and asserts decode
fails or returns the expected error; locate and exercise the module's
serialize/deserialize functions used for requests (the encode/decode helpers in
this codec module) and assert on the fb::SessionOp::Rename discriminant, the
session_id conversion, and proper error behavior for the missing-name case.
In `@crates/embers-server/src/server.rs`:
- Around line 539-555: Replace the generic NodeChanged event emitted by the
SessionRequest::Rename path with a dedicated metadata event: add a new
ServerEvent variant (e.g., SessionRenamed or SessionMetadataChanged) and
corresponding event struct, update the enum used by event dispatch, and emit
that new variant instead of ServerEvent::NodeChanged in the rename handling (the
block that calls state.rename_session and returns
ServerResponse::SessionSnapshot/SessionSnapshotResponse). Also update any
downstream match/handlers that consume ServerEvent::NodeChanged to handle the
new SessionRenamed/SessionMetadataChanged variant to avoid triggering full
layout/structure refreshes where only metadata changed.
In `@crates/embers-server/src/state.rs`:
- Around line 292-295: The rename_session method currently allows empty or
whitespace-only names; update rename_session to trim the provided name (the name
param) and reject it if the result is empty, returning an Err from the same
Result<(), _> used by this module with a clear "invalid/empty session name"
error; only assign the trimmed non-empty string to
self.session_mut(session_id)?.name when valid. Use the existing SessionId,
rename_session and session_mut symbols to locate the change and reuse the
crate's standard error construction (or map an appropriate error type) so
callers get a consistent failure value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f152838e-f294-45b8-be5b-89dc92e0f4a8
📒 Files selected for processing (7)
crates/embers-cli/src/lib.rscrates/embers-cli/tests/sessions.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/types.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rs
|
@coderabbitai review |
1 similar comment
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
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)
445-451:⚠️ Potential issue | 🔴 CriticalMove
session_renamed_eventto the end ofEnvelopeto preserve wire compatibility.Inserting a new field in the middle of the
Envelopetable shifts implicit field IDs for all subsequent fields (buffer_created_eventonward), breaking backward/forward compatibility. Older clients/servers will read wrong fields, causing data corruption.💡 Protocol-safe fix
session_created_event:SessionCreatedEvent; session_closed_event:SessionClosedEvent; - session_renamed_event:SessionRenamedEvent; buffer_created_event:BufferCreatedEvent; buffer_detached_event:BufferDetachedEvent; node_changed_event:NodeChangedEvent; floating_changed_event:FloatingChangedEvent; focus_changed_event:FocusChangedEvent; render_invalidated_event:RenderInvalidatedEvent; + session_renamed_event:SessionRenamedEvent; }🤖 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 445 - 451, The new field session_renamed_event was inserted before buffer_created_event in the Envelope table which shifts implicit FlatBuffers field IDs and breaks wire compatibility; fix it by moving the session_renamed_event field declaration to the very end of the Envelope table (after render_invalidated_event) so existing fields (buffer_created_event, buffer_detached_event, node_changed_event, floating_changed_event, focus_changed_event, render_invalidated_event) keep their original order and IDs; update the Envelope definition in crates/embers-protocol/schema/embers.fbs accordingly and keep the field name session_renamed_event unchanged.
🤖 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/state.rs`:
- Around line 193-198: The ServerEvent::SessionRenamed handler currently updates
session.name for known sessions but always calls
self.dirty_sessions.insert(event.session_id), causing unnecessary resyncs;
change the logic so that self.dirty_sessions.insert(event.session_id) is only
executed when the session is not found in self.sessions (i.e., move the insert
into the else path of the if let Some(session) =
self.sessions.get_mut(&event.session_id) check) so known sessions are not marked
dirty unnecessarily.
In `@crates/embers-server/src/server.rs`:
- Around line 543-553: The code emits the raw request `name` in the
SessionRenamed event even though `rename_session(session_id, name.clone())`
normalizes the stored value; after a successful rename and obtaining `snapshot`
via `session_snapshot(&state, session_id)`, use the canonical name from
`snapshot` (or fetch from state) when constructing ServerEvent::SessionRenamed
(and any response fields) instead of the original `name` so subscribers receive
the normalized value.
---
Outside diff comments:
In `@crates/embers-protocol/schema/embers.fbs`:
- Around line 445-451: The new field session_renamed_event was inserted before
buffer_created_event in the Envelope table which shifts implicit FlatBuffers
field IDs and breaks wire compatibility; fix it by moving the
session_renamed_event field declaration to the very end of the Envelope table
(after render_invalidated_event) so existing fields (buffer_created_event,
buffer_detached_event, node_changed_event, floating_changed_event,
focus_changed_event, render_invalidated_event) keep their original order and
IDs; update the Envelope definition in crates/embers-protocol/schema/embers.fbs
accordingly and keep the field name session_renamed_event unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 124c6dbe-a142-47aa-8ddd-50365ae0d523
📒 Files selected for processing (11)
crates/embers-cli/src/lib.rscrates/embers-cli/tests/sessions.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.rscrates/embers-server/src/state.rs
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/sessions.rs`:
- Around line 81-90: Add a negative assertion that verifies the old session name
"alpha" no longer exists after the rename: invoke
cli_command(&server).arg("has-session").arg("-t").arg("alpha"), assert it exits
successfully but returns non-empty failure output (or specifically check that
stdout is empty using the same predicate as the positive check depending on
command semantics), ensuring you use the existing helpers (cli_command, stdout,
predicate::str::is_empty()) and the "has-session" command to confirm the stale
name lookup path was removed.
In `@crates/embers-client/src/client.rs`:
- Line 219: The match arm for ServerEvent::SessionRenamed currently returns
Ok(()) and never triggers hydration, leaving renamed sessions unresolved; update
that arm to call the existing apply_event(...) for the rename and then invoke
the client-side resync/hydration routine (e.g., call the method that hydrates
dirty sessions — names like hydrate_dirty_sessions(), hydrate_session_records(),
or resync_sessions() — so that any sessions marked dirty by apply_event are
fetched/rehydrated), preserving error handling and awaiting the call if it's
async; ensure you reference ServerEvent::SessionRenamed and apply_event when
making the change.
In `@crates/embers-protocol/src/codec.rs`:
- Around line 2713-2796: Add a focused roundtrip test that encodes and decodes a
ServerEvent::SessionRenamed to ensure the server-event wire path is covered:
create a ServerMessage::Event(ServerEvent::SessionRenamed { request_id:
RequestId(42), session_id: SessionId(123), name: "my-session".to_string() }),
call encode_server_message(...) to serialize and decode_server_message(...) to
deserialize, then pattern-match the decoded message for
ServerMessage::Event(ServerEvent::SessionRenamed { .. }) and assert the decoded
request_id, session_id and name equal the originals; use FlatBufferBuilder if
needed to construct raw envelopes similarly to the existing tests and reference
the ServerEvent::SessionRenamed, ServerMessage, encode_server_message and
decode_server_message symbols to locate where to add the test.
In `@crates/embers-server/src/server.rs`:
- Line 543: The call to state.rename_session(session_id, name.clone())
unnecessarily clones name; change the call to pass name by value
(state.rename_session(session_id, name)) so the clone is removed—ensure the
variable name is not used afterward (or adjust ownership) and keep the
rename_session invocation and parameter order as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0f19a7f-ea31-47e8-8c95-98b9cd0f6c8d
📒 Files selected for processing (11)
crates/embers-cli/src/lib.rscrates/embers-cli/tests/sessions.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.rscrates/embers-server/src/state.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/embers-client/src/client.rs (1)
219-222:⚠️ Potential issue | 🟡 MinorRemove duplicate event application in rename resync path.
process_next_eventalready applies the event before callingresync_for_event, so this branch appliesSessionRenamedtwice.♻️ Proposed fix
ServerEvent::SessionRenamed(event) => { - self.state - .apply_event(&ServerEvent::SessionRenamed(event.clone())); self.resync_session(event.session_id).await }🤖 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 219 - 222, The code is applying SessionRenamed twice: process_next_event already calls self.state.apply_event(...) before invoking the resync path, but this ServerEvent::SessionRenamed branch calls self.state.apply_event(...) again; remove the duplicate apply by deleting the self.state.apply_event(&ServerEvent::SessionRenamed(event.clone())) call in the ServerEvent::SessionRenamed arm and only call self.resync_session(event.session_id).await (or delegate to resync_for_event) so the event is applied exactly once.
🤖 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 545-553: The code currently clones the entire snapshot when
building the SessionSnapshotResponse and separately clones the session name for
the SessionRenamedEvent; instead, avoid the expensive full clone by cloning only
snapshot.session.name and move the original snapshot into
ServerResponse::SessionSnapshot (i.e., construct SessionRenamedEvent with name:
snapshot.session.name.clone() and use snapshot without cloning when creating
SessionSnapshotResponse), keeping the same symbols
ServerResponse::SessionSnapshot, SessionSnapshotResponse,
ServerEvent::SessionRenamed, SessionRenamedEvent and the snapshot variable.
---
Duplicate comments:
In `@crates/embers-client/src/client.rs`:
- Around line 219-222: The code is applying SessionRenamed twice:
process_next_event already calls self.state.apply_event(...) before invoking the
resync path, but this ServerEvent::SessionRenamed branch calls
self.state.apply_event(...) again; remove the duplicate apply by deleting the
self.state.apply_event(&ServerEvent::SessionRenamed(event.clone())) call in the
ServerEvent::SessionRenamed arm and only call
self.resync_session(event.session_id).await (or delegate to resync_for_event) so
the event is applied exactly once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6e36637-57d0-4840-a296-2dbd1692777e
📒 Files selected for processing (11)
crates/embers-cli/src/lib.rscrates/embers-cli/tests/sessions.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.rscrates/embers-server/src/state.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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack
persistance Persist workspace state across clean restarts #3(merged)Summary by CodeRabbit
New Features
Bug Fixes
Tests