Detect React navigation and re-apply features#9830
Conversation
fregante
left a comment
There was a problem hiding this comment.
The last workaround that mentions is #6554. This might have to be fixed via svelte instead.
refined-github/source/features/default-branch-button.tsx
Lines 69 to 72 in adf982b
| include: [ | ||
| pageDetect.isConversation, | ||
| // Workaround for https://github.com/refined-github/refined-github/issues/6554 | ||
| pageDetect.isRepoIssueOrPRList, |
There was a problem hiding this comment.
I'll merge this PR after I verified these
| function init(signal: AbortSignal): void { | ||
| // TODO: Revert #9086 after https://github.com/refined-github/refined-github/issues/6554 | ||
| // Exclude reference to a comment on the same page | ||
| observe('.markdown-body:not(section[aria-label="Events"] *) a.issue-link:not([href*="#"])', underlineSelfReference, { |
There was a problem hiding this comment.
The previous code wasn't substantially better, so it's ok to leave this workaround and drop the comment.
fregante
left a comment
There was a problem hiding this comment.
All issues verified and fixed. Going ahead
| // Do not move to `includes` until React AJAX issues are resolved: | ||
| // https://github.com/refined-github/refined-github/pull/7524#issuecomment-2211692096 | ||
| // https://github.com/refined-github/refined-github/issues/6554 | ||
| if (new URLSearchParams(location.search).get('type') !== 'repositories') { |
There was a problem hiding this comment.
This is actually where "leave the listeners active" is a problem: the selector was too broad, so #7520 was resurfaced:
|
|
||
| function initSearch(signal: AbortSignal): void { | ||
| observe('.search-title a', addSearchConversationLinks, {signal}); | ||
| observe('a[class^="Repositories-module__stargazersLink"]', addSearchConversationLinks, {signal}); |
There was a problem hiding this comment.
So the fix was to make the selector more specific, which actually simplifies the DOM traversing in the add callback
I want to test-drive just this listener, we can then address any issues separately. React navigation is long overdue and this should cover most of the functionality.
I think we don't really need to unload features, with few exceptions, it's mostly a performance optimization. They're still unloaded when a regular pjax navigation happens. Our
delegateandobservecalls are natively deduplicated already, so it won't actually break anything ifinitis called twice in a row.Svelte components will eventually enable us to have features that auto-update.