Conversation
…e routing for tools and resources
…rce handling in StorePlugin
…pts, Resources, Server Builder, Store, and Tools pages
… logging in various components
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a browser-first React demo app with Playwright E2E tests, a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as Browser App
participant SM as ServerManagerProvider
participant SR as ServerRegistry
participant Srv as DirectMcpServer
participant FP as FrontMcpProvider
User->>App: Click "Create Server" (name, selections)
App->>SM: createServer(config)
SM->>SR: ServerRegistry.create(name, config)
SR-->>SM: returns DirectMcpServer (created or pending)
SM-->>App: returns ManagedServer
App->>FP: mount FrontMcpProvider with active server -> connect()
FP->>Srv: client.connect(), fetch tools/resources/prompts
Srv-->>FP: tools/resources/prompts
FP-->>App: context populated, UI updates
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 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)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
… logging in various components
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 100 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-02-22T23:35:25.428Z |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
plugins/plugin-remember/package.json (1)
38-55:⚠️ Potential issue | 🟡 MinorAdd
typesfield to browser export conditions.Browser exports lack TypeScript type definitions while
requireandimportconditions specify them at./dist/index.d.ts. This inconsistency breaks type resolution for consumers using TypeScript withmoduleResolution: "bundler"or"node16"when accessing the browser entry point.Add
"types": "./dist/index.d.ts"to both browser export conditions to match the pattern used in other export conditions:"browser": { "types": "./dist/index.d.ts", "import": "./dist/browser/browser.mjs", "default": "./dist/browser/browser.js" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/plugin-remember/package.json` around lines 38 - 55, The browser export conditions in package.json are missing TypeScript type definitions; update both the top-level "browser" export object and the "./browser" export object to include "types": "./dist/index.d.ts" alongside their existing "import" and "default" entries so they match the "require" and "import" conditions that already reference types; modify the "browser" block and the "./browser" block accordingly so TypeScript consumers (moduleResolution bundler/node16) can resolve types.libs/adapters/src/openapi/openapi.types.ts (1)
12-18:⚠️ Potential issue | 🟡 MinorDocument the
InputTransformContexttype in the OpenAPI adapter documentation.
InputTransformContextis part of the public API (exported from@frontmcp/adapters/openapi) and is used in documented examples ofinputTransformscallbacks (e.g.,inject: (ctx) => ctx.authInfo.user?.tenantId). However, the type structure itself is not documented indocs/frontmcp/adapters/openapi-adapter.mdx. Update the "Input Schema Transforms" section to document what properties are available on thectxparameter, including theenv: Record<string, string | undefined>field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/adapters/src/openapi/openapi.types.ts` around lines 12 - 18, Update the "Input Schema Transforms" section to document the exported type InputTransformContext: describe its properties ctx (type FrontMcpContext) including available subfields like authInfo, sessionId, traceId, etc., env (Record<string, string | undefined>) and tool (McpOpenAPITool); mention that env contains environment variables keyed by name with possibly undefined values and show example usages such as inject: (ctx) => ctx.authInfo.user?.tenantId or using ctx.env.MY_VAR to access env entries so readers understand what fields are available on the ctx parameter passed to inputTransforms callbacks.libs/auth/package.json (1)
35-53:⚠️ Potential issue | 🟡 MinorDocument the
@frontmcp/authbrowser entry points in the authentication guides.The new browser export mappings (root and "./browser" subpath) expand the public API surface but are not documented in the authentication docs. Add documentation to
docs/frontmcp/authentication/explaining how to import from@frontmcp/authand@frontmcp/auth/browserentry points.As per coding guidelines: "libs/: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/ update."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/auth/package.json` around lines 35 - 53, The package.json added new browser export mappings (the root "." mapping with "browser" and the "./browser" subpath mapping) that expose browser-specific entry points for `@frontmcp/auth` but the authentication docs lack guidance; update the documentation under docs/frontmcp/authentication/ to describe how to import and use the new entry points (examples for importing from "@frontmcp/auth" vs "@frontmcp/auth/browser"), note any differences in exported modules or initialization for browser builds, and include a short migration/snippet section referencing the package.json export keys (root "." and "./browser") so consumers know when to use each import.libs/adapters/package.json (1)
29-56:⚠️ Potential issue | 🟡 MinorDocument the browser export for the openapi subpath.
This adds a browser export for
@frontmcp/adapters/openapi, which is a public API change. Please add documentation for this new entry point indocs/frontmcp/adapters/openapi-adapter.mdx, including guidance on when and how to use the browser build variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/adapters/package.json` around lines 29 - 56, The package export for the openapi subpath now includes a "browser" target (exposed as the `@frontmcp/adapters/openapi` entry), so update the openapi-adapter documentation to document this new browser build variant: describe that the package provides a browser-optimized bundle, show example import usage for browser consumers (e.g., importing the openapi subpath), explain when to choose the browser variant vs the default (bundle size, environment constraints, and compatibility with bundlers like webpack/rollup/vite), and add any caveats (ESM vs CJS, types location) so consumers know how and when to use the browser export. Ensure the docs page clearly references the new "browser" export and includes a short usage snippet and compatibility notes.plugins/plugin-cache/package.json (1)
37-54:⚠️ Potential issue | 🟠 MajorAdd typings for the new
./browserexport.TypeScript consumers importing
@frontmcp/plugin-cache/browserwill fail to resolve typings without explicittypesconditions. Since browser.ts exports the same public API as index.ts (identical exports from cache.plugin and cache.types), reuse the generated declaration file for both export conditions.🔧 Proposed fix
".": { "browser": { + "types": "./dist/index.d.ts", "import": "./dist/browser/browser.mjs", "default": "./dist/browser/browser.js" }, @@ "./browser": { + "types": "./dist/index.d.ts", "import": "./dist/browser/browser.mjs", "default": "./dist/browser/browser.js" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/plugin-cache/package.json` around lines 37 - 54, The "./browser" export in package.json is missing a "types" condition so TypeScript imports from "@frontmcp/plugin-cache/browser" won't resolve typings; add a "types" condition under the "./browser" export that points to the generated declaration file (reuse the same declaration used by the main export, e.g. "dist/index.d.ts" or "dist/esm/index.d.ts") so the "./browser" entry mirrors the main export's shape (include "types" alongside "import" and "default" for the "./browser" export).libs/sdk/package.json (1)
33-51:⚠️ Potential issue | 🟠 MajorDocument the new browser export surface in docs/frontmcp/.
This is a public API change; add or update the corresponding docs entry for@frontmcp/sdk/browserin the SDK reference section (e.g.,docs/frontmcp/sdk-reference/or in relevant guides).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/package.json` around lines 33 - 51, The new browser export surface was added to libs/sdk/package.json (the "." browser/import/default entries and the "./browser" export) exposing `@frontmcp/sdk/browser` as a public API; update the SDK reference docs under docs/frontmcp/sdk-reference/ (or relevant guides) to document the new module name `@frontmcp/sdk/browser`, describe the available entry points (browser ESM and UMD/JS defaults), usage examples (importing the browser bundle vs main SDK), and any build/compatibility notes so consumers know how to import and which files (dist/browser/browser.mjs and dist/browser/browser.js) correspond to the browser entry.
🟡 Minor comments (19)
libs/react/project.json-1-6 (1)
1-6:⚠️ Potential issue | 🟡 MinorAdd
@frontmcp/reactdocumentation to docs/docs.json and docs/frontmcp/.The new publishable
@frontmcp/reactpackage with entry points (., ./ai, ./router, ./browser) is not documented in the navigation structure. Update docs/docs.json to include appropriate navigation entries and add corresponding documentation pages in docs/frontmcp/ (likely under a new "react" or "ui" category).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/project.json` around lines 1 - 6, Add documentation entries for the new publishable package `@frontmcp/react` by updating docs/docs.json to include a new navigation node (e.g., "frontmcp" group or "react"/"ui" subsection) that links to pages for the package and its entry points (., ./ai, ./router, ./browser), and create the corresponding markdown files under docs/frontmcp/ (e.g., react.md and per-entrypoint pages) with basic usage, entrypoint descriptions, and export details; ensure the nav ID/paths in docs/docs.json exactly match the filenames in docs/frontmcp/ so the site builds and the package appears in the docs navigation.apps/e2e/demo-e2e-browser/src/components/OutputLog.tsx-27-31 (1)
27-31:⚠️ Potential issue | 🟡 MinorSet
type="button"to avoid unintended form submission.Buttons default to
submitinside forms, which can cause accidental submits if this component is rendered in a form context.🔧 Suggested fix
- <button onClick={onClear} style={{ padding: '0.2rem 0.5rem', fontSize: '0.75rem' }}> + <button type="button" onClick={onClear} style={{ padding: '0.2rem 0.5rem', fontSize: '0.75rem' }}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-browser/src/components/OutputLog.tsx` around lines 27 - 31, The Clear button in the OutputLog component defaults to type="submit" when placed inside a form; update the button element in OutputLog (the button with onClick={onClear}) to include type="button" so it won't trigger unintended form submissions—locate the button in OutputLog.tsx and add the type attribute while keeping the existing onClick and inline styles.plugins/plugin-store/package.json-1-55 (1)
1-55:⚠️ Potential issue | 🟡 MinorAdd documentation for
@frontmcp/plugin-storeplugin.This new official plugin package is missing documentation. Create a guide in
docs/frontmcp/plugins/official/(similar tocache.mdxandremember.mdx) covering usage and the browser entrypoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/plugin-store/package.json` around lines 1 - 55, Add a new documentation page for the `@frontmcp/plugin-store` plugin under docs/frontmcp/plugins/official/ (e.g., plugin-store.mdx) modeled after cache.mdx and remember.mdx; the doc should describe installation and usage of the plugin (importing the package name "@frontmcp/plugin-store"), configuration examples, API/usage snippets referencing key exports (main/module/types) and explicitly document the browser entrypoint ("./dist/browser/browser.mjs" and "./dist/browser/browser.js") and how to import/use the browser bundle in client builds. Ensure the page explains both Node (development import -> ./src/index.ts or module "./dist/esm/index.mjs") and browser usage, and include a short troubleshooting/notes section about the package.json fields like "type": "commonjs" and "exports".libs/sdk/src/context/context-storage.interface.ts-1-27 (1)
1-27:⚠️ Potential issue | 🟡 MinorAdd documentation for the new
IContextStorageinterface.This introduces a new exported interface in libs/sdk; it needs a corresponding documentation file in
docs/frontmcp/sdk-reference/contexts/covering the platform-agnostic context storage API, following the pattern of existing context documentation files.As per coding guidelines: "libs/** contains publishable SDK libraries. When public APIs change, ensure there is a matching docs/frontmcp/** update."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/context/context-storage.interface.ts` around lines 1 - 27, The new exported interface IContextStorage and its methods run and getStore lack public docs; add a new SDK reference documentation page alongside other context docs that documents the platform-agnostic API (IContextStorage), lists the method signatures (run<R>(store: T, fn: () => R): R and getStore(): T | undefined), explains Node.js (AsyncLocalStorage) vs browser stack behavior, and includes short usage examples showing calling run(...) and reading getStore() inside async flow; follow the existing context docs’ structure and wording conventions so the public API change is reflected in the published SDK docs.apps/e2e/demo-e2e-browser/src/components/StyledField.tsx-15-50 (1)
15-50:⚠️ Potential issue | 🟡 MinorTie labels to controls and avoid clobbering falsy enum values.
The label isn’t associated with its control (a11y regression), and
value || enumValues[0]will override valid falsy values (e.g.,"0").♿ Suggested fix
export function StyledField({ name, type, required, description, enumValues, value, onChange, }: FieldRenderProps): React.ReactElement { + const inputId = `field-${name}`; return ( <div className="form-group" key={name}> - <label> + <label htmlFor={inputId}> {name} {required ? ' *' : ''} {type !== 'string' && <span className="field-type">{type}</span>} </label> {enumValues ? ( - <select value={value || enumValues[0] || ''} onChange={(e) => onChange(e.target.value)}> + <select + id={inputId} + value={value ?? enumValues[0] ?? ''} + onChange={(e) => onChange(e.target.value)} + > {enumValues.map((v) => ( <option key={v} value={v}> {v} </option> ))} </select> ) : type === 'number' || type === 'integer' ? ( <input + id={inputId} type="number" step={type === 'integer' ? '1' : 'any'} placeholder={description ?? ''} required={required} value={value} onChange={(e) => onChange(e.target.value)} /> ) : name === 'text' || name === 'code' || name === 'content' ? ( <textarea + id={inputId} placeholder={description ?? ''} required={required} value={value} onChange={(e) => onChange(e.target.value)} /> ) : ( <input + id={inputId} type="text" placeholder={description ?? ''} required={required} value={value} onChange={(e) => onChange(e.target.value)} /> )} </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-browser/src/components/StyledField.tsx` around lines 15 - 50, The label element in StyledField.tsx isn't associated with its form control and the select uses "value || enumValues[0]" which clobbers valid falsy values; fix by generating a stable id (e.g., fieldId) and setting label htmlFor={fieldId} and the corresponding control id={fieldId}, and change the select's value logic to use a null/undefined check (e.g., value !== undefined && value !== null ? value : enumValues[0]) so falsy values like "0" are preserved; apply the same id and undefined-check pattern for the other inputs/textareas and ensure onChange still calls the existing onChange(value) handler with the raw value.plugins/plugin-store/src/store.types.ts-1-38 (1)
1-38:⚠️ Potential issue | 🟡 MinorPublic plugin API requires documentation update in docs/draft/docs.
New public types
StoreAdapter,StoreEntry, andStorePluginOptionswere added but lack corresponding documentation. Per plugin review guidelines, public API changes must include matching docs/draft/docs updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/plugin-store/src/store.types.ts` around lines 1 - 38, Public API types StoreAdapter, StoreEntry, and StorePluginOptions were added but not documented; update the docs/draft/docs to include entries describing these new types. Add a short section for the store plugin API documenting the StoreAdapter interface (methods: getState(), getState(path), setState(), subscribe(), dispose?), the StoreEntry type (name and adapter properties), and StorePluginOptions (stores?: Record<string, StoreAdapter>), include example usage and intended semantics (e.g., empty path replaces root for setState), and link these docs from the plugin-store README or API index so the new types are discoverable.libs/react/src/components/ToolForm.tsx-29-47 (1)
29-47:⚠️ Potential issue | 🟡 MinorMissing
requiredin dependency array causes stale closure.The
handleSubmitcallback usesrequiredarray but it's not in the dependency array. Ifrequiredchanges (e.g., dynamic schema), the callback will use stale values.🐛 Proposed fix
[values, properties, onSubmit], + [values, properties, required, onSubmit],Full diff:
const handleSubmit = useCallback( (e: React.FormEvent) => { e.preventDefault(); const args: Record<string, unknown> = {}; for (const [key, prop] of Object.entries(properties)) { const raw = values[key] ?? ''; if (!required.includes(key) && raw === '') continue; if (prop['type'] === 'number' || prop['type'] === 'integer') { args[key] = Number(raw); } else if (prop['type'] === 'boolean') { args[key] = raw === 'true'; } else { args[key] = raw; } } onSubmit(args); }, - [values, properties, onSubmit], + [values, properties, required, onSubmit], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/components/ToolForm.tsx` around lines 29 - 47, The handleSubmit useCallback closes over the required array but doesn't list it in the dependencies, causing a stale closure when the schema changes; update the dependency array of handleSubmit (the useCallback wrapping the handleSubmit function in ToolForm.tsx) to include required alongside values, properties, and onSubmit so the callback is recreated whenever required changes.libs/react/src/components/OutputDisplay.tsx-26-36 (1)
26-36:⚠️ Potential issue | 🟡 Minor
JSON.stringifycan throw on circular references.If
datacontains circular references,JSON.stringifywill throw aTypeError. Consider wrapping in try-catch for defensive rendering.🛡️ Proposed defensive fix
if (data === null || data === undefined) { return React.createElement('div', { 'data-testid': 'output-empty' }, ''); } - const formatted = typeof data === 'string' ? data : JSON.stringify(data, null, 2); + let formatted: string; + if (typeof data === 'string') { + formatted = data; + } else { + try { + formatted = JSON.stringify(data, null, 2); + } catch { + formatted = '[Unable to serialize data]'; + } + } return React.createElement(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/components/OutputDisplay.tsx` around lines 26 - 36, The component sets `formatted` using JSON.stringify which can throw on circular references; update the `OutputDisplay` render to wrap the JSON.stringify call (used to set the `formatted` variable) in a try-catch and fall back to a safe string representation (e.g., a replacer-based serialization, a simple "[Circular]" marker, or util.inspect-style output) when an error occurs, so the React.createElement('pre', ...) rendering never throws; ensure you only change the serialization logic around `formatted` and keep the existing data null/undefined check and returned elements intact.libs/sdk/src/browser/shims/path.ts-20-25 (1)
20-25:⚠️ Potential issue | 🟡 MinorEdge case:
dirname('/') returns '.' instead of '/'
dirname('/')returns'.'instead of'/'. Node'spath.dirname('/')returns'/'. Similarly,dirname('/a')also incorrectly returns'.'instead of'/'.🐛 Proposed fix
export function dirname(p: string): string { const normalized = p.replace(/\/+$/, ''); + if (normalized === '' || normalized === '/') return '/'; const parts = normalized.split('/'); parts.pop(); - return parts.join('/') || '.'; + return parts.join('/') || '/'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/browser/shims/path.ts` around lines 20 - 25, The dirname implementation incorrectly returns '.' for root and single-segment absolute paths (e.g., dirname('/') and dirname('/a')); update the dirname(p: string) function to detect root cases after normalizing (retain whether the original path started with '/') and if the normalized result has no parent segments but the path was absolute, return '/' instead of '.'; ensure existing behavior for relative paths and trailing-slash trimming stays the same (use the existing variables like normalized and parts to implement this conditional).apps/e2e/demo-e2e-browser/src/components/ServerSelector.tsx-7-12 (1)
7-12:⚠️ Potential issue | 🟡 MinorAdd an accessible label for the server
<select>.There’s no
<label>oraria-label, which hurts screen-reader usability.✅ Suggested fix
- <select + <select + aria-label="Active server" value={activeServerId ?? ''} onChange={(e) => setActiveServer(e.target.value)} data-testid="server-select" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-browser/src/components/ServerSelector.tsx` around lines 7 - 12, The server <select> in ServerSelector.tsx lacks an accessible label; update the select to have either a visible <label> (e.g., create a <label> element associated via htmlFor and add an id to the select) or add a descriptive aria-label/aria-labelledby so screen readers can announce it; ensure the id you add matches the htmlFor on the label (or use aria-label with the same descriptive text) and keep existing props (value={activeServerId ?? ''} and onChange={(e) => setActiveServer(e.target.value)}) intact.libs/react/src/router/ResourceRoute.tsx-12-13 (1)
12-13:⚠️ Potential issue | 🟡 MinorGuard
decodeURIComponentto avoid route crashes on malformed URIs.If the wildcard contains invalid percent-encoding,
decodeURIComponentthrows and the route render fails. Wrap in a try/catch and fall back to the raw value (or surface a decode error).💡 Suggested fix
- const uri = rawUri ? decodeURIComponent(rawUri) : ''; + let uri = ''; + if (rawUri) { + try { + uri = decodeURIComponent(rawUri); + } catch { + uri = rawUri; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/router/ResourceRoute.tsx` around lines 12 - 13, The code currently calls decodeURIComponent(rawUri) directly in the ResourceRoute component (using useParams and the rawUri wildcard) which can throw on malformed percent-encoding; wrap the decode step in a try/catch around decodeURIComponent(rawUri) and if it throws fall back to the original rawUri (or set uri to '' when rawUri is undefined) so the route won't crash — update the uri assignment to use the guarded decode and keep the rest of the component logic unchanged.libs/sdk/src/__tests__/browser-integration.spec.ts-121-125 (1)
121-125:⚠️ Potential issue | 🟡 MinorUse
ToolExecutionErrorinstead of genericError.Replace
throw new Error('Intentional failure')withthrow new ToolExecutionError('fail_tool', new Error('Intentional failure'))to comply with the SDK guideline for MCP error codes in libs/sdk/src files. This also better reflects the semantic intent of the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/__tests__/browser-integration.spec.ts` around lines 121 - 125, The test's FailTool.execute currently throws a plain Error; replace that with a ToolExecutionError to follow SDK MCP error conventions by throwing new ToolExecutionError('fail_tool', new Error('Intentional failure')) from the execute method of the FailTool class so the error includes the tool name and underlying error; update any imports to ensure ToolExecutionError is imported into the test file if not already.libs/sdk/src/context/index.ts-31-39 (1)
31-39:⚠️ Potential issue | 🟡 MinorAdd SDK reference documentation for new context storage exports.
These exports extend the SDK's public API and should be documented in
docs/frontmcp/sdk-reference/core/(similar todirect-client.mdxandserver.mdx). DocumentIContextStorage,BrowserContextStorage, andFrontMcpContextStoragewith usage examples and platform differences.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/context/index.ts` around lines 31 - 39, The SDK now publicly exports IContextStorage, BrowserContextStorage, and FrontMcpContextStorage from libs/sdk/src/context/index.ts but there is no SDK reference documentation; add new docs under docs/frontmcp/sdk-reference/core/ (mirroring style of direct-client.mdx and server.mdx) that document the IContextStorage interface, the BrowserContextStorage implementation, and the FrontMcpContextStorage wrapper including brief usage examples for browser vs server contexts, API surface (methods/signatures), initialization/config options, and notes on platform differences and best practices for choosing/instantiating each class.apps/e2e/demo-e2e-browser/src/main.tsx-77-77 (1)
77-77:⚠️ Potential issue | 🟡 MinorUnhandled promise rejection risk.
bootstrap()returns a Promise that is neither awaited nor has a.catch()handler. If the async setup fails, the error will be silently swallowed or cause an unhandled rejection.Proposed fix
-bootstrap(); +bootstrap().catch((err) => { + console.error('Bootstrap failed:', err); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-browser/src/main.tsx` at line 77, The call to bootstrap() in main.tsx is fire-and-forget and can cause unhandled promise rejections; update the invocation of bootstrap() (the bootstrap function) to handle its returned Promise — either await it from a top-level async IIFE or attach a .catch(...) that logs the error and exits/handles failure appropriately — so any rejection from bootstrap() is observed and handled.apps/e2e/demo-e2e-browser/src/pages/ComponentsPage.tsx-8-13 (1)
8-13:⚠️ Potential issue | 🟡 MinorMinor:
fractionalSecondDigitsnot supported in Internet Explorer.The
fractionalSecondDigitsoption is supported in all modern browsers (Chrome 84+, Firefox 84+, Safari 14.1+), but not in Internet Explorer. For a demo app, this is acceptable, though be aware the option will be ignored if IE is ever tested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-browser/src/pages/ComponentsPage.tsx` around lines 8 - 13, The formatTime function uses the Intl option fractionalSecondDigits which IE ignores; change it to include that property only when supported — detect support by calling Intl.DateTimeFormat.prototype.resolvedOptions and checking for fractionalSecondDigits (or feature-detect via try/catch), then build the options object conditionally and pass it to new Date().toLocaleTimeString so legacy browsers like IE won't receive an unsupported option; update the formatTime function accordingly.libs/react/src/ai/useAITools.ts-21-24 (1)
21-24:⚠️ Potential issue | 🟡 MinorReset formatted tools when disconnected or tool list is empty.
The early return leaves
formattedToolsunchanged, so consumers can render stale tools after disconnects or when tools are removed. Clearing it avoids leaking previous state.🧹 Suggested fix
useEffect(() => { if (status !== 'connected' || toolInfos.length === 0) { + setFormattedTools(null); setLoading(status !== 'connected' && status !== 'error'); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/ai/useAITools.ts` around lines 21 - 24, When the useEffect early-returns on (status !== 'connected' || toolInfos.length === 0) it leaves formattedTools stale; update the effect to clear formattedTools before returning. Specifically, in the useEffect that checks status and toolInfos, call setFormattedTools([]) (and maintain the existing setLoading(status !== 'connected' && status !== 'error')) right before the return so consumers don't render previous tools when disconnected or the tool list is empty.libs/react/src/router/McpNavigation.tsx-31-70 (1)
31-70:⚠️ Potential issue | 🟡 MinorEncode tool/prompt names in route segments to ensure they work with special characters.
Resources already use
encodeURIComponent(r.uri)with corresponding decode logic in ResourceRoute. Tools and prompts should follow the same pattern for consistency and to avoid routing failures when names contain/, spaces, unicode, or other URL-unsafe characters.🔧 Suggested fix
- React.createElement(Link, { to: `${basePath}/tools/${t.name}` }, t.name), + React.createElement(Link, { to: `${basePath}/tools/${encodeURIComponent(t.name)}` }, t.name), @@ - React.createElement(Link, { to: `${basePath}/prompts/${p.name}` }, p.name), + React.createElement(Link, { to: `${basePath}/prompts/${encodeURIComponent(p.name)}` }, p.name),Additionally, update ToolRoute and PromptRoute to decode the name parameter:
- const { name } = useParams<{ name: string }>(); + const { name: encodedName } = useParams<{ name: string }>(); + const name = encodedName ? decodeURIComponent(encodedName) : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/router/McpNavigation.tsx` around lines 31 - 70, Tools and prompts route segments aren’t URL-encoded like resources, which breaks routing for names with unsafe characters; update the React.createElement Link calls within the tools and prompts mappings to use encodeURIComponent when building the to prop (e.g., change `${basePath}/tools/${t.name}` and `${basePath}/prompts/${p.name}` to `${basePath}/tools/${encodeURIComponent(t.name)}` and `${basePath}/prompts/${encodeURIComponent(p.name)}`), and then update the corresponding route handlers ToolRoute and PromptRoute to decode the route param (use decodeURIComponent on the name param) before using it.plugins/plugin-store/src/store.resources.ts-34-130 (1)
34-130:⚠️ Potential issue | 🟡 MinorAdd safe serialization helper to ensure
textis always a string.
StoreAdapter.getState(path)returnsunknownand can beundefinedwhen a path doesn't exist (the valtio adapter explicitly returnsundefinedfromgetNestedValue()for invalid paths). SinceJSON.stringify(undefined)returnsundefinedrather than a string, andReadResourceResult.contents[].textmust be a string, the serialized output becomes invalid.Add a
serializeState()helper that ensures the result is always stringified:Suggested fix
function buildUri(store: string, path: string[]): string { if (path.length === 0) return `state://${store}`; return `state://${store}/${path.join('/')}`; } + +function serializeState(value: unknown): string { + return JSON.stringify(value) ?? 'null'; +} @@ - return { - contents: [{ uri: buildUri(params.store, []), text: JSON.stringify(storeAdapter.getState()) }], - }; + return { + contents: [{ uri: buildUri(params.store, []), text: serializeState(storeAdapter.getState()) }], + }; @@ - return { contents: [{ uri: buildUri(store, path), text: JSON.stringify(value) }] }; + return { contents: [{ uri: buildUri(store, path), text: serializeState(value) }] };Apply this change to all five resource depth classes (lines 40, 60, 79, 101, 129).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/plugin-store/src/store.resources.ts` around lines 34 - 130, The resources call storeAdapter.getState(...) which can return undefined causing JSON.stringify(undefined) => undefined and breaking ReadResourceResult.contents[].text; add a helper serializeState(value: unknown): string that returns JSON.stringify(value) for defined values and "null" or "undefined" (choose "null" to represent missing state) when value is undefined, then replace JSON.stringify(...) in StoreRootResource.execute, StoreDepth1Resource.execute, StoreDepth2Resource.execute, StoreDepth3Resource.execute and StoreDepth4Resource.execute with serializeState(value) (use the same helper for the root case where you serialize storeAdapter.getState()) so contents[].text is always a string.plugins/plugin-store/src/store.plugin.ts-72-74 (1)
72-74:⚠️ Potential issue | 🟡 MinorAlign
generateStoreResourceTemplatesreturn type withResourceType[]instead of using a double cast workaround.The return type
Array<new (...args: unknown[]) => unknown>is too generic and forces aas unknown as ResourceType[]cast at call sites (lines 73, 82). Since the function returns decorated resource template classes extendingResourceContext, consider declaring the return type asResourceType[]directly or using a more precise type annotation to avoid the casting workaround.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/plugin-store/src/store.plugin.ts` around lines 72 - 74, The generateStoreResourceTemplates function currently returns Array<new (...args: unknown[]) => unknown> causing callers like the static override dynamicResources method to use a double cast (as unknown as ResourceType[]); change generateStoreResourceTemplates's signature to return ResourceType[] (or a more specific ctor type that extends ResourceContext and implements ResourceType) so callers (e.g., dynamicResources) can use its result directly without casting; update any internal typings in generateStoreResourceTemplates to reflect that returned classes extend ResourceContext and satisfy ResourceType.
…g, and add type definitions in package.json
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/auth/package.json (1)
35-55:⚠️ Potential issue | 🟠 MajorBrowser export types must be generated from browser.ts, not index.ts.
The build currently generates
dist/index.d.tsonly fromindex.ts(viabuild-cjswithdeclaration: true), which includes Node.js-only exports likeloadDevKey,saveDevKey,deleteDevKey, andresolveKeyPath. However, the browser runtime (browser.ts) intentionally excludes these. When consumers use the browser condition, they receive:
- JavaScript: browser-safe subset from
browser.ts- Types: full API from
index.d.tsThis creates a type/runtime mismatch: code that compiles will fail at runtime. The browser entrypoints in
package.jsonshould reference./dist/browser/browser.d.ts, which must be generated during the build process. Updatebuild-browser-esmto generate declarations frombrowser.ts(setdeclaration: trueand ensureskipTypeCheck: falseor handle type generation separately).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/auth/package.json` around lines 35 - 55, The package exposes browser runtime JS from browser.ts but currently points TypeScript types at dist/index.d.ts (built from index.ts), causing a type/runtime mismatch; update the build so build-browser-esm generates declaration files from browser.ts and update package.json browser entry to point to ./dist/browser/browser.d.ts. Concretely: modify the build-browser-esm task/config (the step that produces dist/browser/browser.mjs) to enable declaration: true (and ensure type emission for that entry — e.g., skipTypeCheck: false or run a separate tsc invocation targeting browser.ts) so ./dist/browser/browser.d.ts is produced, and change the package.json browser/type mapping to reference ./dist/browser/browser.d.ts instead of ./dist/index.d.ts; ensure browser.ts is the source used for declarations (not index.ts) so browser-only symbols like loadDevKey/saveDevKey are omitted from the browser types.
🧹 Nitpick comments (4)
libs/react/src/router/McpNavigation.tsx (1)
31-37: Pass mapped array directly instead of spreading.Using
...tools.map()as rest arguments toReact.createElementworks, but passing the array directly is more idiomatic and avoids unnecessary spread overhead:♻️ Suggested refactor for all three sections
React.createElement( 'ul', null, - ...tools.map((t) => + tools.map((t) => React.createElement( 'li', { key: t.name }, React.createElement(Link, { to: `${basePath}/tools/${encodeURIComponent(t.name)}` }, t.name), ), ), ),Apply the same pattern to
resources.map()(line 48) andprompts.map()(line 65).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/router/McpNavigation.tsx` around lines 31 - 37, The code spreads the mapped arrays into React.createElement as rest args (e.g. ...tools.map(...)) which is unnecessary; change each usage (tools.map, resources.map, prompts.map) to pass the mapped array directly as the children argument to React.createElement instead of spreading, i.e., replace the spread form with a single array child for the React.createElement call that creates the 'li' list (where Link and basePath are used) and do the same pattern for the other two sections.libs/react/src/ai/useAITools.ts (1)
44-54: Consider wrapping errors for consistency with the hook's error state pattern.The hook maintains an
errorstate for formatting failures, butcallToolpropagates exceptions directly to the caller. This inconsistency means consumers must handle errors differently depending on the operation:
- Check
errorstate for formatting issues- Use try/catch for
callToolfailuresIf this is intentional (allowing consumers to handle tool execution errors granularly), consider documenting this behavior. Otherwise, consider returning a result object or updating the
errorstate on tool invocation failure.Additionally, line 51 uses the same
as Parameters<...>[0]cast pattern that bypasses type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/ai/useAITools.ts` around lines 44 - 54, Wrap the body of callTool in a try/catch so tool execution errors update the hook's error state instead of only propagating: call server.callTool(name, args) inside try, on catch call setError(err) (the hook's error state updater) and then rethrow if you want to preserve caller behavior; this keeps the hook's error state consistent with formatting errors handled elsewhere. Also remove the unsafe inline cast "as Parameters<typeof formatResultForPlatform>[0]" — instead declare rawResult with an explicit type of Parameters<typeof formatResultForPlatform>[0] (or narrow it safely) before passing it to formatResultForPlatform to preserve type safety when calling formatResultForPlatform(rawResult, platform).apps/e2e/demo-e2e-browser/src/main.tsx (1)
1-2: Avoid clobbering an existingBufferglobal.If a polyfill (or runtime) already defines
globalThis.Buffer, overwriting it can cause subtle conflicts. A guarded assignment keeps the safety net without forcing replacement.♻️ Proposed change
import { Buffer } from 'buffer'; -globalThis.Buffer = Buffer; +if (!globalThis.Buffer) globalThis.Buffer = Buffer;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-browser/src/main.tsx` around lines 1 - 2, The current code unconditionally overwrites the global Buffer which can clobber an existing polyfill; change the assignment to a guarded one by checking globalThis.Buffer first and only set globalThis.Buffer = Buffer when it is undefined (use if (typeof globalThis.Buffer === 'undefined') or if (!globalThis.Buffer) to guard), referencing the existing Buffer import and the globalThis.Buffer symbol so you only provide the polyfill when no global is present.libs/react/src/router/ResourceRoute.tsx (1)
11-31:refetchis extracted but never used.The type assertion includes
refetch: () => voidbut the function is never called. Consider either removing it from the type or adding a refresh button to the UI for better UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/router/ResourceRoute.tsx` around lines 11 - 31, The typed return from useReadResource in ResourceRoute currently declares refetch but never uses it; either remove refetch from the asserted type or wire it into the UI—e.g., expose the refetch function from useReadResource and add a small refresh control in ResourceRoute (e.g., a button next to the title) that calls refetch when clicked, then pass state.data/state.loading/state.error as before to ResourceViewer; update the type assertion accordingly if you remove the refetch property.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/react/src/ai/useAITools.ts`:
- Around line 28-34: The current double-cast of toolInfos to McpToolInfo
bypasses type safety; before calling formatToolsForPlatform, iterate over the
array returned by useFrontMcp and ensure every ToolInfo has a required
inputSchema (or map missing ones to a safe default schema), e.g. replace the
mcpTools cast with a validation/mapping step that (a) throws or logs and aborts
if any tool is missing inputSchema when that should be an error, or (b)
transforms tools missing inputSchema by adding a default inputSchema shape, then
pass the validated/mapped result into formatToolsForPlatform; update references
to mcpTools, ToolInfo, McpToolInfo, inputSchema and formatToolsForPlatform
accordingly.
In `@libs/react/src/router/McpNavigation.tsx`:
- Around line 16-18: The component currently collapses all non-'connected'
FrontMcpStatus values into a single "Connecting..." message; update the
McpNavigation render logic to branch explicitly: if status === 'error' render a
nav showing the error details (use the error message from the same
context/provider that supplies status, e.g., connectionError or statusMessage),
else if status === 'idle' || status === 'connecting' render the existing
"Connecting..." UI, and keep the existing behavior for status === 'connected'.
Locate the status usage in the McpNavigation component and add the explicit
error branch so users see meaningful feedback when FrontMcpStatus === 'error'.
In `@libs/react/src/router/ResourceRoute.tsx`:
- Around line 19-24: Remove the inline type assertion on the `state` returned
from `useReadResource` in ResourceRoute.tsx and instead import and use the
exported `ResourceState` type from the hook's module (e.g., replace "const state
= useReadResource(uri) as { ... }" with "const state: ResourceState =
useReadResource(uri)"). Also drop the unused `refetch` property extraction (or
leave it on the state object but don't destructure it) and rely on
`ResourceState`'s declared shape so `data.contents` optionality is preserved and
TypeScript checks are enforced; reference the `useReadResource` hook and
`ResourceState` type to locate the change.
In `@plugins/plugin-store/src/adapters/valtio.adapter.ts`:
- Around line 85-112: The new public function createValtioStore must have a
docs/draft/docs entry: add a new documentation page that documents
createValtioStore (signature and generic T), its relationship to StoreAdapter
and the exposed methods getState, setState, subscribe, dispose, expected
parameter types (initialState, path/value), return type, behavior for root
replacement vs nested set, and a short usage example showing typical import and
basic operations; include frontmatter, a brief description, examples, and add
the page to the docs index/sidebar so it appears in the public API docs.
- Around line 44-52: setNestedValue currently silently ignores array path
segments like "[0]" when the traversal `current` node is not an array; change it
to fail fast like getNestedValue by checking the array match (in the block
around segment.match(/^\[(\d+)]$/)) and if `Array.isArray(current)` is false,
immediately fail (throw a descriptive Error or return a clear failure value)
instead of continuing; update the code paths in setNestedValue that use
`current[index]` (and the surrounding logic) to reflect this early-fail
behavior. Also add brief docs for the public createValtioStore API in
docs/draft/ per the plugin documentation requirements.
- Around line 95-102: The root-replace branch currently casts unknown `value` to
Record<string, unknown> without validation; add a type guard to ensure `value`
is a non-null object (e.g. typeof value === 'object' && value !== null) before
treating it as a record, and if invalid either throw a TypeError or return
early; when copying keys, iterate Object.keys(value) (not Object.keys(state))
and only copy keys that pass the existing isSafeKey check (use isSafeKey(key) to
filter) to avoid unsafe keys and ensure consistent behavior with nested path
handling of `path`, `state`, and `value`.
---
Outside diff comments:
In `@libs/auth/package.json`:
- Around line 35-55: The package exposes browser runtime JS from browser.ts but
currently points TypeScript types at dist/index.d.ts (built from index.ts),
causing a type/runtime mismatch; update the build so build-browser-esm generates
declaration files from browser.ts and update package.json browser entry to point
to ./dist/browser/browser.d.ts. Concretely: modify the build-browser-esm
task/config (the step that produces dist/browser/browser.mjs) to enable
declaration: true (and ensure type emission for that entry — e.g.,
skipTypeCheck: false or run a separate tsc invocation targeting browser.ts) so
./dist/browser/browser.d.ts is produced, and change the package.json
browser/type mapping to reference ./dist/browser/browser.d.ts instead of
./dist/index.d.ts; ensure browser.ts is the source used for declarations (not
index.ts) so browser-only symbols like loadDevKey/saveDevKey are omitted from
the browser types.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-browser/src/main.tsx`:
- Around line 1-2: The current code unconditionally overwrites the global Buffer
which can clobber an existing polyfill; change the assignment to a guarded one
by checking globalThis.Buffer first and only set globalThis.Buffer = Buffer when
it is undefined (use if (typeof globalThis.Buffer === 'undefined') or if
(!globalThis.Buffer) to guard), referencing the existing Buffer import and the
globalThis.Buffer symbol so you only provide the polyfill when no global is
present.
In `@libs/react/src/ai/useAITools.ts`:
- Around line 44-54: Wrap the body of callTool in a try/catch so tool execution
errors update the hook's error state instead of only propagating: call
server.callTool(name, args) inside try, on catch call setError(err) (the hook's
error state updater) and then rethrow if you want to preserve caller behavior;
this keeps the hook's error state consistent with formatting errors handled
elsewhere. Also remove the unsafe inline cast "as Parameters<typeof
formatResultForPlatform>[0]" — instead declare rawResult with an explicit type
of Parameters<typeof formatResultForPlatform>[0] (or narrow it safely) before
passing it to formatResultForPlatform to preserve type safety when calling
formatResultForPlatform(rawResult, platform).
In `@libs/react/src/router/McpNavigation.tsx`:
- Around line 31-37: The code spreads the mapped arrays into React.createElement
as rest args (e.g. ...tools.map(...)) which is unnecessary; change each usage
(tools.map, resources.map, prompts.map) to pass the mapped array directly as the
children argument to React.createElement instead of spreading, i.e., replace the
spread form with a single array child for the React.createElement call that
creates the 'li' list (where Link and basePath are used) and do the same pattern
for the other two sections.
In `@libs/react/src/router/ResourceRoute.tsx`:
- Around line 11-31: The typed return from useReadResource in ResourceRoute
currently declares refetch but never uses it; either remove refetch from the
asserted type or wire it into the UI—e.g., expose the refetch function from
useReadResource and add a small refresh control in ResourceRoute (e.g., a button
next to the title) that calls refetch when clicked, then pass
state.data/state.loading/state.error as before to ResourceViewer; update the
type assertion accordingly if you remove the refetch property.
…initialization, and add type safety in state management
# Conflicts: # yarn.lock
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/react/src/router/McpNavigation.tsx (1)
13-76: NormalizebasePathto avoid double-slash routes.If a caller passes
basePath="/mcp/", links become/mcp//tools/.... Normalizing once avoids subtle routing mismatches.♻️ Proposed fix
export function McpNavigation({ basePath = '/mcp' }: McpNavigationProps): React.ReactElement { + const normalizedBasePath = basePath.endsWith('/') ? basePath.slice(0, -1) : basePath; const { tools, resources, prompts, status, error } = useFrontMcp(); @@ - React.createElement(Link, { to: `${basePath}/tools/${encodeURIComponent(t.name)}` }, t.name), + React.createElement(Link, { to: `${normalizedBasePath}/tools/${encodeURIComponent(t.name)}` }, t.name), @@ - React.createElement(Link, { to: `${basePath}/resources/${encodeURIComponent(r.uri)}` }, r.name ?? r.uri), + React.createElement(Link, { to: `${normalizedBasePath}/resources/${encodeURIComponent(r.uri)}` }, r.name ?? r.uri), @@ - React.createElement(Link, { to: `${basePath}/prompts/${encodeURIComponent(p.name)}` }, p.name), + React.createElement(Link, { to: `${normalizedBasePath}/prompts/${encodeURIComponent(p.name)}` }, p.name),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/router/McpNavigation.tsx` around lines 13 - 76, The component McpNavigation should normalize the incoming basePath to remove any trailing slash so links don't get double-slashes; update the top of McpNavigation to derive a normalizedBase (trim trailing "/" characters, but preserve "/" if the result would be empty) and then replace every usage of basePath in the Link to= expressions (the template strings used when building tool/resource/prompt URLs) with normalizedBase so all generated routes are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/react/src/ai/useAITools.ts`:
- Around line 55-62: The returned value from server.callTool() is already the
expected CallToolResult, so remove the redundant cast in useAITools.ts: replace
the call that passes rawResult as Parameters<typeof formatResultForPlatform>[0]
to formatResultForPlatform with passing rawResult directly (i.e.,
formatResultForPlatform(rawResult, platform)), mirroring the pattern used in
DirectClient.callTool and keeping types strict; ensure no other code relies on
the cast.
---
Duplicate comments:
In `@plugins/plugin-store/src/adapters/valtio.adapter.ts`:
- Around line 52-54: The traversal currently silently returns when encountering
an array-segment like "[0]" on a non-array node; change both places that do
"return; // not an array" (the one inside the loop and the final assignment
branch around lines ~70-72) to throw a TypeError instead, mirroring the
root-replacement behavior used around lines 101-103; update the function that
walks path segments (the path traversal/set routine in valtio.adapter.ts) to
include a clear TypeError message indicating the expected array at the given
path segment so callers can detect failed operations.
- Around line 89-124: Add a documentation entry for the new public API
createValtioStore (in the plugin-store adapter) under the project's docs draft
for plugins: describe the function signature and generic parameter T, explain
the initialState parameter and returned StoreAdapter<T>, document behavior for
setState including root replacement semantics and the TypeError when replacing
root with non-object, show getState path behavior, subscription/dispose
behavior, and include a short usage example and migration/visibility note so
users can discover and use the adapter.
---
Nitpick comments:
In `@libs/react/src/router/McpNavigation.tsx`:
- Around line 13-76: The component McpNavigation should normalize the incoming
basePath to remove any trailing slash so links don't get double-slashes; update
the top of McpNavigation to derive a normalizedBase (trim trailing "/"
characters, but preserve "/" if the result would be empty) and then replace
every usage of basePath in the Link to= expressions (the template strings used
when building tool/resource/prompt URLs) with normalizedBase so all generated
routes are consistent.
# Conflicts: # .gitignore
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
68-79:⚠️ Potential issue | 🟡 MinorAdd Playwright browser installation to CI workflows.
The repository uses Playwright for e2e tests (configured in
apps/e2e/demo-e2e-browser/playwright.config.ts), but CI workflows lack an explicit browser install step. Whileyarn install --frozen-lockfileinstalls the@playwright/testpackage, it does not install the required browsers. Addnpx playwright installafter dependency installation in workflows that run e2e tests (perf.yml and similar).Note: The
@vitejs/plugin-react-swcSWC plugin is already properly wired into the Vite config atapps/e2e/demo-e2e-browser/vite.config.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 68 - 79, CI workflows that run the Playwright E2E suite are missing browser binaries: after installing dependencies (where `@playwright/test` is added in package.json) add an explicit browser installation step (run `npx playwright install`) in the workflows that execute the tests (e.g., perf.yml and any other workflow that triggers apps/e2e/demo-e2e-browser/playwright.config.ts) immediately after the dependency install step so Playwright has the required browsers available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/plugin-approval/package.json`:
- Around line 38-42: The package.json change added public browser exports (the
"browser" conditional with types "./dist/index.d.ts", import
"./dist/browser/browser.mjs" and default "./dist/browser/browser.js") which
require corresponding documentation; update docs/draft/docs to document the new
"." browser condition and the "./browser" entry-point for the plugin-approval
plugin, adding usage examples, export surface, and any import paths/notes
required by the plugin SDK guidelines so consumers know how to use the browser
bundle.
---
Outside diff comments:
In `@package.json`:
- Around line 68-79: CI workflows that run the Playwright E2E suite are missing
browser binaries: after installing dependencies (where `@playwright/test` is added
in package.json) add an explicit browser installation step (run `npx playwright
install`) in the workflows that execute the tests (e.g., perf.yml and any other
workflow that triggers apps/e2e/demo-e2e-browser/playwright.config.ts)
immediately after the dependency install step so Playwright has the required
browsers available.
| "browser": { | ||
| "types": "./dist/index.d.ts", | ||
| "import": "./dist/browser/browser.mjs", | ||
| "default": "./dist/browser/browser.js" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for draft docs mentioning plugin-approval browser entrypoint
rg -n "plugin-approval|/browser|@frontmcp/plugin-approval" docs/draft/docs/ 2>/dev/null || echo "No matches found"Repository: agentfront/frontmcp
Length of output: 79
🏁 Script executed:
#!/bin/bash
# Also check if docs/draft/docs directory exists and what structure it has
ls -la docs/draft/docs/ 2>/dev/null | head -20 || echo "docs/draft/docs does not exist or is empty"Repository: agentfront/frontmcp
Length of output: 45
🏁 Script executed:
#!/bin/bash
# View the actual package.json to understand the export structure
cat -n plugins/plugin-approval/package.json | sed -n '30,60p'Repository: agentfront/frontmcp
Length of output: 1106
Add browser export documentation to docs/draft/docs.
This change adds public exports ("." browser condition and "./browser" entry point), which requires a matching docs/draft/docs update per the plugin SDK guidelines. No documentation currently exists for the browser entrypoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/plugin-approval/package.json` around lines 38 - 42, The package.json
change added public browser exports (the "browser" conditional with types
"./dist/index.d.ts", import "./dist/browser/browser.mjs" and default
"./dist/browser/browser.js") which require corresponding documentation; update
docs/draft/docs to document the new "." browser condition and the "./browser"
entry-point for the plugin-approval plugin, adding usage examples, export
surface, and any import paths/notes required by the plugin SDK guidelines so
consumers know how to use the browser bundle.
| } | ||
|
|
||
| export function McpNavigation({ basePath = '/mcp' }: McpNavigationProps): React.ReactElement { | ||
| const normalizedBase = basePath.replace(/\/+$/, '') || '/'; |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/plugins/package.json (1)
36-53:⚠️ Potential issue | 🟠 MajorAdd types mapping for the new
./browserexport.
TypeScript module resolution requires explicittypesentries for conditional exports. Thebrowsercondition under the"."export and the"./browser"subpath export are both missingtypesmappings, preventing TypeScript consumers from resolving declarations when importing@frontmcp/plugins/browser. This pattern is consistent with other core libraries in the repo (e.g.,libs/sdk,plugins/plugin-store) which include"types": "./dist/index.d.ts"in their browser conditions.✍️ Suggested fix
".": { "browser": { + "types": "./dist/index.d.ts", "import": "./dist/browser/browser.mjs", "default": "./dist/browser/browser.js" }, @@ "./browser": { + "types": "./dist/index.d.ts", "import": "./dist/browser/browser.mjs", "default": "./dist/browser/browser.js" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/plugins/package.json` around lines 36 - 53, The "." export's "browser" condition and the "./browser" subpath export are missing a "types" mapping, so add a "types": "./dist/index.d.ts" entry under both the "browser" condition in the main export object and under the "./browser" export object to let TypeScript resolve declarations when consumers import the browser build (update the "browser" condition keys inside the exports object and the "./browser" export to include the "types" field).libs/plugins/src/index.ts (1)
1-18:⚠️ Potential issue | 🟡 MinorAdd
@frontmcp/plugin-approvalto the header plugin list and docs overview.The header comment omits
@frontmcp/plugin-approvaleven though it's exported at line 18. Additionally, this plugin should be documented indocs/frontmcp/plugins/overview.mdxalongside the other official plugins (currently only Remember, CodeCall, and Cache are listed).✍️ Suggested header comment fix
* - `@frontmcp/plugin-cache` * - `@frontmcp/plugin-codecall` + * - `@frontmcp/plugin-approval` * - `@frontmcp/plugin-dashboard` * - `@frontmcp/plugin-remember`The Approval Plugin documentation exists at
docs/frontmcp/plugins/approval-plugin.mdx, but should also be added as a Card to the official plugins section indocs/frontmcp/plugins/overview.mdx(currently lines 32–76).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/plugins/src/index.ts` around lines 1 - 18, The package header comment in libs/plugins/src/index.ts is missing mention of `@frontmcp/plugin-approval` even though the file exports it via export * from '@frontmcp/plugin-approval'; update the top comment to include `@frontmcp/plugin-approval` in the bullet list of official plugins and migration notes, and then add a Card entry linking to docs/frontmcp/plugins/approval-plugin.mdx into the official plugins section of docs/frontmcp/plugins/overview.mdx (the block between lines ~32–76) so the Approval Plugin appears alongside Remember, CodeCall, and Cache.
🧹 Nitpick comments (3)
libs/react/src/router/McpNavigation.tsx (2)
14-14: Replace regex with string methods to avoid potential ReDoS and improve clarity.CodeQL flagged the regex
/\/+$/as potentially vulnerable on user-controlled input. While this specific pattern is unlikely to cause catastrophic backtracking (it's linear, not polynomial), using string methods is both safer and more readable.♻️ Proposed fix using string methods
- const normalizedBase = basePath.replace(/\/+$/, '') || '/'; + let normalizedBase = basePath; + while (normalizedBase.endsWith('/')) { + normalizedBase = normalizedBase.slice(0, -1); + } + normalizedBase = normalizedBase || '/';Or more concisely with
trimEndif targeting modern browsers:const normalizedBase = basePath.replace(/\/+$/, '') || '/'; // Alternative: const normalizedBase = basePath.split('/').filter(Boolean).join('/') || '/';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/router/McpNavigation.tsx` at line 14, Replace the regex-based trimming for normalizedBase (const normalizedBase = basePath.replace(/\/+$/, '') || '/';) with a string-method approach to avoid regex on user input; update the McpNavigation logic to use either basePath.trimEnd().replace(...) or better: split('/') and filter(Boolean).join('/') (or String.prototype.trimEnd if targeting modern runtimes) to remove trailing slashes and fall back to '/' when empty, while keeping the normalizedBase symbol intact.
28-86: Consider using JSX syntax for improved readability.The entire component uses
React.createElementcalls instead of JSX. While functionally correct, JSX would significantly improve readability and maintainability of this navigation structure.♻️ Optional JSX refactor for main render block
return ( <nav> {tools.length > 0 && ( <div> <h4>Tools</h4> <ul> {tools.map((t) => ( <li key={t.name}> <Link to={`${normalizedBase}/tools/${encodeURIComponent(t.name)}`}>{t.name}</Link> </li> ))} </ul> </div> )} {resources.length > 0 && ( <div> <h4>Resources</h4> <ul> {resources.map((r) => ( <li key={r.uri}> <Link to={`${normalizedBase}/resources/${encodeURIComponent(r.uri)}`}> {r.name ?? r.uri} </Link> </li> ))} </ul> </div> )} {prompts.length > 0 && ( <div> <h4>Prompts</h4> <ul> {prompts.map((p) => ( <li key={p.name}> <Link to={`${normalizedBase}/prompts/${encodeURIComponent(p.name)}`}>{p.name}</Link> </li> ))} </ul> </div> )} </nav> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/router/McpNavigation.tsx` around lines 28 - 86, The render function in McpNavigation currently uses nested React.createElement calls which are hard to read; convert the main return block to JSX syntax: replace the React.createElement('nav', ...) tree with a JSX <nav> containing conditional blocks for tools, resources, and prompts that map to <ul>/<li> entries, preserving keys (t.name, r.uri, p.name), Link usage, and encodeURIComponent calls on normalizedBase paths; ensure imports/types support JSX and the component still returns the same structure and props.plugins/plugin-remember/project.json (1)
68-68: Consider: Browser externals differ from Node builds.The browser build only externalizes
zodandreflect-metadata, while CJS/ESM builds also externalizeioredis,@frontmcp/sdk,@frontmcp/utils, and@vercel/kv. This is intentional since these are aliased to shims (lines 104, 109), but ensure consumers understand the browser bundle is self-contained with shimmed Node dependencies.As per coding guidelines for
plugins/**: if this adds a new browser entry point to the published package, ensure the package.json exports and documentation are updated accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/plugin-remember/project.json` at line 68, The browser build currently only externalizes "zod" and "reflect-metadata" (the "external" array) while the CJS/ESM builds externalize additional Node deps that are shimmed (see alias/shim usage around the alias lines), so update the project.json/browser build config to clearly document that the browser bundle includes shims for ioredis, `@frontmcp/sdk`, `@frontmcp/utils` and `@vercel/kv` (or add them to the browser externals if you prefer not to bundle shims); also, if this introduces a new browser entry point for the package, update the package exports and README/docs to advertise the browser entry and note the shim behavior so consumers know the bundle is self-contained for browser usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/frontmcp/plugins/official/store.mdx`:
- Around line 107-114: The Card components inside CardGroup (CardGroup and Card)
use incorrect href props with an extra "docs" segment; update the href values
for the Cache and Remember Card instances (the Card with title "Cache Plugin"
and the Card with title "Remember Plugin") to remove the stray "docs" segment so
they read "/frontmcp/plugins/official/cache" and
"/frontmcp/plugins/official/remember" respectively, ensuring the paths match the
Mintlify navigation and docs/docs.json entries.
In `@libs/react/src/provider/FrontMcpProvider.tsx`:
- Around line 17-33: Add documentation for the new FrontMcpProvider and its
props by creating a guide under docs/frontmcp (either add a section to
docs/frontmcp/guides/ui-library.mdx or create
docs/frontmcp/guides/react-provider.mdx) that explains the purpose of
FrontMcpProvider, documents the FrontMcpProviderProps fields (server,
autoConnect, components, onConnected, onError) with short descriptions and
types, and includes a minimal usage example showing how to wrap the app with
<FrontMcpProvider> and how to pass server, components, and event handlers;
reference the exported symbols FrontMcpProvider and FrontMcpProviderProps so
consumers can find the implementation in libs/react and ensure the doc is linked
from the frontmcp docs index.
- Around line 61-69: The provider currently assigns clientRef.current
immediately after server.connect(), which can leak if the component unmounts
before the list calls finish; change the flow in the async init so you perform
server.connect(), run the mounted check, then assign clientRef.current = client
only when still mounted, and in the component cleanup/unmount handler call
clientRef.current?.close() (or the client's disconnect method) and clear
clientRef.current to ensure the connection is closed; ensure the same cleanup
also handles the case where the async list calls are still pending and may later
resolve.
In `@libs/sdk/src/browser/shims/empty.ts`:
- Around line 8-49: The stub signatures are too strict—update noop and the
Buffer static methods to accept variadic unknown args so callers with parameters
compile under strict mode: change noop to accept (..._args: unknown[]) and make
Router/json/urlencoded return functions that accept (..._args: unknown[]);
change Buffer.from, Buffer.alloc to accept (..._args: unknown[]) and
Buffer.isBuffer to accept (..._args: unknown[]) while preserving their return
types and existing behavior; also update getRequestListener/any other exported
stubs that may be called with args to accept variadic unknowns (e.g.,
getRequestListener returns a function that accepts (..._args: unknown[])).
---
Outside diff comments:
In `@libs/plugins/package.json`:
- Around line 36-53: The "." export's "browser" condition and the "./browser"
subpath export are missing a "types" mapping, so add a "types":
"./dist/index.d.ts" entry under both the "browser" condition in the main export
object and under the "./browser" export object to let TypeScript resolve
declarations when consumers import the browser build (update the "browser"
condition keys inside the exports object and the "./browser" export to include
the "types" field).
In `@libs/plugins/src/index.ts`:
- Around line 1-18: The package header comment in libs/plugins/src/index.ts is
missing mention of `@frontmcp/plugin-approval` even though the file exports it via
export * from '@frontmcp/plugin-approval'; update the top comment to include
`@frontmcp/plugin-approval` in the bullet list of official plugins and migration
notes, and then add a Card entry linking to
docs/frontmcp/plugins/approval-plugin.mdx into the official plugins section of
docs/frontmcp/plugins/overview.mdx (the block between lines ~32–76) so the
Approval Plugin appears alongside Remember, CodeCall, and Cache.
---
Duplicate comments:
In `@libs/react/src/provider/FrontMcpProvider.tsx`:
- Around line 91-102: The effect currently sets mountedRef and conditionally
calls connectClient but has an empty dependency array, so toggling autoConnect
or a changed connectClient won't re-run; update the useEffect that references
mountedRef, autoConnect, connectClient, and clientRef to either include
autoConnect and connectClient in the dependency array so the effect re-runs when
they change, or explicitly document the intentional one-shot behavior with a
clear comment above the useEffect (and keep the empty deps) so future readers
know this is deliberate.
- Around line 64-73: toolsResult from client.listTools() may be a record/object
shape rather than an array; before calling setTools normalize toolsResult into
an array (e.g., if toolsResult is an object, use Object.values(...) or map its
entries) and then pass that array to setTools. Update the block around
toolsResult, setTools and the Promise.all call in FrontMcpProvider (referencing
client.listTools(), toolsResult, and setTools) to coerce/validate the shape
(handle null/undefined too) so setTools always receives a ToolInfo[].
---
Nitpick comments:
In `@libs/react/src/router/McpNavigation.tsx`:
- Line 14: Replace the regex-based trimming for normalizedBase (const
normalizedBase = basePath.replace(/\/+$/, '') || '/';) with a string-method
approach to avoid regex on user input; update the McpNavigation logic to use
either basePath.trimEnd().replace(...) or better: split('/') and
filter(Boolean).join('/') (or String.prototype.trimEnd if targeting modern
runtimes) to remove trailing slashes and fall back to '/' when empty, while
keeping the normalizedBase symbol intact.
- Around line 28-86: The render function in McpNavigation currently uses nested
React.createElement calls which are hard to read; convert the main return block
to JSX syntax: replace the React.createElement('nav', ...) tree with a JSX <nav>
containing conditional blocks for tools, resources, and prompts that map to
<ul>/<li> entries, preserving keys (t.name, r.uri, p.name), Link usage, and
encodeURIComponent calls on normalizedBase paths; ensure imports/types support
JSX and the component still returns the same structure and props.
In `@plugins/plugin-remember/project.json`:
- Line 68: The browser build currently only externalizes "zod" and
"reflect-metadata" (the "external" array) while the CJS/ESM builds externalize
additional Node deps that are shimmed (see alias/shim usage around the alias
lines), so update the project.json/browser build config to clearly document that
the browser bundle includes shims for ioredis, `@frontmcp/sdk`, `@frontmcp/utils`
and `@vercel/kv` (or add them to the browser externals if you prefer not to bundle
shims); also, if this introduces a new browser entry point for the package,
update the package exports and README/docs to advertise the browser entry and
note the shim behavior so consumers know the bundle is self-contained for
browser usage.
| <CardGroup cols={2}> | ||
| <Card title="Cache Plugin" icon="bolt" href="/frontmcp/plugins/docs/official/cache"> | ||
| Response caching | ||
| </Card> | ||
| <Card title="Remember Plugin" icon="brain" href="/frontmcp/plugins/docs/official/remember"> | ||
| Persistent memory | ||
| </Card> | ||
| </CardGroup> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find cache and remember plugin documentation files
fd -t f -e mdx "cache|remember" docs/Repository: agentfront/frontmcp
Length of output: 276
🏁 Script executed:
# Check the docs.json navigation files to understand routing
fd -t f "docs.json" docs/ --exec echo "=== {} ===" \; --exec cat {} \;Repository: agentfront/frontmcp
Length of output: 11632
🏁 Script executed:
# Check the actual directory structure of docs/frontmcp/plugins/
tree -L 4 docs/frontmcp/plugins/ 2>/dev/null || find docs/frontmcp/plugins/ -type f -name "*.mdx" | head -20Repository: agentfront/frontmcp
Length of output: 545
Fix the Card href paths to remove the misplaced "docs" segment.
The href paths contain an extra "docs" segment that doesn't match the Mintlify navigation configuration. The files exist at the correct locations, and docs/docs.json references them as frontmcp/plugins/official/cache and frontmcp/plugins/official/remember.
Correct paths
- <Card title="Cache Plugin" icon="bolt" href="/frontmcp/plugins/docs/official/cache">
+ <Card title="Cache Plugin" icon="bolt" href="/frontmcp/plugins/official/cache">
Response caching
</Card>
- <Card title="Remember Plugin" icon="brain" href="/frontmcp/plugins/docs/official/remember">
+ <Card title="Remember Plugin" icon="brain" href="/frontmcp/plugins/official/remember">
Persistent memory
</Card>📝 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.
| <CardGroup cols={2}> | |
| <Card title="Cache Plugin" icon="bolt" href="/frontmcp/plugins/docs/official/cache"> | |
| Response caching | |
| </Card> | |
| <Card title="Remember Plugin" icon="brain" href="/frontmcp/plugins/docs/official/remember"> | |
| Persistent memory | |
| </Card> | |
| </CardGroup> | |
| <CardGroup cols={2}> | |
| <Card title="Cache Plugin" icon="bolt" href="/frontmcp/plugins/official/cache"> | |
| Response caching | |
| </Card> | |
| <Card title="Remember Plugin" icon="brain" href="/frontmcp/plugins/official/remember"> | |
| Persistent memory | |
| </Card> | |
| </CardGroup> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/frontmcp/plugins/official/store.mdx` around lines 107 - 114, The Card
components inside CardGroup (CardGroup and Card) use incorrect href props with
an extra "docs" segment; update the href values for the Cache and Remember Card
instances (the Card with title "Cache Plugin" and the Card with title "Remember
Plugin") to remove the stray "docs" segment so they read
"/frontmcp/plugins/official/cache" and "/frontmcp/plugins/official/remember"
respectively, ensuring the paths match the Mintlify navigation and
docs/docs.json entries.
| export interface FrontMcpProviderProps { | ||
| server: DirectMcpServer; | ||
| components?: Record<string, ComponentType<Record<string, unknown>>>; | ||
| autoConnect?: boolean; | ||
| children: React.ReactNode; | ||
| onConnected?: (client: DirectClient) => void; | ||
| onError?: (error: Error) => void; | ||
| } | ||
|
|
||
| export function FrontMcpProvider({ | ||
| server, | ||
| components, | ||
| autoConnect = true, | ||
| children, | ||
| onConnected, | ||
| onError, | ||
| }: FrontMcpProviderProps): React.ReactElement { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for FrontMcpProvider in draft docs
rg -n "FrontMcpProvider" docs/draft/docs -g "*" || echo "No matches found"Repository: agentfront/frontmcp
Length of output: 152
🏁 Script executed:
# Check the docs directory structure to understand available docs
find docs -type f -name "*.md*" | head -20Repository: agentfront/frontmcp
Length of output: 901
🏁 Script executed:
# Check if there are any other provider-related docs
rg -l "Provider" docs/draft/docs docs/docs 2>/dev/null | head -10Repository: agentfront/frontmcp
Length of output: 45
🏁 Script executed:
# Verify the file exists and check context around FrontMcpProvider export
head -50 libs/react/src/provider/FrontMcpProvider.tsxRepository: agentfront/frontmcp
Length of output: 1949
🏁 Script executed:
# Search for FrontMcpProvider exports across the codebase
rg "export.*FrontMcpProvider" libs/react/src -A 2 -B 2Repository: agentfront/frontmcp
Length of output: 1336
🏁 Script executed:
# Check main entry point for libs/react
cat libs/react/package.json | grep -A 10 "exports\|main"Repository: agentfront/frontmcp
Length of output: 501
🏁 Script executed:
# Look for any provider-related documentation in the correct docs location
rg -i "provider\|FrontMcpProvider" docs/frontmcp --max-count=5Repository: agentfront/frontmcp
Length of output: 45
🏁 Script executed:
# Check if there's a UI/React guide in docs
find docs/frontmcp -name "*react*" -o -name "*ui*" -o -name "*provider*"Repository: agentfront/frontmcp
Length of output: 695
🏁 Script executed:
# Check git history to see if this is a new file or existing
git log --oneline -n 20 libs/react/src/provider/FrontMcpProvider.tsx 2>/dev/null | head -5 || echo "Unable to check git history"Repository: agentfront/frontmcp
Length of output: 148
🏁 Script executed:
# Check React template docs
cat docs/frontmcp/ui/templates/react.mdxRepository: agentfront/frontmcp
Length of output: 8999
🏁 Script executed:
# Check UI library guide
cat docs/frontmcp/guides/ui-library.mdxRepository: agentfront/frontmcp
Length of output: 6219
🏁 Script executed:
# Double-check for any mentions of FrontMcpProvider in any docs
rg "FrontMcp" docs/frontmcp --max-count=10Repository: agentfront/frontmcp
Length of output: 25335
Add documentation for the new FrontMcpProvider React context.
FrontMcpProvider and FrontMcpProviderProps are new public exports from libs/react but have no documentation in docs/frontmcp/**. Add a guide or section to document this provider's purpose, props (server, autoConnect, components, onConnected, onError), and usage examples. Consider placement in docs/frontmcp/guides/ui-library.mdx or a dedicated React provider guide.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/react/src/provider/FrontMcpProvider.tsx` around lines 17 - 33, Add
documentation for the new FrontMcpProvider and its props by creating a guide
under docs/frontmcp (either add a section to docs/frontmcp/guides/ui-library.mdx
or create docs/frontmcp/guides/react-provider.mdx) that explains the purpose of
FrontMcpProvider, documents the FrontMcpProviderProps fields (server,
autoConnect, components, onConnected, onError) with short descriptions and
types, and includes a minimal usage example showing how to wrap the app with
<FrontMcpProvider> and how to pass server, components, and event handlers;
reference the exported symbols FrontMcpProvider and FrontMcpProviderProps so
consumers can find the implementation in libs/react and ensure the doc is linked
from the frontmcp docs index.
| const client = await server.connect(); | ||
| clientRef.current = client; | ||
|
|
||
| const [toolsResult, resourcesResult, templatesResult, promptsResult] = await Promise.all([ | ||
| client.listTools(), | ||
| client.listResources(), | ||
| client.listResourceTemplates(), | ||
| client.listPrompts(), | ||
| ]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full FrontMcpProvider component to understand the mount/unmount pattern
cat -n libs/react/src/provider/FrontMcpProvider.tsxRepository: agentfront/frontmcp
Length of output: 4668
🏁 Script executed:
# Search for DirectClient definition and its interface
rg -n "type DirectClient|interface DirectClient|class DirectClient" libs/Repository: agentfront/frontmcp
Length of output: 230
🏁 Script executed:
# Look for close/disconnect/cleanup methods on client objects
rg -n "client\.close|client\.disconnect|\.disconnect\(|\.close\(" libs/ -A 2Repository: agentfront/frontmcp
Length of output: 45996
🏁 Script executed:
head -20 libs/sdk/src/direct/client.types.ts | tail -20Repository: agentfront/frontmcp
Length of output: 531
🏁 Script executed:
# Check the close method signature in DirectClient interface
sed -n '353,400p' libs/sdk/src/direct/client.types.tsRepository: agentfront/frontmcp
Length of output: 1710
🏁 Script executed:
# Get more of the DirectClient interface to find the close method
sed -n '353,450p' libs/sdk/src/direct/client.types.tsRepository: agentfront/frontmcp
Length of output: 3227
Close the client connection when the provider unmounts to prevent resource leaks.
If the provider unmounts after server.connect() completes but before all list API calls finish, the client connection is never closed. Move the clientRef.current = client assignment after the mounted check, and ensure proper cleanup in the unmount handler.
🔧 Suggested fix
const client = await server.connect();
+ if (!mountedRef.current) {
+ await client.close();
+ return;
+ }
clientRef.current = client;And in the cleanup function:
return () => {
mountedRef.current = false;
+ const client = clientRef.current;
clientRef.current = null;
+ if (client) {
+ void client.close().catch(() => {
+ // Cleanup on unmount; silently ignore errors
+ });
+ }
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/react/src/provider/FrontMcpProvider.tsx` around lines 61 - 69, The
provider currently assigns clientRef.current immediately after server.connect(),
which can leak if the component unmounts before the list calls finish; change
the flow in the async init so you perform server.connect(), run the mounted
check, then assign clientRef.current = client only when still mounted, and in
the component cleanup/unmount handler call clientRef.current?.close() (or the
client's disconnect method) and clear clientRef.current to ensure the connection
is closed; ensure the same cleanup also handles the case where the async list
calls are still pending and may later resolve.
| // Default export (for modules using `import X from '...'`) | ||
| // Must be a callable function since some modules call the default export as a constructor/factory | ||
| function noop() { | ||
| return noop; | ||
| } | ||
| Object.assign(noop, { | ||
| Router: () => noop, | ||
| json: () => noop, | ||
| urlencoded: () => noop, | ||
| use: noop, | ||
| get: noop, | ||
| post: noop, | ||
| all: noop, | ||
| listen: noop, | ||
| }); | ||
| export default noop; | ||
|
|
||
| // Common named exports that Node.js modules provide | ||
| export const URL = globalThis.URL; | ||
| export const URLSearchParams = globalThis.URLSearchParams; | ||
| export class Buffer { | ||
| static from(): Buffer { | ||
| return new Buffer(); | ||
| } | ||
| static alloc(): Buffer { | ||
| return new Buffer(); | ||
| } | ||
| static isBuffer(): boolean { | ||
| return false; | ||
| } | ||
| toString(): string { | ||
| return ''; | ||
| } | ||
| } | ||
|
|
||
| // HTTP/2 stubs | ||
| export class Http2ServerRequest {} | ||
| export class Http2ServerResponse {} | ||
|
|
||
| // @hono/node-server stubs | ||
| export function getRequestListener() { | ||
| return noop; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, verify the file exists and check its content
ls -la libs/sdk/src/browser/shims/empty.ts 2>/dev/null && echo "--- File exists ---" || echo "File not found"
wc -l libs/sdk/src/browser/shims/empty.tsRepository: agentfront/frontmcp
Length of output: 206
🏁 Script executed:
#!/bin/bash
# Read the specific lines mentioned in the review (8-49)
sed -n '8,49p' libs/sdk/src/browser/shims/empty.tsRepository: agentfront/frontmcp
Length of output: 988
🏁 Script executed:
#!/bin/bash
# Search for call sites that pass arguments to Buffer methods
rg -n 'Buffer\.(from|alloc|isBuffer)\s*\(' --type=ts -B2 -A2Repository: agentfront/frontmcp
Length of output: 48586
🏁 Script executed:
#!/bin/bash
# Search for express Router and handler invocations
rg -n '\.(Router|get|post|all|use|listen)\s*\(' --type=ts -B1 -A1 | head -50Repository: agentfront/frontmcp
Length of output: 3360
🏁 Script executed:
#!/bin/bash
# Search for getRequestListener call sites
rg -n 'getRequestListener\s*\(' --type=ts -B2 -A2Repository: agentfront/frontmcp
Length of output: 341
🏁 Script executed:
#!/bin/bash
# Check if TypeScript strict mode is enabled
if [ -f tsconfig.json ]; then
echo "=== tsconfig.json strict setting ==="
jq '.compilerOptions.strict' tsconfig.json 2>/dev/null || grep -A5 '"strict"' tsconfig.json
fi
if [ -f libs/sdk/tsconfig.json ]; then
echo "=== libs/sdk/tsconfig.json strict setting ==="
jq '.compilerOptions.strict' libs/sdk/tsconfig.json 2>/dev/null || grep -A5 '"strict"' libs/sdk/tsconfig.json
fiRepository: agentfront/frontmcp
Length of output: 113
Broaden stub signatures to prevent TypeScript strict-mode errors at call sites.
The noop and Buffer method stubs currently accept no arguments, but real-world code invokes them with parameters—e.g., Buffer.from(data), Buffer.alloc(size), Buffer.isBuffer(value). With strict: true enabled in libs/sdk/tsconfig.json, these mismatches surface as compile errors. Use variadic (..._args: unknown[]) signatures to keep stubs permissive without relaxing type safety elsewhere.
✅ Proposed fix
// Default export (for modules using `import X from '...'`)
// Must be a callable function since some modules call the default export as a constructor/factory
-function noop() {
- return noop;
-}
-Object.assign(noop, {
- Router: () => noop,
- json: () => noop,
- urlencoded: () => noop,
- use: noop,
- get: noop,
- post: noop,
- all: noop,
- listen: noop,
-});
+type Noop = (..._args: unknown[]) => Noop;
+const noop: Noop = (..._args) => noop;
+Object.assign(noop, {
+ Router: (..._args: unknown[]) => noop,
+ json: (..._args: unknown[]) => noop,
+ urlencoded: (..._args: unknown[]) => noop,
+ use: (..._args: unknown[]) => noop,
+ get: (..._args: unknown[]) => noop,
+ post: (..._args: unknown[]) => noop,
+ all: (..._args: unknown[]) => noop,
+ listen: (..._args: unknown[]) => noop,
+});
export default noop;
// Common named exports that Node.js modules provide
export const URL = globalThis.URL;
export const URLSearchParams = globalThis.URLSearchParams;
export class Buffer {
- static from(): Buffer {
+ static from(..._args: unknown[]): Buffer {
return new Buffer();
}
- static alloc(): Buffer {
+ static alloc(..._args: unknown[]): Buffer {
return new Buffer();
}
- static isBuffer(): boolean {
+ static isBuffer(..._args: unknown[]): boolean {
return false;
}
toString(): string {
return '';
}
}
// HTTP/2 stubs
export class Http2ServerRequest {}
export class Http2ServerResponse {}
// `@hono/node-server` stubs
-export function getRequestListener() {
+export function getRequestListener(..._args: unknown[]): Noop {
return noop;
}📝 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.
| // Default export (for modules using `import X from '...'`) | |
| // Must be a callable function since some modules call the default export as a constructor/factory | |
| function noop() { | |
| return noop; | |
| } | |
| Object.assign(noop, { | |
| Router: () => noop, | |
| json: () => noop, | |
| urlencoded: () => noop, | |
| use: noop, | |
| get: noop, | |
| post: noop, | |
| all: noop, | |
| listen: noop, | |
| }); | |
| export default noop; | |
| // Common named exports that Node.js modules provide | |
| export const URL = globalThis.URL; | |
| export const URLSearchParams = globalThis.URLSearchParams; | |
| export class Buffer { | |
| static from(): Buffer { | |
| return new Buffer(); | |
| } | |
| static alloc(): Buffer { | |
| return new Buffer(); | |
| } | |
| static isBuffer(): boolean { | |
| return false; | |
| } | |
| toString(): string { | |
| return ''; | |
| } | |
| } | |
| // HTTP/2 stubs | |
| export class Http2ServerRequest {} | |
| export class Http2ServerResponse {} | |
| // @hono/node-server stubs | |
| export function getRequestListener() { | |
| return noop; | |
| // Default export (for modules using `import X from '...'`) | |
| // Must be a callable function since some modules call the default export as a constructor/factory | |
| type Noop = (..._args: unknown[]) => Noop; | |
| const noop: Noop = (..._args) => noop; | |
| Object.assign(noop, { | |
| Router: (..._args: unknown[]) => noop, | |
| json: (..._args: unknown[]) => noop, | |
| urlencoded: (..._args: unknown[]) => noop, | |
| use: (..._args: unknown[]) => noop, | |
| get: (..._args: unknown[]) => noop, | |
| post: (..._args: unknown[]) => noop, | |
| all: (..._args: unknown[]) => noop, | |
| listen: (..._args: unknown[]) => noop, | |
| }); | |
| export default noop; | |
| // Common named exports that Node.js modules provide | |
| export const URL = globalThis.URL; | |
| export const URLSearchParams = globalThis.URLSearchParams; | |
| export class Buffer { | |
| static from(..._args: unknown[]): Buffer { | |
| return new Buffer(); | |
| } | |
| static alloc(..._args: unknown[]): Buffer { | |
| return new Buffer(); | |
| } | |
| static isBuffer(..._args: unknown[]): boolean { | |
| return false; | |
| } | |
| toString(): string { | |
| return ''; | |
| } | |
| } | |
| // HTTP/2 stubs | |
| export class Http2ServerRequest {} | |
| export class Http2ServerResponse {} | |
| // `@hono/node-server` stubs | |
| export function getRequestListener(..._args: unknown[]): Noop { | |
| return noop; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/sdk/src/browser/shims/empty.ts` around lines 8 - 49, The stub signatures
are too strict—update noop and the Buffer static methods to accept variadic
unknown args so callers with parameters compile under strict mode: change noop
to accept (..._args: unknown[]) and make Router/json/urlencoded return functions
that accept (..._args: unknown[]); change Buffer.from, Buffer.alloc to accept
(..._args: unknown[]) and Buffer.isBuffer to accept (..._args: unknown[]) while
preserving their return types and existing behavior; also update
getRequestListener/any other exported stubs that may be called with args to
accept variadic unknowns (e.g., getRequestListener returns a function that
accepts (..._args: unknown[])).
# Conflicts: # libs/plugins/package.json # tsconfig.base.json
1 participant
Summary by CodeRabbit
New Features
Tests