fix: enable MCP server management in web mode (CRITICAL) #320
Conversation
Phase 1 complete - Document fixes for 3 MCP server bugs: - Bug AutoMaker-Org#1: JSON format inconsistency (array vs object, IDs lost) - Bug AutoMaker-Org#2: HTTP error handling (cryptic error messages) - Bug AutoMaker-Org#3: Race condition in auto-test (server not found) All bugs validated with multiple MCP server types (stdio + HTTP). Patches available for re-application after upstream updates. Related to upstream PR AutoMaker-Org#286 (MCP server support).
Added links to upstream issues AutoMaker-Org#315, AutoMaker-Org#316, AutoMaker-Org#317 in CUSTOM_CHANGES.md. Updated contribution status to reflect Phase 2 completion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Added links to PRs AutoMaker-Org#318 and AutoMaker-Org#319 in CUSTOM_CHANGES.md. Updated contribution status to reflect Phase 3 completion. Phase 3 completed: - PR AutoMaker-Org#318: HTTP error handling (fixes AutoMaker-Org#315) - PR AutoMaker-Org#319: Race condition + JSON format (fixes AutoMaker-Org#316, AutoMaker-Org#317) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fork-specific feature: Enhanced spec generation with more flexibility ## Changes - Added 200 and 500 feature options with appropriate warnings - Added "Custom" option with number input (1-10,000 range) - Modified FeatureCount type from union literals to number - Implemented custom input UI in both Create and Regenerate dialogs ## User Experience **New Options**: - 200 features (warning: ~10 minutes) - 500 features (warning: ~15 minutes) - Custom (dynamic input with smart warnings) **Custom Mode**: - Click "Custom" button → number input appears - Default value: 150 - Range: 1 to 10,000 - Warning for values > 100 ## Implementation **Files Modified**: - types.ts: Changed FeatureCount to number - constants.ts: Added new options with isCustom flag - create-spec-dialog.tsx: Added custom input UI and handlers - regenerate-spec-dialog.tsx: Added custom input UI and handlers ## Re-application Patch available at: patches/03-custom-feature-count-2025-12-29.patch ```bash git apply patches/03-custom-feature-count-2025-12-29.patch ``` ## Testing - ✅ npm run build:packages passes - ✅ npm run build (UI) passes - ✅ All 6 options render correctly - ✅ Custom input works in both dialogs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Created comprehensive documentation in .plans/ directory covering: - Objectives and requirements - Architecture and data flow - Detailed implementation guide - Use cases and validation tests - Maintenance strategy - Future roadmap 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixes critical issues preventing MCP servers from working properly: Bug AutoMaker-Org#1: Add syncSuccess validation before showing success toasts - Prevents misleading success messages when save actually failed - Shows proper error toast if sync to disk fails Bug AutoMaker-Org#2: Fix MCP export to use array format with preserved IDs - Changes from object with server names as keys to array format - Preserves server IDs and names in export JSON Bug AutoMaker-Org#3: Improve HTTP error handling with better error messages - Extracts error messages from response JSON when available - Falls back to status text for better error reporting Bug AutoMaker-Org#4: Enable MCP functionality in web mode (CRITICAL) - Removes isElectron() check from syncSettingsToServer() - Allows web mode to save MCP servers to disk via backend - Without this fix, web mode cannot save any MCP configuration All changes maintain backward compatibility with Electron mode. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces fork-specific extensions to feature count options (200, 500, and a custom 1–10,000 mode) with corresponding UI support, plus critical bug fixes addressing MCP server synchronization, HTTP error handling, and JSON format consistency. Supporting planning documents and patch files detail implementation strategy, validation, and rollout guidance. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
Summary of ChangesHello @juniorcammel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers crucial bug fixes and a new feature to enhance the Automaker application. It addresses a critical issue preventing web mode users from managing MCP servers, improves the clarity of HTTP error messages, resolves a race condition during server saving, and refines the MCP server JSON export format. Additionally, it introduces a highly requested feature allowing users to specify a custom number of features during app specification generation, offering greater flexibility and control. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Complete documentation of today's work: - Custom Feature Count implementation (fork-specific) - 4 MCP bug fixes (PR AutoMaker-Org#320 submitted) - Maintenance strategy for upstream updates - All patches and documentation referenced This serves as the definitive guide for: 1. Understanding what was changed and why 2. Re-applying patches after upstream updates 3. Tracking PR AutoMaker-Org#320 status 4. Managing fork-specific vs upstream features 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and critical update that successfully enables MCP server management in web mode by removing a blocking check. It also commendably addresses several related bugs, including a race condition in the server save/test flow, poor HTTP error handling, and an incorrect JSON export format that caused data loss. The addition of detailed planning and documentation files is also a great practice.
My main feedback is that this PR combines two unrelated major changes: the critical MCP bug fixes and a new 'Custom Feature Count' enhancement. It's generally better to submit unrelated changes in separate pull requests to simplify the review process and maintain a cleaner commit history. My review includes a few suggestions for refactoring to reduce code duplication and improve maintainability in the new logic.
| const handleSaveGlobalJsonEdit = async () => { | ||
| try { | ||
| const parsed = JSON.parse(globalJsonValue); | ||
|
|
||
| // Support both formats | ||
| // Support both formats: array and object | ||
| const servers = parsed.mcpServers || parsed; | ||
|
|
||
| if (typeof servers !== 'object' || Array.isArray(servers)) { | ||
| toast.error('Invalid format: expected object with server configurations'); | ||
| if (Array.isArray(servers)) { | ||
| // Array format (new format with IDs) | ||
| await handleSaveGlobalJsonArray(servers); | ||
| } else if (typeof servers === 'object' && servers !== null) { | ||
| // Object format (legacy Claude Desktop format) | ||
| await handleSaveGlobalJsonObject(servers); | ||
| } else { | ||
| toast.error('Invalid format: expected array or object with server configurations'); | ||
| return; | ||
| } | ||
|
|
||
| // Validate all servers first | ||
| for (const [name, config] of Object.entries(servers)) { | ||
| if (typeof config !== 'object' || config === null) { | ||
| toast.error(`Invalid config for "${name}"`); | ||
| return; | ||
| } | ||
| const syncSuccess = await syncSettingsToServer(); | ||
|
|
||
| const serverConfig = config as Record<string, unknown>; | ||
| const serverType = (serverConfig.type as string) || 'stdio'; | ||
| if (!syncSuccess) { | ||
| toast.error('Failed to save MCP servers to disk'); | ||
| return; | ||
| } | ||
|
|
||
| if (serverType === 'stdio') { | ||
| if (!serverConfig.command || typeof serverConfig.command !== 'string') { | ||
| toast.error(`Command is required for "${name}" (stdio)`); | ||
| return; | ||
| } | ||
| } else if (serverType === 'sse' || serverType === 'http') { | ||
| if (!serverConfig.url || typeof serverConfig.url !== 'string') { | ||
| toast.error(`URL is required for "${name}" (${serverType})`); | ||
| return; | ||
| } | ||
| toast.success('MCP servers configuration updated'); | ||
| setIsGlobalJsonEditOpen(false); | ||
| setGlobalJsonValue(''); | ||
| } catch (error) { | ||
| toast.error('Invalid JSON: ' + (error instanceof Error ? error.message : 'Parse error')); | ||
| } | ||
| }; |
There was a problem hiding this comment.
In the new handleSaveGlobalJsonEdit function, the helper methods (handleSaveGlobalJsonArray and handleSaveGlobalJsonObject) display a toast for validation errors and then throw an error. The catch block in this function then catches that error and displays a second, more generic toast. This results in two error toasts being shown to the user for a single validation failure, which can be confusing.
To improve the user experience, the helper functions should return a boolean indicating success or failure instead of throwing an error for validation issues. This allows the main function to gracefully stop execution without triggering the catch block, ensuring only one clear error message is displayed.
| const [isCustomMode, setIsCustomMode] = useState(false); | ||
| const [customValue, setCustomValue] = useState<string>(''); | ||
|
|
||
| // Determine if we're in custom mode based on featureCount | ||
| useEffect(() => { | ||
| const isCustom = !FEATURE_COUNT_OPTIONS.slice(0, -1).some((o) => o.value === featureCount); | ||
| setIsCustomMode(isCustom); | ||
| if (isCustom && featureCount > 0) { | ||
| setCustomValue(featureCount.toString()); | ||
| } | ||
| }, [featureCount]); | ||
|
|
||
| const selectedOption = FEATURE_COUNT_OPTIONS.find((o) => o.value === featureCount); | ||
| const isDisabled = isRegenerating || isGeneratingFeatures; | ||
|
|
||
| const handleOptionClick = (option: (typeof FEATURE_COUNT_OPTIONS)[number]) => { | ||
| if (option.isCustom) { | ||
| setIsCustomMode(true); | ||
| const defaultCustom = 150; | ||
| setCustomValue(defaultCustom.toString()); | ||
| onFeatureCountChange(defaultCustom); | ||
| } else { | ||
| setIsCustomMode(false); | ||
| setCustomValue(''); | ||
| onFeatureCountChange(option.value as FeatureCount); | ||
| } | ||
| }; | ||
|
|
||
| const handleCustomValueChange = (value: string) => { | ||
| setCustomValue(value); | ||
| const numValue = parseInt(value, 10); | ||
| if (!isNaN(numValue) && numValue > 0) { | ||
| onFeatureCountChange(numValue); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The state management and rendering logic for the custom feature count is nearly identical to the implementation in create-spec-dialog.tsx. This code duplication can make future updates more time-consuming and error-prone.
To improve maintainability, consider refactoring this shared functionality into a reusable custom hook (e.g., useFeatureCountSelector) and/or a dedicated component (e.g., <FeatureCountSelector />). This would centralize the logic in one place.
| if (!response.ok) { | ||
| let errorMessage = `HTTP ${response.status}: ${response.statusText}`; | ||
| try { | ||
| const errorData = await response.json(); | ||
| if (errorData.error) { | ||
| errorMessage = errorData.error; | ||
| } | ||
| } catch { | ||
| // If parsing JSON fails, use status text | ||
| } | ||
| throw new Error(errorMessage); | ||
| } |
There was a problem hiding this comment.
While adding this error handling is a great improvement, the logic is duplicated across all four HTTP methods (post, get, put, httpDelete). This makes future maintenance more difficult as any changes would need to be applied in four places.
Consider extracting this logic into a private helper function to adhere to the DRY (Don't Repeat Yourself) principle. For example, a method like _handleErrorResponse(response) could be created and called from each fetch operation when !response.ok.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
patches/04-mcp-bugs-complete-fix-2025-12-29.patch (1)
167-279: Helper functions show toast then throw—verify this doesn't cause duplicate errors.The
handleSaveGlobalJsonArrayandhandleSaveGlobalJsonObjecthelpers calltoast.error()and thenthrow new Error(). The callerhandleSaveGlobalJsonEditcatches these errors but shows another toast ('Invalid JSON: ' + error.message). This could result in two error toasts for validation failures.Consider either:
- Not throwing after
toast.error()in helpers (use a return pattern instead)- Or catching the specific validation errors in the caller without showing an additional toast
🔎 Example fix using a return pattern
// Helper: Process array format (with IDs) - const handleSaveGlobalJsonArray = async (serversArray: unknown[]) => { + const handleSaveGlobalJsonArray = async (serversArray: unknown[]): Promise<boolean> => { // Validate all servers first for (const config of serversArray) { if (typeof config !== 'object' || config === null) { toast.error('Invalid server config in array'); - throw new Error('Invalid server config'); + return false; } // ... rest of validation with similar changes } + return true; };Then in the caller:
if (Array.isArray(servers)) { - await handleSaveGlobalJsonArray(servers); + const success = await handleSaveGlobalJsonArray(servers); + if (!success) return; }apps/ui/src/lib/http-api-client.ts (1)
171-183: HTTP error handling is correct; consider extracting to reduce duplication.The error handling pattern is correctly implemented—it checks
response.ok, attempts to extract a server-provided error message from JSON, and falls back to HTTP status text. This same pattern is repeated inget(),put(), andhttpDelete().🔎 Optional: Extract error handling into a helper
+ private async handleErrorResponse(response: Response): Promise<never> { + let errorMessage = `HTTP ${response.status}: ${response.statusText}`; + try { + const errorData = await response.json(); + if (errorData.error) { + errorMessage = errorData.error; + } + } catch { + // If parsing JSON fails, use status text + } + throw new Error(errorMessage); + } private async post<T>(endpoint: string, body?: unknown): Promise<T> { const response = await fetch(`${this.serverUrl}${endpoint}`, { method: 'POST', headers: this.getHeaders(), body: body ? JSON.stringify(body) : undefined, }); - if (!response.ok) { - let errorMessage = `HTTP ${response.status}: ${response.statusText}`; - try { - const errorData = await response.json(); - if (errorData.error) { - errorMessage = errorData.error; - } - } catch { - // If parsing JSON fails, use status text - } - throw new Error(errorMessage); - } + if (!response.ok) { + await this.handleErrorResponse(response); + } return response.json(); }CUSTOM_CHANGES.md (1)
85-92: Add language specifier to fenced code blocks.Static analysis correctly identifies that fenced code blocks should have a language specified for proper syntax highlighting.
🔎 Suggested fix
-``` +```bash cd N:\code\automaker-app # Apply the patch git apply patches/03-custom-feature-count-2025-12-29.patchapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx (2)
64-70: Custom value validation doesn't enforce maximum limit.The
handleCustomValueChangefunction validates that the value is a positive number but doesn't enforce the max limit of 10,000. While the HTML input hasmax="10000", users can still type or paste values exceeding this limit.🔎 Add max validation
const handleCustomValueChange = (value: string) => { setCustomValue(value); const numValue = parseInt(value, 10); - if (!isNaN(numValue) && numValue > 0) { + if (!isNaN(numValue) && numValue > 0 && numValue <= 10000) { onFeatureCountChange(numValue); } };
157-178: Button variant and className have redundant conditional logic.The
variantprop (lines 157-165) andclassNameprop (lines 171-177) both conditionally apply styles based on the sameoption.isCustomandisCustomMode/featureCountconditions. Sincevariant="default"already applies primary styling, the duplicate className logic forbg-primaryis redundant.🔎 Simplify by relying on variant
className={cn( - 'flex-1 min-w-[70px] transition-all', - option.isCustom - ? isCustomMode - ? 'bg-primary hover:bg-primary/90 text-primary-foreground' - : 'bg-muted/30 hover:bg-muted/50 border-border' - : featureCount === option.value - ? 'bg-primary hover:bg-primary/90 text-primary-foreground' - : 'bg-muted/30 hover:bg-muted/50 border-border' + 'flex-1 min-w-[70px] transition-all' )}The
variant="default"vsvariant="outline"should handle the primary styling automatically via the Button component's variants.apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx (2)
38-44: Fragile assumption that Custom option is always last.The
slice(0, -1)assumes the Custom option is always the last item inFEATURE_COUNT_OPTIONS. If the array order changes, this logic will break silently.🔎 Suggested improvement
- useEffect(() => { - const isCustom = !FEATURE_COUNT_OPTIONS.slice(0, -1).some((o) => o.value === featureCount); - setIsCustomMode(isCustom); + useEffect(() => { + const isCustom = !FEATURE_COUNT_OPTIONS.some((o) => !o.isCustom && o.value === featureCount); + setIsCustomMode(isCustom);
62-68: Missing upper bound validation in custom value handler.The HTML
max="10000"attribute doesn't prevent programmatic or paste input of larger values. Consider adding JS validation.🔎 Suggested improvement
const handleCustomValueChange = (value: string) => { setCustomValue(value); const numValue = parseInt(value, 10); - if (!isNaN(numValue) && numValue > 0) { + if (!isNaN(numValue) && numValue > 0 && numValue <= 10000) { onFeatureCountChange(numValue); } };apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (1)
345-356: Consider rolling back state on sync failure.When
syncSettingsToServer()fails, the local state has already been mutated (e.g.,updateMCPServerwas called). This creates a state mismatch where the UI shows the change but it wasn't persisted. Consider either:
- Rolling back the state change on failure, or
- Adding a visual indicator that the change is "pending save"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.plans/custom-feature-count-implementation.md.plans/mcp-bugs-fix-plan.mdCUSTOM_CHANGES.mdapps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.tsapps/ui/src/components/views/spec-view/constants.tsapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/src/components/views/spec-view/types.tsapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/lib/http-api-client.tspatches/01-fix-http-error-handling.patchpatches/02-fix-race-condition-and-json-format.patchpatches/03-custom-feature-count-2025-12-29.patchpatches/04-mcp-bugs-complete-fix-2025-12-29.patchpatches/mcp-fixes-combined-2025-12-29.patch
💤 Files with no reviewable changes (1)
- apps/ui/src/hooks/use-settings-migration.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/spec-view/constants.tsapps/ui/src/components/views/spec-view/types.tsapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.tsapps/ui/src/lib/http-api-client.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/spec-view/constants.tsapps/ui/src/components/views/spec-view/types.tsapps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsxapps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.tsapps/ui/src/lib/http-api-client.ts
🧬 Code graph analysis (3)
apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx (4)
apps/ui/src/components/views/spec-view/constants.ts (1)
FEATURE_COUNT_OPTIONS(10-22)apps/ui/src/components/views/spec-view/types.ts (1)
FeatureCount(2-2)apps/ui/src/components/ui/button.tsx (1)
Button(108-108)apps/ui/src/lib/utils.ts (1)
cn(5-7)
apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx (4)
apps/ui/src/components/views/spec-view/constants.ts (1)
FEATURE_COUNT_OPTIONS(10-22)apps/ui/src/components/views/spec-view/types.ts (1)
FeatureCount(2-2)apps/ui/src/components/ui/button.tsx (1)
Button(108-108)apps/ui/src/lib/utils.ts (1)
cn(5-7)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (3)
apps/ui/src/hooks/use-settings-migration.ts (1)
syncSettingsToServer(196-245)apps/ui/src/components/views/settings-view/mcp-servers/types.ts (1)
ServerType(3-3)libs/types/src/settings.ts (1)
MCPServerConfig(188-213)
🪛 LanguageTool
.plans/custom-feature-count-implementation.md
[misspelling] ~23-~23: ‘minutes’ é um verbo referente a fazer uma minuta. A unidade de tempo escreve-se “minutos”.
Context: ...pção "200" com aviso "May take up to 10 minutes" - Adicionar opção "500" com aviso "May...
(EASILY_CONFUSED_RARE_WORDS)
[misspelling] ~24-~24: ‘minutes’ é um verbo referente a fazer uma minuta. A unidade de tempo escreve-se “minutos”.
Context: ...pção "500" com aviso "May take up to 15 minutes" - Manter opções existentes (20, 50, 10...
(EASILY_CONFUSED_RARE_WORDS)
[misspelling] ~281-~281: ‘minutes’ é um verbo referente a fazer uma minuta. A unidade de tempo escreve-se “minutos”.
Context: ...") 6. Aviso aparece: "May take up to 10 minutes" 7. Clica "Generate Spec" 8. Backend re...
(EASILY_CONFUSED_RARE_WORDS)
[misspelling] ~320-~320: ‘minutes’ é um verbo referente a fazer uma minuta. A unidade de tempo escreve-se “minutos”.
Context: ...do 8. Aviso muda para "May take up to 5 minutes" Resultado: Modo custom desativado...
(EASILY_CONFUSED_RARE_WORDS)
[locale-violation] ~332-~332: “packages” é um estrangeirismo. É preferível dizer “embalagens” ou “pacotes”.
Context: ...estes Realizados #### Build Tests - ✅ npm run build:packages - Todos os pacotes compilam - ✅ `npm r...
(PT_BARBARISMS_REPLACE_PACKAGES)
[uncategorized] ~418-~418: Deve-se utilizar a vírgula antes de ‘etc.’.
Context: ...ustom permite valores como 150, 250, 300 etc. 4. Feedback Claro: Avisos dinâmico...
(ETC_USAGE)
CUSTOM_CHANGES.md
[grammar] ~438-~438: Ensure spelling is correct
Context: ...es - Performance impact is minimal (~50-200ms for sync operations) - Bug #2 (HTTP err...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
.plans/mcp-bugs-fix-plan.md
[locale-violation] ~1-~1: “Servers” é um estrangeirismo. É preferível dizer “servidores”.
Context: ... Plano de Implementação - Correções MCP Servers ## Resumo Executivo Implementar corre...
(PT_BARBARISMS_REPLACE_SERVERS)
[misspelling] ~26-~26: Esta é uma palavra só.
Context: ...rs.ts:133-147, 300-322- **Problema**: Auto-test executa antes desyncSettingsToServer(...
(AUTO)
[misspelling] ~129-~129: Esta é uma palavra só.
Context: ...cesso aparece APÓS sync completar - [ ] Auto-test não falha com "Server not found" - [ ] ...
(AUTO)
[misspelling] ~134-~134: Possível erro ortográfico.
Context: ... falhar --- ### 1.3 Corrigir Bug #1 - JSON Format (TERCEIRO) Arquivo: `apps/ui/src/c...
(PT_MULTITOKEN_SPELLING_TWO)
[misspelling] ~275-~275: Possível erro ortográfico.
Context: ...race condition) - [ ] Bug #1 corrigido (JSON format) - [ ] CUSTOM_CHANGES.md criado - [ ] P...
(PT_MULTITOKEN_SPELLING_TWO)
[locale-violation] ~294-~294: “Template” é um estrangeirismo. É preferível dizer “modelo”.
Context: ...dx, api-client, good-first-issue- Template disponível emMCP_MAINTENANCE_STRATEGY...
(PT_BARBARISMS_REPLACE_TEMPLATE)
[locale-violation] ~296-~296: “Server” é um estrangeirismo. É preferível dizer “servidor”.
Context: ...d **Issue #2**: Race Condition in MCP Server Auto-Test - Label:bug, ux, mcp-s...
(PT_BARBARISMS_REPLACE_SERVER)
[locale-violation] ~298-~298: “servers” é um estrangeirismo. É preferível dizer “servidores”.
Context: ...Server Auto-Test - Label: bug, ux, mcp-servers, electron Issue #3: MCP Servers...
(PT_BARBARISMS_REPLACE_SERVERS)
[locale-violation] ~300-~300: “Servers” é um estrangeirismo. É preferível dizer “servidores”.
Context: ...servers, electron **Issue #3**: MCP Servers JSON Format Inconsistency - Label:bu...
(PT_BARBARISMS_REPLACE_SERVERS)
[misspelling] ~413-~413: Possível erro ortográfico.
Context: ...ks/use-mcp-servers.ts` - Bugs: #1 (JSON Format), #3 (Race Condition) - Linhas: 133...
(PT_MULTITOKEN_SPELLING_TWO)
[misspelling] ~414-~414: Esta é uma palavra só.
Context: ...(Race Condition) - Linhas: 133-147 (auto-test), 300-335 (security confirm), 571-696 (...
(AUTO)
[misspelling] ~449-~449: Possível erro ortográfico.
Context: ... - Garante IDs estáveis 3. ✅ Bug #1 (JSON Format) - TERCEIRO - Requer Bugs #2 e #3 pa...
(PT_MULTITOKEN_SPELLING_TWO)
[locale-violation] ~510-~510: “Performance” é um estrangeirismo. É preferível dizer “desempenho”, “atuação”, “apresentação”, “espetáculo” ou “interpretação”.
Context: ... correções são backward-compatible 4. Performance: Impacto mínimo (~50-200ms em operaçõ...
(PT_BARBARISMS_REPLACE_PERFORMANCE)
[misspelling] ~522-~522: Possível erro ortográfico.
Context: ...ug #3 extensivamente 6. Aplicar Bug #1 (JSON Format) 7. Testar integração completa 8. Criar...
(PT_MULTITOKEN_SPELLING_TWO)
🪛 markdownlint-cli2 (0.18.1)
CUSTOM_CHANGES.md
64-64: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
90-90: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.plans/mcp-bugs-fix-plan.md
308-308: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
314-314: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
320-320: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
361-361: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
384-384: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (19)
patches/01-fix-http-error-handling.patch (1)
1-87: Patch file correctly documents HTTP error handling improvements.This patch adds consistent error handling across all HTTP methods. The actual implementation will be reviewed in
apps/ui/src/lib/http-api-client.ts. The patch structure is correct and applies cleanly..plans/custom-feature-count-implementation.md (1)
1-487: Comprehensive implementation plan for custom feature count.This planning document thoroughly covers the feature requirements, implementation details, data flow, use cases, and maintenance strategy. The static analysis hints about "minutes" spelling are false positives—"minutes" is the correct English spelling for time units in the warning messages.
patches/04-mcp-bugs-complete-fix-2025-12-29.patch (1)
365-377: Critical fix: Removing isElectron() guard enables web mode MCP management.This change correctly removes the early return that blocked web mode from syncing settings. Both Electron and web modes communicate with the backend via HTTP, so this guard was incorrectly preventing web mode functionality.
apps/ui/src/components/views/spec-view/types.ts (1)
2-2: Type widening from union tonumberis intentional for custom input support.This change sacrifices some compile-time safety (previously only
20 | 50 | 100were valid) in favor of runtime flexibility for custom values. The UI enforces bounds (1-10,000) via input constraints, which is an acceptable tradeoff for this fork-specific feature.apps/ui/src/components/views/spec-view/constants.ts (1)
14-22: Well-structured feature count options with clear sentinel value for Custom.The use of
-1as a sentinel value for the Custom option is a clean pattern that won't conflict with valid feature counts. TheisCustomflag provides explicit identification, and the proportional warning times (10 min for 200, 15 min for 500) are reasonable estimates.CUSTOM_CHANGES.md (1)
1-442: Excellent documentation of fork-specific changes.This changelog provides comprehensive documentation including problem descriptions, code snippets (before/after), validation notes, re-application instructions, and upstream contribution tracking. This will be invaluable for maintaining these changes after upstream syncs.
apps/ui/src/components/views/spec-view/dialogs/create-spec-dialog.tsx (1)
37-47: Custom mode state management looks correct.The
useEffectcorrectly infers custom mode by checking iffeatureCountmatches any predefined option (excluding the Custom option itself viaslice(0, -1)). The state is properly synchronized whenfeatureCountchanges externally.apps/ui/src/components/views/spec-view/dialogs/regenerate-spec-dialog.tsx (2)
152-184: LGTM - Feature count UI implementation is correct.The button group logic correctly handles both predefined options and custom mode with proper visual states. The flex-wrap and min-width ensure good responsive behavior.
185-218: Custom input and warnings render correctly.The conditional rendering for custom mode input and warnings is well-structured. The warning threshold of 100 is reasonable for user feedback.
patches/mcp-fixes-combined-2025-12-29.patch (2)
1-82: Patch file documents race condition fix correctly.The patch shows the await pattern for
syncSettingsToServer()with proper error handling before showing success toasts. This aligns with the source implementation.
365-450: HTTP error handling patch is comprehensive.The patch adds consistent
response.okchecks to all HTTP methods (post, get, put, delete) with structured error message extraction. This is the correct approach for robust HTTP error handling..plans/mcp-bugs-fix-plan.md (1)
1-525: Planning document is thorough and well-organized.The implementation plan clearly documents the bug fixes, execution order, validation steps, and rollback procedures. The phased approach (short/medium/long term) is appropriate for managing these critical fixes.
patches/02-fix-race-condition-and-json-format.patch (1)
1-363: Patch correctly documents JSON format and race condition fixes.The patch shows the transition from object-based to array-based JSON export with ID preservation, and the consistent
await syncSettingsToServer()pattern with error handling.apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (4)
276-290: Sync-before-toast pattern correctly implemented for edit flow.The edit path now properly awaits
syncSettingsToServer()before showing the success toast, fixing the race condition.
606-638: JSON export now uses array format with preserved IDs.The change from object-based to array-based export ensures server IDs are preserved during edit/save cycles. This is a good fix for the JSON format inconsistency bug.
641-672: Dual format support maintains backward compatibility.The
handleSaveGlobalJsonEditcorrectly detects array vs object format and routes to appropriate handlers, maintaining compatibility with legacy Claude Desktop format.
757-828: Object format handler correctly mirrors array handler logic.The legacy object format processing validates servers, updates/adds by name, and removes obsolete entries. The implementation is consistent with the array handler.
patches/03-custom-feature-count-2025-12-29.patch (2)
1-19: Constants extension for custom feature count is well-designed.Adding
isCustom?: booleanflag and the new options (200, 500, Custom) extends the feature count system cleanly. Using-1as the sentinel value for Custom is a reasonable choice.
299-307: Type change from union to number is appropriate and necessary.Changing
FeatureCountfrom20 | 50 | 100tonumberis correct. The original type definition was already incomplete—the constants include 200 and 500, which were not in the union type. All consumers properly handle the broadernumbertype: state initialization uses concrete values (50), custom input is validated withparseInt()and a positive-number check, and type assertions are applied correctly. The change enables arbitrary custom feature counts while maintaining validation at the input handler level.
| // Try to match by ID first, then by name | ||
| const existing = id ? existingById.get(id) : existingByName.get(name); | ||
| if (existing) { | ||
| updateMCPServer(existing.id, serverData); | ||
| processedIds.add(existing.id); | ||
| } else { | ||
| addMCPServer(serverData); | ||
| } | ||
| } | ||
|
|
||
| // Remove servers that are no longer in the JSON | ||
| for (const server of mcpServers) { | ||
| if (!processedIds.has(server.id)) { | ||
| removeMCPServer(server.id); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, get the file and check its size
wc -l "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts"Repository: AutoMaker-Org/automaker
Length of output: 150
🏁 Script executed:
#!/bin/bash
# View the specific function mentioned in the review (lines 675-755+)
sed -n '675,760p' "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" | cat -nRepository: AutoMaker-Org/automaker
Length of output: 3979
🏁 Script executed:
#!/bin/bash
# Find where addMCPServer is defined/called
rg "addMCPServer\s*=" "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" -A 5Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check how mcpServers state is declared and updated
rg "mcpServers" "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" -B 2 -A 2 | head -100Repository: AutoMaker-Org/automaker
Length of output: 2958
🏁 Script executed:
#!/bin/bash
# Find the hook or context providing these functions
rg "addMCPServer.*function|addMCPServer.*=.*\(" "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" -B 3 -A 10 | head -80Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for where these functions are returned from the hook
sed -n '800,891p' "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" | cat -nRepository: AutoMaker-Org/automaker
Length of output: 2906
🏁 Script executed:
#!/bin/bash
# Look for the functions being called within the component - are they state setters or external mutations?
rg "const.*MCPServer.*useContext|const.*MCPServer.*useState|const.*MCPServer.*use" "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" -A 2Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for where addMCPServer, updateMCPServer, removeMCPServer are actually defined
rg "const addMCPServer|const updateMCPServer|const removeMCPServer" "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" -B 2 -A 8Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Look at the beginning of the hook to find where these come from
head -100 "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" | cat -nRepository: AutoMaker-Org/automaker
Length of output: 4568
🏁 Script executed:
#!/bin/bash
# Find the app store implementation
find . -name "app-store.*" -type f | head -5Repository: AutoMaker-Org/automaker
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Search for addMCPServer in the app store
rg "addMCPServer" --type ts --type tsx -B 3 -A 10 | grep -A 10 "addMCPServer.*=" | head -60Repository: AutoMaker-Org/automaker
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Search for addMCPServer in the app store
rg "addMCPServer" "apps/ui/src/store/app-store.ts" -B 3 -A 10Repository: AutoMaker-Org/automaker
Length of output: 996
🏁 Script executed:
#!/bin/bash
# Get the app store file size and view relevant sections
wc -l "apps/ui/src/store/app-store.ts"Repository: AutoMaker-Org/automaker
Length of output: 102
🏁 Script executed:
#!/bin/bash
# View the removeMCPServer implementation to understand state updates
rg "removeMCPServer" "apps/ui/src/store/app-store.ts" -A 5 | head -20Repository: AutoMaker-Org/automaker
Length of output: 537
🏁 Script executed:
#!/bin/bash
# Verify whether addMCPServer is synchronous by checking if it returns anything and how state updates work in this store
sed -n '1,50p' "apps/ui/src/store/app-store.ts" | cat -nRepository: AutoMaker-Org/automaker
Length of output: 1284
🏁 Script executed:
#!/bin/bash
# Let me verify the exact issue by tracing through the logic
# Check if addMCPServer generates an ID and returns it
sed -n '675,800p' "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" | grep -A 50 "handleSaveGlobalJsonArray"Repository: AutoMaker-Org/automaker
Length of output: 2110
🏁 Script executed:
#!/bin/bash
# Check if there's any missing code that should track the new server ID
rg "addMCPServer" "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" -B 5 -A 5Repository: AutoMaker-Org/automaker
Length of output: 1679
🏁 Script executed:
#!/bin/bash
# Verify the Zustand store behavior - check if set() is synchronous
sed -n '1,100p' "apps/ui/src/store/app-store.ts" | grep -A 3 "create("Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Double-check the exact sequence in handleSaveGlobalJsonArray
# Extract lines 675-755 to see the full function logic
sed -n '675,755p' "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" | cat -nRepository: AutoMaker-Org/automaker
Length of output: 3700
🏁 Script executed:
#!/bin/bash
# Verify Zustand's set() synchronicity and how it affects state
grep -A 20 "create(" "apps/ui/src/store/app-store.ts" | head -30Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check if there's any await or callback used with addMCPServer in the function
sed -n '700,760p' "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" | grep -E "addMCPServer|await|then"Repository: AutoMaker-Org/automaker
Length of output: 148
🏁 Script executed:
#!/bin/bash
# Verify the full context - is there any awaiting or syncing after the loop?
sed -n '740,780p' "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" | cat -nRepository: AutoMaker-Org/automaker
Length of output: 1852
🏁 Script executed:
#!/bin/bash
# Check the full function signature and whether handleSaveGlobalJsonArray gets called
rg "handleSaveGlobalJsonArray" "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" -B 2 -A 2Repository: AutoMaker-Org/automaker
Length of output: 519
🏁 Script executed:
#!/bin/bash
# Verify by looking at the handleSaveGlobalJsonEdit function to see how handleSaveGlobalJsonArray is invoked
sed -n '600,750p' "apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts" | grep -B 30 "handleSaveGlobalJsonArray"Repository: AutoMaker-Org/automaker
Length of output: 1912
Critical: Newly added servers are not tracked in processedIds, creating a fragile deletion race condition.
When addMCPServer(serverData) is called at line 71, it generates an ID internally (via Zustand's store), but that ID is not added to processedIds. The cleanup loop (lines 76-80) iterates over mcpServers using the closure-captured state from the current render, which hasn't yet reflected the Zustand store update. This accidentally prevents deletion of new servers in the current cycle, but the logic is fragile: if state updates become synchronous or the code structure changes to await addMCPServer calls, newly added servers would appear in mcpServers but remain absent from processedIds, causing them to be immediately deleted.
The fix requires either: (1) having addMCPServer return the generated ID so it can be tracked, or (2) refactoring to match existing servers by name before removal, or (3) separating add/update logic from cleanup logic.
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
around lines 739 to 754, newly added servers aren't added to processedIds
because addMCPServer generates IDs internally and the local mcpServers closure
isn't updated yet, causing a deletion race; fix by changing addMCPServer to
return the created server ID and, when adding, push that returned ID into
processedIds (or alternatively resolve the add call to get the new ID), or
refactor logic so you match by name before the removal loop (e.g., build a set
of names from the incoming JSON and skip removals for any server whose name
exists), ensuring every added or updated server is recorded in processedIds
before the cleanup loop runs.
webdevcody
added
Testers-Requested
|
ty for the fix, I'm grabbing and combining your other one into #325 as you left a ton of .md files and stuff in this |
3 participants
🚨 Critical Bug Fix: Web Mode Cannot Manage MCP Servers
This PR fixes a critical bug that completely breaks MCP server management in web mode.
The Problem
The
syncSettingsToServer()function has an early return that blocks web mode:Impact:
The Fix
Remove the Electron-only check since both modes have a backend server:
Why this works:
getHttpApiClient()works identically in both modesAppData/Roaming/Automaker./dataAdditional Fixes Included
This PR also includes 3 related bug fixes that improve MCP server UX:
Bug #1: Race Condition in Save/Test Flow
syncSettingsToServer()to complete before showing success toastsuse-mcp-servers.tsBug #2: HTTP Error Messages
http-api-client.tsBug #3: JSON Export Format
use-mcp-servers.tsTesting
Validation completed:
Files Changed
apps/ui/src/hooks/use-settings-migration.ts- Bug allow customizablity for all hotkeys in settings #4 (critical)apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts- Bugs feat(sidebar): implement trash functionality for project management #1, TEST DONT MERGE #3apps/ui/src/lib/http-api-client.ts- Bug feat: Extend models support with AI profiles, Codex integration, and worktree isolation #2Checklist
Priority: This is a critical fix that unblocks MCP functionality for all web mode users.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.