From 6226e589d48634469dfb3f6bbfe30954984ed7f1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 14:54:56 +0000 Subject: [PATCH] fix(app-shell): a refused package publish names each refused item with the server's reason The package sheet's publishDrafts and usePublishAllDrafts (Home's Publish all, the draft-preview bar) read the batch's own success === false before failed[]. A refusal carries both, so the author read "Action failed" or "publish-drafts did not publish this package". Both now read failed[] first and name the refused items (the causal item on a rollback, every item on a pre-flight refusal) through one module-private helper, preview/publishRefusal.ts. Claude-Session: https://claude.ai/code/session_01MgfduSkFrfM3eorB3UGfAU Co-authored-by: Claude --- .changeset/11922-publish-refusal-reason.md | 12 ++ ...usePublishAllDrafts.refusal-11922.test.tsx | 180 +++++++++++++++++ .../app-shell/src/preview/publishRefusal.ts | 45 +++++ .../src/preview/usePublishAllDrafts.ts | 8 +- ...PackagesPage.publishRefusal-11922.test.tsx | 183 ++++++++++++++++++ .../src/views/metadata-admin/PackagesPage.tsx | 33 ++-- 6 files changed, 447 insertions(+), 14 deletions(-) create mode 100644 .changeset/11922-publish-refusal-reason.md create mode 100644 packages/app-shell/src/preview/__tests__/usePublishAllDrafts.refusal-11922.test.tsx create mode 100644 packages/app-shell/src/preview/publishRefusal.ts create mode 100644 packages/app-shell/src/views/metadata-admin/PackagesPage.publishRefusal-11922.test.tsx diff --git a/.changeset/11922-publish-refusal-reason.md b/.changeset/11922-publish-refusal-reason.md new file mode 100644 index 0000000000..ba76eb6e94 --- /dev/null +++ b/.changeset/11922-publish-refusal-reason.md @@ -0,0 +1,12 @@ +--- +'@object-ui/app-shell': patch +--- + +A refused package publish names each refused item with the server's reason, on the package sheet and on Home's *Publish all* and the draft-preview bar (objectui#11922). + +`POST /packages/:id/publish-drafts` answers a refusal (`outcome: 'refused'`) on a 200, with the batch's own `success: false` and every item that did not publish, with its reason, in `failed[]`. Both doors read `success` first and stopped there: the package sheet showed "Action failed", and Home's *Publish all* and the draft-preview bar toasted "Publish failed: publish-drafts did not publish this package". Both now read `failed[]` first, as Studio's publish does (objectui#11807). + +- **The package sheet.** A rolled-back batch (ADR-0067 D2) keeps its "Nothing was published — the batch rolled back" row and names the causal item with its reason. The drafts rolled back with it are not listed as causes. A refusal with no rolled-back item (the runtime's pre-flight checks) keeps its "Published N; M failed" row and now names every refused item with its reason after it. +- **Home's *Publish all* and the draft-preview bar.** The "Publish failed" toast names the refused items with their reasons, leaving out the rolled-back ones, in place of "publish-drafts did not publish this package". + +A published batch, and a batch that carries an envelope `error` with an empty `failed[]`, read as before. Nothing is added to the package entry: no export, prop, type member or language-pack key. diff --git a/packages/app-shell/src/preview/__tests__/usePublishAllDrafts.refusal-11922.test.tsx b/packages/app-shell/src/preview/__tests__/usePublishAllDrafts.refusal-11922.test.tsx new file mode 100644 index 0000000000..e4cec4b655 --- /dev/null +++ b/packages/app-shell/src/preview/__tests__/usePublishAllDrafts.refusal-11922.test.tsx @@ -0,0 +1,180 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#11922 — Home's *Publish all* and the draft-preview bar name each + * refused item with the server's reason. + * + * ## The defect + * + * Both surfaces publish through `usePublishAllDrafts`, which sends each + * package's drafts to `POST /packages/:id/publish-drafts`. A refusal answers + * on a 200 with the batch's own `success: false` AND `failed[]`, where every + * item that did not publish carries its `error` + * (`PublishPackageDraftsResponseSchema`). The hook asked `success` first and + * threw "publish-drafts did not publish this package" — the batch carries no + * `error` key — so the toast named no item and no reason. + * + * ## Real vs stubbed + * + * Real: the hook, `useMetadataClient` and the client it builds (which unwraps + * the batch route's `{ success, data }` envelope), the publish-health reader. + * Stubbed: `sonner` (the terminal sink the hook imports directly) and + * `globalThis.fetch` (the server). The drafts are a `view` and a `flow`, types + * the capability lint does not read, so no lint request is in the way. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { act, cleanup, renderHook } from '@testing-library/react'; +import type { TranslateFn } from '@object-ui/i18n'; + +vi.mock('sonner', () => ({ + toast: { + warning: vi.fn(), + error: vi.fn(), + success: vi.fn(), + info: vi.fn(), + message: vi.fn(), + dismiss: vi.fn(), + }, +})); + +import { toast } from 'sonner'; +import { usePublishAllDrafts } from '../usePublishAllDrafts'; + +const PKG_ID = 'com.acme.crm'; + +/** Answers the KEY (and its count), so a toast is told by its row, never by prose. */ +const t: TranslateFn = (key, options) => + options?.count === undefined ? key : `${key}:${String(options.count)}`; + +/** A batch answer as the producer sends it, inside the dispatcher's envelope. */ +function batch(data: Record) { + return { success: true, data: { success: data.outcome === 'published', ...data } }; +} + +let batchBody: unknown; +/** Every publish request the hook sent: the batch door and the by-reference one. */ +const posts: string[] = []; + +function json(body: unknown) { + return new Response(JSON.stringify(body), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); +} + +beforeEach(() => { + vi.clearAllMocks(); + posts.length = 0; + vi.stubGlobal( + 'fetch', + vi.fn(async (input: unknown) => { + const url = String(input); + if (url.endsWith('/publish-drafts')) { + posts.push(url); + return json(batchBody); + } + if (/\/meta\/flow\/[^/]+\/publish$/.test(url)) { + posts.push(url); + return json({ success: true }); + } + if (url.includes('/meta/_drafts')) { + return json({ + drafts: [ + { type: 'view', name: 'lead_list', packageId: PKG_ID }, + { type: 'flow', name: 'nightly_digest', packageId: null }, + ], + }); + } + return json({ success: true, data: {} }); + }) as unknown as typeof fetch, + ); +}); + +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); +}); + +async function publishAll() { + const { result } = renderHook(() => usePublishAllDrafts(t)); + let outcome: Awaited> | undefined; + await act(async () => { + outcome = await result.current.publishAll(); + }); + return outcome; +} + +const calls = (fn: unknown) => vi.mocked(fn as (...a: unknown[]) => unknown).mock.calls.map((c) => c[0]); + +describe('Publish all names what a refused package publish refused (objectui#11922)', () => { + it('a refused batch toasts each refused item with the server’s reason', async () => { + const reason = 'sharingModel is not set'; + const stored = "Draft 'views/lead_list' is stored under the non-canonical metadata type 'views'"; + batchBody = batch({ + outcome: 'refused', + publishedCount: 0, + failedCount: 2, + published: [], + failed: [ + { type: 'object', name: 'lead', error: reason, code: 'SECURITY_OWD_UNSET' }, + { type: 'views', name: 'lead_list', error: stored, code: 'STORED_TYPE_NOT_CANONICAL' }, + ], + }); + const outcome = await publishAll(); + expect(outcome?.ok).toBe(false); + const errors = calls(toast.error); + expect(errors).toHaveLength(1); + expect(errors[0]).toMatch(/^home\.pendingDrafts\.publishFailed: /); + expect(errors[0]).toContain(`object/lead: ${reason}`); + expect(errors[0]).toContain(`views/lead_list: ${stored}`); + expect(errors[0]).not.toContain('publish-drafts did not publish this package'); + expect(calls(toast.success)).toEqual([]); + }); + + it('a rolled-back batch names its causal item, and not the siblings it took down', async () => { + const reason = 'sharingModel is not set'; + batchBody = batch({ + outcome: 'refused', + publishedCount: 0, + failedCount: 2, + published: [], + failed: [ + { + type: 'view', + name: 'lead_list', + error: 'not published — the batch is all-or-nothing (ADR-0067 D2) and object/lead failed; the transaction rolled back', + code: 'BATCH_ABORTED', + }, + { type: 'object', name: 'lead', error: reason, code: 'SECURITY_OWD_UNSET' }, + ], + }); + await publishAll(); + const errors = calls(toast.error); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain(`object/lead: ${reason}`); + expect(errors[0]).not.toContain('view/lead_list'); + }); +}); + +describe('control: a published batch reads as before (objectui#11922)', () => { + it('records the batch’s health and goes on to the package-less drafts', async () => { + batchBody = batch({ + outcome: 'published', + publishedCount: 1, + failedCount: 0, + published: [{ type: 'view', name: 'lead_list', version: 'sha256:0' }], + failed: [], + seedApplied: { success: true, inserted: 3, updated: 0 }, + }); + const outcome = await publishAll(); + expect(outcome).toEqual({ ok: true, attempted: 2 }); + // The orphan flow went out by reference after the batch. + expect(posts).toHaveLength(2); + expect(posts[0]).toMatch(/\/publish-drafts$/); + expect(posts[1]).toMatch(/\/meta\/flow\/nightly_digest\/publish$/); + // The health was recorded: the seeded-rows row, with the batch's 3. + expect(calls(toast.success)).toEqual(['home.pendingDrafts.publishedVerified:3']); + expect(calls(toast.error)).toEqual([]); + }); +}); diff --git a/packages/app-shell/src/preview/publishRefusal.ts b/packages/app-shell/src/preview/publishRefusal.ts new file mode 100644 index 0000000000..9822f5ba2e --- /dev/null +++ b/packages/app-shell/src/preview/publishRefusal.ts @@ -0,0 +1,45 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * What a refused `POST /packages/:id/publish-drafts` batch names to the author + * (objectui#11922). Shared by the two doors that publish a package's drafts + * outside Studio: the package sheet (`PackagesPage`'s `publishDrafts`) and + * Home's *Publish all* with the draft-preview bar (`usePublishAllDrafts`). + * + * A refusal answers 200 with the batch's own `success: false` AND its per-item + * story in `failed[]` (`PublishPackageDraftsResponseSchema`: `outcome` is + * `refused` if and only if `failed` is non-empty). Both doors asked `success` + * first and threw before they reached `failed[]`, so the author read "Action + * failed" or "publish-drafts did not publish this package": no item, no reason. + * Each door now reads `failed[]` first and names the items through this. + * + * Which items: every element that is not a `BATCH_ABORTED` sibling. On the + * ADR-0067 D2 rollback that is the causal item, the one carrying the real + * error; its siblings only say the batch rolled back because of it. On a + * pre-flight refusal no element is aborted, so every refused item is named. + * When every element is aborted (the producer could not attribute the + * failure), the first one's sentence is still the server's own. + * + * Not `formatPublishFailures` (`studio-design/metadataError.ts`), and measured + * rather than assumed: it tells a rolled-back sibling by the pre-ADR-0112 + * spelling `batch_aborted` compared with `===`, so on the producer's + * `BATCH_ABORTED` it lists every sibling as a parallel error and never reaches + * its own rolled-back banner; and its banner is English, where the package + * sheet answers in its own localized rows. `errorCodeIs` matches both spellings. + */ + +import { errorCodeIs } from '@object-ui/types'; +import type { MetadataClient } from '@object-ui/data-objectstack'; + +/** One `failed[]` element of the batch, as the client hands it back (the spec's shape). */ +type PublishDraftsFailure = NonNullable< + Awaited>['failed'] +>[number]; + +/** `type/name: reason` for each refused item, in the server's own words. */ +export function refusedItemsText(failed: readonly PublishDraftsFailure[]): string { + const refused = failed.filter((f) => !errorCodeIs(f, 'BATCH_ABORTED')); + return (refused.length > 0 ? refused : failed.slice(0, 1)) + .map((f) => `${f.type}/${f.name}: ${f.error}`) + .join('; '); +} diff --git a/packages/app-shell/src/preview/usePublishAllDrafts.ts b/packages/app-shell/src/preview/usePublishAllDrafts.ts index ee53bcdc7c..b847395169 100644 --- a/packages/app-shell/src/preview/usePublishAllDrafts.ts +++ b/packages/app-shell/src/preview/usePublishAllDrafts.ts @@ -31,6 +31,7 @@ import { publishHealthFromResponse, type PublishHealth } from '@object-ui/plugin import { useMetadataClient } from '../views/metadata-admin/useMetadata.js'; import { emitMetadataRefresh } from '../assistant/assistantBus.js'; import { lintDraftCapabilityReferences } from './capabilityLint.js'; +import { refusedItemsText } from './publishRefusal.js'; // The one authority for the narrowed `t` (objectui#8261) — imported, not // re-declared. The local copy this replaces differed only in its parameter's // NAME (`opts`), which TypeScript does not compare, so no caller's contract moves. @@ -93,7 +94,12 @@ export function usePublishAllDrafts(t: TranslateFn) { // findings a few lines below were already shouting. const payload = await client.publishPackageDrafts(packageId); // A non-2xx now throws inside the client, with the server's own - // message. What is left to check here is the batch verdict, unchanged. + // message. What is left to check here is the batch verdict. + // + // objectui#11922 — `failed[]` BEFORE the batch's own `success`: a + // refusal answers both, and only `failed[]` says which item and why. + const failed = Array.isArray(payload.failed) ? payload.failed : []; + if (failed.length > 0) throw new Error(refusedItemsText(failed)); if ((payload as { success?: boolean }).success === false) { const error = (payload as { error?: { message?: string } }).error; throw new Error(error?.message || 'publish-drafts did not publish this package'); diff --git a/packages/app-shell/src/views/metadata-admin/PackagesPage.publishRefusal-11922.test.tsx b/packages/app-shell/src/views/metadata-admin/PackagesPage.publishRefusal-11922.test.tsx new file mode 100644 index 0000000000..920bfaff55 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/PackagesPage.publishRefusal-11922.test.tsx @@ -0,0 +1,183 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#11922 — a refused publish on the package sheet names each refused + * item with the server's reason. + * + * ## The defect + * + * `POST /packages/:id/publish-drafts` answers a refusal on a 200: the batch's + * own `success` is false AND `failed[]` carries every item that did not publish + * with its `error` (`PublishPackageDraftsResponseSchema`: `outcome` is + * `refused` if and only if `failed` is non-empty). `publishDrafts` asked + * `success === false` first and threw through the envelope ladder, whose last + * rung is "Action failed" — the batch carries no `error` key — so the author + * never read which item was refused, or why. The `failed[]` reading below it + * was unreachable on every refusal the producer sends. + * + * ## Real vs stubbed + * + * Real: the sheet, `useMetadataClient` and the client it builds, which unwraps + * the route's `{ success, data }` envelope, so the batch is read through the + * composition the console reads it through. Stubbed: `globalThis.fetch` (the + * server). + * + * The rows are read from the string table through the same `tFormat` the sheet + * calls, so what is pinned is which row, which item and which reason, never the + * prose around them. + */ + +import * as React from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +import { PackageDetailSheet, type InstalledPackageRow } from './PackagesPage'; +import { t, tFormat } from './i18n'; + +const PKG_ID = 'com.acme.crm'; +const PKG: InstalledPackageRow = { + manifest: { id: PKG_ID, name: 'Acme CRM', version: '1.0.0', type: 'app' }, + enabled: true, + status: 'installed', +}; +const DRAFTS = [ + { type: 'object', name: 'lead' }, + { type: 'view', name: 'lead_list' }, +]; + +/** + * A batch answer as the producer sends it: the protocol's result, carrying its + * own `success` (`=== (outcome === 'published')`, pinned upstream), inside the + * dispatcher's `{ success: true, data }` envelope — `true` on every 200, the + * refusals included. + */ +function batch(data: Record) { + return { success: true, data: { success: data.outcome === 'published', ...data } }; +} + +let batchBody: unknown; +const posts: string[] = []; + +function json(body: unknown) { + return new Response(JSON.stringify(body), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); +} + +beforeEach(() => { + posts.length = 0; + vi.stubGlobal( + 'fetch', + vi.fn(async (input: unknown) => { + const url = String(input); + if (url.endsWith('/publish-drafts')) { + posts.push(url); + return json(batchBody); + } + if (url.startsWith('/api/v1/meta/_drafts')) return json({ success: true, data: { drafts: DRAFTS } }); + return json({ success: true, data: {} }); + }) as unknown as typeof fetch, + ); +}); + +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); +}); + +/** Press *Publish app* and return the line the sheet settles on. */ +async function publishAndRead(matches: (text: string) => boolean): Promise { + render( + + + , + ); + fireEvent.click( + await screen.findByRole('button', { + name: tFormat('engine.packages.detail.publishApp', 'en', { count: DRAFTS.length }), + }), + ); + await waitFor(() => expect(posts).toHaveLength(1)); + // The message line is a leaf: its whole text is the sheet's `msg.text`. + const line = await screen.findByText( + (_, node) => !!node && node.childElementCount === 0 && matches(node.textContent ?? ''), + ); + return line.textContent ?? ''; +} + +describe('the package sheet names what a refused publish refused (objectui#11922)', () => { + it('a pre-flight refusal names every refused item with the server’s reason', async () => { + const prefix = "Object name 'lead' must start with the package namespace 'acme_'"; + const stored = "Draft 'views/lead_list' is stored under the non-canonical metadata type 'views'"; + batchBody = batch({ + outcome: 'refused', + publishedCount: 0, + failedCount: 2, + published: [], + failed: [ + { type: 'object', name: 'lead', error: prefix, code: 'NAMESPACE_PREFIX' }, + { type: 'views', name: 'lead_list', error: stored, code: 'STORED_TYPE_NOT_CANONICAL' }, + ], + }); + const row = tFormat('engine.packages.detail.publishDraftsPartial', 'en', { published: 0, failed: 2 }); + const shown = await publishAndRead((text) => text.startsWith(row)); + expect(shown).toContain(`object/lead: ${prefix}`); + expect(shown).toContain(`views/lead_list: ${stored}`); + expect(shown).not.toContain(t('engine.packages.detail.actionFailed', 'en')); + }); + + it('a rolled-back batch names its causal item with its reason, and not the siblings it took down', async () => { + const reason = 'sharingModel is not set'; + batchBody = batch({ + outcome: 'refused', + publishedCount: 0, + failedCount: 2, + published: [], + failed: [ + { type: 'object', name: 'lead', error: reason, code: 'SECURITY_OWD_UNSET' }, + { + type: 'view', + name: 'lead_list', + error: 'not published — the batch is all-or-nothing (ADR-0067 D2) and object/lead failed; the transaction rolled back', + code: 'BATCH_ABORTED', + }, + ], + }); + const shown = await publishAndRead((text) => + text.startsWith(tFormat('engine.packages.detail.publishDraftsRolledBack', 'en', { cause: '' })), + ); + expect(shown).toBe( + tFormat('engine.packages.detail.publishDraftsRolledBack', 'en', { cause: `object/lead: ${reason}` }), + ); + expect(shown).not.toContain('view/lead_list'); + }); +}); + +describe('controls: the answers that are not a refusal read as before (objectui#11922)', () => { + it('a published batch keeps the success row', async () => { + batchBody = batch({ + outcome: 'published', + publishedCount: 2, + failedCount: 0, + published: DRAFTS.map((d, i) => ({ ...d, version: `sha256:${i}` })), + failed: [], + }); + const ok = t('engine.packages.detail.publishDraftsOk', 'en'); + expect(await publishAndRead((text) => text === ok)).toBe(ok); + }); + + it('a batch whose envelope carries an error and an empty failed[] still shows the envelope’s text', async () => { + const message = 'Publishing is temporarily unavailable. Nothing was changed.'; + batchBody = batch({ + outcome: 'nothing_to_publish', + publishedCount: 0, + failedCount: 0, + published: [], + failed: [], + error: { code: 'SERVICE_UNAVAILABLE', message }, + }); + expect(await publishAndRead((text) => text.startsWith(message))).toBe(`${message} (SERVICE_UNAVAILABLE)`); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/PackagesPage.tsx b/packages/app-shell/src/views/metadata-admin/PackagesPage.tsx index 0e215f34ec..ade4d28740 100644 --- a/packages/app-shell/src/views/metadata-admin/PackagesPage.tsx +++ b/packages/app-shell/src/views/metadata-admin/PackagesPage.tsx @@ -75,6 +75,7 @@ import { useMetadataClient } from './useMetadata.js'; import { PackageFormDialog } from './PackageFormDialog.js'; import { errorCodeIs } from '@object-ui/types'; import { readEnvelopeFailureText } from '../../utils/apiErrorEnvelope.js'; +import { refusedItemsText } from '../../preview/publishRefusal.js'; import { duplicatePackage, isSpecPackageId } from '../studio-design/packages-io.js'; import { PackageIdInput } from '../studio-design/PackageIdInput.js'; @@ -432,7 +433,12 @@ export function PackageDetailSheet({ const marked = readEnvelopeFailureText((e as { body?: unknown } | null)?.body); throw marked ? new Error(marked) : e; }); - if ((r as { success?: boolean }).success === false) { + // objectui#11922 — `failed[]` is read BEFORE the batch's own `success`. + // A refusal carries both: `success` is false on it, and `failed[]` is + // where its items and reasons are. Asked in the other order, every + // refusal reached the author as "Action failed". + const failedList = Array.isArray(r.failed) ? r.failed : []; + if (failedList.length === 0 && (r as { success?: boolean }).success === false) { // Preserves what `apiJson` did for this call: a batch that did not // publish is an error on this surface, read through the same // envelope ladder. The status is no longer in hand — a non-2xx @@ -452,24 +458,25 @@ export function PackageDetailSheet({ } catch { setDrafts([]); } - if (r?.failedCount) { + if (failedList.length > 0) { // framework 15.1+ (ADR-0067 D2): the batch is all-or-nothing — a // failure means NOTHING landed and `failed[]` marks the rolled-back - // drafts `batch_aborted`, with the causal item carrying the real + // drafts `BATCH_ABORTED`, with the causal item carrying the real // error. Say "rolled back because X", not "{n} failed" (which reads // as a partial publish that no longer exists). - const failedList = Array.isArray(r.failed) ? r.failed : []; - const causal = failedList.find((f) => !errorCodeIs(f, 'BATCH_ABORTED') && f?.error); + const cause = refusedItemsText(failedList); if (failedList.some((f) => errorCodeIs(f, 'BATCH_ABORTED'))) { - throw new Error(tFormat('engine.packages.detail.publishDraftsRolledBack', locale, { - cause: causal ? `${causal.type ?? '?'}/${causal.name ?? '?'}: ${causal.error}` : String(r.failedCount), - })); + throw new Error(tFormat('engine.packages.detail.publishDraftsRolledBack', locale, { cause })); } - // pre-15.1 server — genuine partial publish. - throw new Error(tFormat('engine.packages.detail.publishDraftsPartial', locale, { - published: r.publishedCount ?? 0, - failed: r.failedCount, - })); + // No aborted sibling: a pre-flight refusal, or a pre-15.1 server's + // genuine partial publish. Either way every element is an item that + // did not publish, so each is named with its reason. + throw new Error( + `${tFormat('engine.packages.detail.publishDraftsPartial', locale, { + published: r.publishedCount ?? 0, + failed: failedList.length, + })} ${cause}`, + ); } return r; },