make linux tray failing to start a non-critical error#1081
make linux tray failing to start a non-critical error#1081
Conversation
Summary of ChangesHello @scosman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the application's robustness by preventing crashes when the system tray fails to start. It introduces platform-specific error handling for tray initialization, allowing the application to continue running on Linux even if the tray component encounters an issue, while maintaining critical failure behavior for macOS and Windows. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 WalkthroughModule-level logger added to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes the application more resilient on Linux by treating a failure to start the system tray as a non-critical error. The change involves wrapping the tray initialization in a try-except block, logging any exceptions, and only re-raising them on macOS and Windows. This is a good improvement for Linux users who might not have a graphical environment fully set up. My review includes a minor suggestion to clean up the platform detection logic for better clarity and correctness.
app/desktop/desktop.py
Outdated
| except Exception: | ||
| logger.error("Error running tray", exc_info=True) | ||
| # Tray not starting on MacOS or Windows is critical. Let Linux continue to start the app. | ||
| if sys.platform in ["darwin", "Windows", "win32"]: |
There was a problem hiding this comment.
While this check works, the value "Windows" is not a standard value for sys.platform. On Windows, sys.platform is 'win32'. Including "Windows" is redundant and could be confusing for future maintainers. It would be cleaner to remove it for better code clarity. I see this pattern is used elsewhere in the file, but it would be good to correct it here to avoid propagating it.
| if sys.platform in ["darwin", "Windows", "win32"]: | |
| if sys.platform in ["darwin", "win32"]: |
📊 Coverage ReportOverall Coverage: 91% Diff: origin/main...HEAD
Summary
Line-by-lineView line-by-line diff coverageapp/desktop/desktop.pyLines 127-142 127
128 try:
129 # running detached since we use tk mainloop to get events from dock icon
130 self.tray.run_detached()
! 131 except Exception:
! 132 logger.error("Error running tray", exc_info=True)
133 # Tray not starting on MacOS or Windows is critical.
134 # Let Linux continue to start the app as tray is more fragmented there and requires system deps.
! 135 if sys.platform in ["darwin", "win32"]:
! 136 raise
137 else:
! 138 self.tray = None
139
140 def close_splash(self):
141 try:
142 import pyi_splash # type: ignore
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/desktop/desktop.py`:
- Around line 128-135: If run_detached() fails on Linux we must clear the
partially-initialised KilnTray instance so later quit_app() doesn't call stop()
on a broken tray; in the except block for the self.tray.run_detached() call,
after logging the error and before continuing for Linux, set self.tray = None
(or otherwise reset the KilnTray reference) so quit_app()'s unconditional
self.tray.stop() guard won't attempt to stop an unstarted tray; reference
self.tray, run_detached(), KilnTray and quit_app() to locate where to add this
reset.
- Line 134: The platform check list includes the invalid value "Windows"; update
the sys.platform checks (the one using if sys.platform in ["darwin", "Windows",
"win32"] and the other similar occurrence) to remove "Windows" so they read if
sys.platform in ["darwin", "win32"], ensuring only valid sys.platform values are
used ("darwin" and "win32").
1 participant
See #1080
Summary by CodeRabbit