-
-
Notifications
You must be signed in to change notification settings - Fork 7
feat: open external links in default browser #112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 49856df in 2 minutes and 51 seconds
More details
- Looked at
43
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. src-tauri/src/lib.rs:152
- Draft comment:
The variable 'url' is initialized once outside the loop and never updated, even after navigation. This may cause incorrect behavior if the intended original URL changes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The code appears to be implementing a feature where any non-localhost URLs are opened in the default browser instead of the app window. The original URL is intentionally kept as a reference point to navigate back to. This is a common pattern for desktop apps that want to handle external links by opening them in the system browser. The fact thaturl
never changes is the desired behavior.
Could there be edge cases where we want the "original" URL to update? What if the user legitimately navigates to a different localhost URL?
The code specifically checks for non-localhost URLs, so legitimate localhost navigation would be allowed. The unchangingurl
variable is working as intended as an "anchor" to return to after handling external links.
The comment should be deleted as it misunderstands the intentional behavior of the code. The unchangingurl
variable is a feature, not a bug.
2. src-tauri/src/lib.rs:157
- Draft comment:
Polling for URL changes every second is straightforward but might be inefficient. Consider using an event-driven approach for better performance. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. src-tauri/src/lib.rs:152
- Draft comment:
Capturing the URL once outside the loop means that the original URL is never updated. Confirm this is intended, as subsequent internal navigations may be ignored. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
Looking at the code, this appears to be intentional behavior. The function is designed to intercept navigation to external URLs and open them in the default browser instead, while keeping the app on the original localhost URL. Not updating the URL reference is actually crucial for this functionality to work correctly - it's the "home" URL we want to return to.
Could there be edge cases where we want to allow some internal navigation between different localhost URLs? The current code would prevent that.
The code specifically checks for non-localhost URLs before taking action, so internal localhost navigation would still work. The original URL is only used when redirecting back after handling an external link.
The comment raises a non-issue - the URL behavior is intentional and correct for the feature's purpose of handling external links.
4. src-tauri/src/lib.rs:149
- Draft comment:
Multiple unwraps in the thread may panic if the window or URL becomes unavailable. Consider using proper error handling. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The unwraps could cause panics in edge cases like window closure. However, this is a background thread monitoring URLs - a panic here wouldn't be catastrophic. The code is relatively simple and the failure modes are clear. Adding error handling would make the code more complex without much benefit since we'd likely just want to exit the thread on error anyway.
I may be underestimating the impact of a panic in this thread. If this thread crashes, it could affect URL handling functionality without any user visibility into the failure.
While true, the thread is monitoring URLs in a loop - if it panics, the worst case is that external URLs stop opening in the browser. The main app functionality would continue working.
While technically correct, the comment suggests adding complexity for minimal reliability benefit. The current simple code with clear failure modes is acceptable for this background monitoring task.
5. src-tauri/src/lib.rs:167
- Draft comment:
Consider logging when an external URL is opened, or when navigation is reverted, to aid debugging and visibility into this behavior. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. src-tauri/src/lib.rs:413
- Draft comment:
Typographical error: the error message in the autostart manager disable branch uses 'autosart' instead of 'autostart'. Please correct the typo for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_bSsVaJn3XIA6o89A
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
let app = &*get_app_handle().lock().expect("failed to get app handle"); | ||
let mut main_window = app.get_webview_window("main").unwrap(); | ||
let url = main_window.url().unwrap(); | ||
loop { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spawned thread runs an infinite loop without termination logic. Consider adding a condition to break the loop when the window is closed to prevent thread leaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is expected behavior
Turns out this only works for links without |
#105
Important
Add
listen_for_url_changes()
to open external URLs in the default browser and integrate it into the app setup.listen_for_url_changes()
inlib.rs
to monitor URL changes in the main window.listen_for_url_changes()
inrun()
to activate the new behavior.This description was created by
for 49856df. It will automatically update as commits are pushed.