Merged
Conversation
…e, FmcpButton, FmcpCard, FmcpList, FmcpModal, FmcpSelect, FmcpTable, and FmcpTextField
…ponding schemas and templates
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 100 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-03-02T03:09:37.792Z |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
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 (5)
libs/cli/src/shared/fs.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorFile path comment is inconsistent with actual file location.
The comment indicates
libs/cli/src/utils/fs.tsbut the file is located atlibs/cli/src/shared/fs.ts.Suggested fix
-// file: libs/cli/src/utils/fs.ts +// file: libs/cli/src/shared/fs.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/shared/fs.ts` at line 1, The top-of-file path comment is incorrect — it reads "// file: libs/cli/src/utils/fs.ts" while this file is actually libs/cli/src/shared/fs.ts; update that header comment to the correct path string so the file comment matches the real location (replace the utils path with shared), ensuring any references to this file in comments or documentation use "libs/cli/src/shared/fs.ts".libs/cli/src/shared/env.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorFile path comment is inconsistent with actual file location.
The comment indicates
libs/cli/src/utils/env.tsbut the file is located atlibs/cli/src/shared/env.ts. This could cause confusion during maintenance.Suggested fix
-// file: libs/cli/src/utils/env.ts +// file: libs/cli/src/shared/env.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/shared/env.ts` at line 1, Update the top-of-file path comment in libs/cli/src/shared/env.ts so it matches the actual file location (change "libs/cli/src/utils/env.ts" to "libs/cli/src/shared/env.ts"); locate the header or inline comment at the top of the file and correct the path string to avoid future maintenance confusion.libs/cli/jest.config.ts (1)
36-43:⚠️ Potential issue | 🟠 MajorCoverage thresholds are below repository minimums.
At Lines 39-42, global thresholds (46–53%) are below the required minimum and will allow substantial coverage regressions.
Proposed fix
coverageThreshold: { global: { - statements: 47, - branches: 47, - functions: 53, - lines: 46, + statements: 95, + branches: 95, + functions: 95, + lines: 95, }, },As per coding guidelines: "/*.{test,spec}.{ts,tsx}: Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/jest.config.ts` around lines 36 - 43, The global Jest coverageThreshold values in coverageThreshold.global are set too low; update the coverageThreshold configuration (coverageThreshold.global statements, branches, functions, lines) in jest.config.ts so all four metrics meet the repository minimum (set each to 95) to enforce 95%+ coverage across statements, branches, functions and lines.libs/cli/package.json (1)
26-34:⚠️ Potential issue | 🟠 MajorFix broken dev scripts—they reference non-existent
src/cli.ts.The dev scripts at lines 33-34 point to
tsx src/cli.ts, but this file does not exist. The actual CLI entry point issrc/core/cli.ts. Runningnpm run devwill fail. Update both scripts to usesrc/core/cli.ts:Package.json lines 32-35 fix
"scripts": { "build": "tsc", - "dev": "tsx src/cli.ts", - "dev-inspect": "tsx src/cli.ts inspector", + "dev": "tsx src/core/cli.ts", + "dev-inspect": "tsx src/core/cli.ts inspector", "prepare": "npm run build"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/package.json` around lines 26 - 34, Update the npm scripts "dev" and "dev-inspect" in package.json to point to the real CLI entry (src/core/cli.ts) instead of the nonexistent src/cli.ts; locate the "scripts" section and change the command arguments for the "dev" and "dev-inspect" scripts (referencing the "dev" and "dev-inspect" script entries) so they run tsx src/core/cli.ts (and tsx src/core/cli.ts inspector) respectively.docs/frontmcp/ui/overview.mdx (1)
185-203:⚠️ Potential issue | 🟡 MinorQuick Example import and usage are inconsistent.
Line 185 imports
Card/Badge, but Line 195 callscard(...). This makes the sample internally inconsistent for readers copying it.Suggested minimal fix
-import { Card, Badge } from '@frontmcp/ui/components'; +import { card } from '@frontmcp/uipack';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/ui/overview.mdx` around lines 185 - 203, The example imports `Card` and `Badge` but the UI template uses `card(...)`, causing inconsistency; update the sample so the component call matches the import (either change the import to `card`/`card`-factory or replace the `card(...)` invocation with the imported `Card` component usage), ensuring the symbols `Card`, `Badge`, and the `ui.template`/`card` call in the `@Tool` definition are consistent across the snippet.
🟡 Minor comments (8)
libs/cli/e2e/run-e2e.sh-503-511 (1)
503-511:⚠️ Potential issue | 🟡 MinorBug:
$?is captured afterechoruns, always showing 0.On line 508, the exit code
$?is expanded inside the error message, but by that point$?reflects the exit status of the precedingecho(which succeeded), not the failednodecommand. The actual exit code is lost.🐛 Proposed fix to capture exit code before echo
-if node "dist/${APP_NAME}-cli.bundle.js" --help > /dev/null 2>&1; then +if CLI_OUTPUT=$(node "dist/${APP_NAME}-cli.bundle.js" --help 2>&1); then echo " ✅ CLI --help exited successfully" # Show output for debugging - node "dist/${APP_NAME}-cli.bundle.js" --help 2>&1 | head -5 | sed 's/^/ /' + echo "$CLI_OUTPUT" | head -5 | sed 's/^/ /' else + EXIT_CODE=$? - echo " ❌ CLI --help failed (exit code: $?)" + echo " ❌ CLI --help failed (exit code: $EXIT_CODE)" - node "dist/${APP_NAME}-cli.bundle.js" --help 2>&1 | tail -5 | sed 's/^/ /' + echo "$CLI_OUTPUT" | tail -5 | sed 's/^/ /' exit 1 fiThis also avoids running the CLI twice—once for the check and once for debug output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/e2e/run-e2e.sh` around lines 503 - 511, The error is that the node command's exit code ($?) is read after echo, losing the real code and the CLI is run twice; modify the run-e2e.sh logic to execute node "dist/${APP_NAME}-cli.bundle.js" --help once, capture both its stdout/stderr into a variable and its exit code into a variable (e.g., help_output and help_exit_code) immediately after the command, then branch on help_exit_code to print success or failure and display head/tail of help_output accordingly, using the stored help_exit_code in the failure message; update the block that currently invokes node twice to use these captured symbols instead.docs/frontmcp/deployment/runtime-modes.mdx-12-16 (1)
12-16:⚠️ Potential issue | 🟡 MinorClarify Handler mode entry point to reflect the actual serverless flow.
Lines 16 and 358 list
createHandler()as a Handler mode entry point, but this is inaccurate. In serverless deployments, developers do not callcreateHandler()directly; they set theFRONTMCP_SERVERLESS=1environment variable (which triggers the decorator to callcreateHandler()internally) and then callgetServerlessHandlerAsync()from the entry point. The table should either reference onlyFRONTMCP_SERVERLESS=1as the triggering mechanism or clarify the relationship between these APIs to avoid misleading users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/deployment/runtime-modes.mdx` around lines 12 - 16, Update the Handler Mode entry to correctly reflect the serverless flow: remove or clarify the standalone listing of createHandler() and indicate that Handler Mode is triggered by setting FRONTMCP_SERVERLESS=1 (which causes the decorator to call createHandler() internally) and that user code should call getServerlessHandlerAsync() from the serverless entry point; reference the symbols FRONTMCP_SERVERLESS=1, createHandler(), and getServerlessHandlerAsync() to make the relationship explicit.libs/cli/README.md-214-231 (1)
214-231:⚠️ Potential issue | 🟡 MinorAdd language specification to the fenced code block.
The directory structure code block is missing a language specification, which triggers a markdownlint warning. Add a language identifier (e.g.,
textor leave it as plain markdown) for better rendering and linting compliance.📝 Proposed fix
-``` +```text libs/cli/src/ ├── core/ # CLI entry, program factory, args, help, bridge🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/README.md` around lines 214 - 231, The fenced directory tree code block starting with "libs/cli/src/" is missing a language tag; change the opening fence from ``` to ```text (leave the closing fence as ```), so the block becomes a ```text fenced block to satisfy markdownlint and improve rendering. Ensure any other similar directory-tree blocks in the same README use the same `text` tag for consistency.apps/e2e/demo-e2e-uipack/e2e/browser/xss.pw.test.ts-62-81 (1)
62-81:⚠️ Potential issue | 🟡 MinorEvent listener registered after navigation may miss errors during page load.
The
pageerrorlistener is attached afterpage.goto(url)completes. Any parse errors that occur during the initial page load/script execution might not be captured.Consider moving the event listener registration before
page.goto:🛡️ Proposed fix
test('should handle unicode line and paragraph separators without parse errors', async ({ page }) => { const { url } = await serveShell('<div id="app">OK</div>', { toolName: 'test', output: { text: 'line\u2028separator\u2029paragraph' }, }); - await page.goto(url); - // Page should load without JS errors const errors: string[] = []; page.on('pageerror', (err) => errors.push(err.message)); + await page.goto(url); + // Content should render const appEl = page.locator('#app'); await expect(appEl).toHaveText('OK');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-uipack/e2e/browser/xss.pw.test.ts` around lines 62 - 81, The pageerror listener is being registered after page.goto in the test ('should handle unicode line and paragraph separators without parse errors'), which can miss errors that occur during initial navigation; move the registration of page.on('pageerror', ...) (and creation of the errors array) to before calling page.goto(url) so any runtime/parse errors during load are captured, keeping the rest of the assertions (appEl lookup, expect toHaveText('OK'), and expect(errors).toEqual([])) unchanged and still referencing the same symbols (serveShell, page.goto, page.on('pageerror')).libs/cli/src/core/__tests__/program.spec.ts-43-46 (1)
43-46:⚠️ Potential issue | 🟡 MinorReplace non-null assertions with explicit guards.
Multiple tests use
!onfind()results (lines 43, 58, 73, 88, 129). This violates coding guidelines.🛡️ Proposed pattern for all occurrences
- const buildCmd = program.commands.find((c) => c.name() === 'build')!; - buildCmd.action(() => { + const buildCmd = program.commands.find((c) => c.name() === 'build'); + if (!buildCmd) { + throw new Error('Expected build command to be registered'); + } + buildCmd.action(() => {Apply the same pattern to lines 58, 73, 88, and 129.
As per coding guidelines: "Do not use non-null assertions (!); use proper error handling."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/core/__tests__/program.spec.ts` around lines 43 - 46, The test currently uses a non-null assertion when grabbing commands (e.g., const buildCmd = program.commands.find((c) => c.name() === 'build')!) — replace the `!` usage with an explicit guard: check the result of program.commands.find(...) and if it's undefined, call fail/test.throw or throw a clear Error so the test fails with a helpful message; apply the same change for the other occurrences that use `!` (the finds at the locations for the 'build' command and the finds at lines referenced around 58, 73, 88, and 129) to satisfy the "no non-null assertions" guideline.libs/cli/src/core/__tests__/program.spec.ts-14-37 (1)
14-37:⚠️ Potential issue | 🟡 MinorCommand count mismatch: test asserts 20 but array contains 18.
The test description says "should register all 20 commands" but the expected array only contains 18 command names. This will cause the test to fail or the assertion is incorrect.
🐛 Verify and fix the count
Count the array elements and update the test description accordingly:
- it('should register all 20 commands', () => { + it('should register all 18 commands', () => {Or add the 2 missing commands if they should be registered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/core/__tests__/program.spec.ts` around lines 14 - 37, The test 'should register all 20 commands' in program.spec.ts is inconsistent with the expected array of command names produced by createProgram() (program.commands); either update the test description to reflect the actual count (18) or add the two missing command registrations to createProgram() so the array contains 20 names; locate createProgram() and program.commands usage in the test and then either change the it(...) title to "should register all 18 commands" or modify createProgram() to register the two omitted commands so the expected array and the description match.libs/cli/src/core/__tests__/help.spec.ts-63-72 (1)
63-72:⚠️ Potential issue | 🟡 MinorAvoid non-null assertion; use proper error handling.
The non-null assertion at line 65 violates coding guidelines. If
buildcommand is missing, this would throw an unhelpful error.🛡️ Proposed fix
it('should show subcommand help for build', () => { const program = createProgram(); - const buildCmd = program.commands.find((c) => c.name() === 'build')!; + const buildCmd = program.commands.find((c) => c.name() === 'build'); + if (!buildCmd) { + throw new Error('Expected build command to be registered'); + } let output = ''; buildCmd.configureOutput({ writeOut: (str) => (output += str) });As per coding guidelines: "Do not use non-null assertions (!); use proper error handling and throw specific errors instead."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/core/__tests__/help.spec.ts` around lines 63 - 72, The test uses a non-null assertion on buildCmd when calling createProgram() and find(...), which should be replaced with explicit error handling: retrieve the command via createProgram() and const buildCmd = program.commands.find(c => c.name() === 'build'); then assert/throw if buildCmd is undefined (e.g., throw new Error('build command not found in createProgram()')) before calling buildCmd.configureOutput(...) and buildCmd.outputHelp(); this ensures a clear failure message and avoids the use of !.docs/frontmcp/nx-plugin/overview.mdx-57-63 (1)
57-63:⚠️ Potential issue | 🟡 MinorGenerator count/table is now inconsistent with the generator catalog.
Line 57 still presents 14 generators and Line 61-62 omits newer entries (
job,workflow, and UI generators), while the dedicated generators overview now lists 19. Please align this overview to avoid conflicting docs.Suggested doc update
-### Generators (14) +### Generators (19) | Category | Generators | | -------------- | -------------------------------------------------------------------------------------------------------- | | **Structural** | `workspace`, `app`, `lib`, `server` | -| **Component** | `tool`, `resource`, `prompt`, `skill`, `agent`, `provider`, `plugin`, `adapter`, `auth-provider`, `flow` | +| **Component** | `tool`, `resource`, `prompt`, `skill`, `agent`, `provider`, `plugin`, `adapter`, `auth-provider`, `flow`, `job`, `workflow` | +| **UI** | `ui-component`, `ui-page`, `ui-shell` |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/nx-plugin/overview.mdx` around lines 57 - 63, Update the generator overview header and table to match the generators catalog: change the "Generators (14)" header to "Generators (19)" and add the missing entries (`job`, `workflow`, and UI-related generators) into the table row currently listing Component generators (`tool`, `resource`, `prompt`, `skill`, `agent`, `provider`, `plugin`, `adapter`, `auth-provider`, `flow`) so the Component cell includes the new items (e.g., `job`, `workflow`, `ui-*` or whatever exact UI generator names are used in the generators overview); ensure the table content matches the dedicated generators overview exactly so both lists are consistent.
🧹 Nitpick comments (12)
apps/e2e/demo-e2e-ui/src/apps/widgets/tools/chart-ui.tsx (1)
79-79: Optional: extractcardElevationto avoid duplication.The same ternary is repeated in both Card render paths; a local constant would reduce repetition.
♻️ Suggested refactor
const theme = useTheme(); + const cardElevation = theme === 'dark' ? 3 : 1; const title = input?.title || 'Chart'; @@ - <Card title={title} elevation={theme === 'dark' ? 3 : 1}> + <Card title={title} elevation={cardElevation}> @@ - elevation={theme === 'dark' ? 3 : 1} + elevation={cardElevation}Also applies to: 94-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-ui/src/apps/widgets/tools/chart-ui.tsx` at line 79, The Card elevation ternary (theme === 'dark' ? 3 : 1) is duplicated in the Card render branches; extract it into a local constant (e.g., const cardElevation = theme === 'dark' ? 3 : 1) near the top of the Chart UI component (chart-ui.tsx) and replace both elevation props on the Card elements with cardElevation to remove repetition and improve clarity; ensure the constant is defined in the same scope where title and theme are available.apps/e2e/demo-e2e-ui/src/apps/widgets/tools/form-ui.tsx (1)
128-128: Optional: reuse onecardElevationconstant across both Card paths.This improves readability and keeps render branches aligned.
♻️ Suggested refactor
const theme = useTheme(); + const cardElevation = theme === 'dark' ? 3 : 1; @@ - <Card title="Dynamic Form" elevation={theme === 'dark' ? 3 : 1}> + <Card title="Dynamic Form" elevation={cardElevation}> @@ - elevation={theme === 'dark' ? 3 : 1} + elevation={cardElevation}Also applies to: 157-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-ui/src/apps/widgets/tools/form-ui.tsx` at line 128, Create a single const (e.g., cardElevation) computed from theme === 'dark' ? 3 : 1 and use that constant in both Card components instead of repeating the ternary inline; update the render in form-ui.tsx so both occurrences of <Card title="Dynamic Form" elevation={...}> and the other Card at the second location reference cardElevation, keeping the computation near the top of the component (above the JSX) for clarity.apps/e2e/demo-e2e-uipack/project.json (1)
36-52: Consider clarifying the distinction betweentestandtest:e2etargets.Both targets use the same Jest config file (
jest.e2e.config.ts). The only difference isrunInBandfortest:e2e. Consider either:
- Renaming
testto something more specific (e.g.,test:unitif there are unit tests)- Using separate config files if they serve different purposes
- Adding a comment in the config explaining the distinction
This is minor since
runInBandis appropriate for e2e tests that may have shared state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-uipack/project.json` around lines 36 - 52, The current project targets "test" and "test:e2e" both reference the same Jest config (jest.e2e.config.ts) and only differ by the runInBand option, which is confusing; update the Nx target definitions to clarify intent by either renaming the "test" target (e.g., to "test:unit" if it runs unit tests) or creating a separate Jest config (e.g., jest.unit.config.ts) and point "test" to it, or at minimum add an inline comment in the project.json next to the "test" and "test:e2e" entries explaining that "test:e2e" uses runInBand due to shared state while "test" is intended for other test types—adjust the "test" target name or jestConfig reference accordingly and ensure "test:e2e" keeps runInBand.libs/cli/src/commands/pm/__tests__/pidfile.spec.ts (1)
15-29: Consider using static imports consistently.The test file already imports
isProcessAlivestatically at the top (line 1), but then usesrequire()forreadPidFileandremovePidFileinside test functions. Unless there's a specific need for dynamic imports (e.g., module reset between tests), using the static import for all functions would be more consistent.♻️ Optional: Use static imports consistently
-import { isProcessAlive } from '../pidfile'; +import { isProcessAlive, readPidFile, removePidFile } from '../pidfile'; describe('pm.pidfile', () => { // ... isProcessAlive tests ... describe('readPidFile', () => { it('should return null for non-existent file', () => { - // Import directly - uses real paths but we use a name that won't exist - const { readPidFile } = require('../pidfile'); const result = readPidFile('__nonexistent_test_app_' + Date.now()); expect(result).toBeNull(); }); }); describe('removePidFile', () => { it('should not throw for non-existent file', () => { - const { removePidFile } = require('../pidfile'); expect(() => removePidFile('__nonexistent_test_app_' + Date.now())).not.toThrow(); }); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/commands/pm/__tests__/pidfile.spec.ts` around lines 15 - 29, Switch the dynamic require calls to static imports: add a top-level import statement like "import { readPidFile, removePidFile } from '../pidfile';" alongside the existing isProcessAlive import and remove the inline require(...) usages inside the tests so the spec uses the readPidFile and removePidFile symbols directly; do this unless you specifically need dynamic/module-reset behavior.libs/cli/src/commands/package/registry.ts (1)
15-21: Consider logging errors in the catch block.The empty catch block silently swallows errors when reading the registry. While returning an empty registry is a reasonable fallback, logging the error could help with debugging issues like corrupted registry files.
♻️ Optional: Add error logging
try { if (!fs.existsSync(filePath)) return emptyRegistry(); const content = fs.readFileSync(filePath, 'utf-8'); return JSON.parse(content) as FrontmcpRegistry; - } catch { + } catch (err) { + // Log for debugging but gracefully recover with empty registry + console.warn(`[registry] Failed to read ${filePath}:`, err); return emptyRegistry(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/commands/package/registry.ts` around lines 15 - 21, The catch block that returns emptyRegistry() swallows the error; update it to log the exception (including filePath and the error message/stack) before returning emptyRegistry() so corrupted or unreadable registry files are visible during debugging. In the try/catch around fs.existsSync/fs.readFileSync/JSON.parse (the code that returns FrontmcpRegistry), replace the empty catch with a logging call (e.g., console.error or the repo logger if available) that includes filePath and the caught error, then still return emptyRegistry().libs/cli/src/commands/scaffold/register.ts (1)
15-24: Add explicit type annotation for theoptionsparameter.At line 15, the
optionsparameter should be explicitly typed asCreateFlags(already defined in create.ts) for consistency with the coding guidelines and for clarity. While the code builds successfully, explicitly typing the action callback improves type safety and IDE support.Proposed fix
import { Command } from 'commander'; +import { CreateFlags } from './create.js'; export function registerScaffoldCommands(program: Command): void { program .command('create') .description('Scaffold a new FrontMCP project (interactive if name omitted)') .argument('[name]', 'Project name') .option('-y, --yes', 'Use defaults (non-interactive mode)') .option('--target <target>', 'Deployment target: node, vercel, lambda, cloudflare') .option('--redis <setup>', 'Redis setup: docker, existing, none (node target only)') .option('--pm <pm>', 'Package manager: npm, yarn, pnpm') .option('--cicd', 'Enable GitHub Actions CI/CD') .option('--no-cicd', 'Disable GitHub Actions CI/CD') .option('--nx', 'Scaffold an Nx monorepo instead of standalone project') - .action(async (name: string | undefined, options) => { + .action(async (name: string | undefined, options: CreateFlags) => { const { runCreate } = await import('./create.js'); await runCreate(name, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/commands/scaffold/register.ts` around lines 15 - 24, The action callback's second parameter is untyped; add an explicit type annotation of CreateFlags to the options parameter in the .action(async (name: string | undefined, options: CreateFlags) => { ... }) callback so TypeScript/IDE tooling uses the CreateFlags definition from create.ts; keep the existing property accesses (yes, target, redis, cicd, pm, nx) unchanged and import or reference CreateFlags type if necessary.libs/cli/src/core/__tests__/program.spec.ts (1)
110-116: Test name is misleading: it does throw.The test is named "should not throw on --help" but the assertion expects it to throw with message
'(outputHelp)'. Consider renaming for clarity.♻️ Suggested rename
- it('should not throw on --help', async () => { + it('should exit gracefully via outputHelp when --help is passed', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/core/__tests__/program.spec.ts` around lines 110 - 116, Rename the misleading test description: change the it block that currently reads "should not throw on --help" (the test that creates createProgram(), calls program.exitOverride(), program.configureOutput(...), and asserts await expect(program.parseAsync(['node','frontmcp','--help'])).rejects.toThrow('(outputHelp)')) to a name that reflects that it does throw (e.g. "should throw (outputHelp) on --help" or "should exit with outputHelp on --help"); update only the string passed to the it(...) that wraps the createProgram()/program.parseAsync assertion so the test name matches the asserted behavior.libs/cli/src/commands/package/register.ts (1)
12-15: Consider adding explicit type annotations foroptionsparameters.The
optionsparameter is implicitly typed. While Commander handles this at runtime, adding explicit types would improve type safety and IDE support.♻️ Example typing for install command
- .action(async (source: string, options) => { + .action(async (source: string, options: { registry?: string; yes?: boolean; port?: number }) => { const { runInstall } = await import('./install.js'); await runInstall(toParsedArgs('install', [source], options)); });Apply similar typing to
configurecommand's options at line 31.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/commands/package/register.ts` around lines 12 - 15, Add explicit type annotations for the implicit "options" parameters by importing and using the commander OptionValues type (or a suitable interface) and annotating the action callbacks; e.g., add "import type { OptionValues } from 'commander';" at the top and change the action signatures to ".action(async (source: string, options: OptionValues) => { ... })" for the register command (so runInstall and toParsedArgs('install', ...) receive typed options) and apply the same change to the configure command's action at the referenced configure block so its options parameter is also typed as OptionValues.libs/cli/src/core/bridge.ts (1)
17-56: Consider a helper to reduce repetition.The pattern
if (options['key'] !== undefined) out.key = options['key'] as Type;is repeated ~25 times. A small helper could reduce boilerplate, though this is optional since Phase-2 will replace this entirely.♻️ Optional helper pattern
function mapOption<T>(out: ParsedArgs, options: Record<string, unknown>, key: keyof ParsedArgs): void { if (options[key as string] !== undefined) { (out as Record<string, unknown>)[key as string] = options[key as string]; } } // Usage: mapOption(out, options, 'outDir'); mapOption(out, options, 'entry'); // etc.Given Phase-2 will remove this file, keeping it simple is reasonable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/core/bridge.ts` around lines 17 - 56, Replace the repeated "if (options['key'] !== undefined) out.key = options['key'] as Type" blocks with a small generic helper to DRY the code: add a function mapOption<T>(out: ParsedArgs, options: Record<string, unknown>, key: keyof ParsedArgs): void that checks options[key as string] !== undefined and assigns it to (out as Record<string, unknown>)[key as string]; then replace each block in this file (the mappings for outDir, entry, adapter, exec, cli, yes, target, redis, cicd, pm, nx, runInBand, watch, verbose, timeout, coverage, port, socket, db, background, maxRestarts, force, follow, lines, registry) with a single call like mapOption(out, options, 'outDir') to preserve current behavior while removing boilerplate.libs/nx-plugin/src/generators/ui-page/files/__className__/__className__.spec.tsx__tmpl__ (1)
1-11: Consider adding a basic render test to the template.The current template only verifies the component is defined and is a function. Adding a minimal render test would provide a better starting point for users and align with React Testing Library best practices from the coding guidelines.
💡 Suggested enhancement
import { <%= className %> } from './<%= className %>'; +import { render, screen } from '@testing-library/react'; describe('<%= className %>', () => { it('should be defined', () => { expect(<%= className %>).toBeDefined(); }); it('should be a function component', () => { expect(typeof <%= className %>).toBe('function'); }); + + it('should render without crashing', () => { + render(<<%= className %> />); + // TODO: Add assertions for rendered content + }); });Based on learnings: "Use React Testing Library for component tests" from libs/ui/CLAUDE.md.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/nx-plugin/src/generators/ui-page/files/__className__/__className__.spec.tsx__tmpl__` around lines 1 - 11, Add a minimal render test using React Testing Library for the generated component <%= className %>: import render (and optionally screen) from '@testing-library/react', render the <%= className %> component inside the spec for a simple smoke test, and add an assertion that the rendered output is present (e.g., expect(renderResult.container).toBeDefined() or assert a visible element via screen.getByText/getByRole). Update the spec block that contains the existing "should be defined" and "should be a function component" tests to include this render test so new components have a basic RTL-based verification.libs/nx-plugin/src/generators/ui-page/files/__className__/__className__.tsx__tmpl__ (1)
1-2: Remove the unusedReactdefault import from the template.The
Reactsymbol is not referenced anywhere in the generated component code. While JSX transpilation historically required this import with the classic transform, modern React 17+ JSX runtime and this project's patterns make it unnecessary. Generated components will be cleaner without it.♻️ Proposed fix
-import React from 'react'; import { Box, Typography } from '@mui/material';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/nx-plugin/src/generators/ui-page/files/__className__/__className__.tsx__tmpl__` around lines 1 - 2, Remove the unused default React import from the component template: delete the "React" symbol from the import statement in the __className__.tsx__tmpl__ file so the only import from '@mui/material' is Box and Typography; also scan the template for any remaining references to the React identifier (e.g., use of React.Fragment or React.*) and remove/replace them if present to ensure the generated component doesn't reference React.libs/nx-plugin/src/generators/ui-page/ui-page.spec.ts (1)
42-49: Also assertbuild-esmadditional entry points in this test.
ui-componenttests validate both build targets; mirroring that here improves regression protection for entry-point wiring.As per coding guidelines:
**/*.{test,spec}.{ts,tsx}should maintain high coverage across branches/paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/nx-plugin/src/generators/ui-page/ui-page.spec.ts` around lines 42 - 49, The test in ui-page.spec.ts currently only asserts that the 'build-cjs' target's options.additionalEntryPoints contains 'ui/pages/src/AdminDashboard/index.ts'; update the test (which calls uiPageGenerator) to also assert that project.targets['build-esm'].options.additionalEntryPoints contains the same entry path so both build targets are covered, mirroring the ui-component tests and improving regression protection for entry-point wiring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/demo-e2e-redis/src/apps/vault/tools/get-credentials.tool.ts`:
- Around line 39-40: The truthy checks on optional strings (e.g., in the guard
that throws in the block containing input.providerId and !input.appId and the
routing logic that calls getAllCredentials) allow empty strings to bypass
constraints; change those conditions to explicit undefined/empty checks (e.g.,
input.providerId !== undefined && input.providerId !== '') so empty strings are
treated as provided, and update the request/schema definitions for providerId,
appId (and other optional string fields referenced around lines 48 and 52) to
enforce .min(1) (or equivalent non-empty string constraint) so the validator
rejects empty strings before logic runs. Ensure the error thrown in the guard
still references providerId/appId and that getAllCredentials receives only
validated, non-empty values.
In `@apps/e2e/demo-e2e-uipack/src/apps/uipack/tools/fetch-types.tool.ts`:
- Around line 42-50: The execute method calls createTypeFetcher and then
fetchBatch using user-controlled input (input.imports, input.maxDepth,
input.timeout) without limits; add validation and caps before calling
createTypeFetcher/fetchBatch: enforce a maximum maxDepth and maximum timeout
(e.g., clamp or throw if above safe thresholds), limit the number of import
entries and reject or truncate oversized import payloads, and validate import
shapes; return a clear error when inputs are out-of-bounds and log the
rejection; update execute to sanitize inputs (use the validated/clamped values
when calling createTypeFetcher and fetchBatch) so expensive outbound work cannot
be triggered by arbitrary user input.
In `@apps/e2e/demo-e2e-uipack/src/apps/uipack/tools/load-component.tool.ts`:
- Around line 41-46: In the switch branch handling the 'import' source (the
"case 'import'" block) you currently set source = { import: input.importUrl }
and drop input.exportName; forward the exportName by adding it to the source
object when present (use input.exportName) so named exports can be selected;
update the 'case \'import\'' block that assigns source to include exportName
(passing along input.exportName) while preserving the existing required
importUrl check.
In `@apps/e2e/demo-e2e-uipack/src/apps/uipack/tools/resolve-imports.tool.ts`:
- Around line 28-31: The current rewriteImports call (used in
resolve-imports.tool.ts) lets bare side-effect imports like `import 'react';`
pass through because the upstream import-rewriter only rewrites `import/export
... from` and dynamic imports; update the rewriter in
libs/uipack/src/resolver/import-rewriter.ts to also detect ImportDeclaration
nodes with no import specifiers (side-effect imports) and pass their
source.value through the same resolver logic so the returned replacement string
is applied; ensure rewriteImports/resolver support returning a rewritten
specifier for bare imports and that resolve-imports.tool.ts continues to call
rewriteImports with the same resolver and skipPackages options so side-effect
imports are normalized consistently with other import forms.
In `@docs/frontmcp/authentication/remote.mdx`:
- Around line 161-168: Update the table entry for the "consent" option so it
reflects the actual type and default used across the docs: replace the type
`boolean` with `ConsentConfig` and change the Default from `false` to `{
enabled: false }`, and ensure any description mentions the object form (e.g.,
`consent: { enabled: true }`); look for the table row containing the `consent`
symbol and update its Type and Default columns accordingly so it matches usage
in examples and other docs like remote-proxy.mdx, modes.mdx, and local.mdx.
In `@docs/frontmcp/nx-plugin/generators/ui-shell.mdx`:
- Around line 1-77: The UI Shell Generator doc (title "UI Shell Generator", slug
"nx-plugin/generators/ui-shell") is located under docs/frontmcp/... but must
live in the draft docs tree for public API changes; move or mirror this MDX into
docs/draft/docs/ (preserving frontmatter and content), update the
slug/frontmatter if necessary to reflect its new location, and ensure any
internal import/export references (e.g., generated file examples and consumption
import) still resolve in the docs build; leave the original file removed or
replaced with a pointer to the draft location before release.
In `@docs/frontmcp/ui/getting-started.mdx`:
- Line 138: The docs import React components Card, Badge, Button, Alert from
'@frontmcp/ui/components' but examples call non-existent string-generator
helpers (card, badge, button, alert) and TemplateHelpers only exposes
escapeHtml, formatDate, formatCurrency, uniqueId, jsonEmbed; either update the
examples to use plain HTML markup with the available TemplateHelpers (replace
calls to card/badge/button/alert with equivalent static HTML strings) or
implement and export string-producing helper functions named card(), badge(),
button(), alert() from a utilities module and update the docs imports to use
those helpers; ensure references in the doc (lines showing
card/badge/button/alert and the import) use matching symbols (TemplateHelpers vs
card/Badge etc.) so examples execute.
In `@docs/frontmcp/ui/integration/validation.mdx`:
- Around line 109-110: The docs import a nonexistent function validateOptions
and incorrectly import escapeHtml from the utils subpath; update the example to
use the actual existing validateTypeFetcherOptions (or implement/export
validateOptions if that behavior is required) and import escapeHtml from the
package root instead of '@frontmcp/uipack/utils'; specifically replace
validateOptions with validateTypeFetcherOptions (or add an exported
validateOptions in the library) and change the escapeHtml import to come from
'@frontmcp/uipack' so it matches the re-exports in uipack (ensure references to
validateOptions/validateTypeFetcherOptions and escapeHtml are updated
accordingly).
In `@libs/cli/src/core/cli.ts`:
- Around line 13-22: Move the synchronous call to createProgram() inside the
existing try so any exceptions thrown by getSelfVersion() (which uses
readFileSync() and JSON.parse()) are caught; specifically, call createProgram()
within the try block, then await program.parseAsync(process.argv) and keep the
existing error handler that logs the Error (err.stack || err.message) and
exits—this ensures synchronous failures in createProgram/getSelfVersion are
handled the same way as async parse errors.
In `@libs/nx-plugin/src/generators/ui-component/ui-component.spec.ts`:
- Line 79: Tests in ui-component.spec.ts use non-null assertions on
tree.read(...) (e.g., assignment to content) which hides missing-file failures;
replace each usage of tree.read(..., 'utf-8')! with a nullable assignment (const
content = tree.read(..., 'utf-8')), then immediately guard: if (content ===
null) throw new Error(`Missing file: <filename-from-read-call>`); so subsequent
code can assume a string; apply the same change to the other two tree.read calls
in the file, ensuring the thrown error includes the read-call's filename for
clear diagnostics.
- Line 111: The Jest assertion is using .resolves.not.toThrow() incorrectly for
a Promise<void> returned by uiComponentGenerator; update the two assertions that
call uiComponentGenerator(...) to use .resolves.toBeUndefined() instead so you
assert the promise resolves (e.g., replace the .resolves.not.toThrow() on the
uiComponentGenerator(...) calls with .resolves.toBeUndefined()).
In `@libs/nx-plugin/src/generators/ui-page/ui-page.spec.ts`:
- Line 70: Replace the non-null assertions used on Tree reads (e.g., the
assignment to the variable content from
tree.read('ui/pages/src/AdminDashboard/AdminDashboard.tsx', 'utf-8')!) with
explicit null/undefined checks: call tree.read(..., 'utf-8'), verify the result
is not null, and if it is throw a clear, specific Error that explains which
fixture is missing or failed to load; do the same for the other tree.read usages
in the test file (the reads referenced around the same test: the ones at the
other two locations) so tests fail loudly with a descriptive message instead of
using the ! operator.
- Line 103: Update the test assertions that currently use
.resolves.not.toThrow() for the async uiPageGenerator calls to
.resolves.toBeUndefined() because uiPageGenerator returns Promise<void>;
specifically change the two expectations that await expect(uiPageGenerator(tree,
{ name: 'AdminDashboard', skipFormat: true })) and the similar call at the other
assertion to use .resolves.toBeUndefined() so the promise's resolved undefined
value is asserted correctly.
In `@libs/nx-plugin/src/generators/ui-shared/add-ui-entry.spec.ts`:
- Around line 163-166: Replace the non-null assertion on the tree.read call used
to populate the local variable `barrel` (i.e.,
tree.read('ui/components/src/index.ts', 'utf-8')!) with an explicit
null/undefined check and error handling; read the file into `barrel` without
`!`, assert or throw if the result is null/undefined (for example with
expect(barrel).toBeDefined() or throw new Error('...')), then proceed to run the
regex match and length assertion as before so the test no longer uses a non-null
assertion.
In `@libs/nx-plugin/src/generators/ui-shared/add-ui-entry.ts`:
- Around line 59-62: The export append is brittle: change the duplicate check
that uses existing.includes to a robust regex search that matches export
statements for entryName with either single or double quotes and optional
semicolon/whitespace (use entryName and exportLine as reference), and before
writing ensure you normalize file ending by adding a newline if existing doesn't
already end with one so concatenation won't corrupt formatting; finally, when
writing to barrelPath use the normalized existing content + the exportLine only
if the regex did not find a match.
---
Outside diff comments:
In `@docs/frontmcp/ui/overview.mdx`:
- Around line 185-203: The example imports `Card` and `Badge` but the UI
template uses `card(...)`, causing inconsistency; update the sample so the
component call matches the import (either change the import to
`card`/`card`-factory or replace the `card(...)` invocation with the imported
`Card` component usage), ensuring the symbols `Card`, `Badge`, and the
`ui.template`/`card` call in the `@Tool` definition are consistent across the
snippet.
In `@libs/cli/jest.config.ts`:
- Around line 36-43: The global Jest coverageThreshold values in
coverageThreshold.global are set too low; update the coverageThreshold
configuration (coverageThreshold.global statements, branches, functions, lines)
in jest.config.ts so all four metrics meet the repository minimum (set each to
95) to enforce 95%+ coverage across statements, branches, functions and lines.
In `@libs/cli/package.json`:
- Around line 26-34: Update the npm scripts "dev" and "dev-inspect" in
package.json to point to the real CLI entry (src/core/cli.ts) instead of the
nonexistent src/cli.ts; locate the "scripts" section and change the command
arguments for the "dev" and "dev-inspect" scripts (referencing the "dev" and
"dev-inspect" script entries) so they run tsx src/core/cli.ts (and tsx
src/core/cli.ts inspector) respectively.
In `@libs/cli/src/shared/env.ts`:
- Line 1: Update the top-of-file path comment in libs/cli/src/shared/env.ts so
it matches the actual file location (change "libs/cli/src/utils/env.ts" to
"libs/cli/src/shared/env.ts"); locate the header or inline comment at the top of
the file and correct the path string to avoid future maintenance confusion.
In `@libs/cli/src/shared/fs.ts`:
- Line 1: The top-of-file path comment is incorrect — it reads "// file:
libs/cli/src/utils/fs.ts" while this file is actually libs/cli/src/shared/fs.ts;
update that header comment to the correct path string so the file comment
matches the real location (replace the utils path with shared), ensuring any
references to this file in comments or documentation use
"libs/cli/src/shared/fs.ts".
---
Minor comments:
In `@apps/e2e/demo-e2e-uipack/e2e/browser/xss.pw.test.ts`:
- Around line 62-81: The pageerror listener is being registered after page.goto
in the test ('should handle unicode line and paragraph separators without parse
errors'), which can miss errors that occur during initial navigation; move the
registration of page.on('pageerror', ...) (and creation of the errors array) to
before calling page.goto(url) so any runtime/parse errors during load are
captured, keeping the rest of the assertions (appEl lookup, expect
toHaveText('OK'), and expect(errors).toEqual([])) unchanged and still
referencing the same symbols (serveShell, page.goto, page.on('pageerror')).
In `@docs/frontmcp/deployment/runtime-modes.mdx`:
- Around line 12-16: Update the Handler Mode entry to correctly reflect the
serverless flow: remove or clarify the standalone listing of createHandler() and
indicate that Handler Mode is triggered by setting FRONTMCP_SERVERLESS=1 (which
causes the decorator to call createHandler() internally) and that user code
should call getServerlessHandlerAsync() from the serverless entry point;
reference the symbols FRONTMCP_SERVERLESS=1, createHandler(), and
getServerlessHandlerAsync() to make the relationship explicit.
In `@docs/frontmcp/nx-plugin/overview.mdx`:
- Around line 57-63: Update the generator overview header and table to match the
generators catalog: change the "Generators (14)" header to "Generators (19)" and
add the missing entries (`job`, `workflow`, and UI-related generators) into the
table row currently listing Component generators (`tool`, `resource`, `prompt`,
`skill`, `agent`, `provider`, `plugin`, `adapter`, `auth-provider`, `flow`) so
the Component cell includes the new items (e.g., `job`, `workflow`, `ui-*` or
whatever exact UI generator names are used in the generators overview); ensure
the table content matches the dedicated generators overview exactly so both
lists are consistent.
In `@libs/cli/e2e/run-e2e.sh`:
- Around line 503-511: The error is that the node command's exit code ($?) is
read after echo, losing the real code and the CLI is run twice; modify the
run-e2e.sh logic to execute node "dist/${APP_NAME}-cli.bundle.js" --help once,
capture both its stdout/stderr into a variable and its exit code into a variable
(e.g., help_output and help_exit_code) immediately after the command, then
branch on help_exit_code to print success or failure and display head/tail of
help_output accordingly, using the stored help_exit_code in the failure message;
update the block that currently invokes node twice to use these captured symbols
instead.
In `@libs/cli/README.md`:
- Around line 214-231: The fenced directory tree code block starting with
"libs/cli/src/" is missing a language tag; change the opening fence from ``` to
```text (leave the closing fence as ```), so the block becomes a ```text fenced
block to satisfy markdownlint and improve rendering. Ensure any other similar
directory-tree blocks in the same README use the same `text` tag for
consistency.
In `@libs/cli/src/core/__tests__/help.spec.ts`:
- Around line 63-72: The test uses a non-null assertion on buildCmd when calling
createProgram() and find(...), which should be replaced with explicit error
handling: retrieve the command via createProgram() and const buildCmd =
program.commands.find(c => c.name() === 'build'); then assert/throw if buildCmd
is undefined (e.g., throw new Error('build command not found in
createProgram()')) before calling buildCmd.configureOutput(...) and
buildCmd.outputHelp(); this ensures a clear failure message and avoids the use
of !.
In `@libs/cli/src/core/__tests__/program.spec.ts`:
- Around line 43-46: The test currently uses a non-null assertion when grabbing
commands (e.g., const buildCmd = program.commands.find((c) => c.name() ===
'build')!) — replace the `!` usage with an explicit guard: check the result of
program.commands.find(...) and if it's undefined, call fail/test.throw or throw
a clear Error so the test fails with a helpful message; apply the same change
for the other occurrences that use `!` (the finds at the locations for the
'build' command and the finds at lines referenced around 58, 73, 88, and 129) to
satisfy the "no non-null assertions" guideline.
- Around line 14-37: The test 'should register all 20 commands' in
program.spec.ts is inconsistent with the expected array of command names
produced by createProgram() (program.commands); either update the test
description to reflect the actual count (18) or add the two missing command
registrations to createProgram() so the array contains 20 names; locate
createProgram() and program.commands usage in the test and then either change
the it(...) title to "should register all 18 commands" or modify createProgram()
to register the two omitted commands so the expected array and the description
match.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-ui/src/apps/widgets/tools/chart-ui.tsx`:
- Line 79: The Card elevation ternary (theme === 'dark' ? 3 : 1) is duplicated
in the Card render branches; extract it into a local constant (e.g., const
cardElevation = theme === 'dark' ? 3 : 1) near the top of the Chart UI component
(chart-ui.tsx) and replace both elevation props on the Card elements with
cardElevation to remove repetition and improve clarity; ensure the constant is
defined in the same scope where title and theme are available.
In `@apps/e2e/demo-e2e-ui/src/apps/widgets/tools/form-ui.tsx`:
- Line 128: Create a single const (e.g., cardElevation) computed from theme ===
'dark' ? 3 : 1 and use that constant in both Card components instead of
repeating the ternary inline; update the render in form-ui.tsx so both
occurrences of <Card title="Dynamic Form" elevation={...}> and the other Card at
the second location reference cardElevation, keeping the computation near the
top of the component (above the JSX) for clarity.
In `@apps/e2e/demo-e2e-uipack/project.json`:
- Around line 36-52: The current project targets "test" and "test:e2e" both
reference the same Jest config (jest.e2e.config.ts) and only differ by the
runInBand option, which is confusing; update the Nx target definitions to
clarify intent by either renaming the "test" target (e.g., to "test:unit" if it
runs unit tests) or creating a separate Jest config (e.g., jest.unit.config.ts)
and point "test" to it, or at minimum add an inline comment in the project.json
next to the "test" and "test:e2e" entries explaining that "test:e2e" uses
runInBand due to shared state while "test" is intended for other test
types—adjust the "test" target name or jestConfig reference accordingly and
ensure "test:e2e" keeps runInBand.
In `@libs/cli/src/commands/package/register.ts`:
- Around line 12-15: Add explicit type annotations for the implicit "options"
parameters by importing and using the commander OptionValues type (or a suitable
interface) and annotating the action callbacks; e.g., add "import type {
OptionValues } from 'commander';" at the top and change the action signatures to
".action(async (source: string, options: OptionValues) => { ... })" for the
register command (so runInstall and toParsedArgs('install', ...) receive typed
options) and apply the same change to the configure command's action at the
referenced configure block so its options parameter is also typed as
OptionValues.
In `@libs/cli/src/commands/package/registry.ts`:
- Around line 15-21: The catch block that returns emptyRegistry() swallows the
error; update it to log the exception (including filePath and the error
message/stack) before returning emptyRegistry() so corrupted or unreadable
registry files are visible during debugging. In the try/catch around
fs.existsSync/fs.readFileSync/JSON.parse (the code that returns
FrontmcpRegistry), replace the empty catch with a logging call (e.g.,
console.error or the repo logger if available) that includes filePath and the
caught error, then still return emptyRegistry().
In `@libs/cli/src/commands/pm/__tests__/pidfile.spec.ts`:
- Around line 15-29: Switch the dynamic require calls to static imports: add a
top-level import statement like "import { readPidFile, removePidFile } from
'../pidfile';" alongside the existing isProcessAlive import and remove the
inline require(...) usages inside the tests so the spec uses the readPidFile and
removePidFile symbols directly; do this unless you specifically need
dynamic/module-reset behavior.
In `@libs/cli/src/commands/scaffold/register.ts`:
- Around line 15-24: The action callback's second parameter is untyped; add an
explicit type annotation of CreateFlags to the options parameter in the
.action(async (name: string | undefined, options: CreateFlags) => { ... })
callback so TypeScript/IDE tooling uses the CreateFlags definition from
create.ts; keep the existing property accesses (yes, target, redis, cicd, pm,
nx) unchanged and import or reference CreateFlags type if necessary.
In `@libs/cli/src/core/__tests__/program.spec.ts`:
- Around line 110-116: Rename the misleading test description: change the it
block that currently reads "should not throw on --help" (the test that creates
createProgram(), calls program.exitOverride(), program.configureOutput(...), and
asserts await
expect(program.parseAsync(['node','frontmcp','--help'])).rejects.toThrow('(outputHelp)'))
to a name that reflects that it does throw (e.g. "should throw (outputHelp) on
--help" or "should exit with outputHelp on --help"); update only the string
passed to the it(...) that wraps the createProgram()/program.parseAsync
assertion so the test name matches the asserted behavior.
In `@libs/cli/src/core/bridge.ts`:
- Around line 17-56: Replace the repeated "if (options['key'] !== undefined)
out.key = options['key'] as Type" blocks with a small generic helper to DRY the
code: add a function mapOption<T>(out: ParsedArgs, options: Record<string,
unknown>, key: keyof ParsedArgs): void that checks options[key as string] !==
undefined and assigns it to (out as Record<string, unknown>)[key as string];
then replace each block in this file (the mappings for outDir, entry, adapter,
exec, cli, yes, target, redis, cicd, pm, nx, runInBand, watch, verbose, timeout,
coverage, port, socket, db, background, maxRestarts, force, follow, lines,
registry) with a single call like mapOption(out, options, 'outDir') to preserve
current behavior while removing boilerplate.
In
`@libs/nx-plugin/src/generators/ui-page/files/__className__/__className__.spec.tsx__tmpl__`:
- Around line 1-11: Add a minimal render test using React Testing Library for
the generated component <%= className %>: import render (and optionally screen)
from '@testing-library/react', render the <%= className %> component inside the
spec for a simple smoke test, and add an assertion that the rendered output is
present (e.g., expect(renderResult.container).toBeDefined() or assert a visible
element via screen.getByText/getByRole). Update the spec block that contains the
existing "should be defined" and "should be a function component" tests to
include this render test so new components have a basic RTL-based verification.
In
`@libs/nx-plugin/src/generators/ui-page/files/__className__/__className__.tsx__tmpl__`:
- Around line 1-2: Remove the unused default React import from the component
template: delete the "React" symbol from the import statement in the
__className__.tsx__tmpl__ file so the only import from '@mui/material' is Box
and Typography; also scan the template for any remaining references to the React
identifier (e.g., use of React.Fragment or React.*) and remove/replace them if
present to ensure the generated component doesn't reference React.
In `@libs/nx-plugin/src/generators/ui-page/ui-page.spec.ts`:
- Around line 42-49: The test in ui-page.spec.ts currently only asserts that the
'build-cjs' target's options.additionalEntryPoints contains
'ui/pages/src/AdminDashboard/index.ts'; update the test (which calls
uiPageGenerator) to also assert that
project.targets['build-esm'].options.additionalEntryPoints contains the same
entry path so both build targets are covered, mirroring the ui-component tests
and improving regression protection for entry-point wiring.
libs/nx-plugin/src/generators/ui-component/ui-component.spec.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/frontmcp/guides/role-based-authorization.mdx (1)
383-391:⚠️ Potential issue | 🟠 MajorFix the “Default to Deny” snippet to avoid copy-paste breakage.
Line 389 references
userRolesbefore it is defined in the main plugin example flow, andhasRoleis introduced without definition. As written, this can lead to a runtime/implementation error for readers following the guide.💡 Suggested doc snippet fix
-// In plugin: deny if no authorization specified -if (!metadata.authorization) { - return hasRole(userRoles, 'authenticated'); -} +// In plugin: deny if no authorization specified +const userRoles = this.extractRoles(authInfo); +if (!metadata.authorization) { + return userRoles.includes('authenticated'); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/guides/role-based-authorization.mdx` around lines 383 - 391, The snippet referencing userRoles and hasRole must be self-contained: ensure you extract roles from the request user (e.g., define userRoles from the incoming user/principal used elsewhere in the plugin flow) and call a defined role-check helper (or inline the check) instead of an undefined hasRole; update the plugin example so the conditional uses the actual roles variable (userRoles) and either references an existing helper function (e.g., hasRole(roles, 'authenticated')) that is declared earlier in the example or add a short helper definition for hasRole used in the snippet, and keep the metadata.authorization check as the gating condition.docs/frontmcp/ui/integration/validation.mdx (3)
12-22:⚠️ Potential issue | 🟡 MinorInternal implementation example also uses
validateOptions.The "How Validation Works" section at lines 12-22 demonstrates usage of
validateOptions, which the past review confirmed doesn't exist. Consider updating this section to align with the inlinesafeParsepattern, or clarify that this represents an internal API pattern ifvalidateOptionsis intended to be implemented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/ui/integration/validation.mdx` around lines 12 - 22, The docs show a non-existent helper validateOptions; update the example to use the actual inline safeParse pattern or mark it as internal: replace the validateOptions example with ButtonOptionsSchema.safeParse(options) (check the result.success and result.error) and reference componentName only in prose, or explicitly note that validateOptions is an internal helper if you intend to document it; ensure symbols mentioned are ButtonOptionsSchema, safeParse (or validateOptions if kept as internal), and the componentName string so readers can find the related code.
104-131:⚠️ Potential issue | 🟡 MinorSection title no longer matches the content.
The heading at line 104 says "Using validateOptions" but the example code now demonstrates inline
safeParsevalidation directly on the Zod schema. Update the section title to reflect the actual pattern being shown.📝 Suggested title update
-## Using validateOptions +## Creating Validated ComponentsOr alternatively:
-## Using validateOptions +## Inline Schema Validation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/ui/integration/validation.mdx` around lines 104 - 131, The section title is misleading — it says "Using validateOptions" but the example shows inline Zod validation using MyComponentSchema.safeParse inside the myComponent function; update the heading to something that matches the shown pattern (for example "Inline Zod validation with safeParse" or "Validating options inline with Zod") and ensure any surrounding text references MyComponentSchema and the safeParse approach rather than a validateOptions helper.
246-262:⚠️ Potential issue | 🟡 MinorSecurity examples still reference non-existent
validateOptionsfunction.The Security Considerations section uses
validateOptionsin its code examples (lines 249-250 and 257-258), but per the past review, this function does not exist in the codebase. These examples should be updated to use inlinesafeParsevalidation consistent with the updated pattern shown in lines 122-129.📝 Suggested fix to align with inline validation pattern
// ✅ Good - validates options AND escapes content function card(content: string, options: CardOptions = {}): string { - const validation = validateOptions(options, { schema: CardOptionsSchema, componentName: 'card' }); - if (!validation.success) return validation.error; + const result = CardOptionsSchema.safeParse(options); + if (!result.success) { + const errors = result.error.issues.map((i) => i.message).join(', '); + return `<div style="border:1px solid red; padding:8px; color:red;"> + card: invalid options — ${escapeHtml(errors)} + </div>`; + } return `<div class="card">${escapeHtml(content)}</div>`; } // ❌ Bad - validates but doesn't escape function card(content: string, options: CardOptions = {}): string { - const validation = validateOptions(options, { schema: CardOptionsSchema, componentName: 'card' }); - if (!validation.success) return validation.error; + const result = CardOptionsSchema.safeParse(options); + if (!result.success) { + return `<div>Error: ${result.error.message}</div>`; + } return `<div class="card">${content}</div>`; // XSS vulnerability! }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/ui/integration/validation.mdx` around lines 246 - 262, Replace the nonexistent validateOptions calls in both card examples with inline schema.safeParse validation: call CardOptionsSchema.safeParse(options), check result.success and return a descriptive error when false (mirroring the pattern used earlier with safeParse), then proceed to render—ensuring the first example still uses escapeHtml(content) and the second (bad) example intentionally omits escaping; update variable names to match the safeParse result (e.g., const result = CardOptionsSchema.safeParse(options)) and use result.success/result.error to locate and fix the examples.docs/frontmcp/ui/overview.mdx (1)
135-135:⚠️ Potential issue | 🟡 MinorAlign package naming between prose and code sample.
The sample import on Line 135 uses
@frontmcp/ui/components, but the surrounding section text says these helpers ship from@frontmcp/uipack. Please make both references consistent to avoid user confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/ui/overview.mdx` at line 135, The import line uses `import { resourceWidget } from '@frontmcp/ui/components'` but the prose states the helpers ship from `@frontmcp/uipack`; make them consistent by updating either the prose or the import so both reference the same package (e.g., change the import to `@frontmcp/uipack` or change the prose to mention `@frontmcp/ui/components`), and ensure the named symbol `resourceWidget` matches the chosen package export.
♻️ Duplicate comments (1)
docs/frontmcp/ui/getting-started.mdx (1)
194-194:⚠️ Potential issue | 🟠 MajorDocs example still mixes React component imports with template-helper function calls.
At Line 194, the snippet imports
Card,Badge, andAlertfrom@frontmcp/ui/components, but the same example later callscard(...)andbadge(...)(Lines 230 and 236). As written, that example is inconsistent and won’t execute as shown.💡 Suggested doc fix
-import { Card, Badge, Alert } from '@frontmcp/ui/components'; +// HTML template example: no React component imports needed here- return card(` + return ` <div class="text-center"> <div class="text-6xl mb-4">${icon}</div> <div class="text-4xl font-bold mb-2"> ${output.temperature}${tempUnit} </div> - ${badge(output.conditions, { variant: 'info' })} + <span class="inline-block px-3 py-1 rounded-full text-sm bg-blue-100 text-blue-700"> + ${helpers.escapeHtml(output.conditions)} + </span> </div> @@ - `, { - title: helpers.escapeHtml(output.location), - subtitle: 'Current Weather', - }); + `;#!/bin/bash set -euo pipefail echo "1) Confirm conflicting symbols in docs snippet:" rg -n -C2 "import \{ Card, Badge, Alert \}|return card\(|badge\(" docs/frontmcp/ui/getting-started.mdx echo echo "2) Check whether lowercase card/badge/button/alert exports exist:" rg -n --type=ts --type=tsx "export (const|function) (card|badge|button|alert)\b|export \{[^}]*\b(card|badge|button|alert)\b" libs/ui libs/uipack || trueExpected result: the docs snippet shows
Card/Badge/Alertimports while usingcard()/badge()calls, and lowercase exports are absent or come from a different helper surface than this snippet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/ui/getting-started.mdx` at line 194, The docs import line brings React components Card, Badge, Alert but the example later calls the template-helper functions card(...) and badge(...); update the example so the import and usage match: either change the import to the helper functions (import { card, badge, alert } from '@frontmcp/ui/components') if the snippet intends to demonstrate the template helpers, or change the helper calls to use the React components (e.g., render <Card> / <Badge> / <Alert>) if it intends to show JSX; ensure the symbols Card/Badge/Alert or card()/badge()/alert() are used consistently and adjust the surrounding code/snippet text accordingly.
🧹 Nitpick comments (12)
libs/cli/README.md (1)
19-132: Consider generating CLI docs from command registrations.Given the expanded matrix, manual maintenance will drift quickly. A small doc-generation step from
register.tsmetadata would reduce breakage and keep README authoritative.Also applies to: 232-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/README.md` around lines 19 - 132, The README's large manual CLI matrix should be generated from the actual command registration metadata instead of hand-editing; add a doc-generation step that reads the command registry (e.g., the register.ts file and its exported commands/registerCommands or commands array/CommandDefinition symbols), formats flags and descriptions into the markdown tables, and writes/overwrites libs/cli/README.md (or a README fragment) as part of the build or a new npm script (e.g., "frontmcp docs" or "build:docs"); ensure the generator handles all subcommands, flags, defaults, and linkable usage examples so the README always reflects register.ts source-of-truth.apps/e2e/demo-e2e-uipack/playwright.config.ts (1)
3-11: Consider removing redundant device configuration from rootuse.The
devices['Desktop Chrome']spread in rootuse(line 9) is completely overridden by the project-level configuration (line 10), since project-level settings take precedence. While the root-levelheadless: trueremains useful, the device settings are redundant.♻️ Refactor: Remove redundant device spread from root `use`
export default defineConfig({ testDir: './e2e/browser', testMatch: '**/*.pw.test.ts', fullyParallel: false, workers: 1, timeout: 30000, - use: { headless: true, ...devices['Desktop Chrome'] }, + use: { headless: true }, projects: [{ name: 'chromium', use: { ...devices['Desktop Chrome'] } }], });Note: No
webServerconfiguration is present, unlike some other E2E configs in the project. This is intentional if the dev server is managed externally (e.g., via CI orchestration or a separate startup script).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-uipack/playwright.config.ts` around lines 3 - 11, The root-level use object in defineConfig currently spreads devices['Desktop Chrome'] which is redundant because the same device is set at the project level and project-level settings override root settings; remove the devices['Desktop Chrome'] spread from the root use while keeping headless: true (i.e., keep use: { headless: true }) and leave the projects entry (projects: [{ name: 'chromium', use: { ...devices['Desktop Chrome'] } }]) unchanged so the project-level device config remains authoritative.libs/nx-plugin/src/generators/ui-component/files/__className__/__className__.spec.tsx__tmpl__ (1)
3-10: Increase generated test signal with a render smoke test.The current checks are very weak (
defined+typeof). Consider generating at least one render assertion so failures catch broken JSX/render wiring.♻️ Proposed template improvement
import { <%= className %> } from './<%= className %>'; +import { render } from '@testing-library/react'; describe('<%= className %>', () => { it('should be defined', () => { expect(<%= className %>).toBeDefined(); }); - it('should be a function component', () => { - expect(typeof <%= className %>).toBe('function'); + it('should render', () => { + const { container } = render(<<%= className %> />); + expect(container.firstChild).not.toBeNull(); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/nx-plugin/src/generators/ui-component/files/__className__/__className__.spec.tsx__tmpl__` around lines 3 - 10, The tests only check that <%= className %> is defined and is a function; replace or augment those with a render smoke test that imports render from '@testing-library/react', renders the component as JSX (e.g. render(<%= className % />)) and asserts a basic post-render condition (e.g. container is in the document or no error thrown) so the generated spec for <%= className %> actually exercises JSX/render wiring.apps/e2e/demo-e2e-uipack/webpack.config.js (1)
11-12: Consider environment-aware mode and devtool settings.The
modeis hardcoded to'development'anddevtoolis always set regardless of environment. For an e2e test application this may be intentional, but if production builds are ever needed, consider:- mode: 'development', - devtool: 'eval-cheap-module-source-map', + mode: process.env.NODE_ENV === 'production' ? 'production' : 'development', + devtool: process.env.NODE_ENV === 'production' ? false : 'eval-cheap-module-source-map',If this is exclusively for development/testing purposes, the current configuration is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-uipack/webpack.config.js` around lines 11 - 12, The webpack config currently hardcodes mode: 'development' and devtool: 'eval-cheap-module-source-map'; make these environment-aware by deriving mode from process.env.NODE_ENV (e.g., set mode to 'production' when NODE_ENV==='production' and 'development' otherwise) and conditionally set devtool (e.g., disable or use 'source-map' in production, keep 'eval-cheap-module-source-map' for development/testing) so the Webpack settings in webpack.config.js adapt to build environment; alternatively, if this file is strictly for e2e/dev, document that it is intentionally fixed to development.apps/e2e/demo-e2e-uipack/e2e/browser/helpers.ts (2)
59-69: Edge case:stopServermay hang if server never fully started.If
server.listen()was called but failed before emittinglistening, callingserver.close()might not invoke its callback. Consider adding a timeout or checking the server'slisteningproperty.♻️ Defensive handling suggestion
export async function stopServer(): Promise<void> { if (server) { const s = server; + server = null; // Clear reference early to avoid race conditions return new Promise<void>((resolve) => { - s.close(() => { - server = null; - serverUrl = null; + s.close(() => { resolve(); }); + // Fallback timeout in case close callback never fires + setTimeout(resolve, 100); }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-uipack/e2e/browser/helpers.ts` around lines 59 - 69, stopServer may hang if server.close() callback never fires when the server never reached listening; modify stopServer to guard against this by checking the server.listening property and, if not listening, call server.close() and immediately resolve (or use server.unref/cleanup), otherwise attach the close callback as now but also start a short timeout (e.g., 1-2s) that will forcefully set server = null, serverUrl = null and resolve if the close callback hasn’t run; refer to the stopServer function and the local variable s to implement the conditional/listening check and the fallback timeout.
11-12: Consider removing unusedserverUrlvariable.The
serverUrlvariable is assigned inserveHtmlbut never read outside the function scope. It's set tonullinstopServerbut this has no observable effect since the value is never consumed.♻️ Suggested cleanup
let server: http.Server | null = null; -let serverUrl: string | null = null;And in
serveHtml:if (typeof addr === 'object' && addr !== null) { - serverUrl = `http://127.0.0.1:${addr.port}`; - resolve(serverUrl); + resolve(`http://127.0.0.1:${addr.port}`);And in
stopServer:s.close(() => { server = null; - serverUrl = null; resolve();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-uipack/e2e/browser/helpers.ts` around lines 11 - 12, The variable serverUrl is never used outside serveHtml so remove it to simplify state: delete the top-level declaration let serverUrl and its assignments inside serveHtml and stopServer, keep the server: http.Server | null variable and ensure serveHtml returns or otherwise exposes the URL it needs without relying on the removed serverUrl; update any call sites that expected serverUrl by using the returned value from serveHtml or constructing the URL from server.address() within those call sites instead.apps/e2e/demo-e2e-uipack/e2e/resolver.e2e.test.ts (1)
51-63: Consider tightening the assertion for multiple packages.The test imports 3 distinct packages (
react,react-dom/client,zod) but only assertsrewrittenCount >= 2. If the resolver works correctly, all 3 should be rewritten. A stricter assertion would catch regressions more effectively.♻️ Suggested improvement
expect(result).toBeSuccessful(); const json = result.json<{ rewrittenCount: number; rewrites: Record<string, string> }>(); - expect(json.rewrittenCount).toBeGreaterThanOrEqual(2); + expect(json.rewrittenCount).toBe(3); + expect(json.rewrites['react']).toBeDefined(); + expect(json.rewrites['react-dom/client']).toBeDefined(); + expect(json.rewrites['zod']).toBeDefined();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-uipack/e2e/resolver.e2e.test.ts` around lines 51 - 63, The test "should handle multiple packages" currently asserts json.rewrittenCount >= 2 but imports three packages (react, react-dom/client, zod); update the assertion on json.rewrittenCount in the test to expect at least 3 (e.g., change the matcher from >= 2 to >= 3) so the test fails if fewer than all expected imports are rewritten, keeping the rest of the test (result retrieval via mcp.tools.call and json parsing) unchanged.apps/e2e/demo-e2e-renderer-showcase/src/components/PreviewPanel.tsx (1)
29-39: Consider adding asandboxattribute for defense-in-depth.The preview iframe loads a same-origin route with controlled demo data. While not a security requirement here, adding
sandbox="allow-same-origin"would harden the security posture with minimal risk to functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-renderer-showcase/src/components/PreviewPanel.tsx` around lines 29 - 39, The iframe rendered in PreviewPanel (the <iframe ... title="Renderer Preview" /> with key `${groupId}-${exampleIndex}` and data-testid="preview-iframe") should include a sandbox attribute for defense-in-depth; update the JSX to add sandbox="allow-same-origin" (or a minimal set of sandbox flags you need) on that iframe element so it retains same-origin behavior while restricting other capabilities.docs/frontmcp/nx-plugin/generators/overview.mdx (1)
48-52: Minor inconsistency in output path documentation.The
ui-shellgenerator shows lowercase<name>in the output path whileui-componentandui-pageuse PascalCase<Name>. If this is intentional (shells use kebab-case directories), consider adding a note for clarity. Otherwise, align the casing convention.
Generator Output ui-componentui/components/src/<Name>/ui-pageui/pages/src/<Name>/ui-shellui/shells/src/<name>/← lowercase🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/nx-plugin/generators/overview.mdx` around lines 48 - 52, The documentation shows inconsistent casing for generator output paths: `ui-component` and `ui-page` use `ui/components/src/<Name>/` and `ui/pages/src/<Name>/` while `ui-shell` lists `ui/shells/src/<name>/`; update the `ui-shell` entry to match the PascalCase convention (`<Name>`) if shells follow the same directory naming, or explicitly add a clarifying note next to the `ui-shell` row indicating that shells use kebab/lowercase directories if that is intentional; reference the `ui-component`, `ui-page`, and `ui-shell` generator rows and their output path cells when making the change.apps/e2e/demo-e2e-renderer-showcase/e2e/browser/helpers.ts (1)
22-27: Consider handling potential null frame.
contentFrame()can returnnullif the iframe hasn't loaded or is cross-origin. The calling test code should handle this, but adding an explicit return type improves clarity.♻️ Suggested type annotation
-export async function getPreviewFrame(page: Page) { +export async function getPreviewFrame(page: Page): Promise<import('@playwright/test').Frame | null> { const iframe = page.locator('[data-testid="preview-iframe"]'); await iframe.waitFor({ state: 'attached' }); const frame = await iframe.contentFrame(); return frame; }Alternatively, if the frame should always be available, consider throwing an error:
export async function getPreviewFrame(page: Page) { const iframe = page.locator('[data-testid="preview-iframe"]'); await iframe.waitFor({ state: 'attached' }); const frame = await iframe.contentFrame(); + if (!frame) { + throw new Error('Preview iframe frame not available'); + } return frame; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-renderer-showcase/e2e/browser/helpers.ts` around lines 22 - 27, The getPreviewFrame helper currently calls iframe.contentFrame() which can return null; update getPreviewFrame to explicitly handle that by either (A) changing its signature to return Promise<Frame | null> and return the possibly-null frame from iframe.contentFrame(), or (B) if a frame must always be present, throw a clear error when iframe.contentFrame() is null so callers fail fast; refer to the getPreviewFrame function and the iframe/contentFrame call to implement the chosen approach and add the appropriate return type annotation.apps/e2e/demo-e2e-uipack/e2e/browser/bridge.pw.test.ts (1)
118-138: Test name doesn't match behavior.The test is named "should dispatch bridge:ready event after initialization" but it only checks the
initializedproperty. It doesn't actually verify that thebridge:readyevent was dispatched.Consider either:
- Renaming to "should be initialized after navigation"
- Actually testing the event dispatch by setting up a listener before the bridge initializes
♻️ Option to test actual event dispatch
test.describe('Bridge Ready Event', () => { - test('should dispatch bridge:ready event after initialization', async ({ page }) => { + test('should have initialized state after navigation', async ({ page }) => { const { url } = await serveShell('<div></div>', { includeBridge: true, }); - // Set up event listener before navigating await page.goto(url); - // The bridge:ready event may have already fired, check bridge state const initialized = await page.waitForFunction( () => { const bridge = (window as Record<string, unknown>).FrontMcpBridge as Record<string, unknown> | undefined; return bridge && bridge.initialized === true; }, null, { timeout: 5000 }, ); expect(initialized).toBeTruthy(); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-uipack/e2e/browser/bridge.pw.test.ts` around lines 118 - 138, Rename the test or assert the actual event: either change the test title in the test(...) call from "should dispatch bridge:ready event after initialization" to "should be initialized after navigation" to reflect the current assertion, or update the test to listen for the DOM event before navigation by using page.evaluate to attach a window.addEventListener('bridge:ready', ...) (or use page.waitForEvent if emitting via page.emit) and resolve a Promise when the handler runs, then call page.goto(url) and await that promise instead of only checking FrontMcpBridge.initialized with page.waitForFunction; reference symbols: test(...), serveShell, page.goto, page.evaluate, FrontMcpBridge, and the 'bridge:ready' event.libs/cli/src/commands/package/install.ts (1)
138-142: Consider logging cleanup errors at debug level.The empty catch block silently swallows all cleanup errors. While the comment indicates this is intentional, logging at debug level could help diagnose issues in development without affecting production behavior.
💡 Optional: Log cleanup errors at debug level
try { fs.rmSync(tmpDir, { recursive: true, force: true }); - } catch { - // ignore cleanup errors + } catch (err) { + // Log but don't fail on cleanup errors + if (process.env.DEBUG) { + console.debug(`${c('gray', '[install]')} cleanup warning: ${err}`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/cli/src/commands/package/install.ts` around lines 138 - 142, The empty catch swallowing errors from fs.rmSync(tmpDir, { recursive: true, force: true }) should log the caught error at debug level; modify the catch to capture the error (e.g., catch (err)) and call the existing debug logger (or console.debug if no logger is available) with a short message referencing tmpDir and the error (e.g., "cleanup tmpDir failed:" + err) so cleanup failures are visible in development while preserving current non-throwing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/demo-e2e-renderer-showcase/e2e/browser/renderers.pw.test.ts`:
- Around line 99-188: The test suite "Renderer Showcase - Render-Only Mode" is
missing coverage for the video and audio renderers; add two tests mirroring the
existing pattern: create tests that call navigateToPreview(page, 'video', 0) and
navigateToPreview(page, 'audio', 0), locate the preview wrapper via
page.locator('[data-testid="preview-content"]'), assert it is visible, then
assert the renderer output is visible using selectors similar to other tests
(for video: 'video, .fmcp-video, .MuiAlert-root' and for audio: 'audio,
.fmcp-audio, .MuiAlert-root') with an appropriate timeout (e.g., 10_000 ms) so
they match the style and timing used by other tests like the 'image' and 'pdf'
cases.
In `@apps/e2e/demo-e2e-renderer-showcase/src/components/Navigation.tsx`:
- Line 1: Remove the unused top-level import "import React from 'react';" from
the Navigation component file (the import is unnecessary due to the React 17+
JSX transform) and then run the provided cleanup script node
scripts/fix-unused-imports.mjs to ensure any other unused imports are fixed;
verify the Navigation component (and any exported symbol named Navigation) still
compiles and JSX remains untouched after removal.
In `@apps/e2e/demo-e2e-renderer-showcase/src/components/SourcePanel.tsx`:
- Around line 15-16: The code dereferences group.fixtures[selectedIndex] into
fixture without checking for an empty array, which can lead to crashes; in
SourcePanel, add an explicit guard for group.fixtures.length === 0 (or
!group.fixtures || group.fixtures.length === 0) before computing/using fixture,
return/render an empty-state placeholder or early return when the list is empty,
and only compute const fixture = group.fixtures[selectedIndex] ||
group.fixtures[0] (or access fixture properties) after that guard so subsequent
uses (lines referencing fixture) never operate on undefined.
In `@apps/e2e/demo-e2e-renderer-showcase/src/ShowcaseLayout.tsx`:
- Around line 22-24: Guard against empty renderer groups/fixtures before
computing safeIndex: check that RENDERER_GROUPS has at least one group and that
getGroup(groupId) returns a group with a non-empty fixtures array; if not,
return a safe fallback render (or null) instead of proceeding. Then clamp
exampleIndex into bounds with something like clampedIndex = Math.max(0,
Math.min(exampleIndex, group.fixtures.length - 1)), use
group.fixtures[clampedIndex] for fixture, and set safeIndex = clampedIndex so
getGroup, RENDERER_GROUPS, exampleIndex, fixture, and safeIndex are all robust
to empty data.
In `@apps/e2e/demo-e2e-standalone/src/apps/isolated/tools/isolated-hello.tool.ts`:
- Line 13: The Input type and inputSchema are using a non-idiomatic pattern;
wrap the plain inputSchema definition in z.object(...) so it becomes a Zod
object schema consistent with outputSchema, then change the type alias to use
z.output<typeof inputSchema> (update the symbol names: inputSchema and type
Input) so the code uses the standard Zod v4 inference pattern; ensure any
imports/usage that reference the previous z.ZodObject form are updated to the
new inputSchema shape.
In `@apps/e2e/demo-e2e-uipack/jest.e2e.config.ts`:
- Line 11: The Jest config's testMatch currently only matches
'<rootDir>/e2e/**/*.e2e.test.ts', which excludes Playwright files; update the
testMatch entry in jest.e2e.config.ts to include both patterns (the existing
'*.e2e.test.ts' and '*.pw.test.ts') so both e2e and Playwright tests are
discovered and run (modify the testMatch array property accordingly).
In `@docs/frontmcp/getting-started/cli-reference.mdx`:
- Line 45: Update the documented CLI signature from "service <action>" to
"service <action> <app>" (or "service <action> [app]" if the app is optional) so
it matches the example usage "frontmcp service install my-app"; update the table
entry and any explanatory text so the `service` command signature (service
<action> <app>) and description ("Install/uninstall systemd/launchd service")
accurately reflect that an application name (e.g., my-app) is required/optional
for install/uninstall operations and ensure examples use the same signature.
In `@docs/frontmcp/nx-plugin/overview.mdx`:
- Around line 57-62: The generators overview is missing the three new UI
generators and still shows 14 generators; update the list and summary to reflect
17 total by adding a new **UI** category row containing `ui-component`,
`ui-page`, `ui-shell`, change the displayed total from 14 to 17, and update the
[inconsistent_summary] (or any summary variable/line that reports the generator
count) to match the new count so the docs and AI summary are consistent.
In `@docs/frontmcp/ui/overview.mdx`:
- Line 199: Sanitize and coerce ctx.output.temperature before interpolating into
the HTML snippet that currently uses ${ctx.output.temperature}: ensure you
convert it to a safe, predictable string (e.g., Number(ctx.output.temperature)
or parseFloat with a fallback) and format/round as needed (e.g., toFixed) and
then HTML-escape the resulting string using your project's safe-escape helper
(or a small escapeHtml utility) before placing it into the template so only a
validated, escaped value is rendered in the <div>.
In `@libs/cli/e2e/run-e2e.sh`:
- Around line 521-526: The script currently treats a failed "node
dist/${APP_NAME}-cli.bundle.js $FIRST_CMD --help" as a warning and continues,
which can mask real regressions; update the block that checks FIRST_CMD (the
code using FIRST_CMD and APP_NAME) so that if the node help invocation fails you
log a clear error and exit non‑zero (e.g., echo an error and call exit 1)
instead of printing the warning and continuing, ensuring the failing help check
causes the E2E to fail.
In `@libs/cli/src/commands/dev/doctor.ts`:
- Around line 72-73: Add a unit test that causes resolveEntry to reject with a
non-Error value (e.g., a string or number) and assert that the doctor
command/handler handles it via the non-Error catch branch — specifically
exercise the code path that computes firstLine in the catch (the ternary using e
instanceof Error) so the fallback 'entry not found' or the non-Error message is
used; place the test alongside other doctor tests (matching
**/*.{test,spec}.{ts,tsx}) and spy/match the output/log to verify the non-Error
branch is executed for resolveEntry.
In `@libs/cli/src/commands/dev/register.ts`:
- Line 21: Replace the raw parseInt used as the Commander option parser for "-t,
--timeout <ms>" with an explicit base-10 parser and validation: add a small
parser function (e.g., parseTimeout(value: string): number) that calls
parseInt(value, 10), checks Number.isNaN(result) (and optionally that result >=
0), and throws a clear Error on invalid input, then pass parseTimeout into
.option('-t, --timeout <ms>', 'Set test timeout (default: 60000ms)',
parseTimeout) instead of using parseInt directly so the radix ambiguity and
Commander’s extra-argument behavior are avoided.
- Around line 9-12: The action callbacks registered via .action currently accept
an untyped options parameter (implicit any) — update both action callbacks to
declare the options parameter with an explicit type (e.g., DevOptions or a new
Interface matching the command options) instead of leaving it untyped; import or
define the type and annotate the parameter in the .action(async (options:
DevOptions) => { ... }) callbacks that call runDev and toParsedArgs so
TypeScript strict mode no longer treats options as any.
In `@libs/cli/src/commands/package/register.ts`:
- Around line 11-12: The Commander option currently uses raw parseInt in the
.option('-p, --port <N>', ..., parseInt) call which misinterprets the previous
accumulated value as the radix; replace that with an explicit parser that only
parses the current string with radix 10 and validates the port range (e.g.,
1–65535), then return the numeric port (and throw or exit on invalid values);
update any references to options.port in the action handler to expect a
validated number.
In `@libs/cli/src/commands/pm/register.ts`:
- Around line 69-75: The CLI currently declares name as optional but runService
(and the handler) requires a service name; update the command so the second
positional is required and validate before calling runService: change the
argument signature from '[name]' to '<name>' (or otherwise assert name is
defined), and ensure positionals is built from [action, name] without the
optional branch; reference the command builder, the .argument call for name, the
action handler async (action, name) => { ... }, and the runService/toParsedArgs
call to keep behavior consistent.
In `@libs/cli/src/core/__tests__/program.spec.ts`:
- Around line 122-128: The test title and its assertion disagree: either rename
the test or change the assertion. Either update the spec title string from
"should not throw on --version" to "should throw on --version" to match the
existing rejects.toThrow() assertion, or keep the title and change the assertion
on createProgram().parseAsync(['node','frontmcp','--version']) to assert it does
not reject (use an assertion that expects a resolved promise) so the test name
and the behavior of program.parseAsync remain consistent.
In `@libs/cli/src/core/version.ts`:
- Around line 5-7: getSelfVersion currently computes pkgPath as join(__dirname,
'../../package.json') which points into dist/ and breaks when the package is
published; change the pkgPath resolution to reliably locate the package root
(e.g. climb out to the package root from the compiled module) before reading and
parsing: update the pkgPath computation used by getSelfVersion (replace the
existing pkgPath variable usage) so it targets the package.json at the package
root (instead of dist/package.json), then continue using readFileSync +
JSON.parse as before.
In `@libs/nx-plugin/generators.json`:
- Around line 82-97: The repo added three new public generators (ui-component,
ui-page, ui-shell) but no user docs exist; create documentation entries for each
generator in the project's draft documentation area describing what the
generator does, required/optional options (reflecting each generator's
schema.json), example CLI usage, and a short example output; ensure each doc
references the generator name (ui-component, ui-page, ui-shell), summarizes
relevant options from its schema, links or points to the generator
implementation for advanced usage, and update the docs navigation/index so these
three entries are discoverable before merging.
---
Outside diff comments:
In `@docs/frontmcp/guides/role-based-authorization.mdx`:
- Around line 383-391: The snippet referencing userRoles and hasRole must be
self-contained: ensure you extract roles from the request user (e.g., define
userRoles from the incoming user/principal used elsewhere in the plugin flow)
and call a defined role-check helper (or inline the check) instead of an
undefined hasRole; update the plugin example so the conditional uses the actual
roles variable (userRoles) and either references an existing helper function
(e.g., hasRole(roles, 'authenticated')) that is declared earlier in the example
or add a short helper definition for hasRole used in the snippet, and keep the
metadata.authorization check as the gating condition.
In `@docs/frontmcp/ui/integration/validation.mdx`:
- Around line 12-22: The docs show a non-existent helper validateOptions; update
the example to use the actual inline safeParse pattern or mark it as internal:
replace the validateOptions example with ButtonOptionsSchema.safeParse(options)
(check the result.success and result.error) and reference componentName only in
prose, or explicitly note that validateOptions is an internal helper if you
intend to document it; ensure symbols mentioned are ButtonOptionsSchema,
safeParse (or validateOptions if kept as internal), and the componentName string
so readers can find the related code.
- Around line 104-131: The section title is misleading — it says "Using
validateOptions" but the example shows inline Zod validation using
MyComponentSchema.safeParse inside the myComponent function; update the heading
to something that matches the shown pattern (for example "Inline Zod validation
with safeParse" or "Validating options inline with Zod") and ensure any
surrounding text references MyComponentSchema and the safeParse approach rather
than a validateOptions helper.
- Around line 246-262: Replace the nonexistent validateOptions calls in both
card examples with inline schema.safeParse validation: call
CardOptionsSchema.safeParse(options), check result.success and return a
descriptive error when false (mirroring the pattern used earlier with
safeParse), then proceed to render—ensuring the first example still uses
escapeHtml(content) and the second (bad) example intentionally omits escaping;
update variable names to match the safeParse result (e.g., const result =
CardOptionsSchema.safeParse(options)) and use result.success/result.error to
locate and fix the examples.
In `@docs/frontmcp/ui/overview.mdx`:
- Line 135: The import line uses `import { resourceWidget } from
'@frontmcp/ui/components'` but the prose states the helpers ship from
`@frontmcp/uipack`; make them consistent by updating either the prose or the
import so both reference the same package (e.g., change the import to
`@frontmcp/uipack` or change the prose to mention `@frontmcp/ui/components`),
and ensure the named symbol `resourceWidget` matches the chosen package export.
---
Duplicate comments:
In `@docs/frontmcp/ui/getting-started.mdx`:
- Line 194: The docs import line brings React components Card, Badge, Alert but
the example later calls the template-helper functions card(...) and badge(...);
update the example so the import and usage match: either change the import to
the helper functions (import { card, badge, alert } from
'@frontmcp/ui/components') if the snippet intends to demonstrate the template
helpers, or change the helper calls to use the React components (e.g., render
<Card> / <Badge> / <Alert>) if it intends to show JSX; ensure the symbols
Card/Badge/Alert or card()/badge()/alert() are used consistently and adjust the
surrounding code/snippet text accordingly.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-renderer-showcase/e2e/browser/helpers.ts`:
- Around line 22-27: The getPreviewFrame helper currently calls
iframe.contentFrame() which can return null; update getPreviewFrame to
explicitly handle that by either (A) changing its signature to return
Promise<Frame | null> and return the possibly-null frame from
iframe.contentFrame(), or (B) if a frame must always be present, throw a clear
error when iframe.contentFrame() is null so callers fail fast; refer to the
getPreviewFrame function and the iframe/contentFrame call to implement the
chosen approach and add the appropriate return type annotation.
In `@apps/e2e/demo-e2e-renderer-showcase/src/components/PreviewPanel.tsx`:
- Around line 29-39: The iframe rendered in PreviewPanel (the <iframe ...
title="Renderer Preview" /> with key `${groupId}-${exampleIndex}` and
data-testid="preview-iframe") should include a sandbox attribute for
defense-in-depth; update the JSX to add sandbox="allow-same-origin" (or a
minimal set of sandbox flags you need) on that iframe element so it retains
same-origin behavior while restricting other capabilities.
In `@apps/e2e/demo-e2e-uipack/e2e/browser/bridge.pw.test.ts`:
- Around line 118-138: Rename the test or assert the actual event: either change
the test title in the test(...) call from "should dispatch bridge:ready event
after initialization" to "should be initialized after navigation" to reflect the
current assertion, or update the test to listen for the DOM event before
navigation by using page.evaluate to attach a
window.addEventListener('bridge:ready', ...) (or use page.waitForEvent if
emitting via page.emit) and resolve a Promise when the handler runs, then call
page.goto(url) and await that promise instead of only checking
FrontMcpBridge.initialized with page.waitForFunction; reference symbols:
test(...), serveShell, page.goto, page.evaluate, FrontMcpBridge, and the
'bridge:ready' event.
In `@apps/e2e/demo-e2e-uipack/e2e/browser/helpers.ts`:
- Around line 59-69: stopServer may hang if server.close() callback never fires
when the server never reached listening; modify stopServer to guard against this
by checking the server.listening property and, if not listening, call
server.close() and immediately resolve (or use server.unref/cleanup), otherwise
attach the close callback as now but also start a short timeout (e.g., 1-2s)
that will forcefully set server = null, serverUrl = null and resolve if the
close callback hasn’t run; refer to the stopServer function and the local
variable s to implement the conditional/listening check and the fallback
timeout.
- Around line 11-12: The variable serverUrl is never used outside serveHtml so
remove it to simplify state: delete the top-level declaration let serverUrl and
its assignments inside serveHtml and stopServer, keep the server: http.Server |
null variable and ensure serveHtml returns or otherwise exposes the URL it needs
without relying on the removed serverUrl; update any call sites that expected
serverUrl by using the returned value from serveHtml or constructing the URL
from server.address() within those call sites instead.
In `@apps/e2e/demo-e2e-uipack/e2e/resolver.e2e.test.ts`:
- Around line 51-63: The test "should handle multiple packages" currently
asserts json.rewrittenCount >= 2 but imports three packages (react,
react-dom/client, zod); update the assertion on json.rewrittenCount in the test
to expect at least 3 (e.g., change the matcher from >= 2 to >= 3) so the test
fails if fewer than all expected imports are rewritten, keeping the rest of the
test (result retrieval via mcp.tools.call and json parsing) unchanged.
In `@apps/e2e/demo-e2e-uipack/playwright.config.ts`:
- Around line 3-11: The root-level use object in defineConfig currently spreads
devices['Desktop Chrome'] which is redundant because the same device is set at
the project level and project-level settings override root settings; remove the
devices['Desktop Chrome'] spread from the root use while keeping headless: true
(i.e., keep use: { headless: true }) and leave the projects entry (projects: [{
name: 'chromium', use: { ...devices['Desktop Chrome'] } }]) unchanged so the
project-level device config remains authoritative.
In `@apps/e2e/demo-e2e-uipack/webpack.config.js`:
- Around line 11-12: The webpack config currently hardcodes mode: 'development'
and devtool: 'eval-cheap-module-source-map'; make these environment-aware by
deriving mode from process.env.NODE_ENV (e.g., set mode to 'production' when
NODE_ENV==='production' and 'development' otherwise) and conditionally set
devtool (e.g., disable or use 'source-map' in production, keep
'eval-cheap-module-source-map' for development/testing) so the Webpack settings
in webpack.config.js adapt to build environment; alternatively, if this file is
strictly for e2e/dev, document that it is intentionally fixed to development.
In `@docs/frontmcp/nx-plugin/generators/overview.mdx`:
- Around line 48-52: The documentation shows inconsistent casing for generator
output paths: `ui-component` and `ui-page` use `ui/components/src/<Name>/` and
`ui/pages/src/<Name>/` while `ui-shell` lists `ui/shells/src/<name>/`; update
the `ui-shell` entry to match the PascalCase convention (`<Name>`) if shells
follow the same directory naming, or explicitly add a clarifying note next to
the `ui-shell` row indicating that shells use kebab/lowercase directories if
that is intentional; reference the `ui-component`, `ui-page`, and `ui-shell`
generator rows and their output path cells when making the change.
In `@libs/cli/README.md`:
- Around line 19-132: The README's large manual CLI matrix should be generated
from the actual command registration metadata instead of hand-editing; add a
doc-generation step that reads the command registry (e.g., the register.ts file
and its exported commands/registerCommands or commands array/CommandDefinition
symbols), formats flags and descriptions into the markdown tables, and
writes/overwrites libs/cli/README.md (or a README fragment) as part of the build
or a new npm script (e.g., "frontmcp docs" or "build:docs"); ensure the
generator handles all subcommands, flags, defaults, and linkable usage examples
so the README always reflects register.ts source-of-truth.
In `@libs/cli/src/commands/package/install.ts`:
- Around line 138-142: The empty catch swallowing errors from fs.rmSync(tmpDir,
{ recursive: true, force: true }) should log the caught error at debug level;
modify the catch to capture the error (e.g., catch (err)) and call the existing
debug logger (or console.debug if no logger is available) with a short message
referencing tmpDir and the error (e.g., "cleanup tmpDir failed:" + err) so
cleanup failures are visible in development while preserving current
non-throwing behavior.
In
`@libs/nx-plugin/src/generators/ui-component/files/__className__/__className__.spec.tsx__tmpl__`:
- Around line 3-10: The tests only check that <%= className %> is defined and is
a function; replace or augment those with a render smoke test that imports
render from '@testing-library/react', renders the component as JSX (e.g.
render(<%= className % />)) and asserts a basic post-render condition (e.g.
container is in the document or no error thrown) so the generated spec for <%=
className %> actually exercises JSX/render wiring.
apps/e2e/demo-e2e-renderer-showcase/src/components/Navigation.tsx
Outdated
Show resolved
Hide resolved
apps/e2e/demo-e2e-standalone/src/apps/isolated/tools/isolated-hello.tool.ts
Show resolved
Hide resolved
… output directory types
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores