chore: Add e2e tests for mcp ui and enhance ui lib #129
Conversation
WalkthroughRemoves legacy auth and UI demo apps, consolidates UI demos under apps/e2e/demo-e2e-ui with many new E2E suites, and adds UI rendering into the tool-call flow plus a large UI subsystem (serving-mode, dual-payload, response-builder), dependency/template tooling, and a bundler/file-cache with storage backends. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant CallFlow as CallToolFlow
participant Detector as PlatformDetector
participant UIAdapter as ServingModeResolver
participant Renderer as ResponseBuilder
Client->>CallFlow: callTool(toolId, input, clientInfo)
CallFlow->>Detector: detectAIPlatform(clientInfo, capabilities)
Detector-->>CallFlow: platformType
CallFlow->>UIAdapter: resolveServingMode(toolMeta, platformType, configuredMode)
UIAdapter-->>CallFlow: { effectiveMode, useDualPayload, supportsUI }
alt supportsUI && hasHtmlContent
CallFlow->>Renderer: buildToolResponseContent(rawOutput, htmlContent, effectiveMode, useDualPayload, platformType, htmlPrefix)
alt useDualPayload
Renderer->>Renderer: buildDualPayload(data, html, prefix)
Renderer-->>CallFlow: dual-payload content
else widget/inline
Renderer-->>CallFlow: contentCleared (widget) or markdown fallback
end
else
CallFlow-->>Client: json-only response
end
CallFlow->>Client: finalize response (merged uiResult + _meta)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas to focus during review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
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)
apps/e2e/demo-e2e-ui/src/apps/widgets/tools/html-card.tool.ts (1)
42-61: The split data sourcing pattern is intentional but inconsistent with codebase conventions.The template sources
titlefromctx.outputandcontent/footerfromctx.input(lines 44-45). The inline comment at line 43 indicates this split is deliberate. However, this pattern is inconsistent with the codebase: 6 of 8 widget tools consolidate all template data to eitherctx.outputorctx.input. Onlymdx-interactive.tool.tsalso uses split sourcing.If this split is intentional for E2E test purposes, consider documenting why in a comment (as this tool already does). Otherwise, align the
execute()method to return all display fields in output for consistency with the majority pattern.
♻️ Duplicate comments (2)
libs/ui/src/dependency/template-processor.ts (2)
359-363: Same type assertion issue in convenience functions.The same unsafe cast of
context.inputtoRecord<string, unknown>appears in all three convenience functions. Apply the same fix as suggested forprocessTemplate.Also applies to: 386-390, 418-422
432-451: Duplicate escapeHtml implementation.This is the same
escapeHtmlimplementation duplicated fromprocessTemplate. Extracting to a shared utility would reduce duplication and ensure consistency.
🟡 Minor comments (7)
libs/ui/src/adapters/serving-mode.ts-256-265 (1)
256-265: Inconsistent capability check inplatformSupportsWidgets.This function hardcodes platform checks (
openai,ext-apps,cursor) instead of using thePLATFORM_CAPABILITIESmap. This creates a maintenance risk where the map could be updated but this function wouldn't reflect those changes.Additionally, the function documentation mentions "platforms can read HTML from _meta['ui/html']", but according to
PLATFORM_CAPABILITIES, other platforms likeclaude,continue,cody, andgeneric-mcpalso havesupportsWidgets: true.Consider using the capabilities map for consistency:
export function platformSupportsWidgets(platformType: AIPlatformType): boolean { - return platformType === 'openai' || platformType === 'ext-apps' || platformType === 'cursor'; + const capabilities = PLATFORM_CAPABILITIES[platformType] || PLATFORM_CAPABILITIES.unknown; + return capabilities.supportsWidgets; }If the intent is to check specifically for
_meta['ui/html']support (distinct from general widget support), consider renaming the function to clarify its purpose (e.g.,platformSupportsMetaUIHtml).libs/ui/src/dependency/schemas.ts-176-204 (1)
176-204: Import mapscopesURLs lack HTTPS enforcement.The
importsfield correctly enforces HTTPS URLs (lines 186-188), butscopesnested URLs (line 194) only usez.string().url()without the HTTPS refinement. Similarly,integritykeys (line 199) usez.string().url()without HTTPS enforcement.Consider applying consistent HTTPS validation:
scopes: z.record(z.string().min(1), z.record(z.string().min(1), z.string().url())).optional(), + // Note: scopes URLs should also enforce HTTPS for security integrity: z.record(z.string().url(), z.string().regex(/^sha(256|384|512)-[A-Za-z0-9+/=]+$/)).optional(),Or apply the same refinement for consistency with the security posture of the
importsfield.Committable suggestion skipped: line range outside the PR's diff.
libs/ui/src/bundler/file-cache/component-builder.ts-194-196 (1)
194-196: Unused variable:parseResultis assigned but never used.The result of
parseImports(source)is stored but not referenced anywhere in the build flow.Either remove the unused call or utilize the parsed imports for validation/bundling:
// Parse imports - const parseResult = parseImports(source); + // const parseResult = parseImports(source); + // TODO: Use parsed imports for validation or auto-detection of externalslibs/ui/src/bundler/file-cache/storage/redis.ts-192-196 (1)
192-196: Stats drift:entriescounter increments on everyset, even for overwrites.If
setis called for an existing key,localStats.entriesis incremented without checking if the key already exists. Over time, this causesentriesto diverge from the actual count.Consider checking existence before incrementing, or relying on
cleanup()to correct stats periodically.libs/ui/src/bundler/file-cache/storage/redis.ts-161-165 (1)
161-165: Silent error suppression loses cache miss context.When an exception occurs during
get, the error is silently caught and a miss is recorded, butpersistStats()is not called (unlike the success path at line 138). This creates inconsistency in stats persistence.} catch { this.localStats.misses++; this.updateHitRate(); + await this.persistStats(); return undefined; }libs/ui/src/dependency/template-processor.ts-52-67 (1)
52-67: Custom helpers persist on the singleton across all calls.When
customHelpersare registered on the sharedHandlebarsRenderersingleton, they remain registered for all subsequent calls. If different templates use helpers with the same name but different implementations, later registrations will overwrite earlier ones, potentially causing unexpected behavior.Consider either:
- Documenting this behavior clearly
- Creating a new renderer instance when custom helpers differ
- Only registering helpers that don't already exist
// Register any custom helpers if (customHelpers) { for (const [name, fn] of Object.entries(customHelpers)) { + // Note: This overwrites any previously registered helper with the same name handlebarsRenderer.registerHelper(name, fn as HelperFunction); } }libs/ui/src/dependency/template-processor.ts-214-218 (1)
214-218: Unsafe type assertion forcontext.input.
context.inputis typed asunknownbut cast toRecord<string, unknown>at line 215. Ifcontext.inputisnull,undefined, or a primitive, this could cause unexpected behavior in the Handlebars rendering.const renderContext: RenderContext = { - input: context.input as Record<string, unknown>, + input: (context.input && typeof context.input === 'object' ? context.input : {}) as Record<string, unknown>, output: context.output, structuredContent: context.structuredContent, };
🧹 Nitpick comments (46)
libs/sdk/src/tool/flows/call-tool.flow.ts (3)
26-32: Unused import:platformSupportsWidgetsThe
platformSupportsWidgetsfunction is imported but not used anywhere in this file. The serving mode resolution already handles platform capability detection internally viaresolveServingMode.import { resolveServingMode, buildToolResponseContent, - platformSupportsWidgets, type ToolResponseContent, } from '@frontmcp/ui/adapters';
374-380: Potentialundefinedaccess forinputThe destructured
inputfromthis.statemay beundefinedsince it's not accessed viathis.state.required. While the code at lines 458-461 has a defensive check (input?.arguments), consider usingthis.state.requiredfor consistency with other stages, or add an early guard.async applyUI() { this.logger.verbose('applyUI:start'); - const { tool, rawOutput, input } = this.state; + const { tool, rawOutput } = this.state; + const { input } = this.state.required; // Skip if no tool or no UI config if (!tool || !hasUIConfig(tool.metadata)) {Alternatively, if
applyUIcan legitimately run withoutinputbeing set, the current defensive access is acceptable.
455-466: Potentially misleading comment aboutstructuredContentThe comment at line 463 states
structuredContent"Will be added in finalize", but thefinalizestage doesn't appear to setstructuredContenton the result. If this is intentional (i.e.,structuredContentcomes from the parsed output, not added separately), consider updating or removing the comment to avoid confusion.output: rawOutput, - structuredContent: undefined, // Will be added in finalize + structuredContent: undefined,apps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.ts (3)
1-18: Well-documented test file with clear platform support matrix.The header documentation clearly explains the serving modes and platform support. However, the documentation mentions Gemini as "no UI support" but no tests cover Gemini behavior explicitly.
Consider adding a test case for Gemini to ensure completeness with the documented platform matrix:
test('Gemini should return JSON only for static mode tool', async ({ server }) => { const client = await server.createClient({ transport: 'streamable-http', clientInfo: { name: 'Gemini', version: '1.0.0' }, }); const result = await client.tools.call('static-badge', { label: 'Gemini', value: 'Test', }); expect(result).toBeSuccessful(); const json = result.json<{ label: string; value: string }>(); expect(json.label).toBe('Gemini'); await client.disconnect(); });
46-52: Consider adding explicithasToolUI()assertion for static mode.The comment explains that
hasToolUI()should befalsefor static mode, but the test doesn't assert this. Adding an explicit assertion would make the test more robust and self-documenting.expect(result).toBeSuccessful(); // Static mode: UI is served via resource URL, not in response // So hasToolUI() is false, but JSON data is returned +expect(result.hasToolUI()).toBe(false); const json = result.json<{ label: string; value: string; color: string }>();
275-293: AddhasToolUI()assertion for unsupported platforms in hybrid mode.Supported platforms correctly assert
hasToolUI() === true, but unsupported platforms don't asserthasToolUI() === false. This asymmetry could miss regressions where UI is accidentally rendered for unsupported platforms.expect(result).toBeSuccessful(); +expect(result.hasToolUI()).toBe(false); const json = result.json<{ serviceName: string; status: string }>();Apply similar changes to the Continue, generic-mcp, and Unknown client tests in this block.
apps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.ts (3)
20-23: Arbitrary sleep may cause flaky tests.Using a fixed 100ms timeout for "warm-up" is non-deterministic. If the server isn't ready in 100ms, tests may fail intermittently.
Consider using a health check or readiness probe instead:
- test.beforeAll(async () => { - await new Promise((resolve) => setTimeout(resolve, 100)); - }); + test.beforeAll(async ({ server }) => { + // Wait for server to be ready using a health check if available + await server.waitForReady?.(); + });Alternatively, if the testing framework handles server lifecycle, this hook may be unnecessary.
83-100: Consider verifying the dual-payload structure more explicitly.The test checks that
text()is defined but doesn't verify the expected dual-payload format (JSON + markdown-wrapped HTML) described in the file header. This could miss regressions where the format changes.expect(result).toBeSuccessful(); const text = result.text(); expect(text).toBeDefined(); + // Verify dual-payload contains markdown-wrapped HTML + expect(text).toContain('```html');
1-11: Consider adding tests for Gemini and ext-apps platforms.The file header mentions these platforms but tests are missing:
- Gemini: Listed in platform capabilities but not tested
- ext-apps: Mentioned as supporting
_meta['ui/html']but not testedBased on learnings, platform behavior should be tested across configurations.
apps/e2e/demo-e2e-ui/e2e/discovery-and-metadata.e2e.test.ts (1)
39-57: Consider consolidating individual tool listing tests.The test at lines 39-57 ("should have 8 UI tools registered") already validates all 8 expected tools, and the individual tests at lines 59-97 each call
mcp.tools.list()again to check for one tool. This adds test execution time without additional coverage.Consider removing the individual single-tool tests (lines 59-97) since they're already covered by the comprehensive test.
apps/e2e/demo-e2e-ui/src/apps/widgets/tools/form-ui.tsx (2)
118-118: Unusedsendingvariable.The
sendingvariable is destructured fromuseSendMessagebut never used. Consider either using it to show a loading state during form submission or removing it.- const [sendMessage, { loading: sending }] = useSendMessage(); + const [sendMessage] = useSendMessage();Alternatively, use it to disable the submit button during sending:
- <Button type="submit" variant="primary" className="w-full" disabled={submitted}> + <Button type="submit" variant="primary" className="w-full" disabled={submitted || sending}>
174-187: Using array index as React key.Using
indexas the key for form fields works when the field list is static, but will cause issues if fields are reordered or dynamically added/removed. If fields can change dynamically, usefield.nameas the key instead.- {fields.map((field, index) => ( - <div key={index} className="space-y-1"> + {fields.map((field) => ( + <div key={field.name} className="space-y-1">libs/ui/src/bundler/file-cache/hash-calculator.ts (2)
258-292: Unusedexternalsparameter incollectLocalDependencies.The
externalsparameter is passed through recursive calls but never used to filter out external packages. The filtering at lines 278-281 only checks if imports start with.or/, which already excludes npm packages regardless of theexternalslist.Either remove the unused parameter or implement the intended filtering:
async function collectLocalDependencies( filePath: string, baseDir: string, - externals: string[], collected: Set<string>, maxDepth: number, currentDepth: number, ): Promise<void> {And update the call site at line 196:
- await collectLocalDependencies(absoluteEntryPath, baseDir, externals, files, maxDepth, 0); + await collectLocalDependencies(absoluteEntryPath, baseDir, files, maxDepth, 0);
297-326: Regex-based import parsing has known limitations.The import extraction regexes may produce incorrect results for:
- Imports inside comments or strings
- Multi-line import statements
- Template literal imports (
import(`${path}`))For a caching system, false positives are safer than false negatives (extra files in hash vs. missed dependencies). The current approach is acceptable for typical use cases but consider documenting these limitations.
apps/e2e/demo-e2e-orchestrated/e2e/orchestrated-auth.e2e.test.ts (1)
12-233: Consider adding error path test coverage to meet 95%+ coverage target.The current test suite provides good coverage of happy paths across authentication scenarios. To meet the coding guideline requirement of 95%+ test coverage, consider adding tests for error conditions:
- Invalid/malformed tokens
- Expired tokens
- Tokens missing required scopes
- Tool call errors and error response handling
- Resource not found scenarios
- Invalid prompt parameters
- Constructor validation if applicable
As per coding guidelines, **/*.test.ts files should achieve and maintain 95%+ test coverage across all metrics (statements, branches, functions, lines) and test all code paths including errors.
docs/live/docs/authentication/demo-servers.mdx (1)
148-170: Consider clarifying port usage in test example.The example uses port
4001for the test server, while the table above documents default ports as 3003, 3005, and 3004. While this demonstrates using a custom port (which is good), a brief comment explaining why a different port is chosen could help readers understand this is intentional to avoid conflicts.Example enhancement:
test.beforeAll(async () => { // Use a different port to avoid conflicts with running dev servers serverProcess = await TestServer.startNx('demo-e2e-public', { port: 4001 }); });libs/ui/src/dependency/__tests__/template-loader.test.ts (2)
99-111: Consider testing error message specifics.While the tests correctly verify that HTTP URLs are rejected and invalid URLs throw, they don't assert on the exact error type or detailed error messages. Consider adding assertions on error types or messages for more precise validation.
Example enhancement:
it('should throw for HTTP URL', () => { - expect(() => validateTemplateUrl('http://cdn.example.com/widget.html')).toThrow('Template URLs must use HTTPS'); + expect(() => validateTemplateUrl('http://cdn.example.com/widget.html')) + .toThrow('Template URLs must use HTTPS'); }); it('should throw for invalid URL', () => { - expect(() => validateTemplateUrl('not-a-url')).toThrow(); + expect(() => validateTemplateUrl('not-a-url')) + .toThrow(/invalid|URL/i); });
164-201: Enhance URL cache tests with concurrent access and edge cases.While the basic cache operations are tested, consider adding tests for:
- Concurrent cache access (multiple simultaneous reads/writes)
- Cache size limits or eviction policies (if implemented)
- Clearing cache while operations are in progress
- Memory leak prevention scenarios
Based on learnings, maintaining 95%+ test coverage requires comprehensive edge case testing.
libs/ui/src/dependency/__tests__/cdn-registry.test.ts (2)
69-121: Add tests for malformed registry entries.Consider adding tests for error scenarios such as:
- Registry entries with missing required fields (packageName, providers)
- Invalid URL formats in provider configurations
- Circular dependencies in provider fallback chains
- Null or undefined registry entries
Based on learnings, test all code paths including errors.
159-193: Consider adding edge case tests for registry merging.While the basic merge scenarios are covered, consider testing:
- Merging with conflicting peer dependencies
- Merging empty or null registries
- Merging registries with overlapping but different providers
- Deep merge behavior for nested provider configurations
These scenarios help achieve 95%+ test coverage.
Based on learnings, maintain 95%+ test coverage across statements, branches, functions, and lines.
apps/e2e/demo-e2e-ui/e2e/ui.e2e.test.ts (2)
145-178: Add Gemini platform test for comprehensive coverage.While OpenAI and Claude are tested, consider adding a Gemini platform test to ensure compatibility across all major AI platforms.
Based on learnings, test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable.
Example:
test('should handle Gemini client', async ({ server }) => { const client = await server.createClient({ transport: 'streamable-http', clientInfo: { name: 'Gemini', version: '1.0.0' }, }); const result = await client.tools.call('html-card', { title: 'Test', content: 'Content', }); expect(result).toBeSuccessful(); await client.disconnect(); });
180-194: LGTM! Consider adding concurrent error scenarios.The concurrent operations test is good. Optionally, consider testing concurrent calls with some failures to verify error isolation and no cross-contamination between parallel operations.
libs/ui/src/dependency/__tests__/import-map.test.ts (2)
171-182: LGTM! HTML escaping test is present.The test correctly validates that special characters in URLs don't break the generated JSON. Consider expanding to test additional characters that could cause XSS issues:
<,>,',", and script injection attempts.Based on learnings, test HTML escaping for user-provided content to prevent XSS attacks.
308-346: Enhance validation tests with additional error scenarios.While the basic validation is tested, consider adding tests for:
- Malformed import map structure (missing required fields)
- Invalid scope prefixes
- Duplicate import keys
- Mixed valid/invalid entries
- Cross-origin security violations (if applicable)
Based on learnings, achieve and maintain 95%+ test coverage across all metrics.
libs/ui/src/dependency/__tests__/template-processor.test.ts (1)
330-370: Consider adding stress tests for edge cases.While the basic edge cases are well-covered, consider adding tests for:
- Very large templates (e.g., 10MB+)
- Deeply nested data structures (e.g., 20+ levels)
- Circular references in context data
- Templates with thousands of Handlebars expressions
- Unicode normalization issues
These help ensure robustness under stress conditions while maintaining 95%+ test coverage.
Based on learnings, maintain 95%+ test coverage across statements, branches, functions, and lines.
libs/ui/src/adapters/__tests__/response-builder.test.ts (1)
225-290: LGTM! Edge case handling is robust.The circular reference test (lines 262-277) is particularly important. However, consider adding tests for XSS in the
htmlContentparameter to ensure user-provided HTML is properly sanitized before being embedded in responses.Based on learnings, test HTML escaping for user-provided content to prevent XSS attacks.
Example:
it('should sanitize XSS in htmlContent', () => { const result = buildToolResponseContent( createOptions({ servingMode: 'inline', platformType: 'openai', htmlContent: '<script>alert("xss")</script><div>Safe</div>', }), ); // Verify script tags are escaped or removed expect(result.content[0].text).not.toContain('<script>'); });apps/e2e/demo-e2e-ui/e2e/react-tools.e2e.test.ts (1)
582-628: LGTM! Concurrent operations tests validate thread safety.The concurrent call tests ensure that multiple operations don't interfere with each other. Optionally, consider testing concurrent operations where some fail to verify error isolation.
libs/ui/src/dependency/__tests__/resolver.test.ts (1)
197-214: Consider adding platform-specific tests for Gemini and ngrok.The factory function tests cover
createClaudeResolverandcreateOpenAIResolver, but based on learnings, tests should cover behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable. IfcreateGeminiResolveror similar factory functions exist, add corresponding tests.describe('createOpenAIResolver', () => { it('should create OpenAI-optimized resolver', () => { const resolver = createOpenAIResolver(); const result = resolver.resolve('react'); // OpenAI prefers esm.sh or jsdelivr expect(result.cdnUrl).toBeDefined(); }); }); + + // TODO: Add tests for createGeminiResolver if available + // describe('createGeminiResolver', () => { ... }); });libs/ui/src/adapters/__tests__/dual-payload.test.ts (2)
145-149: Consider removing non-null assertion.Line 148 uses
match![1]which violates the coding guideline to avoid non-null assertions. Although the precedingexpect(match).toBeTruthy()provides a logical guard, consider restructuring:// Extract HTML from result const match = result.content[1].text.match(/```html\n([\s\S]*?)\n```/); expect(match).toBeTruthy(); - expect(match![1]).toBe(html); + if (match) { + expect(match[1]).toBe(html); + }As per coding guidelines, avoid non-null assertions (
!) - use proper error handling instead.
76-97: Consider adding XSS-focused test for user-provided data.The test suite thoroughly covers data handling, but per learnings, tests should verify HTML escaping for user-provided content to prevent XSS attacks. Consider adding a test that explicitly verifies malicious content in the
dataobject is safely serialized:it('should safely serialize data containing potential XSS vectors', () => { const result = buildDualPayload({ data: { userInput: '<script>alert("xss")</script>', nested: { html: '<img onerror="alert(1)" src="x">' } }, html: '<html></html>', }); // Data is JSON-stringified, so HTML is not executed const parsed = JSON.parse(result.content[0].text); expect(parsed.userInput).toBe('<script>alert("xss")</script>'); });Based on learnings: Test HTML escaping for user-provided content to prevent XSS attacks.
libs/ui/src/bundler/file-cache/storage/filesystem.ts (2)
100-102: Use specific error classes instead of genericError.Per coding guidelines, specific error classes with MCP error codes should be used instead of generic errors.
- throw new Error(`Failed to initialize cache directory: ${error}`); + throw new CacheInitializationError(`Failed to initialize cache directory: ${error}`);Also applies to: 248-250, 393-395
134-136: Fire-and-forget write may silently lose metadata updates.The async write for access metadata is not awaited and errors are swallowed. While this avoids blocking reads, it means
lastAccessedAtandaccessCountupdates may be lost without any indication.Consider at minimum logging the error for debugging:
// Write back updated metadata (async, don't await) - this.writeEntry(filePath, entry).catch(() => {}); + this.writeEntry(filePath, entry).catch((err) => { + console.debug(`Failed to update cache metadata for ${key}: ${err}`); + });libs/ui/src/bundler/file-cache/component-builder.ts (1)
462-473: Factory function acceptsunknownthen casts - acceptable pattern but could be typed.The
redisClient: unknownparameter is immediately cast toRedisClient. While this works, consider acceptingRedisClientdirectly for better type safety at call sites.export async function createRedisBuilder( - redisClient: unknown, + redisClient: import('./storage/redis.js').RedisClient, keyPrefix = 'frontmcp:ui:build:', ): Promise<ComponentBuilder> { const { RedisStorage } = await import('./storage/redis.js'); const storage = new RedisStorage({ - client: redisClient as import('./storage/redis.js').RedisClient, + client: redisClient, keyPrefix, });apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts (1)
239-284: Consider adding XSS injection test cases.Based on learnings, tests should verify HTML escaping for user-provided content to prevent XSS attacks. The Unicode and special characters tests are good but don't specifically test malicious payloads.
Add test cases for XSS vectors:
test('should escape HTML/script injection in title', async ({ mcp }) => { const result = await mcp.tools.call('markdown-report', { title: '<script>alert("xss")</script>', summary: 'Test summary', findings: [{ title: 'F', description: 'D', severity: 'low' }], }); expect(result).toBeSuccessful(); const json = result.json<{ markdown: string }>(); expect(json.markdown).not.toContain('<script>'); }); test('should escape HTML in finding descriptions', async ({ mcp }) => { const result = await mcp.tools.call('markdown-report', { title: 'Report', summary: 'Summary', findings: [{ title: 'Finding', description: '<img src=x onerror=alert(1)>', severity: 'low' }], }); expect(result).toBeSuccessful(); const json = result.json<{ markdown: string }>(); expect(json.markdown).not.toContain('onerror='); });Based on learnings about testing HTML escaping for user-provided content.
libs/ui/src/dependency/import-parser.ts (2)
249-251: Fragile skip logic for combined imports.The logic to skip combined imports by checking if
afterMatch.startsWith(',')may produce incorrect results. The calculationmatch.index + match[0].length - specifier.length - 2is complex and assumes a specific structure. Consider using the deduplication mechanism (seenStatements) instead, or add a comment explaining the expected string position.
283-285: Consider documenting the magic number for lookback window.The
50character lookback for detecting if a side-effect import is part of another pattern is reasonable but arbitrary. A brief comment explaining why 50 characters is sufficient would improve maintainability.- // Skip if this is part of another import pattern (has 'from' before it) - const beforeMatch = source.slice(Math.max(0, match.index - 50), match.index); + // Skip if this is part of another import pattern (has 'from' before it) + // 50 chars lookback is sufficient to capture 'from' keyword in typical import statements + const beforeMatch = source.slice(Math.max(0, match.index - 50), match.index);libs/ui/src/bundler/file-cache/storage/interface.ts (1)
178-184: Handle potential JSON.stringify errors in calculateManifestSize.
JSON.stringifycan throw if the manifest contains circular references or BigInt values. Consider adding error handling for robustness.export function calculateManifestSize(manifest: ComponentBuildManifest): number { - // Approximate size by serializing to JSON - return Buffer.byteLength(JSON.stringify(manifest), 'utf8'); + // Approximate size by serializing to JSON + try { + return Buffer.byteLength(JSON.stringify(manifest), 'utf8'); + } catch { + // Fallback estimate if serialization fails (e.g., circular refs) + return 0; + } }libs/ui/src/adapters/dual-payload.ts (1)
148-163: Duplicate safeStringify implementation exists in utils.This local
safeStringifyfunction duplicates the one inlibs/ui/src/utils/safe-stringify.ts, which has a more robust implementation with circular reference handling viaWeakSet. Consider importing from the existing utility instead.+import { safeStringify } from '../utils/safe-stringify'; + // ... in buildDualPayload function ... - const dataBlock: TextContent = { - type: 'text', - text: safeStringify(data), - }; + const dataBlock: TextContent = { + type: 'text', + text: data === undefined || data === null ? '{}' : safeStringify(data), + };Note: The existing utility doesn't handle
undefined/null→'{}'conversion, so you may need to add that wrapper logic or extend the utility.libs/ui/src/bundler/file-cache/storage/redis.ts (1)
243-262:clear()also deletes stats key, then immediately persists empty stats.The
keyspattern${keyPrefix}*includes the stats key (e.g.,frontmcp:ui:build::__stats__). Deleting it, then callingpersistStats()at line 261 recreates it. This is wasteful but not incorrect. Consider filtering out the stats key before deletion for clarity:const pattern = `${this.options.keyPrefix}*`; const keys = await this.options.client.keys(pattern); + const dataKeys = keys.filter((k) => !k.endsWith(STATS_KEY_SUFFIX)); - if (keys.length > 0) { - await this.options.client.del(keys); + if (dataKeys.length > 0) { + await this.options.client.del(dataKeys); }libs/ui/src/dependency/cdn-registry.ts (1)
704-760: Topological sort handles cycles correctly.The algorithm uses a
remainingset and breaks cycles by arbitrarily picking the next package when no progress is made (lines 748-756). The guard against infinite loops is sound.Minor: Line 713 uses non-null assertion
queue.shift()!. This is safe here since the while condition guaranteesqueue.length > 0, but per coding guidelines, consider:- const packageName = queue.shift()!; + const packageName = queue.shift(); + if (!packageName) continue;libs/ui/src/dependency/template-loader.ts (1)
440-453:needsRefetchlogic may be too aggressive.The function returns
true(needs refetch) when!cached.etag. However, many CDNs don't return ETags. This means templates from such CDNs will always be re-fetched, defeating the cache.Consider adding a TTL-based fallback:
// If we have an ETag, we can validate with conditional request // Otherwise, check if within a reasonable TTL (e.g., 5 minutes) if (cached.etag) return false; const fetchedAt = new Date(cached.fetchedAt).getTime(); const ttl = 5 * 60 * 1000; // 5 minutes return Date.now() - fetchedAt > ttl;libs/ui/src/dependency/resolver.ts (2)
103-107: Hardcoded provider 'cloudflare' for overrides may be misleading.When using an override, the provider is always set to
'cloudflare'regardless of the actual URL source. This could cause confusion in logging or debugging.Consider accepting the provider as part of the override, or using a neutral value like
'custom':if (override) { - return this.createResolvedDependency(packageName, override, 'cloudflare'); + return this.createResolvedDependency(packageName, override, 'custom' as CDNProvider); }
27-27: Unused import:getPackageName.
getPackageNameis imported from./import-parserbut never used in this file.-import { parseImports, getPackageName } from './import-parser'; +import { parseImports } from './import-parser';libs/ui/src/dependency/types.ts (1)
692-718: Potential edge case in file-path detection.The file extension check at line 711 could misclassify inline template strings that happen to end with file extensions (e.g.,
"Use component.tsx"would be detected asfile-path). While this is likely rare, consider documenting this heuristic behavior or adding a length/content check to reduce false positives.// Check for file path patterns if ( template.startsWith('./') || template.startsWith('../') || template.startsWith('/') || // Windows-style paths /^[A-Za-z]:\\/.test(template) || - // File extensions - /\.(tsx?|jsx?|mdx?)$/i.test(template) + // File extensions (only if looks like a path - no spaces, reasonable length) + (/\.(tsx?|jsx?|mdx?)$/i.test(template) && !template.includes(' ') && template.length < 256) ) { return 'file-path'; }libs/ui/src/dependency/template-processor.ts (2)
107-116: Potentially unnecessary eslint-disable comment.Line 110 disables
@typescript-eslint/no-require-imports, but line 111 usesawait import()(dynamic import), notrequire(). The eslint-disable comment may be unnecessary unless there's a different linting rule being triggered.try { // Use a variable to prevent TypeScript from checking the module at compile time const moduleName = 'marked'; - // eslint-disable-next-line @typescript-eslint/no-require-imports markedModule = (await import(/* webpackIgnore: true */ moduleName)) as unknown as MarkedModule; return markedModule; } catch {
246-275: Use the sharedescapeHtmlutility from handlebars helpers instead of inline implementations.Per coding guidelines, always use
escapeHtml()utility for user-provided content. The inline implementation at lines 248-255 is duplicated again at lines 433-439. Import and use the shared utility from../handlebars/helpers:-import { HandlebarsRenderer, containsHandlebars, type RenderContext, type HelperFunction } from '../handlebars'; +import { HandlebarsRenderer, containsHandlebars, type RenderContext, type HelperFunction, escapeHtml } from '../handlebars'; // In both templateContext.helpers objects (lines ~250 and ~432): helpers: { - escapeHtml: (str: string) => { - return str - .replace(/&/g, '&') - .replace(/</g, '<') - .replace(/>/g, '>') - .replace(/"/g, '"') - .replace(/'/g, '''); - }, + escapeHtml, // ... other helpers }The shared utility also adds null/undefined handling, making it more robust than the inline versions.
apps/e2e/demo-e2e-ui/src/apps/widgets/tools/react-chart.tool.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
apps/e2e/demo-e2e-ui/src/apps/widgets/tools/static-badge.tool.ts (1)
11-62: LGTM: Schemas and template properly implement security measures.The input and output schemas use
.strict()as required, and the template correctly appliesescapeHtmlto all user-provided content (labelandvalue). The previous XSS vulnerability has been resolved.apps/e2e/demo-e2e-ui/src/apps/widgets/tools/hybrid-status.tool.ts (1)
11-86: LGTM: Schemas and template properly implement security measures.The input and output schemas use
.strict()as required, and the template correctly appliesescapeHtmlto all user-provided string content (serviceName,status, andlastCheck). Theuptimefield is numeric and safely rendered. The previous XSS vulnerability has been resolved.libs/ui/src/dependency/__tests__/template-loader.test.ts (1)
203-290: Test coverage forneedsRefetchis comprehensive.Tests cover non-URL sources, inline sources, URL not in cache, URL with ETag, URL within TTL, and TTL expiration scenarios. The 6-minute offset for TTL expiration test (line 275) correctly exceeds the 5-minute TTL.
As noted in a previous review, consider adding tests for:
- Invalid ETag formats in cache
- Network errors during fetch operations
Based on learnings, test all code paths including errors.
libs/ui/src/bundler/file-cache/storage/filesystem.ts (3)
354-358: Key sanitization may cause cache collisions.Different keys like
"foo/bar","foo:bar", and"foo_bar"all map to the same filename"foo_bar.json". This could cause cache overwrites and incorrect data retrieval.
380-405: Stats inconsistency betweenloadStatsandsetmethods.
loadStatsusesfileStat.size(actual file bytes on disk), whilesetusescalculateManifestSize(manifest)(logical manifest size). This causestotalSizeto be inaccurate after restart.
452-455: Bug:evictLRUcannot recover the original cache key from the sanitized filename.The
getFilePathmethod sanitizes keys, butevictLRUattempts to reverse this by stripping only the extension. This passes the sanitized key todelete(), which then re-sanitizes it.libs/ui/src/dependency/import-map.ts (1)
218-221: Package names in UMD shim are not escaped, risking code injection.This issue was flagged in a past review. If
dep.packageNamecontains a single quote, the generated JavaScript will be syntactically invalid or exploitable.const entries = depsWithGlobals.map((dep) => { const global = dep.global!; - return `'${dep.packageName}': { default: window.${global}, ...window.${global} }`; + const safePkgName = dep.packageName.replace(/\\/g, '\\\\').replace(/'/g, "\\'"); + return `'${safePkgName}': { default: window.${global}, ...window.${global} }`; });Additionally,
globalshould be validated as a valid JavaScript identifier.
🧹 Nitpick comments (18)
libs/ui/src/adapters/dual-payload.ts (2)
150-162: Consider escaping all backtick sequences, not just triple backticks.The current implementation only escapes exactly three consecutive backticks. Markdown allows longer fence delimiters (e.g.,
), and sequences likehtml` could still break parsing if the HTML contains four backticks.A more robust approach would escape all consecutive backtick sequences:
function escapeCodeFence(html: string): string { - // If HTML contains triple backticks, we need to escape or use longer fence - // This is rare in practice, but we handle it for safety - if (html.includes('```')) { - // Replace triple backticks with HTML entity equivalent - return html.replace(/```/g, '```'); - } - return html; + // Escape all backtick sequences to prevent breaking any markdown fence + return html.replace(/`+/g, (match) => '`'.repeat(match.length)); }
223-225: Non-greedy regex may fail on multi-line HTML with escaped backticks.The regex
/```html\n([\s\S]*?)\n```/uses non-greedy matching, which will stop at the first\n\``occurrence. If HTML contains escaped backticks (````) that were converted from triple backticks, this works correctly. However, if for any reason the escaped content contains a literal newline followed by triple backticks, the regex will capture prematurely.This is an edge case given the escaping in
buildDualPayload, but worth noting for robustness.libs/ui/src/adapters/__tests__/dual-payload.test.ts (1)
144-154: Add a test for backticks not at fence boundaries.The test verifies triple backticks within a
<pre>block are escaped. Consider adding a case for backticks that appear without the triple pattern (e.g., inline code with single/double backticks) to ensure they're preserved correctly.it('should preserve single and double backticks unchanged', () => { const htmlWithBackticks = '<code>`inline`</code><pre>``template``</pre>'; const result = buildDualPayload({ data: {}, html: htmlWithBackticks, }); // Single and double backticks should remain unchanged const match = result.content[1].text.match(/```html\n([\s\S]*?)\n```/); expect(match?.[1]).toContain('`inline`'); expect(match?.[1]).toContain('``template``'); });apps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.ts (1)
519-525: Redundant assertion can be simplified.Line 525 re-asserts
result2as successful, but this was already verified on line 520. Consider removing the redundant assertion or replacing the comment with a more meaningful check.// Both should have correct platform behavior expect(result1.hasToolUI()).toBe(true); - // Claude uses dual-payload, so check it succeeded - expect(result2).toBeSuccessful(); + // Claude uses dual-payload with markdown-wrapped HTML + expect(result2.text()).toContain('```html');apps/e2e/demo-e2e-ui/e2e/react-tools.e2e.test.ts (1)
163-177: Non-deterministic test data may cause flaky tests.Using
Math.random()to generate test values can lead to inconsistent test behavior and makes failures harder to reproduce. Consider using deterministic values.test('should handle many data points', async ({ mcp }) => { const data = Array.from({ length: 50 }, (_, i) => ({ label: `Point ${i}`, - value: Math.floor(Math.random() * 1000), + value: (i + 1) * 20, // Deterministic values for reproducibility })); const result = await mcp.tools.call('react-chart', { data, title: 'Large Chart', }); expect(result).toBeSuccessful(); const json = result.json<{ data: Array<{ label: string; value: number }> }>(); expect(json.data).toHaveLength(50); });libs/ui/src/adapters/serving-mode.ts (1)
134-147: Consider using sentinel values for non-widget platform defaultMode.For
geminiandunknownplatforms wheresupportsWidgets: false, settingdefaultMode: 'inline'creates a logical inconsistency. While the code correctly checkssupportsWidgetsfirst (line 187), this could confuse maintainers.Consider one of these approaches:
Option 1: Use a type-safe sentinel for platforms without widget support:
+type PlatformDefaultMode = Exclude<WidgetServingMode, 'auto'> | 'none'; + interface PlatformUICapabilities { - defaultMode: Exclude<WidgetServingMode, 'auto'>; + defaultMode: PlatformDefaultMode; } // Then for gemini/unknown: gemini: { supportsWidgets: false, useDualPayload: false, supportedModes: [], - defaultMode: 'inline', // Not used since supportsWidgets is false + defaultMode: 'none', },Option 2: Make
defaultModeoptional and undefined for non-widget platforms:interface PlatformUICapabilities { - defaultMode: Exclude<WidgetServingMode, 'auto'>; + defaultMode?: Exclude<WidgetServingMode, 'auto'>; }apps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.ts (1)
34-510: Consider extracting common test patterns to reduce duplication.Each test follows the same pattern: create client, call tool, validate, disconnect. While the current structure is clear and ensures test independence, you could reduce duplication with a helper function for the repeated client lifecycle.
Optional helper pattern:
async function testToolCall<T>( server: ServerFixture, clientInfo: { name: string; version: string }, toolName: string, args: unknown, assertions: (result: ToolResult, json: T) => void | Promise<void> ) { const client = await server.createClient({ transport: 'streamable-http', clientInfo, }); const result = await client.tools.call(toolName, args); expect(result).toBeSuccessful(); const json = result.json<T>(); await assertions(result, json); await client.disconnect(); } // Usage: test('OpenAI should return JSON data for static badge', async ({ server }) => { await testToolCall( server, { name: 'ChatGPT', version: '1.0.0' }, 'static-badge', { label: 'Status', value: 'Active', color: 'green' }, (result, json: { label: string; value: string; color: string }) => { expect(result.hasToolUI()).toBe(false); expect(json.label).toBe('Status'); expect(json.value).toBe('Active'); expect(json.color).toBe('green'); } ); });libs/ui/src/dependency/__tests__/template-loader.test.ts (1)
99-111: Consider adding test for FTP/other protocol rejection.The tests verify HTTPS acceptance and HTTP rejection, but could benefit from testing other protocol schemes (e.g.,
ftp://,file://) to ensure comprehensive validation coverage.it('should throw for file:// URL', () => { expect(() => validateTemplateUrl('file:///path/to/file.html')).toThrow('Template URLs must use HTTPS'); });libs/ui/src/dependency/__tests__/resolver.test.ts (1)
132-165: Consider testing empty externals list.The
resolveFromSourcetests verify filtering by externals, but don't test behavior when externals list is empty.it('should return empty array when externals list is empty', () => { const resolver = new DependencyResolver({ platform: 'claude' }); const source = `import React from 'react';`; const results = resolver.resolveFromSource(source, []); expect(results.length).toBe(0); });libs/ui/src/dependency/import-parser.ts (1)
368-371:extractExternalPackagesduplicates parsing work.This "faster version" comment is misleading - it simply calls
parseImportsand extracts one field. If performance is a concern for package-name-only extraction, a dedicated lightweight implementation would be warranted.Consider renaming or updating the JSDoc to clarify this is a convenience wrapper rather than a performance optimization:
/** - * Extract only external package names from source. - * - * This is a faster version when you only need package names. + * Convenience function to extract only external package names from source. * * @param source - Source code to parse * @returns Array of unique external package names */libs/ui/src/bundler/file-cache/storage/interface.ts (2)
181-189: Returning 0 on error silently hides size calculation failures.When
JSON.stringifyfails (e.g., circular references, BigInt), returning 0 can cause incorrect cache eviction decisions since the entry appears to have no size. Consider logging a warning or returning an estimate based on a fallback strategy.export function calculateManifestSize(manifest: ComponentBuildManifest): number { // Approximate size by serializing to JSON try { return Buffer.byteLength(JSON.stringify(manifest), 'utf8'); - } catch { + } catch (error) { // Fallback for circular references or BigInt values + console.warn('Failed to calculate manifest size:', error); return 0; } }
50-116: TwoBuildCacheStorageinterfaces with the same name exist in the public API—consolidate to avoid confusion for SDK consumers.The
dependency/types.tsBuildCacheStorage (6 methods: get, set, has, delete, clear, getStats) andbundler/file-cache/storage/interface.tsBuildCacheStorage (adds: type, initialize, cleanup, close) are both exported publicly but only the latter is actively used in implementations and throughout the codebase. This creates inconsistency for SDK consumers who may inadvertently import the incomplete version.Make
dependency/types.tsBuildCacheStorage extend the fuller interface fromstorage/interface.ts, or re-export from there. This maintains backward compatibility for existing imports while eliminating duplication and ensuring all consumers access the complete API.libs/ui/src/bundler/file-cache/hash-calculator.ts (1)
48-55: Silent failure inhashFilecould hide file access issues.Returning
undefinedwithout logging when a file cannot be read may make debugging difficult when hash mismatches occur due to permission issues or missing files.export async function hashFile(filePath: string): Promise<string | undefined> { try { const content = await readFile(filePath); return sha256Buffer(content); - } catch { + } catch (error) { + // File may not exist or be inaccessible - caller handles undefined return undefined; } }libs/ui/src/dependency/template-loader.ts (1)
35-49: Module-level cache state may cause issues in serverless environments.The
urlCacheMap persists across requests in long-running processes but may behave unexpectedly in serverless environments where module state can be shared across invocations. Consider documenting this behavior or providing a cache instance injection option.libs/ui/src/dependency/import-map.ts (1)
244-246:escapeHtmlAttrshould also escape<and>for defense in depth.While
<and>are less critical in attribute values, escaping them provides defense against edge cases and potential context-switching attacks.function escapeHtmlAttr(str: string): string { - return str.replace(/&/g, '&').replace(/"/g, '"'); + return str + .replace(/&/g, '&') + .replace(/"/g, '"') + .replace(/</g, '<') + .replace(/>/g, '>'); }libs/ui/src/bundler/file-cache/component-builder.ts (1)
231-235: Warning logged for failed external resolution could be noisy.In non-strict mode with many unresolved packages, this could flood logs. Consider using a debug-level log or batching warnings.
} catch (error) { // Log warning but continue - package will be bundled instead - console.warn(`Failed to resolve external "${pkg}": ${error}`); + if (process.env['DEBUG']) { + console.warn(`Failed to resolve external "${pkg}": ${error}`); + } }libs/ui/src/dependency/schemas.ts (2)
19-83: Add JSDoc examples for CDN schemas.Per coding guidelines, components and schemas should include JSDoc examples showing basic usage patterns. Consider adding example tags to demonstrate how to use these schemas.
Example JSDoc for
cdnDependencySchema:/** * Schema for validating CDN dependency configuration. + * + * @example + * ```typescript + * const dependency = cdnDependencySchema.parse({ + * url: 'https://cdn.jsdelivr.net/npm/react@18/umd/react.production.min.js', + * integrity: 'sha384-KyZXEAg3QhqLMpG8r+8fhAXLRk2vvoC2f3B09zVXn8CA5QIVfZOJ3BCsw2P0p/We', + * global: 'React', + * esm: false + * }); + * ``` */ export const cdnDependencySchema = zBased on learnings, all components should have JSDoc examples with example tags showing basic usage and options patterns.
413-476: Consider using Zod's built-in SafeParseReturnType instead of manual SafeParseResult.Zod 4 provides
SafeParseReturnType<Input, Output>which already defines the discriminated union you're manually typing at line 416. Using Zod's built-in type eliminates redundancy and ensures consistency with the library's type system.-/** - * Safe parse result type. - */ -export type SafeParseResult<T> = { success: true; data: T } | { success: false; error: z.ZodError }; - /** * Safely validate a CDN dependency configuration. * * @param data - Data to validate * @returns Safe parse result with success flag */ -export function safeParseCDNDependency(data: unknown): SafeParseResult<CDNDependencyOutput> { - return cdnDependencySchema.safeParse(data) as SafeParseResult<CDNDependencyOutput>; +export function safeParseCDNDependency(data: unknown) { + return cdnDependencySchema.safeParse(data); }The type assertions (
as SafeParseResult<T>) are also unnecessary since Zod's.safeParse()already returns the correct type. Removing these simplifies the code and leverages Zod's type inference.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
apps/e2e/demo-e2e-ui/e2e/discovery-and-metadata.e2e.test.ts(1 hunks)apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts(1 hunks)apps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.ts(1 hunks)apps/e2e/demo-e2e-ui/e2e/react-tools.e2e.test.ts(1 hunks)apps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.ts(1 hunks)apps/e2e/demo-e2e-ui/src/apps/widgets/tools/form-ui.tsx(1 hunks)apps/e2e/demo-e2e-ui/src/apps/widgets/tools/hybrid-status.tool.ts(1 hunks)apps/e2e/demo-e2e-ui/src/apps/widgets/tools/react-chart.tool.ts(2 hunks)apps/e2e/demo-e2e-ui/src/apps/widgets/tools/static-badge.tool.ts(1 hunks)docs/live/docs/authentication/demo-servers.mdx(6 hunks)libs/sdk/src/tool/flows/call-tool.flow.ts(5 hunks)libs/ui/project.json(2 hunks)libs/ui/src/adapters/__tests__/dual-payload.test.ts(1 hunks)libs/ui/src/adapters/__tests__/response-builder.test.ts(1 hunks)libs/ui/src/adapters/dual-payload.ts(1 hunks)libs/ui/src/adapters/response-builder.ts(1 hunks)libs/ui/src/adapters/serving-mode.ts(1 hunks)libs/ui/src/bundler/file-cache/component-builder.ts(1 hunks)libs/ui/src/bundler/file-cache/hash-calculator.ts(1 hunks)libs/ui/src/bundler/file-cache/storage/filesystem.ts(1 hunks)libs/ui/src/bundler/file-cache/storage/interface.ts(1 hunks)libs/ui/src/bundler/file-cache/storage/redis.ts(1 hunks)libs/ui/src/dependency/__tests__/resolver.test.ts(1 hunks)libs/ui/src/dependency/__tests__/template-loader.test.ts(1 hunks)libs/ui/src/dependency/cdn-registry.ts(1 hunks)libs/ui/src/dependency/import-map.ts(1 hunks)libs/ui/src/dependency/import-parser.ts(1 hunks)libs/ui/src/dependency/resolver.ts(1 hunks)libs/ui/src/dependency/schemas.ts(1 hunks)libs/ui/src/dependency/template-loader.ts(1 hunks)libs/ui/src/dependency/template-processor.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/live/docs/authentication/demo-servers.mdx
- libs/ui/src/adapters/tests/response-builder.test.ts
- libs/ui/src/adapters/response-builder.ts
- apps/e2e/demo-e2e-ui/src/apps/widgets/tools/form-ui.tsx
- apps/e2e/demo-e2e-ui/e2e/discovery-and-metadata.e2e.test.ts
- libs/ui/src/bundler/file-cache/storage/redis.ts
🧰 Additional context used
📓 Path-based instructions (6)
libs/ui/src/**/*.ts
📄 CodeRabbit inference engine (libs/ui/CLAUDE.md)
libs/ui/src/**/*.ts: Always useescapeHtml()utility for all user-provided content to prevent XSS vulnerabilities
Hard-code CDN URLs only in theme presets; always reference theme.cdn configuration in component code
Files:
libs/ui/src/bundler/file-cache/storage/interface.tslibs/ui/src/dependency/__tests__/resolver.test.tslibs/ui/src/adapters/__tests__/dual-payload.test.tslibs/ui/src/bundler/file-cache/storage/filesystem.tslibs/ui/src/adapters/serving-mode.tslibs/ui/src/bundler/file-cache/hash-calculator.tslibs/ui/src/dependency/cdn-registry.tslibs/ui/src/dependency/import-parser.tslibs/ui/src/adapters/dual-payload.tslibs/ui/src/dependency/__tests__/template-loader.test.tslibs/ui/src/dependency/import-map.tslibs/ui/src/bundler/file-cache/component-builder.tslibs/ui/src/dependency/template-loader.tslibs/ui/src/dependency/template-processor.tslibs/ui/src/dependency/resolver.tslibs/ui/src/dependency/schemas.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Enable strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use specific error classes with MCP error codes instead of generic errors
Use type parameters with constraints instead of unconstrained generics, and preferunknownoveranyfor generic type defaults
Follow the preset pattern for hierarchical configurations across the codebase
Files:
libs/ui/src/bundler/file-cache/storage/interface.tsapps/e2e/demo-e2e-ui/src/apps/widgets/tools/hybrid-status.tool.tsapps/e2e/demo-e2e-ui/src/apps/widgets/tools/react-chart.tool.tslibs/ui/src/dependency/__tests__/resolver.test.tsapps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/react-tools.e2e.test.tsapps/e2e/demo-e2e-ui/src/apps/widgets/tools/static-badge.tool.tslibs/ui/src/adapters/__tests__/dual-payload.test.tslibs/ui/src/bundler/file-cache/storage/filesystem.tslibs/ui/src/adapters/serving-mode.tslibs/ui/src/bundler/file-cache/hash-calculator.tsapps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.tslibs/ui/src/dependency/cdn-registry.tslibs/ui/src/dependency/import-parser.tslibs/ui/src/adapters/dual-payload.tslibs/ui/src/dependency/__tests__/template-loader.test.tslibs/ui/src/dependency/import-map.tslibs/ui/src/bundler/file-cache/component-builder.tslibs/ui/src/dependency/template-loader.tslibs/ui/src/dependency/template-processor.tslibs/ui/src/dependency/resolver.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/ui/src/dependency/schemas.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/ui/src/bundler/file-cache/storage/interface.tslibs/ui/project.jsonlibs/ui/src/dependency/__tests__/resolver.test.tslibs/ui/src/adapters/__tests__/dual-payload.test.tslibs/ui/src/bundler/file-cache/storage/filesystem.tslibs/ui/src/adapters/serving-mode.tslibs/ui/src/bundler/file-cache/hash-calculator.tslibs/ui/src/dependency/cdn-registry.tslibs/ui/src/dependency/import-parser.tslibs/ui/src/adapters/dual-payload.tslibs/ui/src/dependency/__tests__/template-loader.test.tslibs/ui/src/dependency/import-map.tslibs/ui/src/bundler/file-cache/component-builder.tslibs/ui/src/dependency/template-loader.tslibs/ui/src/dependency/template-processor.tslibs/ui/src/dependency/resolver.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/ui/src/dependency/schemas.ts
libs/ui/src/**/*.test.ts
📄 CodeRabbit inference engine (libs/ui/CLAUDE.md)
libs/ui/src/**/*.test.ts: Maintain 95%+ test coverage across statements, branches, functions, and lines
Test HTML escaping for user-provided content to prevent XSS attacks
Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Files:
libs/ui/src/dependency/__tests__/resolver.test.tslibs/ui/src/adapters/__tests__/dual-payload.test.tslibs/ui/src/dependency/__tests__/template-loader.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts: Achieve and maintain 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including errors, constructor validation, and error classinstanceofchecks
Files:
libs/ui/src/dependency/__tests__/resolver.test.tsapps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/react-tools.e2e.test.tslibs/ui/src/adapters/__tests__/dual-payload.test.tsapps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.tslibs/ui/src/dependency/__tests__/template-loader.test.ts
libs/{sdk,adapters,plugins,cli}/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters,plugins,cli}/src/**/*.ts: Return strictly typed MCP protocol responses (GetPromptResult, ReadResourceResult, etc.) instead ofunknownforexecute()andread()methods
Validate URIs per RFC 3986 at metadata level using Zod validation with custom refinements
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities in adapters
UsechangeScopeinstead ofscopefor change event properties to avoid confusion with the Scope class
Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Don't mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/tool/flows/call-tool.flow.ts
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.ts : Add JSDoc examples with example tags showing basic usage and options patterns for all components
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.ts : Always use `escapeHtml()` utility for all user-provided content to prevent XSS vulnerabilities
Applied to files:
apps/e2e/demo-e2e-ui/src/apps/widgets/tools/hybrid-status.tool.tslibs/ui/project.jsonapps/e2e/demo-e2e-ui/e2e/react-tools.e2e.test.tsapps/e2e/demo-e2e-ui/src/apps/widgets/tools/static-badge.tool.tslibs/ui/src/dependency/import-map.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test HTML escaping for user-provided content to prevent XSS attacks
Applied to files:
apps/e2e/demo-e2e-ui/src/apps/widgets/tools/hybrid-status.tool.tsapps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/react-tools.e2e.test.tsapps/e2e/demo-e2e-ui/src/apps/widgets/tools/static-badge.tool.tslibs/ui/src/adapters/__tests__/dual-payload.test.tsapps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.tslibs/ui/src/dependency/__tests__/template-loader.test.tslibs/ui/src/dependency/import-map.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/*/src/index.ts : Use barrel exports (index.ts) to export all public API surface without legacy exports or aliases
Applied to files:
libs/ui/project.jsonlibs/ui/src/dependency/import-parser.tslibs/ui/src/dependency/import-map.tslibs/ui/src/bundler/file-cache/component-builder.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/** : Organize components into schema.ts (definitions) and implementation .ts files with matching names
Applied to files:
libs/ui/project.jsonlibs/ui/src/dependency/import-parser.tslibs/ui/src/bundler/file-cache/component-builder.tslibs/ui/src/dependency/schemas.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.test.ts : Write validation tests covering invalid variant/options, unknown properties, and valid option acceptance
Applied to files:
libs/ui/project.jsonlibs/ui/src/dependency/__tests__/resolver.test.tsapps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/react-tools.e2e.test.tslibs/ui/src/adapters/__tests__/dual-payload.test.tsapps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.tslibs/ui/src/dependency/__tests__/template-loader.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/ui/project.jsonlibs/ui/src/dependency/__tests__/resolver.test.tsapps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/react-tools.e2e.test.tslibs/ui/src/adapters/__tests__/dual-payload.test.tslibs/ui/src/adapters/serving-mode.tsapps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.tslibs/ui/src/dependency/__tests__/template-loader.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.ts : Hard-code CDN URLs only in theme presets; always reference theme.cdn configuration in component code
Applied to files:
libs/ui/project.jsonlibs/ui/src/dependency/cdn-registry.tslibs/ui/src/dependency/__tests__/template-loader.test.tslibs/ui/src/dependency/import-map.tslibs/ui/src/bundler/file-cache/component-builder.tslibs/ui/src/dependency/template-loader.tslibs/ui/src/dependency/resolver.tslibs/ui/src/dependency/schemas.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.ts : Use pure HTML string generation without React/Vue/JSX - components return HTML strings only
Applied to files:
libs/ui/project.jsonapps/e2e/demo-e2e-ui/src/apps/widgets/tools/static-badge.tool.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/*/src/common/records/**/*.ts : Centralize record types in common/records and import from there instead of from module-specific files
Applied to files:
libs/ui/project.jsonlibs/ui/src/dependency/import-parser.tslibs/ui/src/dependency/resolver.tslibs/ui/src/dependency/schemas.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Every component must have a Zod schema with `.strict()` mode to reject unknown properties
Applied to files:
libs/ui/project.jsonapps/e2e/demo-e2e-ui/src/apps/widgets/tools/react-chart.tool.tslibs/ui/src/dependency/schemas.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Maintain 95%+ test coverage across statements, branches, functions, and lines
Applied to files:
libs/ui/project.jsonlibs/ui/src/dependency/__tests__/resolver.test.tsapps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/react-tools.e2e.test.tslibs/ui/src/adapters/__tests__/dual-payload.test.tsapps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.tslibs/ui/src/dependency/__tests__/template-loader.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.test.ts : Test all code paths including errors, constructor validation, and error class `instanceof` checks
Applied to files:
libs/ui/src/dependency/__tests__/resolver.test.tsapps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/react-tools.e2e.test.tslibs/ui/src/adapters/__tests__/dual-payload.test.tsapps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.tslibs/ui/src/dependency/__tests__/template-loader.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.ts : Add JSDoc examples with example tags showing basic usage and options patterns for all components
Applied to files:
apps/e2e/demo-e2e-ui/e2e/react-tools.e2e.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Validate URIs per RFC 3986 at metadata level using Zod validation with custom refinements
Applied to files:
libs/ui/src/bundler/file-cache/storage/filesystem.tslibs/ui/src/dependency/import-map.tslibs/ui/src/dependency/schemas.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Use `getCapabilities()` for dynamic capability exposure instead of hardcoding capabilities in adapters
Applied to files:
libs/ui/src/adapters/serving-mode.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
libs/ui/src/dependency/resolver.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Don't mutate rawInput in flows - use state.set() for managing flow state instead
Applied to files:
libs/sdk/src/tool/flows/call-tool.flow.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Use FrontMCP's TypeScript-first schema validation framework philosophy - all types should align with MCP protocol definitions
Applied to files:
libs/sdk/src/tool/flows/call-tool.flow.tslibs/ui/src/dependency/schemas.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Use HTMX schema with strict mode including get, post, put, delete, target, swap, and trigger properties
Applied to files:
libs/ui/src/dependency/schemas.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Use consistent enum naming for variants ('primary', 'secondary', 'outline', 'ghost', 'danger', 'success') and sizes ('xs', 'sm', 'md', 'lg', 'xl')
Applied to files:
libs/ui/src/dependency/schemas.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Use Zod schema validation for all component inputs as a core validation strategy
Applied to files:
libs/ui/src/dependency/schemas.ts
🧬 Code graph analysis (17)
libs/ui/src/bundler/file-cache/storage/interface.ts (1)
libs/ui/src/dependency/types.ts (3)
BuildCacheStorage(418-461)ComponentBuildManifest(327-408)CacheStats(466-491)
apps/e2e/demo-e2e-ui/src/apps/widgets/tools/hybrid-status.tool.ts (3)
libs/cli/src/templates/3rd-party-integration/src/tools/example.list.ts (3)
inputSchema(12-17)Input(19-19)Output(41-41)apps/e2e/demo-e2e-ui/src/apps/widgets/tools/react-chart.tool.ts (1)
Tool(39-66)apps/e2e/demo-e2e-ui/src/apps/widgets/tools/static-badge.tool.ts (1)
Tool(31-72)
apps/e2e/demo-e2e-ui/src/apps/widgets/tools/react-chart.tool.ts (2)
apps/e2e/demo-e2e-ui/src/apps/widgets/tools/hybrid-status.tool.ts (1)
Tool(34-104)apps/e2e/demo-e2e-ui/src/apps/widgets/tools/static-badge.tool.ts (1)
Tool(31-72)
libs/ui/src/dependency/__tests__/resolver.test.ts (1)
libs/ui/src/dependency/resolver.ts (8)
DependencyResolver(80-299)DependencyResolutionError(37-42)createResolver(312-320)createClaudeResolver(327-329)createOpenAIResolver(336-338)resolveDependencies(350-357)generateImportMapForPackages(368-372)NoProviderError(47-52)
apps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.ts (2)
scripts/bump-synchronized-versions.mjs (1)
result(130-130)libs/sdk/src/common/interfaces/tool.interface.ts (1)
platform(120-133)
apps/e2e/demo-e2e-ui/src/apps/widgets/tools/static-badge.tool.ts (3)
libs/cli/src/templates/3rd-party-integration/src/tools/example.list.ts (3)
inputSchema(12-17)Input(19-19)Output(41-41)apps/e2e/demo-e2e-ui/src/apps/widgets/tools/hybrid-status.tool.ts (1)
Tool(34-104)apps/e2e/demo-e2e-ui/src/apps/widgets/tools/react-chart.tool.ts (1)
Tool(39-66)
libs/ui/src/adapters/__tests__/dual-payload.test.ts (1)
libs/ui/src/adapters/dual-payload.ts (5)
buildDualPayload(122-144)DEFAULT_HTML_PREFIX(73-73)isDualPayload(172-202)parseDualPayload(208-232)DualPayloadResult(57-64)
libs/ui/src/bundler/file-cache/hash-calculator.ts (1)
libs/ui/src/dependency/types.ts (2)
CDNDependency(57-113)FileBundleOptions(142-191)
apps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.ts (3)
scripts/bump-synchronized-versions.mjs (1)
result(130-130)libs/testing/src/http-mock/http-mock.ts (1)
text(545-551)libs/sdk/src/common/interfaces/tool.interface.ts (1)
platform(120-133)
libs/ui/src/dependency/cdn-registry.ts (1)
libs/ui/src/dependency/types.ts (4)
CDNRegistry(548-548)CDNPlatformType(36-36)CDNProvider(25-25)CDNRegistryEntry(500-543)
libs/ui/src/dependency/import-parser.ts (1)
libs/ui/src/dependency/types.ts (2)
ParsedImportResult(655-675)ParsedImport(600-650)
libs/ui/src/adapters/dual-payload.ts (1)
libs/ui/src/utils/safe-stringify.ts (1)
safeStringify(30-47)
libs/ui/src/dependency/__tests__/template-loader.test.ts (2)
libs/ui/src/dependency/template-loader.ts (9)
detectTemplateSource(73-86)isFileBasedTemplate(91-93)validateTemplateUrl(106-112)detectFormatFromUrl(120-123)resolveFilePath(322-324)clearUrlCache(47-49)getUrlCache(40-42)invalidateUrlCache(486-488)needsRefetch(453-478)libs/ui/src/dependency/types.ts (1)
ResolvedTemplate(780-825)
libs/ui/src/dependency/import-map.ts (1)
libs/ui/src/dependency/types.ts (3)
ResolvedDependency(286-321)ImportMap(259-277)CDNDependency(57-113)
libs/ui/src/dependency/template-loader.ts (2)
libs/ui/src/dependency/types.ts (7)
UrlFetchResult(754-774)TemplateSource(746-749)detectTemplateMode(692-726)TemplateMode(684-684)TemplateFormat(740-740)detectFormatFromPath(927-944)ResolvedTemplate(780-825)libs/ui/src/bundler/file-cache/hash-calculator.ts (1)
sha256(28-30)
libs/ui/src/dependency/template-processor.ts (2)
libs/ui/src/dependency/types.ts (5)
TemplateSource(746-749)ResolvedTemplate(780-825)TemplateProcessingOptions(830-887)ProcessedTemplate(892-919)TemplateFormat(740-740)libs/ui/src/handlebars/index.ts (1)
HandlebarsRenderer(146-358)
libs/sdk/src/tool/flows/call-tool.flow.ts (4)
libs/ui/src/adapters/response-builder.ts (2)
ToolResponseContent(66-87)buildToolResponseContent(125-209)libs/ui/src/index.ts (3)
ToolResponseContent(79-79)resolveServingMode(71-71)buildToolResponseContent(80-80)libs/ui/src/adapters/index.ts (3)
ToolResponseContent(54-54)resolveServingMode(43-43)buildToolResponseContent(56-56)libs/ui/src/adapters/serving-mode.ts (1)
resolveServingMode(182-225)
🪛 ast-grep (0.40.0)
libs/ui/src/dependency/import-parser.ts
[warning] 200-200: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(IMPORT_PATTERNS.defaultAndNamed.source, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 217-217: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(IMPORT_PATTERNS.defaultAndNamespace.source, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 233-233: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(IMPORT_PATTERNS.named.source, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 250-250: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(IMPORT_PATTERNS.default.source, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 269-269: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(IMPORT_PATTERNS.namespace.source, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 284-284: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(IMPORT_PATTERNS.sideEffect.source, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 303-303: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(IMPORT_PATTERNS.dynamic.source, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 317-317: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(IMPORT_PATTERNS.reExport.source, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 331-331: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(IMPORT_PATTERNS.reExportAll.source, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint & Format Checks
- GitHub Check: Build Libraries
…hance metadata handling
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
libs/ui/src/dependency/__tests__/import-parser.test.ts (3)
1-296: Add input variation and error handling tests to reach 95%+ coverage.The test suite lacks coverage for input edge cases and error paths. Per coding guidelines, tests must cover all code paths including errors and maintain 95%+ coverage across all metrics.
Add tests for:
- Mixed quote styles:
it('should handle mixed quote styles', () => { const source = ` import { foo } from "react"; import { bar } from 'lodash'; `; const result = parseImports(source); expect(result.externalPackages).toContain('react'); expect(result.externalPackages).toContain('lodash'); });
- Whitespace variations:
it('should handle extra whitespace in imports', () => { const source = `import { useState , useEffect } from 'react' ;`; const result = parseImports(source); expect(result.imports[0].namedImports).toContain('useState'); expect(result.imports[0].namedImports).toContain('useEffect'); });
- Special characters in specifiers:
it('should handle special characters in package names', () => { const source = `import { foo } from '@scope/pkg-name.v2';`; const result = parseImports(source); expect(result.externalPackages).toContain('@scope/pkg-name.v2'); });
- Malformed imports (if implementation has error handling):
it('should handle malformed import syntax gracefully', () => { const source = `import { from 'react'`; expect(() => parseImports(source)).not.toThrow(); // or .toThrow() if validation exists });Based on learnings, maintain 95%+ test coverage across statements, branches, functions, and lines.
112-118: Enhance line/column edge case testing.The current test only verifies line > 0 and column >= 0. To achieve 95%+ coverage and test boundary conditions, add tests for:
- First-line import (column should be 0)
- Imports after multiple newlines (verify exact line numbers)
- Multiple imports with increasing line numbers
it('should report correct line and column for first-line import', () => { const source = `import React from 'react';`; const result = parseImports(source); expect(result.imports[0].line).toBe(1); expect(result.imports[0].column).toBe(0); }); it('should report correct line numbers after newlines', () => { const source = ` import React from 'react'; import { Chart } from 'chart.js';`; const result = parseImports(source); expect(result.imports[0].line).toBe(3); expect(result.imports[1].line).toBe(5); });Based on learnings, maintain 95%+ test coverage across all metrics.
213-226: Add explicit duplicate import statement test.While this test verifies package-level deduplication, it doesn't test duplicate import statement filtering. The implementation likely uses deduplication logic (e.g.,
seenStatementsSet) that should be verified.Add a test case:
it('should deduplicate identical import statements', () => { const source = ` import React from 'react'; import React from 'react'; import { useState } from 'react'; `; const result = parseImports(source); // Should only have 2 unique import statements, not 3 expect(result.imports.length).toBe(2); expect(result.externalPackages.length).toBe(1); });Based on learnings, maintain 95%+ test coverage across all metrics.
libs/ui/src/bundler/file-cache/storage/redis.ts (1)
178-204: Critical: Same serialization bug asget()method.Line 197 has the identical serialization bug: when
json: false,String(entry)produces"[object Object]", corrupting the cache entry.Apply this diff:
- const serialized = this.options.json ? JSON.stringify(entry) : String(entry); + const serialized = JSON.stringify(entry); await this.options.client.setex(redisKey, effectiveTtl, serialized);
🧹 Nitpick comments (2)
libs/ui/src/bundler/file-cache/storage/filesystem.ts (2)
177-182: Consider the trade-offs of fire-and-forget metadata updates.The fire-and-forget pattern improves read performance but silent failures could lead to slightly stale LRU data, potentially affecting eviction accuracy. This is generally acceptable for cache metadata, but consider whether:
- Adding structured logging (instead of console.debug) would help monitor failure rates in production
- Critical LRU accuracy scenarios might warrant awaiting the write
155-155: Consider using async file operations consistently.The synchronous
existsSync()works correctly but is inconsistent with the async/await pattern used throughout. Consider using the asyncaccess()method fromfs/promisesfor better consistency.Example:
try { await access(filePath); } catch { this.stats.misses++; this.updateHitRate(); return undefined; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/ui/src/bundler/file-cache/storage/filesystem.ts(1 hunks)libs/ui/src/bundler/file-cache/storage/redis.ts(1 hunks)libs/ui/src/dependency/__tests__/import-parser.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
libs/ui/src/**/*.ts
📄 CodeRabbit inference engine (libs/ui/CLAUDE.md)
libs/ui/src/**/*.ts: Always useescapeHtml()utility for all user-provided content to prevent XSS vulnerabilities
Hard-code CDN URLs only in theme presets; always reference theme.cdn configuration in component code
Files:
libs/ui/src/bundler/file-cache/storage/filesystem.tslibs/ui/src/bundler/file-cache/storage/redis.tslibs/ui/src/dependency/__tests__/import-parser.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Enable strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use specific error classes with MCP error codes instead of generic errors
Use type parameters with constraints instead of unconstrained generics, and preferunknownoveranyfor generic type defaults
Follow the preset pattern for hierarchical configurations across the codebase
Files:
libs/ui/src/bundler/file-cache/storage/filesystem.tslibs/ui/src/bundler/file-cache/storage/redis.tslibs/ui/src/dependency/__tests__/import-parser.test.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/ui/src/bundler/file-cache/storage/filesystem.tslibs/ui/src/bundler/file-cache/storage/redis.tslibs/ui/src/dependency/__tests__/import-parser.test.ts
libs/ui/src/**/*.test.ts
📄 CodeRabbit inference engine (libs/ui/CLAUDE.md)
libs/ui/src/**/*.test.ts: Maintain 95%+ test coverage across statements, branches, functions, and lines
Test HTML escaping for user-provided content to prevent XSS attacks
Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Files:
libs/ui/src/dependency/__tests__/import-parser.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts: Achieve and maintain 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including errors, constructor validation, and error classinstanceofchecks
Files:
libs/ui/src/dependency/__tests__/import-parser.test.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Validate URIs per RFC 3986 at metadata level using Zod validation with custom refinements
Applied to files:
libs/ui/src/bundler/file-cache/storage/filesystem.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Maintain 95%+ test coverage across statements, branches, functions, and lines
Applied to files:
libs/ui/src/dependency/__tests__/import-parser.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/ui/src/dependency/__tests__/import-parser.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.test.ts : Write validation tests covering invalid variant/options, unknown properties, and valid option acceptance
Applied to files:
libs/ui/src/dependency/__tests__/import-parser.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.test.ts : Test all code paths including errors, constructor validation, and error class `instanceof` checks
Applied to files:
libs/ui/src/dependency/__tests__/import-parser.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.test.ts : Achieve and maintain 95%+ test coverage across all metrics (statements, branches, functions, lines)
Applied to files:
libs/ui/src/dependency/__tests__/import-parser.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/*/src/index.ts : Use barrel exports (index.ts) to export all public API surface without legacy exports or aliases
Applied to files:
libs/ui/src/dependency/__tests__/import-parser.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test HTML escaping for user-provided content to prevent XSS attacks
Applied to files:
libs/ui/src/dependency/__tests__/import-parser.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Libraries
- GitHub Check: Lint & Format Checks
🔇 Additional comments (14)
libs/ui/src/bundler/file-cache/storage/filesystem.ts (5)
23-57: LGTM! Well-structured error classes.The error hierarchy is properly implemented with specific error types, optional cause tracking, and correct use of the
overridekeyword for TypeScript strict mode compliance.
356-360: Good fix! Hash-based approach resolves collision issue.The SHA-256 hash correctly addresses the previous review concern about cache key collisions. Different keys now always produce different filenames, eliminating the sanitization collision problem.
383-409: Good fix! Stats consistency restored.The
loadStats()method now correctly reads and parses each cache entry to extractentry.metadata.size, ensuring consistency with theset()method's use ofcalculateManifestSize(). This addresses the previous review concern about totalSize inaccuracy after restart.
430-476: Good fix! Eviction now uses original cache keys.The
evictLRU()method correctly readsentry.metadata.keyfrom the parsed cache entry and passes it todelete(), eliminating the previous bug where it tried to reverse-engineer the key from the filename. The special handling for corrupted files is also appropriate.
1-492: Excellent implementation! All previous review concerns addressed.The
FilesystemStorageimplementation is well-structured with:
- ✅ All three previous review issues successfully resolved (hash-based filenames, consistent stats, metadata-based eviction)
- ✅ Proper TypeScript strict mode compliance (no
any, no!)- ✅ Specific error classes with cause tracking
- ✅ Comprehensive error handling throughout
- ✅ Clean separation of concerns
- ✅ Good documentation
The optional refactoring suggestions are minor style improvements and don't block merging.
libs/ui/src/dependency/__tests__/import-parser.test.ts (2)
142-197: Good coverage of re-export patterns and combined imports.The tests for re-exports (named, namespace/all, mixed) and combined import forms (default+named, default+namespace) directly address the previous review feedback and improve branch coverage.
273-294: Helper function branches now tested.Direct unit tests for
getPackageNamenow cover scoped packages, subpaths, and edge cases, addressing the previous review's concern about untested helper branches.libs/ui/src/bundler/file-cache/storage/redis.ts (7)
17-28: LGTM! Well-designed compatibility interface.The
RedisClientinterface provides good compatibility with both ioredis and redis packages. The use ofunknownfor return types is appropriate for flexible Redis client implementations.
33-51: LGTM! Clean interface design.The
RedisStorageOptionsinterface appropriately extendsStorageOptionswith Redis-specific configuration. The requiredclientproperty ensures proper dependency injection.
209-215: LGTM! Clean existence check.The
has()method correctly uses the Redisexistscommand and returns a boolean.
220-246: LGTM! Previous type coercion bug has been fixed.Line 230 now correctly always uses
JSON.parse(data)to parse theCacheEntry, fixing the bug identified in previous reviews. The stats tracking logic is sound.
251-270: LGTM! Correct cache clearing implementation.The
clear()method appropriately useskeys()to find all entries (acceptable performance trade-off for a destructive clear operation) and properly resets statistics.
284-316: LGTM! Previous type coercion bug has been fixed.Line 304 now correctly always uses
JSON.parse(data)to parse theCacheEntry, fixing the bug identified in previous reviews. The cleanup logic properly recalculates statistics and handles corrupted entries gracefully.
351-389: LGTM! Helper methods are well-implemented.The private helper methods correctly handle stats persistence (always using JSON) and provide clean utility functions. The stats are correctly stored separately from cache entries.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
libs/ui/src/dependency/template-loader.ts (1)
289-313: Path traversal check should usepath.relativefor robustness.The current implementation on line 297 uses
startsWith(normalizedBase + sep)which was flagged in a previous review. While usingsephelps, this approach doesn't handle all edge cases on Windows (case sensitivity, normalized paths, junction points).Additionally, line 304 uses a type assertion
as NodeJS.ErrnoExceptionwhich violates coding guidelines.Apply this diff to use the recommended
path.relativeapproach:+import { resolve as resolvePath, isAbsolute, sep, relative } from 'path';// Prevent path traversal attacks - ensure resolved path stays within base directory const normalizedBase = resolvePath(basePath); - if (!absolutePath.startsWith(normalizedBase + sep) && absolutePath !== normalizedBase) { + const relativePath = relative(normalizedBase, absolutePath); + if (relativePath.startsWith('..') || isAbsolute(relativePath)) { throw new Error(`Template path escapes base directory: ${filePath}`); }Also fix the error type assertion:
} catch (error) { - const err = error as NodeJS.ErrnoException; - if (err.code === 'ENOENT') { + if (error && typeof error === 'object' && 'code' in error) { + const err = error as NodeJS.ErrnoException; + if (err.code === 'ENOENT') { - throw new Error(`Template file not found: ${absolutePath}`); - } - if (err.code === 'EACCES') { + throw new Error(`Template file not found: ${absolutePath}`); + } + if (err.code === 'EACCES') { - throw new Error(`Permission denied reading template file: ${absolutePath}`); + throw new Error(`Permission denied reading template file: ${absolutePath}`); + } + throw new Error(`Failed to read template file: ${absolutePath} - ${err.message}`); } - throw new Error(`Failed to read template file: ${absolutePath} - ${err.message}`); + throw new Error(`Failed to read template file: ${absolutePath}`); }As per coding guidelines, avoid type assertions and use proper type guards.
apps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.ts (1)
126-162: Tests now include appropriate field validation.The Continue and Cody tests validate both
labelandvaluefields (lines 139-140 and 158-159). Since these tests don't pass acolorparameter in the input, omitting color validation is correct.
🧹 Nitpick comments (5)
libs/ui/src/dependency/resolver.ts (1)
84-94: Extract duplicated platform default expression.The expression
options.platform ?? 'unknown'is evaluated twice on lines 86 and 87. Consider extracting it to improve readability:constructor(options: DependencyResolverOptions = {}) { + const platform = options.platform ?? 'unknown'; this.options = { - platform: options.platform ?? 'unknown', - preferredProviders: options.preferredProviders ?? CDN_PROVIDER_PRIORITY[options.platform ?? 'unknown'], + platform, + preferredProviders: options.preferredProviders ?? CDN_PROVIDER_PRIORITY[platform], customRegistry: options.customRegistry ?? {}, strictMode: options.strictMode ?? true, requireIntegrity: options.requireIntegrity ?? false, }; this.registry = mergeRegistries(this.options.customRegistry); }apps/e2e/demo-e2e-ui/e2e/mdx-tools.e2e.test.ts (1)
522-531: Consider asserting specific behavior for empty points array.The test uses
toBeDefined()which doesn't verify whether the operation succeeded or failed. Since the comment indicates uncertainty about schema validation, consider verifying the actual behavior and updating the assertion to be more specific.test('should reject empty points array', async ({ mcp }) => { const result = await mcp.tools.call('mdx-interactive', { topic: 'Empty Points', points: [], }); - // Empty array might succeed or fail depending on schema - // Just verify it handles the case - expect(result).toBeDefined(); + // Verify the expected behavior - update if schema allows empty arrays + expect(result).toBeError(); });apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts (3)
209-237: Consider verifying content preservation for boundary tests.The long title/summary/description tests verify success but don't confirm the content is preserved in the output. For thoroughness, consider adding assertions that the long content appears in the result.
test('should handle very long title', async ({ mcp }) => { + const longTitle = 'T'.repeat(300); const result = await mcp.tools.call('markdown-report', { - title: 'T'.repeat(300), + title: longTitle, summary: 'Summary', findings: [{ title: 'F', description: 'D', severity: 'low' }], }); expect(result).toBeSuccessful(); + const json = result.json<{ title: string }>(); + expect(json.title).toBe(longTitle); });
339-406: Consider verifying specific error messages or codes.Error handling tests verify that errors occur but don't check specific error messages or MCP error codes. For more robust validation, consider asserting on error content.
test('should reject missing title', async ({ mcp }) => { const result = await mcp.tools.call('markdown-report', { summary: 'Summary without title', findings: [{ title: 'F', description: 'D', severity: 'low' }], }); expect(result).toBeError(); + // Optionally verify error specifics: + // expect(result.error()).toContain('title'); });
689-750: Consider verifying data integrity in concurrent tests.The concurrent tests verify all calls succeed but don't confirm each result corresponds to its input. Under concurrency bugs, results could be swapped or corrupted.
test('should handle concurrent report calls', async ({ mcp }) => { const results = await Promise.all( Array.from({ length: 5 }, (_, i) => mcp.tools.call('markdown-report', { title: `Report ${i}`, summary: `Summary ${i}`, findings: [{ title: `F${i}`, description: `D${i}`, severity: 'low' }], }), ), ); expect(results).toHaveLength(5); - results.forEach((result) => { + results.forEach((result, i) => { expect(result).toBeSuccessful(); + const json = result.json<{ title: string }>(); + expect(json.title).toBe(`Report ${i}`); }); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/e2e/demo-e2e-ui/e2e/html-tools.e2e.test.ts(1 hunks)apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts(1 hunks)apps/e2e/demo-e2e-ui/e2e/mdx-tools.e2e.test.ts(1 hunks)apps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.ts(1 hunks)apps/e2e/demo-e2e-ui/e2e/react-tools.e2e.test.ts(1 hunks)apps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.ts(1 hunks)apps/e2e/demo-e2e-ui/e2e/ui.e2e.test.ts(1 hunks)libs/ui/src/bundler/file-cache/storage/redis.ts(1 hunks)libs/ui/src/dependency/resolver.ts(1 hunks)libs/ui/src/dependency/template-loader.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/e2e/demo-e2e-ui/e2e/react-tools.e2e.test.ts
- libs/ui/src/bundler/file-cache/storage/redis.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Enable strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use specific error classes with MCP error codes instead of generic errors
Use type parameters with constraints instead of unconstrained generics, and preferunknownoveranyfor generic type defaults
Follow the preset pattern for hierarchical configurations across the codebase
Files:
apps/e2e/demo-e2e-ui/e2e/ui.e2e.test.tslibs/ui/src/dependency/template-loader.tsapps/e2e/demo-e2e-ui/e2e/html-tools.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/mdx-tools.e2e.test.tslibs/ui/src/dependency/resolver.tsapps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts: Achieve and maintain 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including errors, constructor validation, and error classinstanceofchecks
Files:
apps/e2e/demo-e2e-ui/e2e/ui.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/html-tools.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/mdx-tools.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
libs/ui/src/**/*.ts
📄 CodeRabbit inference engine (libs/ui/CLAUDE.md)
libs/ui/src/**/*.ts: Always useescapeHtml()utility for all user-provided content to prevent XSS vulnerabilities
Hard-code CDN URLs only in theme presets; always reference theme.cdn configuration in component code
Files:
libs/ui/src/dependency/template-loader.tslibs/ui/src/dependency/resolver.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/ui/src/dependency/template-loader.tslibs/ui/src/dependency/resolver.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
apps/e2e/demo-e2e-ui/e2e/ui.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/html-tools.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/mdx-tools.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Maintain 95%+ test coverage across statements, branches, functions, and lines
Applied to files:
apps/e2e/demo-e2e-ui/e2e/ui.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/html-tools.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/mdx-tools.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.test.ts : Write validation tests covering invalid variant/options, unknown properties, and valid option acceptance
Applied to files:
apps/e2e/demo-e2e-ui/e2e/ui.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/html-tools.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/mdx-tools.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.test.ts : Test all code paths including errors, constructor validation, and error class `instanceof` checks
Applied to files:
apps/e2e/demo-e2e-ui/e2e/ui.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/html-tools.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/mdx-tools.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test HTML escaping for user-provided content to prevent XSS attacks
Applied to files:
apps/e2e/demo-e2e-ui/e2e/ui.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/html-tools.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/mdx-tools.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.ts : Hard-code CDN URLs only in theme presets; always reference theme.cdn configuration in component code
Applied to files:
libs/ui/src/dependency/template-loader.tslibs/ui/src/dependency/resolver.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.test.ts : Achieve and maintain 95%+ test coverage across all metrics (statements, branches, functions, lines)
Applied to files:
apps/e2e/demo-e2e-ui/e2e/html-tools.e2e.test.tsapps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.ts : Add JSDoc examples with example tags showing basic usage and options patterns for all components
Applied to files:
apps/e2e/demo-e2e-ui/e2e/mdx-tools.e2e.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
libs/ui/src/dependency/resolver.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/*/src/common/records/**/*.ts : Centralize record types in common/records and import from there instead of from module-specific files
Applied to files:
libs/ui/src/dependency/resolver.ts
🧬 Code graph analysis (3)
libs/ui/src/dependency/template-loader.ts (2)
libs/ui/src/dependency/types.ts (7)
UrlFetchResult(755-775)TemplateSource(747-750)detectTemplateMode(693-727)TemplateMode(685-685)TemplateFormat(741-741)detectFormatFromPath(928-945)ResolvedTemplate(781-826)libs/ui/src/bundler/file-cache/hash-calculator.ts (1)
sha256(28-30)
libs/ui/src/dependency/resolver.ts (4)
libs/ui/src/dependency/types.ts (5)
CDNPlatformType(37-37)CDNRegistry(549-549)ResolvedDependency(287-322)ImportMap(260-278)CDNProvider(26-26)libs/ui/src/dependency/cdn-registry.ts (5)
CDN_PROVIDER_PRIORITY(530-538)mergeRegistries(663-668)lookupPackage(551-556)resolveAllDependencies(704-761)getPackagePeerDependencies(679-694)libs/ui/src/dependency/import-parser.ts (1)
parseImports(186-358)libs/ui/src/dependency/import-map.ts (1)
createImportMap(38-54)
apps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.ts (1)
libs/sdk/src/common/interfaces/tool.interface.ts (1)
platform(120-133)
🔇 Additional comments (38)
libs/ui/src/dependency/template-loader.ts (4)
35-49: LGTM: URL cache implementation is clean.The in-memory Map-based cache with accessor functions for testing/debugging is appropriate for this use case.
73-93: LGTM: Source detection logic is correct.The delegation to
detectTemplateModeand proper mapping to discriminated union types is well-structured.
385-437: LGTM: Main resolver logic is well-structured.The
resolveTemplatefunction properly handles all template source types with appropriate format detection and content hashing. The discriminated union pattern makes the code type-safe and maintainable.
453-488: LGTM: Cache invalidation logic is sound.The
needsRefetchfunction properly handles both ETag-based validation (preferred) and TTL-based fallback (for CDNs without ETag support). The 5-minute TTL provides a good balance between freshness and performance.libs/ui/src/dependency/resolver.ts (9)
1-52: Past review comments have been properly addressed.The previous issues have been resolved:
- Non-strict mode now correctly returns
nullinstead of throwing (line 121)generateImportMapnow delegates tocreateImportMap(line 206)- The
'custom'provider is now part of theCDNProvidertype unionHowever, the coding guidelines state: "Use specific error classes with MCP error codes instead of generic errors." The current error classes (
DependencyResolutionError,NoProviderError) are specific but do not include MCP error codes. Please verify whether MCP error codes should be integrated into these errors.
104-141: LGTM!The
resolvemethod correctly handles all cases:
- Explicit overrides bypass the registry
- Non-strict mode returns
nullto signal bundling (properly addressed from previous review)- Provider selection follows platform priority with integrity checks
- Appropriate errors are thrown when resolution fails in strict mode
150-173: LGTM!The
resolveManymethod correctly:
- Resolves all dependencies including peers using
resolveAllDependencies- Filters out
nullresults for packages that should be bundled- Handles errors appropriately based on strict/non-strict mode
186-197: LGTM!The
resolveFromSourcemethod correctly parses imports and filters to only resolve packages explicitly listed in theexternalsparameter, giving developers precise control over CDN vs bundled dependencies.
205-259: LGTM!The utility methods are well-implemented:
generateImportMapcorrectly delegates tocreateImportMap(addressed from previous review)canResolveproperly handles the nullable return fromresolvegetUrluses appropriate silent failure for a convenience getter- Accessor methods provide clean access to internal state
264-279: LGTM!The
createResolvedDependencymethod uses a sensible fallback chain for version resolution (explicit → URL extraction → 'latest') and conservative defaults for ESM support.
284-298: LGTM!The version extraction regex patterns handle common CDN URL formats adequately. While some edge cases may not be covered (e.g., versions without trailing slashes), the method is a fallback mechanism with additional safeguards ('latest' default), making this implementation appropriate.
312-338: LGTM!The factory functions provide clean, type-safe shortcuts for creating resolvers. Platform-specific factories (
createClaudeResolver,createOpenAIResolver) reduce boilerplate while maintaining type safety throughOmit<DependencyResolverOptions, 'platform'>.
350-372: LGTM!The convenience functions (
resolveDependencies,generateImportMapForPackages) provide ergonomic one-liners for common workflows, properly delegating to the resolver class methods.apps/e2e/demo-e2e-ui/e2e/serving-modes.e2e.test.ts (3)
1-56: Well-structured test file with comprehensive platform coverage.The test organization is clear, with good separation of static/hybrid modes and supported/unsupported platforms. The OpenAI static mode test (lines 34-56) properly validates all three fields (
label,value,color) and includes proper client cleanup.
380-407: Good status variant coverage with correct mappings.The status variant tests (lines 380-407) properly verify the status-to-color and status-to-isHealthy mappings. Using
as constfor the statuses array ensures type safety.
474-512: Good coverage of default value behavior.The default value tests properly verify fallback behavior when optional parameters are omitted, which is important for API contract validation.
apps/e2e/demo-e2e-ui/e2e/mdx-tools.e2e.test.ts (4)
1-60: Comprehensive MDX doc tool test coverage.The basic functionality tests properly validate
sectionCountandtitlefields in the JSON response. Good use of structured assertions.
135-215: Thorough edge case testing for MDX documents.The boundary tests cover important scenarios including single/many sections, long content (5000 chars), long headings (200 chars), and empty content. This helps ensure robustness of the tool.
266-300: Good error handling test coverage.The error tests validate required field enforcement for both the document level (
title,sections) and section level (heading,content). This aligns with the coding guidelines to test all code paths including errors.
535-594: Good concurrency test coverage.The concurrent tests properly exercise parallel execution of both MDX tools and verify all operations complete successfully. The mixed tool call test is particularly valuable for validating tool isolation.
apps/e2e/demo-e2e-ui/e2e/html-tools.e2e.test.ts (4)
1-73: Well-structured HTML table tool tests.The basic functionality tests properly validate
rowCountandcolumnCountin the JSON response, covering tables with and without titles. Good foundation for the test suite.
202-260: Good XSS prevention test coverage.The XSS tests properly validate that script tags are escaped in the output (line 211) and that the tool handles various malicious inputs gracefully. The comments clarifying that XSS prevention occurs at template render time help document the security boundary. Based on learnings, this addresses the requirement to test HTML escaping for user-provided content.
396-440: Appropriate handling of special characters in card tool.The tests correctly verify that the JSON output contains raw input values, with the understanding that XSS prevention occurs at template render time. The assertion at line 406 that
json.titleequals the raw script tag is correct for the JSON layer.
262-298: Comprehensive error handling for html-table.The tests cover both missing required fields and invalid types, which is important for schema validation. This aligns with the coding guidelines to test all code paths including errors.
apps/e2e/demo-e2e-ui/e2e/ui.e2e.test.ts (4)
1-19: Good organization as a smoke test integration file.The file properly documents its role as the main integration test with references to detailed test files. The smoke test approach is appropriate for validating all tool types work together.
21-111: Appropriate smoke test coverage for all 8 tools.Each smoke test validates a key field from the JSON response, providing quick verification that all tools are functioning. The minimal input approach is correct for smoke testing.
113-136: Good discovery test coverage.The tests verify all 8 tools are registered and that resources and prompts are properly exposed, ensuring the MCP server is fully configured.
153-186: Appropriate quick platform tests.The OpenAI test validates UI presence while the Claude test verifies successful handling. Detailed platform-specific behavior is covered in
platform-detection.e2e.test.ts. Based on learnings, this provides basic platform configuration testing.apps/e2e/demo-e2e-ui/e2e/platform-detection.e2e.test.ts (5)
1-75: Comprehensive OpenAI platform tests.The OpenAI tests properly validate UI presence across client variants (ChatGPT, OpenAI) and concurrent tool calls. Good use of
hasToolUI()assertions. Based on learnings, this provides thorough platform configuration testing.
77-107: Thorough Claude dual-payload validation.The test properly validates the dual-payload structure expected from Claude, checking both the JSON data block and the markdown-wrapped HTML block. The content type assertions (lines 98, 101) and HTML fence check (line 103) ensure the correct format is returned.
285-321: Good Gemini platform handling.The tests correctly validate that Gemini clients receive JSON-only responses without UI metadata, matching the expected platform behavior. The
hasToolUI() === falseassertion is appropriate.
430-462: Valuable concurrent cross-platform data integrity test.This test validates that data integrity is maintained when multiple platforms make concurrent calls, with each client receiving the correct
maxValuebased on its index. This is important for verifying session isolation under load.
503-558: Good session isolation testing.The session isolation tests verify that separate clients maintain independent sessions and that rapid client creation/disconnection is handled correctly. This helps ensure the server properly manages client lifecycle.
apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts (5)
19-77: LGTM!Basic functionality tests are well-structured with proper type annotations and clear assertions covering finding count, title propagation, and markdown content generation.
79-164: LGTM!Severity level tests comprehensively cover all severity values (low, medium, high), mixed distributions, and properly count occurrences in the markdown output using regex matching.
240-286: LGTM!Unicode and special character tests provide good internationalization coverage including Japanese, Russian, Hebrew (RTL), emoji, and markdown formatting characters. The test at line 284 properly verifies title preservation.
409-687: LGTM!Markdown List Tool tests provide comprehensive coverage including ordered/unordered variants, completion state tracking, edge cases (empty/single/many items), Unicode handling, and proper validation of default values for optional fields.
12-16: Remove the platform configuration suggestion — it doesn't apply to E2E tests for stateless tools.The learning about platform testing applies to
libs/ui/src/**/*.test.ts(UI component tests), not E2E tests. The markdown-report and markdown-list tools are stateless data generators with no platform-dependent behavior. All E2E tests in the codebase consistently use onlypublicMode: truewithout platform configurations.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts (1)
288-354: XSS tests still don't verify actual sanitization in the output.While the tests now parse JSON and verify output structure (findingCount, markdown defined), they still don't verify that XSS payloads are actually escaped or sanitized in the markdown content. Based on learnings, tests should verify HTML escaping for user-provided content.
The comment at lines 289-292 states markdown tools preserve content as-is, but if that's the design decision, these tests should verify that downstream rendering handles sanitization, or the comment should clarify the security boundary.
Add assertions to verify XSS content is escaped or sanitized:
test('should safely handle script tags in title', async ({ mcp }) => { const result = await mcp.tools.call('markdown-report', { title: '<script>alert("xss")</script>', summary: 'Summary', findings: [{ title: 'Finding', description: 'Desc', severity: 'low' }], }); expect(result).toBeSuccessful(); const json = result.json<{ title: string; markdown: string; findingCount: number }>(); - // Title is preserved in output (sanitization is at render layer) - expect(json.title).toBe('<script>alert("xss")</script>'); - // Verify output structure is valid + // Verify XSS content is escaped in markdown output + expect(json.markdown).not.toContain('<script>'); + expect(json.markdown).toContain('<script>'); expect(json.findingCount).toBe(1); - expect(json.markdown).toBeDefined(); }); test('should safely handle script tags in findings', async ({ mcp }) => { const result = await mcp.tools.call('markdown-report', { title: 'XSS Test Report', summary: 'Testing XSS prevention', findings: [ { title: '<img src=x onerror=alert(1)>', description: '<script>document.cookie</script>', severity: 'high', }, ], }); expect(result).toBeSuccessful(); const json = result.json<{ markdown: string; findingCount: number }>(); - // Verify tool processed the input correctly + // Verify XSS payloads are sanitized in output + expect(json.markdown).not.toMatch(/<script|onerror=/i); + expect(json.markdown).toContain('<'); expect(json.findingCount).toBe(1); - expect(json.markdown).toBeDefined(); }); test('should safely handle javascript protocol in links', async ({ mcp }) => { const result = await mcp.tools.call('markdown-report', { title: 'Link Injection Test', summary: '[Click me](javascript:alert(1))', findings: [{ title: 'Finding', description: 'Desc', severity: 'low' }], }); expect(result).toBeSuccessful(); const json = result.json<{ markdown: string }>(); - expect(json.markdown).toBeDefined(); + // Verify javascript: protocol is removed or neutralized + expect(json.markdown).not.toContain('javascript:'); }); test('should safely handle event handlers in content', async ({ mcp }) => { const result = await mcp.tools.call('markdown-report', { title: '<div onmouseover="alert(1)">Hover</div>', summary: '<img onload="alert(1)" src="valid.jpg">', findings: [{ title: 'F', description: 'D', severity: 'low' }], }); expect(result).toBeSuccessful(); const json = result.json<{ title: string; markdown: string }>(); - expect(json.title).toBeDefined(); - expect(json.markdown).toBeDefined(); + // Verify event handlers are stripped or escaped + expect(json.markdown).not.toMatch(/onmouseover|onload/i); + expect(json.markdown).toContain('<'); });Based on learnings, test HTML escaping for user-provided content to prevent XSS attacks.
🧹 Nitpick comments (1)
libs/ui/src/dependency/template-loader.ts (1)
289-303: Path traversal protection could be more robust.While using
path.sepis an improvement, the most reliable cross-platform approach is to usepath.relative()to verify the resolved path doesn't escape the base directory. The past review comment suggests this approach but was marked as addressed.Consider this more robust implementation:
// Resolve to absolute path const absolutePath = isAbsolute(filePath) ? filePath : resolvePath(basePath, filePath); // Prevent path traversal attacks - ensure resolved path stays within base directory const normalizedBase = resolvePath(basePath); - if (!absolutePath.startsWith(normalizedBase + sep) && absolutePath !== normalizedBase) { - throw new Error(`Template path escapes base directory: ${filePath}`); - } + const relative = require('path').relative(normalizedBase, absolutePath); + if (relative.startsWith('..') || require('path').isAbsolute(relative)) { + throw new Error(`Template path escapes base directory: ${filePath}`); + }This approach works reliably across platforms by checking if the relative path attempts to go up directories or is absolute.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts(1 hunks)libs/ui/src/dependency/template-loader.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Enable strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use specific error classes with MCP error codes instead of generic errors
Use type parameters with constraints instead of unconstrained generics, and preferunknownoveranyfor generic type defaults
Follow the preset pattern for hierarchical configurations across the codebase
Files:
apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.tslibs/ui/src/dependency/template-loader.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts: Achieve and maintain 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including errors, constructor validation, and error classinstanceofchecks
Files:
apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
libs/ui/src/**/*.ts
📄 CodeRabbit inference engine (libs/ui/CLAUDE.md)
libs/ui/src/**/*.ts: Always useescapeHtml()utility for all user-provided content to prevent XSS vulnerabilities
Hard-code CDN URLs only in theme presets; always reference theme.cdn configuration in component code
Files:
libs/ui/src/dependency/template-loader.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/ui/src/dependency/template-loader.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test HTML escaping for user-provided content to prevent XSS attacks
Applied to files:
apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Maintain 95%+ test coverage across statements, branches, functions, and lines
Applied to files:
apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.test.ts : Write validation tests covering invalid variant/options, unknown properties, and valid option acceptance
Applied to files:
apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.test.ts : Achieve and maintain 95%+ test coverage across all metrics (statements, branches, functions, lines)
Applied to files:
apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.test.ts : Test all code paths including errors, constructor validation, and error class `instanceof` checks
Applied to files:
apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.ts : Always use `escapeHtml()` utility for all user-provided content to prevent XSS vulnerabilities
Applied to files:
apps/e2e/demo-e2e-ui/e2e/markdown-tools.e2e.test.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.ts : Hard-code CDN URLs only in theme presets; always reference theme.cdn configuration in component code
Applied to files:
libs/ui/src/dependency/template-loader.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
libs/ui/src/dependency/template-loader.ts
🧬 Code graph analysis (1)
libs/ui/src/dependency/template-loader.ts (2)
libs/ui/src/dependency/types.ts (6)
UrlFetchResult(755-775)TemplateSource(747-750)TemplateMode(685-685)TemplateFormat(741-741)detectFormatFromPath(928-945)ResolvedTemplate(781-826)libs/ui/src/bundler/file-cache/hash-calculator.ts (1)
sha256(28-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Libraries
- GitHub Check: Lint & Format Checks
🔇 Additional comments (15)
libs/ui/src/dependency/template-loader.ts (15)
1-26: LGTM!The imports and module documentation are well-structured. Proper use of Node.js promises API and clear package documentation.
27-49: LGTM!The URL cache implementation is clean with proper accessor functions for testing. The in-memory Map approach is appropriate for template caching.
51-86: LGTM!The template source detection logic is clean and well-documented with helpful examples. Good use of discriminated unions for type safety.
88-93: LGTM!Simple utility function with clear purpose.
106-112: LGTM!The HTTPS-only validation is a good security practice for template URLs.
114-123: LGTM!Clean delegation to the format detection utility.
125-145: LGTM!The options interface is well-documented with sensible defaults.
162-223: LGTM! Error handling properly fixed.The error handling at lines 196 and 218 now correctly uses type guards (
error instanceof Error && error.name === 'AbortError') instead of type assertions, addressing the previous review concern. The ETag-based caching logic is well-implemented with proper timeout handling.
225-248: LGTM!Clean response processing with proper error handling and cache management. Good use of nullish coalescing for optional headers.
257-269: LGTM!The options interface is well-documented with appropriate defaults.
315-324: LGTM!Clean utility function for path resolution.
333-356: LGTM!Well-documented options interface with sensible defaults for the main resolver.
385-437: LGTM!The main template resolver is well-structured with clear handling for each source type (inline, file, URL). The use of format detection and override options provides good flexibility. Content hashing for cache validation is a solid approach.
439-478: LGTM!Smart cache invalidation logic with ETag-based validation and TTL fallback for CDNs without ETag support. The 5-minute TTL provides a good balance between freshness and performance.
480-488: LGTM!Simple and effective cache invalidation utility.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.