fix: unsubscribe from actions and hover observable (#102)

Only take from the actions and hover Observables until the code view is unsubscribed. Fixes sourcegraph/sourcegraph#3108
sourcegraph · Apr 1, 2019 · 50263a6 · 50263a6
1 parent f0c6738
commit 50263a6
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 14 deletions.
diff --git a/README.md b/README.md
@@ -50,10 +50,12 @@ You may have to rebase a branch before merging to ensure it has a proper commit
 
 ## Glossary
 
-| Term                | Definition                                                                                                                                                                                  |
-| ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| Code view           | The DOM element that contains all the line elements                                                                                                                                         |
-| Line number element | The DOM element that contains the line number label for that line                                                                                                                           |
-| Code element        | The DOM element that contains the code for one line                                                                                                                                         |
-| Diff part           | The part of the diff, either base, head or both (if the line didn't change). Each line belongs to one diff part, and therefor to a different commit ID and potentially different file path. |
-| Hover overlay       | Also called tooltip                                                                                                                                                                         |
+| Term                | Definition                                                                                                                                                                                                    |
+| ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| Code view           | The DOM element that contains all the line elements                                                                                                                                                           |
+| Line number element | The DOM element that contains the line number label for that line                                                                                                                                             |
+| Code element        | The DOM element that contains the code for one line                                                                                                                                                           |
+| Diff part           | The part of the diff, either base, head or both (if the line didn't change). Each line belongs to one diff part, and therefor to a different commit ID and potentially different file path.                   |
+| Hover overlay       | Also called tooltip                                                                                                                                                                                           |
+| hoverify            | To attach all the listeners needed to a code view so that it will display overlay on hovers and clicks.                                                                                                       |
+| unhoverify          | To unsubscribe from the Subscription returned by `hoverifier.hoverify()`. Removes all event listeners from the code view again and hides the hover overlay if it was triggered by the unhoverified code view. |
diff --git a/src/hoverifier.test.ts b/src/hoverifier.test.ts
@@ -612,8 +612,9 @@ describe('Hoverifier', () => {
                     closeButtonClicks: NEVER,
                     hoverOverlayElements: of(null),
                     hoverOverlayRerenders: EMPTY,
-                    getHover: createStubHoverProvider(),
-                    getActions: () => of(null),
+                    // It's important that getHover() and getActions() emit something
+                    getHover: createStubHoverProvider({}),
+                    getActions: () => of([{}]).pipe(delay(50)),
                 })
                 const positionJumps = new Subject<PositionJump>()
                 const positionEvents = of(codeView.codeView).pipe(findPositionsFromEvents(codeView))

diff --git a/src/hoverifier.ts b/src/hoverifier.ts
@@ -355,6 +355,12 @@ export function createHoverifier<C extends object, D, A>({
 
     const allPositionJumps = new Subject<PositionJump & EventOptions<C>>()
 
+    /**
+     * Whenever a Subscription returned by `hoverify()` is unsubscribed,
+     * emits the code view ID associated with it.
+     */
+    const allUnhoverifies = new Subject<symbol>()
+
     const subscription = new Subscription()
 
     /**
@@ -572,9 +578,9 @@ export function createHoverifier<C extends object, D, A>({
             part?: DiffPart
         }>
     > = resolvedPositions.pipe(
-        map(({ position, ...rest }) => {
+        map(({ position, codeViewId, ...rest }) => {
             if (!position) {
-                return of({ hoverOrError: null, position: undefined, part: undefined, ...rest })
+                return of({ hoverOrError: null, position: undefined, part: undefined, codeViewId, ...rest })
             }
             // Get the hover for that position
             const hover = from(getHover(position)).pipe(
@@ -594,10 +600,13 @@ export function createHoverifier<C extends object, D, A>({
             ).pipe(
                 map(hoverOrError => ({
                     ...rest,
+                    codeViewId,
                     position,
                     hoverOrError,
                     part: position.part,
-                }))
+                })),
+                // Do not emit anything after the code view this action came from got unhoverified
+                takeUntil(allUnhoverifies.pipe(filter(unhoverifiedCodeViewId => unhoverifiedCodeViewId === codeViewId)))
             )
         }),
         share()
@@ -695,11 +704,14 @@ export function createHoverifier<C extends object, D, A>({
      */
     const actionObservables = resolvedPositions.pipe(
         // Get the actions for that position
-        map(({ position }) => {
+        map(({ position, codeViewId }) => {
             if (!position) {
                 return of(null)
             }
-            return concat([LOADING], from(getActions(position)).pipe(catchError(error => [asError(error)])))
+            return concat([LOADING], from(getActions(position)).pipe(catchError(error => [asError(error)]))).pipe(
+                // Do not emit anything after the code view this action came from got unhoverified
+                takeUntil(allUnhoverifies.pipe(filter(unhoverifiedCodeViewId => unhoverifiedCodeViewId === codeViewId)))
+            )
         }),
         share()
     )
@@ -819,6 +831,8 @@ export function createHoverifier<C extends object, D, A>({
                     .subscribe(allPositionJumps)
             )
             subscription.add(() => {
+                // Make sure hover is hidden and associated subscriptions unsubscribed
+                allUnhoverifies.next(codeViewId)
                 if (container.values.codeViewId === codeViewId) {
                     resetHover()
                 }