From 20fe61546a0109fa55c6464a948898f88e4b87bb Mon Sep 17 00:00:00 2001 From: Asif Ansari Date: Fri, 16 Oct 2026 20:23:39 +0530 Subject: [PATCH 1/2] feat(locale): warn in development when localeText re-creates its functions Every rendered row reads the locale from context, so a localeText with inline function entries re-rendered all rows on each parent render. Warn once, naming the entries, after three renders in a row re-create a function with the same source. The functions are not frozen: one that reads new state (a language switch) must still update the rows. Tracker: B8/B9 closed as won't fix (by design). --- CHANGELOG.md | 6 +++ docs/performance.md | 2 +- .../plans/2026-10-13-audit-tracker.md | 5 +- lib/components/DataGrid/GridRoot.tsx | 2 + .../core/useGridLocaleTextWarning.test.ts | 46 +++++++++++++++++++ lib/hooks/core/useGridLocaleTextWarning.ts | 45 ++++++++++++++++++ 6 files changed, 103 insertions(+), 3 deletions(-) create mode 100644 lib/hooks/core/useGridLocaleTextWarning.test.ts create mode 100644 lib/hooks/core/useGridLocaleTextWarning.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 067585a..5af3ea4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ --- +## [Unreleased] + +### Added + +- **Dev warning for re-created `localeText` functions.** A `localeText` whose function entries (`paginationOf`, `rowSelectLabel`, …) are written inline is a new object on every render, and every rendered row reads the locale, so each parent render re-rendered all of them. The grid now warns once in development, naming the entries, after three renders in a row re-create the same function. Behaviour is unchanged: the functions are not frozen, so one that reads new state (a language switch) still updates the rows. + ## [3.10.1] — 2026-10-13 ### Fixed diff --git a/docs/performance.md b/docs/performance.md index cbc3cc1..97e37e0 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -180,7 +180,7 @@ Measured in Chromium on an Apple M1, 1,000,000 rows, from the event to the end o 3.9.0 keeps rows memoised: selecting a row, starting or committing an edit, and vertical scrolling re-render only the rows they change (a click re-rendered every visible row, now one). React's own work per interaction is about 3–5 ms at 100k rows. -To keep this, change rows by passing new row objects (never mutate them), keep `columns` and `localeText` shallowly stable (inline objects of plain values are fine; define function entries outside the render). Handlers such as `onRowClick` may be inline: the grid passes rows its own stable handlers. +To keep this, change rows by passing new row objects (never mutate them), keep `columns` and `localeText` shallowly stable (inline objects of plain values are fine; define function entries outside the render; in development the grid warns when it sees the same function re-created render after render). Handlers such as `onRowClick` may be inline: the grid passes rows its own stable handlers. ### Method diff --git a/docs/superpowers/plans/2026-10-13-audit-tracker.md b/docs/superpowers/plans/2026-10-13-audit-tracker.md index 9f5b67e..f6049e3 100644 --- a/docs/superpowers/plans/2026-10-13-audit-tracker.md +++ b/docs/superpowers/plans/2026-10-13-audit-tracker.md @@ -18,8 +18,8 @@ Release plan: | B5 | Med | Date paste into ISO date-time string lands on previous day west of UTC | `lib/utils/parsing.ts:150-151` | 3.7.1 | ✅ | zoned current → local time of day on pasted day, written as UTC; tested in 3 TZs | | B6 | Med | List view reads undeclared `--ogx-grid-cell-text`; selection colours bypass theme | `ListView.css:82` | 3.7.1 | ✅ | list view uses `--ogx-grid-row-*` vars; old `--ogx-selection-*` still win | | B7 | Low–Med | Duplicate row ids silently drop rows (no dev warning) | row store | later | ✅ | Already handled since 3.0: dev warning naming the ids (`buildRowsState`, tested in `DataGrid.state.test.tsx`); the audit overstated it. Warns once per page | -| B8 | Low | Quick filter stale on in-place row mutation | quick-filter cache | later | ⏸ | Inherent to the per-row text cache; documented in `docs/features/filtering.md` | -| B9 | Low | Quick filter stale when a formatter closure changes | quick-filter cache | later | ⏸ | Inherent to the cache; documented (pass new column definitions) | +| B8 | Low | Quick filter stale on in-place row mutation | quick-filter cache | — | 🚫 | Won't fix (2026-10-16): the cache is keyed by row object; detecting mutation means re-reading every cell per keystroke (the 1.6–2 s cold search each time). Documented in `docs/features/filtering.md` | +| B9 | Low | Quick filter stale when a formatter closure changes | quick-filter cache | — | 🚫 | Won't fix (2026-10-16), same reason as B8; documented (pass new column definitions) | | B10 | Low | `bigint` loses precision in sort | `lib/utils/sorting/index.ts:45` | later | ✅ | Exact re-ranking (`rankNumbersExactly`, `compareExact`) when a column has an unsafe bigint; generic path tie-breaks by `big` | | B11 | Low | Number filters accept `0x10`, `Infinity` | `lib/utils/values.ts:96-101` | later | ✅ | `toNumber` rejects radix prefixes and non-finite text; new `values.test.ts` | @@ -81,3 +81,4 @@ Baselines (Chromium, M1, 1M rows): edit 508 ms · paste/undo/redo ~790 ms · fil - 2026-10-13 — Time-sliced pipeline closed (user agreed): proposal status updated, branch archived as `archive/timesliced-pipeline-2026-10-12`, both agent worktrees removed. Backlog under the feature hold is now empty except R-items done; ready to lift the hold (ask user). - 2026-10-13 — Release 3.10.0 via PR. - 2026-10-13 — 3.10.1 work: Next.js smoke fixture; perf #5 select-all ~60–90 ms / deselect <50 ms (live-check markers), #2 grouping 540 → 190 ms (lazy leaf maps, index partition), #3 quick filter first search 2.7 → ~1.6–2 s (per-day date memo, shown-text lower-case cache). check:compiler caught a getter-in-hook regression during #2 (fixed before push). +- 2026-10-16 — B8/B9 closed as won't fix (by design). Inline-function `localeText`: freezing the functions would bring back B9-style stale row labels, so added a dev-only warning instead (`useGridLocaleTextWarning`, in `GridRoot`). Branch `fix/localetext-functions`. diff --git a/lib/components/DataGrid/GridRoot.tsx b/lib/components/DataGrid/GridRoot.tsx index 1ebb5f0..8aaf6a3 100644 --- a/lib/components/DataGrid/GridRoot.tsx +++ b/lib/components/DataGrid/GridRoot.tsx @@ -2,6 +2,7 @@ import React, { useMemo } from 'react'; import { GridLocaleContext } from './gridLocaleContext'; import { resolveGridLocale } from '../../utils/gridLocale'; import { useShallowStableValue } from '../../hooks/core/useShallowStableValue'; +import { useGridLocaleTextWarning } from '../../hooks/core/useGridLocaleTextWarning'; import { GridToolbarHostContext } from '../../hooks/core/gridToolbarHostContext'; import { GridToolbarSlot } from './GridToolbarSlot'; import { GridAiAssistantArea } from '../AiAssistant/GridAiAssistantArea'; @@ -99,6 +100,7 @@ export function GridRoot(props: GridRootProps) { const showLoadingOverRows = effectiveLoading && allRenderableRows.length > 0 && paginationMode !== 'infinite'; const ToolbarSlot = slots?.toolbar; const stableLocaleText = useShallowStableValue(localeText); + useGridLocaleTextWarning(localeText); const locale = useMemo(() => resolveGridLocale(stableLocaleText), [stableLocaleText]); return ( diff --git a/lib/hooks/core/useGridLocaleTextWarning.test.ts b/lib/hooks/core/useGridLocaleTextWarning.test.ts new file mode 100644 index 0000000..bad584f --- /dev/null +++ b/lib/hooks/core/useGridLocaleTextWarning.test.ts @@ -0,0 +1,46 @@ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { renderHook } from '@testing-library/react'; +import { findRecreatedFunctionKeys, useGridLocaleTextWarning, LOCALE_TEXT_WARN_RENDERS } from './useGridLocaleTextWarning'; +import type { GridLocaleText } from '../../types'; + +const hoisted = (n: number) => `${n} rows found`; + +describe('findRecreatedFunctionKeys', () => { + it('reports a function re-created with the same source', () => { + const make = (): GridLocaleText => ({ liveRegionRowsFound: n => `${n} found`, paginationRowsPerPage: 'Rows' }); + expect(findRecreatedFunctionKeys(make(), make())).toEqual(['liveRegionRowsFound']); + }); + + it('ignores the same function, a changed source and string entries', () => { + expect(findRecreatedFunctionKeys({ liveRegionRowsFound: hoisted }, { liveRegionRowsFound: hoisted })).toEqual([]); + expect(findRecreatedFunctionKeys({ liveRegionRowsFound: n => `${n} a` }, { liveRegionRowsFound: n => `${n} b` })).toEqual([]); + expect(findRecreatedFunctionKeys({ paginationRowsPerPage: 'a' }, { paginationRowsPerPage: 'b' })).toEqual([]); + expect(findRecreatedFunctionKeys(undefined, { liveRegionRowsFound: hoisted })).toEqual([]); + }); +}); + +describe('useGridLocaleTextWarning', () => { + afterEach(() => vi.restoreAllMocks()); + + const inline = (): GridLocaleText => ({ liveRegionRowsFound: n => `${n} found` }); + + it('warns once after repeated renders that re-create a function', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const { rerender } = renderHook(({ text }) => useGridLocaleTextWarning(text), { initialProps: { text: inline() } }); + for (let i = 0; i < LOCALE_TEXT_WARN_RENDERS - 1; i++) rerender({ text: inline() }); + expect(warn).not.toHaveBeenCalled(); + rerender({ text: inline() }); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain('`liveRegionRowsFound`'); + for (let i = 0; i < 5; i++) rerender({ text: inline() }); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('stays silent for a stable localeText', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const text: GridLocaleText = { liveRegionRowsFound: hoisted }; + const { rerender } = renderHook(({ t }) => useGridLocaleTextWarning(t), { initialProps: { t: text } }); + for (let i = 0; i < 6; i++) rerender({ t: { ...text } }); + expect(warn).not.toHaveBeenCalled(); + }); +}); diff --git a/lib/hooks/core/useGridLocaleTextWarning.ts b/lib/hooks/core/useGridLocaleTextWarning.ts new file mode 100644 index 0000000..020ae13 --- /dev/null +++ b/lib/hooks/core/useGridLocaleTextWarning.ts @@ -0,0 +1,45 @@ +import { useEffect, useRef } from 'react'; +import type { GridLocaleText } from '../../types'; + +/** Renders in a row that must re-create the same function before the warning fires. */ +export const LOCALE_TEXT_WARN_RENDERS = 3; + +/** + * Keys whose function is a new object with the same source as before: an inline arrow re-created on + * render. A function with different source is a real change and is not reported. + */ +export function findRecreatedFunctionKeys(prev: GridLocaleText | undefined, next: GridLocaleText | undefined): string[] { + if (!prev || !next || prev === next) return []; + const before = prev as Record; + const after = next as Record; + return Object.keys(after).filter(key => { + const a = before[key]; + const b = after[key]; + return typeof a === 'function' && typeof b === 'function' && a !== b && a.toString() === b.toString(); + }); +} + +/** + * Dev-only: warns once when `localeText` keeps re-creating the same function entries. The locale is + * read from context by every rendered row, so each such render re-renders all of them. The functions + * are not made stable here: a function that reads changed state (a new language) must still update the rows. + */ +export function useGridLocaleTextWarning(localeText: GridLocaleText | undefined): void { + const prevRef = useRef(localeText); + const streakRef = useRef(0); + const warnedRef = useRef(false); + + useEffect(() => { + if (process.env.NODE_ENV === 'production' || warnedRef.current) return; + const keys = findRecreatedFunctionKeys(prevRef.current, localeText); + prevRef.current = localeText; + streakRef.current = keys.length > 0 ? streakRef.current + 1 : 0; + if (streakRef.current < LOCALE_TEXT_WARN_RENDERS) return; + warnedRef.current = true; + console.warn( + `[OpenGridX] \`localeText\` re-creates ${keys.map(key => `\`${key}\``).join(', ')} on every render, ` + + 'so every rendered row re-renders with it. Define these functions outside the component or in `useMemo`. ' + + 'See docs/performance.md.' + ); + }, [localeText]); +} From 9db153f279b15404194ea1c907d27e2660ef4253 Mon Sep 17 00:00:00 2001 From: Asif Ansari Date: Fri, 16 Oct 2026 20:23:40 +0530 Subject: [PATCH 2/2] test(editing): cover the select editor with mouse and keyboard in three engines Open, pick, commit on click-away / Enter / Tab / Shift+Tab, Escape restores the old value, arrows stay with the select. F2 and type-to-edit are not implemented and are marked it.fails. --- .../GridEditInputCell.select.browser.test.tsx | 286 ++++++++++++++++++ 1 file changed, 286 insertions(+) create mode 100644 lib/components/Cell/GridEditInputCell.select.browser.test.tsx diff --git a/lib/components/Cell/GridEditInputCell.select.browser.test.tsx b/lib/components/Cell/GridEditInputCell.select.browser.test.tsx new file mode 100644 index 0000000..70e722a --- /dev/null +++ b/lib/components/Cell/GridEditInputCell.select.browser.test.tsx @@ -0,0 +1,286 @@ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, cleanup } from '@testing-library/react'; +import { userEvent } from 'vitest/browser'; +import type { ReactNode } from 'react'; +import { DataGrid } from '../../index'; +import type { GridColDef } from '../../types'; +import '../../styles/opengridx.css'; + +// The singleSelect editor (a native on mousedown, + // the focusable cell around it would take focus, the blur would commit and the editor unmount. + it('clicking the open select keeps the editor open, focused and uncommitted', async () => { + const { container, processRowUpdate } = renderGrid(); + const select = await openByDoubleClick(container); + await userEvent.click(select); + await frame(); + expect(editorOf(container)).toBe(select); + expect(document.activeElement).toBe(select); + expect(processRowUpdate).not.toHaveBeenCalled(); + // Close a popup the click may have opened, so the rest of the page gets the input again. + await userEvent.keyboard('{Escape}'); + }); + + it('picking an option does not commit or close the editor by itself', async () => { + const { container, processRowUpdate } = renderGrid(); + const select = await openByDoubleClick(container); + await userEvent.selectOptions(select, 'Viewer'); + await frame(); + expect(editorOf(container)).toBe(select); + expect(select.value).toBe('Viewer'); + expect(document.activeElement).toBe(select); + expect(processRowUpdate).not.toHaveBeenCalled(); + }); + + it('clicking another cell commits the picked option and the cell shows it', async () => { + const { container, processRowUpdate } = renderGrid(); + const select = await openByDoubleClick(container); + await userEvent.selectOptions(select, 'Viewer'); + await userEvent.click(cellOf(container, 1, 'name')!); + await expect.poll(() => editorOf(container)).toBeNull(); + await expect.poll(() => processRowUpdate.mock.calls.length).toBe(1); + expect(processRowUpdate.mock.calls[0][0]).toMatchObject({ id: 1, role: 'Viewer' }); + await expect.poll(() => cellOf(container, 0, 'role')!.textContent).toBe('Viewer'); + expect(cellOf(container, 1, 'name')!.className).toContain('ogx__cell--focused'); + }); + + it('click, pick, then Enter commits and returns focus to the cell', async () => { + const { container, processRowUpdate } = renderGrid(); + const select = await openByDoubleClick(container); + await userEvent.click(select); + // Escape on an open popup closes the popup only; on a closed select it cancels the edit + // (headless engines here: always cancels). Reopen the editor in that case. + await userEvent.keyboard('{Escape}'); + const active = editorOf(container) ?? await openByDoubleClick(container); + await userEvent.selectOptions(active, 'Admin'); + active.focus(); + await userEvent.keyboard('{Enter}'); + await expect.poll(() => editorOf(container)).toBeNull(); + expect(processRowUpdate).toHaveBeenCalledTimes(1); + expect(processRowUpdate.mock.calls[0][0]).toMatchObject({ id: 1, role: 'Admin' }); + await expect.poll(() => cellOf(container, 0, 'role')!.textContent).toBe('Admin'); + await expect.poll(() => document.activeElement).toBe(cellOf(container, 0, 'role')); + }); +}); + +describe('singleSelect editor with the keyboard', () => { + it('Enter on the focused cell opens the editor with focus on the select', async () => { + const { container } = renderGrid(); + const select = await openByEnter(container); + expect(select.value).toBe('Editor'); + }); + + // Not implemented: only Enter and double-click open an editor (handleGridKeyDown in + // hooks/core/gridKeyDown.ts has no F2 or printable-key branch). `it.fails` flags it once it is. + it.fails('F2 on the focused cell opens the editor', async () => { + const { container } = renderGrid(); + await expect.poll(() => cellOf(container, 0, 'role')).not.toBeNull(); + await userEvent.click(cellOf(container, 0, 'role')!); + await userEvent.keyboard('{F2}'); + await frame(); + expect(editorOf(container)).not.toBeNull(); + }); + + it.fails('typing a character on the focused cell opens the editor', async () => { + const { container } = renderGrid(); + await expect.poll(() => cellOf(container, 0, 'role')).not.toBeNull(); + await userEvent.click(cellOf(container, 0, 'role')!); + await userEvent.keyboard('v'); + await frame(); + expect(editorOf(container)).not.toBeNull(); + }); + + it('ArrowDown / ArrowUp stay in the select: the grid neither moves focus nor ends the edit', async () => { + const { container, processRowUpdate } = renderGrid(); + const select = await openByEnter(container); + const seen: string[] = []; + await userEvent.keyboard('{ArrowDown}'); + await frame(); + seen.push(select.value); + expect(editorOf(container)).toBe(select); + expect(document.activeElement).toBe(select); + await userEvent.keyboard('{ArrowUp}'); + await frame(); + seen.push(select.value); + expect(editorOf(container)).toBe(select); + expect(document.activeElement).toBe(select); + // The focused cell is still the edited one, not the row below or above. + expect(cellOf(container, 1, 'role')!.className).not.toContain('ogx__cell--focused'); + expect(processRowUpdate).not.toHaveBeenCalled(); + // On a closed select ArrowDown picks the next option (Editor → Viewer) and ArrowUp goes back; + // where the engine opens a popup instead (macOS: all three engines here), the value stays put. + expect(['Viewer,Editor', 'Editor,Editor']).toContain(seen.join(',')); + await userEvent.keyboard('{Escape}'); + }); + + it('ArrowLeft / ArrowRight do not move grid focus while editing', async () => { + const { container } = renderGrid(); + const select = await openByEnter(container); + await userEvent.keyboard('{ArrowRight}'); + await userEvent.keyboard('{ArrowLeft}'); + await frame(); + expect(editorOf(container)).toBe(select); + expect(document.activeElement).toBe(select); + expect(cellOf(container, 0, 'team')!.className).not.toContain('ogx__cell--focused'); + expect(cellOf(container, 0, 'name')!.className).not.toContain('ogx__cell--focused'); + await userEvent.keyboard('{Escape}'); + }); + + it('typing an option\'s first letter on the focused select picks it (type-ahead), Enter commits', async () => { + const { container, processRowUpdate } = renderGrid(); + const select = await openByEnter(container); + await userEvent.keyboard('v'); + await expect.poll(() => select.value).toBe('Viewer'); + expect(editorOf(container)).toBe(select); + await userEvent.keyboard('{Enter}'); + await expect.poll(() => editorOf(container)).toBeNull(); + expect(processRowUpdate).toHaveBeenCalledTimes(1); + expect(processRowUpdate.mock.calls[0][0]).toMatchObject({ id: 1, role: 'Viewer' }); + await expect.poll(() => cellOf(container, 0, 'role')!.textContent).toBe('Viewer'); + await expect.poll(() => document.activeElement).toBe(cellOf(container, 0, 'role')); + }); + + it('a value moved to with the arrows is committed by Enter', async () => { + const { container, processRowUpdate } = renderGrid(); + const select = await openByEnter(container); + await userEvent.keyboard('{ArrowDown}'); + await frame(); + const picked = select.value; + await userEvent.keyboard('{Enter}'); + await expect.poll(() => editorOf(container)).toBeNull(); + if (picked === 'Editor') { + // The engine opened a popup on ArrowDown instead of changing the value: nothing changed. + expect(processRowUpdate).not.toHaveBeenCalled(); + } else { + expect(processRowUpdate).toHaveBeenCalledTimes(1); + expect(processRowUpdate.mock.calls[0][0]).toMatchObject({ id: 1, role: picked }); + } + await expect.poll(() => cellOf(container, 0, 'role')!.textContent).toBe(picked); + }); + + it('Escape cancels and restores the old value', async () => { + const { container, processRowUpdate } = renderGrid(); + const select = await openByEnter(container); + await userEvent.selectOptions(select, 'Admin'); + select.focus(); + await userEvent.keyboard('{Escape}'); + await expect.poll(() => editorOf(container)).toBeNull(); + expect(cellOf(container, 0, 'role')!.textContent).toBe('Editor'); + await frame(); + expect(processRowUpdate).not.toHaveBeenCalled(); + await expect.poll(() => document.activeElement).toBe(cellOf(container, 0, 'role')); + // Reopening shows the stored value, not the abandoned pick. + await userEvent.keyboard('{Enter}'); + await expect.poll(() => editorOf(container)?.value).toBe('Editor'); + }); + + it('Tab commits once and moves focus to the next editable cell', async () => { + const { container, processRowUpdate } = renderGrid(); + const select = await openByEnter(container); + await userEvent.selectOptions(select, 'Admin'); + select.focus(); + await userEvent.keyboard('{Tab}'); + await expect.poll(() => editorOf(container)).toBeNull(); + await frame(); + expect(processRowUpdate).toHaveBeenCalledTimes(1); + expect(processRowUpdate.mock.calls[0][0]).toMatchObject({ id: 1, role: 'Admin' }); + expect(cellOf(container, 0, 'role')!.textContent).toBe('Admin'); + expect(cellOf(container, 0, 'team')!.className).toContain('ogx__cell--focused'); + await expect.poll(() => document.activeElement).toBe(cellOf(container, 0, 'team')); + }); + + it('Shift+Tab commits once and moves focus to the previous editable cell', async () => { + const { container, processRowUpdate } = renderGrid(); + const select = await openByEnter(container); + await userEvent.selectOptions(select, 'Viewer'); + select.focus(); + await userEvent.keyboard('{Shift>}{Tab}{/Shift}'); + await expect.poll(() => editorOf(container)).toBeNull(); + await frame(); + expect(processRowUpdate).toHaveBeenCalledTimes(1); + expect(processRowUpdate.mock.calls[0][0]).toMatchObject({ id: 1, role: 'Viewer' }); + await expect.poll(() => document.activeElement).toBe(cellOf(container, 0, 'name')); + }); + + it('Space and Alt+ArrowDown on the focused select leave the edit open (they open the popup natively)', async () => { + const { container, processRowUpdate } = renderGrid(); + const select = await openByEnter(container); + await userEvent.keyboard(' '); + await frame(); + expect(editorOf(container)).toBe(select); + await userEvent.keyboard('{Escape}'); + const active = editorOf(container) ?? await openByEnter(container); + await userEvent.keyboard('{Alt>}{ArrowDown}{/Alt}'); + await frame(); + expect(editorOf(container)).toBe(active); + expect(processRowUpdate).not.toHaveBeenCalled(); + expect(cellOf(container, 1, 'role')!.className).not.toContain('ogx__cell--focused'); + await userEvent.keyboard('{Escape}'); + }); +});