Skip to content

fix(bindx-dataview): reload, select paging and infinite load - #162

Merged
matej21 merged 10 commits into
mainfrom
fix/dataview-bugs
Nov 4, 2026
Merged

matej21 merged 10 commits into
mainfrom
fix/dataview-bugs

Conversation

@matej21

@matej21 matej21 commented Nov 4, 2026 •

Copy link
Copy Markdown
Member

Fixes three DataView bugs. They share SelectDataView.tsx and build on each other (the infinite-load fix relies on the loader state introduced for reload and on the select page reset), so they land as one PR with one commit per bug plus two follow-ups from review.

#124 — reload() is a no-op (480f41c, follow-ups 7b7049a, eb4eeb9)

reload() in DataGrid, HasManyDataGrid and SelectDataView bumped a counter nothing read.

  • DataGrid / SelectDataView: the counter is folded into the list's queryKey and reload() bumps the paging's count refresh token.
  • HasManyDataGrid: the relation query re-runs (it carries the total) and keeps the rows on screen while the same query loads again.
  • The loader state is derived in render (useLoaderState) instead of an effect, and reports refreshing while the rows on screen answer an earlier query. Before, a stale-while-revalidate refetch stayed loaded, so the reload button never spun and the refreshing overlay never showed. Behaviour change: grids and selects now report refreshing on every refetch (page, sort, filter, reload), as the bindx-ui loader and toolbar expect.
  • Review follow-ups:
    • 7b7049a and eb4eeb9: refreshing means only "the rows on screen answer an earlier query" (a load with nothing to show is initial).
    • DataViewNonEmpty and DataViewEmpty stay on screen while refreshing, so the default grid neither unmounts its table nor hides "No results" during a refetch.
    • HasManyDataGrid keeps its previous rows for any refetch, but only for the relation they were loaded for (844bde1). It derives the refetch in render, and applies responses without overwriting local edits.

#145 — a narrowed select search shows nothing (853511c)

  • SelectDataView resets paging to the first page when its filter (search + static filter) changes, during render, so no query goes out at the old offset.
  • usePagingTotalCount: a current page shorter than the limit sets the total, over a counted total that may still describe the previous search. So hasNext is false for a short page and the sentinel no longer loads an empty page.

#146 — infinite scroll stops after page 2 (123b5d6, follow-ups 6431851, 3dd3746)

DataViewInfiniteLoadProvider:

  • loadMore stores the current page and calls paging.next(); two calls before the next render request one page.
  • Children get a data view whose items are the stored pages followed by the current one (each row once), so DataViewEachRow, the keyboard handler and highlight indices see the whole list.
  • Stored pages belong to one query (filter, order, page size) and to an unbroken run of offsets from the first row.
  • Loading follows the view's loader state, so it ends when the page lands, empty or not.
  • 6431851 and 3dd3746 (review follow-ups): reload() inside the provider restarts from the first page, since the view refetches only its current page. The list shown at the (first) reload stays until the first page lands (abcc609).

Tests

Each new test fails without its fix:

Status

🤖 Generated with Claude Code

@matej21

matej21 commented Nov 4, 2026

Copy link
Copy Markdown
Member Author

Review round 1: independent Claude agent (Claude Code, general-purpose subagent, Opus 5.5), reviewed 09ef650..b1bbe2f.

Verdict: changes

  • Major: DataViewNonEmpty / DataViewEmpty render only on loaded. Since 72c97cc every refetch reports refreshing, so the default bindx-ui grid (DefaultDataGridLayout and the auto-table body, both wrapped in DataViewNonEmpty) unmounts its table, header and filter popovers on every page, sort, filter change and reload.
  • Minor: in an infinite-scroll list, reload() refetches only the current page; the earlier pages stored by DataViewInfiniteLoadProvider stay stale.
  • Minor: the provider keys its stored pages on filtering.resolvedWhere, which leaves out a view's static filter prop. SelectDataView is safe, because its page reset moves to offset 0. A DataGrid with this provider whose filter prop changes at offset > 0 would join old pages to the new one.

Checked, no issue: the render-phase paging.first() (own state, idempotent, string key), the two-call guard vs IntersectionObserver closures, races between list and count, the other usePagingTotalCount callers, no type escapes, each test fails without its fix.

@matej21

matej21 commented Nov 4, 2026

Copy link
Copy Markdown
Member Author

Response to review round 1

  • Major, DataViewNonEmpty unmounting during refetch: fixed in 8eb0e54. DataViewNonEmpty renders on loaded or refreshing while rows are shown. DataViewEmpty still waits for loaded. The new test fails without the fix.
  • Minor, reload() in an infinite list: fixed in c9e2d8a. The provider gives its children a reload that returns to the first page and refetches it. The new test fails without the fix.
  • Minor, static filter missing from the provider's key: won't fix in this PR.
    • SelectDataView, the only view that uses the provider in bindx-ui, resets to the first page on any filter change, which drops the stored pages.
    • A DataGrid does not reset its page when its filter changes at all. That gap is broader than infinite load and needs a query identity on the DataView context; a separate change if wanted.

@matej21

matej21 commented Nov 4, 2026

Copy link
Copy Markdown
Member Author

Review round 2: a fresh independent Claude agent (Claude Code, general-purpose subagent, Opus 5.5) reviewed the follow-up commits 8eb0e54 and c9e2d8a and the regression they address.

Verdict: changes

  • Major: DataViewEmpty still renders only on loaded, so an empty grid's "No results" disappears and comes back on every refetch (search, sort, reload): the state is refreshing with 0 rows. The root cause is that useLoaderState reports refreshing both for "stale rows on screen" and for "loading with nothing on screen".
  • Minor: during reload() of an infinite list, the list shows the stale second page (t051–t100) until the first page lands, because first() drops the stored pages while the view still holds the old offset's rows.
  • Minor: HasManyDataGrid resets to an empty loading state on every page, sort or filter change. That was already the case before this PR, but it still unmounts the table and contradicts "the previous rows stay on screen".
  • Minor: the provider's wrapped reload reaches only its descendants. A reload trigger outside the provider refetches only the current page. No bindx-ui consumer does this.

Checked, no issue:

  • first() and reload() batch into one list query plus one count query.
  • Reload when already on page 0.
  • Identity stability of the context value.
  • The two-call guard.
  • The scroll observer that renders only on loaded.
  • Export, pagination and toolbar do not depend on the loader state.
  • Both new tests fail without their fix.
  • No type escapes.

matej21 and others added 7 commits November 4, 2026 17:53
reload() in DataGrid, HasManyDataGrid and SelectDataView bumped a counter
that nothing read, so the toolbar reload button and useDataViewReload()
left stale rows on screen.

DataGrid and SelectDataView fold the counter into the list's query key
and bump the paging's count refresh token. HasManyDataGrid re-runs its
relation query, which also carries the total, and keeps the rows on
screen while the same query loads again.

The loader state is now derived in render from the list result instead
of an effect, and reports `refreshing` while the rows on screen answer an
earlier query (stale-while-revalidate). Before, a refetch kept the state
at `loaded`, so the reload button never spun and the refreshing overlay
never showed.

Fixes #124

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SelectDataView kept the page index of the previous search, so a search
narrowed after scrolling went out at the old offset and showed nothing
until the count caught up. It now resets paging to the first page when
its filter (search and static filter) changes, during render, so no
query is sent at the old offset.

The paging also judged `hasNext` by a counted total that can still
describe the previous search while the new count loads, so the sentinel
of a short list loaded an empty page. usePagingTotalCount now lets a
current page shorter than the limit set the total: such a page ends the
list whatever the count says.

Fixes #145

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The provider kept `pages` in state but never filled it, and rendered the
view's `items`, which hold only the current page, so "load more"
replaced the rows. `isLoadingMore` was set once the offset moved and
never reset, so no page past the second was ever requested: no
bindx-ui select could reach options past the first 100.

loadMore now stores the current page and moves the view to the next
one; two calls before the next render request one page. Children get a
data view whose `items` are the stored pages followed by the current
one (each row once), so DataViewEachRow, the keyboard handler and the
highlight indices see the whole list. Stored pages belong to one query
(filter, order, page size) and to an unbroken run of offsets from the
first row; anything else shows only the current page. Loading follows
the view's loader state, so it ends when the page lands, empty or not.

Fixes #146

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eshing

Since views report `refreshing` during a stale-while-revalidate refetch,
DataViewNonEmpty, which rendered only on `loaded`, unmounted the default
grid table (header, filter popovers, rows) on every page, sort, filter
change and reload. It now also renders on `refreshing` while rows are on
screen. DataViewEmpty still waits for `loaded`, so a view without rows
does not flash its empty state while it loads.

Refs #124

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The view refetches only its current page, so reload() in an
infinite-scroll list kept the earlier pages the provider had stored.
The provider now hands its children a reload that moves the view back to
the first page and refetches it.

Refs #146

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… screen

The loader state reported `refreshing` both while stale rows were shown
and while a view loaded with nothing to show. So DataViewEmpty, which
waits for `loaded`, hid an empty grid's "No results" on every refetch,
and letting it render on `refreshing` would have flashed it during a
load with no result yet.

- `refreshing` now means the rows on screen answer an earlier query; a
  load with nothing to show is `initial`.
- DataViewEmpty, like DataViewNonEmpty, stays on screen while
  refreshing.
- HasManyDataGrid keeps the previous rows for any refetch (page, sort,
  filter, reload) instead of clearing them, and derives the refetch in
  render from the request key, so its loader state never lags behind
  the rows by a render.

Refs #124

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A reload moved the view to the first page while it still held the rows
of the last offset, so the list showed only its last page until the
first one landed. The provider now keeps the list shown at the reload
until the first page lands (or fails), and drops its stored pages.

Documents that only the provider's descendants get its reload.

Refs #146

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@matej21

matej21 commented Nov 4, 2026

Copy link
Copy Markdown
Member Author

Response to review round 2

The branch was rebased onto current main, which now includes #161 and #163. The SHAs below are post-rebase. Earlier commits map as 8eb0e54 → 7b7049a and c9e2d8a → 6431851.

  • Major, DataViewEmpty hidden on every refetch: fixed in eb4eeb9.
    • The root cause is fixed: refreshing now means only "the rows on screen answer an earlier query". A load with nothing to show is initial.
    • DataViewEmpty stays on screen while refreshing, like DataViewNonEmpty.
    • Test: an empty grid keeps its empty state across reload.
  • Minor, infinite list shows its last page during reload: fixed in 3dd3746. The provider keeps the list shown at the reload until the first page lands or fails, and drops its stored pages. The test asserts the list is unchanged right after reload.
  • Minor, HasManyDataGrid unmounts the table on page, sort or filter change: fixed in eb4eeb9.
    • The previous rows stay for any refetch.
    • The refetch is derived in render from a request key (query options plus reload counter), so the loader state never lags behind the rows by a render.
    • Test: the next page keeps the previous rows on screen while it loads.
  • Minor, the wrapped reload reaches only descendants of the provider: documented in the provider's JSDoc (3dd3746). No bindx-ui consumer places a reload outside it.

Each new test fails without its fix. Local checks on 3dd3746's tree: typecheck clean, 2542/2542 tests, lint 0 errors.

@matej21

matej21 commented Nov 4, 2026

Copy link
Copy Markdown
Member Author

Review round 3: a fresh independent Claude agent (Claude Code, general-purpose subagent, Opus 5.5) reviewed 6431851..3dd3746, i.e. eb4eeb9 and 3dd3746.

Verdict: changes

  • Major: HasManyDataGrid's requestKey covers only the query options and the reload counter, not the parent, the relation or the selection. Switching <Entity by> p1 → p2 → p1 (cached, grid stays mounted) shows p2's rows as loaded on p1. isPageCurrent has the same gap (older). Separately, the effect depends on the setup.selection object, which is new on every parent render, so each parent re-render refetches the grid (older; now silent instead of refreshing).
  • Major: a second reload() while the first one is loading snapshots joinedItems, the stale current page only, so the infinite list shrinks from 100 to 50 rows until the load finishes.
  • Minor: HasManyDataGrid writes the response with setEntityData(..., true), which overwrites local edits. Rows now stay editable during page, sort and filter refetches, so an edit to a stale row can be lost. Use refreshServerData, as useEntityList does.
  • Minor: the JSDoc of DataViewLoaderState / DataViewLoaderState props still describes refreshing as "after initial load".
  • Minor: test gaps. The HasManyDataGrid test reads the state after effects ran, so it doesn't prove there is no lagging render. Nothing tests a retry after an error being initial.
  • Info: on a retry after an error, the toolbar reload icon no longer spins; the static loader shows instead. Acceptable.

Checked, no issue:

  • bindx-ui consumers (datagrid loader, layout, auto-table, toolbar, select list, column filters).
  • Stale HasManyDataGrid rows are never isPageCurrent for another page or sort, so no total is inferred from them.
  • Export uses the current query.
  • Highlight, abort and race ordering.
  • The infinite reload render-phase guard, an empty or failed reload, and the interaction with loadMore.
  • Each new test fails without its fix.
  • No type escapes added.

matej21 and others added 3 commits November 4, 2026 18:15
…tion

HasManyDataGrid keeps the previous rows on screen while a refetch
loads, but its request key covered only the query options. When the
parent changed (p1 -> p2 -> p1 with the grid kept mounted), the rows of
the other parent were shown as `loaded`, and their count fed the
current parent's total.

- Rows are kept only for the relation (parent, field, selection) they
  were loaded for; for another one the grid loads as `initial`. The
  same check gates the page that feeds the total.
- The data effect follows the request key, which covers the
  selection's content, instead of the selection object, which is new on
  every parent render and refetched the grid each time.
- Responses are applied with refreshServerData, as useEntityList does,
  so a refetch no longer overwrites edits made to rows still on screen.

Refs #124

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A second reload() before the first page landed snapshotted the joined
list, which by then held only the stale current page, so the infinite
list shrank to one page until the load finished. The list shown at the
first reload is kept.

Refs #146

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`initial` is now any load with no rows to show, including a retry after
a failure, and `refreshing` means the rows shown answer an earlier
query. Documents the states on DataViewLoaderState and the loader state
components, and covers the retry case with a test.

Refs #124

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@matej21

matej21 commented Nov 4, 2026

Copy link
Copy Markdown
Member Author

Response to review round 3

  • Major, another parent's rows shown as loaded: fixed in 844bde1.
    • HasManyDataGrid keeps rows only for the relation they were loaded for (parent type and id, field, target, selection content). For another relation it loads as initial.
    • The same check gates the page that feeds the total, so isPageCurrent is covered too.
    • The data effect now follows the request key instead of the setup.selection object, so a parent re-render no longer refetches the grid.
    • Test: p1 → p2 → p1 never renders a row of the other parent.
  • Major, a second reload during a reload shrank the infinite list: fixed in abcc609. The snapshot taken at the first reload is kept. Test: a double reload keeps the 100 rows until the first page lands.
  • Minor, a refetch overwrote edits: fixed in 844bde1. Responses are applied with refreshServerData, as useEntityList does. Test: a local edit survives a reload.
  • Minor, outdated JSDoc: fixed in 31736eb. The states are documented on DataViewLoaderState and on the loader state components' props.
  • Minor, test gaps: fixed in 844bde1 and 31736eb.
    • A per-render recorder asserts that no loaded render of HasManyDataGrid shows rows of another page. It guards the in-render derivation from eb4eeb9.
    • A retry after a failure is asserted to be initial.
  • Info, toolbar icon not spinning on a retry after an error: accepted. The static loader shows instead, since there are no rows to keep.

Each new test fails without its fix, except the per-render recorder, which passes on eb4eeb9 by design. Local checks: typecheck clean, 2549/2549 tests, lint 0 errors (the same 13 warnings as main).

@matej21

matej21 commented Nov 4, 2026

Copy link
Copy Markdown
Member Author

Review round 4: a fresh independent Claude agent (Claude Code, general-purpose subagent, Opus 5.5) reviewed 3dd3746..31736eb (844bde1, abcc609, 31736eb).

Verdict: approve. No blockers and no major findings.

Minor findings:

  1. A retry after a failure reports failed for one render before initial. The error state does not record its request key, which contradicts the new docs for that one render. It applies to HasManyDataGrid and, older, to useEntityList.
  2. The fix that stops HasManyDataGrid refetching on every parent render has no regression test.
  3. The latest selection is written to a ref during render. It is safe in practice; the alternative is the content-keyed selection pattern from useEntityList.
  4. The double-reload infinite-load test depends on the mock adapter's 10 ms delay, so it may flake on a loaded machine.

Checked, no issue:

  • The scope key covers the selection content. Static filter and column changes behave as intended.
  • The effect's dependencies are complete; aborts and late responses are handled.
  • INITIAL_LIST_STATE causes no memo churn, and the highlight reset is correct.
  • refreshServerData vs setEntityData(..., true): fresh entities, server existence and dirty tracking are kept, and nested relations still update. No test relied on overwriting.
  • The reload snapshot can't get stuck.
  • Each fix's test fails without it. The scope check inside the effect is redundant with the render guard, but harmless.
  • No type escapes added.

@matej21

matej21 commented Nov 4, 2026

Copy link
Copy Markdown
Member Author

Response to review round 4

Approved, no major findings. To keep the reviewed commit as the merge candidate, all four minors go to a follow-up, not this PR:

  1. Retry reports failed for one render: follow-up. It needs the request key in the error state of both HasManyDataGrid and useEntityList; the latter is shared code outside this PR's scope. The visible effect is one render of the failed state before the loader shows.
  2. No regression test for the extra refetches per parent render: follow-up (a test counting relation queries across parent re-renders).
  3. Writing the selection ref during render: follow-up, together with 2 (switch to the content-keyed selection pattern from useEntityList).
  4. Timing-dependent double-reload test: follow-up (a held-response adapter). It has passed every local and CI run so far.

@matej21
matej21 merged commit 3a6d906 into main Nov 4, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant