Improve diff injection performance (#30)

* Remove await on GetFileDiff
Progress towards #28

* n roots instead of 2n

* Lint

* Trying something with portals

* Portal component, one root

* Fix element clean up, prep for later tasks

src/chrome/content.ts

@@ -1,12 +1,11 @@
 import React from 'react'
-import { createRoot } from 'react-dom/client'
-import { CadDiff } from '../components/diff/CadDiff'
-import { Loading } from '../components/Loading'
-import { Commit, DiffEntry, FileDiff, Message, MessageIds, Pull } from './types'
+import { CadDiffPage } from '../components/diff/CadDiffPage'
+import { Commit, DiffEntry, Message, MessageIds, Pull } from './types'
 import {
     getGithubPullUrlParams,
     mapInjectableDiffElements,
     getGithubCommitUrlParams,
+    createReactRoot,
 } from './web'
 
 // https://github.com/OctoLinker/injection
@@ -23,22 +22,15 @@ async function injectDiff(
     document: Document
 ) {
     const map = mapInjectableDiffElements(document, files)
-    for (const { element } of map) {
-        createRoot(element).render(React.createElement(Loading))
-    }
-
-    for (const { element, file } of map) {
-        const response = await chrome.runtime.sendMessage({
-            id: MessageIds.GetFileDiff,
-            data: { owner, repo, sha, parentSha, file },
+    const root = createReactRoot(document)
+    const cadDiffPage = React.createElement(CadDiffPage, {
+        owner,
+        repo,
+        sha,
+        parentSha,
+        map,
     })
-        if ('error' in response) {
-            console.log(response.error)
-        } else {
-            const diff = response as FileDiff
-            createRoot(element).render(React.createElement(CadDiff, diff))
-        }
-    }
+    root.render(cadDiffPage)
 }
 
 async function injectPullDiff(
@@ -73,7 +65,7 @@ async function injectCommitDiff(
     if (!commit.files) throw Error('Found no file changes in commit')
     if (!commit.parents.length) throw Error('Found no commit parent')
     const parentSha = commit.parents[0].sha
-    injectDiff(owner, repo, sha, parentSha, commit.files, document)
+    await injectDiff(owner, repo, sha, parentSha, commit.files, document)
 }
 
 gitHubInjection(async () => {

src/chrome/web.test.ts

@@ -1,3 +1,4 @@
+import React from 'react'
 import { DiffEntry } from './types'
 import {
     getElementFilename,
@@ -5,6 +6,7 @@ import {
     getGithubPullUrlParams,
     mapInjectableDiffElements,
     getSupportedWebDiffElements,
+    createReactRoot,
 } from './web'
 
 const githubPullHtmlSnippet = `
@@ -165,3 +167,9 @@ it('finds injectable elements from html and api results', () => {
     expect(element).toBeDefined()
     expect(file).toBeDefined()
 })
+
+it('adds a div element, creates a react root inside, and can render', () => {
+    const root = createReactRoot(document)
+    expect(root).toBeDefined()
+    expect(() => root.render(React.createElement('a'))).not.toThrow()
+})

src/chrome/web.ts

@@ -1,3 +1,4 @@
+import { createRoot, Root } from 'react-dom/client'
 import { isFilenameSupported, supportedSrcFormats } from './diff'
 import { DiffEntry } from './types'
 
@@ -76,18 +77,26 @@ export function mapInjectableDiffElements(
 
     const injectableElements: { element: HTMLElement; file: DiffEntry }[] = []
     for (const [index, element] of supportedElements.entries()) {
-        const apiFile = supportedFiles[index]
+        const file = supportedFiles[index]
         const filename = getElementFilename(element)
-        if (filename !== apiFile.filename) {
+        if (filename !== file.filename) {
             throw Error(
                 "Couldn't match API file with a diff element on the page. Aborting."
             )
         }
-        const diffElement = element.querySelector(
-            '.js-file-content'
-        ) as HTMLElement
-        injectableElements.push({ element: diffElement, file: apiFile })
+        injectableElements.push({ element, file })
     }
 
     return injectableElements
 }
+
+export function createReactRoot(
+    document: Document,
+    id: string = 'kittycad-root'
+): Root {
+    // TODO: there's probably a better way than this to create a root?
+    const node = document.createElement('div')
+    node.id = id
+    document.body.appendChild(node)
+    return createRoot(node)
+}

src/components/diff/CadDiff.test.tsx

@@ -3,8 +3,4 @@ import { CadDiff } from './CadDiff'
 
 it('renders the CAD diff element', () => {
     render(<CadDiff />)
-
-    // TODO: find a way to add proper tests for ModelView,
-    // seems non-trivial with the simulated DOM
-    // Probably will have to go for end-to-end
 })

src/components/diff/CadDiff.tsx

@@ -1,6 +1,6 @@
 import React, { useEffect, useState } from 'react'
 import '@react-three/fiber'
-import { Box, ThemeProvider, useTheme } from '@primer/react'
+import { Box, useTheme } from '@primer/react'
 import { FileDiff } from '../../chrome/types'
 import { Viewer3D } from './Viewer3D'
 import { STLLoader } from 'three/examples/jsm/loaders/STLLoader'
@@ -24,12 +24,7 @@ function ViewerSTL({ file, colors }: ViewerSTLProps) {
     return geomety ? <Viewer3D geometry={geomety} colors={colors} /> : null
 }
 
-type CadDiffThemedProps = FileDiff
-
-function CadDiffInternals({
-    before,
-    after,
-}: CadDiffThemedProps): React.ReactElement {
+export function CadDiff({ before, after }: FileDiff): React.ReactElement {
     const { theme } = useTheme()
     const beforeColors: WireframeColors = {
         face: theme?.colors.fg.default,
@@ -58,13 +53,3 @@ function CadDiffInternals({
         </Box>
     )
 }
-
-export type CadDiffProps = FileDiff
-
-export function CadDiff({ before, after }: CadDiffProps): React.ReactElement {
-    return (
-        <ThemeProvider colorMode="auto">
-            <CadDiffInternals before={before} after={after} />
-        </ThemeProvider>
-    )
-}

src/components/diff/CadDiffPage.tsx

@@ -0,0 +1,92 @@
+import React, { useEffect, useState } from 'react'
+import '@react-three/fiber'
+import { ThemeProvider } from '@primer/react'
+import { DiffEntry, FileDiff, MessageIds } from '../../chrome/types'
+import { createPortal } from 'react-dom'
+import { Loading } from '../Loading'
+import { CadDiff } from './CadDiff'
+
+function CadDiffPortal({
+    element,
+    file,
+    owner,
+    repo,
+    sha,
+    parentSha,
+}: {
+    element: HTMLElement
+    file: DiffEntry
+    owner: string
+    repo: string
+    sha: string
+    parentSha: string
+}): React.ReactElement {
+    const [diff, setDiff] = useState<FileDiff>()
+    const [diffElement, setDiffElement] = useState<HTMLElement>()
+    useEffect(() => {
+        const diffElement = element.querySelector(
+            '.js-file-content'
+        ) as HTMLElement
+        setDiffElement(diffElement)
+        // TODO: don't clean up once the rich/source toggle is added
+        for (const e of diffElement.childNodes) {
+            e.remove()
+        }
+        ;(async () => {
+            const response = await chrome.runtime.sendMessage({
+                id: MessageIds.GetFileDiff,
+                data: { owner, repo, sha, parentSha, file },
+            })
+            if ('error' in response) {
+                console.log(response.error)
+            } else {
+                setDiff(response as FileDiff)
+            }
+        })()
+    }, [element, diffElement, file, owner, repo, sha, parentSha])
+    return (
+        <>
+            {diffElement &&
+                createPortal(
+                    diff ? (
+                        <CadDiff before={diff.before} after={diff.after} />
+                    ) : (
+                        <Loading />
+                    ),
+                    diffElement
+                )}
+        </>
+    )
+}
+
+export type CadDiffPageProps = {
+    map: { element: HTMLElement; file: DiffEntry }[]
+    owner: string
+    repo: string
+    sha: string
+    parentSha: string
+}
+
+export function CadDiffPage({
+    map,
+    owner,
+    repo,
+    sha,
+    parentSha,
+}: CadDiffPageProps): React.ReactElement {
+    return (
+        <ThemeProvider colorMode="auto">
+            {map.map(m => (
+                <CadDiffPortal
+                    key={m.file.filename}
+                    element={m.element}
+                    file={m.file}
+                    owner={owner}
+                    repo={repo}
+                    sha={sha}
+                    parentSha={parentSha}
+                />
+            ))}
+        </ThemeProvider>
+    )
+}