Change browser extension to use a single MutationObserver #2909

felixfbecker · 2019-03-22T01:21:42Z

Refactors browser extension to use a single MutationObserver whose events are passed as an Observable to functions, instead of only injecting things once and never removing them.
Code views are then detected inside added and removed nodes. When a node with a code view got removed, its state gets properly cleaned up.

It fixes a bunch of bugs/wrong assumptions I found - for example, switchMap was used on the codeViews Observable which could race to randomly drop code views, and the selections and view components code assumed only the latest code view emitted was relevant. Hoverify was never unsubscribed from.

Also does a few renames that made things less confusing for me, e.g. CodeView -> CodeViewSpec, CodeView.codeView -> CodeView.codeViewElement

Also removes a hacky MutationObserver workaround in favor of the real fix sourcegraph/codeintellify#100

I've not touched unit tests yet. The functions that take MutationRecords can now be unit tested.
Also not touched other code hosts than GitHub yet.

Fixes #2162
Fixes https://github.com/sourcegraph/sourcegraph/issues/2958
Fixes #3103
Fixes #955

Tested on

lguychard

Awesome start 👍

Also not touched other code hosts than GitHub yet.

It seems like this change would be code host-agnostic. What do you have in mind?

The functions that take MutationRecords can now be unit tested.

🎉

client/browser/src/extension/scripts/inject.tsx

client/browser/src/libs/code_intelligence/code_views.ts

lguychard · 2019-03-22T09:52:20Z

client/browser/src/libs/code_intelligence/code_intelligence.tsx

-                            ? null
-                            : originalDOM.getCodeElementFromTarget(target),
-                }
+        selectionsChanges.subscribe(selections => {


This doesn't look right, it assumes that all visible view components have the same selections.

But wasn't that the behaviour before too? The selections are a property of the code host, so I don't see a way to distinguish which selections belong to which code view

The behaviour wasn't exactly the same before - because we only considered the latest code view as relevant, when selections changed they were only updated on the latest code view.

This explicitly sets the same selections on all code views, which is knowingly wrong and I'd like us to avoid that. PTAL https://github.com/sourcegraph/sourcegraph/pull/2937 which offers a solution.

But only considering the "latest" code view was not correct, since there really isn't such a thing as the "latest" code view (the order in which code views are emitted is random as it depends on MutationObserver and the API requests to fetch contents). I think the previous code would still have the previous selections on the non-"latest" code view so selections did not just apply to the latest

lguychard · 2019-03-22T09:59:00Z

client/browser/src/libs/code_intelligence/code_intelligence.tsx

+            // TODO optimize
+            // Maybe getMount() would have to be replaced by a function that determines if a new mount was added from a given mutation record
+
+            // injectCommandPalette({


What happened to command palette injection?

I just commented out things I hadn't vetted for idempotency yet

vetted for idempotency

IMO, idempotency should be provided by the implementor of CodeHost, not the user. This will allow for more logic to stay out of this file. It also seems like the correct place since the code host knows what the DOM looks like but this file should be ignorant of anything code host specific.

@ijsnow could you clarify what you mean by that?

lguychard · 2019-03-22T10:20:16Z

client/browser/src/libs/code_intelligence/code_views.ts

    )
+// .pipe(emitWhenIntersecting(250))


What's your thinking on doing away with emitWithIntersecting()? I assume it was put in place for performance reasons when we used to scrape document contents from the DOM, but now that we're fetching the file contents instead, we're probably OK without.

I just commented it out to exclude it as a possible source of why code views would not be detected properly. So far I've not noticed a perf problem. Maybe @ijsnow can comment on the reasoning behind it

Emitting only when code views are visible or about to be visible was ported over from the old way of handling code hosts. It was added by Kingy in that implementation. It was indeed for performance problems. While the need for it may be reduced, I don't think it would hurt to keep it in.

lguychard · 2019-03-22T10:26:23Z

client/browser/src/libs/code_intelligence/code_intelligence.tsx

@@ -156,16 +157,18 @@ export interface CodeHost {
     */
    getOverlayMount?: () => HTMLElement | null

+    // Code views and code view resolver form a union


I know this is probably a "note to self", but it's probably worth being reworded & moved to the docstrings of codeViewSpecs and codeViewSpecResolver:

The set of code views tracked on a page is the union of all code views found using codeViewSpecs and codeViewResolver (or similar)

I am dissatisfied by the redundancy of codeViewSpecs and codeViewSpecResolver. It causes redundant code to handle both, but every codeViewSpec could be expressed by a codeViewResolver. However, multiple codeViewSpecs are allowed, but only one codeViewSpecResolver. I would like to unify them somehow. We could also think about the usefullness of specifying a selector vs simply having (container: Element) => CodeViewWithoutSelector[]

We could also think about the usefullness of specifying a selector vs simply having (container: Element) => CodeViewWithoutSelector[]

Why would the resolve function return an array?

@ijsnow that would not be a resolve function, it would just return all the code views including their elements given a container.

client/browser/src/libs/code_intelligence/code_views.ts

client/browser/src/libs/code_intelligence/code_intelligence.tsx

Having selections as a property of `CodeHost` is a problem, because [it is incorrect to assume that all code views on the page have the same selections](https://github.com/sourcegraph/sourcegraph/pull/2909#discussion_r268261523). This adds `getSelections` and `observeSelections as optional properties of `CodeViewSpec`, and changes the code to make sure that when selections change, we don't update _all visible view components_ with the same spec of selection. This assumes that only adding `getSelections()` and `observeSelections()` to single file code views is "good enough", which is still an imperfect assumption, but at least avoids having code that explicitly sets the same selections on a bunch of code views.

felixfbecker · 2019-03-22T20:06:02Z

It seems like this change would be code host-agnostic. What do you have in mind?

There is a compile error in client/browser/src/libs/phabricator/extension.tsx since injectCodeIntelligence() is called but now needs the mutations Observable. I am not sure why this error is only in the Phab code. Maybe this is dead code?

ijsnow · 2019-03-25T18:37:55Z

client/browser/src/libs/code_intelligence/code_intelligence.tsx

@@ -156,16 +157,18 @@ export interface CodeHost {
     */
    getOverlayMount?: () => HTMLElement | null

+    // Code views and code view resolver form a union


We could also think about the usefullness of specifying a selector vs simply having (container: Element) => CodeViewWithoutSelector[]

Why would the resolve function return an array?

ijsnow · 2019-03-25T18:40:42Z

client/browser/src/libs/code_intelligence/code_intelligence.tsx

+            // TODO optimize
+            // Maybe getMount() would have to be replaced by a function that determines if a new mount was added from a given mutation record
+
+            // injectCommandPalette({


vetted for idempotency

IMO, idempotency should be provided by the implementor of CodeHost, not the user. This will allow for more logic to stay out of this file. It also seems like the correct place since the code host knows what the DOM looks like but this file should be ignorant of anything code host specific.

ijsnow · 2019-03-25T18:49:24Z

client/browser/src/libs/code_intelligence/code_views.ts

    )
+// .pipe(emitWhenIntersecting(250))


Emitting only when code views are visible or about to be visible was ported over from the old way of handling code hosts. It was added by Kingy in that implementation. It was indeed for performance problems. While the need for it may be reduced, I don't think it would hurt to keep it in.

ijsnow · 2019-03-25T18:57:15Z

client/browser/src/libs/phabricator/extension.tsx

 async function injectModules(): Promise<void> {
+    // This is added so that the browser extension doesn't
+    // interfere with the native Phabricator integration.
+    // TODO is this racy?


It is not intended to be racy. The logic is that the browser extension code runs after all other code is parsed/ran in the document. The native integration runs via a script tag added to the document by Phabricator. However, I just realized that we don't account for async code. I believe that it generally works but we should add the global class name that we look for to an element in the function that loads the phabricator bundle.

codecov · 2019-04-01T15:50:08Z

Codecov Report

Merging #2909 into master will decrease coverage by 3.69%.
The diff coverage is 61.4%.

Impacted Files	Coverage Δ
...wser/src/libs/code_intelligence/external_links.tsx	`97.22% <ø> (-0.28%)`	⬇️
client/browser/src/libs/phabricator/util.tsx	`7.56% <ø> (-0.64%)`	⬇️
.../browser/src/shared/components/CodeViewToolbar.tsx	`27.02% <0%> (ø)`	⬆️
...lient/browser/src/libs/github/code_intelligence.ts	`16.45% <0%> (+0.17%)`	⬆️
client/browser/src/libs/github/inject.tsx	`17.46% <0%> (ø)`	⬆️
...t/browser/src/libs/bitbucket/code_intelligence.tsx	`17.54% <100%> (ø)`	⬆️
.../browser/src/libs/phabricator/code_intelligence.ts	`20.83% <100%> (ø)`	⬆️
...lient/browser/src/libs/gitlab/code_intelligence.ts	`33.33% <100%> (ø)`	⬆️
client/browser/src/shared/util/dom.tsx	`27.77% <44.44%> (+2.77%)`	⬆️
...r/src/libs/code_intelligence/code_intelligence.tsx	`58.62% <65%> (+0.99%)`	⬆️
... and 92 more

)" This reverts commit 9475593.

Refactor browser extension to use a single MutationObserver

0318894

felixfbecker requested review from lguychard and ijsnow March 22, 2019 01:21

lguychard reviewed Mar 22, 2019

View reviewed changes

lguychard mentioned this pull request Mar 22, 2019

3.3 Integrations quality tracking issue #2834

Closed

23 tasks

lguychard reviewed Mar 22, 2019

View reviewed changes

client/browser/src/libs/code_intelligence/code_views.ts Outdated Show resolved Hide resolved

lguychard reviewed Mar 22, 2019

View reviewed changes

client/browser/src/libs/code_intelligence/code_intelligence.tsx Outdated Show resolved Hide resolved

lguychard reviewed Mar 22, 2019

View reviewed changes

client/browser/src/libs/code_intelligence/code_intelligence.tsx Show resolved Hide resolved

lguychard mentioned this pull request Mar 22, 2019

Make selections specific to a code view #2937

Merged

felixfbecker added 4 commits March 22, 2019 14:49

Fix test compile errors

005d0bc

Fix type error in Phabricator integration

0b00a07

Remove unused vars

249985d

Add note

bede95f

felixfbecker mentioned this pull request Mar 25, 2019

1st file in GitHub PR does not have Sourcegraph actions/hovers initially #2958

Closed

felixfbecker added 3 commits March 25, 2019 17:25

Remove emitWhenIntersecting()

83cc264

Uncomment command palette injection

8a51593

Move duplicate code to querySelectorAllOrSelf() function

dcb4213

ijsnow reviewed Mar 25, 2019

View reviewed changes

felixfbecker added 4 commits March 26, 2019 21:35

Merge branch 'master' into single-mutation-observer

7e154fb

Do not call resolver for removed code views

168c80b

Add tests for code_views.ts

16544f9

Ignore test files from coverage

9df97a0

felixfbecker marked this pull request as ready for review March 27, 2019 01:13

felixfbecker mentioned this pull request Mar 29, 2019

Occasional error when hovering tokens on Bitbucket Server: "parameter 1 is not of type 'Node'" #3038

Closed

Merge branch 'master' into single-mutation-observer

6a91cce

felixfbecker mentioned this pull request Mar 30, 2019

Code views unreliably detected on large PRs #2893

Closed

felixfbecker added the browser-extension label Mar 30, 2019

felixfbecker mentioned this pull request Mar 31, 2019

Browser extension j2d fails in some cases on a GitHub PR #1098

Closed

felixfbecker changed the title ~~Refactor browser extension to use a single MutationObserver~~ Change browser extension to use a single MutationObserver Mar 31, 2019

felixfbecker added this to the 3.4 milestone Mar 31, 2019

lguychard mentioned this pull request Apr 1, 2019

GitHub code intelligence doesn't work after soft page load; works after hard page load #750

Closed

felixfbecker added 3 commits April 1, 2019 17:15

Merge branch 'master' into single-mutation-observer

f1d8121

Docs and minor improvements

fd928f8

Consistent naming

dc871f5

felixfbecker mentioned this pull request Apr 1, 2019

Browser extension not working on Phabricator #3103

Closed

felixfbecker added 2 commits April 1, 2019 17:50

Remove logs

a6d3298

Remove log

1d8a142

felixfbecker mentioned this pull request Apr 1, 2019

No codeintel in browser extension, Uncaught Error: File actions not found. Make sure you aren't trying to create a toolbar mount for a code snippet that shouldn't have one #955

Closed

felixfbecker merged commit 9475593 into master Apr 1, 2019

This was referenced Apr 2, 2019

TypeError: [(...O.values(...))].flatMap is not a function #3159

Closed

Sourcegraph browser extension slowing down Firefox #3166

Closed

lguychard mentioned this pull request Apr 2, 2019

Revert mutation observer refactor #3167

Closed

lguychard pushed a commit that referenced this pull request Apr 2, 2019

Revert "Change browser extension to use a single MutationObserver (#2909

3f22f11

)" This reverts commit 9475593.

felixfbecker mentioned this pull request Apr 3, 2019

Use mutation events for injections mounts too #3191

Merged

5 tasks

lguychard mentioned this pull request May 14, 2019

Simplify ViewResolver type #3976

Closed

nicksnyder deleted the single-mutation-observer branch September 30, 2019 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change browser extension to use a single MutationObserver #2909

Change browser extension to use a single MutationObserver #2909

felixfbecker commented Mar 22, 2019 •

edited

Loading

lguychard left a comment

lguychard Mar 22, 2019

felixfbecker Mar 22, 2019

lguychard Mar 22, 2019

felixfbecker Mar 22, 2019

lguychard Mar 22, 2019

felixfbecker Mar 22, 2019

ijsnow Mar 25, 2019

felixfbecker Mar 31, 2019

lguychard Mar 22, 2019

felixfbecker Mar 22, 2019

ijsnow Mar 25, 2019

lguychard Mar 22, 2019

felixfbecker Mar 22, 2019

ijsnow Mar 25, 2019

felixfbecker Mar 26, 2019

felixfbecker commented Mar 22, 2019

ijsnow Mar 25, 2019

ijsnow Mar 25, 2019

ijsnow Mar 25, 2019

ijsnow Mar 25, 2019

codecov bot commented Apr 1, 2019 •

edited

Loading

Change browser extension to use a single MutationObserver #2909

Change browser extension to use a single MutationObserver #2909

Conversation

felixfbecker commented Mar 22, 2019 • edited Loading

lguychard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfbecker commented Mar 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 1, 2019 • edited Loading

Codecov Report

felixfbecker commented Mar 22, 2019 •

edited

Loading

codecov bot commented Apr 1, 2019 •

edited

Loading