Christophe Vila
e2b2bf9611
Garmin auth/sync routes move under /api/garmin/*; sync.Service becomes garmin.Sync with garmin.SyncConfig/ClientConfig; applog becomes internal/log; the test mock moves into the garmin package as MockClient (breaking the test-only import cycle the merge created); stale test URLs and type names updated to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
168 lines
9.7 KiB
Markdown
168 lines
9.7 KiB
Markdown
# Modern Error Displays — Design
|
||
|
||
**Status:** Approved, ready for implementation planning.
|
||
**Origin:** `docs/IDEAS.md`'s "modern error displays" backlog item.
|
||
|
||
## Goal
|
||
|
||
Replace every page/component's ad-hoc inline `error`-state paragraph
|
||
(`<p className="error">{error}</p>` and its several near-duplicates) with a
|
||
single, consistent banner system: ephemeral, color-coded, full-width bars
|
||
(red for error, orange for warning, blue for notice, green for success)
|
||
that stack, auto-dismiss, and can be closed manually.
|
||
|
||
This is a full replacement, not an addition alongside the old pattern —
|
||
every existing inline error display in the frontend is migrated to the new
|
||
system, with one deliberate exception (OnboardingWizard's field-validation
|
||
messages — see below).
|
||
|
||
## Non-goals
|
||
|
||
- No backend changes. This is purely a frontend presentation change; no API
|
||
shape changes anything about how errors are already communicated to the
|
||
frontend.
|
||
- No new "logout failed" error path. `handleSessionLogout` on the backend
|
||
has no failure mode today (unconditional cookie-clear + redirect) — the
|
||
banner system will be *capable* of showing a logout error if one is ever
|
||
added, but nothing is invented here just to exercise it.
|
||
|
||
## Architecture
|
||
|
||
### `frontend/src/banner.ts` — the store
|
||
|
||
A plain module, not a React Context/Provider. The codebase has no existing
|
||
`createContext`/`useContext` usage anywhere, and — critically —
|
||
`frontend/src/api/client.ts` is a plain module (not a component), so a
|
||
Context wouldn't be reachable from it without extra plumbing. A
|
||
module-level singleton store is simpler and callable from anywhere:
|
||
|
||
```ts
|
||
type Severity = "error" | "warning" | "notice" | "success";
|
||
type Banner = { id: number; severity: Severity; message: string };
|
||
|
||
// newest-on-top; auto-dismisses after 8000ms; manually dismissible early.
|
||
export function showError(message: string): void;
|
||
export function showWarning(message: string): void;
|
||
export function showNotice(message: string): void;
|
||
export function showSuccess(message: string): void;
|
||
export function dismiss(id: number): void;
|
||
|
||
// useSyncExternalStore plumbing for the render side.
|
||
export function subscribe(listener: () => void): () => void;
|
||
export function getSnapshot(): Banner[];
|
||
```
|
||
|
||
Multiple banners stack (newest on top); each dismisses independently on its
|
||
own timer. A banner's `message` may contain embedded newlines (`\n`) for
|
||
the sync-result-plus-nudges case (see below) — the renderer treats them as
|
||
line breaks within one banner, not separate banners.
|
||
|
||
### `frontend/src/components/BannerStack.tsx` + `BannerStack.css` — the renderer
|
||
|
||
- `useSyncExternalStore(subscribe, getSnapshot)` to read the current list.
|
||
- Renders one `<div className="banner banner-{severity}">` per active
|
||
banner: the message text (respecting embedded newlines) plus a `×` close
|
||
button that calls `dismiss(id)`.
|
||
- Four CSS variants — `banner-error` (red bg), `banner-warning` (orange
|
||
bg), `banner-notice` (blue bg), `banner-success` (green bg) — each with
|
||
readable foreground text/icon color against its background.
|
||
- **Mounted exactly once**, in `frontend/src/main.tsx`, as a sibling above
|
||
`<LoginGate />`:
|
||
|
||
```tsx
|
||
createRoot(document.getElementById("root")!).render(
|
||
<StrictMode>
|
||
<BannerStack />
|
||
<LoginGate />
|
||
</StrictMode>,
|
||
);
|
||
```
|
||
|
||
Rendered in normal document flow (not `position: fixed`), so it
|
||
naturally pushes down whatever's currently showing below it — the login
|
||
screen, the onboarding wizard, or the full app (header + tabs + content)
|
||
— without needing a separate mount point wired into each of those three
|
||
top-level layouts individually. When no banners are active, it renders
|
||
nothing (no reserved empty space).
|
||
|
||
### `frontend/src/api/client.ts` — friendlier network-failure messages
|
||
|
||
`request()`'s `fetch()` call is wrapped so a network-level failure (the
|
||
`fetch()` promise itself rejecting — offline, connection refused, CORS,
|
||
etc.) throws a friendly, fixed message instead of the raw browser error
|
||
text:
|
||
|
||
```ts
|
||
let res: Response;
|
||
try {
|
||
res = await fetch(`${BASE_URL}${path}`, { ... });
|
||
} catch {
|
||
throw new Error("Can't reach the server — check your connection.");
|
||
}
|
||
```
|
||
|
||
A real HTTP response that just happens to be non-2xx (4xx/5xx) is
|
||
unaffected — that branch keeps its existing `${status} ${body}` message,
|
||
since that's a legitimate API-level error, not a "the backend isn't there"
|
||
condition.
|
||
|
||
This means every call site's existing pattern —
|
||
`.catch((e) => setError(String(e)))` — becomes
|
||
`.catch((e) => showError(String(e)))`, and the friendly message for a truly
|
||
unreachable backend falls out for free, with no per-call-site
|
||
network-error special-casing needed. This is what satisfies IDEAS.md's
|
||
"used on all pages/tabs in case of backend is not here": every page's
|
||
existing catch-and-display path already covers this, once it displays via
|
||
`showError` instead of local state.
|
||
|
||
## Migration inventory
|
||
|
||
Every current inline-error site, and what changes:
|
||
|
||
| File | Before | After |
|
||
|---|---|---|
|
||
| `components/GarminConnection.tsx` | Local `error` state, all catch blocks `setError(String(e))`, inline `{error && <p className="error">{error}</p>}` | Remove the state and the paragraph; every catch block calls `showError(String(e))` directly. |
|
||
| `components/SyncModal.tsx` | Never auto-closes; on completion renders a "final result" block inline (success/error line + optional pending-activities/pending-workouts nudges) with a manual Close button; a status-poll fetch failure renders inline too. | The "final result" JSX block is deleted entirely — **the modal now purely shows the progress bar/spinner and nothing else**. On the poll tick where `status.in_progress` is first observed to be `false`, build one message: the success/error line, followed by a newline-separated line for each applicable pending nudge (`"N more activities pending — click Sync now again"` / `"N more workouts pending — click Sync now again"`), call `showSuccess(...)` or `showError(...)` with that combined message, then call `onClose()` immediately — the modal disappears the instant sync finishes, and the banner carries the result. A transient status-poll fetch failure (modal still open, sync still presumably running) calls `showError(String(e))` but does **not** close the modal — polling continues as it does today. |
|
||
| `pages/Profile.tsx` | Local `error` state (profile load/save failures), inline paragraph. | Same pattern as GarminConnection — state and paragraph removed, catch blocks call `showError`. |
|
||
| `pages/Activities.tsx` | Local `error` state (multiple load/action failures), inline paragraph. | Same. |
|
||
| `pages/Analysis.tsx` | Local `error` state, inline paragraph. | Same. |
|
||
| `components/TrainingTypesCard.tsx` | Local `error` state, inline paragraph. | Same. |
|
||
| `LoginGate.tsx` | Reads `?auth_error=` from the URL synchronously during render, shows `<p className="login-gate-error">` with a mapped message. | A mount effect reads `?auth_error=` once and calls `showError(mappedMessage)` (same `AUTH_ERROR_MESSAGES` mapping as today); the inline paragraph and its CSS class are removed. |
|
||
| `OnboardingWizard.tsx` | Single `error` state used for both field validation ("Please enter a display name.", "Please enter your Garmin email and password.") **and** real failures (Garmin login rejected, MFA rejected, `complete()` failing after a successful Garmin auth) — all rendered via the same `onboarding-wizard-error` paragraph, in 4 places. | **Split.** Field-validation messages stay exactly as they are today (local state, inline paragraph, tied to a specific input the user needs to fix — a transient top-of-page banner is the wrong medium for "you left a required field blank"). Real failures (`submitGarmin`'s catch, `garminMFA`'s catch, `complete()`'s catch) call `showError(...)` instead of `setError(...)`. The `complete()`-failure Retry button, which today is conditionally rendered on `error &&`, switches to a new local boolean `completeFailed` (set `true` in that catch block) — the button's visibility no longer depends on the error text still being present, since that text now lives only in the (transient, auto-dismissing) banner. |
|
||
|
||
**Dead CSS removal:** once no `.tsx` file references them, delete the
|
||
`.error`, `.onboarding-wizard-error`, and `.login-gate-error` rules from
|
||
their respective CSS files.
|
||
|
||
## Data flow example (sync completion)
|
||
|
||
1. User clicks "Sync now" → `GarminConnection.sync()` calls
|
||
`api.syncRun()`, sets `showSyncModal(true)`.
|
||
2. `SyncModal` polls `/api/sync/status` every 1.5s, rendering the
|
||
spinner/progress bar per `progress.Phase` (unchanged from the existing
|
||
implementation).
|
||
3. A poll response shows `in_progress: false` for the first time.
|
||
`SyncModal` builds a message from `last_run.Status`/`ErrorMessage`/
|
||
`ActivitiesFetched` plus `activities_pending_details`/
|
||
`workouts_pending`, calls `showSuccess(message)` or `showError(message)`
|
||
accordingly, then calls `onClose()`.
|
||
4. `GarminConnection` unmounts `SyncModal` (its normal `showSyncModal`
|
||
toggle, unchanged). The banner, now living in the shared store
|
||
independent of any unmounted component, continues to display and
|
||
auto-dismiss on its own schedule.
|
||
|
||
## Testing
|
||
|
||
No frontend test suite exists in this repo (established convention —
|
||
verification is `tsc -b`/`vite build`, `oxlint`, and manual browser
|
||
checks). Verification for this feature is:
|
||
|
||
- `npm run build` / `npm run lint` clean.
|
||
- Manual smoke test covering each severity: trigger a real API error (e.g.
|
||
an invalid save), a network failure (stop the backend, attempt any
|
||
action), a successful sync, a sync ending in error, a sync with pending
|
||
activities/workouts remaining, and the login screen's `auth_error`
|
||
redirect — confirming banners stack, auto-dismiss, and can be closed
|
||
manually, and that `SyncModal` now closes itself the instant sync
|
||
finishes rather than waiting for a manual Close click.
|