Skip to content

fix(web): preserve diff scroll and collapsed files across tabs - #8826

Closed
gshahbazian wants to merge 7 commits into
pingdotgg:mainfrom
gshahbazian:fix/web-diff-collapse-tab-switch
Closed

gshahbazian wants to merge 7 commits into
pingdotgg:mainfrom
gshahbazian:fix/web-diff-collapse-tab-switch

Conversation

@gshahbazian

@gshahbazian gshahbazian commented Aug 30, 2026 •

Copy link
Copy Markdown

Human review required

Switching from Diff to another right-panel tab discards collapsed files and scroll position. Keep both for the session, scoped to the environment, thread, and review section.

This retains Gabe Shahbazian's original collapse-state implementation and history, updates it for current main, and adds the scroll-restoration portion of #8759. The original revision intentionally left scroll restoration out.

Collapse, expand-all, and file-tree reveals now share the retained state. Scroll events capture the current file and its relative offset in a ref; leaving the panel saves that anchor. Remount restores through Pierre's item-target API, using the first known file if the original file disappeared. Its registered metadata is available before painting, so delayed row rendering does not lose restoration. A new explicit file reveal takes precedence over a saved position. Successful local thread deletion and environment removal clear the state, including when deletion precedes panel unmount.

No new dependency, timer, layout, shared contract, or reload persistence. This changes web and the desktop web UI. It does not change mobile or retain state for other right-panel tabs. Remote thread deletions observed outside the existing local delete actions are not newly wired to this store.

Verification

Baseline 84b99f3, September 5, 2026 at 02:45:14 UTC. Candidate 0b7f438 includes main b01771c, September 5 at 03:31:16 UTC.

  • Real Chromium baseline reproduces both failures: collapse a file, scroll to 1300, mount Files, then return to Diff. The file expands and scroll resets to 0. Merely switching back before Files mounts is not a valid control.
  • 44 focused tests pass across the diff store, viewport lifecycle, collapse, rendering, and shared CodeView tests. They cover scope isolation, actual React unmount/remount, scroll-to-top retention, delayed viewer readiness, new reveal precedence, tree reveals, removed-anchor fallback across repeated remounts, same-instance delayed rendering and a first file becoming available, and deletion-before-unmount cleanup. The lifecycle fixture models Pierre's public scroll API, not browser geometry. A separate real-CodeView test verifies that item metadata is registered before any rows render.
  • The original PR's deleted-thread collapse-state regression failed before integration and passes after the fix. Permanent tests cover explicit selections and session-only state.
  • Web typecheck passes. Targeted lint passes with two unchanged ref-during-render warnings in DiffPanel and useThreadActions.
  • The first candidate retained collapse but still drifted from 1300 to 1308 in a settled browser roundtrip. A corrected public-API fixture then failed five lifecycle controls against that candidate. The item-anchor correction passes those controls without a padding constant or timer. The review also caught a delayed-first-render gap, reproduced as 0 instead of 1300 on the same viewer instance and now fixed. Actual current-candidate repeated-unmount browser verification and public before/after evidence are recorded below. The human approval hold remains.

Integrated current-client evidence

The orchestrator tested the exact production changes from 0b7f438 on main b01771c2, in Linux Chromium 152 on September 4, 2026 (PDT).

Two full Diff → source-file → Diff unmount/remount cycles restored exactly 1300 → 1300, with the settled scroll height unchanged at 10136. The four collapsed file headers at the top remained collapsed, while the expanded file remained expanded. Selecting composerInlineTokens.ts in the file tree afterward expanded and revealed that file at scroll 10692/header y=98, taking precedence over the saved position.

These are actual client interactions, not a static-render test. The screenshots contain only owned fixtures and public source. The before and after diffs have different contents because they show the audit's own integration patches; no claim is made that the file list is identical. Native desktop/mobile input and remote-initiated deletion are not verified by these recordings.

Before — at the chosen position:

Before: collapsed-file review scrolled to 1300

Before — returning to Diff resets to the top and expands the file:

Before: tab roundtrip loses scroll and collapse state

Before recording:

https://gh-file-drop-api-prod-mi5fy3sowv63ufte.pinglabs.workers.dev/f/c6786f7af8af632a/8759-before-repro.webm

After — before leaving the tab:

After candidate: review at scroll 1300 before switching tabs

After — returning restores the same position:

After candidate: tab roundtrip restores the same review position

After — retained folds, inspected at the top:

After candidate: collapsed file headers remain collapsed

After recording:

https://gh-file-drop-api-prod-mi5fy3sowv63ufte.pinglabs.workers.dev/f/1d8b23a84d515e10/8759-final-after-roundtrip.webm

Orchestrator decision: human approval required

The root reviewed the whole eight-file change and the subsequent scroll/API/readiness corrections. The integrated Linux evidence gate now passes, but this cumulative candidate goes beyond the original collapse-only patch: it adds viewport/session state and cleanup across thread and environment lifecycles. I am leaving that scope for the maintainer's final decision.

Current-head executed CI passes. The two source-backed review findings are fixed and resolved. Macroscope's latest eligibility comment requires human review; its checks have not attached to this refreshed head, and the dismissed older approval is not current approval. Final Bugbot completion is tracked separately. This is not an all-green merge claim. No merge, auto-merge or issue closure is authorized while the hold remains.

Original author's collapse-only recording

Watch the original after recording. This records the original revision, not the new scroll-restoration candidate.

Original contribution by Gabe Shahbazian. Current-main integration, scroll restoration, and verification by GPT 6 Astra via Codex in T3 Code.


Note

Medium Risk
Touches diff panel UX state, scroll restoration timing with Pierre’s CodeView, and store cleanup on thread/environment removal; regressions would show as wrong scroll or collapse after tab switches or deletes.

Overview
Switching away from the Diff right-panel tab no longer resets collapsed files or scroll position. Both are kept for the session, keyed by environment, thread, and review section (working tree, branch, or turn).

Collapsed files move from local DiffPanel state into session-only maps on diffPanelStore (not written to localStorage). Collapse, expand-all, and tree reveals read and update that shared state.

Scroll is handled by a new useDiffPanelViewport hook wired through optional onScroll on AnnotatableCodeView. Scroll updates stay in a ref; the store is updated when the scope unmounts so Pierre’s reset-on-teardown doesn’t lose position. Remount restores via item-relative anchors (with fallbacks when files disappear or rendering is delayed), while new explicit file reveals still win over a saved position.

Thread deletion and environment teardown call removeThread / removeEnvironment so stale scope data isn’t resurrected on later unmount cleanup.

Reviewed by Cursor Bugbot for commit 0b7f438. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Preserve diff scroll position and collapsed files across tabs

  • Adds useDiffPanelViewport hook in useDiffPanelViewport.ts that captures scroll position and nearest file anchor in memory per review scope, restoring it once per viewer mount/remount without per-scroll store writes
  • Moves collapsed-file state from local DiffPanel component state into session-only, scope-keyed maps in diffPanelStore, so selections persist across tab switches but are not persisted to disk
  • Forwards scroll events from AnnotatableCodeView through an optional onScroll prop so the hook can capture viewport position
  • Cleans up transient diff-panel state (selections, collapsed files, viewport captures) when a thread or environment is deleted in useThreadActions.ts and platform.ts\n- Risk: diffPanelStore now removes all scope-prefixed entries on thread/environment deletion; any out-of-tree consumers reading the old local collapsed-file state from DiffPanel will break since that local state is removed

Macroscope summarized 0b7f438.

The Diff panel stored collapsed files in component state, so opening a
file tab unmounted it and expanded everything again. Keep the collapsed
keys in the existing panel store for the session.
@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: db40bb76-e2f5-4cb5-a060-03c89023650f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Warning

Your free Security trial is over. An organization admin can activate Security or dismiss this notice.


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

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Aug 30, 2026
@macroscopeapp

macroscopeapp Bot commented Aug 30, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — The PR changes production diff-panel behavior across tab remounts, viewer rendering timing, and thread/environment deletion, using new shared session state and a viewport lifecycle hook. Focused tests cover the main flows, but the cross-file cleanup and anchor-restoration interactions are sufficiently nontrivial to warrant human review.

You can add or adjust custom eligibility rules. Learn more.

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Aug 30, 2026
@davidvornholt

Copy link
Copy Markdown

Confirming this is still reproducible on current main: collapse files in Diff, switch to another right-panel tab, then return to Diff and the files are expanded again. This is particularly disruptive when reviewing larger diffs! It would be great to get this merged.

@juliusmarminge juliusmarminge changed the title fix(web): keep collapsed diff files when switching tabs fix(web): preserve diff scroll and collapsed files across tabs Sep 5, 2026
@macroscopeapp
macroscopeapp Bot dismissed their stale review September 5, 2026 03:05

Dismissing prior approval to re-evaluate e945acd

@github-actions github-actions Bot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Sep 5, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 5, 2026
@macroscopeapp
macroscopeapp Bot dismissed their stale review September 5, 2026 03:21

Dismissing prior approval to re-evaluate 9fafc4e

Comment thread apps/web/src/components/diffs/useDiffPanelViewport.ts Outdated

@cursor cursor Bot left a comment

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.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4ac665a. Configure here.

Comment thread apps/web/src/components/diffs/useDiffPanelViewport.ts
@juliusmarminge

Copy link
Copy Markdown
Member

Root verified 0b7f438 on main b01771c2: two actual full tab-unmount roundtrips restore 1300 to 1300 with unchanged settled height, retain collapsed file headers, and allow a new file-tree selection to override the saved position. Public before/after screenshots and recording are attached. The whole eight-file cumulative change and review corrections have been audited. I am holding this for human approval because the cumulative viewport/session state and cross-thread/environment cleanup scope is broader than the original collapse-only patch. Current-head executed CI passes, but the older dismissed Macroscope approval is not current approval. No merge, auto-merge or issue closure.

GPT 6 Astra via Codex in T3 Code.

@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by claude-opus-5-5 on behalf of Maria

Hi! We are cleaning up open PRs, and this one names a harness (Codex) but not which model version created it. If this change is really important, we recommend rebuilding the PR with a newer model and noting the model in the PR description.

@maria-rcks maria-rcks closed this Oct 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants