Trending nav item is repeating in header

[hkdobrev]

Steps to reproduce:

1. Go to https://github.com/sindresorhus/refined-github It's added twice
2. Click on a sub-repo navigation item - e.g. Issues tab.

This has been introduced relatively recently on master.

[fregante]

I cannot reproduce on master. This sounds strange because

• that function is called once (no domready or anything)
• the waiter is being a promise (which resolves once)
• elementReady only selects one element. so $().after() is not adding more of them at the same time.

Not sure how that's happening. Perhaps it's related to #450 and you have a custom domain manually added as a test for that. Try reinstalling it to reset that (toggle/reload doesn't work). Also make sure that it's being built and it's not an old version.

[hkdobrev]

It is indeed quite strange. The function is actually called twice and it's fixed when I remove both async and await. Would reinstall and report back.

[fregante]

It's it's called twice then the script is being injected twice, probably. Stick a console.log at the bottom of content.js; this might still be an issue for GHE (although at that point we can just wait for GHE users to report it)

[fregante]

Do you have multiple extensions loaded? 😃

[busches]

I was able to have this occur today too.

Running latest master, confirmed I do not have two extensions loaded. Additionally the Trending item is being added with the back button as well, so each time I click back, two new Trending items are added.

[fregante]

ahhhh I think the dynamic script injector's ping receiver got lost. Try this #492

[fregante]

New issue report here: #492 (comment)

[fregante]

I saw this issue once I re-enabled the AMO version, but then I reloaded the page and it stopped appearing (for now)

It might happen once.

[SimenB]

What's AMO? 😅

[fregante]

Mozilla Addons site 😃

[OmgImAlexis]

It's getting worse. Yesterday I had 20 of them. 😨

[fregante]

I'm having a hard time figuring this out. Surprisingly, chrome.permissions doesn't include github.com (because in manifest.json, github.com is only specified in content_scripts but not permissions), so the injector should be stopped pretty early and it should not run at all.

Can you inspect the background page and run this code in its console?

chrome.permissions.getAll(console.log);
chrome.tabs.onUpdated.addListener((tabId, ignore, {url}) => console.log(url));

Then open github.com and look back in the background page's console. It should look like this:

origins: Array(0) and then a lot of undefined

[fregante]

Are the Trending links consistently there? All the time every time?

If so, try this:

1. visit a repo
2. paste this in the console:

   chrome.runtime.onMessage.addListener(request => console.log(request, chrome.runtime.id));

3. visit the Issues tab

Nothing should appear in the console, the page should be loaded via ajax (i.e. the console should still contain what you pasted)

[OmgImAlexis]

The trending links seem to start off on 1 or 2 and then add another on the first time I load another section within the repo like the issues/PRs/Releases, etc. e.g. issues only gives 1 more but going to 3 repos and clicking issues each time adds 3.

[fregante]

I'll add some logging at every step and push to Chrome so we can get more details.

[busches]

An easy way to work around this is to add the same code we have for most other items we add to the dom and check for it's existence before adding it. Doesn't solve the root cause of why the extension is running twice, but if it's an odd edge case, it may be worth the time savings of solving it the quick and easy way, especially since we use that pattern for everything else.

[fregante]

I'd rather not do that because it'd push the real problem under the rug, deprioritizing the issue. You don't want an extension to run 10 times.

[paulmolluzzo]

I'd rather not do that because it'd push the real problem under the rug, deprioritizing the issue. You don't want an extension to run 10 times.

While that's fair, I think it's worth fixing the issue as best as we can immediately and then addressing the larger issue without this clear bug in the way.

I think @busches suggestion of checking for the existence of the element before adding it is a fine fix for now (we're doing it elsewhere already). Later when we have time to circle back on the underlying issue we can also remove the unnecessary DOM checks for this element and others when we have a better fix.

[SimenB]

+1 for quick fix, it's really annoying... Even though I get an itch by treating symptoms and not the underlying issues

[busches]

@bfred-it I'm not suggesting we ignore the issue all together, only that we put in the workaround as it's super annoying, as @paulmolluzzo and @SimenB mentioned.