Github plugin mangles unstaged changes/diff buffer

[mraleson]

Thanks in advance for your bug report!

• Have you reproduced issue in safe mode?
• Have you used the debugging guide to try to resolve the issue?
• Have you checked our FAQs to make sure your question isn't answered there?
• Have you checked to make sure your issue does not already exist?
• Have you checked you are on the latest release of Pulsar?

What happened?

The git diff is mangled when toggling between buffers and clicking on the unstaged changes for a file on the right side git bar.

Pulsar version

1.112.1

Which OS does this happen on?

🐧 Debian based (Linux Mint, Ubuntu, etc.)

OS details

Ubuntu 22.04

Which CPU architecture are you running this on?

x86_64/AMD64

What steps are needed to reproduce this?

1. Create a new git repo with a single file
2. Make a commit so you can see git diff changes
3. Modify the file with about 10 new lines
4. Open unstaged changes diff for file Pulsar github plugin toolbar on the right
5. Toggle between buffers (open file and gif diff)
6. Open unstaged changes diff for file Pulsar github plugin toolbar on the right (again)
7. Toggle between buffers (open file and gif diff)
8. git diff should be mangled

Additional Information:

You may have to click back and forth a few times, it essentially looks like line numbers and changes are not shifted down correctly. I feel like it is easier to reproduce when the diff is larger (it happens every time). This does not happen on Atom, so it may be a regression from recent changes.

[confused-Techie]

Thanks a ton for reporting this issue and contributing!

I have a feeling this may have to do with our recent changes to dugite, @DeeDeeG maybe we should create some tests around this?

[savetheclocktower]

I have noticed this but haven't had the bandwidth to dig into it. Glad it's being tracked now.

[confused-Techie]

@savetheclocktower Did you happen to notice this prior to our recent upgrades to this package?

[savetheclocktower]

If I had to guess, I would say yes, but I don't really remember.

[DeeDeeG]

I have a feeling this may have to do with our recent changes to dugite, @DeeDeeG maybe we should create some tests around this?

We do at least run the tests now at github repo, it's just that several are failing. They may be legitimate failures in some obscure features/code paths that don't totally break the package. So, I suppose we should endeavor to finally actually fix those. For all I know this could already be covered in some tests? (FWIW some tests were failing in atom/github at the time we forked, so not all of it is due to our changes. They may still be legitimate issues, such as with the GitHub.com GraphQL API changing maybe?)

[ discussion regarding whether this is a new breakage or an old problem ]

Would love to know whether this is new or not, to know where we should look to solve it. If I can reproduce the reported issue then I can look into it further, but much more explicit and step-by-step "for dummies" steps to reproduce would aid in debugging this issue further and determining which Pulsar versions are affected, which github package versions are affected, etc.

Clearer steps to reproduce would be much appreciated!

[savetheclocktower]

Several things:

1. I have been able to reproduce this issue by

• having two panes open: one to any document and one to a staged changes view; and
• rapidly switching between them via keyboard until the issue appears.

2. Why is this package so complicated (I mean it is really complicated)
3. Amazingly, no code in the github package seems to be the culprit — it's inside of text-editor-component.js in the main Pulsar repo
4. I found one “fix” after a very long while: wrapping this line with a if (this.isVisible()) check — but I have no idea if that's the best fix
5. If you reproduce the display glitch, it will fix itself if you resize the pane or the window

Here's what I think is happening:

• When we switch from one tab to another, one element is hidden and another is shown.
• If a text editor is being shown — and the diff view does wrap around a text editor — then TextEditorComponent#updateSync gets called and hence so does TextEditorComponent#measureBlockDecorations.
• Block “decorations” are elements that go around various portions of the buffer but are not part of the buffer. (Aside from this diff view, the only other kind that I can think of are the fancy controls that go around sections of files when you're fixing merge conflicts.)
• In some scenarios — can't pin down exactly why, but it definitely feels like a race condition — TextEditorComponent#updateSync is called before its actual element has been re-rendered during the switch-active-tab lifecycle. (We can detect this by calling TextEditorComponent#isVisible, which will just check clientHeight and clientWidth and return false if they're both equal to 0.)
• updateSync envisions that it might get called when the element is not being shown — because it checks isVisible in a different code path. But…
• It's definitely wrong to call measureBlockDecorations if the editor isn't visible. The logic inside of it only makes sense if the editor is visible. This is happening (at least in the scenario where I can reproduce this problem) because this method is measuring elements that it thinks are visible but actually aren't, and then memoizing their height values. So when it does become visible mere instants later, it's reusing a bunch of bad measurements.

The good news is that it appears to be safe to skip calling measureBlockDecorations in this scenario. In all my experiments, this code path is run again when the element actually becomes visible.

The bad news is that I don't have any experience with this code and so I can't be 100% certain that this change won't break something. But I think it's the sort of thing that some of us should try locally and see if anything blows up.

It's annoying that the fix for this doesn't actually involve this repo, and it's also annoying that I can't explain why this started happening in the first place.

If anyone else had the bandwidth for this and wanted to dig into it, they might discover an underlying race condition that explains all of this much more elegantly. Or they might not! No way to tell.

[mraleson]

Thank you very much for looking into it. In terms of how long the bug has been around, @DeeDeeG . I think it was present I first started using Pulsar which was maybe 6 months ago?

It was hard for me write duplication instructions because in my real projects it happens every time. If I click diff, then view another tab, then go back to view the diff its mangled.

However, when I went to create a small example for this bug report I noticed it didn't always happen with the smaller example, which makes me think like @savetheclocktower that it is a race condition.

I cloned the add-on and will try to look into it, however I am having some issues running the add-on in development mode due to some build issues / missing imports. I'll try to figure that out, I'm not familiar at all with the project.

[savetheclocktower]

I cloned the add-on and will try to look into it, however I am having some issues running the add-on in development mode due to some build issues / missing imports. I'll try to figure that out, I'm not familiar at all with the project.

Here's the quick approach:

git checkout https://github.com/pulsar-edit/github.git
cd github
npm install
ppm link --dev .

Then launch your other project with

pulsar --dev .

The problem, though, is that if I'm right, the proposed fix isn't within the add-on at all. And the story for running the whole editor from source files is a bit more complicated. But if I were to make a PR with the proposed change, it would generate a binary that you could use to determine whether it fixes your issue, so we might give that a try.

[DeeDeeG]

As a wild guess, perhaps the Electron update we did from 11 to 12 very early on got us newer Chromium and V8 which might behave a bit differently as to layout timing, async JS timing, behavior of non-visible content, or what-have-you.

Or maybe the different version of babel we use to transpile the github package into plain JS produces slightly differently optimized code, and maybe the timing of how it executes is subtly different now? Building the core repo on a GitHub Actions tech stack instead of Azure DevOps??

But this sounds promising, IMO:

I found one “fix” after a very long while: wrapping this line with a if (this.isVisible()) check — but I have no idea if that's the best fix

EDIT to add: Seems like they were really trying not to do this stuff if "we're not visible": atom/atom@6e6dce2. So it must indeed be an issue of not catching that we're invisible at the exact right moment (have to concur seems like a race condition).

[savetheclocktower]

But this sounds promising, IMO:

OK, I'll make that change in time for 1.113. Whether or not it's the root cause, it makes sense in context and probably prevents other bugs as well.