fix(components): bind the row three ways on the four action renderers (#4075) - #4079
Merged
Merged
Conversation
…#4075) `action:button` / `action:icon` / `action:menu` / `action:group` gated their actions on `useCondition(pred, ctx)` with the row spread flat — or, on three of the leaves, with no row at all. Only the bare-field shorthand resolved, so the CANONICAL `record.` root threw `record is not defined`; a fail-closed `visible` turned that into "hidden" and a correctly-authored predicate deleted its own button. Every declared action on `sys_approval_request` gates on `record.viewer.*`, which is how the same binding suppressed the whole server-declared approval decision set until #4077 fixed the declared-action bar. All four now bind `record.status` / bare `status` / `data.status` through one named helper (`usePredicateRecordContext`, beside `useCondition` in `@object-ui/react`) — `evalRowPredicate`'s rule, restated for this tier. `action:icon` reads its `data` prop at all; the menu/group leaves receive the row from their host; `action:bar` forwards the row into the overflow menu it builds, so a predicate no longer answers a different question because its action spilled past `maxVisible`. The evaluation entry and every site's error policy are untouched: a genuinely faulting predicate still fails closed where it did and soft where it did. A surface with no row binds nothing rather than an empty record, so a host that supplies the row through the ambient scope is not blanked out. 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
|
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 #4075
The defect, confirmed on current
mainuseCondition(pred, ctx)evaluates onnew ExpressionEvaluator({ ...scope, ...ctx }), so a context bag that is the row spread flat resolves the shorthand spelling and nothing else. The four action renderers passed exactly that — or, on three of the leaves, no row at all.action:buttondataprop)action:icondataprop was never read, and landed in the props spread onto the DOM buttonaction:menuitemundefined)action:groupinline + dropdown leavesrecord.is not a mistaken spelling, it is the canonical one:ExpressionEvaluator's CEL path binds it (bag.recordas the record namespace,ExpressionEvaluator.ts:276-291),evalRowPredicatebinds it on the record header, list rows, the row kebab and conditional formatting, and the server enforces with it. Under a root-only bagrecord.viewer.can_actdoes not read as false — the legacy evaluator throwsrecord is not defined, and a fail-closedvisibleturns that throw into "hidden".The issue's premise holds on current
main(re-verified post-#4070/#4077; the line numbers moved, the fault did not). Measured as the pre-fix baseline of the new suite: 30 red / 38 green, and the red cases land on different polarities per site, because each site has a different error policy — a detail the card could not predict and the suite states out loud:visible(action:button,action:menu): the holdingrecord.*case is red (the throw hides a gate that held). Its failing sibling is green for the wrong reason — hidden either way — so both polarities are asserted and labelled.action:icon/action:groupvisible, everydisabled/enabledleg): the failing case is red, because the throw returns the fail-softtrue— shown, or greyed out for everybody.Route taken: route 1 (bind in place), not route 2 (move onto
evalRowPredicate)The card left this open and asked for the reasoning. Route 2 was rejected after reading the pins, not on cost:
useConditionandevalRowPredicateare different evaluation entries, not two spellings of one.useConditionroutes a bare string to the legacy JS evaluator and honours the${…}template dialect;evalRowPredicatetreats a bare string as CEL and only falls back to legacy for a legacy-dialect source. Moving the renderers across changes which engine reads every existing predicate.action-empty-predicate-scope(「空谓词」在三处有三种范围:disabled: { dialect: 'cel', source: '' } 仍被判成已声明的门 → 永久置灰(#3842 修完后的残留,需先裁) #3850, what "empty" means per key),action-template-predicate-gate(toPredicateInput 对已经是${…}模板的字符串二次包裹,动作面的谓词一律判成 true(visible 永远显示 / disabled 永远置灰) #3871, the${…}spelling and the per-site fail-closed/fail-soft table),action-member-visible-gate(components/action/**: 同一真值可见性门在动作面上未收口 ——action:group/action:menu成员动作声明visible: false照样渲染（#3758 的同族部分） #3812) andaction-disabled-declared-gate(动作disabled的「已声明」判定用!= null,disabled: ''把按钮永久置灰(#3492 同族的另一半 predicate,探针实证) #3842/disabled 的「已声明」判定还有五处停在 != null(action-icon / action-group ×2 / action-menu / record-quick-actions),#3842 只修了两处 #3849). Route 2 rewrites their subject.So this follows #4077's merged precedent — with the one-implementation preference (#3367, #3842) honoured where it actually applies: the three-way binding is written once, as
usePredicateRecordContextin@object-ui/reactbesideuseCondition, not copy-pasted into four files.What changed
packages/react/src/hooks/useExpression.ts— newusePredicateRecordContext(record). Bindsrecord.status/ barestatus/data.status, withrecordanddatawritten after the spread so a row carrying a field namedrecordcannot shadow the root.evalRowPredicate's rule and precedence, restated for this tier.action:iconreads itsdataprop at all;ActionMenuItem,InlineActionButtonandDropdownActionItemtake an optionalrecordprop their hosts now pass.action-bar.tsx— forwards the row into the overflow menu it builds, not just to its inline members. Without it an action's predicate answered a different question purely because it spilled pastmaxVisible— andmobileMaxVisibledefaults to 1, which made the verdict a function of the viewport.Deliberately unchanged: the evaluation entry, and each site's error policy. A genuinely faulting predicate still fails closed on
action:button/action:menuvisibleand still fails soft elsewhere — pinned in both directions so the binding fix cannot be read as having quietly changed the policy too. That fail-softvisibleasymmetry is pre-existing and already recorded in #3871's table; it is not this PR's to decide.One regression the existing suite caught, and the rule it produced
The first cut bound an empty record when no row was present. That blanked out a
recordsupplied through the ambient predicate scope — a legitimate host pattern, and howaction-group-dropdown-visible.test.tsxdrives these leaves. Two of its cases went red.The rule now: no row → bind nothing; only a row that is actually present may shadow the scope. "This surface has no row of its own" and "this surface's row is empty" are different statements. Both directions are pinned in the new suite (ambient
recordstill resolves with nodataprop; an explicitdataprop still wins over an ambient one).Verification
action-record-predicate-root.test.tsx— 4 renderers x 3 roots x 2 polarities + the nestedrecord.viewer.can_actshape + per-site fault cases + the ambient-scope pair. Before the fix: 30 failed / 38 passed. After: 70 passed. Every "not rendered" assertion carries an ungated companion, so a green can never mean the host itself vanished.packages/components+packages/react+packages/app-shell+packages/plugin-grid— 519 files, 4979 passed, 1 skipped, 0 failed.type-checkon both touched packages: Done.eslinton the touched surface: 0 errors.check:control-bytes,changeset:check: OK.origin/main(picking up objectui: 登录页在无会话时仍拉 /meta/*，登录前刷 30 条 401 报错，且日志打成 [object Object] 无法定位 #4042's app-shell / HTTP-logging work — disjoint packages) and re-ran the action + react suites: 731 passed.Changeset:
.changeset/action-record-predicate-root-4075.md(patch on@object-ui/componentsand@object-ui/react).Out of scope, noted not fixed
DeclaredActionsBar(app-shell) still carries its own inline copy of the three-way binding from #4077. It is correct and identical today, and migrating it ontousePredicateRecordContextwould retire the duplicate — but it is not this card's surface. Filed as #4080 (observation-class; it also records the one semantic difference the migration must preserve).Generated by Claude Code