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(browser): allow sentry in safari extension background page #13209

Conversation

undead-voron
Copy link
Contributor

Add safari-web-extension: to the list of allowed extension protocols. This chage should allow sentry/browser to work in safari browser extensions.

@undead-voron undead-voron changed the title allow sentry in safari background page allow sentry in safari extension background page Aug 2, 2024
@undead-voron undead-voron changed the title allow sentry in safari extension background page feat(browser): allow sentry in safari extension background page Aug 5, 2024
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Hey @undead-voron thanks for opening this PR! I think this sounds reasonable given we've done this before and it only applies to extensions running in dedicated pages, not within a host page. Would you mind adding a test for it? A simple one here is fine for me:

it.each(['chrome-extension', 'moz-extension', 'ms-browser-extension'])(
"doesn't log a browser extension error if executed inside an extension running in a dedicated page (%s)",
extensionProtocol => {
const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
// @ts-expect-error - this is a hack to simulate a dedicated page in a browser extension
delete WINDOW.location;
// @ts-expect-error - this is a hack to simulate a dedicated page in a browser extension
WINDOW.location = {
href: `${extensionProtocol}://mock-extension-id/dedicated-page.html`,
};
Object.defineProperty(WINDOW, 'browser', { value: { runtime: { id: 'mock-extension-id' } }, writable: true });
init(options);
expect(consoleErrorSpy).toBeCalledTimes(0);
consoleErrorSpy.mockRestore();
WINDOW.location = originalLocation;
},
);

@Lms24 Lms24 self-assigned this Aug 8, 2024
@Lms24
Copy link
Member

Lms24 commented Aug 8, 2024

heads-up: I'm only assigning myself to track internally that I'm reviewing it. I'm not gonna take over for the test, please continue :)

@undead-voron
Copy link
Contributor Author

Hi @Lms24
I've added safari extension page protocol to tests. Is there anything else you want me to do in order to make this pr acceptable for merging?

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contributing!

@Lms24 Lms24 enabled auto-merge (squash) August 13, 2024 07:38
@Lms24 Lms24 merged commit e03df37 into getsentry:develop Aug 13, 2024
120 checks passed
@undead-voron undead-voron deleted the feat(browser)/allow-sentry-in-safari-extensions branch August 13, 2024 09:10
Lms24 added a commit that referenced this pull request Aug 13, 2024
This PR adds the external contributor to the CHANGELOG.md file, so that
they are credited for their contribution. See #13209

---

Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants