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: allow manually configuring the browser name #154

Merged
merged 5 commits into from
Feb 9, 2025

Conversation

BelKed
Copy link
Contributor

@BelKed BelKed commented Feb 6, 2025



Important

Adds manual browser name configuration to ActivityWatch extension with UI and storage support.

  • Behavior:
    • Allows manual configuration of browser name via a new settings page (src/settings/index.html).
    • Updates getBucketId() in client.ts to use getBrowser() which retrieves the browser name from storage.
    • Adds a button in popup/index.html to open the settings page for browser name configuration.
  • Functions:
    • Adds getBrowser() in helpers.ts to fetch the browser name from storage or detect it if not set.
    • Adds setBrowserName() and getBrowserName() in storage.ts to manage browser name in local storage.
  • UI:
    • Adds a dropdown and input field in settings/index.html for selecting or entering a custom browser name.
    • Updates popup/main.ts to display the current browser name and provide a button to edit it.
  • Misc:
    • Renames getBrowserName() to detectBrowser() in helpers.ts to clarify its purpose.
    • Updates manifest.json to include the new settings page.

This description was created by Ellipsis for 9f6d2e9. 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 dc530a1 in 2 minutes and 44 seconds

More details
  • Looked at 364 lines of code in 11 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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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.

src/settings/index.html Outdated Show resolved Hide resolved
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 9f6d2e9 in 2 minutes and 3 seconds

More details
  • Looked at 363 lines of code in 11 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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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.

src/settings/index.html Show resolved Hide resolved
@BelKed
Copy link
Contributor Author

BelKed commented Feb 6, 2025

@ErikBjare ErikBjare changed the title Allow manually configuring the browser name feat: allow manually configuring the browser name Feb 9, 2025
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.

Very nice, thanks!

@ErikBjare ErikBjare merged commit 0d71732 into ActivityWatch:master Feb 9, 2025
4 checks passed
@ErikBjare
Copy link
Member

So nice to see so many issues closed!

Now I just need to publish a release.

@melroy89
Copy link

melroy89 commented Feb 9, 2025

Thank you so much!

@jjcastro
Copy link
Contributor

jjcastro commented Feb 9, 2025

Thanks for fixing this issue. Just a heads up I had to add this to get it to work for me: #155

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment