Skip to content
This repository was archived by the owner on Jan 22, 2019. It is now read-only.

fix: code intel on diffs with newly added files #294

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

attfarhan
Copy link

@attfarhan attfarhan commented Oct 31, 2018

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:

  • Chrome
  • Firefox
  • Safari
  • Phabricator Bundle

@sqs
Copy link
Member

sqs commented Nov 1, 2018

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
Copy link
Member

sqs commented Nov 1, 2018

I have been testing this with:

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

@attfarhan
Copy link
Author

attfarhan commented Nov 1, 2018

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

@attfarhan
Copy link
Author

attfarhan commented Nov 1, 2018

  • 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? sourcegraph/sourcegraph-extension-api@f7a6d85

@chrismwendt
Copy link

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants