Repository navigation
fix(app-shell): a record-scoped ConditionBuilder advertises the roots its host binds (objectui#9645) - #9854
Merged
os-sales merged 2 commits intoSep 18, 2026
Conversation
… its host binds
objectui#8167 made the raw CEL editor lint in its mount's scope; its
autocomplete kept offering the engine's whole default advertisement for that
scope. On a hook `condition` the server binds `{ record, previous }` and throws
`HookConditionError` on an unevaluable one, so an accepted `os.user.id`
suggestion linted green and was refused at runtime.
A `scope="record"` mount now advertises RECORD_CONDITION_ROOTS; a mount that
binds more declares it through the new `roots` prop, as ObjectFieldInspector
and ConditionalFormattingEditor already do for their own surfaces. A mount that
declares no scope forwards `undefined` and is unchanged. Suggestions only — the
accept set is the engine's and is untouched.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xm4WFhEe5mwcgyqHjxR2hn
…dition-builder-roots-override
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-sales
marked this pull request as ready for review
September 18, 2026 10:54
os-sales
deleted the
claude/issue-9645-condition-builder-roots-override
branch
September 18, 2026 15:18
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
2 participants
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 #9645
Clause-②: no
What this changes
ConditionBuilder's raw CEL editor forwardedscopetoCelPredicateFieldand nothing else, so its autocomplete offered whateverintrospectScopeadvertises for that scope — every root ANY predicate site may see. objectui#8167 had already made the editor LINT atrecordon this mount; the suggestions never followed.A mount that declares
scope="record"now advertisesRECORD_CONDITION_ROOTS(record,previous). A mount that binds more declares it through the newrootsprop — the shapeObjectFieldInspector(FIELD_RULE_ROOTS,FORMULA_ROOTS) andConditionalFormattingEditor(ROW_PREDICATE_ROOTS) already use for their own surfaces. A mount that declares noscopeforwardsundefinedand is unchanged.Suggestions only.
rootsfeedsintrospectCelScope, which builds the autocomplete catalog; the lint hint is built without it. So the accept set is untouched: every spelling that linted clean before still lints clean, nothing the server does moves, andHookConditionErrorstays exactly as loud as objectstack#4775 made it.Both halves re-measured on this base, not taken from the card
introspectScope('record').roots, installed@objectstack/formula17.4.0record,previous,input,os,current_user,user,varspackages/objectql/src/hook-wrappers.ts, at625db0eExpressionEngine.evaluate(expr, { record: record ?? {}, previous }), then a thrownHookConditionErrortwo lines laterBoth match the card: seven advertised, two bound. The defect is the size the card reported.
The conflation this deliberately did NOT make
ConditionBuilderalready carried a roots concept —REFERENCE_ROOTS, documented as the roots a value typed into the VALUE BOX may plainly reference rather than be quoted as literal text. It is a different population and the two do not coincide, which is measurable rather than a matter of taste:REFERENCE_ROOTScarriesorgandparent, and the engine's record scope advertises neither. Reusing it as the advertised list would have re-introduced the objectui#8155appfailure — advertising a root this editor's own linter refuses. Measured at this base:The two lists stay separate, each with its own docblock saying which question it answers, and a case in the new suite reddens if anyone merges them.
Why one declared default plus a prop, and not a single hardcoded list
ConditionalFormattingEditorcould hardcodeROW_PREDICATE_ROOTSbecause it IS the surface.ConditionBuilderis not: six mounts reach it, and their hosts bind different sets.conditionand the validation rule are evaluated on the SERVER, which bindsrecordandprevious;visible/disabledis evaluated in the browser, whereuseConditionmerges the row over app-shell's ambient predicate scope —buildExpressionScopepublishescurrent_user,user,ctx,os,features.Nothing a mount passes distinguishes the two, so the component cannot derive per-mount truth; declaring it is the mount's job, which is what the
rootsprop is for. The default is therefore the set every host binds, and the asymmetry decides the direction: over-advertising on a server-evaluated mount costs the author their write, while under-advertising costs a client mount an OFFER and never a spelling, because the accept set is untouched and the offer is restored by one declared prop. That cost is real and is listed under Acceptance notes rather than left to be discovered.Evidence
Red before, green after
The pin fails with the forwarding removed and passes with it, run from the committed state, mutation and restore both proven on disk by
ablation-replace:The direction is worth stating precisely: only the five DOM absence cases flip. The derived pins and the two controls stay green under the ablation, because they read the advertised list and the engine rather than the forwarding — they fence a different drift and would redden on a different edit.
Gates
All exit codes captured before any pipe. Everything except the full suite re-ran on the merged head
f65380b3; the full app-shell suite ran at3b811573, and the merge that followed it touchedplugin-formonly.pnpm exec vitest run packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.mountRoots.test.tsxpnpm exec vitest runover the 30 test files namingConditionBuilder/ConditionWidget/CelPredicateFieldpnpm exec vitest run packages/app-shell/pnpm --workspace-concurrency=2 --filter '@object-ui/app-shell^...' buildpnpm --filter @object-ui/app-shell type-checktsc --noEmitANDtsc -p tsconfig.test.json, so the new test file is type-checked too)pnpm exec eslinton the two changed files,--format jsonnode scripts/check-changeset-presence.mjsnode scripts/check-changeset-no-major.mjs·check-changeset-fixed.mjs·check-changeset-claims.mjspnpm check:new-line-citationspnpm check:control-bytespnpm check:test-path-rootsThe lint narrowing, declared. The repo-wide population is
pnpm lint(turbo run lint), which is CI's run and was NOT taken here. Locally eslint ran over exactly the 2 files this diff touches (count read from the--format jsonoutput, not asserted). That narrowing excludes nothing:eslint.config.jsdeclares noparserOptions.projectand noprojectService, so linting is not type-aware and this diff cannot move the verdict on any file it does not contain.ConditionBuilder.tsxends with 0 errors and 3 warnings. One (react-hooks/exhaustive-deps) is at this base already. The two new ones arereact-refresh/only-export-components, raised by exporting a constant from a component file — byte-for-byte the pairConditionalFormattingEditor.tsxalready carries forROW_PREDICATE_ROOTS, and--max-warningsis deliberately unset repo-wide, so they fail nothing. Said here rather than left for a reviewer to find.Acceptance notes
scope="record", because the component has one forwarding site. The client-evaluated mounts (an action'svisible/disabled) stop being OFFEREDos/user/current_user, which their host really does bind. They still lint clean when typed, and onerootsdeclaration at that mount restores the offer — the escape hatch is pinned by a case in the new suite rather than promised. Worth a follow-up card; it is not this one's to land under the declared file surface.CONTEXT_SUBJECTSin this same file offersorg.idas a row SUBJECT at every mount, and the record-scope lint hard-errors on it with the remedyrecord.org— the objectui#8155appshape, in the subject dropdown rather than the autocomplete.orgis not in@objectstack/formula'sSCOPE_ROOTSat all. It also offersuser.*, which no server-evaluated mount binds. Both reach a different control than this card's, and repairing them changes what the row builder EMITS, so they are not folded in here. Dedup words:ConditionBuilder CONTEXT_SUBJECTS·org root unbound·subject dropdown unbound root·SCOPE_ROOTS org.claude/issue-9464-admin-scope-invalid-drafttouchesPermissionAdvancedFacets.tsx, its test,i18n.tsand a changeset. It does not touchConditionBuilder.tsx.ConditionBuilder.tsx, one new sibling test beside it, one changeset.HookDefaultInspector.tsxis NOT edited — the trap closes through the record-scope default, which is why that default exists rather than a per-mount declaration at the hook mount.Generated by Claude Code