-
-
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
feat: allow manually configuring the browser name #154
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 dc530a1 in 2 minutes and 44 seconds
More details
- Looked at
364
lines of code in11
files - Skipped
0
files when reviewing. - Skipped posting
9
drafted comments based on config settings.
1. src/background/client.ts:68
- Draft comment:
Good use of async getBucketId. Consider handling errors if getBrowser() fails. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. src/background/helpers.ts:39
- Draft comment:
Note: Detection for Vivaldi is missing (FIXME present). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, pointing out a FIXME note in the code. It doesn't provide a specific suggestion or request for action. It doesn't align with the rules for good comments, which should be actionable or provide a specific suggestion.
3. src/settings/main.ts:20
- Draft comment:
Consider expanding the allowed pattern for custom browser names if needed (currently only lowercase letters due to toLowerCase and pattern). - 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 seems to make an incorrect assumption - while the code converts to lowercase, it doesn't actually restrict the input pattern at all. There's no regex or validation that would prevent special characters, numbers, or other inputs. The comment is based on a misunderstanding of what toLowerCase() does.
Maybe there's an input validation pattern defined in the HTML form that we can't see? Maybe there are requirements or restrictions defined elsewhere in the codebase?
Even if there are hidden restrictions, this comment is speculative and asks for a potential enhancement without clear evidence of an actual problem. The code as shown has no pattern restrictions.
The comment should be deleted as it makes incorrect assumptions about pattern restrictions that don't exist in the code, and suggests a speculative enhancement without clear justification.
4. src/settings/main.ts:9
- Draft comment:
Reloading the extension on saving options may affect user experience; consider confirming the need to reload immediately. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. src/background/client.ts:68
- Draft comment:
Consider caching the browser name to avoid repeated async storage reads in getBucketId, since heartbeat calls it frequently. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. src/background/helpers.ts:27
- Draft comment:
Consider caching the retrieved browser name to minimize repeated storage access during frequent heartbeat events. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
7. src/settings/index.html:28
- Draft comment:
The custom browser input is set as required in the markup; ensure the initial state reflects the current selection so validation doesn’t block form submission unintentionally. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
8. src/settings/main.ts:20
- Draft comment:
When 'other' is selected without a custom value, 'other' is saved; confirm if this is intended or if additional validation is needed. - 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 asking for confirmation of intended behavior and suggesting potential validation. This violates two rules: 1) Don't ask authors to confirm their intention, and 2) Don't make speculative comments about potential issues. The code handles the UI appropriately by making the custom input required when 'other' is selected (line 51), so this is already handled.
The code could theoretically have a race condition where the required attribute isn't set before form submission. The validation might be worth considering.
The toggleCustomBrowserInput function is called on both page load and select change, and the required attribute is properly managed. The HTML5 form validation would prevent submission without the required field.
The comment should be removed as it asks for confirmation of intended behavior and the validation concern is already handled by the required attribute on the input field.
9. src/manifest.json:27
- Draft comment:
Options page is properly added; double-check compatibility of options_ui in all supported browsers. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_lQFBqdYxPyARu40i
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 9f6d2e9 in 2 minutes and 3 seconds
More details
- Looked at
363
lines of code in11
files - Skipped
0
files when reviewing. - Skipped posting
10
drafted comments based on config settings.
1. src/background/client.ts:68
- Draft comment:
Using an async getBucketId is appropriate. Ensure all callers (e.g. heartbeat functions) now correctly await it. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. src/background/helpers.ts:27
- Draft comment:
The getBrowser helper stores the detected name to storage. Verify this doesn't override user-specified settings unexpectedly. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. src/popup/index.html:61
- Draft comment:
UI addition for browser setting looks good; ensure styling and layout remain consistent across browsers. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. src/popup/main.ts:35
- Draft comment:
Displaying the stored browser name is effective. Confirm getBrowserName aligns with user-set value from settings. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. src/settings/main.ts:9
- Draft comment:
reloadExtension is non-blocking; consider any error handling if extension reload fails, though this is usually acceptable. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. src/background/client.ts:68
- Draft comment:
Ensure all call sites for getBucketId are updated to await its asynchronous nature. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
7. src/background/helpers.ts:27
- Draft comment:
Consider that getBrowser caches the browser name via storage; if a user updates it via settings and the background is already running, the new value will only be applied after reload. This behavior is likely intentional, but double-check if this meets your UX expectations. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
8. src/background/helpers.ts:39
- Draft comment:
The FIXME note for detecting Vivaldi remains; consider if adding Vivaldi detection (e.g. via userAgent checks) is feasible. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
9. src/popup/main.ts:35
- Draft comment:
getBrowserName is used directly in the popup. If the stored browser name isn’t set yet, this could result in 'undefined' being shown. Consider using getBrowser() or providing a default fallback value. - 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 raises a valid concern about error handling. However, I don't have visibility into getBrowserName() implementation or the storage module. Without seeing how getBrowserName works, I can't be certain this is actually an issue. The comment is speculative without more context.
I don't have access to the getBrowserName implementation - what if it already has a fallback? What if undefined is an acceptable display value for their use case?
While the concern about undefined values is reasonable, making assumptions about external code behavior without evidence violates our review principles.
Delete this comment since we don't have strong evidence that undefined values are actually problematic here without seeing the getBrowserName implementation.
10. src/settings/main.ts:21
- Draft comment:
The custom browser input restricts values to lowercase letters via pattern="[a-z]*". Ensure this constraint is acceptable; it excludes numbers or symbols that might be valid for custom browser names. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_kNsHLWJFeDmfgHom
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.
Very nice, thanks!
So nice to see so many issues closed! Now I just need to publish a release. |
Thank you so much! |
Thanks for fixing this issue. Just a heads up I had to add this to get it to work for me: #155 |
Important
Adds manual browser name configuration to ActivityWatch extension with UI and storage support.
src/settings/index.html
).getBucketId()
inclient.ts
to usegetBrowser()
which retrieves the browser name from storage.popup/index.html
to open the settings page for browser name configuration.getBrowser()
inhelpers.ts
to fetch the browser name from storage or detect it if not set.setBrowserName()
andgetBrowserName()
instorage.ts
to manage browser name in local storage.settings/index.html
for selecting or entering a custom browser name.popup/main.ts
to display the current browser name and provide a button to edit it.getBrowserName()
todetectBrowser()
inhelpers.ts
to clarify its purpose.manifest.json
to include the new settings page.This description was created by
for 9f6d2e9. It will automatically update as commits are pushed.