[Communication] PR4: CommsNode and Mailbox Simplification and Compatibility - #234
[Communication] PR4: CommsNode and Mailbox Simplification and Compatibility#234Joseph0120 wants to merge 7 commits into
Conversation
|
🧱 Stack PR · Branch end (9 PRs in stack) Stack Structure:
|
![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 simplifies the communication system by refactoring CommsNode to delegate active state tracking to CommsManager instead of managing its own termination state. Corresponding updates were made to AgentBase, IADS, and Mailbox to register and check node activity via CommsManager. Feedback focuses on the complete deletion of the MailboxTests suite, which significantly reduces test coverage and should instead be updated to match the new design, as well as adding a safety check for SimManager.Instance in Mailbox.Send to prevent potential null reference exceptions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCommunication node registration is now null-safe and centralized through Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Assets/Scripts/IADS/IADS.cs (1)
48-51: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCheck
CommsManager.Instancefor null.In
RegisterSimulationStartedandOnDestroy, you correctly null-checkCommsManager.Instancebefore accessing it. However, inStart(),Instanceis accessed directly. Depending on Unity's script execution order,CommsManagermight not be fully initialized or could be missing from the scene, which would throw aNullReferenceException.Apply the same null-check pattern here for safety and consistency.
🛡️ Proposed fix for consistent validation
// Create a communication node for the IADS. CommsNode = new CommsNode(Configs.AgentType.Iads); - CommsManager.Instance.AddNode(CommsNode); + CommsManager commsManager = CommsManager.Instance; + if (commsManager != null) { + commsManager.AddNode(CommsNode); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/Scripts/IADS/IADS.cs` around lines 48 - 51, Update Start() to guard the CommsManager.Instance access with the same null-check pattern used in RegisterSimulationStarted and OnDestroy, while preserving CommsNode creation and registration when the manager exists.Assets/Scripts/Managers/CommsManager.cs (1)
32-36: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnsubscribe from
SimManagerevents to prevent memory leaks.
CommsManagersubscribes to events onSimManager.Instancebut does not unsubscribe. IfCommsManageris ever destroyed (e.g., during a scene transition or manual removal), it will cause a "Lapsed Listener" memory leak and potential missing reference exceptions sinceSimManagerholds onto the delegates. Furthermore, the use of an anonymous lambda (() => _nodes.Clear()) makes unsubscription impossible.Convert the lambda to a named method and implement
OnDestroy()to safely clean up all three listeners.🐛 Proposed fix to ensure lifecycle safety
private void Start() { - SimManager.Instance.OnSimulationEnded += () => _nodes.Clear(); + SimManager.Instance.OnSimulationEnded += ClearNodes; SimManager.Instance.OnNewInterceptor += RegisterNewAgent; SimManager.Instance.OnNewLauncher += RegisterNewAgent; } + + private void ClearNodes() => _nodes.Clear(); + + private void OnDestroy() { + if (SimManager.Instance != null) { + SimManager.Instance.OnSimulationEnded -= ClearNodes; + SimManager.Instance.OnNewInterceptor -= RegisterNewAgent; + SimManager.Instance.OnNewLauncher -= RegisterNewAgent; + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/Scripts/Managers/CommsManager.cs` around lines 32 - 36, Update CommsManager.Start to subscribe a named handler instead of the anonymous () => _nodes.Clear() lambda, then add OnDestroy to unsubscribe that handler and RegisterNewAgent from all three SimManager.Instance events. Keep the handler’s existing _nodes.Clear behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Assets/Scripts/IADS/IADS.cs`:
- Around line 48-51: Update Start() to guard the CommsManager.Instance access
with the same null-check pattern used in RegisterSimulationStarted and
OnDestroy, while preserving CommsNode creation and registration when the manager
exists.
In `@Assets/Scripts/Managers/CommsManager.cs`:
- Around line 32-36: Update CommsManager.Start to subscribe a named handler
instead of the anonymous () => _nodes.Clear() lambda, then add OnDestroy to
unsubscribe that handler and RegisterNewAgent from all three SimManager.Instance
events. Keep the handler’s existing _nodes.Clear behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3b2a9fd-155c-4c5b-9634-d241bac7a545
📒 Files selected for processing (7)
Assets/Scripts/Agents/AgentBase.csAssets/Scripts/Communication/CommsNode.csAssets/Scripts/Communication/Mailbox.csAssets/Scripts/IADS/IADS.csAssets/Scripts/Managers/CommsManager.csAssets/Tests/EditMode/MailboxTests.csAssets/Tests/EditMode/MailboxTests.cs.meta
💤 Files with no reviewable changes (2)
- Assets/Tests/EditMode/MailboxTests.cs.meta
- Assets/Tests/EditMode/MailboxTests.cs
Joseph0120
changed the title
ec703bb to
136b6f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Assets/Scripts/Communication/Mailbox.cs`:
- Around line 171-176: Update IsNodeActive to return false when the supplied
CommsNode is terminated, preserving the existing null and CommsManager
membership checks. Ensure terminated receivers are rejected before they can be
queued and reported as delivered.
- Line 9: Update all subscribers of Mailbox.OnMessageDelivered, especially the
subscription in MailboxTests, to use a single Message parameter matching the
Action<Message> event contract; alternatively, restore the previous event
signature if two-parameter callbacks must remain supported.
In `@Assets/Scripts/IADS/IADS.cs`:
- Around line 61-64: Update IADS.Start() to safely handle a missing CommsManager
before registering CommsNode, matching the null-check already used in
RegisterSimulationStarted. Preserve registration when CommsManager.Instance is
available and avoid calling AddNode on a null manager.
- Around line 61-64: Update the IADS initialization flow around
CommsManager.Instance and AddNode so CommsNode registration occurs after
CommsManager’s simulation-start handler clears _nodes. Enforce an explicit
execution order or defer registration until the clear has completed, ensuring
CommsNode is not immediately removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a5b4f7d-2ecc-45c9-93f2-9d1bc8fc6570
📒 Files selected for processing (3)
Assets/Scripts/Communication/Mailbox.csAssets/Scripts/IADS/IADS.csAssets/Scripts/Managers/CommsManager.cs
b6cb9d4 to
eebe3c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Assets/Scripts/IADS/IADS.cs`:
- Around line 61-64: Guard the initial communications registration in
IADS.Start() by checking CommsManager.Instance before calling
AddNode(CommsNode), matching the existing null-safe registration logic. Ensure
startup proceeds without throwing when no CommsManager exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e8cdc031-331d-4e58-87a2-60306f1c8beb
📒 Files selected for processing (3)
Assets/Scripts/Communication/Mailbox.csAssets/Scripts/IADS/IADS.csAssets/Scripts/Managers/CommsManager.cs
2 participants
No description provided.