From de48b50c853f2b3bb0945e9a7fc3603715bd5000 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 04:24:23 +0000 Subject: [PATCH] fix(app-shell): gate /meta/* on a resolved session and identify HTTP failures in the log (#4042) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening a logged-out console painted ~30 red `HTTP request failed` lines before the login form was drawn. Two independent causes. 1. Requests fired before the session was known. ConnectedShellInner now withholds the metadata tree until GET /auth/get-session resolves, and the console's `/` route — which mounted ConnectedShell with no AuthGuard above it — is now guarded, so an anonymous visitor reaches /login without a single doomed request. Same fix in console-starter. 2. Two requests per type per mount, signed in as well. Consumers read metadata during the FIRST render, before any effect runs; MetadataProvider's preview-mode effect then cleared the cache on mount, discarding those entries mid-flight so the next render refetched them. That effect now skips its mount run. A second duplicate appeared only after a failure, where callers arriving just after the rejection each started a fresh attempt — a failed type now stays un-retried for ~1s, which collapses one mount's burst without touching refresh()/invalidate(). 3. `HTTP request failed` now names the request. The client passes method/url/status as a third argument, which every console-flattener renders as `[object Object]`; those fields now go into the message string too, alongside the structured bag. Nothing is newly silenced: the only demotion remains 404-on-an-optional- collection, and a 401 surviving the gate stays a visible, identified error. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .changeset/login-meta-401-noise-4042.md | 67 ++++++++++ apps/console/src/App.tsx | 18 ++- examples/console-starter/src/App.tsx | 11 +- .../app-shell/src/console/ConsoleShell.tsx | 24 +++- .../ConnectedShell.sessionGate.test.tsx | 126 ++++++++++++++++++ .../src/providers/MetadataProvider.tsx | 48 +++++++ .../MetadataProvider.requestBudget.test.tsx | 125 +++++++++++++++++ .../src/httpFailureLogging.test.ts | 109 +++++++++++++++ packages/data-objectstack/src/index.ts | 80 +++++++++-- 9 files changed, 596 insertions(+), 12 deletions(-) create mode 100644 .changeset/login-meta-401-noise-4042.md create mode 100644 packages/app-shell/src/console/__tests__/ConnectedShell.sessionGate.test.tsx create mode 100644 packages/app-shell/src/providers/__tests__/MetadataProvider.requestBudget.test.tsx create mode 100644 packages/data-objectstack/src/httpFailureLogging.test.ts diff --git a/.changeset/login-meta-401-noise-4042.md b/.changeset/login-meta-401-noise-4042.md new file mode 100644 index 0000000000..c1a69158bf --- /dev/null +++ b/.changeset/login-meta-401-noise-4042.md @@ -0,0 +1,67 @@ +--- +"@object-ui/app-shell": patch +"@object-ui/data-objectstack": patch +--- + +The console no longer reads `/meta/*` before it knows whether it has a session, and a failed request now says which request failed + +Opening a logged-out console painted ~30 red `HTTP request failed` lines before +the login form was drawn. Two independent causes, fixed independently +(objectui#4042). + +**1. Requests fired before the session was known.** `ConnectedShellInner` now +withholds the metadata tree until `GET /auth/get-session` resolves, so +`meta/object` / `meta/view` / `meta/app` are never issued blind. `useAuth()` +outside an `AuthProvider` reports `isLoading: false`, so an embed with no auth +provider is unaffected, and every protected route already sat behind an +`AuthGuard` that resolves auth first — the signed-in data flow is unchanged. + +The console's landing route (``) was the actual entry point for +the burst: it mounted `ConnectedShell` with no guard above it, so simply opening +`/_console/` mounted the whole data layer as an anonymous visitor. It is now +guarded, which also means an unauthenticated visitor reaches `/login` without a +single doomed request. `examples/console-starter` had the same shape and got the +same fix. + +**2. Two requests per type, per mount — not an unauthenticated artefact.** +Consumers read metadata during the FIRST render (`useActionModal` reads +`objects`, whose getter kicks `ensureType('object')` and `ensureType('view')` +from the render phase), before any effect runs. `MetadataProvider`'s preview-mode +effect then cleared the whole cache on mount, discarding those two entries while +their requests were in flight; the next render found them `idle` and refetched +both. The effect now skips its mount run — on mount the cache is empty and there +was never anything to drop; it only ever meant something on a later +`previewDrafts` change. That halved `meta/object` and `meta/view` on **every** +mount, signed in included. + +A second duplicate only appeared once a read had failed: `entry.promise` +collapses callers that arrive while a request is in flight, but callers arriving +just after a failure each started a fresh attempt. A failed type now stays +un-retried for ~1s, which collapses one mount's burst of callers into a single +attempt. This is deliberately not the 5-minute `ttlMs` — later callers still +retry on their own, and `refresh()` / `invalidate()` retry immediately and +unconditionally, so no explicit recovery path changes. + +**3. `HTTP request failed` now identifies the request.** `@objectstack/client` +reports every non-2xx as +`logger.error("HTTP request failed", undefined, { method, url, status, error })`, +and the console's logger forwarded that verbatim — so the identifying fields +lived only in the third argument, and anything that flattens a console record to +text rendered them `[object Object]` / `Object`. A screenful of failures could +not tell you a single URL or status. The message string now carries them: + +```text +HTTP request failed: GET /api/v1/meta/object -> 401 [UNAUTHORIZED] +``` + +The structured bag is still passed alongside for DevTools to expand — text for +the flatteners, object for the inspectors, neither at the other's expense. The +formatter is exported as `formatHttpFailureMessage`, and `createQuietHttpLogger` +is now exported too so an app wiring its own `ObjectStackClient` gets the same +identified failures. + +Nothing is newly silenced. The only demotion remains 404-on-an-optional- +collection (`sys_presence`, `sys_activity`), which is an expected outcome of a +request we still mean to make; a 401 that survives the session gate — a +mid-session expiry, say — stays a visible, fully-identified error. The cure for +doomed requests is not issuing them, never hiding them once issued. diff --git a/apps/console/src/App.tsx b/apps/console/src/App.tsx index 70252bdfa5..2201d1126e 100644 --- a/apps/console/src/App.tsx +++ b/apps/console/src/App.tsx @@ -350,7 +350,23 @@ export function App() { } /> - } /> + {/* + * The landing resolver reads App METADATA, so it needs the data + * layer — which needs a session. It used to mount `ConnectedShell` + * bare, with no AuthGuard above it, so an unauthenticated visitor + * opening `/_console/` mounted the whole metadata tree and fired + * `meta/object` + `meta/view` + `meta/app` straight into 401 + * before the login form was drawn (objectui#4042). Guarding it + * sends those visitors to /login without a single doomed request; + * a signed-in visitor still lands exactly where `isDefault` + * resolves. `requireOrganization={false}` because `/` only + * redirects — the org gate belongs to the destination route. + */} + + + + } /> } /> diff --git a/examples/console-starter/src/App.tsx b/examples/console-starter/src/App.tsx index 4520b07f6d..bc43c23d4f 100644 --- a/examples/console-starter/src/App.tsx +++ b/examples/console-starter/src/App.tsx @@ -17,7 +17,6 @@ import { AuthProvider, useAuth } from '@object-ui/auth'; import { Toaster } from 'sonner'; import { ConsoleShell, - ConnectedShell, AuthenticatedRoute, RootRedirect, SystemRedirect, @@ -69,7 +68,15 @@ export function App() { } /> - } /> + {/* `RootRedirect` resolves the landing from metadata, so it needs a + * session. Guarded (not a bare `ConnectedShell`) so an + * unauthenticated visitor goes to /login instead of firing a round + * of doomed 401 `/meta/*` reads first — objectui#4042. */} + + + + } /> } /> diff --git a/packages/app-shell/src/console/ConsoleShell.tsx b/packages/app-shell/src/console/ConsoleShell.tsx index 3f5361b961..3bab0dc1cf 100644 --- a/packages/app-shell/src/console/ConsoleShell.tsx +++ b/packages/app-shell/src/console/ConsoleShell.tsx @@ -189,6 +189,28 @@ const apiProviderFetch = withSettleSignal(createAuthenticatedFetch({ sameOriginO function ConnectedShellInner({ children }: { children: ReactNode }) { const adapter = useAdapter(); const { language } = useObjectTranslation(); + // ── Session gate (objectui#4042) ── + // + // Everything below this line reads `/api/v1/meta/*`, which requires a + // session. Mounting the metadata tree while `GET /auth/get-session` is still + // in flight fires `meta/object` + `meta/view` + `meta/app` blind: on an + // unauthenticated visitor they all come back 401, and the console's landing + // route (``) mounts this shell with no AuthGuard above it, so + // simply opening `/_console/` produced a screenful of red `HTTP request + // failed` before the login form was even drawn. + // + // Waiting for auth to RESOLVE is the whole gate. We deliberately do not gate + // on `isAuthenticated`: a consumer that mounts ConnectedShell outside an + // AuthGuard still renders through once the session answer is known (unchanged + // behaviour, including a legitimate 401 that must stay visible), and the + // console's own login bounce is the route guard's job. `useAuth()` outside an + // AuthProvider reports `isLoading: false`, so a provider-less embed is + // untouched. + // + // Post-login flow is unchanged — AuthGuard already withholds every protected + // route until auth resolves, so on those routes this gate is already open by + // the time the shell mounts and the same queries fire, once each. + const { isLoading: isAuthLoading } = useAuth(); // ── Language switch → relabel without a page refresh (issue #1319) ── // @@ -216,7 +238,7 @@ function ConnectedShellInner({ children }: { children: ReactNode }) { } if (adapter) lastLanguage.current = language; - if (!adapter) return ; + if (!adapter || isAuthLoading) return ; // Expose the adapter via SchemaRendererContext so descendant hooks like // useDiscovery() (used to gate the global AI chatbot) can resolve it. return ( diff --git a/packages/app-shell/src/console/__tests__/ConnectedShell.sessionGate.test.tsx b/packages/app-shell/src/console/__tests__/ConnectedShell.sessionGate.test.tsx new file mode 100644 index 0000000000..56647ddb41 --- /dev/null +++ b/packages/app-shell/src/console/__tests__/ConnectedShell.sessionGate.test.tsx @@ -0,0 +1,126 @@ +/** + * objectui#4042 — the console must not read `/meta/*` before it knows whether + * it has a session. + * + * The reported symptom was ~30 red `HTTP request failed` lines on a freshly + * opened, still-logged-out console: `GET /auth/get-session` had already + * answered "no session" (and, worse, was sometimes still in flight) while + * `meta/object` / `meta/view` / `meta/app` were fired anyway, all doomed to + * 401. `ConnectedShellInner` now withholds the metadata tree until auth + * resolves. + * + * Both directions are pinned, because the fix is worth nothing if it also + * stops the signed-in console from loading: + * - auth pending → zero metadata requests; + * - auth resolved → the same three requests, once each. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, waitFor } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +/** Flipped per-test; read by the mocked `useAuth`. */ +const authState = { isLoading: true, isAuthenticated: false }; + +/** Every `meta.getItems(type)` the shell issues, in order. */ +const metaCalls: string[] = []; + +const fakeAdapter = { + clearCache: vi.fn(), + getObjectSchema: vi.fn(async () => null), + getClient: () => ({ + meta: { + getItems: (type: string) => { + metaCalls.push(type); + return Promise.resolve({ type, items: [] }); + }, + getItem: (type: string, name: string) => { + metaCalls.push(`${type}/${name}`); + return Promise.resolve({ item: null }); + }, + }, + }), +}; + +vi.mock('@object-ui/auth', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useAuth: () => ({ + ...authState, + user: authState.isAuthenticated ? { id: 'u1', email: 'u@example.com' } : null, + session: null, + isAuthEnabled: true, + error: null, + }), + createAuthenticatedFetch: () => globalThis.fetch, + }; +}); + +// The adapter itself is not under test — the shell's ORDERING is. Hand it a +// ready fake so `useAdapter()` never gates the render for an unrelated reason. +vi.mock('../../providers/AdapterProvider', () => ({ + AdapterProvider: ({ children }: { children: React.ReactNode }) => <>{children}, + useAdapter: () => fakeAdapter, +})); + +import { ConnectedShell } from '../ConsoleShell'; + +function renderShell() { + return render( + + +
ROUTE CONTENT
+ + , + ); +} + +describe('ConnectedShell session gate (objectui#4042)', () => { + beforeEach(() => { + metaCalls.length = 0; + authState.isLoading = true; + authState.isAuthenticated = false; + }); + + it('issues no /meta/* request while get-session is still in flight', async () => { + const { queryByTestId } = renderShell(); + + // Give every effect and microtask a chance to fire a request. + await new Promise((resolve) => setTimeout(resolve, 100)); + + expect(metaCalls).toEqual([]); + // And the gate is a WAIT, not a render-through: the metadata-backed route + // content must not be showing either (it would read empty metadata). + expect(queryByTestId('route-content')).toBeNull(); + }); + + it('issues each /meta/* request exactly once as soon as the session resolves', async () => { + authState.isLoading = false; + authState.isAuthenticated = true; + + const { getByTestId } = renderShell(); + + await waitFor(() => expect(metaCalls).toContain('app')); + await new Promise((resolve) => setTimeout(resolve, 100)); + + expect(metaCalls.filter((c) => c === 'object')).toHaveLength(1); + expect(metaCalls.filter((c) => c === 'view')).toHaveLength(1); + expect(metaCalls.filter((c) => c === 'app')).toHaveLength(1); + expect(getByTestId('route-content')).toBeTruthy(); + }); + + it('renders through once auth resolves with NO session (a 401 is then real, not noise)', async () => { + // A consumer may mount ConnectedShell outside an AuthGuard. The gate is + // "wait for the answer", not "require a yes" — once the answer is known the + // tree renders and any resulting 401 is a genuine, loggable event rather + // than a request the console fired blind. + authState.isLoading = false; + authState.isAuthenticated = false; + + const { getByTestId } = renderShell(); + + await waitFor(() => expect(metaCalls).toContain('app')); + expect(getByTestId('route-content')).toBeTruthy(); + }); +}); diff --git a/packages/app-shell/src/providers/MetadataProvider.tsx b/packages/app-shell/src/providers/MetadataProvider.tsx index 397aded8d5..d8e8023b08 100644 --- a/packages/app-shell/src/providers/MetadataProvider.tsx +++ b/packages/app-shell/src/providers/MetadataProvider.tsx @@ -49,6 +49,25 @@ interface MetadataProviderProps { const DEFAULT_TTL_MS = 5 * 60 * 1000; const EAGER_TYPES = ['app', 'view'] as const; +/** + * How long a FAILED type stays un-retried (objectui#4042). + * + * `entry.promise` already collapses callers that arrive while a request is in + * flight — but callers that arrive just AFTER a failure found `status: 'error'` + * with `promise: null` and each started a fresh attempt. That is a real + * sequence, not a hypothetical: the mount effect walks EAGER_TYPES serially, so + * by the time it reaches `view` the render-phase read of `view` has already + * failed, and it re-requested it. Signed out, that was a second doomed 401 per + * type; signed in, a second doomed request on any transient failure. + * + * Deliberately ~1s and NOT `ttlMs`: this exists to collapse the burst of + * callers that start together during one mount, not to cache failures. A later + * caller (route change, remount) still retries on its own, and `refresh()` / + * `invalidate()` — which zero `fetchedAt` / reset the status — retry + * immediately and unconditionally, so no explicit recovery path is affected. + */ +const ERROR_RETRY_COOLDOWN_MS = 1000; + const TYPE_BY_STATE_KEY: Record, string> = { apps: 'app', objects: 'object', @@ -387,7 +406,24 @@ export function MetadataProvider({ children, adapter, ttlMs = DEFAULT_TTL_MS }: // Entering/leaving preview swaps the entire metadata source — the published // and draft-overlaid worlds must never mix in one cache. Drop everything and // let consumers refetch through the new source. + // + // ⚠️ Deliberately skipped on MOUNT (objectui#4042). Consumers read metadata + // during the FIRST render — `useActionModal` reads `objects`, which kicks + // `ensureType('object')` and `ensureType('view')` from a render-phase getter, + // before any effect has run. Clearing unconditionally in this mount effect + // threw those two entries away while their requests were still in flight, so + // the very next render found them `idle` again and refetched BOTH. That is + // the "same round, `meta/object` / `meta/view` each fired twice" the card + // reported — and it is not an unauthenticated-only artefact: it doubled the + // two requests on every mount, signed in as well. There is nothing to drop on + // mount anyway (the cache is per-provider-instance and starts empty), so the + // clear only ever had meaning on a LATER `previewDrafts` change. + const previewModeMounted = useRef(false); useEffect(() => { + if (!previewModeMounted.current) { + previewModeMounted.current = true; + return; + } cacheRef.current.clear(); itemPromisesRef.current.clear(); bump(); @@ -413,6 +449,14 @@ export function MetadataProvider({ children, adapter, ttlMs = DEFAULT_TTL_MS }: return Promise.resolve(entry.items); } + // Just failed — see ERROR_RETRY_COOLDOWN_MS. `refresh()`/`invalidate()` + // zero `fetchedAt` / reset the status, so explicit retries fall straight + // through this. + if (entry.status === 'error' && Date.now() - entry.fetchedAt < ERROR_RETRY_COOLDOWN_MS) { + debug(`cache hit (recent failure) type=${type}`); + return Promise.resolve(entry.items); + } + const started = Date.now(); entry.status = 'loading'; entry.error = null; @@ -455,6 +499,10 @@ export function MetadataProvider({ children, adapter, ttlMs = DEFAULT_TTL_MS }: entry.status = 'error'; entry.error = error; entry.promise = null; + // Stamped on failure too, so ERROR_RETRY_COOLDOWN_MS has a clock to + // measure from. `refresh()` zeroes it, which is what makes an + // explicit retry immediate. + entry.fetchedAt = Date.now(); debug(`fetch failed type=${type}`, error); bump(); return [] as any[]; diff --git a/packages/app-shell/src/providers/__tests__/MetadataProvider.requestBudget.test.tsx b/packages/app-shell/src/providers/__tests__/MetadataProvider.requestBudget.test.tsx new file mode 100644 index 0000000000..7a79fdff45 --- /dev/null +++ b/packages/app-shell/src/providers/__tests__/MetadataProvider.requestBudget.test.tsx @@ -0,0 +1,125 @@ +/** + * objectui#4042 — one `/meta/` request per type per mount. + * + * The card reported that opening the console produced, in a single render + * round, `meta/object` and `meta/view` TWICE each alongside a single + * `meta/app`. It surfaced as pre-login 401 noise, but the doubling is not an + * unauthenticated artefact — these tests pin BOTH directions (the 200 path and + * the 401 path) because the wasted round trip happened just as much signed in. + * + * Mechanism, for whoever breaks this next: consumers read metadata during the + * FIRST render (`useActionModal` reads `objects`, whose getter kicks + * `ensureType('object')` and `ensureType('view')` from the render phase), which + * is before any effect runs. MetadataProvider's preview-mode effect used to + * clear the whole cache unconditionally on mount, throwing those two entries + * away mid-flight; the next render found them `idle` and refetched both. The + * effect now skips its mount run — there is nothing to drop on mount anyway. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { useEffect } from 'react'; +import { render, waitFor, act } from '@testing-library/react'; +import { MetadataProvider, useMetadata } from '../MetadataProvider'; + +/** Records every `meta.getItems(type)` the provider issues, in order. */ +function makeRecordingAdapter(calls: string[], mode: 'ok' | 'unauthorized') { + return { + clearCache: vi.fn(), + getClient: () => ({ + meta: { + getItems: (type: string) => { + calls.push(type); + if (mode === 'unauthorized') { + return Promise.reject( + Object.assign(new Error('Unauthorized'), { httpStatus: 401, code: 'UNAUTHORIZED' }), + ); + } + return Promise.resolve({ type, items: [] }); + }, + getItem: (type: string, name: string) => { + calls.push(`${type}/${name}`); + return Promise.resolve({ item: null }); + }, + }, + }), + } as unknown as Parameters[0]['adapter']; +} + +/** + * Mirrors the real first-render consumer (`useActionModal`, mounted by + * `GlobalActionRuntimeProvider`): it reads `objects` on EVERY render, and the + * `objects` getter reads both the `object` and the `view` type. + */ +function ObjectsConsumer() { + const ctx = useMetadata(); + return
{ctx.objects.length}
; +} + +async function countCalls(mode: 'ok' | 'unauthorized'): Promise { + const calls: string[] = []; + render( + + + , + ); + // Wait for the eager types to have been requested, then let every pending + // microtask/timer settle so a late duplicate would still be recorded. + await waitFor(() => expect(calls).toContain('app')); + await new Promise((resolve) => setTimeout(resolve, 100)); + return calls; +} + +describe('MetadataProvider request budget (objectui#4042)', () => { + it('fetches each metadata type exactly once per mount on the success path', async () => { + const calls = await countCalls('ok'); + + expect(calls.filter((c) => c === 'object')).toHaveLength(1); + expect(calls.filter((c) => c === 'view')).toHaveLength(1); + expect(calls.filter((c) => c === 'app')).toHaveLength(1); + // Nothing else — no stray type, no third request. + expect([...calls].sort()).toEqual(['app', 'object', 'view']); + }); + + it('does not retry-storm when every metadata read is refused (401)', async () => { + const calls = await countCalls('unauthorized'); + + // A refused read must not become a louder read. Same budget as the success + // path: one attempt per type, then the entry parks in `error`. + expect([...calls].sort()).toEqual(['app', 'object', 'view']); + }); + + it('still retries a failed type immediately on an EXPLICIT refresh()', async () => { + // The failure cooldown collapses the mount-time burst; it must not turn + // `refresh()` — the console's recover-from-error path — into a no-op. + const calls: string[] = []; + // Holder + effect, not a bare outer assignment during render — the same + // pattern the other console tests use, and what the react-compiler lint + // rule requires. + const captured: { current: ReturnType | null } = { current: null }; + + function Capture() { + const ctx = useMetadata(); + useEffect(() => { + captured.current = ctx; + }, [ctx]); + return
{ctx.objects.length}
; + } + + render( + + + , + ); + await waitFor(() => expect(calls).toContain('app')); + await new Promise((resolve) => setTimeout(resolve, 50)); + const beforeRefresh = calls.length; + + // Well inside ERROR_RETRY_COOLDOWN_MS — an explicit refresh must fetch anyway. + await act(async () => { + await captured.current!.refresh('object'); + }); + + expect(calls.filter((c) => c === 'object')).toHaveLength(2); + expect(calls.length).toBe(beforeRefresh + 1); + }); +}); diff --git a/packages/data-objectstack/src/httpFailureLogging.test.ts b/packages/data-objectstack/src/httpFailureLogging.test.ts new file mode 100644 index 0000000000..e25ef17878 --- /dev/null +++ b/packages/data-objectstack/src/httpFailureLogging.test.ts @@ -0,0 +1,109 @@ +/** + * objectui#4042 half 2 — a request failure must SAY which request failed. + * + * `@objectstack/client` reports every non-2xx as + * `logger.error("HTTP request failed", undefined, { method, url, status, error })`. + * The console's logger used to forward that verbatim, so the identifying + * fields lived only in the third argument: anything that flattens a console + * record to text rendered them `[object Object]` / `Object`, and a screenful of + * failures told you nothing about which URL or which status. These tests pin + * that the message string itself now carries method + url + status (+ code). + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { formatHttpFailureMessage, createQuietHttpLogger } from './index'; + +const META_401 = { + method: 'GET', + url: '/api/v1/meta/object', + status: 401, + error: { code: 'UNAUTHORIZED', message: 'Unauthorized' }, +}; + +describe('formatHttpFailureMessage', () => { + it('folds method, url and status into the message', () => { + expect(formatHttpFailureMessage('HTTP request failed', META_401)).toBe( + 'HTTP request failed: GET /api/v1/meta/object -> 401 [UNAUTHORIZED]', + ); + }); + + it('omits the code when the body carries none', () => { + expect( + formatHttpFailureMessage('HTTP request failed', { + method: 'POST', + url: '/api/v1/data/task', + status: 500, + }), + ).toBe('HTTP request failed: POST /api/v1/data/task -> 500'); + }); + + it('reads the `statusCode` spelling too', () => { + expect( + formatHttpFailureMessage('HTTP request failed', { url: '/api/v1/meta/app', statusCode: 403 }), + ).toBe('HTTP request failed: GET /api/v1/meta/app -> 403'); + }); + + it('returns null — not a husk — when meta identifies nothing', () => { + expect(formatHttpFailureMessage('HTTP request failed', undefined)).toBeNull(); + expect(formatHttpFailureMessage('HTTP request failed', {})).toBeNull(); + expect(formatHttpFailureMessage('HTTP request failed', { error: { message: 'boom' } })).toBeNull(); + }); + + it('never yields the un-diagnosable string the card reported', () => { + const line = formatHttpFailureMessage('HTTP request failed', META_401)!; + expect(line).not.toContain('[object Object]'); + expect(line).toContain('/api/v1/meta/object'); + expect(line).toContain('401'); + }); +}); + +describe('createQuietHttpLogger', () => { + afterEach(() => vi.restoreAllMocks()); + + it('logs a 401 as an error whose FIRST argument identifies the request', () => { + const spy = vi.spyOn(console, 'error').mockImplementation(() => {}); + createQuietHttpLogger().error('HTTP request failed', undefined, META_401); + + expect(spy).toHaveBeenCalledTimes(1); + const [message, , meta] = spy.mock.calls[0]; + expect(message).toBe('HTTP request failed: GET /api/v1/meta/object -> 401 [UNAUTHORIZED]'); + // The structured bag is still handed over for DevTools to expand — the + // string is added alongside it, not instead of it. + expect(meta).toEqual(META_401); + }); + + it('keeps demoting an expected 404, and identifies it in the demoted line too', () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const debugSpy = vi.spyOn(console, 'debug').mockImplementation(() => {}); + + createQuietHttpLogger().error('HTTP request failed', undefined, { + method: 'GET', + url: '/api/v1/data/sys_presence', + status: 404, + error: { code: 'OBJECT_NOT_FOUND' }, + }); + + expect(errorSpy).not.toHaveBeenCalled(); + expect(debugSpy).toHaveBeenCalledTimes(1); + expect(debugSpy.mock.calls[0][0]).toContain('/api/v1/data/sys_presence -> 404'); + }); + + it('does NOT silence a 401 — only 404-on-an-optional-collection is demoted', () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const debugSpy = vi.spyOn(console, 'debug').mockImplementation(() => {}); + + // Session expiry mid-use: the console gates doomed requests, it does not + // hide the ones that still fail. + createQuietHttpLogger().error('HTTP request failed', undefined, META_401); + + expect(errorSpy).toHaveBeenCalledTimes(1); + expect(debugSpy).not.toHaveBeenCalled(); + }); + + it('falls back to the bare message when there is nothing to identify', () => { + const spy = vi.spyOn(console, 'error').mockImplementation(() => {}); + createQuietHttpLogger().error('Something broke', undefined, undefined); + + expect(spy.mock.calls[0][0]).toBe('Something broke'); + }); +}); diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index bc6555e182..a4248edcd0 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -642,17 +642,78 @@ export function normaliseClientError(error: unknown): unknown { } /** - * Build a Logger compatible with @objectstack/client that demotes expected - * 404 noise to console.debug. The client logs every non-2xx response with - * `logger.error("HTTP request failed", undefined, { status, error })`, but - * 404s on optional collections (sys_presence, sys_activity, …) are part of + * Fold an @objectstack/client HTTP-failure `meta` bag into the log MESSAGE. + * + * The client already hands us everything worth knowing — + * `logger.error("HTTP request failed", undefined, { method, url, status, error })` + * — but it hands it as the THIRD argument. Everything that flattens a console + * record to text (a headless/CDP console capture, a log shipper, a copied + * DevTools line) keeps only the message and renders the rest as `[object + * Object]` / `Object`, so a wall of failures carried no method, no URL and no + * status: the reporter of objectui#4042 had to diff the network panel by hand + * to find out that 30 red lines were all one benign pre-login burst. + * + * So the identifying fields go into the string itself, and the structured bag + * is STILL passed alongside for DevTools to expand — text for the flatteners, + * object for the inspectors, neither at the other's expense. + * + * Exported for tests. Returns `null` when `meta` carries none of the three + * fields, so callers keep the original message rather than printing a husk. + */ +export function formatHttpFailureMessage( + message: string, + meta?: Record, +): string | null { + if (!meta || typeof meta !== 'object') return null; + const method = typeof meta.method === 'string' && meta.method ? meta.method : undefined; + const url = typeof meta.url === 'string' && meta.url ? meta.url : undefined; + const status = + typeof meta.status === 'number' + ? meta.status + : typeof meta.statusCode === 'number' + ? meta.statusCode + : undefined; + if (!method && !url && status === undefined) return null; + + // `code` is the ADR-0112 semantic error code. The client puts the parsed + // body under `meta.error`; some call sites hoist the code to the top level. + const errBody = meta.error && typeof meta.error === 'object' ? meta.error : undefined; + const rawCode = + (typeof meta.code === 'string' && meta.code) || + (errBody && typeof (errBody as Record).code === 'string' + ? ((errBody as Record).code) + : undefined); + + const parts = [method ?? 'GET', url ?? '']; + parts.push(`-> ${status ?? 'no status'}`); + if (rawCode) parts.push(`[${rawCode}]`); + return `${message}: ${parts.join(' ')}`; +} + +/** + * Build a Logger compatible with @objectstack/client that (a) spells every + * request failure out in the message — see {@link formatHttpFailureMessage} — + * and (b) demotes expected 404 noise to console.debug. The client logs every + * non-2xx response with + * `logger.error("HTTP request failed", undefined, { method, url, status, error })`, + * but 404s on optional collections (sys_presence, sys_activity, …) are part of * normal degraded operation when those plugins aren't installed on the * server — they should not surface as errors in the browser DevTools. * + * NOTE the asymmetry, and keep it: 404-on-an-optional-collection is demoted + * because it is an EXPECTED outcome of a request we still mean to make. No + * other status is demoted — a 401 that survives the console's session gate + * (objectui#4042: a mid-session expiry, say) is a real event and must stay a + * visible, fully-identified error. The cure for doomed requests is not issuing + * them, never hiding them once issued. + * * Returned object is loosely typed because the spec's Logger interface lives * in a transitive package; using `any` keeps us decoupled. + * + * Exported so the console's log contract is testable, and so an app wiring its + * own `ObjectStackClient` gets the same identified failures. */ -function createQuietHttpLogger(): any { +export function createQuietHttpLogger(): any { const isExpected404 = (meta?: Record): boolean => { if (!meta || typeof meta !== 'object') return false; if (meta.status === 404 || meta.statusCode === 404) return true; @@ -672,13 +733,16 @@ function createQuietHttpLogger(): any { console.warn(message, meta ?? ''), error: (message: string, error?: Error, meta?: Record) => { if (isExpected404(meta)) { - console.debug(`[ObjectStack] ${message} (suppressed expected 404)`, meta); + console.debug( + `[ObjectStack] ${formatHttpFailureMessage(message, meta) ?? message} (suppressed expected 404)`, + meta, + ); return; } - console.error(message, error ?? '', meta ?? ''); + console.error(formatHttpFailureMessage(message, meta) ?? message, error ?? '', meta ?? ''); }, fatal: (message: string, error?: Error, meta?: Record) => - console.error(message, error ?? '', meta ?? ''), + console.error(formatHttpFailureMessage(message, meta) ?? message, error ?? '', meta ?? ''), log: (message: string, ...args: any[]) => console.log(message, ...args), child: () => logger, withTrace: () => logger,