UI polish: rounded radii, search cache, and resize#296
Merged
Conversation
Polish UI geometry and interactions across the app: introduce consistent rounded radii for settings, command palette, scrollbars and toasts; adjust search bar sizing and alpha; update toast timings and easing for smoother animation. Improve settings and sidebar text selection to handle double/triple clicks and avoid accidental drag-select. Add a cached sidebar search matcher to avoid repeated computation and rename "Theme Store" to "Themes" in UI. Native SDK: add Rename/Close tab items and IDs and wire handlers into the terminal view context menu. Pane resizing: increase divider hit margin, introduce PaneResizeResult enum, return rich results from hit/resize routines, track blocked state and hovered divider for cursor feedback, and throttle window-resize -> terminal resize to avoid SIGWINCH floods (with a scheduled follow-up). Other tweaks: grow/shrink-safe row cache resizing in the terminal grid, minor Dockerfile build helper, bump crate versions, and assorted render/shadow adjustments for toasts.
|
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 40 minutes and 27 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 (9)
📝 WalkthroughWalkthroughThis PR adds shell-integration and OSC handling, notification/progress state plumbing, settings UI/search improvements, tab context menu actions (Rename/Close) and tab progress badges, pane-resize hover/throttle semantics, UI styling/geometry updates, grid paint-cache reuse, new shell integration scripts, and assorted tests/docs/version bumps. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Mouse as Mouse Handler
participant Pane as Pane Resizer
participant UI as UI Renderer
participant Native as Native Terminal
User->>Mouse: Move / click on divider
Mouse->>Mouse: pane_resize_hit_test()
alt Hovered
Mouse->>UI: set hovered_pane_divider, request redraw
UI->>User: show resize cursor
else Not hovered
Mouse->>UI: clear hover
end
User->>Mouse: Drag divider
loop drag steps
Mouse->>Pane: native_resize_pane_step(delta)
Pane->>Native: compute/apply geometry, may resize terminal
alt Applied
Pane-->>Mouse: PaneResizeResult::Applied
Mouse->>UI: update layout
else BlockedByMinimum
Pane-->>Mouse: PaneResizeResult::BlockedByMinimum
Mouse->>UI: set pane_resize_blocked, stop drag
else NoChange
Pane-->>Mouse: PaneResizeResult::NoChange
end
end
Mouse->>Mouse: Release drag
Mouse->>UI: clear pane_resize_blocked and hovered_pane_divider
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/settings_view/search.rs (1)
141-165:⚠️ Potential issue | 🟡 MinorFinish the “Themes” rename.
This change only updates the sidebar/search labels. The content pane still renders
Theme Store/THEME STOREinsrc/settings_view/sections.rs, so users will see both names in the same settings screen. Please update those strings in the same pass to keep navigation, search results, and section headers aligned.Also applies to: 589-589
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/settings_view/search.rs` around lines 141 - 165, Search for the Section header strings that still read "Theme Store" / "THEME STORE" in src/settings_view/sections.rs and replace them to match the sidebar/search rename: use "Themes" for title-case headers and "THEMES" for all-caps variants (the enum variant SettingsSection::ThemeStore stays unchanged); update any occurrences used in render_header, section_title, or similar functions so the UI shows "Themes" consistently across sidebar, search, and content pane.
🧹 Nitpick comments (3)
crates/terminal_ui/src/grid.rs (1)
448-463: Redundanttruncatecall fordirty_col_ranges.The
resizemethod already handles both growing and shrinking the vector. Whennew_len < len,resizeinternally truncates the vector, making the subsequenttruncatecall on line 461 redundant.♻️ Proposed simplification
// dirty_col_ranges is per-pass scratch — resize and reset every frame. // Use resize + fill to reuse the existing allocation when row count is stable. self.dirty_col_ranges.resize(row_count, None); - self.dirty_col_ranges.truncate(row_count); self.dirty_col_ranges.fill(None);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/terminal_ui/src/grid.rs` around lines 448 - 463, In ensure_row_capacity, remove the redundant dirty_col_ranges.truncate(row_count) call: keep the existing resize(row_count, None) and then call dirty_col_ranges.fill(None) so dirty_col_ranges is resized and reset each frame; update the function's body around the row_ops and dirty_col_ranges handling (references: ensure_row_capacity, row_ops, dirty_col_ranges) to eliminate the unnecessary truncate invocation.src/terminal_view/tabs/drag.rs (1)
416-419: Prefer deriving test coordinates from constants instead of hardcoding.Line 417 and Line 419 use magic numbers (
53.0,110.0). Computing these fromTAB_ITEM_HEIGHT/TAB_ITEM_GAPwill keep the test intent clear when layout constants evolve.Suggested refactor
- // 53.0 is past tab 1's midpoint (52), so slot 2 - assert_eq!(layout.drop_slot_for_pointer(53.0, 0.0), 2); - // Past all tabs - assert_eq!(layout.drop_slot_for_pointer(110.0, 0.0), 3); + let second_midpoint = (TAB_ITEM_HEIGHT + TAB_ITEM_GAP) + (TAB_ITEM_HEIGHT * 0.5); + // Just past tab 1 midpoint => slot 2 + assert_eq!(layout.drop_slot_for_pointer(second_midpoint + 1.0, 0.0), 2); + let past_all_tabs = (TAB_ITEM_HEIGHT * 3.0) + (TAB_ITEM_GAP * 2.0) + 1.0; + assert_eq!(layout.drop_slot_for_pointer(past_all_tabs, 0.0), 3);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/terminal_view/tabs/drag.rs` around lines 416 - 419, The test uses hardcoded pointer x-coordinates (53.0 and 110.0) which will break if layout constants change; replace those magic numbers by computing them from the tab sizing constants (e.g., TAB_ITEM_HEIGHT, TAB_ITEM_GAP or TAB_ITEM_WIDTH if present) so the assertions call layout.drop_slot_for_pointer(using computed midpoint and end positions) instead of fixed literals; update the two assertions around layout.drop_slot_for_pointer(...) to derive the pointer positions from the constants to preserve intent as TAB_* values evolve.src/terminal_view/render.rs (1)
2525-2553: Consider extracting shared divider styling logic.The visual state computation for right and bottom dividers is nearly identical—both compute
cursor_color_hslaandblocked_color, then apply the same conditional width/height and color logic. Extracting this into a helper could reduce duplication.♻️ Optional: Extract divider styling helper
fn divider_visual_state( is_dragging: bool, is_hovered: bool, is_blocked: bool, cursor_color: gpui::Hsla, default_color: gpui::Hsla, ) -> (f32, gpui::Hsla) { let blocked_color = gpui::Hsla { h: 0.0, s: 0.7, l: 0.5, a: 1.0 }; if is_dragging && is_blocked { (3.0, blocked_color) } else if is_dragging { (3.0, cursor_color) } else if is_hovered { (2.0, cursor_color) } else { (1.0, default_color) } }Also applies to: 2588-2616
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/terminal_view/render.rs` around lines 2525 - 2553, The right- and bottom-divider visual-state logic is duplicated; extract a helper (e.g., divider_visual_state) that takes (is_dragging: bool, is_hovered: bool, is_blocked: bool, cursor_color: gpui::Hsla, default_color: gpui::Hsla) and returns (f32, gpui::Hsla), then replace the blocks that compute cursor_color_hsla, blocked_color, right_divider_width/right_divider_color and the equivalent bottom_divider_width/bottom_divider_color with calls to this helper; update the code paths that use is_dragging_right/is_hovered_right and is_dragging_bottom/is_hovered_bottom so they pass pane_resize_blocked and divider_color into the helper and then use the returned width/color when building pane_dividers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/native_sdk/src/lib.rs`:
- Around line 523-526: The code calls MF_SEPARATOR in the AppendMenuW call but
the constant isn't imported; update the Windows import list from
windows::Win32::UI::WindowsAndMessaging to include MF_SEPARATOR (alongside
AppendMenuW, CreatePopupMenu, DestroyMenu, etc.) so the symbol is available for
the AppendMenuW call in lib.rs.
In `@src/terminal_view/mod.rs`:
- Around line 2424-2442: The scheduled task in
schedule_resize_throttle_follow_up only clears resize_throttle_task and
last_resize_applied_at but never applies any pending resize; change the spawn
closure so after clearing the bookkeeping it also invokes the view's resize-sync
routine (the same path used when a frame triggers a resize) to actually apply
deferred sizing — e.g. inside the this.update(...) block call the function that
applies pending resize (rename/replace apply_pending_resize with the actual
method in this file if different) so the terminal is resized even if no further
frames are rendered.
In `@src/terminal_view/tabs/drag.rs`:
- Around line 406-409: Update the misleading test comment that claims
TAB_ITEM_GAP=4 to reflect the actual constant TAB_ITEM_GAP=0.0 and recompute the
tab ranges and midpoints accordingly; locate the comment in the drag calculation
test block that lists "Tab 0: 0-32, midpoint=16" etc., and replace those
ranges/midpoints with the correct values derived using TAB_ITEM_GAP=0.0 so the
documentation matches the current constants used by the drag logic.
---
Outside diff comments:
In `@src/settings_view/search.rs`:
- Around line 141-165: Search for the Section header strings that still read
"Theme Store" / "THEME STORE" in src/settings_view/sections.rs and replace them
to match the sidebar/search rename: use "Themes" for title-case headers and
"THEMES" for all-caps variants (the enum variant SettingsSection::ThemeStore
stays unchanged); update any occurrences used in render_header, section_title,
or similar functions so the UI shows "Themes" consistently across sidebar,
search, and content pane.
---
Nitpick comments:
In `@crates/terminal_ui/src/grid.rs`:
- Around line 448-463: In ensure_row_capacity, remove the redundant
dirty_col_ranges.truncate(row_count) call: keep the existing resize(row_count,
None) and then call dirty_col_ranges.fill(None) so dirty_col_ranges is resized
and reset each frame; update the function's body around the row_ops and
dirty_col_ranges handling (references: ensure_row_capacity, row_ops,
dirty_col_ranges) to eliminate the unnecessary truncate invocation.
In `@src/terminal_view/render.rs`:
- Around line 2525-2553: The right- and bottom-divider visual-state logic is
duplicated; extract a helper (e.g., divider_visual_state) that takes
(is_dragging: bool, is_hovered: bool, is_blocked: bool, cursor_color:
gpui::Hsla, default_color: gpui::Hsla) and returns (f32, gpui::Hsla), then
replace the blocks that compute cursor_color_hsla, blocked_color,
right_divider_width/right_divider_color and the equivalent
bottom_divider_width/bottom_divider_color with calls to this helper; update the
code paths that use is_dragging_right/is_hovered_right and
is_dragging_bottom/is_hovered_bottom so they pass pane_resize_blocked and
divider_color into the helper and then use the returned width/color when
building pane_dividers.
In `@src/terminal_view/tabs/drag.rs`:
- Around line 416-419: The test uses hardcoded pointer x-coordinates (53.0 and
110.0) which will break if layout constants change; replace those magic numbers
by computing them from the tab sizing constants (e.g., TAB_ITEM_HEIGHT,
TAB_ITEM_GAP or TAB_ITEM_WIDTH if present) so the assertions call
layout.drop_slot_for_pointer(using computed midpoint and end positions) instead
of fixed literals; update the two assertions around
layout.drop_slot_for_pointer(...) to derive the pointer positions from the
constants to preserve intent as TAB_* values evolve.
🪄 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: 2c7b282f-9727-453b-8ae1-d8acec8ea99b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
CLAUDE.mdCargo.tomlDockerfile.apicrates/cli/Cargo.tomlcrates/native_sdk/src/lib.rscrates/terminal_ui/src/grid.rscrates/toast_sdk/src/lib.rssrc/settings_view/components.rssrc/settings_view/keybinds.rssrc/settings_view/mod.rssrc/settings_view/search.rssrc/settings_view/sections.rssrc/settings_view/style.rssrc/terminal_view/command_palette/render.rssrc/terminal_view/command_palette/style.rssrc/terminal_view/interaction/context_menu.rssrc/terminal_view/interaction/mouse.rssrc/terminal_view/interaction/terminal.rssrc/terminal_view/mod.rssrc/terminal_view/render.rssrc/terminal_view/search.rssrc/terminal_view/tab_strip/chrome_tests.rssrc/terminal_view/tab_strip/constants.rssrc/terminal_view/tab_strip/layout.rssrc/terminal_view/tab_strip/render_horizontal.rssrc/terminal_view/tab_strip/render_tab_item.rssrc/terminal_view/tabs/drag.rssrc/terminal_view/tabs/lifecycle.rssrc/terminal_view/tabs/sizing.rs
Add end-to-end support for custom OSC sequences: shell integration (OSC 133), notifications (OSC 9/777), progress indicators (OSC 9;4) and working directory (OSC 7). Introduces shell integration scripts for bash/fish/zsh, new AppConfig fields and defaults to control notifications, shell integration and progress indicators, and schema/parser support for these settings. Adds an osc_intercept module to pre-parse PTY output and extract OSC events, and wires those events into the terminal runtime (native event loop, new TerminalEvent variants and dispatch). Adds platform notification helpers (show_notification, is_app_active) in native_sdk and unit tests for the OSC parsing logic. Also updates terminal_ui crate to depend on polling and adjusts related code to support the new native event loop and OSC handling.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
crates/config_core/src/parser.rs (1)
744-782: LGTM with minor suggestion on error message.The parsing logic correctly handles all five new settings. For
NotificationMinDuration, the expected message says "a positive number of seconds" (line 757), but zero is actually valid (line 759 usesmax(0.0)). Consider changing to "a non-negative number of seconds" for clarity, though this is minor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/config_core/src/parser.rs` around lines 744 - 782, Update the user-facing error/validation message passed to parse_finite_f32_field for RootSettingId::NotificationMinDuration: change the description string "a positive number of seconds" to accurately reflect that zero is allowed (e.g. "a non-negative number of seconds") so it matches the behavior where config.notification_min_duration is clamped with parsed.max(0.0); locate the call in the match arm for NotificationMinDuration and update that literal string.crates/terminal_ui/src/shell_integration.rs (1)
45-49: Consider clearingcommand_startincommand_finished.After a command finishes,
command_startretains the timestamp from when execution began. This meanselapsed()will continue to return a duration even after the command completes, which could be misleading if queried later. If the intent is to only track elapsed time during execution, consider clearing it:💡 Suggested improvement
pub fn command_finished(&mut self, exit_code: Option<i32>) { self.last_exit_code = exit_code; self.phase = CommandPhase::Idle; + self.command_start = None; }If retaining the timestamp for post-completion queries (e.g., displaying "last command took X seconds") is intentional, this is fine as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/terminal_ui/src/shell_integration.rs` around lines 45 - 49, The command_finished method should also clear the stored start timestamp so elapsed() won't continue to report a duration after completion; in command_finished (which sets self.last_exit_code and self.phase = CommandPhase::Idle) add logic to reset self.command_start (e.g., set the Option/field to None or equivalent) so the start time is cleared when a command finishes while preserving behavior if you intentionally want to keep the timestamp for post-completion display.src/settings_view/state_apply.rs (1)
542-555: Missing upper bound enforcement for notification duration.The
NumericStepSpecinstate.rs(lines 613-620) defines bounds0.0-3600.0, and the increment/decrement logic uses.clamp(step.min, step.max). However, direct text input here only enforces the lower bound viamax(0.0), allowing values above 3600.0.💡 Suggested fix to enforce consistent bounds
EditableField::NotificationMinDuration => { let parsed = value.parse::<f32>().map_err(|_| { "Minimum command duration must be a non-negative number".to_string() })?; if !parsed.is_finite() { return Err("Minimum command duration must be finite".to_string()); } - let parsed = parsed.max(0.0); + let parsed = parsed.clamp(0.0, 3600.0); self.config.notification_min_duration = parsed; config::set_root_setting( termy_config_core::RootSettingId::NotificationMinDuration, &format!("{parsed:.3}"), ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/settings_view/state_apply.rs` around lines 542 - 555, The NotificationMinDuration branch (EditableField::NotificationMinDuration) only enforces a lower bound; enforce the same upper bound used by NumericStepSpec (3600.0) before assigning and persisting: after parsing and finiteness check clamp the value into [0.0, 3600.0] (or use parsed.min(3600.0).max(0.0)) and then set self.config.notification_min_duration and call config::set_root_setting as before; this keeps the direct-text input consistent with the increment/decrement bounds defined in NumericStepSpec in state.rs.src/settings_view/state.rs (1)
613-620: Consider aligning upper bound validation withstate_apply.rs.The
NumericStepSpecdefinesmax: 3600.0, but perstate_apply.rs(lines 542-555), text input validation only applies.max(0.0)(lower bound) without enforcing the upper bound. Direct text entry could accept values exceeding 3600 seconds.For consistency, consider updating
state_apply.rsto also clamp with.min(3600.0)or document that the max is only enforced via the stepper UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/settings_view/state.rs` around lines 613 - 620, The NumericStepSpec for EditableField::NotificationMinDuration sets max: 3600.0 but the text-entry validation in state_apply.rs only enforces the lower bound; update the parsing/validation for EditableField::NotificationMinDuration in state_apply.rs (the branch that currently calls .max(0.0)) to also enforce the upper bound (e.g., use .clamp(0.0, 3600.0) or apply a .min(3600.0) after the lower-bound check) so text input cannot exceed 3600 seconds and remains consistent with Self::numeric_field_spec/NumericStepSpec.assets/shell/termy.zsh (1)
39-42: Potential conflict:zle-line-initwidget override.Using
zle -N zle-line-init __termy_zle_line_initwill replace any existing user-definedzle-line-initwidget, potentially breaking custom Zsh configurations (e.g., vi-mode indicators, syntax highlighting hooks).Consider checking for an existing widget and chaining to it:
♻️ Suggested approach
+# Save existing zle-line-init if present +if (( ${+widgets[zle-line-init]} )); then + zle -A zle-line-init __termy_orig_zle_line_init +fi + __termy_zle_line_init() { printf '\e]133;B\a' + # Call original if it existed + (( ${+functions[__termy_orig_zle_line_init]} )) && __termy_orig_zle_line_init } zle -N zle-line-init __termy_zle_line_init🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/shell/termy.zsh` around lines 39 - 42, The current registration with zle -N zle-line-init __termy_zle_line_init will override any existing zle-line-init widget; change it to detect and preserve/chaining: if an existing zle-line-init exists, rename or save it (e.g., __termy_orig_zle_line_init) and then define __termy_zle_line_init to call the preserved original widget (if present) before/after sending the '\e]133;B\a' sequence, and finally register the new wrapper under zle-line-init; use the presence check for the widget (via functions/whence/ zle -l) to decide whether to rename and call the original so you don’t break user-defined hooks like vi-mode or syntax-highlighting.crates/terminal_ui/src/osc_intercept.rs (1)
82-157: Consider adding a buffer size limit to prevent memory exhaustion.The
bufferaccumulates bytes indefinitely while parsing an OSC payload. A malformed or malicious input stream that starts an OSC sequence but never terminates it could cause unbounded memory growth.🛡️ Suggested defensive limit
+const MAX_OSC_PAYLOAD_LEN: usize = 4096; + pub fn process(&mut self, input: &[u8]) -> (Vec<u8>, Vec<OscEvent>) { // ... ParseState::OscPayload => { if byte == 0x07 { // BEL = OSC terminator // ... + } else if self.buffer.len() >= MAX_OSC_PAYLOAD_LEN { + // Payload too long, abort and pass through + self.emit_osc_to_output(&mut output); + output.push(byte); + self.buffer.clear(); + self.state = ParseState::Ground; } else if byte == 0x1B { // ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/terminal_ui/src/osc_intercept.rs` around lines 82 - 157, The OSC payload buffer in process() (used by ParseState::OscStart, ParseState::OscPayload, ParseState::OscEscape and referenced by parse_osc_payload() and emit_osc_to_output()) can grow unbounded; add a defensive maximum buffer size constant (e.g. MAX_OSC_BUFFER) and check the buffer length before pushing bytes in OscStart/OscPayload/OscEscape branches, and if exceeded: stop accumulating, clear or emit the partial OSC back to output via emit_osc_to_output(), reset state to ParseState::Ground, and (optionally) record/log the overflow; ensure parse_osc_payload() is only called when buffer is within the limit so we never OOM on malformed input.assets/shell/termy.bash (2)
59-62: Consider URL-encoding special characters in PWD.If
$PWDcontains spaces or special characters, the OSC 7 URL may be malformed. Some terminals expect percent-encoding for the path component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/shell/termy.bash` around lines 59 - 62, The OSC 7 emitter __termy_report_cwd currently injects raw $PWD into the URL which can break for spaces/special characters; update __termy_report_cwd to percent‑encode the path component (i.e., URL‑encode $PWD) before composing the OSC 7 string and use that encoded value in the printf call (keep HOSTNAME/$(hostname) as before); implement the encoding either by adding a small shell helper (e.g., url_encode) or invoking a standard tool (python/perl/printf+xxd/etc.) to produce a percent-encoded path and fall back to raw $PWD only if encoding is unavailable.
36-49: DEBUG trap may fire for subshells and command substitutions.The DEBUG trap fires before every simple command, including those in command substitutions and subshells. While the
__termy_in_commandflag prevents duplicate C markers within a single command line, nested subshells could still emit unexpected markers.This is a known limitation of Bash shell integration. Consider documenting this behavior or using
BASH_SUBSHELLto guard:if [[ "$__termy_in_command" == "0" && "$BASH_SUBSHELL" == "0" ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/shell/termy.bash` around lines 36 - 49, The DEBUG trap (__termy_debug_trap) can fire in subshells/command substitutions; update the guard so it only emits the C marker for top-level commands by checking BASH_SUBSHELL as well as __termy_in_command (i.e., only set __termy_in_command=1 and printf when __termy_in_command is "0" and BASH_SUBSHELL is "0"), and keep the existing early-return for prompt commands; reference __termy_debug_trap, __termy_in_command and the trap declaration 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 `@assets/shell/termy.bash`:
- Around line 21-27: The first prompt handler __termy_prompt_command currently
always prints the D marker (OSC 133;D;0) on the initial prompt; add a simple
guard variable (e.g., __termy_prompt_seen or __termy_initialized) inside
__termy_prompt_command and only emit the D marker if that flag is already set,
then set the flag after the first invocation (or after emitting A) so subsequent
prompts behave normally; update references to the flag within
__termy_prompt_command to locate the change.
In `@crates/native_sdk/src/lib.rs`:
- Around line 1267-1275: The Windows branch of is_app_active currently only
checks that GetForegroundWindow() returns a non-null handle; change it to obtain
the foreground window's owning process ID via GetWindowThreadProcessId, compare
that PID to the current process ID (e.g., GetCurrentProcessId or
std::process::id() cast to DWORD) and return true only if they match; also add
GetWindowThreadProcessId to the Windows imports (in the list around line 24) and
keep the call inside the existing unsafe block.
In `@crates/terminal_ui/src/runtime.rs`:
- Around line 916-918: The code registers the PTY using a hard-coded token 0 via
PollingEvent::readable(0) so the platform-specific
NATIVE_EVENT_LOOP_READ_WRITE_TOKEN (value 2 on Windows) is never used; replace
the hard-coded 0 with NATIVE_EVENT_LOOP_READ_WRITE_TOKEN when constructing the
PollingEvent/interest (and any writable counterpart) so the PTY is registered
with the platform-specific read/write token; update the lines that create
interest (PollMode/PollingEvent::readable and any PollingEvent::writable usage)
to use NATIVE_EVENT_LOOP_READ_WRITE_TOKEN so the match handling PTY readiness
sees the correct token.
In `@src/terminal_view/mod.rs`:
- Around line 1177-1178: The tab-level ProgressState and CommandLifecycle fields
(ProgressState, command_lifecycle) are shared across panes and must be moved to
per-pane state so OSC 133 handling in the per-pane loop uses the correct
pane-specific lifecycle; update the struct that represents a pane (add
ProgressState and CommandLifecycle fields to the per-pane struct used by the
per-pane loop), initialize and clone them when creating panes, remove or stop
using the tab-scoped ProgressState/command_lifecycle, and change the OSC 133
handler code paths to read/update the lifecycle and progress_state from the
current pane instance rather than from the tab-level fields so each split pane
tracks its own running state and emits completion notifications for the correct
pane.
- Around line 3861-3864: The TerminalEvent::WorkingDirectory arm currently drops
the path; instead capture the path (e.g., TerminalEvent::WorkingDirectory(path))
and propagate it into the view state: call the appropriate updater (for example
PaneState::set_cwd, Tab::set_working_directory, or Workspace::update_active_cwd)
on the active pane/tab so the new cwd is stored and the UI can inherit it for
new tabs/panes; also trigger any necessary UI refresh or event emission so other
components observe the change. Ensure the path is persisted in the same place
other components read when creating new tabs/panes.
- Around line 3123-3127: The view-only flag shell_integration_enabled is not
driving the runtime's TabTitleShellIntegration.enabled (which is built from
tab_title.shell_integration), causing mismatches; update the code so the runtime
derives TERM_*_SHELL_INTEGRATION from the global config flag instead of the
per-tab value: either set TabTitleShellIntegration.enabled using the
config.shell_integration_enabled when constructing TabTitleShellIntegration in
this file (replace use of tab_title.shell_integration) or change
crates/terminal_ui/src/runtime.rs to read config.shell_integration_enabled as
the authoritative source; reference symbols: shell_integration_enabled,
TabTitleShellIntegration.enabled, TabTitle.shell_integration, and the runtime
file crates/terminal_ui/src/runtime.rs.
---
Nitpick comments:
In `@assets/shell/termy.bash`:
- Around line 59-62: The OSC 7 emitter __termy_report_cwd currently injects raw
$PWD into the URL which can break for spaces/special characters; update
__termy_report_cwd to percent‑encode the path component (i.e., URL‑encode $PWD)
before composing the OSC 7 string and use that encoded value in the printf call
(keep HOSTNAME/$(hostname) as before); implement the encoding either by adding a
small shell helper (e.g., url_encode) or invoking a standard tool
(python/perl/printf+xxd/etc.) to produce a percent-encoded path and fall back to
raw $PWD only if encoding is unavailable.
- Around line 36-49: The DEBUG trap (__termy_debug_trap) can fire in
subshells/command substitutions; update the guard so it only emits the C marker
for top-level commands by checking BASH_SUBSHELL as well as __termy_in_command
(i.e., only set __termy_in_command=1 and printf when __termy_in_command is "0"
and BASH_SUBSHELL is "0"), and keep the existing early-return for prompt
commands; reference __termy_debug_trap, __termy_in_command and the trap
declaration when making the change.
In `@assets/shell/termy.zsh`:
- Around line 39-42: The current registration with zle -N zle-line-init
__termy_zle_line_init will override any existing zle-line-init widget; change it
to detect and preserve/chaining: if an existing zle-line-init exists, rename or
save it (e.g., __termy_orig_zle_line_init) and then define __termy_zle_line_init
to call the preserved original widget (if present) before/after sending the
'\e]133;B\a' sequence, and finally register the new wrapper under zle-line-init;
use the presence check for the widget (via functions/whence/ zle -l) to decide
whether to rename and call the original so you don’t break user-defined hooks
like vi-mode or syntax-highlighting.
In `@crates/config_core/src/parser.rs`:
- Around line 744-782: Update the user-facing error/validation message passed to
parse_finite_f32_field for RootSettingId::NotificationMinDuration: change the
description string "a positive number of seconds" to accurately reflect that
zero is allowed (e.g. "a non-negative number of seconds") so it matches the
behavior where config.notification_min_duration is clamped with parsed.max(0.0);
locate the call in the match arm for NotificationMinDuration and update that
literal string.
In `@crates/terminal_ui/src/osc_intercept.rs`:
- Around line 82-157: The OSC payload buffer in process() (used by
ParseState::OscStart, ParseState::OscPayload, ParseState::OscEscape and
referenced by parse_osc_payload() and emit_osc_to_output()) can grow unbounded;
add a defensive maximum buffer size constant (e.g. MAX_OSC_BUFFER) and check the
buffer length before pushing bytes in OscStart/OscPayload/OscEscape branches,
and if exceeded: stop accumulating, clear or emit the partial OSC back to output
via emit_osc_to_output(), reset state to ParseState::Ground, and (optionally)
record/log the overflow; ensure parse_osc_payload() is only called when buffer
is within the limit so we never OOM on malformed input.
In `@crates/terminal_ui/src/shell_integration.rs`:
- Around line 45-49: The command_finished method should also clear the stored
start timestamp so elapsed() won't continue to report a duration after
completion; in command_finished (which sets self.last_exit_code and self.phase =
CommandPhase::Idle) add logic to reset self.command_start (e.g., set the
Option/field to None or equivalent) so the start time is cleared when a command
finishes while preserving behavior if you intentionally want to keep the
timestamp for post-completion display.
In `@src/settings_view/state_apply.rs`:
- Around line 542-555: The NotificationMinDuration branch
(EditableField::NotificationMinDuration) only enforces a lower bound; enforce
the same upper bound used by NumericStepSpec (3600.0) before assigning and
persisting: after parsing and finiteness check clamp the value into [0.0,
3600.0] (or use parsed.min(3600.0).max(0.0)) and then set
self.config.notification_min_duration and call config::set_root_setting as
before; this keeps the direct-text input consistent with the increment/decrement
bounds defined in NumericStepSpec in state.rs.
In `@src/settings_view/state.rs`:
- Around line 613-620: The NumericStepSpec for
EditableField::NotificationMinDuration sets max: 3600.0 but the text-entry
validation in state_apply.rs only enforces the lower bound; update the
parsing/validation for EditableField::NotificationMinDuration in state_apply.rs
(the branch that currently calls .max(0.0)) to also enforce the upper bound
(e.g., use .clamp(0.0, 3600.0) or apply a .min(3600.0) after the lower-bound
check) so text input cannot exceed 3600 seconds and remains consistent with
Self::numeric_field_spec/NumericStepSpec.
🪄 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: 9dc5fc5a-0667-4033-b723-9b0e61f05571
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
assets/shell/termy.bashassets/shell/termy.fishassets/shell/termy.zshcrates/config_core/src/parser.rscrates/config_core/src/parser_tests.rscrates/config_core/src/schema.rscrates/config_core/src/types.rscrates/native_sdk/src/lib.rscrates/terminal_ui/Cargo.tomlcrates/terminal_ui/src/grid.rscrates/terminal_ui/src/lib.rscrates/terminal_ui/src/osc_intercept.rscrates/terminal_ui/src/render_metrics.rscrates/terminal_ui/src/runtime.rscrates/terminal_ui/src/shell_integration.rsscripts/test_osc_sequences.shsrc/settings_view/sections.rssrc/settings_view/state.rssrc/settings_view/state_apply.rssrc/terminal_view/agents/render.rssrc/terminal_view/agents/status.rssrc/terminal_view/agents/workspace.rssrc/terminal_view/command_palette/state.rssrc/terminal_view/interaction/mouse.rssrc/terminal_view/interaction/native_panes.rssrc/terminal_view/mod.rssrc/terminal_view/render.rssrc/terminal_view/runtime/tmux/mod.rssrc/terminal_view/search.rssrc/terminal_view/tab_strip/constants.rssrc/terminal_view/tab_strip/render_horizontal.rssrc/terminal_view/tab_strip/render_tab_item.rssrc/terminal_view/tab_strip/render_vertical.rssrc/terminal_view/tabs/drag.rs
✅ Files skipped from review due to trivial changes (8)
- crates/terminal_ui/Cargo.toml
- src/terminal_view/interaction/native_panes.rs
- src/terminal_view/command_palette/state.rs
- crates/terminal_ui/src/render_metrics.rs
- src/terminal_view/agents/status.rs
- src/terminal_view/agents/workspace.rs
- src/terminal_view/agents/render.rs
- src/terminal_view/render.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/terminal_view/tab_strip/constants.rs
- src/terminal_view/tabs/drag.rs
- crates/terminal_ui/src/grid.rs
Comment on lines
+1177
to
+1178
| progress_state: ProgressState, | ||
| command_lifecycle: CommandLifecycle, |
There was a problem hiding this comment.
Keep shell lifecycle state per pane, not per tab.
The OSC 133 handling runs inside the per-pane loop at Lines 3748-3858, but this state is shared by the whole tab. In a split tab, one pane can overwrite another pane’s running state and fire the “command complete” notification for the wrong pane.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/terminal_view/mod.rs` around lines 1177 - 1178, The tab-level
ProgressState and CommandLifecycle fields (ProgressState, command_lifecycle) are
shared across panes and must be moved to per-pane state so OSC 133 handling in
the per-pane loop uses the correct pane-specific lifecycle; update the struct
that represents a pane (add ProgressState and CommandLifecycle fields to the
per-pane struct used by the per-pane loop), initialize and clone them when
creating panes, remove or stop using the tab-scoped
ProgressState/command_lifecycle, and change the OSC 133 handler code paths to
read/update the lifecycle and progress_state from the current pane instance
rather than from the tab-level fields so each split pane tracks its own running
state and emits completion notifications for the correct pane.
Comment on lines
+3123
to
+3127
| notifications_enabled: config.notifications_enabled, | ||
| notification_min_duration: config.notification_min_duration, | ||
| notify_only_unfocused: config.notify_only_unfocused, | ||
| shell_integration_enabled: config.shell_integration_enabled, | ||
| progress_indicator_enabled: config.progress_indicator_enabled, |
There was a problem hiding this comment.
shell_integration_enabled is only a view-side flag right now.
crates/terminal_ui/src/runtime.rs still derives TERMY_SHELL_INTEGRATION from TabTitleShellIntegration.enabled, which this file builds from tab_title.shell_integration, not from shell_integration_enabled. So the new toggle can be on while new PTYs emit no OSC 133/7/9, or off while shells keep emitting them.
Also applies to: 3441-3445
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/terminal_view/mod.rs` around lines 3123 - 3127, The view-only flag
shell_integration_enabled is not driving the runtime's
TabTitleShellIntegration.enabled (which is built from
tab_title.shell_integration), causing mismatches; update the code so the runtime
derives TERM_*_SHELL_INTEGRATION from the global config flag instead of the
per-tab value: either set TabTitleShellIntegration.enabled using the
config.shell_integration_enabled when constructing TabTitleShellIntegration in
this file (replace use of tab_title.shell_integration) or change
crates/terminal_ui/src/runtime.rs to read config.shell_integration_enabled as
the authoritative source; reference symbols: shell_integration_enabled,
TabTitleShellIntegration.enabled, TabTitle.shell_integration, and the runtime
file crates/terminal_ui/src/runtime.rs.
Comment on lines
+3861
to
+3864
| // Working directory (OSC 7) | ||
| TerminalEvent::WorkingDirectory(_path) => { | ||
| // Future use: store for tab title, etc. | ||
| } |
There was a problem hiding this comment.
Don’t discard OSC 7 working-directory events.
crates/terminal_ui/src/runtime.rs now emits TerminalEvent::WorkingDirectory, but this arm drops it. That leaves the new cwd propagation inert, so new tabs/panes still cannot reliably inherit the shell’s current directory unless some other signal happens to update it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/terminal_view/mod.rs` around lines 3861 - 3864, The
TerminalEvent::WorkingDirectory arm currently drops the path; instead capture
the path (e.g., TerminalEvent::WorkingDirectory(path)) and propagate it into the
view state: call the appropriate updater (for example PaneState::set_cwd,
Tab::set_working_directory, or Workspace::update_active_cwd) on the active
pane/tab so the new cwd is stored and the UI can inherit it for new tabs/panes;
also trigger any necessary UI refresh or event emission so other components
observe the change. Ensure the path is persisted in the same place other
components read when creating new tabs/panes.
- Shell: guard D marker on initial prompt, check BASH_SUBSHELL in DEBUG trap - Shell: URL-encode PWD in OSC 7, preserve existing zle-line-init widget - Windows: is_app_active now compares foreground window PID to current process - Runtime: use NATIVE_EVENT_LOOP_READ_WRITE_TOKEN for PTY registration - OSC: add 64KB max buffer to prevent unbounded memory growth - Store OSC 7 working directory in last_prompt_cwd for new tab inheritance - Clear command_start timestamp on command_finished - Clamp notification_min_duration to [0, 3600] in text input - Fix error message: "positive" -> "non-negative" for notification duration - Settings: add spacing between rows in settings groups
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.
Polish UI geometry and interactions across the app: introduce consistent rounded radii for settings, command palette, scrollbars and toasts; adjust search bar sizing and alpha; update toast timings and easing for smoother animation. Improve settings and sidebar text selection to handle double/triple clicks and avoid accidental drag-select. Add a cached sidebar search matcher to avoid repeated computation and rename "Theme Store" to "Themes" in UI. Native SDK: add Rename/Close tab items and IDs and wire handlers into the terminal view context menu. Pane resizing: increase divider hit margin, introduce PaneResizeResult enum, return rich results from hit/resize routines, track blocked state and hovered divider for cursor feedback, and throttle window-resize -> terminal resize to avoid SIGWINCH floods (with a scheduled follow-up). Other tweaks: grow/shrink-safe row cache resizing in the terminal grid, minor Dockerfile build helper, bump crate versions, and assorted render/shadow adjustments for toasts.
Summary by CodeRabbit
New Features
Style
Bug Fixes
Documentation