Conversation
📝 WalkthroughWalkthroughThis PR adds optional Embers multiplexer backend support to Wisp alongside the existing tmux integration. It introduces a new wisp-embers client library, a configurable backend system with auto-detection, and refactors the CLI to dispatch operations (session activation, preview, sidebar handling) per-backend while maintaining tmux-only features like statusline integration. ChangesEmbers Backend Support
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/wisp-app/src/lib.rs`:
- Around line 479-481: Rename the misleading SessionRecord::tmux_id field to a
generic name (e.g., native_id or backend_id) and update all places that
construct or access it: change the struct definition in wisp-core and update the
constructor call in crates/wisp-app/src/lib.rs (the SessionRecord literal where
tmux_id: Some(session.native_id.clone()) is set), adapters that populate/read
this field from backends (tmux and Embers), any serialization/deserialization or
DB mapping code, and tests/examples; ensure matching Option types (Some/None)
and update imports/usages to the new symbol name so compilation and runtime
behavior remain the same.
In `@crates/wisp-bin/src/main.rs`:
- Around line 1339-1405: The UI loop contains fallible backend calls
(sidebar_requires_handoff, persist_sidebar_ui_state,
reconcile_sidebar_for_current_context,
client.join_buffer_to_current_session_root, runtime.rename_session,
reload_sidebar_runtime_state) that can early-return and skip terminal teardown;
refactor so terminal raw-mode/alternate-screen cleanup is always executed on any
early exit by moving these backend operations into a fallible helper that
returns Result and ensuring the outer loop uses an RAII guard or explicit
finally-style cleanup (e.g., a TerminalTeardown guard constructed before
entering the loop or calling a restore function in a match on the helper's
Result) so that even when those functions return Err the terminal teardown runs.
Ensure references to sidebar_runtime, backend, and runtime/session mutation
remain valid across this change.
- Around line 1141-1154: The buffer is created with client.create_buffer and
then a follow-up call like client.create_floating_for_buffer_in_current_session
may fail and leave an orphaned buffer; wrap the follow-up call so that on error
you clean up the newly created buffer (use the client method that removes
buffers such as client.delete_buffer or the repo's equivalent) before returning
the error, and apply the same rollback pattern to the other two occurrences (the
sidebar-popup float path and the sidebar-pane join path referenced in the
comment); make sure to reference buffer_id from the create_buffer call and call
the remove/delete API with that id in the error arm.
- Line 2520: The worker loop currently uses queue.lock().ok() which swallows a
poisoned mutex and causes the worker to exit silently; change the lock handling
in the loop that reads from queue (the while let Some(work_item) =
queue.lock().ok().and_then(|mut queue| queue.pop_front()) { ... }) to explicitly
handle PoisonError: if lock() returns Ok(guard) use it; if Err(poison) recover
with poison.into_inner() (or otherwise obtain the inner guard), log the poison
error with context (e.g., which git worker and a descriptive message) and
continue processing so pending git status work is not dropped; ensure you still
call queue.pop_front() on the recovered guard and keep the rest of the loop
logic unchanged.
In `@rust-toolchain.toml`:
- Line 2: The pinned Rust toolchain in rust-toolchain.toml (channel = "1.95.0")
is inconsistent with the workspace MSRV declared as rust-version = "1.90" in the
root Cargo.toml; decide which MSRV to use and make them match: either update the
root Cargo.toml rust-version to "1.95" (or "1.95.0") if the code/deps require
newer Rust, or change rust-toolchain.toml channel to "1.90" (or "1.90.0") if
1.90 is intended; ensure both the rust-toolchain.toml channel and the root
Cargo.toml rust-version fields are identical after the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d46f70c-eb23-404a-9a83-f7431bc3c155
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlREADME.mdcrates/wisp-app/Cargo.tomlcrates/wisp-app/src/lib.rscrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/main.rscrates/wisp-bin/tests/smoke.rscrates/wisp-config/src/lib.rscrates/wisp-core/src/view.rscrates/wisp-embers/Cargo.tomlcrates/wisp-embers/src/lib.rscrates/wisp-embers/tests/integration.rscrates/wisp-preview/src/lib.rsdocs/config.schema.tomldocs/configuration.mdrust-toolchain.toml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/wisp-embers/src/lib.rs (1)
405-426:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t detach the buffer before the cross-session join is known to succeed.
If
JoinBufferAtNodefails afterdetach_buffer_record, the buffer is left unattached and disappears from the original session. This needs either an atomic server-side move or explicit rollback using the original location.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/wisp-embers/src/lib.rs` around lines 405 - 426, The code detaches the buffer via detach_buffer_record(runtime, state, buffer_id) before issuing the cross-session request JoinBufferAtNode, which can leave the buffer unattached if the request fails; change the flow in the join logic so you first record the original location (using buffer_location), then perform the remote request (runtime.block_on(state.client.request_message(...)) calling NodeRequest::JoinBufferAtNode) and only call detach_buffer_record when the request and apply_session_layout_response(...) succeed; alternatively, if you must detach first implement an explicit rollback that reattaches the original location on any failure by using the saved original location to restore the buffer record.crates/wisp-bin/src/main.rs (1)
1336-1337:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop hard-coding
defaultfor Embers projections.
build_embers_statestores client focus undersnapshot.context.client_id, but this path always rebuilds session items withSome(DEFAULT_CLIENT_ID). On a real Embers snapshot that meansstate.current_session_id(...)andstate.previous_session_id(...)never resolve, so current/previous markers break. The same hard-coded key is reused in the later reload paths too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/wisp-bin/src/main.rs` around lines 1336 - 1337, The call to rebuild_session_items_for_picker_mode is hard-coded with Some(DEFAULT_CLIENT_ID), which prevents session lookup from using the actual Embers snapshot client id (snapshot.context.client_id) and breaks state.current_session_id/previous_session_id resolution; change the call site(s) to pass the real client id from the state/snapshot (e.g., use the client id stored at snapshot.context.client_id via the current state object) instead of DEFAULT_CLIENT_ID, and update any other reload paths that reuse DEFAULT_CLIENT_ID to similarly forward the snapshot.context.client_id so current/previous markers resolve correctly..github/workflows/ci.yml (1)
47-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the Embers tests in CI.
This workflow only executes default-feature tests, so the new
#[cfg(feature = "embers")]coverage added in this PR never runs.cargo clippy --all-featuresonly proves the code compiles; it does not execute those tests.Suggested change
- - name: Run tests - run: cargo test --workspace --all-targets + - name: Run tests + run: cargo test --workspace --all-targets --all-features🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 47 - 48, The CI step named "Run tests" currently runs tests without feature flags; update its command so embers-enabled tests execute by running cargo test with all features and targets (e.g. replace `cargo test --workspace --all-targets` with `cargo test --workspace --all-targets --all-features`) so that `#[cfg(feature = "embers")]` tests are exercised in CI.
♻️ Duplicate comments (1)
crates/wisp-embers/src/lib.rs (1)
667-695:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClean up the temporary root buffer when
AddRootTabfails.This creates a buffer and then performs a second request. If the second step fails, the buffer is orphaned in the mux and in the local cache.
Suggested rollback pattern
let buffer_id = create_buffer_record( runtime, state, &command, &title, Some(directory), &BTreeMap::new(), )?; let response = runtime.block_on(state.client.request_message(ClientMessage::Session( SessionRequest::AddRootTab { request_id: state.client.next_request_id(), session_id, title, buffer_id: Some(buffer_id), child_node_id: None, }, )))?; match response { ServerResponse::SessionSnapshot(response) => { state .client .state_mut() .apply_session_snapshot(response.snapshot); Ok(()) } - _ => Err(EmbersError::UnexpectedResponse("add root tab")), + _ => { + let _ = runtime.block_on(state.client.request_message(ClientMessage::Buffer( + BufferRequest::Kill { + request_id: state.client.next_request_id(), + buffer_id, + force: true, + }, + ))); + Err(EmbersError::UnexpectedResponse("add root tab")) + } }
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 47-48: The CI step named "Run tests" currently runs tests without
feature flags; update its command so embers-enabled tests execute by running
cargo test with all features and targets (e.g. replace `cargo test --workspace
--all-targets` with `cargo test --workspace --all-targets --all-features`) so
that `#[cfg(feature = "embers")]` tests are exercised in CI.
In `@crates/wisp-bin/src/main.rs`:
- Around line 1336-1337: The call to rebuild_session_items_for_picker_mode is
hard-coded with Some(DEFAULT_CLIENT_ID), which prevents session lookup from
using the actual Embers snapshot client id (snapshot.context.client_id) and
breaks state.current_session_id/previous_session_id resolution; change the call
site(s) to pass the real client id from the state/snapshot (e.g., use the client
id stored at snapshot.context.client_id via the current state object) instead of
DEFAULT_CLIENT_ID, and update any other reload paths that reuse
DEFAULT_CLIENT_ID to similarly forward the snapshot.context.client_id so
current/previous markers resolve correctly.
In `@crates/wisp-embers/src/lib.rs`:
- Around line 405-426: The code detaches the buffer via
detach_buffer_record(runtime, state, buffer_id) before issuing the cross-session
request JoinBufferAtNode, which can leave the buffer unattached if the request
fails; change the flow in the join logic so you first record the original
location (using buffer_location), then perform the remote request
(runtime.block_on(state.client.request_message(...)) calling
NodeRequest::JoinBufferAtNode) and only call detach_buffer_record when the
request and apply_session_layout_response(...) succeed; alternatively, if you
must detach first implement an explicit rollback that reattaches the original
location on any failure by using the saved original location to restore the
buffer record.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7fb83e26-3343-47df-aa50-f3bc219750af
📒 Files selected for processing (14)
.github/workflows/ci.yml.github/workflows/prepare-release.yml.github/workflows/release.ymlCargo.tomlcrates/wisp-app/src/lib.rscrates/wisp-bin/src/main.rscrates/wisp-core/benches/projections.rscrates/wisp-core/src/domain.rscrates/wisp-core/src/reduce.rscrates/wisp-core/src/view.rscrates/wisp-embers/Cargo.tomlcrates/wisp-embers/src/lib.rscrates/wisp-preview/src/lib.rsrust-toolchain.toml
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
Documentation
Tests
Chores