feat: add reinitialization support for terminated sessions and improve data injection in weather widget#325
Merged
frontegg-david merged 4 commits intorelease/1.0.xfrom Mar 31, 2026
Merged
Conversation
…e data injection in weather widget
Contributor
📝 WalkthroughWalkthroughRefactors weather tool UI into external templates and exports types; implements dual-pattern resource argument completers (convention + override) with unit and E2E tests; changes session re-initialization to unmark terminated sessions and add transport re-registration; updates bridge IIFE/handshake and UI bundling/runtime behaviors; CI matrix and various tests updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Framework as Completion Framework
participant Resource as Resource Instance
participant Service as Catalog/DI Service
Client->>Framework: request getArgumentCompleter(argName)
alt Convention completer exists
Framework->>Resource: detect prototype argNameCompleter
Framework->>Service: resolve DI
Framework->>Resource: instantiate ResourceContext
Resource->>Framework: call argNameCompleter(partial)
Resource-->>Framework: return { values, total }
else Override completer exists
Framework->>Resource: detect getArgumentCompleter override
Framework->>Service: resolve DI
Framework->>Resource: instantiate ResourceContext
Resource->>Framework: call instance.getArgumentCompleter(argName)
Resource-->>Framework: return completer result
else No completer
Framework-->>Client: return null
end
Framework-->>Client: return completer function or null
sequenceDiagram
participant Client
participant HTTP as HTTP Flow
participant Notification as NotificationService
participant Transport as Transporter
participant Session as SessionStore
Client->>HTTP: POST /initialize with mcp-session-id (terminated)
HTTP->>Notification: isTerminated(sessionId)?
Notification-->>HTTP: true
HTTP->>Notification: unmarkTerminated(sessionId)
Notification-->>HTTP: removed
HTTP->>HTTP: set request[ServerRequestTokens.reinitialize]=true
HTTP->>Transport: resetForReinitialization()
Transport->>Session: clear local session state
HTTP->>Transport: reregisterServer()
Transport->>Notification: register(server, sessionId)
Notification-->>Transport: registered
HTTP-->>Client: 200 OK (same sessionId)
sequenceDiagram
participant Browser
participant HTML as Page HTML
participant IIFE as Bridge IIFE
participant Host as Parent Window
Browser->>HTML: load widget page
HTML->>IIFE: check globalThis.FrontMcpBridge
alt existing bridge
IIFE-->>HTML: reuse bridge instance
else no bridge
HTML->>IIFE: execute generated IIFE (install bridge)
IIFE->>Browser: inject loading indicator in head
end
Browser->>Browser: DOMContentLoaded
IIFE->>Host: postMessage ui/initialize (handshake)
Host-->>IIFE: handshake response
IIFE->>Host: post ui/notifications/initialized
IIFE-->>HTML: bridge ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 14
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 (1)
libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (1)
358-372:⚠️ Potential issue | 🟠 MajorMake re-registration atomic with the reinitialize handshake.
transport.reregisterServer()runs beforetransport.initialize(). If the initialize call then throws, the terminated session is left re-registered even though the reconnect failed, which can skew later DELETE/notification behavior. Please add a rollback on the failure path, or move the registration into the transport once the initialize handshake has actually succeeded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/transport/flows/handle.streamable-http.flow.ts` around lines 358 - 372, The current sequence calls transport.reregisterServer() before awaiting transport.initialize(), leaving the server registered if initialize throws; update the flow so registration is atomic with the reinitialize handshake by either moving the reregisterServer() call into the success path of transport.initialize() or adding a rollback on failure (call the inverse unregister/unregisterServer method) when initialize rejects; specifically, adjust the block that checks ServerRequestTokens.reinitialize to call transport.resetForReinitialization() as now but defer transport.reregisterServer() until after await transport.initialize(request, response) succeeds, or wrap await transport.initialize(...) in try/catch and on catch call transport.unregisterServer()/the appropriate unregister method to undo transport.reregisterServer() and rethrow the error.
🟡 Minor comments (7)
libs/sdk/src/scope/flows/__tests__/http.request.reconnect.spec.ts-19-31 (1)
19-31:⚠️ Potential issue | 🟡 MinorThe reconnect helper never verifies the
reinitializehandoff.These tests now model
unmarkTerminated(), but they still don't set/assertrequest[ServerRequestTokens.reinitialize]. That's the flag the downstream reconnect flow uses to runresetForReinitialization()andreregisterServer(), so this suite can stay green while the actual reconnect path regresses.🧪 Tighten the helper contract
}): { action: 'allowed' | 'blocked-404'; sessionCleared: boolean; headerCleared: boolean; sessionIdTokenCleared: boolean; + reinitializeTokenSet: boolean; } { const { isTerminated, requestBody, authorization, requestHeaders, unmarkTerminated } = params; const request: Record<string | symbol, unknown> = { headers: requestHeaders, body: requestBody, @@ if (body?.method === 'initialize') { // Initialize with terminated session — unmark and allow through. // Session refs kept intact so parseInput can reuse the session ID. unmarkTerminated?.(authorization.session.id); + request[ServerRequestTokens.reinitialize] = true; return { action: 'allowed', sessionCleared: false, headerCleared: false, sessionIdTokenCleared: false, + reinitializeTokenSet: true, }; } else { return { action: 'blocked-404', sessionCleared: false, headerCleared: false, sessionIdTokenCleared: false, + reinitializeTokenSet: false, }; } } } return { action: 'allowed', sessionCleared: false, headerCleared: false, sessionIdTokenCleared: false, + reinitializeTokenSet: false, }; }As per coding guidelines "
**/*.spec.ts: Test all code paths including error conditions and edge cases to achieve comprehensive coverage."Also applies to: 42-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/scope/flows/__tests__/http.request.reconnect.spec.ts` around lines 19 - 31, The helper simulateTerminatedSessionCheck currently models unmarkTerminated but never sets or asserts the reinitialization handoff, so update the helper to set request[ServerRequestTokens.reinitialize] (or the equivalent token used by the downstream flow) when unmarkTerminated is invoked and add assertions that request[ServerRequestTokens.reinitialize] is set/cleared as appropriate; ensure tests covering the reconnect path assert that resetForReinitialization() and reregisterServer() would be triggered by checking the reinitialize flag, and add expectations in the spec to validate request[ServerRequestTokens.reinitialize] state after calling unmarkTerminated and after the simulated reconnect flow runs.apps/e2e/demo-e2e-transport-recreation/e2e/session-reconnect.e2e.spec.ts-507-522 (1)
507-522:⚠️ Potential issue | 🟡 MinorAssert the concurrent state, not just three 200s.
This block can stay green even if one increment is lost, because the only checks are the HTTP statuses. Add an assertion on the returned counter payloads or a final read of the session state after
Promise.all.As per coding guidelines "Test all code paths including error conditions and edge cases to achieve comprehensive coverage".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-transport-recreation/e2e/session-reconnect.e2e.spec.ts` around lines 507 - 522, The test "should handle concurrent requests on a valid session" currently only asserts HTTP 200s; update it to verify the concurrent state by checking returned counter payloads from sendToolCall (e.g., inspect r1/r2/r3 response bodies for expected counter values) or perform a final read of the session state after Promise.all using the existing sessionId (e.g., call the session read/get endpoint) and assert the counter reflects the sum of increments (1+2+3) and that each individual response matches expected intermediate values; modify the assertions around sendToolCall responses (and/or add a final state check) to confirm no increment was lost.apps/e2e/demo-e2e-transport-recreation/e2e/session-reconnect.e2e.spec.ts-370-374 (1)
370-374:⚠️ Potential issue | 🟡 MinorDon't let this invalid-session test pass on 5xx responses.
toBeGreaterThanOrEqual(400)will also accept 500/503, so a server regression still looks green here. If both client-error branches are acceptable, assert the explicit set instead.As per coding guidelines "Test all code paths including error conditions and edge cases to achieve comprehensive coverage".🎯 Suggested assertion
- expect(status).toBeGreaterThanOrEqual(400); + expect([400, 404]).toContain(status);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-transport-recreation/e2e/session-reconnect.e2e.spec.ts` around lines 370 - 374, The test "should return 404 for non-initialize with invalid session ID" currently uses expect(status).toBeGreaterThanOrEqual(400) which will accept 5xx and hide server regressions; change the assertion to explicitly allow only the expected client errors (400 or 404) instead—locate the test and replace the broad toBeGreaterThanOrEqual(400) with an explicit membership/assertion (e.g., assert status is one of [400, 404]) for the response returned by sendToolsList.libs/uipack/src/component/__tests__/renderer.spec.ts-135-135 (1)
135-135:⚠️ Potential issue | 🟡 MinorAssertion is now too permissive for mount wiring verification.
At Line 135, checking only
'Widget'can pass from the component declaration alone, even if mount code stops rendering the export correctly.🔧 Suggested tightening
- expect(buildCall.stdin.contents).toContain('Widget'); + expect(buildCall.stdin.contents).toMatch( + /(React\.createElement\(\s*Widget\b|__h\(\s*Widget\b)/, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/uipack/src/component/__tests__/renderer.spec.ts` at line 135, The current assertion using buildCall.stdin.contents.toContain('Widget') is too permissive; change it to assert the actual mount wiring is present by checking for the mount invocation string emitted by the renderer (e.g., assert buildCall.stdin.contents contains the specific mount call such as "mount('root', Widget)" or the framework-specific render call like "ReactDOM.render(" or "createRoot(...).render(" depending on the renderer used). Update the assertion in renderer.spec.ts to look for that exact mount invocation instead of the generic 'Widget'.libs/sdk/src/resource/__tests__/resource.instance.completer.spec.ts-62-242 (1)
62-242:⚠️ Potential issue | 🟡 MinorAdd coverage for function-backed resources too.
ResourceInstance.getArgumentCompleter()now has a dedicated null-return path forResourceKind.FUNCTION/ResourceTemplateKind.FUNCTION, but this suite only exercises class-backed records. A regression in that new guard would currently slip through.As per coding guidelines "Test all code paths including error conditions and edge cases to achieve comprehensive coverage".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/resource/__tests__/resource.instance.completer.spec.ts` around lines 62 - 242, Add a unit test that covers the function-backed-resource path so the new null-return guard in ResourceInstance.getArgumentCompleter is exercised: create a function-style resource template/record (set kind to ResourceKind.FUNCTION / ResourceTemplateKind.FUNCTION or use whatever helper produces a function-backed record), call normalizeResourceTemplate to get the record, instantiate new ResourceInstance(record, providers, createMockOwner()), await instance.ready, then assert instance.getArgumentCompleter('anyArg') returns null; this new test should be added alongside the existing completer specs (e.g., in the same describe block) to validate the dedicated FUNCTION path.apps/e2e/demo-e2e-ui/e2e/widget-rendering.e2e.spec.ts-38-46 (1)
38-46:⚠️ Potential issue | 🟡 MinorAvoid non-null assertions; use proper error handling instead.
The file contains multiple non-null assertions (
!) that hide nullability issues:
resource.text()!at lines 38, 55, 71, 84, 96, 109mapMatch![1]at lines 46, 101For E2E tests, prefer optional chaining with explicit checks before use:
Safer pattern example
-const html = resource.text()!; +const html = resource.text(); +expect(html).toBeDefined(); +if (!html) throw new Error('HTML content required'); const mapMatch = html.match(/<script type="importmap">([\s\S]*?)<\/script>/); expect(mapMatch).toBeTruthy(); -const importMap = JSON.parse(mapMatch![1]); +const mapJson = mapMatch?.[1]; +if (!mapJson) throw new Error('Import map payload required'); +const importMap = JSON.parse(mapJson);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-ui/e2e/widget-rendering.e2e.spec.ts` around lines 38 - 46, The tests currently use non-null assertions on resource.text() and mapMatch (e.g., resource.text()!, const html, mapMatch![1]) which can mask null/undefined failures; update the spec to use optional chaining and explicit checks: call resource.text() and assign to html, assert html is truthy (or fail with a clear message) before using it, use html.match(...) and check mapMatch is truthy before accessing mapMatch[1] (or throw/expect a failure), then parse JSON only after these guards; apply the same pattern to the other occurrences of resource.text() and mapMatch in this spec so no `!` non-null assertions remain.apps/e2e/demo-e2e-ui/e2e/widget-rendering.e2e.spec.ts-128-133 (1)
128-133:⚠️ Potential issue | 🟡 MinorUse a concrete type for weather output assertions.
Line 128 casts
result.raw.structuredContenttoRecord<string, unknown>, which loses field-level type safety. Define a concrete type for the weather output to align with guidelines on strict MCP protocol typing.Suggested typing refinement
+type WeatherStructuredContent = { + location: string; + temperature: number; + conditions: string; + icon: string; +}; + -const data = result.raw.structuredContent as Record<string, unknown>; +const data = result.raw.structuredContent as WeatherStructuredContent;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-ui/e2e/widget-rendering.e2e.spec.ts` around lines 128 - 133, The test is casting result.raw.structuredContent to a loose Record<string, unknown>, losing field-level type safety; define a concrete interface (e.g., WeatherOutput with properties location: string, temperature: number, conditions: string, icon: string) and use that type for the structured content (replace the Record cast with a WeatherOutput assertion or generic) so the assertions target well-typed fields (reference result.raw.structuredContent and the local variable data in the test). Ensure the new WeatherOutput type is imported/declared near the test and update the expect checks to use the typed fields.
🧹 Nitpick comments (2)
apps/e2e/demo-e2e-resource-providers/src/apps/main/resources/category-products.resource.ts (1)
15-39: Use the concrete read-resource result type here.
ResourceContext<{ categoryName: string }>leavesOutasunknown, even thoughexecute()always returns a read-resource payload. Making that generic/return type explicit keeps this new resource path fully typed.As per coding guidelines "For MCP response types, use strict protocol types (e.g., GetPromptResult, ReadResourceResult); do not use
unknownfor protocol types".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-resource-providers/src/apps/main/resources/category-products.resource.ts` around lines 15 - 39, The resource currently leaves the output type as unknown by extending ResourceContext<{ categoryName: string }>; update the generic to the concrete MCP response type (e.g., ReadResourceResult) so the resource is fully typed: change the class generic to ResourceContext<{ categoryName: string }, ReadResourceResult>, update execute() to return Promise<ReadResourceResult> (instead of an implicit unknown), and ensure ReadResourceResult is imported; keep categoryNameCompleter as-is returning ResourceCompletionResult.apps/e2e/demo-e2e-ui/e2e/widget-rendering.e2e.spec.ts (1)
158-173: Guard client cleanup withtry/finallyin the discovery test.If an assertion fails before Line 172, the client may stay open and affect subsequent tests.
🔧 Suggested lifecycle-safe structure
test('should have ui.resourceUri in tools/list metadata', async ({ server }) => { const client = await server.createClient({ transport: 'streamable-http', clientInfo: { name: 'ChatGPT', version: '1.0.0' }, }); - - const tools = await client.tools.list(); - const tool = tools.find((t) => t.name === 'react-weather'); - expect(tool).toBeDefined(); - - const uiMeta = tool?._meta?.['ui'] as Record<string, unknown> | undefined; - expect(uiMeta).toBeDefined(); - expect(uiMeta?.resourceUri).toBe('ui://widget/react-weather.html'); - - await client.disconnect(); + try { + const tools = await client.tools.list(); + const tool = tools.find((t) => t.name === 'react-weather'); + expect(tool).toBeDefined(); + + const uiMeta = tool?._meta?.['ui'] as Record<string, unknown> | undefined; + expect(uiMeta).toBeDefined(); + expect(uiMeta?.resourceUri).toBe('ui://widget/react-weather.html'); + } finally { + await client.disconnect(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-ui/e2e/widget-rendering.e2e.spec.ts` around lines 158 - 173, The test "should have ui.resourceUri in tools/list metadata" can leave the created client open if an assertion fails; wrap the client usage in a try/finally so client.disconnect() always runs: after creating the client with server.createClient(...) run the assertions inside a try block and call await client.disconnect() inside a finally block (guarding that client is defined) to ensure cleanup even on failure.
🤖 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/demo/src/apps/weather/tools/get-weather.ui-2.tsx`:
- Around line 1-7: The Card component currently uses an implicit any for props;
define an explicit props type (e.g., interface CardProps { title?: string;
subtitle?: string; children?: React.ReactNode }) and update the Card declaration
to use that type and destructure the props (following the Badge pattern), for
example by typing Card as React.FC<CardProps> or annotating the parameter as
(props: CardProps) and then using { title, subtitle, children } inside the
component so the file complies with strict TypeScript.
In `@apps/demo/src/apps/weather/tools/get-weather.ui.tsx`:
- Around line 38-40: The Refresh button currently hardcodes getWeather({
location: 'San Francisco' }) causing any widget to reset to SF; change the
onClick to call getWeather with the current location (e.g., use output.location
or the component's location state) and only fall back to a sensible default if
that value is missing. Update the button's handler so it reads the existing
location from output.location (or the local prop/state that holds the displayed
city) and passes that into getWeather({ location }) instead of the literal 'San
Francisco'.
In `@apps/e2e/demo-e2e-resource-providers/e2e/resource-completion.e2e.spec.ts`:
- Around line 24-27: The JSON-RPC request ID generation uses Date.now() which
can collide under parallel requests; replace this with a stable incrementing
counter used for each call to mcp.raw.request (e.g., a module-scoped let
requestId = 0 and use ++requestId whenever building the request) so each
JSON-RPC call (method 'completion/complete') has a unique, monotonically
increasing id; update all places that build the request payload to reference
this counter instead of Date.now() (search for mcp.raw.request and the jsonrpc
id usage) so concurrent tests produce distinct IDs.
- Around line 18-24: The requestCompletion helper currently uses loose typing
and Date.now() IDs; update the function (requestCompletion) to replace the `any`
types with explicit request/response interfaces (e.g., CompletionRequest and
CompletionResponse or a shared ProtocolRequest/ProtocolResponse shape) and type
mcp.raw.request accordingly so TypeScript validates the payload and return
shape, and replace the Date.now() ID generation with a module-scoped monotonic
counter (e.g., let nextRequestId = 1) that you increment (nextRequestId++) to
produce unique numeric IDs before sending the request to avoid collisions under
concurrent tests; ensure you update usages of the id field in both the request
object and response typing to match the new types.
In `@apps/e2e/demo-e2e-transport-recreation/e2e/session-reconnect.e2e.spec.ts`:
- Around line 124-137: sendSseGet opens an SSE connection but never closes the
response stream, leaking TCP sockets; modify sendSseGet (the async function
sendSseGet) to keep the fetched Response in an outer-scope variable and in the
finally block explicitly close the stream (e.g., if (response?.body) { try {
response.body.getReader()?.cancel(); } catch {} } or call response.body.cancel()
depending on runtime) before clearing the timeout so the underlying connection
is freed after each call.
In `@libs/sdk/src/scope/flows/http.request.flow.ts`:
- Around line 311-319: The code currently calls
this.scope.notifications.unmarkTerminated(sessionId) before the
initialize/reinitialize flow completes; move that call so it executes only after
the initialize/onInitialize path completes successfully (i.e., in the
post-initialize success branch), leaving
request[ServerRequestTokens.reinitialize] = true and the log intact before
initialization; ensure parseInput still reuses the session ID and that no other
code path clears the terminated guard on error so terminatedSessions remains
protected if initialize fails.
In `@libs/sdk/src/transport/flows/handle.streamable-http.flow.ts`:
- Around line 240-275: The initialize fast-path in
handle.streamable-http.flow.ts bypasses validateMcpSessionHeader and treats
malformed mcp-session-id headers as missing, causing silently starting fresh
sessions; update the isInitialize branch to run the same header validation as
resolveStreamableHttpSession: call validateMcpSessionHeader (or reuse the logic
inside resolveStreamableHttpSession) on request.headers['mcp-session-id'] and if
validation fails return the error path (don’t fall through to createSession),
otherwise if validation succeeds and authorization.session?.id is present reuse
it, else call createSession(); ensure validateMcpSessionHeader and
resolveStreamableHttpSession are referenced so the behavior matches
non-initialize flows.
In `@libs/ui/src/react/hooks/context.tsx`:
- Around line 106-121: The current logic sets loading based solely on
hasExisting which makes ready true even if existingBridge.initialized is false;
change the initial loading state to consider initialization
(useState(!hasExisting || !existingBridge?.initialized)), and in the useEffect
branch where you handle existingBridge.initialized === true call
setLoading(false) and setBridge(existingBridge) (and invoke onReady), and in the
'bridge:ready' handler also call setLoading(false) before/after setBridge so the
hook's ready flag only becomes true when the bridge is actually initialized;
update references to bridge/existingBridge, the useState for loading, and the
handler in the useEffect accordingly.
In `@libs/uipack/src/bridge-runtime/iife-generator.ts`:
- Around line 495-500: The callTool implementation currently always sends
'tools/call' which the current ext-apps host doesn't handle; change callTool
(referencing callTool, hostCapabilities.serverTools,
hostCapabilities.serverToolProxy, and sendRequest) to choose the RPC method
based on host capability: if this.hostCapabilities.serverTools is true use
'tools/call', otherwise fall back to 'ui/callServerTool'; keep the same payload
(name and arguments/args) shape when calling sendRequest so existing callers
(and serverToolProxy consumers) continue to work.
- Around line 459-466: The capability mapping in the pending request resolve
function (self.pendingRequests[id].resolve) overwrites self.hostCapabilities and
sets self.capabilities using only serverTools/openLinks, which breaks backward
compatibility because older hosts still advertise serverToolProxy and openLink;
update the mapping in that resolve block (where self.hostCapabilities is read
and self.capabilities is set) to consider both new and legacy keys (e.g., treat
serverToolProxy as equivalent to serverTools, and openLink as equivalent to
openLinks) so canCallTools and canOpenLinks remain true for legacy hosts before
the ext-apps migration.
In `@libs/uipack/src/component/transpiler.ts`:
- Around line 75-76: The mounted widget receives a static window.__mcpToolInput
so it never updates; change the input prop passed to the component inside
McpBridgeProvider to use the bridge/runtime context value (e.g., the runtime's
toolInput state) or a reactive value that is updated from the bridge's
"tool:input" and "tool:input-partial" events. Locate the JSX where
McpBridgeProvider renders ${componentName} (in transpiler.ts) and replace input:
window.__mcpToolInput with the runtime-backed input (for example input:
bridgeContext.toolInput or a local state variable updated by subscribing to the
bridge events) so the component receives live updates. Ensure the subscription
updates that state and is cleaned up appropriately.
- Around line 72-76: The current __hasData and render logic treat only non-empty
objects as loaded and use "output || null", which discards valid falsy payloads
(0, false, '', [], null) and keeps loading stuck; update __hasData to return
true for any non-undefined value that is not strictly undefined (accept
primitives and arrays, e.g., check v !== undefined and for objects allow empty
arrays) and stop coercing output with "output || null" so __render passes the
actual output through to the component (McpBridgeProvider / ${componentName})
and compute loading as the negation of the improved __hasData(output); apply the
same changes in the other occurrences around lines 80-94.
- Around line 117-130: The code in transpiler.ts uses require('fs') (nodeFs) to
check paths (nodeFs.existsSync) when building the candidates/alias mapping
(candidates, uiEsmBase, subpaths, mjs, alias); replace direct fs access with the
approved helpers from `@frontmcp/utils` (import the appropriate exists/existsSync
or stat helper) and call that instead of nodeFs.existsSync for all existence
checks, keeping the rest of the logic (nodePath.join, candidates loop, alias
assignment) identical; ensure you remove the require('fs') and use the
`@frontmcp/utils` API consistently for these filesystem operations.
In `@libs/uipack/src/shell/__tests__/data-injector.spec.ts`:
- Around line 3-42: The tests miss an XSS regression check for user-controlled
toolName in buildDataInjectionScript: add a unit test in data-injector.spec.ts
that calls buildDataInjectionScript with a malicious toolName (e.g., containing
"</script>" or JS payload) and assert the returned string does not contain the
raw malicious substring and that the injected window.__mcpToolName value is
safely escaped/serialized (or appears quoted/encoded), ensuring the payload
cannot break out of the surrounding <script> tag; locate the function
buildDataInjectionScript in the same test file to add this assertion.
---
Outside diff comments:
In `@libs/sdk/src/transport/flows/handle.streamable-http.flow.ts`:
- Around line 358-372: The current sequence calls transport.reregisterServer()
before awaiting transport.initialize(), leaving the server registered if
initialize throws; update the flow so registration is atomic with the
reinitialize handshake by either moving the reregisterServer() call into the
success path of transport.initialize() or adding a rollback on failure (call the
inverse unregister/unregisterServer method) when initialize rejects;
specifically, adjust the block that checks ServerRequestTokens.reinitialize to
call transport.resetForReinitialization() as now but defer
transport.reregisterServer() until after await transport.initialize(request,
response) succeeds, or wrap await transport.initialize(...) in try/catch and on
catch call transport.unregisterServer()/the appropriate unregister method to
undo transport.reregisterServer() and rethrow the error.
---
Minor comments:
In `@apps/e2e/demo-e2e-transport-recreation/e2e/session-reconnect.e2e.spec.ts`:
- Around line 507-522: The test "should handle concurrent requests on a valid
session" currently only asserts HTTP 200s; update it to verify the concurrent
state by checking returned counter payloads from sendToolCall (e.g., inspect
r1/r2/r3 response bodies for expected counter values) or perform a final read of
the session state after Promise.all using the existing sessionId (e.g., call the
session read/get endpoint) and assert the counter reflects the sum of increments
(1+2+3) and that each individual response matches expected intermediate values;
modify the assertions around sendToolCall responses (and/or add a final state
check) to confirm no increment was lost.
- Around line 370-374: The test "should return 404 for non-initialize with
invalid session ID" currently uses expect(status).toBeGreaterThanOrEqual(400)
which will accept 5xx and hide server regressions; change the assertion to
explicitly allow only the expected client errors (400 or 404) instead—locate the
test and replace the broad toBeGreaterThanOrEqual(400) with an explicit
membership/assertion (e.g., assert status is one of [400, 404]) for the response
returned by sendToolsList.
In `@apps/e2e/demo-e2e-ui/e2e/widget-rendering.e2e.spec.ts`:
- Around line 38-46: The tests currently use non-null assertions on
resource.text() and mapMatch (e.g., resource.text()!, const html, mapMatch![1])
which can mask null/undefined failures; update the spec to use optional chaining
and explicit checks: call resource.text() and assign to html, assert html is
truthy (or fail with a clear message) before using it, use html.match(...) and
check mapMatch is truthy before accessing mapMatch[1] (or throw/expect a
failure), then parse JSON only after these guards; apply the same pattern to the
other occurrences of resource.text() and mapMatch in this spec so no `!`
non-null assertions remain.
- Around line 128-133: The test is casting result.raw.structuredContent to a
loose Record<string, unknown>, losing field-level type safety; define a concrete
interface (e.g., WeatherOutput with properties location: string, temperature:
number, conditions: string, icon: string) and use that type for the structured
content (replace the Record cast with a WeatherOutput assertion or generic) so
the assertions target well-typed fields (reference result.raw.structuredContent
and the local variable data in the test). Ensure the new WeatherOutput type is
imported/declared near the test and update the expect checks to use the typed
fields.
In `@libs/sdk/src/resource/__tests__/resource.instance.completer.spec.ts`:
- Around line 62-242: Add a unit test that covers the function-backed-resource
path so the new null-return guard in ResourceInstance.getArgumentCompleter is
exercised: create a function-style resource template/record (set kind to
ResourceKind.FUNCTION / ResourceTemplateKind.FUNCTION or use whatever helper
produces a function-backed record), call normalizeResourceTemplate to get the
record, instantiate new ResourceInstance(record, providers, createMockOwner()),
await instance.ready, then assert instance.getArgumentCompleter('anyArg')
returns null; this new test should be added alongside the existing completer
specs (e.g., in the same describe block) to validate the dedicated FUNCTION
path.
In `@libs/sdk/src/scope/flows/__tests__/http.request.reconnect.spec.ts`:
- Around line 19-31: The helper simulateTerminatedSessionCheck currently models
unmarkTerminated but never sets or asserts the reinitialization handoff, so
update the helper to set request[ServerRequestTokens.reinitialize] (or the
equivalent token used by the downstream flow) when unmarkTerminated is invoked
and add assertions that request[ServerRequestTokens.reinitialize] is set/cleared
as appropriate; ensure tests covering the reconnect path assert that
resetForReinitialization() and reregisterServer() would be triggered by checking
the reinitialize flag, and add expectations in the spec to validate
request[ServerRequestTokens.reinitialize] state after calling unmarkTerminated
and after the simulated reconnect flow runs.
In `@libs/uipack/src/component/__tests__/renderer.spec.ts`:
- Line 135: The current assertion using
buildCall.stdin.contents.toContain('Widget') is too permissive; change it to
assert the actual mount wiring is present by checking for the mount invocation
string emitted by the renderer (e.g., assert buildCall.stdin.contents contains
the specific mount call such as "mount('root', Widget)" or the
framework-specific render call like "ReactDOM.render(" or
"createRoot(...).render(" depending on the renderer used). Update the assertion
in renderer.spec.ts to look for that exact mount invocation instead of the
generic 'Widget'.
---
Nitpick comments:
In
`@apps/e2e/demo-e2e-resource-providers/src/apps/main/resources/category-products.resource.ts`:
- Around line 15-39: The resource currently leaves the output type as unknown by
extending ResourceContext<{ categoryName: string }>; update the generic to the
concrete MCP response type (e.g., ReadResourceResult) so the resource is fully
typed: change the class generic to ResourceContext<{ categoryName: string },
ReadResourceResult>, update execute() to return Promise<ReadResourceResult>
(instead of an implicit unknown), and ensure ReadResourceResult is imported;
keep categoryNameCompleter as-is returning ResourceCompletionResult.
In `@apps/e2e/demo-e2e-ui/e2e/widget-rendering.e2e.spec.ts`:
- Around line 158-173: The test "should have ui.resourceUri in tools/list
metadata" can leave the created client open if an assertion fails; wrap the
client usage in a try/finally so client.disconnect() always runs: after creating
the client with server.createClient(...) run the assertions inside a try block
and call await client.disconnect() inside a finally block (guarding that client
is defined) to ensure cleanup even on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0556a25-1a08-4e86-a0fe-600986dfb0db
📒 Files selected for processing (38)
apps/demo/src/apps/weather/tools/get-weather.tool.tsxapps/demo/src/apps/weather/tools/get-weather.ui-2.tsxapps/demo/src/apps/weather/tools/get-weather.ui.tsxapps/e2e/demo-e2e-resource-providers/e2e/resource-completion.e2e.spec.tsapps/e2e/demo-e2e-resource-providers/src/apps/main/index.tsapps/e2e/demo-e2e-resource-providers/src/apps/main/providers/catalog.provider.tsapps/e2e/demo-e2e-resource-providers/src/apps/main/resources/category-products.resource.tsapps/e2e/demo-e2e-resource-providers/src/apps/main/resources/plain-template.resource.tsapps/e2e/demo-e2e-resource-providers/src/apps/main/resources/product-detail.resource.tsapps/e2e/demo-e2e-transport-recreation/e2e/session-reconnect.e2e.spec.tsapps/e2e/demo-e2e-ui/e2e/widget-rendering.e2e.spec.tsapps/e2e/demo-e2e-ui/src/apps/widgets/index.tsapps/e2e/demo-e2e-ui/src/apps/widgets/tools/react-weather.tool.tsapps/e2e/demo-e2e-ui/src/apps/widgets/tools/react-weather.ui.tsxlibs/sdk/src/common/interfaces/resource.interface.tslibs/sdk/src/common/tokens/server.tokens.tslibs/sdk/src/notification/notification.service.tslibs/sdk/src/resource/__tests__/resource.instance.completer.spec.tslibs/sdk/src/resource/resource.instance.tslibs/sdk/src/scope/flows/__tests__/http.request.reconnect.spec.tslibs/sdk/src/scope/flows/http.request.flow.tslibs/sdk/src/tool/ui/ui-shared.tslibs/sdk/src/transport/adapters/transport.local.adapter.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/transport/transport.local.tslibs/sdk/src/transport/transport.remote.tslibs/sdk/src/transport/transport.types.tslibs/ui/src/react/hooks/context.tsxlibs/ui/src/react/hooks/tools.tsxlibs/uipack/src/adapters/template-renderer.tslibs/uipack/src/bridge-runtime/__tests__/iife-generator.spec.tslibs/uipack/src/bridge-runtime/iife-generator.tslibs/uipack/src/component/__tests__/renderer.spec.tslibs/uipack/src/component/__tests__/transpiler.spec.tslibs/uipack/src/component/transpiler.tslibs/uipack/src/shell/__tests__/builder.spec.tslibs/uipack/src/shell/__tests__/data-injector.spec.tslibs/uipack/src/shell/data-injector.ts
💤 Files with no reviewable changes (1)
- libs/ui/src/react/hooks/tools.tsx
apps/e2e/demo-e2e-resource-providers/e2e/resource-completion.e2e.spec.ts
Show resolved
Hide resolved
apps/e2e/demo-e2e-resource-providers/e2e/resource-completion.e2e.spec.ts
Show resolved
Hide resolved
apps/e2e/demo-e2e-transport-recreation/e2e/session-reconnect.e2e.spec.ts
Show resolved
Hide resolved
…to reduce API rate limits
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 101 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-03-31T05:21:33.831Z |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
libs/uipack/src/component/transpiler.ts (2)
117-136: 🛠️ Refactor suggestion | 🟠 MajorKeep
uipackfilesystem access behind@frontmcp/utils.This alias discovery path now uses
require('fs')andexistsSyncdirectly. Please route these existence checks through the approved@frontmcp/utilsfilesystem helpers instead of raw Node FS calls. As per coding guidelines "Use@frontmcp/utilsfor all file system operations (readFile, writeFile, mkdir, etc.); never usefs/promisesornode:fsdirectly".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/uipack/src/component/transpiler.ts` around lines 117 - 136, The alias discovery block uses raw Node fs (nodeFs.existsSync) and require('fs')—replace those direct FS calls with the approved helpers from `@frontmcp/utils`: import the path helper (or keep require('path') for nodePath) and import the filesystem existence helper (e.g., fileExists/existsSync) from '@frontmcp/utils', then use that helper in place of nodeFs.existsSync when checking uiEsmBase and each mjs; update references in the loop that set alias[`@frontmcp/ui/${sub}`] to only use the results of the utils helper and preserve the current try/catch behavior and existing variables (nodePath, candidates, uiEsmBase, subpaths, mjs, alias).
72-94:⚠️ Potential issue | 🟠 MajorTreat
nullas a completed result.Line 81 still uses
nullas the loading sentinel, and Lines 72, 76, 86, and 94 still classifynullas “no data”. A tool that legitimately returnsnullwill never clearloading, and the component still can’t distinguish “loaded with null” from “not loaded yet”.Suggested fix
- function __hasData(v) { return v !== undefined && v !== null; } + function __hasData(v) { return v !== undefined; } function __render(output) { __reactRoot.render( __h(McpBridgeProvider, null, - __h(${componentName}, { output: output !== undefined && output !== null ? output : null, input: window.__mcpToolInput, loading: !__hasData(output) }) + __h(${componentName}, { output: output === undefined ? null : output, input: window.__mcpToolInput, loading: !__hasData(output) }) ) ); } // Render immediately (component shows loading state until data arrives) - __render(null); + __render(undefined); // 1. Try OpenAI SDK (toolOutput set synchronously or after load) if (typeof window !== 'undefined') { if (!window.openai) window.openai = {}; var __cur = window.openai.toolOutput; - if (__hasData(__cur)) { __render(__cur); } + if (__cur !== undefined) { __render(__cur); } Object.defineProperty(window.openai, 'toolOutput', { get: function() { return __cur; }, set: function(v) { __cur = v; __render(v); }, configurable: true, enumerable: true }); } // 2. Try injected data globals - if (__hasData(window.__mcpToolOutput)) { __render(window.__mcpToolOutput); } + if (window.__mcpToolOutput !== undefined) { __render(window.__mcpToolOutput); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/uipack/src/component/transpiler.ts` around lines 72 - 94, The code treats null as “no data”; change the sentinel to undefined by updating __hasData to return v !== undefined (so null is a valid completed result), change the initial render call from __render(null) to __render(undefined) so the component shows loading until a value (including null) is set, and adjust the prop handling in __render to use output !== undefined ? output : null for the output prop while keeping loading as !__hasData(output); apply these changes for all checks that use __hasData (including the window.openai.toolOutput getter/setter and the window.__mcpToolOutput check) so null is treated as a real result.
🧹 Nitpick comments (3)
apps/e2e/demo-e2e-resource-providers/e2e/resource-completion.e2e.spec.ts (2)
48-53: Fail fast instead of defaulting malformed success responses to empty values.At Line 52,
?? { values: [] }can hide protocol-shape regressions in tests that expect empty completions. Prefer throwing whenresponse.erroris absent butresult.completion.valuesis missing.Suggested change
if (response.error) { throw new Error(`Completion error: ${JSON.stringify(response.error)}`); } - return response.result?.completion ?? { values: [] }; + const completion = response.result?.completion; + if (!completion || !Array.isArray(completion.values)) { + throw new Error('Malformed completion response: missing result.completion.values'); + } + return completion; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-resource-providers/e2e/resource-completion.e2e.spec.ts` around lines 48 - 53, Replace the silent fallback that returns `{ values: [] }` when the protocol shape is malformed: after the existing `if (response.error) ...` check, assert that `response.result`, `response.result.completion`, and `response.result.completion.values` are present and valid; if any are missing or not an array, throw an Error that includes `JSON.stringify(response)` (or a descriptive message) so tests fail fast instead of returning a default empty completion; finally return `response.result.completion` when the shape is valid.
141-147: Consider loosening the fixture-size assertion to reduce brittleness.Line 145 (
toBeGreaterThan(10)) tightly couples this test to catalog seed size; benign fixture changes can fail this test without breaking completion behavior.Suggested change
- expect(result.values.length).toBeGreaterThan(10); + expect(result.values.length).toBeGreaterThan(0); + expect(result.values).toContain('laptop'); expect(result.hasMore).toBe(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-resource-providers/e2e/resource-completion.e2e.spec.ts` around lines 141 - 147, The assertion that result.values.length is greater than 10 in the 'should return all products for empty productName partial' test is brittle; change the check on requestCompletion(mcp, 'catalog://{categoryName}/products/{productName}', 'productName', '') to assert a len > 0 (e.g., expect(result.values.length).toBeGreaterThan(0) or toBeGreaterThanOrEqual(1)) so the test verifies non-empty completion without tying it to a specific seed size, leaving the expect(result.hasMore).toBe(false) assertion as-is..github/workflows/push.yml (1)
160-165: Consider using a more portable jq chunking pattern.The
_nwisefunction works but is a semi-internal jq function (underscore prefix suggests it's not part of the stable public API). While it's available in jq 1.5+, a more explicit approach improves portability:Alternative chunking approach
# Chunk projects into groups of 4 to reduce API rate limit pressure CHUNK_SIZE=4 - CHUNKS=$(echo $PROJECTS_JSON | jq -c "[_nwise($CHUNK_SIZE)] | to_entries | map({index: .key, projects: .value})") + CHUNKS=$(echo $PROJECTS_JSON | jq -c '[range(0; length; '"$CHUNK_SIZE"') as $i | .[$i:$i+'"$CHUNK_SIZE"']] | to_entries | map({index: .key, projects: .value})')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/push.yml around lines 160 - 165, The current chunking uses jq's semi-internal _nwise which can be unstable; update the CHUNKS assignment so it uses a portable explicit chunking expression instead of _nwise (keep the same CHUNK_SIZE, PROJECTS_JSON, CHUNKS and CHUNK_COUNT variables). Replace CHUNKS=$(echo $PROJECTS_JSON | jq -c "[_nwise($CHUNK_SIZE)] | to_entries | map({index: .key, projects: .value})") with a jq pipeline that splits the input array into slices of length CHUNK_SIZE (for example by using range, . as an array, and slicing with .[start:start+CHUNK_SIZE] inside a map or reducerequivalent) so the rest of the script (CHUNK_COUNT and echo "chunks=$CHUNKS") continues to work with an array of {index, projects} entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/perf.yml:
- Around line 81-88: The workflow step currently uses `yarn nx run
"$project":test:perf || true`, which hides failures; change this so test
failures are not silently swallowed by either removing `|| true` (so the step
fails on the first failing project) or implement explicit failure tracking:
capture each `yarn nx run "$project":test:perf` exit code, increment a failure
counter when non-zero, continue running the remaining projects, and at the end
`exit` with a non-zero status if the counter > 0; refer to the step name "Run
performance tests (chunk ${{ matrix.chunk.index }})" and the `yarn nx run
"$project":test:perf` invocation when updating the script.
In @.github/workflows/push.yml:
- Around line 320-325: The workflow mixes actions/download-artifact@v5 with
actions/upload-artifact@v6 which can cause incompatibilities; update the
download and upload action usages to the same major version (pick one, e.g.,
change actions/download-artifact@v5 to actions/download-artifact@v6 or downgrade
upload to `@v5`) so all artifact steps use the same major release; locate the
"Download E2E coverage artifacts" step (pattern: e2e-coverage-chunk-*, path:
coverage/e2e/) plus the other artifact steps referenced (the steps at the other
artifact locations mentioned in the review) and make the version change
consistently for actions/download-artifact and actions/upload-artifact across
the file.
In `@libs/uipack/src/component/transpiler.ts`:
- Around line 96-103: The code currently renders bridge results twice (via
__bridge.onToolResult => __render(data) and via window 'tool:result' listener)
and also collapses valid falsy payloads; fix by using a single source of truth
and explicit undefined checks: remove or no-op one of the render paths (either
stop calling __render(data) inside __bridge.onToolResult or skip rendering in
the 'tool:result' handler when the bridge has already delivered the payload),
and in the 'tool:result' listener replace the loose `d.structuredContent ||
d.content || d` with explicit checks like `if (d.structuredContent !==
undefined) __render(d.structuredContent); else if (d.content !== undefined)
__render(d.content); else __render(d);` so falsy but valid values (0, false, '')
are preserved; reference symbols: __bridge, onToolResult, __render, and the
window.addEventListener('tool:result') handler.
---
Duplicate comments:
In `@libs/uipack/src/component/transpiler.ts`:
- Around line 117-136: The alias discovery block uses raw Node fs
(nodeFs.existsSync) and require('fs')—replace those direct FS calls with the
approved helpers from `@frontmcp/utils`: import the path helper (or keep
require('path') for nodePath) and import the filesystem existence helper (e.g.,
fileExists/existsSync) from '@frontmcp/utils', then use that helper in place of
nodeFs.existsSync when checking uiEsmBase and each mjs; update references in the
loop that set alias[`@frontmcp/ui/${sub}`] to only use the results of the utils
helper and preserve the current try/catch behavior and existing variables
(nodePath, candidates, uiEsmBase, subpaths, mjs, alias).
- Around line 72-94: The code treats null as “no data”; change the sentinel to
undefined by updating __hasData to return v !== undefined (so null is a valid
completed result), change the initial render call from __render(null) to
__render(undefined) so the component shows loading until a value (including
null) is set, and adjust the prop handling in __render to use output !==
undefined ? output : null for the output prop while keeping loading as
!__hasData(output); apply these changes for all checks that use __hasData
(including the window.openai.toolOutput getter/setter and the
window.__mcpToolOutput check) so null is treated as a real result.
---
Nitpick comments:
In @.github/workflows/push.yml:
- Around line 160-165: The current chunking uses jq's semi-internal _nwise which
can be unstable; update the CHUNKS assignment so it uses a portable explicit
chunking expression instead of _nwise (keep the same CHUNK_SIZE, PROJECTS_JSON,
CHUNKS and CHUNK_COUNT variables). Replace CHUNKS=$(echo $PROJECTS_JSON | jq -c
"[_nwise($CHUNK_SIZE)] | to_entries | map({index: .key, projects: .value})")
with a jq pipeline that splits the input array into slices of length CHUNK_SIZE
(for example by using range, . as an array, and slicing with
.[start:start+CHUNK_SIZE] inside a map or reducerequivalent) so the rest of the
script (CHUNK_COUNT and echo "chunks=$CHUNKS") continues to work with an array
of {index, projects} entries.
In `@apps/e2e/demo-e2e-resource-providers/e2e/resource-completion.e2e.spec.ts`:
- Around line 48-53: Replace the silent fallback that returns `{ values: [] }`
when the protocol shape is malformed: after the existing `if (response.error)
...` check, assert that `response.result`, `response.result.completion`, and
`response.result.completion.values` are present and valid; if any are missing or
not an array, throw an Error that includes `JSON.stringify(response)` (or a
descriptive message) so tests fail fast instead of returning a default empty
completion; finally return `response.result.completion` when the shape is valid.
- Around line 141-147: The assertion that result.values.length is greater than
10 in the 'should return all products for empty productName partial' test is
brittle; change the check on requestCompletion(mcp,
'catalog://{categoryName}/products/{productName}', 'productName', '') to assert
a len > 0 (e.g., expect(result.values.length).toBeGreaterThan(0) or
toBeGreaterThanOrEqual(1)) so the test verifies non-empty completion without
tying it to a specific seed size, leaving the expect(result.hasMore).toBe(false)
assertion as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: abb24bd3-e8ef-4d34-b259-d2ee61eff6f8
📒 Files selected for processing (10)
.github/workflows/perf.yml.github/workflows/push.ymlapps/demo/src/apps/weather/tools/get-weather.ui-2.tsxapps/demo/src/apps/weather/tools/get-weather.ui.tsxapps/e2e/demo-e2e-resource-providers/e2e/resource-completion.e2e.spec.tsapps/e2e/demo-e2e-transport-recreation/e2e/session-reconnect.e2e.spec.tslibs/sdk/src/skill/__tests__/read-skill-content.spec.tslibs/uipack/src/bridge-runtime/iife-generator.tslibs/uipack/src/component/transpiler.tslibs/uipack/src/shell/__tests__/data-injector.spec.ts
✅ Files skipped from review due to trivial changes (3)
- libs/sdk/src/skill/tests/read-skill-content.spec.ts
- apps/demo/src/apps/weather/tools/get-weather.ui-2.tsx
- apps/e2e/demo-e2e-transport-recreation/e2e/session-reconnect.e2e.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/demo/src/apps/weather/tools/get-weather.ui.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
libs/uipack/src/component/transpiler.ts (4)
72-76:⚠️ Potential issue | 🟠 MajorReserve
undefinedfor the pre-result state.Most falsy payloads are handled now, but
Line 72still treatsnullas “not loaded” andLine 81bootstraps withnull. A widget whose real result isnullwill stay stuck inloading=true, and the component still can’t distinguish “no result yet” from “loaded null”.Suggested fix
- function __hasData(v) { return v !== undefined && v !== null; } + function __hasData(v) { return v !== undefined; } function __render(output) { __reactRoot.render( __h(McpBridgeProvider, null, - __h(${componentName}, { output: output !== undefined && output !== null ? output : null, input: window.__mcpToolInput, loading: !__hasData(output) }) + __h(${componentName}, { output: output === undefined ? null : output, input: window.__mcpToolInput, loading: !__hasData(output) }) ) ); } // Render immediately (component shows loading state until data arrives) - __render(null); + __render(undefined);Also applies to: 81-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/uipack/src/component/transpiler.ts` around lines 72 - 76, The render logic treats null as "not loaded" which keeps widgets with a real null result stuck in loading; update the helper and render usage so undefined is the only pre-result sentinel: change __hasData to return v !== undefined (allow null as valid), and update the __render call (and the bootstrap code around the McpBridgeProvider / componentName usage at the later bootstrap block) to set loading based on output === undefined (or !(__hasData(output)) if you invert accordingly) while passing output: output === undefined ? null : output only where you intentionally want null preserved; adjust both the __render function and the bootstrap block (lines where McpBridgeProvider and componentName are used) to use this undefined-only convention.
76-76:⚠️ Potential issue | 🟠 MajorKeep
inputreactive instead of rereading the bootstrap global.
Line 76passeswindow.__mcpToolInputstraight through. This mount path never owns or updates that value, so any post-mount input change stays invisible until some unrelated output update happens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/uipack/src/component/transpiler.ts` at line 76, The mount call in transpiler.ts is passing window.__mcpToolInput directly to the component (in the __h(...) invocation), which prevents the component from observing later changes; replace the global read with the local reactive input reference used by the transpiler so the prop is tied to the reactive variable in scope (e.g., pass the local input identifier or a reactive getter instead of window.__mcpToolInput), ensuring the mount path owns/receives updates; update the __h call around the componentName prop list to use that local reactive input and remove the direct global access.
117-130: 🛠️ Refactor suggestion | 🟠 MajorUse the shared FS abstraction instead of
require('fs').
Line 119reintroduces direct Node filesystem access, andLines 126and130callnodeFs.existsSync(...). Inuipack, these existence checks should go through@frontmcp/utilsinstead of rawfs.As per coding guidelines "Use
@frontmcp/utilsfor all file system operations (readFile, writeFile, mkdir, etc.); never usefs/promisesornode:fsdirectly".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/uipack/src/component/transpiler.ts` around lines 117 - 130, The code is using require('fs') and nodeFs.existsSync for path existence checks; replace that with the shared filesystem abstraction from `@frontmcp/utils` (import the appropriate exists/existsSync helper) and call that helper instead of nodeFs.existsSync when checking candidates/uiEsmBase and mjs in the for-loops inside transpiler.ts (references: nodeFs, nodePath, candidates, uiEsmBase, subpaths, alias). Keep the same control flow and alias assignments, only swap the direct fs usage for the `@frontmcp/utils` FS helper.
96-104:⚠️ Potential issue | 🟠 MajorChoose one tool-result channel as the source of truth.
The explicit
!== undefinedunwrap onLine 103is good, butlibs/uipack/src/bridge-runtime/iife-generator.ts:383-388still emits bothonToolResult(params.content)andtool:resultfor the same notification. That meansLine 98renders rawcontent, thenLine 103immediately rerendersstructuredContent/content, so bridge-backed widgets can see two different output shapes for one response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/uipack/src/component/transpiler.ts` around lines 96 - 104, The code currently renders twice because both the bridge callback (__bridge.onToolResult) and the DOM CustomEvent ('tool:result' listener) invoke __render; change the logic so only one channel is the source of truth: if __bridge exists and typeof __bridge.onToolResult === 'function' register only __bridge.onToolResult(function(data){ __render(data); }) and skip adding window.addEventListener('tool:result', ...); otherwise keep the 'tool:result' listener; update places that emit both (e.g., iife-generator's emission of onToolResult(params.content) and dispatch of 'tool:result') to use a single path if needed.
🧹 Nitpick comments (1)
apps/e2e/demo-e2e-resource-providers/e2e/resource-completion.e2e.spec.ts (1)
195-211: Consider using optional chaining for TypeScript strictness.The assertions on lines 208-210 access
response.result.completionandresponse.result.completion.valueswithout TypeScript-level narrowing aftertoBeDefined(). While this works at runtime, it may produce TypeScript warnings in strict mode.♻️ Optional fix using optional chaining with non-null assertions
expect(response.error).toBeUndefined(); expect(response.result).toBeDefined(); - expect(response.result.completion).toBeDefined(); - expect(Array.isArray(response.result.completion.values)).toBe(true); - expect(response.result.completion.values).toContain('electronics'); + expect(response.result?.completion).toBeDefined(); + expect(Array.isArray(response.result!.completion!.values)).toBe(true); + expect(response.result!.completion!.values).toContain('electronics');Alternatively, extract and assert in a type-safe manner:
const completion = response.result?.completion; expect(completion).toBeDefined(); expect(Array.isArray(completion?.values)).toBe(true); expect(completion?.values).toContain('electronics');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-resource-providers/e2e/resource-completion.e2e.spec.ts` around lines 195 - 211, The assertions in the test "should return proper completion response shape" access response.result.completion and its values without TypeScript-level narrowing; update the test to safely narrow/extract the completion before further assertions (for example, assign const completion = response.result?.completion or use optional chaining) then call expect(completion).toBeDefined() followed by Array.isArray(completion?.values) and completion?.values assertions so TypeScript strict mode no longer complains about possible undefined accesses on response.result/completion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libs/uipack/src/component/transpiler.ts`:
- Around line 72-76: The render logic treats null as "not loaded" which keeps
widgets with a real null result stuck in loading; update the helper and render
usage so undefined is the only pre-result sentinel: change __hasData to return v
!== undefined (allow null as valid), and update the __render call (and the
bootstrap code around the McpBridgeProvider / componentName usage at the later
bootstrap block) to set loading based on output === undefined (or
!(__hasData(output)) if you invert accordingly) while passing output: output ===
undefined ? null : output only where you intentionally want null preserved;
adjust both the __render function and the bootstrap block (lines where
McpBridgeProvider and componentName are used) to use this undefined-only
convention.
- Line 76: The mount call in transpiler.ts is passing window.__mcpToolInput
directly to the component (in the __h(...) invocation), which prevents the
component from observing later changes; replace the global read with the local
reactive input reference used by the transpiler so the prop is tied to the
reactive variable in scope (e.g., pass the local input identifier or a reactive
getter instead of window.__mcpToolInput), ensuring the mount path owns/receives
updates; update the __h call around the componentName prop list to use that
local reactive input and remove the direct global access.
- Around line 117-130: The code is using require('fs') and nodeFs.existsSync for
path existence checks; replace that with the shared filesystem abstraction from
`@frontmcp/utils` (import the appropriate exists/existsSync helper) and call that
helper instead of nodeFs.existsSync when checking candidates/uiEsmBase and mjs
in the for-loops inside transpiler.ts (references: nodeFs, nodePath, candidates,
uiEsmBase, subpaths, alias). Keep the same control flow and alias assignments,
only swap the direct fs usage for the `@frontmcp/utils` FS helper.
- Around line 96-104: The code currently renders twice because both the bridge
callback (__bridge.onToolResult) and the DOM CustomEvent ('tool:result'
listener) invoke __render; change the logic so only one channel is the source of
truth: if __bridge exists and typeof __bridge.onToolResult === 'function'
register only __bridge.onToolResult(function(data){ __render(data); }) and skip
adding window.addEventListener('tool:result', ...); otherwise keep the
'tool:result' listener; update places that emit both (e.g., iife-generator's
emission of onToolResult(params.content) and dispatch of 'tool:result') to use a
single path if needed.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-resource-providers/e2e/resource-completion.e2e.spec.ts`:
- Around line 195-211: The assertions in the test "should return proper
completion response shape" access response.result.completion and its values
without TypeScript-level narrowing; update the test to safely narrow/extract the
completion before further assertions (for example, assign const completion =
response.result?.completion or use optional chaining) then call
expect(completion).toBeDefined() followed by Array.isArray(completion?.values)
and completion?.values assertions so TypeScript strict mode no longer complains
about possible undefined accesses on response.result/completion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7378d1b5-9af8-41b8-83aa-e9019047fe52
📒 Files selected for processing (4)
.github/workflows/perf.yml.github/workflows/push.ymlapps/e2e/demo-e2e-resource-providers/e2e/resource-completion.e2e.spec.tslibs/uipack/src/component/transpiler.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/perf.yml
…data checks in transpiler
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/uipack/src/component/transpiler.ts`:
- Around line 96-104: The bridge path currently only receives content via
__bridge.onToolResult and thus never sees params. Modify the
__bridge.onToolResult callback (the handler registered on __bridge via
onToolResult) to accept the full params object and pass it to __render with the
same resolution logic used for the CustomEvent (i.e., prefer
params.structuredContent, then params.content, then params), or alternatively
update the producer to call context.notifyToolResult(params) so onToolResult
receives the full params; ensure __render(...) is invoked with the resolved
value and keep the existing window.addEventListener('tool:result', ...) behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 468ed71e-03e8-4259-8d49-d2d355708225
📒 Files selected for processing (2)
apps/e2e/demo-e2e-resource-providers/e2e/resource-completion.e2e.spec.tslibs/uipack/src/component/transpiler.ts
Contributor
Cherry-pick CreatedA cherry-pick PR to Please review and merge if this change should also be in If the cherry-pick is not needed, close the PR. |
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
Bug Fixes
Tests