fix(experiment-tag): defer history API url change handler to avoid mid-navigation DOM mutations - #318
Open
tyiuhc wants to merge 3 commits into
Open
fix(experiment-tag): defer history API url change handler to avoid mid-navigation DOM mutations#318tyiuhc wants to merge 3 commits into
tyiuhc wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Internal ticket reference in public source code comment
- Removed the internal Jira ticket reference (SKY-9617) from the comment while preserving the technical explanation of why setTimeout is used.
Or push these changes by commenting:
@cursor push 0f39ec6aef
Preview (0f39ec6aef)
diff --git a/packages/experiment-tag/src/subscriptions/subscriptions.ts b/packages/experiment-tag/src/subscriptions/subscriptions.ts
--- a/packages/experiment-tag/src/subscriptions/subscriptions.ts
+++ b/packages/experiment-tag/src/subscriptions/subscriptions.ts
@@ -860,7 +860,7 @@
// navigation cycle before we apply DOM mutations. Running synchronously
// inside pushState interleaves with framework rendering and can corrupt
// component bindings (e.g. Angular async pipe receiving non-Observable
- // values — SKY-9617).
+ // values).
setTimeout(handleUrlChange, 0);
return result;
};You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 560e10b. Configure here.
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
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.


Summary
The SDK wraps
history.pushStateandhistory.replaceStateto detect SPA navigations and re-evaluate experiments. Previously thehandleUrlChange()callback — which runsapplyVariants()and applies DOM mutations viadom-mutator— was called synchronously inside those wrappers.This meant DOM mutations fired mid-navigation, before the calling framework (e.g. Angular, Vue, or any microfrontend router) had finished its own navigation cycle. In Angular specifically, this interleaved with component initialization: mutations ran while bindings were still being set up, causing the
asyncpipe to receive a non-Observable value and throw:Fix: replace the synchronous call with
setTimeout(handleUrlChange, 0), deferring experiment re-evaluation to the next event loop task. By that point the framework's navigation cycle — guards, resolvers, component creation — has fully completed and DOM bindings are stable.Tradeoff: experiment mutations on SPA navigations apply ~1 event loop tick later than before. Anti-flicker on initial page load is unaffected (handled separately).
Checklist
Note
Medium Risk
Touches core SPA navigation instrumentation for all customers using the default handler; timing shift is small but could affect race-sensitive UIs, while fixing real framework breakage.
Overview
Defers SPA experiment re-application when navigation uses
history.pushStateorhistory.replaceState:handleUrlChange()(which publishesurl_changeand drives variant re-apply / DOM mutations) now runs viasetTimeout(..., 0)instead of synchronously inside the history wrappers.That lets the host router/framework finish its navigation/render pass before the SDK mutates the DOM, avoiding interleaved updates that broke Angular bindings (e.g.
asyncpipe errors).popstatehandling is unchanged (still immediate). Experiment updates on programmatic navigations may land one macrotask later; initial-load anti-flicker paths are outside this hook.Reviewed by Cursor Bugbot for commit af1a093. Bugbot is set up for automated code reviews on this repo. Configure here.