fix(bindx-dataview): scope the export query the way the view loads - #105
Closed
matej21 wants to merge 4 commits into
Closed
fix(bindx-dataview): scope the export query the way the view loads#105matej21 wants to merge 4 commits into
matej21 wants to merge 4 commits into
Conversation
DataViewExportTrigger built its unpaged query from filtering.resolvedWhere only, so a grid's static `filter` prop never reached the export and the file carried rows outside the grid's visible scope. On HasManyDataGrid it went further: a root list query over the whole target entity table, with no parent constraint at all. The context now carries `fetchAllData`, and each view implements it as the query it already loads with, minus paging — DataGrid and SelectDataView as their list query with the combined filter, HasManyDataGrid as the same GET on the parent record. Exposing a filter instead would leave the next view free to forget to combine it; owning the query shape cannot be forgotten. Fixes #67 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Euwkf2wtutqvtE4YRutU5R
The reporter's repro covers the static filter on a root DataGrid. This adds the has-many half: the export must read the relation through the parent record, so it can never reach another parent's rows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Euwkf2wtutqvtE4YRutU5R
DataGridToolbarUI hard-coded <DataGridAutoExport />, and the context feeding its mobile filter chips was private, so swapping the export meant rebuilding the toolbar and losing the mobile filters toggle. Adds an `exportControl` slot (null hides it) and exports DataGridShowFiltersContext. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Euwkf2wtutqvtE4YRutU5R
Member
Author
|
Consolidated into #109, merged there. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The leak
DataViewExportTriggerbuilt its unpaged export query fromfiltering.resolvedWhereonly. The grid's staticfilterprop — the one that scopes every row the operator can see — never reached it, so the exported file contained rows from outside the grid's visible scope. Any statically scoped list ("non-archived contacts", "sessions of this program") leaked.On
HasManyDataGridit degenerated further. The grid loads its rows through a GET on the parent record with a nested relation spec, but publishesentityType: targetEntityTypeinto the context. The export took that at face value and issued a root list query over the whole target entity table, with no parent constraint at all.useDataViewFetchAllData()had the same gap.Why a context-level fetch-all, not an exposed filter
The obvious fix is to publish
combinedFilteron the context and use it in the export. That fixes today's bug and leaves the class of bug open: a filter is only correct for views whose query shape is a root list.HasManyDataGridproves it is not universal — no filter it could publish makes a root list query over the target table correct, because the parent constraint is not expressible there.So the context now carries the operation, not the ingredient:
Each view implements it as the query it already loads with, minus paging:
DataGrid— its list query withsetup.combinedFilterand the resolved orderBy, no limit/offset.SelectDataView— the same, via the shareduseListFetchAllDatahook.HasManyDataGrid— the same GET on the parent, no limit/offset. Load and export now share onebuildHasManyRelationQuerybuilder, so they cannot drift.A future view cannot reintroduce this bug by forgetting to combine a filter: it has to supply its own unpaged query or it does not compile.
DataViewExportTriggeranduseDataViewFetchAllDatastop building queries and just call the context. Their public props and return shapes are unchanged.Toolbar slot
DataGridToolbarUIhard-coded<DataGridAutoExport />with no way to replace or hide it, and theDataGridShowFiltersContextit feeds the mobile filter chips was not exported — so a consumer needing a different export control had to rebuild the toolbar and lose the mobile filters toggle.DataGridToolbarUIgainsexportControl?: ReactNode | null— default<DataGridAutoExport />,nullhides it.DataGridShowFiltersContextis now exported from@contember/bindx-ui, so a custom toolbar keeps the mobile filters toggle.Verification
tests/react/dataview/exportIgnoresStaticFilter.test.tsx— the reporter's repro, cherry-picked with their authorship. Confirmed failing before any source change (export queryfilterwas{}, notpublished).HasManyDataGridexport must read through the parent record. It asserts on what the adapter actually receives — zero root list queries on the target entity, a GET onAuthorby id whose relation field is unpaged and carries the static filter, and exported rows containing only this parent's matching row.export.tsxand fail there: the static-filter one on the empty filter, the has-many one on the root list query it issues (1, expected 0).bun run typecheck— clean.bun run test— 2028 pass, 0 fail, 212 files. (tests/browserexcluded as usual; it needs a live playground.)bun run lint— 0 errors; the 17 warnings are pre-existing and untouched.No
ascasts,any, or ts-ignores added; a couple were removed along the way (the redundantListQueryResultcasts in the export path and anas unknown[]on the has-many alias params).Fixes #67
🤖 Generated with Claude Code
https://claude.ai/code/session_01Euwkf2wtutqvtE4YRutU5R