Use mutation events for injections mounts too

[felixfbecker]

#2909 used a mutation observer for code view detection, but still called the inject* functions on every mutation event. It turns out ReactDOM.render() is not idempotent and triggered mutation events in Firefox, resulting in an infinite loop of mutation events.
The solution is to only re-render the mounts when a node was added to the DOM that should contain a mount, which means the render only happens once.

To accomplish that, this PR changes all get*Mount functions to receive a container. The mount getter needs to check if the mount belongs into the container, if yes get or create the mount inside, otherwise return null.
The mount getters are then called with addedNodes from mutation events.

This necessitates other changes:

• We can't use React context for telemetry anymore, see Stop using React context API #3180. This refactors the webapp to use props instead (also renames eventLogger to telemetryService for consistency / easier passing).
• The HoverOverlayContainer component had unneeded code in it (portals, hover overlay mount container container) that I removed
• componentWillUnmount() and handling Subscriptions was missing in some places

Fixes #3180
Fixes #3166

Test plan:

• GitHub
• GHE
• Gitlab
• Phabricator
• Bitbucket

x (Chrome, Firefox)

[lguychard]

Code changes LGTM.

Could we add a unit test in code_intelligence.test.tsx that reproduces the issue we ran into? Something like this:

    test('only renders elements if their mount locations have been emitted in mutations', async () => {
        const { services } = await integrationTestContext()
        const mutations = new Subject<MutationRecordLike[]>()
        const globalDebugMount = createTestElement()
        const getGlobalDebugMount: MountGetter = container => (container === globalDebugMount ? globalDebugMount : null)
        subscriptions.add(
            handleCodeHost({
                mutations: mutations.pipe(startWith([{ addedNodes: [document.body], removedNodes: [] }])),
                codeHost: {
                    name: 'test',
                    check: () => true,
                    getGlobalDebugMount,
                },
                extensionsController: createMockController(services),
                showGlobalDebug: true,
                ...createMockPlatformContext(),
            })
        )
        // Simulate the MutationObserver firing after each React render (this happens on FF)
        RENDER.mockImplementation(() => {
            mutations.next([{ addedNodes: [], removedNodes: [] }])
        })
        // Simulate mount being added to the DOM
        mutations.next([{ addedNodes: [globalDebugMount], removedNodes: [] }])
        expect(elementRenderedAtMount(globalDebugMount)).not.toBeUndefined()
    })

[lguychard]

Potential bug here: if the MountGetter is not precise enough (for example if its selector is too generic) and returns non-null results for several addedNodes, we could render several elements.

This is hypothetical, I'd estimate the likelihood of it actually happening to be pretty low.

[felixfbecker]

Can you give an example? Which selector do you mean, the one that gets the container or the one that checks the preexisting element?

[lguychard]

Consider this:

const getCommandPaletteMount: MountGetter = (container: HTMLElement): HTMLElement | null => {
  const headerElement = querySelectorOrSelf(container, '.aui-header-primary .aui-nav')
  if (!headerElement) {
   return null
  }
 
...
}

If '.aui-header-primary .aui-nav' matches for n separate added nodes, we will inject the command palette n times.

[felixfbecker]

Yeah, the functions will have to ensure that doesn't happen 👍

[sentry]

Sentry issue: BROWSER-EXTENSION-Q5

[sentry]

Sentry issue: BROWSER-EXTENSION-NB

[sentry]

Sentry issue: BROWSER-EXTENSION-PZ