Skip to content

PT-4175: First-run state machine and modal shell#2576

Open
katherinejensen00 wants to merge 19 commits into
mainfrom
worktree-pt-4175
Open

PT-4175: First-run state machine and modal shell#2576
katherinejensen00 wants to merge 19 commits into
mainfrom
worktree-pt-4175

Conversation

@katherinejensen00

@katherinejensen00 katherinejensen00 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Summary

PT-4175 delivers the foundation for the simple-mode first-run setup wizard: a persisted platform.firstRunComplete flag and a startup state machine that gates the app behind a non-dismissable modal until onboarding completes. It ships the shell, the gate, and four placeholder steps that the sibling tickets (PT-4176 Language / PT-4177 Identify / PT-4178 Sync consent / PT-4179 Sync progress) fill in by swapping entries in the shell's stepComponents map. Additive and backward-compatible; the four review passes found no Critical or Important issues.

What changed & where to look

Authored source below (lib/papi-dts/papi.d.ts and lib/platform-bible-react/dist/** in the diff are regenerated build output, not hand-edited).

State machine — start here

  • src/renderer/services/first-run.reducer.ts — pure decideFirstRun(firstRunComplete, wizardActive, registrationValidity)showApp / completeThenShowApp (existing-user migration) / waitForRegistration / startWizard(step). The whole routing decision in one function; its test mirrors every row.
  • src/renderer/services/first-run-store.ts — renderer store that feeds the reducer and exposes a subscribable status. Owns: idempotent startup resolve (StrictMode-safe), migration backfill, resume-to-sync after the registration relaunch, cache-non-clobber on read failure, retry, and the power-mode fast-path.
  • src/renderer/services/resolve-registration-validity.ts — bounded (15s) tri-state check of paratextRegistration.doesUserHaveValidRegistration.

UI

  • src/renderer/components/first-run/first-run-overlay.component.tsx — the full-viewport non-dismissable gate (loading / error+Retry / wizard) at Z_INDEX_FIRST_RUN.
  • .../first-run-shell.component.tsx — wizard chrome (title, "Step X of Y", Back/Skip/Next footer, busy + inline error) and the injectable stepComponents map siblings use to swap in real steps.
  • .../steps/*.placeholder.component.tsx — the four temporary placeholders; first-run-step-props.model.ts — the step contract.

Wiring & settings

  • src/renderer/app.component.tsx — mounts the overlay and kicks off resolution on startup.
  • src/main/startup-tasks.ts — gates simple-mode auto-sync on firstRunComplete (a fresh user never syncs before consenting).
  • src/extension-host/data/core-settings-info.data.ts + src/declarations/papi-shared-types.ts — the new hidden platform.firstRunComplete setting.
  • lib/platform-bible-react/src/components/z-index.tsZ_INDEX_FIRST_RUN (700, above the menubar).

How to review

  1. Read first-run.reducer.ts (+ its test) — the entire routing decision, small and pure.
  2. Skim first-run-store.ts to see how the reducer is fed and how migration / resume / retry / power-mode work.
  3. Glance at the overlay + shell for the gate and footer.

The test files mirror each behavior, so they double as behavioral docs.

Testing

53 unit tests: reducer truth table; store (resume-after-relaunch, StrictMode idempotency, concurrent-retry dedup, cache-fallback, power-mode initial seed); resolver (timeout + non-boolean); the startup gate; and component/wiring tests. All user-facing text is localized; Storybook stories cover the gate (loading/error/wizard) and shell (all four steps).

Notes / follow-ups (mostly for the sibling tickets)

  • PT-4177 (Identify) triggers platform.restart and must keep the paratext-registration startup float suppressed while the wizard owns Identify.
  • PT-4179 (Sync progress) should trigger its own sync rather than rely on the once-at-startup backstop.
  • Deferred by design: a modal/notification opened while the gate is up would render behind it (latent — nothing opens one today); the full-screen Dialog CSS override is brittle against a future shadcn upgrade (a dedicated blocking-overlay primitive is the deeper follow-up).
  • AI-assisted — full findings + interview notes below.
Full /review-paratext summary (findings, positives, interview notes)

Code Review Summary

Branch: worktree-pt-4175

Base: origin/main

Date: 2026-07-17

Review model: Claude Opus 4.8 (1M context)

Files changed: 45 (28 authored source files, plus regenerated lib/papi-dts/papi.d.ts and lib/platform-bible-react/dist/** build output)

Overview

PT-4175 lays the foundation for the simple-mode first-run setup wizard: a persisted platform.firstRunComplete flag and a startup state machine that, in simple mode, gates the app behind a non-dismissable modal until onboarding completes. It ships the shell, the gate, and four placeholder steps (Language / Identify / Sync consent / Sync progress) that the sibling tickets PT-4176/77/78/79 will replace by swapping entries in the shell's stepComponents map. The state machine handles the existing-user migration (silently marks complete, no wizard), the resume-to-sync flow after the registration relaunch, and a bounded registration-validity check with a Retry/error affordance. Startup auto-sync is gated on the flag so a fresh user never syncs before consenting.

The four analysis passes (API/correctness, style/patterns, coverage/compliance, UX) found no Critical or Important issues. The change is additive and backward-compatible, has thorough unit coverage for the reducer, store, resolver, and startup gate, routes all user-facing text through localization, and follows the established store/component patterns. Minor findings were resolved or consciously deferred during the interview (below).

API Changes

  • @papi/core (papi-shared-typespapi.d.ts): new user setting 'platform.firstRunComplete': boolean (default false, isHidden: true). Additive, backward-compatible.
  • platform-bible-react: new export Z_INDEX_FIRST_RUN (= 700). Additive.
  • src/renderer/hooks/use-interface-mode.hook.ts: INTERFACE_MODE_CACHE_KEY changed module-private → exported (renderer-internal, not a public @papi/platform-bible-react surface). Additive.
  • src/extension-host/data/core-settings-info.data.ts: added the platform.firstRunComplete contribution entry + validator; widened booleanValidator's generic to include the new key (internal, additive).

No changes to platform-bible-utils, no extension *.d.ts changes. The papi.d.ts diff is a regenerated additive entry, not a hand-edit.

Findings

Critical — Must address before merge

None.

Important — Should address before merge

None.

Minor — Consider

  • first-run.reducer.ts comment used a bare W for wizardActive (fixed during review: spelled out as wizardActive in the reducer comment and the two matching first-run-store.test.ts references)
  • %firstRun_button_skip% label "Skip" finishes the whole wizard, not one step (fixed during review: relabeled to "Skip setup" to name the outcome)
  • package-lock.json top-level name flipped to "pt-4175" (worktree artifact) (fixed during review: reverted so the lockfile keeps the canonical package name and produces no diff)
  • %firstRun_button_finish% "Finish" is generic; "Finish setup"/"Get started" would be more specific (kept — "Finish" is a widely-understood wizard terminal label; PT-4179 owns the real sync-progress step and can revisit copy in context)
  • %firstRun_stepIndicator% uses a camelCase segment vs the firstRun_step_* snake segments (left as-is — reasonable camelCase segment; renaming for a pure nit isn't worth the churn, and there are no translations yet)
  • The four placeholder step components are structurally identical (DRY) (intentional — each is a deliberately temporary placeholder with its own localization key and a JSDoc pointing at its replacement ticket; extracting a shared component would complicate the per-ticket swap)
  • Per-step heading: the shell renders only the product title + "Step X of Y" (informational for the sibling tickets — each real step should supply its own heading; not a placeholder-shell issue)
  • %settings_platform_firstRunComplete_label% label text never surfaces (setting is isHidden) (required by the settings-contribution shape; follows the existing hidden-setting pattern)
  • lib/platform-bible-react/src/index.ts z-index export block isn't fully alphabetized (pre-existing; not introduced by this change)

[Author response: Directed the three cheap fixes to be applied during review (bare-letter comment, Skip label, lockfile revert); chose to keep "Finish" and the stepIndicator key name; the remaining minors are informational/no-action.]

Template Propagation

Shared Regions Modified

None.

Extension Config Changes

None.

Positive Observations

  • State machine cleanly decomposed: a pure, exhaustively-tested reducer (decideFirstRun) + an idempotent renderer store + the gate/shell — good separation and consistency with the existing workspace-updating-store pattern.
  • Fails in the safe direction throughout: startup auto-sync is skipped on an unreadable flag (consent-safe), and the store avoids clobbering a valid cache on a transient settings-read failure.
  • INTERFACE_MODE_CACHE_KEY is exported and shared (rather than a duplicated literal), preventing a silent startup-routing bug if the key ever changes.
  • Accessibility handled: role="status" (loading) and role="alert" (error) live regions, visually-hidden DialogTitle/DialogDescription, and a real focus trap; the non-dismissable gate blocks Escape/outside-click and the close button intentionally.
  • Thorough tests: reducer truth table, store resume-after-relaunch + StrictMode idempotency + concurrent-retry dedup + cache-fallback + power-mode initial-seed, tri-state resolver (incl. timeout and non-boolean), the startup gate, and component/wiring tests.
  • Storybook coverage for the meaningful states (first-run-overlay.stories.tsx: loading/error/wizard; first-run-shell.stories.tsx: all four steps).
  • Feature-folder colocation (components/first-run/) and additive, well-documented API surface (Z_INDEX_FIRST_RUN, the hidden setting).

Interview Notes

  • Purpose (author-confirmed): PT-4175 — first-run state machine and modal shell; the persisted flag + startup gate in simple mode, placeholder steps for the sibling tickets, and resume-to-sync after the registration relaunch.
  • AI-assisted: implementation was AI-generated (Claude) and reviewed across multiple adversarial passes before this run; the design rationale and decisions (state model, migration, consent-safe defaults, gating-primitive choice) are captured in the team's design spec. No Critical/Important findings required author explanation.
  • Decisions: author applied the cheap correctness/clarity fixes (bare-letter comment, "Skip setup" label, lockfile revert), kept "Finish" and the stepIndicator key name, and deferred the real-step microcopy/headings to the sibling tickets that own those steps.
  • No lack-of-understanding items.

In-Review Quality Check

Changes made during review: spelled out the wizardActive comment/references, relabeled %firstRun_button_skip% → "Skip setup", reverted the package-lock.json name artifact. Verification: the affected test files (shell, store, overlay, startup-tasks) pass (36/36); Prettier reports the changed files are correctly formatted; lint/format run on the staged files via the pre-commit hook. No new typecheck errors (a comment + a string-label + a test-name change; nothing type-relevant).

Suggested Review Focus

Prioritized areas for the author-reviewer meeting:

  • The state machine's (firstRunComplete, wizardActive, registrationValidity, interfaceMode, cache) routing — especially the existing-user migration and the resume-after-relaunch path (both unit-tested). Worth a quick sanity read of first-run.reducer.ts + first-run-store.ts.
  • The gating primitive: a full-viewport, non-dismissable Radix Dialog at Z_INDEX_FIRST_RUN (700). Known, deliberate deferrals: a modal/notification opened during the gate would render behind it (latent — nothing opens one today), and the full-screen CSS override is brittle against a future shadcn upgrade (a dedicated blocking-overlay primitive is the deeper follow-up).
  • Handoffs to the sibling tickets: PT-4177 (Identify) must trigger platform.restart and keep the registration extension's startup float suppressed; PT-4179 (Sync progress) should trigger its own sync rather than rely on the once-at-startup backstop; real steps supply their own headings and finalize footer copy.
  • This is AI-assisted work — worth confirming the reviewer is comfortable with the state-model choices captured in the design spec.

AI-assisted — session


This change is Reviewable

katherinejensen00 and others added 18 commits July 17, 2026 12:03
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…tent resolve

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements the step contract that all first-run wizard steps implement, plus
four placeholder components (Language, Identify, Sync Consent, Sync Progress)
with localized body text only. Placeholders are replaced by sibling tickets
(PT-4176/77/78/79). Includes test that verifies mock localized strings render.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Haiku <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix storybook import in first-run-shell.stories.tsx (@storybook/react → @storybook/react-webpack5)
- Fix import/no-named-as-default warnings: use named imports for placeholder steps
- Fix no-null/no-null errors: replace () => null with () => undefined in test stubs
- Add required comment above eslint-disable-next-line import/first in app.component.test.tsx
- Switch to named import { App } in app.component.test.tsx to match renderer/index.tsx
- Regenerate lib/papi-dts/papi.d.ts to include platform.firstRunComplete (was missing from prior task)

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ng test

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…f from code review

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… docs, loading/error UX polish

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ng Storybook states

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eset across step nav; dedup interface-mode cache key; sort localization block

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Spell out the bare `W` as `wizardActive` in the reducer comment and the two
  matching first-run-store.test.ts references (initialism guidance)
- Relabel %firstRun_button_skip% "Skip" -> "Skip setup" (names the outcome: it
  ends setup rather than skipping a single step)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Session-URL: <session URL>
Resolved conflicts:
- src/main/startup-tasks.ts (auto-merged, verified): keeps both PT-4162's
  power-mode session-sync restructure and PT-4175's first-run auto-sync gate;
  the gate sits in the simple-mode branch, before syncProjects.
- src/main/startup-tasks.test.ts: kept main's power-mode + unreadable-settings
  tests alongside PT-4175's simple-mode first-run gate tests.
- lib/platform-bible-react/dist/**: regenerated from merged source.
- lib/papi-dts/papi.d.ts: clean 3-way merge (retains platform.firstRunComplete).

Verified: startup-tasks + first-run tests pass (32); typecheck clean apart from
the pre-existing buildInfo.json error.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Session-URL: <session URL>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant