From e65f0165efa55a1e881f9f915a6b614af4677c51 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Jul 2026 14:11:09 +0000 Subject: [PATCH] fix(approvals): Approval Center triage + drawer readability pass (#2762 P1-2/3/4/5, P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Inbox/cards surface the request amount inline (from the snapshot, preferring server-formatted payload_display); sort adds oldest-first and amount high→low next to newest-first (P1-3) - Flow-/system-initiated requests read "Flow-initiated" with a workflow icon instead of a bare person icon + "—" (desktop, mobile, drawer) (P1-4) - "Waiting on" chips dedupe a repeated approver to one chip with a ×N count, tooltip keeping every underlying id (P1-2) - DeclaredActionsBar maps the spec action variant enum onto Button variants (primary→default, danger→destructive) so Approve/Reject get hierarchy (P1-5) - Resolved lookup keys render as "Owner", not "Owner Id" (P2) - New approvalsInbox keys (flowOrigin, sort*) in all ten locales Refs objectui#2762 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Cu48mLFUdRBmMh8Z8R3CVz --- .changeset/approval-center-triage-pass.md | 27 +++ .../src/pages/system/ApprovalsInboxPage.tsx | 182 ++++++++++++++++-- .../src/views/DeclaredActionsBar.tsx | 12 +- .../__tests__/DeclaredActionsBar.test.tsx | 23 +++ packages/i18n/src/locales/ar.ts | 5 + packages/i18n/src/locales/de.ts | 5 + packages/i18n/src/locales/en.ts | 5 + packages/i18n/src/locales/es.ts | 5 + packages/i18n/src/locales/fr.ts | 5 + packages/i18n/src/locales/ja.ts | 5 + packages/i18n/src/locales/ko.ts | 5 + packages/i18n/src/locales/pt.ts | 5 + packages/i18n/src/locales/ru.ts | 5 + packages/i18n/src/locales/zh.ts | 5 + 14 files changed, 275 insertions(+), 19 deletions(-) create mode 100644 .changeset/approval-center-triage-pass.md diff --git a/.changeset/approval-center-triage-pass.md b/.changeset/approval-center-triage-pass.md new file mode 100644 index 0000000000..cd7d17eee2 --- /dev/null +++ b/.changeset/approval-center-triage-pass.md @@ -0,0 +1,27 @@ +--- +"@object-ui/app-shell": patch +"@object-ui/i18n": patch +--- + +fix(approvals): Approval Center triage + drawer readability pass (#2762 P1-2/P1-3/P1-4/P1-5/P2) + +- **Decision-relevant data in the queue (P1-3)** — list rows and mobile cards + now surface the request's amount/total inline (detected from the snapshot, + preferring the server-formatted `payload_display` value), so a reviewer can + triage without opening each request. A sort control adds "Oldest first" and + "Amount (high→low)" alongside the default newest-first. +- **Empty applicant column (P1-4)** — flow-/system-initiated requests (no human + submitter) now read "Flow-initiated" with a workflow icon instead of a bare + person icon + "—", in the desktop table, mobile card, and drawer. +- **Approver chips deduped (P1-2)** — a person filling more than one approver + slot rendered as N identical "Waiting on" chips; they collapse to one chip + with a ×N count, the tooltip keeping every underlying id. +- **Action hierarchy (P1-5)** — `DeclaredActionsBar` maps the spec action + `variant` enum onto the Button variants (`primary` → filled default, + `danger` → destructive), so the drawer's Approve stands out and Reject reads + as destructive once `@objectstack/plugin-approvals` declares them. +- **Label polish (P2)** — `owner_id`-style resolved lookup keys render as + "Owner", not the awkward "Owner Id", in the drawer summary. + +New `approvalsInbox` keys (`flowOrigin`, `sortBy`/`sortRecent`/`sortOldest`/ +`sortAmount`) added to all ten locales. diff --git a/apps/console/src/pages/system/ApprovalsInboxPage.tsx b/apps/console/src/pages/system/ApprovalsInboxPage.tsx index fd91f69302..fa20451132 100644 --- a/apps/console/src/pages/system/ApprovalsInboxPage.tsx +++ b/apps/console/src/pages/system/ApprovalsInboxPage.tsx @@ -82,10 +82,12 @@ import { AlertCircle, CheckSquare, Search, + ArrowUpDown, Copy, X, ExternalLink, User as UserIcon, + Workflow, ChevronLeft, ChevronRight, Send, @@ -159,6 +161,38 @@ function submitterDisplay(r: ApprovalRequestRow): string { function approverDisplay(a: string, r: ApprovalRequestRow): string { return r.pending_approver_names?.[a] || formatIdentity(a); } +/** + * Dedupe the pending-approver chips by display label (#2762 P1-2): a person + * who fills more than one approver slot showed up as N identical chips. Collapse + * them to one chip carrying a count, preserving first-seen order; the tooltip + * keeps every underlying id so the raw slots stay inspectable. + */ +function approverChips(r: ApprovalRequestRow): Array<{ label: string; count: number; title: string }> { + const order: string[] = []; + const byLabel = new Map(); + for (const a of r.pending_approvers || []) { + const label = approverDisplay(a, r); + const seen = byLabel.get(label); + if (seen) { + seen.count += 1; + if (a && !seen.title.split(', ').includes(a)) seen.title += `, ${a}`; + } else { + byLabel.set(label, { label, count: 1, title: a || label }); + order.push(label); + } + } + return order.map((l) => byLabel.get(l)!); +} +/** + * A request with no human submitter — flow- or system-initiated (#2762 P1-4). + * These rows have an empty `submitter_id` or a synthetic `flow:` / `system:` + * actor, and rendering a bare person icon + "—" reads as missing data. + */ +function isSystemSubmitter(r: ApprovalRequestRow): boolean { + if (r.submitter_name) return false; + const id = (r.submitter_id || '').trim(); + return !id || id.startsWith('flow:') || id.startsWith('system:'); +} /** Object subtitle: schema label when resolved, else the machine name. */ function objectDisplay(r: ApprovalRequestRow): string { return r.object_label || r.object_name; @@ -197,7 +231,12 @@ const PAYLOAD_SYSTEM_KEYS = new Set([ ]); function prettifyKey(k: string): string { - return k.split('_').filter(Boolean).map(w => w.charAt(0).toUpperCase() + w.slice(1)).join(' '); + const tokens = k.split('_').filter(Boolean); + // Drop a trailing `id` token so a resolved lookup key reads as its subject — + // `owner_id` → "Owner", not the awkward "Owner Id" (#2762 P2). Keep at least + // one token (a bare `id` is already dropped as a system key upstream). + if (tokens.length > 1 && tokens[tokens.length - 1].toLowerCase() === 'id') tokens.pop(); + return tokens.map(w => w.charAt(0).toUpperCase() + w.slice(1)).join(' '); } function formatPayloadValue(key: string, v: unknown): string { @@ -246,6 +285,36 @@ function payloadSummary( return out; } +/** + * Amount-like keys worth surfacing in the queue so a reviewer can triage + * without opening each request (#2762 P1-3). Deliberately narrow — a decision + * turns on the amount/total/budget, not on every numeric field. + */ +const AMOUNT_KEY_RE = /(amount|total|price|value|cost|sum|budget|salary|fee|revenue|balance|金额|总额|价格|费用|预算|金额)/i; + +/** + * The one decision-relevant numeric field (amount/total/…) of the snapshot, + * for the inline list display and amount sort. Prefers the server-formatted + * `payload_display` value (currency, etc.) but always keeps the raw number for + * ordering. Null when the snapshot has no such field. + */ +function decisionAmountEntry( + r: ApprovalRequestRow, +): { label: string; value: number; display: string } | null { + const payload = r.payload; + if (!payload || typeof payload !== 'object' || Array.isArray(payload)) return null; + for (const [k, v] of Object.entries(payload as Record)) { + if (PAYLOAD_SYSTEM_KEYS.has(k)) continue; + if (!AMOUNT_KEY_RE.test(k)) continue; + const num = typeof v === 'number' + ? v + : (typeof v === 'string' && v.trim() !== '' && Number.isFinite(Number(v)) ? Number(v) : null); + if (num == null || !Number.isFinite(num)) continue; + return { label: prettifyKey(k), value: num, display: r.payload_display?.[k] ?? num.toLocaleString() }; + } + return null; +} + export function ApprovalsInboxPage() { const { t, language } = useObjectTranslation(); const { user } = useAuth(); @@ -389,6 +458,10 @@ export function ApprovalsInboxPage() { const [processFilter, setProcessFilter] = useState('all'); const [objectFilter, setObjectFilter] = useState('all'); const [statusFilter, setStatusFilter] = useState('all'); + // Client-side ordering of the visible rows (#2762 P1-3). Default keeps the + // server's newest-first; the others let a reviewer triage by wait time or by + // the decision-relevant amount. + const [sortKey, setSortKey] = useState<'recent' | 'oldest' | 'amount'>('recent'); // Bulk selection (only meaningful on "pending" tab where the user can act) const [selectedRowIds, setSelectedRowIds] = useState>(new Set()); @@ -648,10 +721,10 @@ export function ApprovalsInboxPage() { return Array.from(set).sort(); }, [rows]); - /** Client-side filtered rows shown in table. */ + /** Client-side filtered + sorted rows shown in table. */ const filteredRows = useMemo(() => { const q = query.trim().toLowerCase(); - return rows.filter(r => { + const matched = rows.filter(r => { if (processFilter !== 'all' && processLabel(r) !== processFilter) return false; if (objectFilter !== 'all' && r.object_name !== objectFilter) return false; if (statusFilter !== 'all' && r.status !== statusFilter) return false; @@ -667,7 +740,25 @@ export function ApprovalsInboxPage() { ].filter(Boolean).join(' ').toLowerCase(); return hay.includes(q); }); - }, [rows, query, processFilter, objectFilter, statusFilter, tab]); + if (sortKey === 'recent') return matched; // server order is already newest-first + const sorted = [...matched]; + if (sortKey === 'amount') { + // Highest amount first; rows without a detectable amount sink to the + // bottom (keeping their relative newest-first order). + sorted.sort((a, b) => { + const av = decisionAmountEntry(a)?.value; + const bv = decisionAmountEntry(b)?.value; + if (av == null && bv == null) return 0; + if (av == null) return 1; + if (bv == null) return -1; + return bv - av; + }); + } else { + // Oldest first — flip the newest-first submitted timestamp. + sorted.sort((a, b) => (submittedAt(a) || '').localeCompare(submittedAt(b) || '')); + } + return sorted; + }, [rows, query, processFilter, objectFilter, statusFilter, tab, sortKey]); /** Position of the open request within the visible list (drawer prev/next). */ const drawerIndex = useMemo( () => (selectedId ? filteredRows.findIndex(r => r.id === selectedId) : -1), @@ -876,6 +967,7 @@ export function ApprovalsInboxPage() { setProcessFilter('all'); setObjectFilter('all'); setQuery(''); + setSortKey('recent'); setFocusIndex(-1); }; @@ -898,6 +990,9 @@ export function ApprovalsInboxPage() { } function RecordCell({ r }: { r: ApprovalRequestRow }) { + // Surface the decision-relevant amount inline so a reviewer can triage the + // queue without opening each request (#2762 P1-3). + const amount = decisionAmountEntry(r); return (
{r.record_title || formatIdentity(r.record_id)} -
{objectDisplay(r)}
+
+ {objectDisplay(r)} + {amount && ( + + · {amount.display} + + )} +
); } @@ -1047,6 +1149,19 @@ export function ApprovalsInboxPage() { )} + {/* Triage ordering (#2762 P1-3): newest by default, or by wait + time / decision amount. */} + {hasFilters && ( {tr('filterCount', '{{shown}} of {{total}}', { shown: filteredRows.length, total: rows.length })} @@ -1211,10 +1326,19 @@ export function ApprovalsInboxPage() { -
- - {submitterDisplay(r)} -
+ {isSystemSubmitter(r) ? ( + // Flow-/system-initiated: name the origin instead of a + // bare person icon + "—" (#2762 P1-4). +
+ + {tr('flowOrigin', 'Flow-initiated')} +
+ ) : ( +
+ + {submitterDisplay(r)} +
+ )} {r.record_title || formatIdentity(r.record_id)} {objectDisplay(r)} + {(() => { + const amount = decisionAmountEntry(r); + return amount ? ( + · {amount.display} + ) : null; + })()}
- - {submitterDisplay(r)} - + {isSystemSubmitter(r) ? ( + + {tr('flowOrigin', 'Flow-initiated')} + + ) : ( + + {submitterDisplay(r)} + + )} {formatRelative(submittedAt(r))} @@ -1437,8 +1573,17 @@ export function ApprovalsInboxPage() {
- - {submitterDisplay(selected)} + {isSystemSubmitter(selected) ? ( + <> + + {tr('flowOrigin', 'Flow-initiated')} + + ) : ( + <> + + {submitterDisplay(selected)} + + )}
@@ -1537,9 +1682,12 @@ export function ApprovalsInboxPage() { {tr('waitingOn', 'Waiting on')}
- {(selected.pending_approvers || []).map((a, i) => ( - - {approverDisplay(a, selected)} + {approverChips(selected).map((chip) => ( + + {chip.label} + {chip.count > 1 && ( + ×{chip.count} + )} ))}
diff --git a/packages/app-shell/src/views/DeclaredActionsBar.tsx b/packages/app-shell/src/views/DeclaredActionsBar.tsx index 06d042d408..26a87936e4 100644 --- a/packages/app-shell/src/views/DeclaredActionsBar.tsx +++ b/packages/app-shell/src/views/DeclaredActionsBar.tsx @@ -153,9 +153,17 @@ const DeclaredActionButton: React.FC<{ if ((action as any).visible && !isVisible) return null; const iconName = typeof (action as any).icon === 'string' ? (action as any).icon as string : undefined; - const variant = (action as any).variant === 'primary' + // Map the spec's action `variant` enum (primary|secondary|danger|ghost|link) + // onto the Button's variants. `primary` → the filled default, `danger` → + // `destructive` (the two names the enum and the Button component spell + // differently); the rest pass through, and an undeclared variant stays + // `outline` so a plain declared action still reads as a secondary button. + const declaredVariant = (action as any).variant; + const variant = declaredVariant === 'primary' ? 'default' - : ((action as any).variant || 'outline'); + : declaredVariant === 'danger' + ? 'destructive' + : (declaredVariant || 'outline'); const fallbackLabel = action.label || action.name || ''; const label = action.name ? actionLabel(objectName, action.name, fallbackLabel) : fallbackLabel; diff --git a/packages/app-shell/src/views/__tests__/DeclaredActionsBar.test.tsx b/packages/app-shell/src/views/__tests__/DeclaredActionsBar.test.tsx index e4c5db8186..029650bed3 100644 --- a/packages/app-shell/src/views/__tests__/DeclaredActionsBar.test.tsx +++ b/packages/app-shell/src/views/__tests__/DeclaredActionsBar.test.tsx @@ -172,6 +172,29 @@ describe('DeclaredActionsBar', () => { expect(dispatch.params).toEqual({ _rowRecord: REQUEST }); }); + it('maps the spec action `variant` onto Button variants', () => { + render( + , + ); + // primary → the filled default; danger → destructive (the two names the + // spec enum and Button component spell differently); undeclared → outline; + // the rest pass through unchanged. + expect(screen.getByTestId('declared-action-a_primary')).toHaveAttribute('variant', 'default'); + expect(screen.getByTestId('declared-action-a_danger')).toHaveAttribute('variant', 'destructive'); + expect(screen.getByTestId('declared-action-a_plain')).toHaveAttribute('variant', 'outline'); + expect(screen.getByTestId('declared-action-a_ghost')).toHaveAttribute('variant', 'ghost'); + }); + it('renders a labeled divider only when actions are present', () => { render(