Fix duplicate connection verification logs: add debounce and state-ch…

[dsarno]

…ange-only logging

Summary by CodeRabbit

• Bug Fixes

  • Improved health-check handling to prevent overlapping verifications, reduce duplicate logs, and provide consistent health labels and UI indicators (Unknown/Healthy/Warning/Unhealthy).

• Performance

  • Added a debounce to global refreshes to avoid excessive refresh cycles and improve responsiveness.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds debounce to the editor window refresh and introduces a concurrency guard plus explicit health-state tracking in the connection UI to avoid overlapping verifications and redundant health-status logs.

Changes

Cohort / File(s)	Summary
Editor Window Debouncing MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs	Added lastRefreshTime and RefreshDebounceSeconds (0.5s) and early-return debounce logic in RefreshAllData to prevent rapid successive refreshes.
Connection Verification & Health Status MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs	Added verificationTask guard and lastHealthStatus plus health-status constants. Split verification into VerifyBridgeConnectionInternalAsync, track explicit newStatus values, avoid duplicate logs by only logging on status change, and ensure UI health indicator classes are applied consistently.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Files requiring extra attention:
  • MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs — async concurrency, lifecycle of verificationTask, and correct handling of lastHealthStatus across all verification paths.
  • MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs — debounce timing and interactions with existing refresh logic to ensure no missed updates.

Poem

🐰
I hopped through code to slow the race,
A debounce stitch, a guarded place.
No noisy pings, just one bright cheer—
Healthy signs when rabbits near. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main objectives of the PR: fixing duplicate connection verification logs through debounce and state-change-only logging.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 019bdbe and cdd28ab.

📒 Files selected for processing (1)

• MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (4 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (2)

MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)

• MCPServiceLocator (11-85)

MCPForUnity/Editor/Helpers/McpLog.cs (4)

• McpLog (7-52)
• Warn (43-46)
• Debug (31-35)
• Error (48-51)

🔇 Additional comments (3)

MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (3)

47-54: LGTM! Well-structured state tracking.

The health status constants eliminate magic strings and the instance fields appropriately support the debounce/state-tracking goals of this PR.

---

418-428: Concurrency guard is appropriate for Unity Editor context.

The guard pattern correctly prevents overlapping verification calls. Since Unity Editor UI callbacks execute on the main thread, the check-then-assign is safe here. The implementation achieves the PR's goal of reducing duplicate verification attempts.

---

430-495: State-change-only logging is well implemented.

The logic correctly tracks state transitions and only logs when the health status actually changes, which directly addresses reducing log spam. However, verify that bridgeService.VerifyAsync() at line 449 never returns null. If it could, accessing result.Success would throw a NullReferenceException.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (3)

47-48: New verification state fields are appropriate; enums could further tighten status handling

verificationTask and lastHealthStatus cleanly capture “one in‑flight verification” and “last logged health state” without changing the public API. Consider, optionally, using an enum (or constants) instead of raw strings for states like "Disconnected", "Healthy", "Ping Failed", and "Unhealthy" to avoid future typos and make refactors safer.

---

412-422: Concurrency guard on verification effectively debounces overlapping checks

The guard on verificationTask prevents overlapping calls to VerifyBridgeConnectionInternalAsync, which should materially reduce log spam from rapid triggers. Note that callers that hit the early‑return path won’t wait on the in‑flight verification; they simply no‑op. If you ever need later callers to observe the result of the already‑running verification, you could instead await the existing task:

public async Task VerifyBridgeConnectionAsync()
{
    if (verificationTask != null && !verificationTask.IsCompleted)
    {
        await verificationTask;
        return;
    }

    verificationTask = VerifyBridgeConnectionInternalAsync();
    await verificationTask;
}

Right now, the current behavior aligns well with the “debounce repeated checks” goal.

---

427-441: Health-state logging de‑dupe looks good; align comments and status strings

The use of lastHealthStatus to gate logging in all branches (success, ping‑failed, unhealthy, not‑running) nicely eliminates duplicate messages when repeated verifications yield the same state.

A couple of small follow‑ups you may want to address:

• The comments // Always log warnings/errors no longer match behavior, since warnings and errors now log only on state change. Consider updating them to something like “Log once per distinct warning/error state” or removing them.
• When the bridge is not running, this method sets healthStatusLabel.text = "Disconnected" while UpdateConnectionStatus() uses "Unknown" for the disconnected case. Because UpdateConnectionStatus() runs every editor update, the "Disconnected" text will be very transient. For consistency in the UI copy (and to make lastHealthStatus values more self‑evident), it might be worth standardizing on a single string for the “not running” state.
• Minor nit: removing then immediately re‑adding "unknown" to healthIndicator is redundant; you can safely drop the RemoveFromClassList("unknown") call here.

None of these affect correctness, but they’ll keep behavior and comments in sync with the new log‑deduping semantics.

Also applies to: 450-489

MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (1)

24-25: RefreshAllData debounce is simple and effective; confirm the 0.5s window matches UX expectations

Using EditorApplication.timeSinceStartup with lastRefreshTime to short‑circuit RefreshAllData calls within RefreshDebounceSeconds should eliminate the rapid double‑refresh from CreateGUI and OnFocus without adding complexity. The only behavioral change to be aware of is that very quick re‑focuses (<0.5s) won’t trigger a refresh, which seems like a reasonable trade‑off.

If you later find this too aggressive or too lax, you could promote RefreshDebounceSeconds to a more discoverable constant (or setting), but the current hard‑coded value is fine for now.

Also applies to: 186-193

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b57a2ec and 019bdbe.

📒 Files selected for processing (2)

• MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (3 hunks)
• MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (2 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-10-13T13:27:23.040Z

Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.

Applied to files:

• MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs
• MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs

📚 Learning: 2025-09-04T01:01:11.927Z

Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.

Applied to files:

• MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs

🧬 Code graph analysis (1)

MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)

MCPForUnity/Editor/Helpers/McpLog.cs (4)

• McpLog (7-52)
• Warn (43-46)
• Debug (31-35)
• Error (48-51)

[dsarno]

Small cleanup of previous PR to reduce duplicate logs from toggline window (when debug logs is on)