From dc2235fa9c0271aa3f111df2d804c9a463d8097f Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 14 Jul 2026 17:16:12 +0000 Subject: [PATCH] fix(tui): preserved plan review scroll position Retained relative body progress across transient non-scrollable render frames and added regression coverage for terminal-height changes. Fixes #5232 --- packages/coding-agent/CHANGELOG.md | 4 ++ .../modes/components/plan-review-overlay.ts | 59 +++++++++++++++---- .../components/plan-review-overlay.test.ts | 32 ++++++++++ 3 files changed, 85 insertions(+), 10 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 30fc7c72b..84abe32ea 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed fullscreen Plan Review jumping to the top while scrolling when a transient terminal resize or Markdown reflow made the body temporarily non-scrollable. ([#5232](https://github.com/can1357/oh-my-pi/issues/5232)) + ## [16.4.5] - 2026-07-11 ### Breaking Changes diff --git a/packages/coding-agent/src/modes/components/plan-review-overlay.ts b/packages/coding-agent/src/modes/components/plan-review-overlay.ts index 11f7a96d2..7be362d6c 100644 --- a/packages/coding-agent/src/modes/components/plan-review-overlay.ts +++ b/packages/coding-agent/src/modes/components/plan-review-overlay.ts @@ -136,6 +136,8 @@ export class PlanReviewOverlay implements Component { #tocCursor = 0; #sidebarShown = false; #pendingScrollToToc = false; + /** Last meaningful relative body position, retained while a frame cannot scroll. */ + #scrollProgress = 0; // Click hit-testing, rebuilt every render. Keys are 0-based rendered-line // indices (== screen rows, since the fullscreen overlay paints from row 0). @@ -194,6 +196,7 @@ export class PlanReviewOverlay implements Component { setPlanContent(planContent: string): void { this.#setSections(planContent); this.#scrollView.scrollToTop(); + this.#scrollProgress = 0; this.#tocCursor = 0; // A wholesale external-editor swap supersedes prior in-overlay deletions. this.#deleted = []; @@ -337,6 +340,7 @@ export class PlanReviewOverlay implements Component { if (event.wheel !== null) { // Scroll wheel: three rows per notch. this.#scrollView.scroll(event.wheel * 3); + this.#captureScrollProgress(); return true; } if (event.release) return true; @@ -439,13 +443,21 @@ export class PlanReviewOverlay implements Component { // drops into the actions ("next step"); scrolling off the top steps back up // to the ToC. if (matchesSelectUp(data) || data === "k") { - if (this.#scrollView.getScrollOffset() <= 0 && this.#sidebarShown) this.#setFocus("toc"); - else this.#scrollView.scroll(-1); + if (this.#scrollView.getScrollOffset() <= 0 && this.#sidebarShown) { + this.#setFocus("toc"); + } else { + this.#scrollView.scroll(-1); + this.#captureScrollProgress(); + } return; } if (matchesSelectDown(data) || data === "j") { - if (this.#scrollView.getScrollOffset() >= this.#scrollView.getMaxScrollOffset()) this.#setFocus("actions"); - else this.#scrollView.scroll(1); + if (this.#scrollView.getScrollOffset() >= this.#scrollView.getMaxScrollOffset()) { + this.#setFocus("actions"); + } else { + this.#scrollView.scroll(1); + this.#captureScrollProgress(); + } return; } this.#handleBodyScroll(data); @@ -458,9 +470,17 @@ export class PlanReviewOverlay implements Component { * before this runs, so here it only ever sees the paging/fast keys. */ #handleBodyScroll(data: string): void { - if (this.#scrollView.handleScrollKey(data)) return; - if (data === "g") this.#scrollView.scrollToTop(); - else if (data === "G") this.#scrollView.scrollToBottom(); + if (this.#scrollView.handleScrollKey(data)) { + this.#captureScrollProgress(); + return; + } + if (data === "g") { + this.#scrollView.scrollToTop(); + this.#scrollProgress = 0; + } else if (data === "G") { + this.#scrollView.scrollToBottom(); + this.#scrollProgress = 1; + } } #handleToc(data: string): void { @@ -511,7 +531,10 @@ export class PlanReviewOverlay implements Component { const sectionIndex = this.#toc[this.#tocCursor]; if (sectionIndex === undefined) return; const offset = this.#sectionOffsets[sectionIndex]; - if (offset !== undefined) this.#scrollView.setScrollOffset(offset); + if (offset !== undefined) { + this.#scrollView.setScrollOffset(offset); + this.#captureScrollProgress(); + } } /** Greatest ToC position whose section starts at or above the scroll offset. */ @@ -683,6 +706,23 @@ export class PlanReviewOverlay implements Component { return parts.join(sep); } + /** + * Retain relative progress across reflow frames. A non-scrollable intermediate + * frame has no meaningful offset, so it must not erase the last scroll position. + */ + #captureScrollProgress(): void { + const maxOffset = this.#scrollView.getMaxScrollOffset(); + if (maxOffset > 0) this.#scrollProgress = this.#scrollView.getScrollOffset() / maxOffset; + } + + #layoutBody(lines: readonly string[], height: number): void { + this.#captureScrollProgress(); + this.#scrollView.setLines(lines); + this.#scrollView.setHeight(height); + const maxOffset = this.#scrollView.getMaxScrollOffset(); + if (maxOffset > 0) this.#scrollView.setScrollOffset(Math.round(this.#scrollProgress * maxOffset)); + } + /** Build the concatenated body lines and record each section's start row. */ #buildBody(bodyContentWidth: number): string[] { const lines: string[] = []; @@ -800,8 +840,7 @@ export class PlanReviewOverlay implements Component { const regionRows = Math.max(MIN_BODY_ROWS, termHeight - chrome); const bodyLines = this.#buildBody(bodyContentWidth); - this.#scrollView.setLines(bodyLines); - this.#scrollView.setHeight(regionRows); + this.#layoutBody(bodyLines, regionRows); if (this.#pendingScrollToToc) { this.#pendingScrollToToc = false; this.#scrubBodyToToc(); diff --git a/packages/coding-agent/test/modes/components/plan-review-overlay.test.ts b/packages/coding-agent/test/modes/components/plan-review-overlay.test.ts index ef9841fd4..9d19e9d2f 100644 --- a/packages/coding-agent/test/modes/components/plan-review-overlay.test.ts +++ b/packages/coding-agent/test/modes/components/plan-review-overlay.test.ts @@ -180,6 +180,38 @@ describe("PlanReviewOverlay", () => { expect(backToTop).not.toContain("para 199"); }); + it("preserves scroll progress across a transient non-scrollable render", () => { + const originalRows = Object.getOwnPropertyDescriptor(process.stdout, "rows"); + const setRows = (rows: number): void => { + Object.defineProperty(process.stdout, "rows", { configurable: true, value: rows }); + }; + const codeRows = Array.from({ length: 400 }, (_, i) => `L${String(i).padStart(3, "0")}`).join("\n"); + const overlay = new PlanReviewOverlay( + `# Plan\n\n\`\`\`\n${codeRows}\n\`\`\`\n`, + { promptTitle: "next", options: APPROVAL_OPTIONS }, + { onPick: vi.fn(), onCancel: vi.fn() }, + ); + + try { + setRows(40); + render(overlay); + overlay.handleInput("G"); + const bottom = render(overlay); + expect(bottom).toContain("L399"); + expect(bottom).not.toContain("L000"); + + setRows(1000); + render(overlay); + setRows(40); + const restored = render(overlay); + expect(restored).toContain("L399"); + expect(restored).not.toContain("L000"); + } finally { + if (originalRows) Object.defineProperty(process.stdout, "rows", originalRows); + else Reflect.deleteProperty(process.stdout, "rows"); + } + }); + it("swaps the displayed plan and resets scroll on setPlanContent", () => { const longPlan = Array.from({ length: 200 }, (_, i) => `para ${i}`).join("\n\n"); const overlay = new PlanReviewOverlay(