DG2: consolidate virtual scroll height lock (WC-3333, WC-3390)

[yordan-st]

Pull request type

Bug fix and refactor

---

Description

Two virtual scroll bugs in GridSizeStore shared the same root cause — incomplete height cache invalidation and incorrect overflow detection. This PR consolidates both fixes instead of band-aiding each symptom separately.

What changed:

• Replaced parallel lockedAtPageSize / lockedAtColumnCount guard fields with a single layoutKey private getter (pageSize-columnCount). One place to invalidate, one place to extend.
• Height is no longer cleared to undefined on invalidation — the old value stays until overwritten. Prevents CSS snap-to-full-height flicker and eliminates false scroll-bottom triggers during column toggle.
• Fixed overflow detection: gridContainer.clientHeight < fullHeight instead of scrollHeight > fullHeight. scrollHeight was inflated by horizontal overflow on many-column grids, causing the synthetic 30px scroll offset to be skipped.
• Guarded bumpPage in scroll handler behind scrollTop > 0 — prevents page loading on content-resize events when the user is at the top.

Closes #2167, closes #2193.

---

What should be covered while testing?

Bug 1 — WC-3333: Scrollbar disappears after hiding a column

1. Open a page with Data Grid, virtual scrolling enabled, enough rows to produce a scrollbar. Include at least one column with wide/wrapping text.
2. Confirm vertical scrollbar is visible.
3. Open column selector, hide the wide text column.
4. Should: scrollbar stays visible, row count unchanged.
5. Should not: scrollbar disappears, or extra rows load without scrolling.
6. Show the column again — scrollbar should adjust correctly.

Bug 2 — WC-3390: Many columns, only first page loads

1. Open a page with Data Grid, virtual scrolling enabled, 50+ row dataset, enough columns to require horizontal scrolling.
2. On load, confirm ~pageSize rows are visible and a vertical scrollbar is present.
3. Scroll down — more rows should load.
4. Should not: only 4–5 rows visible with no vertical scrollbar, loading blocked until zoom or horizontal scroll.

[github-actions]

AI Code Review

🔶 Changes requested — one or more medium-severity items must be addressed

---

What was reviewed

File	Change
packages/pluggableWidgets/datagrid-web/src/model/stores/GridSize.store.ts	Replaces lockedAtPageSize guard with lockedAtLayoutKey (page size + column count); fixes overflow detection; retains old height on invalidation instead of clearing to undefined
packages/pluggableWidgets/datagrid-web/src/model/containers/Datagrid.container.ts	Adds CORE.atoms.visibleColumnsCount injection to GridSizeStore
packages/pluggableWidgets/datagrid-web/src/model/hooks/useInfiniteControl.tsx	Guards bumpPage behind scrollTop > 0 to prevent phantom loads on content-resize
packages/pluggableWidgets/datagrid-web/src/model/stores/__tests__/GridSize.store.spec.ts	New test file for GridSizeStore.lockGridContainerHeight()

Skipped (out of scope): dist/, pnpm-lock.yaml

CI check status could not be confirmed (command required approval).

---

Findings

🔶 Medium — Missing CHANGELOG entry for bug fixes

File: packages/pluggableWidgets/datagrid-web/CHANGELOG.md
Problem: Both bug fixes change observable runtime behaviour — the scrollbar/column toggle fix (WC-3333) and the many-column first-page-only fix (WC-3390). The ## [Unreleased] block only has an ### Added entry for the custom row key feature; there is no ### Fixed section for this PR.
Fix: Add a ### Fixed block under ## [Unreleased]:

### Fixed

- We fixed an issue where the vertical scrollbar disappeared after hiding a wide column while virtual scrolling was enabled. (WC-3333)
- We fixed an issue where only the first page loaded when the grid had enough columns to require horizontal scrolling. (WC-3390)

Run pnpm -w changelog if the tooling assists with formatting.

---

⚠️ Low — Overflow branch is not tested

File: packages/pluggableWidgets/datagrid-web/src/model/stores/__tests__/GridSize.store.spec.ts
Note: Every existing test uses containerClientHeight: 1000 with a fullHeight well below 1000, so gridContainer.clientHeight < fullHeight is always false. The "overflows" branch (overflows = true → height = fullHeight without subtracting VIRTUAL_SCROLLING_OFFSET) is never exercised. A test where containerClientHeight is smaller than fullHeight would verify the corrected clientHeight-based overflow detection — the exact logic that fixes WC-3390.

it("does not subtract offset when container height is smaller than full content height (overflow case)", () => {
    const { store } = buildStore({ pageSize: 3, columnCount: 2 });
    // fullHeight = 3×40 + 50 = 170px; container is only 100px → overflows
    setupRefs(store, { rowHeight: 40, rowCount: 3, headerHeight: 50, containerClientHeight: 100 });

    store.lockGridContainerHeight();

    expect(store.gridContainerHeight).toBe(170); // no offset subtracted
});

---

⚠️ Low — scrollTop > 0 guard in scroll handler is untested

File: packages/pluggableWidgets/datagrid-web/src/model/hooks/useInfiniteControl.tsx line 35
Note: The scrollTop > 0 guard is the direct fix for the phantom page-load on content-resize (part of WC-3333). There is no unit test for useInfiniteControl verifying that gridSizeStore.bumpPage is NOT called when the scroll handler fires with scrollTop === 0 at a position that would otherwise satisfy the bottom condition. Consider adding a focused test to lock in this invariant.

---

Positives

• The layoutKey string approach is a clean generalization — adding a third layout input later is a one-liner.
• Retaining the old gridContainerHeight on invalidation instead of resetting to undefined is a well-reasoned flicker fix; the PR description explains the tradeoff clearly.
• lockedAtLayoutKey: false in the makeAutoObservable override map correctly excludes the bookkeeping field from MobX observation, keeping reactivity limited to gridContainerHeight only.
• The test factory (buildStore + setupRefs) is well-structured: observable boxes are exposed for mutation in change-scenario tests, and DOM property overrides use configurable: true so they can be reasserted in the double-lock test.
• visibleColumnsCount token already existed in the DI graph (Root.container.ts line 75); injecting it into GridSizeStore via Datagrid.container.ts follows the established wiring pattern without requiring any new tokens.

[github-actions]

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge

---

What was reviewed

File	Change
packages/pluggableWidgets/datagrid-web/CHANGELOG.md	Added two Fixed entries under unreleased section
packages/pluggableWidgets/datagrid-web/src/model/containers/Datagrid.container.ts	Added CORE.atoms.visibleColumnsCount injection into GridSizeStore
packages/pluggableWidgets/datagrid-web/src/model/hooks/useInfiniteControl.tsx	Added scrollTop > 0 guard before bottom-detection logic
packages/pluggableWidgets/datagrid-web/src/model/stores/GridSize.store.ts	Replaced lockedAtPageSize with composite layoutKey; fixed overflow detection to use clientHeight
packages/pluggableWidgets/datagrid-web/src/model/stores/__tests__/GridSize.store.spec.ts	New — full unit test suite for lockGridContainerHeight()
packages/pluggableWidgets/datagrid-web/src/model/hooks/__tests__/useInfiniteControl.spec.tsx	New — unit tests for trackTableScrolling scroll guard

Skipped (out of scope): dist/, pnpm-lock.yaml

Note: gh pr checks could not be retrieved due to permissions — CI status unverified.

---

Findings

⚠️ Low — mockLockGridContainerHeight never cleared between tests

File: src/model/hooks/__tests__/useInfiniteControl.spec.tsx line 9
Note: mockLockGridContainerHeight is created with jest.fn() but mockLockGridContainerHeight.mockClear() is never called in beforeEach. The mock is never asserted against in any test, so no false positives today — but adding it alongside mockBumpPage.mockClear() at line 39 would prevent future test pollution if an assertion is ever added.

---

⚠️ Low — lockGridContainerHeight timer path is untested in the hook spec

File: src/model/hooks/__tests__/useInfiniteControl.spec.tsx line 36–98
Note: jest.useFakeTimers() is called in beforeEach, but jest.runAllTimers() is never called. This means the useEffect → setTimeout → lockGridContainerHeight() path (line 46 of the hook) is never exercised. The timer logic was tested in GridSize.store.spec.ts, but the wiring inside the hook is untested. Consider adding a test that advances timers and asserts mockLockGridContainerHeight was called:

it("calls lockGridContainerHeight after timeout when visible", () => {
    renderHook(() => useInfiniteControl());
    act(() => {
        jest.runAllTimers();
    });
    expect(mockLockGridContainerHeight).toHaveBeenCalledTimes(1);
});

---

⚠️ Low — No explicit assertion that gridContainerHeight is not cleared on key invalidation

File: src/model/stores/__tests__/GridSize.store.spec.ts line 161–200
Note: The core behavioral improvement — that the old height value is preserved (not set to undefined) when the layout key changes — is the fix for the scroll flicker bug. This is currently tested only indirectly: lockGridContainerHeight() is called immediately after changing pageSizeBox, so the height is always overwritten. A dedicated test that inspects gridContainerHeight between key invalidation and the next lock call would make the intended invariant explicit:

it("preserves old gridContainerHeight when layout key changes (no flicker)", () => {
    const { store, pageSizeBox } = buildStore({ pageSize: 3, columnCount: 2 });
    setupRefs(store, { rowHeight: 40, rowCount: 3, headerHeight: 50, containerClientHeight: 1000 });
    store.lockGridContainerHeight();
    const oldHeight = store.gridContainerHeight;

    // Invalidate the key but do NOT call lockGridContainerHeight again
    pageSizeBox.set(5);

    // Height should still be the old value, not undefined
    expect(store.gridContainerHeight).toBe(oldHeight);
    expect(store.gridContainerHeight).not.toBeUndefined();
});

---

Positives

• layoutKey as a single string encodes all cache inputs — the "one place to invalidate" design is clean and makes it easy to add a third dimension (e.g. row height mode) in one line.
• clientHeight < fullHeight for overflow detection is a precise fix: scrollHeight is inflated by horizontal overflow in wide grids, which was the root cause of WC-3390. The code comment explains the distinction well.
• GridSize.store.spec.ts uses observable.box + computed() correctly to mirror real atom behavior, avoiding fragile manual mocks.
• All test cases in GridSize.store.spec.ts include explicit arithmetic comments (e.g. // bodyViewport = 3 rows × 40px = 120px) — makes failures immediately diagnosable.
• CHANGELOG entries are accurate, concise, and in the correct Keep a Changelog Fixed section.