Repository navigation
Fix observe styles piling up on React navigations - #10312
jakeonrails wants to merge 3 commits into
Conversation
Since refined-github#9830, features re-run on `soft-nav:react-done`, but React navigations don't fire the Turbo events that call `unloadAll`. The previous run was never aborted, so every navigation added another set of `observe` styles and `animationstart` listeners. Fixes refined-github#10311
Without the signal, the observer's style was never removed, so every run of a feature that waits for a merge left another one behind. Refs refined-github#10311
|
To add some context the real bug this fixes is that typing into code comments on PRs was getting painfully slow. I would type, and characters wouldn't appear for multiple seconds. I am now running this fork of the extension and so far I have not experienced the slowdown while typing code comments. This had been happening regularly for about a week (since I noticed) and now it is not happening. |
|
Thanks for your report and PR! I merged #10318 to address this via stylesheet deduplication. I'm still interested in this PR, which solves the issue higher up in the chain. I am only concerned that aborting the signal this way may cause unwanted side effects when navigating back and forth specifically on React pages (components/event listeners might not work anymore) Have you been using Refined GitHub from this PR? Tangentially related: |
That file has a lot of global state, as can be seen by the numerous mocks. We don't add tests that have to mock everything. |
Yes I have and I have not noticed any issues, but I'm not a heavy refined Github user so I'm not sure that I would notice anything not working. Or maybe let's put it another way: I have had this extension installed for so long I don't even remember what stock Github looks like so I would just think "Oh, Github is broken", and I haven't noticed that. I'm happy to keep working off my patched PR and report back in a week or so to give me time to notice any issues. |
Would you like me to remove that spec? Happy to do so. |
|
I've been using the PR version of the extension for over two weeks now (since this PR was opened) and it's been running great with no issues. |
Closes #10311
Two leaks made the
observe<style>elements pile up in a long-lived tab, slowing every style recalculation (and so typing in comment boxes on large PRs).soft-nav:react-done, but those navigations don't fire the Turbo events that callunloadAll, so the previous run'sAbortControllerwas never aborted. Each feature's loop now aborts its own previous run when it wakes up, before any check that could skip the new run. On Turbo navigations the extra abort is a no-op.waitForPrMergecalledobserve()without its signal, so its style was never removed, regardless of navigation type.Added a
feature-managertest that firessoft-nav:react-donethree times and checks that only the latest run stays active and only one observer<style>exists. It fails without the fix.Test URLs
https://github.com/crmne/spotifast/pull/961/changes
Switch Conversation ⇄ Files changed 10 times, then count:
<style>count, fresh → after 10 round tripsScreenshot
Dev build after 10 round trips (the label is a temporary overlay injected to show the live counts):