fix(bindx-dataview): analyze the JSX a relation column renderer returns - #82
Closed
matej21 wants to merge 1 commit into
Closed
fix(bindx-dataview): analyze the JSX a relation column renderer returns#82matej21 wants to merge 1 commit into
matej21 wants to merge 1 commit into
Conversation
All three selection-collection sites in createRelationColumn invoked the
cell renderer against a collector proxy and threw the returned JSX away.
Selections were captured only as a side effect of property accesses on
the proxy, so a declarative renderer such as
{p => <HasMany field={p.tags}>{tag => <Field field={tag.name}/>}</HasMany>}
registered `tags` but never `tag.name` — the inner callback is owned by a
nested component and is never invoked during collection. Rows came back
with their nested fields missing, which forced consumers to write cells
imperatively as `p.tags.map(t => t.name.value)` purely so the proxy would
see the accesses.
Feed the renderer's return value through collectSelection() and merge the
result into the relation's own child SelectionScope. The scope matters:
the collected fields are relative to the related entity, so merging them
into the row-level selection would attach them to the wrong entity.
The merge is additive — <HasMany>/<HasOne> getSelection already register
into the child scope as a side effect of map()/$entity, and Field returns
null during collection, so nothing is double-counted. The existing
.map()-on-proxy pattern keeps working; it is guarded by a test.
buildLeaf's relatedSelection had the same defect. It feeds
extractScalarFieldNames, so a declaratively-rendered has-many stayed in
the list as a bare scalar and was handed to the fulltext filter handler
as a searchable path — a `contains` against a relation. It now carries
its nested selection and is correctly excluded.
Known gap, left alone: buildLeaf builds its proxy without a
schemaRegistry, which is not reachable from staticRender(props). At
nesting depth >= 2 a related field named like a collector built-in
(`value`, `items`, `length`, ...) resolves to the stub and is dropped.
Top level is immune - the root proxy uses an allowlist.
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 bug
All three selection-collection sites in
createRelationColumn.tsxinvoked the cell renderer against a collector proxy and discarded the JSX it returned:Selections were captured only as a side effect of property accesses on the proxy. So a declarative cell:
registered
tagsbut nevertag.name. The inner callback belongs to a nested component and is never invoked during collection, so the query came back with nested fields missing and cells rendered empty.The workaround consumers landed on is to write the cell imperatively —
p.tags.map(t => t.name.value)— purely so the proxy sees the accesses.The fix
Feed the renderer's return value through
collectSelection()(already exported from@contember/bindx-react; nothing new had to be exposed) and merge the result into the relation's own childSelectionScope.The scope is the load-bearing part: the collected entries are relative to the related entity (
path: ['members'], not['organization','members']), so merging them into the row-level selection would attach them to the wrong entity. The child scope is reachable off the collector field ref via itsSCOPE_REFgetter.The merge is additive and safe against double-counting —
HasMany/HasOnegetSelectionalready register into the child scope as a side effect ofmap()/$entity, andField.getSelectionreturnsnullduring collection, so scalars come purely from the proxy touch. Outside the collection phase the scope lookup returnsnulland the merge is a no-op.The existing
.map()-on-proxy pattern keeps working — this adds a path, it does not replace one, and two regression tests guard it.Three sites, not two
buildLeaf'srelatedSelectionhad the identical defect. Its consumer isextractScalarFieldNames→ the relation filter's fulltext handler, so a declaratively-rendered<HasMany>stayed in the list as a bare scalar and was handed tocreateFullTextFilterHandleras a searchable path — a fulltextcontainsagainst a has-many relation. It now carries its nested selection and is correctly excluded.(An earlier draft of this description claimed the bug caused blank rows in the filter popover. That was wrong:
SelectDataViewruns its own registry-awarecollectSelection, so rows were never blank. The search box is the real consumer.)Known gap, deliberately left
buildLeafbuilds its proxy without aschemaRegistry, and one is not reachable there —buildLeafruns fromstaticRender(props), outside React rendering, andpropscarries no registry. Threading one in means changing thestaticRendercontract.Measured consequence, at nesting depth ≥ 2 only:
relatedSelection(none)org => <><Field field={org.country.code}/><Field field={org.country.value}/></>country{code,id,value}country{code,id}—valuedroppedWithout a registry the nested ref takes the non-has-one proxy branch where
prop in targetwins, so a related field named like a collector built-in (value,items,length,map,errors,isDirty, …) resolves to the stub. Top level is immune — the root proxy uses an explicit allowlist. Neither this nor the related "relation touched but never entered" case affects the declarative path this PR fixes.Testing
10 new tests in
tests/react/dataview/relationColumnSelection.test.tsx, asserting on the collectedSelectionMetarather than rendered output.Negative controls, run separately per fix:
collectSelectionfixes → 3 fail (nested<HasMany>and nested<HasOne>children fields missing, in both column variants)relatedSelectionfix → 2 fail, withExpected relation "members" with a nested selection, got: members— which is itself the evidence for the fulltext bug aboveThree tests pass in both directions and are kept as behaviour locks, not dressed up as regressions: a top-level
<Field field={org.name}/>cannot fail without the fix, becauseorg.nameis evaluated while the element is being constructed, so the proxy is touched before the JSX is ever discarded. Only nested-component renderers were broken.Gates
bun test tests/react/dataview/112 pass / 0 fail ·bun run typecheckclean ·bun test --path-ignore-patterns='**/tests/browser/**'1747 pass / 0 fail / 150 files (baseline 1737 / 149; delta is entirely this PR's new tests).