fix: ignore stale observer map array notifications

[pradeep-ramola]

Pull Request

📖 Description

Fixes stale observer-map array notifications after arrays are replaced.

Previously, observer-map tracked observed arrays with a single WeakSet, so the first owner/root context for an array could keep receiving notifications even after that array was no longer reachable from the current data. This changes array observation to use owner-specific registrations and guards notifications by checking whether the changed array is still reachable from the registered owner’s current root property.

This is a fix and is not a breaking change.

🎫 Issues

Fixes #7579

👩‍💻 Reviewer Notes

The main area to review is the new array registration/reachability logic in observer-map-utilities.ts.

A useful smoke test is the observer-map deep-merge fixture. It covers stale root arrays, stale nested arrays, direct assignment replacement, proxy assignment replacement, and shared arrays where only the owner that still references the array should be notified.

📑 Test Plan

• npm run build:tsc -w @microsoft/fast-element
• npx biome check packages/fast-element/src/declarative/observer-map-utilities.ts packages/fast-element/test/declarative/fixtures/extensions/observer-map-deep-merge/main.ts packages/fast-element/test/declarative/fixtures/extensions/observer-map-deep-merge/observer-map-deep-merge.spec.ts change/@microsoft-fast-element-ab7cde20-cff1-47a9-8e95-5895dd961321.json
• npx beachball check --verbose
• npx playwright test --config=packages/fast-element/playwright.declarative.config.ts --project=chromium fixtures/extensions/observer-map-deep-merge/observer-map-deep-merge.spec.ts

✅ Checklist

General

• I have included a change request file using $ npm run change
• I have added tests for my changes.
• I have tested my changes.
• I have updated the project documentation to reflect my changes.
• I have read the CONTRIBUTING documentation and followed the standards for this project.

Agents

• I have linked to an existing issue in this project that this change addresses
• I have read the skills
• I have read the DESIGN.md file(s) in packages relevent to my changes
• I have updated the DESIGN.md file(s) in packages relevent to my changes

⏭ Next Steps

None.

[janechu]

Thanks for the contribution @pradeep-ramola! I've added a few comments but overall this looks like a good fix.

[janechu]

registrations are never cleaned up, a subscriber may have an old notifier attached, can you ensure these get cleaned up?

[janechu]

This test does not actually fail on the issue, consider this change:

public async mutateStaleNestedItemsAfterSecondDeepMerge() {
    deepMerge(this.users[0], {
        orders: [this.createOrder(901, "First replacement")],
    });
    await Updates.next();

    const oldItems = this.users[0].orders[0].items;

    deepMerge(this.users[0], {
        orders: [this.createOrder(902, "Second replacement")],
    });
    await Updates.next();

    let notifications = 0;
    const notifier = Observable.getNotifier(this);
    const subscriber = {
        handleChange() {
            notifications++;
        },
    };

    notifier.subscribe(subscriber, "users");

    oldItems.push(this.createProduct(9000, "Stale item"));
    await Updates.next();

    notifier.unsubscribe(subscriber, "users");

    return {
        notifications,
        currentItemCount: this.users[0].orders[0].items.length,
        staleItemCount: oldItems.length,
    };
}

Pair this with the following test in the spec file:

test("should ignore stale nested array mutations after replacing arrays with deepMerge", async ({
    page,
}) => {
    const hydrationCompleted = page.waitForFunction(
        () => (window as any).hydrationCompleted === true,
    );
    await page.goto("/fixtures/extensions/observer-map-deep-merge/");
    await hydrationCompleted;

    const result = await page
        .locator("deep-merge-test-element")
        .evaluate((element: any) =>
            element.mutateStaleNestedItemsAfterSecondDeepMerge(),
        );

    expect(result.notifications).toBe(0);
    expect(result.currentItemCount).toBe(1);
    expect(result.staleItemCount).toBe(2);
});

This pattern fails without the fix because  oldItems.push(...)  incorrectly notifies the element’s users root property after the second replacement.

[pradeep-ramola]

Thanks, updated in 8937b49.

• stale array registrations are now removed and their array notifier subscriber is unsubscribed
• the nested stale-array regression now uses the second deepMerge repro case from your comment

The focused Playwright fixture test, tsc build, Biome check, and Beachball check all pass locally.

[janechu]

Thanks for updating this. The stale-registration cleanup and the revised nested stale test address my earlier concerns.

I found one remaining edge case worth covering/fixing: shared primitive-array aliases within the same owner/root can lose notifications. Primitive arrays are wrapped in separate proxies but registered by the same raw array/context, and observeArray() updates the existing registration's reachableArray. If two current paths share the same raw primitive array, then one path is replaced, mutating the still-current other proxy can be treated as stale and skip Observable.notify().

Concrete scenario I reproduced against this branch:

const sharedTags = ["shared"];
const itemA = element.users[0].orders[0].items[0];
const itemB = element.users[0].orders[0].items[1];

itemA.tags = sharedTags;
itemB.tags = sharedTags;
await Updates.next();

itemB.tags = ["replacement"];
await Updates.next();

itemA.tags.push("still owned by A");
await Updates.next();

itemA.tags remains current and includes the new value, but the root users notification count is 0 after the push. I think the fix is to track all reachable primitive-array wrappers/aliases for a registration, or otherwise distinguish registrations by wrapper/path enough that replacing one alias does not invalidate another still-reachable alias.

[pradeep-ramola]

Thanks for updating this. The stale-registration cleanup and the revised nested stale test address my earlier concerns.

I found one remaining edge case worth covering/fixing: shared primitive-array aliases within the same owner/root can lose notifications. Primitive arrays are wrapped in separate proxies but registered by the same raw array/context, and observeArray() updates the existing registration's reachableArray. If two current paths share the same raw primitive array, then one path is replaced, mutating the still-current other proxy can be treated as stale and skip Observable.notify().

Concrete scenario I reproduced against this branch:

const sharedTags = ["shared"];
const itemA = element.users[0].orders[0].items[0];
const itemB = element.users[0].orders[0].items[1];

itemA.tags = sharedTags;
itemB.tags = sharedTags;
await Updates.next();

itemB.tags = ["replacement"];
await Updates.next();

itemA.tags.push("still owned by A");
await Updates.next();

itemA.tags remains current and includes the new value, but the root users notification count is 0 after the push. I think the fix is to track all reachable primitive-array wrappers/aliases for a registration, or otherwise distinguish registrations by wrapper/path enough that replacing one alias does not invalidate another still-reachable alias.

Thanks, updated in d9db2f5.

This now tracks all reachable primitive-array aliases and notifies the owning root when a nested shared alias is still current after another alias is replaced. I also added the regression case from your comment for shared tags arrays.