Skip to content

Add support for Gerrit code views

Warren Gifford requested to merge gerrit-code-host-support into main

Created by: marekweb

This PR adds support for Gerrit as a code host in the browser extension.

I've listed a few challenges and considerations below.

Mutation observers

Update: this problem has been solved. Discussion below is kept for posterity.

Resolving code views in the page, which may appear or disappear during interaction, relies on a DOM mutation observers which fire events when DOM nodes are added or removed.

A big assumption in the ability to resolve code views is that we can observe mutations to the DOM. The challenge is that mutation observers attached to the DOM do not see mutations that happen inside of shadow DOMs.

If observing changes using a mutation observer doesn't work, then possible options are:

  • Polling the document, and using selectors to check for added nodes.
  • Finding out about removed nodes by checking if they're still attached to the document.
  • Because of shadow roots, we can't use plain string selectors. So the selectors need to be functions that access DOM properties like shadowRoot.

Using a shadow root observer library

https://allyjs.io/api/observe/shadow-mutations.html

I wasn't able to get this library to work and after some attempts, I set it aside because:

  • The code hasn't been updated in several years and it's not clear if it's maintained
  • There are no TS typings available

Implementing a polling version of observeMutations

I experimented with a DOM polling mechanism, which would poll for the existence of elements matching selectors. However, the current expectation of observeMutations is that if it emits added nodes, then these can be considered new code views, and when it emits removed nodes, then any code views contained within can be considered removed.

A polling mechanism would need to track which code views were already in the page, which may already be done elsewhere but I don't think that knowledge is available when mutations are being observed.

Re-injection of the content script

The background page triggers an injection of the content script on any tab that:

  • fires a "complete" event when the tab is loaded (which was assumed to fire once per tab)
  • matches an optional permission URL

On Gerrit, on some interactions (sometimes on every click?), the content page gets re-injected. It shouldn't have any side effects because the extra content script stops running once it detects the DOM marker.

I found that the reason for re-injecting is that in the background page, the "complete" event gets repeatedly fired from the Gerrit tab. This breaks the assumption that a tab can only fire "completed" once between reloads. It's not clear if there's a simple way to differentiate these spurious events from real page loads.

Selectors that can be custom functions

The original assumption was that we can detect the presence (or addition/removal) of a code view node when an element matches a selector string. This assumption breaks under the presence of shadow roots.

The solution I've implemented here is to allow a custom selector function to be provided, as an alternative to a selector string. The custom selector function then takes the responsibility of returning a list of elements, similar to how a string selector is passed to querySelectorAllOrSelf but it can use any custom DOM logic. That opens the door to querying shadow root DOMS.

Progress checklist

Important features

  • Code intelligence hovers (and other hovers) working in individual file diffs
  • Code intelligence hovers (and other hovers) working in change overview diffs
  • Open in Sourcegraph:
    • On the change overview page (open repository branch)
    • On the change overview page (on file diff)
    • On individual file diff pages
  • Extension line decorations
    • Git blame annotations are tested. Other decoration types untested.

Implementation work, fixes and other tasks

  • Implement custom selector functions for selecting code views
  • Use the custom selector functions in the Gerrit code view resolvers
  • Solve mutation observer problem properly
  • Check if getScrollBoundaries needs implementing
    • the answer appears to be no.
  • Use an appropriate fallback when parent commit cannot be found (on individual file pages)
    • The current solution seems to be working fine.
  • When switching between file diff pages, the code view is not recognized as being new.

Deferred or out-of-scope

  • Issue with content page being re-injected
  • Extension action buttons
  • When switching between unified and side-by-side diffs, line decorations are lost.
  • In expandable diffs in the change overview, the font style of the code intel hovers is slightly off.
  • Long line annotations (example: git blame) push the diff table width larger. In side-by-side diff, this introduces undesirable horizontal scrolling

Bugs

  • Fix bug where "go to definition" should go to Sourcegraph instead of staying on Gerrit

Merge request reports

Loading