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

[Analytics] Replace _execute_action with Custom Command #1799

Merged
merged 19 commits into from
Mar 28, 2025

Conversation

BrodyHughes
Copy link
Member

@BrodyHughes BrodyHughes commented Jan 10, 2025

Fixes BX-1625

What changed (plus any additional context for devs)

Currently our extension uses _execute_action for the Alt+Shift+R shortcut to open the popup. While functional, this reserved command doesn't dispatch any events we can hook into for analytics. From documentation:

The _execute_action (Manifest V3), _execute_browser_action (Manifest V2), and _execute_page_action
(Manifest V2) commands are reserved for the action of trigger your action, browser action, or page action 
respectively. These commands don't dispatch [command.onCommand] events like standard commands.

This PR replaces _execute_action with a standard command that checks command.onCommand for the execution of this shortcut, and then runs chrome.action.openPopup() in our background.js service-worker script. This gives us identical popup opening behavior while allowing us to track shortcut usage through the command.onCommand listener.

The change does not affect users at all; the shortcut remains the same and popup opening performance is unchanged since we're using Chrome's native popup API.

What to test

  • Does extension opening shortcut work identical to how it did before?
  • Does analytics for it populate?

Copy link

linear bot commented Jan 10, 2025

@BrodyHughes BrodyHughes marked this pull request as ready for review January 10, 2025 20:47
@BrodyHughes BrodyHughes changed the title Replace _execute_action with Custom Command for Analytics [Analytics] Replace _execute_action with Custom Command Jan 14, 2025
@BrodyHughes BrodyHughes self-assigned this Jan 14, 2025
@BrodyHughes BrodyHughes requested a review from walmat January 16, 2025 18:05
Copy link
Contributor

@walmat walmat left a comment

Choose a reason for hiding this comment

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

Any performance implications to always listening to chrome commands? Also, any need for other browser support?

@BrodyHughes
Copy link
Member Author

Any performance implications to always listening to chrome commands? Also, any need for other browser support?

i dont think it should have any performance implications since it's event-driven and only runs when a keyboard shortcut is pressed. I did assume firefox would work with this but i dont think it does. ill check / fix that today.

@BrodyHughes BrodyHughes requested a review from walmat January 22, 2025 19:10
@BrodyHughes BrodyHughes requested a review from jinchung February 18, 2025 16:57
@DanielSinclair
Copy link
Collaborator

Moved to WIP since e2e is failing as a side effect; maybe expecting different key registration

@DanielSinclair DanielSinclair marked this pull request as draft February 20, 2025 21:31
@BrodyHughes BrodyHughes marked this pull request as ready for review March 4, 2025 22:15
@BrodyHughes BrodyHughes requested a review from derHowie March 27, 2025 20:08
@BrodyHughes BrodyHughes merged commit 7ce293e into master Mar 28, 2025
14 checks passed
@BrodyHughes BrodyHughes deleted the brody/shortcut-analytics branch March 28, 2025 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants