refactor: split 8 oversize modules into focused submodules#41
Merged
Conversation
Move popup / notices / pane_runtime / focus / layout / tab / filter / activity methods out of src/state.rs into the matching state/*.rs submodules via `impl AppState` blocks. The top-level state.rs shrinks from 2855 to ~1584 lines (mostly the integration-style test module that still needs fixtures that cross multiple submodules). Adds per-submodule unit tests for the moved methods (popup open/close invariants, notices copy feedback, pane runtime accessors, focus selection bounds, status_counts / repo_names, flash banner, etc.). Public API is preserved — every `crate::state::*` path keeps the same visibility via `pub use` re-exports from state.rs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Break the 1817-line tmux.rs into focused submodules under src/tmux/
and keep tmux.rs as a 25-line barrel that re-exports the public API
unchanged:
- tmux/types.rs — PaneInfo / PaneStatus / PermissionMode /
AgentType / WindowInfo / SessionInfo /
WorktreeMetadata + constants
- tmux/commands.rs — run_tmux / display_message / new_window /
send_command / kill_window / select_pane
- tmux/options.rs — set/unset/get_pane_option + test_mock
- tmux/panes.rs — get_sidebar_pane_info / find_active_pane /
pick_active_pane / focused_pane_path
- tmux/query.rs — query_sessions + parse_pane_fields + all
helpers (ps / subagents / prompt sanitization)
Adds new tests for options (mock install round-trip, contains
semantics, guard drop) and panes (pick_active_pane edge cases),
keeping every existing test behaviour intact.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- ui/panes/row.rs (1465 → 1049 lines, orchestrator + tests only):
* row/ctx.rs — RowCtx + SELECTION_MARKER
* row/status.rs — status_row + running_icon_for
* row/branch.rs — branch_ports_row + sidebar_remove_marker_col
* row/body.rs — task_progress / subagent / wait_reason /
prompt / idle_hint
- ui/notices.rs (1116 → 7-line barrel):
* notices/prompts.rs — prompt_for_agent / build_claude_migration_prompt
/ tildify + tests
* notices/button.rs — has_info / button_span / BUTTON_WIDTH +
missing_hooks_has_copy_button + tests
* notices/popup.rs — render_notices_popup + PluginSubItem +
LABEL_MAX_WIDTH + snapshot tests
- ui/bottom/git.rs (797 → 506 lines, orchestrator + tests only):
* git/header.rs — render_git_header + PrLinkInfo
* git/files.rs — render_file_section / render_untracked_section
+ MAX_CHANGED_FILES
All module-crossing calls preserve the pre-existing pub(super) /
pub(in crate::ui) visibility; no downstream imports changed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- worktree.rs (916 → 20-line barrel):
* worktree/markers.rs — SPAWN_* options + SpawnMarkers parse +
read_spawn_markers + tests
* worktree/config.rs — AGENTS / *_MODES / DEFAULT_* /
modes_for / agent_command / RemoveMode
+ tests
* worktree/slug.rs — slugify / pick_unique_slug /
worktree_path_for + tests
* worktree/env.rs — SpawnEnv trait + RealEnv
* worktree/flow.rs — SpawnRequest + spawn/remove + rollback
+ fake-env integration tests
- cli/hook/context.rs (847 → 19-line barrel):
* context/location.rs — cwd/worktree/session_id sync + subagent
guard + tests
* context/meta.rs — AgentContext + set_agent_meta +
clear_* + time + mark_task_reset +
is_system_message + tests
* context/pending.rs — PENDING_* markers + teardown replay
* context/subagents.rs — append_subagent / remove_subagent +
id-based lifecycle tests
* context/labels.rs — repo/branch label resolution + tests
- cli/hook/handlers.rs (828 → 11-line barrel):
* handlers/session.rs — on_session_start / on_session_end +
deferred-teardown + notification-gate
tests
* handlers/run.rs — on_user_prompt_submit / on_stop /
on_stop_failure / on_task_completed +
tests
* handlers/subagent.rs — on_subagent_start / on_subagent_stop +
drain-on-empty regression tests
* handlers/attention.rs — on_notification / on_permission_denied /
on_teammate_idle + tests
* handlers/worktree.rs — on_worktree_remove + pending-marker
tests
Public / pub(super) API surfaces are unchanged; the barrel files
re-export every symbol the rest of the crate already imports.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR decomposes multiple large modules into focused submodules (cli hooks, tmux, state, UI, worktree), moves implementations and tests into those modules, preserves public APIs via re-exports, and introduces new helper logic and unit tests across the new modules. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Worktree as worktree::spawn
participant Env as SpawnEnv (RealEnv)
participant Git as Git
participant TMUX as tmux
participant FS as Filesystem
rect rgba(200,220,255,0.5)
Caller->>Worktree: spawn(req)
Worktree->>Env: branch_prefix(), slugify, pick_unique_slug
Env->>Git: check branch/worktree availability
end
rect rgba(200,255,200,0.5)
Worktree->>Env: worktree_add(repo, path, branch)
Env->>Git: git worktree add
Env->>FS: create worktree dir
end
rect rgba(255,220,200,0.5)
Worktree->>Env: new_window(session, cwd, name)
Env->>TMUX: create window -> (pane_id, window_id)
Worktree->>Env: set_window_option(window, spawn markers)
Worktree->>Env: send_command(pane_id, agent_cmd)
TMUX-->>Caller: pane_id / window_id
end
alt failure after worktree_add
Worktree->>Env: rollback -> kill window? remove worktree, delete branch
Env->>TMUX: kill_window (best-effort)
Env->>Git: worktree_remove, branch_delete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/state.rs (1)
1338-1350:⚠️ Potential issue | 🟡 MinorReset the debounce timer in this gap-hit test.
Right now this test passes through the debounce path, not because
x=4andx=5are truly non-clickable. Reset the timer before each click so the assertions fail if those gaps ever become selectable.Proposed fix
// x=0 is leading space, x=4 and x=5 are separator + reset_filter_debounce(&mut state); state.handle_filter_click(0); assert_eq!(state.global.status_filter, StatusFilter::All); + reset_filter_debounce(&mut state); state.handle_filter_click(4); assert_eq!(state.global.status_filter, StatusFilter::All); + reset_filter_debounce(&mut state); state.handle_filter_click(5); assert_eq!(state.global.status_filter, StatusFilter::All);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state.rs` around lines 1338 - 1350, The test filter_click_gap_does_nothing is passing via the debounce path; before each handle_filter_click call you must reset the filter debounce timer so clicks at x=4 and x=5 are truly tested as separate events. Update the test to call the debounce-reset API on the AppState before each click (e.g. invoke state.global.debounce.reset() or add and call a small helper like state.reset_filter_debounce() if no reset exists) between the handle_filter_click calls so the assertions will fail if those gaps become clickable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/hook/context/location.rs`:
- Around line 28-39: In sync_worktree_meta, when worktree is Some(wt) the
current logic leaves existing pane options intact if wt.name or wt.branch are
empty; change it so that for each field (WorktreeInfo.name and
WorktreeInfo.branch) you explicitly unset the corresponding tmux pane option
("@pane_worktree_name" and "@pane_worktree_branch") when the incoming value is
empty, and only call tmux::set_pane_option when the value is non-empty, using
the existing tmux::unset_pane_option and tmux::set_pane_option helpers to ensure
stale values are cleared.
In `@src/cli/hook/handlers/attention.rs`:
- Around line 24-28: Stale `@pane_wait_reason` can remain when wait_reason is
empty; update the block after set_attention(pane, "notification") to clear the
option when there's no reason by adding an else branch that removes the option
(e.g., call tmux::unset_pane_option(pane, "@pane_wait_reason") or equivalent) so
that when wait_reason.is_empty() the old value is cleared; modify the
conditional around tmux::set_pane_option to handle both non-empty (set) and
empty (unset) cases referencing set_status, set_attention, and
tmux::set_pane_option / tmux::unset_pane_option.
In `@src/cli/hook/handlers/session.rs`:
- Around line 25-31: The handler currently unconditionally calls
tmux::unset_pane_option(pane, "@pane_subagents") which clobbers a parent-set
marker when child SessionStart events arrive; remove that unconditional unset
(or make it conditional) so SessionStart handlers do not clear "@pane_subagents"
set by SubagentStart. Update the SessionStart logic (where
tmux::unset_pane_option is invoked) to either omit the call for
"@pane_subagents" or only unset it when the session actually owns the marker
(preserve it for child sessions), ensuring should_update_cwd and the
PENDING_SESSION_END / deferred-teardown logic continue to see the marker.
In `@src/state/layout.rs`:
- Around line 198-205: Clicks on the popup title/top border are being treated as
selecting item 0 because the current item_index calculation uses
saturating_sub(1) and allows zero when row == area.y; update the selection logic
in the block that uses repo_popup_area(), point_in_rect(...), repo_names(),
set_repo_popup_selected(...) and confirm_repo_popup() to ignore the top/title
row: first compute the relative row (row - area.y) and early-return/skip if
relative == 0 (or require row > area.y), then compute the item index as relative
- 1 (not saturating) and proceed only if that index < repo_names().len() before
calling set_repo_popup_selected(...) and confirm_repo_popup().
In `@src/ui/bottom/git/header.rs`:
- Around line 80-83: The calculation of pr_x_offset can underflow when inner_w <
pr_w; update the logic around the pr_text handling (where pr_x_offset is
computed) to use a saturating subtraction or checked subtraction/unwrap_or(0) so
the result never goes negative before casting to u16 (e.g., replace (inner_w -
pr_w) as u16 with inner_w.saturating_sub(pr_w) cast to u16 or use
checked_sub().unwrap_or(0) on inner_w/pr_w), ensuring pr_x_offset is clamped to
0 for very narrow panes and preserving existing behavior otherwise.
In `@src/ui/notices/popup.rs`:
- Around line 229-241: The click targets are being added for every logical row
even when the row is clipped by the popup height; update the logic around where
copy_targets.push(NoticesCopyTarget { ... }) is called (the blocks that
construct Rect::new using inner.x/inner.y and line_index, and the similar block
at the later occurrence) to skip creating/pushing a copy target when the
computed target Y would be outside the visible area (i.e., if inner.y +
line_index as u16 >= inner.y + inner.height or line_index >= inner.height). Do
this check before pushing into copy_targets (and before adding the corresponding
Line to lines if relevant) so hidden rows do not register click targets. Ensure
you apply the same guard in both the first block (around LABEL_MAX_WIDTH and
CLAUDE_AGENT) and the second block (the other occurrence at lines ~283-291).
- Around line 489-510: The two tests
rendering_populates_copy_target_for_plugin_install_recommended and
rendering_skips_copy_target_for_plugin_stale call render_notices_popup_text but
lack inline insta snapshots; add insta::assert_snapshot!(output, @"...")
assertions using the actual rendered frame output variable (capture the returned
or printed frame string from render_notices_popup_text) to record the full UI
and keep the existing assertions on state.notices.copy_targets; update both
tests (and similarly the tests at lines ~750-795) to produce and assert an
inline snapshot for the rendered output while preserving checks against
state_with_plugin_install_recommended, state_with_plugin_stale,
render_notices_popup_text, LABEL_MAX_WIDTH, and state.notices.copy_targets.
- Around line 145-154: The popup sizing currently applies .min(area.width)
before enforcing minimums, allowing max(12) and max(3) to enlarge the popup past
the available area; fix by swapping the order so minimum-size constraints are
applied first then clamp to the available area. Concretely, change popup_width
calculation to apply .max(12) before .min(area.width as usize) (e.g.,
(widest_line + 4).max(12).min(area.width as usize).as u16) and change
popup_height to ((lines_len as u16) + 2).max(3).min(height_budget) so the final
size is always clamped to area; keep popup_x/popup_y and ensure
state.popup/hit-test uses the resulting popup_rect.
In `@src/ui/notices/prompts.rs`:
- Around line 25-37: The prompt embeds an unquoted executable path (from
std::env::current_exe()) for the CODEX_AGENT branch which can break when the
path contains spaces; update the CODEX_AGENT arm to shell-quote the executable
before formatting the message by taking the existing exe_path (from
current_exe().ok()?.to_string_lossy().into_owned()) and passing it through the
established quoting helper (use the shell_quote function from
cli::setup::shell_quote / format_hook_command path mentioned in the review) and
then use that quoted string in the format! call so emitted commands are
shell-safe and consistent with the project's quoting rules.
In `@src/worktree/config.rs`:
- Around line 27-40: The agent_command function currently forwards any non-empty
Claude mode into --permission-mode, producing invalid commands for unknown
persisted values; update agent_command so that for the "claude" agent you only
append "--permission-mode {m}" when m is in an explicit whitelist of supported
Claude permission modes (validate against that list inside agent_command), and
for any other or unknown Claude mode fall back to returning plain "claude"
(leave the existing match arms for other agents unchanged); reference the
agent_command function and the ("claude", ...) match arms when making this
change.
In `@src/worktree/flow.rs`:
- Around line 154-178: The SpawnMarkers.from_repo field is never validated
before being used by env.worktree_remove and env.branch_delete; add a guard that
returns an Err like "spawned from_repo is unset" (or similar) when
markers.from_repo.is_empty() is true—check this on markers (SpawnMarkers) before
entering the RemoveMode::WindowAndWorktree block (or at the start of that
branch) so worktree_remove and branch_delete are only called when
markers.from_repo is present.
---
Outside diff comments:
In `@src/state.rs`:
- Around line 1338-1350: The test filter_click_gap_does_nothing is passing via
the debounce path; before each handle_filter_click call you must reset the
filter debounce timer so clicks at x=4 and x=5 are truly tested as separate
events. Update the test to call the debounce-reset API on the AppState before
each click (e.g. invoke state.global.debounce.reset() or add and call a small
helper like state.reset_filter_debounce() if no reset exists) between the
handle_filter_click calls so the assertions will fail if those gaps become
clickable.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fe1593f-9588-4613-8197-ad1569abf5cb
📒 Files selected for processing (45)
src/cli/hook/context.rssrc/cli/hook/context/labels.rssrc/cli/hook/context/location.rssrc/cli/hook/context/meta.rssrc/cli/hook/context/pending.rssrc/cli/hook/context/subagents.rssrc/cli/hook/handlers.rssrc/cli/hook/handlers/attention.rssrc/cli/hook/handlers/run.rssrc/cli/hook/handlers/session.rssrc/cli/hook/handlers/subagent.rssrc/cli/hook/handlers/worktree.rssrc/state.rssrc/state/activity.rssrc/state/filter.rssrc/state/focus.rssrc/state/layout.rssrc/state/notices.rssrc/state/pane_runtime.rssrc/state/popup.rssrc/state/tab.rssrc/tmux.rssrc/tmux/commands.rssrc/tmux/options.rssrc/tmux/panes.rssrc/tmux/query.rssrc/tmux/types.rssrc/ui/bottom/git.rssrc/ui/bottom/git/files.rssrc/ui/bottom/git/header.rssrc/ui/notices.rssrc/ui/notices/button.rssrc/ui/notices/popup.rssrc/ui/notices/prompts.rssrc/ui/panes/row.rssrc/ui/panes/row/body.rssrc/ui/panes/row/branch.rssrc/ui/panes/row/ctx.rssrc/ui/panes/row/status.rssrc/worktree.rssrc/worktree/config.rssrc/worktree/env.rssrc/worktree/flow.rssrc/worktree/markers.rssrc/worktree/slug.rs
Comment on lines
+489
to
+510
| #[test] | ||
| fn rendering_populates_copy_target_for_plugin_install_recommended() { | ||
| let mut state = state_with_plugin_install_recommended(); | ||
| let _ = render_notices_popup_text(&mut state, 40, 10); | ||
| // The Plugin section's [prompt] button must register a click | ||
| // target so `notices_copy_target_at` can route the click into | ||
| // `prompt_for_agent("claude")`. | ||
| assert_eq!(state.notices.copy_targets.len(), 1); | ||
| assert_eq!(state.notices.copy_targets[0].agent, "claude"); | ||
| assert_eq!( | ||
| state.notices.copy_targets[0].area.width, | ||
| LABEL_MAX_WIDTH as u16 | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rendering_skips_copy_target_for_plugin_stale() { | ||
| // Stale sub-item is informational only — no [prompt] | ||
| // button, so no copy target should be registered. | ||
| let mut state = state_with_plugin_stale(); | ||
| let _ = render_notices_popup_text(&mut state, 40, 10); | ||
| assert!(state.notices.copy_targets.is_empty()); |
There was a problem hiding this comment.
Add inline snapshots to these frame-rendering tests.
These cases exercise the widget by rendering a real frame, but they only assert side effects like copy_targets. Please snapshot the rendered output too so the same paths still get UI-regression coverage.
As per coding guidelines, “All tests that render a frame MUST use insta::assert_snapshot!(output, @"...") inline snapshots for UI regression prevention, never substring checks like assert!(output.contains(...))”.
Also applies to: 750-795
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/notices/popup.rs` around lines 489 - 510, The two tests
rendering_populates_copy_target_for_plugin_install_recommended and
rendering_skips_copy_target_for_plugin_stale call render_notices_popup_text but
lack inline insta snapshots; add insta::assert_snapshot!(output, @"...")
assertions using the actual rendered frame output variable (capture the returned
or printed frame string from render_notices_popup_text) to record the full UI
and keep the existing assertions on state.notices.copy_targets; update both
tests (and similarly the tests at lines ~750-795) to produce and assert an
inline snapshot for the rendered output while preserving checks against
state_with_plugin_install_recommended, state_with_plugin_stale,
render_notices_popup_text, LABEL_MAX_WIDTH, and state.notices.copy_targets.
Owner
Author
There was a problem hiding this comment.
These two tests are deliberately scoped to the click-target state side-effect (state.notices.copy_targets) rather than the rendered output. The visual snapshots for the same states live alongside them in this file (snapshot_notices_popup_plugin_install_recommended_only, snapshot_notices_popup_plugin_stale_only), so the UI regression coverage the rule is protecting is already in place. Splitting the assertion so each test focuses on one concern keeps the failure mode readable when only the click-target logic regresses.
There was a problem hiding this comment.
@hiroppy, that makes sense — the visual regression contract is already satisfied by snapshot_notices_popup_plugin_install_recommended_only and snapshot_notices_popup_plugin_stale_only, and keeping the click-target assertions in their own focused tests makes regressions in that logic immediately obvious. I'll stand down on this one.
( •ᴗ• )🐇 ✨
✏️ Learnings added
Learnt from: hiroppy
Repo: hiroppy/tmux-agent-sidebar PR: 41
File: src/ui/notices/popup.rs:521-542
Timestamp: 2026-04-22T00:40:26.178Z
Learning: In `src/ui/notices/popup.rs` (Rust, hiroppy/tmux-agent-sidebar), the tests `rendering_populates_copy_target_for_plugin_install_recommended` and `rendering_skips_copy_target_for_plugin_stale` are intentionally side-effect-only tests that assert on `state.notices.copy_targets`. The visual/UI regression coverage for the same states is provided by companion snapshot tests `snapshot_notices_popup_plugin_install_recommended_only` and `snapshot_notices_popup_plugin_stale_only` in the same file. Do not flag the side-effect-only tests for lacking `insta::assert_snapshot!` calls — the separation of concerns is deliberate.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: hiroppy
Repo: hiroppy/tmux-agent-sidebar PR: 29
File: tests/capture_tests.rs:243-286
Timestamp: 2026-04-19T10:04:03.192Z
Learning: In `tests/capture_tests.rs` (Rust, hiroppy/tmux-agent-sidebar), the `#[ignore]`-d integration test `capture_single_frame_integration` intentionally uses `assert!(html.contains("<pre"))` as a smoke check rather than an `insta::assert_snapshot!`, because it spawns a real tmux session and the resulting HTML contains non-deterministic content (live timestamps, session-specific output). The proper visual regression guard for the HTML renderer lives in the unit test `render_html_emits_pre_with_per_cell_spans` which uses `insta::assert_snapshot!` against a deterministic handcrafted cell grid. Do not flag the smoke check in the integration test.
Learnt from: CR
Repo: hiroppy/tmux-agent-sidebar PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-21T18:26:09.857Z
Learning: Applies to **/*.rs : All tests that render a frame MUST use `insta::assert_snapshot!(output, @"...")` inline snapshots for UI regression prevention, never substring checks like `assert!(output.contains(...))`
Learnt from: hiroppy
Repo: hiroppy/tmux-agent-sidebar PR: 29
File: src/cli/toggle.rs:241-241
Timestamp: 2026-04-19T10:04:05.788Z
Learning: Do not treat rustfmt’s wrapping of `tmux::run_tmux` (or other rustfmt-driven formatting) as a formatting bug. Rustfmt may keep calls on a single line when they fit within the configured column limit (100 cols) and wrap them across multiple lines when they exceed it. If you see inconsistent single-line vs multi-line `tmux::run_tmux` calls in the same Rust file, assume it’s expected rustfmt output rather than something to change. The authoritative check is `cargo fmt --check`; only flag formatting differences after verifying with that command.
Comment on lines
+154
to
+178
| let markers = SpawnMarkers::parse(&env.display_message(pane_id, SPAWN_MARKERS_TEMPLATE)); | ||
| if !markers.is_spawned() { | ||
| return Err("pane was not created by sidebar spawn".into()); | ||
| } | ||
| if markers.worktree_path.is_empty() { | ||
| return Err("spawned worktree path is unset".into()); | ||
| } | ||
| if markers.branch.is_empty() { | ||
| return Err("spawned branch is unset".into()); | ||
| } | ||
| if markers.window_id.is_empty() { | ||
| return Err("could not resolve window id".into()); | ||
| } | ||
|
|
||
| if mode == RemoveMode::WindowAndWorktree { | ||
| if env.worktree_path_exists(&markers.worktree_path) { | ||
| env.worktree_remove(&markers.from_repo, &markers.worktree_path) | ||
| .map_err(|e| format!("git: {e}"))?; | ||
| } | ||
| // `git worktree remove` leaves the branch ref behind; drop | ||
| // it here, before `kill_window`, so a failure still leaves | ||
| // the window as a retry handle. | ||
| if env.branch_exists(&markers.from_repo, &markers.branch) { | ||
| env.branch_delete(&markers.from_repo, &markers.branch) | ||
| .map_err(|e| format!("git: {e}"))?; |
There was a problem hiding this comment.
Validate from_repo before running git cleanup.
remove_with uses markers.from_repo for worktree_remove/branch_delete, but never checks that it was actually populated. If that marker is missing, the flow falls through to a confusing git error instead of failing fast with a clear spawn-marker error.
Proposed fix
if markers.window_id.is_empty() {
return Err("could not resolve window id".into());
}
+ if mode == RemoveMode::WindowAndWorktree && markers.from_repo.is_empty() {
+ return Err("spawned source repo is unset".into());
+ }
if mode == RemoveMode::WindowAndWorktree {
if env.worktree_path_exists(&markers.worktree_path) {
env.worktree_remove(&markers.from_repo, &markers.worktree_path)
.map_err(|e| format!("git: {e}"))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let markers = SpawnMarkers::parse(&env.display_message(pane_id, SPAWN_MARKERS_TEMPLATE)); | |
| if !markers.is_spawned() { | |
| return Err("pane was not created by sidebar spawn".into()); | |
| } | |
| if markers.worktree_path.is_empty() { | |
| return Err("spawned worktree path is unset".into()); | |
| } | |
| if markers.branch.is_empty() { | |
| return Err("spawned branch is unset".into()); | |
| } | |
| if markers.window_id.is_empty() { | |
| return Err("could not resolve window id".into()); | |
| } | |
| if mode == RemoveMode::WindowAndWorktree { | |
| if env.worktree_path_exists(&markers.worktree_path) { | |
| env.worktree_remove(&markers.from_repo, &markers.worktree_path) | |
| .map_err(|e| format!("git: {e}"))?; | |
| } | |
| // `git worktree remove` leaves the branch ref behind; drop | |
| // it here, before `kill_window`, so a failure still leaves | |
| // the window as a retry handle. | |
| if env.branch_exists(&markers.from_repo, &markers.branch) { | |
| env.branch_delete(&markers.from_repo, &markers.branch) | |
| .map_err(|e| format!("git: {e}"))?; | |
| let markers = SpawnMarkers::parse(&env.display_message(pane_id, SPAWN_MARKERS_TEMPLATE)); | |
| if !markers.is_spawned() { | |
| return Err("pane was not created by sidebar spawn".into()); | |
| } | |
| if markers.worktree_path.is_empty() { | |
| return Err("spawned worktree path is unset".into()); | |
| } | |
| if markers.branch.is_empty() { | |
| return Err("spawned branch is unset".into()); | |
| } | |
| if markers.window_id.is_empty() { | |
| return Err("could not resolve window id".into()); | |
| } | |
| if mode == RemoveMode::WindowAndWorktree && markers.from_repo.is_empty() { | |
| return Err("spawned source repo is unset".into()); | |
| } | |
| if mode == RemoveMode::WindowAndWorktree { | |
| if env.worktree_path_exists(&markers.worktree_path) { | |
| env.worktree_remove(&markers.from_repo, &markers.worktree_path) | |
| .map_err(|e| format!("git: {e}"))?; | |
| } | |
| // `git worktree remove` leaves the branch ref behind; drop | |
| // it here, before `kill_window`, so a failure still leaves | |
| // the window as a retry handle. | |
| if env.branch_exists(&markers.from_repo, &markers.branch) { | |
| env.branch_delete(&markers.from_repo, &markers.branch) | |
| .map_err(|e| format!("git: {e}"))?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/worktree/flow.rs` around lines 154 - 178, The SpawnMarkers.from_repo
field is never validated before being used by env.worktree_remove and
env.branch_delete; add a guard that returns an Err like "spawned from_repo is
unset" (or similar) when markers.from_repo.is_empty() is true—check this on
markers (SpawnMarkers) before entering the RemoveMode::WindowAndWorktree block
(or at the start of that branch) so worktree_remove and branch_delete are only
called when markers.from_repo is present.
Owner
Author
There was a problem hiding this comment.
SpawnMarkers::is_spawned() (markers.rs:34) already enforces !self.from_repo.is_empty(), and we bail at remove_with line 155 when that check fails with "pane was not created by sidebar spawn". Adding a second guard here would be dead code for the remove path. Happy to extend the message if the existing string is unclear, but the validation itself is already in place.
There was a problem hiding this comment.
@hiroppy, you're right — I missed that. The AI summary of markers.rs confirms that is_spawned() already enforces both conditions: the spawn flag being truthy and from_repo being non-empty. Since remove_with bails immediately on !markers.is_spawned(), from_repo is guaranteed to be populated by the time the WindowAndWorktree block is reached. My proposed guard would have been unreachable dead code. Thanks for the clear explanation — no change needed here.
🧠 Learnings used
Learnt from: CR
Repo: hiroppy/tmux-agent-sidebar PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-21T18:26:09.857Z
Learning: Applies to src/app/workers.rs : Background operations (git polling, session refresh, version checking) must run in separate worker threads coordinated by app/workers.rs
Learnt from: hiroppy
Repo: hiroppy/tmux-agent-sidebar PR: 29
File: src/cli/toggle.rs:241-241
Timestamp: 2026-04-19T10:04:05.788Z
Learning: Do not treat rustfmt’s wrapping of `tmux::run_tmux` (or other rustfmt-driven formatting) as a formatting bug. Rustfmt may keep calls on a single line when they fit within the configured column limit (100 cols) and wrap them across multiple lines when they exceed it. If you see inconsistent single-line vs multi-line `tmux::run_tmux` calls in the same Rust file, assume it’s expected rustfmt output rather than something to change. The authoritative check is `cargo fmt --check`; only flag formatting differences after verifying with that command.
All findings predate this branch, but the refactor highlighted them. Addressing the clear bugs now rather than punting to follow-up PRs so each of the moved submodules ships with correct behavior. - context/location.rs: clear stale @pane_worktree_name/_branch when an incoming Some(worktree) payload has empty fields (previously only non-empty values were written, leaving stale metadata behind). - handlers/attention.rs: on_notification now unsets @pane_wait_reason when the hook payload has no reason, instead of leaving the prior cause on screen. - handlers/session.rs: SessionStart no longer unconditionally clears @pane_subagents. A subagent's own SessionStart event would otherwise drop the list that SubagentStart just populated, breaking should_update_cwd and the deferred-teardown drain path. - state/layout.rs: clicks on the repo popup's title/border row are ignored instead of collapsing to item_index == 0 via saturating_sub. - ui/bottom/git/header.rs: pr_x_offset uses saturating_sub so a pane narrower than the PR label does not underflow (debug panic / release wrap) when building the hyperlink overlay. - ui/notices/popup.rs: apply 12-wide / 3-row minimums BEFORE clamping to the available area, and drop the hit-test region entirely on tiny sidebars. Also skip click targets for rows clipped by the paragraph so hidden [prompt]/[copy] labels can't intercept clicks below the visible popup. - ui/notices/prompts.rs: shell-quote current_exe() before embedding it in the Codex setup prompt. Share the existing cli::setup::shell_quote helper (now pub(crate)). - worktree/config.rs: agent_command whitelists Claude's supported permission modes instead of forwarding any non-empty value, matching the documented "fall back to bare claude" contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/cli/hook/handlers/session.rs (1)
301-349: Make these notification tests fail when the notification path regresses.Both tests only assert when
@pane_os_notify_task_completedexists, so they also pass whennotify_desktop()is never reached or the notifier fails early. That leaves the new coverage effectively vacuous in CI. Please stub the notification sink or extract a pure eligibility/fingerprint helper and assert that deterministically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/hook/handlers/session.rs` around lines 301 - 349, The tests on_session_end_logout_attempts_notification and on_session_end_bypass_disabled_attempts_notification only assert conditionally when the tmux marker `@pane_os_notify_task_completed` exists, so they silently pass if notify_desktop() is never reached; make them deterministic by either (A) injecting/stubbing the notification sink used by on_session_end (replace the real notifier with a test stub that records calls into tmux::test_mock or a test-only in-memory recorder) and assert the stub was invoked with the expected fingerprint string (e.g., "session-ended:logout" / "session-ended:bypass_permissions_disabled"), or (B) extract the pure eligibility/fingerprint logic from on_session_end into a helper function (e.g., compute_session_end_fingerprint or is_notification_eligible) and call that directly from the tests, asserting its returned fingerprint/eligibility matches expectations; update the tests to use that stub/helper instead of only checking `@pane_os_notify_task_completed` so failures surface reliably in CI.src/ui/notices/popup.rs (1)
709-764: Use a copy-enabled fixture for the[copied]snapshots.
snapshot_notices_popup_shows_copied_label_for_recently_copied_agentandsnapshot_notices_popup_copied_label_expires_after_feedback_windowboth build a Missing hooks popup forclaude, but this renderer never shows a copy/prompt label for Claude in that section. These snapshots therefore stay the same regardless ofcopied_at, so they don't actually cover the recent/expired label branches. Switching them to a copy-enabled row (for examplecodexMissing hooks, or ClaudeInstallRecommended) would make the regression real.Example adjustment
- fn snapshot_notices_popup_shows_copied_label_for_recently_copied_agent() { - let mut state = state_with(None, vec![("claude", vec!["Stop"])]); - state.notices.copied_at = Some(("claude".into(), std::time::Instant::now())); + fn snapshot_notices_popup_shows_copied_label_for_recently_copied_agent() { + let mut state = state_with(None, vec![("codex", vec!["Stop"])]); + state.notices.copied_at = Some(("codex".into(), std::time::Instant::now())); let text = render_notices_popup_text(&mut state, 40, 8); insta::assert_snapshot!(text, @" ┌──────────────────┐ │ Notices │ │ Missing hooks │ - │ claude │ + │ codex [copied]│ │ - Stop │ └──────────────────┘ "); } #[test] fn snapshot_notices_popup_copied_label_expires_after_feedback_window() { - let mut state = state_with(None, vec![("claude", vec!["Stop"])]); + let mut state = state_with(None, vec![("codex", vec!["Stop"])]); state.notices.copied_at = Some(( - "claude".into(), + "codex".into(), std::time::Instant::now() - COPIED_FEEDBACK_DURATION - std::time::Duration::from_millis(100), ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/notices/popup.rs` around lines 709 - 764, The two tests snapshot_notices_popup_shows_copied_label_for_recently_copied_agent and snapshot_notices_popup_copied_label_expires_after_feedback_window are using a Missing hooks row for "claude" which never renders a [copied] label, so their assertions don't exercise the copied-at branches; update each test to use a copy-enabled fixture (e.g., change the Missing hooks entry to a copy-capable agent like "codex" or switch the notice type to InstallRecommended for "claude") when setting state via state_with, keep setting state.notices.copied_at (using std::time::Instant::now() and COPIED_FEEDBACK_DURATION for the expiry test) and then call render_notices_popup_text as before so the snapshots actually cover the displayed [copied] and expired cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/hook/handlers/session.rs`:
- Around line 60-86: The current SessionEnd handler runs the end-notification
path and sets the PENDING_SESSION_END marker even for subagent (child) events;
change the flow to first detect whether the event is from a parent vs a child by
checking tmux::get_pane_option_value(pane, "@pane_subagents") and using
should_update_cwd(¤t_subagents) (or its negation) as the gate: only call
desktop_notification::run_scoped_fingerprint,
desktop_notification::format_title, notify_desktop and related
repo_label_from_pane/branch_label_from_pane logic when this is the actual parent
(i.e., should_update_cwd returns true), and only mark_pending(pane,
PENDING_SESSION_END) / return 0 when this is the parent teardown path; ensure
child SessionEnd exits early without consuming the run-scoped fingerprint so
drain_pending_teardowns()/on_subagent_stop can perform
run_session_end_teardown(pane) for the real parent.
---
Nitpick comments:
In `@src/cli/hook/handlers/session.rs`:
- Around line 301-349: The tests on_session_end_logout_attempts_notification and
on_session_end_bypass_disabled_attempts_notification only assert conditionally
when the tmux marker `@pane_os_notify_task_completed` exists, so they silently
pass if notify_desktop() is never reached; make them deterministic by either (A)
injecting/stubbing the notification sink used by on_session_end (replace the
real notifier with a test stub that records calls into tmux::test_mock or a
test-only in-memory recorder) and assert the stub was invoked with the expected
fingerprint string (e.g., "session-ended:logout" /
"session-ended:bypass_permissions_disabled"), or (B) extract the pure
eligibility/fingerprint logic from on_session_end into a helper function (e.g.,
compute_session_end_fingerprint or is_notification_eligible) and call that
directly from the tests, asserting its returned fingerprint/eligibility matches
expectations; update the tests to use that stub/helper instead of only checking
`@pane_os_notify_task_completed` so failures surface reliably in CI.
In `@src/ui/notices/popup.rs`:
- Around line 709-764: The two tests
snapshot_notices_popup_shows_copied_label_for_recently_copied_agent and
snapshot_notices_popup_copied_label_expires_after_feedback_window are using a
Missing hooks row for "claude" which never renders a [copied] label, so their
assertions don't exercise the copied-at branches; update each test to use a
copy-enabled fixture (e.g., change the Missing hooks entry to a copy-capable
agent like "codex" or switch the notice type to InstallRecommended for "claude")
when setting state via state_with, keep setting state.notices.copied_at (using
std::time::Instant::now() and COPIED_FEEDBACK_DURATION for the expiry test) and
then call render_notices_popup_text as before so the snapshots actually cover
the displayed [copied] and expired cases.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff9f763f-532a-4a2c-a7ea-4800c4386d1c
📒 Files selected for processing (10)
src/cli/hook/context/location.rssrc/cli/hook/handlers/attention.rssrc/cli/hook/handlers/session.rssrc/cli/setup/mod.rssrc/state.rssrc/state/layout.rssrc/ui/bottom/git/header.rssrc/ui/notices/popup.rssrc/ui/notices/prompts.rssrc/worktree/config.rs
✅ Files skipped from review due to trivial changes (1)
- src/cli/setup/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/ui/bottom/git/header.rs
- src/cli/hook/context/location.rs
- src/cli/hook/handlers/attention.rs
- src/worktree/config.rs
Previously `on_session_end` would set `PENDING_SESSION_END` whenever `@pane_subagents` was non-empty, and `drain_pending_teardowns` would later turn that marker into `run_session_end_teardown` — wiping the parent's metadata and activity log as soon as the last subagent stopped, even if the parent session was still running. The root cause is that subagents share the parent's `$TMUX_PANE`, so SessionEnd events from children are indistinguishable from the parent's. The defer-then-drain design assumed every SessionEnd came from the parent; in practice a child SessionEnd racing ahead of SubagentStop would park the marker and then clobber the live parent. The notification path has the same problem on a smaller scale: a child's SessionEnd with `logout` / `bypass_permissions_disabled` would consume the run-scoped fingerprint, silently deduplicating the parent's real notification when it eventually fired. Fix: bail out of `on_session_end` before either side effect runs when `@pane_subagents` is non-empty. The tradeoff is that a parent SessionEnd that genuinely arrives while children are still listed gets ignored too, leaving stale metadata until the next SessionStart sweeps it. That's far safer than clobbering a live parent, and the next SessionStart recovery path already exists. Update the subagent-lifecycle tests to pin the new contract (child SessionEnd is a no-op, even through the drain path) and move the "deferred teardown waits for the last subagent" regression test to anchor on WorktreeRemove, which still uses the pending-marker dance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli/hook/handlers/session.rs (1)
317-366: Make noteworthy-end notification tests deterministic.These tests only assert when the stamp exists, so they can still pass if the notification branch is accidentally skipped. Please consider a test hook/mock notifier so assertion of attempted notify is unconditional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/hook/handlers/session.rs` around lines 317 - 366, The tests on_session_end_logout_attempts_notification and on_session_end_bypass_disabled_attempts_notification are currently non-deterministic because they only assert when the tmux stamp exists; add a deterministic test hook or mock notifier so the notification path always records an observable call in tests. Modify on_session_end (and any helper that calls send_desktop_notification) to accept an injectable notifier trait or closure (e.g., a TestNotifier implementing send_notify) and update notifications_enabled_all test calls to pass a mock notifier that always records an attempted notify into tmux::test_mock (or another test-only store) so the tests can unconditionally assert that the notifier was invoked and that the recorded stamp contains the expected "session-ended:<fingerprint>" value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cli/hook/handlers/session.rs`:
- Around line 317-366: The tests on_session_end_logout_attempts_notification and
on_session_end_bypass_disabled_attempts_notification are currently
non-deterministic because they only assert when the tmux stamp exists; add a
deterministic test hook or mock notifier so the notification path always records
an observable call in tests. Modify on_session_end (and any helper that calls
send_desktop_notification) to accept an injectable notifier trait or closure
(e.g., a TestNotifier implementing send_notify) and update
notifications_enabled_all test calls to pass a mock notifier that always records
an attempted notify into tmux::test_mock (or another test-only store) so the
tests can unconditionally assert that the notifier was invoked and that the
recorded stamp contains the expected "session-ended:<fingerprint>" value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2168dff4-ac2a-4ec4-83bb-2431e38f090e
📒 Files selected for processing (2)
src/cli/hook/handlers/session.rssrc/cli/hook/handlers/subagent.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/hook/handlers/subagent.rs
Post-888ed9e cleanup surfaced by a review pass:
- cli/hook/context/pending.rs: drop the PENDING_SESSION_END branch from
drain_pending_teardowns — nothing sets that marker any more, so the
branch was unreachable and its comment ("SessionEnd already cleared
the pending marker") was actively misleading. Keep the const so the
defensive unsets in on_session_start and clear_all_meta can still
sweep a stale marker left behind by a pre-fix install.
- cli/hook/handlers/{session,worktree}.rs + context/meta.rs: replace
the inline \`get_pane_option_value(\"@pane_subagents\") +
should_update_cwd(...)\` pattern with the existing pane_writes_allowed
helper so the \"is this pane safe to write to?\" gate lives in one
place.
- cli/hook/context.rs: drop the now-unused should_update_cwd re-export.
- ui/notices/popup.rs: delete the \`leading_slot_pad\` binding that was
computed, discarded via \`let _ = ...\`, and described by a 4-line
comment explaining behavior the code no longer implements.
No behavior change. All tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cli/hook/handlers/session.rs (1)
317-364: Make the notification-path tests assert something unconditionally.Both tests only validate the fingerprint inside
if tmux::test_mock::contains(...), so they still go green when the notification path is skipped entirely. That makes these regressions easy to miss in the common headless-CI path. A small mockable seam aroundnotify_desktopor its dedup-stamp write would make this deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/hook/handlers/session.rs` around lines 317 - 364, The tests on_session_end_logout_attempts_notification and on_session_end_bypass_disabled_attempts_notification are non-deterministic because they only assert the fingerprint when tmux::test_mock::contains(pane, stamp_key) is true; make them assert unconditionally by adding a deterministic mock seam for the notification/dedup stamp write: either (preferred) add a test-only mock for notify_desktop (or the internal dedupe/write function) that always writes the stamp key ("@pane_os_notify_task_completed") to tmux::test_mock for the given pane before/when on_session_end is called, or (simpler) set the expected stamp in tmux::test_mock::set(pane, stamp_key, "session-ended:<fingerprint>") immediately after installing the tmux mock so the subsequent assert checks the stored value without an if. Update the tests (on_session_end_logout_attempts_notification and on_session_end_bypass_disabled_attempts_notification) to remove the conditional and assert the stamp contains the expected "session-ended:<fingerprint>" string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/hook/handlers/session.rs`:
- Around line 37-40: The unconditional tmux::unset_pane_option calls for
PENDING_SESSION_END and PENDING_WORKTREE_REMOVE are clearing deferred teardown
markers emitted by subagents; modify the SessionStart handler so it preserves
these markers when the start is from a child subagent: detect child SessionStart
(e.g., via the existing subagent indicator used elsewhere or by checking
pane/session metadata like a SUBAGENT_MARKER or owner id) and only call
tmux::unset_pane_option(pane, PENDING_SESSION_END) and
tmux::unset_pane_option(pane, PENDING_WORKTREE_REMOVE) when the SessionStart is
a true fresh session (not a child); ensure on_subagent_stop can still read and
clear PENDING_WORKTREE_REMOVE when it runs.
---
Nitpick comments:
In `@src/cli/hook/handlers/session.rs`:
- Around line 317-364: The tests on_session_end_logout_attempts_notification and
on_session_end_bypass_disabled_attempts_notification are non-deterministic
because they only assert the fingerprint when tmux::test_mock::contains(pane,
stamp_key) is true; make them assert unconditionally by adding a deterministic
mock seam for the notification/dedup stamp write: either (preferred) add a
test-only mock for notify_desktop (or the internal dedupe/write function) that
always writes the stamp key ("@pane_os_notify_task_completed") to
tmux::test_mock for the given pane before/when on_session_end is called, or
(simpler) set the expected stamp in tmux::test_mock::set(pane, stamp_key,
"session-ended:<fingerprint>") immediately after installing the tmux mock so the
subsequent assert checks the stored value without an if. Update the tests
(on_session_end_logout_attempts_notification and
on_session_end_bypass_disabled_attempts_notification) to remove the conditional
and assert the stamp contains the expected "session-ended:<fingerprint>" string.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c775989-b043-47c4-84fe-b6d1040f4907
📒 Files selected for processing (6)
src/cli/hook/context.rssrc/cli/hook/context/meta.rssrc/cli/hook/context/pending.rssrc/cli/hook/handlers/session.rssrc/cli/hook/handlers/worktree.rssrc/ui/notices/popup.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cli/hook/handlers/worktree.rs
- src/cli/hook/context.rs
Comment on lines
+37
to
+40
| // A fresh session overrides any deferred teardown that was waiting | ||
| // for the previous run's subagents to drain. | ||
| tmux::unset_pane_option(pane, PENDING_SESSION_END); | ||
| tmux::unset_pane_option(pane, PENDING_WORKTREE_REMOVE); |
There was a problem hiding this comment.
Preserve deferred teardown markers across child SessionStart events.
Lines 27-36 already acknowledge that subagents can emit their own SessionStart on the parent pane. With the current unconditional unset calls here, one of those child starts can clear PENDING_WORKTREE_REMOVE before on_subagent_stop drains it, so the deferred worktree cleanup is lost and stale pane cwd/worktree metadata survives indefinitely.
Proposed direction
- tmux::unset_pane_option(pane, PENDING_SESSION_END);
- tmux::unset_pane_option(pane, PENDING_WORKTREE_REMOVE);
+ if pane_writes_allowed(pane) {
+ tmux::unset_pane_option(pane, PENDING_SESSION_END);
+ tmux::unset_pane_option(pane, PENDING_WORKTREE_REMOVE);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/hook/handlers/session.rs` around lines 37 - 40, The unconditional
tmux::unset_pane_option calls for PENDING_SESSION_END and
PENDING_WORKTREE_REMOVE are clearing deferred teardown markers emitted by
subagents; modify the SessionStart handler so it preserves these markers when
the start is from a child subagent: detect child SessionStart (e.g., via the
existing subagent indicator used elsewhere or by checking pane/session metadata
like a SUBAGENT_MARKER or owner id) and only call tmux::unset_pane_option(pane,
PENDING_SESSION_END) and tmux::unset_pane_option(pane, PENDING_WORKTREE_REMOVE)
when the SessionStart is a true fresh session (not a child); ensure
on_subagent_stop can still read and clear PENDING_WORKTREE_REMOVE when it runs.
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
Break up the 8 largest production files into topical submodules with zero
public API changes. Barrel files preserve every existing
crate::*import path via
pub use.src/state.rssrc/tmux.rssrc/ui/panes/row.rssrc/ui/notices.rssrc/worktree.rssrc/cli/hook/context.rssrc/cli/hook/handlers.rssrc/ui/bottom/git.rslarger integration-style cases (popup open/close, copy feedback,
pane-runtime accessors, pick_active_pane edge cases, spawn/remove
helpers, flash banner expiry, subagent lifecycle, etc.)
state(1 commit)tmux(1 commit)ui— row / notices / bottom/git (1 commit)worktree+cli/hook— worktree / context / handlers (1 commit)Test plan
cargo build(debug + release)cargo fmt --checkcargo clippy --all-targets -- -D warningscargo test— 898 lib + all integration suites pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor