Skip to content

fix: DH-22991: Command history is disappearing - #2720

Merged
mofojed merged 5 commits into
deephaven:mainfrom
SimonVutov:DH-22991-Command-history-scroll-issues
Jul 22, 2026
Merged

fix: DH-22991: Command history is disappearing#2720
mofojed merged 5 commits into
deephaven:mainfrom
SimonVutov:DH-22991-Command-history-scroll-issues

Conversation

@SimonVutov

Copy link
Copy Markdown
Contributor

Problem

Removing a sibling panel next to Command History caused it to go blank and lose its scroll position. Golden Layout re-parents the Command History DOM node (via replaceChild), which resets the scroll container to the top while react-window keeps its internal offset — so no items render in the viewport.

Fix

ItemList now re-asserts its scroll position when it's resized (the reliable in-component signal for the reparent):

  • restoreScrollPosition() is now purely geometric — it clamps the last scroll offset to the current bounds (clamp(scrollOffset, 0, itemCount * rowHeight - height)). This restores the correct position and, for a bottom-anchored list moving into a smaller/larger panel, keeps it at the new bottom without any special-casing.
  • handleResize() calls restoreScrollPosition() after applying the new height.

No Golden Layout changes.

Test

Adds an e2e test (tests/command-history.spec.ts) that scrolls Command History to the middle, removes a sibling panel, and asserts the scroll position and rendered items are preserved.

@SimonVutov
SimonVutov requested a review from mofojed July 16, 2026 18:35
@SimonVutov SimonVutov self-assigned this Jul 16, 2026
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.39%. Comparing base (05c795d) to head (8d0439a).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
packages/components/src/ItemList.tsx 0.00% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2720      +/-   ##
==========================================
- Coverage   51.40%   51.39%   -0.02%     
==========================================
  Files         796      796              
  Lines       45720    45738      +18     
  Branches    11665    11864     +199     
==========================================
+ Hits        23504    23507       +3     
- Misses      22197    22212      +15     
  Partials       19       19              
Flag Coverage Δ
unit 51.39% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines +348 to +358
// Re-parenting the panel (e.g. removing a sibling panel) resets the scroll
// container to the top while react-window keeps its internal offset, which
// leaves the list blank. Re-assert the scroll position purely from the
// current geometry: clamp the last real offset to the current bounds. On a
// grow this naturally keeps a bottom-anchored list at the (new) bottom, so
// restore never needs to reason about sticky state.
const { itemCount, rowHeight } = this.props;
const { height } = this.state;
const maxOffset = Math.max(0, itemCount * rowHeight - (height ?? 0));
const newOffset = Math.min(Math.max(scrollOffset, 0), maxOffset);
container.scrollTo(0, newOffset);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should be checking the isStuckToBottom and if that's true, scroll to maxOffset. Otherwise we can just scroll to the newOffset like we do here.

@SimonVutov
SimonVutov requested a review from mofojed July 17, 2026 13:20

@mofojed mofojed left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work

@mofojed
mofojed merged commit 8088377 into deephaven:main Jul 22, 2026
11 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 22, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants