fix(core): pass propertyKey to the of template of an array property - #949
Open
marianmoldovan wants to merge 1 commit into
Open
fix(core): pass propertyKey to the of template of an array property#949marianmoldovan wants to merge 1 commit into
of template of an array property#949marianmoldovan wants to merge 1 commit into
Conversation
`resolveArrayProperty` destructures `propertyKey` out of its parameters, so
the `...props` rest object no longer carries it. Three of the four array
resolution paths re-supply it explicitly, but the singular `of` resolution
did not:
- `of` as a tuple -> `${propertyKey}.${index}`
- per index (getArrayResolvedProperties) -> `${propertyKey}.${index}`
- `oneOf` -> `${propertyKey}.${index}` per element,
bare `propertyKey` for `oneOf.properties`
- singular `of` -> nothing
As a result a property builder used as an array's `of`, or nested inside it,
was invoked with `propertyKey: undefined`. Builders that use `propertyKey`
to locate sibling values by dot notation (the case in #659: a `reference`
inside an array of maps that needs its sibling `parent`) had to work around
this with a conditional fallback, because the builder is resolved once per
element with a usable key and once more, as the `of` template, without one.
Which key is correct for the singular `of`
------------------------------------------
`of` describes the shape of *any* element of the array, not a specific one,
so there is no index available to qualify the key with — `props.index`, when
present, belongs to the enclosing array, not to this element. Inventing a
sentinel (`example.#`) would break the dot-notation derivation that builders
do. Reusing index 0 would be a lie: it would hand the first element's value
to a template that is also used for elements that do not exist yet
(RepeatFieldBinding and ArrayPropertyPreview both fall back to the resolved
`of` via `resolvedProperties[index] ?? ofProperty`).
The bare `propertyKey` is therefore used, which is also exactly what the
`oneOf` branch already does when resolving its `oneOf.properties` templates
a few lines below. Builders now consistently receive the array key as the
base of the path in every path, and nested builders get template paths such
as `example.child` alongside the per element `example.0.child`.
One consequence worth stating: `resolveProperty` derives `propertyValue`
from `getIn(values, propertyKey)`, so a builder used *directly* as `of` now
receives the whole array as `propertyValue` for the template resolution
instead of `undefined`. That is the honest value at that key, and separating
key identity from value derivation would mean changing the public
`resolveProperty` signature. Builders that survive today already have to
guard `propertyValue`, since the template call previously passed `undefined`.
Non-builder properties are entirely unaffected: `propertyKey` is never
stored on the resolved property, it only feeds builder invocations.
`propertyKey` was already being spread into every builder call but was
missing from `PropertyBuilderProps`, forcing TypeScript users to cast. It is
now declared and documented.
Tests: extends test/resolutions.test.ts with seven cases covering the
reported nested-map shape, a builder used directly as `of`, the empty-array
case, and regression coverage for the tuple `of`, `oneOf`, plain-property
and non-array paths. The four regression cases pass before and after the
change; the three `of` template cases fail before it.
Fixes #659
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
marianmoldovan
requested review from
Copilot and
fgatti675
and removed request for
fgatti675
July 29, 2026 17:07
There was a problem hiding this comment.
Pull request overview
This pull request fixes inconsistent propagation of propertyKey when resolving array properties whose of is a property builder, ensuring builders can reliably derive sibling paths during both per-element and template resolution.
Changes:
- Pass the array’s
propertyKeyinto the singularoftemplate resolution path inresolveArrayProperty. - Extend
PropertyBuilderPropsto officially include (and document)propertyKey. - Add regression tests covering all array resolution paths (
oftuple, singularof, nested builder inof, andoneOf), plus non-array builder behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/firecms_core/src/util/resolutions.ts | Ensures the singular of template resolution passes propertyKey consistently. |
| packages/firecms_core/src/types/properties.ts | Adds propertyKey to PropertyBuilderProps and documents expected semantics for array of builders. |
| packages/firecms_core/test/resolutions.test.ts | Adds regression tests validating propertyKey/propertyValue behavior across array resolution paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+570
to
+575
| * When a property builder is used as the `of` prop of an array property, | ||
| * it is resolved twice: once per existing element, with an index | ||
| * qualified key (`my_array.0`), and once as the template for the element | ||
| * shape, with the array's own key (`my_array`). In the latter case | ||
| * `propertyValue` is the whole array, since the template is not bound to | ||
| * any element. |
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.
Fixes #659
The bug
resolveArrayPropertydestructurespropertyKeyout of its parameters, so the...propsrest object no longer carries it. Three of the four resolution paths then re-supply it explicitly and one did not:propertyKey?of`${propertyKey}.${index}`getArrayResolvedProperties)`${propertyKey}.${index}`oneOf`${propertyKey}.${index}`ofbuilderSo a builder used as an array's
ofwas invoked withpropertyKey: undefined, forcing the conditional fallback in the reporter's config. Confirmed empirically — before the fix the recorded builder keys were["example.0.child", "example.1.child", undefined].Which key value, and why
The bare
propertyKey(example), not index-qualified. This follows existing precedent rather than inventing a convention: theoneOfbranch 30 lines below already resolves itsoneOf.propertiestemplates with the bare key.Index-qualifying was rejected because the
oftemplate is used as the live property for elements that don't exist yet — bothRepeatFieldBindingandArrayPropertyPreviewdoresolvedProperties[index] ?? ofProperty. Binding the template to element 0 would hand element 0's value to newly-added elements.Also included
propertyKeywas never declared onPropertyBuilderProps, despite being spread into every builder call — so it could only be read via a cast. Now declared and documented. A reliably-present-but-untyped prop would only be half a fix.Tests
7 cases added to the existing
test/resolutions.test.ts. Three fail before the fix and pass after; four pass either way as regression guards (tupleof,oneOf, non-builder deep-equality, plain/map builders).@firecms/core: 244/247 passing (was 237/240) — 3 pre-existing failures unchanged.Known, deliberate consequence
A builder used directly as
ofnow receives the whole array aspropertyValueon the template call instead ofundefined. This is the honest value at that key; avoiding it would mean adding apropertyValueoverride to the publicresolvePropertysignature that then travels stickily through the whole template subtree. Pinned with an explicit test assertion and documented in the type's doc comment.Non-builder properties are provably unaffected —
propertyKeyis never stored on the resolved property, and a deep-equality test passes identically before and after.🤖 Generated with Claude Code