fix(app-shell): all CEL-hosting inspectors block Save on parse faults (#4527) - #4547
Merged
Merged
Conversation
…#4527) CelPredicateField has always reported its lint verdict through onLintChange, but ConditionBuilder and ConditionalFormattingEditor passed no listener, so a predicate that does not parse rendered its inline error and Save still saved -- the objectui#4306 defect, one editor family over. Both editors gain an optional callback surfacing their blocking-error count (severity 'error' only), and the inspectors above them aggregate and report through MetadataInspectorProps.onBlockingIssuesChange, the channel PR #4536 shipped. PageBlockInspector gates on a page block's visibleWhen; ViewVariantInspector gates on a view's conditionalFormatting rules, forwarded through ViewInspector on the scoped path. Counts are DERIVED from what they describe, never repaired by reset effects. ConditionBuilder's CEL editor exists only in raw mode and can vanish while its last verdict was an error -- the adopt effect flips back to rows in the same commit, unmounting it and cancelling its pending lint -- so the count is read as 0 whenever that editor is not mounted. ConditionalFormattingEditor keys a per-rule map and counts only rules that still exist: a shared counter would let whichever rule linted last overwrite the others, and a deleted rule's remembered error would wedge Save shut with no editor left to fix it in. Three sites named in the report are NOT wired: HookDefaultInspector, ActionDefaultInspector and the view's home panel are MetadataDefaultInspectorProps components whose contract has no blocking-issues member, and widgets.tsx's condition widget is a SchemaForm widget rather than an inspector. Wiring those needs a second contract decision plus edits to the hosts, so they are escalated rather than guessed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Collaborator
Author
|
PM step-7 复核 — ACCEPT（本 PR 为 #4527 的第一批,卡保持开放）(session_017Qqyix2QcnpUC9XeYVDzx3)
Phase-2 ruling is posted on #4527 — the same dev continues. Auto-merge armed (squash), landing verified per the merge-queue discipline. Generated by Claude Code Generated by Claude Code |
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 #4527.
Part of, notFixes: the card names five inspectors and this wires two of them. The other three cannot reach the shipped contract at all, which is a decision for the PM rather than something to guess at — see "What is NOT wired, and why" below.What was wired
CelPredicateFieldhas always reported its verdict throughonLintChange; these two shared editors passed no listener, so a predicate that does not parse rendered its inline error and Save still saved — the #4306 defect, one family over. Both editors gain an optional callback surfacing their blocking-error count (severity: 'error'only, matching the RLS editor and #4306), and the inspectors above them aggregate and report throughMetadataInspectorProps.onBlockingIssuesChange, the channel PR #4536 shipped.ConditionBuilder(raw-expression mode)PageBlockInspector(visibleWhen)ResourceEditPageConditionalFormattingEditor(one condition per rule)ViewVariantInspector(conditionalFormatting), forwarded throughViewInspectorResourceEditPageCounts are derived, never repaired by reset effects (#4527 ruling item 2, mirroring #4536):
ConditionBuilder's CEL editor is mounted only in raw mode; the row builder has noCelPredicateFieldat all. The adopt effect flips the builder back to rows in the same commit when an externally-changed value round-trips as a simple predicate, which unmounts the editor and cancels its pending debounced lint — nothing will ever report0for it again. The count is therefore read as0whenever the raw editor is not mounted, or Save wedges shut with no editor on screen to fix it.ConditionalFormattingEditorkeys a per-rule map and counts only indices below the current rule count. One shared counter would let whichever rule linted last overwrite the others, so fixing one of two broken rules would hand back a writable Save while the other was still malformed; and a deleted rule's remembered error would wedge Save.ViewInspectorforwards the host callback on the scoped path. Without that one hop the channel stops one component short of the editor and the whole wiring is inert, so it is pinned by its own test.Red-first
Predicted the split in writing before running, then ran the new suites against unfixed code. Predicted red signature (inherited from #4536): the reporter is never called, so the count a host would hold is
undefinedand every assertion fails, the0cases included.Measured, unfixed — 16 failed / 2 passed across the four editor+inspector suites:
The 2 that passed are the "the prop stays optional" cases, predicted green on both sides. After the fix: 20/20 green including the host suite.
Reverse verification took the fix out with
git diff+git checkout --(nevergit stash— objectui#3430), re-ran all five suites, and restored with a sha256 check on all five files. Direction was the ordinary one, as predicted: 5 files failed, 18 failed / 2 passed with the fix removed.Full regression over the metadata-admin + studio-design trees: 173 files, 1691 passed, 1 skipped, 0 failed — both #4536 suites included and untouched.
Host-level coverage
ResourceEditPage.celGate.test.tsxis new: no test in this repo renderedMetadataResourceEditPagebefore (the console suites stub it out), so this host's Save gate had never been exercised end to end. It drives the real registeredPageBlockInspector, the realConditionBuilderand the real host gating; only the page canvas is stubbed, and only to emit the selection, since turning a click into aMetadataSelectionisPageBlockCanvas's own tested concern.The ruling also asked for a
StudioDesignSurfacehost case. That is not possible within this card's surface and no test claims it: the design pillar's scoped inspector render passes noonBlockingIssuesChangeat all (onlyDataPillar's object inspector does, from #4536), andStudioDesignSurface.tsxis on the do-not-touch list. So on the Studio design surface this fix is inert until that render site is wired — recorded here rather than papered over.What is NOT wired, and why
Measured on post-#4536
main: there are two inspector registries and #4536 extended only one.MetadataInspectorPropscarriesonBlockingIssuesChange;MetadataDefaultInspectorPropshas no such member. Repo-wide there are exactly two host call sites passing the channel —ResourceEditPage(scoped path) andStudioDesignSurface'sDataPillar.PageBlockInspectorMetadataInspectorPropsViewVariantInspector(scoped, viaViewInspector)MetadataInspectorPropsHookDefaultInspectorMetadataDefaultInspectorPropsActionDefaultInspectorMetadataDefaultInspectorPropswidgets.tsxcondition widgetWidgetProps— aSchemaFormwidget, not an inspectorViewVariantInspector(home path, viaViewDefaultInspector)MetadataDefaultInspectorPropsWiring those needs
MetadataDefaultInspectorPropsextended and the two forbidden host files edited, plus three further hosts nobody has modelled that render default inspectors and own their own Save:ObjectSettingsPanel,ObjectActionsPanel,ObjectHooksPanel. That is a contract decision, so it is escalated rather than guessed. A sixthConditionBuilderconsumer also exists that the card does not list:studio-design/ObjectValidationsPanel.Changeset
@object-ui/app-shell: patch, by.d.tsmeasurement both ways (dist/andtsconfig.tsbuildinfocleared between builds).dist/index.d.tsis byte-identical before and after; only four module-local per-file.d.tschange, and the package declares no subpath exports, so none of it is consumer-reachable. (#4536 graded minor because it added a member to the exportedMetadataInspectorProps; this card adds none.)Gates
Both tsc passes green (
tsc --noEmitandtsc -p tsconfig.test.json). ESLint net zero new warnings, measured both ways on the same scoped set: 37 with the fix, 37 onorigin/maincontent — derivation rather than effects, so no set-state-in-effect warnings. New test files lint clean.check-control-bytesOK,check:phantom-depsOK, changeset gates OK.Surface
Touched: the two editors,
PageBlockInspector,ViewVariantInspector,ViewInspector, five new test files, one changeset.ViewInspector.tsxis the one file outside the card's listed surface — a single forwarding line, without which theViewVariantInspectorhalf cannot work; declared rather than silent. Not touched:StudioDesignSurface.tsx,ResourceEditPage.tsx,inspector-registry.ts,ObjectFieldInspector.tsx,PermissionAdvancedFacets.tsx,PermissionMatrixEditor.tsx,permission-slice.ts, and nothing undercontent/docs/releases/. Noclient.get(call site was touched (#4271 is in flight).Generated by Claude Code