Skip to content

fix(mcp): use static Message API to prevent null context crash#1775

Merged
piorpua merged 2 commits intomainfrom
fix/sentry-ELECTRON-D
Mar 29, 2026
Merged

fix(mcp): use static Message API to prevent null context crash#1775
piorpua merged 2 commits intomainfrom
fix/sentry-ELECTRON-D

Conversation

@kaizhou-lab
Copy link
Copy Markdown
Collaborator

@kaizhou-lab kaizhou-lab commented Mar 27, 2026

Summary

  • Replace hook-based useMessage with static Message API in useMcpServerCRUD to prevent TypeError: Cannot read properties of null (reading 'addInstance') when the component unmounts during async MCP operations
  • Remove message parameter from useMcpServerCRUD signature and update both callers (ToolsModalContent, McpManagement)
  • Add 4 unit tests verifying static Message API is used for toggle, delete, and edit error/success paths

Changes

src/renderer/hooks/mcp/useMcpServerCRUD.ts

  • Import static Message from @arco-design/web-react
  • Remove message parameter (hook-based useMessage return)
  • Replace all message.success() / message.error() → Message.success() / Message.error()
  • Remove message from all useCallback dependency arrays

src/renderer/components/settings/SettingsModal/contents/ToolsModalContent.tsx
src/renderer/pages/settings/ToolsSettings/McpManagement.tsx

  • Remove message argument from useMcpServerCRUD() call

tests/unit/useMcpServerCRUD.dom.test.tsx (new)

  • Test: handleToggleMcpServer calls Message.error on sync failure
  • Test: handleToggleMcpServer calls Message.error on remove failure
  • Test: handleDeleteMcpServer calls Message.success for disabled server
  • Test: handleEditMcpServer calls Message.success after edit

Related Issue

Closes #1774

Sentry: ELECTRON-D — 29 occurrences, last seen 2026-03-26

Verification

  • Unit tests pass (4/4)
  • Type check passes (tsc --noEmit)
  • This is main process/renderer code — unit tests are sufficient

Test Plan

  • Unit tests cover all Message.* call sites in the hook
  • Type check passes
  • Lint and format pass
  • Manual: open Settings > Tools, toggle an MCP server on/off, close settings during operation — no crash

@kaizhou-lab
fix(mcp): use static Message API to prevent null context crash in use…
96c2a25 
…McpServerCRUD

Replace hook-based useMessage with static Message import from @arco-design/web-react.
When the component unmounts during an async MCP operation (toggle/delete/edit), the
hook-based contextHolderRef becomes null, causing TypeError on addInstance. The static
API is lifecycle-independent and safe to call after unmount.

Fixes ELECTRON-D
@kaizhou-lab kaizhou-lab marked this pull request as ready for review March 27, 2026 03:29
This was referenced Mar 27, 2026
Closed
Merged
@piorpua piorpua added the bot:reviewing Review in progress (mutex) label Mar 29, 2026
@piorpua
Copy link
Copy Markdown
Contributor

piorpua commented Mar 29, 2026

CI 检查未通过

以下 job 在本次自动化 review 时未通过，请修复：

Job 结论
Unit Tests (ubuntu-latest) ❌ FAILURE

本次自动化 review 暂缓，待 CI 全部通过后将重新处理。

@piorpua piorpua added bot:ci-waiting CI failed and author notified — snoozed until new commits are pushed and removed bot:reviewing Review in progress (mutex) labels Mar 29, 2026
@piorpua piorpua added bot:reviewing Review in progress (mutex) and removed bot:ci-waiting CI failed and author notified — snoozed until new commits are pushed labels Mar 29, 2026
@piorpua
Copy link
Copy Markdown
Contributor

piorpua commented Mar 29, 2026

Code Review：fix(mcp): use static Message API to prevent null context crash (#1775)

变更概述

本 PR 修复了 Sentry 错误 ELECTRON-D（TypeError: Cannot read properties of null (reading 'addInstance')）。根本原因是 useMcpServerCRUD 使用了 hook-based useMessage，当组件在异步 MCP 操作中途卸载时，context holder 变为 null，导致崩溃。修复方案是将所有 message.success/error 替换为静态 Message.success/error API，并同步更新两处调用方（ToolsModalContent、McpManagement）。

方案评估

结论：✅ 方案合理

使用静态 Message API 是 Arco Design 处理组件卸载后仍需弹出提示场景的标准方案，与项目已有代码（如 OfficeDocViewer、PreviewPanel 中的相同修复）保持一致。fix 范围准确，仅改动受影响的 hook 及两处调用方，没有引入额外复杂度。

问题清单

🔵 LOW — 删除操作错误路径缺少测试覆盖

文件：tests/unit/useMcpServerCRUD.dom.test.tsx

问题说明：测试覆盖了 handleDeleteMcpServer 的成功路径（disabled 服务器 → Message.success），但未覆盖 removeMcpFromAgents 抛出时的错误路径（Message.error(t('settings.mcpDeleteError'))）。若将来有人误将此处改回 hook-based API，该 catch 分支不会有测试失败。

其余三个已测试路径（handleToggleMcpServer 两个错误路径 + handleEditMcpServer 成功路径）均有覆盖，整体测试质量良好。

汇总

# 严重级别 文件 问题
1 🔵 LOW tests/unit/useMcpServerCRUD.dom.test.tsx handleDeleteMcpServer 错误路径（Message.error）未测试

结论

✅ 批准合并 — 方案正确，无阻塞性问题，仅一处低优先级测试覆盖缺口。

本报告由本地 pr-review skill 生成，包含完整项目上下文，无截断限制。

CONCLUSION: APPROVED
IS_CRITICAL_PATH: false
PR_NUMBER: 1775

@piorpua
Copy link
Copy Markdown
Contributor

piorpua commented Mar 29, 2026

✅ 已自动 review，无阻塞性问题，正在触发自动合并。

@piorpua piorpua merged commit 802e3d0 into main Mar 29, 2026
17 checks passed
@piorpua piorpua deleted the fix/sentry-ELECTRON-D branch March 29, 2026 07:00
@piorpua piorpua added bot:done Auto-merged by bot and removed bot:reviewing Review in progress (mutex) labels Mar 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bot:done Auto-merged by bot

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix(mcp): TypeError on null addInstance when toggling MCP server after unmount

2 participants

@kaizhou-lab @piorpua