Skip to content

fix: preserve Pi scroll position across resizes - #2086

Closed
drgnchan wants to merge 1 commit into
herdrdev:masterfrom
drgnchan:fix/2082-pi-resize-scroll-position
Closed

drgnchan wants to merge 1 commit into
herdrdev:masterfrom
drgnchan:fix/2082-pi-resize-scroll-position

Conversation

@drgnchan

Copy link
Copy Markdown

Summary

  • preserve a scrolled Pi viewport while Pi rebuilds its primary-screen transcript after a resize
  • allow Pi's scrollback clear to replace the old transcript instead of accumulating duplicate redraws
  • restore the saved offset only after synchronized output completes, including redraws split across PTY reads and overlapping resizes
  • keep the existing Droid-specific scrollback-clear filtering behavior scoped to Droid

Testing

  • just ci
  • live Pi 0.83 reproduction in an isolated Herdr session: five 35/53-column round trips kept offset_from_bottom at 232, with stable scrollback size and visible-content hash for each width

Refs #2082

No public documentation update is needed; this restores the expected resize behavior.

@kangal-bot kangal-bot added coderabbit-review greptile-review Trigger Greptile review for contributor-approved pull requests labels Jul 30, 2026
@coderabbitai

coderabbitai Bot commented Jul 30, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Scrollback compatibility

Layer / File(s) Summary
Agent detection and compatibility filtering
src/pane/osc.rs
Foreground jobs are classified through identify_agent_in_job; Droid-only scrollback filtering and Pi viewport-preservation predicates are tested.
Primary-screen viewport restoration
src/pane/terminal.rs
Primary-screen rebuilds save, preserve through resize and synchronized output, then restore the scroll offset; repeated rebuild behavior is tested.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PTY
  participant GhosttyPaneTerminal
  participant ScrollbackPredicate
  PTY->>GhosttyPaneTerminal: Process primary-screen scrollback-clear bytes
  GhosttyPaneTerminal->>ScrollbackPredicate: Check foreground agent and clear sequence
  ScrollbackPredicate-->>GhosttyPaneTerminal: Preserve viewport decision
  GhosttyPaneTerminal->>GhosttyPaneTerminal: Save offset during rebuild
  GhosttyPaneTerminal->>GhosttyPaneTerminal: Restore offset after rebuild
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately summarizes the main change: preserving Pi scroll position across resizes.
Description check ✅ Passed The description is clearly related to the Pi resize and scrollback preservation changes in the pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

Comment thread src/pane/terminal.rs
Comment on lines 1438 to +1440
let offset_from_bottom = core
.terminal
.scrollbar()
.ok()
.map(|scrollbar| {
scrollbar
.total
.saturating_sub(scrollbar.offset + scrollbar.len)
})
.unwrap_or(0);
.primary_scrollback_redraw_offset_from_bottom
.unwrap_or_else(|| ghostty_scroll_offset_from_bottom(&core.terminal));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Resize ends redraw tracking early

When a resize overlaps Pi's synchronized redraw, terminal.resize() disables synchronized-output mode while the saved redraw offset remains set. The next partial PTY read therefore consumes and restores that offset before the transcript rebuild completes, causing an intermediate viewport jump and render instead of waiting for Pi's closing synchronized-output sequence.

@greptile-apps

greptile-apps Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Preserves Pi’s primary-screen viewport while synchronized transcript redraws replace scrollback after terminal resizes.

  • Uses shared foreground-agent detection while retaining Droid-only scrollback-clear filtering.
  • Saves Pi’s pre-redraw offset and restores it after synchronized output completes.
  • Reuses the saved offset during overlapping resizes and adds regression coverage for split and repeated redraws.

Confidence Score: 4/5

The overlapping-resize state transition needs to be fixed before merging because it can complete viewport restoration before Pi finishes rebuilding its transcript.

The saved offset survives terminal.resize(), while that resize disables synchronized-output mode; subsequent partial output then satisfies the restoration condition before Pi emits its synchronized-output terminator.

Files Needing Attention: src/pane/terminal.rs

Important Files Changed

Filename Overview
src/pane/osc.rs Centralizes foreground-agent identification, keeps ED3 filtering scoped to Droid, and identifies Pi redraws that require viewport preservation.
src/pane/terminal.rs Tracks Pi’s pre-redraw offset across PTY reads, but an overlapping resize can disable synchronized output and trigger restoration before the redraw completes.

Sequence Diagram

sequenceDiagram
    participant Pi
    participant Terminal as Herdr terminal
    participant Ghostty
    Pi->>Terminal: Begin synchronized output + ED3
    Terminal->>Terminal: Save offset from bottom
    Terminal->>Ghostty: Apply ED3 and transcript redraw
    opt Resize overlaps redraw
        Terminal->>Ghostty: Resize using saved offset
    end
    Pi->>Terminal: End synchronized output
    Terminal->>Ghostty: Restore saved viewport offset
    Terminal->>Terminal: Request render
Loading

Reviews (1): Last reviewed commit: "fix: preserve Pi scroll position across ..." | Re-trigger Greptile

@ogulcancelik

Copy link
Copy Markdown
Collaborator

please see my comment on your issue

#2082 (comment)

i don't want to bring flaky detection if upstream pi gonan fix it.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/pane/terminal.rs (1)

1113-1147: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Hoist foreground_job() out of the core lock foreground_job() can snapshot the process table / walk process groups, so holding self.core across this lookup can block render() and resize handling. Snapshot the needed state, drop the lock, then query foreground_job(), and reacquire the lock for the rest of the byte handling.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 082735bc-c033-460b-8367-5bbfa5dfd0d6

📥 Commits

Reviewing files that changed from the base of the PR and between 5319927 and 3517ba3.

📒 Files selected for processing (2)
  • src/pane/osc.rs
  • src/pane/terminal.rs

@drgnchan

Copy link
Copy Markdown
Author

Understood, thanks for the context. I’ll keep this as a local compatibility patch for now and revisit it after Pi releases the experimental alternate-screen renderer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coderabbit-review greptile-review Trigger Greptile review for contributor-approved pull requests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants