Repository navigation
test(coach): guard the run-end snapshot → curPw seam (v0.2.6) - #20
Merged
Merged
Conversation
Closes the test gap that let the v0.2.5 coach bug ship green: smoke-coach-rx
only exercised the PURE coachRxFor with a hand-built ctx (explicit curPw), so
it never covered the _lastEndedSession snapshot construction that dropped
pathway_id/practice_type.
New smoke section drives a REAL >2s pathway run (select rung → Play → Stop),
then asserts against the actual seam:
- _lastEndedSession carries mode==='pathway', pathway_id (=== the run's rung),
and a truthy practice_type;
- the REAL buildCoachRx, fed that snapshot + a finger fault, does NOT
re-prescribe the just-run rung and falls to its sibling (fs_spider_adjacent).
Verified the guard bites: re-dropping pathway_id from the snapshot turns the
new asserts red (behavioral one re-prescribes chromatic_warmup, the exact bug),
green once restored.
Exposes buildCoachRx + lastEndedSession on the existing window.__virtuosoCoach
debug hook (inert for users; test-only surface). No user-facing behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i
📝 WalkthroughWalkthroughThe plugin version is updated to 0.2.6, the coach debug hook exposes the latest ended session, and a smoke test validates pathway session snapshots and follow-up prescription selection. ChangesCoach prescription regression
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)screen.jsast-grep timed out on this file Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/run-virtuoso/smoke-coach-rx.mjs:
- Around line 160-164: Guard the stop sequence after the `#virtuoso-play` startup
in the smoke test using the started result. Only wait for the duration gate and
click `#virtuoso-stop` when startup succeeds; otherwise fail fast with a focused
startup failure while preserving the existing ok assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c4c43ef-a130-44d7-acd9-e977dea04525
📒 Files selected for processing (3)
.claude/skills/run-virtuoso/smoke-coach-rx.mjsplugin.jsonscreen.js
Comment on lines
+160
to
+164
| await page.click("#virtuoso-play"); | ||
| const started = await page.waitForSelector("#virtuoso-stop:not([disabled])", { timeout: 8000 }).then(() => true).catch(() => false); | ||
| ok(started, "real pathway run started (stop button armed)"); | ||
| await page.waitForTimeout(4200); // clear the 2s duration gate even with a count-in | ||
| await page.click("#virtuoso-stop"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the stop sequence when startup fails.
ok(started, ...) records the failure, but execution still clicks Stop. If startup times out, that action can fail separately and obscure the actual root cause. Guard the wait/stop sequence with started or fail fast with a focused startup error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/run-virtuoso/smoke-coach-rx.mjs around lines 160 - 164, Guard
the stop sequence after the `#virtuoso-play` startup in the smoke test using the
started result. Only wait for the duration gate and click `#virtuoso-stop` when
startup succeeds; otherwise fail fast with a focused startup failure while
preserving the existing ok assertion.
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.
Why
The v0.2.5 coach bug (
_lastEndedSessionsnapshot droppedpathway_id/practice_type→ "Practice next" re-prescribed the just-run rung) shipped green becausesmoke-coach-rxonly exercised the purecoachRxForwith a hand-builtctx(explicitcurPw) — it never covered the snapshot construction that broke. This closes that gap.What
A new smoke section drives a real >2s pathway run (select rung → Play → Stop) and asserts against the actual seam:
_lastEndedSessioncarriesmode==='pathway',pathway_id(=== the run's rung), and a truthypractice_type;buildCoachRx, fed that snapshot + a finger fault, does not re-prescribe the just-run rung and falls to its sibling (fs_spider_adjacent).Exposes
buildCoachRx+lastEndedSessionon the existingwindow.__virtuosoCoachdebug hook (test-only surface, inert for users — no behavior change).Proof the guard bites
Re-dropping
pathway_idfrom the snapshot turns the new asserts red — the behavioral one re-prescribeschromatic_warmup(the exact bug) — and green once restored:Full suite green with the fix in place.
node --checkclean.🤖 Generated with Claude Code
Summary by CodeRabbit