Skip to content

Conversation

dsarno
Copy link
Collaborator

@dsarno dsarno commented Oct 3, 2025

The includeStacktrace parameter was working backwards - when false, it would return the full message with embedded stack traces, and when true, it would extract the stack trace but the logic was inverted.

Changes:

  • Always extract the first line as the message text
  • Only populate stackTrace field when includeStacktrace is true
  • Ensures clean, summary-only messages when includeStacktrace is false
  • Properly separates stack traces into their own field when requested

This matches the expected Unity console behavior where the summary is shown by default, and stack traces are only shown when expanded.

Should help with #302

Summary by CodeRabbit

  • Bug Fixes

    • Ensures stack traces are never shown when disabled, preventing unintended noise in console output.
    • Displays the first line of each log consistently, improving readability across messages.
  • Refactor

    • Streamlined log formatting to use a single, consistent parsing path for messages and stack traces.
  • Documentation

    • Clarified behavior of message display and stack trace inclusion in console output to set correct expectations.

The includeStacktrace parameter was working backwards - when false,
it would return the full message with embedded stack traces, and when
true, it would extract the stack trace but the logic was inverted.

Changes:
- Always extract the first line as the message text
- Only populate stackTrace field when includeStacktrace is true
- Ensures clean, summary-only messages when includeStacktrace is false
- Properly separates stack traces into their own field when requested

This matches the expected Unity console behavior where the summary
is shown by default, and stack traces are only shown when expanded.
Copy link
Contributor

coderabbitai bot commented Oct 3, 2025

Walkthrough

Reworked ReadConsole message parsing to always split the log into lines and use the first line as the message. Stack trace is explicitly nulled when includeStacktrace is false. Formatting logic now consistently derives messageOnly from the pre-split lines array.

Changes

Cohort / File(s) Summary
Console message parsing & stack trace handling
UnityMcpBridge/Editor/Tools/ReadConsole.cs
Unified message extraction by always splitting into lines and using the first line. Explicitly sets stackTrace to null when includeStacktrace is false. Simplified formatting path by deriving messageOnly from a single pre-split source.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant ReadConsole

  Caller->>ReadConsole: FormatLog(message, includeStacktrace)
  activate ReadConsole
  Note over ReadConsole: Split message into lines<br/>messageOnly = lines[0]

  alt includeStacktrace == true
    ReadConsole->>ReadConsole: stackTrace = extracted or existing
  else includeStacktrace == false
    ReadConsole->>ReadConsole: stackTrace = null
  end

  ReadConsole-->>Caller: (messageOnly, stackTrace, formatted output)
  deactivate ReadConsole
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested labels

bug

Poem

I nibble logs with tidy bite,
First line bright, the rest takes flight.
If stacks aren’t asked, I tuck them low,
A hush of trace you’ll never know.
Hop-hop—clean trails through console grass,
A bunny’s trim that lets signals pass. 🐇✨

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 succinctly identifies that the pull request corrects the behavior of the includeStacktrace parameter in the read_console function, which directly reflects the primary change in stack trace handling described in the changeset.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
UnityMcpBridge/Editor/Tools/ReadConsole.cs (1)

314-317: Redundant null assignment.

The stackTrace variable is already null when includeStacktrace is false due to the ternary operator on Line 305. This conditional block has no functional effect.

Consider removing the redundant block:

     string messageOnly = messageLines.Length > 0 ? messageLines[0] : message;
-
-    // If not including stacktrace, ensure we only show the first line
-    if (!includeStacktrace)
-    {
-        stackTrace = null;
-    }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5488af2 and 0a30e13.

📒 Files selected for processing (1)
  • UnityMcpBridge/Editor/Tools/ReadConsole.cs (1 hunks)
🔇 Additional comments (1)
UnityMcpBridge/Editor/Tools/ReadConsole.cs (1)

306-311: LGTM! Consistent message formatting logic.

The change ensures that the message field always displays the first line (summary) regardless of the includeStacktrace parameter, which aligns with the PR objective to fix the inverted behavior. The fallback to the full message when no lines are found is a good safety measure.

@dsarno dsarno merged commit ff73601 into CoplayDev:main Oct 4, 2025
1 check passed
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.

1 participant