From f586a6a5d4f3235c64bddf917fcc739b4ac2dbc7 Mon Sep 17 00:00:00 2001 From: woksin Date: Mon, 28 Sep 2026 17:40:08 +0200 Subject: [PATCH 01/17] Fix stale table results when query arguments change Key only the internal Arc subscriber by serialized arguments so the table and paginator keep their UI state. Show loading for uncached arguments and report cached results before paint. --- .../DataTableForObservableQuery.tsx | 81 +++++++----- Source/DataTables/DataTableForQuery.tsx | 60 ++++++--- ...nging_arguments_before_the_next_result.tsx | 118 ++++++++++++++++++ ...nging_arguments_before_the_next_result.tsx | 117 +++++++++++++++++ Source/DataTables/isSameTableResult.ts | 23 ++++ Source/DataTables/serializeQueryArguments.ts | 12 ++ 6 files changed, 364 insertions(+), 47 deletions(-) create mode 100644 Source/DataTables/for_DataTableForObservableQuery/when_changing_arguments_before_the_next_result.tsx create mode 100644 Source/DataTables/for_DataTableForQuery/when_changing_arguments_before_the_next_result.tsx create mode 100644 Source/DataTables/isSameTableResult.ts create mode 100644 Source/DataTables/serializeQueryArguments.ts diff --git a/Source/DataTables/DataTableForObservableQuery.tsx b/Source/DataTables/DataTableForObservableQuery.tsx index bb50fdcc..d8be3076 100644 --- a/Source/DataTables/DataTableForObservableQuery.tsx +++ b/Source/DataTables/DataTableForObservableQuery.tsx @@ -3,9 +3,9 @@ import type { DataTableParts } from './DataTableCore'; import type { Constructor } from '@cratis/fundamentals'; -import { type IObservableQueryFor, Paging } from '@cratis/arc/queries'; +import { type IObservableQueryFor, Paging, type QueryResultWithState } from '@cratis/arc/queries'; import { useObservableQueryWithPaging } from '@cratis/arc.react/queries'; -import { type ReactNode, useState, useRef, useEffect } from 'react'; +import { type ReactNode, useState, useRef, useEffect, useLayoutEffect, useCallback } from 'react'; import { DataTableCore } from './DataTableCore'; import { TablePaginator, @@ -15,6 +15,9 @@ import { import type { DataTableFilterMeta } from './DataTableFilterMeta'; import type { DataTableSelectionChangeEvent } from './DataTableSelectionChangeEvent'; import { resolveDataTableStatus } from './resolveDataTableStatus'; +import { DataTableStatus } from './DataTableStatus'; +import { serializeQueryArguments } from './serializeQueryArguments'; +import { isSameTableResult } from './isSameTableResult'; /** * Props for {@link DataTableForObservableQuery}. @@ -144,6 +147,26 @@ export interface DataTableForObservableQueryProps< } const paging = new Paging(0, 20); +const useIsomorphicLayoutEffect = typeof window === 'undefined' ? useEffect : useLayoutEffect; + +const KeyedQuerySubscriber = < + TQuery extends IObservableQueryFor | IObservableQueryFor, + TDataType extends object, + TArguments extends object, +>({ query, args, queryKey, onResult }: { + query: Constructor; + args?: TArguments; + queryKey: string; + onResult: (key: string, result: QueryResultWithState, setPage: (page: number) => void) => void; +}) => { + // Arc's nominal subscription type requires constructor erasure and an explicit row type + // to avoid the separate private declarations in different consumer copies of Arc. + const [result, , setPage] = useObservableQueryWithPaging( + query as never, paging, args, + ); + useIsomorphicLayoutEffect(() => { onResult(queryKey, result, setPage); }, [queryKey, result, onResult]); + return null; +}; /** * A paged data table bound to a real-time Cratis Arc observable query @@ -185,38 +208,28 @@ export const DataTableForObservableQuery = < >( props: DataTableForObservableQueryProps, ) => { - // Type arguments are supplied explicitly, and the constructor is erased on the way in - // (Cratis/Components#135). Two separate defects in `@cratis/arc.react` make the plain call - // fail to type-check: - // - // 1. `useObservableQueryWithPaging, …>` - // gives `TDataType` no inference site — it appears only inside `TQuery`'s constraint — so it - // silently falls back to `unknown` and `result` comes back as `QueryResultWithState`. - // 2. `ObservableQuerySubscription` carries a `private _connection`, which makes it nominally - // typed. `@cratis/arc.react` pins `@cratis/arc` to an exact version, so any consumer whose own - // `@cratis/arc` differs by even a patch gets a second nested copy — two declarations of that - // class, and TS2345 "types have separate declarations of a private property". Our peer range - // allows multiple Arc versions, so we cannot assume a consumer's tree is deduped. - // - // Erasing the constructor type sidesteps the nominal comparison, and naming `TDataType` - // explicitly recovers the row type this component is generic over — strictly better than the - // `unknown` the inference would otherwise produce. Remove both once Arc returns an interface - // from `subscribe()` and makes `TDataType` inferable. - const [result, , setPage] = useObservableQueryWithPaging< - TDataType, - never, - TArguments - >(props.query as never, paging, props.queryArguments); + const [snapshot, setSnapshot] = useState<{ + queryKey: string; + result: QueryResultWithState; + setPage: (page: number) => void; + }>(); + const onResult = useCallback((queryKey: string, result: QueryResultWithState, setPage: (page: number) => void) => { + setSnapshot((previous) => previous?.queryKey === queryKey && isSameTableResult(previous.result, result) + ? previous : { queryKey, result, setPage }); + }, []); + const queryKey = serializeQueryArguments(props.queryArguments); + const current = snapshot?.queryKey === queryKey ? snapshot : undefined; + const result = current?.result; const containerRef = useRef(null); const [tableHeight, setTableHeight] = useState(600); const timeoutRef = useRef | undefined>(undefined); - const totalItems = result.paging.totalItems; - const pageCount = result.paging.totalPages; + const totalItems = result?.paging.totalItems ?? 0; + const pageCount = result?.paging.totalPages ?? 0; const showPaginator = totalItems > 0 && pageCount > 1; - // SAFETY: Arc observable collection queries are row-typed while runtime data is the current row array. - const rows = result.data as unknown as TDataType[]; - const status = resolveDataTableStatus(result); + // Arc's unauthorized result has null data; the table always receives an array. + const rows = Array.isArray(result?.data) ? result.data as TDataType[] : []; + const status = result ? resolveDataTableStatus(result) : DataTableStatus.Loading; useEffect(() => { if (!containerRef.current) return; @@ -267,6 +280,10 @@ export const DataTableForObservableQuery = < overflow: 'hidden', }} > + + key={queryKey} query={props.query} args={props.queryArguments} + queryKey={queryKey} onResult={onResult} + />
data={rows} @@ -297,7 +314,7 @@ export const DataTableForObservableQuery = <
- {showPaginator && ( + {showPaginator && current && (
| IQueryFor, + TDataType extends object, + TArguments extends object, +>({ query, args, queryKey, onResult }: { + query: Constructor; + args?: TArguments; + queryKey: string; + onResult: (key: string, result: QueryResultWithState, setPage: (page: number) => void) => void; +}) => { + const [result, , , setPage] = useQueryWithPaging(query, paging, args); + useIsomorphicLayoutEffect(() => { onResult(queryKey, result, setPage); }, [queryKey, result, onResult]); + return null; +}; /** * A paged data table bound to a snapshot Cratis Arc query @@ -194,17 +213,24 @@ export const DataTableForQuery = < >( props: DataTableForQueryProps, ) => { - const [result, , , setPage] = useQueryWithPaging( - props.query, - paging, - props.queryArguments, - ); - const totalItems = result.paging.totalItems; - const pageCount = result.paging.totalPages; + const [snapshot, setSnapshot] = useState<{ + queryKey: string; + result: QueryResultWithState; + setPage: (page: number) => void; + }>(); + const onResult = useCallback((queryKey: string, result: QueryResultWithState, setPage: (page: number) => void) => { + setSnapshot((previous) => previous?.queryKey === queryKey && isSameTableResult(previous.result, result) + ? previous : { queryKey, result, setPage }); + }, []); + const queryKey = serializeQueryArguments(props.queryArguments); + const current = snapshot?.queryKey === queryKey ? snapshot : undefined; + const result = current?.result; + const totalItems = result?.paging.totalItems ?? 0; + const pageCount = result?.paging.totalPages ?? 0; - // SAFETY: Arc collection queries are row-typed while their runtime data is the current row array. - const rows = result.data as unknown as TDataType[]; - const status = resolveDataTableStatus(result); + // Arc's unauthorized result has null data; the table always receives an array. + const rows = Array.isArray(result?.data) ? result.data as TDataType[] : []; + const status = result ? resolveDataTableStatus(result) : DataTableStatus.Loading; return (
+ + key={queryKey} query={props.query} args={props.queryArguments} + queryKey={queryKey} onResult={onResult} + /> {/* The scroll container must be `DataTableCore`'s own bounded container rather than this wrapper. `.cratis-datatable__container` sets `overflow-x: auto`, and CSS computes the matching `overflow-y: visible` to `auto`, so that element is always @@ -253,7 +283,7 @@ export const DataTableForQuery = <
- {totalItems > 0 && pageCount > 1 && ( + {current && totalItems > 0 && pageCount > 1 && (
) => void>(); +class ScopedQuery extends ObservableQueryFor { + readonly route = '/api/sample/observable-scoped-rows'; + readonly defaultValue: Row[] = []; + readonly parameterDescriptors = []; + get requiredRequestParameters() { return []; } + constructor() { super(Object, true); } + override subscribe(callback: (result: QueryResult) => void, args?: { scope: string }): ObservableQuerySubscription { + deliverResult.set(args!.scope, callback); + return { unsubscribe: () => { deliverResult.delete(args!.scope); } } as unknown as ObservableQuerySubscription; + } +} +const resultWith = (data: Row[], page = 0) => new QueryResult({ + data, isSuccess: true, isReady: true, isAuthorized: true, isValid: true, + hasExceptions: false, validationResults: [], exceptionMessages: [], exceptionStackTrace: '', + paging: { page, size: 20, totalItems: 40, totalPages: 2 }, +}, Object, true); +const failed = new QueryResult({ + data: [{ id: 1, name: 'Example stale row' }], isSuccess: false, isReady: true, + isAuthorized: true, isValid: true, hasExceptions: true, validationResults: [], + exceptionMessages: [], exceptionStackTrace: '', + paging: { page: 0, size: 20, totalItems: 40, totalPages: 2 }, +}, Object, true); +let root: Root; +let container: HTMLDivElement; +let queryCache: QueryInstanceCache; +const tableFor = (scope: string) => ( + + + + query={ScopedQuery} queryArguments={{ scope }} emptyMessage='No example rows' + loadingMessage='Loading example rows' failureMessage='Example rows failed' + unauthorizedMessage='Example rows denied' globalFilterFields={['name']}> + field='name' header='Name' sortable /> + + + +); +const show = async (scope: string) => { await act(async () => { root.render(tableFor(scope)); }); }; +const deliver = async (scope: string, result: QueryResult) => { + await act(async () => { deliverResult.get(scope)!(result); }); +}; +const rowText = () => container.querySelector('[data-cratis-part="row"]')?.textContent ?? null; +const loadingText = () => container.querySelector('[role="status"]')?.textContent ?? null; + +beforeEach(() => { + (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + deliverResult.clear(); queryCache = new QueryInstanceCache(); + globalThis.ResizeObserver = class { + observe() { /* jsdom has no layout */ } + disconnect() { /* jsdom has no layout */ } + unobserve() { /* jsdom has no layout */ } + }; + container = document.createElement('div'); document.body.append(container); + root = createRoot(container); +}); +afterEach(async () => { + await act(async () => { root.unmount(); }); + queryCache.dispose(); container.remove(); +}); + +describe.each([ + ['unauthorized', QueryResult.unauthorized(), 'Example rows denied'], + ['failed', failed, 'Example rows failed'], +])('when observable arguments change after a %s result', (_state, previous, alert) => { + it('should show loading without the previous status or rows until the new result arrives', async () => { + await show('A'); + await deliver('A', previous); + (container.querySelector('[role="alert"]')?.textContent === alert).should.equal(true); + await show('B'); + (loadingText() === 'Loading example rows').should.equal(true); + (container.querySelector('[role="alert"]') === null).should.equal(true); + (rowText() === null).should.equal(true); + await deliver('B', resultWith([{ id: 2, name: 'Example B' }])); + rowText()?.should.contain('Example B'); + }); +}); + +describe('when returning to cached observable arguments', () => { + it('should restore rows before paint and keep paging without a loading flash', async () => { + await show('A'); + await deliver('A', resultWith([{ id: 1, name: 'Example A' }])); + const search = container.querySelector('[data-cratis-part="search-input"]'); + await act(async () => { container.querySelector('[data-cratis-part="sort"]')!.click(); }); + await show('B'); + (loadingText() === 'Loading example rows').should.equal(true); + (rowText() === null).should.equal(true); + (container.querySelector('[data-cratis-part="search-input"]') === search).should.equal(true); + container.querySelector('[aria-sort="ascending"]')?.textContent.should.contain('Name'); + await deliver('B', resultWith([{ id: 2, name: 'Example B' }])); + await show('A'); + (loadingText() === null).should.equal(true); + rowText()?.should.contain('Example A'); + (container.querySelector('[data-cratis-part="search-input"]') === search).should.equal(true); + await act(async () => { + container.querySelector('[aria-label="Next page"]')!.click(); + }); + (loadingText() === null).should.equal(true); + rowText()?.should.contain('Example A'); + await deliver('A', resultWith([{ id: 21, name: 'Example A page two' }], 1)); + rowText()?.should.contain('Example A page two'); + container.querySelector('.cratis-table-paginator-info')?.textContent.should.equal('2 / 2'); + }); +}); diff --git a/Source/DataTables/for_DataTableForQuery/when_changing_arguments_before_the_next_result.tsx b/Source/DataTables/for_DataTableForQuery/when_changing_arguments_before_the_next_result.tsx new file mode 100644 index 00000000..44a29440 --- /dev/null +++ b/Source/DataTables/for_DataTableForQuery/when_changing_arguments_before_the_next_result.tsx @@ -0,0 +1,117 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +// @vitest-environment jsdom + +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { QueryFor, QueryInstanceCache, QueryResult } from '@cratis/arc/queries'; +import { QueryInstanceCacheContext } from '@cratis/arc.react/queries'; +import { CratisComponentsProvider } from '../../Common/CratisComponentsProvider'; +import { Column } from '../Column'; +import { DataTableForQuery } from '../DataTableForQuery'; + +interface Row { id: number; name: string } +const pending = new Map) => void>(); +const requestedPages: number[] = []; + +class ScopedQuery extends QueryFor { + readonly route = '/api/sample/scoped-rows'; + readonly defaultValue: Row[] = []; + readonly parameterDescriptors = []; + get requiredRequestParameters() { return []; } + constructor() { super(Object, true); } + override perform(args?: { scope: string }): Promise> { + requestedPages.push(this.paging.page); + return new Promise((resolve) => { pending.set(args!.scope, resolve); }); + } +} + +const resultWith = (data: Row[], page = 0) => new QueryResult({ + data, isSuccess: true, isReady: true, isAuthorized: true, isValid: true, + hasExceptions: false, validationResults: [], exceptionMessages: [], exceptionStackTrace: '', + paging: { page, size: 20, totalItems: 40, totalPages: 2 }, +}, Object, true); +const failed = new QueryResult({ + data: [{ id: 1, name: 'Example stale row' }], isSuccess: false, isReady: true, + isAuthorized: true, isValid: true, hasExceptions: true, validationResults: [], + exceptionMessages: [], exceptionStackTrace: '', + paging: { page: 0, size: 20, totalItems: 40, totalPages: 2 }, +}, Object, true); + +let root: Root; +let container: HTMLDivElement; +let queryCache: QueryInstanceCache; +const tableFor = (scope: string) => ( + + + + query={ScopedQuery} queryArguments={{ scope }} emptyMessage='No example rows' + loadingMessage='Loading example rows' failureMessage='Example rows failed' + unauthorizedMessage='Example rows denied' globalFilterFields={['name']}> + field='name' header='Name' sortable /> + + + +); +const show = async (scope: string) => { await act(async () => { root.render(tableFor(scope)); }); }; +const deliver = async (scope: string, result: QueryResult) => { + await act(async () => { pending.get(scope)!(result); }); +}; +const rowText = () => container.querySelector('[data-cratis-part="row"]')?.textContent ?? null; +const loadingText = () => container.querySelector('[role="status"]')?.textContent ?? null; + +beforeEach(() => { + (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + pending.clear(); requestedPages.length = 0; + queryCache = new QueryInstanceCache(); + container = document.createElement('div'); document.body.append(container); + root = createRoot(container); +}); +afterEach(async () => { + await act(async () => { root.unmount(); }); + queryCache.dispose(); container.remove(); +}); + +describe.each([ + ['unauthorized', QueryResult.unauthorized(), 'Example rows denied'], + ['failed', failed, 'Example rows failed'], +])('when snapshot arguments change after a %s result', (_state, previous, alert) => { + it('should show loading without the previous status or rows until the new result arrives', async () => { + await show('A'); + await deliver('A', previous); + (container.querySelector('[role="alert"]')?.textContent === alert).should.equal(true); + await show('B'); + (loadingText() === 'Loading example rows').should.equal(true); + (container.querySelector('[role="alert"]') === null).should.equal(true); + (rowText() === null).should.equal(true); + await deliver('B', resultWith([{ id: 2, name: 'Example B' }])); + rowText()?.should.contain('Example B'); + }); +}); + +describe('when returning to cached snapshot arguments', () => { + it('should restore rows before paint and retain paging without a loading flash', async () => { + await show('A'); + await deliver('A', resultWith([{ id: 1, name: 'Example A' }])); + const search = container.querySelector('[data-cratis-part="search-input"]'); + await act(async () => { container.querySelector('[data-cratis-part="sort"]')!.click(); }); + await show('B'); + (loadingText() === 'Loading example rows').should.equal(true); + (rowText() === null).should.equal(true); + (container.querySelector('[data-cratis-part="search-input"]') === search).should.equal(true); + container.querySelector('[aria-sort="ascending"]')?.textContent.should.contain('Name'); + await deliver('B', resultWith([{ id: 2, name: 'Example B' }])); + await show('A'); + (loadingText() === null).should.equal(true); + rowText()?.should.contain('Example A'); + (container.querySelector('[data-cratis-part="search-input"]') === search).should.equal(true); + await act(async () => { + container.querySelector('[aria-label="Next page"]')!.click(); + }); + requestedPages.at(-1)?.should.equal(1); + (loadingText() === null).should.equal(true); + await deliver('A', resultWith([{ id: 21, name: 'Example A page two' }], 1)); + rowText()?.should.contain('Example A page two'); + }); +}); diff --git a/Source/DataTables/isSameTableResult.ts b/Source/DataTables/isSameTableResult.ts new file mode 100644 index 00000000..8f6f76cc --- /dev/null +++ b/Source/DataTables/isSameTableResult.ts @@ -0,0 +1,23 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +import type { QueryResultWithState } from '@cratis/arc/queries'; +import { resolveDataTableStatus } from './resolveDataTableStatus'; + +/** Avoid a render loop if a paging hook returns an equivalent new object on each render. */ +export const isSameTableResult = ( + previous: QueryResultWithState, + next: QueryResultWithState, +): boolean => { + const previousData = previous.data; + const nextData = next.data; + const sameData = previousData === nextData || + (Array.isArray(previousData) && Array.isArray(nextData) && + previousData.length === nextData.length && + previousData.every((row, index) => row === nextData[index])); + return sameData && resolveDataTableStatus(previous) === resolveDataTableStatus(next) && + previous.paging.page === next.paging.page && + previous.paging.totalItems === next.paging.totalItems && + previous.paging.totalPages === next.paging.totalPages && + previous.paging.size === next.paging.size; +}; diff --git a/Source/DataTables/serializeQueryArguments.ts b/Source/DataTables/serializeQueryArguments.ts new file mode 100644 index 00000000..8838b49b --- /dev/null +++ b/Source/DataTables/serializeQueryArguments.ts @@ -0,0 +1,12 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +/** Match Arc's sorted, JSON-serialized argument values when keying a bound query subscriber. */ +export const serializeQueryArguments = (args?: object): string => { + if (!args || Object.keys(args).length === 0) return ''; + const sorted = Object.keys(args).sort().reduce>((entries, key) => { + entries[key] = (args as Record)[key]; + return entries; + }, {}); + return JSON.stringify(sorted); +}; From 5d8207745c6f4d50d2e9bc730215316a365979f1 Mon Sep 17 00:00:00 2001 From: woksin Date: Mon, 28 Sep 2026 17:40:13 +0200 Subject: [PATCH 02/17] Keep filter panels inside classic-scrollbar viewports Clamp against the layout viewport rather than including scrollbar space. Document the unsupported rotation and skew containing blocks. --- Documentation/Filter/index.md | 2 ++ Source/Filter/FilterPanel.tsx | 6 ++++- Source/Filter/scrollbars.browser.test.tsx | 29 +++++++++++++++++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/Documentation/Filter/index.md b/Documentation/Filter/index.md index 3de144ef..8349d85e 100644 --- a/Documentation/Filter/index.md +++ b/Documentation/Filter/index.md @@ -315,6 +315,8 @@ The first filter group starts expanded. The hook re-syncs its state when the set ## Accessibility and keyboard +Translated and scaled overlay containers are supported; rotated or skewed overlay containers are not supported. + - The panel portals into the `CratisComponentsProvider` [overlay container](../Common/cratis-components-provider.md#choose-an-overlay-container), or `document.body` without one. It stays anchored at a fixed position below `anchorRef` and follows the anchor on scroll and resize. If a configured container is temporarily unavailable (`getContainer()` returns `null`), the panel waits rather than rendering into the body; a later render can mount it when the container is available. Inside a Cratis modal `Dialog`, a shared container outside the modal would hide the panel from assistive technology and pull focus back into the modal. In that case the panel mounts inside the dialog's modal root instead. If the dialog has a persistent fixed containing block (for example, a `pt.root` transform), the panel stays within the dialog's visible bounds and its contents scroll to keep options reachable. A container already inside the modal remains the destination. - The panel renders as a `
` with `role="dialog"`, so it is exposed as a named, non-modal dialog, not an `