Workshop: CSS-driven journey-tag visibility for rendered step content#46720
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds CSS-driven journey-specific visibility to rendered workshop steps.
Changes:
- Rewrites journey comment blocks into classed wrappers.
- Tracks active journey IDs on the workshop root.
- Shows matching blocks through CSS selectors.
Show a summary per file
| File | Description |
|---|---|
docs/src/components/workshop/WorkshopExperience.astro |
Adds journey parsing, visibility state, and conditional styling. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Medium
| const scenario = manifest.scenarios.find((item) => item.id === state.scenarioId) || manifest.scenarios[0]; | ||
| const stepIndex = visibleFlow.indexOf(activeStep.key); | ||
| const visibleJourneyIds = ['all', ...((journey && journey.contentJourneyIds) || [])]; | ||
| root.dataset.workshopVisibleJourneys = uniqueItems(visibleJourneyIds).join(' '); |
| ); | ||
| } | ||
|
|
||
| function rewriteJourneyBlocks(html: string) { |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ 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. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: CSS-driven journey-tag visibility
The approach is sound. Three issues found:
uniqueItemsscope error (blocking, line 880) —uniqueItemsis used inside theis:inlinebrowser script where it is not in scope; this will throw aReferenceErrorat runtime and break route selection.- Hardcoded CSS journey names (non-blocking, line 1357) — The CSS selector table hardcodes
ui,terminal,local,codespace,copilot. A newcontentJourneyIdnot in this list will be silently hidden. Consider toggling a class per journey block via JS, or add a comment documenting the exhaustive list. - No test coverage (non-blocking, line 249) — Journey-marker parsing and visibility are not covered by the existing Playwright suite.
Issue #1 is a blocking runtime regression and must be fixed before merging.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 21 AIC · ⌖ 4.46 AIC · ⊞ 5K
| .aw-workshop[data-workshop-visible-journeys~='terminal'] .aw-workshop-step-content :global(.aw-workshop-journey-block.aw-workshop-journey-block-terminal), | ||
| .aw-workshop[data-workshop-visible-journeys~='local'] .aw-workshop-step-content :global(.aw-workshop-journey-block.aw-workshop-journey-block-local), | ||
| .aw-workshop[data-workshop-visible-journeys~='codespace'] .aw-workshop-step-content :global(.aw-workshop-journey-block.aw-workshop-journey-block-codespace), | ||
| .aw-workshop[data-workshop-visible-journeys~='copilot'] .aw-workshop-step-content :global(.aw-workshop-journey-block.aw-workshop-journey-block-copilot) { |
There was a problem hiding this comment.
The CSS visibility selectors hardcode the known journey names (ui, terminal, local, codespace, copilot). Any workshop manifest that introduces a new contentJourneyId value will silently keep those blocks hidden — the data-workshop-visible-journeys attribute will be updated correctly but no CSS rule will match.
Consider generating the selectors dynamically in JavaScript (e.g., adding/removing a is-visible class on the journey blocks themselves) rather than relying on a hardcoded CSS list, or at minimum add a comment here listing the known values so reviewers know to update this selector table when a new journey type is introduced.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — two blocking issues
Two issues should be resolved before merging.
1. (Critical) Sanitization order likely breaks the feature entirely — rewriteJourneyBlocks runs after sanitizeWorkshopHtml; if the sanitizer strips HTML comments (the common default), no journey wrapper divs are ever injected and all journey-specific content stays hidden for every journey.
2. (High) CSS selector set is a closed, undocumented enum — The CSS hardcodes exactly 5 journey names. The rewriting pipeline accepts any normalized slug, so authors who use any other token get silently invisible content. The two halves of this feature need to be in sync, either by making the CSS dynamic or by formally validating tokens against the known list at build time.
🔎 Code quality review by PR Code Quality Reviewer · 46.9 AIC · ⌖ 7.29 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
docs/src/components/workshop/WorkshopExperience.astro:266
Silent feature failure if sanitizeWorkshopHtml strips HTML comments: rewriteJourneyBlocks runs after sanitizeWorkshopHtml, so if the sanitizer removes HTML comments (which is common behavior), all <!-- journey: ... --> markers are gone before this function sees them — the wrapping divs are never emitted and journey content stays hidden for all journeys.
<details>
<summary>💡 Suggested fix</summary>
Move rewriteJourneyBlocks before sanitizeWorkshopHtml, so comment markers are…
docs/src/components/workshop/WorkshopExperience.astro:289
Hardcoded journey names in CSS mean any new journey token is silently invisible: The CSS selectors enumerate exactly 5 journey names (ui, terminal, local, codespace, copilot). Content tagged with any other token will have the correct wrapper div and data attribute set, but no matching CSS rule — it stays hidden with no error or warning.
<details>
<summary>💡 Why this is fragile</summary>
The normalizeJourneyToken + parseJourneyTokens pipeline is deliberately open-ended (acce…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on three correctness/maintainability issues.
📋 Key Themes & Highlights
Key Issues
uniqueItemsscope bug (line 880) — already flagged by Copilot;uniqueItemsis a server-side Astro helper and is not available inside theis:inlinebrowser script, causing aReferenceErrorat runtime on everyrender()call.- Parenthesisation bug (line 266) —
transformTables(rewriteJourneyBlocks(...))has a paren count mismatch. The.replace()chain for heading level bumping, href rewriting, and image src rewriting appears to be chained insidetransformTables's argument rather than applied after it. This needs a careful manual audit. - Hardcoded journey token list in CSS (lines 1353–1357) — new journeys silently fail to render; no guard or comment warns future authors to add a CSS rule.
display: contentsa11y concern (line 1358) — the choice removes the wrapper from the accessibility tree;display: blockis safer unless flex/grid layout explicitly requires it.- Test coverage gap (line 249) — already flagged; the new journey-block transform path has no Playwright coverage.
Positive Highlights
- ✅ Clean, composable transform pipeline (
sanitize → alerts → task-lists → journey-blocks → tables) - ✅
normalizeJourneyTokenis a small, pure function — easy to unit-test - ✅
data-workshop-visible-journeysattribute is a clean, CSS-query-able state carrier - ✅ Using
~=attribute selectors is the right CSS tool for space-separated token matching
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 45 AIC · ⌖ 4.83 AIC · ⊞ 6.7K
Comment /matt to run again
| .aw-workshop[data-workshop-visible-journeys~='local'] .aw-workshop-step-content :global(.aw-workshop-journey-block.aw-workshop-journey-block-local), | ||
| .aw-workshop[data-workshop-visible-journeys~='codespace'] .aw-workshop-step-content :global(.aw-workshop-journey-block.aw-workshop-journey-block-codespace), | ||
| .aw-workshop[data-workshop-visible-journeys~='copilot'] .aw-workshop-step-content :global(.aw-workshop-journey-block.aw-workshop-journey-block-copilot) { | ||
| display: contents; |
There was a problem hiding this comment.
[/codebase-design] display: contents is intentional for flow preservation, but it has a known browser accessibility-tree bug: the wrapper <div> is removed from the a11y tree, which can break landmark/list semantics for screen readers if journey blocks wrap <li>, <td>, or interactive elements.
💡 Safer alternative
Use display: block / display: none (the default already hides with display: none; the reveal just needs display: block). Since the <div> is a neutral wrapper, display: block is safe and avoids the a11y issue:
.aw-workshop-step-content :global(.aw-workshop-journey-block.aw-workshop-journey-block-all),
/* ... */
{
display: block; /* was: display: contents */
}display: contents would only be needed if journey blocks were placed inside flex/grid containers where an extra wrapper would break layout — verify that is actually the case before keeping it.
@copilot please address this.
| } | ||
|
|
||
| .aw-workshop-step-content :global(.aw-workshop-journey-block.aw-workshop-journey-block-all), | ||
| .aw-workshop[data-workshop-visible-journeys~='ui'] .aw-workshop-step-content :global(.aw-workshop-journey-block.aw-workshop-journey-block-ui), |
There was a problem hiding this comment.
[/codebase-design] The CSS hardcodes a closed list of journey tokens (ui, terminal, local, codespace, copilot). Any new journey added to the manifest will be invisible without a corresponding CSS rule update — a silent failure mode.
💡 Data-driven alternative
The data-workshop-visible-journeys attribute already holds the active token list. A single attribute selector can match any token dynamically without enumerating every journey:
/* hide all journey blocks by default */
.aw-workshop-step-content :global(.aw-workshop-journey-block) {
display: none;
}
/* JS sets: root.dataset.workshopActiveJourney = "ui" (single token)
then CSS can match generically -- requires a different data model */Alternatively, document in a code comment that new journeys require a corresponding CSS rule, so it is not accidentally missed.
@copilot please address this.
|
|
||
| function rewriteWorkshopHtml(html: string) { | ||
| return transformTables(rewriteGfmTaskLists(rewriteGfmAlerts(sanitizeWorkshopHtml(html))) | ||
| return transformTables(rewriteJourneyBlocks(rewriteGfmTaskLists(rewriteGfmAlerts(sanitizeWorkshopHtml(html)))) |
There was a problem hiding this comment.
[/codebase-design] The closing paren for rewriteJourneyBlocks(...) is mismatched — transformTables(rewriteJourneyBlocks(rewriteGfmTaskLists(rewriteGfmAlerts(sanitizeWorkshopHtml(html)))) has four closing parens for five opening parens. The .replace(...) chain that follows is chained on rewriteJourneyBlocks's result, not on the transformTables call, meaning transformTables receives only the raw string without heading/link/image rewrites.
💡 Expected parenthesisation
If this compiles, verify whether the heading level-bumps, local-link rewriting, and asset-URL resolution are intentionally applied before transformTables, or whether there is a missing closing paren that should wrap the entire chain.
@copilot please address this.
|
@copilot run pr-finisher skill |
…sibility tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed two issues surfaced by the automated review:
Commit: |
🤖 PR Triage - run 29724276600
Score breakdown: Impact 15/50 - Urgency 5/30 - Quality 8/20 Rationale: Single-file CSS change. CHANGES_REQUESTED from CI bot. Workshop journey-tag visibility feature. Next: Author should address CI feedback before progressing.
|
|
🎉 This pull request is included in a new release. Release: |
Workshop markdown supports
<!-- journey: ... --> ... <!-- /journey -->blocks, but rendered step HTML did not enforce journey-specific visibility. This change makes journey-scoped sections render only for the active journey path, primarily via CSS.Journey-tag block transformation
WorkshopExperience.astroto convert journey comment blocks into wrapper nodes with deterministic classes:aw-workshop-journey-blockaw-workshop-journey-block-<journey>Active journey visibility state
data-workshop-visible-journeyson the workshop root.all+ mapped content journey IDs).CSS-based conditional rendering
display: contentsto preserve document flow while gating visibility.