diff --git a/web/e2e/task-2202-network-unreachable-toast.spec.ts b/web/e2e/task-2202-network-unreachable-toast.spec.ts new file mode 100644 index 000000000..6f7d24389 --- /dev/null +++ b/web/e2e/task-2202-network-unreachable-toast.spec.ts @@ -0,0 +1,47 @@ +import { test, expect } from './fixtures'; +import { browserLogin } from './lib/collab-helpers'; +import { createWorkspace, deleteWorkspace } from './lib/attachment-viewer'; + +/** + * TASK-2202: when the server cannot be reached, the app says so, the error is + * announced assertively, and it stays long enough to read. + * + * The page loads normally; then every API request is aborted the way an + * offline network aborts it (`internetdisconnected`), and the command palette + * fires a search. The search's own UI may handle its failure however it likes; + * the "can't reach the server" toast comes from the API client's seam, so it + * appears whatever the call site does. + */ + +test.describe('network unreachable toast (TASK-2202)', () => { + test.beforeEach(({}, testInfo) => { + test.skip(testInfo.project.name !== 'desktop-chromium', 'one browser is enough'); + }); + + test('an unreachable server raises an assertive error toast that outlasts the old 3s', async ({ page, fixture, request }) => { + const { slug: ws } = await createWorkspace(fixture, request, 'Toast outage'); + try { + await browserLogin(page); + await page.goto(`/${fixture.adminUsername}/${ws}`); + const announcer = page.locator('[data-toast-region="assertive"]'); + await expect(announcer, 'the assertive region exists before any toast').toHaveCount(1); + await expect(announcer).toHaveAttribute('aria-live', 'assertive'); + await expect(announcer.locator('.toast')).toHaveCount(0); + + await page.route('**/api/v1/**', (route) => route.abort('internetdisconnected')); + await page.keyboard.press('ControlOrMeta+k'); + await page.keyboard.type('anything'); + + const toast = page.locator('.toast-error', { hasText: "Can't reach the server" }); + await expect(toast).toBeVisible(); + await expect(announcer).toContainText("Can't reach the server"); + + // Errors linger: still up well past the old 3s default. + await page.waitForTimeout(4000); + await expect(toast).toBeVisible(); + } finally { + await page.unroute('**/api/v1/**'); + await deleteWorkspace(fixture, request, ws); + } + }); +}); diff --git a/web/src/lib/api/client.ts b/web/src/lib/api/client.ts index 65c903048..a75423968 100644 --- a/web/src/lib/api/client.ts +++ b/web/src/lib/api/client.ts @@ -522,6 +522,70 @@ function notifyRateLimited(retryAfterMs?: number): void { } } +type NetworkUnreachableHandler = () => void; + +let networkUnreachableHandler: NetworkUnreachableHandler | null = null; + +/** + * Register a single callback fired when a request cannot reach the server at + * all (TASK-2202): the browser's fetch rejected before any response, which is + * what an offline laptop or a dead route looks like. +layout.svelte wires it to + * a deduped "can't reach the server" toast, the same split as + * setRateLimitHandler, so client.ts stays free of the toast store. + */ +export function setNetworkUnreachableHandler(handler: NetworkUnreachableHandler | null): void { + networkUnreachableHandler = handler; +} + +function notifyNetworkUnreachable(): void { + if (!networkUnreachableHandler) return; + try { + networkUnreachableHandler(); + } catch (err) { + // eslint-disable-next-line no-console + console.warn('network-unreachable handler threw', err); + } +} + +/** + * The error a request throws when the server could not be reached (TASK-2202). + * Its own code, so call sites and the toast can tell it from a refusal the + * server made. A WRITE's message does not claim the change was lost: a fetch + * can fail after the request left (a connection reset mid-response), so, as + * with request_timeout, "not saved" is a claim nobody can make. + */ +export function networkUnreachableError(isIdempotent: boolean): PadApiError { + return new PadApiError({ + code: 'network_unreachable', + message: isIdempotent + ? "Can't reach the server. Check your connection and try again." + : "Can't reach the server, so your change may not have been saved. Check your connection and try again.", + }); +} + +/** + * `fetch` for a direct WRITE that bypasses request() (TASK-2202, codex r1): + * attachment upload and transform, artifact import, checkout. Same rule as + * requestAttempt: an abort keeps its own path (each site's deadline turns its + * timeout into request_timeout), and any other rejection, which happened + * before a response existed, reports the outage and becomes + * network_unreachable instead of a raw TypeError. + */ +async function fetchWrite(input: string, init: RequestInit): Promise { + try { + return await fetch(input, init); + } catch (err) { + if (init.signal?.aborted) throw err; + notifyNetworkUnreachable(); + throw networkUnreachableError(false); + } +} + +/** True for the error a request throws when the server could not be reached. */ +export function isNetworkUnreachable(err: unknown): boolean { + return err instanceof PadApiError && err.code === 'network_unreachable'; +} + /** * Parse a URL path and decide whether a 403 on it indicates that the * caller's READ access to the workspace's item set has been revoked. @@ -904,12 +968,25 @@ async function requestAttempt( issuedAs: ReturnType, deadline: ReturnType, ): Promise { - const resp = await fetch(BASE + path, { - headers, - credentials: 'same-origin', - ...options, - signal: deadline.signal, - }); + let resp: Response; + try { + resp = await fetch(BASE + path, { + headers, + credentials: 'same-origin', + ...options, + signal: deadline.signal, + }); + } catch (err) { + // An abort, the caller's or our deadline's, keeps its own path: + // request() turns a timeout into request_timeout, and a caller's abort + // stays the AbortError it asked for. Anything else rejected before a + // response existed, so the server was not reached (TASK-2202). Only + // the fetch is wrapped: a TypeError from code below is a bug, not an + // outage, and must not be reported as one. + if (deadline.signal.aborted) throw err; + notifyNetworkUnreachable(); + throw networkUnreachableError(isIdempotent); + } // Every response, error statuses included, is a reading of the server's // clock; the sync cursor is stamped from these, never from Date.now() // (BUG-3207). @@ -2984,7 +3061,7 @@ export const api = { if (csrf) headers['X-CSRF-Token'] = csrf; const qs = itemId ? `?item_id=${encodeURIComponent(itemId)}` : ''; - const resp = await fetch(`${BASE}/workspaces/${workspaceSlug}/attachments${qs}`, { + const resp = await fetchWrite(`${BASE}/workspaces/${workspaceSlug}/attachments${qs}`, { method: 'POST', headers, credentials: 'same-origin', @@ -3052,7 +3129,7 @@ export const api = { // created. const deadline = requestDeadline(TRANSFORM_TIMEOUT_MS); try { - const resp = await fetch( + const resp = await fetchWrite( `${BASE}/workspaces/${workspaceSlug}/attachments/${attachmentId}/transform`, { method: 'POST', @@ -3323,7 +3400,7 @@ export const api = { const headers: Record = { 'Content-Type': 'text/markdown' }; const csrf = getCSRFToken(); if (csrf) headers['X-CSRF-Token'] = csrf; - const resp = await fetch(`${BASE}/workspaces/${ws}/import-artifact`, { + const resp = await fetchWrite(`${BASE}/workspaces/${ws}/import-artifact`, { method: 'POST', headers, credentials: 'same-origin', @@ -3427,7 +3504,7 @@ export const api = { // disabled on "Redirecting…" until it settles. const deadline = requestDeadline(requestTimeoutMs); try { - const r = await fetch('/billing/checkout', { + const r = await fetchWrite('/billing/checkout', { method: 'POST', credentials: 'same-origin', headers: { 'Content-Type': 'application/json' }, diff --git a/web/src/lib/api/networkUnreachable.test.ts b/web/src/lib/api/networkUnreachable.test.ts new file mode 100644 index 000000000..35f6e79e6 --- /dev/null +++ b/web/src/lib/api/networkUnreachable.test.ts @@ -0,0 +1,119 @@ +// TASK-2202 — a request that cannot reach the server says so. +// +// A fetch that rejects before any response (offline, a dead route) used to +// surface as a raw TypeError, so every call site showed its generic failure +// text and nothing said the server was unreachable. It is now a PadApiError +// with its own code, and the registered handler fires once per such request. +// Aborts keep their own paths, and an error thrown AFTER a response exists is +// not relabelled. +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { + api, + PadApiError, + isNetworkUnreachable, + setNetworkUnreachableHandler, + setRequestTimeoutForTests +} from './client'; + +let notified = 0; + +beforeEach(() => { + vi.unstubAllGlobals(); + notified = 0; + setNetworkUnreachableHandler(() => { + notified++; + }); +}); +afterEach(() => { + vi.unstubAllGlobals(); + setNetworkUnreachableHandler(null); + setRequestTimeoutForTests(); +}); + +describe('network_unreachable (TASK-2202)', () => { + it('a GET whose fetch rejects is network_unreachable, and the handler fires once', async () => { + vi.stubGlobal('fetch', vi.fn(async () => Promise.reject(new TypeError('Failed to fetch')))); + const err = await api.workspaces.get('ws').catch((e) => e); + expect(err).toBeInstanceOf(PadApiError); + expect(err.code).toBe('network_unreachable'); + expect(isNetworkUnreachable(err)).toBe(true); + expect(err.message).toBe("Can't reach the server. Check your connection and try again."); + expect(notified).toBe(1); + }); + + it("a WRITE's message does not claim the change was lost", async () => { + const fetchMock = vi.fn(async () => Promise.reject(new TypeError('Failed to fetch'))); + vi.stubGlobal('fetch', fetchMock); + const err = await api.workspaces.create({ name: 'x' } as never).catch((e) => e); + expect(err.code).toBe('network_unreachable'); + expect(err.message).toContain('may not have been saved'); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("a caller's abort stays an AbortError and does not report an outage", async () => { + vi.stubGlobal( + 'fetch', + vi.fn( + (_url: string, init?: RequestInit) => + new Promise((_resolve, reject) => { + const signal = init?.signal as AbortSignal; + signal.addEventListener('abort', () => reject(signal.reason), { once: true }); + }) + ) + ); + const ctl = new AbortController(); + const p = api.items.copyPreflight('ws', 'item', {} as never, { signal: ctl.signal }).catch((e) => e); + ctl.abort(); + const err = await p; + expect(err).not.toBeInstanceOf(PadApiError); + expect((err as DOMException).name).toBe('AbortError'); + expect(notified).toBe(0); + }); + + it('a timeout stays request_timeout and does not report an outage', async () => { + setRequestTimeoutForTests(30); + vi.stubGlobal( + 'fetch', + vi.fn( + (_url: string, init?: RequestInit) => + new Promise((_resolve, reject) => { + const signal = init?.signal as AbortSignal; + signal.addEventListener('abort', () => reject(new TypeError('Failed to fetch')), { once: true }); + }) + ) + ); + const err = await api.workspaces.get('ws').catch((e) => e); + expect(err.code).toBe('request_timeout'); + expect(notified).toBe(0); + }); + + it('the direct-fetch WRITES (attachment upload, artifact import) report it too (codex r1)', async () => { + vi.stubGlobal('fetch', vi.fn(async () => Promise.reject(new TypeError('Failed to fetch')))); + const upload = await api.attachments.upload('ws', new Blob(['x'])).catch((e) => e); + expect(upload.code).toBe('network_unreachable'); + expect(upload.message).toContain('may not have been saved'); + const imported = await api.importArtifact('ws', '# body').catch((e) => e); + expect(imported.code).toBe('network_unreachable'); + expect(notified).toBe(2); + }); + + it('a TypeError after a response arrived is not relabelled as an outage', async () => { + vi.stubGlobal( + 'fetch', + vi.fn( + async () => + ({ + status: 200, + ok: true, + headers: new Headers(), + json: async () => { + throw new TypeError('body went wrong'); + } + }) as unknown as Response + ) + ); + const err = await api.workspaces.get('ws').catch((e) => e); + expect(isNetworkUnreachable(err)).toBe(false); + expect(notified).toBe(0); + }); +}); diff --git a/web/src/lib/api/networkUnreachableToast.test.ts b/web/src/lib/api/networkUnreachableToast.test.ts new file mode 100644 index 000000000..69ea2641c --- /dev/null +++ b/web/src/lib/api/networkUnreachableToast.test.ts @@ -0,0 +1,38 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +// Mock the rune-based toast store so this stays a node test, like the busy +// toast's test. +vi.mock('$lib/stores/toast.svelte', () => ({ + toastStore: { show: vi.fn() } +})); + +import { toastStore } from '$lib/stores/toast.svelte'; +import { + notifyNetworkUnreachable, + NETWORK_UNREACHABLE_MESSAGE, + __resetNetworkUnreachableToastForTest +} from './networkUnreachableToast'; + +const showMock = toastStore.show as ReturnType; + +describe('notifyNetworkUnreachable (TASK-2202)', () => { + beforeEach(() => { + showMock.mockClear(); + __resetNetworkUnreachableToastForTest(); + }); + + it('shows one ERROR toast with the unreachable message (an error, so it lingers and is assertive)', () => { + expect(notifyNetworkUnreachable(0)).toBe(true); + expect(showMock).toHaveBeenCalledTimes(1); + expect(showMock).toHaveBeenCalledWith(NETWORK_UNREACHABLE_MESSAGE, 'error'); + }); + + it('an outage burst shows one toast, and a later outage shows another', () => { + notifyNetworkUnreachable(0); + notifyNetworkUnreachable(100); + notifyNetworkUnreachable(14_999); + expect(showMock).toHaveBeenCalledTimes(1); + expect(notifyNetworkUnreachable(15_000)).toBe(true); + expect(showMock).toHaveBeenCalledTimes(2); + }); +}); diff --git a/web/src/lib/api/networkUnreachableToast.ts b/web/src/lib/api/networkUnreachableToast.ts new file mode 100644 index 000000000..1ffa25e0a --- /dev/null +++ b/web/src/lib/api/networkUnreachableToast.ts @@ -0,0 +1,39 @@ +import { toastStore } from '$lib/stores/toast.svelte'; + +/** + * "Can't reach the server" toast for requests whose fetch rejected before any + * response (TASK-2202). The API client fires `setNetworkUnreachableHandler` + * once per such request; +layout.svelte wires that seam here, the same split + * as serverBusyToast, so client.ts stays free of the toast store. + * + * It exists because most call sites show a fixed string ("Failed to save") + * whatever went wrong, so an outage and a validation refusal looked the same. + * This says WHY, once, beside whatever the call site shows. + * + * Deduped: an outage fails every in-flight request at once, and a stack of + * identical toasts helps nobody. At most one inside DEDUPE_WINDOW_MS. + */ + +const DEDUPE_WINDOW_MS = 15_000; + +export const NETWORK_UNREACHABLE_MESSAGE = + "Can't reach the server. Check your connection; changes made now may not be saved."; + +let lastShownAt = Number.NEGATIVE_INFINITY; + +/** + * Show the deduped error toast. An error, so it is announced assertively and + * lingers (toast store defaults). Returns whether a toast was shown; `now` is + * injectable for tests. + */ +export function notifyNetworkUnreachable(now: number = Date.now()): boolean { + if (now - lastShownAt < DEDUPE_WINDOW_MS) return false; + lastShownAt = now; + toastStore.show(NETWORK_UNREACHABLE_MESSAGE, 'error'); + return true; +} + +/** Test-only: reset the dedupe clock. */ +export function __resetNetworkUnreachableToastForTest(): void { + lastShownAt = Number.NEGATIVE_INFINITY; +} diff --git a/web/src/lib/components/common/ToastContainer.svelte b/web/src/lib/components/common/ToastContainer.svelte index 76b596d18..d0198069b 100644 --- a/web/src/lib/components/common/ToastContainer.svelte +++ b/web/src/lib/components/common/ToastContainer.svelte @@ -18,39 +18,84 @@ goto(toast.link); } } + + // ANNOUNCEMENT (TASK-2202). The stacks below ARE the live regions, and + // both are mounted for the page's whole life, empty until a toast arrives: + // screen readers announce a node added to a region that already exists, + // and miss one that arrives together with its region. The single stack + // used to be wrapped in {#if toasts.length}, so a toast after an idle + // spell mounted with its region and was usually never read. + // + // Errors go in an assertive region (they interrupt), the rest in a polite + // one. aria-atomic is left false, so only the added toast is read, not the + // whole stack. Each toast is its own keyed node, so the same text twice is + // still a new node and is read again. The text lives in the toast itself, + // once: no hidden copy for a test or a screen reader to find twice. + const errors = $derived(toastStore.toasts.filter((t) => t.type === 'error')); + const others = $derived(toastStore.toasts.filter((t) => t.type !== 'error')); + + // HOLD (TASK-2202): a toast stays while the pointer is over it or focus is + // inside it. The two are separate holds, so leaving with the mouse while a + // button inside still has focus keeps it up. + function holdOnFocusOut(e: FocusEvent, id: string) { + const next = e.relatedTarget as Node | null; + if (next && (e.currentTarget as HTMLElement).contains(next)) return; + toastStore.resume(id); + } + function holdOnFocusIn(e: FocusEvent, id: string) { + const prev = e.relatedTarget as Node | null; + if (prev && (e.currentTarget as HTMLElement).contains(prev)) return; + toastStore.pause(id); + } -{#if toastStore.toasts.length > 0} -
- {#each toastStore.toasts as toast (toast.id)} - -
handleClick(toast)} - role={toast.link ? 'button' : undefined} - tabindex={toast.link ? 0 : -1} - onkeydown={(e) => { if (toast.link && e.key === 'Enter') handleClick(toast); }} - > - {iconForType(toast.type)} - {toast.message}{#if toast.link} →{/if} - {#if toast.action} - - {/if} - -
- {/each} +{#snippet toastItem(toast: Toast)} + +
handleClick(toast)} + onmouseenter={() => toastStore.pause(toast.id)} + onmouseleave={() => toastStore.resume(toast.id)} + onfocusin={(e) => holdOnFocusIn(e, toast.id)} + onfocusout={(e) => holdOnFocusOut(e, toast.id)} + role={toast.link ? 'button' : undefined} + tabindex={toast.link ? 0 : -1} + onkeydown={(e) => { + if (!toast.link || e.target !== e.currentTarget) return; + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleClick(toast); + } + }} + > + {iconForType(toast.type)} + {toast.message}{#if toast.link} →{/if} + {#if toast.action} + + {/if} +
-{/if} +{/snippet} + +
+
+ {#each errors as toast (toast.id)}{@render toastItem(toast)}{/each} +
+
+ {#each others as toast (toast.id)}{@render toastItem(toast)}{/each} +
+