-
-
Notifications
You must be signed in to change notification settings - Fork 58
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
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 3d7048c in 2 minutes and 57 seconds
More details
- Looked at
86
lines of code in1
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%
<= threshold50%
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%
<= threshold50%
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.
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 fb381a3 in 2 minutes and 40 seconds
More details
- Looked at
85
lines of code in1
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%
<= threshold50%
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%
<= threshold50%
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%
<= threshold50%
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%
<= threshold50%
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.
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.
👍 Looks good to me! Reviewed everything up to e1975ee in 2 minutes and 45 seconds
More details
- Looked at
88
lines of code in1
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%
<= threshold50%
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%
<= threshold50%
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%
<= threshold50%
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%
<= threshold50%
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%
<= threshold50%
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%
<= threshold50%
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%
<= threshold50%
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%
<= threshold50%
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.
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.
Does Firefox not expose similar accessibility APIs as Chrome and Safari?
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 :) |
Important
Adds support for extracting URLs from Firefox browsers using AppleScript in the
MainThing
class.MainThing
class.FIREFOX_BROWSERS
list inMainThing
.getFirefoxURL()
function inwindowTitleChanged()
to extract URL.pollActiveWindow()
to detect Firefox browsers and extract URLs.This description was created by
for e1975ee. It will automatically update as commits are pushed.