From a95ee67a965bbe582389e79ce6c5286aa1f5a617 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 01:05:24 +0000 Subject: [PATCH] fix(app-shell): a failed package-list refresh is not a deletion (objectui#7821) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `onManageChanged` refreshed the package list into a local `list` initialised to `[]` and swallowed the rejection under a comment reading "keep the stale list". That is true of the `pkgs` state — which is simply not written — and false of the local, which stayed `[]`. So after a failed `GET /api/v1/packages` the `!list.some((p) => p.id === managedId)` check three lines down was unconditionally true, the code took the branch labelled `// Deleted`, and when the managed package was the one under the editor it navigated away with `list[0]` undefined — to `/home`. One transient 503 evicted the author from the Studio editor, silently, while the package was still there. The local now starts as `null` ("the refresh told us nothing"), and only a list that actually came back, without the managed package in it, is read as a deletion. A failure draws no inference: no navigation. It is reported instead through the posture this surface already carries — `formatMetadataError` on the shared `studio-package-list` sonner id, and recorded so the switcher reads `failed` rather than presenting a stale list as current. Still a `.catch` and still no retry: one 503 must not take the Studio down, and no retry policy has been ruled on. Behavioural pins in StudioDesignSurface.packageDeletionInference.test.tsx cover all three legs: no navigation on failure, the failure still reported, and a real deletion navigating exactly as before (to a surviving sibling, or `/home`). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3 --- ...-manage-refresh-failure-is-not-deletion.md | 29 ++ ...nSurface.packageDeletionInference.test.tsx | 300 ++++++++++++++++++ .../studio-design/StudioDesignSurface.tsx | 37 ++- 3 files changed, 362 insertions(+), 4 deletions(-) create mode 100644 .changeset/7821-manage-refresh-failure-is-not-deletion.md create mode 100644 packages/app-shell/src/views/studio-design/StudioDesignSurface.packageDeletionInference.test.tsx diff --git a/.changeset/7821-manage-refresh-failure-is-not-deletion.md b/.changeset/7821-manage-refresh-failure-is-not-deletion.md new file mode 100644 index 0000000000..e67ce38cba --- /dev/null +++ b/.changeset/7821-manage-refresh-failure-is-not-deletion.md @@ -0,0 +1,29 @@ +--- +'@object-ui/app-shell': patch +--- + +A failed package-list refresh no longer reads as "the package was deleted" and no +longer evicts the author from the Studio (objectui#7821). + +`onManageChanged` — the callback the Studio's `PackageDetailSheet` fires after every +package lifecycle action (disable / duplicate / delete / publish / manifest edit) — +refreshed the list into a local `list` initialised to `[]` and swallowed the rejection +under a comment reading "keep the stale list". That is true of the `pkgs` state, which +is simply not written, and false of the local, which stayed `[]`. So after a failed +`GET /api/v1/packages` the `!list.some(...)` check three lines down was +unconditionally true, the code took the branch labelled `// Deleted`, and — when the +managed package was the one under the editor — navigated away with `list[0]` +undefined, i.e. to `/home`. One transient 503, network blip or auth expiry threw the +author out of the editor with no toast and no confirmation, while the package was +still there. + +The local now starts as `null` — "the refresh told us nothing" — and only a list that +actually came back, without the managed package in it, is read as a deletion. A +failure draws no inference at all: no navigation. It is reported instead, through the +posture this surface already has (objectui#7368): `formatMetadataError` on the shared +`studio-package-list` sonner id, and recorded so the switcher reads `failed` rather +than presenting a now-stale list as current. A real deletion navigates exactly as +before — to the first surviving package, or `/home` when none is left. + +Still a `.catch` and still no retry: one 503 must not take the Studio down, and no +retry policy has been ruled on. diff --git a/packages/app-shell/src/views/studio-design/StudioDesignSurface.packageDeletionInference.test.tsx b/packages/app-shell/src/views/studio-design/StudioDesignSurface.packageDeletionInference.test.tsx new file mode 100644 index 0000000000..9671d48257 --- /dev/null +++ b/packages/app-shell/src/views/studio-design/StudioDesignSurface.packageDeletionInference.test.tsx @@ -0,0 +1,300 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#7821 — `onManageChanged` read a FAILED package-list refresh as + * "the package was deleted" and evicted the author out of the Studio. + * + * The callback runs after every package lifecycle action fired from the + * `PackageDetailSheet` (disable / duplicate / delete / publish / manifest + * edit). It refreshed the list into a LOCAL `list`, initialised to `[]`, and + * swallowed the rejection under a comment reading "keep the stale list" — + * true of the `pkgs` state, which is simply not written, and false of the + * local, which stayed `[]`. Three lines later `!list.some((p) => p.id === + * managedId)` was therefore UNCONDITIONALLY true after a failure, so the code + * took the branch labelled `// Deleted`; when the managed package was the one + * under the editor it navigated away, and `list[0]` being `undefined` made + * the destination `/home`. + * + * Net effect: one transient `GET /api/v1/packages` failure — a 503 from the + * durable half, a network blip, an auth expiry — threw the author out of the + * editor, with no toast and no confirmation, while the package was still + * there. That is the sibling defect of objectui#7368 escalated: not a + * swallowed failure, but a swallowed failure that then decides the OPPOSITE + * of the truth. + * + * These pins are behavioural, and there are three of them because the fix has + * two ways to be wrong: + * 1. a FAILED refresh causes NO navigation (the defect), and + * 2. it is still REPORTED, through this file's existing objectui#7368 + * posture (`formatMetadataError` on the shared sonner id, recorded so + * the trigger reads `failed`) — a fix must not buy pin 1 with silence; + * 3. a REAL deletion — the list comes back successfully WITHOUT the package + * — still navigates exactly as before. ⛔ Not optional: pinning only 1 + * and 2 lets the fix degrade into "never navigate", which strands the + * author on a package that no longer exists. + * + * ⛔ The `.catch` is deliberately still a `.catch` (one 503 must not take the + * Studio down — objectui#7368's ruling) and there is deliberately still no + * retry (count / backoff / what-after-giving-up are unruled policy). + */ + +import '@testing-library/jest-dom/vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { MemoryRouter, Route, Routes, useLocation } from 'react-router-dom'; + +const PACKAGE_ID = 'app.b2r4'; +const SIBLING_ID = 'app.other'; +const START_PATH = `/studio/${PACKAGE_ID}/interfaces`; + +const row = (id: string) => ({ id, name: id, writable: true, namespace: id.split('.')[1] }); + +// The package list refresh is the system under test — every case installs its +// own resolution/rejection for the refresh that `onManageChanged` performs. +const fetchPackagesMock = vi.fn(); +vi.mock('./packages-io', async (importOriginal) => { + const actual = await importOriginal>(); + return { ...actual, fetchPackages: (...args: unknown[]) => fetchPackagesMock(...args) }; +}); + +// The report channel (objectui#7368's posture). Captured rather than rendered +// so a pin can read both the message and the shared sonner id. +const toastError = vi.fn(); +vi.mock('sonner', () => ({ + toast: { + error: (...args: unknown[]) => toastError(...args), + success: vi.fn(), + info: vi.fn(), + dismiss: vi.fn(), + }, + Toaster: () => null, +})); + +/** + * The lifecycle sheet, stubbed down to the one thing this file is about: the + * `onChanged` callback it fires after a lifecycle action. Driving the real + * sheet's delete/disable/publish buttons would test THOSE, and the defect is + * in the surface's reaction, which every one of them reaches the same way. + */ +vi.mock('../metadata-admin/PackagesPage', async (importOriginal) => { + const actual = await importOriginal>(); + return { + ...actual, + PackageDetailSheet: ({ + pkg, + open, + onChanged, + }: { + pkg: { manifest?: { id?: string } } | null; + open: boolean; + onChanged: () => void | Promise; + }) => + open ? ( +
+ +
+ ) : null, + }; +}); + +const mockClient = { + list: vi.fn(async () => []), + listDrafts: vi.fn(async () => []), + layered: vi.fn(async (_t: string, name: string) => ({ effective: { name } })), + getDraft: vi.fn(async () => null), + get: vi.fn(async () => undefined), + save: vi.fn(async () => ({})), +}; + +vi.mock('../metadata-admin/useMetadata', async (importOriginal) => { + const actual = await importOriginal>(); + return { + ...actual, + useMetadataClient: () => mockClient, + useMetadataTypes: () => ({ loading: false, error: null, entries: [] }), + }; +}); + +vi.mock('@object-ui/react', async (importOriginal) => { + const actual = await importOriginal>(); + return { ...actual, useAdapter: () => ({}) }; +}); + +// Rail siblings / docks irrelevant to the top bar — keep the render light. +vi.mock('../../components/SuggestedBindingsPanel', () => ({ SuggestedBindingsPanel: () => null })); +vi.mock('../metadata-admin/AccessExplainPanel', () => ({ AccessExplainPanel: () => null })); +vi.mock('./StudioAiCopilot', () => ({ StudioChatDock: () => null })); +vi.mock('../../preview/DraftChangesPanel', () => ({ DraftChangesPanel: () => null })); + +import { StudioDesignSurface } from './StudioDesignSurface'; + +// jsdom ships neither of these; useIsMobile / useIsWideViewport and the Radix +// popover's floating-ui measurement need them. +window.matchMedia = ((query: string) => ({ + matches: false, + media: query, + onchange: null, + addEventListener: () => {}, + removeEventListener: () => {}, + addListener: () => {}, + removeListener: () => {}, + dispatchEvent: () => false, +})) as unknown as typeof window.matchMedia; +(globalThis as unknown as { ResizeObserver: unknown }).ResizeObserver = + (globalThis as unknown as { ResizeObserver?: unknown }).ResizeObserver ?? + class { + observe() {} + unobserve() {} + disconnect() {} + }; + +/** Raw-`fetch` calls to the bare packages endpoint — that is `fetchFullPackage`, + * the managed-snapshot refresh `onManageChanged` performs AFTER the + * deletion decision. Counting it gives the negative pin a positive event to + * wait for: the callback provably ran past the branch that used to navigate. */ +let snapshotFetches = 0; + +beforeEach(() => { + fetchPackagesMock.mockReset(); + toastError.mockReset(); + snapshotFetches = 0; + vi.stubGlobal( + 'fetch', + vi.fn(async (input: unknown) => { + const url = String(input); + if (url === '/api/v1/packages') { + snapshotFetches += 1; + // What `fetchFullPackage` reads: the full installed record. + return { ok: true, json: async () => [{ manifest: { id: PACKAGE_ID, name: PACKAGE_ID } }] }; + } + // The pending-drafts counter and the automation status probe. + return { ok: true, json: async () => [] }; + }) as unknown as typeof fetch, + ); +}); + +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); +}); + +function LocationProbe() { + return
{useLocation().pathname}
; +} + +function renderSurface() { + return render( + + + + } /> + } /> + + , + ); +} + +const trigger = () => screen.getByTitle('Switch / create package'); +const where = () => screen.getByTestId('location').textContent; + +/** + * Open the lifecycle sheet the way the author does — switcher trigger → + * "Package info & settings" — and hand back its "a lifecycle action ran" + * button. Every case shares this drive, so the ONLY difference between the + * pins below is what the refresh does. + */ +async function openLifecycleSheet(initial = [row(PACKAGE_ID)]): Promise { + fetchPackagesMock.mockResolvedValue(initial); + renderSurface(); + await waitFor(() => expect(trigger()).toHaveAttribute('data-pkg-list-state', 'loaded')); + fireEvent.click(trigger()); + fireEvent.click(await screen.findByText('Package info & settings')); + const sheet = await screen.findByTestId('pkg-sheet'); + expect(sheet).toHaveAttribute('data-managed-id', PACKAGE_ID); + toastError.mockReset(); + return screen.getByTestId('lifecycle-ran'); +} + +describe('Studio package lifecycle — a failed refresh is not a deletion (#7821)', () => { + it('FAILED refresh: the author is NOT evicted — no navigation at all', async () => { + const lifecycle = await openLifecycleSheet(); + const before = snapshotFetches; + + // The refresh that follows the lifecycle action rejects. + fetchPackagesMock.mockRejectedValue(new Error('Service Unavailable')); + fireEvent.click(lifecycle); + + // Wait until the callback has SETTLED, whichever way it goes: either it + // navigated (the defect — the `/home` route mounts) or it went on to + // refresh the managed snapshot (what follows the deletion decision). Both + // arms are observable, so the assertions below are not racing a pending + // navigation, and the red lands on the eviction itself rather than on a + // timeout. + await waitFor(() => + expect(snapshotFetches > before || screen.queryByTestId('home-page') !== null).toBe(true), + ); + + expect(where()).toBe(START_PATH); + expect(screen.queryByTestId('home-page')).not.toBeInTheDocument(); + // Still in the editor, still on the same package. + expect(trigger()).toHaveTextContent(PACKAGE_ID); + }); + + it('FAILED refresh: the failure is still REPORTED through this file\'s own posture', async () => { + const lifecycle = await openLifecycleSheet(); + + fetchPackagesMock.mockRejectedValue(new Error('Service Unavailable')); + fireEvent.click(lifecycle); + + // objectui#7368's channel, reused: the error's own message, on the one + // shared sonner id, so an outage that rejects several call sites is one + // toast rather than a stack. + await waitFor(() => expect(toastError).toHaveBeenCalled()); + expect(toastError.mock.calls[0][0]).toBe('Service Unavailable'); + expect((toastError.mock.calls[0][1] as { id?: string } | undefined)?.id).toBe('studio-package-list'); + + // …and recorded, so the switcher stops presenting the now-stale list as + // though it were current. + await waitFor(() => expect(trigger()).toHaveAttribute('data-pkg-list-state', 'failed')); + // ⛔ Never a throw: the top bar is still a working trigger. + fireEvent.click(trigger()); + expect(await screen.findByText('Packages (apps)')).toBeInTheDocument(); + }); + + it('REAL deletion, nothing left: still navigates to /home (behaviour unchanged)', async () => { + const lifecycle = await openLifecycleSheet(); + + // The list came back — successfully — and the package is gone from it. + fetchPackagesMock.mockResolvedValue([]); + fireEvent.click(lifecycle); + + expect(await screen.findByTestId('home-page')).toBeInTheDocument(); + expect(where()).toBe('/home'); + // A successful refresh is not a failure: nothing was reported. + expect(toastError).not.toHaveBeenCalled(); + }); + + it('REAL deletion, a sibling survives: still navigates to that sibling (behaviour unchanged)', async () => { + const lifecycle = await openLifecycleSheet([row(PACKAGE_ID), row(SIBLING_ID)]); + + fetchPackagesMock.mockResolvedValue([row(SIBLING_ID)]); + fireEvent.click(lifecycle); + + await waitFor(() => expect(where()).toBe(`/studio/${SIBLING_ID}/interfaces`)); + expect(screen.queryByTestId('home-page')).not.toBeInTheDocument(); + }); + + it('successful refresh, package still there: no navigation, and an earlier failure clears', async () => { + const lifecycle = await openLifecycleSheet(); + + fetchPackagesMock.mockResolvedValue([row(PACKAGE_ID)]); + const before = snapshotFetches; + fireEvent.click(lifecycle); + + await waitFor(() => expect(snapshotFetches).toBeGreaterThan(before)); + expect(where()).toBe(START_PATH); + expect(trigger()).toHaveAttribute('data-pkg-list-state', 'loaded'); + }); +}); diff --git a/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx b/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx index c4239c932a..781e0c893d 100644 --- a/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx +++ b/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx @@ -388,16 +388,45 @@ function PackageSwitcher({ // snapshot (so an edit shows immediately). If the managed package was the one // we're editing and it's now gone (deleted), jump to another package / home. const onManageChanged = React.useCallback(async () => { - let list: PkgEntry[] = []; + /** + * `null` means "the refresh did not tell us anything", which is NOT the + * same fact as "the server listed the packages and yours is not among + * them" (objectui#7821). Those two used to be the SAME value: `list` + * started as `[]` and the `.catch` left it that way — the comment there + * said "keep the stale list", true of the `pkgs` state, which simply is + * not written, but never of this local. So after a failed + * `GET /api/v1/packages` the `!list.some(...)` below was unconditionally + * true, the code took the branch labelled `// Deleted`, and with `list[0]` + * undefined a transient 503 evicted the author from the editor to + * `/home` — no toast, no confirmation, package still there. Absence of + * evidence is not evidence of deletion. + */ + let list: PkgEntry[] | null = null; try { list = await fetchPackages(); setPkgs(list); - } catch { - /* keep the stale list */ + // A list we did receive is current, so it also clears an earlier + // failure — otherwise the trigger would keep reading `failed` over + // names that are now fresh. + setPkgsErr(null); + } catch (e) { + // Reported through this surface's EXISTING posture (objectui#7368): + // `formatMetadataError` on the shared sonner id (one outage, one toast) + // and recorded, so the trigger reads `failed` rather than showing a + // stale list as though it were current. ⛔ Still not a `throw` — one + // 503 must not take the Studio down — and ⛔ still no retry, whose + // count / backoff / give-up state nobody has ruled on. + const message = formatMetadataError(e); + setPkgsErr(message); + toast.error(message, { id: PACKAGE_LIST_TOAST_ID }); } const managedId = manage?.manifest.id; if (!managedId) return; - if (!list.some((p) => p.id === managedId)) { + // A list we actually received, that does not contain the managed package, + // is the ONLY evidence of deletion. `list === null` draws no inference + // either way: the author stays put, and the managed snapshot below is + // still refreshed (that call reports its own outcome). + if (list !== null && !list.some((p) => p.id === managedId)) { // Deleted — only navigate away if it was the package we're editing. if (managedId === packageId) { const next = list[0];