feat: filter WorkshopExperience steps by journey/adventure frontmatter - #46582
Conversation
…frontmatter Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…n in callbacks Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. PR #46582 only modified source files in docs/: WorkshopExperience.astro, workshop-content.ts, manifest.ts, and sync-workshop-content.js. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46582 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Replaces hardcoded workshop routing with frontmatter-driven journey and adventure filtering.
Changes:
- Parses and propagates workshop frontmatter metadata.
- Filters server and client workshop flows by journey/scenario.
- Adds hub detection and Copilot-specific routing.
Show a summary per file
| File | Description |
|---|---|
docs/src/lib/workshop/manifest.ts |
Defines mappings and server-side filtering. |
docs/src/components/workshop/WorkshopExperience.astro |
Implements matching client-side flow selection. |
docs/scripts/sync-workshop-content.js |
Extracts frontmatter into generated metadata. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/4 changed files
- Comments generated: 2
- Review effort level: Medium
| const candidates = steps.filter((step) => { | ||
| if (isCopilot && step.journey === 'ui' && !['core', 'setup', 'advanced', 'scenario-d'].includes(step.adventure)) { | ||
| return false; | ||
| } | ||
| const journeyMatch = step.journey === 'all' || contentJourneyIds.includes(step.journey); | ||
| return journeyMatch && includedAdventures.has(step.adventure) && step.adventure !== 'side-quest'; |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat: filter WorkshopExperience steps by journey/adventure frontmatter
The data-driven filtering approach is a clean architectural improvement over the hardcoded route tables. Three issues need to be addressed before merging:
Blocking
1. Local dev artifact committed (docs/src/generated/workshop-content.ts, line 4)
The source URL was changed from the real upstream GitHub URL to /tmp/workshop-source. This is a local path that should never appear in committed code and will break reproducibility documentation.
2. Redundant parseFrontmatter call (docs/scripts/sync-workshop-content.js, line ~232)
After addEntryMetadata strips and re-assigns the frontmatter-free body back to entry.body, the loop at line 232 calls parseFrontmatter(entry.body) again on the already-stripped body. The second call is harmless but misleading — entry.body should be used directly.
Non-blocking (worth verifying)
3. hubPrefixes filter excludes journey === 'all' variants (WorkshopExperience.astro line ~171, manifest.ts line ~1211)
Hub detection builds hubPrefixes from steps matching contentJourneyIds.includes(s.journey). Since 'all' is never in any contentJourneyIds array, 'all'-journey variants with letter prefixes won't suppress their numeric hub entry. This appears safe with the current content (all letter-variant steps carry specific journey IDs), but the assumption should be documented or guarded.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 68.8 AIC · ⌖ 4.83 AIC · ⊞ 5K
Comments that could not be inline-anchored
docs/src/generated/workshop-content.ts:4
The source URL comment was changed to /tmp/workshop-source — this is a local dev artifact and should not be committed. The comment should point to the real upstream workshop source URL (e.g. https://github.com/githubnext/gh-aw-workshop/tree/main/workshop) before merging.
@copilot please address this.
docs/scripts/sync-workshop-content.js:232
Double frontmatter stripping: addEntryMetadata already parses and strips the frontmatter from entry.body, reassigning the body-without-frontmatter back. Calling parseFrontmatter(entry.body) again here on the already-stripped body has no effect but is misleading. Use entry.body directly — the frontmatter was already removed by addEntryMetadata.
// Instead of:
const { body } = parseFrontmatter(entry.body);
writeFileSync(..., rewriteWorkshopMarkdownForAstro(body, ...));
// Use:
w…
</details>
<details><summary>docs/src/components/workshop/WorkshopExperience.astro:171</summary>
The `hubPrefixes` set is built by filtering steps where `contentJourneyIds.includes(s.journey)`. This never matches steps with `journey === 'all'` (since `'all'` is never a `contentJourneyId`). If a hub-letter variant (e.g. `11a-build-foo-ui.md`) has `journey: 'ui'`, it will be captured. But if such a variant has `journey: 'all'`, it won't contribute to `hubPrefixes`, so its numeric hub entry (e.g. `11.md`) won't be suppressed.
Verify this is intentional — if hub variants can have `journey: 'a…
</details>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, /codebase-design, and /grill-with-docs — requesting changes on two correctness issues and flagging architecture and documentation gaps.
📋 Key Themes & Highlights
Key Issues
- Double-strip bug (
sync-workshop-content.jsline 54):parseFrontmattercalled on already-stripped bodies from the existing-entries cache path. The existing review comment (id 3610257883) correctly flags this. - Trailing
\rin values (parseFrontmatterline 14): splitting on\nafter a\r\nregex match leaves\ron each value in CRLF files, silently breaking journey/adventure comparisons. - Duplicated filter logic (
manifest.ts+WorkshopExperience.astro): server-sidebuildWorkshopFlowand client-sidebuildFlowimplement the same hub-detection algorithm separately — a future change must be made in both places. - No unit tests for hub detection: the two-case regex-based hub-suppression logic is untested; naming-convention changes will silently break it.
- Undocumented
copilot→uicoupling: the compensating exclusion inbuildWorkshopFlowis non-obvious and will surprise the next person who adds a UI-journey adventure.
Positive Highlights
- ✅ Clean replacement of hardcoded route maps with data-driven frontmatter — the approach is sound and aligns well with the content model.
- ✅ Good use of
contentJourneyIdsto decouple manifest IDs from content-file values. - ✅ Hub detection handles both naming conventions (numeric-only
06and alphanumeric11a) explicitly. - ✅
normalizeStepIdhelper avoids repeated inline.replace(/\.md$/u, '').
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 80 AIC · ⌖ 5.72 AIC · ⊞ 6.7K
Comment /matt to run again
Comments that could not be inline-anchored
docs/scripts/sync-workshop-content.js:54
[/diagnosing-bugs] Double-stripping bug: parseFrontmatter is called here on entries from loadExistingGeneratedWorkshopEntries(), but those entries have already had frontmatter stripped by addEntryMetadata. On a cache hit entry.body no longer starts with ---, so the parse is a no-op — journey/adventure won't be reparsed, meaning cached entries silently keep their stored values only if the generated file is already up to date.
<details>
<summary>💡 Suggested fix</summary>
Writ…
docs/scripts/sync-workshop-content.js:14
[/diagnosing-bugs] Trailing \r in frontmatter values: the regex matches \r\n line endings, but yaml.split('\n') leaves a trailing \r on each value in CRLF files. A frontmatter entry like journey: terminal\r will produce frontmatter['journey'] === 'terminal\r', which won't match contentJourneyIds comparisons downstream.
<details>
<summary>💡 Suggested fix</summary>
Strip \r when splitting:
for (const line of yaml.split(/\r?\n/u)) {Or trim the captured value:
`…
docs/src/lib/workshop/manifest.ts:1201
[/tdd] The hub-detection algorithm is the most complex new logic in this PR — two cases, a regex, a Set scan — but there are no unit tests for it. A stale journey ID or a new step naming convention could silently drop or include hub pages for only one journey.
<details>
<summary>💡 Suggested tests</summary>
At minimum, test these cases for buildWorkshopFlow:
it('excludes numeric hub when letter-variant step exists for journey', () => {
// e.g. '06' is excluded when '06a-...' ex…
</details>
<details><summary>docs/src/lib/workshop/manifest.ts:1192</summary>
**[/codebase-design]** The server-side `buildWorkshopFlow` and the client-side `buildFlow` in `WorkshopExperience.astro` now implement the same hub-detection and journey-filter algorithm in two places. Any future change to the logic (new hub-naming convention, new copilot exclusion rule) must be made in both places and kept in sync manually — a maintenance hazard.
<details>
<summary>💡 Suggested approach</summary>
Emit the pre-computed step array (result of `buildWorkshopFlow`) into the manif…
</details>
<details><summary>docs/src/lib/workshop/manifest.ts:1141</summary>
**[/grill-with-docs]** The `copilot` manifest journey maps to `contentJourneyIds: ['copilot', 'ui']`. This means UI-journey steps are included in the copilot flow by default, and the copilot-specific exclusion at line 1198 (`isCopilot && entry.journey === 'ui' && scenario-a/b/c`) is compensating for that. The coupling is fragile: adding a new UI-journey adventure value would silently include it in the copilot flow unless a matching exclusion rule is added.
<details>
<summary>💡 Suggested appro…
</details>
There was a problem hiding this comment.
REQUEST_CHANGES — three blocking issues found
The data-driven filtering approach is sound, but three issues need resolution before merge.
### Blocking issues
1. journey value is unmapped (high) — Workshop content files use frontmatter (3 entries: , , ), but no manifest journey declares containing "codespace". These steps are silently dropped from every flow. See inline comment on manifest.ts line 88.
2. Generated file has committed local path (high) — docs/src/generated/workshop-content.ts line 2 reads // Source: /tmp/workshop-source. This should be regenerated from upstream before merge, or the script should normalize the source comment for local builds. See inline comment.
3. parseFrontmatter doesn't strip YAML quotes (medium) — Quoted YAML values like journey: "all" produce literal '"all"' strings, silently breaking journey/adventure comparisons for any content file that uses quoted values. See inline comment on sync-workshop-content.js line 17.
The cached-content fallback issue (destroying journey/adventure on network failure) was already flagged in an earlier review comment and is not duplicated here.
🔎 Code quality review by PR Code Quality Reviewer · 101.1 AIC · ⌖ 6.37 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
docs/src/lib/workshop/manifest.ts:88
codespace journey value has no matching contentJourneyIds entry — codespace-journey steps are silently excluded from all flows.
<details>
<summary>💡 Details and suggested fix</summary>
The workshop content now uses journey: codespace frontmatter (e.g. 02a-setup-codespace.md, 06a-install-terminal.md), but no entry in workshopJourneys declares contentJourneyIds containing "codespace". The filter entry.journey === 'all' || contentJourneyIds.includes(entry.journey) therefor…
docs/scripts/sync-workshop-content.js:17
parseFrontmatter doesn't strip quotes from YAML values — journey: "all" produces '"all"' with literal quote characters, silently breaking all journey/adventure comparisons.
<details>
<summary>💡 Details and suggested fix</summary>
The parser does line.slice(colonIdx + 1).trim() which preserves any surrounding quotes in the value. If a workshop content file uses quoted YAML values (valid YAML), the extracted string will include the quote characters:
// Input frontmatter: jo…
</details>
<details><summary>docs/src/generated/workshop-content.ts:2</summary>
**Generated file contains a local filesystem path `/tmp/workshop-source` as the source comment — this leaks a machine-specific path and will appear as a permanent diff in CI.**
<details>
<summary>💡 Details</summary>
Line 2 of the generated file reads:
```ts
// Source: /tmp/workshop-sourceThis was generated with GH_AW_WORKSHOP_SOURCE_DIR=/tmp/workshop-source set locally. The sync script uses source: sourceDir (the resolved local path) directly when localWorkshopSourceDir is set —…
|
Thanks for working on the data-driven workshop filtering system! 🎯 This is a clean refactor that makes the routing logic much more maintainable. Here's what would help get this across the finish line:
Tests would validate that the refactored filtering matches the original hardcoded routes and catch edge cases. If you'd like a hand, assign this to your coding agent:
|
|
@copilot run pr-finisher skill |
|
@copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
… Playwright tests - addEntryMetadata: fall back to entry.journey/entry.adventure when frontmatter is empty (cached entries have already had frontmatter stripped); add explanatory comment for the three-level fallback chain - docs/tests/workshop.spec.ts: add 10 Playwright tests covering scenario isolation, hub page removal, and Copilot scenario-d substitution Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Two issues from the review threads were addressed in commit
|
|
🎉 This pull request is included in a new release. Release: |
Replaces the hardcoded curriculum-route system in
WorkshopExperiencewith data-driven filtering based onjourneyandadventurefrontmatter fields now present in workshop content files (aligned to gh-aw-workshop#1538).Frontmatter schema
Each workshop
.mdfile now declares:Sync script (
sync-workshop-content.js)parseFrontmatter()to extractjourney/adventurefrom markdown files and strip frontmatter from bodyWorkshopContentEntrytype gainsjourney: stringandadventure: stringManifest (
manifest.ts)WorkshopJourneygainscontentJourneyIds: string[]mapping manifest journey IDs to content frontmatter values:github → ['ui'],terminal → ['terminal', 'local'],vscode → ['local', 'terminal'],copilot → ['copilot', 'ui']workshopScenarioAdventuresmapping scenario IDs to adventure values (scenario-a/b/c)buildWorkshopFlowreplaced with a filter that:contentJourneyIdsand relevant adventures (core, setup, advanced, selected scenario, scenario-d for copilot)06) are hubs when letter-variants exist (06a/b/c); alphanumeric prefixes (11a) are hubs when same-prefix specific-journey entries existui-journey scenario-a/b/c build steps since11d(scenario-d) covers all scenarios; hub detection uses full content so hub pages are still suppressed even when the trigger variant was filtered outWorkshopExperience.astrojourney/adventurethreaded throughWorkshopEntry→WorkshopStep→ step JSON payloadroutesremoved,scenarioAdventuresadded;journeysnow includescontentJourneyIdsbuildFlowrewritten to mirror server-side filter logic (usingstepsarray metadata rather than route maps)