feat: per-client active workspace views (review fixes) - #7
Open
Castrozan wants to merge 2 commits into
Open
Conversation
Each attached app client now views its own workspace independently instead of mirroring one shared foreground view. A ClientView bundle holds the per-client, workspace-relative state: active and selected workspace by stable id, mode, view geometry, the navigator, the interaction singletons (copy mode, selection, context menu, drag, press tracking), the modal payloads (worktree create/open/remove, rename target, release notes, product announcement, keybind help), the name input, and the per-view scrolls and collapse set. The bundle is saved per ClientConnection and swapped into the single AppState around each client's render and input boundaries, including the resize path before a client is promoted to foreground. Saved views are reconciled against the live workspace set whenever a view is loaded, so workspace removal falls a client back to its selected or the first workspace, and clients attached before the first workspace existed adopt it once created. Server-wide truth (the workspace tree itself, global settings and palette, terminal appearance) stays in shared AppState and is not carried per client.
The view swap made the active workspace per-client, but the rest of the server still assumed one loaded state, so deferred commands, expiry timers and broadcast decisions landed on whichever view happened to be swapped in. Four paths are corrected. Deferred requests are harvested from the client that raised them while its view is still loaded, then replayed with that view focused. Without this, one client clicking "+ new workspace" force-switched a different client into the new workspace, and a worktree-create submit could drop silently when the handler ran against a view whose dialog payload was None. Index-bearing view state is canceled when the workspace tree changes. Saved views restore context menus, presses and drags verbatim, and those payloads carry raw indices, so a menu left open while another client closed a workspace could close the wrong one or index out of bounds. Workspace id lookups driven by such an index are now checked, so the worst case is a no-op instead of a panic that takes down every client. Transient expiry deadlines travel with the view that owns the transient. copy_feedback and the selection highlight are per-client, but their deadlines lived on App, so a deadline firing while another view was loaded cleared the wrong state and left the original stuck with nothing to expire it. The loop deadline now also accounts for deadlines parked in saved views. Host mouse capture is computed per client instead of broadcasting one value. With mouse_capture disabled, capture depends on each client's own mode and focused pane, so a client on another workspace was told the wrong mode and stopped receiving the mouse events it needed.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Preview of the upstream change against
ogulcancelik/herdr, rebuilt on the sameupstream-masterbase (4617456) as #5 so the diff is exactly the feature commit plus the fixes. Supersedes #5.Makes the active workspace per-client through a
ClientViewcontext swap, so each attached terminal holds its own independent workspace view (the tmux model, no shared mirror), and then closes the four bugs the review on discussion #651 found in that swap.What the review found and what changed
P1 — deferred
request_*actions ran against whichever view was loaded. The one-tick-deferred flags live on the sharedAppStateand are not part ofClientView, so they survived view swaps and executed against whoever was swapped in when the batch drained. Client A clicking "+ new workspace" force-switched client B into the new workspace; a worktree-create submit from A ran against B's view, foundworktree_create: None, and dropped silently.Fixed by harvesting the flags into a
DeferredClientRequestsbundle immediately after each client's input batch, while that client still owns the loaded view, then replaying each bundle with its own view focused (harvest_deferred_requests_from_client/handle_deferred_requests_headless). Requests raised outside client input (socket API, timers) still run against the loaded view, as before.P1 — reconciliation missed index-bearing state.
reconcile_client_views_with_workspacesre-pointed only the id-based fields. Saved views also carry raw indices restored verbatim:context_menu,workspace_press,tab_press,drag, and mode-carrying dialogs such asConfirmClose. A menu left open on workspace 2 while another client closed a workspace could close the wrong workspace, or panic the server through the unchecked indexing inpublic_workspace_id.Fixed both ways the review suggested: a workspace-tree change now cancels the transient index-bearing state in every saved view and in the loaded one (
cancel_workspace_index_state, gated onworkspace_tree_changed_since_last_reconcileso the cost is a comparison per swap, not a clear per swap), and the index-driven entry points (focus_workspace_idx_via_api,close_workspace_idx_via_api,move_workspace_via_api) go through a checkedworkspace_id_at, so a stale index degrades to a no-op instead of a crash.P2 — per-client transients had global expiry deadlines.
copy_feedbackand the selection highlight/autoscroll state were saved per client, but their deadlines lived onApp. A deadline firing while that client's view was swapped out cleared someone else's state, and the original transient came back on restore with no deadline and stuck.Fixed by moving the three deadlines into
ClientView, snapshotting and restoring them throughApp::snapshot_client_view/App::restore_client_view.expire_due_client_view_transientsfocuses each client whose saved deadline is due before expiring it, and the loop deadline now mins in deadlines parked in saved views so a non-loaded client still wakes on time (selection autoscroll ticks at 30ms, well under the 250ms accept poll).P2 — mouse capture was computed from one view and broadcast to all. With
mouse_capture = false, the desired capture state depends on each client's own mode and its focused terminal's mouse reporting.stream_host_mouse_capture_modenow computes the value per client from that client's view (should_capture_host_mouse_in_view) and sends each client its own.Tests
src/server/headless/tests/client_view.rsgets one test per finding, each verified to fail against the unfixed code:deferred_workspace_create_lands_on_the_requesting_client_onlyworkspace_tree_change_cancels_index_bearing_state_in_saved_viewscopy_feedback_expires_against_the_client_that_raised_ithost_mouse_capture_follows_each_clients_own_viewjust checkis green: 2591 tests pass, formatting clean.just windows-lintwas not run here because it shells out torustup, which is not in the flake dev shell.Known limitations, unchanged by this PR
Per the hands-on testing earlier in the discussion: all clients still share one render grid, so differently sized clients clip;
ClientViewis still ephemeral, so a reattaching client lands on the default workspace; the API exposes no per-client view state; and thedone -> idleseen transition is still consumed by whichever client looks first. Those are feature and API-shape decisions rather than defects in the swap, and they need a maintainer call on scope.