recover from process message instead crash the whole app #4016
Conversation
WalkthroughThe changes modify the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (1)
v2/internal/frontend/dispatcher/dispatcher.go (1)
34-45: Solid panic recovery implementation with room for enhancement.The implementation successfully prevents app crashes while preserving error context. Consider these enhancements:
- Use structured logging for better error tracking:
- d.log.Error("process message error: %s -> %s", message, err) + d.log.Error("process message error: message=%q error=%q stack=%s", + message, + err, + string(debug.Stack()))
- Add more context to non-error panics:
- err = fmt.Errorf("%v", e) + err = fmt.Errorf("panic recovered while processing message: %v", e)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/internal/frontend/dispatcher/dispatcher.go(2 hunks)
🔇 Additional comments (2)
v2/internal/frontend/dispatcher/dispatcher.go (2)
5-5: LGTM!The addition of the "fmt" import is necessary for error formatting in the panic recovery mechanism.
33-33: LGTM! Well-structured signature change.The use of named return values, particularly for the error, enables clean error handling in the deferred function while maintaining backward compatibility.
| func (d *Dispatcher) ProcessMessage(message string, sender frontend.Frontend) (_ string, err error) { | ||
| defer func() { | ||
| if e := recover(); e != nil { | ||
| if errPanic, ok := e.(error); ok { | ||
| err = errPanic | ||
| } else { | ||
| err = fmt.Errorf("%v", e) | ||
| } | ||
| } | ||
| if err != nil { | ||
| d.log.Error("process message error: %s -> %s", message, err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for ProcessMessage error handling and panic recovery
The error handling in callers is consistent, but test coverage is missing for:
- ProcessMessage error handling
- Panic recovery scenarios
- Error propagation through the dispatcher
Please add test cases to verify:
- Normal message processing
- Error propagation
- Panic recovery functionality
- Logger error reporting
🔗 Analysis chain
Verify error handling in callers and test coverage.
Please ensure:
- All callers of
ProcessMessageproperly handle the returned errors - Test coverage exists for panic scenarios
Let's verify the callers and test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check callers of ProcessMessage
echo "Checking ProcessMessage callers:"
rg -B 2 -A 2 "ProcessMessage\(" --type go
# Check test coverage
echo "Checking test coverage:"
fd -e go -e _test.go | xargs rg "TestProcessMessage|TestDispatcher.*Process"
Length of output: 2390
Script:
#!/bin/bash
# Check for any dispatcher tests with broader patterns
echo "Checking dispatcher tests:"
fd -e _test.go . v2/internal/frontend/dispatcher/ | xargs cat
# Check for any test files that might test ProcessMessage
echo -e "\nChecking for test files containing 'dispatch' or 'message':"
fd -e _test.go . v2/internal/frontend/ | xargs rg -i "dispatch|message" -C 2
Length of output: 66531
Script:
#!/bin/bash
# Search for test files in dispatcher directory
echo "Checking for test files in dispatcher directory:"
fd -e go -g "*_test.go" . v2/internal/frontend/dispatcher/
# Search for dispatcher tests with broader pattern
echo -e "\nChecking for dispatcher-related tests:"
fd -e go -g "*_test.go" . v2/internal/frontend/ | xargs cat
Length of output: 65928
|
Thanks 🙏 Please could you add an entry to the changelog located at |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
website/src/pages/changelog.mdx (3)
Line range hint
1-1584: Consider standardizing the PR reference format throughout the changelog.For consistency, consider adopting a single format for PR references. Currently there's a mix of:
- Full URLs:
[PR](https://github.com/wailsapp/wails/pull/xxxx)- Short format:
[#xxxx]- With/without contributor references
Choose one format and apply it consistently throughout the changelog. For example:
- Fixed by @leaanthony in https://github.com/wailsapp/wails/pull/1230 + Fixed by @leaanthony in [#1230](https://github.com/wailsapp/wails/pull/1230)
Line range hint
1-1584: Consider reorganizing breaking changes sections.To improve clarity for users upgrading between versions:
- Move all breaking changes to the top of each version entry
- Ensure each deprecated feature clearly states the recommended alternative
- Consider adding migration guides for significant breaking changes
This helps users quickly assess the impact of upgrading and plan accordingly.
Line range hint
1-1584: Consider improving visual organization of the changelog.To improve readability of this large changelog:
- Add year groupings as headers
- Add more prominent visual separation between major versions
- Consider splitting into separate files by major version
This would make it easier to navigate the changelog history and find specific versions.
|
Thanks for this! |
Fixes #4015
Summary by CodeRabbit