-
Notifications
You must be signed in to change notification settings - Fork 60
Feat zoning 010526 background capture #1502
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
base: feat-zoning-010526
Are you sure you want to change the base?
Conversation
Add background capture setup to
|
| // Setup background capture messenger if it is not already setup for visual tagging selector | ||
| if (window.opener && backgroundCaptureOptions.enabled && !visualTaggingOptions.messenger) { | ||
| /* istanbul ignore next */ | ||
| backgroundCaptureOptions.messenger?.setup({ |
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.
Repeated inits leak/duplicate listeners/observers. Consider an explicit lifecycle: store handler refs; teardown removes listeners and closes any existing instance before re‑init (e.g., WindowMessenger.setup, initialize-background-capture).
🚀 Want me to fix this? Reply ex: "fix it for me".
Add default background capture initialization to
|
| dataExtractor: dataExtractor, | ||
| logger: config?.loggerProvider, | ||
| }); | ||
| } |
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.
Background capture setup omits the endpoint derived from serverZone, so WindowMessenger defaults to US origin and rejects messages from EU (origin mismatch). Consider passing the same endpoint here as in the visual tagging path.
| dataExtractor: dataExtractor, | |
| logger: config?.loggerProvider, | |
| }); | |
| } | |
| backgroundCaptureOptions.messenger?.setup({ | |
| dataExtractor: dataExtractor, | |
| logger: config?.loggerProvider, | |
| ...(config?.serverZone && { endpoint: constants.AMPLITUDE_ORIGINS_MAP[config.serverZone] }), | |
| }); |
🚀 Want me to fix this? Reply ex: "fix it for me".
| }); | ||
| } else if (action === 'initialize-background-capture') { | ||
| this.logger?.debug?.('Initializing background capture (external script)'); | ||
| asyncLoadScript(new URL(AMPLITUDE_BACKGROUND_CAPTURE_SCRIPT_URL, this.endpoint).toString()) |
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.
Re-initializing background-capture overwrites amplitudeBackgroundCaptureInstance without closing the previous instance. Consider closing any existing instance before creating a new one to avoid leaked listeners and duplicate capture.
| asyncLoadScript(new URL(AMPLITUDE_BACKGROUND_CAPTURE_SCRIPT_URL, this.endpoint).toString()) | |
| // eslint-disable-next-line | |
| amplitudeBackgroundCaptureInstance?.close?.(); | |
| amplitudeBackgroundCaptureInstance = null; | |
| asyncLoadScript(new URL(AMPLITUDE_BACKGROUND_CAPTURE_SCRIPT_URL, this.endpoint).toString()) |
🚀 Want me to fix this? Reply ex: "fix it for me".
| }); | ||
| } else if (action === 'initialize-background-capture') { | ||
| this.logger?.debug?.('Initializing background capture (external script)'); | ||
| asyncLoadScript(new URL(AMPLITUDE_BACKGROUND_CAPTURE_SCRIPT_URL, this.endpoint).toString()) |
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.
new URL(AMPLITUDE_BACKGROUND_CAPTURE_SCRIPT_URL, this.endpoint) will throw if this.endpoint is '*'. Consider passing the absolute AMPLITUDE_BACKGROUND_CAPTURE_SCRIPT_URL directly to avoid crashing the listener.
| asyncLoadScript(new URL(AMPLITUDE_BACKGROUND_CAPTURE_SCRIPT_URL, this.endpoint).toString()) | |
| asyncLoadScript(AMPLITUDE_BACKGROUND_CAPTURE_SCRIPT_URL) |
🚀 Want me to fix this? Reply ex: "fix it for me".
Summary
Checklist