From 3478763c7d0dc412635a2fd5589c5202bac59a60 Mon Sep 17 00:00:00 2001 From: G-ELM Date: Sun, 30 Aug 2026 23:47:55 +0100 Subject: [PATCH 1/4] docs(contracts): add contract testing guide Documents the standard Soroban test scaffolding, the three levels of auth mocking and the risk of blanket mock_all_auths() hiding a missing require_auth(), how to test expiry via the virtual ledger clock, and the proposals -> group_treasury cross-contract test setup. --- contracts/docs/testing.md | 312 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 312 insertions(+) create mode 100644 contracts/docs/testing.md diff --git a/contracts/docs/testing.md b/contracts/docs/testing.md new file mode 100644 index 0000000..21bb443 --- /dev/null +++ b/contracts/docs/testing.md @@ -0,0 +1,312 @@ +# Contract testing guide + +How the Soroban test environment is used across the three contracts in this workspace +(`token_transfer`, `group_treasury`, `proposals`): the standard scaffolding, how +authorization is tested (and how to avoid hiding a missing `require_auth`), how +time-dependent behaviour like proposal expiry is exercised, and how cross-contract tests +wire `proposals` into `group_treasury`. + +All examples below are taken from the real test suites in this workspace — see +`contracts/contracts/*/src/test.rs`. + +--- + +## Standard test scaffolding + +Every contract test module is gated with `#![cfg(test)]` and starts from a fresh +`Env::default()`. There is no shared/global environment between tests — each `#[test]` +function builds its own. + +```rust +#![cfg(test)] + +use super::*; +use soroban_sdk::testutils::Address as _; +use soroban_sdk::testutils::Ledger; +use soroban_sdk::Env; + +#[test] +fn create_then_vote_then_pass_then_execute_happy_path() { + let env = Env::default(); + let (client, _admin, alice, bob, carol, ..) = setup(&env); + + let id = create_proposal_in(&env, &client, &alice, 1_000, /* ... */); + + client.vote(&alice, &id, &true); + client.vote(&bob, &id, &true); + client.vote(&carol, &id, &false); + + advance_time(&env, 1_001); + let status = client.finalize_proposal(&id); + assert_eq!(status, ProposalStatus::Passed); +} +``` + +### Address generation + +Test addresses are never hardcoded strings — they are generated per-test with +`Address::generate(&env)`, which requires `soroban_sdk::testutils::Address` to be in scope +(imported as `Address as _` since only the trait method is used, not the type name): + +```rust +use soroban_sdk::testutils::Address as _; + +let alice = Address::generate(&env); +let bob = Address::generate(&env); +``` + +Each call produces a fresh, unique address scoped to that `env`. Addresses are never reused +across tests. + +### The `setup()` helper + +Each contract's test module defines a `setup(&env)` helper that registers the contract(s), +generates the standard cast of addresses (an admin plus a small number of named actors — +`alice`, `bob`, `carol` are the convention here), and returns a tuple of the client plus +every address the test body will need. Keeping this in one function means every test starts +from the same known-good state and individual tests stay short. + +Where a contract needs a token to move, `setup()` also registers a minimal `mock_token` +module scoped to the test file (`#[contract] struct MockToken` with `mint`/`transfer`/ +`balance`) rather than depending on a real token contract. This keeps tests fast and +self-contained, and lets tests that only care about authorization use a token whose +`transfer` still calls real `require_auth()`. + +--- + +## Testing authorization + +Soroban's test host does not simulate real signatures. Instead, `Env` exposes mocking +utilities that make `require_auth()` calls succeed (or fail) without a signature ever being +produced. Three levels are used across this workspace, in increasing order of precision: + +### 1. `mock_all_auths()` — blanket mocking + +```rust +let env = Env::default(); +env.mock_all_auths(); +``` + +This makes **every** `require_auth()` / `require_auth_for_args()` call in the transaction +succeed, regardless of which address it was called on. It is the fastest way to get a +happy-path test running, and it is what most tests in this workspace use. + +**The risk:** `mock_all_auths()` proves nothing about *which* address is required to +authorize a call — only that *some* auth check, if present, would pass. A contract method +that is missing a `require_auth()` call entirely will pass a `mock_all_auths()`-based test +just as easily as a correct one. Blanket mocking is safe for tests whose purpose is +something other than authorization (e.g. asserting vote tallying, or balance arithmetic), +but it must never be the *only* test covering a security-sensitive entry point. + +### 2. `mock_all_auths_allowing_non_root_auth()` — cross-contract calls + +`proposals::execute_withdraw` calls `group_treasury::withdraw`, which itself calls +`admin.require_auth()` as a **nested** (non-root) invocation — the admin address never +appears in the root call's argument list. The default `mock_all_auths()` only mocks +auth for the root invocation, so cross-contract setups that need a nested `require_auth` +to succeed use the non-root variant instead: + +```rust +// execute_withdraw calls treasury.withdraw() as a nested (non-root) call, which +// itself calls admin.require_auth() — an address not present in the root +// invocation's argument list, so the non-root variant is required. +env.mock_all_auths_allowing_non_root_auth(); +``` + +### 3. `env.auths()` and `mock_auths()` — asserting the real requirement + +To actually prove that a specific address's authorization is required — not just that +*an* auth check exists — combine one of two techniques: + +**Assert which address was required**, after a successful call under blanket mocking: + +```rust +env.mock_all_auths(); +client.transfer(&sender, &receiver, &100, &memo); + +let auths = env.auths(); +assert!(auths.iter().any(|(addr, _)| *addr == sender)); +``` + +**Mock auth for one address only**, and confirm the call panics when the address that +should be required did not authorize: + +```rust +#[test] +#[should_panic] +fn test_upgrade_non_admin_panics() { + let env = Env::default(); + let admin = Address::generate(&env); + let contract_id = setup_with_admin(&env, &admin); + let client = TokenTransferContractClient::new(&env, &contract_id); + + // Only mock auth for a non-admin address. The contract must panic when + // require_auth() is invoked on `admin`, because `admin` never signed. + let intruder = Address::generate(&env); + env.mock_auths(&[soroban_sdk::testutils::MockAuth { + address: &intruder, + invoke: &soroban_sdk::testutils::MockAuthInvoke { + contract: &contract_id, + fn_name: "upgrade", + args: soroban_sdk::vec![&env], + sub_invokes: &[], + }, + }]); + + client.upgrade(&wasm_hash); // panics: admin never authorized +} +``` + +**Test with no mocking at all**, when the point of the test is that the call must fail +without authorization: + +```rust +#[test] +#[should_panic] +fn test_vote_without_auth_panics() { + let env = Env::default(); + let admin = Address::generate(&env); + let member = Address::generate(&env); + // ... register + initialize the contract ... + + env.mock_all_auths(); + client.add_member(&member); + // ... + env.set_auths(&[]); // clear mocked auths — the vote must now fail + + client.approve_withdraw(&member, &0); // panics: no auth present +} +``` + +`env.set_auths(&[])` is useful when setup steps genuinely need mocked auth (e.g. +initializing a contract as admin) but the assertion under test needs a clean slate with +none mocked. + +**Rule of thumb:** every entry point that calls `require_auth()` should have at least one +test that exercises the real requirement — via `env.auths()`, a scoped `mock_auths()`, or +an unmocked `should_panic` test — in addition to any `mock_all_auths()`-based happy-path +tests. A codebase with only blanket-mocked tests can silently lose a `require_auth()` call +in a refactor and no test will catch it. + +--- + +## Testing time-dependent behaviour (expiry) + +Soroban's test `Env` uses a virtual ledger clock (`env.ledger()`), which starts at a fixed +timestamp and only advances when a test explicitly moves it forward. This makes +expiry-style logic deterministic to test: no real waiting, no flaky timers. + +The convention in this workspace is a small `advance_time` helper: + +```rust +use soroban_sdk::testutils::Ledger; + +fn advance_time(env: &Env, seconds: u64) { + env.ledger() + .set_timestamp(env.ledger().timestamp() + seconds); +} +``` + +Usage — a proposal created with a 500-second voting window cannot be finalized before +expiry, and must be finalizable once the clock has passed it: + +```rust +#[test] +#[should_panic(expected = "cannot finalize before expiry")] +fn finalize_before_expiry_panics() { + let env = Env::default(); + let (client, .., alice, .., m, token_id) = setup(&env); + let id = create_proposal_in(&env, &client, &alice, 1_000, &m, &token_id, &alice, 1); + + client.finalize_proposal(&id); // no advance_time — still active +} + +#[test] +fn finalize_expired_success() { + let env = Env::default(); + let (client, .., alice, .., m, token_id) = setup(&env); + let id = create_proposal_in(&env, &client, &alice, 500, &m, &token_id, &alice, 1); + + advance_time(&env, 501); // past the 500s window + client.finalize_expired_proposal(&id); + + assert_eq!(client.get_proposal(&id).status, ProposalStatus::Expired); +} +``` + +Both the "too early" boundary (`should_panic`, no time advance) and the "just past expiry" +boundary (advance exactly past the window) should be covered for any time-gated state +transition — off-by-one errors in expiry math are easy to introduce and easy to catch this +way. + +--- + +## Cross-contract test setup: `proposals` calling into `group_treasury` + +`proposals::execute_withdraw` invokes `group_treasury::withdraw` as a real cross-contract +call inside the test host — not a mock of the treasury contract. The `proposals` test +`setup()` registers both contracts in the same `Env` and wires the treasury's address into +the proposal so the nested call resolves correctly: + +```rust +fn setup(env: &Env) -> ( /* ... */ ) { + // Nested require_auth() inside execute_withdraw needs the non-root variant. + env.mock_all_auths_allowing_non_root_auth(); + + let proposals_id = env.register(ProposalsContract, ()); + let proposals = ProposalsContractClient::new(env, &proposals_id); + proposals.initialize(&proposals_admin); + + let token_id = env.register(mock_token::MockToken, ()); + let token = MockTokenClient::new(env, &token_id); + token.mint(&treasury_member, &1_000_000); + + let treasury_addr = env.register(group_treasury::GroupTreasuryContract, ()); + let treasury = group_treasury::GroupTreasuryContractClient::new(env, &treasury_addr); + treasury.initialize(&treasury_admin, &token_id, &1); + treasury.add_member(&treasury_member); + treasury.deposit(&treasury_member, &token_id, &500); + + (proposals, proposals_admin, alice, bob, carol, treasury, treasury_admin, treasury_member, token_id) +} +``` + +Points worth keeping in mind when writing a test like this: + +- The `proposals` crate depends on `group_treasury` as a regular Rust dependency (see + `contracts/contracts/proposals/Cargo.toml` and `treasury_interface_client.rs`), so the + treasury client type is available directly — no WASM re-import step is needed inside + tests. +- Both contracts are registered against the **same** `env`, which is what makes the nested + invocation and its nested `require_auth()` resolvable at all. + `mock_all_auths_allowing_non_root_auth()` (not plain `mock_all_auths()`) is required for + exactly this reason — see [Testing authorization](#testing-authorization) above. + Registering the deposit is done through the treasury client's own `deposit()` method + (which itself calls the mock token's `transfer`) so the treasury has a real balance for + `execute_withdraw` to draw down, rather than writing to treasury storage directly. +- Assertions after `execute_withdraw` read state back through the *treasury's* client + (`treasury.balance(&token_id)`), confirming the effect actually crossed the contract + boundary rather than only checking the proposal's own status flipped to `Executed`. + +--- + +## The `test_snapshots/` directory + +Running `cargo test` against a Soroban contract writes ledger/state snapshot files under +each contract's `test_snapshots/` directory (e.g. +`contracts/contracts/proposals/test_snapshots/`). These capture the test host's storage +state at the end of each test run and are how Soroban's test harness detects unexpected +storage-footprint changes between runs. + +- **Generated, not hand-written.** Never edit files under `test_snapshots/` by hand — they + are regenerated automatically the next time the corresponding test runs. +- **Committed.** Despite being generated, these files are checked into the repository. They + are part of what makes a contract's storage footprint reviewable in a diff — a PR that + unexpectedly grows a snapshot is a signal worth looking at during review. +- **Prettier-ignored.** The root `.prettierignore` excludes `contracts/**/test_snapshots/` + under the "generated artifacts" section, alongside `apps/backend/drizzle/meta/` and + `**/*.d.ts`. This is intentional: reformatting a generated, machine-written file produces + noisy diffs and gets overwritten on the next test run regardless. + +If a snapshot diff shows up in a PR that didn't intentionally change contract storage +layout, treat it as a signal to re-check the change rather than committing it as noise. From cd47b5b5108742d508baa34c462e90bcae0714f3 Mon Sep 17 00:00:00 2001 From: G-ELM Date: Sun, 30 Aug 2026 23:48:44 +0100 Subject: [PATCH 2/4] docs(web): add service worker and offline behaviour guide Documents sw.js registration/update lifecycle via skipWaiting + clients.claim, the content-free push handler, click-to-route focusing an existing tab instead of opening a duplicate, and what does and does not work offline today (no precaching, no fetch interception, no background sync). --- apps/web/docs/concepts-service-worker.md | 202 +++++++++++++++++++++++ 1 file changed, 202 insertions(+) create mode 100644 apps/web/docs/concepts-service-worker.md diff --git a/apps/web/docs/concepts-service-worker.md b/apps/web/docs/concepts-service-worker.md new file mode 100644 index 0000000..10924ef --- /dev/null +++ b/apps/web/docs/concepts-service-worker.md @@ -0,0 +1,202 @@ +# Service worker and offline behaviour + +This document covers `public/sw.js`: how it is registered and updated, what it does with +push events, how it routes notification clicks back into the app, and — plainly — what +does and does not work when the device is offline. + +For the push *subscription* flow (permission prompt, VAPID keys, backend registration), +see [Push notification subscription flow](concepts-push-subscription.md). This document +focuses on the service worker itself. + +--- + +## Registration lifecycle + +The service worker is registered by `hooks/usePushSubscription.ts`, not by Next.js or a +PWA plugin — there is no automatic registration on every page load. Registration happens +on mount of whatever component calls `usePushSubscription`: + +```ts +navigator.serviceWorker.register('/sw.js', { scope: '/' }); +``` + +Scope `/` means the worker controls every page on the origin, not just the page that +registered it. + +### Install and activate + +```js +self.addEventListener('install', () => { + self.skipWaiting(); +}); + +self.addEventListener('activate', (event) => { + event.waitUntil(self.clients.claim()); +}); +``` + +- **`install` → `skipWaiting()`** — By default, a newly installed service worker sits in a + "waiting" state until every open tab controlled by the old worker is closed. Calling + `skipWaiting()` during install skips that wait: the new worker activates as soon as it + finishes installing, even while old tabs are still open. +- **`activate` → `clients.claim()`** — By default, an activated worker only controls pages + loaded *after* activation. `clients.claim()` takes control of every already-open, + uncontrolled page on the origin immediately. + +### The update path — how a stale worker is replaced + +Together, `skipWaiting()` + `clients.claim()` mean this service worker updates itself +eagerly rather than waiting for a natural refresh cycle: + +1. The browser fetches `/sw.js` on navigation/periodically and byte-compares it against the + currently installed worker. +2. If it differs, the browser installs the new version in the background. `install` fires, + `skipWaiting()` runs immediately, and the new worker moves straight to "activating" + instead of "waiting". +3. `activate` fires, `clients.claim()` runs, and the new worker takes over **every open + tab** on the origin — including ones that were loaded under the old worker and never + reloaded. + +The practical effect: there is no user-visible "a new version is available, refresh to +update" step for the service worker itself. The trade-off is that a page whose in-memory +JS was loaded under the old worker can, in principle, keep running while a different tab +is now served by the new worker — the two do not roll forward in lockstep mid-session. +Since this worker carries no versioned caching logic (see [Offline behaviour](#offline-behaviour-today) +below), this has not been a practical problem: there is no cached asset list that could +go stale between the two workers. + +--- + +## The push handler + +```js +self.addEventListener('push', (event) => { + let data = {}; + try { + data = event.data ? event.data.json() : {}; + } catch { + // malformed payload — show a generic notification + } + + const conversationId = data.conversationId ?? null; + + event.waitUntil( + self.registration.showNotification('Clicked', { + body: 'You have a new message', + icon: '/icons/icon-192.png', + badge: '/icons/badge-96.png', + tag: conversationId ? `conv-${conversationId}` : 'new-message', + renotify: true, + data: { conversationId }, + }), + ); +}); +``` + +### Content-free by design + +The notification title is always the literal string `Clicked`, and the body is always the +literal string `You have a new message`. The only thing extracted from the push payload is +an optional `conversationId`, used purely for click-routing (see below) — it is never +rendered into the notification UI. A malformed or unparsable payload is caught and still +produces a generic notification rather than failing silently. + +This is the client-side half of a deliberate privacy design: the backend never puts message +content in a push payload in the first place. See +[`apps/backend/docs/api-push.md`](../../backend/docs/api-push.md) for how the backend +constructs push payloads and why content is excluded server-side. The two halves only work +together — a service worker that refuses to render content is not a substitute for a +backend that never sends it, and vice versa. + +`tag: conv-` (or `new-message` when there is no conversation ID) lets a +second push for the same conversation replace the existing OS notification instead of +stacking a duplicate; `renotify: true` re-alerts the user when that replacement happens. + +--- + +## Notification click routing + +```js +self.addEventListener('notificationclick', (event) => { + event.notification.close(); + + const conversationId = event.notification.data?.conversationId ?? null; + const target = conversationId + ? `/app/conversations/${conversationId}` + : '/app/messages'; + + event.waitUntil( + self.clients + .matchAll({ type: 'window', includeUncontrolled: true }) + .then((windowClients) => { + for (const client of windowClients) { + if (new URL(client.url).origin === self.location.origin) { + client.postMessage({ type: 'sw:sync', conversationId }); + client.focus(); + return; + } + } + return self.clients.openWindow(target); + }), + ); +}); +``` + +1. The clicked notification is closed immediately. +2. The worker looks for any open window on the app's origin via + `clients.matchAll({ type: 'window', includeUncontrolled: true })`. +3. **If one is found**, the worker does **not** navigate it directly — `postMessage` and + `navigate()` on a `WindowClient` both require the client to be controlled, and an + `includeUncontrolled` match may not be. Instead it posts `{ type: 'sw:sync', + conversationId }` to the page and calls `client.focus()` to bring the existing tab to + the front. The app's own layout listens for this message client-side: + + ```ts + // apps/web/src/app/app/layout.tsx + navigator.serviceWorker.addEventListener('message', (event) => { + if (event.data?.type !== 'sw:sync') return; + const { conversationId } = event.data; + router.push(conversationId ? `/app/conversations/${conversationId}` : '/app/messages'); + }); + ``` + + The Next.js router then performs the actual client-side navigation. This is the + "focus an existing tab rather than opening a duplicate" behaviour — a user with the app + already open in one tab never ends up with a second tab per notification. +4. **If no window is found**, the worker calls `self.clients.openWindow(target)`, opening a + fresh tab directly at `/app/conversations/` (or `/app/messages` with no ID) — there + is no existing page to `postMessage` into, so the destination URL does the routing work + instead. + +--- + +## Offline behaviour today + +This service worker registers no `fetch` handler and maintains no `caches` entries. It +exists solely for push notifications (`push`, `notificationclick`) and its own lifecycle +(`install`, `activate`). Concretely: + +**Works offline:** + +- Previously loaded pages that are still in the browser's own HTTP cache or Next.js's + client-side router cache may continue to render from memory until a hard navigation is + needed. +- Already-decrypted messages held in IndexedDB (see + [IndexedDB schemas](contracts-indexeddb-schemas.md)) remain readable if a view reads from + local storage rather than the network. +- Push notifications continue to be received and shown while offline is not the relevant + condition here — push delivery is the browser vendor's push service waking the service + worker, independent of whether the tab is open. + +**Does not work offline:** + +- No page, route, or static asset is precached — a hard reload or first navigation to an + unvisited route while offline fails exactly as it would with no service worker at all. +- No API requests are intercepted or served from a cache; `fetch` calls to the REST API and + the WebSocket connection simply fail offline (see + [Frontend error handling and user feedback](concepts-error-handling.md) for how those + failures surface to the user). +- There is no background sync / outbox queue — a message typed while offline is not queued + by the service worker for later delivery. +- Nothing is precached ahead of going offline, so there is no "app shell" guarantee — this + is not a full offline-first PWA today. From 75397a63cd21876529f4f7ebd581101cea38d9b8 Mon Sep 17 00:00:00 2001 From: G-ELM Date: Sun, 30 Aug 2026 23:51:54 +0100 Subject: [PATCH 3/4] docs(web): add frontend error handling and user feedback guide Documents the toast API vs inline error state, maps backend error responses (REST schema error format, per-endpoint docs) onto user-facing messages, states the hard rule that decryption failures render as UnavailableMessagePlaceholder rather than a generic crash, and covers current send-failure/offline behaviour including known gaps (silent socket errors, no send retry or offline queue). --- apps/web/docs/concepts-error-handling.md | 229 +++++++++++++++++++++++ 1 file changed, 229 insertions(+) create mode 100644 apps/web/docs/concepts-error-handling.md diff --git a/apps/web/docs/concepts-error-handling.md b/apps/web/docs/concepts-error-handling.md new file mode 100644 index 0000000..26ef283 --- /dev/null +++ b/apps/web/docs/concepts-error-handling.md @@ -0,0 +1,229 @@ +# Frontend error handling and user feedback + +How failures reach the user in `apps/web`: REST API errors, WebSocket/socket errors, +wallet rejections, and decryption failures. Covers the toast system +(`lib/useToast.ts`), when a route uses inline error state instead, the socket `error` +event, and what happens to a message when sending it ultimately fails. + +There is **no global error interceptor** in this app — no fetch middleware, no error +boundary that turns every failure into the same UI. Each call site decides how to surface +its own failure. This document describes the two patterns in use, when to reach for each, +and the one case (decryption) where the presentation is a hard rule rather than a +per-call-site choice. + +--- + +## The toast API (`lib/useToast.ts`) + +```ts +const { notify, success, error, info, dismiss } = useToast(); + +success('Withdrawal proposal submitted successfully'); +error(body.error ?? 'Failed to submit proposal'); +info('Reconnecting…'); +``` + +`useToast()` reads a `ToastContext` provided by `` (mounted once in +`app/layout.tsx`, above the whole app). Calling it outside the provider throws +immediately — this is intentional, so a missing provider fails loudly in development +rather than showing nothing in production. + +Toasts: + +- Render bottom-right, stack, and auto-dismiss after 4 seconds (`ToastProvider.tsx`), or + can be dismissed early by the user or by calling `dismiss(id)`. +- Have three variants — `success`, `error`, `info` — each with its own colour and icon. +- Are **transient and global**. They are the right choice for the result of a discrete + action the user just took (submit a proposal, save a setting, a network error on submit) + where the relevant context (a modal, a form) may already be gone by the time the result + is known. + +### When to use a toast vs. inline error state + +| Use a toast when… | Use inline error state when… | +| --- | --- | +| The action is a one-off submission (form, modal, button click) and success/failure is transient feedback. | The error is about a specific field or a specific piece of persistent UI (a route, a panel) that stays on screen. | +| The user can immediately retry the same action from where they are. | The user needs to see *why*, next to *what*, for more than 4 seconds — e.g. a validation message under an input. | +| Example: `ProposeWithdrawalModal` toasts `'Network error — please try again'` on a failed `POST /treasury/propose`, while validation errors (bad recipient address) are set as component state and rendered under the field, not toasted. | Example: a route that fails to load its primary data (a conversation, a proposal list) renders an inline empty/error state in place of the content, since a toast would disappear while the broken screen remains. | + +Both can apply to the same failure at different layers: a form's *field-level* validation +error is inline, while the same form's *submit* failure (a rejected `fetch`) is a toast. +See `components/treasury/ProposeWithdrawalModal.tsx` for both patterns side by side in one +component. + +Toast adoption is not yet universal — some call sites currently only `console.error` a +failure with no user-visible feedback at all (see [Socket errors](#socket-errors) and +[Send failures](#send-failures-and-what-happens-to-the-message) below). Treat "toast or +inline state, chosen deliberately" as the standard for new code, not the current state of +every existing call site. + +--- + +## Mapping backend errors to user-facing messages + +There is no global response interceptor, so every call site is responsible for checking +`response.ok` and reading the backend's error body itself: + +```ts +const res = await apiFetch('/treasury/propose', { method: 'POST', body: /* ... */ }); + +if (!res.ok) { + const body = (await res.json().catch(() => ({}))) as { error?: string }; + toastError(body.error ?? 'Failed to submit proposal'); + return; +} +``` + +The backend's validation failures follow one consistent shape — +`{ error: string, issues?: [{ field, message }] }` at `400 Bad Request` — documented in +[REST schemas — Error Response Format](../../backend/docs/contracts-rest-schemas.md#error-response-format). +When a route surfaces a validation failure to the user, prefer showing `issues[].message` +next to the relevant field (inline) over the top-level `error` string in a toast, since the +top-level string is often generic (`"Validation failed"`) while `issues` names the actual +problem. + +For endpoint-specific error conditions (expired sessions, ownership checks, rate limits), +consult the relevant backend API doc — e.g. +[Auth API](../../backend/docs/api-auth.md), +[Devices and prekeys API](../../backend/docs/api-devices.md) — for the exact status codes +and error strings a given route can return, so the frontend message matches what actually +happened rather than a generic fallback. + +### 401 / expired session + +`lib/api.ts` does not intercept 401s globally (see +[REST client — 401 / expired-token handling](api-rest-client.md)). A 401 on any given call +surfaces through that call's own `!res.ok` branch like any other error; there is currently +no app-wide "your session expired, please sign in again" redirect triggered centrally — +each screen's own error handling is what the user sees. + +### Wallet rejections + +Wallet interactions (`connect()`, transaction signing) reject their promise when the user +declines in the wallet UI. The current pattern: + +```ts +try { + await connect(); +} catch (err) { + console.error('Wallet connection failed:', err); +} finally { + setIsConnecting(false); +} +``` + +A rejected wallet connection currently resets loading state but does not show the user a +toast or inline message distinguishing "you declined" from "the wallet extension isn't +installed" from "the connection timed out" — all three collapse to a silent no-op from the +user's point of view. This is a gap worth closing with a toast (`toastError('Connection +request was declined')` or similar) rather than a pattern to copy into new wallet-driven +flows. + +--- + +## Socket errors + +`lib/socket.ts` registers a generic handler on the shared socket: + +```ts +socket.on('error', (error) => console.error('Socket error:', error)); +``` + +This is diagnostic only — a socket-level error (auth rejected on connect, a malformed +server event) is logged to the console and otherwise invisible to the user. Reconnection +itself is handled separately by `socket.io-client`'s built-in `reconnection: true` option, +so transient network drops recover without user action; what is *not* currently surfaced is +a persistent failure (e.g. repeated reconnection failures) as a toast or a "reconnecting…" +indicator. If you are building UI that depends on socket delivery (typing indicators, +presence, live message arrival), do not assume the user will see any signal if the socket +is silently failing — treat this as a known gap rather than an existing pattern to rely on. + +--- + +## Decryption failures — never a generic crash + +**Rule:** a message that fails to decrypt, fails verification, or arrives before the local +session exists must never be rendered as a thrown error, a broken row, or a generic "something +went wrong" state. It always renders as `UnavailableMessagePlaceholder`, a calm, specific, +in-flow placeholder — never a toast, never an error boundary. + +```ts +export type UnavailableReason = 'pre-link' | 'undecryptable' | 'verification-failed'; +``` + +```tsx +// components/messaging/UnavailableMessagePlaceholder.tsx +const REASON_COPY: Record = { + 'pre-link': 'Waiting for secure session — message from before this device was linked.', + undecryptable: 'Unable to decrypt this message.', + 'verification-failed': 'Message could not be verified.', +}; + +
+ 🔒 {REASON_COPY[reason]} +
+``` + +Why this is a hard rule rather than a style preference: + +- **It is expected, not exceptional.** `pre-link` in particular happens routinely — any + message sent before the current device completed session setup is legitimately + undecryptable on this device by design, not a bug. Presenting it as an alarming failure + would train users to distrust normal E2EE behaviour. +- **It is isolated.** Crashing that row (or the whole thread) on one bad message would + take down an otherwise healthy conversation. +- **It is inline, in the message's position in the thread** (`InboundMessageRow` renders + it in place of the decrypted bubble), so the conversation's shape and ordering are + preserved — the user sees *that* a message exists and roughly when, just not its content. + +When adding a new code path that can fail to decrypt or verify a message, route it through +`UnavailableReason` / `UnavailableMessagePlaceholder` rather than letting the failure +propagate as a thrown exception into the render tree. + +--- + +## Retry and offline behaviour + +### Send failures and what happens to the message + +Sending an encrypted text message (`handleSendEncrypted` in +`app/app/conversations/[id]/page.tsx`) is not currently optimistic in the UI-state sense — +the composer's text is only cleared (`setSendText('')`) **after** the encrypt-and-send +call succeeds: + +```ts +async function handleSendEncrypted() { + if (!sendText.trim() || !socket || !token) return; + setSending(true); + try { + await sendEncryptedMessage({ /* ... */ }); + socket.emit('send_message', { /* ... */ }); + setSendText(''); + } catch (err) { + console.error('Failed to send encrypted message:', err); + } finally { + setSending(false); + } +} +``` + +Practically, this means: + +- **On failure**, the typed text is preserved in the composer (never cleared), so nothing + the user wrote is lost — but there is currently no toast or inline indicator telling the + user the send failed at all; the only signal today is a console error and the message + simply not appearing in the thread. The user's next reasonable action is pressing send + again, which works because the text is still there, but nothing prompts them to. +- **There is no automatic retry.** A failed send is not queued or retried by the app; it is + a dead end until the user notices and resends manually. +- **There is no offline queue.** The service worker does not intercept `fetch` or queue + failed sends (see + [Service worker — offline behaviour today](concepts-service-worker.md#offline-behaviour-today)). + A message typed while offline fails the same way any other network error does, with the + same silent-console-only signal — it is not saved for automatic delivery once + connectivity returns. + +If you are extending the composer, wiring `toastError('Message failed to send')` into that +`catch` block is a small, low-risk improvement consistent with the rest of this document's +[toast guidance](#the-toast-api-libusetoastts) — the current silent failure is a gap, not a +pattern to preserve. From 25483a8a7d6029baa48df698e8da4108c3c83919 Mon Sep 17 00:00:00 2001 From: G-ELM Date: Mon, 31 Aug 2026 00:02:05 +0100 Subject: [PATCH 4/4] docs(web): add frontend accessibility guide States the WCAG 2.1 AA target and that conformance is currently checked manually (no automated a11y tooling in the repo yet). Documents keyboard navigation for the conversation list and composer, contrasts Modal's full focus-trap/restore implementation against the safety-number panel's current gap, explains the aria-live approach for incoming messages without stealing focus, flags the low-opacity text contrast risk, and gives a pre-merge checklist. Also registers all four new docs (contract testing, service worker, error handling, accessibility) in the documentation index. --- apps/web/docs/accessibility.md | 231 +++++++++++++++++++++++++++++++++ docs/README.md | 4 + 2 files changed, 235 insertions(+) create mode 100644 apps/web/docs/accessibility.md diff --git a/apps/web/docs/accessibility.md b/apps/web/docs/accessibility.md new file mode 100644 index 0000000..c5e95fd --- /dev/null +++ b/apps/web/docs/accessibility.md @@ -0,0 +1,231 @@ +# Frontend accessibility guide + +The accessibility standard `apps/web` targets, and the patterns required to meet it: +keyboard navigation through the conversation list and message composer, focus management +in modals and the safety-number panel, screen-reader announcement of incoming messages, +and colour contrast. + +This document describes both the pattern to follow and, honestly, where the current code +already meets it and where it does not yet. Where a gap is called out, treat it as +something to fix when you touch that area, not as the intended design. + +--- + +## Target standard + +**WCAG 2.1 Level AA.** This is the conventional baseline for a web application handling +real user-to-user communication, and is the standard assumed throughout this document — +there is no stricter internal bar and no formal deviation from it. + +**How it is checked today: manually, not automatically.** There is no `eslint-plugin-jsx-a11y`, +no automated axe/Lighthouse run in CI, and no accessibility test suite in this repo as of +this writing. Conformance currently depends entirely on the patterns below being followed +by hand and reviewed in PRs. If you are adding accessibility tooling, wiring an automated +check (axe-core in CI, or `eslint-plugin-jsx-a11y` at minimum) closes a real gap rather +than adding redundant coverage — until then, the [pre-merge checklist](#pre-merge-checklist-for-a-new-interactive-component) +below is the actual enforcement mechanism. + +--- + +## Keyboard navigation + +### Conversation list + +`components/conversations/ConversationListSidebar.tsx` renders each conversation as a +plain ``: + +```tsx + + {/* avatar, title, preview */} + +``` + +There is no custom keyboard handling — no roving `tabindex`, no arrow-key list navigation. +Keyboard support comes entirely from using a real `` (an anchor): it is naturally +focusable, appears in Tab order, and activates on Enter, all for free. **This is the +required pattern for list items in this app** — a clickable row must be a real +link/button, never a `
`, precisely so keyboard support does not have to be +hand-built. Arrow-key roving-tabindex navigation (a full ARIA `listbox`/`menu` pattern) is +not implemented and is not required — sequential Tab order through the list is the +supported navigation model. + +### Message composer + +The composer (`app/app/conversations/[id]/page.tsx`) supports Enter-to-send / +Shift+Enter-for-newline on the message input: + +```tsx + setSendText(e.target.value)} + onKeyDown={(e) => { + if (e.key === 'Enter' && !e.shiftKey) { + e.preventDefault(); + void handleSendEncrypted(); + } + }} +/> +``` + +When adding a keyboard shortcut like this, always `preventDefault()` only on the branch +that consumes the key (here, plain Enter) and let every other key (including Shift+Enter) +fall through untouched — do not swallow keys you are not handling. + +The composer's send and attach-file buttons are real `