diff --git a/.changeset/approvals-queue-hidden-amount-6020.md b/.changeset/approvals-queue-hidden-amount-6020.md new file mode 100644 index 0000000000..3c0b81e920 --- /dev/null +++ b/.changeset/approvals-queue-hidden-amount-6020.md @@ -0,0 +1,18 @@ +--- +"@object-ui/console": patch +--- + +Approvals inbox: the queue rows and the amount sort now honour each request +object's own `hidden: true` field declaration (objectui#6020). + +The `hidden: true` trim added for the drawer summary card reached only the +drawer — the desktop queue row, the mobile card and the amount comparator +still read the field, so an amount an app author declared hidden rendered +inline in the queue and ordered the list, which leaked its relative magnitude +even to a viewer who never saw the figure. + +The queue spans many objects, so the trim is a per-object lookup and every row +is answered about its own object; a row left with no renderable amount now +sorts with the other amount-less rows. `hidden` stays a UI contract +(objectstack#10749) and the filter still fails open: an unanswered or failed +metadata read renders today's figure. diff --git a/apps/console/src/pages/system/ApprovalsInboxPage.queueHiddenAmount.test.tsx b/apps/console/src/pages/system/ApprovalsInboxPage.queueHiddenAmount.test.tsx new file mode 100644 index 0000000000..97011ed744 --- /dev/null +++ b/apps/console/src/pages/system/ApprovalsInboxPage.queueHiddenAmount.test.tsx @@ -0,0 +1,481 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Approvals QUEUE — the rows, and the amount sort, honour each row's own + * object `hidden: true` declaration (objectui#6020). + * + * ## The defect + * + * objectui#5565 put the filter INSIDE `decisionAmountEntry` behind an optional + * `hiddenKeys` parameter, and passed it at exactly one of five call sites — the + * drawer. The desktop row, the mobile card and both halves of the amount + * comparator called it bare, so a field the author declared `hidden: true` + * still rendered inline in the queue and still ordered the list. A filter + * present in a function body but unpassed at the call site reads exactly like a + * fixed defect, which is why this file measures the CALL SITES. + * + * ## ⚠️ Why the fixture spans TWO objects — the naive fix passes a one-object test + * + * The obvious repair is to thread the page's existing `hiddenPayloadKeys` into + * the four sites. That set is keyed to the OPEN request (`useHiddenFields( + * selected?.object_name)`), while the queue is N rows spanning K objects, so it + * would apply one object's declarations to every row: fields hidden on rows + * whose object never declared them, fields missed on rows whose object did. + * A single-object fixture cannot tell that apart from the real fix, so the + * fixture below is two objects with DIFFERENT declarations and asserts both + * directions: + * + * showcase_purchase — declares `total_amount` hidden + * showcase_invoice — declares `service_fee` hidden, `total_amount` NOT + * + * row P (purchase) `total_amount` → trimmed … the reported defect + * row I1 (invoice) `total_amount` → RENDERS … ⭐ the naive fix hides this + * row I2 (invoice) `service_fee` → trimmed … ⭐ a one-object fix misses this + * + * ## Why the trimmed row still shows a figure + * + * Row P carries a second, undeclared amount key (`freight_cost`) after the + * hidden one. Trimmed, the row renders THAT — so the drop happens inside the + * scan, before the field is chosen, and "the amount is gone" cannot be + * satisfied by a row that simply stopped rendering amounts. It is the same + * promotion signal `ApprovalsInboxPage.hiddenFieldTrim.test.tsx` uses for the + * drawer's 6-field cut, transposed onto the pick. + * + * ## Why every figure is asserted TWICE + * + * The queue ships two surfaces — the desktop table row and the `md:hidden` + * mobile card — and both are in the DOM here (no stylesheet applies media + * queries). A rendered figure is therefore exactly 2 nodes and a trimmed one + * exactly 0, which fails a fix that repairs one surface and forgets the other. + * + * ## The sort is the half that leaks without rendering anything + * + * Ordering IS disclosure: sorting on a hidden figure tells a viewer who never + * sees it how it compares to every other row. The posture pinned below is + * **the queue orders on exactly the figure it renders** — a row left with no + * renderable amount sinks with the other amount-less rows, which is the + * behaviour that surface already has for a request with no amount at all. + * + * ## Counter-probes + * + * "The figure is gone" and "the row sank" are both satisfiable by breaking the + * queue. So every denial case asserts the three record titles are still there, + * and each half has a counter-probe running the SAME fixture with nothing + * declared hidden, where the figure renders and the row sorts where its amount + * puts it. + * + * No build artifact sits between the edit and this test: the root Vitest config + * aliases every `@object-ui` specifier at that package's source directory, and + * the page under test is this app's own source. + */ + +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, beforeAll, beforeEach, afterEach } from 'vitest'; +import { render, screen, cleanup, within, waitFor, fireEvent } from '@testing-library/react'; +import { MemoryRouter, Routes, Route } from 'react-router-dom'; +import { MePermissionsProvider, type MePermissionsResponse } from '@object-ui/permissions'; + +const APP = 'com.objectstack.account'; +const PURCHASE = 'showcase_purchase'; +const INVOICE = 'showcase_invoice'; + +/** `showcase_purchase` declares this hidden — must never reach the queue. */ +const HIDDEN_PURCHASE_AMOUNT = 'USD 99,000.00'; +/** Second amount key on the same row: renders ONLY once the hidden one drops. */ +const PROMOTED_PURCHASE_AMOUNT = 'USD 320.00'; +/** `showcase_invoice` does NOT declare `total_amount` hidden — must render. */ +const VISIBLE_INVOICE_AMOUNT = 'USD 5,000.00'; +/** `showcase_invoice` DOES declare `service_fee` hidden — must not render. */ +const HIDDEN_INVOICE_FEE = 'USD 250.00'; + +const TITLES = ['PO-4417', 'INV-8801', 'INV-8802']; + +const { approvalsApiStub, getObjectSchema, SCHEMAS, ADAPTER, AUTH, I18N, ROWS } = vi.hoisted(() => { + /** Three rows, two objects — see the header on why one object cannot do. */ + const ROWS: Array> = [ + { + id: 'req_p', + process_name: 'purchase_approval', + process_label: 'Purchase Approval', + object_name: 'showcase_purchase', + object_label: 'Purchase', + record_id: 'po_1', + record_title: 'PO-4417', + status: 'pending', + pending_approvers: ['u_1'], + submitter_id: 'u_2', + submitter_name: 'Sam Submitter', + submitted_at: '2026-08-20T03:00:00.000Z', + payload: { + total_amount: 99000, + subject: 'Ventilator service contract', + freight_cost: 320, + }, + payload_display: { total_amount: 'USD 99,000.00', freight_cost: 'USD 320.00' }, + }, + { + id: 'req_i1', + process_name: 'invoice_approval', + process_label: 'Invoice Approval', + object_name: 'showcase_invoice', + object_label: 'Invoice', + record_id: 'inv_1', + record_title: 'INV-8801', + status: 'pending', + pending_approvers: ['u_1'], + submitter_id: 'u_2', + submitter_name: 'Sam Submitter', + submitted_at: '2026-08-20T02:00:00.000Z', + payload: { total_amount: 5000, subject: 'Quarterly retainer' }, + payload_display: { total_amount: 'USD 5,000.00' }, + }, + { + id: 'req_i2', + process_name: 'invoice_approval', + process_label: 'Invoice Approval', + object_name: 'showcase_invoice', + object_label: 'Invoice', + record_id: 'inv_2', + record_title: 'INV-8802', + status: 'pending', + pending_approvers: ['u_1'], + submitter_id: 'u_2', + submitter_name: 'Sam Submitter', + submitted_at: '2026-08-20T01:00:00.000Z', + payload: { service_fee: 250, subject: 'Filing service' }, + payload_display: { service_fee: 'USD 250.00' }, + }, + ]; + + /** Per-object metadata, rewritten by each test through `declare()`. */ + const SCHEMAS: Record = {}; + const getObjectSchema = vi.fn(async (name: string): Promise => { + const schema = SCHEMAS[name]; + if (schema === undefined) return { fields: {} }; + if (schema instanceof Error) throw schema; + return schema; + }); + + const approvalsApiStub = { + listRequests: vi.fn(async () => ({ data: ROWS, total: ROWS.length })), + getRequest: vi.fn(async () => ({ data: ROWS[0] })), + listActions: vi.fn(async () => ({ data: [] })), + approve: vi.fn(async () => ({ data: ROWS[0], finalized: true })), + reject: vi.fn(async () => ({ data: ROWS[0], finalized: true })), + }; + + // STABLE singletons: a mocked hook handing back a fresh object per render + // re-runs the page's load effect forever and the queue never settles. + const ADAPTER = { + // Echo the probed ids back so every row stays readable (objectui#5211) and + // the record link is not what this file is measuring. + find: vi.fn(async (_object: string, params?: Record) => { + const ids: string[] = params?.$filter?.id?.$in ?? []; + return { data: ids.map((id) => ({ id })) }; + }), + getObjectSchema, + }; + const AUTH = { user: { id: 'u_1', email: 'approver@example.com' } }; + const I18N = { + t: (key: string, options?: Record) => String(options?.defaultValue ?? key), + language: 'en', + }; + + return { approvalsApiStub, getObjectSchema, SCHEMAS, ADAPTER, AUTH, I18N, ROWS }; +}); + +vi.mock('@object-ui/i18n', () => ({ useObjectTranslation: () => I18N })); + +vi.mock('@object-ui/auth', () => { + const authFetch = vi.fn(async () => new Response('{}', { status: 200 })); + return { + useAuth: () => AUTH, + createAuthenticatedFetch: () => authFetch, + TokenStorage: { get: () => null }, + }; +}); + +vi.mock('@object-ui/app-shell', () => ({ + useAdapter: () => ADAPTER, + DeclaredActionsBar: () => null, + isViaOverrideRow: () => false, +})); + +vi.mock('../../services/approvalsApi', async (importOriginal) => ({ + ...(await importOriginal>()), + approvalsApi: approvalsApiStub, +})); + +// Imported after the mocks so the page picks them up. +import { ApprovalsInboxPage } from './ApprovalsInboxPage'; + +function permissionsPayload(): MePermissionsResponse { + return { + authenticated: true, + userId: 'u_1', + tenantId: 't_1', + roles: [], + permissionSets: [], + objects: {}, + fields: {}, + // A real, reported grant that is NOT platform-admin-only — the same + // business-approver posture the drawer trim test renders under. + systemPermissions: ['setup.access'], + }; +} + +/** Point an object's metadata at a schema (or at a read that throws). */ +function declare(objectName: string, schema: unknown): void { + SCHEMAS[objectName] = schema; +} + +/** Both objects, as the app author wrote them for the defect case. */ +function declareTheReportedHiddenFields(): void { + declare(PURCHASE, { + fields: { + total_amount: { type: 'currency', hidden: true }, + freight_cost: { type: 'currency' }, + subject: { type: 'text' }, + }, + }); + declare(INVOICE, { + fields: { + total_amount: { type: 'currency' }, + service_fee: { type: 'currency', hidden: true }, + subject: { type: 'text' }, + }, + }); +} + +/** The same two objects with nothing hidden — the counter-probe. */ +function declareNothingHidden(): void { + declare(PURCHASE, { + fields: { + total_amount: { type: 'currency' }, + freight_cost: { type: 'currency' }, + subject: { type: 'text' }, + }, + }); + declare(INVOICE, { + fields: { + total_amount: { type: 'currency' }, + service_fee: { type: 'currency' }, + subject: { type: 'text' }, + }, + }); +} + +function renderQueue() { + return render( + + + + } /> + + + , + ); +} + +/** Render and settle: the queue has its rows AND both objects have answered. */ +async function loadedQueue(): Promise { + renderQueue(); + await screen.findAllByText(TITLES[0]); + await waitFor(() => { + expect(getObjectSchema).toHaveBeenCalledWith(PURCHASE); + expect(getObjectSchema).toHaveBeenCalledWith(INVOICE); + }); +} + +/** + * The leaf nodes carrying an inline figure (`· USD 5,000.00`). + * + * Leaf-only so an ancestor cannot match, and the count is the assertion: 2 for + * a rendered figure (desktop row + mobile card), 0 for a trimmed one. + */ +function amountNodes(display: string): HTMLElement[] { + return screen.queryAllByText((_content, element) => { + if (!element || element.children.length > 0) return false; + return element.textContent?.replace(/\s+/g, ' ').trim() === `· ${display}`; + }); +} + +/** The queue really rendered its rows — run by every denial case. */ +function expectQueueIntact(): void { + for (const title of TITLES) { + expect(screen.getAllByText(title).length).toBeGreaterThan(0); + } +} + +/** Record titles in desktop table order — the amount sort's observable. */ +function desktopRowTitles(): string[] { + const table = screen.getByRole('table'); + return within(table) + .getAllByRole('row') + .slice(1) // header + .map((row) => TITLES.find((t) => row.textContent?.includes(t)) ?? '?'); +} + +/** Switch the queue to "Amount (high→low)" the way a reviewer does. */ +async function sortByAmount(): Promise { + const trigger = Array.from(document.body.querySelectorAll('[role="combobox"]')) + .find((el) => el.textContent?.includes('Newest first')); + expect(trigger, 'the sort select is rendered').toBeTruthy(); + fireEvent.pointerDown(trigger!, { button: 0, ctrlKey: false, pointerType: 'mouse' }); + fireEvent.click(await screen.findByRole('option', { name: 'Amount (high→low)' })); + // Settled: the default newest-first order leads with INV-8802, every + // amount order with PO-4417 — so this waits for the sort, not for a paint. + await waitFor(() => expect(desktopRowTitles()[0]).toBe('PO-4417')); +} + +beforeAll(() => { + // Radix Select drives its trigger off pointer events, which happy-dom does + // not implement — the same shim the components-side Select tests install. + class MockPointerEvent extends Event { + button: number; + ctrlKey: boolean; + pointerType: string; + constructor(type: string, props: any = {}) { + super(type, props); + this.button = props.button ?? 0; + this.ctrlKey = props.ctrlKey ?? false; + this.pointerType = props.pointerType ?? 'mouse'; + } + } + (window as any).PointerEvent = MockPointerEvent; + (HTMLElement.prototype as any).hasPointerCapture = vi.fn(); + (HTMLElement.prototype as any).releasePointerCapture = vi.fn(); + (HTMLElement.prototype as any).scrollIntoView = vi.fn(); +}); + +beforeEach(() => { + ADAPTER.find.mockClear(); + getObjectSchema.mockClear(); + for (const key of Object.keys(SCHEMAS)) delete SCHEMAS[key]; + for (const fn of Object.values(approvalsApiStub)) fn.mockClear(); + approvalsApiStub.listRequests.mockResolvedValue({ data: ROWS, total: ROWS.length }); +}); +afterEach(cleanup); + +describe('Approvals queue rows — per-object `hidden: true` trim (objectui#6020)', () => { + it('trims the figure each row’s OWN object declares hidden, and only that one', async () => { + declareTheReportedHiddenFields(); + await loadedQueue(); + + // The reported defect: the purchase row's hidden amount. + await waitFor(() => expect(amountNodes(HIDDEN_PURCHASE_AMOUNT)).toHaveLength(0)); + // …and the row still carries an amount — its next, undeclared one — so the + // drop is inside the scan, not the row giving up on amounts. + expect(amountNodes(PROMOTED_PURCHASE_AMOUNT)).toHaveLength(2); + + // ⭐ The invoice object does NOT declare `total_amount` hidden. A fix that + // threads the drawer's single set through the queue hides this figure. + expect(amountNodes(VISIBLE_INVOICE_AMOUNT)).toHaveLength(2); + + // ⭐ The invoice object DOES declare `service_fee` hidden. A fix that reads + // only one object's declarations leaves this figure on screen. + expect(amountNodes(HIDDEN_INVOICE_FEE)).toHaveLength(0); + + expectQueueIntact(); + }); + + it('COUNTER-PROBE: same rows, nothing declared hidden — every figure renders', async () => { + declareNothingHidden(); + await loadedQueue(); + + expect(amountNodes(HIDDEN_PURCHASE_AMOUNT)).toHaveLength(2); + expect(amountNodes(VISIBLE_INVOICE_AMOUNT)).toHaveLength(2); + expect(amountNodes(HIDDEN_INVOICE_FEE)).toHaveLength(2); + // Untrimmed, the purchase row leads with the first amount key, so the + // second one stays out — the mirror image of the promotion above. + expect(amountNodes(PROMOTED_PURCHASE_AMOUNT)).toHaveLength(0); + expectQueueIntact(); + }); + + it('FAILS OPEN: a source that cannot describe the objects renders today’s figures', async () => { + declare(PURCHASE, Object.assign(new Error('Forbidden'), { status: 403 })); + declare(INVOICE, Object.assign(new Error('Forbidden'), { status: 403 })); + await loadedQueue(); + + // Unknown is not hidden: the queue must not degrade an approver's decision + // surface on a metadata error. See `hiddenFields.ts` on failing open. + expect(amountNodes(HIDDEN_PURCHASE_AMOUNT)).toHaveLength(2); + expect(amountNodes(HIDDEN_INVOICE_FEE)).toHaveLength(2); + expectQueueIntact(); + }); + + it('costs one metadata read per distinct OBJECT, not one per row', async () => { + declareTheReportedHiddenFields(); + await loadedQueue(); + + // Three rows, two objects. Per-row reads would be 3 — and would grow with + // the page. `planHiddenFieldReads` is the cost model; this pins it at the + // call site. + await waitFor(() => expect(getObjectSchema).toHaveBeenCalledTimes(2)); + expect(getObjectSchema.mock.calls.map(([name]) => name).sort()) + .toEqual([INVOICE, PURCHASE].sort()); + }); +}); + +describe('Approvals amount sort — ordering is disclosure too (objectui#6020)', () => { + /** + * The hidden figure is deliberately the MIDDLE magnitude, and the three + * orders are three DIFFERENT permutations — so "sorted", "not sorted" and + * "sorted without the hidden figure" can never be confused for one another: + * + * default, newest-first INV-8802 · INV-8801 · PO-4417 + * amount, undeclared PO-4417(9,000) · INV-8801(5,000) · INV-8802(900) + * amount, declared hidden PO-4417(9,000) · INV-8802(900) · INV-8801(sunk) + * + * Each row's amount key is one its OWN object leaves visible, except + * INV-8801's `service_fee` — the one `showcase_invoice` declares hidden. + */ + const SORT_ROWS = [ + { + ...ROWS[0], + submitted_at: '2026-08-20T01:00:00.000Z', + payload: { freight_cost: 9000 }, + payload_display: { freight_cost: 'USD 9,000.00' }, + }, + { + ...ROWS[1], + submitted_at: '2026-08-20T02:00:00.000Z', + payload: { service_fee: 5000 }, + payload_display: { service_fee: 'USD 5,000.00' }, + }, + { + ...ROWS[2], + submitted_at: '2026-08-20T03:00:00.000Z', + payload: { total_amount: 900 }, + payload_display: { total_amount: 'USD 900.00' }, + }, + ]; + + beforeEach(() => { + approvalsApiStub.listRequests.mockResolvedValue({ data: SORT_ROWS, total: SORT_ROWS.length }); + }); + + it('does not order the queue on a figure it declines to render', async () => { + declareTheReportedHiddenFields(); + await loadedQueue(); + await sortByAmount(); + + // INV-8801's 5,000 is `service_fee`, which its object hides: it has no + // renderable amount, so it sinks with the amount-less rows instead of + // sitting second and telling the viewer 900 < it < 9,000. + expect(desktopRowTitles()).toEqual(['PO-4417', 'INV-8802', 'INV-8801']); + expect(amountNodes('USD 5,000.00')).toHaveLength(0); + expectQueueIntact(); + }); + + it('COUNTER-PROBE: the same fixture, undeclared, orders by that same figure', async () => { + declareNothingHidden(); + await loadedQueue(); + await sortByAmount(); + + // Proof the assertion above is the trim and not a broken comparator: here + // the very same row sorts into the middle, on the very same value. + expect(desktopRowTitles()).toEqual(['PO-4417', 'INV-8801', 'INV-8802']); + expect(amountNodes('USD 5,000.00')).toHaveLength(2); + }); +}); diff --git a/apps/console/src/pages/system/ApprovalsInboxPage.tsx b/apps/console/src/pages/system/ApprovalsInboxPage.tsx index f3fb6d4418..a892b5d131 100644 --- a/apps/console/src/pages/system/ApprovalsInboxPage.tsx +++ b/apps/console/src/pages/system/ApprovalsInboxPage.tsx @@ -105,7 +105,7 @@ import { type ApprovalActionAttachment, } from '../../services/approvalsApi'; import { useRecordReadability } from './recordReadability'; -import { useHiddenFields } from './hiddenFields'; +import { useHiddenFieldsByObject } from './hiddenFields'; import { holdsStudioAccess } from '../../components/studioEntry'; type TabKey = 'pending' | 'submitted' | 'all'; @@ -367,16 +367,29 @@ const AMOUNT_KEY_RE = /(amount|total|price|value|cost|sum|budget|salary|fee|reve * 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. + * + * `hiddenKeys` is the fields THIS request's object declares `hidden: true`, and + * it is **required** on purpose (objectui#6020). It was optional when + * objectui#5565 added it, and four of the five call sites simply did not pass + * it — a filter that lives in the function body but is unpassed at the call + * site reads exactly like a fixed defect. Required, the compiler asks the one + * question that matters at every present and future call site: *whose* hidden + * keys? Pass `NO_HIDDEN_FIELDS` only where the answer is genuinely "nothing + * known", never to silence the parameter. + * + * An empty set means "nothing known to be hidden" — including the case where + * the metadata read has not answered — and yields today's amount. See + * `hiddenFields.ts` on why this presentation filter fails open. */ function decisionAmountEntry( r: ApprovalRequestRow, - hiddenKeys?: ReadonlySet, + hiddenKeys: ReadonlySet, ): { key: string; 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 (hiddenKeys?.has(k)) continue; // author declared `hidden: true` (#5565) + if (hiddenKeys.has(k)) continue; // author declared `hidden: true` (#5565) if (!AMOUNT_KEY_RE.test(k)) continue; const num = typeof v === 'number' ? v @@ -437,11 +450,20 @@ function RequestCell({ r, tr }: { r: ApprovalRequestRow; tr: Translate }) { * `href` is `null` for a record this viewer cannot open (objectui#5211) — the * readable/unreadable decision and the URL are ONE prop so the two cannot be * handed in disagreeing with each other. + * + * `hiddenKeys` belongs to THIS row's object (objectui#6020). The queue spans + * many objects, so it is a per-row prop and not something this cell could + * resolve for itself — see the page body, which drives it off one lookup. */ -function RecordCell({ r, href }: { r: ApprovalRequestRow; href: string | null }) { +function RecordCell({ r, href, hiddenKeys }: { + r: ApprovalRequestRow; + href: string | null; + hiddenKeys: ReadonlySet; +}) { // Surface the decision-relevant amount inline so a reviewer can triage the - // queue without opening each request (#2762 P1-3). - const amount = decisionAmountEntry(r); + // queue without opening each request (#2762 P1-3) — minus anything the + // author declared `hidden: true` (objectui#6020). + const amount = decisionAmountEntry(r, hiddenKeys); // objectui#5211: no link into a record this viewer cannot open. The title // still shows — it comes from the request's own payload snapshot, which the // approver was already given — it just stops being an anchor. @@ -708,17 +730,30 @@ export function ApprovalsInboxPage() { const readability = useRecordReadability(readabilityTargets); /** - * objectui#5565 — the fields the open request's object declares - * `hidden: true`. The drawer's business summary card is default UI, and - * `hidden` is a UI contract (objectstack#10749: "`hidden: true` stays - * UI-only; `internal: true` is the serialization primitive"), so the card - * must honour it. Scoped to the OPEN request's object: the queue rows are a - * different surface with a different cost model, and are not trimmed here. + * objectui#5565 + objectui#6020 — the fields each object on screen declares + * `hidden: true`. `hidden` is a UI contract (objectstack#10749: "`hidden: + * true` stays UI-only; `internal: true` is the serialization primitive"), and + * every surface below is default UI, so all of them must honour it: the + * drawer's summary card and lead amount (#5565), and — this is #6020 — the + * queue rows, the mobile cards, and the amount sort. + * + * ⛔ It is keyed BY OBJECT and not a single set. The queue is N rows spanning + * K objects; threading the open request's set into the rows would apply one + * object's declarations to every row — hiding fields on rows whose object + * never declared them, missing fields on rows whose object did, and looking + * fixed while doing it. Every consumer asks with its OWN `object_name`. * - * One cached metadata read per object per mount, empty until it answers — - * see `hiddenFields.ts` for the cost model and why this fails open. + * Same targets as the readability probe, so the deep-linked drawer's object + * is covered even when its request is not in the current row set. One cached + * metadata read per distinct object per mount, empty until it answers — see + * `hiddenFields.ts` for the cost model and why this fails open. */ - const hiddenPayloadKeys = useHiddenFields(selected?.object_name); + const hiddenFieldObjects = useMemo( + () => readabilityTargets.map((t) => t.object_name), + [readabilityTargets], + ); + const hiddenFields = useHiddenFieldsByObject(hiddenFieldObjects); + const hiddenPayloadKeys = hiddenFields.forObject(selected?.object_name); // Approve/reject/reassign/send-back/… are server-declared actions rendered by // DeclaredActionsBar (objectui#2697 + framework#3300); their param dialog // collects the comment and — since the shared upload-widget renderer (#2700/ @@ -1057,9 +1092,16 @@ export function ApprovalsInboxPage() { if (sortKey === 'amount') { // Highest amount first; rows without a detectable amount sink to the // bottom (keeping their relative newest-first order). + // + // objectui#6020: a row whose amount field its object declares + // `hidden: true` has no amount HERE either, so it sinks with them. An + // ordering IS a disclosure — it leaks the relative magnitude of a hidden + // figure to a viewer who never sees the figure — so the queue must not + // order on a value it declines to render. Each row is asked about its + // own object; a page spanning several objects gets several answers. sorted.sort((a, b) => { - const av = decisionAmountEntry(a)?.value; - const bv = decisionAmountEntry(b)?.value; + const av = decisionAmountEntry(a, hiddenFields.forObject(a.object_name))?.value; + const bv = decisionAmountEntry(b, hiddenFields.forObject(b.object_name))?.value; if (av == null && bv == null) return 0; if (av == null) return 1; if (bv == null) return -1; @@ -1070,7 +1112,7 @@ export function ApprovalsInboxPage() { sorted.sort((a, b) => (submittedAt(a) || '').localeCompare(submittedAt(b) || '')); } return sorted; - }, [rows, query, processFilter, objectFilter, statusFilter, tab, sortKey]); + }, [rows, query, processFilter, objectFilter, statusFilter, tab, sortKey, hiddenFields]); /** Position of the open request within the visible list (drawer prev/next). */ const drawerIndex = useMemo( () => (selectedId ? filteredRows.findIndex(r => r.id === selectedId) : -1), @@ -1580,7 +1622,13 @@ export function ApprovalsInboxPage() { )} - + + + {isSystemSubmitter(r) ? ( // Flow-/system-initiated: name the origin instead of a @@ -1646,7 +1694,8 @@ export function ApprovalsInboxPage() { {r.record_title || formatIdentity(r.record_id)} {objectDisplay(r)} {(() => { - const amount = decisionAmountEntry(r); + // objectui#6020: this row's OWN object decides. + const amount = decisionAmountEntry(r, hiddenFields.forObject(r.object_name)); return amount ? ( · {amount.display} ) : null; diff --git a/apps/console/src/pages/system/hiddenFields.test.ts b/apps/console/src/pages/system/hiddenFields.test.ts index 1cf9d40965..fb00cc6118 100644 --- a/apps/console/src/pages/system/hiddenFields.test.ts +++ b/apps/console/src/pages/system/hiddenFields.test.ts @@ -14,7 +14,7 @@ */ import { describe, it, expect, vi } from 'vitest'; -import { hiddenFieldNames, readHiddenFields } from './hiddenFields'; +import { hiddenFieldNames, readHiddenFields, planHiddenFieldReads } from './hiddenFields'; describe('hiddenFieldNames — both served `fields` shapes', () => { it('reads the record shape (`{ name: def }`)', () => { @@ -77,3 +77,25 @@ describe('readHiddenFields — every not-an-answer is the empty set', () => { expect([...(await readHiddenFields({ getObjectSchema }, 'o'))]).toEqual(['x']); }); }); + +describe('planHiddenFieldReads — the cost model for a queue of N rows', () => { + it('is one read per distinct object, not one per row', () => { + // A page of six rows spanning two objects costs two metadata reads + // (objectui#6020). The returned length IS the call count, so the cost is + // asserted rather than described. + const rows = ['showcase_purchase', 'showcase_invoice', 'showcase_purchase', + 'showcase_invoice', 'showcase_purchase', 'showcase_invoice']; + expect(planHiddenFieldReads(rows)).toEqual(['showcase_purchase', 'showcase_invoice']); + }); + + it('keeps first-seen order — stable output for stable input', () => { + expect(planHiddenFieldReads(['b', 'a', 'b', 'c'])).toEqual(['b', 'a', 'c']); + }); + + it('drops what is not an object name, rather than reading it', () => { + // A row mid-load, or one the server sent without an object, is not an + // object to ask about — and `''` would be a request for `/meta/object/`. + expect(planHiddenFieldReads([null, undefined, '', 'showcase_purchase'])) + .toEqual(['showcase_purchase']); + }); +}); diff --git a/apps/console/src/pages/system/hiddenFields.ts b/apps/console/src/pages/system/hiddenFields.ts index ee37ac70d3..c4b7786d87 100644 --- a/apps/console/src/pages/system/hiddenFields.ts +++ b/apps/console/src/pages/system/hiddenFields.ts @@ -16,8 +16,9 @@ * `hidden` is neither of those. The producer is **correct** to ship a `hidden` * field in an approval request's `payload_json` snapshot, so the UI is not * compensating for a bad producer when it declines to render it — the UI is - * the *only* place that contract lives, and the Approvals drawer's business - * summary card is default UI. That is why the trim is here and not in + * the *only* place that contract lives, and every Approvals Inbox surface — + * the drawer's business summary card, the queue rows, and the amount sort + * (objectui#6020) — is default UI. That is why the trim is here and not in * `@objectstack/plugin-approvals`. * * ⛔ Do not extend this to `internal`. They are distinct primitives with @@ -38,7 +39,10 @@ * * ## Cost * - * One `getObjectSchema(objectName)` per distinct object, per mount. The read is + * One `getObjectSchema(objectName)` per distinct object, per mount — so a page + * of N queue rows spanning K objects costs `K` reads, not N, and paging in + * more rows costs only the objects not seen yet. See `planHiddenFieldReads`, + * which IS the cost model and is pinned by `hiddenFields.test.ts`. The read is * `GET /api/v1/meta/object/:name` — the same read the record form and detail * view already perform for any business user — and it lands on the adapter's * own `MetadataCache` (LRU, 5-minute TTL, in-flight de-duplication), so @@ -130,35 +134,96 @@ export async function readHiddenFields( } /** - * The hidden-field set for `objectName`, resolved once per mount. + * The distinct objects one render needs answers for — **the cost model**. * - * Returns the empty set until the read answers, so the first paint of a drawer - * renders exactly what it renders today and the trim applies as soon as the - * declaration is known. Callers must treat the empty set as "nothing known to - * be hidden" — see the header on failing open. + * The returned length is exactly the number of metadata reads a render adds, + * so a test can assert the cost directly instead of describing it. Names are + * deduped and kept in first-seen order (stable output for stable input); a + * missing or empty name is not an object and is dropped. */ -export function useHiddenFields(objectName: string | null | undefined): ReadonlySet { +export function planHiddenFieldReads( + objectNames: readonly (string | null | undefined)[], +): string[] { + const out: string[] = []; + const seen = new Set(); + for (const name of objectNames) { + if (typeof name !== 'string' || name === '' || seen.has(name)) continue; + seen.add(name); + out.push(name); + } + return out; +} + +/** What a row asks the lookup at render time. */ +export interface HiddenFieldLookup { + /** + * The fields `objectName` declares `hidden: true`. + * + * The empty set is "nothing known to be hidden" — it covers *nothing + * declared*, *this object was never asked about*, *the read has not answered + * yet*, and *the read failed*, which are one answer on purpose. See the + * header on failing open: callers render the field for all four. + */ + forObject(objectName: string | null | undefined): ReadonlySet; +} + +/** + * Hidden-field sets for every object on screen, each resolved once per mount. + * + * A queue is N rows spanning K objects, so the answer cannot be one set — one + * object's declarations applied to every row would hide fields on rows whose + * object never declared them and miss fields on rows whose object did + * (objectui#6020). The lookup is therefore keyed by object name, and a row + * always asks with its OWN `object_name`. + * + * Every object is read at most once per mount: newly loaded rows cost only the + * objects not seen yet, and re-renders (search typing, the minute clock, a + * drawer opening) cost nothing. Until a read answers, `forObject` returns the + * empty set, so the first paint renders exactly what it renders today and the + * trim applies as soon as the declaration is known — see the header on why + * this presentation filter fails open, and never the other way. + */ +export function useHiddenFieldsByObject( + objectNames: readonly (string | null | undefined)[], +): HiddenFieldLookup { const adapter = useAdapter() as HiddenFieldsSource | null | undefined; const [known, setKnown] = useState>>(() => new Map()); /** Object names already handed to a read — the "read once" ledger. */ const attempted = useRef>(new Set()); + /** Latest names without making them an effect dependency (see `signature`). */ + const latest = useRef(objectNames); + latest.current = objectNames; - const name = typeof objectName === 'string' && objectName !== '' ? objectName : null; + // `objectNames` is a fresh array every render, so the effect keys off the SET + // of objects rather than the array's identity. + const signature = useMemo(() => planHiddenFieldReads(objectNames).join('|'), [objectNames]); useEffect(() => { - if (!adapter || !name || attempted.current.has(name)) return; - attempted.current.add(name); + if (!adapter) return; + const fresh = planHiddenFieldReads(latest.current).filter((n) => !attempted.current.has(n)); + if (fresh.length === 0) return; + for (const n of fresh) attempted.current.add(n); let cancelled = false; - void readHiddenFields(adapter, name).then((hidden) => { + // `readHiddenFields` never rejects, so one object that cannot be described + // resolves to the empty set without dropping the others' answers. + void Promise.all( + fresh.map(async (n) => [n, await readHiddenFields(adapter, n)] as const), + ).then((entries) => { if (cancelled) return; setKnown((prev) => { const next = new Map(prev); - next.set(name, hidden); + for (const [n, hidden] of entries) next.set(n, hidden); return next; }); }); return () => { cancelled = true; }; - }, [adapter, name]); + }, [adapter, signature]); - return useMemo(() => (name ? known.get(name) ?? NONE : NONE), [known, name]); + return useMemo(() => ({ + forObject: (objectName) => ( + typeof objectName === 'string' && objectName !== '' + ? known.get(objectName) ?? NONE + : NONE + ), + }), [known]); }