refactor: decompose complex modules (state / tmux / panes / hook)#26
Merged
Conversation
Decomposes AppState (state.rs), draw_agents (ui/panes.rs), handle_event (cli/hook.rs), and PaneInfo (tmux.rs) in 13 reviewable commits within a single PR. Preserves byte-identical UI snapshots and unchanged external CLI/hook/tmux-option surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes field count (31 not 32), clarifies CollectedRows ownership (no borrow from state), specifies coordinator helper signatures incl. side-effect writes on render_secondary_header_into and compute_scroll_offset, and preserves existing is_*_open() popup guard helpers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds visibility and test co-location rules for the hook and ui/panes splits: AgentContext and moved helpers become pub(super), and tests migrate with the functions they exercise. Also expands commit 13 to cover docs/state-management.md, which already lacks repo_spawn_targets and spawn_remove_targets on FrameLayout. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Breaks the refactor design into 13 reviewable tasks across 5 chunks: state decomposition, tmux cleanup, draw_agents split, handle_event split, and tests+docs. Every task specifies exact files, substitution tables, grep commands for call-site discovery, and verification steps. Address reviewer blockers: adds src/ui/bottom.rs to Task 1.2 file list, adds get_mut delegator and retain substitution rule to PaneRuntimeMap, and provides stable grep anchors for row_collector/click_targets/popups extraction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-existing clippy warnings at baseline (6d0bafc) are not caused by this refactor. CI runs plain 'cargo clippy' without -D warnings; update plan to match so per-task verification does not false-alarm on unrelated warnings.
Move focus-related fields (sidebar_focused, focus, focused_pane_id, prev_focused_pane_id) into a new FocusState struct in `src/state/focus.rs`, and bundle the non-activity scroll offsets (panes, git) into ScrollStates in `src/state/scroll.rs`. ActivityState continues to own its own scroll field. Replaces six top-level AppState fields with two grouped sub-states, shrinking the struct and making the "focus tracking vs. scroll positions" boundaries explicit. No runtime behavior changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Desktop notifications for subagent / Task tool completion are now off by default. Claude Code emits TaskCompleted frequently during long runs, and pairing it with Stop produced noisy duplicate alerts. Users can still enable it via `@sidebar_notifications_events` (explicit list or `all`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…loning Per frame draw path reclaims move semantics the original draw_agents relied on before the refactor. compute_scroll_offset now receives total_lines directly and the caller installs line_to_row into state.layout before it is read, and click_targets::materialize + render_pane_rows consume their inputs by value so the Vec<Line>, pending_spawn, and pending_remove vectors never get cloned on the hot render path.
The refactor is landed. The design spec and implementation plan served their purpose during the rollout and are no longer needed as long-lived documentation.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughRefactors AppState into many Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent (external)
participant Hook as hook.rs
participant Handlers as handlers
participant Context as context
participant Activity as activity
participant Notifs as notifications
participant Tmux as tmux/filesystem
Note over Agent,Hook: external event arrives
Agent->>Hook: deliver event JSON
Hook->>Handlers: dispatch to handlers::<event>(...,&make_ctx(...))
Handlers->>Context: resolve ctx / sync pane meta
Handlers->>Activity: handle_activity_log(...) or write_activity_entry
Handlers->>Notifs: prepare fingerprint/body -> notify_desktop(...)
Activity->>Tmux: update pane status options / set `@pane_started_at`
Notifs->>Tmux: read/write pane options (run id) and call desktop_notification::notify_if_allowed
Handlers->>Hook: return numeric exit code
Note: colored rectangles not required for this simple sequence. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Moves AgentEventKind + its external_name/from_external_name impl into src/event/kind.rs, and EventAdapter trait + resolve_adapter (plus all adapter integration tests) into src/event/adapter.rs. event.rs keeps WorktreeInfo, AgentEvent, and AgentEvent::kind(), with pub use re-exports so external imports continue to resolve via crate::event:: unchanged.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
src/state/scroll.rs (1)
2-13: Consider encapsulating viewport updates to preserve scroll invariants.
offset,total_lines, andvisible_heightare all public, so callers can update dimensions without re-clampingoffset. A small setter (for dimensions + clamp) would prevent stale offsets and keep behavior consistent across render paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/scroll.rs` around lines 2 - 13, Public fields offset, total_lines, and visible_height allow callers to change dimensions without re-clamping offset, breaking ScrollState invariants; make the fields private (or at least keep offset private) and add small setter(s) on ScrollState such as set_total_lines, set_visible_height or update_dimensions that update total_lines/visible_height and then re-clamp offset using the existing scroll/clamp logic (reuse the scroll method or its clamping math) so offset is always bounded between 0 and max(total_lines - visible_height, 0).tests/bottom_tests.rs (1)
92-94: Consider extracting a tiny helper for repeated focus setup in tests.The repeated
focus_state.focus+focus_state.sidebar_focusedassignments add noise; a helper would reduce duplication and future churn.Also applies to: 150-151, 182-183, 219-220, 292-293, 351-352, 385-386, 427-428, 469-470, 523-524, 636-637, 669-670, 731-732, 907-908, 943-944, 977-978, 1010-1011, 1045-1046, 1079-1080, 1155-1156, 1223-1224, 1269-1270, 1313-1314, 1366-1367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bottom_tests.rs` around lines 92 - 94, Extract a small test helper like fn set_sidebar_focus(state: &mut State, focus: Focus) (or similarly named) and replace the repeated assignments to focus_state.focus and focus_state.sidebar_focused with a single helper call; e.g., call set_sidebar_focus(&mut state, Focus::ActivityLog) wherever you currently do state.focus_state.focus = Focus::ActivityLog; state.focus_state.sidebar_focused = true; to reduce duplication and update the other listed occurrences similarly.src/ui/panes.rs (1)
138-138: Clarify popup width calculation.The expression
area.width.min(32).max(area.width.min(14))simplifies toarea.width.min(32)whenarea.width >= 14, and14whenarea.width < 14. This effectively clamps the popup width to the range[14, 32]while respecting the available area width. Consider adding a brief comment or refactoring for clarity:let popup_width = area.width.clamp(14, 32);However, note that
clampwould enforce minimum 14 even whenarea.width < 14, which differs from the current behavior that allows narrower popups. If the intent is to allow popups narrower than 14 on very small terminals, the current logic is correct but could use a comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/panes.rs` at line 138, The popup width calculation using let popup_width = area.width.min(32).max(area.width.min(14)); is confusing—decide whether you want to strictly clamp to [14,32] or allow widths <14 on very small terminals; if you want strict clamping, replace the expression with area.width.clamp(14, 32) (update any uses of popup_width accordingly), otherwise keep the current logic but add a short comment next to popup_width explaining that the code intentionally allows widths smaller than 14 on tiny terminals and effectively clamps to 32 for larger areas; reference popup_width and area.width when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 312-317: Update the earlier default note that says "(default:
all)" to match the new opt-in behavior for `@sidebar_notifications_events`: change
it to indicate the unset/default is "every event except task_completed" (or
similar wording such as "(default: all except task_completed)") so the earlier
note aligns with the later sentence "unset = every event except
`task_completed`"; update the mention of `@sidebar_notifications_events` and any
parenthetical default text to the new wording.
In `@src/cli/hook/context.rs`:
- Around line 39-53: sync_pane_location currently only checks `@pane_subagents`
via current_subagents, which loses the race when a child SessionStart arrives
before SubagentStart; to fix, additionally guard against session mismatches when
current_subagents is empty: inside sync_pane_location, if
should_update_cwd(¤t_subagents) is false (or current_subagents is empty)
and session_id.is_some(), fetch the pane's recorded session option (e.g., call
tmux::get_pane_option_value(pane, "@pane_session") or the existing pane-owner
session option) and if that recorded session differs from the incoming
session_id, return early to skip the write; update the logic around
current_subagents/should_update_cwd to perform this extra session check so a
child SessionStart cannot overwrite the parent's pane identity.
In `@src/cli/hook/handlers.rs`:
- Around line 20-31: on_session_start is unconditionally clearing pane-scoped
subagent and teardown tracking (tmux::unset_pane_option for "@pane_subagents",
PENDING_SESSION_END, PENDING_WORKTREE_REMOVE), which erases state for parent
sessions when a child SessionStart runs; change on_session_start to detect
whether the incoming session is the owning/parent session (e.g., via
AgentContext session metadata or a is_parent/is_root flag on ctx) and only
perform the unset of "@pane_subagents" and the pending teardown markers when
this is the parent/root session; leave the other setup calls (set_agent_meta,
set_attention, clear_run_state, set_notification_run_id, unsetting `@pane_prompt`
and `@pane_prompt_source`) unchanged for all sessions so on_subagent_stop can
still drain PENDING_SESSION_END / PENDING_WORKTREE_REMOVE.
In `@src/state/filter.rs`:
- Around line 85-89: The from_label match treats the literal "all" as
RepoFilter::All which prevents a real repo named "all" from being round-tripped;
change the encoding scheme so RepoFilter::Repo values are serialized with a
reserved prefix (e.g. "r:") and update RepoFilter::from_label to recognize that
prefix and return Repo(name) for prefixed strings while still mapping the
unprefixed "all" (and empty) to RepoFilter::All; also update the serializer (the
function that produces labels for RepoFilter) to emit the same "r:" prefix for
Repo variants so parsing and serialization are consistent.
In `@src/state/global.rs`:
- Around line 54-85: flush_pending_cursor_save currently clears
pending_cursor_save_since before attempting persistence so a failed
tmux::run_tmux() (invoked by save_cursor) drops the queued update; change the
logic so the pending timestamp is only cleared on a successful write: either
make save_cursor return a bool (or expose the tmux::run_tmux result) and call
save_cursor() then set pending_cursor_save_since = None only when it returns
success, or perform the tmux::run_tmux call directly inside
flush_pending_cursor_save and clear pending_cursor_save_since only if that call
succeeds; update references to flush_pending_cursor_save, save_cursor, and
pending_cursor_save_since accordingly.
In `@src/state/notices.rs`:
- Around line 30-32: The doc comment above the ClaudePluginNotice enum is
incorrect—update it to reflect the current three variants instead of saying “two
states”; mention that the enum drives the `Plugin / claude` section in the
notices popup and that its state is derived from
`claude_plugin_installed_version` in `refresh_notices`, and optionally list or
refer to the three variants of `ClaudePluginNotice` to avoid future drift.
In `@src/tmux.rs`:
- Around line 263-270: The code currently re-joins escaped pane fields into a
single string which reintroduces pipe splits and corrupts downstream indexes;
instead, stop round-tripping and pass the slice of unescaped fields straight
into the pane parser (e.g. change the call site that builds pane_line from
parts[session_line_field::PANE_LINE_OFFSET..].join("|") to pass the slice), and
update or add a parse_pane_fields(&[String]) helper (or adjust parse_pane_line
to accept &[String]) to index directly into parts; apply the same change to the
similar block around the other occurrence mentioned (lines 284-290) so no
pane-level field is re-split after split_tmux_fields().
In `@src/ui/bottom/activity.rs`:
- Around line 54-58: After updating state.activity.scroll.total_lines and
state.activity.scroll.visible_height, clamp state.activity.scroll.offset to
ensure it is within 0..=(total_lines.saturating_sub(visible_height)) before
creating the Paragraph; specifically, compute a max_offset (e.g.,
total_lines.saturating_sub(visible_height).min(total_lines)) and set
state.activity.scroll.offset = state.activity.scroll.offset.min(max_offset) (and
ensure it is not negative if using signed types) so the value passed to
Paragraph::new(lines).scroll((scroll_offset, 0)) is always in-range.
In `@src/ui/bottom/git.rs`:
- Around line 387-391: After updating state.scrolls.git.total_lines and
state.scrolls.git.visible_height, clamp state.scrolls.git.offset to a valid
range before using it for rendering: if total_lines <= visible_height set offset
to 0, otherwise ensure offset <= total_lines - visible_height. Do the clamp just
before reading state.scrolls.git.offset (prior to creating scroll_offset and
Paragraph::new(lines).scroll), taking care to handle integer type conversions
between usize and the offset's type.
---
Nitpick comments:
In `@src/state/scroll.rs`:
- Around line 2-13: Public fields offset, total_lines, and visible_height allow
callers to change dimensions without re-clamping offset, breaking ScrollState
invariants; make the fields private (or at least keep offset private) and add
small setter(s) on ScrollState such as set_total_lines, set_visible_height or
update_dimensions that update total_lines/visible_height and then re-clamp
offset using the existing scroll/clamp logic (reuse the scroll method or its
clamping math) so offset is always bounded between 0 and max(total_lines -
visible_height, 0).
In `@src/ui/panes.rs`:
- Line 138: The popup width calculation using let popup_width =
area.width.min(32).max(area.width.min(14)); is confusing—decide whether you want
to strictly clamp to [14,32] or allow widths <14 on very small terminals; if you
want strict clamping, replace the expression with area.width.clamp(14, 32)
(update any uses of popup_width accordingly), otherwise keep the current logic
but add a short comment next to popup_width explaining that the code
intentionally allows widths smaller than 14 on tiny terminals and effectively
clamps to 32 for larger areas; reference popup_width and area.width when making
the change.
In `@tests/bottom_tests.rs`:
- Around line 92-94: Extract a small test helper like fn
set_sidebar_focus(state: &mut State, focus: Focus) (or similarly named) and
replace the repeated assignments to focus_state.focus and
focus_state.sidebar_focused with a single helper call; e.g., call
set_sidebar_focus(&mut state, Focus::ActivityLog) wherever you currently do
state.focus_state.focus = Focus::ActivityLog; state.focus_state.sidebar_focused
= true; to reduce duplication and update the other listed occurrences similarly.
🪄 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: bd4a275d-fd5a-40da-a84d-ffb6073d5059
📒 Files selected for processing (42)
CLAUDE.mdREADME.mddocs/state-management.mdsrc/cli/hook.rssrc/cli/hook/activity.rssrc/cli/hook/context.rssrc/cli/hook/handlers.rssrc/cli/hook/notifications.rssrc/desktop_notification.rssrc/group.rssrc/main.rssrc/state.rssrc/state/activity.rssrc/state/filter.rssrc/state/focus.rssrc/state/global.rssrc/state/layout.rssrc/state/notices.rssrc/state/pane_runtime.rssrc/state/popup.rssrc/state/refresh.rssrc/state/scroll.rssrc/state/session.rssrc/state/tab.rssrc/state/timers.rssrc/tmux.rssrc/ui/bottom.rssrc/ui/bottom/activity.rssrc/ui/bottom/git.rssrc/ui/panes.rssrc/ui/panes/click_targets.rssrc/ui/panes/filter_bar.rssrc/ui/panes/popups.rssrc/ui/panes/row.rssrc/ui/panes/row_collector.rstests/bottom_tests.rstests/color_tests.rstests/panes_tests.rstests/state_tests.rstests/styled_tests.rstests/test_helpers.rstests/ui_snapshot.rs
Comment on lines
+39
to
+53
| pub(super) fn sync_pane_location( | ||
| pane: &str, | ||
| cwd: &str, | ||
| worktree: &Option<WorktreeInfo>, | ||
| session_id: &Option<String>, | ||
| ) { | ||
| // Subagents share the parent's $TMUX_PANE and can fire their own hook | ||
| // events with a different session_id, cwd, or worktree. While children | ||
| // are active, every pane-scoped write must be skipped so the parent's | ||
| // identity is preserved — including `@pane_worktree_*`, which used to | ||
| // leak through and misgroup the pane under the child's repo. | ||
| let current_subagents = tmux::get_pane_option_value(pane, "@pane_subagents"); | ||
| if !should_update_cwd(¤t_subagents) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The subagent write guard still loses the SessionStart race.
The new test on Lines 704-712 already shows the failure mode: if a child SessionStart arrives before SubagentStart, @pane_subagents is still empty here, so the child's cwd/session/worktree overwrites the parent pane. That means the refactor still leaves a real mis-grouping path open instead of actually defending the parent identity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/hook/context.rs` around lines 39 - 53, sync_pane_location currently
only checks `@pane_subagents` via current_subagents, which loses the race when a
child SessionStart arrives before SubagentStart; to fix, additionally guard
against session mismatches when current_subagents is empty: inside
sync_pane_location, if should_update_cwd(¤t_subagents) is false (or
current_subagents is empty) and session_id.is_some(), fetch the pane's recorded
session option (e.g., call tmux::get_pane_option_value(pane, "@pane_session") or
the existing pane-owner session option) and if that recorded session differs
from the incoming session_id, return early to skip the write; update the logic
around current_subagents/should_update_cwd to perform this extra session check
so a child SessionStart cannot overwrite the parent's pane identity.
Comment on lines
+20
to
+31
| pub(super) fn on_session_start(pane: &str, ctx: &AgentContext<'_>) -> i32 { | ||
| set_agent_meta(pane, ctx); | ||
| set_attention(pane, "clear"); | ||
| clear_run_state(pane); | ||
| set_notification_run_id(pane); | ||
| tmux::unset_pane_option(pane, "@pane_prompt"); | ||
| tmux::unset_pane_option(pane, "@pane_prompt_source"); | ||
| tmux::unset_pane_option(pane, "@pane_subagents"); | ||
| // 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.
Don't clear subagent tracking on every SessionStart.
A child SessionStart that lands after SubagentStart will hit these lines and erase both @pane_subagents and the pending teardown markers. After that, on_subagent_stop() can no longer drain PENDING_SESSION_END / PENDING_WORKTREE_REMOVE, so stale parent pane metadata can survive indefinitely. This needs a parent-session check before clearing pane-scoped tracking state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/hook/handlers.rs` around lines 20 - 31, on_session_start is
unconditionally clearing pane-scoped subagent and teardown tracking
(tmux::unset_pane_option for "@pane_subagents", PENDING_SESSION_END,
PENDING_WORKTREE_REMOVE), which erases state for parent sessions when a child
SessionStart runs; change on_session_start to detect whether the incoming
session is the owning/parent session (e.g., via AgentContext session metadata or
a is_parent/is_root flag on ctx) and only perform the unset of "@pane_subagents"
and the pending teardown markers when this is the parent/root session; leave the
other setup calls (set_agent_meta, set_attention, clear_run_state,
set_notification_run_id, unsetting `@pane_prompt` and `@pane_prompt_source`)
unchanged for all sessions so on_subagent_stop can still drain
PENDING_SESSION_END / PENDING_WORKTREE_REMOVE.
Comment on lines
+85
to
+89
| pub fn from_label(s: &str) -> Self { | ||
| match s { | ||
| "all" | "" => Self::All, | ||
| name => Self::Repo(name.to_string()), | ||
| } |
There was a problem hiding this comment.
RepoFilter cannot round-trip a repo literally named "all".
On parse, "all" is always treated as RepoFilter::All, so a real group/repo named "all" cannot be persisted/restored correctly. Consider an encoded tmux value format (e.g., reserved prefix for Repo(name)) to avoid this collision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state/filter.rs` around lines 85 - 89, The from_label match treats the
literal "all" as RepoFilter::All which prevents a real repo named "all" from
being round-tripped; change the encoding scheme so RepoFilter::Repo values are
serialized with a reserved prefix (e.g. "r:") and update RepoFilter::from_label
to recognize that prefix and return Repo(name) for prefixed strings while still
mapping the unprefixed "all" (and empty) to RepoFilter::All; also update the
serializer (the function that produces labels for RepoFilter) to emit the same
"r:" prefix for Repo variants so parsing and serialization are consistent.
Comment on lines
+54
to
58
| state.activity.scroll.total_lines = lines.len(); | ||
| state.activity.scroll.visible_height = inner.height as usize; | ||
|
|
||
| let scroll_offset = state.activity_scroll.offset as u16; | ||
| let scroll_offset = state.activity.scroll.offset as u16; | ||
| let paragraph = Paragraph::new(lines).scroll((scroll_offset, 0)); |
There was a problem hiding this comment.
Clamp activity scroll offset after recalculating totals.
Like the git panel, this path updates total_lines/visible_height but can keep an out-of-range offset when entries shrink. Clamp once before Paragraph::scroll.
💡 Proposed fix
state.activity.scroll.total_lines = lines.len();
state.activity.scroll.visible_height = inner.height as usize;
+ state.activity.scroll.scroll(0);
let scroll_offset = state.activity.scroll.offset as u16;📝 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
| state.activity.scroll.total_lines = lines.len(); | |
| state.activity.scroll.visible_height = inner.height as usize; | |
| let scroll_offset = state.activity_scroll.offset as u16; | |
| let scroll_offset = state.activity.scroll.offset as u16; | |
| let paragraph = Paragraph::new(lines).scroll((scroll_offset, 0)); | |
| state.activity.scroll.total_lines = lines.len(); | |
| state.activity.scroll.visible_height = inner.height as usize; | |
| state.activity.scroll.scroll(0); | |
| let scroll_offset = state.activity.scroll.offset as u16; | |
| let paragraph = Paragraph::new(lines).scroll((scroll_offset, 0)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/bottom/activity.rs` around lines 54 - 58, After updating
state.activity.scroll.total_lines and state.activity.scroll.visible_height,
clamp state.activity.scroll.offset to ensure it is within
0..=(total_lines.saturating_sub(visible_height)) before creating the Paragraph;
specifically, compute a max_offset (e.g.,
total_lines.saturating_sub(visible_height).min(total_lines)) and set
state.activity.scroll.offset = state.activity.scroll.offset.min(max_offset) (and
ensure it is not negative if using signed types) so the value passed to
Paragraph::new(lines).scroll((scroll_offset, 0)) is always in-range.
Comment on lines
+387
to
391
| state.scrolls.git.total_lines = lines.len(); | ||
| state.scrolls.git.visible_height = content_height as usize; | ||
|
|
||
| let scroll_offset = state.git_scroll.offset as u16; | ||
| let scroll_offset = state.scrolls.git.offset as u16; | ||
| let paragraph = Paragraph::new(lines).scroll((scroll_offset, 0)); |
There was a problem hiding this comment.
Clamp git scroll offset after viewport metadata updates.
After Line 387–388, offset may be out of range if content shrank since the last frame. Clamp before rendering to avoid empty/overscrolled output.
💡 Proposed fix
state.scrolls.git.total_lines = lines.len();
state.scrolls.git.visible_height = content_height as usize;
+ state.scrolls.git.scroll(0);
let scroll_offset = state.scrolls.git.offset as u16;📝 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
| state.scrolls.git.total_lines = lines.len(); | |
| state.scrolls.git.visible_height = content_height as usize; | |
| let scroll_offset = state.git_scroll.offset as u16; | |
| let scroll_offset = state.scrolls.git.offset as u16; | |
| let paragraph = Paragraph::new(lines).scroll((scroll_offset, 0)); | |
| state.scrolls.git.total_lines = lines.len(); | |
| state.scrolls.git.visible_height = content_height as usize; | |
| state.scrolls.git.scroll(0); | |
| let scroll_offset = state.scrolls.git.offset as u16; | |
| let paragraph = Paragraph::new(lines).scroll((scroll_offset, 0)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/bottom/git.rs` around lines 387 - 391, After updating
state.scrolls.git.total_lines and state.scrolls.git.visible_height, clamp
state.scrolls.git.offset to a valid range before using it for rendering: if
total_lines <= visible_height set offset to 0, otherwise ensure offset <=
total_lines - visible_height. Do the clamp just before reading
state.scrolls.git.offset (prior to creating scroll_offset and
Paragraph::new(lines).scroll), taking care to handle integer type conversions
between usize and the offset's type.
…tice doc drift - README: the parenthetical '(default: all)' no longer matches the actual default after task_completed became opt-in; update to '(default: all except task_completed)' so it matches the later 'unset = every event except task_completed' paragraph. - state/notices.rs: ClaudePluginNotice currently has three variants, but the rustdoc comment still said 'two states'. Reword to point at the enum instead of hardcoding a count.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/event/adapter.rs (1)
7-18: Consider resolving event names toAgentEventKindbefore adapter dispatch.Line 9 still takes a raw
&str, so adapters can drift from the canonical mapping insrc/event/kind.rs. That means renames/additions still require keeping multiple string tables in sync instead of letting the enum be the actual source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event/adapter.rs` around lines 7 - 18, Change the EventAdapter contract to accept a resolved AgentEventKind instead of a raw &str to ensure adapters use the canonical enum; update the trait signature EventAdapter::parse(&self, event_kind: &AgentEventKind, input: &Value) -> Option<AgentEvent>, then update resolve_adapter to resolve the incoming event name to AgentEventKind (use the enum/parser in src/event/kind.rs) before dispatching to the adapter implementations (adapter::claude::ClaudeAdapter, adapter::codex::CodexAdapter) and update those adapter implementations to match the new parse signature returning AgentEvent.
🤖 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/app/input.rs`:
- Around line 171-176: When handling MouseEventKind::Down for bottom tabs,
calling state.handle_bottom_tab_click(mouse.column) alone leaves the worker's
git_tab_active atomic out of sync; after invoking state.handle_bottom_tab_click
you must immediately update the git_tab_active atomic (the same flag BackTab
updates) to reflect the new active bottom tab so background polling toggles
immediately. Locate the MouseEventKind::Down branch that calls
state.handle_bottom_tab_click and add an immediate update to git_tab_active
(e.g., store the result of whatever state method indicates the Git tab is
active) so the worker flag matches the UI change.
In `@src/app/render.rs`:
- Around line 22-27: The code currently uses state.pending_osc52_copy.take()
which drops the payload before performing I/O and it ignores write/flush errors;
instead, read the payload without removing it (e.g., use
state.pending_osc52_copy.as_ref() or clone the payload), build the OSC52
sequence with clipboard::osc52_sequence, perform
backend.write_all(seq.as_bytes())? and backend.flush()? propagating any errors
(use ? or return Err) rather than ignoring them, and only call
state.pending_osc52_copy.take() to clear the payload after both write and flush
succeed; reference the pending_osc52_copy access, clipboard::osc52_sequence,
backend.write_all, backend.flush, and render_frame in your fix.
---
Nitpick comments:
In `@src/event/adapter.rs`:
- Around line 7-18: Change the EventAdapter contract to accept a resolved
AgentEventKind instead of a raw &str to ensure adapters use the canonical enum;
update the trait signature EventAdapter::parse(&self, event_kind:
&AgentEventKind, input: &Value) -> Option<AgentEvent>, then update
resolve_adapter to resolve the incoming event name to AgentEventKind (use the
enum/parser in src/event/kind.rs) before dispatching to the adapter
implementations (adapter::claude::ClaudeAdapter, adapter::codex::CodexAdapter)
and update those adapter implementations to match the new parse signature
returning AgentEvent.
🪄 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: 04e4d37c-70a4-4209-b60a-bb07e4c16527
📒 Files selected for processing (10)
src/app.rssrc/app/input.rssrc/app/render.rssrc/app/setup.rssrc/app/workers.rssrc/event.rssrc/event/adapter.rssrc/event/kind.rssrc/lib.rssrc/main.rs
✅ Files skipped from review due to trivial changes (1)
- src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main.rs
1. state/global.rs: flush_pending_cursor_save was clearing pending_cursor_save_since before tmux wrote. A transient tmux failure would drop the queued save. save_cursor now returns bool and the flush only clears the marker on success. 2. tmux.rs: build_session_hierarchy used to split tmux output on '|', unescape '\|', then re-join the pane fields with '|' before handing them to parse_pane_line, which re-split on '|'. Any pane field containing a literal pipe (prompt, cwd, branch) would have its fields shifted on the second pass and corrupt the row. Split parse_pane_line into parse_pane_fields(&[String]) so production passes the already-unescaped slice directly; the &str wrapper now only exists for unit-test fixtures (cfg(test)). 3. ui/bottom/activity.rs: total_lines and visible_height were updated every frame but the offset was not clamped, so a shrinking entry buffer (focus change, log trim) left the paragraph over-scrolled. Clamp via ScrollState::scroll(0) before rendering. 4. ui/bottom/git.rs: same clamp gap on the git file panel. Scroll to bottom with fewer files than the viewport used to cut off the head of the list; now it shows the full list. Updated the existing snapshot test to document the new no-overflow behavior.
- app/input.rs: mouse click on a bottom tab was not updating the git_tab_active atomic, so clicking into Git Status left the background poller disabled until the next refresh tick. Keyboard BackTab already does this store; mirror it here. - app/render.rs: render_frame dropped state.pending_osc52_copy before write/flush, and both I/O calls ignored errors. A failed write made the copy a silent no-op. Now write+flush propagate errors and the payload is restored on failure so the next tick retries.
This was referenced Apr 17, 2026
Merged
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
Pure refactor that decomposes four high-complexity modules while preserving every observable behavior.
state.rs→AppStateplus topical submodules (activity,session,focus,scroll,pane_runtime,layout,popup,notices,timers,filter,global,refresh,tab).tmux.rs→WorktreeMetadatasub-struct + named constants forparse_pane_linefield indices.ui/panes.rs→draw_agentssplit intofilter_bar,row_collector,click_targets,popupssubmodules with aPaneLayoutcoordinator.cli/hook.rs(1934 → 175 lines) →context,handlers,activity,notificationssubmodules.Snapshot tests remained byte-identical throughout — no
cargo insta acceptwas needed. Extracted units got unit-test coverage where behavior-bearing:ActivityState,SessionNamesState,FocusState,ScrollStates,PaneRuntimeMap,PaneLayout,row_collector::collect,click_targets::materialize,make_ctx, plus a subset ofon_*handlers.Also bundled (unrelated):
feat(notifications): make task_completed opt-in(55b0772) — desktop notifications for TaskCompleted now require explicit opt-in via@sidebar_notifications_events.Test plan
cargo fmt --checkcargo clippy(no new warnings; 5 pre-existing warnings on untouched code)cargo test— 992 passed / 0 failed (946 → 992: +46 new tests)cargo insta pending-snapshots— emptycargo build --release🤖 Generated with Claude Code
Summary by CodeRabbit