fix(app-shell): let a ConditionBuilder mount declare its CEL lint scope (objectui#8167) - #9494
Merged
Merged
Conversation
The builder's raw expression editor had no `scope` prop, so every mount fell
through to `celAuthoring`'s `hint.scope ?? 'flattened'` default and no caller
could override it. A bare `status == 'done'` typed into an action's "Visible
when" therefore linted clean and never matched — `usePredicateRecordContext`
binds `record` and nothing else, and Phase 2 of the row-predicate canon retired
the bare shorthand on runtime record surfaces.
`scope` is now an optional prop forwarded verbatim to `CelPredicateField`;
omitting it forwards `undefined`, so every mount that passes nothing keeps the
engine default byte for byte. It is deliberately not derived from
`subjects.fieldPrefix`, which would silently decide the mounts whose tier is
still open.
Three mounts declare `record`: an action's `visible` and `disabled` (named as a
row surface by `rowPredicateCanon.ts` in its own words), and an object
validation rule's `condition` (the server's rule validator binds
`{ record, previous }` and fail-closes on an unevaluable predicate, so a bare
reference there rejected every write while the editor stayed green).
The page-block, hook and schema-driven ConditionWidget mounts are left passing
nothing on purpose — their tier is an open question and an explicit value would
be a claim about it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011QreXiyMEqKLN4U5daMPVa
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
|
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.
Part of objectui#8167 — the structural half plus the three SETTLED mounts. The card stays open: three mounts still carry an unanswered tier question and are deliberately untouched here (see the table below).
The defect
ConditionBuilderhad noscopeprop at all — not on its interface, not at its ownCelPredicateFieldmount. Every mount therefore fell through tocelAuthoring's own default, whose spelling ishint.scope ?? 'flattened', and no caller could override it. So a barestatus == 'done'typed into an action's Visible when linted CLEAN and then never matched:usePredicateRecordContextbindsrecordand nothing else, and objectui#5741 Phase 2 retired the bare shorthand on runtime record surfaces.rowPredicateCanon.tsin@object-ui/corenames an action renderer'svisible/disabledas a row surface in its own words.This is objectui#7727's defect at a component objectui#7727 does not touch. PR objectui#8164 turned the same defect at the conditional-formatting mount; this is the same turn at two more surfaces.
What changed
scopeis now an optional prop onConditionBuilder, forwarded verbatim toCelPredicateField. Omitting it forwardsundefined, so the engine hint is exactly what it was and every mount that passes nothing is unchanged. It deliberately does not derive the scope fromsubjects.fieldPrefix— that would silently decide the mounts whose tier is still open.scope="record"on an action's Visible when / Disabled when (ActionDefaultInspector). Conformance, not taste: the canon names those two by name.scope="record"on an object validation rule'scondition(ObjectValidationsPanel). See the departure note below — the basis is not the one the dispatch gave.The row builder at all three mounts was already emitting
record.+ field name. This only makes the raw expression editor agree with the rows its own sibling mode produces.The dispatch justified mount 3 with: "the draft-level validator for that same surface already runs
scope: 'record'(clientValidation), so the editor and the validator currently disagree about the same expression." Verified on disk, as instructed, and it does not hold:clientValidation's twoscope: 'record'lint calls live invalidateObjectFieldRules, which lintsvisibleWhen/readonlyWhen/requiredWhenandformulafieldexpressions. That is a sibling surface — the word the filing card itself uses; the dispatch upgraded it to "that same surface".conditionat draft level at all. So there is no editor-vs-validator disagreement about that expression to point at.The change is still right, on a stronger and independent basis found on disk:
objectql's rule validator evaluates ascript/cross_fieldconditionwith{ record, previous }and nothing else (itscheckPredicate), and since objectstack#4649 an unevaluable predicate there is fail-CLOSED — it rejects the write. A bare reference authored in this editor therefore does not merely fail to match: it makes every write to the object fail, while the editor linted it green.ObjectValidationEngine.scopeForin@object-ui/corebinds the bare field names andrecord, and says in its own comment that objectui's own rules have always been written bare. That engine is deprecated and deliberately unwired —validation-engine-stays-unwired.test.tsreddens if a production module imports it — so it is not an authority on what an author should type.I proceeded rather than stopping because the dispatch's literal stop condition was "if the validator does not run
recordthere", and it does runrecordat both cited call sites; what is wrong is the sentence's claim about which surface those call sites serve. Flag this for the seat: if the seat intended the stop to fire on the surface-identity claim rather than the citation, revert mount 3 alone — the other two and the prop stand on their own.Two smaller transcription drifts, both harmless: the card's line addresses were taken at
c14d3a090/ff1d5eaand have since moved (theConditionBuildermount the card calls:368is:371on6d5db7b;clientValidation's:792/:806are now the comment and two calls further down). Cited by content here per AGENTS.md commandment 11.Deliberately OUT — effect-today scope recorded, as the ruling asked
All four pass no
scopeand therefore run thecelAuthoringdefault, i.e. flattened, both before and after this change:PageBlockInspectorvisibleWhenwidgets.tsxConditionWidgetHookDefaultInspectorconditionFlowNodeConfigFieldNone were "tidied" with an explicit
flattened: an explicit value is a claim, and the claim is what is unsettled.Pins — before / after, from real run output
New file
ConditionBuilder.mountScope.test.tsx, running against the real@objectstack/formula(no stub — a stub would let the suite stay green while asking the engine the wrong question, which is the failure being pinned). The red leg types the bare shorthand, not a "did the prop arrive" probe.Engine verdicts measured directly on the installed engine, both directions:
AFTER (fix in place), commit
179d4c6:BEFORE (ablation — the
scope="record"WIRING removed at all three mounts, the prop itself kept, so this measures the wiring and not the prop):Exactly the three red-leg cases fail; the three canonical-spelling cases and both controls stay green. Restoration was verified by an empty
git diff HEAD, not by an exit code, and the occurrence counts came back to 3 / 2.Controls — "unchanged" made falsifiable, not asserted. A hook guard (a real OUT mount) still lints the bare shorthand clean, and a bare
ConditionBuilderwith noscopestill lints it clean. Both go red if the default is ever changed, or derived fromsubjects.fieldPrefix.No existing pin needed updating. The whole
ConditionBuilder.*.test.tsxfamily is green unchanged; none of them encoded the old flattened behaviour at a mount this changes.Checks run locally, all on
179d4c6pnpm exec vitest run packages/app-shell/Test Files 704 passed (704)·Tests 6897 passed | 1 skipped (6898)· exit 0pnpm --filter @object-ui/app-shell type-check(tsc --noEmit && tsc -p tsconfig.test.json)eslint .inpackages/app-shell--format json, not estimated); 3023 warnings, all pre-existingcheck:control-bytes·check:new-line-citations·check:vi-mock-specifiers·check:vi-mock-inherit·check:vi-mock-override-shape·check:test-path-roots·changeset:check·check:changeset-claims·check:unreferenced-sourcescheck-changeset-presence.mjscheck-governed-queue-guard.mjs --testThe two lint warnings on the changed files (
react-hooks/exhaustive-depson the first-mount-onlyinitmemo;react-hooks/set-state-in-effectin the panel) are both on lines outside every hunk of this diff — pre-existing.Lint narrowing, declared.
pnpm lintisturbo run lintacross every package; what ran here ispackages/app-shell's owneslint .— the complete CI unit for the only package this diff touches, not a file subset. The invariance that makes that a measurement rather than a gap: this repo does not enable type-aware linting (eslint.config.jsextendstseslint.configs.recommended, and carries noprojectServiceand noproject:), so a change insideapp-shellcannot move the verdict on a file in any other package..changeset/*.mdis not in eslint's population at all. CI runs the full farm regardless.Not owed, stated so silence is not read as a pass.
scripts/pm/check-half-states.mjsis a board patrol over issue state, not a diff gate — nothing in this diff moves its input. An earlier invocation of it from the wrong path exited 1 withMODULE_NOT_FOUND; that is a mistyped path, not a red gate, and it is not reported as a measurement either way.Scope and clause 2
inspectors/ConditionBuilder.tsx,inspectors/ActionDefaultInspector.tsx,views/studio-design/ObjectValidationsPanel.tsx, one new member of theConditionBuilder.*.test.tsxfamily, and the changeset.Clause-2: nostill holds.ConditionBuilderis not exported frompackages/app-shell/src/index.tsand is not exported here; no published type moves; nothing changed about whatcelAuthoringACCEPTS — only which scope three call sites ask for.ConditionalFormattingEditor.tsx, a different file, already correct via PR objectui#8164, and untouched by this branch.Session (as prose, so it survives a body rewrite):
https://claude.ai/code/session_011QreXiyMEqKLN4U5daMPVaGenerated by Claude Code