feat: add API Gateway target TUI wizard and address review feedback#511
feat: add API Gateway target TUI wizard and address review feedback#511aidandaly24 wants to merge 4 commits intomainfrom
Conversation
…t tool-filter input
…uth validation, clean dispatch
…n, setTargetType comment
Coverage Report
|
tejaskash
left a comment
There was a problem hiding this comment.
Code Review
1. Non-null assertion (!) proliferation — 8 new instances (Medium)
Making fields optional on AddGatewayTargetConfig and then using ! at every call site trades one problem (dummy values) for another (suppressed type safety). There are 8 new ! assertions across GatewayTargetPrimitive.ts (config.toolDefinition! ×2, config.sourcePath! ×4, config.language! ×1) and useCreateMcp.ts (config.language! ×1).
If a code path ever reaches these without the field set (e.g., a new target type that doesn't set sourcePath), it'll be a silent undefined runtime error. A discriminated union would be the proper fix:
type AddGatewayTargetConfig =
| { targetType: 'apiGateway'; name: string; restApiId: string; stage: string; ... }
| { targetType: 'mcpServer'; name: string; sourcePath: string; language: TargetLanguage; host: ComputeHost; toolDefinition: ToolDefinition; ... }Worth tracking before adding more target types.
2. Tool filter HTTP methods are not validated in the TUI (Medium)
In AddGatewayTargetScreen.tsx (lines ~233-238), user input is split, trimmed, and uppercased, but never validated against the allowed set (GET, POST, PUT, DELETE, PATCH, HEAD, OPTIONS). If a user types GET,TRACE, the value TRACE will be written to the config and only fail at deploy time when the Zod schema (ApiGatewayHttpMethodSchema) rejects it. Consider validating in the wizard or using a multi-select instead of free-text input.
3. outboundAuth schema check allows type: 'NONE' (Low)
In mcp.ts line 360, the check is data.outboundAuth && data.outboundAuth.type !== 'NONE'. This means { outboundAuth: { type: 'NONE' } } is technically valid on an apiGateway target. If outbound auth is "not applicable" for apiGateway, all values should be rejected, not just non-NONE. Consider simplifying to if (data.outboundAuth).
4. Two-phase tool-filters step uses component-local state (Low)
In AddGatewayTargetScreen.tsx, the tool-filters step is split into two prompts (path pattern → HTTP methods) managed by a local filterPath state variable, creating a sub-step outside the wizard's step tracking. The wizard's step value stays at 'tool-filters' throughout both sub-prompts. This works but is fragile — if more sub-steps are needed later, this pattern doesn't scale. Consider promoting these to separate wizard steps (tool-filter-path, tool-filter-methods).
5. Triple-nested ternary in ResourceGraph (Nit)
In ResourceGraph.tsx (lines ~247-252), the displayText computation is now a three-level ternary, duplicated at line ~292. Consider extracting a helper:
function getTargetDisplayText(target: AgentCoreGatewayTarget): string {
if (target.targetType === 'mcpServer' && target.endpoint) return target.endpoint;
if (target.targetType === 'apiGateway' && target.apiGateway)
return `${target.apiGateway.restApiId}/${target.apiGateway.stage}`;
return target.name;
}Introduce McpServerTargetConfig and ApiGatewayTargetConfig as a discriminated union, replacing the single bag-of-optionals interface. The wizard uses GatewayTargetWizardState internally and narrows to the union at the confirm step boundary. - Remove all 8 non-null assertions (!) from GatewayTargetPrimitive and useCreateMcp by narrowing method signatures - Add HTTP method validation to tool-filters TUI input - Add explanatory comment on two-phase tool-filters local state - Remove dead code-scaffolding branch from handleCreateComplete
|
@tejaskash I will fix #3 in the next PR for auth |
Description
Adds the TUI wizard flow for API Gateway targets, along with review feedback fixes and polish from CLI #509.
TUI Wizard:
setStep()to dynamicgoToNextStep()for all setters exceptsetTargetType(closure issue documented)createApiGatewayTarget()for apiGateway targetsResourceGraph:
restApiId/stageand[apiGateway]badge for API Gateway targetsReview Feedback Fixes (from CLI #2 review):
sourcePath,host,toolDefinition,description,languageoptional onAddGatewayTargetConfig— apiGateway targets don't use thesetargetTypefrom dispatch config (method hardcodes it)outboundAuthrejection to schema superRefine for apiGateway targetsshowDevOption={false}for gateway targetsRelated Issue
Closes #
Documentation PR
Type of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsAdditionally:
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.