Feat zoning 010526 background capture#1502
Feat zoning 010526 background capture#1502jxiwang wants to merge 3 commits intofeat-zoning-010526from
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.
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.
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.
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.
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