From 599e6a8a75e091c55b717c119dcca21af620d13d Mon Sep 17 00:00:00 2001 From: Ross Miller Date: Wed, 22 Jul 2026 21:58:56 -0500 Subject: [PATCH 1/5] docs: explain every port, label the diagram groups, and split the e2e doc The hexagon diagram's boxes are now named themes with a note saying they are not layers, and every one of the 15 ports gets a plain what-and-why line. The e2e doc splits into a how-to README and a design.md for the reasoning, fixing a duplicated breadcrumb. CONTEXT.md's intro drops the cleverness and the small overloads collapse into one table. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5 --- ARCHITECTURE.md | 87 +++++++++++++++++++++++++++++++++++++------- CONTEXT.md | 29 +++++++-------- e2e/README.md | 97 ++++++++++++------------------------------------- e2e/design.md | 59 ++++++++++++++++++++++++++++++ 4 files changed, 169 insertions(+), 103 deletions(-) create mode 100644 e2e/design.md diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 2cbbb7a..cf7db6d 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1,8 +1,14 @@ # Architecture -Tutor is one Electron desktop app for one reader on one machine. It generates a book chapter by chapter, and each chapter is shaped by the feedback and quiz results of the one before it. Everything below follows from that, and from there being exactly one writer and no cloud. +Tutor is one Electron desktop app for one reader on one machine. It generates a book chapter by chapter, and each chapter is shaped by the feedback and quiz results of the one before it. -Start here, then follow the links. Domain words are defined in [CONTEXT.md](CONTEXT.md), decisions and their costs in [docs/adr/](docs/adr/README.md), and the HTTP surface in [docs/api-routes.md](docs/api-routes.md), which is generated from the route registry rather than written by hand. +Everything below follows from those two facts. There is exactly one writer, and there is no cloud. + +Start here, then follow the links. + +- Domain words are defined in [CONTEXT.md](CONTEXT.md). +- Decisions and what each one cost are in [docs/adr/](docs/adr/README.md). +- The HTTP surface is in [docs/api-routes.md](docs/api-routes.md), generated from the route registry, never written by hand. ## 1. What talks to what @@ -27,10 +33,14 @@ flowchart LR server --> local ``` -The Fastify server is embedded in the Electron main process rather than deployed anywhere. It binds `127.0.0.1` on a free port at launch, or 3147 when run standalone with `pnpm dev:server`. The library is plain Markdown and YAML under the OS data directory, which is [ADR 0001](docs/adr/0001-filesystem-as-the-database.md). Narration is synthesized locally instead of by a metered cloud service, which is [ADR 0003](docs/adr/0003-local-kokoro-tts.md). +The Fastify server runs inside the Electron main process. It is not deployed anywhere. It binds `127.0.0.1` on a free port at launch, or port 3147 when run standalone with `pnpm dev:server`. + +The library is plain Markdown and YAML under the OS data directory ([ADR 0001](docs/adr/0001-filesystem-as-the-database.md)). Narration is synthesized locally instead of by a metered cloud service ([ADR 0003](docs/adr/0003-local-kokoro-tts.md)). ## 2. The server hexagon +The core never does I/O. It talks to 15 small interfaces, the ports, and each port has one or more adapters that do the real work. + ```mermaid flowchart LR subgraph core["server core"] @@ -44,18 +54,18 @@ flowchart LR subgraph ports["ports/ (15 interfaces)"] direction TB - pStore["BookRepository
ArtifactStore
LibraryMigrator
JobJournal
KeyVault"] - pAi["TextGeneration
ImageGeneration"] - pMedia["SpeechSynthesis
AudioAssembly
DiagramRenderer
EpubImport
EpubExport"] - pSys["BackgroundTasks
Clock
OsFileManager"] + pStore["books and files
BookRepository, ArtifactStore,
LibraryMigrator, JobJournal, KeyVault"] + pAi["AI
TextGeneration, ImageGeneration"] + pMedia["media
SpeechSynthesis, AudioAssembly,
DiagramRenderer, EpubImport, EpubExport"] + pSys["system
BackgroundTasks, Clock, OsFileManager"] end subgraph adapters["adapters/ (the only I/O)"] direction TB aStore["fs-*.ts
file-key-vault.ts"] aAi["ai-sdk-text-generation.ts
http-image-generation.ts"] - aMedia["kokoro-speech-synthesis.ts
ffmpeg-audio-assembly.ts
kroki- and electron-diagram-renderer.ts
epub2-import.ts, epub-gen-export.ts"] - aSys["in-memory- and journalled-background-tasks.ts
system-clock.ts, os-file-manager.ts"] + aMedia["kokoro, ffmpeg, kroki,
electron, epub2, epub-gen"] + aSys["in-memory and journalled tasks,
system-clock, os-file-manager"] end services --> pStore @@ -68,7 +78,50 @@ flowchart LR pSys --> aSys ``` -Nothing in the core names an adapter. [`server/composition-root.ts`](server/composition-root.ts) is the one place a real adapter is chosen, and `buildServer(overrides)` lets a test or the Electron shell substitute one. Every port ships an in-memory fake and a shared contract test, and every adapter that can be exercised without spending money or downloading a model runs that same contract, which is what stops a fake from drifting into a convenient fiction. See [`server/ports/README.md`](server/ports/README.md) and [`server/adapters/README.md`](server/adapters/README.md) for the full mapping, and [ADR 0005](docs/adr/0005-ai-sdk-behind-a-port.md) for why the AI SDK sits behind one. +The boxes inside `ports/` are just themes to keep the diagram readable. They are not layers, and they do not exist in the code. What each port actually is, and why it exists, in one line each. + +**Books and files** + +| Port | What it does | Why it is a port | +|---|---|---| +| BookRepository | Reads and writes the library itself, book metadata, chapters, quizzes, feedback | Services never touch `fs`, and tests run against an in-memory library | +| ArtifactStore | The binary files that belong to a book, covers, audio, EPUBs | Big blobs with their own lifecycle, kept apart from the YAML the repository owns | +| LibraryMigrator | Upgrades an older on-disk library to the current schema at boot | Works on raw YAML that may not validate yet, so it sits below the repository | +| JobJournal | One file per long-running job, so a restart can resume it | Added as a decorator, the in-memory task adapter never had to change | +| KeyVault | Stores the reader's API keys | Keys live in one place and never end up in logs or the journal | + +**AI** + +| Port | What it does | Why it is a port | +|---|---|---| +| TextGeneration | Every prompt to a language model, streaming and structured | The only doorway to AI in the whole app. One fake makes everything testable without a key | +| ImageGeneration | Generates book cover images | Same idea as TextGeneration with a smaller surface | + +**Media** + +| Port | What it does | Why it is a port | +|---|---|---| +| SpeechSynthesis | Turns chapter text into narration audio | The real model is a 100MB download. The fake keeps tests instant | +| AudioAssembly | Stitches chapter audio into one M4B audiobook | ffmpeg is a separate binary with its own failure modes | +| DiagramRenderer | Renders Mermaid blocks to images for EPUB export | The app renders offscreen in Electron, dev uses kroki. One interface hides which | +| EpubImport | Parses an uploaded EPUB into plain book data | Returns data only. Saving it is the service's job | +| EpubExport | Builds an EPUB file from rendered chapters | Wraps a CJS library with awkward packaging, quarantined here | + +**System** + +| Port | What it does | Why it is a port | +|---|---|---| +| BackgroundTasks | The task tray. Start a job, report progress, cancel | Long jobs outlive a request, and the UI watches them over SSE | +| Clock | The current time and fresh ids | Tests pin time instead of sleeping | +| OsFileManager | Reveals a file in Finder | The one place the app shells out to `open` | + +Three rules hold all of this together. + +- Nothing in the core names an adapter. [`server/composition-root.ts`](server/composition-root.ts) is the one place a real adapter is chosen, and `buildServer(overrides)` lets a test swap any of them out. +- Every port ships an in-memory fake and a shared contract test. +- Every adapter that can run without spending money or downloading a model runs that same contract test. That is what keeps a fake honest about how the real adapter behaves. + +See [`server/ports/README.md`](server/ports/README.md), [`server/adapters/README.md`](server/adapters/README.md), and [ADR 0005](docs/adr/0005-ai-sdk-behind-a-port.md) for why the AI SDK sits behind a port. ## 3. How a request travels @@ -83,7 +136,9 @@ flowchart LR ports --> adapters["adapters/"] ``` -Components render and hooks decide. Every call to the server goes through [`client/api/`](client/README.md), and a raw `fetch` or `new EventSource` anywhere else is an ESLint error rather than a convention, because the client previously held eighty four scattered fetch calls and two competing reconnect policies. +Components render. Hooks decide. + +Every call to the server goes through [`client/api/`](client/README.md). A raw `fetch` or `new EventSource` anywhere else is an ESLint error, not a convention. The client used to hold 84 scattered fetch calls and two competing reconnect policies, and the lint rule is what keeps that from coming back. ## 4. The adaptive loop @@ -113,7 +168,9 @@ sequenceDiagram Note over Server,AI: chapter 2 is shaped by chapter 1's feedback and quiz result ``` -Chapters are generated one at a time rather than up front, and the quiz exists partly to cover the generation latency. That is [ADR 0002](docs/adr/0002-just-in-time-chapter-generation.md). If the app is closed mid-generation the work is not lost, because jobs are journalled to disk and resumed at the next boot, which is [ADR 0008](docs/adr/0008-persisted-job-journal.md). +Chapters are generated one at a time, not up front, and the quiz exists partly to cover the generation wait ([ADR 0002](docs/adr/0002-just-in-time-chapter-generation.md)). + +Closing the app mid-generation loses nothing. Jobs are journalled to disk and resumed at the next boot ([ADR 0008](docs/adr/0008-persisted-job-journal.md)). ## 5. The dependency rule @@ -126,11 +183,13 @@ flowchart TD shared -. "ESLint error" .-> client ``` -`shared/` is the dependency root and imports neither side. It holds the Zod schemas, the status predicates, the HTTP contract types, and the SSE event unions, so the two halves of the app validate against the same definitions. This is one package shaped like a monorepo rather than real workspaces, which is [ADR 0004](docs/adr/0004-single-package-monorepo-shaped.md), and the folders are already package-shaped if that ever needs to change. +`shared/` is the root. It imports neither side. It holds the Zod schemas, the status predicates, the HTTP contract types, and the SSE event unions, so both halves of the app validate against the same definitions. + +This is one package shaped like a monorepo, not real workspaces ([ADR 0004](docs/adr/0004-single-package-monorepo-shaped.md)). The folders are already package-shaped if that ever needs to change. ## Deliberately out of scope -Observability and telemetry, a security-hardening pass, and release engineering were all considered and declined. The app runs locally on one machine, holds one reader's data, has no cloud component, and has no multi-user surface, so each of those would add machinery with nothing to protect or measure. The reasoning is recorded in [ADR 0004](docs/adr/0004-single-package-monorepo-shaped.md) rather than left as an unexplained gap. +Observability, a security-hardening pass, and release engineering were considered and declined. The app runs on one machine, holds one reader's data, and has no cloud component, so each of those would add machinery with nothing to protect or measure. The reasoning is in [ADR 0004](docs/adr/0004-single-package-monorepo-shaped.md) so the gap reads as a decision, not an oversight. ## Where to read next diff --git a/CONTEXT.md b/CONTEXT.md index 9d9fac6..db70108 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -2,9 +2,9 @@ Up: [ARCHITECTURE.md](ARCHITECTURE.md) # Domain language -The words this codebase uses, and the type or module that owns each one. Names here are the ubiquitous language. They appear unchanged in schemas, services, components, prompts, and UI copy, and a synonym for one of them is a bug in the writing rather than a stylistic choice. +The words this codebase uses, and the type or module that owns each one. These names appear unchanged in schemas, services, components, prompts, and UI copy. Do not invent synonyms for them. -Several words in this domain are overloaded, some of them badly. The later sections exist because those collisions are real and permanent, so the fix is to qualify them consistently rather than to rename around them. +Some words carry more than one meaning. Those collisions are permanent, so the sections below say how to qualify each one instead of renaming around them. ## Core domain @@ -44,21 +44,18 @@ Both keep their names. The domain type is never renamed to avoid the collision, **Job** and **task** are not synonyms. A background task is the live, in-memory unit the tray shows. A job is its persisted record on disk, written by the `JobJournal` port so an interrupted task can be resumed after a restart. The schemas are `GenerationJobSchema` and friends in [`shared/domain.ts`](shared/domain.ts), and `GenerationJobType` covers every `TaskType` plus `generate-chapter`, which is the just-in-time single chapter path that never went through background tasks. Use "job" only when you mean the durable record. -**Section** is an intra-chapter slice used for pagination and reader navigation, in `client/lib/split-sections.ts` and `client/features/reader/hooks/useSectionNavigation.ts`. It is never a synonym for Chapter. +The rest of the overloads fit in one table. -**Review** is two unrelated things. `toc_review` is the BookStatus in which a generated TOC is waiting for the reader to approve it. Smart Review is the spaced-repetition flow that re-quizzes a reader on questions they missed, in `client/features/quiz/components/SmartReviewFlow.tsx`. - -**Prompt** and **brief** are different lengths of the same idea. A `prompt` is the reader's short topic request stored on `BookMeta`. A `brief` is the long agentic-generation specification stored per book and consumed by the MCP authoring services. - -**Summary** is either a `ChapterSummary`, which is cross-chapter context fed into generation, or the book-completion summary the reader sees at the end. Qualify which. - -**Reference** is source material saved for agentic generation, as `ReferenceEntry` in [`shared/domain.ts`](shared/domain.ts). It is not a citation. - -**Narration** is chapter Markdown transformed for speech, in [`server/services/markdown-to-narration.ts`](server/services/markdown-to-narration.ts). It is a derived form of the chapter text and not the chapter text itself. - -**Progress** appears three ways, as reading scroll progress, as `TaskProgress` on a background task, and as skill progress on the profile. Always qualify it. - -**Error kind** is `AiErrorKind` in [`shared/responses.ts`](shared/responses.ts), one of `auth-failed`, `rate-limited`, `overloaded`, `timed-out`, `network-failed`, `content-refused`, or `unknown`. It classifies why an AI call failed, which is what decides whether the adapter retries. It is not a status. +| Word | It can mean | How to tell | +|---|---|---| +| **Section** | An intra-chapter slice used for pagination and reader navigation | Never a synonym for Chapter. Lives in `client/lib/split-sections.ts` | +| **Review** | `toc_review`, the BookStatus waiting for TOC approval, or Smart Review, the spaced-repetition re-quiz flow | Unrelated features. Name the one you mean | +| **Prompt** vs **brief** | A `prompt` is the reader's short topic request on `BookMeta`. A `brief` is the long generation spec the MCP authoring services consume | Length and audience | +| **Summary** | `ChapterSummary`, cross-chapter context fed into generation, or the completion summary the reader sees at the end | Qualify which | +| **Reference** | Source material saved for agentic generation, `ReferenceEntry` | Not a citation | +| **Narration** | Chapter Markdown transformed for speech | A derived form, never the chapter text itself | +| **Progress** | Reading scroll progress, `TaskProgress` on a background task, or skill progress on the profile | Always qualify | +| **Error kind** | `AiErrorKind`, why an AI call failed, `auth-failed` through `unknown` | Decides whether the adapter retries. Not a status | ## Numbering diff --git a/e2e/README.md b/e2e/README.md index ee584b6..4b8fb25 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -1,9 +1,10 @@ Up: [ARCHITECTURE.md](../ARCHITECTURE.md) + # End-to-end journeys -Up: [../README.md](../README.md) +This suite drives the real browser UI against the real Fastify server. Only the server's outermost adapters are swapped for fakes. -This suite drives the real browser UI against the real Fastify server, with the server's outermost adapters swapped for fakes. It is the answer to a fair question a cold reader asks about a hexagonal codebase, which is whether the ports are load bearing or decorative. Every journey here boots the production `buildServer` and hands it fakes through the same `overrides` argument Electron uses to hand it a diagram renderer, and it works. +Every journey boots the production `buildServer` and hands it fakes through the same `overrides` argument the Electron shell uses. The design reasoning lives in [design.md](design.md). This file is about using the suite. ## Running it @@ -15,11 +16,12 @@ pnpm e2e create-book # one file pnpm e2e --ui # Playwright's time-travel debugger ``` -The run builds `dist/` first, every time, in `global-setup.ts`. That is one code path rather than two, so a journey can never test yesterday's client. Set `E2E_SKIP_BUILD=1` while iterating on a journey to skip it. +Useful flags. -Set `E2E_VERBOSE=1` to see the Fastify server's log lines. They are suppressed by default because each test boots its own instance and the JSON request log drowns the reporter. +- `E2E_SKIP_BUILD=1` skips the `dist/` build while you iterate. By default `global-setup.ts` rebuilds every run so a journey can never test yesterday's client. +- `E2E_VERBOSE=1` shows the Fastify log lines, which are suppressed because every test boots its own server. -`pnpm test` is a different thing and stays fast. It is the vitest unit and contract suite and it never opens a browser. The one file under `e2e/` that vitest DOES own is `support/scripted-text-generation.test.ts`, because that test needs no browser. +`pnpm test` is a different suite. It is the fast vitest unit and contract run and it never opens a browser. ## What a journey gets @@ -31,81 +33,30 @@ test('...', async ({ page, model, app }) => { /* ... */ }) | Fixture | What it is | |---|---| -| `page` | A browser page already pointed at this test's own server instance, so `page.goto('/')` just works | -| `app` | `{ origin, dataDir, bookDir(id), fastify }`. `app.fastify.inject(...)` gives an API-level assertion without a second HTTP client | -| `model` | The scripted `TextGeneration` this test's server is wired to. Add rules before the action that triggers a call, read `model.requests` after | +| `page` | A browser page already pointed at this test's own server, so `page.goto('/')` just works | +| `app` | `{ origin, dataDir, bookDir(id), fastify }`. Use `app.fastify.inject(...)` for an API-level assertion | +| `model` | The scripted `TextGeneration` this server is wired to. Add rules before acting, read `model.requests` after | -Plus the helpers: +Plus the helpers. - `support/seed.ts` puts books on disk through the real filesystem repository. - `support/journeys/*.ts` are page objects that expose intent, such as `wizard.approveToc()` and `reader.finishChapter()`. -- `fixtures/*.ts` are the content the scripted model returns, as TypeScript so `tsc` catches drift. +- `fixtures/*.ts` hold the content the scripted model returns, written as TypeScript so `tsc` catches drift. ## Adding a journey -1. Add a `*.spec.ts` under `e2e/journeys/`, importing `test` and `expect` from `../support/app.js`, never from `@playwright/test`. The re-export is what carries the fixtures. -2. Get to your subject the cheap way. If the journey is about exporting a book, `seedBook(app.dataDir, ...)` rather than driving the whole creation wizard, so a wizard regression fails the wizard journey and not yours. -3. Address the UI the way a screen reader does. `getByRole` first, `getByText` second, and never a CSS class. -4. Assert with `await expect(locator)...`, which retries to a deadline. Never sleep. -5. If your journey needs a model call nobody has scripted yet, add a rule to `support/default-script.ts` matching a phrase lifted verbatim from the server prompt that produces it, and put the content in `fixtures/`. - -## Why fakes rather than mocks - -The server is faked at its ports, not mocked at its HTTP boundary. Everything between the browser and the AI provider is the real thing, meaning real routes, real services, real domain rules, real YAML on a real disk. Only the four edges that would cost money, need a network, or need a 90MB binary are swapped, and they are swapped through `buildServer(overrides)`, the same seam production already has. - -Mocking `fetch` in the browser instead would have tested the client against a fiction of the server. This tests it against the server. - -Zero live-provider traffic is a property of the design rather than a promise. The real key vault is never constructed, the fake one lives only in memory, and the scripted model has no SDK import and throws on any call nobody wrote a fixture for. There is no path from a journey to a provider even if someone wanted one. - -## Why the model answers by intent - -`support/scripted-text-generation.ts` matches on the shape of a request, not on the order calls arrive in. Phase 2's `createFakeTextGeneration` answers strictly first-in first-out, which is right for a unit test that knows exactly how many calls the code under test makes. A journey does not know that. Approving a table of contents fires a skill classification, a chapter stream, and a quiz, in an order the UI decides and may reorder tomorrow, and a fixture list coupled to that order is the largest available source of flake. - -Rules are consulted newest first, so a test shadows a default with one line, which is also how failure injection works: - -```ts -model.onStreamText({ - name: 'chapter 2 is rate limited', - match: isChapterStreamFor(2), - respond: { throws: new Error('rate limit exceeded') }, -}) -``` - -`throws` takes any value and rethrows it unchanged, so a typed error class survives all the way to the caller. - -The fidelity guarantee is `support/scripted-text-generation.test.ts`, which runs the port's own contract suite against this adapter with no exemptions, plus `req.schema.parse(value)` on every generated object so a drifted fixture fails as loudly as a malformed model response would. - -## Why no test ids - -There is not one `data-testid` in the client, and adding one would be the easy way out of every hard locator in this suite. The rule holds because addressing the UI by role and by name means the suite doubles as an accessibility net. Writing these journeys found seven real gaps, all filed as issue 51 rather than papered over with a test id. - -1. A book card is a `div` with an `onClick`, so it has no role, no accessible name, and no focus. A keyboard user cannot open a book at all. This is the serious one. -2. Two controls share the name "Next section", the chapter rail's and the tap zone's, both wired to the same callback. -3. The book context menu is a plain `div` of buttons with no `menu` or `menuitem` semantics. -4. The feedback form's textareas are not label-associated, so neither has an accessible name. -5. The Rename dialog's Title and Subtitle labels are not wired to their inputs. -6. The Delete dialog's confirmation input has no label. -7. The library toolbar's icon-only search and view toggles carry a `title` attribute but no `aria-label`. - -The one sanctioned exception is a hidden ``, which has no role and no accessible name by construction. It is commented where it appears. - -## Flake policy - -`retries: 0`, and that is the most load-bearing line in `playwright.config.ts`. The server is in process, the adapters are fakes, and nothing touches a network, so there is no legitimate source of nondeterminism left. A retry could only convert a real bug into a green run. A journey that flakes twice gets quarantined with `test.fixme` and an issue, never a blanket retry. - -Not hiding nondeterminism is only half the policy. The other half is going looking for it before CI does. Before trusting a green run on a change that touches timing, `pnpm e2e --project=web --repeat-each=4 --workers=4` oversubscribes the worker count past what the machine can actually run at once, which starves the server side of CPU the same way a busy CI runner does. Repeating a test on an idle machine only repeats the same fast timing, so it proves nothing a single run did not already prove. Oversubscription is the part that manufactures a slow, contended server and gives a timing bug the chance to appear on a laptop instead of only in CI. - -Some races need a wider window than contention alone reliably opens. Give a scripted stream's response a `chunkDelayMs` and the gap between the last chunk reaching the client and the server's own later write widens on purpose, turning a rare write-after-signal race into one every run reproduces. Reach for that before blaming CI infrastructure. A bug that only appears when the window is wide has not gone away when the window is narrow again. It has only gone back to being rare. - -Two assertions are quarantined today, and neither is flake. Both are blocked on issue 50, a real bug this suite found in chapter generation's event stream, and each `test.fixme` names it. They flip green when it is fixed. - -One real source of nondeterminism does exist inside the product and journeys have to respect it. `server/services/generate-quiz.ts` shuffles a quiz's options with `Math.random()` before saving, so a quiz option must be located by its text and never by its position. +1. Add a `*.spec.ts` under `e2e/journeys/`. Import `test` and `expect` from `../support/app.js`, never from `@playwright/test`. The re-export carries the fixtures. +2. Get to your subject the cheap way. If the journey is about exporting a book, seed one with `seedBook(app.dataDir, ...)` instead of driving the whole creation wizard. A wizard regression should fail the wizard journey, not yours. +3. Address the UI the way a screen reader does. `getByRole` first, `getByText` second, never a CSS class. +4. Assert with `await expect(locator)`, which retries to a deadline. Never sleep. +5. If your journey needs a model call nobody scripted yet, add a rule to `support/default-script.ts` that matches a phrase lifted verbatim from the server prompt, and put the content in `fixtures/`. -## Projects +## The rules that keep it honest -| Project | Selects | Runs where | -|---|---|---| -| `web` | everything not tagged `@electron` | the `e2e-web` CI job, Chromium, headless | -| `electron` | tests tagged `@electron` | the `e2e-electron` CI job, which is the only one that downloads the Electron binary | +- `retries: 0`. Everything is in process and faked at the edges, so there is no legitimate source of nondeterminism. A retry could only hide a real bug. A journey that flakes twice gets `test.fixme` and an issue. +- Before trusting a green run on a timing-sensitive change, run `pnpm e2e --project=web --repeat-each=4 --workers=4`. Oversubscribed workers starve the server of CPU the way a busy CI runner does. Repetition on an idle machine proves nothing new. +- To reproduce a suspected race deterministically, give a scripted stream a `chunkDelayMs`. Widening the gap between the UI signal and the server's later write turns a rare race into one that fires every run. +- Quiz options are shuffled with `Math.random()` at save time, so locate an option by its text, never by its position. +- No `data-testid`, anywhere. Why that rule exists, and what it caught, is in [design.md](design.md). -The Electron project exists for one thing that the web project cannot reach at all. In the browser redux-persist writes to localStorage, and in the packaged app it writes through IPC to a file the main process owns, so the rehydration path is only observable with a real main process. Fakes cannot be injected into a packaged main process, so that journey seeds its library rather than generating one. +Related: [design.md](design.md), [../server/ports/README.md](../server/ports/README.md) diff --git a/e2e/design.md b/e2e/design.md new file mode 100644 index 0000000..6c1bc22 --- /dev/null +++ b/e2e/design.md @@ -0,0 +1,59 @@ +Up: [README.md](README.md) + +# Why the suite is built this way + +Five decisions shape this suite. Each one traded something away, and this file says what. + +## Fakes at the ports, not mocks at HTTP + +The server is faked at its ports. Everything between the browser and the AI provider is real, meaning real routes, real services, real domain rules, real YAML on a real disk. Only the four edges that would cost money, need a network, or need a 90MB binary are swapped, through `buildServer(overrides)`, the same seam production uses. + +Mocking `fetch` in the browser would have tested the client against a fiction of the server. This tests it against the server. + +No journey can reach a live provider. The real key vault is never constructed, the fake one lives only in memory, and the scripted model has no SDK import and throws on any call nobody wrote a fixture for. + +## The model answers by intent, not by order + +`support/scripted-text-generation.ts` matches on the shape of a request. The unit-test fake answers first-in first-out instead, which is right when the test knows exactly how many calls the code makes. A journey does not know that. Approving a table of contents fires a skill classification, a chapter stream, and a quiz in an order the UI decides and may reorder tomorrow. A fixture list coupled to that order would be the suite's largest source of flake. + +Rules are consulted newest first, so one line shadows a default. Failure injection works the same way. + +```ts +model.onStreamText({ + name: 'chapter 2 is rate limited', + match: isChapterStreamFor(2), + respond: { throws: new Error('rate limit exceeded') }, +}) +``` + +`throws` rethrows its value unchanged, so a typed error class survives to the caller. + +Two checks keep the scripted model honest. It passes the port's own contract suite in `support/scripted-text-generation.test.ts`, and every generated object goes through `req.schema.parse(value)`, so a drifted fixture fails as loudly as a malformed model response would. + +## No test ids + +There is not one `data-testid` in the client. Journeys address the UI by role and by name, which means the suite doubles as an accessibility check. + +Writing it that way found seven real gaps, filed as issue 51. The worst was a book card rendered as a `div` with an `onClick`, no role, no name, and no focus, so a keyboard user could not open a book at all. Six of the seven are fixed. The context menu keeps its plain buttons deliberately, because an explicit `menuitem` role would replace the `button` role these journeys and a pinned test comment rely on. + +The one sanctioned exception is a hidden ``, which has no role or name by construction. It is commented where it appears. + +## Zero retries + +The flake policy has two halves. + +- Never hide nondeterminism. `retries: 0` in `playwright.config.ts`, and quarantine with `test.fixme` plus an issue instead of retrying. +- Go looking for it before CI does. The oversubscription stress run and the `chunkDelayMs` window-widening technique are both described in [README.md](README.md). + +The policy has been tested for real. The suite found a production SSE bug on its first day (issue 50, an event stream that closed before its first byte), and a post-merge timing race in one of its own tests. Both were fixed at the root. Neither got a retry. + +## Two projects + +| Project | Selects | Runs where | +|---|---|---| +| `web` | everything not tagged `@electron` | the `e2e-web` CI job, headless Chromium | +| `electron` | tests tagged `@electron` | the `e2e-electron` CI job, the only one that downloads the Electron binary | + +The Electron project exists for one path the web project cannot reach. In a browser, redux-persist writes to localStorage. In the packaged app it writes through IPC to a file the main process owns, so rehydration is only observable with a real main process. Fakes cannot be injected into a packaged main process, which is why that journey seeds its library on disk instead of generating one. + +Related: [README.md](README.md), [../docs/adr/0005-ai-sdk-behind-a-port.md](../docs/adr/0005-ai-sdk-behind-a-port.md) From a3aaed8ef7ce51b0c615b69875b0dba26d2ff823 Mon Sep 17 00:00:00 2001 From: Ross Miller Date: Wed, 22 Jul 2026 22:06:08 -0500 Subject: [PATCH 2/5] docs: plain language pass on CLAUDE.md, CONTRIBUTING, and the routes doc note CLAUDE.md keeps every fact and loses every dash and sentence colon, with the dense packaging bullet split in two. CONTRIBUTING drops the boilerplate gush and states the policy in four lines. The OpenAPI note in the generator template breaks into bullets so every regeneration inherits the readable form. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5 --- CLAUDE.md | 105 +++++++++++++++++---------------- CONTRIBUTING.md | 21 +++---- docs/api-routes.md | 9 ++- scripts/generate-routes-doc.ts | 9 ++- 4 files changed, 77 insertions(+), 67 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 60a2486..99547c9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,6 +1,6 @@ # Tutor — Adaptive Learning Library -AI-generated books tailored to your learning style. Books are generated chapter-by-chapter with a feedback loop: after each chapter, quiz questions and feedback shape how subsequent chapters are generated. The book literally rewrites itself based on how you're learning. +AI-generated books tailored to your learning style. Books are generated chapter by chapter with a feedback loop. After each chapter, quiz results and feedback shape how the next one is generated. The book literally rewrites itself based on how you're learning. ## Start here @@ -15,14 +15,14 @@ AI-generated books tailored to your learning style. Books are generated chapter- ## How It Works -1. **Create a book** — Enter a topic + prompt, AI generates a table of contents -2. **Approve the TOC** — Review, edit, reorder chapters, then approve -3. **Read chapter-by-chapter** — Quick, digestible chapters (~1,500 words, 5-10 min read) teaching specific concepts -4. **Inline chat** — Select any text to slide out a chat panel for deeper AI explanation, then return to where you left off -5. **Feedback** — After finishing a chapter, give feedback on what resonated/didn't -6. **Generation triggered** — Submitting feedback triggers next chapter generation in the background -7. **Quiz while waiting** — Optional 3-question quiz to test retention and aid memory while next chapter generates -8. **Adaptive** — Next chapter incorporates feedback + quiz results (wrong answers trigger brief recap at start) +1. **Create a book.** Enter a topic and a prompt, and the AI generates a table of contents. +2. **Approve the TOC.** Review, edit, and reorder chapters, then approve. +3. **Read chapter by chapter.** Quick, digestible chapters, roughly 1,500 words and a 5 to 10 minute read, each teaching specific concepts. +4. **Inline chat.** Select any text to slide out a chat panel for a deeper AI explanation, then return to where you left off. +5. **Feedback.** After finishing a chapter, say what resonated and what didn't. +6. **Generation triggered.** Submitting feedback starts the next chapter generating in the background. +7. **Quiz while waiting.** An optional 3-question quiz tests retention while the next chapter generates. +8. **Adaptive.** The next chapter incorporates the feedback and quiz results, and a wrong answer triggers a brief recap at its start. ## Repo layout @@ -62,18 +62,18 @@ docs/ ADRs, generated routes, plans → docs/adr/README.md ## Key Design Decisions -- **Single-user app** — No auth, no concurrency concerns -- **Chapter length** — ~1,500 words (5-10 min), flex longer when content demands -- **TOC approval** — Step-by-step wizard before generation begins -- **Progress tracking** — Scroll-based auto-tracking (completed at ≥90%) -- **Generation flow** — Just-in-time: one chapter at a time, quiz masks latency ([ADR 0002](docs/adr/0002-just-in-time-chapter-generation.md)) -- **Background work** — A `BackgroundTask` in memory, journalled to disk by the `JobJournal` port so an interrupted one can resume at the next boot ([ADR 0008](docs/adr/0008-persisted-job-journal.md)) -- **If the app restarts mid-generation** — `generate-all` and audiobook jobs resume from disk without redoing finished work; a single interrupted chapter surfaces in the reader's retry panel -- **Inline chat** — Select any text to open a slide-out panel for AI-powered deeper explanation; dismissing returns to reading position +- **Single-user app.** No auth and no concurrency concerns. +- **Chapter length.** Roughly 1,500 words, a 5 to 10 minute read, flexing longer when the content demands it. +- **TOC approval.** A step-by-step wizard runs before any generation begins. +- **Progress tracking.** Scroll-based auto-tracking, with a chapter completed at 90 percent or more. +- **Generation flow.** Just in time, one chapter at a time, with the quiz masking the latency ([ADR 0002](docs/adr/0002-just-in-time-chapter-generation.md)). +- **Background work.** A `BackgroundTask` lives in memory and is journalled to disk by the `JobJournal` port, so an interrupted one can resume at the next boot ([ADR 0008](docs/adr/0008-persisted-job-journal.md)). +- **Restart mid-generation.** `generate-all` and audiobook jobs resume from disk without redoing finished work. A single interrupted chapter surfaces in the reader's retry panel. +- **Inline chat.** Selecting text opens a slide-out panel for a deeper AI explanation, and dismissing it returns to the reading position. ## Electron Packaging -This is an Electron app using `vite-plugin-electron`. Three modes exist with different behaviors: +This is an Electron app using `vite-plugin-electron`. Three modes exist with different behaviors. | Mode | Command | Renderer loads from | API routing | Origin header | |------|---------|--------------------|--------------|----| @@ -83,11 +83,12 @@ This is an Electron app using `vite-plugin-electron`. Three modes exist with dif ### Critical conventions -- **Address**: Always use `127.0.0.1` (not `localhost`) for server communication — avoids IPv6 mismatch on macOS -- **CORS**: Server must accept `Origin: null` (file:// protocol) and any `localhost`/`127.0.0.1` origin — enforced in `server/index.ts:isAllowedOrigin()` -- **CSP**: Both `index.html` meta tag and `electron/main.ts` header must allow `http://localhost:*` AND `http://127.0.0.1:*` in `connect-src` -- **pnpm + electron-builder**: `.npmrc` requires `node-linker=hoisted`. The electron build bundles the unified/remark/rehype ecosystem into `dist-electron/` (via rollup) since electron-builder can't resolve their deep transitive deps. The bundle list is in `vite.config.ts` `external()`. CJS packages (fastify, etc.) stay external — if a CJS transitive dep is missing, add it to `package.json` `dependencies` (e.g., `json-schema-ref-resolver` for fastify). For CJS packages imported dynamically, handle the double-default: `mod.default?.default ?? mod.default` (see `epub-gen-memory` import in `server/adapters/epub-gen-export.ts`). -- **Never modify `index.html` or `package.json` to match build output** — `dist/` is the build target, source files must keep source references (`/client/app/main.tsx`) +- **Address.** Always use `127.0.0.1`, never `localhost`, for server communication. This avoids an IPv6 mismatch on macOS. +- **CORS.** The server must accept `Origin: null` (the file protocol) and any `localhost` or `127.0.0.1` origin. Enforced in `server/index.ts:isAllowedOrigin()`. +- **CSP.** Both the `index.html` meta tag and the `electron/main.ts` header must allow `http://localhost:*` and `http://127.0.0.1:*` in `connect-src`. +- **pnpm + electron-builder.** `.npmrc` requires `node-linker=hoisted`. The electron build bundles the unified, remark, and rehype ecosystem into `dist-electron/` via rollup, because electron-builder cannot resolve their deep transitive deps. The bundle list is in `vite.config.ts` `external()`. +- **CJS packages stay external.** If a CJS transitive dep is missing, add it to `package.json` `dependencies`, the way `json-schema-ref-resolver` is there for fastify. A CJS package imported dynamically needs the double-default dance, `mod.default?.default ?? mod.default`, as in `server/adapters/epub-gen-export.ts`. +- **Never modify `index.html` or `package.json` to match build output.** `dist/` is the build target, and source files must keep source references (`/client/app/main.tsx`). ## Development @@ -106,47 +107,47 @@ pnpm mcp:dev # MCP server, needs dev:server on 3147 (see .mcp.json) ## Conventions -- Zod domain schemas live in `shared/`, the single source of truth for both sides -- YAML for all metadata, Markdown for chapter content -- Vercel AI SDK is reached only through the `TextGeneration` port, never imported outside `server/adapters/` -- Tests colocated with source files (`*.test.ts`) -- **TDD** — tests land before or with implementation, visible in commit order. Contract test before adapter, service test before service, api-client test before the client function -- Path aliases: `@client/*` → `client/*`, `@server/*` → `server/*`, `@shared/*` → `shared/*` -- Domain names come from `CONTEXT.md`. Do not invent a synonym for a word that already has an owner +- Zod domain schemas live in `shared/`, the single source of truth for both sides. +- YAML for all metadata, Markdown for chapter content. +- The Vercel AI SDK is reached only through the `TextGeneration` port and never imported outside `server/adapters/`. +- Tests are colocated with source files (`*.test.ts`). +- **TDD.** Tests land before or with implementation, visible in commit order. Contract test before adapter, service test before service, api-client test before the client function. +- Path aliases: `@client/*` → `client/*`, `@server/*` → `server/*`, `@shared/*` → `shared/*`. +- Domain names come from `CONTEXT.md`. Do not invent a synonym for a word that already has an owner. ## Domain & Architecture These are the rules new and refactored code follows. The server conforms today. The client conforms on the api boundary and is still converging elsewhere. -- **Ubiquitous domain language**: `Book`, `Chapter`, `TOC`, `Feedback`, `Quiz`, `Progress`, `LearningProfile`, `Audiobook`, `BackgroundTask`. Use these names everywhere — schemas, services, components, prompts, UI copy. `CONTEXT.md` is the register -- **Pure domain core** in `shared/` and `server/domain/` — Zod types and pure functions only. No `fs`, `fetch`, AI SDK imports, or env vars inside the domain -- **Ports for every external dependency**: each gets a single named module the rest of the app depends on by shape, not by SDK. Every port ships an in-memory fake and a contract test -- **Adapters do the I/O**: only the adapter touches the SDK, library, child process, or filesystem. Swappable and testable in isolation -- **Routes are thin**: parse input → call a service → return result. No business logic, no direct `fs` or SDK calls in `server/routes/*.ts` -- **Frontend goes through one client**: components import from `client/api/`, not raw `fetch`. A raw `fetch` or `new EventSource` outside `client/api/` is an ESLint error. New endpoints get a function in the client -- **No new SDK sprinkling**: when adding a third-party SDK, wrap it behind a port first, then consume the port from services +- **Ubiquitous domain language.** `Book`, `Chapter`, `TOC`, `Feedback`, `Quiz`, `Progress`, `LearningProfile`, `Audiobook`, `BackgroundTask`. Use these names everywhere, in schemas, services, components, prompts, and UI copy. `CONTEXT.md` is the register. +- **Pure domain core** in `shared/` and `server/domain/`. Zod types and pure functions only. No `fs`, `fetch`, AI SDK imports, or env vars inside the domain. +- **Ports for every external dependency.** Each gets a single named module the rest of the app depends on by shape, not by SDK. Every port ships an in-memory fake and a contract test. +- **Adapters do the I/O.** Only the adapter touches the SDK, library, child process, or filesystem. Swappable and testable in isolation. +- **Routes are thin.** Parse input, call a service, return the result. No business logic and no direct `fs` or SDK calls in `server/routes/*.ts`. +- **The frontend goes through one client.** Components import from `client/api/`, and a raw `fetch` or `new EventSource` outside it is an ESLint error. A new endpoint gets a function in the client. +- **No new SDK sprinkling.** When adding a third-party SDK, wrap it behind a port first, then consume the port from services. ## UI / Frontend Design ### Desktop-First Design -- Optimize for 1280–2560px desktop resolutions — no mobile-first layouts -- Keyboard-first navigation with shortcuts everywhere (use `lucide-react` icons + ``) -- Horizontal layouts: sidebars, resizable panels (shadcn `ResizablePanelGroup`), command palettes (`cmdk`) -- Fluid typography/spacing for large screens: `text-3xl` → `text-4xl` on `lg`, `container mx-auto max-w-7xl` -- Minimum layout target: `min-width: 1024px` +- Optimize for 1280 to 2560px desktop resolutions, no mobile-first layouts. +- Keyboard-first navigation with shortcuts everywhere (use `lucide-react` icons + ``). +- Horizontal layouts: sidebars, resizable panels (shadcn `ResizablePanelGroup`), command palettes. +- Fluid typography and spacing for large screens: `text-3xl` → `text-4xl` on `lg`, `container mx-auto max-w-7xl`. +- Minimum layout target: `min-width: 1024px`. ### shadcn/ui + Tailwind v4 -- Use CVA for component variants, `cn()` for class merging, CSS variables from theme -- Respect system dark mode via `prefers-color-scheme` + manual toggle with shadcn theme provider -- Build using composition: small shadcn-extended primitives first (e.g., ``, ``), then compose pages -- Use CVA to add custom variants (e.g., button with `glass`, `command`) -- Keep logic out of UI files — prefer hooks + context +- Use CVA for component variants, `cn()` for class merging, CSS variables from theme. +- Respect system dark mode via `prefers-color-scheme` plus a manual toggle with the shadcn theme provider. +- Build by composition, small shadcn-extended primitives first, then compose pages. +- Use CVA to add custom variants, such as a button with `glass` or `command`. +- Keep logic out of UI files. Prefer hooks and context. ### Page Layout Patterns -- **Header**: Centered title only, draggable region (`-webkit-app-region: drag`), no navigation buttons inside header -- **Back button**: Absolute-positioned overlay on the content area below the header — `absolute left-6 top-3 z-20` on a plain `