Repository navigation
perf: use keyed terminal lookup for named targets - #4669
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughTerminal IDs now support borrowed string lookups in maps. Terminal target matching and validation retrieve terminals directly by ID. Added tests cover resolution after pane and tab reordering, detached terminals, shared-name ambiguity, and lookup profiling. A justfile recipe runs the ignored profile test in release mode. Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The terminal lookup optimization is mergeable after normal checks; no actionable risk remains identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 2 files. (1 skipped: 1 unsupported.) 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 review |
✅ Action performedReview finished.
|
|
623fbbe to
41f3da9
Compare
JJLiebig
left a comment
There was a problem hiding this comment.
Rebased onto current master (21d71a03); final head is clean (Linux/macOS/Windows CI green).
The Borrow<str> impl is sound: TerminalId is a String newtype whose derived Hash/Eq delegate to str, so terminals.get(id.as_str()) resolves the same entry that was inserted as TerminalId. The three converted lookups keep target precedence, pane order, detached-terminal exclusion and ambiguity handling, which the new tests exercise.
Windows benchmark (x86_64-pc-windows-msvc, rustc 1.96.1, release, median of 7 samples, just bench-terminal-targets), before -> after:
| panes | terminal name | agent name |
|---|---|---|
| 1 | 0.384 -> 0.360 us | 0.285 -> 0.223 us |
| 15 | 6.999 -> 2.643 us (-62%) | 5.745 -> 1.501 us (-74%) |
| 128 | 346.4 -> 20.6 us (-94%) | 339.5 -> 12.7 us (-96%) |
| 512 | 4846.5 -> 88.7 us (-98%) | 4886.0 -> 49.2 us (-99%) |
Note on the Greptile summary: it flags agent-session restore / configured-shell reporting, but neither is in this PR's diff (3 files: justfile, src/app/terminal_targets.rs, src/terminal/id.rs). Those behaviors come from unrelated master commits that entered the compare range after the rebase; there are no inline findings on this change.
Approving.
41f3da9 to
ac6fd02
Compare
(cherry picked from commit 7945e91) ported from upstream 7945e91: the fork has no AppPolicy, so the new terminal_targets.rs test builds its App with the fork's App::new(&config, true, ...) signature. refs https://gitea.cat-bluegill.ts.net/AC-forks/herdr/issues/124
Merges 46 upstream commits spanning stable releases v0.9.2 and v0.9.3: multiple prefix keys (herdrdev#4653), self-reported agent resume commands (herdrdev#4687), Go To workspace-grouped navigator rows (herdrdev#4384), clear_pane keybinding (herdrdev#4383), non-interactive machine status/reconnect with SSH MFA (herdrdev#3763), interactive machine add session picker (herdrdev#4204, herdrdev#4678), mouse events on terminal session control (herdrdev#4685), staggered agent restarts (herdrdev#4102), SSH compression (herdrdev#4340), row-shift scrolling (herdrdev#4711), TERM_PROGRAM identity (herdrdev#4104), PowerShell 7 default on Windows (herdrdev#4297), tab-close confirmation (herdrdev#4379, herdrdev#4409), full event subscription bursts (herdrdev#4178, herdrdev#4225), SIGWINCH color refresh (herdrdev#4349), saved-layout shutdown survival (herdrdev#4320), stray-Escape click fix (herdrdev#3480), first-task-done detection (herdrdev#3338, herdrdev#3990, herdrdev#3916), CPU/perf work across navigator/splits/named-targets/event-batching (herdrdev#4506, herdrdev#4546, herdrdev#4426, herdrdev#4669, herdrdev#4670, herdrdev#4671, herdrdev#4672), synchronized-frame tearing and cursor flicker fixes (herdrdev#2968, herdrdev#4303), API socket transient-error resilience (herdrdev#4601), async worktree git lookups (herdrdev#4492), worktree-open workspace-takeover fix (herdrdev#4293), SSH agent forwarding after reconnect (herdrdev#1931, herdrdev#4252), OpenCode V1 subagent status (herdrdev#1362, herdrdev#4240), Codex idle/mention/interrupt-key detection (herdrdev#4507, herdrdev#4196), Kiro/Grok detection fixes (herdrdev#4203, herdrdev#4372, herdrdev#4333), Droid scrollback clears (herdrdev#4432), copy-mode-stays-active fix (herdrdev#3812, herdrdev#4281), Ctrl+Shift+letter fix (herdrdev#4581, herdrdev#4597), split mouse-report fix (herdrdev#4630), several Windows paste/mouse-capture/clipboard/detection fixes (herdrdev#4251, herdrdev#4284, herdrdev#4470, herdrdev#4314, herdrdev#4313, herdrdev#4579, herdrdev#4309), navigator search/highlight fixes (herdrdev#4300, herdrdev#4408, herdrdev#4273, herdrdev#4535, herdrdev#4355), session-attach-without-terminal error (herdrdev#4393), socket error request-ID preservation (herdrdev#4344), plugin install option ordering (herdrdev#4446), Windows host color detection (herdrdev#1530, herdrdev#4369), post-update saved-machine summary (herdrdev#4722), a hotfix for Escape-prefixed terminal key bindings (herdrdev#4759) and pinned Windows ConPTY bundling (herdrdev#4760), Windows same-user cross-elevation attach (herdrdev#4777), and the libghostty-vt binding extraction into a ghostty-vt workspace crate (herdrdev#4661). Also fixes two merge seam breaks: overlays.rs combines the fork's rounded_borders threading with upstream's per-client prefix-label onboarding overlay, and the fork's command_palette test file is updated for the new Vec<KeyCombo> prefix representation and the BindingConfig::one constructor introduced by the multi-prefix-key feature. refs upstream herdrdev/herdr@331775c3
Preserve the existing fork on an isolated migration branch while importing three independently verified v0.9.2 changes. Adapt the API fairness fixtures to retain the existing stream_active contract. Upstream sources: 7945e91, 6502492, c210c95. Refs herdrdev#4669, herdrdev#4670, herdrdev#4671 Co-authored-by: Eric Yue <jiacheng.yue@foxmail.com>
Named terminal and agent lookup scans the terminal registry for every pane candidate. Use borrowed string keys in the existing HashMap to reduce that join from O(P×T) to expected O(P), preserving target precedence, pane order, detached-terminal exclusion and ambiguity handling.
Release builds on Apple M3 / 24 GiB, macOS 27.0, Rust 1.96.1; medians of three serial runs with alternating baseline/candidate order, compared with upstream
fff6c820. Time reduction =(before − after) / before × 100%.Reproduce:
just bench-terminal-targets. Tests cover borrowed-key lookup after serialization and adversarial pane/tab identities. Nativejust ci,just docs-contract-test, and GitHub Linux/macOS/Windows CI passed.