feat(cli): support mcp apps and improved error handling#85
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds MCP app HTML generation and an App client; implements three MCP app UIs (icons, tokens, examples) and server registration; refactors tool infrastructure to support app metadata and structured ToolError results; introduces strict/non-strict linting and updates services, tests, CLI error handling, types, and build inputs. ChangesTool Type System and Error Handling
Tool Service Updates
Linting System Strict/Non-Strict Modes
MCP Client-Side App Infrastructure
MCP Custom Element Modules
MCP Server Integration
Supporting Changes
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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: 12
🤖 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 `@projects/cli/package.json`:
- Line 37: The dev script uses npx which breaks pnpm workspace resolution;
update the "dev" script (the package.json "dev" entry) to use pnpm dlx instead
of npx and pass the inspected package/version before the script args (e.g.
invoke pnpm dlx `@modelcontextprotocol/inspector`@0.21.2 with a "--" separating
package invocation from "node ./dist/index.js mcp") so the workspace uses pnpm's
installer and resolution.
In `@projects/cli/src/index.test.ts`:
- Around line 145-151: The test is fragile because it JSON-parses stdout+stderr
as one blob and assumes the target is the first item; instead make parsing
stream-safe and order-independent by splitting the combined output into
newline-delimited JSON entries (or parsing each non-empty line separately) into
an array (replace the current use of combined and JSON.parse) and then assert
using Array.prototype.some on the resulting lintMessages array to confirm at
least one message.message contains 'Unexpected use of restricted attribute
"nve-layout"'; update references to combined, lintMessages and result in the
test to implement this safer parsing and use .some rather than indexing [0].
- Around line 121-132: Update the test "should exit with error when exact api
lookup has no matches" to use a guaranteed-impossible lookup key instead of
"nve-badges": change the argument passed to spawnSync (the third element in
['dist/index.js', 'api.get', 'nve-badges']) to a deliberately impossible
identifier (e.g. a UUID-like or clearly namespaced token such as
"no-such-component-00000000-0000-0000-0000-000000000000") so the test cannot be
broken by future real components; keep the rest of the assertions (result.status
and combined contains checks) and the use of spawnSync/result variables intact.
In `@projects/cli/src/mcp/apps/api-tokens-list.js`:
- Around line 250-290: The classifyToken method is long and hard to maintain;
refactor it to use a data-driven lookup table instead of nested conditionals:
create a configuration object/array mapping category names (e.g., 'shadow',
'radius', 'typography', 'spacing', 'color') to arrays of name-keywords and
separate value-check predicates (for color checks: startsWith
'#','rgb','hsl','oklch' and a set of exact value names like
'canvas','buttontext', etc.), then iterate the config in the original priority
order inside classifyToken to test token.name.toLowerCase() and
token.value.toLowerCase() against each category's keyword list and predicates
and return the category on first match; ensure the original special-case checks
(value startsWith and exact matches) are preserved and that the final fallback
returns 'other'.
In `@projects/cli/src/mcp/apps/examples-render.js`:
- Around line 43-46: The code uses createContextualFragment(this.state.template)
and this.append(fragment) which injects raw HTML; add a client-side sanitization
step before creating the fragment: validate that this.state.template is a
string, then sanitize it (e.g., call a sanitizer like DOMPurify.sanitize or
escape HTML when templates must be treated as text) and only pass the sanitized
string into createContextualFragment; as a fallback, render via textContent or a
safe template renderer if validation fails to avoid XSS.
In `@projects/cli/src/mcp/apps/utils.ts`:
- Around line 110-117: The catch on customElements.whenDefined in
awaitElementUpgrades is silently swallowing failures; update the map to catch
and log the error along with the tag so failures are visible for debugging (e.g.
use console.warn or a provided logger inside awaitElementUpgrades when
customElements.whenDefined(tag) rejects), keeping the promise resolution
behavior but emitting a clear message that includes the tag and error from the
rejected promise.
- Around line 67-68: The private `#send` method uses window.parent.postMessage
with a wildcard origin which can leak data; change it to use a specific allowed
origin (or window.parent.origin) and validate that origin before posting (e.g.,
determine expectedOrigin or use a whitelist and only call
window.parent.postMessage(msg, expectedOrigin) from `#send`), so replace the '*'
argument and add origin validation logic in the `#send` implementation.
In `@projects/internals/tools/src/api/service.test.ts`:
- Around line 275-285: The test hard-codes the query 'shadow' which makes it
flaky against metadata churn; update the test that calls ApiService.tokensList
to derive a deterministic query from the returned data (or use mocked token
data) instead of a fixed string: call ApiService.tokensList without a query or
with a broad query, inspect the first token's name/value/description, pick a
substring or token attribute (e.g., first token.name.split(/\s+/)[0]) and re-run
tokensList with that derived query, then assert results are non-empty and every
returned item contains that derived substring; reference ApiService.tokensList
and the existing test block to locate and change the assertions.
In `@projects/internals/tools/src/distill/examples.ts`:
- Around line 31-37: The default-example detection treats missing IDs ('') as
default because isDefault = !id.includes('-'); change the guard so an empty id
is not considered default: update the isDefault calculation (used where you
compute const id = example.id ?? '' and const isDefault = ...) to require a
non-empty id, e.g. set isDefault to id !== '' && !id.includes('-') or
Boolean(id) && !id.includes('-'); keep the existing return condition (if
(composition || isDefault) return true;) unchanged.
In `@projects/internals/tools/src/internal/tools.ts`:
- Around line 72-76: Replace the loose ToolOutput interface with a discriminated
union so callers can rely on result when status === 'complete': change
ToolOutput<T> into two variants — one with status: 'complete' that requires
result: T (message can remain optional) and one with status: 'error' that
requires a message (and should not have result or can set result?: never) —
update the ToolOutput type (symbol: ToolOutput) accordingly so type-narrowing on
status guarantees presence/absence of result for downstream checks and tests.
In `@projects/internals/tools/src/playground/service.ts`:
- Around line 129-130: The thrown ToolError currently uses a generic message
"Template validation failed" and omits lint details; update the throw in the
validation branch to include the lint diagnostics in the error message (e.g.,
interpolate or stringify lintResult) so consumers see rule violations in
ToolError.message while still passing lintResult as ToolError.result; locate the
throw that references ToolError and lintResult in the validation path of the
Examples/Playground render flow and modify the message to incorporate the
diagnostics text.
In `@projects/internals/tools/src/playground/utils.ts`:
- Around line 149-152: The current createURL builds the query string via
interpolation and then calls encodeURI, which allows special characters in
individual values (e.g., defaultOptions.referer) to break parsing; change it to
encode each query parameter value with encodeURIComponent (for
defaultOptions.name, defaultOptions.theme, defaultOptions.openFile,
defaultOptions.referer, and files) and build the query string using those
encoded values (or a helper that maps key/value pairs to key=encodedValue joined
by &), then prepend ELEMENTS_PLAYGROUND_BASE_URL and the encoded query string
without wrapping the whole URL in encodeURI.
🪄 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: Enterprise
Run ID: 0e093bea-9631-4b6d-b945-d02a5a5989ad
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
projects/cli/package.jsonprojects/cli/src/index.test.tsprojects/cli/src/index.tsprojects/cli/src/mcp/apps/api-icons-list.app.tsprojects/cli/src/mcp/apps/api-icons-list.jsprojects/cli/src/mcp/apps/api-tokens-list.app.tsprojects/cli/src/mcp/apps/api-tokens-list.jsprojects/cli/src/mcp/apps/examples-render.app.tsprojects/cli/src/mcp/apps/examples-render.jsprojects/cli/src/mcp/apps/utils.tsprojects/cli/src/mcp/mcp.test.tsprojects/cli/src/mcp/mcp.tsprojects/cli/src/types.d.tsprojects/cli/src/utils.test.tsprojects/cli/vitest.config.tsprojects/core/src/button/button.cssprojects/core/src/internal/services/global.utils.tsprojects/core/src/internal/utils/audit-logs.tsprojects/internals/tools/src/api/service.test.tsprojects/internals/tools/src/api/service.tsprojects/internals/tools/src/api/utils.test.tsprojects/internals/tools/src/api/utils.tsprojects/internals/tools/src/distill/examples.tsprojects/internals/tools/src/examples/service.test.tsprojects/internals/tools/src/examples/service.tsprojects/internals/tools/src/index.test.tsprojects/internals/tools/src/index.tsprojects/internals/tools/src/internal/tools.test.tsprojects/internals/tools/src/internal/tools.tsprojects/internals/tools/src/playground/service.test.tsprojects/internals/tools/src/playground/service.tsprojects/internals/tools/src/playground/utils.tsrelease.config.js
💤 Files with no reviewable changes (2)
- projects/core/src/internal/utils/audit-logs.ts
- projects/core/src/internal/services/global.utils.ts
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
projects/internals/tools/src/distill/examples.ts (1)
33-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard default detection against empty IDs.
id === ''is currently treated as default, so malformed entries can pass inclusion on Line 35.Suggested fix
- const isDefault = !id.includes('-'); // default is only the element/api name, no suffix + const isDefault = id.length > 0 && !id.includes('-'); // default is only the element/api name, no suffix🤖 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 `@projects/internals/tools/src/distill/examples.ts` around lines 33 - 36, The current default detection sets isDefault = !id.includes('-') which treats an empty string id as default; update the logic in the function where isDefault is computed (the variable `isDefault` used in the `if (composition || isDefault)` check) to first ensure id is non-empty (e.g., use `id && !id.includes('-')` or `id.length > 0 && !id.includes('-')`) so that malformed empty IDs do not pass the default check.projects/cli/src/index.test.ts (2)
122-132:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse an impossible lookup token in the no-match test.
The identifier on Line 122 can become valid later and make this test nondeterministic.
Suggested fix
- const result = spawnSync('node', ['dist/index.js', 'api.get', 'nve-badges'], { + const result = spawnSync('node', ['dist/index.js', 'api.get', '__nve_nonexistent_component__'], { @@ - expect(combined).toContain('nve-badges'); + expect(combined).toContain('__nve_nonexistent_component__');🤖 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 `@projects/cli/src/index.test.ts` around lines 122 - 132, The test in index.test.ts uses a potentially valid identifier 'nve-badges' in the spawnSync call which can become valid later and break determinism; update the argument passed to node (currently 'nve-badges' in the spawnSync invocation that produces result and combined) to an impossible lookup token (e.g., a GUID-like or clearly invalid string such as '__NO_SUCH_COMPONENT__' or 'no-such-component-00000000') so the no-match branch stays deterministic, leaving the rest of the assertions (expect(result.status).toBe(1); expect(combined).toContain('No components or APIs found matching'); expect(combined).toContain(...)) unchanged.
145-150:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake JSON error assertions stream-safe and order-independent.
Parsing
stdout + stderras one JSON payload and asserting only index0is brittle.Suggested fix
- const combined = `${result.stdout}${result.stderr}`; - const lintMessages = JSON.parse(combined) as { message: string }[]; + const lintMessages = result.stdout + .split('\n') + .map(line => line.trim()) + .filter(Boolean) + .map(line => JSON.parse(line) as { message: string }) + .flat(); @@ - expect(lintMessages[0]?.message).toContain('Unexpected use of restricted attribute "nve-layout"'); + expect( + lintMessages.some(m => m.message.includes('Unexpected use of restricted attribute "nve-layout"')) + ).toBe(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 `@projects/cli/src/index.test.ts` around lines 145 - 150, The test is brittle because it blindly JSON.parses stdout+stderr and asserts lintMessages[0], which breaks when output is split or order changes; instead parse JSON from stdout and stderr separately (or split combined into JSON-per-line), merge the resulting arrays into a single lintMessages array, then assert result.status === 1 and that some entry in the merged lintMessages has a message containing 'Unexpected use of restricted attribute "nve-layout"'; update references in this test to use the existing variables result, combined (or replace combined with separate parsing of result.stdout/result.stderr) and replace the index-based assertion on lintMessages[0]?.message with an order-independent contains check over the merged messages.
🤖 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 `@projects/cli/src/mcp/apps/utils.ts`:
- Around line 16-21: The McpAppHtmlOptions.interface declares a style field
that's not used by buildMcpAppHtml; update buildMcpAppHtml to honor
options.style by injecting it into the generated HTML head (e.g., if
options.style is a CSS string wrap in a <style> tag, or if it's a URL treat as a
<link rel="stylesheet">) and ensure the function uses McpAppHtmlOptions.style
when composing the head, or alternatively remove the style field from
McpAppHtmlOptions and any references; modify the function named buildMcpAppHtml
and the McpAppHtmlOptions interface accordingly so the option is either
implemented or removed consistently.
In `@projects/cli/src/mcp/mcp.test.ts`:
- Around line 272-277: The test currently slices HTML using marker strings
('class ElementsExamplePreview',
"customElements.define('nve-mcp-examples-render'", 'const app = new App')
without checking indexOf results, so if a marker is missing the slice will use
-1 and produce misleading outcomes; update the test to guard each index lookup
by storing the indexes (e.g., elementsStart = html.indexOf('class
ElementsExamplePreview'), elementsEnd =
html.indexOf("customElements.define('nve-mcp-examples-render'") and asserting
they are >= 0 (or throwing/using expect(elementsStart).toBeGreaterThan(-1))
before calling slice for elementScript and similarly check the index for 'const
app = new App' used for appScript; apply the same pattern for the other slices
referenced around lines 296-301 and 318-323 so all marker-based slices validate
marker existence first.
In `@projects/core/src/button/button.css`:
- Line 147: The hover selector currently excludes [pressed] but not the
pseudo-state :state(pressed), so update the selector
:host(:hover:not([interaction], [disabled], [readonly], [pressed])) to also
exclude :state(pressed) (e.g., add :state(pressed) inside the :not()) so
elements in the pressed pseudo-state don't receive hover styling; target the
selector in the CSS where :host(:hover:...) is defined.
In `@projects/internals/tools/src/playground/service.test.ts`:
- Around line 121-127: The test currently calls createTool (found via
loadTools(PlaygroundService) and tools.find(...)) without asserting it exists;
add an explicit assertion or guard before invoking createTool to fail fast with
a clear message if the tool isn't registered (e.g., assert/createTool !==
undefined or throw with "create tool not found"), then proceed to call
createTool and cast to ToolOutput<{ message: string }[]>; apply the same
existence check for the other occurrence around lines 129-134 so both test
invocations fail clearly when the tool lookup fails.
---
Duplicate comments:
In `@projects/cli/src/index.test.ts`:
- Around line 122-132: The test in index.test.ts uses a potentially valid
identifier 'nve-badges' in the spawnSync call which can become valid later and
break determinism; update the argument passed to node (currently 'nve-badges' in
the spawnSync invocation that produces result and combined) to an impossible
lookup token (e.g., a GUID-like or clearly invalid string such as
'__NO_SUCH_COMPONENT__' or 'no-such-component-00000000') so the no-match branch
stays deterministic, leaving the rest of the assertions
(expect(result.status).toBe(1); expect(combined).toContain('No components or
APIs found matching'); expect(combined).toContain(...)) unchanged.
- Around line 145-150: The test is brittle because it blindly JSON.parses
stdout+stderr and asserts lintMessages[0], which breaks when output is split or
order changes; instead parse JSON from stdout and stderr separately (or split
combined into JSON-per-line), merge the resulting arrays into a single
lintMessages array, then assert result.status === 1 and that some entry in the
merged lintMessages has a message containing 'Unexpected use of restricted
attribute "nve-layout"'; update references in this test to use the existing
variables result, combined (or replace combined with separate parsing of
result.stdout/result.stderr) and replace the index-based assertion on
lintMessages[0]?.message with an order-independent contains check over the
merged messages.
In `@projects/internals/tools/src/distill/examples.ts`:
- Around line 33-36: The current default detection sets isDefault =
!id.includes('-') which treats an empty string id as default; update the logic
in the function where isDefault is computed (the variable `isDefault` used in
the `if (composition || isDefault)` check) to first ensure id is non-empty
(e.g., use `id && !id.includes('-')` or `id.length > 0 && !id.includes('-')`) so
that malformed empty IDs do not pass the default check.
🪄 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: Enterprise
Run ID: 77a74132-1969-43f8-8823-742b64189c79
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
projects/cli/package.jsonprojects/cli/src/index.test.tsprojects/cli/src/index.tsprojects/cli/src/mcp/apps/api-icons-list.app.tsprojects/cli/src/mcp/apps/api-icons-list.jsprojects/cli/src/mcp/apps/api-tokens-list.app.tsprojects/cli/src/mcp/apps/api-tokens-list.jsprojects/cli/src/mcp/apps/examples-render.app.tsprojects/cli/src/mcp/apps/examples-render.jsprojects/cli/src/mcp/apps/utils.tsprojects/cli/src/mcp/mcp.test.tsprojects/cli/src/mcp/mcp.tsprojects/cli/src/types.d.tsprojects/cli/src/utils.test.tsprojects/cli/vitest.config.tsprojects/code/src/codeblock/codeblock.test.lighthouse.tsprojects/core/src/button/button.cssprojects/core/src/internal/services/global.utils.tsprojects/core/src/internal/utils/audit-logs.tsprojects/internals/tools/src/api/service.test.tsprojects/internals/tools/src/api/service.tsprojects/internals/tools/src/api/utils.test.tsprojects/internals/tools/src/api/utils.tsprojects/internals/tools/src/distill/examples.tsprojects/internals/tools/src/examples/service.test.tsprojects/internals/tools/src/examples/service.tsprojects/internals/tools/src/index.test.tsprojects/internals/tools/src/index.tsprojects/internals/tools/src/internal/tools.test.tsprojects/internals/tools/src/internal/tools.tsprojects/internals/tools/src/playground/service.test.tsprojects/internals/tools/src/playground/service.tsprojects/internals/tools/src/playground/utils.tsrelease.config.js
💤 Files with no reviewable changes (2)
- projects/core/src/internal/utils/audit-logs.ts
- projects/core/src/internal/services/global.utils.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
projects/cli/src/mcp/mcp.test.ts (1)
287-292:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard marker lookups before slicing HTML sections.
These slices depend on
indexOf(...)markers but don’t assert the markers exist first. If a marker changes,-1offsets can make these assertions misleading instead of clearly failing at the root cause.Suggested fix pattern
-const elementScript = html.slice( - html.indexOf('class ElementsExamplePreview'), - html.indexOf("customElements.define('nve-mcp-examples-render'") -); -const appScript = html.slice(html.indexOf('const app = new App')); +const elementStart = html.indexOf('class ElementsExamplePreview'); +const elementEnd = html.indexOf("customElements.define('nve-mcp-examples-render'"); +const appStart = html.indexOf('const app = new App'); +expect(elementStart).toBeGreaterThanOrEqual(0); +expect(elementEnd).toBeGreaterThan(elementStart); +expect(appStart).toBeGreaterThanOrEqual(0); + +const elementScript = html.slice(elementStart, elementEnd); +const appScript = html.slice(appStart);Apply the same guard pattern to the icons and tokens slices.
Also applies to: 311-316, 333-338
🤖 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 `@projects/cli/src/mcp/mcp.test.ts` around lines 287 - 292, The test currently slices HTML using markers like 'class ElementsExamplePreview', "customElements.define('nve-mcp-examples-render'", and 'const app = new App' without checking that indexOf(...) returned a valid index; add guards that assert or throw if any marker index === -1 before performing the slice (e.g., compute const start = html.indexOf(...); if (start === -1) throw new Error('missing marker: class ElementsExamplePreview');), and apply the same guard pattern to the icons and tokens slices so tests fail with a clear message instead of producing misleading slices.projects/cli/src/mcp/apps/utils.ts (1)
16-21: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winHonor
styleor remove it from the options contract.
McpAppHtmlOptions.styleis still ignored bybuildMcpAppHtml, so callers can pass a no-op option and assume their CSS was applied.Also applies to: 139-150
🤖 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 `@projects/cli/src/mcp/apps/utils.ts` around lines 16 - 21, The McpAppHtmlOptions.type includes a style property that is never used by buildMcpAppHtml, so either wire it up or remove it: update the buildMcpAppHtml function to accept the style string from McpAppHtmlOptions and inject it into the generated HTML (for example by adding a <style>...</style> block or a <link> tag placeholder) so callers’ CSS is applied, and ensure any server-side rendering path (same function) also includes it; alternatively, remove style from the McpAppHtmlOptions interface and update all callers to stop passing style (and remove any related tests). Reference symbols: McpAppHtmlOptions and buildMcpAppHtml.
🤖 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 `@projects/cli/src/mcp/apps/utils.ts`:
- Around line 36-49: The message listener in connect() must validate the inbound
message source and origin before processing: call the existing
`#getExpectedParentOrigin`() to obtain the trusted origin and ignore the event if
e.origin !== expectedOrigin or if e.source !== window.parent (or the stored
parent window reference you use for RPC); only after passing these checks should
you parse e.data as JSON-RPC and dispatch to the handler map (`#handlers`) or the
ontoolinput/ontoolresult callbacks. Ensure you perform these checks at the top
of the window.addEventListener('message', ...) handler so spoofed frames cannot
hijack pending requests or inject notifications.
- Around line 134-156: The template allows HTML/script injection because title,
bundle strings and script are interpolated raw; add small escaping helpers
(e.g., escapeHtml for text nodes and escapeScript for script content that
replaces occurrences of "</script>" and other risky sequences) and use them:
call escapeHtml(title) in buildMcpAppHtml, use escapeScript when embedding
appClientScript and the incoming script, and update getBundles to escape
coreBundle/codeBundle output before injecting into the `<script type="module">`
tags (references: getBundles, buildMcpAppHtml, McpAppBundles, McpAppHtmlOptions,
appClientScript, coreBundle, codeBundle). Ensure the helpers are used
consistently so no unescaped user-controlled string is written into HTML or
script tags.
---
Duplicate comments:
In `@projects/cli/src/mcp/apps/utils.ts`:
- Around line 16-21: The McpAppHtmlOptions.type includes a style property that
is never used by buildMcpAppHtml, so either wire it up or remove it: update the
buildMcpAppHtml function to accept the style string from McpAppHtmlOptions and
inject it into the generated HTML (for example by adding a <style>...</style>
block or a <link> tag placeholder) so callers’ CSS is applied, and ensure any
server-side rendering path (same function) also includes it; alternatively,
remove style from the McpAppHtmlOptions interface and update all callers to stop
passing style (and remove any related tests). Reference symbols:
McpAppHtmlOptions and buildMcpAppHtml.
In `@projects/cli/src/mcp/mcp.test.ts`:
- Around line 287-292: The test currently slices HTML using markers like 'class
ElementsExamplePreview', "customElements.define('nve-mcp-examples-render'", and
'const app = new App' without checking that indexOf(...) returned a valid index;
add guards that assert or throw if any marker index === -1 before performing the
slice (e.g., compute const start = html.indexOf(...); if (start === -1) throw
new Error('missing marker: class ElementsExamplePreview');), and apply the same
guard pattern to the icons and tokens slices so tests fail with a clear message
instead of producing misleading slices.
🪄 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: Enterprise
Run ID: fddeb2be-e5c8-46c5-ac42-8a0afd3c69d6
📒 Files selected for processing (2)
projects/cli/src/mcp/apps/utils.tsprojects/cli/src/mcp/mcp.test.ts
Comment on lines
+134
to
+156
| const getBundles = ({ code = false }: McpAppBundles = {}): string => { | ||
| const scripts = code ? [coreBundle, codeBundle] : [coreBundle]; | ||
| return scripts.map(script => /* html */ `<script type="module">${script}</script>`).join('\n'); | ||
| }; | ||
|
|
||
| export function buildMcpAppHtml({ title, bundles, script }: McpAppHtmlOptions): string { | ||
| return /* html */ `<!doctype html> | ||
| <html lang="en" nve-theme=""> | ||
| <head> | ||
| <meta charset="utf-8" /> | ||
| <title>${title}</title> | ||
| <style> | ||
| ${[themesIndexCss, themesDarkCss, themesHighContrastCss, stylesTypographyCss, stylesLayoutCss].join('\n')} | ||
| :root[nve-theme=""] { color-scheme: light dark; } | ||
| nve-page { height: fit-content;} | ||
| body:not(:has(nve-page)) { padding: var(--nve-ref-space-sm); } | ||
| </style> | ||
| </head> | ||
| <body> | ||
| ${getBundles(bundles)} | ||
| <script type="module"> | ||
| ${appClientScript} | ||
| ${script} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
HTML parsing rules for inline script tags: does a literal </script> inside JavaScript source terminate the tag unless escaped, and should dynamic text inserted into a <title> element be HTML-escaped?
💡 Result:
- Does a literal </script> inside JavaScript source terminate the tag unless escaped? Yes. In HTML parsing, a <script> element’s contents are not tokenized as JavaScript by the HTML parser; they follow the special “script element” content restrictions. For HTML (text/html), the <script> element ends when the parser finds the script end tag delimiter “</script>”. The HTML Standard explicitly notes that you must avoid (escape) an ASCII case-insensitive match for “</script>” inside the script contents, because otherwise the parser will treat it as the end tag rather than part of the JavaScript string/comment/etc. [1][2] The usual safe pattern is to break the sequence or escape it at the HTML level (e.g., in JS string literals use the JavaScript escape for “<” so you don’t emit a literal “</script>” substring to the HTML parser). The Standard’s example shows writing “\x3C” (escape sequence for “<”) so the HTML parser never sees “</script>” as a literal substring inside the script block. [1] So: a literal “</script>” in your JavaScript source (not just in a string) can terminate the <script> element; it is not “protected” by being inside JavaScript quotes or comments as far as the outer HTML tokenizer is concerned. [1][3] 2) Should dynamic text inserted into a <title> element be HTML-escaped? For the <title> element itself: treat it as text content. MDN states that the <title> element “only contains text; HTML tags within the element, if any, are also treated as plain text.” [4] Therefore, if you set the title dynamically via DOM APIs such as document.title or titleElement.textContent/text, the platform will treat the value as text, not HTML, and you should not HTML-escape it (escaping can lead to entities like & being displayed literally instead of being interpreted as characters). [5][6] Example rule of thumb: - If you use a DOM/text API (e.g., document.title = userText, or titleEl.textContent = userText), do not HTML-escape; provide the raw string. - If you instead construct markup strings (e.g., via innerHTML or string concatenation that you then insert as HTML source), then you must escape special HTML characters in that string to prevent markup injection. The “ampersand” behavior people observe is consistent with this: setting in HTML source requires & for an ampersand, while setting via DOM APIs uses the literal “&” string (the API treats it as text, not HTML source). [6] If you want a direct answer: - HTML parsing / inline <script>: avoid emitting the literal case-insensitive substring “</script>” inside the script contents; escape or break it so the HTML parser cannot see it. [1][2] - Dynamic <title>: if inserted as text via DOM APIs, do not HTML-escape; if inserted as HTML source, then HTML-escape required characters. [4][6]
Citations:
- 1: https://html.spec.whatwg.org/dev/scripting.html
- 2: http://w3.org/TR/html5/scripting.html
- 3: https://dbushell.com/2024/10/01/html-parser-conundrum/
- 4: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/title
- 5: https://developer.mozilla.org/docs/Web/API/HTMLTitleElement
- 6: https://stackoverflow.com/questions/35002700/setting-html-title-attribute-with-js-doesnt-render-amp
🏁 Script executed:
# Read the full file to see context and value sources
cat -n projects/cli/src/mcp/apps/utils.ts | head -200Repository: NVIDIA/elements
Length of output: 6828
🏁 Script executed:
# Check type definitions for McpAppHtmlOptions to understand data sources
rg -A 5 "McpAppHtmlOptions" projects/cli/src/mcp/apps/Repository: NVIDIA/elements
Length of output: 816
🏁 Script executed:
# Find where buildMcpAppHtml is called to understand data flow
rg -B 3 "buildMcpAppHtml" projects/cli/src/mcp/apps/ | head -100Repository: NVIDIA/elements
Length of output: 2553
🏁 Script executed:
# Check the call sites to see what title and script values are passed
cat -n projects/cli/src/mcp/apps/examples-render.app.ts | grep -A 10 "buildMcpAppHtml"Repository: NVIDIA/elements
Length of output: 508
🏁 Script executed:
cat -n projects/cli/src/mcp/apps/api-tokens-list.app.ts | grep -A 10 "buildMcpAppHtml"Repository: NVIDIA/elements
Length of output: 430
🏁 Script executed:
cat -n projects/cli/src/mcp/apps/api-icons-list.app.ts | grep -A 10 "buildMcpAppHtml"Repository: NVIDIA/elements
Length of output: 422
🏁 Script executed:
# Also check if there are any other files that might use this function
rg "buildMcpAppHtml" projects/cli/src/ -B 2 -A 8Repository: NVIDIA/elements
Length of output: 3868
Add HTML and script escaping helpers to prevent injection when parameters are user-controlled.
title, bundle strings, and script are directly interpolated into HTML template markup. A literal </script> in any script payload terminates the tag early (per HTML spec), and unescaped title text breaks markup. While current usage passes only hard-coded strings, the function accepts string parameters that could become externally sourced, making this unsafe if values ever stop being fully hard-coded.
Suggested fix
+const escapeHtml = (value: string): string =>
+ value.replaceAll('&', '&').replaceAll('<', '<').replaceAll('>', '>');
+
+const escapeInlineScript = (value: string): string =>
+ value.replace(/<\/script/gi, '<\\/script');
+
const getBundles = ({ code = false }: McpAppBundles = {}): string => {
const scripts = code ? [coreBundle, codeBundle] : [coreBundle];
- return scripts.map(script => /* html */ `<script type="module">${script}</script>`).join('\n');
+ return scripts
+ .map(script => /* html */ `<script type="module">${escapeInlineScript(script)}</script>`)
+ .join('\n');
};
export function buildMcpAppHtml({ title, bundles, script }: McpAppHtmlOptions): string {
return /* html */ `<!doctype html>
<html lang="en" nve-theme="">
<head>
<meta charset="utf-8" />
- <title>${title}</title>
+ <title>${escapeHtml(title)}</title>
<style>
${[themesIndexCss, themesDarkCss, themesHighContrastCss, stylesTypographyCss, stylesLayoutCss].join('\n')}
:root[nve-theme=""] { color-scheme: light dark; }
nve-page { height: fit-content;}
body:not(:has(nve-page)) { padding: var(--nve-ref-space-sm); }
</style>
</head>
<body>
${getBundles(bundles)}
<script type="module">
- ${appClientScript}
- ${script}
+ ${escapeInlineScript(appClientScript)}
+ ${escapeInlineScript(script)}
</script>
</body>
</html>`;
}🤖 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 `@projects/cli/src/mcp/apps/utils.ts` around lines 134 - 156, The template
allows HTML/script injection because title, bundle strings and script are
interpolated raw; add small escaping helpers (e.g., escapeHtml for text nodes
and escapeScript for script content that replaces occurrences of "</script>" and
other risky sequences) and use them: call escapeHtml(title) in buildMcpAppHtml,
use escapeScript when embedding appClientScript and the incoming script, and
update getBundles to escape coreBundle/codeBundle output before injecting into
the `<script type="module">` tags (references: getBundles, buildMcpAppHtml,
McpAppBundles, McpAppHtmlOptions, appClientScript, coreBundle, codeBundle).
Ensure the helpers are used consistently so no unescaped user-controlled string
is written into HTML or script tags.
coryrylan
commented
May 15, 2026
| const iconsListStyleSheet = new CSSStyleSheet(); | ||
| iconsListStyleSheet.replaceSync(iconsListStyle); | ||
|
|
||
| class ElementsIconsList extends HTMLElement { |
Collaborator
Author
There was a problem hiding this comment.
Right now these components are vanilla js so they can be inlined into the mcp app html page. I think long term I'd like to create a new internal tools-ui style repo that can build these in our typical flow and then bundle into the html file.
coryrylan
commented
May 15, 2026
| ) { | ||
| super(message); | ||
| this.name = 'ToolError'; | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Created ToolError so tool errors can pass back object/arrays to the caller (mcp/cli)
coryrylan
commented
May 15, 2026
| declare const __ELEMENTS_PLAYGROUND_BASE_URL__: string; | ||
| declare const __ELEMENTS_ESM_CDN_BASE_URL__: string; | ||
| const ELEMENTS_PLAYGROUND_BASE_URL = process.env.ELEMENTS_PLAYGROUND_BASE_URL ?? ''; | ||
| const ELEMENTS_ESM_CDN_BASE_URL = process.env.ELEMENTS_ESM_CDN_BASE_URL ?? ''; |
Collaborator
Author
There was a problem hiding this comment.
This allows local build to dynamically populate the url path
coryrylan
commented
May 15, 2026
| { path: 'dist/nve-windows-x64.exe', name: 'nve-windows-x64.exe', label: 'CLI (Windows x64)' } | ||
| ] | ||
| } | ||
| : {}) |
Collaborator
Author
There was a problem hiding this comment.
Dropped these from the release step as the upload was flakey and causing timeouts. They are still available through our typical install path via the docs install/deploy
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@projects/cli/src/index.ts`:
- Around line 49-50: The error outputs are being written to stdout via
console.log in the error handler and in exitWithToolError; change those to write
to stderr by replacing console.log calls used for error messages with
console.error in the .on('error', ...) handler and inside the exitWithToolError
function (the invocation that prints result === undefined ? colors.error(...) :
await renderResult(result)), preserving the existing process.exit(1) behavior so
only error text goes to stderr while normal command output remains on stdout.
In `@projects/cli/src/mcp/apps/app.js`:
- Around line 44-50: callServerTool currently inserts {resolve, reject} into
this.#handlers without any cleanup, causing leaked handlers if the parent never
responds; modify callServerTool to attach a timeout (e.g., setTimeout) when
creating the promise, store the timer handle alongside resolve/reject in
this.#handlers (e.g., {resolve,reject,timeoutId}), and on either receiving a
response or on timeout ensure you clearTimeout(timeoutId) and remove the entry
via this.#handlers.delete(id); on timeout reject the promise with a clear error
message so callers can handle failures. Ensure any existing response-paths (the
code that invokes resolve/reject by id) also delete the handler and clear the
timeout to avoid double work.
- Around line 29-35: The initialization call 'ui/initialize' is sent with an
unused request id (++this.#nextId) so any response will be ignored; change it to
a notification by removing the id and the ++this.#nextId increment (i.e. call
this.#send with jsonrpc: '2.0', method: 'ui/initialize', params: { name:
this.name, version: this.version }) or alternatively add a proper response
handler keyed by this.#nextId if you intend to keep it as a request; update
references to this.#nextId and ensure no stale increment remains.
In `@projects/cli/src/mcp/apps/index.ts`:
- Around line 75-95: The title passed into buildMcpAppHtml is injected raw into
the <title> element which can break HTML if it contains characters like <, >, &,
etc.; add an HTML-escaping step (e.g., an escapeHtml utility that replaces &, <,
>, " and ') and apply it to the title parameter before interpolation in
buildMcpAppHtml so the emitted <title>${escapedTitle}</title> is safe; implement
the escape helper near buildMcpAppHtml (or reuse an existing sanitizer) and
ensure all callers still pass the plain title string.
In `@projects/cli/src/mcp/mcp.ts`:
- Around line 33-49: Replace the loose any for the second parameter of
attachProgress with a typed interface (e.g., ProgressHandlerExtra) that declares
optional _meta { progressToken?: string|number } and an optional
sendNotification method with the correct notification shape; change the function
signature to accept extra?: ProgressHandlerExtra, add a runtime guard to return
early if progressToken is undefined or typeof extra.sendNotification !==
'function', and keep the existing params.onProgress implementation but rely on
the typed sendNotification for compile-time safety.
- Around line 20-29: The AppResource interface and resource registration need a
title field: add title: string to the AppResource interface, update each object
in appResources to include title (extract the human-readable title from the
corresponding buildMcpAppHtml call or source that produces the HTML), and pass
that title into the metadata when calling server.registerResource inside
registerAppResources (i.e., include title along with mimeType and description in
the metadata object used by registerResource for each app in appResources);
update references to APP_MIME_TYPE, app.getHtml(), and
McpServer.registerResource accordingly so metadata contains the new title
property.
In `@projects/internals/tools/src/examples/service.test.ts`:
- Around line 165-191: The test's mocked lint message (via
ApiService.templateValidate) does not match the expectation: update the mocked
lint output in the vi.spyOn(...).mockResolvedValue call so the mocked object's
message field equals "Unexpected use of unknown tag <nve-invalid>" (or
alternatively change the expect to assert the current mocked message "Invalid
template."); locate the mock in the test for the render tool (test uses
loadTools(ExamplesService) and invokes the render tool) and make the message
strings consistent between the mock and the expect so the assertion on
result.result?.lintMessages[0]?.message passes.
🪄 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: Enterprise
Run ID: c72c5d0c-25e3-4e28-a860-14bf32ab7a6f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
projects/cli/package.jsonprojects/cli/src/index.test.tsprojects/cli/src/index.tsprojects/cli/src/mcp/apps/api-icons-list.jsprojects/cli/src/mcp/apps/api-tokens-list.jsprojects/cli/src/mcp/apps/app.jsprojects/cli/src/mcp/apps/examples-render.jsprojects/cli/src/mcp/apps/index.test.tsprojects/cli/src/mcp/apps/index.tsprojects/cli/src/mcp/mcp.test.tsprojects/cli/src/mcp/mcp.tsprojects/cli/src/types.d.tsprojects/cli/src/utils.test.tsprojects/cli/vitest.config.tsprojects/core/src/button/button.cssprojects/core/src/internal/services/global.utils.tsprojects/core/src/internal/utils/audit-logs.tsprojects/internals/tools/src/api/service.test.tsprojects/internals/tools/src/api/service.tsprojects/internals/tools/src/api/utils.test.tsprojects/internals/tools/src/api/utils.tsprojects/internals/tools/src/distill/examples.tsprojects/internals/tools/src/examples/service.test.tsprojects/internals/tools/src/examples/service.tsprojects/internals/tools/src/index.test.tsprojects/internals/tools/src/index.tsprojects/internals/tools/src/internal/tools.test.tsprojects/internals/tools/src/internal/tools.tsprojects/internals/tools/src/playground/service.test.tsprojects/internals/tools/src/playground/service.tsprojects/internals/tools/src/playground/utils.tsprojects/lint/src/eslint/internals/index.test.tsprojects/lint/src/eslint/internals/index.tsrelease.config.js
💤 Files with no reviewable changes (2)
- projects/core/src/internal/services/global.utils.ts
- projects/core/src/internal/utils/audit-logs.ts
Comment on lines
+29
to
+35
| this.#send({ | ||
| jsonrpc: '2.0', | ||
| id: ++this.#nextId, | ||
| method: 'ui/initialize', | ||
| params: { name: this.name, version: this.version } | ||
| }); | ||
| this.#send({ jsonrpc: '2.0', method: 'ui/notifications/initialized' }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Initialization request-response id is unused.
The ui/initialize request uses ++this.#nextId but no handler is registered for it. If the parent responds, the response will be silently ignored. If no response is expected, use a notification (no id field) instead.
Proposed fix to send notification instead
- this.#send({
- jsonrpc: '2.0',
- id: ++this.#nextId,
- method: 'ui/initialize',
- params: { name: this.name, version: this.version }
- });
+ this.#send({
+ jsonrpc: '2.0',
+ method: 'ui/initialize',
+ params: { name: this.name, version: this.version }
+ });🤖 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 `@projects/cli/src/mcp/apps/app.js` around lines 29 - 35, The initialization
call 'ui/initialize' is sent with an unused request id (++this.#nextId) so any
response will be ignored; change it to a notification by removing the id and the
++this.#nextId increment (i.e. call this.#send with jsonrpc: '2.0', method:
'ui/initialize', params: { name: this.name, version: this.version }) or
alternatively add a proper response handler keyed by this.#nextId if you intend
to keep it as a request; update references to this.#nextId and ensure no stale
increment remains.
Comment on lines
+165
to
+191
| it('should return a managed tool error when render validation fails', async () => { | ||
| vi.spyOn(ApiService, 'templateValidate').mockResolvedValue([ | ||
| { | ||
| id: 'invalid-template', | ||
| severity: 'error', | ||
| message: 'Invalid template.', | ||
| suggestions: [], | ||
| line: 1, | ||
| column: 2, | ||
| endLine: 1, | ||
| endColumn: 3 | ||
| } | ||
| ]); | ||
| const tools = loadTools(ExamplesService); | ||
| const renderTool = tools.find(tool => tool.metadata.name === 'render'); | ||
|
|
||
| const result = (await renderTool?.({ template: '<nve-invalid></nve-invalid>' })) as ToolOutput<{ | ||
| template: string; | ||
| lintMessages: { message: string }[]; | ||
| }>; | ||
|
|
||
| expect(result.status).toBe('error'); | ||
| expect(result.message).toBe('Template validation failed.'); | ||
| expect(result.result?.template).toBe('<nve-invalid></nve-invalid>'); | ||
| expect(result.result?.lintMessages).toHaveLength(1); | ||
| expect(result.result?.lintMessages[0]?.message).toBe('Unexpected use of unknown tag <nve-invalid>'); | ||
| }); |
There was a problem hiding this comment.
Mocked lint message and asserted message are inconsistent.
Line 190 asserts a message different from the mocked lint output defined on Lines 166-176, which makes this test fail for the wrong reason.
Suggested fix
- expect(result.result?.lintMessages[0]?.message).toBe('Unexpected use of unknown tag <nve-invalid>');
+ expect(result.result?.lintMessages[0]?.message).toBe('Invalid template.');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should return a managed tool error when render validation fails', async () => { | |
| vi.spyOn(ApiService, 'templateValidate').mockResolvedValue([ | |
| { | |
| id: 'invalid-template', | |
| severity: 'error', | |
| message: 'Invalid template.', | |
| suggestions: [], | |
| line: 1, | |
| column: 2, | |
| endLine: 1, | |
| endColumn: 3 | |
| } | |
| ]); | |
| const tools = loadTools(ExamplesService); | |
| const renderTool = tools.find(tool => tool.metadata.name === 'render'); | |
| const result = (await renderTool?.({ template: '<nve-invalid></nve-invalid>' })) as ToolOutput<{ | |
| template: string; | |
| lintMessages: { message: string }[]; | |
| }>; | |
| expect(result.status).toBe('error'); | |
| expect(result.message).toBe('Template validation failed.'); | |
| expect(result.result?.template).toBe('<nve-invalid></nve-invalid>'); | |
| expect(result.result?.lintMessages).toHaveLength(1); | |
| expect(result.result?.lintMessages[0]?.message).toBe('Unexpected use of unknown tag <nve-invalid>'); | |
| }); | |
| it('should return a managed tool error when render validation fails', async () => { | |
| vi.spyOn(ApiService, 'templateValidate').mockResolvedValue([ | |
| { | |
| id: 'invalid-template', | |
| severity: 'error', | |
| message: 'Invalid template.', | |
| suggestions: [], | |
| line: 1, | |
| column: 2, | |
| endLine: 1, | |
| endColumn: 3 | |
| } | |
| ]); | |
| const tools = loadTools(ExamplesService); | |
| const renderTool = tools.find(tool => tool.metadata.name === 'render'); | |
| const result = (await renderTool?.({ template: '<nve-invalid></nve-invalid>' })) as ToolOutput<{ | |
| template: string; | |
| lintMessages: { message: string }[]; | |
| }>; | |
| expect(result.status).toBe('error'); | |
| expect(result.message).toBe('Template validation failed.'); | |
| expect(result.result?.template).toBe('<nve-invalid></nve-invalid>'); | |
| expect(result.result?.lintMessages).toHaveLength(1); | |
| expect(result.result?.lintMessages[0]?.message).toBe('Invalid template.'); | |
| }); |
🤖 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 `@projects/internals/tools/src/examples/service.test.ts` around lines 165 -
191, The test's mocked lint message (via ApiService.templateValidate) does not
match the expectation: update the mocked lint output in the
vi.spyOn(...).mockResolvedValue call so the mocked object's message field equals
"Unexpected use of unknown tag <nve-invalid>" (or alternatively change the
expect to assert the current mocked message "Invalid template."); locate the
mock in the test for the render tool (test uses loadTools(ExamplesService) and
invokes the render tool) and make the message strings consistent between the
mock and the expect so the assertion on result.result?.lintMessages[0]?.message
passes.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
projects/internals/tools/src/distill/examples.ts (1)
33-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard default-example detection against empty IDs.
Line 33 treats
''as default (!''.includes('-') === true), so Lines 35-37 can include malformed examples unintentionally.Suggested fix
- const isDefault = !id.includes('-'); // default is only the element/api name, no suffix + const isDefault = id.length > 0 && !id.includes('-'); // default is only the element/api name, no suffix🤖 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 `@projects/internals/tools/src/distill/examples.ts` around lines 33 - 37, The detection of default examples treats empty id ('') as default because isDefault is computed as !id.includes('-'); change the guard to ensure id is non-empty before checking for '-' (e.g., compute isDefault as id is truthy/non-empty AND does not include '-') so that empty or malformed ids are not classified as default; update the usage in the existing conditional that returns true when (composition || isDefault) accordingly (referencing the id and isDefault variables in this block).projects/core/src/button/button.css (1)
147-149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExclude
:state(pressed)from hover styling as well.Line 147 still allows pressed pseudo-state hosts to receive hover color, because only
[pressed]is excluded.Suggested fix
-:host(:hover:not([interaction], [disabled], [readonly], [pressed])) { +:host(:hover:not([interaction], [disabled], [readonly], [pressed]):not(:state(pressed))) { --color: var(--nve-sys-interaction-hover-color); }🤖 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 `@projects/core/src/button/button.css` around lines 147 - 149, The hover selector currently excludes the [pressed] attribute but still matches hosts using the pressed pseudo-state; update the selector that reads :host(:hover:not([interaction], [disabled], [readonly], [pressed])) to also exclude the pressed pseudo-state by adding a :not(:state(pressed)) (e.g., :host(:hover:not([interaction],[disabled],[readonly],[pressed]):not(:state(pressed)))) so hovered pressed elements do not receive the hover color; adjust the selector in the button hover rule accordingly.projects/cli/src/index.ts (1)
48-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSend tool failures to stderr, not stdout.
Line 49 uses
console.logfor error output, which pollutes stdout and breaks script consumers expecting stdout to remain data-only. Useconsole.errorinstead:async function exitWithToolError(result: unknown, message: string | undefined): Promise<never> { - console.log(result === undefined ? colors.error(message ?? 'unknown error') : await renderResult(result)); + console.error(result === undefined ? colors.error(message ?? 'unknown error') : await renderResult(result)); process.exit(1); }Also applies to line 35 in the
.on('error', ...)handler:.on('error', (error) => { const message = error?.message; if (message !== null) { - console.log(colors.error(message)); + console.error(colors.error(message)); } process.exit(1); });🤖 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 `@projects/cli/src/index.ts` around lines 48 - 51, The function exitWithToolError currently writes errors to stdout via console.log; change it to console.error so failures go to stderr (i.e., replace console.log(...) with console.error(...)) while keeping the existing message construction and await renderResult(result) logic in exitWithToolError; also update the .on('error', ...) error handler (the event listener around line 35) to use console.error instead of console.log so all tool failures are emitted to stderr.projects/internals/tools/src/api/service.test.ts (1)
277-287:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the query-filtering test deterministic against metadata churn.
The hard-coded
"shadow"query creates a flaky test that can fail when token data changes, even if the filtering logic is correct. Derive the query from returned token data so the test validates behavior rather than catalog contents.✅ Suggested fix
- it('should filter tokens by query', async () => { - const result = (await ApiService.tokensList({ format: 'json', query: 'shadow' })) as { + it('should filter tokens by query', async () => { + const allTokens = (await ApiService.tokensList({ format: 'json' })) as { + name: string; + value: string; + description: string; + }[]; + expect(allTokens.length).toBeGreaterThan(0); + + const query = allTokens[0].name.substring(0, 5); + const result = (await ApiService.tokensList({ format: 'json', query })) as { name: string; value: string; description: string; }[]; expect(result.length).toBeGreaterThan(0); expect( - result.every(token => [token.name, token.value, token.description].join(' ').toLowerCase().includes('shadow')) + result.every(token => + [token.name, token.value, token.description].join(' ').toLowerCase().includes(query.toLowerCase()) + ) ).toBe(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 `@projects/internals/tools/src/api/service.test.ts` around lines 277 - 287, The current test in projects/internals/tools/src/api/service.test.ts uses a hard-coded query "shadow" which makes it flaky; change it to derive the query from actual token data by first calling ApiService.tokensList({ format: 'json' }) to obtain tokens, pick a token with a non-empty name/value/description (from the returned array), extract a short substring (e.g., a word or prefix) from that token's name/value/description, then call ApiService.tokensList({ format: 'json', query: derivedQuery }) and assert that all returned tokens contain the derivedQuery in their name, value, or description; update the test block referencing ApiService.tokensList and the test name to reflect deterministic query derivation.projects/internals/tools/src/playground/service.test.ts (1)
133-149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert tool discovery before invoking
createTool.Line 138 uses optional chaining (
createTool?.(...)) which returnsundefinedif the tool lookup fails. The test would then fail with a confusing property-access error instead of clearly reporting the missing registration.Suggested fix
const tools = loadTools(PlaygroundService); const createTool = tools.find(tool => tool.metadata.name === 'create'); +expect(createTool).toBeDefined(); -const result = (await createTool?.({ +const result = (await createTool!({ template: '<nve-button nve-layout="column">hello</nve-button>', start: false })) as ToolOutput<{ message: string }[]>;🤖 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 `@projects/internals/tools/src/playground/service.test.ts` around lines 133 - 149, The test uses optional chaining on createTool which can hide a missing registration; after obtaining createTool from loadTools(PlaygroundService) assert it exists (e.g., expect(createTool).toBeDefined() or throw a clear error) before invoking it, so failures report missing tool registration instead of causing downstream undefined property errors; update the test around the createTool lookup (loadTools, PlaygroundService, createTool) to validate presence before calling createTool(...).projects/cli/src/mcp/mcp.ts (2)
20-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd title property to AppResource interface and registration.
Per previous review, MCP clients need a human-readable resource title for UI display. The current code embeds titles in HTML but omits them from resource metadata registration. The
AppResourceinterface lacks atitleproperty.Required fixes:
- Add
title: string;to theAppResourceinterface inprojects/cli/src/mcp/apps/index.ts- Add
titleto each resource definition (extract frombuildMcpAppHtmlcalls)- Pass
titlein theregisterResourcemetadata object on line 25🤖 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 `@projects/cli/src/mcp/mcp.ts` around lines 20 - 30, Add a title field to the AppResource model and include it in registration: update the AppResource interface (symbol AppResource) to add title: string; in the apps index, update every appResources entry to include the title (use the same human-readable title used when calling buildMcpAppHtml for each app), and modify registerAppResources so the server.registerResource call (in function registerAppResources on McpServer) includes title in the metadata object alongside mimeType and description (i.e. { mimeType: APP_MIME_TYPE, description: app.description, title: app.title }). Ensure appResources and registerResource usage are consistent with the new title property.
33-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace
anytype with a properly typed interface.The
extra: anyparameter bypasses TypeScript's type system, allowing runtime errors whensendNotificationis missing. Define a typed interface to enable compile-time safety:interface ProgressHandlerExtra { _meta?: { progressToken?: string | number }; sendNotification?: (notification: { method: 'notifications/progress'; params: { progressToken: string | number; progress: number; message: string }; }) => Promise<unknown>; } function attachProgress( params: Record<string, unknown>, extra?: ProgressHandlerExtra ): void { const progressToken = extra?._meta?.progressToken; if (progressToken === undefined || typeof extra?.sendNotification !== 'function') return; // ... }As per coding guidelines, TypeScript code must follow type safety guidelines including type guards and avoiding
any.🤖 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 `@projects/cli/src/mcp/mcp.ts` around lines 33 - 50, The function attachProgress uses extra: any which disables type checking and can cause runtime errors if sendNotification is missing; introduce a ProgressHandlerExtra interface describing _meta.progressToken and sendNotification signature, change the extra parameter to extra?: ProgressHandlerExtra, add a type guard checking typeof extra?.sendNotification === 'function' and that progressToken is defined before assigning params.onProgress, and reference params.onProgress and extra.sendNotification in the guard so the implementation is type-safe and avoids using any.
🤖 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 `@projects/cli/src/mcp/apps/api-tokens-list.js`:
- Around line 694-702: app.ontoolresult currently calls loadTokens({ force:
true, silent: true }) whenever setTokensFromPayload succeeds and tokenList.query
is truthy, which discards the just-set tokens; either make the intent explicit
or change the behavior: if the reload is intentional to get server-filtered
results, add a one-line comment above the conditional describing that
(mentioning tokenList.query triggers server-side filtering so we force-refresh
via loadTokens), otherwise remove the force reload and simply return after
setTokensFromPayload so the newly-set tokens are preserved; reference
app.ontoolresult, setTokensFromPayload, tokenList.query and loadTokens when
editing.
In `@projects/cli/src/mcp/apps/app.js`:
- Around line 65-67: The current `#isAllowedParentOrigin` method is too permissive
(accepts any non-empty string); change it to validate origin against a
configured allowlist: load a trustedOrigins array (from configuration, env var,
or App settings) and return true only if origin exactly matches an entry (or
matches a defined pattern if you implement explicit wildcard support). Update
the method name and usage (keep `#isAllowedParentOrigin`) to reference the
trustedOrigins config, ensure trustedOrigins is validated at startup, and
fail-safe to deny unknown origins rather than allowing them.
In `@projects/cli/src/mcp/apps/index.test.ts`:
- Around line 7-25: Replace the brittle string-slicing assertions in the test
for buildMcpAppHtml with a runtime behavior test: render the generated HTML into
a JSDOM fixture using the createFixture helper, wait for stability with
elementIsStable, then simulate postMessage events from different origins/sources
to the iframe/window and assert that legitimate messages are processed and
malicious ones are ignored; target the postMessage handling code by exercising
buildMcpAppHtml-generated listener (look for window.addEventListener('message'))
and verify outcomes (e.g., processed data handler called or not) instead of
asserting raw HTML text.
---
Duplicate comments:
In `@projects/cli/src/index.ts`:
- Around line 48-51: The function exitWithToolError currently writes errors to
stdout via console.log; change it to console.error so failures go to stderr
(i.e., replace console.log(...) with console.error(...)) while keeping the
existing message construction and await renderResult(result) logic in
exitWithToolError; also update the .on('error', ...) error handler (the event
listener around line 35) to use console.error instead of console.log so all tool
failures are emitted to stderr.
In `@projects/cli/src/mcp/mcp.ts`:
- Around line 20-30: Add a title field to the AppResource model and include it
in registration: update the AppResource interface (symbol AppResource) to add
title: string; in the apps index, update every appResources entry to include the
title (use the same human-readable title used when calling buildMcpAppHtml for
each app), and modify registerAppResources so the server.registerResource call
(in function registerAppResources on McpServer) includes title in the metadata
object alongside mimeType and description (i.e. { mimeType: APP_MIME_TYPE,
description: app.description, title: app.title }). Ensure appResources and
registerResource usage are consistent with the new title property.
- Around line 33-50: The function attachProgress uses extra: any which disables
type checking and can cause runtime errors if sendNotification is missing;
introduce a ProgressHandlerExtra interface describing _meta.progressToken and
sendNotification signature, change the extra parameter to extra?:
ProgressHandlerExtra, add a type guard checking typeof extra?.sendNotification
=== 'function' and that progressToken is defined before assigning
params.onProgress, and reference params.onProgress and extra.sendNotification in
the guard so the implementation is type-safe and avoids using any.
In `@projects/core/src/button/button.css`:
- Around line 147-149: The hover selector currently excludes the [pressed]
attribute but still matches hosts using the pressed pseudo-state; update the
selector that reads :host(:hover:not([interaction], [disabled], [readonly],
[pressed])) to also exclude the pressed pseudo-state by adding a
:not(:state(pressed)) (e.g.,
:host(:hover:not([interaction],[disabled],[readonly],[pressed]):not(:state(pressed))))
so hovered pressed elements do not receive the hover color; adjust the selector
in the button hover rule accordingly.
In `@projects/internals/tools/src/api/service.test.ts`:
- Around line 277-287: The current test in
projects/internals/tools/src/api/service.test.ts uses a hard-coded query
"shadow" which makes it flaky; change it to derive the query from actual token
data by first calling ApiService.tokensList({ format: 'json' }) to obtain
tokens, pick a token with a non-empty name/value/description (from the returned
array), extract a short substring (e.g., a word or prefix) from that token's
name/value/description, then call ApiService.tokensList({ format: 'json', query:
derivedQuery }) and assert that all returned tokens contain the derivedQuery in
their name, value, or description; update the test block referencing
ApiService.tokensList and the test name to reflect deterministic query
derivation.
In `@projects/internals/tools/src/distill/examples.ts`:
- Around line 33-37: The detection of default examples treats empty id ('') as
default because isDefault is computed as !id.includes('-'); change the guard to
ensure id is non-empty before checking for '-' (e.g., compute isDefault as id is
truthy/non-empty AND does not include '-') so that empty or malformed ids are
not classified as default; update the usage in the existing conditional that
returns true when (composition || isDefault) accordingly (referencing the id and
isDefault variables in this block).
In `@projects/internals/tools/src/playground/service.test.ts`:
- Around line 133-149: The test uses optional chaining on createTool which can
hide a missing registration; after obtaining createTool from
loadTools(PlaygroundService) assert it exists (e.g.,
expect(createTool).toBeDefined() or throw a clear error) before invoking it, so
failures report missing tool registration instead of causing downstream
undefined property errors; update the test around the createTool lookup
(loadTools, PlaygroundService, createTool) to validate presence before calling
createTool(...).
🪄 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: Enterprise
Run ID: d688fe20-2e36-45fd-b410-f372997768fb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
projects/cli/package.jsonprojects/cli/src/index.test.tsprojects/cli/src/index.tsprojects/cli/src/mcp/apps/api-icons-list.jsprojects/cli/src/mcp/apps/api-tokens-list.jsprojects/cli/src/mcp/apps/app.jsprojects/cli/src/mcp/apps/examples-render.jsprojects/cli/src/mcp/apps/index.test.tsprojects/cli/src/mcp/apps/index.tsprojects/cli/src/mcp/mcp.test.tsprojects/cli/src/mcp/mcp.tsprojects/cli/src/types.d.tsprojects/cli/src/utils.test.tsprojects/cli/vitest.config.tsprojects/core/src/button/button.cssprojects/core/src/internal/services/global.utils.tsprojects/core/src/internal/utils/audit-logs.tsprojects/internals/tools/src/api/service.test.tsprojects/internals/tools/src/api/service.tsprojects/internals/tools/src/api/utils.test.tsprojects/internals/tools/src/api/utils.tsprojects/internals/tools/src/distill/examples.tsprojects/internals/tools/src/examples/service.test.tsprojects/internals/tools/src/examples/service.tsprojects/internals/tools/src/index.test.tsprojects/internals/tools/src/index.tsprojects/internals/tools/src/internal/tools.test.tsprojects/internals/tools/src/internal/tools.tsprojects/internals/tools/src/playground/service.test.tsprojects/internals/tools/src/playground/service.tsprojects/internals/tools/src/playground/utils.tsprojects/lint/src/eslint/internals/index.test.tsprojects/lint/src/eslint/internals/index.tsrelease.config.js
💤 Files with no reviewable changes (2)
- projects/core/src/internal/services/global.utils.ts
- projects/core/src/internal/utils/audit-logs.ts
Comment on lines
+694
to
+702
| app.ontoolresult = params => { | ||
| if (setTokensFromPayload(params)) { | ||
| if (tokenList.query) { | ||
| void loadTokens({ force: true, silent: true }); | ||
| } | ||
| return; | ||
| } | ||
| void loadTokens(); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Clarify the force reload logic when query exists.
When setTokensFromPayload succeeds and tokenList.query is truthy, the code immediately calls loadTokens({ force: true, silent: true }). This discards the just-set tokens and re-fetches them. If this is intentional (e.g., to get server-filtered results), add a comment explaining the purpose.
🤖 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 `@projects/cli/src/mcp/apps/api-tokens-list.js` around lines 694 - 702,
app.ontoolresult currently calls loadTokens({ force: true, silent: true })
whenever setTokensFromPayload succeeds and tokenList.query is truthy, which
discards the just-set tokens; either make the intent explicit or change the
behavior: if the reload is intentional to get server-filtered results, add a
one-line comment above the conditional describing that (mentioning
tokenList.query triggers server-side filtering so we force-refresh via
loadTokens), otherwise remove the force reload and simply return after
setTokensFromPayload so the newly-set tokens are preserved; reference
app.ontoolresult, setTokensFromPayload, tokenList.query and loadTokens when
editing.
coryrylan
commented
May 18, 2026
| ${script} | ||
| </script> | ||
| </body> | ||
| </html>`; |
Collaborator
Author
There was a problem hiding this comment.
due to some mcp app client restrictions with dynamic resources, we need to inline/bundles all the required assets to make the mcp app reliable
coryrylan
commented
May 18, 2026
| expect(elementScript).not.toMatch(/this\.app\s*=/); | ||
| expect(appScript).toContain('app.callServerTool'); | ||
| expect(appScript).toContain("tokenList.addEventListener('tokens-request'"); | ||
| }); |
Collaborator
Author
There was a problem hiding this comment.
Added specific tests to check that the custom element inlined is separated from the mcp app api's. The goal here is the vanilla custom elements would eventually be extracted out to stand alone internal ui project.
coryrylan
commented
May 18, 2026
| declare module '*?raw' { | ||
| const content: string; | ||
| export default content; | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Right now the three mcp app ui resources are plain js custom elements inlined into the index.html resource
coryrylan
commented
May 18, 2026
| ], | ||
| "scripts": { | ||
| "dev": "pnpm run nve:install && pnpm dlx @modelcontextprotocol/inspector@0.21.2 node ./dist/index.js mcp", | ||
| "dev": "pnpm run nve:install && npx @modelcontextprotocol/inspector@0.21.2 node ./dist/index.js mcp", |
Collaborator
Author
There was a problem hiding this comment.
Added npx here due to modelcontextprotocol/inspector#873
coryrylan
commented
May 18, 2026
| }, | ||
| { | ||
| "script": "../styles:build", | ||
| "cascade": false |
Collaborator
Author
There was a problem hiding this comment.
CLI now relies on these builds since we create the mcp-app html files
johnyanarella
approved these changes
May 18, 2026
| @@ -0,0 +1,115 @@ | |||
| class Client { | |||
Signed-off-by: Cory Rylan <crylan@nvidia.com>
- fix contrast issue for pressed state - remove unused esm logging message Signed-off-by: Cory Rylan <crylan@nvidia.com>
- Added new app resources for icons and tokens lists, and example previews. - Improved error handling in the CLI tool for better user feedback. Signed-off-by: Cory Rylan <crylan@nvidia.com>
Collaborator
Author
|
🎉 This issue has been resolved in version 0.1.2 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.2.0 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests