1
0
mirror of https://github.com/bitwarden/browser synced 2026-02-22 04:14:04 +00:00

Address PR feedback: extract constants, add reset method, and real-timer test

- Extract magic numbers (500ms, 300ms) to named constants
  - shadowDomCheckTimeoutMs and shadowDomCheckDebounceMs
  - Improves maintainability and self-documentation

- Add resetObservedShadowRoots() method to DomQueryService
  - Clears WeakSet tracking when mutation observer is recreated
  - Called on navigation (handleWindowLocationMutation)
  - Prevents stale shadow root references

- Add real-timer test for debouncing behavior
  - Uses actual debounce implementation instead of mock
  - Verifies 5 rapid mutations result in only 1 page update
  - Validates debouncing works in production timing
This commit is contained in:
Miles Blackwood
2026-01-30 15:48:11 -05:00
parent c670735118
commit 73c37b6d97
4 changed files with 76 additions and 2 deletions

View File

@@ -9,4 +9,5 @@ export interface DomQueryService {
checkPageContainsShadowDom(): boolean;
checkMutationsInShadowRoots(mutations: MutationRecord[]): boolean;
checkForNewShadowRoots(): boolean;
resetObservedShadowRoots(): void;
}

View File

@@ -2361,6 +2361,66 @@ describe("CollectAutofillContentService", () => {
clearTimeout(initialTimeout);
jest.useRealTimers();
});
it("debounces multiple rapid shadow root mutations with real timers", (done) => {
jest.useRealTimers();
// Use real debounce for this test
const actualUtils = jest.requireActual("../utils");
const realDebounce = actualUtils.debounce;
const shadowHost = document.createElement("div");
const shadowRoot = shadowHost.attachShadow({ mode: "open" });
document.body.appendChild(shadowHost);
const mutationRecord: MutationRecord = {
type: "attributes",
addedNodes: document.querySelectorAll("nonexistent"),
attributeName: "value",
attributeNamespace: null,
nextSibling: null,
oldValue: null,
previousSibling: null,
removedNodes: document.querySelectorAll("nonexistent"),
target: shadowRoot,
};
collectAutofillContentService["currentLocationHref"] = window.location.href;
jest.spyOn(domQueryService, "checkMutationsInShadowRoots").mockReturnValue(true);
// Track actual calls to requirePageDetailsUpdate
let callCount = 0;
const originalRequirePageDetailsUpdate =
collectAutofillContentService["requirePageDetailsUpdate"];
collectAutofillContentService["requirePageDetailsUpdate"] = () => {
callCount++;
originalRequirePageDetailsUpdate.call(collectAutofillContentService);
};
// Temporarily override with real debounce
const originalDebounced = collectAutofillContentService["debouncedRequirePageDetailsUpdate"];
collectAutofillContentService["debouncedRequirePageDetailsUpdate"] = realDebounce(() => {
collectAutofillContentService["requirePageDetailsUpdate"]();
}, 300);
// Trigger 5 rapid mutations
for (let i = 0; i < 5; i++) {
collectAutofillContentService["handleMutationObserverMutation"]([mutationRecord]);
}
// Should only call requirePageDetailsUpdate once after debounce
setTimeout(() => {
expect(callCount).toBe(1);
// Restore original
collectAutofillContentService["debouncedRequirePageDetailsUpdate"] = originalDebounced;
collectAutofillContentService["requirePageDetailsUpdate"] =
originalRequirePageDetailsUpdate;
document.body.removeChild(shadowHost);
done();
}, 350);
});
});
describe("setupOverlayListenersOnMutatedElements", () => {

View File

@@ -55,6 +55,8 @@ export class CollectAutofillContentService implements CollectAutofillContentServ
private pendingShadowDomCheck = false;
private ownedExperienceTagNames: string[] = [];
private readonly updateAfterMutationTimeout = 1000;
private readonly shadowDomCheckTimeoutMs = 500;
private readonly shadowDomCheckDebounceMs = 300;
private readonly formFieldQueryString;
private readonly nonInputFormFieldTags = new Set(["textarea", "select"]);
private readonly ignoredInputTypes = new Set([
@@ -1003,7 +1005,7 @@ export class CollectAutofillContentService implements CollectAutofillContentServ
this.shadowDomCheckTimeout = setTimeout(() => {
this.checkForNewShadowRoots();
this.pendingShadowDomCheck = false;
}, 500);
}, this.shadowDomCheckTimeoutMs);
}
if (!this.mutationsQueue.length) {
@@ -1031,6 +1033,9 @@ export class CollectAutofillContentService implements CollectAutofillContentServ
this._autofillFormElements.clear();
this.autofillFieldElements.clear();
// Reset shadow root tracking on navigation
this.domQueryService.resetObservedShadowRoots();
this.updateAutofillElementsAfterMutation();
}
@@ -1074,7 +1079,7 @@ export class CollectAutofillContentService implements CollectAutofillContentServ
*/
private debouncedRequirePageDetailsUpdate = debounce(() => {
this.requirePageDetailsUpdate();
}, 300);
}, this.shadowDomCheckDebounceMs);
/**
* Checks for new shadow roots that aren't being observed and triggers

View File

@@ -115,6 +115,14 @@ export class DomQueryService implements DomQueryServiceInterface {
return false;
};
/**
* Resets the observed shadow roots tracking. This should be called when the mutation
* observer is recreated or on significant lifecycle events (like navigation).
*/
resetObservedShadowRoots = (): void => {
this.observedShadowRoots = new WeakSet<ShadowRoot>();
};
/**
* Initializes the DomQueryService, checking for the presence of shadow DOM elements on the page.
*/