chore: upgrade GitHub Actions dependencies and adjust Node.js setup configuration#276
Merged
frontegg-david merged 9 commits intomainfrom Mar 15, 2026
Merged
chore: upgrade GitHub Actions dependencies and adjust Node.js setup configuration#276frontegg-david merged 9 commits intomainfrom
frontegg-david merged 9 commits intomainfrom
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughCI workflows upgraded; a new React E2E demo with Playwright tests added; Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WrappedServer as WrappedServer\n(rgba(120,160,200,0.5))
participant DynamicRegistry as DynamicRegistry\n(rgba(200,160,120,0.5))
participant BaseServer as BaseServer\n(rgba(160,200,120,0.5))
Client->>WrappedServer: listTools(opts)
WrappedServer->>BaseServer: listTools(opts)
WrappedServer->>DynamicRegistry: getTools()
WrappedServer-->>Client: merged tools (dynamic take precedence)
Client->>WrappedServer: callTool(name,args)
WrappedServer->>DynamicRegistry: findTool(name)
alt dynamic tool found
DynamicRegistry-->>WrappedServer: execute(...)
WrappedServer-->>Client: dynamic result
else not found
WrappedServer->>BaseServer: callTool(name,args)
BaseServer-->>WrappedServer: base result
WrappedServer-->>Client: base result
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/package.json`:
- Around line 74-75: The peerDependencies for "@frontmcp/sdk" and
"@frontmcp/utils" are pinned to the exact version "0.12.1"; change those entries
to use a semver range (for example "^0.12.0" or a caret range matching your
intended compatibility) instead of an exact pin, update the same pattern
wherever these packages are declared as peers (e.g., in other package.json files
such as the testing and plugin adapter packages), and run a quick package.json
scan to ensure all peerDependencies follow the semver-range convention used
elsewhere (e.g., "^0.4.0"-style) to avoid blocking compatible patch updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9d1f56e-ab68-4d04-953d-9814da4c77ff
📒 Files selected for processing (11)
.github/workflows/cherry-pick-prompt.yml.github/workflows/codeql.yml.github/workflows/create-release-branch.yml.github/workflows/package-e2e.yml.github/workflows/perf-release.yml.github/workflows/perf.yml.github/workflows/pr-testing-registry.yml.github/workflows/publish-release.yml.github/workflows/push.yml.github/workflows/trigger-docs-update.ymllibs/react/package.json
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 100 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-03-15T10:46:00.736Z |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
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 (2)
tsconfig.base.json (1)
86-94:⚠️ Potential issue | 🟠 MajorAdd missing
@frontmcp/reactsubpath mappings to workspace TypeScript configuration.
libs/react/package.jsonexports./stateand./api, buttsconfig.base.jsononly maps@frontmcp/react,@frontmcp/react/ai, and@frontmcp/react/router. These new subpaths are already imported throughout documentation and demos, andapps/e2e/demo-e2e-react/vite.config.tscontains a local workaround for the missing mappings (lines 13–14). Without workspace-level path resolution,tsc, editors, and bundlers will resolve these imports inconsistently.Suggested fix
"@frontmcp/react": ["libs/react/src/index.ts"], "@frontmcp/react/ai": ["libs/react/src/ai/index.ts"], + "@frontmcp/react/api": ["libs/react/src/api/index.ts"], "@frontmcp/react/router": ["libs/react/src/router/index.ts"], + "@frontmcp/react/state": ["libs/react/src/state/index.ts"],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tsconfig.base.json` around lines 86 - 94, Add workspace path mappings for the missing React subpaths so TypeScript and bundlers resolve imports like `@frontmcp/react/state` and `@frontmcp/react/api`; update the tsconfig base "paths" for the existing "@frontmcp/react" entry to include entries mapping "@frontmcp/react/state" -> libs/react/src/state/index.ts and "@frontmcp/react/api" -> libs/react/src/api/index.ts (alongside the existing "@frontmcp/react", "@frontmcp/react/ai", and "@frontmcp/react/router" mappings).libs/react/src/provider/FrontMcpProvider.tsx (1)
67-92:⚠️ Potential issue | 🟠 MajorDynamic entries can’t target
servers[...]with this wiring.Only the primary
serveris wrapped withcreateWrappedServer; every extra named server is still registered raw. That leaves the newserveroption exposed by the hooks/docs with nowhere to route dynamic tools/resources for secondary servers, so they either end up on the primary server or stay unreachable. This needs per-server wrapped registries, or the option should be removed until it’s implemented.🤖 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 67 - 92, The code only wraps the primary server with createWrappedServer using dynamicRegistry (wrappedServer) but registers secondary servers raw, so dynamic entries cannot target servers[...]; update the useEffect that registers servers to wrap each entry in servers with createWrappedServer(srv, dynamicRegistry) before calling serverRegistry.register(sName, ...), and similarly unregister by reference to the same sName; ensure dynamicRegistry is used for every server registration (use dynamicRegistry and createWrappedServer when processing servers) so secondary named servers get per-server overlays instead of remaining unwrapped.
🟡 Minor comments (11)
docs/frontmcp/react/components.mdx-202-257 (1)
202-257:⚠️ Potential issue | 🟡 MinorDocument these two APIs as deprecated, not recommended.
Both sections currently read like normal guidance, but this page's doc rules say
AgentContentandAgentSearchshould steer users towardmcpComponenttable-mode usage plus migration examples. Please add an explicit deprecation note and point the examples at the replacement surface.As per coding guidelines, "
docs/frontmcp/react/components.mdx: AgentContent and AgentSearch are deprecated in favor of mcpComponent table-mode usage. ... Migration guidance provides concrete code examples converting AgentContent/AgentSearch to mcpComponent equivalents."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/react/components.mdx` around lines 202 - 257, Mark the AgentContent and AgentSearch APIs as deprecated and replace their usage guidance with a clear deprecation note that directs readers to use mcpComponent in table-mode instead; update the examples so they show the equivalent mcpComponent table-mode usage (replace the AgentContent/AgentSearch example snippets and their prop tables with mcpComponent equivalents or add an adjacent "Migration" subsection) and add concise migration guidance converting AgentContent -> mcpComponent table-mode and AgentSearch -> mcpComponent table-mode (include mapping of key props: name/toolName -> mcpComponent name, render/renderInput -> table-mode render cell, fallback/onResults -> appropriate table-mode callbacks) and a link to the migration docs/examples for further details.apps/e2e/demo-e2e-react/src/sections/StoreAdapterSection.tsx-27-45 (1)
27-45:⚠️ Potential issue | 🟡 MinorGuard the increment action until the tool is ready.
handleIncrementalways callscounter_increment, even when the client is disconnected or the tool is not registered. An early click will reject out of the event handler and skip the refresh path.Possible fix
const handleIncrement = async () => { - await callIncrement({ args: [] }); - // Re-read state after increment - await handleReadState(); + if (status !== 'connected' || !hasIncrementTool) return; + try { + await callIncrement({ args: [] }); + await handleReadState(); + } catch { + setStoreValue('error'); + } }; @@ - <button data-testid="store-increment" onClick={handleIncrement}> + <button + data-testid="store-increment" + onClick={handleIncrement} + disabled={status !== 'connected' || !hasIncrementTool} + > Increment </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-react/src/sections/StoreAdapterSection.tsx` around lines 27 - 45, handleIncrement currently always invokes callIncrement even if the client/tool is not ready; update handleIncrement to first check readiness (e.g., verify hasIncrementTool is true and the client/connection is available) and return early if not ready, and only call callIncrement({ args: [] }) when ready, then await handleReadState() in the success path; also ensure the UI button (store-increment) is disabled when hasIncrementTool is false to prevent early clicks.libs/react/src/hooks/__tests__/useDynamicResource.spec.tsx-113-135 (1)
113-135:⚠️ Potential issue | 🟡 MinorThe stale-closure test does not currently validate callback replacement.
Mutating
counterkeeps the samereadfunction identity, so this still passes even ifreadRef.currentstops updating. Use two differentreadcallbacks across rerenders.Suggested fix
it('uses latest read function via ref (no stale closures)', async () => { - let counter = 0; - const read = async (): Promise<ReadResourceResult> => ({ - contents: [{ uri: 'app://counter', text: String(counter) }], - }); + const makeRead = (value: number) => async (): Promise<ReadResourceResult> => ({ + contents: [{ uri: 'app://counter', text: String(value) }], + }); const { rerender } = renderHook( - () => + ({ read }) => useDynamicResource({ uri: 'app://counter', name: 'counter-resource', read, }), - { wrapper: createWrapper(dynamicRegistry) }, + { + initialProps: { read: makeRead(0) }, + wrapper: createWrapper(dynamicRegistry), + }, ); - counter = 42; - rerender(); + rerender({ read: makeRead(42) }); - const res = dynamicRegistry.findResource('app://counter')!; + const res = dynamicRegistry.findResource('app://counter'); + expect(res).toBeDefined(); + if (!res) throw new Error('Expected resource app://counter to be registered'); const result = await res.read(); expect(result.contents[0].text).toBe('42'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/hooks/__tests__/useDynamicResource.spec.tsx` around lines 113 - 135, The test currently mutates closure state but keeps the same read function identity, so change it to pass a new read callback on rerender to verify the hook actually replaces the ref; specifically, call useDynamicResource initially with one read function (e.g., readA) and then rerender with a different read function (e.g., readB) that returns the updated value, then call dynamicRegistry.findResource('app://counter')!.read() and assert the returned contents text is '42' to ensure useDynamicResource (and its internal readRef handling) uses the latest callback.libs/react/src/hooks/__tests__/useDynamicResource.spec.tsx-47-51 (1)
47-51:⚠️ Potential issue | 🟡 MinorReplace non-null assertions with proper type narrowing in tests.
Test files should use
expect()assertions for type guards, not throw statements. At line 69 and 132, addexpect(res).toBeDefined()before using the resource. At lines 49–51, wrap property assertions in an if block instead of using!operators.Examples
Lines 69–71:
const res = dynamicRegistry.findResource('app://data')!; + expect(res).toBeDefined(); + if (!res) return; const result = await res.read(); expect(result.contents[0].text).toBe('{"x":1}');Lines 49–51:
const res = dynamicRegistry.findResource('app://test'); expect(res).toBeDefined(); - expect(res!.name).toBe('test-resource'); + if (res) { + expect(res.name).toBe('test-resource'); + expect(res.description).toBe('A test resource'); + expect(res.mimeType).toBe('text/plain'); + }Applies to lines 49–51, 69, 132 per coding guideline: "Use proper error handling instead of non-null assertions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/hooks/__tests__/useDynamicResource.spec.tsx` around lines 47 - 51, The test uses non-null assertions on the result of dynamicRegistry.findResource (variable res); instead add type guards: first assert existence with expect(res).toBeDefined() before any property checks, then narrow the type with an if (res) { ... } block and move the expect(res.name), expect(res.description), expect(res.mimeType) into that block; also add the same expect(res).toBeDefined() checks before other uses of res in the test cases that reference dynamicRegistry.findResource (the other occurrences mentioned) so no `!` assertions are needed.apps/e2e/demo-e2e-react/e2e/browser/tool-listing.pw.spec.ts-17-21 (1)
17-21:⚠️ Potential issue | 🟡 MinorAwait element visibility before reading
textContentto avoid flaky tests.
textContent()can returnnullif the element hasn't rendered yet, causingNumber(null)to equal0and produce a misleading test failure. Consider awaiting visibility first.Proposed fix
test('reports tools count >= 2', async ({ page }) => { const count = page.locator('[data-testid="tools-count"]'); + await expect(count).toBeVisible({ timeout: 10_000 }); const text = await count.textContent(); - expect(Number(text)).toBeGreaterThanOrEqual(2); + expect(Number(text ?? '0')).toBeGreaterThanOrEqual(2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-react/e2e/browser/tool-listing.pw.spec.ts` around lines 17 - 21, The test 'reports tools count >= 2' reads textContent() immediately which can be null if the element hasn't rendered; before calling count.textContent() wait for the locator to be visible (e.g., await count.waitFor({ state: 'visible' }) or await expect(count).toBeVisible()) so textContent returns a string, then parse Number(text) and assert; update the locator variable 'count' and the call to textContent() accordingly to avoid flaky failures.libs/react/src/api/createFetchClient.ts-20-22 (1)
20-22:⚠️ Potential issue | 🟡 MinorMissing
Content-Typeheader when body is present.When sending a JSON body, the
Content-Type: application/jsonheader should be set to ensure servers correctly interpret the request payload.Proposed fix
if (config.body !== undefined) { fetchOptions.body = JSON.stringify(config.body); + fetchOptions.headers = { + ...fetchOptions.headers, + 'Content-Type': 'application/json', + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/api/createFetchClient.ts` around lines 20 - 22, When config.body is set and you stringify it into fetchOptions.body, ensure the request includes the Content-Type: application/json header; update the code around config.body/fetchOptions.body (in createFetchClient) to add Content-Type if not already present (case-insensitive) to fetchOptions.headers or merged headers (respecting any existing config.headers) so JSON requests are sent with the correct content type.libs/react/src/hooks/useDynamicResource.ts-20-21 (1)
20-21:⚠️ Potential issue | 🟡 MinorRemove the unused
serveroption or implement it.The
serveroption is defined inUseDynamicResourceOptionsbut is never destructured or passed todynamicRegistry.registerResource(). Either implement this feature by passingserverthrough the registration chain, or remove it from the interface if not yet needed. (Note:useDynamicToolhas the same issue.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/hooks/useDynamicResource.ts` around lines 20 - 21, The UseDynamicResourceOptions type includes a server?: string property that is never used; either remove it from UseDynamicResourceOptions (and the similar one in useDynamicTool) or thread it through the registration call chain by reading server from the hook options and passing it into dynamicRegistry.registerResource(...) (and any intermediate functions that build the registration payload). Update useDynamicResource (and useDynamicTool) to destructure server from the options, include it in the data passed to dynamicRegistry.registerResource (or remove the field from the interface and all call sites), and adjust types accordingly so the server field is either consumed or removed consistently.libs/react/src/state/useValtioResource.ts-38-46 (1)
38-46:⚠️ Potential issue | 🟡 MinorMisleading comment: mutations wrapper doesn't pass proxy as first arg.
The comment states "Wrap mutations to pass the proxy as first arg" but the implementation simply forwards arguments without prepending the proxy. Either update the comment to reflect the actual behavior or modify the implementation if proxy injection is intended.
Proposed fix (update comment)
- // Wrap mutations to pass the proxy as first arg + // Wrap mutations for consistent action interface const actions = useMemo(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/state/useValtioResource.ts` around lines 38 - 46, The comment above the mutations wrapper in useValtioResource is misleading: it claims "Wrap mutations to pass the proxy as first arg" but wrapped currently just forwards args; either update the comment to state that mutations are forwarded as-is (no proxy injection) or change the wrapper implementation to prepend the proxy when calling mutation; locate the useValtioResource function and its mutations handling (the wrapped variable created in the useMemo) and either (A) change the comment to "Wrap mutations and forward arguments as-is" or (B) modify wrapped[key] to call mutation(proxy, ...args) so the proxy is passed as the first argument.libs/react/src/state/useStoreResource.ts-14-16 (1)
14-16:⚠️ Potential issue | 🟡 MinorUnused
serverparameter.The
serverproperty is destructured from options but never used in the hook. Either utilize it for server targeting or remove it from the destructuring if it's reserved for future use.Proposed fix (if not needed)
export function useStoreResource(options: StoreResourceOptions): void { - const { name, getState, subscribe, selectors, actions, server } = options; + const { name, getState, subscribe, selectors, actions } = options; const { dynamicRegistry } = useContext(FrontMcpContext);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/state/useStoreResource.ts` around lines 14 - 16, The hook useStoreResource destructures a server property from the options but never uses it; remove server from the destructuring (i.e., adjust the line in useStoreResource that reads "const { name, getState, subscribe, selectors, actions, server } = options;" to omit server) or, if server is intended to be used, wire it into the hook logic where server targeting is required (update useStoreResource to pass server into any client/subscription setup such as subscribe/getState calls). Also ensure StoreResourceOptions type/signature is updated to reflect whether server remains part of the API.libs/react/src/registry/__tests__/createWrappedServer.spec.ts-451-459 (1)
451-459:⚠️ Potential issue | 🟡 MinorDon’t lock this test to raw client identity.
createWrappedServer.connect()is the place where dynamic behavior gets attached to the client.expect(result).toBe(mockClient)will either fail or enshrine the wrong contract; assert delegation plus the patched client’s observable behavior instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/registry/__tests__/createWrappedServer.spec.ts` around lines 451 - 459, The test currently asserts raw identity (expect(result).toBe(mockClient)) which ties the spec to the unwrapped client; instead, keep the delegation assertion for base.connect (ensure base.connect was called with 'session-1') and assert the returned client from wrapped.connect exhibits the patched/dynamic behavior added by createWrappedServer.connect — for example, call the client's observable method (mockClient.listTools or whatever method createWrappedServer augments) and assert the wrapped behavior/output rather than strict object identity; update assertions around wrapped.connect, base.connect, mockClient, and the client's listTools to reflect this.libs/react/src/hooks/__tests__/useComponentTree.spec.tsx-358-368 (1)
358-368:⚠️ Potential issue | 🟡 MinorThis doesn’t actually verify
serverrouting.The assertion only proves the resource was registered in the local
dynamicRegistry, so it still passes ifserveris ignored completely. To cover the contract, assert which wrapped server sees the resource, or that a different named server does not.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/hooks/__tests__/useComponentTree.spec.tsx` around lines 358 - 368, The test for useComponentTree currently only checks dynamicRegistry.hasResource and doesn't verify the server routing; update the test to assert the resource was registered with the intended server by (1) invoking useComponentTree({ rootRef, server: 'my-server' }) and then checking the registry tied to 'my-server' (or a mocked registry created for that server) for 'react://component-tree', and (2) optionally asserting that the default/other server registry does not contain that resource; locate references to useComponentTree and dynamicRegistry in the spec and adjust the assertions to inspect the correct server-specific registry (or add a mock for useDynamicResource to capture the server argument) so the test fails if the server option is ignored.
🧹 Nitpick comments (13)
apps/e2e/demo-e2e-react/playwright.config.ts (1)
9-9: Make retries CI-scoped to avoid masking local flakes.Line 9 currently retries locally as well; consider enabling retries only in CI.
♻️ Suggested tweak
- retries: 1, + retries: process.env['CI'] ? 1 : 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-react/playwright.config.ts` at line 9, The static retries: 1 setting causes local runs to retry; change the retries field to be CI-scoped by making it conditional on the CI environment variable (process.env.CI) so retries is 1 in CI and 0 locally; update the retries property in the exported Playwright config (the retries key in the config object) accordingly so local developers don't get masked flakes while CI still benefits from retries.libs/react/src/state/adapters/valtioAdapter.ts (1)
52-55: Consider accepting an optional caller-providedgetStatefunction instead of always JSON-serializing the proxy.
JSON.parse(JSON.stringify(proxy))stripsDate,Map,Set, andundefinedvalues, and throws onBigIntor cyclic references. While this is intentional for ensuring snapshot isolation, it limits state shape options if a consumer needs to store non-JSON-serializable values.Rather than making
getStaterequired inValtioStoreOptions(which would break the API), consider:
- Adding an optional
getState?: () => unknownparameter toValtioStoreOptions- Defaulting to the current JSON serialization behavior if not provided
- Documenting the constraint that state must be JSON-serializable unless overridden
This preserves backward compatibility while allowing advanced users to provide custom snapshot logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/state/adapters/valtioAdapter.ts` around lines 52 - 55, Update the Valtio adapter to accept an optional caller-provided snapshot function: add an optional getState?: () => unknown to ValtioStoreOptions and use it when building the returned object (instead of always calling JSON.parse(JSON.stringify(proxy))) so the returned getState uses options.getState ?? (() => JSON.parse(JSON.stringify(proxy))); keep subscribe as valtioSubscribe(proxy, cb) and ensure the default behavior (JSON serialization) is preserved when options.getState is not provided; also add a short doc comment on ValtioStoreOptions describing that the default requires JSON-serializable state unless overridden.apps/e2e/demo-e2e-react/src/sections/DynamicTool.tsx (2)
1-1: Remove unuseduseMemoimport.
useMemois imported but not used in this component.🧹 Proposed fix
-import React, { useState, useCallback, useMemo } from 'react'; +import React, { useState, useCallback } from 'react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-react/src/sections/DynamicTool.tsx` at line 1, The import list at the top of the DynamicTool component includes an unused symbol `useMemo`; remove `useMemo` from the React import (e.g., change "import React, { useState, useCallback, useMemo } from 'react';" to exclude `useMemo`) so only used hooks (`useState`, `useCallback`) remain referenced in the DynamicTool component.
32-34: Registration status is not tied to actual tool registration.The
registeredstate is set totrueunconditionally on mount, rather than reflecting the actual registration outcome fromuseDynamicTool. For a demo app, this is acceptable, but consider exposing registration status from the hook for more accurate feedback if available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-react/src/sections/DynamicTool.tsx` around lines 32 - 34, The component currently sets the local registered state to true unconditionally in the useEffect; instead, derive registration status from the dynamic tool hook or the registration result. Update the useEffect to call the registration API returned by useDynamicTool (or read a status flag like isRegistered/registered from useDynamicTool if available) and call setRegistered(true) only after a successful registration (or setRegistered(false) on error), using the hook's register function and any returned promise/result to determine success; keep references to useDynamicTool, setRegistered, and the useEffect callback to locate the change.libs/react/src/hooks/useComponentTree.ts (1)
33-33: Remove inferrable type annotation.Per ESLint, the
numbertype is trivially inferred from the default value0.Proposed fix
-function walkDom(element: Element, maxDepth: number, includeProps: boolean, depth: number = 0): TreeNode | null { +function walkDom(element: Element, maxDepth: number, includeProps: boolean, depth = 0): TreeNode | null {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/hooks/useComponentTree.ts` at line 33, Remove the redundant explicit type on the depth parameter of walkDom; change the function signature in walkDom(element: Element, maxDepth: number, includeProps: boolean, depth: number = 0) to omit the `: number` on `depth` (since the default value `= 0` infers the type), leaving depth with the default initializer only.libs/react/src/api/createFetchClient.ts (1)
10-41: Consider documenting JSON-only body limitation or adding flexibility.The implementation always serializes
bodyviaJSON.stringify, which will throw for non-serializable values (e.g., circular references,BigInt). This is acceptable for typical API use cases, but consider documenting this constraint in the JSDoc or allowing raw body pass-through for advanced use cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/api/createFetchClient.ts` around lines 10 - 41, The createFetchClient implementation unconditionally JSON.stringify's config.body which breaks for non-JSON body types; update the request logic in createFetchClient to detect and pass-through raw bodies for common non-JSON types (e.g., string, FormData, URLSearchParams, Blob, ArrayBuffer, ArrayBufferView, ReadableStream) by assigning fetchOptions.body = config.body for those types, and only JSON.stringify when the body is a plain object; also set or preserve Content-Type headers accordingly (e.g., add application/json when you stringify) so HttpRequestConfig handling in request() remains flexible and safe.apps/e2e/demo-e2e-react/src/sections/McpComponentTable.tsx (1)
51-58: Handle tool-call failures inhandleTrigger.If
callShowTablerejects, the click handler currently has no failure path. Catching the error will prevent unhandled promise noise and make demo behavior clearer.Suggested update
- const handleTrigger = async () => { - await callShowTable({ - rows: [ - { name: 'Alice', age: 30, role: 'Engineer' }, - { name: 'Bob', age: 25, role: 'Designer' }, - ], - }); - }; + const handleTrigger = async (): Promise<void> => { + try { + await callShowTable({ + rows: [ + { name: 'Alice', age: 30, role: 'Engineer' }, + { name: 'Bob', age: 25, role: 'Designer' }, + ], + }); + } catch (error) { + console.error('Failed to call show_users_table:', error); + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-react/src/sections/McpComponentTable.tsx` around lines 51 - 58, The click handler handleTrigger should catch failures from the async callShowTable to avoid unhandled promise rejections; wrap the await callShowTable(...) in a try/catch (inside handleTrigger), log or surface the error (e.g., console.error or a demo-level logger) and optionally show a user-friendly fallback/notification so demo behavior is clear on failure. Ensure you update the existing handleTrigger function and reference callShowTable in the catch handling.libs/react/src/api/parseOpenApiSpec.ts (1)
33-36: Harden parser against non-string OpenAPI text fields.Direct assertions can let malformed values leak through. A small runtime guard keeps outputs consistently typed.
Suggested update
- const operationId = (operation['operationId'] as string) ?? `${method}_${path.replace(/[^a-zA-Z0-9]/g, '_')}`; - const description = - (operation['summary'] as string) ?? (operation['description'] as string) ?? `${method.toUpperCase()} ${path}`; + const rawOperationId = operation['operationId']; + const rawSummary = operation['summary']; + const rawDescription = operation['description']; + + const operationId = + typeof rawOperationId === 'string' + ? rawOperationId + : `${method}_${path.replace(/[^a-zA-Z0-9]/g, '_')}`; + const description = + (typeof rawSummary === 'string' ? rawSummary : undefined) ?? + (typeof rawDescription === 'string' ? rawDescription : undefined) ?? + `${method.toUpperCase()} ${path}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/api/parseOpenApiSpec.ts` around lines 33 - 36, The parser currently assumes operation['operationId'], operation['summary'], and operation['description'] are strings; add runtime guards to ensure outputs are always strings by checking typeof before using them and falling back to the generated defaults (use the existing `${method}_${path.replace(...)} ` and `${method.toUpperCase()} ${path}` forms). Update the lines that compute operationId and description to coerce or validate values from operation (referencing operationId, description, operation, method, path) so non-string OpenAPI fields won’t be propagated.libs/react/src/state/useStoreRegistration.ts (1)
17-19: Remove unusedstoresRef.The
storesRefis assigned on lines 18-19 but never read. ThegetStateWrapperon line 30 usesadapter.getState()directly rather than referencingstoresRef. This appears to be leftover scaffolding.♻️ Proposed fix
export function useStoreRegistration(stores: StoreAdapter[], dynamicRegistry: DynamicRegistry): void { - const storesRef = useRef(stores); - storesRef.current = stores; - useEffect(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/state/useStoreRegistration.ts` around lines 17 - 19, The local ref storesRef in useStoreRegistration is unused; remove its declaration and assignments (storesRef and storesRef.current = stores) and update any related scaffolding so getStateWrapper continues to call adapter.getState() directly; ensure no other code in useStoreRegistration references storesRef and that TypeScript types (StoreAdapter, dynamicRegistry) still satisfy the function signature.libs/react/src/state/useValtioResource.ts (1)
48-49: Consider performance implications of deep clone approach.Using
JSON.parse(JSON.stringify(proxy))for state snapshots works for JSON-serializable state but:
- Has O(n) performance overhead on every read
- Silently drops
undefined, functions, and symbols- Throws on circular references
For typical Valtio usage with simple JSON state this is acceptable, but consider documenting this limitation or using Valtio's
snapshot()utility if available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/state/useValtioResource.ts` around lines 48 - 49, The current getState in useValtioResource uses JSON.parse(JSON.stringify(proxy)) which is an expensive deep clone and drops undefined/functions, and fails on circular refs; replace this with Valtio's snapshot utility (import snapshot from 'valtio' and return snapshot(proxy) inside getState) or, if snapshot isn't available, add clear comments in useValtioResource explaining the limitations of the JSON deep-clone approach (O(n) cost, loss of non-JSON types, circular reference failures) and consider caching or memoizing snapshots to reduce read overhead.libs/react/src/api/__tests__/useApiClient.spec.tsx (1)
19-23: Minor style inconsistency: JSX vs React.createElement.This wrapper uses JSX syntax while other test files in the PR (
mcpComponent.spec.tsx,AgentSearch.spec.tsx,useValtioResource.spec.tsx) useReact.createElement. Both are valid, but consistency across test files improves maintainability.Align with other test files using React.createElement
function createWrapper(ctx: FrontMcpContextValue) { return function Wrapper({ children }: { children: React.ReactNode }) { - return <FrontMcpContext.Provider value={ctx}>{children}</FrontMcpContext.Provider>; + return React.createElement(FrontMcpContext.Provider, { value: ctx }, children); }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/api/__tests__/useApiClient.spec.tsx` around lines 19 - 23, The createWrapper function currently returns JSX via <FrontMcpContext.Provider>, which is inconsistent with other tests; update createWrapper to return React.createElement calls instead (use React.createElement(FrontMcpContext.Provider, { value: ctx }, children)) so it matches the style used in mcpComponent.spec.tsx, AgentSearch.spec.tsx and useValtioResource.spec.tsx; keep the function name createWrapper and the FrontMcpContext.Provider value={ctx} usage intact.libs/react/src/hooks/useDynamicTool.ts (1)
56-63: Reuse the canonical tool-schema conversion path.This adds a second Zod→JSON Schema translator in the SDK. If
ToolEntry.getInputJsonSchema()is the source of truth elsewhere, local conversion here will drift on metadata/defaults/refinements. Please route through the shared path instead of callingzodToJsonSchemadirectly.As per coding guidelines
Use ToolEntry.getInputJsonSchema() as single source of truth for tool input schema instead of duplicating conversion logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/hooks/useDynamicTool.ts` around lines 56 - 63, The code in useDynamicTool.ts currently converts a Zod schema to JSON Schema inline by calling zodToJsonSchema inside the useMemo for resolvedInputSchema; replace that direct conversion with the canonical conversion path by using ToolEntry.getInputJsonSchema() when a Zod schema is provided so the project-wide metadata/defaults/refinements are preserved. Concretely, in the useMemo that computes resolvedInputSchema, detect if options has a Zod schema (options.schema) and call the shared conversion function ToolEntry.getInputJsonSchema(...) or equivalent accessor instead of zodToJsonSchema, otherwise fall back to (options as UseDynamicToolJsonSchemaOptions).inputSchema; update the dependency array to reference the same option keys used to decide the path.libs/react/src/registry/DynamicRegistry.ts (1)
112-115: Use block body to avoid implicit return in forEach callback.The Biome linter flags this because the arrow function implicitly returns the result of
l(). While the return value isvoidand harmless, using a block body makes the intent explicit and silences the warning.🔧 Proposed fix
private notify(): void { this.version++; - this.listeners.forEach((l) => l()); + this.listeners.forEach((l) => { + l(); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/registry/DynamicRegistry.ts` around lines 112 - 115, The notify method currently uses an arrow function with an implicit return in this.listeners.forEach, which triggers a lint warning; update notify (the method that increments version and iterates listeners) to call listeners.forEach with an arrow function that uses a block body and explicitly invokes the listener (i.e., replace the implicit-return callback with a block that calls the listener), leaving the version++ increment intact so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/demo-e2e-react/e2e/browser/mcp-component-table.pw.spec.ts`:
- Around line 15-16: The test currently uses a hard-coded sleep via
page.waitForTimeout(500) which causes flakiness; instead gate the subsequent
page.click(...) on the table trigger by waiting for the trigger element to be
ready. Replace the page.waitForTimeout usage with an explicit readiness check
such as awaiting page.waitForSelector('<table-trigger-selector>') or using
locator.waitFor({ state: 'visible' })/locator.waitFor({ state: 'attached' }) for
the same selector used by the page.click call, then perform page.click on that
selector so the click only happens when the UI is ready.
In `@apps/e2e/demo-e2e-react/e2e/browser/mcp-component.pw.spec.ts`:
- Around line 15-16: Replace the brittle fixed sleep (the call to
page.waitForTimeout(500)) with a state-based wait that watches for the dynamic
tool/trigger to become ready: remove the page.waitForTimeout(500) call and
instead wait for a deterministic condition such as a DOM selector or attribute
that signals readiness (e.g. wait for a specific element/test-id to appear or
for a trigger element to have a `data-ready`/`aria-*` flag) or use
page.waitForFunction to poll a window/global flag (e.g.
window.__triggerRegistered) that your app sets when registration completes;
update the test around the registration logic (the code surrounding
page.waitForTimeout(500)) to use that readiness check so CI is not flaky.
In `@apps/e2e/demo-e2e-react/src/App.tsx`:
- Line 44: The forEach callback uses a concise arrow body which violates Biome's
useIterableCallbackReturn rule; change the arrow function passed to
listeners.forEach to use a block statement with an explicit call (e.g., replace
(cb) => cb() with (cb) => { cb(); }) so the callback has a block body—update the
listeners.forEach(...) invocation accordingly.
In `@apps/e2e/demo-e2e-react/src/main.tsx`:
- Around line 6-10: Replace the non-null assertion on
document.getElementById('root') used by createRoot with explicit null checking:
call document.getElementById('root'), validate the result, and if it's null
throw a clear Error (e.g., "Root element with id 'root' not found") before
calling createRoot(...). Update the code around createRoot, StrictMode and App
to use that validated element instead of the '!' assertion so the runtime fails
with a clear message when the root element is missing.
In `@apps/e2e/demo-e2e-react/src/sections/StoreAdapterSection.tsx`:
- Around line 17-20: Replace the handwritten type assertion on the result of
client.readResource with the official MCP type: import the ReadResourceResult
type from '@frontmcp/sdk', cast or annotate the response from
client.readResource as ReadResourceResult (e.g., const result = await
client.readResource('state://counter') as ReadResourceResult), then use
result.contents and pass the text into setStoreValue; update any related typings
so the MCP protocol shape is enforced rather than the ad-hoc { contents?:
Array<{ text?: string }> } assertion.
In `@libs/react/src/api/useApiClient.ts`:
- Around line 14-18: interpolatePath currently injects raw param values which
can break routing for reserved characters; update the interpolatePath function
to URL-encode interpolated values by applying encodeURIComponent to the
stringified param (while still returning the original {key} when value is
null/undefined). Locate interpolatePath and change the replacement to use
String(value) passed through encodeURIComponent (or a small helper to handle
non-primitive values deterministically) so every substituted path segment is
safely percent-encoded.
- Around line 22-82: useApiClient currently destructures the server option from
ApiClientOptions but never uses it when registering tools via
dynamicRegistry.registerTool, so server-scoped behavior is silently ignored;
either remove server from ApiClientOptions (and from useApiClient signature) or
extend the dynamic tool registration to pass server through: add a server field
to DynamicToolDef and include server when calling dynamicRegistry.registerTool({
name: toolName, description: op.description, inputSchema: op.inputSchema,
execute, server }), updating any types (ApiClientOptions, DynamicToolDef) and
consumers accordingly so the server scope is actually propagated from
useApiClient to the registry.
In `@libs/react/src/components/mcpComponent.tsx`:
- Around line 125-151: The dynamic tool registration is keyed only by name so
multiple McpWrappedComponentInner instances clobber each other; fix by making
registration instance-safe: either (A) generate a stable per-instance id inside
McpWrappedComponentInner (useRef) and call useDynamicTool with a composite key
like `${name}:${instanceId}` so each instance registers separately, or (B)
update DynamicRegistry to be reference-counted (increment on register, decrement
on unregister) and ensure useDynamicTool calls the shared register/unregister
APIs; modify the execute/cleanup usage so the same key/path is used for both
registration and unregistration (reference McpWrappedComponentInner,
useDynamicTool, name, DynamicRegistry, execute) to prevent one instance unmount
from removing another's handler.
- Around line 79-87: The current isLazyImport detection (function isLazyImport)
wrongly treats zero-arity functional components as lazy loaders because it
checks fn.length === 0; change the heuristic to require an explicit lazy marker
instead: detect a dedicated property (e.g. (fn as any).__isLazyImport === true)
or a branded wrapper type rather than relying on arity, and update any code that
constructs lazy imports (where you previously passed () => import(...)) to set
that marker or wrap via a new helper (e.g., createLazyFactory) so isLazyImport
only returns true for true lazy factories and not synchronous zero-arg
components.
- Around line 112-119: Replace the runtime require('zod') with a static import
at module scope and use that imported symbol to build the rows wrapper;
specifically, add a top-level import like "import * as z from 'zod'" (or "import
{ z } from 'zod'") and then in mcpComponent.tsx replace the require usage so the
isTableMode branch sets toolSchema = z.object({ rows: z.array(schema) }) as
unknown as z.ZodObject<z.ZodRawShape>; keep the variable names toolSchema,
isTableMode and schema the same so types remain correct and tree-shaking/ESM
consumers are not broken.
In `@libs/react/src/provider/FrontMcpProvider.tsx`:
- Around line 125-165: toolsResult fallback and extraction are wrong:
client.listTools() returns an object { tools: ToolInfo[] } but safeList
currently uses [] and code casts toolsResult to ToolInfo[] (baseTools) causing
Array.isArray to fail and drop server tools. Change the safeList call for
listTools to use a matching fallback like { tools: [] }, then extract the tools
array via the tools property (use toolsResult.tools or similar) before
filtering; update references to baseTools/filteredBaseTools and the
serverRegistry.update call to use the extracted array so server tools are
preserved (referencing safeList, client.listTools, toolsResult, baseTools,
filteredBaseTools, and serverRegistry.update).
In `@libs/react/src/state/useStoreResource.ts`:
- Around line 58-89: The selector sub-resources register read handlers but never
update when the store changes; inside the same useEffect that iterates selectors
(the block creating selectorRef, uri, readSelector, and calling
dynamicRegistry.registerResource), also subscribe to store updates and call
updateResourceRead(uri, {uri, mimeType:'application/json', text:
JSON.stringify(selectorRef.current(getStateRef.current())) }) whenever the store
changes; keep the subscription handle and call it in the returned cleanup along
with the registered resource cleanup, and include getStateRef and
updateResourceRead (and any subscribe API you use) in the effect dependencies so
selector resources refresh automatically on store updates.
---
Outside diff comments:
In `@libs/react/src/provider/FrontMcpProvider.tsx`:
- Around line 67-92: The code only wraps the primary server with
createWrappedServer using dynamicRegistry (wrappedServer) but registers
secondary servers raw, so dynamic entries cannot target servers[...]; update the
useEffect that registers servers to wrap each entry in servers with
createWrappedServer(srv, dynamicRegistry) before calling
serverRegistry.register(sName, ...), and similarly unregister by reference to
the same sName; ensure dynamicRegistry is used for every server registration
(use dynamicRegistry and createWrappedServer when processing servers) so
secondary named servers get per-server overlays instead of remaining unwrapped.
In `@tsconfig.base.json`:
- Around line 86-94: Add workspace path mappings for the missing React subpaths
so TypeScript and bundlers resolve imports like `@frontmcp/react/state` and
`@frontmcp/react/api`; update the tsconfig base "paths" for the existing
"@frontmcp/react" entry to include entries mapping "@frontmcp/react/state" ->
libs/react/src/state/index.ts and "@frontmcp/react/api" ->
libs/react/src/api/index.ts (alongside the existing "@frontmcp/react",
"@frontmcp/react/ai", and "@frontmcp/react/router" mappings).
---
Minor comments:
In `@apps/e2e/demo-e2e-react/e2e/browser/tool-listing.pw.spec.ts`:
- Around line 17-21: The test 'reports tools count >= 2' reads textContent()
immediately which can be null if the element hasn't rendered; before calling
count.textContent() wait for the locator to be visible (e.g., await
count.waitFor({ state: 'visible' }) or await expect(count).toBeVisible()) so
textContent returns a string, then parse Number(text) and assert; update the
locator variable 'count' and the call to textContent() accordingly to avoid
flaky failures.
In `@apps/e2e/demo-e2e-react/src/sections/StoreAdapterSection.tsx`:
- Around line 27-45: handleIncrement currently always invokes callIncrement even
if the client/tool is not ready; update handleIncrement to first check readiness
(e.g., verify hasIncrementTool is true and the client/connection is available)
and return early if not ready, and only call callIncrement({ args: [] }) when
ready, then await handleReadState() in the success path; also ensure the UI
button (store-increment) is disabled when hasIncrementTool is false to prevent
early clicks.
In `@docs/frontmcp/react/components.mdx`:
- Around line 202-257: Mark the AgentContent and AgentSearch APIs as deprecated
and replace their usage guidance with a clear deprecation note that directs
readers to use mcpComponent in table-mode instead; update the examples so they
show the equivalent mcpComponent table-mode usage (replace the
AgentContent/AgentSearch example snippets and their prop tables with
mcpComponent equivalents or add an adjacent "Migration" subsection) and add
concise migration guidance converting AgentContent -> mcpComponent table-mode
and AgentSearch -> mcpComponent table-mode (include mapping of key props:
name/toolName -> mcpComponent name, render/renderInput -> table-mode render
cell, fallback/onResults -> appropriate table-mode callbacks) and a link to the
migration docs/examples for further details.
In `@libs/react/src/api/createFetchClient.ts`:
- Around line 20-22: When config.body is set and you stringify it into
fetchOptions.body, ensure the request includes the Content-Type:
application/json header; update the code around config.body/fetchOptions.body
(in createFetchClient) to add Content-Type if not already present
(case-insensitive) to fetchOptions.headers or merged headers (respecting any
existing config.headers) so JSON requests are sent with the correct content
type.
In `@libs/react/src/hooks/__tests__/useComponentTree.spec.tsx`:
- Around line 358-368: The test for useComponentTree currently only checks
dynamicRegistry.hasResource and doesn't verify the server routing; update the
test to assert the resource was registered with the intended server by (1)
invoking useComponentTree({ rootRef, server: 'my-server' }) and then checking
the registry tied to 'my-server' (or a mocked registry created for that server)
for 'react://component-tree', and (2) optionally asserting that the
default/other server registry does not contain that resource; locate references
to useComponentTree and dynamicRegistry in the spec and adjust the assertions to
inspect the correct server-specific registry (or add a mock for
useDynamicResource to capture the server argument) so the test fails if the
server option is ignored.
In `@libs/react/src/hooks/__tests__/useDynamicResource.spec.tsx`:
- Around line 113-135: The test currently mutates closure state but keeps the
same read function identity, so change it to pass a new read callback on
rerender to verify the hook actually replaces the ref; specifically, call
useDynamicResource initially with one read function (e.g., readA) and then
rerender with a different read function (e.g., readB) that returns the updated
value, then call dynamicRegistry.findResource('app://counter')!.read() and
assert the returned contents text is '42' to ensure useDynamicResource (and its
internal readRef handling) uses the latest callback.
- Around line 47-51: The test uses non-null assertions on the result of
dynamicRegistry.findResource (variable res); instead add type guards: first
assert existence with expect(res).toBeDefined() before any property checks, then
narrow the type with an if (res) { ... } block and move the expect(res.name),
expect(res.description), expect(res.mimeType) into that block; also add the same
expect(res).toBeDefined() checks before other uses of res in the test cases that
reference dynamicRegistry.findResource (the other occurrences mentioned) so no
`!` assertions are needed.
In `@libs/react/src/hooks/useDynamicResource.ts`:
- Around line 20-21: The UseDynamicResourceOptions type includes a server?:
string property that is never used; either remove it from
UseDynamicResourceOptions (and the similar one in useDynamicTool) or thread it
through the registration call chain by reading server from the hook options and
passing it into dynamicRegistry.registerResource(...) (and any intermediate
functions that build the registration payload). Update useDynamicResource (and
useDynamicTool) to destructure server from the options, include it in the data
passed to dynamicRegistry.registerResource (or remove the field from the
interface and all call sites), and adjust types accordingly so the server field
is either consumed or removed consistently.
In `@libs/react/src/registry/__tests__/createWrappedServer.spec.ts`:
- Around line 451-459: The test currently asserts raw identity
(expect(result).toBe(mockClient)) which ties the spec to the unwrapped client;
instead, keep the delegation assertion for base.connect (ensure base.connect was
called with 'session-1') and assert the returned client from wrapped.connect
exhibits the patched/dynamic behavior added by createWrappedServer.connect — for
example, call the client's observable method (mockClient.listTools or whatever
method createWrappedServer augments) and assert the wrapped behavior/output
rather than strict object identity; update assertions around wrapped.connect,
base.connect, mockClient, and the client's listTools to reflect this.
In `@libs/react/src/state/useStoreResource.ts`:
- Around line 14-16: The hook useStoreResource destructures a server property
from the options but never uses it; remove server from the destructuring (i.e.,
adjust the line in useStoreResource that reads "const { name, getState,
subscribe, selectors, actions, server } = options;" to omit server) or, if
server is intended to be used, wire it into the hook logic where server
targeting is required (update useStoreResource to pass server into any
client/subscription setup such as subscribe/getState calls). Also ensure
StoreResourceOptions type/signature is updated to reflect whether server remains
part of the API.
In `@libs/react/src/state/useValtioResource.ts`:
- Around line 38-46: The comment above the mutations wrapper in
useValtioResource is misleading: it claims "Wrap mutations to pass the proxy as
first arg" but wrapped currently just forwards args; either update the comment
to state that mutations are forwarded as-is (no proxy injection) or change the
wrapper implementation to prepend the proxy when calling mutation; locate the
useValtioResource function and its mutations handling (the wrapped variable
created in the useMemo) and either (A) change the comment to "Wrap mutations and
forward arguments as-is" or (B) modify wrapped[key] to call mutation(proxy,
...args) so the proxy is passed as the first argument.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-react/playwright.config.ts`:
- Line 9: The static retries: 1 setting causes local runs to retry; change the
retries field to be CI-scoped by making it conditional on the CI environment
variable (process.env.CI) so retries is 1 in CI and 0 locally; update the
retries property in the exported Playwright config (the retries key in the
config object) accordingly so local developers don't get masked flakes while CI
still benefits from retries.
In `@apps/e2e/demo-e2e-react/src/sections/DynamicTool.tsx`:
- Line 1: The import list at the top of the DynamicTool component includes an
unused symbol `useMemo`; remove `useMemo` from the React import (e.g., change
"import React, { useState, useCallback, useMemo } from 'react';" to exclude
`useMemo`) so only used hooks (`useState`, `useCallback`) remain referenced in
the DynamicTool component.
- Around line 32-34: The component currently sets the local registered state to
true unconditionally in the useEffect; instead, derive registration status from
the dynamic tool hook or the registration result. Update the useEffect to call
the registration API returned by useDynamicTool (or read a status flag like
isRegistered/registered from useDynamicTool if available) and call
setRegistered(true) only after a successful registration (or
setRegistered(false) on error), using the hook's register function and any
returned promise/result to determine success; keep references to useDynamicTool,
setRegistered, and the useEffect callback to locate the change.
In `@apps/e2e/demo-e2e-react/src/sections/McpComponentTable.tsx`:
- Around line 51-58: The click handler handleTrigger should catch failures from
the async callShowTable to avoid unhandled promise rejections; wrap the await
callShowTable(...) in a try/catch (inside handleTrigger), log or surface the
error (e.g., console.error or a demo-level logger) and optionally show a
user-friendly fallback/notification so demo behavior is clear on failure. Ensure
you update the existing handleTrigger function and reference callShowTable in
the catch handling.
In `@libs/react/src/api/__tests__/useApiClient.spec.tsx`:
- Around line 19-23: The createWrapper function currently returns JSX via
<FrontMcpContext.Provider>, which is inconsistent with other tests; update
createWrapper to return React.createElement calls instead (use
React.createElement(FrontMcpContext.Provider, { value: ctx }, children)) so it
matches the style used in mcpComponent.spec.tsx, AgentSearch.spec.tsx and
useValtioResource.spec.tsx; keep the function name createWrapper and the
FrontMcpContext.Provider value={ctx} usage intact.
In `@libs/react/src/api/createFetchClient.ts`:
- Around line 10-41: The createFetchClient implementation unconditionally
JSON.stringify's config.body which breaks for non-JSON body types; update the
request logic in createFetchClient to detect and pass-through raw bodies for
common non-JSON types (e.g., string, FormData, URLSearchParams, Blob,
ArrayBuffer, ArrayBufferView, ReadableStream) by assigning fetchOptions.body =
config.body for those types, and only JSON.stringify when the body is a plain
object; also set or preserve Content-Type headers accordingly (e.g., add
application/json when you stringify) so HttpRequestConfig handling in request()
remains flexible and safe.
In `@libs/react/src/api/parseOpenApiSpec.ts`:
- Around line 33-36: The parser currently assumes operation['operationId'],
operation['summary'], and operation['description'] are strings; add runtime
guards to ensure outputs are always strings by checking typeof before using them
and falling back to the generated defaults (use the existing
`${method}_${path.replace(...)} ` and `${method.toUpperCase()} ${path}` forms).
Update the lines that compute operationId and description to coerce or validate
values from operation (referencing operationId, description, operation, method,
path) so non-string OpenAPI fields won’t be propagated.
In `@libs/react/src/hooks/useComponentTree.ts`:
- Line 33: Remove the redundant explicit type on the depth parameter of walkDom;
change the function signature in walkDom(element: Element, maxDepth: number,
includeProps: boolean, depth: number = 0) to omit the `: number` on `depth`
(since the default value `= 0` infers the type), leaving depth with the default
initializer only.
In `@libs/react/src/hooks/useDynamicTool.ts`:
- Around line 56-63: The code in useDynamicTool.ts currently converts a Zod
schema to JSON Schema inline by calling zodToJsonSchema inside the useMemo for
resolvedInputSchema; replace that direct conversion with the canonical
conversion path by using ToolEntry.getInputJsonSchema() when a Zod schema is
provided so the project-wide metadata/defaults/refinements are preserved.
Concretely, in the useMemo that computes resolvedInputSchema, detect if options
has a Zod schema (options.schema) and call the shared conversion function
ToolEntry.getInputJsonSchema(...) or equivalent accessor instead of
zodToJsonSchema, otherwise fall back to (options as
UseDynamicToolJsonSchemaOptions).inputSchema; update the dependency array to
reference the same option keys used to decide the path.
In `@libs/react/src/registry/DynamicRegistry.ts`:
- Around line 112-115: The notify method currently uses an arrow function with
an implicit return in this.listeners.forEach, which triggers a lint warning;
update notify (the method that increments version and iterates listeners) to
call listeners.forEach with an arrow function that uses a block body and
explicitly invokes the listener (i.e., replace the implicit-return callback with
a block that calls the listener), leaving the version++ increment intact so
behavior is unchanged.
In `@libs/react/src/state/adapters/valtioAdapter.ts`:
- Around line 52-55: Update the Valtio adapter to accept an optional
caller-provided snapshot function: add an optional getState?: () => unknown to
ValtioStoreOptions and use it when building the returned object (instead of
always calling JSON.parse(JSON.stringify(proxy))) so the returned getState uses
options.getState ?? (() => JSON.parse(JSON.stringify(proxy))); keep subscribe as
valtioSubscribe(proxy, cb) and ensure the default behavior (JSON serialization)
is preserved when options.getState is not provided; also add a short doc comment
on ValtioStoreOptions describing that the default requires JSON-serializable
state unless overridden.
In `@libs/react/src/state/useStoreRegistration.ts`:
- Around line 17-19: The local ref storesRef in useStoreRegistration is unused;
remove its declaration and assignments (storesRef and storesRef.current =
stores) and update any related scaffolding so getStateWrapper continues to call
adapter.getState() directly; ensure no other code in useStoreRegistration
references storesRef and that TypeScript types (StoreAdapter, dynamicRegistry)
still satisfy the function signature.
In `@libs/react/src/state/useValtioResource.ts`:
- Around line 48-49: The current getState in useValtioResource uses
JSON.parse(JSON.stringify(proxy)) which is an expensive deep clone and drops
undefined/functions, and fails on circular refs; replace this with Valtio's
snapshot utility (import snapshot from 'valtio' and return snapshot(proxy)
inside getState) or, if snapshot isn't available, add clear comments in
useValtioResource explaining the limitations of the JSON deep-clone approach
(O(n) cost, loss of non-JSON types, circular reference failures) and consider
caching or memoizing snapshots to reduce read overhead.
apps/e2e/demo-e2e-react/e2e/browser/mcp-component-table.pw.spec.ts
Outdated
Show resolved
Hide resolved
…isibility checks and enhance lazy loading support in mcpComponent
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (3)
libs/react/src/components/mcpComponent.tsx (1)
151-157:⚠️ Potential issue | 🟠 MajorTool registration key may still be instance-unsafe.
This still registers by shared
name(Line 152). If the underlying registry is keyed/cleaned up byname, multiple mounted instances can overwrite/unregister each other.#!/bin/bash set -euo pipefail echo "1) Locate useDynamicTool implementation" fd 'useDynamicTool.ts' libs -x sh -c 'echo "===== $1 ====="; sed -n "1,240p" "$1"' _ {} echo "2) Locate dynamic registry implementations in React/UI libs" fd '*registry*.ts' libs -x sh -c 'echo "===== $1 ====="; rg -n -C4 "Map<|register|unregister|set\\(|delete\\(|refCount|count|name" "$1"' _ {} echo "3) Show callsites using useDynamicTool({ name: ... })" rg -n -C3 'useDynamicTool\(\s*\{' libs/react/srcExpected verification result:
- Issue confirmed if register/unregister uses a single
namekey with direct delete on cleanup and no ref-count/token.- Issue resolved if hook/registry tracks instance identity or reference counts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/components/mcpComponent.tsx` around lines 151 - 157, The hook registers tools by the shared "name" which lets multiple mounted instances overwrite/unregister each other; update useDynamicTool to generate and pass a unique instance key (e.g., instanceId/symbol/UUID) alongside name into the registry and ensure the registry's register/unregister APIs accept that instance key (or implement ref-counting) so cleanup only removes the specific instance; change the call-site in useDynamicTool usage (the block passing name, description, schema, execute, server) to include the new instance identifier and update the corresponding register/unregister logic in the registry to use the instance key or ref-count instead of deleting by the shared name.libs/react/src/api/useApiClient.ts (1)
21-23:⚠️ Potential issue | 🟠 Major
ApiClientOptions.serveris still a no-op in this hook.The hook accepts
ApiClientOptionsbut does not apply server scoping during tool registration. This keeps a misleading public option in the API surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/api/useApiClient.ts` around lines 21 - 23, The hook is ignoring ApiClientOptions.server so server scoping is a no-op; update useApiClient to apply options.server when registering tools/operations with the context registry by reading options.server and forwarding it into the registration calls (e.g., when you call dynamicRegistry.registerTool / registerOperation or similar) or by obtaining a server-scoped registry (e.g., dynamicRegistry.forServer(server)) before registering operations; ensure useApiClient (and any helper like registerOperationsInRegistry) uses the server value so the public ApiClientOptions.server actually controls registration scope and remove any misleading unused parameter handling.libs/react/src/state/useStoreResource.ts (1)
100-100:⚠️ Potential issue | 🟡 MinorThese cleanup callbacks still fail the Biome iterable-callback rule.
cleanups.forEach((fn) => fn())implicitly returnsfn(), which triggerslint/suspicious/useIterableCallbackReturnin both cleanup paths. Wrap the calls in block bodies to keep this hook lint-clean.Suggested fix
- return () => cleanups.forEach((fn) => fn()); + return () => { + cleanups.forEach((fn) => { + fn(); + }); + }; ... - return () => cleanups.forEach((fn) => fn()); + return () => { + cleanups.forEach((fn) => { + fn(); + }); + };Also applies to: 134-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/state/useStoreResource.ts` at line 100, The cleanup callback in useStoreResource currently uses concise arrow callbacks that implicitly return the result of fn(), triggering the lint rule; update the two cleanup invocations (the return cleanup at the end of useStoreResource and the other cleanup at the line noted) to use block-bodied arrow functions so they call fn() without returning a value (e.g., replace (fn) => fn() with (fn) => { fn(); }) to satisfy lint/suspicious/useIterableCallbackReturn.
🧹 Nitpick comments (1)
libs/react/src/api/parseOpenApiSpec.ts (1)
33-36: FallbackoperationIdgeneration can collide.Line 35 normalizes many distinct paths into the same slug, which can produce duplicate
operationIdvalues. Consider de-duplicating IDs as they are generated.💡 Proposed refactor
const operations: ApiOperation[] = []; + const usedOperationIds = new Set<string>(); @@ - const operationId = - typeof rawOperationId === 'string' ? rawOperationId : `${method}_${path.replace(/[^a-zA-Z0-9]/g, '_')}`; + const baseOperationId = + typeof rawOperationId === 'string' ? rawOperationId : `${method}_${path.replace(/[^a-zA-Z0-9]/g, '_')}`; + let operationId = baseOperationId; + let suffix = 2; + while (usedOperationIds.has(operationId)) { + operationId = `${baseOperationId}_${suffix++}`; + } + usedOperationIds.add(operationId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/api/parseOpenApiSpec.ts` around lines 33 - 36, The fallback operationId generation in parseOpenApiSpec (where rawOperationId -> operationId using `${method}_${path.replace(/[^a-zA-Z0-9]/g, '_')}`) can produce duplicates; update parseOpenApiSpec to track generated IDs (e.g., a Set or Map) and de-duplicate by appending a deterministic suffix (like _1, _2, ...) when a collision is detected, ensuring you still prefer a string rawOperationId when present and update any references to operationId accordingly (use the same collision logic for both computed and normalized names).
🤖 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/api/parseOpenApiSpec.ts`:
- Around line 49-57: The parameter extraction is brittle: ensure you merge and
iterate both pathItem.parameters and operation.parameters (with operation-level
params taking precedence), guard against non-array or null entries before
iterating, and skip non-object or missing-name entries; in the code that builds
properties/required (the loop over parameters and use of OpenApiParameter,
properties, required, and operation), normalize parameters into a safe array
(e.g., Array.isArray check and concat pathItem.parameters || [] and
operation.parameters || []), filter out null/undefined and non-object items,
then use param.name safely and fall back for param.schema to { type: 'string' }
when null/undefined; also ensure required is only pushed for truthy
param.required so shared path params aren’t lost.
In `@libs/react/src/api/useApiClient.ts`:
- Line 85: The returned cleanup function in useApiClient.ts currently uses an
arrow callback with an implicit return inside cleanups.forEach which triggers
the lint rule; change the callback to a statement body that invokes each cleanup
without returning (e.g., replace the implicit-return arrow with a block that
calls fn();), or alternatively iterate with a for...of over cleanups to call
each fn(), ensuring no value is returned from the forEach/iteration.
In `@libs/react/src/components/mcpComponent.tsx`:
- Around line 52-56: The ComponentArg union is too permissive and lets plain
async import functions be treated as eager ComponentType<Props>; define a
branded LazyFactory<Props> type (e.g., type LazyFactory<Props> = () => Promise<{
default: ComponentType<Props> }> & { __isLazy: true }) and replace the raw async
signature in ComponentArg with that branded LazyFactory<Props>, then update
usages (e.g., where async loaders are provided) to return/attach the brand so
only explicitly marked lazy factories follow the lazy runtime path; update
related occurrences referenced in the file (the ComponentArg type and any helper
that accepts loaders) so the compiler rejects unbranded () => import(...)
functions.
In `@libs/react/src/state/__tests__/useStoreRegistration.spec.tsx`:
- Line 18: Replace the implicit-return arrow callback used in
listeners.forEach((l) => l()) with a block-style callback to satisfy the
useIterableCallbackReturn rule; locate the listeners.forEach call in
useStoreRegistration.spec.tsx and change the callback to a block-bodied form
(e.g., (l) => { l(); }) so it clearly expresses a side effect rather than a
return value.
In `@libs/react/src/state/__tests__/useStoreResource.spec.tsx`:
- Around line 184-203: The test currently only spies on
dynamicRegistry.updateResourceRead when exercising useStoreResource; change it
to assert the observable invalidation contract by checking
dynamicRegistry.getVersion() changed (or by subscribing a listener via
dynamicRegistry.subscribe and asserting it was called) after store.setState()
instead of only asserting updateResourceRead; update the test that renders
useStoreResource (and the similar test at the 205-233 block) to record the
registry version before render, call store.setState({ value: 'updated' }), and
assert dynamicRegistry.getVersion() > previousVersion (or that the subscribed
listener invoked) to ensure mutations actually bump the registry and notify
subscribers.
- Around line 31-33: Update the concise arrow callback in the setState
implementation so the forEach callback uses a block body instead of implicitly
returning a value: inside the setState method (the function that updates `state`
and iterates `listeners`), change the `listeners.forEach((l) => l())` callback
to a block-bodied form such as `listeners.forEach((l) => { l(); })` so it no
longer returns a value and complies with the `useIterableCallbackReturn` rule.
In `@libs/react/src/state/useStoreRegistration.ts`:
- Around line 33-57: Validate the generated resource URIs (the `uri` values
built as `state://${name}` and `state://${name}/${key}`) against RFC3986 before
calling dynamicRegistry.registerResource or pushing into `selectorEntries`; in
the `useStoreRegistration` logic, add a URI validation step (e.g., using the URL
constructor or a RFC3986 validator) that throws or rejects invalid values and
surface the schema validation error message "URI must have a valid scheme (e.g.,
file://, https://, custom://)"; apply this check where
`dynamicRegistry.registerResource` is invoked for the store
(`readState`/`getStateWrapper`) and for each selector entry so malformed `name`
or selector `key` cannot be registered.
- Line 122: The forEach callback on cleanups currently uses an implicit-return
arrow (cleanups.forEach((fn) => fn());) which triggers the lint rule; change the
callback to use a block body so it does not return a value (e.g., update the
cleanups.forEach callback that accepts fn to a block that calls fn() inside `{
... }`), keeping the same behavior but preventing any return from the forEach
callback.
In `@libs/react/src/state/useStoreResource.ts`:
- Around line 46-49: The subscribe callback is replacing the read callback via
dynamicRegistry.updateResourceRead(`state://${name}`, readState) but
DynamicRegistry.updateResourceRead currently only swaps the callback and does
not increment the resource version, so consumers of DynamicRegistry.subscribe()
/ getVersion() never observe changes; fix this by making the invalidation route
notify subscribers: either (A) change DynamicRegistry.updateResourceRead to also
bump the version for the `state://${name}` resource when swapping the callback,
or (B) call an explicit notifying method (e.g. dynamicRegistry.bumpVersion or
dynamicRegistry.notifyVersionChange) for `state://${name}` from
useStoreResource's subscribe handlers (both the one at
dynamicRegistry.updateResourceRead call around readState and the second
occurrence at lines 93-96) so that getVersion()/subscribe() observers see the
update. Ensure you update the implementation and both subscribe callbacks to use
the notifying path so consumers observing DynamicRegistry.getVersion/subscribe
are triggered.
- Around line 111-116: The execute function is serializing the raw return value
of action(...) without awaiting it, so async actions become empty objects when
JSON.stringify is called; update execute (in useStoreResource.ts) to await the
action call result (handle both Array args and single arg) before building the
CallToolResult so the resolved value is serialized, and ensure any thrown errors
are propagated or caught consistently.
---
Duplicate comments:
In `@libs/react/src/api/useApiClient.ts`:
- Around line 21-23: The hook is ignoring ApiClientOptions.server so server
scoping is a no-op; update useApiClient to apply options.server when registering
tools/operations with the context registry by reading options.server and
forwarding it into the registration calls (e.g., when you call
dynamicRegistry.registerTool / registerOperation or similar) or by obtaining a
server-scoped registry (e.g., dynamicRegistry.forServer(server)) before
registering operations; ensure useApiClient (and any helper like
registerOperationsInRegistry) uses the server value so the public
ApiClientOptions.server actually controls registration scope and remove any
misleading unused parameter handling.
In `@libs/react/src/components/mcpComponent.tsx`:
- Around line 151-157: The hook registers tools by the shared "name" which lets
multiple mounted instances overwrite/unregister each other; update
useDynamicTool to generate and pass a unique instance key (e.g.,
instanceId/symbol/UUID) alongside name into the registry and ensure the
registry's register/unregister APIs accept that instance key (or implement
ref-counting) so cleanup only removes the specific instance; change the
call-site in useDynamicTool usage (the block passing name, description, schema,
execute, server) to include the new instance identifier and update the
corresponding register/unregister logic in the registry to use the instance key
or ref-count instead of deleting by the shared name.
In `@libs/react/src/state/useStoreResource.ts`:
- Line 100: The cleanup callback in useStoreResource currently uses concise
arrow callbacks that implicitly return the result of fn(), triggering the lint
rule; update the two cleanup invocations (the return cleanup at the end of
useStoreResource and the other cleanup at the line noted) to use block-bodied
arrow functions so they call fn() without returning a value (e.g., replace (fn)
=> fn() with (fn) => { fn(); }) to satisfy
lint/suspicious/useIterableCallbackReturn.
---
Nitpick comments:
In `@libs/react/src/api/parseOpenApiSpec.ts`:
- Around line 33-36: The fallback operationId generation in parseOpenApiSpec
(where rawOperationId -> operationId using
`${method}_${path.replace(/[^a-zA-Z0-9]/g, '_')}`) can produce duplicates;
update parseOpenApiSpec to track generated IDs (e.g., a Set or Map) and
de-duplicate by appending a deterministic suffix (like _1, _2, ...) when a
collision is detected, ensuring you still prefer a string rawOperationId when
present and update any references to operationId accordingly (use the same
collision logic for both computed and normalized names).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27a9f75b-90bc-477f-b6ab-5343ca258a67
📒 Files selected for processing (25)
apps/e2e/demo-e2e-react/e2e/browser/mcp-component-table.pw.spec.tsapps/e2e/demo-e2e-react/e2e/browser/mcp-component.pw.spec.tsapps/e2e/demo-e2e-react/e2e/browser/tool-listing.pw.spec.tsapps/e2e/demo-e2e-react/playwright.config.tsapps/e2e/demo-e2e-react/src/App.tsxapps/e2e/demo-e2e-react/src/main.tsxapps/e2e/demo-e2e-react/src/sections/DynamicTool.tsxapps/e2e/demo-e2e-react/src/sections/StoreAdapterSection.tsxlibs/react/src/api/__tests__/useApiClient.spec.tsxlibs/react/src/api/createFetchClient.tslibs/react/src/api/parseOpenApiSpec.tslibs/react/src/api/useApiClient.tslibs/react/src/components/__tests__/mcpComponent.spec.tsxlibs/react/src/components/index.tslibs/react/src/components/mcpComponent.tsxlibs/react/src/hooks/__tests__/useDynamicToolZod.spec.tsxlibs/react/src/index.tslibs/react/src/registry/DynamicRegistry.tslibs/react/src/registry/__tests__/DynamicRegistry.spec.tslibs/react/src/state/__tests__/useStoreRegistration.spec.tsxlibs/react/src/state/__tests__/useStoreResource.spec.tsxlibs/react/src/state/useStoreRegistration.tslibs/react/src/state/useStoreResource.tslibs/react/src/state/useValtioResource.tstsconfig.base.json
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/e2e/demo-e2e-react/src/sections/DynamicTool.tsx
- apps/e2e/demo-e2e-react/src/main.tsx
- tsconfig.base.json
- libs/react/src/components/index.ts
- libs/react/src/components/tests/mcpComponent.spec.tsx
- apps/e2e/demo-e2e-react/src/sections/StoreAdapterSection.tsx
- libs/react/src/api/createFetchClient.ts
- apps/e2e/demo-e2e-react/e2e/browser/mcp-component.pw.spec.ts
- apps/e2e/demo-e2e-react/e2e/browser/tool-listing.pw.spec.ts
- libs/react/src/api/tests/useApiClient.spec.tsx
- apps/e2e/demo-e2e-react/e2e/browser/mcp-component-table.pw.spec.ts
…and operationId deduplication
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
libs/react/src/state/useStoreRegistration.ts (1)
19-23: 🛠️ Refactor suggestion | 🟠 MajorUse the RFC3986 metadata validator instead of regex-only checks.
The store and selector guards here still rely on custom regexes and custom error strings, so this path stays out of sync with the required URI-validation contract for SDK metadata. As per coding guidelines:
Validate URIs per RFC 3986 at metadata level using schema validation with message 'URI must have a valid scheme (e.g., file://, https://, custom://)'.Also applies to: 64-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/state/useStoreRegistration.ts` around lines 19 - 23, Replace the custom regex check in validateStoreName with an RFC3986 URI validation step: instead of testing VALID_NAME_RE in validateStoreName(name: string) (and the similar guard used around lines 64–67), validate the name against an RFC3986-compliant URI validator or schema and throw an Error with the exact message "URI must have a valid scheme (e.g., file://, https://, custom://)" when invalid; update the thrown Error in validateStoreName and the other guard to use the standardized message and ensure the validation library/predicate checks for a valid URI scheme per RFC3986 rather than relying on the regex.libs/react/src/state/useStoreResource.ts (2)
71-94:⚠️ Potential issue | 🟠 MajorValidate selector keys before composing resource URIs.
useStoreRegistrationalready rejects invalid selector keys, but the publicuseStoreResourcepath doesn't. Keys containing/, whitespace, or?/#will register unexpectedstate://${name}/${key}identities here while the batch API rejects them, so the two hooks no longer share the same contract. As per coding guidelines:Validate URIs per RFC 3986 at metadata level using schema validation with message 'URI must have a valid scheme (e.g., file://, https://, custom://)'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/state/useStoreResource.ts` around lines 71 - 94, The selector keys used to build URIs in useStoreResource are not validated, so before composing uri = `state://${name}/${key}` (and before pushing to selectorUris or calling dynamicRegistry.registerResource), validate each selector key (same rules as useStoreRegistration) against an allowed pattern (reject characters like '/', whitespace, '?' and '#'); if a key is invalid, throw or reject with a clear validation error (e.g., message per guidelines: "URI must have a valid scheme (e.g., file://, https://, custom://)") or skip registration and log, ensuring selectorRef/readSelector logic and selectorUris/dynamicRegistry.registerResource are only used for validated keys.
17-18:⚠️ Potential issue | 🟠 Major
serveris still a silent no-op in the public hook.
StoreResourceOptionsinlibs/react/src/state/state.types.tsdocumentsserver?: stringas "Target a specific named server", but this hook never reads it and every registration in this file still goes through the ambientdynamicRegistry. Callers can passserverand still publish to the default/global registry. Either wire it through or remove the option until it is supported.Also applies to: 43-49, 88-95, 130-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/state/useStoreResource.ts` around lines 17 - 18, The public hook useStoreResource ignores the optional server field from StoreResourceOptions and always uses the ambient dynamicRegistry; update useStoreResource to read options.server and route operations to the correct registry: compute a targetRegistry = server ? dynamicRegistry.get(server) : dynamicRegistry (or use the registry lookup method provided by dynamicRegistry) and replace all uses of dynamicRegistry in registration/subscribe/publish calls (the registration logic around registerResource/subscribe/actions/selectors within useStoreResource) to use targetRegistry so callers that pass server target the intended registry.
🧹 Nitpick comments (2)
libs/react/src/api/__tests__/parseOpenApiSpec.spec.ts (1)
158-702: Add regression tests for unsafe keys and cross-location name collisions.Please add cases for parameter names like
__proto__and forpath:id+query:idin one operation. Current suite is strong, but these edge cases will prevent silent schema corruption regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/api/__tests__/parseOpenApiSpec.spec.ts` around lines 158 - 702, Add two regression tests in parseOpenApiSpec.spec.ts using parseOpenApiSpec: one that supplies a parameter named "__proto__" (e.g., in query) and asserts ops[0].inputSchema.properties has a key "__proto__" and that parseOpenApiSpec returns normally (no prototype pollution); and another that defines both a path parameter and a query parameter with the same name (e.g., name: "id" in path and name: "id" in query) for the same operation and asserts the result preserves both parameter definitions semantically (e.g., properties contains the parameter name and ops[0].inputSchema.required includes the path param) so the parser doesn’t silently lose or corrupt one of them; use the existing helpers and checks (parseOpenApiSpec, ops[0].inputSchema.properties, ops[0].inputSchema.required) to implement these assertions.libs/react/src/registry/__tests__/DynamicRegistry.spec.ts (1)
605-611: Don't cement emptyclear()notifications as public behavior.In
libs/react/src/registry/DynamicRegistry.ts,clear()currently callsnotify()unconditionally, so this assertion makes a no-op clear bumpversionand wake subscribers even when nothing changed. That is an avoidable rerender path for any consumer keyed offsubscribe()/getVersion(). Prefer treating emptyclear()as a no-op and asserting that instead.♻️ Suggested expectation change
registry.clear(); - // Still notifies even when empty (no conditional guard) - expect(listener).toHaveBeenCalledTimes(1); + expect(listener).not.toHaveBeenCalled(); + expect(registry.getVersion()).toBe(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/registry/__tests__/DynamicRegistry.spec.ts` around lines 605 - 611, The test and implementation currently treat DynamicRegistry.clear() as always calling notify(), causing version bumps and subscriber wakeups even when registry is already empty; change DynamicRegistry.clear() so it checks whether the registry contains any entries (or whether version/state would change) and only calls notify() and increments version when there is an actual removal, and update the test expectation in DynamicRegistry.spec.ts to assert that clearing an already-empty registry is a no-op (i.e., listener is not called and getVersion() does not change). Ensure you modify the logic in the DynamicRegistry.clear() method and reference subscribe(), notify(), clear(), getVersion(), and the DynamicRegistry class so the behavior matches the new no-op semantics.
🤖 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/api/parseOpenApiSpec.ts`:
- Around line 75-81: The loop over paramMap flattens parameters by param.name
and can silently overwrite same-name params from different locations; update the
keying logic that assigns into properties and required to include the parameter
location (e.g., use param.name combined with param.in or a deterministic
separator) or otherwise detect duplicates and throw; adjust references where
properties and required are later consumed to expect the new composite key (or
normalize back) and ensure you still copy param.schema/param.description and set
required based on param.required (symbols: paramMap, properties, required,
param.name, param.in, param.schema, param.description, param.required).
- Around line 54-80: The properties object is vulnerable to prototype pollution
when using untrusted param names from OpenApiParameter; change the properties
container to a prototype-less object (e.g., create via Object.create(null)) and
skip or reject dangerous parameter names (e.g., "__proto__", "constructor",
"prototype") before writing to properties or pushing to required; update the
logic around paramMap iteration where you set properties[param.name] and
required.push(param.name) to first validate/sanitize param.name and only then
assign into the prototype-less properties map.
In `@libs/react/src/registry/__tests__/DynamicRegistry.spec.ts`:
- Around line 46-53: The cleanup returned from
DynamicRegistry.registerTool/registerResource is not idempotent and calling it
twice incorrectly decrements shared ref counts; update the implementations
(methods: registerTool, registerResource) to return an idempotent cleanup (e.g.,
closure with a called boolean that only calls unregisterTool/unregisterResource
once) and add regression tests in DynamicRegistry.spec.ts that call the returned
cleanup twice and assert the registration remains removed after the first call
and that a second call has no additional effect; mirror the same idempotent test
for registerResource/unregisterResource using createToolDef/createResourceDef
and registry.hasTool/hasResource assertions.
---
Duplicate comments:
In `@libs/react/src/state/useStoreRegistration.ts`:
- Around line 19-23: Replace the custom regex check in validateStoreName with an
RFC3986 URI validation step: instead of testing VALID_NAME_RE in
validateStoreName(name: string) (and the similar guard used around lines 64–67),
validate the name against an RFC3986-compliant URI validator or schema and throw
an Error with the exact message "URI must have a valid scheme (e.g., file://,
https://, custom://)" when invalid; update the thrown Error in validateStoreName
and the other guard to use the standardized message and ensure the validation
library/predicate checks for a valid URI scheme per RFC3986 rather than relying
on the regex.
In `@libs/react/src/state/useStoreResource.ts`:
- Around line 71-94: The selector keys used to build URIs in useStoreResource
are not validated, so before composing uri = `state://${name}/${key}` (and
before pushing to selectorUris or calling dynamicRegistry.registerResource),
validate each selector key (same rules as useStoreRegistration) against an
allowed pattern (reject characters like '/', whitespace, '?' and '#'); if a key
is invalid, throw or reject with a clear validation error (e.g., message per
guidelines: "URI must have a valid scheme (e.g., file://, https://, custom://)")
or skip registration and log, ensuring selectorRef/readSelector logic and
selectorUris/dynamicRegistry.registerResource are only used for validated keys.
- Around line 17-18: The public hook useStoreResource ignores the optional
server field from StoreResourceOptions and always uses the ambient
dynamicRegistry; update useStoreResource to read options.server and route
operations to the correct registry: compute a targetRegistry = server ?
dynamicRegistry.get(server) : dynamicRegistry (or use the registry lookup method
provided by dynamicRegistry) and replace all uses of dynamicRegistry in
registration/subscribe/publish calls (the registration logic around
registerResource/subscribe/actions/selectors within useStoreResource) to use
targetRegistry so callers that pass server target the intended registry.
---
Nitpick comments:
In `@libs/react/src/api/__tests__/parseOpenApiSpec.spec.ts`:
- Around line 158-702: Add two regression tests in parseOpenApiSpec.spec.ts
using parseOpenApiSpec: one that supplies a parameter named "__proto__" (e.g.,
in query) and asserts ops[0].inputSchema.properties has a key "__proto__" and
that parseOpenApiSpec returns normally (no prototype pollution); and another
that defines both a path parameter and a query parameter with the same name
(e.g., name: "id" in path and name: "id" in query) for the same operation and
asserts the result preserves both parameter definitions semantically (e.g.,
properties contains the parameter name and ops[0].inputSchema.required includes
the path param) so the parser doesn’t silently lose or corrupt one of them; use
the existing helpers and checks (parseOpenApiSpec,
ops[0].inputSchema.properties, ops[0].inputSchema.required) to implement these
assertions.
In `@libs/react/src/registry/__tests__/DynamicRegistry.spec.ts`:
- Around line 605-611: The test and implementation currently treat
DynamicRegistry.clear() as always calling notify(), causing version bumps and
subscriber wakeups even when registry is already empty; change
DynamicRegistry.clear() so it checks whether the registry contains any entries
(or whether version/state would change) and only calls notify() and increments
version when there is an actual removal, and update the test expectation in
DynamicRegistry.spec.ts to assert that clearing an already-empty registry is a
no-op (i.e., listener is not called and getVersion() does not change). Ensure
you modify the logic in the DynamicRegistry.clear() method and reference
subscribe(), notify(), clear(), getVersion(), and the DynamicRegistry class so
the behavior matches the new no-op semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a7da0c7-5153-4908-8ff3-a442e82f183e
📒 Files selected for processing (12)
apps/e2e/demo-e2e-react/vite.config.tslibs/react/src/api/__tests__/parseOpenApiSpec.spec.tslibs/react/src/api/parseOpenApiSpec.tslibs/react/src/api/useApiClient.tslibs/react/src/components/index.tslibs/react/src/components/mcpComponent.tsxlibs/react/src/registry/DynamicRegistry.tslibs/react/src/registry/__tests__/DynamicRegistry.spec.tslibs/react/src/state/__tests__/useStoreRegistration.spec.tsxlibs/react/src/state/__tests__/useStoreResource.spec.tsxlibs/react/src/state/useStoreRegistration.tslibs/react/src/state/useStoreResource.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- libs/react/src/components/index.ts
- libs/react/src/components/mcpComponent.tsx
- libs/react/src/registry/DynamicRegistry.ts
- libs/react/src/state/tests/useStoreRegistration.spec.tsx
- libs/react/src/api/useApiClient.ts
- libs/react/src/state/tests/useStoreResource.spec.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
libs/react/src/api/parseOpenApiSpec.ts (1)
75-82:⚠️ Potential issue | 🟠 MajorPrevent silent parameter collisions across different parameter locations.
properties[param.name]flattens by name only, so valid specs with bothpath:idandquery:idsilently overwrite one another. This drops schema detail and can corrupt required semantics. Please detect and reject ambiguity (or namespace keys) before assignment. Downstream impact: the “last write wins” expectation inlibs/react/src/api/__tests__/parseOpenApiSpec.spec.ts(Line 612-631) should be updated after this fix.💡 Proposed fix (ambiguity guard)
const required: string[] = []; + const locationByName = new Map<string, string>(); @@ for (const param of paramMap.values()) { if (param.name === '__proto__' || param.name === 'constructor' || param.name === 'prototype') continue; + const seenLocation = locationByName.get(param.name); + if (seenLocation && seenLocation !== param.in) { + throw new Error( + `Ambiguous parameter "${param.name}" appears in both "${seenLocation}" and "${param.in}".`, + ); + } + locationByName.set(param.name, param.in); + properties[param.name] = { ...(param.schema ?? { type: 'string' }), description: param.description, }; if (param.required) required.push(param.name); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/api/parseOpenApiSpec.ts` around lines 75 - 82, The loop in parseOpenApiSpec that assigns properties[param.name] from paramMap currently collapses parameters with the same name across different locations; update the logic in the paramMap iteration to detect ambiguity before assignment: before setting properties[param.name] check if an entry already exists for that name and if the existing parameter's "in" (location) or effective schema/required semantics differ from the current param, throw a descriptive error indicating the conflicting parameter name and locations (e.g., from paramMap and existing entry) so ambiguous path vs query/header params are rejected; ensure you still populate required[] only for unambiguous params and reference the paramMap, properties, and required symbols when making the change so tests can be updated accordingly.libs/react/src/provider/FrontMcpProvider.tsx (1)
182-207:⚠️ Potential issue | 🔴 CriticalFix the
listTools()fallback shape and extraction logic to prevent dropping server tools.
client.listTools()returns{ tools: ToolInfo[] }, not a bare array. The current fallback[]and castas ToolInfo[]cause the success path to failArray.isArray(baseTools), dropping all server tools from the registry after connection.Suggested fix
const [toolsResult, resourcesResult, templatesResult, promptsResult] = await Promise.all([ - safeList(() => client.listTools(), []), + safeList(() => client.listTools(), { tools: [] }), safeList(() => client.listResources(), { resources: [] }), safeList(() => client.listResourceTemplates(), { resourceTemplates: [] }), safeList(() => client.listPrompts(), { prompts: [] }), ]); if (mountedRef.current) { // Merge dynamic tools/resources into the initial listing const dynamicTools = dynamicRegistry.getTools().map((t) => ({ name: t.name, description: t.description, inputSchema: t.inputSchema, })); const dynamicResources = dynamicRegistry.getResources().map((r) => ({ uri: r.uri, name: r.name, description: r.description, mimeType: r.mimeType, })); - const baseTools = toolsResult as ToolInfo[]; + const baseTools = (toolsResult as { tools?: ToolInfo[] }).tools ?? []; const dynamicToolNames = new Set(dynamicTools.map((t) => t.name)); - const filteredBaseTools = (Array.isArray(baseTools) ? baseTools : []).filter( - (t) => !dynamicToolNames.has(t.name), - ); + const filteredBaseTools = baseTools.filter((t) => !dynamicToolNames.has(t.name));🤖 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 182 - 207, The tools fallback and extraction are wrong: safeList(() => client.listTools(), []) uses a bare array but client.listTools() returns { tools: ToolInfo[] }, so toolsResult is an object and the code casts it to ToolInfo[] causing Array.isArray(baseTools) to fail and drop server tools; change the fallback to match the shape (e.g., { tools: [] }) and update the extraction to pull the array from toolsResult.tools (use a variable like baseTools = Array.isArray(toolsResult?.tools) ? toolsResult.tools : []) before filtering against dynamicRegistry.getTools().
🧹 Nitpick comments (1)
libs/react/src/ai/__tests__/useTools.spec.tsx (1)
50-51: Use a stableDynamicRegistryin the wrapper.
dynamicRegistryandgetDynamicRegistry()now point at different objects. That diverges from the realFrontMcpContextcontract, so hooks that resolve registries viagetDynamicRegistry()can silently miss registrations or lose them on rerender. The same wrapper pattern shows up in the other updated spec files too.♻️ Suggested wrapper shape
function createWrapper(overrides?: { tools?: ToolInfo[]; status?: string; name?: string }) { const name = overrides?.name ?? 'default'; + const registries = new Map<string | undefined, DynamicRegistry>(); + const getDynamicRegistry = (server?: string) => { + const key = server ?? name; + const existing = registries.get(key); + if (existing) return existing; + const registry = new DynamicRegistry(); + registries.set(key, registry); + return registry; + }; const defaultTools: ToolInfo[] = [ { name: 'greet', description: 'Greet', inputSchema: { type: 'object', properties: {} } }, ]; @@ const ctx: FrontMcpContextValue = { name, registry: new ComponentRegistry(), - dynamicRegistry: new DynamicRegistry(), - getDynamicRegistry: () => new DynamicRegistry(), + dynamicRegistry: getDynamicRegistry(), + getDynamicRegistry, connect: jest.fn(), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/react/src/ai/__tests__/useTools.spec.tsx` around lines 50 - 51, The wrapper is creating two different DynamicRegistry instances so dynamicRegistry and getDynamicRegistry() diverge from the real FrontMcpContext; fix by instantiating a single DynamicRegistry (e.g., const registry = new DynamicRegistry()) and return that same instance for both dynamicRegistry and getDynamicRegistry() so hooks that call getDynamicRegistry() see the same registry used by dynamicRegistry.
🤖 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/__tests__/useAITools.spec.tsx`:
- Around line 43-44: The test wrappers currently create a new DynamicRegistry on
every getDynamicRegistry() call which breaks shared dynamic registrations; fix
by instantiating a single const dynamicRegistry = new DynamicRegistry() once per
wrapper/context before building the ctx and then set both dynamicRegistry:
dynamicRegistry and getDynamicRegistry: () => dynamicRegistry so the same
registry instance is reused across calls (update each wrapper that constructs
ctx to follow this pattern).
In `@libs/react/src/hooks/__tests__/useListTools.spec.tsx`:
- Around line 27-28: The test seeds a DynamicRegistry instance but
getDynamicRegistry() currently returns a fresh new DynamicRegistry() each call,
causing the hook under test to see a different empty registry; fix by creating a
single DynamicRegistry instance (reuse the same object as dynamicRegistry) and
have getDynamicRegistry return that same instance instead of constructing a new
one so both dynamicRegistry and getDynamicRegistry() reference the same
DynamicRegistry.
In `@libs/react/src/hooks/__tests__/useReadResource.spec.tsx`:
- Around line 49-50: The test stub creates dynamicRegistry: new
DynamicRegistry() but getDynamicRegistry() returns a new instance each call,
causing callers to see different registries; change getDynamicRegistry to return
the same instance created for dynamicRegistry (e.g., have getDynamicRegistry
return the captured dynamicRegistry variable or otherwise return the shared
DynamicRegistry instance) so dynamicRegistry and getDynamicRegistry are aligned
and consumers observe the same registry.
In `@libs/react/src/hooks/__tests__/useStoreResource.spec.tsx`:
- Around line 49-50: The getter getDynamicRegistry should return the same
DynamicRegistry instance used by the dynamicRegistry property instead of
creating a new one each call; update the implementation so getDynamicRegistry
returns the previously constructed DynamicRegistry (the same instance referenced
by ctx.dynamicRegistry) to keep ctx.dynamicRegistry and ctx.getDynamicRegistry()
consistent (referencing DynamicRegistry, ctx.dynamicRegistry, and
ctx.getDynamicRegistry()).
In `@libs/react/src/hooks/useComponentTree.ts`:
- Around line 67-69: In useComponentTree, validate the provided uri before any
dynamic resource registration: ensure uri (from the options destructure)
contains a valid scheme per RFC 3986 (e.g., starts with "<scheme>://") and
reject or throw with the exact message "URI must have a valid scheme (e.g.,
file://, https://, custom://)" if it does not; perform this check immediately
after extracting uri and before calling any server registration logic
(referencing useComponentTree, uri and server) so invalid URIs are never passed
downstream.
In `@libs/react/src/state/__tests__/useReduxResource.spec.tsx`:
- Around line 40-43: The lint error comes from the concise arrow callback in the
_setState implementation where listeners.forEach((l) => l()); returns the
callback result; change that callback to a block body to avoid returning a value
by writing listeners.forEach((l) => { l(); }); within the _setState function so
the listener invocation does not produce a returned value and satisfies the lint
rule; update the function body where _setState merges state and iterates
listeners (reference: _setState, listeners, and l).
In `@libs/react/src/state/__tests__/useValtioResource.spec.tsx`:
- Line 41: The forEach callback in the notify function returns a value due to
the concise arrow body; change notify so the callback uses a block body that
calls each listener without returning (i.e., replace listeners.forEach((l) =>
l()) with a block-bodied callback that calls l();). Update the notify function
(symbol: notify) and the forEach invocation on listeners to use a statement body
to satisfy the useIterableCallbackReturn rule.
In `@libs/react/src/state/useStoreResource.ts`:
- Around line 31-37: The code in useStoreResource builds an application/json
resource by doing JSON.stringify(getStateRef.current()), which yields undefined
for an uninitialized store or selector that returns undefined and results in
invalid JSON; change the serialization to explicitly convert undefined to a
valid JSON value (e.g., serialize undefined as null) when building the contents
text. Update the async resource factory inside useStoreResource (the block that
constructs ReadResourceResult with uri `state://${name}` and mimeType
'application/json') and the analogous block around lines 79-85 to use
JSON.stringify(value === undefined ? null : value) (or equivalent) so the
emitted resource always contains valid JSON text. Ensure you reference
getStateRef.current() (and any selector value variable used) when applying this
fix.
---
Duplicate comments:
In `@libs/react/src/api/parseOpenApiSpec.ts`:
- Around line 75-82: The loop in parseOpenApiSpec that assigns
properties[param.name] from paramMap currently collapses parameters with the
same name across different locations; update the logic in the paramMap iteration
to detect ambiguity before assignment: before setting properties[param.name]
check if an entry already exists for that name and if the existing parameter's
"in" (location) or effective schema/required semantics differ from the current
param, throw a descriptive error indicating the conflicting parameter name and
locations (e.g., from paramMap and existing entry) so ambiguous path vs
query/header params are rejected; ensure you still populate required[] only for
unambiguous params and reference the paramMap, properties, and required symbols
when making the change so tests can be updated accordingly.
In `@libs/react/src/provider/FrontMcpProvider.tsx`:
- Around line 182-207: The tools fallback and extraction are wrong: safeList(()
=> client.listTools(), []) uses a bare array but client.listTools() returns {
tools: ToolInfo[] }, so toolsResult is an object and the code casts it to
ToolInfo[] causing Array.isArray(baseTools) to fail and drop server tools;
change the fallback to match the shape (e.g., { tools: [] }) and update the
extraction to pull the array from toolsResult.tools (use a variable like
baseTools = Array.isArray(toolsResult?.tools) ? toolsResult.tools : []) before
filtering against dynamicRegistry.getTools().
---
Nitpick comments:
In `@libs/react/src/ai/__tests__/useTools.spec.tsx`:
- Around line 50-51: The wrapper is creating two different DynamicRegistry
instances so dynamicRegistry and getDynamicRegistry() diverge from the real
FrontMcpContext; fix by instantiating a single DynamicRegistry (e.g., const
registry = new DynamicRegistry()) and return that same instance for both
dynamicRegistry and getDynamicRegistry() so hooks that call getDynamicRegistry()
see the same registry used by dynamicRegistry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e033a59c-50eb-4568-9002-e24925117576
📒 Files selected for processing (34)
libs/react/src/ai/__tests__/useAITools.spec.tsxlibs/react/src/ai/__tests__/useTools.spec.tsxlibs/react/src/api/__tests__/parseOpenApiSpec.spec.tslibs/react/src/api/__tests__/useApiClient.spec.tsxlibs/react/src/api/parseOpenApiSpec.tslibs/react/src/api/useApiClient.tslibs/react/src/components/__tests__/AgentContent.spec.tsxlibs/react/src/components/__tests__/AgentSearch.spec.tsxlibs/react/src/components/__tests__/mcpComponent.spec.tsxlibs/react/src/hooks/__tests__/useCallTool.spec.tsxlibs/react/src/hooks/__tests__/useComponentTree.spec.tsxlibs/react/src/hooks/__tests__/useDynamicResource.spec.tsxlibs/react/src/hooks/__tests__/useDynamicToolZod.spec.tsxlibs/react/src/hooks/__tests__/useFrontMcp.spec.tsxlibs/react/src/hooks/__tests__/useGetPrompt.spec.tsxlibs/react/src/hooks/__tests__/useListPrompts.spec.tsxlibs/react/src/hooks/__tests__/useListResources.spec.tsxlibs/react/src/hooks/__tests__/useListTools.spec.tsxlibs/react/src/hooks/__tests__/useReadResource.spec.tsxlibs/react/src/hooks/__tests__/useResolvedServer.spec.tsxlibs/react/src/hooks/__tests__/useStoreResource.spec.tsxlibs/react/src/hooks/useComponentTree.tslibs/react/src/hooks/useDynamicResource.tslibs/react/src/hooks/useDynamicTool.tslibs/react/src/provider/FrontMcpContext.tslibs/react/src/provider/FrontMcpProvider.tsxlibs/react/src/provider/__tests__/FrontMcpProvider.spec.tsxlibs/react/src/registry/DynamicRegistry.tslibs/react/src/registry/__tests__/DynamicRegistry.spec.tslibs/react/src/state/__tests__/useReduxResource.spec.tsxlibs/react/src/state/__tests__/useStoreResource.spec.tsxlibs/react/src/state/__tests__/useValtioResource.spec.tsxlibs/react/src/state/useStoreResource.tslibs/react/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- libs/react/src/api/useApiClient.ts
- libs/react/src/components/tests/AgentSearch.spec.tsx
- libs/react/src/hooks/tests/useDynamicResource.spec.tsx
- libs/react/src/components/tests/AgentContent.spec.tsx
- libs/react/src/hooks/useDynamicResource.ts
- libs/react/src/hooks/tests/useComponentTree.spec.tsx
- libs/react/src/provider/FrontMcpContext.ts
- libs/react/src/api/tests/useApiClient.spec.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores