-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
Allow manually configuring the browser name #149
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 9bb0688 in 1 minute and 42 seconds
More details
- Looked at
212
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. src/client.js:19
- Draft comment:
Bug: Incorrect variable name in logHttpError. Use 'error' instead of 'err'. - 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.
2. src/client.js:25
- Draft comment:
In getBrowserName, resolve() is called immediately after chrome.storage.local.set rather than waiting for confirmation of a successful write. This may hide storage errors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is technically correct - we don't wait for the storage operation to complete. However, this isn't a critical issue because: 1) The browser name is already determined correctly before the save attempt 2) A storage failure just means we won't cache it 3) The error is logged 4) The core functionality continues working. The comment suggests a change that would add complexity without much benefit.
The storage error could indicate a more systemic issue that should be handled. Also, resolving before confirmation means the next read might not get the cached value.
While true, the impact is minimal - at worst we just recompute the browser name next time. The current error logging is sufficient for debugging systemic issues.
The comment should be deleted as it suggests a change that would add complexity without meaningful benefit. The current implementation has adequate error handling for its purpose.
3. static/options.js:18
- Draft comment:
Using a fixed 500ms timeout to reload the extension may lead to race conditions if saving hasn't completed. It is safer to reload in the storage set callback. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
Since storage.set() is awaited, the timeout won't start until after the save completes. This means there isn't actually a race condition between saving and reloading. The 500ms delay seems to be intentional, possibly to allow UI updates to complete. The comment appears to have missed the await keyword.
I could be wrong about why the 500ms delay exists. Maybe there are other asynchronous operations happening that I'm not aware of.
Even if there are other async operations, the core concern about a race with storage.set is invalid due to the await keyword. The 500ms delay appears intentional rather than problematic.
The comment should be deleted because it incorrectly identifies a race condition that doesn't exist due to the await keyword on storage.set().
Workflow ID: wflow_RJ1H39NZWUhq0ba5
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 e0759c6 in 1 minute and 30 seconds
More details
- Looked at
214
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. src/client.js:19
- Draft comment:
Typo: Use 'error' instead of 'err' in logHttpError. - 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.
2. src/client.js:25
- Draft comment:
Consider error handling on chrome.storage.local.set callback. - Reason this comment was not posted:
Confidence changes required:50%
None
3. static/options.html:30
- Draft comment:
Ensure a newline at the end of file per best practices. - Reason this comment was not posted:
Confidence changes required:33%
None
4. static/options.js:34
- Draft comment:
Ensure a newline at the end of file per best practices. - Reason this comment was not posted:
Confidence changes required:33%
None
5. manifest.json:30
- Draft comment:
Verify that using 'options_ui' is compatible with all target browsers (e.g. Firefox may expect 'options_page' in some cases). - Reason this comment was not posted:
Confidence changes required:33%
None
Workflow ID: wflow_MIvS38UivzB94Eke
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 e0759c6 in 51 seconds
More details
- Looked at
214
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
13
drafted comments based on config settings.
1. manifest.json:30
- Draft comment:
Consider using the MV2 key 'options_page' instead of 'options_ui' for broader compatibility. If 'options_ui' is intentional, please confirm this fits your browser targets. - Reason this comment was not posted:
Comment did not seem useful.
2. src/client.js:25
- Draft comment:
The new getBrowserName function returns a promise that stores and retrieves the browserName. Consider handling potential cases where chrome.storage.set may fail, as the error is only logged and not acted upon. - Reason this comment was not posted:
Confidence changes required:50%
None
3. src/client.js:69
- Draft comment:
Ensure that client.browserName is always valid before using toLowerCase() in getBucketId to prevent possible issues if browserName remains null or undefined. - Reason this comment was not posted:
Confidence changes required:50%
None
4. static/options.js:11
- Draft comment:
Directly querying the button via e.target.querySelector works for the single button scenario. If multiple buttons are introduced in the future, consider selecting the button more explicitly. - Reason this comment was not posted:
Confidence changes required:20%
None
5. static/popup.js:59
- Draft comment:
The click listener attached to the new edit-btn reliably opens the options page. Consider checking for potential errors if chrome.runtime.openOptionsPage() is unsupported. - Reason this comment was not posted:
Confidence changes required:20%
None
6. manifest.json:27
- Draft comment:
Added options_ui for the options page. Ensure it works as expected in Manifest V2 and consider adding a newline at the end of the file for consistency. - Reason this comment was not posted:
Confidence changes required:33%
None
7. src/client.js:25
- Draft comment:
The getBrowserName function now caches the browser name in chrome.storage. This is a neat optimization, but note that storage.set is done asynchronously without awaiting its completion. If persisting the value is critical before use, consider handling potential errors more robustly. - Reason this comment was not posted:
Confidence changes required:33%
None
8. src/client.js:53
- Draft comment:
The async setup function now awaits getBrowserName. This improves the initialization order and ensures browserName is available when needed. - Reason this comment was not posted:
Confidence changes required:0%
None
9. static/options.html:25
- Draft comment:
The options UI is well structured. Consider adding a newline at the end of the file for consistency with other source files. - Reason this comment was not posted:
Confidence changes required:33%
None
10. static/options.js:7
- Draft comment:
Using an async promise wrapper around chrome.storage.set works fine here, though you might consider leveraging a promise-based API (if available) for cleaner syntax. Also, reloading the extension on save should be confirmed as the intended behavior. - Reason this comment was not posted:
Confidence changes required:33%
None
11. static/popup.html:61
- Draft comment:
The new browser information row (displaying browserName and an edit button) integrates well with the popup UI. For accessibility, consider adding ARIA labels to the edit button. - Reason this comment was not posted:
Confidence changes required:33%
None
12. static/popup.js:59
- Draft comment:
The event listener for the edit button correctly opens the options page. The integration for retrieving and displaying browserName from storage works well. - Reason this comment was not posted:
Confidence changes required:0%
None
13. static/style.css:64
- Draft comment:
New styling for the form and label is appropriately scoped and aligns with rest of the UI. This enhances the options page's appearance. - Reason this comment was not posted:
Confidence changes required:0%
None
Workflow ID: wflow_yHHDsXPKv7YalmO7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Maybe we could turn the browser list into a text box instead? 👀 |
Looks great! I'd be happy to merge it if you think it's ready :) |
Thanks ❤️ |
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 e0759c6 in 2 minutes and 8 seconds
More details
- Looked at
214
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. manifest.json:30
- Draft comment:
Consider adding the 'chrome_style': true property in options_ui for a more native options page appearance. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. src/client.js:25
- Draft comment:
The promise in getBrowserName resolves before confirming that chrome.storage.local.set succeeded. Consider resolving in the callback after checking for errors if persistence is critical. - 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 comment is technically correct about the timing. However, the storage operation is just caching the browser name - if it fails, the correct value is still returned. The error is logged if storage fails. The function's main purpose of getting the browser name is still achieved reliably. This seems more like an optimization than a critical issue.
The storage failure could cause unnecessary recomputation of the browser name on subsequent calls. Also, the comment could be pointing out a general pattern issue with promise handling.
While better promise handling would be nice, the current code handles failures gracefully by logging errors, and the core functionality works correctly. The performance impact of occasional storage failures would be minimal.
This comment should be deleted as it suggests an optimization that isn't critical to the code's correctness. The current implementation handles failures appropriately for this use case.
3. static/options.js:15
- Draft comment:
Consider handling potential errors from chrome.storage.local.set rather than silently reloading on success. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. static/popup.js:40
- Draft comment:
If browserName is not set in storage, the UI might display undefined. Consider providing a default value. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. src/client.js:19
- Draft comment:
Bug: In logHttpError, the error parameter is named 'error' but 'err' is used. Replace 'err.response' with 'error.response'. - 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.
6. static/options.html:13
- Draft comment:
The browser options list is hardcoded with a comment linking to a specific queries.ts line. Ensure this list stays up-to-date if browser names change. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_hlRnqhH3uzBfnVOD
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.
} | ||
|
||
document.addEventListener("DOMContentLoaded", restoreOptions); | ||
document.querySelector("form").addEventListener("submit", saveOptions); |
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.
Consider moving the form event listener registration inside a DOMContentLoaded handler to ensure the form element exists, even if the HTML structure changes in the future.
Important
Adds manual browser name configuration to ActivityWatch extension with UI for selection and storage.
options.html
andoptions.js
.getBrowserName()
inclient.js
now checkschrome.storage
for a manually set browser name.popup.html
andpopup.js
to display and edit the browser name.options.html
for browser selection.options.html
for browser selection with options like Chrome, Firefox, etc.popup.html
updated to include a button to open the options page.chrome.storage.local
inoptions.js
.popup.js
andclient.js
.This description was created by
for e0759c6. It will automatically update as commits are pushed.