Phase 5 — Final sweep and demo readiness - #58
Merged
Conversation
… state the forwarder convention The generate-all poll and the capture settle delay become named constants. Two adapter docblocks stop narrating where their code came from and keep only the constraints. The two pure-forward services gain a comment stating they exist to keep the route-service-port layering uniform, answering the review sweep rather than removing them. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
…section The README had no status signals at the top, so add three badges for CI, license, and the latest release, using shields.io endpoints backed by the real workflow, license, and release data. Move the ARCHITECTURE.md pointer up to sit directly under the badges and state plainly that it is the entry point for technical readers, ahead of CONTEXT.md and docs/adr/. Add an Architecture section after Features with one small mermaid diagram showing the client to server request path in six nodes, real folder names only, plus a link to ARCHITECTURE.md for the full five-diagram hub. Refresh the Tech Stack table and Development commands against the current package.json and folder layout, since Playwright e2e tests exist but were not mentioned anywhere in the README. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
BookCard rendered as a plain div with onClick and onContextMenu, no
role, no tab stop, and no accessible name, so a keyboard user could not
open a book at all. This is issue 51 item 1, the most severe of the
seven accessibility gaps the Phase 6 journey suite surfaced.
The card now carries role="button", a tabIndex that follows the same
isGenerating gate the click handler already uses, an aria-label taken
from the title, and a keydown handler that activates on Enter or Space
the same way a native button would. The card stays a div rather than a
real button element on purpose, since its cover art, badges, and the
readonly StarRating it nests are easier to reason about without also
managing a native button's default styling and its interaction with
form semantics.
The rename journey's own locator collided with this fix in a way worth
recording. That fixture seeds a book titled "Rename Me", and once the
card exposes an accessible name, getByRole("button", { name: "Rename" })
matches both the card and the actual "Rename" menu item, because
Playwright's default name matching is a substring match and "Rename Me"
contains "Rename". The test now asks for an exact match, which is the
smallest change that keeps its original intent of clicking the menu
item rather than the card.
Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
ChapterRail and SectionTapZones both rendered a button named "Next
section", and another pair both named "Previous section", wired to the
same goNext and goPrev callbacks. This is issue 51 item 2. Assistive
technology announced the same control twice, and Playwright's strict
mode rejected either name outright until a journey disambiguated them.
The tap zones are the redundant pair here, not the rail. They sit
invisibly over the far edges of the reading content and only ever
reveal themselves on hover, which a keyboard or screen reader user has
no way to trigger, while the rail's chevrons are always present and
already reachable by tab. The two tap zone buttons now carry
aria-hidden and tabIndex={-1}, pulling them out of the accessibility
tree and the tab order entirely, so the rail's buttons are the only
ones assistive technology or a keyboard user ever reaches.
Their aria-label attributes stay in place even though aria-hidden makes
them inert for real assistive technology, because scripts/find-unnamed-buttons.mts
has no concept of aria-hidden and would otherwise flag an icon-only
button as unnamed. The comment on the component explains this so a
future reader does not mistake it for an oversight.
Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
The "What worked well?" and "What could be better?" labels sat as plain text siblings of their textareas, with no htmlFor and id pair and no aria-label, so neither field had an accessible name. This is issue 51 item 4. A screen reader announced two unlabeled text areas, and the journey suite could only reach them by placeholder text. Each label now carries an htmlFor that matches an id on its textarea, scoped by chapterNum since a future caller could in principle mount more than one of these at once even though today's ReaderBody only ever mounts one at a time. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
Title and Subtitle both lacked htmlFor and a matching id, so getByLabel could not find either field and a screen reader announced two unlabeled text inputs. This is issue 51 item 5. The rename journey worked around it by selecting the dialog's textboxes positionally, Title first and Subtitle second, a workaround its own comment already flagged as temporary. Both labels now carry an htmlFor that matches an id on their input. The ids are static rather than scoped to any per-book value, since RenameBookDialog stays mounted for the lifetime of LibraryDialogs and only one instance ever exists on screen at a time. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
The field where a reader types "delete" to confirm had no label element and no aria-label, so a screen reader announced an unlabeled text input with no indication of what it wanted. This is issue 51 item 6. The input now carries an aria-label rather than a new visible label element, since the instruction to type "delete" already lives in the dialog's description text above it and a visible label would change the dialog's layout for sighted readers for no benefit. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
The search toggle and the grid and list view toggles in LibraryToolbar carried a title attribute, which a browser only ever exposes as a tooltip, not as an accessible name. This is issue 51 item 7. The search journey worked around it by using the ⌘F keyboard shortcut instead of clicking the button. Each of the three now also carries an aria-label. The two view toggles reuse their existing title text verbatim since it already read cleanly on its own. The search toggle gets a shorter "Search" rather than repeating its "Search (⌘F)" tooltip, since the shortcut hint belongs in a tooltip a sighted mouse user hovers to see, not in the name a screen reader announces. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
A grep cannot tell whether a button has an accessible name, because a button holding both an icon and a text label looks identical to a grep as one holding only an icon. This script walks the JSX for every button and Button element under a given root, asks whether it has an aria-label or a title, and if not, asks whether anything in its children could render visible text a screen reader could announce. It exits non-zero when it finds one that has neither, so it can act as a cheap acceptance check for exactly the class of accessibility gap the Phase 6 journeys were the first thing in this codebase to notice. Run it with pnpm tsx scripts/find-unnamed-buttons.mts client. It currently exits clean, zero unnamed buttons found, both before and after this branch's other fixes, since the client's icon-only buttons already carried an aria-label or a title. Its real value is catching the next one that does not. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
client/api/profile.ts declared its own ProfileResponse interface, the same shape shared/responses.ts already exports under the identical name for exactly the reason this module's own removed comment gave, the wire shape folds identity and style into a single aboutMe string and is deliberately not LearningProfile. Two independent declarations of the same wire shape is the drift risk shared/responses.ts exists to close for every other route. profile.ts now imports the shared type and re-exports it under the same name, so every existing caller going through client/api's barrel, or importing directly from profile.ts the way profile.test.ts does, keeps working unchanged. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
The switch over provider in listProviderModels covers every member of ProviderId today, so its default case is unreachable, isProviderId already rejects anything else before the switch runs. Nothing enforced that this stays true. Adding a fourth provider to ProviderSchema without adding a matching case here would fall through to the default branch at runtime and 400 with "Invalid provider" for a provider the rest of the app considers valid, and nothing would fail to compile to catch it. The default case now assigns provider to a local typed never, the same pattern resume-interrupted-jobs.ts already uses for its job type switch. Provider is exhaustively narrowed to never there today, so this compiles clean, but the moment a provider is added without a case, that assignment stops type checking and the build fails instead of the route silently rejecting a real provider. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
When a reference by that name already existed, saveReference replaced
manifest[idx] with a shallow copy of itself, spreading every existing
field back in and then setting name to the value the findIndex above it
already matched on. The result was always value identical to the entry
already there, since name never changes and nothing else in this
function touches url, heading, or tokenEstimate.
The branch now does nothing when the name already exists, which is the
same outcome the spread produced, and still pushes a new { name } entry
for a name that was not found. Behavior is identical either way.
Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
One script proves the whole stack from a bare clone, install, tests, typecheck, lint, knip, both doc checkers, routes drift, a real server boot, the e2e journeys, and a packaged build smoke, all against throwaway data and with notarization disabled so it cannot hang. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
Adds knip as a devDependency with knip.json declaring the entry points across server, electron, client, scripts, and both test suites. Every finding was verified with a repo-wide grep before acting on it, never deleted on knip's word alone. Deleted as genuinely dead code. Four shadcn primitives that only imported each other and nothing else in the app, Command, InputGroup, Textarea, and Tooltip. A handful of Redux selectors and API re-exports with zero consumers anywhere, including the deprecated setChapterPosition wrapper and an unused clearChatHistory action. Several server-side types and a test seam that never got wired into its own test. A stale ProviderSchema re-export in shared/contracts.ts left over from when that schema moved to shared/provider.ts. Unexported rather than deleted where the code is still used, just never outside its own file. This applies to many small interfaces and constants across client/store, server/services, and server/ports. Fixed two barrel-bypassing imports along the way. QuizReviewPage and ChapterBreakdownList imported selectors and a type straight from client/store/quizHistorySelectors.ts and quizHistorySlice.ts instead of through the client/store barrel those modules document as the only door in. Routing them through the barrel is what made the barrel's own re-exports of those names become genuinely used again. Kept and added to knip.json's ignores, each with why. Vendored shadcn/ui primitives keep their full generated API even when today's app only uses part of it. shared/domain.ts and its neighbors are the domain's single source of truth for data shapes, so a schema or type used only to compose another schema in the same file is still part of the published vocabulary. Four client/store state interfaces have no importer of their own but must stay exported because tsconfig's declaration output fails to compile otherwise. FinalQuizBodySchema and SuggestCoverPromptBodySchema deliberately alias AiRequestSchema rather than redeclare it. TASK_CLEANUP_DELAY_MS is the documented value three other files mirror in a hardcoded constant plus a comment, so deleting it would orphan those comments. Removed six dependencies confirmed to have zero references anywhere, @fastify/cors, @hookform/resolvers, cmdk, fluent-ffmpeg, react-hook-form, and rehype-sanitize, plus two devDependencies, @types/diff and @types/fluent-ffmpeg, the latter pair also flagged directly by npm as redundant stub types. Removing fluent-ffmpeg also meant pruning its now-stale entries from package.json's electron builder file list and vite.config.ts's external list, since the actual ffmpeg invocation goes through a downloaded binary and never imports the npm package. Kept @fastify/merge-json-schemas, json-schema-ref-resolver, and onnxruntime-node, all genuinely needed at runtime as transitive dependencies pinned directly for electron builder's packaging step. Added the one dependency knip found used but missing from package.json, @huggingface/transformers, at the version already resolved in the lockfile. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
Closes GitHub issue 52. scripts/diagnose-quiz.ts imported generateQuiz and QUIZ_QUALITY_RULES from server/services/generation-manager.js, which Phase 2 deleted when the generation hub moved to chapter-generation-stream.ts. The script could not run. Step 6 now builds the real TextGeneration adapter and KeyVault the production code path uses, createAiSdkTextGeneration and createFileKeyVault, then calls createGenerateQuiz from the module that actually owns quiz generation today, generate-quiz.ts. Steps 1 through 5 are untouched, since they deliberately call the AI SDK directly to bisect whether a failure is in the SDK or in the app's own wiring, and that design still holds. The verdict text at the end pointed at generation-manager.ts and books.ts, both gone, so it now names the real files and lines, start-book.ts, generate-next-chapter.ts, and assessment.ts. This was fixable in well under thirty minutes, so the script was repointed rather than deleted. Widens the gates so a script like this cannot rot invisibly again. scripts/**/*.ts is now in tsconfig's include and in both lint globs, in place of the two individual filenames Phase 6 added for its own script. This is exactly how the stale import was found in the first place, adding the whole directory to typecheck immediately surfaced the one broken file. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
Walks every committed markdown file, extracts each markdown link
target and each backticked span that looks like a repo path, and
verifies every one exists on disk. Exits non-zero with the full list
of misses otherwise.
A markdown link is checked strictly, since a real link is a deliberate
pointer and CommonMark resolves an implicit relative target, one with
no leading `./`, against the linking file's own directory rather than
the repo root. A backtick span gets an extra gate before it counts as
a path candidate at all. It must contain a slash, and it must either
use an alias this repo recognizes or start with a real top level entry
of the tree, computed from `git ls-files` rather than hardcoded. That
gate is what tells a real path like `server/index.ts` apart from
inline code that only looks like one, an npm scoped package name, or
prose describing a naming convention such as `chapters/NN.md` rather
than pointing at a real file. URLs, bare anchors, brace glob patterns
like `{id}`, and angle bracket template placeholders are all skipped
outright.
docs/plans/refactor/** and server/migrations/__fixtures__/**.md are
excluded from the walk entirely. The refactor plans are an archived
record of a migration that already happened, so they are full of
paths that were true historically and are deliberately not true
today, an old `@src/` alias, a removed `@marp-team/marp-cli`
devDependency, a `server/schemas.ts` that no longer exists.
Hand maintaining a list of which of hundreds of historical mentions
are intentionally stale would cost more than simply not scanning a
folder whose whole purpose is to be a historical record. The fixtures
are fake book chapters used to test a library migration, not
documentation.
Reproduces Phase 4's ad hoc run closely, 290 references across 27
docs today against that pass's 286 across 27, with zero missing
either time.
Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
Nothing references it. It reads docs/todos.yaml, a file that does not exist anywhere in this repo, so the script has been non-functional since this project moved to docs/plans/refactor/ for planning. The only other mention of it anywhere is docs/plans/refactor/phase-5.md's own note to review it with the owner's intent in mind and likely prune it. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
progress.png and reader.png are not linked from README.md or from any other committed doc, verified by grepping every filename in docs/screenshots/ across the whole repo. The other seven, feedback, inline-chat, library, new-book, personalize, quiz, and settings, are all embedded in README.md and stay. This matches docs/plans/refactor/phase-5.md's own note that these two specific files were unreferenced and should be verified and pruned. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
Adds pnpm knip and pnpm tsx scripts/check-doc-paths.ts right after pnpm test, ahead of the generated routes doc check that already runs last on purpose. Also adds a guarded step for scripts/find-unnamed-buttons.mts, which lands from a sibling task and does not exist in this branch yet. The step checks for the file first and only runs the check when it is there, printing a skip line otherwise, so this step is a harmless no-op today and starts enforcing itself automatically the moment that sibling branch merges, with no further edit to this workflow needed. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
The existing retries 0 paragraph is half of the policy, do not hide nondeterminism behind a retry. These two new paragraphs are the other half, go looking for it before CI does. The first names the gate stress run, pnpm e2e --project=web --repeat-each=4 --workers=4, and why it works. Oversubscribing workers past what the machine can run at once starves the server side of CPU the way a busy runner does, while repeating a test on an idle machine only ever repeats the same fast timing and proves nothing new. The second names the widen the window technique, giving a scripted stream's response a chunkDelayMs so the gap between the last chunk reaching the client and the server's own later write opens wide enough to turn a rare write-after-signal race into one every run reproduces. Reach for it before blaming CI infrastructure. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
The three entries knip infers on its own are removed and the scripts glob now matches the mts survey script that landed from the sibling branch. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
One line per root directory, including where the unit tests actually live, so a thirty minute reviewer never has to guess what an entry is for. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
Every other file in server/domain pairs with a test, these two now do as well, covering the documented blunt regex trade-off in sanitizeFeedback and the bucket, truncation, and zero-weight behavior of the skill report. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
This was referenced Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 5 — Final sweep and demo readiness
The closing phase. Dead code removed with evidence, the last sanctioned fixes landed, every repo check promoted into a committed, CI-wired script, and the whole stack proven from a bare clone.
What landed
check-doc-paths.ts(290 path references across 27 docs, zero missing),find-unnamed-buttons.mts(the AST survey, with its false-negative cautionary comment), andverify-fresh-clone.sh.scripts/is now inside the tsconfig and lint gates so it cannot rot invisibly again (issue scripts/diagnose-quiz.ts imports a module the hexagonal restructure deleted #52's failure mode, the script itself is also fixed and re-pointed at the hexagonal services).Deliberately not done, with dispositions
role="menuitem"replaces the button role that six E2E locators and a pinned test comment depend on; labeled buttons in a popover is a legitimate pattern. Attempted, verified breaking, reverted with evidence.Gate evidence
pnpm test1,324 tests in 136 files green.pnpm typecheckclean.pnpm lintzero warnings.pnpm knipexit 0.pnpm e2e --project=web23 passed. Contention stress--repeat-each=4 --workers=492 passed, retries 0.pnpm tsx scripts/check-doc-paths.tsall 290 references exist.find-unnamed-buttonszero unnamed.scripts/verify-fresh-clone.shexit 0 in 174 seconds, bare clone → install → all tests → typecheck → lint → knip → both doc checkers → routes-doc drift → real server boot → full E2E run → signed DMG built with notarization off. Run at 24cf847; the two later commits are docs and hook config, covered by their own checks..bak-v1files untouched.https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5