Fix logMessageReceivedThreaded#2
Closed
kurobon-jp wants to merge 3 commits into
Closed
Conversation
🐙 Octopus Review — PR #2SummaryThis PR switches Score
Risk Assessment
Positive Highlights
Important Files Changed
Diagramgraph TD
LogSource["Log Source (any thread)"]
ThreadedEvent["Application.logMessageReceivedThreaded"]
Handler["OnLogMessageReceived()"]
Guard{"IsShowErrorIndicator?"}
TypeCheck{"Error/Exception/Assert<br/>and not blinking?"}
SetError["HasError = true"]
StopCo["StopAllCoroutines()"]
StartCo["StartCoroutine(Blink())"]
Danger["⚠️ Unity API - Main Thread ONLY"]
LogSource --> ThreadedEvent
ThreadedEvent --> Handler
Handler --> Guard
Guard -- "false" --> Exit["return"]
Guard -- "true" --> TypeCheck
TypeCheck -- "false" --> Exit
TypeCheck -- "true" --> SetError
SetError --> StopCo
StopCo --> StartCo
StopCo --> Danger
StartCo --> Danger
classDef changed fill:#fbbf24,stroke:#92400e,color:#000
classDef danger fill:#f87171,stroke:#991b1b,color:#000
classDef unchanged fill:#374151,stroke:#6b7280,color:#e5e7eb
class Handler,Guard changed
class StopCo,StartCo,Danger danger
class LogSource,ThreadedEvent,TypeCheck,SetError,Exit unchanged
Last reviewed commit: 8c738bd Checklist
|
🐙 Octopus Review — PR #2SummaryThis PR makes two changes: (1) switches Score
Risk Assessment
Positive Highlights
Important Files Changed
Diagramgraph TD
LogThread["Background Thread / Main Thread"]
LogThread -- "logMessageReceivedThreaded" --> Callback["OnLogMessageReceived"]
Callback --> Guard{"IsShowErrorIndicator?"}
Guard -- "No" --> Return["return (safe)"]
Guard -- "Yes" --> TypeCheck{"Error / Exception / Assert?"}
TypeCheck -- "No / already blinking" --> Return
TypeCheck -- "Yes" --> SetError["HasError = true"]
SetError --> StopCoros["StopAllCoroutines()"]
StopCoros --> StartCoros["StartCoroutine(Blink())"]
classDef changed fill:#fbbf24,stroke:#92400e,color:#000
classDef unsafe fill:#f87171,stroke:#991b1b,color:#000
classDef safe fill:#4ade80,stroke:#166534,color:#000
class Callback changed
class StopCoros,StartCoros unsafe
class Guard,TypeCheck,SetError,Return safe
Last reviewed commit: 8c738bd Checklist
Reviewed by Octopus |
![octopus-review[bot]](https://avatars.githubusercontent.com/in/2917745?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 files reviewed, 2 findings | View scores & details
🔴 Findings that could not be mapped to diff lines:
| Severity | File | Title | Description |
|---|---|---|---|
| 🔴 | Assets/Runtime/Scripts/Log/ErrorIndicator.cs:L42 |
Unity Coroutine API called from non-main thread | logMessageReceivedThreaded fires on the thread that emitted the log message, which can be any background thread. `Stop… |
🟡 Additional findings
| Severity | File | Title | Description |
|---|---|---|---|
| 🟡 | Assets/Runtime/Scripts/Log/ErrorIndicator.cs:L23 |
IsShowErrorIndicator guard removed from subscription — always registers now | Previously the subscription was conditional: if IsShowErrorIndicator was false the handler was never registered, so th… |
Reviewed by Octopus Review, an AI-powered PR review tool.
🐙 Octopus Review — PR #2SummaryThis PR switches Score
Risk Assessment
Positive Highlights
Important Files Changed
Diagramgraph TD
LogThread["Worker Thread / Main Thread"]
LogThread -- "logMessageReceivedThreaded" --> Handler["OnLogMessageReceived()"]
Handler --> Guard1{"IsShowErrorIndicator?"}
Guard1 -- "false" --> Drop["return (no-op)"]
Guard1 -- "true" --> Guard2{"Error type AND not blinking?"}
Guard2 -- "false" --> Drop
Guard2 -- "true" --> SetError["HasError = true"]
SetError --> StopCoro["StopAllCoroutines()"]
StopCoro --> StartCoro["StartCoroutine(Blink())"]
classDef changed fill:#fbbf24,stroke:#92400e,color:#000
classDef unchanged fill:#374151,stroke:#6b7280,color:#e5e7eb
classDef danger fill:#f87171,stroke:#991b1b,color:#000
class Handler,Guard1 changed
class Guard2,SetError,StopCoro,StartCoro danger
class LogThread,Drop unchanged
Last reviewed commit: 8c738bd Checklist
|
There was a problem hiding this comment.
2 files reviewed, 0 findings | View scores & details
Reviewed by Octopus Review, an AI-powered PR review tool.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Application.logMessageReceived += OnLogMessageReceived; | ||
| } | ||
|
|
||
| Application.logMessageReceivedThreaded += OnLogMessageReceived; |
There was a problem hiding this comment.
🟡 Subscription occurs even when IsShowErrorIndicator is false, wasting callback overhead
The original code only subscribed to logMessageReceived when IsShowErrorIndicator was true. The new code subscribes unconditionally and checks the flag inside the handler. While functionally equivalent, this means every log message (including verbose Info logs at high frequency) will invoke the handler and read SDSettings.Instance even when the feature is disabled. If the setting is stable at runtime, restoring the conditional subscription in OnEnable is the lower-overhead pattern. If the setting can change at runtime, consider caching it or using a property-change event.
Suggested change
| Application.logMessageReceivedThreaded += OnLogMessageReceived; | |
| protected override void OnEnable() | |
| { | |
| if (SDSettings.Instance.IsShowErrorIndicator) | |
| { | |
| Application.logMessageReceivedThreaded += OnLogMessageReceived; | |
| } | |
| Clear(); | |
| } |
AI Fix Prompt
Fix the following Medium (Performance) issue in `Assets/Runtime/Scripts/Log/ErrorIndicator.cs` at line 22:
Problem: The original code only subscribed to logMessageReceived when IsShowErrorIndicator was true. The new code subscribes unconditionally and checks the flag inside the handler. While functionally equivalent, this means every log message (including verbose Info logs at high frequency) will invoke the handler and read SDSettings.Instance even when the feature is disabled. If the setting is stable at runtime, restoring the conditional subscription in OnEnable is the lower-overhead pattern. If the setting can change at runtime, consider caching it or using a property-change event.
Suggested fix:
protected override void OnEnable()
{
if (SDSettings.Instance.IsShowErrorIndicator)
{
Application.logMessageReceivedThreaded += OnLogMessageReceived;
}
Clear();
}
There was a problem hiding this comment.
2 files reviewed, 0 findings | View scores & details
Reviewed by Octopus Review, an AI-powered PR review tool.
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.
Octopus Review テスト