Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Browser extension: code intelligence on diffs does not work #605

Closed
attfarhan opened this issue Oct 30, 2018 · 7 comments · Fixed by #846
Closed

Browser extension: code intelligence on diffs does not work #605

attfarhan opened this issue Oct 30, 2018 · 7 comments · Fixed by #846
Assignees
Labels
browser-extension bug An error, flaw or fault that produces an incorrect or unexpected result, or behavior.
Milestone

Comments

@attfarhan
Copy link
Contributor

  • Sourcegraph version: Browser ext 1.18.0
  • Platform information: macOS High Sierra, Chrome 70

Steps to reproduce:

  1. Go to https://github.com/sourcegraph/sourcegraph/pull/586/files
  2. Try to hover over anything. Notice no hovers work.
  3. Notice this error in the console:
    image

It appears there is an issue w this line now:
https://sourcegraph.com/github.com/sourcegraph/browser-extensions/-/blob/src/libs/github/dom_functions.ts#L207:22

Expected behavior:

Hovers work.

Actual behavior:

Hovers don't appear.

@attfarhan attfarhan added bug An error, flaw or fault that produces an incorrect or unexpected result, or behavior. browser-extension labels Oct 30, 2018
@attfarhan
Copy link
Contributor Author

cc @chrismwendt (looks like the line above was written by you)

@chrismwendt
Copy link
Contributor

I committed and @ijsnow authored it. I'm guessing the particular td element doesn't exist, so querySelector is returning null, but I'm not sure what could be causing that.

@ijsnow
Copy link
Contributor

ijsnow commented Oct 30, 2018

The DOM could have changed. I'm working on other high priority issues right now. This is probably a simple change. If I were to start debugging, I'd log the built selector and the parent we're looking for the link in then try to find something that looks like it.

@attfarhan
Copy link
Contributor Author

attfarhan commented Oct 31, 2018

This breaks diffs when there's a newly added file, since the blue border on the top is considered a row, but line will be null. https://github.com/sourcegraph/sourcegraph/pull/586/files#diff-f9e579844875cb46d3b49926faf8c740. PR open to fix: sourcegraph/browser-extensions#294

@sqs sqs added this to the October 2018 milestone Nov 1, 2018
@sqs
Copy link
Member

sqs commented Nov 1, 2018

@attfarhan When you saw this issue, did you have the Use Sourcegraph extensions feature flag enabled?

@attfarhan
Copy link
Contributor Author

@sqs yeah the feature flag is enabled when I run into this error, without it enabled it works

@ijsnow
Copy link
Contributor

ijsnow commented Nov 1, 2018

It's not directly an extensions bug though. It's a bug in GitHub's getLineRanges function for its CodeHost interface which is only required for extensions (because we have to send extensions the full file contents which we can only get from the DOM so we have to hackily scrape it :/ )

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
browser-extension bug An error, flaw or fault that produces an incorrect or unexpected result, or behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants