fix: honor droid scrollback clear requests in panes - #4432
Conversation
Remove the droid-specific filter that discarded ED3 before PTY output reached the terminal parser. Keeping stale scrollback beneath replayed primary-screen frames duplicated the welcome header and old transcript. Pass the original bytes to the parser and trackers. Add process-aware regression coverage across PTY chunk boundaries and preserve ED2/ED3 screen-history separation and alternate-screen isolation. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with 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)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change removes Droid-specific scrollback-clear filtering from OSC handling and PTY processing. Scrollback-clear sequences now pass through unchanged, and terminal trackers inspect the original PTY bytes. Migration tests can provide a foreground process ID and cover ED3 behavior across complete, bytewise, and split writes, including primary-screen, alternate-screen, ED2, and ED3 cases. Suggested reviewers: Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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.
|
|
JJLiebig
left a comment
There was a problem hiding this comment.
Approved, tested the current master binary against this PR head with a fresh Droid session. Droid’s /clear leaves one welcome header, removes stale transcript and shell rows, and resets scrollback to zero. The model and Herdr lifecycle hook continued working normally.
Only note: I tested this with a the cli-proxy-api since I don't have a factory sub. But I see no reason why this should change the outcome.
Ty! Yup, no difference in TUI whether you use direct or byok |
Merged upstream/master b88e811..2836010. Zero conflicts: all six overlapping files split into disjoint regions (mouse.rs navigator scrollbar/row-click work, state.rs navigator fields and close-tab confirmation, config/model.rs clear_pane keybinding, config-reference.json keys.clear_pane, AGENTS.md detection policy, tests/mod.rs mod close_tab). No src/protocol/wire.rs change, so no protocol conflict. Notable upstream fixes absorbed: - distinguish agent completion from startup and session changes (herdrdev#4457) - honor droid scrollback clear requests in panes (herdrdev#4432) - refresh forwarded ssh agents after reconnect (herdrdev#4443) - index navigator tabs and panes per endpoint (herdrdev#4426) - preserve session layouts across shutdown and restore failures (herdrdev#4400) - confirm closing the last tab in the tui (herdrdev#4409) - reject terminal-less attach before starting a session (herdrdev#4395) - drain event subscriptions and report history loss (herdrdev#4225) - preserve explicit worktree workspace membership (herdrdev#4301) - keep workspace navigation visible in terminal theme (herdrdev#4408) - preserve delayed mouse reports with confirmed keyboard input (herdrdev#4247) - detect codex status with custom interrupt keys (herdrdev#4196) - detect kiro status from live controls and osc signals (herdrdev#4372) - recover the cursor during animated redraws (herdrdev#4404) - stabilize windows cursor redraws and settle deadlines (herdrdev#4389) - show every agent and terminal in the go to picker (herdrdev#4384) - add configurable pane screen and scrollback clearing (herdrdev#4383) - reveal selected agent when cycling the sidebar (herdrdev#4355) - preserve request ids in socket error responses (herdrdev#4353) - enable ssh compression for remote connections (herdrdev#4340) - correct grok activity detection with custom or disabled osc signals (herdrdev#4337) - preserve windows mouse capture during refresh (herdrdev#4319)
Merges 6 upstream commits: ssh-agent identity clearing on session reconnect (herdrdev#4461, herdrdev#4443), agent-completion vs startup/session-change detection (herdrdev#4457), droid scrollback clear requests (herdrdev#4432), navigator tab/pane indexing per endpoint (herdrdev#4426), and a contributor approval. refs upstream herdrdev/herdr@8ac95427
Description
What: Honor Droid's scrollback-clear requests so replaying a primary-screen transcript does not retain duplicate welcome headers and stale rows. The process-aware regression fails on current
masterwith two headers and passes with this change.Why: Herdr's Droid-specific filter strips
ESC[3JandESC[?3Jbefore they reach the terminal parser. Clearing the visible screen still works, but the old scrollback survives beneath the replayed content. Filtering also depends on where PTY chunks split the escape sequence.How: Remove the filter and pass the original PTY bytes to the existing parser and trackers. The vendored parser already implements the screen/history boundaries; no new rendering logic is needed.
Related Issue
No linked GitHub issue.
Reviewer Guide
src/pane/osc.rs: removes the process-specific filter and tests that enforced suppression.src/pane/terminal.rs: passes unmodified bytes through the existing write path, eliminating the extra foreground-process lookup for ED3.src/pane/terminal/migration_tests.rs: extends the existing harness with a real foreground process identifying asdroid; checks replay across whole writes, bytewise writes, seven-byte chunks, and every split within the erase prefix.Risk & Impact
Droid can now clear primary-screen history when it explicitly requests ED3, rather than retaining that history. ED2 still preserves scrollback, ED3 preserves the visible screen, and alternate-screen clearing leaves the saved primary screen intact. Manual clear-pane handling is unchanged. No dependencies, persistence, or protocol changes.
Verification
Behavior verified @
0fe2b62b: two 55-line frames replayed into a 24-row terminal leave one welcome header and no old-frame rows, both at the bottom and when scrolled up.primary_screen_replay_honors_ed3_for_droid_at_chunk_boundaries, observed failing on original production code and passing after the fix.erase_display_preserves_screen_and_history_boundariespasses for both clear sequences.just ci 'test(pane::terminal::)'passed: formatting, Clippy on all targets, 148 pane-terminal tests, 144 maintenance tests, six architecture tests, and 44 Bun tests. Commit hooks also passed.