[WC-3333]- DG2 virtual scroll loses scrollbar after column hide

[rahmanunver]

Pull request type

Bug fix

---

Description

What: lockGridBodyHeight() in GridSizeStore locked the virtual scroll body height once and only reset it when the page size changed. When a column is hidden, row heights can decrease (text no longer wraps), so the content height drops below the locked max-height — the scrollbar disappears.

Why: The reset condition was incomplete. The existing lockedAtPageSize guard pattern was correct but not applied to column-count changes. The fix adds a parallel lockedAtColumnCount guard: the locked height is now invalidated whenever the visible column count changes, mirroring the existing page-size reset exactly.

Files changed:

• src/model/stores/GridSize.store.ts — added lockedAtColumnCount guard
• src/model/containers/Datagrid.container.ts — wired visibleColumnsCountAtom into GridSizeStore via DI
• src/components/Pagination.tsx, src/components/WidgetFooter.tsx — pre-existing TS7006 implicit-any fixes (were already failing on main)
• e2e/DataGrid.spec.js — regression test: virtual scroll + column hide
• AGENTS.md — architecture reference for the datagrid-web package

What should be covered while testing?

1. Open a page with a Data Grid configured for virtual scrolling and enough rows to produce a scrollbar
2. Confirm the scrollbar is visible in the grid body
3. Open the column selector and hide any column
4. Confirm the scrollbar is still visible after hiding — this was the bug
5. Show the column again and confirm the scrollbar remains consistent

[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	Added lockedAtColumnCount guard to invalidate cached height on column-count change
packages/pluggableWidgets/datagrid-web/src/model/containers/Datagrid.container.ts	Wired CORE.atoms.visibleColumnsCount into GridSizeStore via DI
packages/pluggableWidgets/datagrid-web/src/components/Pagination.tsx	Added explicit (n: number) type annotation to fix TS7006 implicit-any
packages/pluggableWidgets/datagrid-web/src/components/WidgetFooter.tsx	Same TS7006 fix for load-more button callback
packages/pluggableWidgets/datagrid-web/e2e/DataGrid.spec.js	Added regression test for virtual scroll + column hide
packages/pluggableWidgets/datagrid-web/AGENTS.md	New package-level architecture reference
packages/pluggableWidgets/datagrid-web/CLAUDE.md	New Claude Code proxy pointing to AGENTS.md
packages/pluggableWidgets/datagrid-web/CHANGELOG.md	Added Fixed entry for the scrollbar bug

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

---

Findings

🔶 Medium — page.waitForTimeout(300) hardcoded sleep in E2E test

File: packages/pluggableWidgets/datagrid-web/e2e/DataGrid.spec.js line 215
Problem: Hardcoded waitForTimeout is a Playwright anti-pattern — it makes the test flaky on slow CI machines and masks real timing issues. The correct pattern is to wait for the observable state change instead (e.g. a class toggle, attribute change, or locator assertion that Playwright will retry automatically).
Fix: Replace the timeout with a locator-based wait. The column selector closes after selection, so waiting for it to disappear is a reliable synchronisation point:

await grid.locator(".column-selector-button").click();
await page.locator(".column-selectors > li").first().click();
// Wait for the column-selector popover to close before measuring layout
await page.locator(".column-selectors").waitFor({ state: "hidden" });

const after = await gridBody.evaluate(el => ({
    hasScrollbar: el.scrollHeight > el.clientHeight
}));
expect(after.hasScrollbar).toBe(true);

---

🔶 Medium — No unit tests for the new lockedAtColumnCount reset path in GridSizeStore

File: packages/pluggableWidgets/datagrid-web/src/model/stores/GridSize.store.ts
Problem: The new column-count invalidation branch (lines 96–100 and 126) is the core fix and has no unit-level coverage. The only regression test is an E2E spec that depends on a live Mendix app. A focused unit test would catch regressions faster and document the invariant without needing a running server.
Fix: Add a spec in src/model/stores/__tests__/GridSize.store.spec.ts (or alongside existing model tests) that:

1. Constructs a GridSizeStore with a mock visibleColumnsCountAtom returning 3.
2. Calls lockGridContainerHeight() to set gridContainerHeight.
3. Changes the atom to return 2 and calls lockGridContainerHeight() again.
4. Asserts that gridContainerHeight was reset (undefined) before being re-locked.

---

Positives

• The fix mirrors the existing lockedAtPageSize pattern exactly — same field shape, same reset condition, same assignment at lock time. This makes the code immediately readable to anyone familiar with the page-size guard.
• Both private fields are correctly excluded from MobX tracking via the makeAutoObservable type parameter, avoiding unintended reactivity.
• The DI wiring in Datagrid.container.ts follows the established injected() + token pattern precisely — no new abstractions introduced.
• CHANGELOG entry is present and in Keep a Changelog format.
• The new AGENTS.md is thorough and will significantly reduce onboarding time for contributors working on this widget.

[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/CHANGELOG.md	Added ### Fixed entry for virtual-scroll scrollbar bug
packages/pluggableWidgets/datagrid-web/src/components/Pagination.tsx	Added explicit : number annotation on implicit-any arrow params (TS7006 fix)
packages/pluggableWidgets/datagrid-web/src/components/WidgetFooter.tsx	Same TS7006 fix for onClick handler
packages/pluggableWidgets/datagrid-web/src/model/containers/Datagrid.container.ts	Wired CORE.atoms.visibleColumnsCount into GridSizeStore injection
packages/pluggableWidgets/datagrid-web/src/model/stores/GridSize.store.ts	Added lockedAtColumnCount guard to invalidate locked height on column-count change

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

CI checks status: unknown — gh pr checks was unavailable in this environment.

---

Findings

🔶 Medium — E2E regression test claimed in PR body but absent from diff

File: packages/pluggableWidgets/datagrid-web/e2e/DataGrid.spec.js (not in diff)
Problem: The PR description explicitly lists e2e/DataGrid.spec.js — regression test: virtual scroll + column hide as a changed file, but it does not appear in the diff. The regression test for the fixed scenario (virtual scroll + column hide → scrollbar persists) was never committed. For a DOM-layout bug that only manifests at runtime, an E2E test is especially important — unit tests cannot exercise scrollHeight/clientHeight in jsdom.
Fix: Add a Playwright test in e2e/DataGrid.spec.js covering the bug scenario:

test("scrollbar remains visible after hiding a column in virtual scrolling mode", async ({ page }) => {
    await page.goto("/p/virtual-scrolling");
    await page.waitForLoadState("networkidle");

    const grid = page.locator(".mx-name-dataGrid1");
    const gridBody = grid.locator(".widget-datagrid-scroll-body");

    // Confirm scrollbar is present before hiding a column
    await expect(gridBody).toBeVisible();

    // Hide a column via the column selector
    await page.locator(".mx-name-dataGrid1 .widget-datagrid-column-selector-btn").click();
    await page.locator(".widget-datagrid-column-selector-item").first().click();

    // Scrollbar must still be visible
    await expect(gridBody).toBeVisible();
});

---

🔶 Medium — No unit tests for lockedAtColumnCount guard logic

File: packages/pluggableWidgets/datagrid-web/src/model/stores/GridSize.store.ts
Problem: There are zero unit tests for GridSizeStore (no src/model/stores/__tests__/ directory). The new lockedAtColumnCount guard introduces three branches worth testing: (a) height is reset when column count changes while locked, (b) height is NOT reset when column count is unchanged, (c) both lockedAtPageSize and lockedAtColumnCount are recorded atomically when the height is first locked. Without tests, a refactor could silently break the guard.
Fix: Add src/model/stores/__tests__/GridSize.store.spec.ts. Example skeleton:

describe("GridSizeStore.lockGridContainerHeight", () => {
    describe("column count guard", () => {
        it("resets locked height when visible column count changes", () => { ... });
        it("does not reset locked height when column count is unchanged", () => { ... });
        it("records lockedAtColumnCount atomically with gridContainerHeight", () => { ... });
    });
});

---

⚠️ Low — PR title format deviates from repo convention

File: PR metadata
Note: Title is [WC-3333]- DG2 virtual scroll loses scrollbar after column hide (dash after bracket). Repo convention (from AGENTS.md conventional-commits note and other PRs) is [WC-3333]: description (colon). Not blocking, but worth correcting for consistency.

---

Positives

• The lockedAtColumnCount guard exactly mirrors the existing lockedAtPageSize pattern — same condition shape, same reset approach, same atomic write at lock time. This makes the new code easy to read and maintain.
• Both private fields are correctly excluded from MobX's makeAutoObservable tracking via the false annotation, consistent with how lockedAtPageSize was already handled.
• CORE.atoms.visibleColumnsCount was already used in _04_emptyStateBindings and _08_rowInteractionBindings, so this DI wiring follows the established injection pattern with no new token definitions needed.
• The TS7006 fixes in Pagination.tsx and WidgetFooter.tsx are self-contained and non-breaking.
• CHANGELOG entry is present and correctly placed under [Unreleased] → ### Fixed.