Skip to content

fix(image-generation-mcp): address review issues from PR #1505#1601

Merged
IceyLiu merged 2 commits intomainfrom
fix/image-generation-mcp-review-followup
Mar 20, 2026
Merged

fix(image-generation-mcp): address review issues from PR #1505#1601
IceyLiu merged 2 commits intomainfrom
fix/image-generation-mcp-review-followup

Conversation

@piorpua
Copy link
Copy Markdown
Contributor

@piorpua piorpua commented Mar 20, 2026

Summary

Follow-up to #1505 — addresses all issues identified in the code review.

Issues Fixed

# Severity File Issue Fix Applied
1 🟡 MEDIUM ToolsModalContent.tsx:394 clearImageGenerationAgentStatus calls ConfigStorage.set inside React state updater, causing potential double invocation in Strict Mode Moved side effect outside the updater; pass direct value to setAgentInstallStatus
2 🔵 LOW McpService.ts:151 getDetectionTarget duplicates the fork-Gemini detection logic already present in getAgentForConfig Refactored getDetectionTarget to call getAgentForConfig, eliminating the duplicate conditional

Note: Issue #3 (LOW — Object.assign vs spread in map) was investigated but not changed: the project lint rule no-map-spread (oxlint) flags spread inside map as inefficient and explicitly prefers Object.assign. The original code is correct per lint rules.

Related

Follow-up to #1505

Test Plan

  • bun run test — all tests pass (1160 passed)
  • bunx tsc --noEmit — no type errors
  • bunx oxlint --fix — lint clean (0 warnings, 0 errors)
  • bunx oxfmt — format clean
  • Verify clearImageGenerationAgentStatus no longer triggers double ConfigStorage.set in React Strict Mode

- Fix clearImageGenerationAgentStatus calling ConfigStorage.set inside
  React state updater, causing potential double invocation in Strict Mode;
  move side effect outside the updater and pass direct value to setter
- Refactor getDetectionTarget to reuse getAgentForConfig, eliminating
  duplicated fork-Gemini detection logic

Review follow-up for #1505
@piorpua
Copy link
Copy Markdown
Contributor Author

piorpua commented Mar 20, 2026

PR Fix 验证报告

原始 PR: #1505
Follow-up PR: #1601

# 严重级别 文件 问题 修复方式 状态
1 🟡 MEDIUM ToolsModalContent.tsx:394 clearImageGenerationAgentStatus 在 state updater 内调用 ConfigStorage.set 副作用 ConfigStorage.set 移至 updater 外部,直接传值给 setAgentInstallStatus;dependency array 补充 agentInstallStatus ✅ 已修复
2 🔵 LOW McpService.ts:151 getDetectionTargetgetAgentForConfig 重复 fork-Gemini 判断逻辑 重构 getDetectionTarget 改为调用 getAgentForConfig,消除重复条件 ✅ 已修复
3 🔵 LOW ToolsModalContent.tsx:364 Object.assign 替代 spread,风格不一致 经调查,项目 lint 规则 no-map-spread 明确要求在 map 中使用 Object.assign,原代码符合 lint 规则,保持不变 ⏭️ 跳过(lint 规则要求保留)

总结: ✅ 已修复 2 个 | ⏭️ 跳过 1 个(lint 规则限制)

Add Step 7 to run oxlint on changed .ts/.tsx files before reading
file contents. The lint output serves as a baseline so that
lint-clean patterns are not flagged as style issues during review.

Renumber subsequent steps (7->8, 8->9, 9->10, 10->11) accordingly.
@IceyLiu IceyLiu merged commit c69b2b4 into main Mar 20, 2026
15 of 17 checks passed
@piorpua piorpua deleted the fix/image-generation-mcp-review-followup branch March 20, 2026 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants