Skip to content
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

feat: add support for extracting URLs from Firefox browsers #109

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

BelKed
Copy link

@BelKed BelKed commented Mar 7, 2025

Important

Adds support for extracting URLs from Firefox browsers using AppleScript in the MainThing class.

  • Behavior:
    • Adds support for extracting URLs from Firefox browsers using AppleScript in MainThing class.
    • Handles Firefox, Firefox Developer Edition, and Firefox Nightly.
    • Uses AppleScript to extract URL from Firefox's UI hierarchy.
  • Code:
    • Adds FIREFOX_BROWSERS list in MainThing.
    • Implements getFirefoxURL() function in windowTitleChanged() to extract URL.
    • Updates pollActiveWindow() to detect Firefox browsers and extract URLs.

This description was created by Ellipsis for e1975ee. 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 3d7048c in 2 minutes and 57 seconds

More details
  • Looked at 86 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. aw_watcher_window/macos.swift:281
  • Draft comment:
    Ensure naming consistency for browser constants; consider using lowerCamelCase or documenting naming for FIREFOX_BROWSERS.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
2. aw_watcher_window/macos.swift:363
  • Draft comment:
    Consider logging a message when AppleScript returns no URL to aid debugging.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The code already logs errors when AppleScript fails with an error message. The only case not logged is when AppleScript succeeds but returns nil. This is likely an expected case that doesn't need logging - probably means no window/URL was found. Adding more logging here would likely just add noise.
    I could be wrong about nil being an expected case - maybe it represents a failure mode that should be logged for debugging.
    Looking at the AppleScript code, nil is returned when no window exists or no navigation toolbar exists - these are normal states that don't need error logging.
    The comment should be deleted since the code already has appropriate error logging, and the nil case represents normal operation that doesn't need logging.
3. aw_watcher_window/macos.swift:327
  • Draft comment:
    Use optional binding for frontmost.localizedName to avoid force unwrapping.
  • 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.
4. aw_watcher_window/macos.swift:377
  • Draft comment:
    The local variable 'error' in getFirefoxURL shadows the global logging function 'error'. Rename the variable (e.g. to 'scriptError') to avoid confusion.
  • Reason this comment was not posted:
    Marked as duplicate.
5. aw_watcher_window/macos.swift:363
  • Draft comment:
    Consider extracting the nested getFirefoxURL function into a separate helper for better clarity and potential reuse.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
6. aw_watcher_window/macos.swift:87
  • Draft comment:
    In the comment on line 87, please update "// there a complex open source one, but it makes it harder to compile this simple one-file swift application" to "// there is a complex open source one, but it makes it harder to compile this simple one-file swift application" for clarity.
  • 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.
7. aw_watcher_window/macos.swift:227
  • Draft comment:
    Please fix the typo in the comment on line 227: change 'hearbeat' to 'heartbeat'.
  • 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_TVg1RVBiRcRE5i3Z


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.

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 fb381a3 in 2 minutes and 40 seconds

More details
  • Looked at 85 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. aw_watcher_window/macos.swift:363
  • Draft comment:
    The AppleScript for Firefox URL extraction is hardcoded and may break if Firefox changes its UI structure. Consider handling failures more gracefully or adding fallback logic.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    The code is new, adding Firefox support. The comment raises a valid concern about UI brittleness. However, the code already has error handling and logs failures. AppleScript UI automation inherently requires some UI structure assumptions. The suggested "fallback logic" is vague - what other ways could we get the URL? This seems more like an inherent limitation than an actionable issue.
    The comment identifies a real technical risk. Firefox UI changes could break this. Maybe there are alternative AppleScript approaches that would be more robust?
    While the brittleness is real, the code handles failures gracefully by returning nil and logging errors. The comment doesn't suggest any concrete alternatives that would be more robust.
    Delete the comment. While it identifies a real limitation, the code already handles failures appropriately and the comment doesn't provide actionable suggestions for improvement.
2. aw_watcher_window/macos.swift:355
  • Draft comment:
    Log level change: using debug instead of error for tab title difference seems intentional; please confirm this log level is correct.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to confirm their intention regarding the log level change from error to debug. This violates the rule against asking the author to confirm their intention or ensure behavior is intended. The comment does not provide a specific code suggestion or ask for a test to be written.
3. aw_watcher_window/macos.swift:3
  • Draft comment:
    Explicit import of Foundation may be redundant since Cocoa already imports it. Remove if not needed.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. aw_watcher_window/macos.swift:281
  • Draft comment:
    Consider using a Set for FIREFOX_BROWSERS for O(1) lookup, although the list is small now.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
5. aw_watcher_window/macos.swift:345
  • Draft comment:
    Changed log from 'error' to 'debug' for tab title differences. Ensure this is intentional and that no errors go unreported.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
6. aw_watcher_window/macos.swift:87
  • Draft comment:
    There's a grammatical issue in the comment at line 87. The current text 'there a complex open source one, but it makes it harder to compile this simple one-file swift application' could be made clearer by adding a missing word. Consider changing it to 'there is a complex open source one, but it makes it harder to compile this simple one-file Swift application' for improved clarity.
  • 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_bVVWlM9wA8AhBWwp


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.

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.

👍 Looks good to me! Reviewed everything up to e1975ee in 2 minutes and 45 seconds

More details
  • Looked at 88 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 9 drafted comments based on config settings.
1. aw_watcher_window/macos.swift:284
  • Draft comment:
    Good job adding FIREFOX_BROWSERS constant for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. aw_watcher_window/macos.swift:366
  • Draft comment:
    Consider extracting the inline getFirefoxURL function to a separate method for improved readability and reuse.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
3. aw_watcher_window/macos.swift:379
  • Draft comment:
    NSAppleScript execution is synchronous and could block the thread; evaluate if asynchronous execution is needed to maintain UI responsiveness.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. aw_watcher_window/macos.swift:390
  • Draft comment:
    Consider logging a debug message if getFirefoxURL returns nil for troubleshooting.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
5. aw_watcher_window/macos.swift:327
  • Draft comment:
    Changed logging from 'error' to 'debug' for tab title differences improves log clarity.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
6. aw_watcher_window/macos.swift:3
  • Draft comment:
    Check if explicit 'import Foundation' is needed; Cocoa usually re-exports it.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
7. aw_watcher_window/macos.swift:281
  • Draft comment:
    Consider making FIREFOX_BROWSERS a static constant and add commentary about potential UI naming changes in Firefox.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
8. aw_watcher_window/macos.swift:360
  • Draft comment:
    The AppleScript extraction logic for Firefox is fragile because it relies on a specific UI hierarchy. Consider extracting getFirefoxURL into a helper function and adding additional fallback or error handling if Firefox changes its UI.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The comment is partially incorrect since getFirefoxURL is already a helper function. The fragility concern is valid but is already acknowledged in the code comments. The code already has error handling. No clear actionable improvements are suggested beyond what's already implemented.
    The fragility concern is legitimate - Firefox UI changes could break this. Am I being too harsh in dismissing the comment when it raises a valid architectural concern?
    While the concern is valid, the code already acknowledges this limitation in comments and implements reasonable error handling. The comment doesn't provide actionable suggestions beyond what's already done.
    Delete the comment since it suggests changes that are already implemented (helper function, error handling) and doesn't provide additional actionable improvements.
9. aw_watcher_window/macos.swift:367
  • Draft comment:
    Ensure the multiline AppleScript string’s indentation doesn’t unintentionally affect the script execution.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the author to ensure that the indentation of a multiline AppleScript string doesn't affect execution. This is a general cautionary statement and doesn't provide a specific suggestion or point out a specific issue in the code. It violates the rule against asking the author to ensure behavior is intended or tested.

Workflow ID: wflow_jFJEWAvoQpYqkSPv


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Member

@ErikBjare ErikBjare left a comment

Choose a reason for hiding this comment

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

Does Firefox not expose similar accessibility APIs as Chrome and Safari?

@BelKed
Copy link
Author

BelKed commented Mar 7, 2025

No, Firefox does not expose the same Apple Events APIs for accessing tab and window information like Chrome and Safari do on macOS. This is why we have to use AppleScript to interact with Firefox's UI elements through the Accessibility framework instead :)

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.

None yet

2 participants