feat(standalone): wire up the dev environment setup module - #61
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The standalone module wiring is consistent with existing routing/player patterns and includes filesystem gating to prevent dead links, with only minor follow-up nits noted.
Pull request overview
This PR wires up rendering and discovery of standalone (trackless) modules so the “Setting Up Your Development Environment” content can actually be reached from the course UI, and it aligns the setup documentation with bootstrap’s real hosts-file behavior.
Changes:
- Model and resolve standalone modules via manifest + filesystem gating (avoid generating dead routes/links).
- Add a standalone module route that reuses the existing module player and progress storage scheme.
- Add a “Before you start” standalone section on the homepage and update setup docs to remove duplicate
/etc/hostsinstructions (except Windows).
File summaries
| File | Description |
|---|---|
| lib/types.ts | Adds typed support for standalone modules/categories in the manifest. |
| lib/manifest.ts | Adds standalone lookups and static param generation gated by existing markdown. |
| lib/content.ts | Adds standalone markdown loading and a “written standalone modules” list for homepage gating. |
| content/shared/setup.md | Removes redundant manual hosts edits on macOS/Linux; clarifies Windows requirements. |
| content/course-manifest.json | Wires standalone-env-setup to shared/setup.md via a file field. |
| components/ModulePlayerClient.tsx | Adds optional back-link props and suppresses module prefix for standalone index 0. |
| components/CatalogClient.tsx | Adds a “Before you start” standalone section based on server-provided availability. |
| app/standalone/[module]/page.tsx | New standalone module page reusing ModulePlayerClient + static params. |
| app/page.tsx | Passes written-standalone IDs to the catalog for gating. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
One deliberate deferral, noted so it isn't mistaken for an oversight: the "Before you start" section ignores the track and level filter chips, so filtering to PHP still shows it. That seemed right for a module that belongs to no track and is a prerequisite for both, but it does mean the section sits outside the filter model and isn't counted in the "66 modules" total. Worth revisiting when there is more than one standalone module written — with one, filtering it is not a real interaction. Same reasoning for not adding a "Standalone" filter chip yet. |
…to test existence Both from Copilot's review on #61, and both fair. The feedback context read "Standalone / Setting Up Your Development Environment / Setting Up Your Development Environment", because the route passed the module title as levelTitle. That was left over from the first pass, where levelTitle also labelled the sidebar back-link; once backLabel took that over, levelTitle only reached the feedback context, where repeating the title loses the module's declared level. It now passes the capitalised level, so a report arrives as "Standalone / Beginner / <module>" — the same shape a track module produces, which is what makes issues triageable. getWrittenStandaloneModules() also called getStandaloneContent() purely to decide whether a file existed, reading the whole markdown to answer a question fs.existsSync() answers. getWrittenModules() already does the cheap thing for track modules. Fixed by extracting standaloneContentPath(), which resolves and containment- checks the manifest's `file` without reading it, mirroring moduleContentPath(). getStandaloneContent() now builds on it, so the path-traversal guard stays in exactly one place. Applied the same fix to getAllStandaloneParams(), which Copilot did not flag but had the identical problem. AI-Assisted-By: Claude Opus 5 (1M context) Signed-off-by: Anna Larch <[email protected]>
There was a problem hiding this comment.
🟡 Changes recommended
Standalone route/listing gating should verify paths are regular files (and containment guarantees should match implementation) to avoid generating routes/cards for unreadable paths and to prevent runtime/build crashes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
lib/content.ts:76
getWrittenStandaloneModules()treats any existing path as renderable, butfs.existsSync()also returns true for directories; that would incorrectly surface a standalone module as "written" (and generate links) even though the module can't be read. PreferstatSync(...).isFile()here.
for (const category of Object.values(manifest.standalone?.categories ?? {})) {
for (const mod of category.modules) {
if (!mod.file) continue
const filePath = standaloneContentPath(mod.file)
if (filePath && fs.existsSync(filePath)) ids.push(mod.id)
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
… crash Copilot's second review on #61, and two of the three points are the same real defect. fs.existsSync() returns true for a directory, so a manifest typo of `"file": "shared"` instead of `"shared/setup.md"` would have readFileSync() throw EISDIR and take the build down, while route generation emitted a route that 404s at runtime. Both should behave as "not written yet". Gated all three call sites behind one helper, standaloneContentFile(), which returns a path only for a readable regular file. statSync(..., { throwIfNoEntry: false }) keeps the common missing-file case free of try/catch. getWrittenStandaloneModules() and getAllStandaloneParams() now share it, so the definition of "has content" exists once. Also softened standaloneContentPath()'s docstring, which claimed a manifest entry "cannot reach outside the content tree". A symlink committed inside content/ could still resolve elsewhere, so the check catches a stray `../` rather than providing a sandbox. The manifest is source code in this repository; a typo is the threat model, not an attacker. Saying so is better than implying a guarantee that is not there. Found while testing the directory case, and worth more than the bug that prompted it: if generateStaticParams() returns an empty array, `output: 'export'` rejects the build with "Page /standalone/[module] is missing generateStaticParams()", because empty is indistinguishable from absent. So this route requires at least one standalone module to have content. True today, and it fails loudly rather than shipping a dead route, but the error message does not explain itself - now documented where it bites. AI-Assisted-By: Claude Opus 5 (1M context) Signed-off-by: Anna Larch <[email protected]>
…to test existence Both from Copilot's review on #61, and both fair. The feedback context read "Standalone / Setting Up Your Development Environment / Setting Up Your Development Environment", because the route passed the module title as levelTitle. That was left over from the first pass, where levelTitle also labelled the sidebar back-link; once backLabel took that over, levelTitle only reached the feedback context, where repeating the title loses the module's declared level. It now passes the capitalised level, so a report arrives as "Standalone / Beginner / <module>" — the same shape a track module produces, which is what makes issues triageable. getWrittenStandaloneModules() also called getStandaloneContent() purely to decide whether a file existed, reading the whole markdown to answer a question fs.existsSync() answers. getWrittenModules() already does the cheap thing for track modules. Fixed by extracting standaloneContentPath(), which resolves and containment- checks the manifest's `file` without reading it, mirroring moduleContentPath(). getStandaloneContent() now builds on it, so the path-traversal guard stays in exactly one place. Applied the same fix to getAllStandaloneParams(), which Copilot did not flag but had the identical problem. AI-Assisted-By: Claude Opus 5 (1M context) Signed-off-by: Anna Larch <[email protected]>
… crash Copilot's second review on #61, and two of the three points are the same real defect. fs.existsSync() returns true for a directory, so a manifest typo of `"file": "shared"` instead of `"shared/setup.md"` would have readFileSync() throw EISDIR and take the build down, while route generation emitted a route that 404s at runtime. Both should behave as "not written yet". Gated all three call sites behind one helper, standaloneContentFile(), which returns a path only for a readable regular file. statSync(..., { throwIfNoEntry: false }) keeps the common missing-file case free of try/catch. getWrittenStandaloneModules() and getAllStandaloneParams() now share it, so the definition of "has content" exists once. Also softened standaloneContentPath()'s docstring, which claimed a manifest entry "cannot reach outside the content tree". A symlink committed inside content/ could still resolve elsewhere, so the check catches a stray `../` rather than providing a sandbox. The manifest is source code in this repository; a typo is the threat model, not an attacker. Saying so is better than implying a guarantee that is not there. Found while testing the directory case, and worth more than the bug that prompted it: if generateStaticParams() returns an empty array, `output: 'export'` rejects the build with "Page /standalone/[module] is missing generateStaticParams()", because empty is indistinguishable from absent. So this route requires at least one standalone module to have content. True today, and it fails loudly rather than shipping a dead route, but the error message does not explain itself - now documented where it bites. AI-Assisted-By: Claude Opus 5 (1M context) Signed-off-by: Anna Larch <[email protected]>
2ed6fb5 to
92a900a
Compare
The PHP beginner track defers to a standalone "Setting Up Your Development Environment" module twice (php/beginner/1.md, in both the Docker and the Codespaces paths), telling readers to find it in the Standalone section on the home screen or under "Before you start". Neither existed in course-v2, so a beginner without Docker was sent to a module that rendered nowhere. The content was already written at content/shared/setup.md. What was missing was the wiring: - Manifest: standalone-env-setup gains a `file` field. Track modules resolve their markdown by convention (content/<track>/<level>/<index>.md), which standalone modules cannot do because they have no index, so the path has to be declared. - Types: CourseManifest now models `standalone`, which was present in the JSON but invisible to the app. - Route: app/standalone/[module]/page.tsx, with generateStaticParams() as the static export requires. It reuses ModulePlayerClient, so progress, section navigation and feedback behave exactly as in a track module. - Player: `backHref`/`backLabel` are now optional props, defaulting to the current behaviour. Standalone modules have no level listing to return to, so they send readers back to the catalogue. The "M1." prefix is suppressed for modules with index 0, since track modules are 1-indexed. - Homepage: a "Before you start" section above the tracks, matching the wording the track content already uses. Route generation and the homepage listing are both gated on the markdown existing rather than on the manifest, following getAllModuleParams(): the standalone section declares sixteen modules and one is written, so listing from the manifest would advertise fifteen dead links. Also settles a contradiction between the three setup docs. shared/setup.md told macOS and Linux readers to add the hosts entry by hand after running bootstrap.sh, while both track modules said bootstrap does it. Bootstrap is right: it calls scripts/update-hosts, which greps before appending. The manual line does not, so following the doc appended a duplicate /etc/hosts entry and asked for sudo for nothing. Removed for macOS and Linux, kept for Windows, where it is genuinely needed because bootstrap runs inside WSL 2 and only updates Linux's hosts file, which the Windows browser never reads. Verified with a production build: 27 static pages, including /standalone/standalone-env-setup. AI-Assisted-By: Claude Opus 5 (1M context) Signed-off-by: Anna Larch <[email protected]>
The first pass gave the "Before you start" section its own bespoke card, which was visibly lighter than the module cards in the PHP and ExApp sections: no hover rule, no icon tile, no progress badge, no level badge, and a different grid. Reuse ModuleCard instead. Two optional escape hatches make that possible without touching how track modules render: - `href` overrides the path, which is otherwise derived from trackId/levelId/index. A standalone module has no index to derive one from. - `eyebrow` overrides the "M<index> · <track label>" line, which is meaningless for a module belonging to no track. Both default to the existing behaviour, so every track card is byte-identical. Also: - TRACK_STYLE gains a `standalone` entry, so the section badge and card icon use a real glyph and colour rather than falling through to the neutral fallback style. - Standalone modules now get the same Done / In progress badge as track modules, read from the progress store under the same `standalone/<level>/<id>` key the standalone route writes. - The section header matches the track sections' furniture: styled badge, heading, and a module count in the meta line. Verified with a production build: 27 static pages, unchanged from the previous commit. AI-Assisted-By: Claude Opus 5 (1M context) Signed-off-by: Anna Larch <[email protected]>
…to test existence Both from Copilot's review on #61, and both fair. The feedback context read "Standalone / Setting Up Your Development Environment / Setting Up Your Development Environment", because the route passed the module title as levelTitle. That was left over from the first pass, where levelTitle also labelled the sidebar back-link; once backLabel took that over, levelTitle only reached the feedback context, where repeating the title loses the module's declared level. It now passes the capitalised level, so a report arrives as "Standalone / Beginner / <module>" — the same shape a track module produces, which is what makes issues triageable. getWrittenStandaloneModules() also called getStandaloneContent() purely to decide whether a file existed, reading the whole markdown to answer a question fs.existsSync() answers. getWrittenModules() already does the cheap thing for track modules. Fixed by extracting standaloneContentPath(), which resolves and containment- checks the manifest's `file` without reading it, mirroring moduleContentPath(). getStandaloneContent() now builds on it, so the path-traversal guard stays in exactly one place. Applied the same fix to getAllStandaloneParams(), which Copilot did not flag but had the identical problem. AI-Assisted-By: Claude Opus 5 (1M context) Signed-off-by: Anna Larch <[email protected]>
… crash Copilot's second review on #61, and two of the three points are the same real defect. fs.existsSync() returns true for a directory, so a manifest typo of `"file": "shared"` instead of `"shared/setup.md"` would have readFileSync() throw EISDIR and take the build down, while route generation emitted a route that 404s at runtime. Both should behave as "not written yet". Gated all three call sites behind one helper, standaloneContentFile(), which returns a path only for a readable regular file. statSync(..., { throwIfNoEntry: false }) keeps the common missing-file case free of try/catch. getWrittenStandaloneModules() and getAllStandaloneParams() now share it, so the definition of "has content" exists once. Also softened standaloneContentPath()'s docstring, which claimed a manifest entry "cannot reach outside the content tree". A symlink committed inside content/ could still resolve elsewhere, so the check catches a stray `../` rather than providing a sandbox. The manifest is source code in this repository; a typo is the threat model, not an attacker. Saying so is better than implying a guarantee that is not there. Found while testing the directory case, and worth more than the bug that prompted it: if generateStaticParams() returns an empty array, `output: 'export'` rejects the build with "Page /standalone/[module] is missing generateStaticParams()", because empty is indistinguishable from absent. So this route requires at least one standalone module to have content. True today, and it fails loudly rather than shipping a dead route, but the error message does not explain itself - now documented where it bites. AI-Assisted-By: Claude Opus 5 (1M context) Signed-off-by: Anna Larch <[email protected]>
92a900a to
ccf6c09
Compare
Why
content/php/beginner/1.mddefers to a standalone "Setting Up Your Development Environment" module twice — once in the Docker path (line 36) and once in the Codespaces path (line 74) — telling readers to "find it in the Standalone section on the course home screen, or in the 'Before you start' section".Neither existed in
course-v2. So a beginner who had not set up Docker was sent to a module that rendered nowhere. This fixes a live broken instruction, not a missing extra.The content was already written, at
content/shared/setup.md. Only the wiring was missing.What
standalone-env-setupgains afilefield. Track modules resolve markdown by convention (content/<track>/<level>/<index>.md); standalone modules have no index to derive a path from, so it has to be declared.CourseManifestnow modelsstandalone, which was already in the JSON but invisible to the app.app/standalone/[module]/page.tsx, withgenerateStaticParams()as the static export requires. It reusesModulePlayerClient, so progress, section navigation and the feedback link behave exactly as in a track module. Progress keys asstandalone/<level>/<id>, which slots into the existingmoduleKey()scheme with no special-casing.backHref/backLabelare new optional props defaulting to current behaviour. Standalone modules have no level listing to return to, so they send readers back to the catalogue. TheM1.prefix is suppressed for index 0, since track modules are 1-indexed.Route generation and the homepage listing are both gated on the markdown existing rather than on the manifest, following the precedent in
getAllModuleParams(): the standalone section declares sixteen modules and one is written, so listing from the manifest would advertise fifteen dead links.Also settles the hosts-file contradiction
The three setup docs had drifted.
shared/setup.mdtold macOS and Linux readers to add the/etc/hostsentry by hand after runningbootstrap.sh, while both track modules said bootstrap does it.Bootstrap is right. It calls
scripts/update-hosts, which greps before appending. The manual line does not, so following the doc appended a duplicate/etc/hostsentry and asked forsudofor nothing.Removed for macOS and Linux. Kept for Windows, where it is genuinely required and now says why: bootstrap runs inside WSL 2 and only updates Linux's hosts file, which the Windows browser never reads.
Verification
Production build, 27 static pages including
/standalone/standalone-env-setup:tsc --noEmitclean.Deliberately not in this PR
The two references in
php/beginner/1.mdare left as prose rather than turned into links.ReactMarkdownhas no customarenderer, so a markdown link renders as a plain<a href>with nobasePathprefix and would 404 on the deployed site under/academy. No content currently relies on absolute internal links. Worth its own issue.AI-Assisted-By: Claude Opus 5 (1M context)