Rerun Octo-Linker when blobs are expanded #505
Conversation
leomoty
changed the title
|
@stefanbuck I noticed the same |
|
running
Octolinker is not enabled at all for that page? It works for chrome tho. It seems unrelated to this PR. |
packages/core/octo-linker.js
Outdated
| observer.observe(document.body, { | ||
| childList: true, | ||
| subtree: true, | ||
| }); |
There was a problem hiding this comment.
Not sure if this is really an issue, but I think we should narrow down this observe a little bit to avoid potential side effects / performance issues.
for (const el of document.querySelectorAll('.diff-table tbody')) {
observerElement(el);
}I think, by observing diff-table tbody you can also get rid of subtree: true since the mutation happens on that element.
There was a problem hiding this comment.
True, I wanted to get an early review, didn't think about the performance implications
There was a problem hiding this comment.
Good idea to get feedback early one 👍
e2e/automated.test.js
Outdated
|
|
||
| await page.waitForSelector(selector); | ||
|
|
||
| await Promise.all([ |
There was a problem hiding this comment.
I think it's enough to check if OctoLinker insert new links into the dom without actually clicking them. Therefore I would suggest to remove everything between line 87-92.
There was a problem hiding this comment.
Right, i'll update it.
|
Yeah, I noticed that same that e2e tests related to diff view are a bit flaky. I will disable them until I found the root cause. |
|
Regarding your issue with |
|
I can't realistically make If you want I can open an issue regarding those. |
|
Ohhh, haven't seen |
|
I think I was able to fix the e2e test issue by using eslint rather than wepack #506. |
|
@stefanbuck they might lower the barrier a little bit, I cant state the usefulness of |
leomoty
changed the title
|
@stefanbuck squashed & force pushed with the changes. |
packages/core/octo-linker.js
Outdated
| const observer = new MutationObserver(mutations => { | ||
| mutations.forEach(mutation => { | ||
| const nodes = Array.from(mutation.addedNodes); | ||
| if ( |
There was a problem hiding this comment.
Are you sure that this condition is still needed? I think run can be invoked anytime when a mutation is triggered (assume only one mutation is happing when clicking the expand button).
There was a problem hiding this comment.
I'll confirm, missed that when you talked about restricting the mutation target.
|
If you could open an issue for the |
|
@stefanbuck Updated again, also yes, just running |
|
Sorry, took me a bit longer than usually to release a new version. However it's on the way and thanks again for adding this cool feature |
|
Hey @stefanbuck, no problem, had some water damage at my house, i'll take a look at #510 this week. |
|
Oh no what a nightmare. #510 is not that critical so no need to rushing it. Good luck with fix your water issue. |
|
It is fixed by now, no hardware died. |
This is a possible implementation for #453, we observe the DOM, if any new
.blob-expandednodes are added, then the extension is rerun.As far as testing goes, I will wait for feedback, I added a single test that clickes the specific
.js-expandelement and then tries to process a single line from it.Checklist: