fix: replace readFileSync with async readFile in library tool handler#212
fix: replace readFileSync with async readFile in library tool handler#212
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace webp-only social card with PNG exported from Playwright. helixir purple (#8b5cf6) branded at 1200x630. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ontextprotocolsdk fix: align @modelcontextprotocol/sdk versions across monorepo packages
…dcomponent-and fix: wire scaffold_component and extend_component into MCP server
Adds the helixir-vscode VS Code extension package at packages/vscode/. The extension registers helixir as an MCP server definition provider (vscode.lm, VS Code ≥ 1.99.0) so AI assistants receive full component library awareness automatically when a workspace is opened. Files created: - packages/vscode/package.json — extension manifest with publisher, mcpServerDefinitionProviders contribution, Run Health Check command, helixir.configPath setting, and vsce/ovsx scripts - packages/vscode/tsconfig.json — extends root tsconfig, noEmit for type-check-only (esbuild handles transpilation) - packages/vscode/esbuild.config.mjs — dual-bundle config: extension.js (CJS, vscode externalized) + mcp-server.js (ESM, bundles helixir) - packages/vscode/src/extension.ts — activate/deactivate exports, wires MCP provider and Run Health Check command - packages/vscode/src/mcpProvider.ts — registerMcpServerDefinitionProvider call; spawns bundled mcp-server.js via stdio with MCP_WC_PROJECT_ROOT set to the current workspace folder; degrades gracefully on older VS Code - packages/vscode/src/mcp-server-entry.ts — imports helixir/mcp and calls main(); bundled into the self-contained dist/mcp-server.js - packages/vscode/.vscodeignore — excludes src, tsconfig, node_modules, and build artefacts from the .vsix package - packages/vscode/README.md — marketplace listing with setup, config reference, commands table, and troubleshooting guide Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ckagesvscode-extension feat: scaffold packages/vscode VS Code extension MVP
…fault case
Replaces the '/* TODO: set value */' literal in the default branch of
lightPlaceholder() with var(${tokenName}), which produces valid CSS that
degrades gracefully when a token category is unknown.
Adds a test case that exercises the default code path by creating a CEM
with a token name that does not match any known CATEGORY_PATTERNS entry,
causing it to land in the 'other' bucket and hit the default switch case.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add comprehensive test coverage for packages/core/src/tools/styling.ts — previously the single largest untested file in the codebase (1089 lines, zero coverage). - Tests all 29 MCP tool handlers via mocked dependencies - Verifies happy paths, missing-arg validation errors, and error propagation - Covers isStylingTool guard and the handleStylingCall dispatcher - Designed to achieve 80%+ line coverage per vitest thresholds Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r-returns-todo fix: replace TODO placeholder with var() fallback in theme handler
…ite-for-styling-tools test: add test suite for styling tools (29 tools, zero coverage)
Replaces the blocking readFileSync() call at packages/core/src/tools/library.ts:127 with the async readFile() equivalent from fs/promises. This was blocking the Node.js event loop during MCP request processing whenever a local CEM file was loaded via the load_library tool. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughThis pull request adds a comprehensive VS Code extension ( Changes
Sequence Diagram(s)sequenceDiagram
participant VSCode as VS Code
participant Ext as Extension
participant Provider as MCP Provider
participant Context as Context/Config
participant Server as MCP Server<br/>(dist/mcp-server.js)
participant WS as Workspace
VSCode->>Ext: onStartupFinished
activate Ext
Ext->>Provider: registerMcpProvider(context)
activate Provider
Provider->>WS: Get workspace folder
Provider->>Context: Get helixir.configPath setting
Provider->>Provider: Build env mapping<br/>(MCP_WC_PROJECT_ROOT, MCP_WC_CONFIG_PATH)
Provider->>VSCode: vscode.lm.registerMcpServerDefinitionProvider<br/>('helixir', provider)
VSCode->>Provider: Returns disposable
Provider->>Context: context.subscriptions.push(disposable)
deactivate Provider
Ext->>Ext: Register runHealthCheck command
deactivate Ext
VSCode->>Ext: User invokes command or AI requests tool
activate Ext
Ext->>Server: Spawn stdio child process<br/>with env vars
activate Server
Server->>Server: Initialize Helixir MCP tools
Server->>VSCode: MCP protocol messages
deactivate Server
deactivate Ext
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build`:
- Line 1: The repository contains an accidental tracked artifact named "build"
that is an absolute-path file/symlink; remove it from git history by deleting
the tracked "build" entry (git rm --cached or git rm as appropriate) and commit
that deletion, ensure you do not remove any intended generated build/ directory
locally, then add "build/" to .gitignore so generated build outputs are not
tracked going forward; verify the working tree and CI by committing the
.gitignore change and pushing the removal commit.
In `@node_modules`:
- Line 1: Remove the accidental absolute-path artifact by untracking the file
entry that contains "/Volumes/Development/booked/helixir/node_modules" and
ensuring the repository only tracks the project-relative "node_modules/"
convention; unstage/remove that tracked path from git, add "node_modules/" to
.gitignore (or ensure it is present), and commit the removal so the absolute
path no longer appears in history or future commits. Target the repository entry
named exactly "/Volumes/Development/booked/helixir/node_modules" and the
repository-level .gitignore to apply the fix.
In `@packages/core/package.json`:
- Line 19: The pnpm lockfile is out of sync with the bumped dependency
"@modelcontextprotocol/sdk" in package manifests; update the lockfile by running
a lockfile regeneration (e.g., pnpm install or pnpm i) at the repo root so
pnpm-lock.yaml reflects the new version ^1.27.1 used in packages/core and
packages/mcp, then commit the updated pnpm-lock.yaml so CI's frozen-lockfile
installs succeed.
In `@packages/core/src/handlers/theme.ts`:
- Around line 147-149: The default branch in the theme handler currently returns
a self-referential CSS variable (`return var(${tokenName})`) which produces
cycles like `--x: var(--x);`; change the default to return a non-cyclic
fallback, e.g. `var(${tokenName}, unset)` (or another concrete non-custom
fallback value you prefer) inside the function that returns token CSS values
(the code path using tokenName in packages/core/src/handlers/theme.ts), and
update the test in tests/handlers/theme.test.ts (the "produces a var() fallback
for tokens with unknown categories" assertion) to expect the new non-cyclic
fallback (`unset` or your chosen fallback) instead of `var(--my-custom-widget)`.
In `@packages/vscode/src/mcpProvider.ts`:
- Around line 25-26: The current fallback in mcpProvider.ts uses process.cwd()
for workspaceFolder (const workspaceFolder =
vscode.workspace.workspaceFolders?.[0]?.uri.fsPath ?? process.cwd()), which can
be misleading in VS Code; update the provider (the function or class that
computes workspaceFolder in mcpProvider.ts) to avoid using process.cwd() as a
silent fallback—instead detect when vscode.workspace.workspaceFolders is empty
and either return an empty array from the provider or log a clear warning via
the extension logger (include context like "no workspace folder open in VS
Code") and then return an empty/non-operational result; keep the reference to
workspaceFolder and ensure downstream code handles the empty result safely.
- Around line 36-41: The extension sets MCP_WC_CONFIG_PATH (from
helixir.configPath) but the core's loadConfig() never reads it, making the
setting a no-op; update loadConfig() to check process.env.MCP_WC_CONFIG_PATH
and, if present, use that path (resolve relative paths the same way other env
vars like MCP_WC_PROJECT_ROOT are handled) as the config file location before
falling back to the current lookup logic, so helixir.configPath functions as
intended (alternatively, if you prefer not to extend core, remove the
MCP_WC_CONFIG_PATH write in the mcpProvider code and the corresponding user
setting to avoid dead code).
In `@README.md`:
- Line 20: The README has inconsistent tool counts: the badge markdown
[] shows "87+" while
the body text still reads "30+ MCP tools"; update the README so both references
match (e.g., change the body copy "30+ MCP tools" to "87+ MCP tools" or vice
versa) to keep the messaging consistent; locate the badge markup and the phrase
"30+ MCP tools" and make them identical.
In `@tests/tools/styling.test.ts`:
- Around line 427-434: The test in tests/tools/styling.test.ts assumes 'svelte'
is invalid when calling handleStylingCall('check_event_usage', {..., framework:
'svelte'}, FAKE_CEM), which will break if Svelte is later added to the schema;
add a brief comment immediately above this test (or next to the framework value)
listing the currently accepted framework enum values (e.g., "valid frameworks:
react, vue, angular") and note that the test should be updated if the schema
adds support for Svelte so future readers know why 'svelte' is expected to be
invalid; reference the test invoking handleStylingCall and the framework field
in the test data so maintainers can find and update it easily.
- Around line 194-197: The afterEach hook currently calls both
vi.restoreAllMocks() and vi.clearAllMocks(), which is redundant; remove the
vi.clearAllMocks() call and keep only vi.restoreAllMocks() inside the afterEach
to both reset mock state and restore original implementations (locate the
afterEach block in tests/tools/styling.test.ts and delete the vi.clearAllMocks()
line).
- Around line 862-866: The test passes a redundant cem property inside the args
object to handleStylingCall; remove the cem: FAKE_CEM entry from the args object
so that handleStylingCall('check_css_scope', { cssText: ':root {
--my-button-color: red; }', tagName: 'my-button' }, FAKE_CEM) relies on the
third parameter for the CEM, keeping the call signature consistent with the
function's parameters and avoiding misleading extras in the args object.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d1aa082-546e-401b-8b8b-26def4e649fe
⛔ Files ignored due to path filters (1)
assets/social-card.pngis excluded by!**/*.png
📒 Files selected for processing (19)
README.mdassets/social-card.webpbuildnode_modulespackages/core/package.jsonpackages/core/src/handlers/theme.tspackages/core/src/tools/library.tspackages/mcp/package.jsonpackages/vscode/.vscodeignorepackages/vscode/README.mdpackages/vscode/esbuild.config.mjspackages/vscode/package.jsonpackages/vscode/src/extension.tspackages/vscode/src/mcp-server-entry.tspackages/vscode/src/mcpProvider.tspackages/vscode/tsconfig.jsonsrc/mcp/index.tstests/handlers/theme.test.tstests/tools/styling.test.ts
| @@ -0,0 +1 @@ | |||
| /Volumes/Development/booked/helixir/build No newline at end of file | |||
There was a problem hiding this comment.
Remove the accidental absolute-path build artifact.
Line 1 introduces a local absolute path as a tracked file/symlink. This is non-portable and may break build pipelines that expect build/ to be a generated directory.
Proposed fix
-/Volumes/Development/booked/helixir/build📝 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.
| /Volumes/Development/booked/helixir/build |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build` at line 1, The repository contains an accidental tracked artifact
named "build" that is an absolute-path file/symlink; remove it from git history
by deleting the tracked "build" entry (git rm --cached or git rm as appropriate)
and commit that deletion, ensure you do not remove any intended generated build/
directory locally, then add "build/" to .gitignore so generated build outputs
are not tracked going forward; verify the working tree and CI by committing the
.gitignore change and pushing the removal commit.
| @@ -0,0 +1 @@ | |||
| /Volumes/Development/booked/helixir/node_modules No newline at end of file | |||
There was a problem hiding this comment.
Remove the accidental absolute-path node_modules artifact.
Line 1 tracks a machine-specific absolute path. This is non-portable, leaks local path info, and can break installs by conflicting with the expected node_modules/ directory.
Proposed fix
-/Volumes/Development/booked/helixir/node_modules📝 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.
| /Volumes/Development/booked/helixir/node_modules |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@node_modules` at line 1, Remove the accidental absolute-path artifact by
untracking the file entry that contains
"/Volumes/Development/booked/helixir/node_modules" and ensuring the repository
only tracks the project-relative "node_modules/" convention; unstage/remove that
tracked path from git, add "node_modules/" to .gitignore (or ensure it is
present), and commit the removal so the absolute path no longer appears in
history or future commits. Target the repository entry named exactly
"/Volumes/Development/booked/helixir/node_modules" and the repository-level
.gitignore to apply the fix.
| "license": "MIT", | ||
| "dependencies": { | ||
| "@modelcontextprotocol/sdk": "^1.26.0", | ||
| "@modelcontextprotocol/sdk": "^1.27.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify manifest vs lockfile specifiers for `@modelcontextprotocol/sdk` without modifying repo.
set -euo pipefail
echo "== package.json entries =="
rg -n '"@modelcontextprotocol/sdk"\s*:' packages/core/package.json packages/mcp/package.json
echo
echo "== lockfile entries (importers + packages) =="
rg -n '@modelcontextprotocol/sdk|packages/core|packages/mcp' pnpm-lock.yaml -C2
echo
echo "Expected: lockfile importer specifiers match '^1.27.1' where corresponding manifests were updated."Repository: bookedsolidtech/helixir
Length of output: 1454
Update pnpm-lock.yaml to match the dependency bump in both packages (CI blocker).
The manifests for both packages/core (line 19) and packages/mcp (line 49) were updated to ^1.27.1, but the lockfile still specifies ^1.26.0 for both importers. Regenerate and commit pnpm-lock.yaml so frozen-lockfile installs work correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/package.json` at line 19, The pnpm lockfile is out of sync with
the bumped dependency "@modelcontextprotocol/sdk" in package manifests; update
the lockfile by running a lockfile regeneration (e.g., pnpm install or pnpm i)
at the repo root so pnpm-lock.yaml reflects the new version ^1.27.1 used in
packages/core and packages/mcp, then commit the updated pnpm-lock.yaml so CI's
frozen-lockfile installs succeed.
| default: | ||
| return '/* TODO: set value */'; | ||
| return `var(${tokenName})`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In CSS Custom Properties, what is the computed behavior of a self-referential declaration like --x: var(--x);? Is it treated as invalid due to a cycle?
💡 Result:
In CSS Custom Properties, a self-referential declaration like --x: var(--x); creates a cyclic dependency. The browser constructs a dependency graph for custom properties on each element. If a custom property's value contains a var reference to itself (directly or indirectly through other properties), it forms a cycle. All custom properties in the cycle are invalid at computed-value time, resulting in the "guaranteed-invalid value" (initial value for custom properties). When var(--x) is used elsewhere, it substitutes nothing (empty token stream), and fallbacks (if provided) are used. This behavior is consistent across browsers and prevents infinite recursion during resolution.
Citations:
- 1: https://stackoverflow.com/questions/51660196/unable-to-overwrite-css-variable-with-its-own-value
- 2: https://kizu.dev/cyclic-toggles
- 3: https://kizu.dev/indirect-cyclic-conditions
- 4: https://drafts.csswg.org/css-variables/
- 5: https://drafts.csswg.org/css-conditional-values/
- 6: https://stackoverflow.com/questions/55717060/css-custom-properties-dependencies-not-resolving-why-also-did-i-discover-undo
🏁 Script executed:
cat -n packages/core/src/handlers/theme.ts | sed -n '140,160p'Repository: bookedsolidtech/helixir
Length of output: 870
🏁 Script executed:
# Find the test file mentioned in the scratchpad
fd "theme.test" -t fRepository: bookedsolidtech/helixir
Length of output: 95
🏁 Script executed:
# Check the full context of the lightPlaceholder function
rg -A 10 -B 10 "lightPlaceholder" packages/core/src/handlers/theme.tsRepository: bookedsolidtech/helixir
Length of output: 1596
🏁 Script executed:
# Find and examine the test file
fd "theme.test.ts" -t f -x wc -l {}Repository: bookedsolidtech/helixir
Length of output: 101
🏁 Script executed:
# Look at tests related to default/unknown categories
rg -A 20 "unknown|default" tests/handlers/theme.test.tsRepository: bookedsolidtech/helixir
Length of output: 1914
🏁 Script executed:
# Show how lightValues are used in the createTheme function
rg -A 10 "lightValues" packages/core/src/handlers/theme.tsRepository: bookedsolidtech/helixir
Length of output: 1494
Self-referential fallback creates invalid custom-property values
Line 148 generates --x: var(--x); for unknown categories, which creates a CSS custom-property cycle. Per the W3C CSS Variables specification, cyclic custom properties are treated as invalid at computed-value time, making unknown tokens effectively unusable in generated CSS.
Proposed fix
default:
- return `var(${tokenName})`;
+ return 'initial';Please also adjust the test in tests/handlers/theme.test.ts (line ~105: produces a var() fallback for tokens with unknown categories) to assert a non-cyclic fallback value instead of var(--my-custom-widget).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/handlers/theme.ts` around lines 147 - 149, The default
branch in the theme handler currently returns a self-referential CSS variable
(`return var(${tokenName})`) which produces cycles like `--x: var(--x);`; change
the default to return a non-cyclic fallback, e.g. `var(${tokenName}, unset)` (or
another concrete non-custom fallback value you prefer) inside the function that
returns token CSS values (the code path using tokenName in
packages/core/src/handlers/theme.ts), and update the test in
tests/handlers/theme.test.ts (the "produces a var() fallback for tokens with
unknown categories" assertion) to expect the new non-cyclic fallback (`unset` or
your chosen fallback) instead of `var(--my-custom-widget)`.
| const workspaceFolder = | ||
| vscode.workspace.workspaceFolders?.[0]?.uri.fsPath ?? process.cwd(); |
There was a problem hiding this comment.
process.cwd() fallback may be unpredictable in VS Code context.
When no workspace folder is open, process.cwd() returns the VS Code installation directory or an arbitrary working directory—not a meaningful project root. Consider logging a warning or returning an empty array from the provider when no workspace is available, rather than silently using a potentially incorrect path.
💡 Suggested approach
const workspaceFolder =
- vscode.workspace.workspaceFolders?.[0]?.uri.fsPath ?? process.cwd();
+ vscode.workspace.workspaceFolders?.[0]?.uri.fsPath;
+
+ if (!workspaceFolder) {
+ console.warn('[helixir-vscode] No workspace folder open; MCP server not registered.');
+ return [];
+ }📝 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.
| const workspaceFolder = | |
| vscode.workspace.workspaceFolders?.[0]?.uri.fsPath ?? process.cwd(); | |
| const workspaceFolder = | |
| vscode.workspace.workspaceFolders?.[0]?.uri.fsPath; | |
| if (!workspaceFolder) { | |
| console.warn('[helixir-vscode] No workspace folder open; MCP server not registered.'); | |
| return []; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vscode/src/mcpProvider.ts` around lines 25 - 26, The current
fallback in mcpProvider.ts uses process.cwd() for workspaceFolder (const
workspaceFolder = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath ??
process.cwd()), which can be misleading in VS Code; update the provider (the
function or class that computes workspaceFolder in mcpProvider.ts) to avoid
using process.cwd() as a silent fallback—instead detect when
vscode.workspace.workspaceFolders is empty and either return an empty array from
the provider or log a clear warning via the extension logger (include context
like "no workspace folder open in VS Code") and then return an
empty/non-operational result; keep the reference to workspaceFolder and ensure
downstream code handles the empty result safely.
| // If the user specified a custom config path, resolve it and pass it on. | ||
| if (configPath && configPath.trim() !== '') { | ||
| env['MCP_WC_CONFIG_PATH'] = path.isAbsolute(configPath) | ||
| ? configPath | ||
| : path.join(workspaceFolder, configPath); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for MCP_WC_CONFIG_PATH usage in the codebase
rg -n "MCP_WC_CONFIG_PATH" --type ts --type jsRepository: bookedsolidtech/helixir
Length of output: 168
🏁 Script executed:
cat -n packages/core/src/config.ts | head -100Repository: bookedsolidtech/helixir
Length of output: 4385
🏁 Script executed:
cat -n packages/core/src/config.ts | tail -30Repository: bookedsolidtech/helixir
Length of output: 1353
🏁 Script executed:
# Double-check if MCP_WC_CONFIG_PATH is referenced anywhere else in the vscode package
rg -n "CONFIG_PATH" packages/vscode/ --type ts --type jsRepository: bookedsolidtech/helixir
Length of output: 168
MCP_WC_CONFIG_PATH is set but never consumed by the core package.
According to packages/core/src/config.ts, the loadConfig() function reads MCP_WC_PROJECT_ROOT, MCP_WC_CEM_PATH, and other specific environment variables, but it does not read MCP_WC_CONFIG_PATH. This makes the helixir.configPath setting non-functional—users will configure it expecting it to work, but the server ignores it entirely.
Either:
- Update
packages/core/src/config.tsto consumeMCP_WC_CONFIG_PATHwhen locating the config file, or - Remove this dead code and the corresponding setting until the core package supports it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vscode/src/mcpProvider.ts` around lines 36 - 41, The extension sets
MCP_WC_CONFIG_PATH (from helixir.configPath) but the core's loadConfig() never
reads it, making the setting a no-op; update loadConfig() to check
process.env.MCP_WC_CONFIG_PATH and, if present, use that path (resolve relative
paths the same way other env vars like MCP_WC_PROJECT_ROOT are handled) as the
config file location before falling back to the current lookup logic, so
helixir.configPath functions as intended (alternatively, if you prefer not to
extend core, remove the MCP_WC_CONFIG_PATH write in the mcpProvider code and the
corresponding user setting to avoid dead code).
| [](https://github.com/bookedsolidtech/helixir/actions/workflows/test.yml) | ||
| [](https://modelcontextprotocol.io) | ||
| [](https://www.typescriptlang.org) | ||
| [](https://www.npmjs.com/package/helixir) |
There was a problem hiding this comment.
Unify the tool-count messaging.
Line 20 shows tools-87+, but the README copy still says 30+ MCP tools (Line 31). Please make these consistent to avoid confusing users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 20, The README has inconsistent tool counts: the badge
markdown [] shows "87+"
while the body text still reads "30+ MCP tools"; update the README so both
references match (e.g., change the body copy "30+ MCP tools" to "87+ MCP tools"
or vice versa) to keep the messaging consistent; locate the badge markup and the
phrase "30+ MCP tools" and make them identical.
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| vi.clearAllMocks(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant mock cleanup calls.
vi.restoreAllMocks() already clears mock state and restores original implementations. The subsequent clearAllMocks() is unnecessary.
🔧 Suggested simplification
afterEach(() => {
vi.restoreAllMocks();
- vi.clearAllMocks();
});📝 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.
| afterEach(() => { | |
| vi.restoreAllMocks(); | |
| vi.clearAllMocks(); | |
| }); | |
| afterEach(() => { | |
| vi.restoreAllMocks(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/tools/styling.test.ts` around lines 194 - 197, The afterEach hook
currently calls both vi.restoreAllMocks() and vi.clearAllMocks(), which is
redundant; remove the vi.clearAllMocks() call and keep only vi.restoreAllMocks()
inside the afterEach to both reset mock state and restore original
implementations (locate the afterEach block in tests/tools/styling.test.ts and
delete the vi.clearAllMocks() line).
| it('returns error for invalid framework enum value', () => { | ||
| const result = handleStylingCall( | ||
| 'check_event_usage', | ||
| { codeText: 'code', tagName: 'my-button', framework: 'svelte' }, | ||
| FAKE_CEM, | ||
| ); | ||
| expect(result.isError).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test assumes 'svelte' is invalid for framework enum.
This test verifies that framework: 'svelte' returns an error. If Svelte support is added to the schema in the future, this test will need updating. Consider adding a comment documenting the expected valid frameworks for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/tools/styling.test.ts` around lines 427 - 434, The test in
tests/tools/styling.test.ts assumes 'svelte' is invalid when calling
handleStylingCall('check_event_usage', {..., framework: 'svelte'}, FAKE_CEM),
which will break if Svelte is later added to the schema; add a brief comment
immediately above this test (or next to the framework value) listing the
currently accepted framework enum values (e.g., "valid frameworks: react, vue,
angular") and note that the test should be updated if the schema adds support
for Svelte so future readers know why 'svelte' is expected to be invalid;
reference the test invoking handleStylingCall and the framework field in the
test data so maintainers can find and update it easily.
| const result = handleStylingCall( | ||
| 'check_css_scope', | ||
| { cssText: ':root { --my-button-color: red; }', tagName: 'my-button', cem: FAKE_CEM }, | ||
| FAKE_CEM, | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unnecessary cem property in args object.
The cem: FAKE_CEM property in the args is redundant—handleStylingCall receives the CEM as its third parameter, not via the args object. Zod will strip unknown properties, so this won't cause failures, but it's misleading.
🔧 Suggested cleanup
const result = handleStylingCall(
'check_css_scope',
- { cssText: ':root { --my-button-color: red; }', tagName: 'my-button', cem: FAKE_CEM },
+ { cssText: ':root { --my-button-color: red; }', tagName: 'my-button' },
FAKE_CEM,
);📝 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.
| const result = handleStylingCall( | |
| 'check_css_scope', | |
| { cssText: ':root { --my-button-color: red; }', tagName: 'my-button', cem: FAKE_CEM }, | |
| FAKE_CEM, | |
| ); | |
| const result = handleStylingCall( | |
| 'check_css_scope', | |
| { cssText: ':root { --my-button-color: red; }', tagName: 'my-button' }, | |
| FAKE_CEM, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/tools/styling.test.ts` around lines 862 - 866, The test passes a
redundant cem property inside the args object to handleStylingCall; remove the
cem: FAKE_CEM entry from the args object so that
handleStylingCall('check_css_scope', { cssText: ':root { --my-button-color: red;
}', tagName: 'my-button' }, FAKE_CEM) relies on the third parameter for the CEM,
keeping the call signature consistent with the function's parameters and
avoiding misleading extras in the args object.
1 participant
Summary
readFileSync()with asyncawait readFile()fromfs/promisesinpackages/core/src/tools/library.tsline 127handleLibraryCallfunction is alreadyasync— callers alreadyawaitit — so no call-site changes are neededTest plan
packages/core/src/tools/library.tsis modified🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation