browser-extensions icon indicating copy to clipboard operation
browser-extensions copied to clipboard

fix: code intel on diffs with newly added files

Open attfarhan opened this issue 7 years ago • 5 comments
trafficstars

This PR changes:

Fixes https://github.com/sourcegraph/sourcegraph/issues/605

This blue border on newly added files in diffs counted as a row, and since it doesn't have any class names, it isn't filtered out in isCode. Hence, we can't assume that line is non-null.

image

Testing plan:

I've tested on this PR: https://github.com/sourcegraph/sourcegraph/pull/586/files. The browser extension would break once scrolling past licenseusercount.go. I confirmed this PR fixes the issue so code intel works.

I have tested on:

  • [x] Chrome
  • [ ] Firefox
  • [ ] Safari
  • [ ] Phabricator Bundle

attfarhan avatar Oct 31 '18 19:10 attfarhan

I still see a lot of glitchiness with extensions enabled on this PR:

  • Sometimes the hover will be duplicated (row of type signature, row of docs, row of type signature, row of docs)
  • Sometimes the hover won't show up at all (and there is no error)

I don't think this PR causes the first issue, but it may cause or exacerbate the 2nd one. This PR is likely a partial fix, but we can't be sure when there are so many other issues, so let's fix those first before merging this PR.

sqs avatar Nov 01 '18 16:11 sqs

I have been testing this with:

  • The Use Sourcegraph extensions browser ext feature flag enabled
  • and ONLY the https://sourcegraph.com/extensions/sqs/sourcegraph-hello-world extension enabled (which makes it easier to isolate problems in our DOM code vs. the extensions).

Strangely, I can't repro the original issue. All hovers work, above and below the added file.

sqs avatar Nov 01 '18 16:11 sqs

I followed your conditions above using browser ext version 2.0.0, and I can still repro the original issue.

attfarhan avatar Nov 01 '18 17:11 attfarhan

  • Sometimes the hover will be duplicated (row of type signature, row of docs, row of type signature, row of docs)
  • Sometimes the hover won't show up at all (and there is no error)

@sqs I can't repro either of these issues with this branch, can you point me to particular reproducible examples?

I don't think this PR causes the first issue, but it may cause or exacerbate the 2nd one. This PR is likely a partial fix, but we can't be sure when there are so many other issues, so let's fix those first before merging this PR.

I don't see how this PR would exacerbate the second issue. It simply checks adds a check to make sure that we are trying to add a hover to a line of code, and not one of the blue borders. I could see my recent commit in sourcegraph-extension-api exacerbating this, though, is that what you're referring to? https://github.com/sourcegraph/sourcegraph-extension-api/commit/f7a6d8541aca55a916b7050e9a0c8ed5b72696e9

attfarhan avatar Nov 01 '18 18:11 attfarhan

Here's how to move this PR to https://github.com/sourcegraph/sourcegraph/tree/master/packages/browser-extensions

cd browser-extensions
git format-patch master --stdout > /tmp/patch

cd ../sourcegraph
cat /tmp/patch | git am -3 --directory=packages/browser-extensions/
# and fixup merge conflicts

chrismwendt avatar Nov 05 '18 03:11 chrismwendt