Skip to content

Conversation

0xbrayo
Copy link
Member

@0xbrayo 0xbrayo commented Mar 3, 2025

#105


Important

Add listen_for_url_changes() to open external URLs in the default browser and integrate it into the app setup.

  • Behavior:
    • Adds listen_for_url_changes() in lib.rs to monitor URL changes in the main window.
    • Opens external URLs (non-localhost) in the default browser and navigates back to the previous URL.
  • Integration:
    • Calls listen_for_url_changes() in run() to activate the new behavior.

This description was created by Ellipsis for 49856df. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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 that url 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 unchanging url 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 unchanging url 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% <= threshold 50%
    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% <= threshold 50%
    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 {
Copy link

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is expected behavior

@0xbrayo 0xbrayo marked this pull request as draft March 3, 2025 15:24
@0xbrayo
Copy link
Member Author

0xbrayo commented Apr 30, 2025

Turns out this only works for links without target="_blank" attribute. Been messing around with workarounds this week, none that I feel are good enough.

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