diff --git a/.changeset/10290-container-member-params-evaluated.md b/.changeset/10290-container-member-params-evaluated.md new file mode 100644 index 0000000000..e8d1f97fc6 --- /dev/null +++ b/.changeset/10290-container-member-params-evaluated.md @@ -0,0 +1,39 @@ +--- +'@object-ui/components': patch +'@object-ui/react': minor +--- + +fix(components): an action container's members evaluate `properties.params` the way a top-level `action:button` does + +A member of `action:bar`, `action:group` or `action:menu` never passes through +`SchemaRenderer`, so the evaluation that resolves `"recordId": "${record.id}"` +in a node's `properties.params` did not run for it. On a record page an +`action:bar` member drawn inline handed its `navigate_edit` handler the raw +`${record.id}` text, and a member that the bar placed in its overflow menu, or +an `action:group` / `action:menu` item, did not forward its `properties.params` +at all. + +Each container now evaluates its members' `properties` with the evaluator and +scope `SchemaRenderer` uses: `action:bar` where it composes an inline member, +and `action:group` / `action:menu` when they run an item. A container member's +static values therefore reach the handler as a top-level `action:button`'s do, +with one difference: the development-build unevaluated-expression diagnostic +runs in the `SchemaRenderer` memo, so it does not report an unresolvable +template on a container member. An item that carries an `ActionParam[]` input +list forwards both, as `action:button` does. A node-level `params` object is +still not read as values (objectui#10289). The one exception is unchanged from +objectui#10462: an `action:group` / `action:menu` item of `type: 'api'` still +forwards it as the request payload (objectstack#5777 window), unless the item +also carries a `properties.params` bag, which now takes precedence. + +`@object-ui/react` exports `useConfigBagEvaluator()` for this. It returns a +function that applies the `SchemaRenderer` memo's `properties` evaluation (the +same per-key rule and the same `record` / `current_user` / `page` scope) to a +config bag rendered without `SchemaRenderer`. It is not a second template +engine. Its plain half, `evaluateConfigBagInScope()`, is exported beside +`SchemaRenderer`. Both are built from the module-scope scope builder and +per-key rule that the memo itself calls. + +Declared as a minor (Clause-②, widening): `@object-ui/react` gains two public +exports, `useConfigBagEvaluator` and `evaluateConfigBagInScope`. Nothing is +removed or renamed. diff --git a/packages/components/src/renderers/action/__tests__/action-container-member-params-10290.test.tsx b/packages/components/src/renderers/action/__tests__/action-container-member-params-10290.test.tsx new file mode 100644 index 0000000000..57c3df4659 --- /dev/null +++ b/packages/components/src/renderers/action/__tests__/action-container-member-params-10290.test.tsx @@ -0,0 +1,200 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#10290: a member of an action CONTAINER (`action:bar`, + * `action:group`, `action:menu`) carries its static execution values in + * `properties.params` (objectui#10289, ruling A), and those values are + * templates evaluated where `properties` are (objectui#7867, ruling A). A + * container member never passes through the `SchemaRenderer` evaluation memo, + * so each container evaluates its members' `properties` through the evaluator + * that memo uses, against the scope that memo builds. + * + * Driven end to end through the real pieces: the real `SchemaRenderer` renders + * the real container, the click or menu selection goes through the real + * `ActionRunner`, and the value asserted is the `params` on the `ActionDef` the + * registered `navigate_edit` handler receives. The row is bound the way a + * record page binds it, through `RecordContextProvider`. + * + * The CONTROL is a top-level `action:button` with the same `properties.params`. + * It goes through the `SchemaRenderer` memo, so it resolved before this change + * too: a red container row beside a green control means the container, never + * an unbound `record` root. + */ + +import { describe, it, expect, vi, beforeEach, afterEach, type Mock } from 'vitest'; +import { render, screen, fireEvent, waitFor, within } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import React from 'react'; +import type { ActionContext, ActionDef, ActionResult } from '@object-ui/core'; +import type { BaseSchema } from '@object-ui/types'; +import { ActionProvider, RecordContextProvider, SchemaRenderer } from '@object-ui/react'; +// Module-scope side-effect imports: `action:bar` resolves its members and its +// overflow menu through the ComponentRegistry at render time, and the light +// `dom` project does not load the components graph. Module scope, not a +// `beforeAll`, per AGENTS.md 测试纪律. +import '../action-bar'; +import '../action-button'; +import '../action-icon'; +import '../action-menu'; +import '../action-group'; +import { resetStaticParamsWarnings } from '../static-params'; + +const ROW = { id: 'rec_1', name: 'Acme' }; + +/** The authored values: one template leaf, one literal leaf. */ +const AUTHORED = { objectName: 'account', recordId: '${record.id}' }; + +/** What the handler must receive once the template is evaluated. */ +const RESOLVED = { objectName: 'account', recordId: 'rec_1' }; + +/** The overflow trigger's accessible name, with no i18n bundle loaded. */ +const MORE = 'More actions'; + +let navigateEdit: Mock<(action: ActionDef, ctx: ActionContext) => Promise>; +let warn: ReturnType; +let consoleError: ReturnType; + +beforeEach(() => { + navigateEdit = vi.fn(async () => ({ success: true })); + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + resetStaticParamsWarnings(); +}); + +afterEach(() => { + warn.mockRestore(); + consoleError.mockRestore(); +}); + +/** One member action, as an author writes it inside a container's `actions`. */ +function member(extra: Record = {}): Record { + return { name: 'edit', label: 'Edit', type: 'navigate_edit', ...extra }; +} + +/** Render `schema` on a record page bound to {@link ROW}. */ +function renderOnRecordPage(schema: Record) { + return render( + + + + + , + ); +} + +/** Radix opens a dropdown on `pointerdown` and mounts its content in a portal. */ +async function selectFromMenu(trigger: HTMLElement, label: string) { + fireEvent.pointerDown(trigger, { button: 0, ctrlKey: false, pointerType: 'mouse' }); + const menu = await screen.findByRole('menu'); + fireEvent.click(within(menu).getByRole('menuitem', { name: label })); +} + +/** How the member is reached once its container is on screen. */ +type Reach = (label: string) => Promise; + +const clickButton: Reach = async (label) => { + fireEvent.click(screen.getByRole('button', { name: label })); +}; + +const viaTrigger = (triggerName: string): Reach => async (label) => { + await selectFromMenu(screen.getByRole('button', { name: triggerName }), label); +}; + +/** Every path a container member reaches the runner by. */ +const CONTAINERS: ReadonlyArray<{ + path: string; + schema: (m: Record) => Record; + reach: Reach; +}> = [ + { + path: 'action:bar, inline member', + schema: (m) => ({ type: 'action:bar', actions: [m] }), + reach: clickButton, + }, + { + path: 'action:bar, member drawn as an icon (`component: action:icon`)', + schema: (m) => ({ type: 'action:bar', actions: [{ ...m, component: 'action:icon' }] }), + reach: clickButton, + }, + { + path: 'action:bar, member placed in a button group (`component: action:group`)', + schema: (m) => ({ type: 'action:bar', actions: [{ ...m, component: 'action:group' }] }), + reach: clickButton, + }, + { + path: 'action:bar, member placed in the overflow menu (`component: action:menu`)', + schema: (m) => ({ type: 'action:bar', actions: [{ ...m, component: 'action:menu' }] }), + reach: viaTrigger(MORE), + }, + { + path: 'action:bar, member spilled past `maxVisible` into the overflow menu', + schema: (m) => ({ + type: 'action:bar', + maxVisible: 1, + mobileMaxVisible: 1, + actions: [{ name: 'first', label: 'First', type: 'navigate_edit' }, m], + }), + reach: viaTrigger(MORE), + }, + { + path: 'action:group, inline display', + schema: (m) => ({ type: 'action:group', actions: [m] }), + reach: clickButton, + }, + { + path: 'action:group, dropdown display', + schema: (m) => ({ type: 'action:group', display: 'dropdown', label: 'Group', actions: [m] }), + reach: viaTrigger('Group'), + }, + { + path: 'action:menu', + schema: (m) => ({ type: 'action:menu', actions: [m] }), + reach: viaTrigger(MORE), + }, +]; + +/** Mount, reach the member, return the `params` the handler received. */ +async function paramsReceived(schema: Record, reach: Reach): Promise { + renderOnRecordPage(schema); + await reach('Edit'); + await waitFor(() => expect(navigateEdit).toHaveBeenCalledTimes(1)); + return (navigateEdit.mock.calls[0][0] as ActionDef).params; +} + +describe('objectui#10290 - a container member\'s `properties.params` is evaluated where `properties` are', () => { + it('CONTROL: a top-level `action:button` resolves `${record.id}` through the SchemaRenderer memo', async () => { + const params = await paramsReceived( + { + type: 'action:button', + name: 'edit', + label: 'Edit', + actionType: 'navigate_edit', + properties: { params: AUTHORED }, + }, + clickButton, + ); + expect(params).toEqual(RESOLVED); + }); + + describe.each(CONTAINERS)('$path', ({ schema, reach }) => { + it('the handler receives `properties.params` with `${record.id}` resolved', async () => { + const params = await paramsReceived(schema(member({ properties: { params: AUTHORED } })), reach); + expect(params).toEqual(RESOLVED); + }); + + it('a node-level `params` OBJECT is still not a values channel, and says so once (objectui#10289)', async () => { + const params = await paramsReceived(schema(member({ params: AUTHORED })), reach); + expect(params).toBeUndefined(); + const lines = warn.mock.calls + .map((c: unknown[]) => String(c[0])) + .filter((m: string) => m.includes('action "edit"') && m.includes('`params`')); + expect(lines).toHaveLength(1); + }); + }); +}); diff --git a/packages/components/src/renderers/action/action-bar.tsx b/packages/components/src/renderers/action/action-bar.tsx index dd60d8a9d2..34d3f68888 100644 --- a/packages/components/src/renderers/action/action-bar.tsx +++ b/packages/components/src/renderers/action/action-bar.tsx @@ -51,11 +51,12 @@ import React, { forwardRef, useMemo } from 'react'; import { ComponentRegistry } from '@object-ui/core'; import type { UIActionSchema, ActionLocation, ActionComponent } from '@object-ui/types'; import { ACTION_LOCATIONS, actionRendersAt } from '@object-ui/types'; -import { useCondition, toPredicateInput, useCapabilityGate } from '@object-ui/react'; +import { useCondition, toPredicateInput, useCapabilityGate, useConfigBagEvaluator } from '@object-ui/react'; import { useObjectTranslation } from '@object-ui/i18n'; import { cn } from '../../lib/utils'; import { useIsMobile } from '../../hooks/use-mobile'; import { ButtonGroup } from '../../custom/button-group'; +import { withEvaluatedProperties } from './static-params'; function useActionsLabel(): string { // useObjectTranslation is provider-safe (never throws); no try/catch, which @@ -157,6 +158,9 @@ const ActionBarRenderer = forwardRef { @@ -327,6 +331,14 @@ const ActionBarRenderer = forwardRef { const Renderer = ComponentRegistry.get(componentType); if (!Renderer) return null; @@ -335,7 +347,7 @@ const ActionBarRenderer = forwardRef | undefined { + const properties = subject.properties; + return isConfigBag(properties) && isConfigBag(properties.params) + ? (properties.params as Record) + : undefined; +} + /** * The static values to forward as the runner's `ActionDef.params`: the node's * `properties.params` when it is an object bag, otherwise `undefined`. @@ -82,11 +92,7 @@ export function readStaticParamValues( subject: StaticParamsSubject, where: string, ): Record | undefined { - const properties = subject.properties; - const values = - isConfigBag(properties) && isConfigBag(properties.params) - ? (properties.params as Record) - : undefined; + const values = propertiesParams(subject); const nodeLevel = subject.params; if (isConfigBag(nodeLevel) && nodeLevel !== values) { warnNodeLevelObjectParams(subject, where); @@ -94,11 +100,64 @@ export function readStaticParamValues( return values; } +/** + * Evaluates a config bag the way `SchemaRenderer` evaluates `properties`: the + * function `useConfigBagEvaluator()` (`@object-ui/react`) returns. + */ +export type ConfigBagEvaluator = (bag: unknown) => unknown; + +/** + * A container MEMBER with its `properties` evaluated (objectui#10290). + * + * `action:bar`, `action:group` and `action:menu` draw their member actions + * themselves, so a member never passes through the `SchemaRenderer` memo that + * evaluates a node's `properties`. Its static values ride `properties.params` + * (objectui#10289), and they are templates (objectui#7867): on a record page, + * `"recordId": "${record.id}"` has to reach the handler as the record's id. The + * container evaluates the member's `properties` with the memo's own evaluator + * and scope (`evaluateBag`, from `useConfigBagEvaluator()`), once, where it + * composes or runs the member. + * + * Returns the member itself when it carries no `properties` bag. Only + * `properties` is evaluated. It is not copied onto the member (the memo's + * hoist), and the member's other keys are left as authored. + */ +export function withEvaluatedProperties( + member: T, + evaluateBag: ConfigBagEvaluator, +): T { + const properties = (member as { properties?: unknown }).properties; + return isConfigBag(properties) ? { ...member, properties: evaluateBag(properties) } : member; +} + +/** + * A container member's static values (objectui#10290): its `properties.params`, + * evaluated through {@link withEvaluatedProperties}, or `undefined`. + * + * For `action:group` / `action:menu` items, which run the member themselves. + * (`action:bar` hands an inline member to `action:button` / `action:icon` with + * its `properties` already evaluated, and `readStaticParamValues` reads it + * there.) + */ +export function readMemberStaticParamValues( + member: StaticParamsSubject, + evaluateBag: ConfigBagEvaluator, +): Record | undefined { + if (!propertiesParams(member)) return undefined; + return propertiesParams(withEvaluatedProperties(member, evaluateBag)); +} + /** * The same ruling on a spec ACTION ENTRY (objectui#10462): `element:button`'s * inline `action`, `action:group` / `action:menu` items and `page:header`'s - * actions. An entry is not an SDUI node, so it has no `properties.params` bag; - * `params` is its `ActionParam[]` input list and nothing else. + * actions. `params` is an entry's `ActionParam[]` input list and nothing else. + * (An `action:group` / `action:menu` item is also a container member, so its + * static values are read from `properties.params` and evaluated by the + * container through {@link readMemberStaticParamValues} (objectui#10290). + * `UIActionSchema` declares no `properties`, so on a container member this bag + * is read off the authored object, not off a declared key. `element:button`'s + * inline `action` and `page:header`'s actions do not read + * `properties.params`.) * * One type is still different: `api`. The objectstack#5777 window keeps the * runner reading an object `params` as the request payload (with its own diff --git a/packages/react/src/SchemaRenderer.tsx b/packages/react/src/SchemaRenderer.tsx index 364d47f781..628a223faa 100644 --- a/packages/react/src/SchemaRenderer.tsx +++ b/packages/react/src/SchemaRenderer.tsx @@ -536,6 +536,92 @@ const isExpressionEnvelope = (value: unknown): boolean => const isDataObjectValue = (key: string, value: unknown): boolean => !PREDICATE_CHAIN_KEYS.has(key) && isConfigBag(value) && !isExpressionEnvelope(value); +/** + * The scope the evaluation memo in {@link SchemaRenderer} evaluates a node's + * values against: the ambient host scope, `current_user` aliased to `user`, the + * page's bound row as `record` (only when there is one), and page variables as + * `page`. Why each root is (and `data` is not) bound is stated where the memo + * builds its evaluator from this. + * + * One function, so the memo and {@link evaluateConfigBagInScope} cannot build + * two different scopes (objectui#10290). + */ +const configEvaluationScope = ( + predicateScope: Record, + boundRecord: unknown, + pageVariables: unknown, +): Record => ({ + ...predicateScope, + current_user: predicateScope?.user, + ...(boundRecord && typeof boundRecord === 'object' && !Array.isArray(boundRecord) + ? { record: boundRecord } + : null), + page: pageVariables, +}); + +/** + * Evaluate ONE config value by its key. This is the per-key rule of the + * evaluation memo (the `params` rule, objectui#7867, is stated there): a + * `params` bag has every string leaf evaluated, a data object is handed over as + * authored, and every other value is evaluated per value and shallow, with a + * CEL predicate envelope preserved. + */ +const evaluateConfigValueWith = ( + evaluator: ExpressionEvaluator, + key: string, + value: unknown, +): unknown => + key === PARAMS_KEY && isParamsBag(value) + ? mapParamsLeaves(value, (leaf) => evaluator.evaluate(leaf)) + : isDataObjectValue(key, value) + ? value + : preservePredicateEnvelope(key, value, (v) => evaluator.evaluate(v as any)); + +/** + * Evaluate every value of a config bag with {@link evaluateConfigValueWith}. + * The memo's `properties` loop, as a function. Returns a new object; the + * authored bag is never mutated. + */ +const evaluateConfigBagWith = ( + evaluator: ExpressionEvaluator, + bag: Record, +): Record => { + const evaluated: Record = { ...bag }; + for (const [key, val] of Object.entries(evaluated)) { + evaluated[key] = evaluateConfigValueWith(evaluator, key, val); + } + return evaluated; +}; + +/** + * The `properties` evaluation of the {@link SchemaRenderer} memo, for a config + * bag that does not pass through `SchemaRenderer` (objectui#10290): the same + * per-key rule, the same `ExpressionEvaluator`, and the same scope, built from + * the same three inputs. It is not a second template engine. + * + * The plain half of `useConfigBagEvaluator()` (`hooks/useConfigBagEvaluator`), + * which reads those inputs from the contexts the memo reads them from. Call + * the hook; this function exists because the rule it applies is this module's. + * (The hook is not declared in this module: a hook here puts this file under + * the React Compiler lint, which then reports the render-time + * `performance.now()` / `Date.now()` debug probes below.) + * + * A value that is not a config bag is returned unchanged; a bag comes back as a + * new, evaluated object and the authored one is never mutated. + */ +export function evaluateConfigBagInScope( + bag: unknown, + predicateScope: Record, + boundRecord: unknown, + pageVariables: unknown, +): unknown { + if (!isConfigBag(bag)) return bag; + const evaluator = new ExpressionEvaluator( + configEvaluationScope(predicateScope, boundRecord, pageVariables), + ); + return evaluateConfigBagWith(evaluator, bag as Record); +} + /** * Which CONSEQUENCE the diagnostic should print for a faulting predicate on * this leg (objectui#6503). @@ -1005,14 +1091,13 @@ export const SchemaRenderer: ForwardRefExoticComponent< // (how `action:group`'s dropdown leaf is driven), turning "this surface has // no row of its own" into "this surface's row is empty" — only the latter // is entitled to shadow. - const evaluator = new ExpressionEvaluator({ - ...predicateScope, - current_user: (predicateScope as any)?.user, - ...(boundRecord && typeof boundRecord === 'object' && !Array.isArray(boundRecord) - ? { record: boundRecord } - : null), - page: pageVariables, - }); + // + // Built by `configEvaluationScope`, which `useConfigBagEvaluator` shares, so + // an action container's members evaluate against this same scope + // (objectui#10290). + const evaluator = new ExpressionEvaluator( + configEvaluationScope(predicateScope, boundRecord, pageVariables), + ); // Shallow copy const newSchema = { ...schema }; @@ -1310,12 +1395,12 @@ export const SchemaRenderer: ForwardRefExoticComponent< // A DATA OBJECT on any other key ({@link isDataObjectValue}, // objectui#10288) is handed over as authored: `evaluate` would collapse // one that carries a string `source` to that string. + // + // The rule itself is `evaluateConfigValueWith`, at module scope, so that + // `useConfigBagEvaluator` applies the same one to an action container's + // members (objectui#10290). const evaluateConfigValue = (key: string, value: unknown): unknown => - key === PARAMS_KEY && isParamsBag(value) - ? mapParamsLeaves(value, (leaf) => evaluator.evaluate(leaf)) - : isDataObjectValue(key, value) - ? value - : preservePredicateEnvelope(key, value, (v) => evaluator.evaluate(v as any)); + evaluateConfigValueWith(evaluator, key, value); // Carrier 1 of the `params` rule above: the node-level bag. A non-bag // node-level `params` (the `ActionParam[]` definition list, or anything @@ -1400,15 +1485,13 @@ export const SchemaRenderer: ForwardRefExoticComponent< : undefined; if (rawPropertiesBag) { - const newProperties: Record = { ...rawPropertiesBag }; - for (const [key, val] of Object.entries(newProperties)) { - // objectui#9100 — a CEL predicate envelope survives this loop; see - // `preservePredicateEnvelope`. objectui#7867 — a `params` bag has every - // string leaf evaluated (carrier 2 of `evaluateConfigValue`). Every - // other value evaluates as before. - newProperties[key] = evaluateConfigValue(key, val); - } - newSchema.properties = newProperties; + // objectui#9100 — a CEL predicate envelope survives this loop; see + // `preservePredicateEnvelope`. objectui#7867 — a `params` bag has every + // string leaf evaluated (carrier 2 of `evaluateConfigValue`). Every + // other value evaluates as before. The loop is `evaluateConfigBagWith`, + // the one `useConfigBagEvaluator` runs over an action container's + // member (objectui#10290). + newSchema.properties = evaluateConfigBagWith(evaluator, rawPropertiesBag); } /** diff --git a/packages/react/src/hooks/index.ts b/packages/react/src/hooks/index.ts index 93b35397da..9bac804df8 100644 --- a/packages/react/src/hooks/index.ts +++ b/packages/react/src/hooks/index.ts @@ -43,6 +43,9 @@ export * from './useActionEngine.js'; // authored strings an action carries (label / confirmText / successMessage). export * from './useActionTextLocalizer.js'; export * from './useCapabilityGate.js'; +// The `SchemaRenderer` memo's `properties` evaluation, for an action container's +// members, which are drawn without `SchemaRenderer` (objectui#10290). +export * from './useConfigBagEvaluator.js'; // The analytics label net's React glue, consumed by BOTH plugin-dashboard's // `DatasetWidget` and plugin-report's dataset block (objectui#4389). It lives // here rather than in `@object-ui/core` because it reads `SchemaRendererContext` diff --git a/packages/react/src/hooks/useConfigBagEvaluator.ts b/packages/react/src/hooks/useConfigBagEvaluator.ts new file mode 100644 index 0000000000..8b01c52958 --- /dev/null +++ b/packages/react/src/hooks/useConfigBagEvaluator.ts @@ -0,0 +1,42 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import { useRecordContext } from '../context/RecordContext.js'; +import { evaluateConfigBagInScope } from '../SchemaRenderer.js'; +import { usePredicateScope } from './useExpression.js'; +import { usePageVariables } from './usePageVariables.js'; + +/** + * The `properties` evaluation of the `SchemaRenderer` memo, for a node that is + * rendered WITHOUT `SchemaRenderer` (objectui#10290). + * + * The action containers (`action:bar`, `action:group`, `action:menu` in + * `@object-ui/components`) draw their member actions themselves, so a member's + * `properties` never passes through the memo. Its static execution values ride + * `properties.params` (objectui#10289), and those values are templates + * evaluated where `properties` are (objectui#7867). This hook is that + * evaluation: the memo's per-key rule, its `ExpressionEvaluator`, and its + * scope (the host's ambient roots, `current_user`, the page's bound row as + * `record`, and page variables as `page`), read from the same contexts the memo + * reads. It is not a second template engine. See `evaluateConfigBagInScope`. + * + * The returned function takes a config bag and returns a new, evaluated bag. A + * value that is not a config bag is returned unchanged. Call it once per + * authored bag: the result is already evaluated, and evaluating it again would + * interpolate text that came out of the row. + * + * A fresh function each render. Nothing may depend on its identity + * (AGENTS.md #10). + */ +export function useConfigBagEvaluator(): (bag: unknown) => unknown { + const predicateScope = usePredicateScope(); + const boundRecord = useRecordContext()?.data; + const { variables: pageVariables } = usePageVariables(); + return (bag: unknown): unknown => + evaluateConfigBagInScope(bag, predicateScope, boundRecord, pageVariables); +}