diff --git a/docs/superpowers/specs/2026-07-26-onboarding-wizard-deferred-commit-design.md b/docs/superpowers/specs/2026-07-26-onboarding-wizard-deferred-commit-design.md new file mode 100644 index 0000000..afedd4f --- /dev/null +++ b/docs/superpowers/specs/2026-07-26-onboarding-wizard-deferred-commit-design.md @@ -0,0 +1,232 @@ +# Onboarding wizard with deferred DB commit + +Status: approved, not yet implemented. + +## Problem + +Feedback on the just-shipped mandatory Garmin-connect onboarding +(`docs/superpowers/specs/2026-07-26-improve-first-connection-design.md`): + +1. UX: a wizard-style Previous/Next pair reads better than the current + Continue/Log out pairing across `CreateProfile` → `ConnectGarmin`. +2. If the user quits mid-flow without logging out, the account is left in a + provisioned-but-not-Garmin-connected state -- confirmed this should + instead mean **nothing is written to the database at all** until Garmin + authentication actually succeeds, at which point display name + Garmin + credentials + the connected flag are all committed together, atomically. + +(A third piece of feedback, structured JSON logging, is unrelated and gets +its own design afterward.) + +## Design + +### Consequence: the three-way LoginGate fork collapses + +Since account provisioning and Garmin-connection now happen together in one +atomic step, `has_profile=true && garmin_connected=false` becomes +structurally unreachable -- confirmed to collapse `LoginGate` back to a +two-way fork (`has_profile ? App : OnboardingWizard`). `profile.garmin_connected_at` +stays as an informational timestamp (still written by the new atomic +commit); it just no longer gates anything on its own. + +### The core problem: Garmin auth needs an account today + +`garminFor` builds a per-user `garmin.Client` keyed by a DB user id and +reads Garmin credentials from that user's `profile` row. To authenticate +with Garmin *before* an account exists, onboarding needs a **temporary, +not-yet-persisted** Garmin session -- keyed by the OIDC subject (already +known from the session cookie, before any `users` row exists) instead of a +user id. A long-held-open SQL transaction across the wizard's multiple +HTTP round trips is not an option: SQLite's single connection +(`SetMaxOpenConns(1)`, see `db.go`) means one held-open transaction would +block every other request in the app for as long as the user takes to +finish onboarding. + +### Backend: ephemeral pre-account Garmin sessions + +New type and `Server` field (`server.go`, alongside the existing +`userGarmin`/`userSync`/etc. per-user maps): + +```go +// setupSession is a temporary, not-yet-persisted Garmin authentication +// attempt made during onboarding, before any users/profile row exists -- +// keyed by OIDC subject (the only stable identifier available pre-account) +// rather than a user id. Promoted into Server.userGarmin once +// /api/setup/complete actually creates the account; evicted lazily (the +// next setup-endpoint touch for that subject checks staleness first) once +// idle past setupSessionIdleTimeout. +type setupSession struct { + Client garmin.Client + Email, Password string + Status garmin.AuthStatus + Message string + LastUsed time.Time +} + +const setupSessionIdleTimeout = 15 * time.Minute +``` + +`Server` gains `setupGarmin map[string]*setupSession` (keyed by OIDC +subject string), initialized alongside the other maps in `NewServer`. + +Helper methods on `Server` (`server.go`): + +- `setupSessionFor(sub string) (*setupSession, bool)` -- returns the + session if present and not stale; a stale one is closed and evicted + first (lazy cleanup, no background sweep goroutine -- an abandoned + session otherwise leaks one idle subprocess until the backend restarts, + an accepted trade-off for a single-operator app). +- `replaceSetupSession(sub, email, password string) *setupSession` -- + closes and replaces any existing session for `sub` (same "close old, + spawn new" semantics as `garmin.Client.UpdateCredentials`), builds a + fresh ephemeral client whose `TokenStorePath` is namespaced under + `{root}/setup/{sub}` (distinct from the permanent `{root}/{userID}` + namespacing `garminFor` uses, so two people onboarding concurrently never + collide, and so the ephemeral session's persisted Garmin tokens have + *some* home even though no user id exists yet). +- `recordSetupAuthResult(sub string, res garmin.AuthResult)` -- updates a + session's `Status`/`Message`/`LastUsed` after a login or MFA attempt. +- `removeSetupSession(sub string)` -- closes and drops `sub`'s session + (best-effort removes its `{root}/setup/{sub}` token-store directory too). + Called both when `/api/setup/complete` promotes a session into the + permanent cache, and from `handleSessionLogout` so abandoning mid-wizard + via logout doesn't linger until the idle timeout. + +### Backend: three new endpoints replace `/api/setup` + +Registered where `r.Post("/setup", s.handleSetup)` used to be (same +middleware group: `RequireSession` + `resolveUser`, but *not* +`requireProvisionedUser` -- these must work before a profile exists): + +```go +r.Route("/setup", func(r chi.Router) { + r.Post("/complete", s.handleSetupComplete) + r.Route("/garmin", func(r chi.Router) { + r.Post("/login", s.handleSetupGarminLogin) + r.Post("/mfa", s.handleSetupGarminMFA) + }) +}) +``` + +- **`POST /api/setup/garmin/login`** `{garmin_email, garmin_password}`: + 401 if unauthenticated; 409 if a profile already exists for this subject + (mirrors old `handleSetup`'s existing check); 400 on missing fields. + Otherwise `replaceSetupSession` + `Authenticate()` + `recordSetupAuthResult`, + responding with the same `{status, message}` shape `/api/auth/login` + already uses. +- **`POST /api/setup/garmin/mfa`** `{code}`: 409 if no session exists for + this subject (never started, or expired) -- frontend resets to the + Garmin step and asks the user to reconnect. Otherwise + `CompleteMFA()` + `recordSetupAuthResult` on the *same* session (never + replaces it -- MFA continues an in-progress attempt on the same + subprocess). +- **`POST /api/setup/complete`** `{display_name}`: 409 if already + provisioned; 409 if no session exists for this subject or its `Status != + garmin.AuthSuccess` ("Garmin isn't connected yet"). Otherwise, in order: + 1. `ProvisionUser(sub, displayName)` (unchanged). + 2. `GetProfile` + set `GarminEmail`/`GarminPassword` from the session + + `UpdateProfile` (unchanged method, just called here instead of from + the Profile page). + 3. `MarkGarminConnected(userID)`. + 4. Promote: move the session's already-authenticated `garmin.Client` + into `s.userGarmin[userID]` and its recorded status into + `s.userAuthStatus[userID]`/`userAuthMessage[userID]` (avoids a + redundant re-authentication -- and possibly a repeat MFA prompt -- + immediately after signup), then `delete(s.setupGarmin, sub)` without + closing the client (ownership transferred, not discarded). + 5. Best-effort `os.Rename` the ephemeral token-store directory + (`{root}/setup/{sub}`) to the permanent one (`{root}/{userID}`), so a + later respawn of this user's subprocess (e.g. after a server restart) + resumes the already-established Garmin session instead of requiring a + fresh login. Logged, not fatal, on error. + +`internal/api/auth.go`'s existing `handleAuthLogin`/`handleAuthMFA`/`handleAuthStatus` +(used by the Profile page's `GarminConnection.tsx` for reconnecting an +already-provisioned user) are untouched. + +### Frontend: one wizard component instead of two screens + +`CreateProfile.tsx` and `ConnectGarmin.tsx` are deleted, replaced by +`frontend/src/OnboardingWizard.tsx` (and `OnboardingWizard.css`, replacing +`CreateProfile.css`). Two top-level steps (`"name" | "garmin"`); the +in-progress-MFA and already-authenticated states are just conditions on +`auth?.status` *within* the `"garmin"` step, not a third step value -- +simpler than threading a separate `"mfa"` step through, and matches how +`GarminConnection.tsx` already branches on `status` today: + +- **`name` step**: display name field, held in local state only -- no API + call. "Next" just validates non-empty and advances `step` to `"garmin"`. +- **`garmin` step**, branching on `auth?.status`: + - `undefined` / `"failed"`: email/password fields. "Previous" returns to + the `name` step (pure local state, nothing to undo server-side). + "Next" calls `api.setupGarminLogin(email, password)`. + - `"mfa_required"`: MFA code field. "Previous" clears `auth` back to + `null`, which re-shows the credentials form (submitting it again just + replaces the ephemeral session, same as the backend's "close old, + spawn new" semantics). "Submit code" calls `api.setupGarminMFA(code)`. + - `"authenticated"`: a confirmation message and "Continue to geniusrun", + which calls `api.setupComplete(displayName)` and then `onCreated(...)`. +- "Log out" (`
`) stays + visible throughout, the same escape hatch as before. + +`frontend/src/api/client.ts`: remove `setup`; add `setupGarminLogin`, +`setupGarminMFA`, `setupComplete` (bodies/shapes as described above). + +`frontend/src/LoginGate.tsx`: collapse back to +`!session.has_profile ? : ` -- +`garmin_connected` is no longer read here at all (still present on +`SessionInfo`/`session/me`, just unused by the gate). + +### Error handling + +- Garmin login/MFA failure: same as today -- message shown, fields stay + editable, retry always available, nothing persisted regardless of + outcome. +- `/api/setup/complete` with no session, or session not yet `authenticated`: + `409`, frontend falls back to the `garmin` step (credentials form) so the + user can (re)connect. +- `/api/setup/complete`/`/api/setup/garmin/login` when already provisioned + (double-submit, duplicate tab): `409` "profile already exists"; frontend + treats this as success and refreshes session info rather than showing an + error. +- Idle sessions (15 minutes unused) are evicted lazily on next touch -- + `/api/setup/garmin/mfa` or `/api/setup/complete` against an expired + session behaves identically to "never connected" (409), prompting a + fresh `/api/setup/garmin/login`. +- Logging out mid-wizard closes and drops that subject's ephemeral session + and its token-store directory. + +### Testing + +- `internal/api`: `POST /api/setup/garmin/login` (mock `AuthSuccess`) + creates no DB user yet (`GetUserBySub` still not found); the + `mfa_required` path via `/api/setup/garmin/mfa` on the same session; + `/api/setup/complete` after a successful session creates the user with + `GarminEmail`/`GarminPassword`/`GarminConnectedAt` all set, and promotes + the *same* `*mock.Client` instance into `s.userGarmin[userID]` (asserted + via `garminFor` returning that exact pointer, and `ClosedCalled` still + false -- proving no redundant re-authentication happened); + `/api/setup/complete` with no prior login attempt → 409; + already-provisioned subject hitting either endpoint → 409; two different + OIDC subjects' ephemeral sessions never interfere (isolation test, + matching this repo's adversarial-isolation convention). A token-store + rename test mirroring the existing `TestDeleteProfile_RemovesTokenStoreDirectory` + pattern (set `GarminBase.TokenStorePath`, confirm the directory ends up + under the new user id's path after `/api/setup/complete`, not under + `setup/{sub}`). +- No frontend test suite -- manual smoke test: fresh account → name step → + Previous/Next between name and Garmin steps preserves typed input → + wrong password shows an error and stays editable → MFA prompt → Previous + from MFA returns to credentials → correct credentials (or MFA resolved) + → "Continue to geniusrun" → lands in the app → confirm `GetUserBySub` + now exists with everything set. Separately: start the wizard, enter + Garmin credentials, then just close the tab without finishing -- + confirm no `users`/`profile` row was created at all. + +## Out of scope + +- A background sweep goroutine for abandoned ephemeral sessions -- accepted + as a known, low-stakes trade-off (see Error handling). +- Structured JSON logging (the third piece of feedback) -- separate design. +- Any change to the Profile page's own reconnect flow + (`GarminConnection.tsx`, `/api/auth/*`) -- untouched.