fix(app-shell): run sys_approval_request's declared decision actions on the record page (#3055)

.changeset/record-page-declared-approval-actions-3055.md

@@ -0,0 +1,16 @@
+---
+'@object-ui/app-shell': patch
+---
+
+Run `sys_approval_request`'s server-declared decision actions on the business record page, and retire the hard-coded two-button approval path (objectui#3055).
+
+A record with an approval pending on it showed exactly two buttons — Approve and Reject — hand-written into the record header behind a bespoke `type:'approval'` handler and a client-side approver test. The approvals list, looking at the same request over the same nine REST routes, offered five decisions plus the submitter's levers and took decision attachments. On a business record, **reassign / send back / request info had no entry point at all**, a decision could not carry a file, and the copy on the two surfaces was maintained separately.
+
+The record page now renders the object's own declared actions through the shared declared-action bar — the same metadata, the same action runtime, the same param dialogs the approvals list uses. Approve, Reject, Reassign, Send back and Request info reach a business record, with their declared params (comment, attachments, the new approver picker) and the per-request decision outputs an approval node declares. Remind stays with the approvals panel, which owns a richer, throttle-aware version of it. Adding a tenth decision action is now a metadata change with no console work.
+
+Two behaviour changes come with it:
+
+- **Who sees a decision is the server's answer, not the console's.** Visibility was `pending_approvers.includes(currentUserId)` evaluated in the browser; it is now each action's declared `visible` predicate over the server-computed `viewer` block (`can_act` / `is_submitter` / `can_override`) — the same block that gates the approvals list, computed by the same service that authorizes the decision. A platform or tenant admin's override levers, the recovery path for a request routed to an unstaffed position, now reach the record page for the first time. On a backend too old to send `viewer`, the predicate cannot be evaluated and no decision is offered rather than one whose precondition is unknown.
+- **A declared `visible` written against the canonical `record.` root now evaluates.** The declared-action bar passed the row in as the bare predicate scope, so only the shorthand spelling (`status == "pending"`) resolved; `record.viewer.can_act` raised `record is not defined`, and the fail-closed gate turned that into "hidden". Every declared action on `sys_approval_request` gates on `record.viewer.*`, so the whole server-declared decision set was invisible on every surface this bar renders, the approvals inbox included. The row now binds the three ways the record header and list rows bind it — `record.status`, bare `status`, `data.status` — so both spellings reach a verdict.
+
+`useRecordApprovals` keeps only its read half (status, `lock_record`, the request rows). Its `canDecide` / `approve` / `reject` members and its `currentUserId` parameter are gone: deciding is the declared action's POST, and every remaining question about the viewer is answered on the row by the server.

packages/app-shell/src/hooks/useRecordApprovals.quorum.test.tsx

@@ -94,7 +94,7 @@ function stubApi(detail: unknown, opts: { detailStatus?: number } = {}) {
 }
 
 const mount = () =>
-  renderHook(() => useRecordApprovals('showcase_expense_report', 'AyG40_bAHSP_gi8T', 'u_manager'));
+  renderHook(() => useRecordApprovals('showcase_expense_report', 'AyG40_bAHSP_gi8T'));
 
 describe('useRecordApprovals — quorum progress (objectstack#4478)', () => {
   beforeEach(() => stubApi(DETAIL_ROW));
@@ -144,8 +144,9 @@ describe('useRecordApprovals — quorum progress (objectstack#4478)', () => {
     const { result } = mount();
     await waitFor(() => expect(result.current.pendingRequest).toBeTruthy());
     expect(result.current.pendingRequest?.decision_progress).toBeUndefined();
-    // …and the decision surface the list read does support is still live.
-    expect(result.current.canDecide).toBe(true);
+    // …and everything the LIST read already carried is still live — the row the
+    // decision actions run against, and the lock the header reads.
+    expect(result.current.pendingRequest?.id).toBe('req_committee_1');
     expect(result.current.pendingRequest?.lock_record).toBe(true);
   });
 

packages/app-shell/src/hooks/useRecordApprovals.ts

@@ -1,16 +1,25 @@
 /**
  * useRecordApprovals
  *
- * Resolves the approval state for a single record so the detail-view header
- * can surface a status badge and — when the current user is a pending
- * approver — "Approve" / "Reject" actions.
+ * Resolves the approval state for a single record — the READ half only: the
+ * status badge, the record lock (`lock_record`), and the request rows the
+ * record page's approvals panel renders.
  *
  * Since ADR-0019 an approval is a **flow node** (`type: 'approval'`), not a
  * standalone process: the flow opens the request when it reaches the node,
- * and a decision resumes the run down its `approve` / `reject` edge. There is
- * therefore no manual "submit" or "recall" from the record header — those
- * endpoints were removed. This hook reads the record's requests and lets a
- * pending approver record a decision.
+ * and a decision resumes the run down its `approve` / `reject` edge.
+ *
+ * ⛔ This hook does NOT decide (objectui#3055). It used to own a second,
+ * hand-written decision path — an `approve()` / `reject()` pair plus a
+ * CLIENT-side `canDecide` (`pending_approvers.includes(currentUserId)`) — that
+ * the record page injected as two hard-coded header buttons. That fork covered
+ * two of the nine approval routes: reassign / send-back / request-info had zero
+ * entry point on a business record, decision attachments were impossible, and
+ * the copy was maintained separately from the approvals list's. Decisions now
+ * go through the SAME server-declared `sys_approval_request` actions the
+ * approvals list runs (`DeclaredActionsBar`), gated by the server-computed
+ * `viewer` block rather than by a second client-side opinion — so a new
+ * decision action is metadata, not console code.
  *
  * Talks directly to the framework REST endpoints under
  * `/api/v1/approvals/*`. Fails open: if the approvals plugin is not installed
@@ -182,27 +191,20 @@ interface UseRecordApprovalsResult {
    * node the flow has reached (and per ADR-0044 revision round), so a
    * multi-level flow accumulates several. The record page's approval panel
    * renders them all (objectui#3461); `pendingRequest` / `latestRequest`
-   * remain the derived single-row reads the header actions consume.
+   * remain the derived single-row reads the status badge and the decision
+   * bar's record consume.
    */
   requests: ApprovalRequestLite[];
+  /**
+   * The one still-`pending` request, enriched by `getRequest` — so it carries
+   * the server-computed `viewer` block the declared decision actions gate on
+   * (objectui#3055) as well as the `decision_progress` tally.
+   */
   pendingRequest: ApprovalRequestLite | null;
   latestRequest: ApprovalRequestLite | null;
-  /** The current user is among the pending approvers and may record a decision. */
-  canDecide: boolean;
-  approve: (input?: DecisionInput) => Promise<ApprovalRequestLite | undefined>;
-  reject: (input?: DecisionInput) => Promise<ApprovalRequestLite | undefined>;
   refresh: () => Promise<void>;
 }
 
-/**
- * What an approver submits with a decision: the free-text comment, plus the
- * node's declared decision outputs keyed by their declared `key` (objectui#2955).
- */
-export interface DecisionInput {
-  comment?: string;
-  outputs?: Record<string, any>;
-}
-
 function apiBase() {
   const url = (import.meta as any).env?.VITE_SERVER_URL || '';
   return `${String(url).replace(/\/$/, '')}/api/v1`;
@@ -290,10 +292,17 @@ export async function remindApprovalRequest(
   return out ?? {};
 }
 
+/**
+ * ⛔ No `currentUserId` parameter (objectui#3055). It existed only to feed the
+ * retired client-side `canDecide`; every remaining question about the viewer —
+ * may they act, are they the submitter, may they override — is answered by the
+ * server on the row itself (`viewer`, framework#3310 / #3424). A surface that
+ * still needs the signed-in id for a pre-`viewer` fallback (the panel's remind)
+ * reads it from `useAuth()` where it renders.
+ */
 export function useRecordApprovals(
   objectName: string | undefined,
   recordId: string | undefined,
-  currentUserId?: string | null,
 ): UseRecordApprovalsResult {
   const [loading, setLoading] = useState(false);
   const [available, setAvailable] = useState(true);
@@ -351,46 +360,18 @@ export function useRecordApprovals(
 
   const latestRequest = sortedRequests[0] ?? null;
 
-  const canDecide = !!pendingRequest && !!currentUserId
-    && (pendingRequest.pending_approvers ?? []).includes(currentUserId);
-
-  const decide = useCallback(
-    async (decision: 'approve' | 'reject', input?: DecisionInput) => {
-      if (!pendingRequest) throw new Error('No pending request');
-      const outputs = input?.outputs && Object.keys(input.outputs).length > 0 ? input.outputs : undefined;
-      const out = await fetchJson<{ request?: ApprovalRequestLite }>(
-        `/approvals/requests/${encodeURIComponent(pendingRequest.id)}/${decision}`,
-        {
-          method: 'POST',
-          body: JSON.stringify({
-            ...(currentUserId ? { actorId: currentUserId } : {}),
-            ...(input?.comment ? { comment: input.comment } : {}),
-            // The node's declared decision outputs, under the same nested key
-            // the Approval Center's `type:'api'` decide actions post
-            // (objectui#2955). Omitted entirely when nothing was collected, so
-            // a node without `decisionOutputs` posts the body it always did.
-            ...(outputs ? { outputs } : {}),
-          }),
-        },
-      );
-      await refresh();
-      return out?.request;
-    },
-    [pendingRequest, currentUserId, refresh],
-  );
-
-  const approve = useCallback((input?: DecisionInput) => decide('approve', input), [decide]);
-  const reject = useCallback((input?: DecisionInput) => decide('reject', input), [decide]);
+  // ⛔ No `canDecide` / `approve` / `reject` here (objectui#3055). Whether the
+  // viewer may act is the SERVER's answer (`pendingRequest.viewer`), read by
+  // the declared actions' own `visible` gate; recording the decision is the
+  // declared `type:'api'` action's POST. A client-side second opinion on the
+  // same question is what let the record page and the approvals list disagree.
 
   return {
     loading,
     available,
     requests: sortedRequests,
     pendingRequest,
     latestRequest,
-    canDecide,
-    approve,
-    reject,
     refresh,
   };
 }

packages/app-shell/src/views/DeclaredActionsBar.tsx

@@ -43,7 +43,16 @@ import { useObjectLabel, useObjectTranslation } from '@object-ui/i18n';
 import { Loader2 } from 'lucide-react';
 import { useConsoleActionRuntime } from '../hooks/useConsoleActionRuntime';
 import { useAdapter } from '../providers/AdapterProvider';
-import { useMetadataItem } from '../providers/MetadataProvider';
+// Straight from `@object-ui/react`, NOT through `../providers/MetadataProvider`
+// (which merely re-exports it). The provider module pulls in the console
+// metadata client factory, and that module builds its shared authenticated
+// fetch AT IMPORT TIME — so importing the hook by the convenient path drags an
+// eager side effect into the module graph of every host that renders this bar.
+// It surfaced when the record page started mounting the bar (objectui#3055):
+// two RecordDetailView suites died at import with `Cannot access
+// 'authFetchSpy' before initialization`, the side effect running inside the
+// hoisted `@object-ui/auth` mock factory before the spy existed.
+import { useMetadataItem } from '@object-ui/react';
 import { decisionOutputDefs, decisionOutputParams } from '../utils/decisionOutputParams';
 import { getIcon } from '../utils/getIcon';
 
@@ -113,10 +122,38 @@ const DeclaredActionButton: React.FC<{
   const { t } = useObjectTranslation();
 
   const recordData = record != null && typeof record === 'object' ? (record as Record<string, any>) : {};
+  /**
+   * The predicate scope, with the record bound the THREE ways the platform's
+   * row surfaces bind it (objectui#3055).
+   *
+   * The bar used to hand the row in as the bare context bag, so only the
+   * shorthand spelling — `status == "pending"` — resolved. The CANONICAL
+   * spelling is the `record.` root: it is what `ExpressionEvaluator`'s CEL path
+   * binds (`bag.record` as the record namespace), what `evalRowPredicate` binds
+   * on the record header and on list rows, and what the server itself
+   * enforces with. Under a root-only bag `record.viewer.can_act` does not read
+   * as false — it throws `record is not defined`, and `throwOnError` turns that
+   * into "hidden".
+   *
+   * Which is not hypothetical: EVERY declared action on `sys_approval_request`
+   * gates on `record.viewer.*` (framework#3310 / #3424), so the whole
+   * server-declared decision set was invisible on every surface this bar
+   * renders. The record page's two hand-written buttons were, in practice, the
+   * only decision UI that could still be reached — the fork objectui#3055 is
+   * about, kept alive by the "full" path being unable to evaluate its own gate.
+   *
+   * `record` / `data` are written AFTER the spread, so a row that happens to
+   * carry a field of either name cannot shadow the namespace a predicate means.
+   */
+  const predicateRecord = useMemo(
+    () => ({ ...recordData, record: recordData, data: recordData }),
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    [record],
+  );
   // `visible` fails CLOSED on a throwing predicate — mirrors action:button and
   // ActionEngine.getActionsForLocation: a guard that can't be evaluated hides
   // the action rather than exposing one whose precondition is broken.
-  const isVisible = useCondition(toPredicateInput((action as any).visible), recordData, {
+  const isVisible = useCondition(toPredicateInput((action as any).visible), predicateRecord, {
     throwOnError: true,
     label: `declared action "${action.name ?? action.label ?? 'action'}" (visible)`,
   });
@@ -125,7 +162,7 @@ const DeclaredActionButton: React.FC<{
   // this bar ignored it, so a spec-authored `disabled` guard on a declared
   // action did nothing here. (No legacy `enabled` fallback: server-declared
   // actions are spec-shaped and never carried the non-spec key.)
-  const isDisabledPred = useCondition(toPredicateInput((action as any).disabled), recordData);
+  const isDisabledPred = useCondition(toPredicateInput((action as any).disabled), predicateRecord);
 
   const handleClick = useCallback(async () => {
     if (loading) return;