feat: support diff code views

Passes the diff part to DOM functions

Author: ijsnow

src/hoverifier.ts

@@ -19,7 +19,7 @@ import { isDefined } from './helpers'
 import { overlayUIHasContent, scrollIntoCenterIfNeeded } from './helpers'
 import { HoverOverlayProps, isJumpURL } from './HoverOverlay'
 import { calculateOverlayPosition } from './overlay_position'
-import { PositionEvent, SupportedMouseEvent } from './positions'
+import { DiffPart, PositionEvent, SupportedMouseEvent } from './positions'
 import { createObservableStateContainer } from './state'
 import {
     convertNode,
@@ -104,7 +104,7 @@ export interface PositionJump {
     /**
      * The position within the code view to jump to
      */
-    position: LineOrPositionOrRange
+    position: LineOrPositionOrRange & { part?: DiffPart }
     /**
      * The code view
      */
@@ -306,15 +306,15 @@ export const createHoverifier = ({
     /** Emits DOM elements at new positions found in the URL */
     const jumpTargets = allPositionJumps.pipe(
         // Only use line and character for comparison
-        map(({ position: { line, character }, ...rest }) => ({ position: { line, character }, ...rest })),
+        map(({ position: { line, character, part }, ...rest }) => ({ position: { line, character, part }, ...rest })),
         // Ignore same values
         // It's important to do this before filtering otherwise navigating from
         // a position, to a line-only position, back to the first position would get ignored
         distinctUntilChanged((a, b) => isEqual(a, b)),
         // Ignore undefined or partial positions (e.g. line only)
         filter((jump): jump is typeof jump & { position: Position } => Position.is(jump.position)),
         map(({ position, codeView, dom, ...rest }) => {
-            const cell = dom.getCodeElementFromLineNumber(codeView, position.line)
+            const cell = dom.getCodeElementFromLineNumber(codeView, position.line, position.part)
             if (!cell) {
                 return undefined
             }
@@ -365,7 +365,7 @@ export const createHoverifier = ({
     const hoverObservables = resolvedPositions.pipe(
         map(({ position, ...rest }) => {
             if (!position) {
-                return of({ ...rest, hoverOrError: undefined })
+                return of({ ...rest, hoverOrError: undefined, part: undefined })
             }
             // Fetch the hover for that position
             const hoverFetch = fetchHover(position).pipe(
@@ -387,15 +387,15 @@ export const createHoverifier = ({
                     takeUntil(hoverFetch)
                 ),
                 hoverFetch
-            ).pipe(map(hoverOrError => ({ ...rest, hoverOrError })))
+            ).pipe(map(hoverOrError => ({ ...rest, hoverOrError, part: position.part })))
         }),
         share()
     )
     // Highlight the hover range returned by the language server
     subscription.add(
         hoverObservables
             .pipe(switchMap(hoverObservable => hoverObservable))
-            .subscribe(({ hoverOrError, codeView, dom }) => {
+            .subscribe(({ hoverOrError, codeView, dom, part }) => {
                 container.update({
                     hoverOrError,
                     // Reset the hover position, it's gonna be repositioned after the hover was rendered
@@ -410,7 +410,7 @@ export const createHoverifier = ({
                 }
                 // LSP is 0-indexed, the code in the webapp currently is 1-indexed
                 const { line, character } = hoverOrError.range.start
-                const token = getTokenAtPosition(codeView, { line: line + 1, character: character + 1 }, dom)
+                const token = getTokenAtPosition(codeView, { line: line + 1, character: character + 1 }, dom, part)
                 if (!token) {
                     return
                 }

src/positions.test.ts

@@ -10,7 +10,7 @@ import { propertyIsDefined } from './helpers'
 import { findPositionsFromEvents } from './positions'
 import { HoveredToken } from './token_position'
 
-describe('position_listener', () => {
+describe('positions', () => {
     const dom = new DOM()
     after(dom.cleanup)
 

src/positions.ts

@@ -1,7 +1,7 @@
 import { fromEvent, merge, Observable } from 'rxjs'
 import { filter, map, switchMap, tap } from 'rxjs/operators'
 import { Position } from 'vscode-languageserver-types'
-import { convertCodeElementIdempotent, DOMFunctions, HoveredToken, locateTarget } from './token_position'
+import { convertCodeElementIdempotent, DiffPart, DOMFunctions, HoveredToken, locateTarget } from './token_position'
 
 export type SupportedMouseEvent = 'click' | 'mousemove' | 'mouseover'
 
@@ -25,7 +25,7 @@ export interface PositionEvent {
     codeView: HTMLElement
 }
 
-export { DOMFunctions }
+export { DOMFunctions, DiffPart }
 export const findPositionsFromEvents = (options: DOMFunctions) => (
     elements: Observable<HTMLElement>
 ): Observable<PositionEvent> =>
@@ -67,7 +67,10 @@ export const findPositionsFromEvents = (options: DOMFunctions) => (
     ).pipe(
         // Find out the position that was hovered over
         map(({ target, codeView, ...rest }) => {
-            const hoveredToken = locateTarget(target, options)
+            const hoveredToken = locateTarget(target, {
+                ignoreFirstChar: !!options.getDiffCodePart,
+                ...options,
+            })
             const position = Position.is(hoveredToken) ? hoveredToken : undefined
             return { position, codeView, ...rest }
         })

src/testutils/dom.ts

@@ -44,18 +44,19 @@ export interface CodeViewProps extends DOMFunctions {
 
 // BEGIN setup test cases
 
-// Abstract implemetation for GitHub and Sourcegraph. Could potentially be sufficient for any code host
+// Abstract implementation for GitHub and Sourcegraph. Could potentially be sufficient for any code host
 // but we may want to keep this as a configuration point.
-const getDiffCodePart = (codeElement: HTMLElement): 'head' | 'base' | undefined => {
-    switch (codeElement.textContent!.charAt(0)) {
-        case '+':
-            return 'head'
-        case '-':
-            return 'base'
-        default:
-            return undefined
-    }
-}
+// Commented out cause we only have tests for non-diff code views so far
+// const getDiffCodePart = (codeElement: HTMLElement): DiffPart => {
+//     switch (codeElement.textContent!.charAt(0)) {
+//         case '+':
+//             return 'head'
+//         case '-':
+//             return 'base'
+//         default:
+//             return null
+//     }
+// }
 
 const createGitHubCodeView = (): CodeViewProps => {
     const codeView = document.createElement('div')
@@ -114,7 +115,6 @@ const createGitHubCodeView = (): CodeViewProps => {
         getCodeElementFromTarget,
         getCodeElementFromLineNumber,
         getLineNumberFromCodeElement,
-        getDiffCodePart,
     }
 }
 
@@ -176,7 +176,6 @@ const createSourcegraphCodeView = (): CodeViewProps => {
         getCodeElementFromTarget,
         getCodeElementFromLineNumber,
         getLineNumberFromCodeElement,
-        getDiffCodePart,
     }
 }
 

src/token_position.ts

@@ -14,25 +14,33 @@ export interface DOMFunctions {
      * @returns the element containing the code for a line or null if it can't be found. For example, the second <td> inside a <tr> on Sourcegraph and Github.
      */
     getCodeElementFromTarget: (target: HTMLElement) => HTMLElement | null
+
     /**
      * Get the element containing the code for a line from a code view given a line number.
+     *
      * @param codeView is the code view itself. For example, the <code> element on Sourcegraph or a <table> on GitHub.
+     * @param part If the code view is a diff view, the part of the diff that the line number refers to.
      * @returns the element containing the code for the given line number or null if it can't be found.
      */
-    getCodeElementFromLineNumber: (codeView: HTMLElement, line: number) => HTMLElement | null
+    getCodeElementFromLineNumber: (codeView: HTMLElement, line: number, part?: DiffPart) => HTMLElement | null
+
     /**
      * Gets the line number for a given element containing code for a line.
-     * @param codeElement is the element containing code for a line. When this function is called,
-     * it will be passed the result of either `getCodeElementFromTarget` or `getCodeElementFromLineNumber`.
-     * @returns the line number.
+     * When this function is called, it will be passed the result of either `getCodeElementFromTarget` or `getCodeElementFromLineNumber`.
+     * @param codeElement The element containing code for a line.
+     * @returns The line number.
      */
     getLineNumberFromCodeElement: (codeElement: HTMLElement) => number
+
     /**
-     * Determine whether a code element is from the base or head part of a diff or not part of a diff.
+     * If the code view is a diff view, must be provided to determine whether
+     * a code element is from the base, head or unchanged part of the diff.
+     * Must be `undefined` if the code view is not a diff view.
+     *
      * @param codeElement is the element containing a line of code.
-     * @returns whether the line is `'base'`, `'head'` or `undefined` if not part of a diff.
+     * @returns The part of the diff `codeElement` belongs to
      */
-    getDiffCodePart?: (codeElement: HTMLElement) => 'base' | 'head' | undefined
+    getDiffCodePart?: (codeElement: HTMLElement) => DiffPart
 }
 
 /**
@@ -199,6 +207,11 @@ export function findElementWithOffset(codeElement: HTMLElement, offset: number):
     return walkNode(codeElement)
 }
 
+/**
+ * Whether a line belongs to the base rev of the diff (removed), the head (added) or `null` if either (not changed).
+ */
+export type DiffPart = 'base' | 'head' | null
+
 /**
  * Returned when only the line is known.
  *
@@ -213,7 +226,7 @@ export interface HoveredToken {
     line: number
     /** 1-indexed */
     character: number
-    part?: 'base' | 'head'
+    part?: DiffPart
 }
 
 interface LocateTargetOptions extends DOMFunctions {
@@ -284,11 +297,13 @@ export function locateTarget(
 interface GetCodeElementsInRangeOptions extends Pick<DOMFunctions, 'getCodeElementFromLineNumber'> {
     codeView: HTMLElement
     position?: LineOrPositionOrRange
+    part?: DiffPart
 }
 
 export const getCodeElementsInRange = ({
     codeView,
     position,
+    part,
     getCodeElementFromLineNumber,
 }: GetCodeElementsInRangeOptions): {
     /** 1-indexed line number */
@@ -302,7 +317,7 @@ export const getCodeElementsInRange = ({
 
     const elements: { line: number; element: HTMLElement }[] = []
     for (let line = position.line; line <= (position.endLine || position.line); line++) {
-        const element = getCodeElementFromLineNumber(codeView, position.line)
+        const element = getCodeElementFromLineNumber(codeView, position.line, part)
         if (!element) {
             break
         }
@@ -316,13 +331,16 @@ export const getCodeElementsInRange = ({
  *
  * @param codeView The code view
  * @param position 1-indexed position
+ * @param options Code-host specific implementations of DOM retrieval functions
+ * @param part If the code view is a diff view, the part of the diff that the position refers to
  */
 export const getTokenAtPosition = (
     codeView: HTMLElement,
     position: Position,
-    options: DOMFunctions
+    options: DOMFunctions,
+    part?: DiffPart
 ): HTMLElement | undefined => {
-    const codeCell = options.getCodeElementFromLineNumber(codeView, position.line)
+    const codeCell = options.getCodeElementFromLineNumber(codeView, position.line, part)
     if (!codeCell) {
         return undefined
     }