From 6cfa28005350caf9e0e47b38ed8a1ca7dfc8620b Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Mon, 27 Jul 2026 12:43:31 +0800 Subject: [PATCH] feat(approvals): typed output pickers, quick-path guard, expression completion (framework#3447, #2829) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Typed decision_output_defs render record pickers (user people-picker / department/position/team lookups; multiple → id array). Quick decision paths (inline a/r, hover, mobile, bulk) are guarded off requests that declare decision outputs — only the drawer dialog collects them; buttons disable with an explanation. The expression approver input gains a three-group scope picker (current/trigger/vars) with inline root validation; nodeOutputRefs models approval outputs (decision + declared keys) so the previous stage's outputs are pickable, and vars.previous is always listed so legitimate vars.* references never flag as unknown. Browser-verified end to end (picker multi-select → co-sign slate). Co-Authored-By: Claude Fable 5 --- ...roval-typed-picker-quickpath-completion.md | 10 ++++ .../src/pages/system/ApprovalsInboxPage.tsx | 59 +++++++++++++++---- apps/console/src/services/approvalsApi.ts | 13 ++++ .../src/views/DeclaredActionsBar.tsx | 50 ++++++++++++---- .../inspectors/FlowNodeConfigField.tsx | 5 +- .../inspectors/FlowNodeInspector.tsx | 3 +- .../inspectors/FlowObjectListField.tsx | 24 +++++--- .../metadata-admin/inspectors/flow-scope.ts | 20 ++++++- .../metadata-admin/inspectors/useFlowScope.ts | 48 ++++++++++++++- 9 files changed, 193 insertions(+), 39 deletions(-) create mode 100644 .changeset/approval-typed-picker-quickpath-completion.md diff --git a/.changeset/approval-typed-picker-quickpath-completion.md b/.changeset/approval-typed-picker-quickpath-completion.md new file mode 100644 index 000000000..9a4acf174 --- /dev/null +++ b/.changeset/approval-typed-picker-quickpath-completion.md @@ -0,0 +1,10 @@ +--- +"@object-ui/app-shell": minor +"@object-ui/console": minor +--- + +feat(approvals): typed decision-output pickers, quick-path guard, and approval-expression completion (framework#3447 follow-ups, #2829) + +- Decision dialogs render TYPED decision outputs as record pickers: `decision_output_defs` (`{ key, label?, type, multiple? }`) maps `user` to the sys_user people picker and `department`/`position`/`team` to the matching system-object lookup; `multiple` collects an id array. Bare keys keep the text input. +- Quick decision paths (inline a/r keyboard, hover buttons, mobile card buttons, bulk apply) no longer decide a request whose node declares decision outputs (#2829) — only the drawer dialog collects those fields. Buttons render disabled with an explanation; bulk selection excludes such rows via the existing "N actionable" messaging. +- The approval `expression` approver input now has a scope-aware data picker and inline root validation: three groups — `current.` (live at node entry), `trigger.` (submit snapshot), `vars.*` (flow variables) — built by `useFlowScope` from the same materials as the condition picker but with the approval root set. `nodeOutputRefs` now models approval nodes (`.decision` + declared `decisionOutputs` keys), so the previous stage's outputs are pickable, and `vars.previous` is always listed so a legitimate `vars.*` reference is never flagged as out of scope. diff --git a/apps/console/src/pages/system/ApprovalsInboxPage.tsx b/apps/console/src/pages/system/ApprovalsInboxPage.tsx index 9866b85c2..70eb041d7 100644 --- a/apps/console/src/pages/system/ApprovalsInboxPage.tsx +++ b/apps/console/src/pages/system/ApprovalsInboxPage.tsx @@ -803,13 +803,29 @@ export function ApprovalsInboxPage() { return (r.pending_approvers || []).some(a => idSet.has(a)); }, [identities]); + /** + * #2829 / framework#3447: a node that declares decision outputs expects the + * approver to fill them — only the drawer's declared-action dialog collects + * them, so the quick paths (inline a/r, hover buttons, bulk) must not decide + * such a request: a quick approve would silently hand the flow nothing and + * the next stage's expression approver would resolve an empty slate. + */ + const needsDecisionInputs = useCallback((r: ApprovalRequestRow): boolean => + Array.isArray(r.decision_outputs) && r.decision_outputs.length > 0, []); + + /** Quick-decidable = actionable AND no declared decision outputs (#2829). */ + const quickDecidable = useCallback((r: ApprovalRequestRow): boolean => + isActionable(r) && !needsDecisionInputs(r), [isActionable, needsDecisionInputs]); + /** * Rows the user is actually allowed to bulk-act on: - * status=pending AND one of the user's identities is in pending_approvers. + * status=pending AND one of the user's identities is in pending_approvers + * AND the node declares no decision outputs (#2829 — those need the drawer + * dialog; they surface in the bulk bar as skipped). */ const actionableSelectedRows = useMemo( - () => filteredRows.filter(r => selectedRowIds.has(r.id) && isActionable(r)), - [filteredRows, selectedRowIds, isActionable], + () => filteredRows.filter(r => selectedRowIds.has(r.id) && quickDecidable(r)), + [filteredRows, selectedRowIds, quickDecidable], ); const allFilteredSelectable = filteredRows.filter(r => r.status === 'pending'); @@ -942,17 +958,17 @@ export function ApprovalsInboxPage() { } else if ((e.key === 'x' || e.key === ' ') && idx >= 0 && list[idx] && tab === 'pending') { e.preventDefault(); toggleRow(list[idx].id); - } else if (e.key === 'a' && idx >= 0 && list[idx] && isActionable(list[idx])) { + } else if (e.key === 'a' && idx >= 0 && list[idx] && quickDecidable(list[idx])) { e.preventDefault(); setApproveTarget(list[idx]); - } else if (e.key === 'r' && idx >= 0 && list[idx] && isActionable(list[idx])) { + } else if (e.key === 'r' && idx >= 0 && list[idx] && quickDecidable(list[idx])) { e.preventDefault(); setRejectTarget(list[idx]); } }; window.addEventListener('keydown', onKey); return () => window.removeEventListener('keydown', onKey); - }, [selectedId, approveTarget, rejectTarget, tab, openDrawer, toggleRow, isActionable]); + }, [selectedId, approveTarget, rejectTarget, tab, openDrawer, toggleRow, quickDecidable]); // Drawer keyboard: ←/→ walk the visible list without going back to it. useEffect(() => { @@ -1033,18 +1049,27 @@ export function ApprovalsInboxPage() { function InlineActions({ r }: { r: ApprovalRequestRow }) { if (!isActionable(r)) return null; const busy = inlineActing === r.id; + // #2829: a request whose node declares decision outputs must go through + // the drawer's dialog (the only place those fields are collected) — render + // the quick buttons disabled with an explanation instead of hiding them. + const needsInputs = needsDecisionInputs(r); + const needsInputsHint = tr( + 'needsDecisionInputs', + 'This approval collects decision outputs — open it to decide.', + ); return (
e.stopPropagation()} + title={needsInputs ? needsInputsHint : undefined} > @@ -1052,9 +1077,9 @@ export function ApprovalsInboxPage() { size="sm" variant="ghost" className="h-7 px-2 text-red-600 hover:text-red-700 hover:bg-red-50 dark:text-red-400" - disabled={busy} + disabled={busy || needsInputs} onClick={() => setRejectTarget(r)} - aria-label={tr('reject', 'Reject')} + aria-label={needsInputs ? needsInputsHint : tr('reject', 'Reject')} > @@ -1419,13 +1444,21 @@ export function ApprovalsInboxPage() {
{isActionable(r) && ( -
e.stopPropagation()}> - diff --git a/apps/console/src/services/approvalsApi.ts b/apps/console/src/services/approvalsApi.ts index 7f4d630a2..225bf8778 100644 --- a/apps/console/src/services/approvalsApi.ts +++ b/apps/console/src/services/approvalsApi.ts @@ -112,6 +112,19 @@ export interface ApprovalRequestRow { * `.` variables. Absent when the node declares none. */ decision_outputs?: string[]; + /** + * framework#3447 follow-up: the normalized TYPED declarations behind + * `decision_outputs` — `{ key, label?, type?, multiple? }` — so the decide + * dialog renders a record picker (user / department / position / team; id + * values, `multiple` → id array) instead of free text. Prefer this and fall + * back to the bare key list (older backend). + */ + decision_output_defs?: Array<{ + key: string; + label?: string; + type?: 'text' | 'user' | 'department' | 'position' | 'team'; + multiple?: boolean; + }>; } /** diff --git a/packages/app-shell/src/views/DeclaredActionsBar.tsx b/packages/app-shell/src/views/DeclaredActionsBar.tsx index 0e66942b1..b1b285854 100644 --- a/packages/app-shell/src/views/DeclaredActionsBar.tsx +++ b/packages/app-shell/src/views/DeclaredActionsBar.tsx @@ -131,18 +131,44 @@ const DeclaredActionButton: React.FC<{ // handler folds them into the nested `outputs` body the decide route // expects. Comma-separated values are legal (the service accepts CSV for // multi-id outputs). - const declaredOutputKeys: string[] = - Array.isArray(recordData.decision_outputs) - && /\/(approve|reject)$/.test(String((action as any).target ?? '')) - ? (recordData.decision_outputs as unknown[]).map(String) - : []; - const outputParams = declaredOutputKeys.map((key) => ({ - name: `outputs.${key}`, - label: key.split(/[_-]+/).filter(Boolean).map(w => w.charAt(0).toUpperCase() + w.slice(1)).join(' '), - type: 'text' as const, - required: false, - helpText: 'Handed to the flow as a decision output. Comma-separate multiple values.', - })); + const isDecideAction = /\/(approve|reject)$/.test(String((action as any).target ?? '')); + // Prefer the typed declarations (framework#3447 follow-up); fall back to + // the bare key list from an older backend. + const declaredOutputs: Array<{ key: string; label?: string; type?: string; multiple?: boolean }> = + isDecideAction && Array.isArray(recordData.decision_output_defs) && recordData.decision_output_defs.length + ? (recordData.decision_output_defs as Array<{ key: string; label?: string; type?: string; multiple?: boolean }>) + : isDecideAction && Array.isArray(recordData.decision_outputs) + ? (recordData.decision_outputs as unknown[]).map((k) => ({ key: String(k) })) + : []; + // Widget mapping: `user` has a dedicated sys_user picker widget; the + // other record kinds ride a lookup param pointed at the system object. + const OUTPUT_LOOKUP_OBJECTS: Record = { + department: 'sys_business_unit', + position: 'sys_position', + team: 'sys_team', + }; + const outputParams = declaredOutputs.filter((d) => d.key).map((d) => { + const label = d.label + ?? d.key.split(/[_-]+/).filter(Boolean).map(w => w.charAt(0).toUpperCase() + w.slice(1)).join(' '); + const base = { name: `outputs.${d.key}`, label, required: false } as Record; + if (d.type === 'user') { + return { + ...base, type: 'user', multiple: d.multiple === true, + helpText: 'Handed to the flow as a decision output.', + }; + } + const lookupObject = d.type ? OUTPUT_LOOKUP_OBJECTS[d.type] : undefined; + if (lookupObject) { + return { + ...base, type: 'lookup', referenceTo: lookupObject, multiple: d.multiple === true, + helpText: 'Handed to the flow as a decision output.', + }; + } + return { + ...base, type: 'text', + helpText: 'Handed to the flow as a decision output. Comma-separate multiple values.', + }; + }); const dispatch: any = { ...rest, // Localized copies ride the dispatch: the runner reads `label` for the diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx index 090f17fbb..24d0e7082 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx @@ -34,9 +34,11 @@ export interface FlowNodeConfigFieldProps { context?: FlowReferenceContext; /** In-scope variable references for the data-picker (#1934). */ scopeGroups?: ScopeGroup[]; + /** #3447: approval-expression picker groups (current/trigger/vars roots). */ + approvalScopeGroups?: ScopeGroup[]; } -export function FlowNodeConfigField({ field, value, onCommit, disabled, locale, context, scopeGroups }: FlowNodeConfigFieldProps) { +export function FlowNodeConfigField({ field, value, onCommit, disabled, locale, context, scopeGroups, approvalScopeGroups }: FlowNodeConfigFieldProps) { const refMode: 'expression' | 'template' = field.refMode ?? (field.kind === 'expression' ? 'expression' : 'template'); const control = (() => { @@ -114,6 +116,7 @@ export function FlowNodeConfigField({ field, value, onCommit, disabled, locale, itemLabel={t('engine.inspector.flowNode.list.item', locale)} context={context} scopeGroups={scopeGroups} + approvalScopeGroups={approvalScopeGroups} /> ); case 'number': diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx index 1883fd4dd..deeeaa88e 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx @@ -136,7 +136,7 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection, [loc], ); // In-scope variable references for this node, for the data-picker (#1934). - const { groups: scopeGroups } = useFlowScope(draft as Record, loc?.scopeAnchorId, nestedLoopRefs); + const { groups: scopeGroups, approvalExpressionGroups } = useFlowScope(draft as Record, loc?.scopeAnchorId, nestedLoopRefs); const fields = React.useMemo(() => { const schema = node?.type ? configSchemas[node.type] : undefined; const serverFields = schema !== undefined ? jsonSchemaToFlowFields(schema) : null; @@ -364,6 +364,7 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection, locale={locale} context={{ draft, node }} scopeGroups={scopeGroups} + approvalScopeGroups={approvalExpressionGroups} /> ); })} diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowObjectListField.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowObjectListField.tsx index 1e19c79a9..2cb80a1b3 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowObjectListField.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowObjectListField.tsx @@ -103,6 +103,13 @@ export interface FlowObjectListFieldProps { context?: FlowReferenceContext; /** In-scope variable references for `expression` columns (#1934). */ scopeGroups?: ScopeGroup[]; + /** + * #3447: picker groups for approval `expression` approver cells — the + * closed current/trigger/vars root set. Regular flow scopeGroups must NOT + * be offered there (record.* / bare-field spellings are rejected at + * runtime), which is why this rides as its own prop. + */ + approvalScopeGroups?: ScopeGroup[]; } export function FlowObjectListField({ @@ -117,6 +124,7 @@ export function FlowObjectListField({ itemLabel, context, scopeGroups, + approvalScopeGroups, }: FlowObjectListFieldProps) { const external = React.useMemo( () => @@ -271,12 +279,12 @@ export function FlowObjectListField({ // 'expression' authors a CEL expression over the approval // roots (current/trigger/vars). Render the expression // input (mono + syntax check) instead of a dead free-text - // reference box. The flow-scope picker/roots are - // deliberately NOT wired in: approval expressions have - // their own root set, and offering flow-scope paths here - // (record.x, bare fields) would teach exactly the - // spelling the runtime rejects; root validation runs - // server-side (os lint + node-entry pre-check). + // reference box, with the APPROVAL scope groups — never + // the regular flow scopeGroups, whose record.x / + // bare-field spellings the runtime rejects. The same + // groups feed FlowExprIssue, so an out-of-contract root + // warns inline with a "did you mean" before os lint / + // the node-entry pre-check reject it server-side. if (resolved === undefined && disc === 'expression') { const raw = typeof row.values[col.key] === 'string' ? (row.values[col.key] as string) : ''; return ( @@ -290,11 +298,11 @@ export function FlowObjectListField({ onKeyDown={(e) => { if (e.key === 'Enter') (e.target as HTMLInputElement).blur(); }} - groups={[]} + groups={approvalScopeGroups ?? []} placeholder={col.placeholder ?? 'current. · trigger. · vars..'} disabled={disabled} /> - +
); } diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/flow-scope.ts b/packages/app-shell/src/views/metadata-admin/inspectors/flow-scope.ts index 031766e69..9ff9a81ed 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/flow-scope.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/flow-scope.ts @@ -31,7 +31,13 @@ */ /** Which group a reference belongs to (drives the picker's section headers). */ -export type ScopeGroupId = 'variables' | 'outputs' | 'loop' | 'trigger'; +export type ScopeGroupId = + | 'variables' | 'outputs' | 'loop' | 'trigger' + // #3447: approval `expression` approvers see a DIFFERENT root set than flow + // conditions (current/trigger/vars — never `record`/bare fields). Their + // picker groups carry their own ids so they can never leak into the regular + // condition picker. + | 'approval_current' | 'approval_trigger' | 'approval_vars'; /** * One pickable reference. `token` is the BARE form (no braces); the picker @@ -186,6 +192,18 @@ export function nodeOutputRefs(node: FlowNodeLike): ScopeRef[] { for (const f of asArray(cfg.fields)) add(str(asRecord(f).name)); } + // Approval (framework#3447): the resume envelope writes `.decision`, + // and each author-declared decision output (`decisionOutputs` — bare key or + // typed `{ key, … }`) lands as `.` — the values a later + // approval node's `expression` approver reads as `vars..`. + if (type === 'approval' && nodeId) { + add(`${nodeId}.decision`); + for (const entry of asArray(cfg.decisionOutputs)) { + const key = typeof entry === 'string' ? entry : str(asRecord(entry).key); + if (key) add(`${nodeId}.${key}`); + } + } + return out; } diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/useFlowScope.ts b/packages/app-shell/src/views/metadata-admin/inspectors/useFlowScope.ts index 186cb4d57..819d5cc36 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/useFlowScope.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/useFlowScope.ts @@ -31,14 +31,25 @@ export interface UseFlowScopeResult { groups: ScopeGroup[]; /** Flat, de-duplicated ref list (all groups). */ refs: ScopeRef[]; + /** + * #3447: the picker groups for an approval node's `expression` APPROVER, + * whose closed root set differs from a flow condition's — `current.` + * (live record at node entry), `trigger.` (submit-time snapshot) and + * `vars.*` (flow variables / upstream outputs, prefix-mapped). `record.*` + * and bare fields are deliberately absent: inserting them would author + * exactly the spelling the runtime rejects. + */ + approvalExpressionGroups: ScopeGroup[]; /** True while the trigger object's fields are still loading. */ loading: boolean; /** No references in scope — the field should render as a plain input. */ isEmpty: boolean; } -const GROUP_ORDER: ScopeGroupId[] = ['variables', 'outputs', 'loop', 'trigger']; -const GROUP_LABELS: Record = { +// The REGULAR picker's sections — the approval_* group ids (#3447) never +// appear here; they ride the separately-emitted approvalExpressionGroups. +const GROUP_ORDER = ['variables', 'outputs', 'loop', 'trigger'] as const; +const GROUP_LABELS: Record<(typeof GROUP_ORDER)[number], string> = { variables: 'Flow variables', outputs: 'Upstream outputs', loop: 'Loop item', @@ -77,6 +88,37 @@ export function useFlowScope( label: GROUP_LABELS[id], refs: refs.filter((r) => r.group === id), })).filter((g) => g.refs.length > 0); - return { groups, refs, loading: !!scope.trigger && loading, isEmpty: refs.length === 0 }; + + // #3447: approval-expression picker groups, built from the same materials. + // Trigger-object fields expand under BOTH times (current/trigger); the + // graph-walk refs (variables / upstream outputs / loop items) re-home + // under the `vars.` prefix. Trigger-group refs (record., previous., + // bare fields) are excluded — those spellings are not bound there. + const approvalCurrent: ScopeRef[] = []; + const approvalTrigger: ScopeRef[] = []; + for (const f of fields) { + if (!f?.name) continue; + const detail = f.label && f.label !== f.name ? f.label : f.type; + approvalCurrent.push({ token: `current.${f.name}`, label: `current.${f.name}`, detail, group: 'approval_current' }); + approvalTrigger.push({ token: `trigger.${f.name}`, label: `trigger.${f.name}`, detail, group: 'approval_trigger' }); + } + const approvalVars: ScopeRef[] = refs + .filter((r) => r.group === 'variables' || r.group === 'outputs' || r.group === 'loop') + .map((r) => ({ ...r, token: `vars.${r.token}`, label: `vars.${r.token}`, group: 'approval_vars' as const })); + // `vars.previous` (the pre-update row) is always addressable — listing it + // both teaches the spelling and keeps the `vars` root in the known set so + // FlowExprIssue never flags a legitimate vars.* reference on a flow with + // no declared variables or upstream outputs. + approvalVars.push({ + token: 'vars.previous', label: 'vars.previous', + detail: 'pre-update row', group: 'approval_vars', + }); + const approvalExpressionGroups: ScopeGroup[] = [ + { id: 'approval_current' as const, label: 'Current record (live at node entry)', refs: approvalCurrent }, + { id: 'approval_trigger' as const, label: 'Trigger snapshot (at submit)', refs: approvalTrigger }, + { id: 'approval_vars' as const, label: 'Flow variables', refs: approvalVars }, + ].filter((g) => g.refs.length > 0); + + return { groups, refs, approvalExpressionGroups, loading: !!scope.trigger && loading, isEmpty: refs.length === 0 }; }, [scope, fields, loading, extraRefs]); }