fix(devtools): prevent rename race on unmount and correct state cleanup #213

apps/demo/src/app/devtools/todo-store.ts

@@ -30,7 +30,14 @@ export const TodoStore = signalStore(
       },
 
       remove(id: number) {
-        updateState(store, 'remove todo', removeEntity(id));
+        updateState(
+          store,
+          'remove todo',
+          removeEntity(id),
+          ({ selectedIds }) => ({
+            selectedIds: selectedIds.filter((selectedId) => selectedId !== id),
+          }),
+        );
       },
 
       toggleFinished(id: number): void {

libs/ngrx-toolkit/src/lib/devtools/internal/devtools-syncer.service.ts

@@ -1,7 +1,6 @@
 import { isPlatformBrowser } from '@angular/common';
 import { inject, Injectable, OnDestroy, PLATFORM_ID } from '@angular/core';
 import { StateSource } from '@ngrx/signals';
-import { throwIfNull } from '../../shared/throw-if-null';
 import { REDUX_DEVTOOLS_CONFIG } from '../provide-devtools-config';
 import { currentActionNames } from './current-action-names';
 import { DevtoolsInnerOptions } from './devtools-feature';
@@ -150,7 +149,7 @@ Enable automatic indexing via withDevTools('${storeName}', { indexNames: true })
     this.#currentState = Object.entries(this.#currentState).reduce(
       (newState, [storeName, state]) => {
         if (storeName !== name) {
-          newState[name] = state;
+          newState[storeName] = state;
         }
         return newState;
       },
@@ -162,23 +161,36 @@ Enable automatic indexing via withDevTools('${storeName}', { indexNames: true })
     }
   }
 
-  renameStore(oldName: string, newName: string) {
-    const storeNames = Object.values(this.#stores).map((store) => store.name);
-    const id = throwIfNull(
-      Object.keys(this.#stores).find((id) => this.#stores[id].name === oldName),
-    );
-    if (storeNames.includes(newName)) {
+  /**
+   * Renames a store identified by its internal id. If the store has already
+   * been removed (e.g. due to component destruction), this is a no-op.
+   */
+  renameStore(id: string, newName: string) {
+    const storeEntry = this.#stores[id];
+    if (!storeEntry) {
+      return;
+    }
+    const oldName = storeEntry.name;
+
+    if (oldName === newName) {
+      return;
+    }
+
+    const otherStoreNames = Object.entries(this.#stores)
+      .filter(([entryId]) => entryId !== id)
+      .map(([, s]) => s.name);
+    if (otherStoreNames.includes(newName)) {
       throw new Error(
         `NgRx Toolkit/DevTools: cannot rename from ${oldName} to ${newName}. ${newName} is already assigned to another SignalStore instance.`,
       );
     }
 
     this.#stores = Object.entries(this.#stores).reduce(
-      (newStore, [id, value]) => {
-        if (value.name === oldName) {
-          newStore[id] = { ...value, name: newName };
+      (newStore, [entryId, value]) => {
+        if (entryId === id) {
+          newStore[entryId] = { ...value, name: newName };
         } else {
-          newStore[id] = value;
+          newStore[entryId] = value;
         }
         return newStore;
       },

libs/ngrx-toolkit/src/lib/devtools/tests/naming.spec.ts

@@ -28,7 +28,7 @@ describe('withDevtools / renaming', () => {
     TestBed.inject(Store);
     runInInjectionContext(childContext, () => inject(Store));
 
-    TestBed.flushEffects();
+    TestBed.tick();
 
     expect(sendSpy).toHaveBeenLastCalledWith(
       { type: 'Store Update' },
@@ -51,7 +51,7 @@ describe('withDevtools / renaming', () => {
     const childContext2 = createEnvironmentInjector([Store], envInjector);
 
     runInInjectionContext(childContext1, () => inject(Store));
-    TestBed.flushEffects();
+    TestBed.tick();
     childContext1.destroy();
 
     expect(sendSpy.mock.calls).toEqual([
@@ -64,7 +64,7 @@ describe('withDevtools / renaming', () => {
     ]);
 
     runInInjectionContext(childContext2, () => inject(Store));
-    TestBed.flushEffects();
+    TestBed.tick();
     expect(sendSpy.mock.calls).toEqual([
       [
         { type: 'Store Update' },
@@ -113,7 +113,7 @@ Enable automatic indexing via withDevTools('flights', { indexNames: true }), or
       signalStore({ providedIn: 'root' }, withDevtools('flights')),
     );
 
-    TestBed.flushEffects();
+    TestBed.tick();
     expect(sendSpy.mock.calls).toEqual([
       [
         { type: 'Store Update' },
@@ -169,7 +169,7 @@ Enable automatic indexing via withDevTools('flights', { indexNames: true }), or
 
       const store = TestBed.inject(Store);
       renameDevtoolsName(store, 'flights');
-      TestBed.flushEffects();
+      TestBed.tick();
 
       expect(sendSpy).toHaveBeenCalledWith(
         { type: 'Store Update' },
@@ -192,7 +192,7 @@ Enable automatic indexing via withDevTools('flights', { indexNames: true }), or
       );
       TestBed.inject(Store1);
       const store = TestBed.inject(Store2);
-      TestBed.flushEffects();
+      TestBed.tick();
 
       expect(() => renameDevtoolsName(store, 'shop')).toThrow(
         'NgRx Toolkit/DevTools: cannot rename from mall to shop. shop is already assigned to another SignalStore instance.',
@@ -212,5 +212,33 @@ Enable automatic indexing via withDevTools('flights', { indexNames: true }), or
         "Devtools extensions haven't been added to this store.",
       );
     });
+
+    it('should ignore rename after the store has been destroyed', () => {
+      const { sendSpy } = setupExtensions();
+
+      const Store = signalStore(
+        withDevtools('flight'),
+        withState({ name: 'Product', price: 10.5 }),
+      );
+
+      const childContext = createEnvironmentInjector(
+        [Store],
+        TestBed.inject(EnvironmentInjector),
+      );
+
+      const store = childContext.get(Store);
+      TestBed.tick();
+
+      expect(sendSpy).toHaveBeenCalledWith(
+        { type: 'Store Update' },
+        { flight: { name: 'Product', price: 10.5 } },
+      );
+
+      childContext.destroy();
+      TestBed.tick();
+
+      // Previously this could throw; now it is a no-op
+      expect(() => renameDevtoolsName(store, 'flights')).not.toThrow();
+    });
   });
 });

libs/ngrx-toolkit/src/lib/devtools/with-devtools.ts

@@ -51,7 +51,7 @@ export function withDevtools(name: string, ...features: DevtoolsFeature[]) {
       // TODO: use withProps and symbols
       return {
         [renameDevtoolsMethodName]: (newName: string) => {
-          syncer.renameStore(name, newName);
+          syncer.renameStore(id, newName);
         },
         [uniqueDevtoolsId]: () => id,
       } as Record<string, (newName?: unknown) => unknown>;