From c0555d911a7a49bbb9f4d5e81c82ef87fb7d40d1 Mon Sep 17 00:00:00 2001 From: Ben White Date: Fri, 22 Mar 2024 11:58:28 +0100 Subject: [PATCH 1/5] Fix multiple loading of toolbar --- src/extensions/toolbar.ts | 77 ++++++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 30 deletions(-) diff --git a/src/extensions/toolbar.ts b/src/extensions/toolbar.ts index fc68a45e6..32eed1226 100644 --- a/src/extensions/toolbar.ts +++ b/src/extensions/toolbar.ts @@ -11,8 +11,13 @@ const STATE_FROM_WINDOW = window?.location ? _getHashParam(window.location.hash, '__posthog') || _getHashParam(location.hash, 'state') : null +const LOCALSTORAGE_KEY = '_postHogToolbarParams' + export class Toolbar { instance: PostHog + + private _toolbarScriptLoaded = false + constructor(instance: PostHog) { this.instance = instance } @@ -98,8 +103,8 @@ export class Toolbar { } } } else { - // get credentials from localStorage from a previous initialzation - toolbarParams = JSON.parse(localStorage.getItem('_postHogToolbarParams') || '{}') + // get credentials from localStorage from a previous initialization + toolbarParams = JSON.parse(localStorage.getItem(LOCALSTORAGE_KEY) || '{}') toolbarParams.source = 'localstorage' // delete "add-action" or other intent from toolbarParams, otherwise we'll have the same intent @@ -118,22 +123,16 @@ export class Toolbar { } } + private _callLoadToolbar(params: ToolbarParams) { + ;(assignableWindow['ph_load_toolbar'] || assignableWindow['ph_load_editor'])(params, this.instance) + } + loadToolbar(params?: ToolbarParams): boolean { - if (!window || assignableWindow['_postHogToolbarLoaded']) { + if (!window || window.localStorage.getItem(LOCALSTORAGE_KEY)) { + // If we have a toolbar in localStorage, we don't want to load it again return false } - // only load the toolbar once, even if there are multiple instances of PostHogLib - assignableWindow['_postHogToolbarLoaded'] = true - - // toolbar.js is served from the PostHog CDN, this has a TTL of 24 hours. - // the toolbar asset includes a rotating "token" that is valid for 5 minutes. - const fiveMinutesInMillis = 5 * 60 * 1000 - // this ensures that we bust the cache periodically - const timestampToNearestFiveMinutes = Math.floor(Date.now() / fiveMinutesInMillis) * fiveMinutesInMillis - const toolbarUrl = this.instance.requestRouter.endpointFor( - 'assets', - `/static/toolbar.js?t=${timestampToNearestFiveMinutes}` - ) + const disableToolbarMetrics = this.instance.requestRouter.region === 'custom' && this.instance.config.advanced_disable_toolbar_metrics @@ -143,23 +142,41 @@ export class Toolbar { apiURL: this.instance.requestRouter.endpointFor('ui'), ...(disableToolbarMetrics ? { instrument: false } : {}), } - const { source: _discard, ...paramsToPersist } = toolbarParams // eslint-disable-line - window.localStorage.setItem('_postHogToolbarParams', JSON.stringify(paramsToPersist)) + window.localStorage.setItem(LOCALSTORAGE_KEY, JSON.stringify(paramsToPersist)) + + if (this._toolbarScriptLoaded) { + this._callLoadToolbar(toolbarParams) + } else { + // only load the toolbar once, even if there are multiple instances of PostHogLib + this._toolbarScriptLoaded = true + + // toolbar.js is served from the PostHog CDN, this has a TTL of 24 hours. + // the toolbar asset includes a rotating "token" that is valid for 5 minutes. + const fiveMinutesInMillis = 5 * 60 * 1000 + // this ensures that we bust the cache periodically + const timestampToNearestFiveMinutes = Math.floor(Date.now() / fiveMinutesInMillis) * fiveMinutesInMillis + const toolbarUrl = this.instance.requestRouter.endpointFor( + 'assets', + `/static/toolbar.js?t=${timestampToNearestFiveMinutes}` + ) + + loadScript(toolbarUrl, (err) => { + if (err) { + logger.error('Failed to load toolbar', err) + return + } + this._callLoadToolbar(toolbarParams) + }) + + // Turbolinks doesn't fire an onload event but does replace the entire body, including the toolbar. + // Thus, we ensure the toolbar is only loaded inside the body, and then reloaded on turbolinks:load. + _register_event(window, 'turbolinks:load', () => { + this._toolbarScriptLoaded = false + this.loadToolbar(toolbarParams) + }) + } - loadScript(toolbarUrl, (err) => { - if (err) { - logger.error('Failed to load toolbar', err) - return - } - ;(assignableWindow['ph_load_toolbar'] || assignableWindow['ph_load_editor'])(toolbarParams, this.instance) - }) - // Turbolinks doesn't fire an onload event but does replace the entire body, including the toolbar. - // Thus, we ensure the toolbar is only loaded inside the body, and then reloaded on turbolinks:load. - _register_event(window, 'turbolinks:load', () => { - assignableWindow['_postHogToolbarLoaded'] = false - this.loadToolbar(toolbarParams) - }) return true } From df11f16be1ebfdeb1994db3450fd81eeb5880b60 Mon Sep 17 00:00:00 2001 From: Ben White Date: Fri, 22 Mar 2024 12:00:22 +0100 Subject: [PATCH 2/5] Fix comment --- src/extensions/toolbar.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extensions/toolbar.ts b/src/extensions/toolbar.ts index 32eed1226..485b1fbf8 100644 --- a/src/extensions/toolbar.ts +++ b/src/extensions/toolbar.ts @@ -129,7 +129,7 @@ export class Toolbar { loadToolbar(params?: ToolbarParams): boolean { if (!window || window.localStorage.getItem(LOCALSTORAGE_KEY)) { - // If we have a toolbar in localStorage, we don't want to load it again + // The toolbar will clear the localStorage key when it's done with it. If it is present that indicates the toolbar is already open and running return false } From 9edd55029ac79d75d631d0da23c320adacbce13a Mon Sep 17 00:00:00 2001 From: Ben White Date: Fri, 22 Mar 2024 12:03:49 +0100 Subject: [PATCH 3/5] Fixes --- src/extensions/toolbar.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/extensions/toolbar.ts b/src/extensions/toolbar.ts index 485b1fbf8..8be7600fd 100644 --- a/src/extensions/toolbar.ts +++ b/src/extensions/toolbar.ts @@ -128,7 +128,7 @@ export class Toolbar { } loadToolbar(params?: ToolbarParams): boolean { - if (!window || window.localStorage.getItem(LOCALSTORAGE_KEY)) { + if (!window || (window.localStorage.getItem(LOCALSTORAGE_KEY) && this._toolbarScriptLoaded)) { // The toolbar will clear the localStorage key when it's done with it. If it is present that indicates the toolbar is already open and running return false } @@ -142,8 +142,13 @@ export class Toolbar { apiURL: this.instance.requestRouter.endpointFor('ui'), ...(disableToolbarMetrics ? { instrument: false } : {}), } - const { source: _discard, ...paramsToPersist } = toolbarParams // eslint-disable-line - window.localStorage.setItem(LOCALSTORAGE_KEY, JSON.stringify(paramsToPersist)) + window.localStorage.setItem( + LOCALSTORAGE_KEY, + JSON.stringify({ + ...toolbarParams, + source: undefined, + }) + ) if (this._toolbarScriptLoaded) { this._callLoadToolbar(toolbarParams) From 2b7d46cfb620897fc3d42fb80abfaf2ce340a1b8 Mon Sep 17 00:00:00 2001 From: Ben White Date: Fri, 22 Mar 2024 12:04:29 +0100 Subject: [PATCH 4/5] Fix --- src/extensions/toolbar.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/extensions/toolbar.ts b/src/extensions/toolbar.ts index 8be7600fd..db15fabd1 100644 --- a/src/extensions/toolbar.ts +++ b/src/extensions/toolbar.ts @@ -169,6 +169,7 @@ export class Toolbar { loadScript(toolbarUrl, (err) => { if (err) { logger.error('Failed to load toolbar', err) + this._toolbarScriptLoaded = false return } this._callLoadToolbar(toolbarParams) From bf7a0c1da387ce9dcc079a78b12e732e0606a42e Mon Sep 17 00:00:00 2001 From: Ben White Date: Fri, 22 Mar 2024 12:06:11 +0100 Subject: [PATCH 5/5] Added test --- src/__tests__/extensions/toolbar.test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/__tests__/extensions/toolbar.test.ts b/src/__tests__/extensions/toolbar.test.ts index c5cd49cfe..091b706f9 100644 --- a/src/__tests__/extensions/toolbar.test.ts +++ b/src/__tests__/extensions/toolbar.test.ts @@ -35,7 +35,6 @@ describe('Toolbar', () => { beforeEach(() => { assignableWindow.ph_load_toolbar = jest.fn() - delete assignableWindow['_postHogToolbarLoaded'] }) describe('maybeLoadToolbar', () => { @@ -176,6 +175,13 @@ describe('Toolbar', () => { expect(toolbar.loadToolbar(toolbarParams)).toBe(true) expect(toolbar.loadToolbar(toolbarParams)).toBe(false) }) + + it('should load if previously loaded but closed via localstorage', () => { + expect(toolbar.loadToolbar(toolbarParams)).toBe(true) + expect(toolbar.loadToolbar(toolbarParams)).toBe(false) + localStorage.removeItem('_postHogToolbarParams') + expect(toolbar.loadToolbar(toolbarParams)).toBe(true) + }) }) describe('load and close toolbar with minimal params', () => {