fix(remote-popup): stop the refresh loop from re-triggering itself - #169
Open
divya0795 wants to merge 1 commit into
Open
fix(remote-popup): stop the refresh loop from re-triggering itself#169divya0795 wants to merge 1 commit into
divya0795 wants to merge 1 commit into
Conversation
updateLinks assigned anchor.textContent unconditionally. Assigning textContent always removes the existing child text node and inserts a new one, so it emits a childList mutation record even when the string is identical. The script observes childList/subtree on document.documentElement, so that write fed straight back into its own observer: refresh -> updateLinks writes textContent -> childList record -> scheduleRefresh -> setTimeout(0) -> refresh -> ... Once any DOM change kicked it off, the loop sustained itself for as long as a remote-tooltip anchor was in the document, re-running querySelectorAll sweeps and text-node churn one pass per macrotask and never quiescing. Compare the href write, which is safe because attributes are not observed, and updateQrCodes, which already guards with "canvas.dataset.wandRemoteUrl === remoteUrl". Apply the same idea: compare before writing, so a link that already holds the right value is left untouched. Added bridge/src/remote-popup-cleanup.test.ts, which kicks the loop with one external mutation and then asserts the document stops changing. Against the unfixed script it records 8 mutations over 8 macrotasks -- one per pass, confirming the loop is self-sustaining -- and 0 with the fix. The suite needs DOM globals, which bridge/tsconfig.json omits because it targets the Node-side bridge, so the test file pulls the DOM lib in locally with a reference directive rather than widening the project config.
divya0795
force-pushed
the
fix/remote-popup-refresh-loop
branch
from
July 26, 2026 14:32
19a10fc to
9a6c2b8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #168
Problem
updateLinksassignedanchor.textContentunconditionally. AssigningtextContentalways removes the existing child text node and inserts a new one, so it emits achildListrecord even when the string is unchanged.The script observes
childList/subtreeondocument.documentElement, so that write fed straight back into its own observer:refreshScheduledis reset beforerefresh()runs, so each pass schedules the next. Once any DOM change kicked the loop off, it sustained itself for as long as aremote-tooltipanchor was present.Change
Compare before writing — the same idea
updateQrCodesalready uses at:116(canvas.dataset.wandRemoteUrl === remoteUrl). Thehrefwrite is guarded too for symmetry, though it was never a contributor sinceattributesis not observed.No behavioural change to what the link ends up showing; only the redundant rewrites go away.
Test
New
bridge/src/remote-popup-cleanup.test.ts. The script's own initialrefresh()runs before it starts observing, so the loop needs one external mutation to start — the test supplies exactly oneappendChild, then counts mutations over 8 macrotasks.Verified to fail against the unfixed script:
One mutation per macrotask — the loop is self-sustaining. With the fix, 0.
bridge/tsconfig.jsondeliberately omits the DOM lib (it targets the Node-side bridge,lib: ["ES2022"],types: ["node"]). Rather than widen that config for one test, the file pulls DOM types in locally with/// <reference lib="dom" />.Verification
Node 22.23.1 / pnpm 10.17.0, matching the versions
.github/workflows/build.ymlpins:pnpm test— 35 passed (13 files), up from 34 onmasterpnpm lint— clean at--max-warnings=0pnpm typecheck— clean (typecheck:webandtypecheck:bridge)I do not have a licensed WeMod install, so this has not been exercised against a live Wand renderer — the evidence is the jsdom reproduction above.