fix: post-merge fixes for #802 (tests, StdioBridge flakiness, focus nudge)#804
Merged
dsarno merged 5 commits intoCoplayDev:betafrom Feb 21, 2026
Merged
fix: post-merge fixes for #802 (tests, StdioBridge flakiness, focus nudge)#804dsarno merged 5 commits intoCoplayDev:betafrom
dsarno merged 5 commits intoCoplayDev:betafrom
Conversation
LoadAssetAtPath returns the asset filename as .name for prefab roots, not the internally renamed root object name. Remove the rename + assert that was causing CombinesWithOtherModifications to fail. The test still verifies that componentProperties works alongside position changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts a prefab CRUD edit-mode test to stop asserting on a root name change that never occurs, while still validating that componentProperties and position modifications work together. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRemoves a prefab-root rename assertion and payload name in a prefab test, drops a frame-yield in a stdio reconnect test, attaches the rotating file handler to the root logger, schedules fire-and-forget Unity focus nudges for stalled running test jobs, and adds unit tests for focus-nudge utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server_get_test_job as Server.get_test_job
participant Scheduler as AsyncNudgeScheduler
participant UnitySvc as nudge_unity_focus
Client->>Server_get_test_job: Request (no wait_timeout)
Server_get_test_job->>Server_get_test_job: validate response type/status
alt response == running and should_nudge == true
Server_get_test_job->>Scheduler: asyncio.create_task(nudge_unity_focus(project_path,...))
Scheduler->>UnitySvc: nudge_unity_focus (background)
UnitySvc-->>Scheduler: (fire-and-forget result)
end
Server_get_test_job-->>Client: Immediate MCPResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider adding a brief code comment in this test explaining why prefab root names from
LoadAssetAtPath<GameObject>can’t be asserted (Unity using the filename as.name), so future maintainers don’t try to reintroduce a similar check. - Given that the test no longer verifies a rename, double-check whether the test name and scenario description still accurately reflect its purpose, and rename or clarify if necessary to avoid confusion about what behavior is under test.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding a brief code comment in this test explaining why prefab root names from `LoadAssetAtPath<GameObject>` can’t be asserted (Unity using the filename as `.name`), so future maintainers don’t try to reintroduce a similar check.
- Given that the test no longer verifies a rename, double-check whether the test name and scenario description still accurately reflect its purpose, and rename or clarify if necessary to avoid confusion about what behavior is under test.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The 5-frame yield between client2 handshake and ping created a window where the MCP Python server could reconnect and close our test client as stale. Stale-client cleanup runs synchronously in HandleClientAsync before the read loop, so no yield is needed after reading the handshake. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add file handler to root logger so __name__-based loggers (focus_nudge, run_tests) write to the log file instead of only stderr - Add fire-and-forget nudge check on non-wait_timeout get_test_job calls so stalls are detected regardless of client polling style - Add 13 unit tests for should_nudge logic, backoff reset, and gating Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dsarno
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Server/tests/test_focus_nudge.py (1)
70-96: LGTM — gating logic is well tested; one optional coverage gap.The three gates (unavailable, already focused, rate-limited) are correctly exercised. One untested path:
_get_frontmost_app()returningNone(line 526–528 offocus_nudge.pyreturnsFalsein that case). Consider adding a fourth case if the gating logic here becomes more complex, but it's not critical.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/test_focus_nudge.py` around lines 70 - 96, Add a fourth test to cover the branch where _get_frontmost_app() returns None: patch "utils.focus_nudge._is_available" to True and "utils.focus_nudge._get_frontmost_app" to return None, then call nudge_unity_focus(force=True or False as appropriate) and assert it returns False; locate this alongside existing tests for nudge_unity_focus and reference the functions/variables _get_frontmost_app, _is_available, and nudge_unity_focus when adding the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Server/src/services/tools/run_tests.py`:
- Line 336: The call to
asyncio.create_task(nudge_unity_focus(unity_project_path=project_path)) can be
GC'd because no strong reference is kept; create a module-level sentinel set
(e.g., _background_tasks = set()) near the top of the file and, when scheduling
the task from wherever create_task is called, store the returned Task in that
set, then add a done callback or use add_done_callback to remove the Task from
_background_tasks when it completes; reference the nudge_unity_focus coroutine
and the Task returned by asyncio.create_task to implement this safe
storage-and-cleanup pattern.
---
Nitpick comments:
In `@Server/tests/test_focus_nudge.py`:
- Around line 70-96: Add a fourth test to cover the branch where
_get_frontmost_app() returns None: patch "utils.focus_nudge._is_available" to
True and "utils.focus_nudge._get_frontmost_app" to return None, then call
nudge_unity_focus(force=True or False as appropriate) and assert it returns
False; locate this alongside existing tests for nudge_unity_focus and reference
the functions/variables _get_frontmost_app, _is_available, and nudge_unity_focus
when adding the new test.
- Store asyncio.create_task reference in module-level set to prevent GC (CodeRabbit) - Add test for _get_frontmost_app() returning None (CodeRabbit nitpick) - Add comment explaining why prefab root rename is not tested (Sourcery) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Server/src/services/tools/run_tests.py`:
- Around line 327-328: The bug is that progress = data.get("progress", {})
returns None when the JSON has "progress": null, causing progress.get(...) to
raise; change to use a fallback that also handles explicit None by replacing the
assignment with progress = data.get("progress") or {} so editor_is_focused =
progress.get("editor_is_focused", True) is safe, and apply the identical pattern
to the existing wait_timeout path (replace whatever assigns wait_timeout =
data.get("wait_timeout", ...) with wait_timeout = data.get("wait_timeout") or
<same-default>) in run_tests.py to avoid AttributeError when the key exists but
is null.
Comment on lines
+327
to
+328
| progress = data.get("progress", {}) | ||
| editor_is_focused = progress.get("editor_is_focused", True) |
Contributor
There was a problem hiding this comment.
progress = data.get("progress", {}) returns None when the key is present with value null.
dict.get(key, default) only substitutes default when the key is absent; an explicit null/None value is returned as-is. Since TestJobProgress | None = None means Unity can legally send "progress": null (e.g., before the first test starts), line 328 will raise AttributeError: 'NoneType' object has no attribute 'get' in that case.
The identical pattern in the pre-existing wait_timeout path (line 285) has the same bug and should be fixed there too.
🐛 Proposed fix (new path at line 327, and matching fix at line 285)
- progress = data.get("progress", {})
+ progress = data.get("progress") or {}Apply the same one-line change to the pre-existing wait_timeout path at line 285.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/src/services/tools/run_tests.py` around lines 327 - 328, The bug is
that progress = data.get("progress", {}) returns None when the JSON has
"progress": null, causing progress.get(...) to raise; change to use a fallback
that also handles explicit None by replacing the assignment with progress =
data.get("progress") or {} so editor_is_focused =
progress.get("editor_is_focused", True) is safe, and apply the identical pattern
to the existing wait_timeout path (replace whatever assigns wait_timeout =
data.get("wait_timeout", ...) with wait_timeout = data.get("wait_timeout") or
<same-default>) in run_tests.py to avoid AttributeError when the key exists but
is null.
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Post-merge fixes for #802 that weren't included in the squash merge, plus two related issues discovered during test validation.
CombinesWithOtherModifications—LoadAssetAtPathreturns filename as.namefor prefab roots, so the assertion always failedHandleClientAsync, so no yield is needed__name__-based loggers (utils.focus_nudge,services.tools.run_tests) write to the log file instead of only stderrwait_timeoutget_test_jobcalls so stalls are detected regardless of client polling styleshould_nudge()logic, backoff reset, andnudge_unity_focus()gatingTest plan
should_nudge()returns correct results,nudge_unity_focus()successfully focuses Unity via osascript__name__-based logger entries now appear in log file🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
New Features
Chores
Documentation