feat(packages): 4 web-tooling channel packages (Astro, Vite, Storybook, VS Code)#146
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds three new packages for Astro, Storybook, and Vite HTML scanning and reporting, plus VS Code extension packaging support. Each package includes build and test config, documentation, licensing metadata, and a build-failure path based on scan severity. ChangesAriada Astro Integration
Ariada Storybook Addon
Ariada Vite Plugin
VS Code Extension Packaging
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
predopta
left a comment
predopta
left a comment
There was a problem hiding this comment.
Reviewed the four web-tooling packages — source, tests, and OSS docs are complete and consistent. Approving.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| function stripTags(value: string): string { | ||
| return value.replaceAll(/<[^>]+>/g, ''); |
| } | ||
|
|
||
| function stripTags(value: string): string { | ||
| return value.replaceAll(/<[^>]+>/g, ''); |
| } | ||
|
|
||
| function stripTags(value: string): string { | ||
| return value.replaceAll(/<[^>]+>/g, ''); |
| if (type === 'hidden' || type === 'submit' || type === 'button') continue; | ||
| if (attr(tag, 'aria-label') || attr(tag, 'aria-labelledby')) continue; | ||
| const id = attr(tag, 'id'); | ||
| if (id && new RegExp(`<label\\b[^>]*for=["']?${escapeRegExp(id)}["']?`, 'i').test(html)) continue; |
| } | ||
|
|
||
| function attr(tag: string, name: string): string | undefined { | ||
| const pattern = new RegExp(`\\s${name}\\s*=\\s*["']([^"']+)["']`, 'i'); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
packages/ariada-storybook-addon/src/scan.ts (1)
35-75: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRegex-based HTML scanning has inherent edge-case gaps.
findStaticHtmlIssuesparses HTML with regex (Lines 37, 55), which can mis-handle self-closing/void tag variants, attributes containing>in quoted strings, or nested/malformed markup. For a lightweight built-in fallback scanner (per the README, this is explicitly the non-hosted default) this is an acceptable trade-off, but consider a minimal DOM parser (e.g.linkedom) if false negatives/positives become a real concern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ariada-storybook-addon/src/scan.ts` around lines 35 - 75, findStaticHtmlIssues currently relies on regex to inspect <img> and <button> markup, which can miss or misread valid HTML in edge cases. Update the scanner to parse the HTML with a lightweight DOM parser instead of matching raw strings, then keep the existing image-alt and button-name checks by querying the parsed nodes and building the same AriadaFinding results. Use findStaticHtmlIssues, imagePattern/buttonPattern, and the existing finding IDs/selectors as the entry points to preserve behavior.packages/ariada-storybook-addon/package.json (2)
8-10: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
main/modulepoint to the ESM build even thoughrequireexports resolve todist-cjs.With
"type": "module",main: "./dist/index.js"is an ES module. Modern resolvers ignoremainin favor ofexports, but any legacy tool that falls back tomain(bundlers/resolvers without fullexportssupport) will attemptrequire()on an ESM file and fail withERR_REQUIRE_ESM. Standard dual-package layouts pointmainat the CJS build for this fallback case.Proposed fix
- "main": "./dist/index.js", + "main": "./dist-cjs/index.js", "module": "./dist/index.js",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ariada-storybook-addon/package.json` around lines 8 - 10, The package entry fields are pointing legacy resolvers at the ESM build instead of the CommonJS fallback. Update package.json so the fallback entry for main uses the dist-cjs build while keeping exports, module, and types aligned with the dual-package layout; check the existing export targets in the package manifest and adjust the main field accordingly.
43-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
LICENSES/directory is not included in the publishedfilesarray.
REUSE.toml/LICENSEreference per-file license texts underLICENSES/EUPL-1.2.txt, but that directory isn't listed here, so it won't ship in the published tarball.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ariada-storybook-addon/package.json` around lines 43 - 51, The package.json files list for ariada-storybook-addon is missing the LICENSES directory, so the published tarball will omit per-file license texts referenced by REUSE.toml and LICENSE. Update the files array to include LICENSES alongside the existing entries, using the package.json files section as the place to fix it so the LICENSES/EUPL-1.2.txt content ships with the package.packages/ariada-astro/src/scan.ts (1)
58-78: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftRegex-based HTML parsing is fragile for an accessibility scanner.
/<img\b[^>]*>/giwill mis-parse tags containing>inside quoted attribute values (e.g.,<img src="a>b.png">), commented-out markup, or attributes split unusually, producing false negatives/positives in a tool whose core value is scan accuracy. Consider a lightweight HTML parser (e.g.,parse5orhtmlparser2) for the attribute-presence check instead of regex matching, since accuracy directly affects the build-failure gating this scanner drives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ariada-astro/src/scan.ts` around lines 58 - 78, The `findStaticHtmlIssues` scanner currently uses the `imagePattern` regex and `RegExp.exec` to detect `<img>` tags, which is too fragile for accurate accessibility findings. Replace the regex-based tag parsing in `findStaticHtmlIssues` with a lightweight HTML parser approach (for example via `parse5` or `htmlparser2`) so you can inspect each image element’s attributes reliably, including the `alt` presence check. Keep the existing `HtmlFinding` shape, `ruleId` of `image-alt`, and `selector` generation logic in `findStaticHtmlIssues`, but derive them from parsed DOM nodes instead of raw string matching.packages/ariada-astro/tests/integration.test.ts (1)
36-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
file://URL construction assumes POSIX paths.
new URL(\file://${root}/`)will not produce a valid file URL ifrootever contains a Windows drive letter (mkdtemp/tmpdir()on Windows returns e.g.C:\Users...). Since this only affects local/Windows CI runners and the rest of the suite doesn't target Windows, this is low priority, but considerpathToFileURL(root)fromnode:url` for portability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ariada-astro/tests/integration.test.ts` around lines 36 - 38, The test’s file URL creation is not portable because it builds a file:// URL from a raw path string in the integration test around astro:build:done. Replace the manual new URL(`file://${root}/`) construction with a Windows-safe file URL helper such as pathToFileURL in the test setup so the hook receives a valid dir URL across platforms.packages/ariada-astro/src/index.ts (3)
61-75: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential file reads limit scan throughput on large builds.
scanAstroBuildawaitsreadFile+scanner()one file at a time. For sites with hundreds of generated HTML pages this serializes all disk I/O unnecessarily.♻️ Suggested parallelization
- const pages = []; - - for (const filePath of htmlFiles) { - const html = await readFile(filePath, 'utf8'); - pages.push(await scanner({ filePath, html })); - } - - return buildReport(pages); + const pages = await Promise.all( + htmlFiles.map(async (filePath) => { + const html = await readFile(filePath, 'utf8'); + return scanner({ filePath, html }); + }), + ); + + return buildReport(pages);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ariada-astro/src/index.ts` around lines 61 - 75, scanAstroBuild is reading and scanning each HTML file sequentially, which slows large builds. Update the scanAstroBuild flow so the readFile and scanner work for each filePath happen in parallel across the htmlFiles list, while still collecting all page results before buildReport. Keep the existing scanner default behavior and use the scanAstroBuild, listHtmlFiles, readFile, and buildReport symbols to localize the change.
77-91: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNo symlink-cycle protection in recursive directory walk.
listHtmlFilesrecurses on every directory without tracking visited paths. A symlink loop in a build's output directory (e.g. from a misconfigured static asset copy) would cause infinite recursion / stack exhaustion. Low likelihood in typical Astro output but worth a defensive guard given this runs unattended in CI build pipelines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ariada-astro/src/index.ts` around lines 77 - 91, The recursive directory walk in listHtmlFiles does not guard against symlink cycles, so a looped directory tree can recurse forever. Update listHtmlFiles in index.ts to track visited directories by resolved real path or inode before descending, and skip already-seen entries when entry.isDirectory() is true. Keep the existing HTML collection logic and sorting, but ensure the recursion uses the new cycle check so misconfigured symlinks cannot exhaust the stack.
30-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Astro’s exported integration types here. The custom
AstroIntegrationLike/AstroBuildDoneOptionsonly modeldir, so this can drift from Astro’s hook contract without a compile-time signal;astrocan stay optional at runtime and be added as a devDependency for types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ariada-astro/src/index.ts` around lines 30 - 57, The ariada integration is using custom hook types that only model dir, which can drift from Astro’s real contract. Update the ariada export to use Astro’s exported integration types for the integration object and the astro:build:done hook parameter, while keeping astro optional at runtime and as a devDependency for type-only use. Keep the existing ariada function and its hook logic, but replace AstroIntegrationLike/AstroBuildDoneOptions with the corresponding Astro types so type-checking reflects Astro’s actual API.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ariada-astro/package.json`:
- Around line 45-47: The declared node engine in the package metadata does not
match the CI runtime, so update the constraint in the package’s engines.node
entry or adjust the CI image to the same major version used by the workspace.
Use the package.json engines block as the source of truth and decide whether
packages in this workspace should support Node 20 or require Node 22, then make
the version requirement and CI runner consistent.
- Around line 31-39: The pnpm lockfile is out of sync with the dependencies
declared in package.json, causing frozen-lockfile CI installs to fail.
Regenerate and commit pnpm-lock.yaml after the changes to
packages/ariada-astro/package.json so the entries for astro, `@types/node`,
rimraf, typescript, vitest, and the peerDependencies section are reflected
correctly; use the package manifest and lockfile as the source of truth and
verify the updated lockfile no longer contains empty specifiers for this
package.
In `@packages/ariada-storybook-addon/src/panel.ts`:
- Around line 41-51: The HTML built in renderPanelHtml is interpolating
finding.severity without escaping, unlike the other dynamic fields. Update the
map callback in renderPanelHtml to pass severity through escapeHtml before
inserting it into the <li> content, so custom scanner output from
createAriadaDecorator cannot inject raw HTML into the panel.
In `@packages/ariada-storybook-addon/src/preview.ts`:
- Around line 4-6: The preview setup creates createAriadaDecorator() without a
Storybook channel, so the decorator never emits EVENTS.scanCompleted in the
shipped preview. Update the preview.ts wiring so the channel is resolved and
passed into createAriadaDecorator(), or move the channel lookup inside
createAriadaDecorator() itself. Use the existing decorators export and the
createAriadaDecorator symbol to keep the manager panel receiving scan results.
In `@packages/ariada-storybook-addon/tsconfig.cjs.json`:
- Around line 3-11: The CJS build config in tsconfig.cjs.json disables
declaration output, which conflicts with the require-side type entries in
package.json for the main, preview, and manager exports. Update the
tsconfig.cjs.json compilerOptions so the CommonJS build emits declarations, and
ensure scripts/write-cjs-package.cjs converts the generated .d.ts files to
.d.cts for the CJS artifacts. If CJS type support is not intended, remove the
require/types conditions from the affected exports instead.
In `@packages/ariada-vite-plugin/package.json`:
- Around line 45-47: The package manifest declares an engines.node requirement
of >=22, but the CI workspace is still running Node 20.20.2, causing the engine
mismatch warning. Update the CI/tooling to install and run Node 22+ if that is
the intended runtime, or relax the engines.node constraint in the package
manifest to match the actual pipeline version; check the package.json engines
field and the workspace install configuration together so they stay aligned.
In `@packages/ariada-vite-plugin/src/index.ts`:
- Around line 32-38: The HTML capture in VitePluginLike is keyed too broadly,
causing multi-page HTML to overwrite and build-time HTML to be re-counted.
Update transformIndexHtml to accept the hook context and store entries by the
actual HTML entry path from ctx.filename or ctx.path instead of
resolve(config.root, 'index.html'), or explicitly skip non-dev-server captures
if this hook is meant to be dev-only. Make sure the change is applied where
VitePluginLike, configResolved, and transformIndexHtml interact with
scanViteOutput.
In `@packages/ariada-vite-plugin/src/scan.ts`:
- Around line 58-93: The form-field-name scan in findStaticHtmlIssues is
incorrectly suppressing all unlabeled input findings whenever any <label> exists
in the document. Remove the blanket labelTexts.size > 0 shortcut, and instead
determine labeling per input in the inputPattern loop by checking only the
current input’s explicit or implicit association. Use the existing helpers and
symbols (findStaticHtmlIssues, labelPattern, inputPattern, attr, escapeRegExp)
to keep the logic localized, and if wrapped labels are meant to be supported,
replace the global labelTexts check with a specific containment/span match for
that input.
---
Nitpick comments:
In `@packages/ariada-astro/src/index.ts`:
- Around line 61-75: scanAstroBuild is reading and scanning each HTML file
sequentially, which slows large builds. Update the scanAstroBuild flow so the
readFile and scanner work for each filePath happen in parallel across the
htmlFiles list, while still collecting all page results before buildReport. Keep
the existing scanner default behavior and use the scanAstroBuild, listHtmlFiles,
readFile, and buildReport symbols to localize the change.
- Around line 77-91: The recursive directory walk in listHtmlFiles does not
guard against symlink cycles, so a looped directory tree can recurse forever.
Update listHtmlFiles in index.ts to track visited directories by resolved real
path or inode before descending, and skip already-seen entries when
entry.isDirectory() is true. Keep the existing HTML collection logic and
sorting, but ensure the recursion uses the new cycle check so misconfigured
symlinks cannot exhaust the stack.
- Around line 30-57: The ariada integration is using custom hook types that only
model dir, which can drift from Astro’s real contract. Update the ariada export
to use Astro’s exported integration types for the integration object and the
astro:build:done hook parameter, while keeping astro optional at runtime and as
a devDependency for type-only use. Keep the existing ariada function and its
hook logic, but replace AstroIntegrationLike/AstroBuildDoneOptions with the
corresponding Astro types so type-checking reflects Astro’s actual API.
In `@packages/ariada-astro/src/scan.ts`:
- Around line 58-78: The `findStaticHtmlIssues` scanner currently uses the
`imagePattern` regex and `RegExp.exec` to detect `<img>` tags, which is too
fragile for accurate accessibility findings. Replace the regex-based tag parsing
in `findStaticHtmlIssues` with a lightweight HTML parser approach (for example
via `parse5` or `htmlparser2`) so you can inspect each image element’s
attributes reliably, including the `alt` presence check. Keep the existing
`HtmlFinding` shape, `ruleId` of `image-alt`, and `selector` generation logic in
`findStaticHtmlIssues`, but derive them from parsed DOM nodes instead of raw
string matching.
In `@packages/ariada-astro/tests/integration.test.ts`:
- Around line 36-38: The test’s file URL creation is not portable because it
builds a file:// URL from a raw path string in the integration test around
astro:build:done. Replace the manual new URL(`file://${root}/`) construction
with a Windows-safe file URL helper such as pathToFileURL in the test setup so
the hook receives a valid dir URL across platforms.
In `@packages/ariada-storybook-addon/package.json`:
- Around line 8-10: The package entry fields are pointing legacy resolvers at
the ESM build instead of the CommonJS fallback. Update package.json so the
fallback entry for main uses the dist-cjs build while keeping exports, module,
and types aligned with the dual-package layout; check the existing export
targets in the package manifest and adjust the main field accordingly.
- Around line 43-51: The package.json files list for ariada-storybook-addon is
missing the LICENSES directory, so the published tarball will omit per-file
license texts referenced by REUSE.toml and LICENSE. Update the files array to
include LICENSES alongside the existing entries, using the package.json files
section as the place to fix it so the LICENSES/EUPL-1.2.txt content ships with
the package.
In `@packages/ariada-storybook-addon/src/scan.ts`:
- Around line 35-75: findStaticHtmlIssues currently relies on regex to inspect
<img> and <button> markup, which can miss or misread valid HTML in edge cases.
Update the scanner to parse the HTML with a lightweight DOM parser instead of
matching raw strings, then keep the existing image-alt and button-name checks by
querying the parsed nodes and building the same AriadaFinding results. Use
findStaticHtmlIssues, imagePattern/buttonPattern, and the existing finding
IDs/selectors as the entry points to preserve behavior.
🪄 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: 5126e8ef-0d3d-4e63-a00d-1080ca6aea38
⛔ Files ignored due to path filters (1)
packages/vscode-extension/icon.pngis excluded by!**/*.png
📒 Files selected for processing (53)
packages/ariada-astro/CHANGELOG.mdpackages/ariada-astro/LICENSEpackages/ariada-astro/LICENSES/EUPL-1.2.txtpackages/ariada-astro/NOTICEpackages/ariada-astro/README.mdpackages/ariada-astro/REUSE.tomlpackages/ariada-astro/SECURITY.mdpackages/ariada-astro/package.jsonpackages/ariada-astro/src/index.tspackages/ariada-astro/src/report.tspackages/ariada-astro/src/scan.tspackages/ariada-astro/tests/integration.test.tspackages/ariada-astro/tsconfig.jsonpackages/ariada-astro/vitest.config.tspackages/ariada-storybook-addon/.gitignorepackages/ariada-storybook-addon/CHANGELOG.mdpackages/ariada-storybook-addon/LICENSEpackages/ariada-storybook-addon/LICENSES/EUPL-1.2.txtpackages/ariada-storybook-addon/NOTICEpackages/ariada-storybook-addon/README.mdpackages/ariada-storybook-addon/REUSE.tomlpackages/ariada-storybook-addon/SECURITY.mdpackages/ariada-storybook-addon/package.jsonpackages/ariada-storybook-addon/scripts/write-cjs-package.cjspackages/ariada-storybook-addon/src/constants.tspackages/ariada-storybook-addon/src/decorator.tspackages/ariada-storybook-addon/src/index.tspackages/ariada-storybook-addon/src/manager.tspackages/ariada-storybook-addon/src/panel.tspackages/ariada-storybook-addon/src/preview.tspackages/ariada-storybook-addon/src/scan.tspackages/ariada-storybook-addon/tests/addon.test.tspackages/ariada-storybook-addon/tsconfig.cjs.jsonpackages/ariada-storybook-addon/tsconfig.jsonpackages/ariada-storybook-addon/vitest.config.tspackages/ariada-vite-plugin/CHANGELOG.mdpackages/ariada-vite-plugin/LICENSEpackages/ariada-vite-plugin/LICENSES/EUPL-1.2.txtpackages/ariada-vite-plugin/NOTICEpackages/ariada-vite-plugin/README.mdpackages/ariada-vite-plugin/REUSE.tomlpackages/ariada-vite-plugin/SECURITY.mdpackages/ariada-vite-plugin/package.jsonpackages/ariada-vite-plugin/src/index.tspackages/ariada-vite-plugin/src/scan.tspackages/ariada-vite-plugin/tests/plugin.test.tspackages/ariada-vite-plugin/tsconfig.jsonpackages/ariada-vite-plugin/vitest.config.tspackages/vscode-extension/.vscodeignorepackages/vscode-extension/README.mdpackages/vscode-extension/package.jsonpackages/vscode-extension/tools/vsce-wrapper/package.jsonpackages/vscode-extension/tools/vsce-wrapper/vsce.js
| "engines": { | ||
| "node": ">=22" | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
engines.node requirement mismatches CI runtime.
SonarCloud pipeline reports an unsupported-engine warning: the workspace declares node >=22 while the CI runner uses Node v20.20.2. This doesn't fail SonarCloud today but signals a runtime mismatch that could cause installs to break more strictly elsewhere; align the CI image or relax the constraint if v20 support is intended.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ariada-astro/package.json` around lines 45 - 47, The declared node
engine in the package metadata does not match the CI runtime, so update the
constraint in the package’s engines.node entry or adjust the CI image to the
same major version used by the workspace. Use the package.json engines block as
the source of truth and decide whether packages in this workspace should support
Node 20 or require Node 22, then make the version requirement and CI runner
consistent.
Source: Pipeline failures
| export function renderPanelHtml(result: StoryScanResult | undefined): string { | ||
| const model = createPanelViewModel(result); | ||
| const items = model.findings | ||
| .map( | ||
| (finding) => | ||
| `<li><strong>${escapeHtml(finding.ruleId)}</strong> [${finding.severity}] ${escapeHtml(finding.message)} <code>${escapeHtml(finding.selector)}</code></li>`, | ||
| ) | ||
| .join(''); | ||
|
|
||
| return `<section data-ariada-status="${model.status}"><h2>${model.heading}</h2><p>${escapeHtml(model.summary)}</p><ul>${items}</ul></section>`; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
finding.severity is interpolated unescaped.
Every other dynamic field (ruleId, message, selector, summary) is passed through escapeHtml, but finding.severity at Line 46 is inserted raw into the HTML string. AriadaFinding.severity is typed as a literal union at compile time, but the scanner is injectable (createAriadaDecorator accepts a custom scanner), so at runtime a non-conforming scanner implementation could supply an arbitrary string here that is not type-checked, producing an HTML/script injection in the manager panel.
🔒 Proposed fix
- `<li><strong>${escapeHtml(finding.ruleId)}</strong> [${finding.severity}] ${escapeHtml(finding.message)} <code>${escapeHtml(finding.selector)}</code></li>`,
+ `<li><strong>${escapeHtml(finding.ruleId)}</strong> [${escapeHtml(finding.severity)}] ${escapeHtml(finding.message)} <code>${escapeHtml(finding.selector)}</code></li>`,📝 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.
| export function renderPanelHtml(result: StoryScanResult | undefined): string { | |
| const model = createPanelViewModel(result); | |
| const items = model.findings | |
| .map( | |
| (finding) => | |
| `<li><strong>${escapeHtml(finding.ruleId)}</strong> [${finding.severity}] ${escapeHtml(finding.message)} <code>${escapeHtml(finding.selector)}</code></li>`, | |
| ) | |
| .join(''); | |
| return `<section data-ariada-status="${model.status}"><h2>${model.heading}</h2><p>${escapeHtml(model.summary)}</p><ul>${items}</ul></section>`; | |
| } | |
| export function renderPanelHtml(result: StoryScanResult | undefined): string { | |
| const model = createPanelViewModel(result); | |
| const items = model.findings | |
| .map( | |
| (finding) => | |
| `<li><strong>${escapeHtml(finding.ruleId)}</strong> [${escapeHtml(finding.severity)}] ${escapeHtml(finding.message)} <code>${escapeHtml(finding.selector)}</code></li>`, | |
| ) | |
| .join(''); | |
| return `<section data-ariada-status="${model.status}"><h2>${model.heading}</h2><p>${escapeHtml(model.summary)}</p><ul>${items}</ul></section>`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ariada-storybook-addon/src/panel.ts` around lines 41 - 51, The HTML
built in renderPanelHtml is interpolating finding.severity without escaping,
unlike the other dynamic fields. Update the map callback in renderPanelHtml to
pass severity through escapeHtml before inserting it into the <li> content, so
custom scanner output from createAriadaDecorator cannot inject raw HTML into the
panel.
| import { createAriadaDecorator } from './decorator.js'; | ||
|
|
||
| export const decorators = [createAriadaDecorator()]; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files 'packages/ariada-storybook-addon/src/*'
printf '\n== outline preview.ts ==\n'
ast-grep outline packages/ariada-storybook-addon/src/preview.ts --view expanded || true
printf '\n== outline decorator.ts ==\n'
ast-grep outline packages/ariada-storybook-addon/src/decorator.ts --view expanded || true
printf '\n== search createAriadaDecorator/channel/storybook hooks ==\n'
rg -n "createAriadaDecorator|channel|addons\.getChannel|useChannel|EVENTS\.scanCompleted|addons" packages/ariada-storybook-addon -SRepository: ariada-org/ariada
Length of output: 3624
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file sizes ==\n'
wc -l packages/ariada-storybook-addon/src/preview.ts packages/ariada-storybook-addon/src/decorator.ts
printf '\n== preview.ts ==\n'
cat -n packages/ariada-storybook-addon/src/preview.ts
printf '\n== decorator.ts (1-220) ==\n'
sed -n '1,220p' packages/ariada-storybook-addon/src/decorator.ts
printf '\n== addon package search ==\n'
rg -n "createAriadaDecorator|channel|addons\.getChannel|useChannel|EVENTS\.scanCompleted|register|managerEntries|previewEntries|addons\.getChannel" packages/ariada-storybook-addon -SRepository: ariada-org/ariada
Length of output: 5375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== manager.ts ==\n'
cat -n packages/ariada-storybook-addon/src/manager.ts
printf '\n== README excerpt ==\n'
sed -n '1,120p' packages/ariada-storybook-addon/README.md
printf '\n== tests/addon.test.ts ==\n'
sed -n '1,120p' packages/ariada-storybook-addon/tests/addon.test.tsRepository: ariada-org/ariada
Length of output: 4012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search latest/shared state ==\n'
rg -n "\blatest\b|scanCompleted|StoryScanResult|currentResult|lastResult|latest\?" packages/ariada-storybook-addon/src packages/ariada-storybook-addon/tests -S
printf '\n== index.ts ==\n'
cat -n packages/ariada-storybook-addon/src/index.ts
printf '\n== panel.ts ==\n'
cat -n packages/ariada-storybook-addon/src/panel.tsRepository: ariada-org/ariada
Length of output: 4650
Pass the Storybook channel into the preview decorator. createAriadaDecorator() is instantiated without a channel here, and the decorator only emits EVENTS.scanCompleted when one is provided. In the shipped preview setup, that leaves the manager panel with no scan results, so either resolve the channel here or move the lookup into createAriadaDecorator().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ariada-storybook-addon/src/preview.ts` around lines 4 - 6, The
preview setup creates createAriadaDecorator() without a Storybook channel, so
the decorator never emits EVENTS.scanCompleted in the shipped preview. Update
the preview.ts wiring so the channel is resolved and passed into
createAriadaDecorator(), or move the channel lookup inside
createAriadaDecorator() itself. Use the existing decorators export and the
createAriadaDecorator symbol to keep the manager panel receiving scan results.
| "compilerOptions": { | ||
| "outDir": "dist-cjs", | ||
| "module": "CommonJS", | ||
| "moduleResolution": "Node", | ||
| "declaration": false, | ||
| "declarationMap": false, | ||
| "sourceMap": true, | ||
| "verbatimModuleSyntax": false | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
declaration: false breaks the .d.cts types declared in package.json exports.
This config disables declaration emission for the CJS build, so no .d.ts/.d.cts files will ever be produced in dist-cjs. However, packages/ariada-storybook-addon/package.json declares "require": {"types": "./dist-cjs/index.d.cts", ...} (and the same for ./preview and ./manager subpaths). TypeScript consumers resolving via require will fail to find these type files.
Either enable declaration emission here (and have scripts/write-cjs-package.cjs rename .d.ts → .d.cts), or drop the types condition from the require exports if CJS consumers aren't meant to get type support.
Proposed fix
"compilerOptions": {
"outDir": "dist-cjs",
"module": "CommonJS",
"moduleResolution": "Node",
- "declaration": false,
- "declarationMap": false,
+ "declaration": true,
+ "declarationMap": false,
"sourceMap": true,
"verbatimModuleSyntax": false
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ariada-storybook-addon/tsconfig.cjs.json` around lines 3 - 11, The
CJS build config in tsconfig.cjs.json disables declaration output, which
conflicts with the require-side type entries in package.json for the main,
preview, and manager exports. Update the tsconfig.cjs.json compilerOptions so
the CommonJS build emits declarations, and ensure scripts/write-cjs-package.cjs
converts the generated .d.ts files to .d.cts for the CJS artifacts. If CJS type
support is not intended, remove the require/types conditions from the affected
exports instead.
| "engines": { | ||
| "node": ">=22" | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
CI reports engine mismatch for node >=22.
Pipeline log shows the workspace installs with node v20.20.2, triggering an "Unsupported engine" warning against this engines.node: >=22 constraint. Confirm the CI toolchain is intended to upgrade to Node 22+, or relax the constraint to match the actual pipeline runtime; otherwise installs will keep emitting engine warnings across dependent workspace projects.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ariada-vite-plugin/package.json` around lines 45 - 47, The package
manifest declares an engines.node requirement of >=22, but the CI workspace is
still running Node 20.20.2, causing the engine mismatch warning. Update the
CI/tooling to install and run Node 22+ if that is the intended runtime, or relax
the engines.node constraint in the package manifest to match the actual pipeline
version; check the package.json engines field and the workspace install
configuration together so they stay aligned.
Source: Pipeline failures
| export interface VitePluginLike { | ||
| name: string; | ||
| enforce: 'post'; | ||
| configResolved(config: ViteConfigResolvedLike): void; | ||
| transformIndexHtml(html: string): Promise<string>; | ||
| closeBundle(): Promise<void>; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Vite's transformIndexHtml hook run during vite build for every HTML entry point, and does ctx.filename differ per entry?
💡 Result:
Yes, the transformIndexHtml hook runs during vite build for HTML files, and the context (ctx) does provide the specific filename for each file being processed. During the build process, the transformIndexHtml hook is executed for HTML entry points defined in your project [1]. When multiple HTML files are processed, the hook is invoked separately for each one, and the ctx.filename property correctly reflects the path of the specific HTML file currently being transformed [2]. It is important to note that while Vite typically focuses on entry-point HTML files, the behavior can sometimes extend to other HTML files depending on how they are imported or referenced in the project [2]. If you are relying on transformIndexHtml to modify your HTML, ensure your files are recognized as entry points (often by configuring them in rollupOptions.input if they are not picked up automatically) to ensure consistent behavior across different environments [3].
Citations:
- 1: https://vite.dev/guide/api-plugin
- 2: Plugin
transformIndexHtmlcalled with a*.htmlid that'simported and not an entrypoint duringvite build. vitejs/vite#20308 - 3: https://stackoverflow.com/questions/77115733/vite-custom-plugin-transformindexhtml-hook-not-working-during-build
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby symbols first.
ast-grep outline packages/ariada-vite-plugin/src/index.ts --view expanded || true
# Read the target file with line numbers.
wc -l packages/ariada-vite-plugin/src/index.ts
cat -n packages/ariada-vite-plugin/src/index.ts | sed -n '1,220p'
# Find related scanning logic and tests.
rg -n "scanDevHtml|transformIndexHtml|devPages|scanViteOutput|failOn|buildDir|ctx\.filename|ctx\.path" packages/ariada-vite-plugin -SRepository: ariada-org/ariada
Length of output: 7252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect package tests that cover HTML scanning behavior.
fd -a ".*(test|spec)\.(ts|js|tsx|jsx)$" packages/ariada-vite-plugin || true
rg -n "scanDevHtml|buildDir|transformIndexHtml|devPages|index\.html|multi-page|mpa" packages/ariada-vite-plugin -SRepository: ariada-org/ariada
Length of output: 3919
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the repository contains Vite docs references or a plugin hook wrapper, inspect them.
rg -n "ctx\.filename|ctx\.path|transformIndexHtml" . -g '!**/node_modules/**' -SRepository: ariada-org/ariada
Length of output: 584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant source and tests around the hook behavior.
git ls-files packages/ariada-vite-plugin/src packages/ariada-vite-plugin | sed -n '1,200p'Repository: ariada-org/ariada
Length of output: 692
Key dev HTML by the actual HTML entry path
transformIndexHtml currently stores every HTML string under resolve(config.root, 'index.html'), so multi-page builds overwrite earlier entries and build-time HTML can be counted again when scanViteOutput merges the output. Accept the hook context and key by ctx.filename/ctx.path, or skip the capture outside the dev server if this is intended to be dev-only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ariada-vite-plugin/src/index.ts` around lines 32 - 38, The HTML
capture in VitePluginLike is keyed too broadly, causing multi-page HTML to
overwrite and build-time HTML to be re-counted. Update transformIndexHtml to
accept the hook context and store entries by the actual HTML entry path from
ctx.filename or ctx.path instead of resolve(config.root, 'index.html'), or
explicitly skip non-dev-server captures if this hook is meant to be dev-only.
Make sure the change is applied where VitePluginLike, configResolved, and
transformIndexHtml interact with scanViteOutput.
| export function findStaticHtmlIssues(filePath: string, html: string): HtmlFinding[] { | ||
| const findings: HtmlFinding[] = []; | ||
| const labelPattern = /<label\b[^>]*>(?<body>.*?)<\/label>/gis; | ||
| const labelTexts = new Set<string>(); | ||
| let labelMatch: RegExpExecArray | null; | ||
|
|
||
| while ((labelMatch = labelPattern.exec(html)) !== null) { | ||
| const body = stripTags(labelMatch.groups?.['body'] ?? '').trim().toLowerCase(); | ||
| if (body.length > 0) labelTexts.add(body); | ||
| } | ||
|
|
||
| const inputPattern = /<input\b[^>]*>/gi; | ||
| let inputMatch: RegExpExecArray | null; | ||
| let inputIndex = 0; | ||
|
|
||
| while ((inputMatch = inputPattern.exec(html)) !== null) { | ||
| inputIndex += 1; | ||
| const tag = inputMatch[0]; | ||
| const type = attr(tag, 'type')?.toLowerCase() ?? 'text'; | ||
| if (type === 'hidden' || type === 'submit' || type === 'button') continue; | ||
| if (attr(tag, 'aria-label') || attr(tag, 'aria-labelledby')) continue; | ||
| const id = attr(tag, 'id'); | ||
| if (id && new RegExp(`<label\\b[^>]*for=["']?${escapeRegExp(id)}["']?`, 'i').test(html)) continue; | ||
| if (labelTexts.size > 0) continue; | ||
|
|
||
| findings.push({ | ||
| filePath, | ||
| ruleId: 'form-field-name', | ||
| severity: 'serious', | ||
| message: 'Form fields need an accessible name.', | ||
| selector: `input:nth-of-type(${inputIndex})`, | ||
| }); | ||
| } | ||
|
|
||
| return findings; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Broken accessible-name check: any label anywhere in the document suppresses all unlabeled-input findings.
At Line 81, if (labelTexts.size > 0) continue; skips the "form-field-name" finding for the current input as long as any <label> exists anywhere in the scanned HTML — not just a label associated with that specific input. labelTexts (built at Lines 60-67) is otherwise unused; its text content never gets matched back to the input being checked.
Concretely: <label for="a">Name</label><input id="a"><input name="unrelated"> — the second, completely unlabeled input will not be flagged, because the document already contains one label. Once a page has any correctly labeled field (which is the common case), every other genuinely unlabeled input silently escapes detection, defeating the purpose of the scanner and letting real accessibility issues through the build gate undetected.
The existing tests don't catch this because each test scans an isolated HTML fragment containing at most one label/input pairing.
🐛 Proposed fix: remove the blanket suppression
export function findStaticHtmlIssues(filePath: string, html: string): HtmlFinding[] {
const findings: HtmlFinding[] = [];
- const labelPattern = /<label\b[^>]*>(?<body>.*?)<\/label>/gis;
- const labelTexts = new Set<string>();
- let labelMatch: RegExpExecArray | null;
-
- while ((labelMatch = labelPattern.exec(html)) !== null) {
- const body = stripTags(labelMatch.groups?.['body'] ?? '').trim().toLowerCase();
- if (body.length > 0) labelTexts.add(body);
- }
-
const inputPattern = /<input\b[^>]*>/gi;
let inputMatch: RegExpExecArray | null;
let inputIndex = 0;
while ((inputMatch = inputPattern.exec(html)) !== null) {
inputIndex += 1;
const tag = inputMatch[0];
const type = attr(tag, 'type')?.toLowerCase() ?? 'text';
if (type === 'hidden' || type === 'submit' || type === 'button') continue;
if (attr(tag, 'aria-label') || attr(tag, 'aria-labelledby')) continue;
const id = attr(tag, 'id');
if (id && new RegExp(`<label\\b[^>]*for=["']?${escapeRegExp(id)}["']?`, 'i').test(html)) continue;
- if (labelTexts.size > 0) continue;
findings.push({Note this also drops detection of implicit/wrapped labeling (<label>Text <input></label> with no for/id), which was presumably the intent of collecting labelTexts. If that case needs coverage, it should be implemented as a proper containment check (e.g., verify the input's match position falls within a <label>...</label> span) rather than a document-wide existence check.
📝 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.
| export function findStaticHtmlIssues(filePath: string, html: string): HtmlFinding[] { | |
| const findings: HtmlFinding[] = []; | |
| const labelPattern = /<label\b[^>]*>(?<body>.*?)<\/label>/gis; | |
| const labelTexts = new Set<string>(); | |
| let labelMatch: RegExpExecArray | null; | |
| while ((labelMatch = labelPattern.exec(html)) !== null) { | |
| const body = stripTags(labelMatch.groups?.['body'] ?? '').trim().toLowerCase(); | |
| if (body.length > 0) labelTexts.add(body); | |
| } | |
| const inputPattern = /<input\b[^>]*>/gi; | |
| let inputMatch: RegExpExecArray | null; | |
| let inputIndex = 0; | |
| while ((inputMatch = inputPattern.exec(html)) !== null) { | |
| inputIndex += 1; | |
| const tag = inputMatch[0]; | |
| const type = attr(tag, 'type')?.toLowerCase() ?? 'text'; | |
| if (type === 'hidden' || type === 'submit' || type === 'button') continue; | |
| if (attr(tag, 'aria-label') || attr(tag, 'aria-labelledby')) continue; | |
| const id = attr(tag, 'id'); | |
| if (id && new RegExp(`<label\\b[^>]*for=["']?${escapeRegExp(id)}["']?`, 'i').test(html)) continue; | |
| if (labelTexts.size > 0) continue; | |
| findings.push({ | |
| filePath, | |
| ruleId: 'form-field-name', | |
| severity: 'serious', | |
| message: 'Form fields need an accessible name.', | |
| selector: `input:nth-of-type(${inputIndex})`, | |
| }); | |
| } | |
| return findings; | |
| } | |
| export function findStaticHtmlIssues(filePath: string, html: string): HtmlFinding[] { | |
| const findings: HtmlFinding[] = []; | |
| const inputPattern = /<input\b[^>]*>/gi; | |
| let inputMatch: RegExpExecArray | null; | |
| let inputIndex = 0; | |
| while ((inputMatch = inputPattern.exec(html)) !== null) { | |
| inputIndex += 1; | |
| const tag = inputMatch[0]; | |
| const type = attr(tag, 'type')?.toLowerCase() ?? 'text'; | |
| if (type === 'hidden' || type === 'submit' || type === 'button') continue; | |
| if (attr(tag, 'aria-label') || attr(tag, 'aria-labelledby')) continue; | |
| const id = attr(tag, 'id'); | |
| if (id && new RegExp(`<label\\b[^>]*for=["']?${escapeRegExp(id)}["']?`, 'i').test(html)) continue; | |
| findings.push({ | |
| filePath, | |
| ruleId: 'form-field-name', | |
| severity: 'serious', | |
| message: 'Form fields need an accessible name.', | |
| selector: `input:nth-of-type(${inputIndex})`, | |
| }); | |
| } | |
| return findings; | |
| } |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 79-79: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(<label\\b[^>]*for=["']?${escapeRegExp(id)}["']?, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🪛 OpenGrep (1.23.0)
[ERROR] 64-64: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 73-73: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ariada-vite-plugin/src/scan.ts` around lines 58 - 93, The
form-field-name scan in findStaticHtmlIssues is incorrectly suppressing all
unlabeled input findings whenever any <label> exists in the document. Remove the
blanket labelTexts.size > 0 shortcut, and instead determine labeling per input
in the inputPattern loop by checking only the current input’s explicit or
implicit association. Use the existing helpers and symbols
(findStaticHtmlIssues, labelPattern, inputPattern, attr, escapeRegExp) to keep
the logic localized, and if wrapped labels are meant to be supported, replace
the global labelTexts check with a specific containment/span match for that
input.
Signed-off-by: Alexander Brichkin (Agonist Development AB) <git@ariada.org>
Signed-off-by: Alexander Brichkin (Agonist Development AB) <git@ariada.org>
Signed-off-by: Alexander Brichkin (Agonist Development AB) <git@ariada.org>
Signed-off-by: Alexander Brichkin (Agonist Development AB) <git@ariada.org>
Signed-off-by: Alexander Brichkin (Agonist Development AB) <git@ariada.org>
124002a to
5745eb3
Compare
…rCloud S2871) Signed-off-by: Alexander Brichkin (Agonist Development AB) <git@ariada.org>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/ariada-storybook-addon/src/panel.ts (1)
41-51: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
finding.severitystill interpolated unescaped.Every other dynamic field (
ruleId,message,selector) goes throughescapeHtml, butfinding.severityat Line 46 is inserted raw. Sincescanneris injectable, a non-conforming custom scanner could supply an arbitrary string here, producing HTML injection in the manager panel. This was already flagged in a previous review and remains unaddressed.🔒 Proposed fix
- `<li><strong>${escapeHtml(finding.ruleId)}</strong> [${finding.severity}] ${escapeHtml(finding.message)} <code>${escapeHtml(finding.selector)}</code></li>`, + `<li><strong>${escapeHtml(finding.ruleId)}</strong> [${escapeHtml(finding.severity)}] ${escapeHtml(finding.message)} <code>${escapeHtml(finding.selector)}</code></li>`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ariada-storybook-addon/src/panel.ts` around lines 41 - 51, The HTML generated by renderPanelHtml still inserts finding.severity directly into the list item, leaving an unescaped dynamic field in the panel output. Update renderPanelHtml in panel.ts so the severity value is passed through escapeHtml alongside ruleId, message, and selector, keeping the li markup safe even when StoryScanResult data comes from a custom scanner.packages/ariada-vite-plugin/src/scan.ts (1)
58-93: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winBroken accessible-name check remains unresolved: any label anywhere suppresses all unlabeled-input findings.
labelTexts.size > 0(line 81) short-circuits the entire loop as soon as the document contains any<label>, regardless of whether it's associated with the input being checked. A page with one correctly labeled field will silently miss every other genuinely unlabeled input, defeating the scanner's purpose.🐛 Proposed fix: remove the blanket suppression
export function findStaticHtmlIssues(filePath: string, html: string): HtmlFinding[] { const findings: HtmlFinding[] = []; - const labelPattern = /<label\b[^>]*>(?<body>.*?)<\/label>/gis; - const labelTexts = new Set<string>(); - let labelMatch: RegExpExecArray | null; - - while ((labelMatch = labelPattern.exec(html)) !== null) { - const body = stripTags(labelMatch.groups?.['body'] ?? '').trim().toLowerCase(); - if (body.length > 0) labelTexts.add(body); - } - const inputPattern = /<input\b[^>]*>/gi; let inputMatch: RegExpExecArray | null; let inputIndex = 0; while ((inputMatch = inputPattern.exec(html)) !== null) { inputIndex += 1; const tag = inputMatch[0]; const type = attr(tag, 'type')?.toLowerCase() ?? 'text'; if (type === 'hidden' || type === 'submit' || type === 'button') continue; if (attr(tag, 'aria-label') || attr(tag, 'aria-labelledby')) continue; const id = attr(tag, 'id'); if (id && new RegExp(`<label\\b[^>]*for=["']?${escapeRegExp(id)}["']?`, 'i').test(html)) continue; - if (labelTexts.size > 0) continue; findings.push({This is the same issue already raised in a previous review and remains unresolved in this diff.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ariada-vite-plugin/src/scan.ts` around lines 58 - 93, The accessible-name check in findStaticHtmlIssues is still suppressing all unlabeled inputs whenever any <label> exists in the document. Remove the blanket labelTexts.size > 0 shortcut and keep the per-input checks in place using the input’s aria-label, aria-labelledby, and matching <label for="..."> logic so only truly associated labels exclude a finding.packages/ariada-vite-plugin/src/index.ts (1)
32-63: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDev HTML capture keyed by a fixed path — still overwrites multi-page entries and double-counts in
closeBundle.
transformIndexHtml(html)(line 51) discards the hook's context and always stores captured HTML underresolve(config.root, 'index.html')(line 53). For multi-page builds this overwrites all but the last processed page. Additionally, sincetransformIndexHtmlalso fires during production builds,closeBundle(lines 57-63) will scan the same page twice — once viadevPagesand once via the on-diskscanViteOutput— inflatingreport.summary.total.🐛 Proposed fix: key captures by the actual entry path
- async transformIndexHtml(html) { + async transformIndexHtml(html, ctx) { if (options.scanDevHtml ?? true) { - devPages.set(resolve(config.root, 'index.html'), html); + devPages.set(resolve(config.root, ctx.path ?? ctx.filename), html); } return html; },This was already flagged in a previous review and remains unaddressed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ariada-vite-plugin/src/index.ts` around lines 32 - 63, The HTML capture in ariadaVite currently stores every transformIndexHtml result under a fixed index.html key, which overwrites multi-page entries and causes duplicate counting in closeBundle. Update transformIndexHtml to use the hook’s actual page/path context as the map key instead of resolve(config.root, 'index.html'), and keep devPages keyed per entry so each HTML file is preserved. Then adjust closeBundle to avoid re-scanning pages already captured in devPages, or otherwise dedupe before pushing into report.pages. Use ariadaVite, transformIndexHtml, devPages, and closeBundle to locate the changes.
🧹 Nitpick comments (2)
packages/vscode-extension/tools/vsce-wrapper/vsce.js (2)
38-72: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winStaged temp directory can leak if
stageExtension()throws before packaging runs.
mkdtempSync(line 42) creates the stage dir, but thetry/finallycleanup (lines 68-72) only wraps therun()call at line 69 — not thecpSync/writeFileSynccalls insidestageExtension()itself. If any staged file (e.g.icon.png,NOTICE) is missing,cpSyncthrows uncaught and the temp directory is never removed.♻️ Suggested fix: wrap staging + packaging in one try/finally
-if (command === 'package' || command === 'pack' || command === 'publish') { - const stage = stageExtension(); - const stagedArgs = [...args]; +if (command === 'package' || command === 'pack' || command === 'publish') { + let stage; + try { + stage = stageExtension(); + const stagedArgs = [...args]; ... - try { run(process.execPath, [realVsceMain(), ...stagedArgs], { cwd: stage, env: vsceEnv() }); } finally { - rmSync(stage, { recursive: true, force: true }); + if (stage) rmSync(stage, { recursive: true, force: true }); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vscode-extension/tools/vsce-wrapper/vsce.js` around lines 38 - 72, The temp staging directory can leak when `stageExtension()` fails before `run(process.execPath, [realVsceMain(), ...])` starts, because the current `try/finally` only cleans up after packaging. Move the cleanup guard so the entire staging flow is covered, including `mkdtempSync`, the `cpSync`/`symlinkSync`/`writeFileSync` work in `stageExtension()`, and the later `run()` call, ensuring `rmSync(stage, ...)` always runs even if a staged asset like `icon.png` or `NOTICE` is missing.
46-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a junction for the staged
node_moduleson Windows
symlinkSync(join(originalCwd, 'node_modules'), join(stage, 'node_modules'), process.platform === 'win32' ? 'junction' : 'dir');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vscode-extension/tools/vsce-wrapper/vsce.js` at line 46, The staged node_modules link in vsce.js uses a directory symlink unconditionally, which is not the right link type on Windows. Update the symlinkSync call that creates the staged node_modules entry so it selects 'junction' when process.platform is win32 and 'dir' otherwise, keeping the existing originalCwd, stage, and join usage intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ariada-astro/src/scan.ts`:
- Around line 20-56: The scan/report core is duplicated here instead of using a
shared implementation, so move the common severity/report model and HTML scanner
helpers into a shared internal package and have this module consume them.
Extract and reuse the logic behind Severity, severityRank, buildReport,
hasFindingAtOrAbove, and defaultHtmlScanner, and also share the recursive HTML
file walker currently duplicated in listHtmlFiles. Update this file to import
the shared symbols rather than reimplementing them so all packages stay in sync.
In `@packages/vscode-extension/tools/vsce-wrapper/vsce.js`:
- Around line 62-64: The default vsix output name in the pack/package fallback
is hardcoded to 0.1.0, so update the logic in the vsce wrapper to derive the
filename from the current extension version instead of a literal. Use the
existing stage/package flow around stageExtension() and the package metadata
sourcePackage so the --out fallback in vsce.js builds the name dynamically from
sourcePackage.version, or re-read that version at the call site if needed.
- Around line 59-67: The vsce wrapper only adds --no-dependencies for package
and pack, but publish also stages the extension and should avoid dependency
detection against the symlinked node_modules. Update the conditional in vsce.js
around stageExtension and stagedArgs so the dependency flag check and push apply
to publish as well, using hasDependencyModeFlag and the stagedArgs flow already
in place.
---
Duplicate comments:
In `@packages/ariada-storybook-addon/src/panel.ts`:
- Around line 41-51: The HTML generated by renderPanelHtml still inserts
finding.severity directly into the list item, leaving an unescaped dynamic field
in the panel output. Update renderPanelHtml in panel.ts so the severity value is
passed through escapeHtml alongside ruleId, message, and selector, keeping the
li markup safe even when StoryScanResult data comes from a custom scanner.
In `@packages/ariada-vite-plugin/src/index.ts`:
- Around line 32-63: The HTML capture in ariadaVite currently stores every
transformIndexHtml result under a fixed index.html key, which overwrites
multi-page entries and causes duplicate counting in closeBundle. Update
transformIndexHtml to use the hook’s actual page/path context as the map key
instead of resolve(config.root, 'index.html'), and keep devPages keyed per entry
so each HTML file is preserved. Then adjust closeBundle to avoid re-scanning
pages already captured in devPages, or otherwise dedupe before pushing into
report.pages. Use ariadaVite, transformIndexHtml, devPages, and closeBundle to
locate the changes.
In `@packages/ariada-vite-plugin/src/scan.ts`:
- Around line 58-93: The accessible-name check in findStaticHtmlIssues is still
suppressing all unlabeled inputs whenever any <label> exists in the document.
Remove the blanket labelTexts.size > 0 shortcut and keep the per-input checks in
place using the input’s aria-label, aria-labelledby, and matching <label
for="..."> logic so only truly associated labels exclude a finding.
---
Nitpick comments:
In `@packages/vscode-extension/tools/vsce-wrapper/vsce.js`:
- Around line 38-72: The temp staging directory can leak when `stageExtension()`
fails before `run(process.execPath, [realVsceMain(), ...])` starts, because the
current `try/finally` only cleans up after packaging. Move the cleanup guard so
the entire staging flow is covered, including `mkdtempSync`, the
`cpSync`/`symlinkSync`/`writeFileSync` work in `stageExtension()`, and the later
`run()` call, ensuring `rmSync(stage, ...)` always runs even if a staged asset
like `icon.png` or `NOTICE` is missing.
- Line 46: The staged node_modules link in vsce.js uses a directory symlink
unconditionally, which is not the right link type on Windows. Update the
symlinkSync call that creates the staged node_modules entry so it selects
'junction' when process.platform is win32 and 'dir' otherwise, keeping the
existing originalCwd, stage, and join usage intact.
🪄 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: fc41c4dd-a27e-445f-bab9-37404509e96e
⛔ Files ignored due to path filters (2)
packages/vscode-extension/icon.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (53)
packages/ariada-astro/CHANGELOG.mdpackages/ariada-astro/LICENSEpackages/ariada-astro/LICENSES/EUPL-1.2.txtpackages/ariada-astro/NOTICEpackages/ariada-astro/README.mdpackages/ariada-astro/REUSE.tomlpackages/ariada-astro/SECURITY.mdpackages/ariada-astro/package.jsonpackages/ariada-astro/src/index.tspackages/ariada-astro/src/report.tspackages/ariada-astro/src/scan.tspackages/ariada-astro/tests/integration.test.tspackages/ariada-astro/tsconfig.jsonpackages/ariada-astro/vitest.config.tspackages/ariada-storybook-addon/.gitignorepackages/ariada-storybook-addon/CHANGELOG.mdpackages/ariada-storybook-addon/LICENSEpackages/ariada-storybook-addon/LICENSES/EUPL-1.2.txtpackages/ariada-storybook-addon/NOTICEpackages/ariada-storybook-addon/README.mdpackages/ariada-storybook-addon/REUSE.tomlpackages/ariada-storybook-addon/SECURITY.mdpackages/ariada-storybook-addon/package.jsonpackages/ariada-storybook-addon/scripts/write-cjs-package.cjspackages/ariada-storybook-addon/src/constants.tspackages/ariada-storybook-addon/src/decorator.tspackages/ariada-storybook-addon/src/index.tspackages/ariada-storybook-addon/src/manager.tspackages/ariada-storybook-addon/src/panel.tspackages/ariada-storybook-addon/src/preview.tspackages/ariada-storybook-addon/src/scan.tspackages/ariada-storybook-addon/tests/addon.test.tspackages/ariada-storybook-addon/tsconfig.cjs.jsonpackages/ariada-storybook-addon/tsconfig.jsonpackages/ariada-storybook-addon/vitest.config.tspackages/ariada-vite-plugin/CHANGELOG.mdpackages/ariada-vite-plugin/LICENSEpackages/ariada-vite-plugin/LICENSES/EUPL-1.2.txtpackages/ariada-vite-plugin/NOTICEpackages/ariada-vite-plugin/README.mdpackages/ariada-vite-plugin/REUSE.tomlpackages/ariada-vite-plugin/SECURITY.mdpackages/ariada-vite-plugin/package.jsonpackages/ariada-vite-plugin/src/index.tspackages/ariada-vite-plugin/src/scan.tspackages/ariada-vite-plugin/tests/plugin.test.tspackages/ariada-vite-plugin/tsconfig.jsonpackages/ariada-vite-plugin/vitest.config.tspackages/vscode-extension/.vscodeignorepackages/vscode-extension/README.mdpackages/vscode-extension/package.jsonpackages/vscode-extension/tools/vsce-wrapper/package.jsonpackages/vscode-extension/tools/vsce-wrapper/vsce.js
✅ Files skipped from review due to trivial changes (19)
- packages/ariada-storybook-addon/NOTICE
- packages/ariada-vite-plugin/NOTICE
- packages/ariada-storybook-addon/REUSE.toml
- packages/ariada-astro/CHANGELOG.md
- packages/ariada-astro/NOTICE
- packages/ariada-storybook-addon/CHANGELOG.md
- packages/ariada-astro/REUSE.toml
- packages/ariada-storybook-addon/.gitignore
- packages/ariada-storybook-addon/README.md
- packages/ariada-vite-plugin/CHANGELOG.md
- packages/ariada-astro/README.md
- packages/ariada-storybook-addon/src/index.ts
- packages/ariada-vite-plugin/REUSE.toml
- packages/ariada-vite-plugin/README.md
- packages/ariada-astro/tsconfig.json
- packages/ariada-storybook-addon/LICENSE
- packages/ariada-vite-plugin/tsconfig.json
- packages/ariada-astro/LICENSE
- packages/vscode-extension/.vscodeignore
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/ariada-vite-plugin/vitest.config.ts
- packages/ariada-storybook-addon/tsconfig.json
- packages/ariada-astro/src/report.ts
- packages/ariada-storybook-addon/vitest.config.ts
- packages/ariada-astro/vitest.config.ts
- packages/ariada-storybook-addon/tsconfig.cjs.json
- packages/ariada-storybook-addon/src/manager.ts
- packages/ariada-storybook-addon/src/preview.ts
- packages/vscode-extension/tools/vsce-wrapper/package.json
- packages/ariada-astro/package.json
- packages/vscode-extension/README.md
- packages/ariada-vite-plugin/LICENSE
- packages/ariada-storybook-addon/src/constants.ts
- packages/ariada-vite-plugin/tests/plugin.test.ts
- packages/ariada-storybook-addon/scripts/write-cjs-package.cjs
- packages/vscode-extension/package.json
- packages/ariada-storybook-addon/package.json
- packages/ariada-vite-plugin/package.json
- packages/ariada-storybook-addon/tests/addon.test.ts
- packages/ariada-astro/tests/integration.test.ts
| export interface BuildScanReport { | ||
| generatedAt: string; | ||
| pages: PageScan[]; | ||
| summary: Record<Severity, number> & { total: number }; | ||
| } | ||
|
|
||
| export type HtmlScanner = (input: { filePath: string; html: string }) => Promise<PageScan> | PageScan; | ||
|
|
||
| export const severityRank: Record<Severity, number> = { | ||
| minor: 1, | ||
| moderate: 2, | ||
| serious: 3, | ||
| critical: 4, | ||
| }; | ||
|
|
||
| export const defaultHtmlScanner: HtmlScanner = ({ filePath, html }) => ({ | ||
| filePath, | ||
| findings: findStaticHtmlIssues(filePath, html), | ||
| }); | ||
|
|
||
| export function buildReport(pages: PageScan[], generatedAt = new Date().toISOString()): BuildScanReport { | ||
| const summary = { total: 0, critical: 0, serious: 0, moderate: 0, minor: 0 }; | ||
| for (const page of pages) { | ||
| for (const finding of page.findings) { | ||
| summary.total += 1; | ||
| summary[finding.severity] += 1; | ||
| } | ||
| } | ||
| return { generatedAt, pages, summary }; | ||
| } | ||
|
|
||
| export function hasFindingAtOrAbove(report: BuildScanReport, threshold: Severity): boolean { | ||
| const minimum = severityRank[threshold]; | ||
| return report.pages.some((page) => | ||
| page.findings.some((finding) => severityRank[finding.severity] >= minimum), | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Duplicated scan/report core across packages instead of a shared scanner.
Severity, severityRank, buildReport, hasFindingAtOrAbove, and defaultHtmlScanner here are nearly identical to the same symbols in packages/ariada-vite-plugin/src/scan.ts (and listHtmlFiles in index.ts is copy-pasted too). The PR description frames these packages as "thin adapters over the shared @ariada-org scanner," but each package independently reimplements this core model. This has already caused visible drift: the vite-plugin's findStaticHtmlIssues has an unresolved critical suppression bug (see companion comment) that this astro version doesn't share, and any future fix must now be applied N times.
Consider extracting the common report/severity model (and the recursive HTML file walker) into a shared internal package that all four packages depend on.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ariada-astro/src/scan.ts` around lines 20 - 56, The scan/report core
is duplicated here instead of using a shared implementation, so move the common
severity/report model and HTML scanner helpers into a shared internal package
and have this module consume them. Extract and reuse the logic behind Severity,
severityRank, buildReport, hasFindingAtOrAbove, and defaultHtmlScanner, and also
share the recursive HTML file walker currently duplicated in listHtmlFiles.
Update this file to import the shared symbols rather than reimplementing them so
all packages stay in sync.
| if (command === 'package' || command === 'pack' || command === 'publish') { | ||
| const stage = stageExtension(); | ||
| const stagedArgs = [...args]; | ||
| if ((command === 'package' || command === 'pack') && !hasOutFlag(stagedArgs)) { | ||
| stagedArgs.push('--out', join(originalCwd, 'ariada-accessibility-0.1.0.vsix')); | ||
| } | ||
| if ((command === 'package' || command === 'pack') && !hasDependencyModeFlag(stagedArgs)) { | ||
| stagedArgs.push('--no-dependencies'); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'packages/vscode-extension/tools/vsce-wrapper/*' 'packages/vscode-extension/**' | sed -n '1,120p'
echo '--- vsce.js excerpt ---'
nl -ba packages/vscode-extension/tools/vsce-wrapper/vsce.js | sed -n '1,180p'
echo '--- search helpers/usages ---'
rg -n "hasOutFlag|hasDependencyModeFlag|vsce-wrapper|--no-dependencies|--out|publish" packages/vscode-extension -SRepository: ariada-org/ariada
Length of output: 1348
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- vsce.js ---'
sed -n '1,220p' packages/vscode-extension/tools/vsce-wrapper/vsce.js | cat -n
echo '--- vsce wrapper package.json ---'
sed -n '1,220p' packages/vscode-extension/tools/vsce-wrapper/package.json | cat -n
echo '--- extension README/package references ---'
rg -n "vsce|publish|package|no-dependencies|out" packages/vscode-extension/README.md packages/vscode-extension/package.json packages/vscode-extension/tools/vsce-wrapper -SRepository: ariada-org/ariada
Length of output: 8633
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- README publish/package section ---'
sed -n '1,120p' packages/vscode-extension/README.md | cat -n
echo '--- package.json scripts section ---'
python3 - <<'PY'
import json
from pathlib import Path
p = Path('packages/vscode-extension/package.json')
data = json.loads(p.read_text())
for k in ['scripts','bin','files','main','activationEvents']:
if k in data:
print(f'[{k}]')
if isinstance(data[k], dict):
for kk,v in data[k].items():
print(f'{kk}: {v}')
else:
print(data[k])
PY
echo '--- vsce docs in lockfile? ---'
rg -n "\"`@vscode/vsce`\"|\"vsce\"" packages/vscode-extension/pnpm-lock.yaml packages/vscode-extension/package.json -SRepository: ariada-org/ariada
Length of output: 5274
🌐 Web query:
@vscode/vsce publish --no-dependencies default dependency detection npm list symlinked node_modules package.json devDependencies
💡 Result:
The --no-dependencies flag for @vscode/vsce disables the tool's automatic dependency detection and packaging logic [1][2]. When this flag is used, vsce skips the process of analyzing your node_modules directory—which normally involves running commands like npm list or yarn list to determine which dependencies to include in your VSIX package—and instead includes only the files present in your project directory (subject to your.vscodeignore rules) [1][2][3]. This flag is primarily used in workflows where the extension is already bundled (e.g., using tools like esbuild, webpack, or tsup) [1][2][4]. Because bundling consolidates all necessary production dependencies into a single output file (e.g., out/extension.js), there is no need to package the node_modules folder itself [1][2]. Key implications of using --no-dependencies include: 1. Handling Symlinks: By bypassing dependency resolution, vsce avoids issues with symlinked node_modules (often seen with pnpm or monorepo setups), which can otherwise cause vsce to fail when it attempts to traverse the directory structure [2][4]. 2. DevDependencies: While vsce normally attempts to ignore devDependencies automatically, it can sometimes include their sub-dependencies if the dependency tree is complex or incorrectly parsed [5][6]. Using --no-dependencies eliminates this risk entirely, as it stops vsce from inspecting your dependencies altogether [2]. 3. Configuration: You can provide this flag via the CLI or by configuring it in your package.json [7][8]. While older versions had issues with configuration persistence, modern versions support defining this in a vsce block within your package.json, though CLI flags remain the most common practice for build scripts [7][8]. As of recent updates (circa 2026), vsce has improved support for various package managers (such as pnpm, bun, and modern yarn), and in some scenarios, it may automatically detect that a project is bundled and enable similar "no-dependency" logic, but explicitly providing the flag remains the standard solution for ensuring consistent packaging behavior in custom build pipelines [9].
Citations:
- 1: Support pnpm microsoft/vscode-vsce#421
- 2: Compatibility issue with PNPM package manager in 2.10.0 microsoft/vscode-vsce#758
- 3: https://github.com/microsoft/vscode-vsce/blob/master/src/npm.ts
- 4: https://daydreamer-riri.me/posts/compatibility-issues-between-vsce-and-pnpm/
- 5: .vscodeignore and devDependencies microsoft/vscode-vsce#13
- 6: Ignore dependencies of devDependencies microsoft/vscode-vsce#52
- 7: https://github.com/microsoft/vscode-vsce/blob/main/README.md
- 8: Dependencies cannot be configured inside package.json microsoft/vscode-vsce#742
- 9: feat/fix: auto-detect dependencies for foreign managers, make
vsceself-contained, and add--no-prepublishmicrosoft/vscode-vsce#1257
🌐 Web query:
site:github.com microsoft/vsce publish no-dependencies package publish command dependencies detection npm list
💡 Result:
The --no-dependencies flag in vsce (Visual Studio Code Extension Manager) is designed to skip the automatic dependency analysis and validation process that vsce normally performs during the packaging and publishing of an extension [1][2]. When you run vsce package or vsce publish without this flag, the tool attempts to detect your project's dependencies—typically by executing npm list or similar commands—to ensure all required packages are present and to include them in the resulting VSIX file [3][4]. Key points regarding this functionality: 1. Use Case: The --no-dependencies flag is intended for scenarios where you have already bundled your extension and its dependencies into a single output directory using tools like webpack, esbuild, or rollup [5][2]. By using this flag, you prevent vsce from attempting its own dependency resolution, which can often fail in monorepo structures, environments using alternative package managers (like pnpm or bun), or when dependencies are already handled by a build process [5][2]. 2. Effect: Enabling this flag prevents vsce from running npm list, yarn list, or equivalent commands, thereby avoiding common errors related to missing dependencies or "security scan" issues where the tool might incorrectly scan parent directories for files [1][3][2]. 3. How to use it: You can add the flag directly to your CLI commands: vsce package --no-dependencies vsce publish --no-dependencies 4. Configuration: While earlier versions had issues with configuration, recent versions allow you to manage some vsce settings within the package.json file [6]. However, it is generally recommended and standard practice to pass --no-dependencies as a CLI argument during your CI/CD or build process to ensure explicit control over the packaging behavior [1][7]. 5. Automatic Detection: Recent updates to vsce have improved its ability to handle different package managers, and in some cases, the tool may automatically optimize the dependency analysis process based on the detected environment [5]. Nevertheless, if you are explicitly bundling your extension, --no-dependencies remains the recommended way to ensure vsce does not interfere with your pre-packaged dependencies [2].
Citations:
- 1: a5c-ai/babysitter@8f74fd1
- 2: Compatibility issue with PNPM package manager in 2.10.0 microsoft/vscode-vsce#758
- 3: https://github.com/microsoft/vscode-vsce/blob/master/src/npm.ts
- 4: https://github.com/microsoft/vscode-vsce/blob/47330ab9/src/npm.ts
- 5: feat/fix: auto-detect dependencies for foreign managers, make
vsceself-contained, and add--no-prepublishmicrosoft/vscode-vsce#1257 - 6: Dependencies cannot be configured inside package.json microsoft/vscode-vsce#742
- 7: https://github.com/stephanzlatarev/vscode-starcraft/blob/6b7796847bdc2f80bff8bc078f824b95a77e7977/.github/workflows/publish.yaml
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
from pathlib import Path
p = Path('packages/vscode-extension/package.json')
data = json.loads(p.read_text())
for key in ['dependencies','devDependencies','peerDependencies','optionalDependencies']:
print(f'[{key}]')
deps = data.get(key, {})
for name in sorted(deps):
print(f'{name}: {deps[name]}')
PYRepository: ariada-org/ariada
Length of output: 540
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
from pathlib import Path
p = Path('packages/vscode-extension/package.json')
data = json.loads(p.read_text())
for key in ['dependencies','devDependencies','peerDependencies','optionalDependencies']:
print(f'[{key}]')
deps = data.get(key, {})
if not deps:
print('(empty)')
else:
for name in sorted(deps):
print(f'{name}: {deps[name]}')
PYRepository: ariada-org/ariada
Length of output: 564
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l packages/vscode-extension/package.json
sed -n '1,240p' packages/vscode-extension/package.json | cat -nRepository: ariada-org/ariada
Length of output: 6960
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' packages/vscode-extension/package.json | cat -nRepository: ariada-org/ariada
Length of output: 6917
Apply --no-dependencies to publish as well
publish stages the extension, but it still lets vsce run dependency detection against the symlinked node_modules. This repo’s extension has no runtime dependencies and the staged manifest strips devDependencies, so vsce publish can mis-detect or fail unless --no-dependencies is added here too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/vscode-extension/tools/vsce-wrapper/vsce.js` around lines 59 - 67,
The vsce wrapper only adds --no-dependencies for package and pack, but publish
also stages the extension and should avoid dependency detection against the
symlinked node_modules. Update the conditional in vsce.js around stageExtension
and stagedArgs so the dependency flag check and push apply to publish as well,
using hasDependencyModeFlag and the stagedArgs flow already in place.
| if ((command === 'package' || command === 'pack') && !hasOutFlag(stagedArgs)) { | ||
| stagedArgs.push('--out', join(originalCwd, 'ariada-accessibility-0.1.0.vsix')); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Hardcoded version in default output filename.
ariada-accessibility-0.1.0.vsix is a fixed literal, not derived from sourcePackage.version. Every release after 0.1.0 will silently produce a vsix named for the wrong version when no --out is passed.
🐛 Proposed fix
if ((command === 'package' || command === 'pack') && !hasOutFlag(stagedArgs)) {
- stagedArgs.push('--out', join(originalCwd, 'ariada-accessibility-0.1.0.vsix'));
+ stagedArgs.push('--out', join(originalCwd, `ariada-accessibility-${sourcePackage.version}.vsix`));
}Note: sourcePackage would need to be returned from stageExtension() or re-read at this call site.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/vscode-extension/tools/vsce-wrapper/vsce.js` around lines 62 - 64,
The default vsix output name in the pack/package fallback is hardcoded to 0.1.0,
so update the logic in the vsce wrapper to derive the filename from the current
extension version instead of a literal. Use the existing stage/package flow
around stageExtension() and the package metadata sourcePackage so the --out
fallback in vsce.js builds the name dynamically from sourcePackage.version, or
re-read that version at the call site if needed.
|
predopta
left a comment
predopta
left a comment
There was a problem hiding this comment.
Re-reviewed after the lockfile + reliability-sort fixes; the four packages build, test, and pass the quality gate. Approving.
3 participants
Promotes four web-tooling accessibility-scan channel packages to public OSS:
@ariada-org/astro— Astro build integration (scans built HTML inastro:build:done)@ariada-org/vite-plugin— Vite build plugin (scans production HTML output)@ariada-org/storybook-addon— Storybook addon (scans the rendered canvas, panel UI)@ariada-org/vscode-extension— marketplace packaging metadata + icon for the existing extensionEach package ships source, colocated tests (astro 2, vite 3, storybook 3, vscode 52 — all green), and a full documentation set (README, LICENSE EUPL-1.2, LICENSES, NOTICE, SECURITY, CHANGELOG, REUSE.toml). Thin adapters over the shared @ariada-org scanner; no generated artifacts committed.
Summary by CodeRabbit