Codex/optimize and paginate read console tool #511
Conversation
Further optimize token usage by reducing the default count from 50 to 10 entries. Even 10-20 messages with stack traces can be token-heavy. Added tests for default behavior and paging functionality. Updated tool description to document defaults and paging support.
The default types filter changed to ['error', 'warning'] (excluding 'log'), so tests using Debug.Log() need to explicitly request log messages. Also added format='detailed' to HandleCommand_Get_Works test since it accesses structured message fields.
Reviewer's GuideOptimizes the Unity console read tool by changing defaults (fewer messages, plain output, no stacktraces, error/warning only), adding server- and editor-side support for paginated reads via page_size/cursor, and updating tests to cover the new behavior and stacktrace handling. Sequence diagram for paginated Unity console readssequenceDiagram
actor User
participant Client as MCP_client
participant Server as read_console_tool
participant Unity as Unity_instance
participant CS as ReadConsole_HandleCommand
User->>Client: Request console logs page 1
Client->>Server: read_console(action=get, page_size, cursor=None)
Server->>Server: coerce_int(page_size, cursor)
Server->>Unity: send_tool_request(params_dict)
Unity->>CS: HandleCommand(params)
CS->>CS: GetConsoleEntries(types, count, pageSize, cursor,
CS->>CS: filterText, format, includeStacktrace)
CS->>CS: Compute resolvedPageSize, resolvedCursor,
CS->>CS: pageEndExclusive, usePaging
CS->>CS: Iterate console entries
CS->>CS: totalMatches++, add entries within
CS->>CS: (cursor, cursor+pageSize]
CS-->>Unity: SuccessResponse(payload with
CS-->>Unity: cursor, pageSize, next_cursor,
CS-->>Unity: truncated, total, items)
Unity-->>Server: Response JSON
Server->>Server: Strip stacktrace from items
Server-->>Client: Tool result (paginated items,
Server-->>Client: next_cursor, total)
Client-->>User: Render page 1 logs
User->>Client: Request next page
Client->>Server: read_console(action=get,
Client->>Server: page_size, cursor=next_cursor)
note over Server,CS: Same flow repeats with new cursor
Class diagram for updated ReadConsole tool methodsclassDiagram
class ReadConsole {
+static object HandleCommand(JObject params)
-static object GetConsoleEntries(List~string~ types, int? count, int? pageSize, int? cursor, string filterText, string format, bool includeStacktrace)
}
class GetConsoleEntriesLocals {
-List~object~ formattedEntries
-int retrievedCount
-int totalMatches
-bool usePaging
-int resolvedPageSize
-int resolvedCursor
-int pageEndExclusive
}
ReadConsole .. GetConsoleEntriesLocals : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds cursor-based paging to console retrieval across the Unity C# editor handler and Python service, changes defaults (types → error/warning, format → plain, include_stacktrace → false, default get count → 10), and updates integration and Unity tests to exercise paging and new defaults. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Service as Python read_console
participant Editor as Unity Editor (C# ReadConsole)
participant Buffer as Editor Log Buffer
rect rgb(220,235,255)
Note over Client,Service: Request first page (page_size, cursor)
Client->>Service: read_console(page_size=PS, cursor=CUR, types, count...)
Service->>Editor: HandleCommand { action:get, pageSize:PS, cursor:CUR, types, ... }
Editor->>Buffer: scan/filter entries by types/filterText/since
Editor->>Editor: compute resolvedCursor, pageEndExclusive, totalMatches
Editor->>Editor: accumulate items for [cursor..pageEnd)
Editor-->>Service: response { items, nextCursor, truncated, total, cursor, pageSize }
Service-->>Client: normalized response (items, nextCursor, truncated, total)
end
rect rgb(255,245,210)
Note over Client,Service: Request subsequent page if nextCursor provided
Client->>Service: read_console(page_size=PS, cursor=nextCursor, ...)
Service->>Editor: HandleCommand { pageSize:PS, cursor:nextCursor, ... }
Editor->>Buffer: scan/filter and slice next window
Editor-->>Service: next page payload or final page (truncated=false)
Service-->>Client: normalized response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new paging branch in
GetConsoleEntriesreturnsdata.items,next_cursor, andtruncated, but the Python tool and new tests work withdata.linesandhasMore; consider aligning the C# response shape with what the tool expects (or adding a translation layer) so paging works consistently in real Unity runs, not just with the mockedfake_send. - You changed the default
typesfrom['error', 'warning', 'log']/{"error","warning","log"}to only error and warning in both Python and C#; if existing callers rely on seeinglogmessages without specifyingtypes, consider whether this should remain the default or if a migration/explicit behavior change is needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new paging branch in `GetConsoleEntries` returns `data.items`, `next_cursor`, and `truncated`, but the Python tool and new tests work with `data.lines` and `hasMore`; consider aligning the C# response shape with what the tool expects (or adding a translation layer) so paging works consistently in real Unity runs, not just with the mocked `fake_send`.
- You changed the default `types` from `['error', 'warning', 'log']`/`{"error","warning","log"}` to only error and warning in both Python and C#; if existing callers rely on seeing `log` messages without specifying `types`, consider whether this should remain the default or if a migration/explicit behavior change is needed.
## Individual Comments
### Comment 1
<location> `Server/tests/integration/test_read_console_truncate.py:63` </location>
<code_context>
+ assert captured["params"]["includeStacktrace"] is False
@pytest.mark.asyncio
</code_context>
<issue_to_address>
**issue (testing):** Paging integration test payload is now out of sync with the Unity handler’s paging response shape
`test_read_console_paging` still expects a `lines`/`hasMore` payload, but the C# handler now returns paging data as `items`, `cursor`, `next_cursor`, `truncated`, and `total`. Since `fake_send` is acting as Unity, it should be updated to mirror this new shape and the assertions adjusted to verify the new fields instead of `hasMore`, so the test accurately validates the real paging contract and can catch regressions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assert captured["params"]["includeStacktrace"] is False | ||
|
|
||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
issue (testing): Paging integration test payload is now out of sync with the Unity handler’s paging response shape
test_read_console_paging still expects a lines/hasMore payload, but the C# handler now returns paging data as items, cursor, next_cursor, truncated, and total. Since fake_send is acting as Unity, it should be updated to mirror this new shape and the assertions adjusted to verify the new fields instead of hasMore, so the test accurately validates the real paging contract and can catch regressions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
MCPForUnity/Editor/Tools/ReadConsole.cs (3)
362-380: Consider early exit optimization in paging mode.In paging mode, the loop continues iterating through all entries to compute
totalMatches, even after the current page is filled. While this is needed for thetruncatedflag andtotalcount, it could be slow with large console logs.If performance becomes an issue, consider:
- Only counting one entry past
pageEndExclusiveto determinetruncated(avoiding full count)- Adding an optional parameter to skip total count calculation
For now, the implementation is correct and provides accurate totals.
410-428: Inconsistent property naming in paging payload.The payload mixes naming conventions:
pageSize(camelCase)next_cursor(snake_case)truncated,total,items,cursor(lowercase)Consider standardizing to either camelCase (
nextCursor) or snake_case (page_size) for consistency with the rest of the API.🔎 Suggested fix for consistent naming
var payload = new { cursor = resolvedCursor, pageSize = resolvedPageSize, - next_cursor = nextCursor, + nextCursor = nextCursor, truncated = truncated, total = totalMatches, items = formattedEntries, };Or alternatively, use snake_case throughout for alignment with Python conventions.
383-408:EndGettingEntriesmay be invoked twice on error.When an exception occurs during iteration,
EndGettingEntriesis called in the catch block (line 389) and again in the finally block (line 401). While both calls are wrapped in try-catch, this could cause unintended side effects if Unity's internal API doesn't handle double invocation gracefully.Consider removing the call from the catch block since the finally block guarantees cleanup:
🔎 Proposed fix
catch (Exception e) { Debug.LogError($"[ReadConsole] Error while retrieving log entries: {e}"); - // Ensure EndGettingEntries is called even if there's an error during iteration - try - { - _endGettingEntriesMethod.Invoke(null, null); - } - catch - { /* Ignore nested exception */ - } + // EndGettingEntries will be called in the finally block return new ErrorResponse($"Error retrieving log entries: {e.Message}"); }Server/src/services/tools/read_console.py (1)
88-106: Consider extracting stacktrace stripping into a helper function.The stacktrace stripping logic is repeated for
data["lines"],data["items"], and the legacy list format. This could be simplified with a helper:🔎 Proposed refactor
def _strip_stacktrace_from_list(items: list) -> None: """Remove stacktrace fields from a list of log entries.""" for item in items: if isinstance(item, dict) and "stacktrace" in item: item.pop("stacktrace", None) # Then in the response handling: if isinstance(resp, dict) and resp.get("success") and not include_stacktrace: try: data = resp.get("data") if isinstance(data, dict): for key in ("lines", "items"): if key in data and isinstance(data[key], list): _strip_stacktrace_from_list(data[key]) break elif isinstance(data, list): _strip_stacktrace_from_list(data) except Exception: passTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (1)
44-78: Consider adding a unit test for paging behavior.The current tests verify basic get/clear functionality but don't exercise the new paging parameters (
pageSize,cursor). Consider adding a test that:
- Generates multiple log entries
- Requests with
pageSizeandcursor- Verifies the paging payload structure (
next_cursor,truncated,total,items)Would you like me to draft a paging test case for
ReadConsoleTests.cs?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
MCPForUnity/Editor/Tools/ReadConsole.csServer/src/services/tools/read_console.pyServer/tests/integration/test_read_console_truncate.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Tools/ReadConsole.cs
🧬 Code graph analysis (3)
Server/tests/integration/test_read_console_truncate.py (1)
Server/src/services/tools/read_console.py (1)
read_console(17-108)
Server/src/services/tools/read_console.py (1)
Server/src/services/tools/utils.py (2)
coerce_bool(11-24)coerce_int(63-77)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialReproTests.cs (1)
JObject(40-43)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.cs (1)
JObject(58-61)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.cs (1)
JObject(67-70)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs (1)
JObject(79-82)
🪛 Ruff (0.14.10)
Server/tests/integration/test_read_console_truncate.py
99-99: Unused function argument: cmd
(ARG001)
99-99: Unused function argument: kwargs
(ARG001)
129-129: Unused function argument: cmd
(ARG001)
129-129: Unused function argument: kwargs
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (10)
MCPForUnity/Editor/Tools/ReadConsole.cs (2)
166-178: LGTM on default behavior changes and parameter extraction.The changes appropriately:
- Default
typesto["error", "warning"](excluding verbose logs by default)- Support both camelCase (
pageSize) and snake_case (page_size) for client flexibility- Default
formatto"plain"for lighter payloads- Default
includeStacktracetofalseto reduce response size
245-249: Verify paging boundary logic uses correct indexing.The paging logic uses 1-based
totalMatches(incremented after a match is found) compared against 0-based cursor boundaries. On line 364, the conditiontotalMatches > resolvedCursor && totalMatches <= pageEndExclusivecorrectly implements inclusive start, exclusive end semantics.For example, with
cursor=0, pageSize=5:
pageEndExclusive = 5- Matches 1-5 are included (where
totalMatches > 0 && totalMatches <= 5)This appears correct, but please verify with edge cases (empty results, single item, exact page boundary).
Server/src/services/tools/read_console.py (4)
14-16: Good documentation update in description.The tool description now clearly documents the default behavior (10 most recent entries) and mentions paging support with
page_size/cursor.
40-46: LGTM on default behavior alignment.The Python service defaults now match the C# handler:
types→['error', 'warning']format→'plain'include_stacktrace→FalseUsing
coerce_intandcoerce_boolutilities ensures robust handling of string inputs from various clients.
57-63: Breaking change: default count now 10 instead of unlimited.Previously, omitting
countreturned all console entries. Now it defaults to 10. While this improves performance, it's a breaking change for clients that relied on the old behavior.Clients wanting all entries must now explicitly pass
count="all"orcount="*".Ensure this breaking change is documented in release notes or migration guide.
72-73: LGTM on parameter mapping.The parameter names correctly map to the C# handler expectations (
pageSize,cursorin camelCase).TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (1)
22-22: Tests correctly adapted for new defaults.The tests now explicitly pass
types = ["error", "warning", "log"]andcount = 10to ensure they captureDebug.Logmessages, which are no longer included by default. This maintains test coverage despite the default behavior change.Also applies to: 38-38
Server/tests/integration/test_read_console_truncate.py (3)
55-60: LGTM on updated expectations.The test correctly expects:
stacktracefield stripped from response wheninclude_stacktrace=FalseincludeStacktraceparameter sent asFalseby default
91-118: Good test coverage for default count behavior.The test correctly verifies that
count=10is sent to the Unity handler when the caller doesn't specify a count. The static analysis warnings about unusedcmdandkwargsare false positives—these are required by the function signature being mocked.
173-177: Verify final page returns expected item count.With
cursor=20andpageSize=5against 25 total messages, the last page should return exactly 5 items (indices 20-24). The assertions look correct.Note: The
hasMoreassertion should be updated if the mock structure is aligned with the C# response (usingtruncatedinstead).
- Fix property naming consistency: next_cursor -> nextCursor (C# camelCase) - Remove redundant EndGettingEntries call from catch block (already in finally) - Extract stacktrace stripping to helper function (reduce duplication) - Fix test mock to match actual C# response structure (items, nextCursor, truncated, total)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
MCPForUnity/Editor/Tools/ReadConsole.cs (1)
360-380: Consider adding early exit for paging to improve performance.The paging logic is correct, but the loop continues iterating through all console entries even after the current page is filled. For large console logs, this could be inefficient.
🔎 Proposed optimization with early exit
totalMatches++; if (usePaging) { if (totalMatches > resolvedCursor && totalMatches <= pageEndExclusive) { formattedEntries.Add(formattedEntry); retrievedCount++; } + // Early exit: we've filled the page and only need to check if more exist + else if (totalMatches > pageEndExclusive) + { + // We've passed the page; totalMatches now indicates truncation + break; + } }Note: This optimization changes the semantics of
totalfrom "total matching entries" to "at least this many". If accurate total count is required, keep the current implementation.Server/tests/integration/test_read_console_truncate.py (1)
99-104: Consider underscore-prefixing unused mock arguments (optional).The
cmdandkwargsarguments are required to match the signature ofasync_send_command_with_retry, but Ruff flags them as unused. This is a common pattern for mock functions and can be safely ignored, though prefixing with underscores would suppress the warnings.🔎 Optional: Suppress linter warnings
- async def fake_send(cmd, params, **kwargs): + async def fake_send(_cmd, params, **_kwargs):Also applies to: 129-152
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Tools/ReadConsole.csServer/src/services/tools/read_console.pyServer/tests/integration/test_read_console_truncate.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/src/services/tools/read_console.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Tools/ReadConsole.cs
🧬 Code graph analysis (2)
MCPForUnity/Editor/Tools/ReadConsole.cs (1)
MCPForUnity/Editor/Helpers/Response.cs (2)
SuccessResponse(11-33)SuccessResponse(28-32)
Server/tests/integration/test_read_console_truncate.py (2)
Server/src/services/tools/read_console.py (1)
read_console(24-108)Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
🪛 Ruff (0.14.10)
Server/tests/integration/test_read_console_truncate.py
99-99: Unused function argument: cmd
(ARG001)
99-99: Unused function argument: kwargs
(ARG001)
129-129: Unused function argument: cmd
(ARG001)
129-129: Unused function argument: kwargs
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (7)
MCPForUnity/Editor/Tools/ReadConsole.cs (4)
166-178: LGTM on parameter extraction and new defaults.The new defaults align well with the PR objectives:
- Types default to
["error", "warning"]instead of requiring explicit specification- Format defaults to
"plain"for leaner responsesincludeStacktracedefaults tofalseto exclude verbose stack traces by default- Pagination parameters (
pageSize,cursor) are extracted with fallback support for both camelCase and snake_case
243-249: LGTM on pagination variable initialization.The bounds checking is appropriate:
resolvedPageSizeis clamped to[1, 500]preventing excessive requestsresolvedCursoris floored at 0 preventing negative offsetsusePagingactivates when either pagination parameter is present
403-421: LGTM on pagination response structure.The response payload correctly includes all pagination metadata:
cursorandpageSizeecho back the resolved valuesnextCursoras string (or null) for cursor-based navigationtruncatedboolean indicates if more entries existtotalprovides the count of all matching entriesitemscontains the current page's entriesThe camelCase naming aligns with the commit message noting the intentional C# naming convention.
389-401: Good defensive cleanup pattern.Moving
EndGettingEntriesto afinallyblock ensures the Unity console's internal state is properly cleaned up even when exceptions occur. The nested try-catch prevents cleanup failures from masking the original error while still logging the issue.Server/tests/integration/test_read_console_truncate.py (3)
54-60: LGTM on updated test expectations.The test correctly validates the new default behavior:
stacktracefield is stripped from response whenincludeStacktraceisFalse- The
timefield is also removed (consistent with the stripping logic)includeStacktracenow defaults toFalseas expected
91-118: LGTM on default count test.The test correctly validates that
countdefaults to10when not specified. The mock returning 15 items is intentional - this test verifies the Python layer's parameter defaulting, not the C# truncation behavior.
121-185: Comprehensive paging test with correct assertions.The test thoroughly validates the pagination contract:
- First page: cursor=0, page_size=5 → 5 items, truncated=True, nextCursor="5"
- Middle page: cursor=5, page_size=5 → 5 items, truncated=True, nextCursor="10"
- Last page: cursor=20, page_size=5 → 5 items, truncated=False, nextCursor=None
The mock response structure now correctly mirrors the C# implementation (
items,nextCursor,truncated,total), addressing the concerns from previous review comments.
- Add early exit in paging loop once page is filled, avoiding iteration through remaining console entries (total becomes 'at least N') - Prefix unused mock arguments with underscores in test_read_console_truncate.py to suppress Ruff linter warnings
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
MCPForUnity/Editor/Tools/ReadConsole.cs (1)
409-427: Document the "at least" semantics of thetotalfield.The early-exit optimization at lines 370-374 means that when
truncated=true, thetotalfield represents "at least N matches exist" rather than an exact count. This is an intentional performance optimization to avoid scanning the entire console.However, the field name
totalmight mislead callers into expecting an exact count. Consider:
- Adding a code comment documenting this behavior
- Or renaming to
matchedSoFaror similar when truncated- Or documenting this in the API specification
Suggested documentation comment
var payload = new { cursor = resolvedCursor, pageSize = resolvedPageSize, nextCursor = nextCursor, truncated = truncated, + // When truncated=true, total represents "at least N" matches (early-exit optimization) + // When truncated=false, total is the exact count of matching entries total = totalMatches, items = formattedEntries, };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Tools/ReadConsole.csServer/tests/integration/test_read_console_truncate.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/tests/integration/test_read_console_truncate.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Tools/ReadConsole.cs
🧬 Code graph analysis (1)
MCPForUnity/Editor/Tools/ReadConsole.cs (1)
MCPForUnity/Editor/Helpers/Response.cs (2)
SuccessResponse(11-33)SuccessResponse(28-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (7)
MCPForUnity/Editor/Tools/ReadConsole.cs (7)
168-168: LGTM! Default types now exclude logs.The new default
["error", "warning"]is a sensible production-oriented choice that reduces noise. This aligns with the PR objective to optimize defaults.
170-173: LGTM! Pagination parameters properly extracted.The camelCase/snake_case fallback for
pageSizeprovides good API flexibility. Both parameters are correctly typed as nullable integers.
176-178: LGTM! Optimized defaults reduce response size.Changing format to
"plain"andincludeStacktracetofalsesignificantly reduces payload size for typical use cases. Clients needing detailed information can opt-in explicitly.
360-386: LGTM! Pagination logic is correct.The pagination accumulation logic properly handles:
- Cursor-based page boundaries with 1-indexed
totalMatches- Early exit optimization at line 370-374 to stop iterating once the page is filled
- Backward compatibility with non-paging mode (lines 376-386)
The condition
totalMatches > resolvedCursor && totalMatches <= pageEndExclusivecorrectly includes entries within the current page window.
389-407: LGTM! Robust error handling with proper cleanup.The error handling correctly ensures
EndGettingEntriesis invoked in the finally block (lines 395-407), even when exceptions occur during entry retrieval. The inner try-catch aroundEndGettingEntriesprevents masking the original error while logging cleanup failures.
411-421: LGTM! Well-structured pagination payload.The paging response includes all necessary metadata:
cursorandpageSizefor request contextnextCursorfor the next page (null when complete)truncatedflag for quick "has more" checktotalfor matched countitemsfor the actual entriesThe computation of
nextCursor = pageEndExclusive(line 412) correctly positions the next request to start where this page ended.
429-433: LGTM! Backward compatibility preserved.The non-paging response path (lines 429-433) maintains backward compatibility by returning a simple list when pagination parameters are absent. This ensures existing clients continue to work without modification.
- Change pageSize resolution from 'pageSize ?? count ?? 50' to 'pageSize ?? 50' so pageSize has its own default independent of count - count now only serves as the non-paging limit - Add XML docs to GetConsoleEntries with clear parameter descriptions - Update Python tool annotations to document pageSize default (50) and clarify that count is ignored when paging
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
MCPForUnity/Editor/Tools/ReadConsole.cs (1)
256-261: LGTM: Past review feedback successfully addressed.The paging resolution now gives
pageSizean independent default (50) without falling back tocount, eliminating the dual-semantics confusion mentioned in the previous review. Thecountparameter is now only used as the non-paging limit, and clampingpageSizebetween 1-500 prevents potential abuse.Note: This addresses the previous review comment about clarifying dual semantics of the
countparameter in paging mode.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Tools/ReadConsole.csServer/src/services/tools/read_console.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Tools/ReadConsole.cs
🧬 Code graph analysis (1)
Server/src/services/tools/read_console.py (1)
Server/src/services/tools/utils.py (2)
coerce_bool(11-24)coerce_int(63-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (9)
Server/src/services/tools/read_console.py (4)
14-19: LGTM: Efficient stacktrace removal helper.The in-place mutation approach is efficient, and using
popwith a default value prevents KeyError exceptions.
47-51: LGTM: Sensible defaults for console reads.The new defaults (error/warning types, plain format, no stacktrace) reduce noise and response payload size, making the tool more responsive by default.
69-70: LGTM: Reasonable default prevents performance issues.Capping the default to 10 entries when
countis not specified prevents slow responses when Unity has a large console, as noted in the comments above.
95-108: LGTM: Robust response processing.The logic correctly handles both response formats (paging dict with "items" and non-paging list), and the exception handling ensures that stacktrace stripping failures don't break the response.
MCPForUnity/Editor/Tools/ReadConsole.cs (5)
168-168: LGTM: Consistent defaults across server and Unity.The default parameter changes (types to error/warning, format to plain, includeStacktrace to false) align with the Python service defaults and improve performance.
Also applies to: 176-178
170-173: LGTM: Robust parameter extraction with naming flexibility.Supporting both
pageSize(camelCase) andpage_size(snake_case) improves client compatibility without breaking existing integrations.
233-243: LGTM: Clear and comprehensive XML documentation.The documentation accurately describes parameter behavior, including the independent defaults for
pageSize(50) andcursor(0), and clarifies thatcountis ignored during paging.
372-399: LGTM: Correct paging accumulation logic.The paging logic correctly handles cursor-based pagination:
- It skips entries before the cursor (
totalMatches > resolvedCursor)- It accumulates entries within the page window (
totalMatches <= pageEndExclusive)- It exits early once the page is filled to avoid unnecessary iteration
The non-paging branch correctly applies the
countlimit as before.
421-445: LGTM: Well-structured paging response.The paging response correctly includes:
cursorandpageSizefor request contextnextCursorpointing to the start of the next page (or null if no more results)truncatedflag indicating whether more results existtotalcount of all matching entriesitemsarray for the current pageThe non-paging path maintains backward compatibility by returning a simple list.
| page_size: Annotated[int | str, "Page size for paginated console reads. Defaults to 50 when omitted."] | None = None, | ||
| cursor: Annotated[int | str, "Opaque cursor for paging (0-based offset). Defaults to 0."] | None = None, |
There was a problem hiding this comment.
Clarify where paging defaults are applied.
The annotations state "Defaults to 50 when omitted" and "Defaults to 0", but the Python code passes None to the Unity handler (lines 52-53). The actual defaults are applied in the C# handler (ReadConsole.cs lines 259-260), not here.
Consider revising to: "Page size for paginated reads. Unity handler defaults to 50 if omitted." and "Paging cursor (0-based offset). Unity handler defaults to 0 if omitted."
🤖 Prompt for AI Agents
In Server/src/services/tools/read_console.py around lines 35 to 36, the
parameter annotations incorrectly imply Python-level defaults (50 and 0) while
the actual defaults are applied in the Unity C# handler; update the annotation
strings to state that these defaults are applied by the Unity handler (e.g.,
"Page size for paginated reads. Unity handler defaults to 50 if omitted." and
"Paging cursor (0-based offset). Unity handler defaults to 0 if omitted.") so
callers understand where defaults are enforced and no code behavior needs to
change.
1 participant
Summary by Sourcery
Optimize the read_console tool defaults and add support for paginated console reads between the server and Unity editor.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.