Feat: CLI tool for scaffolding new Accessible Astro projects with interactive configuration prompts#184
Merged
Conversation
✅ Deploy Preview for accessible-astro-starter-incluud ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new monorepo-scoped CLI package ChangesCreate-accessible-astro-starter CLI & monorepo
Unrelated component tweak
Sequence DiagramsequenceDiagram
participant User
participant CLI as "create-accessible-astro-starter CLI"
participant Parser as "parseCliArgs"
participant Prompter as "Prompts (clack)"
participant Resolver as "Options Resolver"
participant Manifest as "buildManifest"
participant Scaffold as "scaffoldProject"
participant FS as "File System / Template"
User->>CLI: run `npm create accessible-astro-starter`
CLI->>Parser: parseCliArgs(argv)
Parser-->>CLI: ParsedFlags
alt required fields missing and --yes not set
CLI->>Prompter: prompt for targetDir, siteName, preset, launcher
Prompter-->>CLI: responses
end
CLI->>Resolver: derive siteId & resolve options
Resolver-->>CLI: ResolvedOptions
CLI->>Manifest: buildManifest(preset, includeLauncher)
Manifest-->>CLI: ProjectManifest (pathsToDelete)
CLI->>Scaffold: scaffoldProject(options, manifest)
Scaffold->>FS: materialize template (download/copy)
FS-->>Scaffold: template ready
Scaffold->>FS: remove paths per manifest & write generated files
FS-->>Scaffold: updated project files
Scaffold-->>CLI: scaffold complete
CLI->>User: print success message & next steps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
markteekman
changed the title
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const manifest = buildManifest(options) | ||
|
|
||
| await scaffoldProject(options, manifest) | ||
| await runCommand('npm', ['install'], targetDir) |
There was a problem hiding this comment.
WARNING: Windows npm invocation can fail
runCommand('npm', ...) assumes npm is on PATH; on Windows the executable is typically npm.cmd, so this call can fail in CI runners. Consider resolving the npm command based on process.platform (as in the local run script) or using process.execPath with npm's JS entrypoint to keep the e2e test cross-platform.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 files)
Reviewed by gpt-5.2-codex-20260114 · 687,615 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
package.json (1)
67-71: Confirm exact-version pinning of Tailwind is intentional.
tailwindcssand@tailwindcss/viteare now pinned to exact4.2.2(no caret), while every other dep still uses caret ranges. If the goal is to avoid surprise Tailwind v4 minor regressions, consider applying the same policy consistently (e.g., pin both Tailwind packages and document the reason), or revert to^4.2.2for parity. As-is, the inconsistency may confuse future contributors runningnpm update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 67 - 71, The package.json currently pins "tailwindcss" and "@tailwindcss/vite" to exact version "4.2.2" while other deps use caret ranges; decide and apply a consistent policy: either change both "tailwindcss" and "@tailwindcss/vite" to use caret ranges "^4.2.2" for parity with other deps, or pin both packages exactly and add a brief comment in repo docs (e.g., README or CONTRIBUTING) explaining why Tailwind is locked; update the two package names "tailwindcss" and "@tailwindcss/vite" accordingly so they match the chosen policy.packages/create-accessible-astro-starter/src/presets.ts (1)
83-90: RedundantkeepThankYouPagedeletion branch.Since
keepThankYouPage = keepContactPage(line 36), the second branch is unreachable as a distinct case —src/pages/thank-you.astrois already added topathsToDeletein the!keepContactPagebranch. Either remove the dead block or decouple the two flags if a future preset is expected to keep contact but drop thank-you.♻️ Proposed simplification
if (!keepContactPage) { pathsToDelete.add('src/pages/contact.astro') pathsToDelete.add('src/pages/thank-you.astro') } - if (!keepThankYouPage) { - pathsToDelete.add('src/pages/thank-you.astro') - } - if (!keepMdx) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-accessible-astro-starter/src/presets.ts` around lines 83 - 90, The second conditional checking keepThankYouPage is redundant because keepThankYouPage is set equal to keepContactPage; remove the entire if (!keepThankYouPage) { pathsToDelete.add('src/pages/thank-you.astro') } block, or if intended behavior is to allow independent control, decouple the flags by changing where keepThankYouPage is assigned so it can differ from keepContactPage and update callers/values accordingly; locate symbols keepContactPage, keepThankYouPage, and pathsToDelete in presets.ts and either delete the dead branch or adjust the flag assignment so thank-you removal can be controlled separately.packages/create-accessible-astro-starter/test/scaffold.test.ts (1)
26-33: Hoist the env var setup once and consider cleanup.
process.env.ACCESSIBLE_ASTRO_STARTER_TEMPLATE_DIR = repoRootis repeated in every test (and again inside the parameterized loop) and never restored. Withnode:testrunning sequentially today this is safe, but ifconcurrency: trueis ever enabled — or another test in the same process expects this var to be unset — this becomes a hidden coupling.Consider setting it once at module top (or via
test.before) and clearing it intest.after, e.g.:test.before(() => { process.env.ACCESSIBLE_ASTRO_STARTER_TEMPLATE_DIR = repoRoot }) test.after(() => { delete process.env.ACCESSIBLE_ASTRO_STARTER_TEMPLATE_DIR })Also, none of the tests
rm -rftheirtempRoots — the OS cleanstmpdir()eventually, but locally these accumulate. Optional follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-accessible-astro-starter/test/scaffold.test.ts` around lines 26 - 33, Hoist repeated environment setup by moving the assignment to process.env.ACCESSIBLE_ASTRO_STARTER_TEMPLATE_DIR out of individual tests and into a module-level test.before hook (and remove the per-test assignment inside tests like the one calling scaffoldProject with options from createOptions and buildManifest); add a test.after hook to delete process.env.ACCESSIBLE_ASTRO_STARTER_TEMPLATE_DIR to restore global state, and consider adding cleanup for temporary directories created via mkdtemp (e.g., remove tempRoot in a test.after or test.afterEach) so tests no longer leave tmp files behind.packages/create-accessible-astro-starter/src/scaffold.ts (1)
49-67: Template-ref coupling: CLI version → starter repo Git tag creates operational fragility.
getPublishedTemplateRefreturnsv${packageJson.version}(e.g.,v5.1.0) anddownloadTemplaterequires that tag to exist onincluud/accessible-astro-starter. While the matching tag currently exists, this approach creates a tight coupling that relies on manual alignment during release—if the CLI is published before the Git tag is pushed, or if the tag naming scheme drifts, users will encounter confusing download failures.Consider one of:
- Documenting the release procedure to ensure the matching
vX.Y.Ztag is always pushed before CLI publication.- Falling back to a known-good ref (e.g.,
mainor the latest successful tag) on failure.- Pinning the template ref in the CLI's package.json explicitly rather than reusing the version field, so the two can be bumped independently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-accessible-astro-starter/src/scaffold.ts` around lines 49 - 67, getPublishedTemplateRef currently builds a template ref from the CLI package version and materializeTemplate always attempts to download that tag via downloadTemplate (unless ACCESSIBLE_ASTRO_STARTER_TEMPLATE_DIR is set), which tightly couples CLI publishing to a matching repo tag; change materializeTemplate to handle missing or mismatched tags by attempting a safe fallback: call downloadTemplate with the version-based ref first (using getPublishedTemplateRef), and on failure retry with a known-good ref such as "main" or a configurable template ref from package.json (or env var), and surface a clear error if all attempts fail; ensure the retry logic references materializeTemplate, getPublishedTemplateRef, and downloadTemplate so maintainers can find and update the fallback strategy or move the template ref into package.json later.packages/create-accessible-astro-starter/src/templates.ts (3)
1078-1241: Optional: tighten launcher blog/portfolio fetching paths.Two small things in the generated
LauncherConfig.astro:
- Line 1090 checks
if (!BLOG_API_URL) return [], butBLOG_API_URLhas a non‑empty default inastro.config(line 222), so this guard is effectively dead in the default project. That's fine, but worth a comment in the generated code so users editing.envunderstand the contract.- Line 1104 builds
href: \/blog/${slugify(truncatedTitle)}`` from a 4‑word title slug. If two upstream posts share the same first 4 words, both launcher items point to the same slug, and neither necessarily resolves to a real route. Consider also passing through a stable identifier from the API when available.Both are nice‑to‑haves and don't block correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-accessible-astro-starter/src/templates.ts` around lines 1078 - 1241, The generated launcher blog code (launcherBlogItems / truncateBlogTitle) currently guards on BLOG_API_URL (which may have a default) but lacks a comment explaining that contract and it builds hrefs using slugify(truncatedTitle) only, which can collide; add an inline comment near the BLOG_API_URL check describing that the env has a default and that users can override it via .env, and modify the launcherBlogItems construction (in the async block that fetches BLOG_API_URL and in the map that builds hrefs) to prefer a stable identifier from the API response when present (e.g., post.id or post.slug) falling back to slugify(truncatedTitle) otherwise; reference the symbols BLOG_API_URL, launcherBlogItems, truncateBlogTitle and slugify when making the changes.
173-200: Minor:createFeatureCardsbody is interpolated raw into HTML.
card.titleandcard.body(line 177–178) are pasted directly into the generated HTML without escaping. Today the only callers (createMinimalIndex) pass hardcoded literals so this is safe, but if these helpers later accept user‑provided strings (e.g. fromsiteNameor theme config), the same escaping issue raised oncreateHero/ index pages will recur. Worth either documenting that the inputs must be HTML‑safe or running them through anescapeForHtmlhelper at the boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-accessible-astro-starter/src/templates.ts` around lines 173 - 200, createFeatureCards currently injects card.title and card.body directly into HTML; update createFeatureCards to escape both values using the existing/ new HTML-escaping utility (e.g., escapeForHtml) before interpolation so untrusted input cannot break markup or inject scripts. Locate createFeatureCards and call escapeForHtml on card.title and card.body (or add/rename helper if none exists) to ensure safe output, and keep the rest of the template unchanged.
215-265: EmptyenvBlockleaves a stray blank line inastro.config.When
manifest.keepBlogEnvis false,envBlockis''and the template still emits a blank line right before}). Cosmetic, but the generated config will have an empty line that Prettier will normalize away on first format. If you want the output clean as written, conditionally include the leading newline:♻️ Suggested tweak
- vite: viteConfig, -${envBlock} -}) + vite: viteConfig,${envBlock ? `\n${envBlock}` : ''} +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-accessible-astro-starter/src/templates.ts` around lines 215 - 265, The template always injects envBlock (declared above, based on manifest.keepBlogEnv) into the returned string with a leading newline, which leaves a stray blank line when envBlock is ''. Update the return construction so the leading newline is only included when envBlock is non-empty: either build envBlock to include its own leading newline when manifest.keepBlogEnv is true, or conditionally append `\n${envBlock}` in the template only when envBlock !== ''. Modify references in this file to the envBlock/manifest.keepBlogEnv logic (where viteConfig and the final defineConfig string are assembled) so the generated astro.config has no leftover blank line.packages/create-accessible-astro-starter/src/utils.ts (1)
74-82: Optional: filter is path‑agnostic and can be too permissive.
filter(source)only checks the basename, so it will exclude.git/node_modulesat any depth (good), but it won't exclude common build artifacts likedist,coverage,.cache, or.DS_Storethat sometimes leak into a source directory. If the scaffolder is ever pointed at a developer working copy (rather than a curated published template), these would be copied verbatim.Consider extending the deny‑list, or normalizing on
isDirectory()for accuracy:♻️ Suggested tightening
export async function copyLocalTemplate(sourceDir: string, targetDir: string): Promise<void> { await cp(sourceDir, targetDir, { recursive: true, filter(source) { const name = basename(source) - return !['.git', 'node_modules'].includes(name) + return !['.git', 'node_modules', 'dist', '.cache', '.DS_Store'].includes(name) }, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-accessible-astro-starter/src/utils.ts` around lines 74 - 82, The current copyLocalTemplate filter in copyLocalTemplate only checks basename and is too permissive; update the cp filter to normalize the path and reject a broader deny-list (e.g., dist, coverage, .cache, .DS_Store, .env, build) and also distinguish dirs vs files by using fs.lstat or fs.stat (or the provided filter's stats if available) so you can only exclude directory names for directory artifacts while excluding specific file names like .DS_Store; reference the copyLocalTemplate function, the cp call, the filter(source) callback and basename usage when implementing these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/create-accessible-astro-starter/README.md`:
- Around line 11-17: The "Local development" commands in README.md call
workspace-scoped scripts build:cli and test:cli that only exist in the repo
root; update this section to either (a) explicitly state these commands must be
run from the starter repo root (e.g., "From the starter repo root: npm run
build:cli && npm run test:cli") or (b) replace them with package-local
equivalents (npm run build and npm run test) and note that those run only within
the package; edit the README.md Local development block to include the chosen
clarification and reference the build:cli, test:cli (root-level) and build, test
(package-level) script names so contributors know where to run each command.
In `@packages/create-accessible-astro-starter/src/cli.ts`:
- Around line 132-148: Replace the unsafe startsWith logic for relativeDir by
using Node's path.relative to compute a correct relative path from process.cwd()
to options.targetDir (e.g., const rel = path.relative(process.cwd(),
options.targetDir)), normalize it to POSIX separators (replace backslashes with
'/'), default to '.' when rel is empty, and then use that value in the p.note
output (update the const relativeDir and keep using options.targetDir and p.note
as before); ensure you import path and handle the case where rel is '' -> '.' so
cd and display are correct.
In `@packages/create-accessible-astro-starter/src/templates.ts`:
- Around line 858-960: The template uses escapeForSingleQuotedString in
createHero to escape firstWord and remainingWords for inclusion in HTML/JSX
text, which produces backslash-escaped output (e.g. O\'Brien) and doesn't
HTML-escape special chars; add a new helper escapeForHtml in utils.ts that
replaces &, <, >, and " with their HTML entities, then update createHero to stop
calling escapeForSingleQuotedString and instead call escapeForHtml for firstWord
and the joined remainingWords (used in remainingTitle) as well as any other
interpolations emitted into HTML text nodes, keeping escapeForSingleQuotedString
only for actual JS single-quoted literal contexts.
- Around line 1243-1415: Generated Astro markup is vulnerable because
escapeForSingleQuotedString only handles backslash/apos and is used in
double‑quoted attributes and raw HTML text; create a small escapeForHtml helper
that escapes &, <, > and " (and optionally ') and replace usages of
escapeForSingleQuotedString in createFullIndex, createBlogIndex,
createPortfolioIndex, createMinimalIndex (the Heading interpolation),
createBarebonesIndex, and createAboutPage to call escapeForHtml for any value
interpolated into attributes or element text; also add a unit test in
scaffold.test.ts that generates pages with site names containing ', ", &, < to
assert the produced .astro is valid and contains the escaped text.
- Around line 396-576: The active-menu detection in setActiveMenuItem is using
currentPathname.includes(menuItem.pathname.replaceAll('/', '')) which produces
false positives; change the logic to compare pathname segments or exact paths
instead — for each menuItem (in setActiveMenuItem) normalize both
window.location.pathname and menuItem.pathname (ensure leading/trailing slashes
removed or added consistently), split them on '/' and compare the relevant
segment(s) or use strict equality for full-path matches (and treat '/' specially
for Home), so menuItem.classList.add('is-active') and aria-current are only set
when the normalized segments or exact path match.
In `@packages/create-accessible-astro-starter/src/utils.ts`:
- Around line 84-105: removeNamedFiles currently treats symlinks as regular
entries because entry.isDirectory() is false for symlinks; to avoid accidentally
removing symlinked targets, detect and skip symlinks before acting: in
removeNamedFiles (and the entries.map callback) check entry.isSymbolicLink() (or
use lstat on entryPath) and return/skip if true, otherwise keep the existing
behavior (recurse when entry.isDirectory(), compare entry.name to fileName and
call removePath). This ensures symlinks are not followed or unlinked by mistake.
In `@packages/create-accessible-astro-starter/test/e2e.ts`:
- Around line 26-58: The runCommand helper and main test need three fixes: (1)
make spawn cross-platform by resolving the executable as done in
scripts/run-local.mjs (e.g., use command + (process.platform === 'win32' ?
'.cmd' : '') or use cross-spawn) so calling runCommand('npm', ...) works on
Windows; (2) add an 'error' listener on the spawned child in runCommand to
reject the Promise on spawn errors (child.on('error', rejectPromise)); and (3)
remove the no-op assert.ok(true) in main and replace it with a real post-build
assertion (e.g., check that the built output directory like join(targetDir,
'dist') exists using fs.existsSync or fs.stat) to verify the build succeeded.
Ensure you update references to runCommand and main accordingly.
---
Nitpick comments:
In `@package.json`:
- Around line 67-71: The package.json currently pins "tailwindcss" and
"@tailwindcss/vite" to exact version "4.2.2" while other deps use caret ranges;
decide and apply a consistent policy: either change both "tailwindcss" and
"@tailwindcss/vite" to use caret ranges "^4.2.2" for parity with other deps, or
pin both packages exactly and add a brief comment in repo docs (e.g., README or
CONTRIBUTING) explaining why Tailwind is locked; update the two package names
"tailwindcss" and "@tailwindcss/vite" accordingly so they match the chosen
policy.
In `@packages/create-accessible-astro-starter/src/presets.ts`:
- Around line 83-90: The second conditional checking keepThankYouPage is
redundant because keepThankYouPage is set equal to keepContactPage; remove the
entire if (!keepThankYouPage) { pathsToDelete.add('src/pages/thank-you.astro') }
block, or if intended behavior is to allow independent control, decouple the
flags by changing where keepThankYouPage is assigned so it can differ from
keepContactPage and update callers/values accordingly; locate symbols
keepContactPage, keepThankYouPage, and pathsToDelete in presets.ts and either
delete the dead branch or adjust the flag assignment so thank-you removal can be
controlled separately.
In `@packages/create-accessible-astro-starter/src/scaffold.ts`:
- Around line 49-67: getPublishedTemplateRef currently builds a template ref
from the CLI package version and materializeTemplate always attempts to download
that tag via downloadTemplate (unless ACCESSIBLE_ASTRO_STARTER_TEMPLATE_DIR is
set), which tightly couples CLI publishing to a matching repo tag; change
materializeTemplate to handle missing or mismatched tags by attempting a safe
fallback: call downloadTemplate with the version-based ref first (using
getPublishedTemplateRef), and on failure retry with a known-good ref such as
"main" or a configurable template ref from package.json (or env var), and
surface a clear error if all attempts fail; ensure the retry logic references
materializeTemplate, getPublishedTemplateRef, and downloadTemplate so
maintainers can find and update the fallback strategy or move the template ref
into package.json later.
In `@packages/create-accessible-astro-starter/src/templates.ts`:
- Around line 1078-1241: The generated launcher blog code (launcherBlogItems /
truncateBlogTitle) currently guards on BLOG_API_URL (which may have a default)
but lacks a comment explaining that contract and it builds hrefs using
slugify(truncatedTitle) only, which can collide; add an inline comment near the
BLOG_API_URL check describing that the env has a default and that users can
override it via .env, and modify the launcherBlogItems construction (in the
async block that fetches BLOG_API_URL and in the map that builds hrefs) to
prefer a stable identifier from the API response when present (e.g., post.id or
post.slug) falling back to slugify(truncatedTitle) otherwise; reference the
symbols BLOG_API_URL, launcherBlogItems, truncateBlogTitle and slugify when
making the changes.
- Around line 173-200: createFeatureCards currently injects card.title and
card.body directly into HTML; update createFeatureCards to escape both values
using the existing/ new HTML-escaping utility (e.g., escapeForHtml) before
interpolation so untrusted input cannot break markup or inject scripts. Locate
createFeatureCards and call escapeForHtml on card.title and card.body (or
add/rename helper if none exists) to ensure safe output, and keep the rest of
the template unchanged.
- Around line 215-265: The template always injects envBlock (declared above,
based on manifest.keepBlogEnv) into the returned string with a leading newline,
which leaves a stray blank line when envBlock is ''. Update the return
construction so the leading newline is only included when envBlock is non-empty:
either build envBlock to include its own leading newline when
manifest.keepBlogEnv is true, or conditionally append `\n${envBlock}` in the
template only when envBlock !== ''. Modify references in this file to the
envBlock/manifest.keepBlogEnv logic (where viteConfig and the final defineConfig
string are assembled) so the generated astro.config has no leftover blank line.
In `@packages/create-accessible-astro-starter/src/utils.ts`:
- Around line 74-82: The current copyLocalTemplate filter in copyLocalTemplate
only checks basename and is too permissive; update the cp filter to normalize
the path and reject a broader deny-list (e.g., dist, coverage, .cache,
.DS_Store, .env, build) and also distinguish dirs vs files by using fs.lstat or
fs.stat (or the provided filter's stats if available) so you can only exclude
directory names for directory artifacts while excluding specific file names like
.DS_Store; reference the copyLocalTemplate function, the cp call, the
filter(source) callback and basename usage when implementing these checks.
In `@packages/create-accessible-astro-starter/test/scaffold.test.ts`:
- Around line 26-33: Hoist repeated environment setup by moving the assignment
to process.env.ACCESSIBLE_ASTRO_STARTER_TEMPLATE_DIR out of individual tests and
into a module-level test.before hook (and remove the per-test assignment inside
tests like the one calling scaffoldProject with options from createOptions and
buildManifest); add a test.after hook to delete
process.env.ACCESSIBLE_ASTRO_STARTER_TEMPLATE_DIR to restore global state, and
consider adding cleanup for temporary directories created via mkdtemp (e.g.,
remove tempRoot in a test.after or test.afterEach) so tests no longer leave tmp
files behind.
🪄 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
Run ID: 48664536-90fe-4bee-bbf3-168b1f89efb5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.gitignoreREADME.mdpackage.jsonpackages/create-accessible-astro-starter/README.mdpackages/create-accessible-astro-starter/package.jsonpackages/create-accessible-astro-starter/scripts/clean-output.mjspackages/create-accessible-astro-starter/scripts/run-local.mjspackages/create-accessible-astro-starter/src/cli.tspackages/create-accessible-astro-starter/src/index.tspackages/create-accessible-astro-starter/src/options.tspackages/create-accessible-astro-starter/src/presets.tspackages/create-accessible-astro-starter/src/scaffold.tspackages/create-accessible-astro-starter/src/templates.tspackages/create-accessible-astro-starter/src/types.tspackages/create-accessible-astro-starter/src/utils.tspackages/create-accessible-astro-starter/test/e2e.tspackages/create-accessible-astro-starter/test/scaffold.test.tspackages/create-accessible-astro-starter/tsconfig.jsonpackages/create-accessible-astro-starter/tsconfig.test.jsonsrc/components/CallToAction.astrosrc/components/ColorContrast.astrosrc/components/Feature.astrosrc/components/Header.astrosrc/components/Navigation.astrosrc/components/ResponsiveToggle.astrosrc/layouts/DefaultLayout.astrosrc/pages/accessible-components.astro
Comment on lines
+84
to
+105
| export async function removeNamedFiles(rootDir: string, fileName: string): Promise<void> { | ||
| if (!(await pathExists(rootDir))) { | ||
| return | ||
| } | ||
|
|
||
| const entries = await readdir(rootDir, { withFileTypes: true }) | ||
|
|
||
| await Promise.all( | ||
| entries.map(async (entry) => { | ||
| const entryPath = resolve(rootDir, entry.name) | ||
|
|
||
| if (entry.isDirectory()) { | ||
| await removeNamedFiles(entryPath, fileName) | ||
| return | ||
| } | ||
|
|
||
| if (entry.name === fileName) { | ||
| await removePath(entryPath) | ||
| } | ||
| }), | ||
| ) | ||
| } |
There was a problem hiding this comment.
removeNamedFiles doesn't handle symlinks.
entry.isDirectory() is false for symlinks (even those pointing to directories), so symlinked entries are compared as a file by name and unlinked if they match. Conversely, a symlink to a directory whose basename equals fileName will be removed even if its target isn't a regular file. For a scaffolder operating on a freshly copied template this is unlikely to bite, but worth a brief note since cp can preserve symlinks. No change needed unless the template tree may contain symlinks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/create-accessible-astro-starter/src/utils.ts` around lines 84 - 105,
removeNamedFiles currently treats symlinks as regular entries because
entry.isDirectory() is false for symlinks; to avoid accidentally removing
symlinked targets, detect and skip symlinks before acting: in removeNamedFiles
(and the entries.map callback) check entry.isSymbolicLink() (or use lstat on
entryPath) and return/skip if true, otherwise keep the existing behavior
(recurse when entry.isDirectory(), compare entry.name to fileName and call
removePath). This ensures symlinks are not followed or unlinked by mistake.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 4: The root package.json was bumped to version "5.2.0" but the CLI
package still declares version "5.1.0"; update the "version" field in the
create-accessible-astro-starter package's package.json (the package named
"create-accessible-astro-starter") to match the root (5.2.0) so the CLI and
monorepo versions track together, then run your usual monorepo/versioning checks
to ensure consistency.
- Around line 67-71: Update the pinned Tailwind package versions to allow
patch-level updates by changing the exact versions for "tailwindcss" and
"@tailwindcss/vite" from "4.2.2" to a caret range "^4.2.2"; locate the entries
for the "tailwindcss" key in the root dependencies block and the
"@tailwindcss/vite" key in the "dependencies" section and replace their version
strings accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/create-accessible-astro-starter/test/e2e.ts (1)
48-66: Note: temp directories are never cleaned up, andnpm run buildwon't execute generated<script>blocks.Two small caveats worth being aware of:
- Each preset × launcher combination creates a fresh
mkdtempdirectory (10 per run) and never removes it. On developer machines this leaks roughly 10 fullnode_modulestrees per invocation. Consider wrapping the inner loop body intry/finallyandrm(tempRoot, { recursive: true, force: true })on completion (CI is fine either way).npm run buildproduces static HTML and emits client<script>blocks verbatim — it doesn't execute them. So a syntax error insideNavigation.astro's script (see the regex/template-literal issue flagged intemplates.ts) won't fail this test. If you want this harness to catch script-level breakage, add a quicknode --check(or a headless DOM smoke test) on the generateddist/_astro/*.jschunks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-accessible-astro-starter/test/e2e.ts` around lines 48 - 66, The test's main() currently creates temporary dirs with mkdtemp into tempRoot and never removes them and it also doesn't validate generated client scripts; wrap the inner loop body (the code that sets targetDir, options, manifest, scaffoldProject, runCommand installs and build, and stat) in a try/finally so you always call rm(tempRoot, { recursive: true, force: true }) in finally to clean up, and after runCommand(npmCommand, ['run', 'build'], targetDir) add a lightweight script-level check (e.g., run node --check against the generated JS chunks under dist/_astro/*.js using runCommand and npmCommand or a direct child_process invocation) so syntax errors in generated scripts (e.g., from Navigation.astro) cause the test to fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/create-accessible-astro-starter/src/utils.ts`:
- Around line 115-141: The removeEmptyDirectories function currently deletes the
passed-in rootDir itself (via rm(rootDir, { recursive: true, force: true }))
which can remove the user's target project directory (options.targetDir) when
everything inside is pruned; update removeEmptyDirectories so it only removes
nested empty directories and never deletes the initial rootDir: change the
recursion to accept an additional flag/parameter (e.g., isRoot or stopAtRoot) or
add a wrapper so the top-level call from scaffold.ts passes a guard that
prevents rm from running on the original root, ensuring only child directories
(resolved via resolve(rootDir, entry.name) in the entries.map) are removed and
leave the root intact; also either remove or document the boolean return value
since scaffold.ts ignores it.
In `@packages/create-accessible-astro-starter/test/scaffold.test.ts`:
- Around line 252-255: The test helper expectMissing currently passes a plain
string as the second argument to assert.rejects, which Node treats as the
optional message overload; update expectMissing so assert.rejects receives a
proper error matcher (e.g., Error or a RegExp) as the second argument and the
message as the third. Locate the expectMissing function and change the call to
assert.rejects(() => readFile(file, 'utf8'), Error, `${relativePath} should be
removed`) (or use a suitable RegExp matcher) so the matcher is the second param
and the message is the third.
---
Nitpick comments:
In `@packages/create-accessible-astro-starter/test/e2e.ts`:
- Around line 48-66: The test's main() currently creates temporary dirs with
mkdtemp into tempRoot and never removes them and it also doesn't validate
generated client scripts; wrap the inner loop body (the code that sets
targetDir, options, manifest, scaffoldProject, runCommand installs and build,
and stat) in a try/finally so you always call rm(tempRoot, { recursive: true,
force: true }) in finally to clean up, and after runCommand(npmCommand, ['run',
'build'], targetDir) add a lightweight script-level check (e.g., run node
--check against the generated JS chunks under dist/_astro/*.js using runCommand
and npmCommand or a direct child_process invocation) so syntax errors in
generated scripts (e.g., from Navigation.astro) cause the test to fail.
🪄 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
Run ID: a9374e68-5467-4030-b100-4e375e0c7f52
📒 Files selected for processing (8)
package.jsonpackages/create-accessible-astro-starter/README.mdpackages/create-accessible-astro-starter/package.jsonpackages/create-accessible-astro-starter/src/cli.tspackages/create-accessible-astro-starter/src/templates.tspackages/create-accessible-astro-starter/src/utils.tspackages/create-accessible-astro-starter/test/e2e.tspackages/create-accessible-astro-starter/test/scaffold.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/create-accessible-astro-starter/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/create-accessible-astro-starter/README.md
- package.json
Comment on lines
+115
to
+141
| export async function removeEmptyDirectories(rootDir: string): Promise<boolean> { | ||
| if (!(await pathExists(rootDir))) { | ||
| return true | ||
| } | ||
|
|
||
| const metadata = await stat(rootDir) | ||
| if (!metadata.isDirectory()) { | ||
| return false | ||
| } | ||
|
|
||
| const entries = await readdir(rootDir, { withFileTypes: true }) | ||
|
|
||
| await Promise.all( | ||
| entries | ||
| .filter((entry) => entry.isDirectory()) | ||
| .map((entry) => removeEmptyDirectories(resolve(rootDir, entry.name))), | ||
| ) | ||
|
|
||
| const remainingEntries = await readdir(rootDir) | ||
|
|
||
| if (remainingEntries.length === 0) { | ||
| await rm(rootDir, { recursive: true, force: true }) | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
removeEmptyDirectories will delete rootDir itself when fully empty.
When every nested entry has been pruned, line 136 calls rm(rootDir, { recursive: true, force: true }) on the root passed in. From scaffold.ts, that root is options.targetDir, so a pathological/empty scaffold could delete the user's just-created project directory. The boolean return value is also unused by the only caller, so the "did I delete myself" signal is silently dropped.
Consider only removing nested directories and leaving the root intact (or document that the caller must guard the root):
♻️ Proposed refactor
-export async function removeEmptyDirectories(rootDir: string): Promise<boolean> {
- if (!(await pathExists(rootDir))) {
- return true
- }
-
- const metadata = await stat(rootDir)
- if (!metadata.isDirectory()) {
- return false
- }
-
- const entries = await readdir(rootDir, { withFileTypes: true })
-
- await Promise.all(
- entries
- .filter((entry) => entry.isDirectory())
- .map((entry) => removeEmptyDirectories(resolve(rootDir, entry.name))),
- )
-
- const remainingEntries = await readdir(rootDir)
-
- if (remainingEntries.length === 0) {
- await rm(rootDir, { recursive: true, force: true })
- return true
- }
-
- return false
-}
+export async function removeEmptyDirectories(rootDir: string, isRoot = true): Promise<boolean> {
+ if (!(await pathExists(rootDir))) {
+ return true
+ }
+
+ const metadata = await stat(rootDir)
+ if (!metadata.isDirectory()) {
+ return false
+ }
+
+ const entries = await readdir(rootDir, { withFileTypes: true })
+
+ await Promise.all(
+ entries
+ .filter((entry) => entry.isDirectory())
+ .map((entry) => removeEmptyDirectories(resolve(rootDir, entry.name), false)),
+ )
+
+ const remainingEntries = await readdir(rootDir)
+
+ if (remainingEntries.length === 0 && !isRoot) {
+ await rm(rootDir, { recursive: true, force: true })
+ return true
+ }
+
+ return remainingEntries.length === 0
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/create-accessible-astro-starter/src/utils.ts` around lines 115 -
141, The removeEmptyDirectories function currently deletes the passed-in rootDir
itself (via rm(rootDir, { recursive: true, force: true })) which can remove the
user's target project directory (options.targetDir) when everything inside is
pruned; update removeEmptyDirectories so it only removes nested empty
directories and never deletes the initial rootDir: change the recursion to
accept an additional flag/parameter (e.g., isRoot or stopAtRoot) or add a
wrapper so the top-level call from scaffold.ts passes a guard that prevents rm
from running on the original root, ensuring only child directories (resolved via
resolve(rootDir, entry.name) in the entries.map) are removed and leave the root
intact; also either remove or document the boolean return value since
scaffold.ts ignores it.
Comment on lines
+252
to
+255
| async function expectMissing(rootDir: string, relativePath: string): Promise<void> { | ||
| const file = resolve(rootDir, relativePath) | ||
| await assert.rejects(() => readFile(file, 'utf8'), `${relativePath} should be removed`) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js assert.rejects: is the second argument allowed to be a plain string in Node 20+, and does it serve as the assertion message?
💡 Result:
In Node.js 20+ (and still in current docs), the second argument to assert.rejects is not allowed to be a plain string as the “error matcher” parameter. - Signature: assert.rejects(asyncFn[, error][, message]). - The “error” parameter cannot be a string. The docs explicitly say: if a string is provided as the second argument, then error is assumed to be omitted and the string is treated as the optional “message” argument instead. - Therefore, a plain string as the second argument does serve as the assertion message (not as the error matcher). However, relying on this overload is discouraged: the docs warn that it’s easy to make mistakes and mention that this confusing notation can lead to specific ambiguous-argument behavior. Recommendation: if you want to supply an assertion message, pass it as the third argument: assert.rejects(fn, expectedErrorMatcher, 'your message'). If you want to match on error messages/content, prefer passing a RegExp or predicate via the second argument, not a string.
Citations:
Correct the assert.rejects signature to follow Node.js best practices.
The second argument to assert.rejects should be an error matcher (Class/RegExp/validation object/function), not a plain string. When a string is passed as the second argument, Node.js treats it as the optional message parameter due to an overload—this confusing notation is explicitly discouraged in the Node.js documentation. Pass the message as the third argument instead for clarity and adherence to best practices:
♻️ Proposed fix
- await assert.rejects(() => readFile(file, 'utf8'), `${relativePath} should be removed`)
+ await assert.rejects(() => readFile(file, 'utf8'), undefined, `${relativePath} should be removed`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function expectMissing(rootDir: string, relativePath: string): Promise<void> { | |
| const file = resolve(rootDir, relativePath) | |
| await assert.rejects(() => readFile(file, 'utf8'), `${relativePath} should be removed`) | |
| } | |
| async function expectMissing(rootDir: string, relativePath: string): Promise<void> { | |
| const file = resolve(rootDir, relativePath) | |
| await assert.rejects(() => readFile(file, 'utf8'), undefined, `${relativePath} should be removed`) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/create-accessible-astro-starter/test/scaffold.test.ts` around lines
252 - 255, The test helper expectMissing currently passes a plain string as the
second argument to assert.rejects, which Node treats as the optional message
overload; update expectMissing so assert.rejects receives a proper error matcher
(e.g., Error or a RegExp) as the second argument and the message as the third.
Locate the expectMissing function and change the call to assert.rejects(() =>
readFile(file, 'utf8'), Error, `${relativePath} should be removed`) (or use a
suitable RegExp matcher) so the matcher is the second param and the message is
the third.
peterpadberg
previously approved these changes
May 2, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/create-accessible-astro-starter/package.json (1)
22-22: ⚡ Quick win
@clack/prompts ^0.10.1is locked to the 0.x series while the stable release line is 1.xThe latest published version of
@clack/promptsis 1.3.0. The caret range^0.10.1only allows semver updates within0.x.x, so this package will never resolve to any1.xrelease. All consumers and CI jobs will install an old pre-stable build, missing every fix and API addition shipped in the 1.x line.📦 Proposed fix
- "@clack/prompts": "^0.10.1", + "@clack/prompts": "^1.0.0",Verify that the
cli.tsimport surface is compatible with the 1.x API before updating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-accessible-astro-starter/package.json` at line 22, Update the `@clack/prompts` dependency from ^0.10.1 to a 1.x caret range (e.g., ^1.3.0) in package.json so consumers receive the stable 1.x releases, then audit the CLI import surface in cli.ts to ensure every imported symbol and API usage (functions/types you call from `@clack/prompts`) matches the 1.x API and adjust call sites to the new signatures if needed; once verified, commit the package.json change and run CI/locally to confirm no runtime or type errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/create-accessible-astro-starter/src/cli.ts`:
- Around line 127-131: The spinner started with p.spinner() is not stopped if
scaffoldProject(options, manifest) throws; wrap the await scaffoldProject(...)
call in a try/finally so spinner.stop(...) is always invoked: call
spinner.stop(`Created ${options.siteName}`) on successful completion and ensure
spinner.stop() (or spinner.fail(...) if you prefer) is executed in the finally
block to guarantee the terminal spinner is cleaned up even on errors; reference
spinner, p.spinner(), scaffoldProject, and options.siteName when making the
change.
---
Nitpick comments:
In `@packages/create-accessible-astro-starter/package.json`:
- Line 22: Update the `@clack/prompts` dependency from ^0.10.1 to a 1.x caret
range (e.g., ^1.3.0) in package.json so consumers receive the stable 1.x
releases, then audit the CLI import surface in cli.ts to ensure every imported
symbol and API usage (functions/types you call from `@clack/prompts`) matches the
1.x API and adjust call sites to the new signatures if needed; once verified,
commit the package.json change and run CI/locally to confirm no runtime or type
errors.
🪄 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
Run ID: 62c6431d-65dc-4e6f-a332-10ffa383b39c
📒 Files selected for processing (4)
packages/create-accessible-astro-starter/package.jsonpackages/create-accessible-astro-starter/src/cli-output.tspackages/create-accessible-astro-starter/src/cli.tspackages/create-accessible-astro-starter/test/cli-output.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/create-accessible-astro-starter/test/cli-output.test.ts
Comment on lines
+127
to
+131
| const spinner = p.spinner() | ||
|
|
||
| spinner.start('Scaffolding your project') | ||
| await scaffoldProject(options, manifest) | ||
| spinner.stop(`Created ${options.siteName}`) |
There was a problem hiding this comment.
Spinner is not stopped on scaffoldProject failure, leaving the terminal in an inconsistent state.
If scaffoldProject throws, spinner.stop() is never called and the error propagates up to the index.ts catch block, where console.error writes over a still-"running" spinner.
🛠️ Proposed fix
spinner.start('Scaffolding your project')
- await scaffoldProject(options, manifest)
- spinner.stop(`Created ${options.siteName}`)
+
+ try {
+ await scaffoldProject(options, manifest)
+ } catch (error) {
+ spinner.stop('Scaffolding failed')
+ throw error
+ }
+
+ spinner.stop(`Created ${options.siteName}`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const spinner = p.spinner() | |
| spinner.start('Scaffolding your project') | |
| await scaffoldProject(options, manifest) | |
| spinner.stop(`Created ${options.siteName}`) | |
| const spinner = p.spinner() | |
| spinner.start('Scaffolding your project') | |
| try { | |
| await scaffoldProject(options, manifest) | |
| } catch (error) { | |
| spinner.stop('Scaffolding failed') | |
| throw error | |
| } | |
| spinner.stop(`Created ${options.siteName}`) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/create-accessible-astro-starter/src/cli.ts` around lines 127 - 131,
The spinner started with p.spinner() is not stopped if scaffoldProject(options,
manifest) throws; wrap the await scaffoldProject(...) call in a try/finally so
spinner.stop(...) is always invoked: call spinner.stop(`Created
${options.siteName}`) on successful completion and ensure spinner.stop() (or
spinner.fail(...) if you prefer) is executed in the finally block to guarantee
the terminal spinner is cleaned up even on errors; reference spinner,
p.spinner(), scaffoldProject, and options.siteName when making the change.
This reverts commit aaccfc0.
peterpadberg
force-pushed
the
feat/cli-tool
branch
from
d4b84c9 to
3d2ef41
Compare
peterpadberg
approved these changes
May 2, 2026
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
2 participants
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.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests