Repository navigation
fix(coach): batch 1 — post-review P1 reliability fixes - #27
Merged
Merged
Conversation
A planner regression or out-of-range lesson data throws inside useStore at render time, and nanostores' computed does not catch it — so any such throw blanks the entire SPA. Add a react-error-boundary around the Coach route with a French recovery fallback (Réessayer + Retour au Coach) that resets on lesson change, mirroring the Solver's $solveError UI. No class component (house rule). Refs #13
resolveRecipe silently returned an empty recipe when a teaching demo's groupIndex exceeded the plan's group count — a 0-move demo with no animation, no steps, and no way out. Throw a descriptive error instead; the Coach ErrorBoundary now turns that into a recoverable state. Export runTeachingPlan and assert in lessons.spec that every demo groupIndex addresses a real group, so the throw can never fire from shipped data. Refs #12
…pter On a lessonId change, the completion effect and the startLesson effect re-run in the same commit with the previous chapter's step index still captured in the closure. If that index reached the incoming (shorter) chapter's last step, the new chapter was marked complete before its first view (e.g. white-corners step 7 to finish, whose last index is 2). Guard the effect on the live store: bail unless $currentLessonId matches, and read $lessonStepIndex live — startLesson runs first and resets both. Add a regression test for the failing pair. Refs #14
compute() ran all five teaching planners with no guard and memoised the result; if it threw, the cache stayed null and every milestones() call retried and rethrew — permanently breaking the Coach route until a page refresh. Wrap the computation in try/catch returning solved-cube fallback milestones (cached, so no rethrow loop): a planner regression degrades the illustrative visuals instead of crashing the route. Refs #15
There was a problem hiding this comment.
Pull request overview
This PR applies a focused set of P1 Coach reliability/correctness fixes (from the prior Coach v1 review), ensuring planner/data errors fail loudly but remain recoverable, and addressing a lesson-navigation race that could incorrectly auto-complete chapters.
Changes:
- Add a Coach-scoped React error boundary (with a French recovery UI) and reset-on-navigation behavior.
- Enforce
groupIndexvalidity for teaching demos (throw on out-of-range at runtime; add a registry test to prevent shipping invalid data). - Fix a
lessonIdchange race inLessonPlayerthat could mark shorter incoming chapters complete; add a regression test. - Harden
illustrativemilestone computation with a safe fallback to solved-cube milestones on planner/solver regressions.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| bun.lock | Locks react-error-boundary dependency. |
| apps/web/package.json | Adds react-error-boundary to web dependencies. |
| apps/web/src/pages/Coach.tsx | Wraps Coach route content in CoachErrorBoundary. |
| apps/web/src/features/coach/index.ts | Re-exports CoachErrorBoundary from the feature entrypoint. |
| apps/web/src/features/coach/components/index.ts | Exposes the new CoachErrorBoundary component. |
| apps/web/src/features/coach/components/CoachErrorBoundary.tsx | Introduces Coach-specific error boundary + fallback recovery UI. |
| apps/web/src/features/coach/components/CoachErrorBoundary.spec.tsx | Tests fallback rendering and reset behavior on navigation. |
| apps/web/src/features/coach/stores/coachStore.ts | Exports runTeachingPlan; throws on out-of-range teaching demo groupIndex. |
| apps/web/src/features/coach/data/lessons.spec.ts | Adds integrity test asserting all teaching demo groupIndex values are in-range. |
| apps/web/src/features/coach/components/LessonPlayer.tsx | Guards completion effect against stale step index during lessonId changes. |
| apps/web/src/features/coach/components/LessonPlayer.spec.tsx | Adds regression test for the stale-step-index auto-completion bug. |
| apps/web/src/features/coach/data/illustrative.ts | Wraps milestone computation in try/catch with solved-cube fallback to avoid permanent crash loops. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced Jun 26, 2026
TituxMetal
added a commit
that referenced
this pull request
Jun 27, 2026
Record the batch 1 reliability cluster (PR #27): nanostores computed throwing at render blanks the SPA without a boundary, out-of-range groupIndex must fail loud not silent, an effect keyed on a routing prop must read the store live to dodge a stale-closure race, and a memoised compute must guard against a permanent rethrow loop. Symptom -> root cause -> fix -> prevention for the next encounter.
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
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.
What
Batch 1 of the Coach v1 post-review issues — the four P1 reliability/correctness findings from the
ce-code-reviewon PR #11. Each is a small, self-contained fix with a test where the path is reachable.resolveRecipesilently returned an empty recipe for an out-of-range demogroupIndex(0-move demo, no way out). Now throws a descriptive error;lessons.specasserts every demogroupIndexaddresses a real group, so it can't fire from shipped data.react-error-boundaryaround the Coach route with a French recovery fallback that resets on lesson change.lessonIdchange re-ran the completion effect with the previous chapter's stale step index, auto-completing a shorter incoming chapter before its first view. Guarded on the live store.illustrative.compute()ran the planners unguarded and memoised the result; a throw left the cache null and rethrew forever. Now degrades to solved-cube milestones on failure.Design note — #12 + #13 coexist deliberately
The two findings appear to conflict (#12 wants a throw, #13 suggested swallowing throws into
EMPTY_RECIPE). Swallowing would reintroduce the silent dead-end #12 fights. Instead: the guard throws loudly, and the ErrorBoundary catches it at render and offers recovery — loud and recoverable, no silent empty cube.Verification
format:check,lint:check,typecheck,test(296 pass),build— all green locally.Closes #12, #13, #14, #15