feat(@angular/cli): standardize MCP tools around workspace/project options #32294
Conversation
angular-robot
bot
added
detected: feature
amishne
added
the
action: review
dgp1130
left a comment
dgp1130
left a comment
There was a problem hiding this comment.
Just some minor suggestions, I'm not sure how much of the resolveWorkspaceAndProject implementation was reused so I might be repeating previous feedback. Feel free to ignore / defer anything we've already discussed or which can be followed up on later.
| } | ||
|
|
||
| const deadline = Date.now() + input.timeout; | ||
| async function performWait(devserver: Devserver, timeout: number) { |
There was a problem hiding this comment.
Suggestion: Use an explicit return type. Otherwise, TS will implicitly union all the return statement types and wouldn't prevent from accidentally returning the wrong type.
https://techhub.social/@develwithoutacause/111842243141199473
| fail('Should have thrown'); | ||
| } catch (e) { | ||
| expect((e as Error).message).toContain('Dev server not found. Currently running servers:'); | ||
| expect((e as Error).message).toContain("- Project 'app-one' in workspace path '/test'"); |
There was a problem hiding this comment.
Consider: Should this message contain the port number too?
| ); | ||
| } | ||
|
|
||
| export function createDevServerNotFoundError( |
There was a problem hiding this comment.
Consider: Might be better to leave this in devserver-stop.ts since it specifically relates to devserver functionality.
| 'Please provide a project name or set a default project in angular.json. ' + | ||
| "You can use 'list_projects' to find available projects.", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Consider: Do these helpers need to be exported? Will they ever realistically be called outside this file if resolveWorkspaceAndProject is doing most of the work?
Going a step further, if these errors are only created in one place, would it be more clear to just inline them at their individual call sites?
| workspacePathInput, | ||
| projectNameInput, |
There was a problem hiding this comment.
Suggestion: I don't think we need the Input suffix here, I'd just call these workspacePath and projectName.
If the goal is to have a local variable with the same name, you can do that by aliasing in the destructure without altering the public API of the function.
function func({foo: bar}: {foo: string}): string {
const foo = bar;
return foo;
}
func({foo: 'test'});| * current directory as the workspace. | ||
| * If `projectNameInput` is absent, uses the default project in the workspace. | ||
| */ | ||
| export async function resolveWorkspaceAndProject({ |
There was a problem hiding this comment.
Suggestion: I'm a bit wary of this utils file getting a bit long and have a nebulous purpose. Could we split it up into separate (or at least rename to a well-defined) domain-specific file(s)? Something like projects.ts or workspaces.ts? Could also combined shared-options.ts into the same file.
| let workspace: AngularWorkspace; | ||
|
|
||
| if (workspacePathInput) { | ||
| if (!host.existsSync(workspacePathInput)) { |
There was a problem hiding this comment.
Consider: Can/should we use async APIs here for improved parallelism? Not sure how much of a concern multiple MCP requests would be.
| * current directory as the workspace. | ||
| * If `projectNameInput` is absent, uses the default project in the workspace. | ||
| */ | ||
| export async function resolveWorkspaceAndProject({ |
There was a problem hiding this comment.
Consider: Can we split this implementation into separate resolveWorkspace and resolveProject functions? I think this would make the internals easier to follow as we can leverage early returns rather than stateful local variables.
| let workspace: AngularWorkspace; | ||
|
|
||
| if (workspacePathInput) { | ||
| if (!host.existsSync(workspacePathInput)) { |
There was a problem hiding this comment.
Consider: Do we really care about this as a distinct error case from ${workspacePathInput}/angular.json as checked below? I'm not sure this is worth the maintenance effort / performance cost of an additional stat call.
| workspacePathInput, | ||
| projectNameInput, | ||
| mcpWorkspace, | ||
| workspaceLoader = AngularWorkspace.load, |
There was a problem hiding this comment.
Consider: Is this option just for testing purposes? Can we drop it from the public API or at least make it more clearly not for general usage?
I'm thinking you can either directly depend on AngularWorkspace.load and then spy on it in the test, or define this as a test only parameter like:
function resolveWorkspaceAndProject(
{/* regular options */}: {/* ... */},
testOnlyOptions: {
workspaceLoader = AngularWorkspace.load,
} = {},
): Promise<{ /* ... */}>;2 participants
workspaceandprojectoptions that make them better suited to monorepo scenarios.Tested manually with Gemini-CLI, both in an Angular directory and in a non-Angular parent directory.