From b777f1630c4732c07ec819297fc81e028ad0f436 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 00:24:07 +0000 Subject: [PATCH] fix(app-shell): unwrap the declared response envelope on the datasource page and the api-action runner (objectstack#3843) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit framework#3843 moves four more route modules onto BaseResponseSchema, so /api/v1/datasources/*, /api/v1/packages/* and /api/settings/* now answer { success, data } / { success: false, error: { code, message } }. Two consumers here were not envelope-aware: DatasourceResourcePage — its shared api() helper returned the raw body, so all nine payload reads (data.datasources, data.drivers, data.tables, { draft }, …) would have gone undefined, and its error path did `body.message || body.error` — which stringifies the new nested error object to "[object Object]". MetadataTypeActions — the generic `type: 'api'` action runner. Same "[object Object]" on any nested error, plus a resultDialog would have bound its `path` fields against the envelope wrapper instead of the payload. Both now unwrap `{ success, data }` centrally and read error.message. The packages readers needed no change: parsePackages, apiJson and fetchFullPackage were already tolerant (`payload?.data ?? payload`). Both shapes are accepted on purpose. The framework converts these modules one at a time (#3675 → #3689 → #3843), so a console build must talk to a backend on either side of that sequence — the same tolerance the attachment openers carry for #3689. It is a migration device, not a second contract; the producer stays the authority. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z --- .../MetadataTypeActions.test.tsx | 79 ++++++++++++++++++- .../metadata-admin/MetadataTypeActions.tsx | 31 +++++++- .../datasource/DatasourceResourcePage.tsx | 30 ++++++- 3 files changed, 133 insertions(+), 7 deletions(-) diff --git a/packages/app-shell/src/views/metadata-admin/MetadataTypeActions.test.tsx b/packages/app-shell/src/views/metadata-admin/MetadataTypeActions.test.tsx index a18578e3a8..ebaf9581c9 100644 --- a/packages/app-shell/src/views/metadata-admin/MetadataTypeActions.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/MetadataTypeActions.test.tsx @@ -13,6 +13,11 @@ const mockFetch = vi.fn(async () => ({ })); vi.mock('@object-ui/auth', () => ({ createAuthenticatedFetch: () => mockFetch })); +// Toasts are the only observable output of the error path. +const toastError = vi.fn(); +const toastSuccess = vi.fn(); +vi.mock('sonner', () => ({ toast: { error: (...a: unknown[]) => toastError(...a), success: (...a: unknown[]) => toastSuccess(...a) } })); + // Stub the param dialog: when open, expose a button that resolves with values — // lets us drive the collect-params promise without the heavy field renderers. vi.mock('../ActionParamDialog', () => ({ @@ -31,7 +36,11 @@ vi.mock('../ActionResultDialog', () => ({ import { MetadataTypeActions } from './MetadataTypeActions'; -beforeEach(() => mockFetch.mockClear()); +beforeEach(() => { + mockFetch.mockClear(); + toastError.mockClear(); + toastSuccess.mockClear(); +}); describe('MetadataTypeActions', () => { it('runs an api action without params directly (no dialog)', async () => { @@ -79,4 +88,72 @@ describe('MetadataTypeActions', () => { await waitFor(() => expect(screen.getByTestId('result-dialog')).toBeTruthy()); expect(screen.getByTestId('result-dialog').textContent).toContain('done'); }); + + // ── framework#3843: the declared `{ success, data }` envelope ────────────── + // Service route modules (`/api/v1/datasources`, `/api/v1/packages`, + // `/api/settings`, …) answer `BaseResponseSchema`, so an action's payload + // arrives under `data`. Both shapes are accepted so this repo is not coupled + // to the framework's module-by-module merge order. + + it('unwraps the { success, data } envelope before binding the result dialog', async () => { + mockFetch.mockResolvedValueOnce({ + ok: true, + status: 200, + statusText: 'OK', + json: async () => ({ success: true, data: { message: 'connection ok', latencyMs: 7 } }), + } as never); + render( + , + ); + fireEvent.click(screen.getByTitle('Probe')); + await waitFor(() => expect(screen.getByTestId('result-dialog')).toBeTruthy()); + const shown = screen.getByTestId('result-dialog').textContent ?? ''; + expect(shown).toContain('connection ok'); + // The wrapper itself must not reach the dialog's `path` bindings. + expect(JSON.parse(shown)).toEqual({ message: 'connection ok', latencyMs: 7 }); + }); + + it('reads the failure message out of the nested error object, not "[object Object]"', async () => { + mockFetch.mockResolvedValueOnce({ + ok: false, + status: 400, + statusText: 'Bad Request', + json: async () => ({ success: false, error: { code: 'datasource_admin_error', message: 'duplicate name' } }), + } as never); + render( + , + ); + fireEvent.click(screen.getByTitle('Probe')); + await waitFor(() => expect(toastError).toHaveBeenCalled()); + const msg = String(toastError.mock.calls[0][0]); + expect(msg).toContain('duplicate name'); + expect(msg).not.toContain('[object Object]'); + }); + + it('still reads a pre-envelope bare-string error — merge order is not a coupling', async () => { + mockFetch.mockResolvedValueOnce({ + ok: false, + status: 503, + statusText: 'Service Unavailable', + json: async () => ({ error: 'datasource_admin_unavailable' }), + } as never); + render( + , + ); + fireEvent.click(screen.getByTitle('Probe')); + await waitFor(() => expect(toastError).toHaveBeenCalled()); + expect(String(toastError.mock.calls[0][0])).toContain('datasource_admin_unavailable'); + }); }); diff --git a/packages/app-shell/src/views/metadata-admin/MetadataTypeActions.tsx b/packages/app-shell/src/views/metadata-admin/MetadataTypeActions.tsx index b85b5be409..5bc8db2369 100644 --- a/packages/app-shell/src/views/metadata-admin/MetadataTypeActions.tsx +++ b/packages/app-shell/src/views/metadata-admin/MetadataTypeActions.tsx @@ -155,16 +155,39 @@ export function MetadataTypeActions({ entry, location, recordId, onAfter }: Meta if (method !== 'GET' && method !== 'DELETE') init.body = JSON.stringify(params); const res = await authFetch(url, init); - let data: Record | null = null; + let body: Record | null = null; try { - data = (await res.json()) as Record; + body = (await res.json()) as Record; } catch { /* non-JSON / empty body — fall back to status text */ } - if (!res.ok || (data && data.success === false)) { + // framework#3843: the service route modules now answer the declared + // `{ success, data }` envelope (`BaseResponseSchema`), so the payload a + // `resultDialog` binds to — and the `message` the success toast reads — + // lives under `data` rather than at the top level. Unwrap it here, in the + // one place every `type: 'api'` action passes through. + // + // Deliberately tolerant of an already-unwrapped body: endpoints an action + // may target are converted module by module (framework#3675 → #3689 → + // #3843), so this repo must not be coupled to the merge order of that + // sequence. Same reason the two attachment openers read + // `body?.url ?? body?.data?.url` for framework#3689. + const data = + body && typeof body.success === 'boolean' && 'data' in body + ? (body.data as Record | null) + : body; + + if (!res.ok || (body && body.success === false)) { + // `error` is `{ code, message }` in the envelope, and was a bare string + // before it — read both so a partially-converted backend still explains + // itself instead of toasting "[object Object]". + const err = body?.error as { message?: string } | string | undefined; const detail = - (data?.error as string) || (data?.message as string) || `HTTP ${res.status} ${res.statusText}`.trim(); + (typeof err === 'object' && err !== null ? err.message : undefined) || + (typeof err === 'string' ? err : undefined) || + (body?.message as string) || + `HTTP ${res.status} ${res.statusText}`.trim(); toast.error(`${action.errorMessage ? `${action.errorMessage}: ` : ''}${title}: ${detail}`); return; } diff --git a/packages/app-shell/src/views/metadata-admin/datasource/DatasourceResourcePage.tsx b/packages/app-shell/src/views/metadata-admin/datasource/DatasourceResourcePage.tsx index 1d9f2da712..260221e804 100644 --- a/packages/app-shell/src/views/metadata-admin/datasource/DatasourceResourcePage.tsx +++ b/packages/app-shell/src/views/metadata-admin/datasource/DatasourceResourcePage.tsx @@ -172,14 +172,40 @@ export function DatasourceResourcePage(_props: { type?: string }): React.ReactEl const [loading, setLoading] = React.useState(true); const [busy, setBusy] = React.useState(null); + /** + * Fetch + unwrap the declared response envelope. + * + * framework#3843 moved `/api/v1/datasources/*` onto `BaseResponseSchema`, so + * every body is now `{ success, data }` / `{ success: false, error: { code, + * message } }`. Unwrapping here keeps all nine call sites below reading their + * payload key unchanged (`data.datasources`, `data.drivers`, `data.tables`, + * `{ draft }`, …) — the keys did not change, only their depth. + * + * Both shapes are accepted on purpose, error and success alike. This is the + * same tolerance the console already carries for framework#3689 (the + * attachment openers' `body?.url ?? body?.data?.url`), and it exists so the + * two repos are not coupled by merge order: a console on either side of the + * framework change talks to a backend on either side of it. It is a migration + * device, not a permanent second contract — the producer is the authority + * (framework Prime Directive #12). + */ const api = React.useCallback( async (path: string, init?: RequestInit) => { const res = await authFetch(`${SERVER}${path}`, { credentials: 'include', headers: { 'Content-Type': 'application/json' }, ...init }); const text = await res.text(); let body: any = text; try { body = JSON.parse(text); } catch { /* keep text */ } - if (!res.ok) throw new Error((body && (body.message || body.error)) || `HTTP ${res.status}`); - return body; + if (!res.ok) { + // `error` is `{ code, message }` post-#3843 and was a bare string + // before it; without the first read this threw "[object Object]". + const detail = + (body && typeof body.error === 'object' && body.error?.message) || + (body && typeof body.error === 'string' ? body.error : undefined) || + (body && body.message) || + `HTTP ${res.status}`; + throw new Error(detail); + } + return body && typeof body.success === 'boolean' && 'data' in body ? body.data : body; }, [authFetch], );