Remove the workshop scenario selection step from the initial tutorial flow#46763
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46763 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Simplifies workshop onboarding by starting the default scenario immediately after entry-path selection.
Changes:
- Removes scenario-selection UI and defaults to
daily-status. - Updates URL/state handling and Playwright coverage.
- Includes unrelated workflow/status maintenance changes.
Show a summary per file
| File | Description |
|---|---|
docs/tests/workshop.spec.ts |
Updates workshop navigation tests. |
docs/src/content/docs/agent-factory-status.mdx |
Adds Daily VulnHunter status entry. |
docs/src/components/workshop/WorkshopExperience.astro |
Removes scenario picker and updates routing/state. |
.github/workflows/daily-vulnhunter-scan.lock.yml |
Upgrades generated workflow dependencies. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Medium
| const scenarioId = rawScenarioId && manifest.scenarios.find((s) => s.id === rawScenarioId) ? rawScenarioId : manifest.defaults.scenarioId; | ||
| const stepKey = rawStepKey && stepMap.has(rawStepKey) ? rawStepKey : null; | ||
| return { screen: 'tutorial', journeyId, scenarioId, stepKey }; |
| # gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"f6b6a6bae81bf07348dc7b4bf8d6b1f1befb67eb7e7a805e238c72712b903d9b","body_hash":"a48dbec98bfea6f55c1ae6e80414c0759b46e692b7c5712b5c122f80dd909fcd","strict":true,"agent_id":"claude","agent_model":"claude-opus-4.6","engine_versions":{"claude":"2.1.214"}} | ||
| # gh-aw-manifest: {"version":1,"secrets":["ANTHROPIC_API_KEY","COPILOT_GITHUB_TOKEN","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GH_AW_OTEL_GRAFANA_AUTHORIZATION","GH_AW_OTEL_GRAFANA_ENDPOINT","GH_AW_OTEL_SENTRY_AUTHORIZATION","GH_AW_OTEL_SENTRY_ENDPOINT","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0","version":"v7.0.0"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"820762786026740c76f36085b0efc47a31fe5020","version":"v7.0.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.37","digest":"sha256:0d35e8682845f183c1c634699a8e8a6cbe2c271b867031410df74533243c5f67","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.37@sha256:0d35e8682845f183c1c634699a8e8a6cbe2c271b867031410df74533243c5f67"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.37","digest":"sha256:fc2970aadaeae05993e76697d29f03dc8bfb9248ff87a8f3d8b0975485a4b317","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.37@sha256:fc2970aadaeae05993e76697d29f03dc8bfb9248ff87a8f3d8b0975485a4b317"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.37","digest":"sha256:5abc51995e5901c5d1daeefc957301ee409980e2e607391ec22c06cb2513327b","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.37@sha256:5abc51995e5901c5d1daeefc957301ee409980e2e607391ec22c06cb2513327b"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.4.1","digest":"sha256:ad2a979c2cd8b50098e84938ca9c9c1580eb8e91526f101a90adfba7859b2c32","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.4.1@sha256:ad2a979c2cd8b50098e84938ca9c9c1580eb8e91526f101a90adfba7859b2c32"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b","pinned_image":"ghcr.io/github/gh-aw-node@sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b"},{"image":"ghcr.io/github/github-mcp-server:v1.6.0","digest":"sha256:2b0c48b070f61e9d3969269ead600f62d00fb237b60ac849ef3d166ee7de9ad3","pinned_image":"ghcr.io/github/github-mcp-server:v1.6.0@sha256:2b0c48b070f61e9d3969269ead600f62d00fb237b60ac849ef3d166ee7de9ad3"}]} |
| | [Daily Syntax Error Quality Check](https://github.com/github/gh-aw/blob/main/.github/workflows/daily-syntax-error-quality.md) | copilot | [](https://github.com/github/gh-aw/actions/workflows/daily-syntax-error-quality.lock.yml) | - | - | | ||
| | [Daily Team Evolution Insights](https://github.com/github/gh-aw/blob/main/.github/workflows/daily-team-evolution-insights.md) | claude | [](https://github.com/github/gh-aw/actions/workflows/daily-team-evolution-insights.lock.yml) | - | - | | ||
| | [Daily Testify Uber Super Expert](https://github.com/github/gh-aw/blob/main/.github/workflows/daily-testify-uber-super-expert.md) | copilot | [](https://github.com/github/gh-aw/actions/workflows/daily-testify-uber-super-expert.lock.yml) | - | - | | ||
| | [Daily VulnHunter Scan](https://github.com/github/gh-aw/blob/main/.github/workflows/daily-vulnhunter-scan.md) | claude | [](https://github.com/github/gh-aw/actions/workflows/daily-vulnhunter-scan.lock.yml) | - | - | |
There was a problem hiding this comment.
The changes are clean and consistent. Removing the scenario selection step simplifies the onboarding flow — defaulting to manifest.defaults.scenarioId is applied correctly in all state paths (createDefaultState, parseHashUrl, readState, applyHashState, and setJourney). The buildHashUrl simplification is correct since the setup-scenario screen no longer exists. Tests are updated to match the new flow. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 15.5 AIC · ⌖ 5.42 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review
Applied /tdd and /codebase-design — one minor observation, otherwise a clean and well-executed simplification.
Key Themes and Highlights
Key Themes
- Test coupling to default value: New tests assert
daily-statusas a literal string. Ifmanifest.defaults.scenarioIdchanges, these tests break without a clear signal — see inline comment.
Positive Highlights
- Removal of
scenarioStepCountsByJourneyprecomputation is a good deep-module improvement. - Replacing the empty-string sentinel with
manifest.defaults.scenarioIdthroughout makes the state machine easier to reason about. - Test names updated to reflect new behaviour, reading as specifications.
- New journey-only-hash test covers the key regression path introduced by URL routing change.
- Dead code (
showSetupStep,tunnelPanel) removed cleanly alongside the UI change.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 34.3 AIC · ⌖ 4.5 AIC · ⊞ 6.7K
Comment /matt to run again
Comments that could not be inline-anchored
docs/tests/workshop.spec.ts:555
[/tdd] Test hardcodes daily-status as the expected default scenario ID, coupling the assertion directly to the current default value rather than verifying the behaviour contract.
If the default scenario is ever changed in manifest.defaults.scenarioId, this test will fail with a cryptic URL mismatch rather than a meaningful message.
<details>
<summary>💡 Suggestion</summary>
Consider reading the default scenario ID from the manifest or a shared constant, or at minimum add a comment th…
There was a problem hiding this comment.
Two correctness bugs in the simplified state machine and one test coverage gap that leaves the new click-to-tutorial flow unverified.
Blocking issues summary
1. applyHashState drops the stepKey reset (high)
The refactor removed the else-branch that called getFallbackStepKey() when no scenario was in the hash. The new if (parsed.stepKey) guard means a journey-only hash (#j=github) never resets state.stepKey. Users with session state from a previous run will resume at a stale step position — the new deep-link feature ("treats journey-only hashes as a tutorial start") is broken for returning users. See inline comment.
2. Invalid scenario IDs silently fall back to default (medium→high)
The old code sent users with invalid/renamed scenario IDs in their bookmarks to the picker so they could correct the problem. The new code silently substitutes manifest.defaults.scenarioId. This is a silent regression for any shared or bookmarked link that includes a now-invalid s= param. See inline comment.
3. No UI-click test covers the URL rewrite after entry-path selection (medium)
The new test for journey-only hashes uses page.goto to pre-seed the hash, but the click path — the primary new behavior — is never asserted to produce &s=daily-status in the URL. See inline comment.
🔎 Code quality review by PR Code Quality Reviewer · 79.8 AIC · ⌖ 4.92 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
docs/src/components/workshop/WorkshopExperience.astro:383
applyHashState no longer resets stepKey when no step is present in the hash — a stale step from session storage or a previous journey persists silently, resuming the tutorial at the wrong position.
<details>
<summary>💡 Details and suggested fix</summary>
The old code had an explicit else branch that reset stepKey:
if (parsed.scenarioId) {
state.scenarioId = parsed.scenarioId;
} else {
state.scenarioId = '';
state.stepKey = getFallbackStepKey(); // reset here
}The n…
docs/src/components/workshop/WorkshopExperience.astro:308
Invalid/deleted scenario IDs in bookmarked URLs silently fall back to the default scenario with no user feedback — users with broken deep-links will see the wrong tutorial content without knowing why.
<details>
<summary>💡 Details</summary>
Old behavior:
const scenarioId = rawScenarioId && manifest.scenarios.find((s) => s.id === rawScenarioId) ? rawScenarioId : null;
if (!scenarioId) return { screen: 'setup-scenario', journeyId, scenarioId: null, stepKey: null };An invalid scen…
docs/tests/workshop.spec.ts:555
The new journey-only hash test uses page.goto instead of a UI click, leaving the UI-driven hash rewrite path untested — the test validates hash-navigation-by-URL but not the entry-path-click path that actually produces that URL.
<details>
<summary>💡 Details</summary>
The test added at line 550:
test('treats journey-only hashes as a tutorial start with the default scenario', async ({ page }) => {
await page.goto(`${WORKSHOP_URL}#j=github`);
await page.waitForLoadState('netw…
</details>|
🎉 This pull request is included in a new release. Release: |
The workshop currently pauses on a scenario-selection screen before the tutorial starts, which adds friction too early in the experience. This change starts the tutorial immediately after entry-path selection while preserving the existing scenario-driven flow behind the scenes via the default scenario.
Workshop flow
WorkshopExperience.daily-status) as the implicit flow selector.State and URL behavior
scenarioIdempty during setup transitions.journey + scenario + stephashes.Test updates