From d0cd74ace79355bef1b35d1bf18be816c15237db Mon Sep 17 00:00:00 2001
From: Sheik Althaf <althafmech@outlook.com>
Date: Fri, 22 Nov 2024 14:19:00 +0530
Subject: [PATCH] refactor(devtools): use signals for template properties in
 frame manager (#58818)

convert the frames and selectedFrame properties to signal so that it can react to changes on OnPush

PR Close #58818
---
 .../devtools-tabs.component.html              |  4 +-
 .../devtools-tabs/devtools-tabs.component.ts  |  2 +-
 .../lib/devtools-tabs/devtools-tabs.spec.ts   |  2 +-
 .../directive-explorer.component.ts           |  6 +-
 .../ng-devtools/src/lib/frame_manager.ts      | 67 ++++++++++---------
 .../ng-devtools/src/lib/frame_manager_spec.ts | 51 +++++++-------
 6 files changed, 71 insertions(+), 61 deletions(-)

diff --git a/devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.component.html b/devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.component.html
index 5625475524e..cb7f46929dc 100644
--- a/devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.component.html
+++ b/devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.component.html
@@ -24,7 +24,7 @@
     class="frame-selector"
     (change)="emitSelectedFrame($event.target.value)"
   >
-    @for (frame of frameManager.frames; track frame.id) {
+    @for (frame of frameManager.frames(); track frame.id) {
       <option [value]="frame.id" [selected]="frameManager.isSelectedFrame(frame)">
         @if (frame.id === TOP_LEVEL_FRAME_ID) {
           top
@@ -69,7 +69,7 @@
 </nav>
 
 <mat-tab-nav-panel #tabPanel>
-  @if (!applicationEnvironment.frameSelectorEnabled || frameManager.selectedFrame !== null) {
+  @if (!applicationEnvironment.frameSelectorEnabled || frameManager.selectedFrame() !== null) {
     <div class="tab-content">
       <ng-directive-explorer
         [showCommentNodes]="showCommentNodes()"
diff --git a/devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.component.ts b/devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.component.ts
index dbd48decdcd..7a014a2542c 100644
--- a/devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.component.ts
+++ b/devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.component.ts
@@ -101,7 +101,7 @@ export class DevToolsTabsComponent {
   }
 
   emitSelectedFrame(frameId: string): void {
-    const frame = this.frameManager.frames.find((frame) => frame.id === parseInt(frameId, 10));
+    const frame = this.frameManager.frames().find((frame) => frame.id === parseInt(frameId, 10));
     this.frameSelected.emit(frame!);
   }
 
diff --git a/devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.spec.ts b/devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.spec.ts
index 9f9ba76cb49..8c981ab8991 100644
--- a/devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.spec.ts
+++ b/devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.spec.ts
@@ -94,6 +94,6 @@ describe('DevtoolsTabsComponent', () => {
     spyOn(comp.frameSelected, 'emit');
     comp.emitSelectedFrame('1');
 
-    expect(comp.frameSelected.emit).toHaveBeenCalledWith(comp.frameManager.frames[0]);
+    expect(comp.frameSelected.emit).toHaveBeenCalledWith(comp.frameManager.frames()[0]);
   });
 });
diff --git a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.component.ts b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.component.ts
index f38990d0728..3f0e07979a7 100644
--- a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.component.ts
+++ b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.component.ts
@@ -191,7 +191,7 @@ export class DirectiveExplorerComponent {
       (directive) => directive.name === directiveName,
     );
 
-    const selectedFrame = this._frameManager.selectedFrame;
+    const selectedFrame = this._frameManager.selectedFrame();
     if (!this._frameManager.frameHasUniqueUrl(selectedFrame)) {
       this._messageBus.emit('log', [
         {
@@ -210,7 +210,7 @@ export class DirectiveExplorerComponent {
   }
 
   handleSelectDomElement(node: IndexedNode): void {
-    const selectedFrame = this._frameManager.selectedFrame;
+    const selectedFrame = this._frameManager.selectedFrame();
     if (!this._frameManager.frameHasUniqueUrl(selectedFrame)) {
       this._messageBus.emit('log', [
         {
@@ -290,7 +290,7 @@ export class DirectiveExplorerComponent {
   }): void {
     const objectPath = constructPathOfKeysToPropertyValue(node.prop);
 
-    const selectedFrame = this._frameManager.selectedFrame;
+    const selectedFrame = this._frameManager.selectedFrame();
     if (!this._frameManager.frameHasUniqueUrl(selectedFrame)) {
       this._messageBus.emit('log', [
         {
diff --git a/devtools/projects/ng-devtools/src/lib/frame_manager.ts b/devtools/projects/ng-devtools/src/lib/frame_manager.ts
index 60c27f4326f..1825a0c7447 100644
--- a/devtools/projects/ng-devtools/src/lib/frame_manager.ts
+++ b/devtools/projects/ng-devtools/src/lib/frame_manager.ts
@@ -6,30 +6,29 @@
  * found in the LICENSE file at https://angular.dev/license
  */
 
-import {Injectable, inject} from '@angular/core';
+import {Injectable, inject, signal, computed} from '@angular/core';
 import {Events, MessageBus} from 'protocol';
 
 import {Frame, TOP_LEVEL_FRAME_ID} from './application-environment';
 
 @Injectable()
 export class FrameManager {
-  private _selectedFrameId: number | null = null;
-  private _frames = new Map<number, Frame>();
+  private _selectedFrameId = signal<number | null>(null);
+  private _frames = signal(new Map<number, Frame>());
   private _inspectedWindowTabId: number | null = null;
   private _frameUrlToFrameIds = new Map<string, Set<number>>();
   private _messageBus = inject<MessageBus<Events>>(MessageBus);
 
-  get frames(): Frame[] {
-    return Array.from(this._frames.values());
-  }
+  readonly frames = computed(() => Array.from(this._frames().values()));
 
-  get selectedFrame(): Frame | null {
-    if (this._selectedFrameId === null) {
+  readonly selectedFrame = computed(() => {
+    const selectedFrameId = this._selectedFrameId();
+    if (selectedFrameId === null) {
       return null;
     }
 
-    return this._frames.get(this._selectedFrameId) ?? null;
-  }
+    return this._frames().get(selectedFrameId) ?? null;
+  });
 
   static initialize(inspectedWindowTabIdTestOnly?: number | null) {
     const manager = new FrameManager();
@@ -45,8 +44,8 @@ export class FrameManager {
     }
 
     this._messageBus.on('frameConnected', (frameId: number) => {
-      if (this._frames.has(frameId)) {
-        this._selectedFrameId = frameId;
+      if (this._frames().has(frameId)) {
+        this._selectedFrameId.set(frameId);
       }
     });
 
@@ -58,36 +57,38 @@ export class FrameManager {
 
       this.addFrame({name, id: frameId, url: urlWithoutHash});
 
-      if (this.frames.length === 1) {
-        this.inspectFrame(this._frames.get(frameId)!);
+      if (this.frames().length === 1) {
+        this.inspectFrame(this._frames().get(frameId)!);
       }
     });
 
     this._messageBus.on('contentScriptDisconnected', (frameId: number) => {
-      if (!this._frames.has(frameId)) {
+      const frame = this._frames().get(frameId);
+      if (!frame) {
         return;
       }
 
-      this.removeFrame(this._frames.get(frameId)!);
+      this.removeFrame(frame);
 
       // Defensive check. This case should never happen, since we're always connected to at least
       // the top level frame.
-      if (this.frames.length === 0) {
-        this._selectedFrameId = null;
+      if (this.frames().length === 0) {
+        this._selectedFrameId.set(null);
         console.error('Angular DevTools is not connected to any frames.');
         return;
       }
 
-      if (frameId === this._selectedFrameId) {
-        this._selectedFrameId = TOP_LEVEL_FRAME_ID;
-        this.inspectFrame(this._frames.get(this._selectedFrameId!)!);
+      const selectedFrameId = this._selectedFrameId();
+      if (frameId === selectedFrameId) {
+        this._selectedFrameId.set(TOP_LEVEL_FRAME_ID);
+        this.inspectFrame(this._frames().get(TOP_LEVEL_FRAME_ID)!);
         return;
       }
     });
   }
 
   isSelectedFrame(frame: Frame): boolean {
-    return this._selectedFrameId === frame.id;
+    return this._selectedFrameId() === frame.id;
   }
 
   inspectFrame(frame: Frame): void {
@@ -95,11 +96,11 @@ export class FrameManager {
       return;
     }
 
-    if (!this._frames.has(frame.id)) {
+    if (!this._frames().has(frame.id)) {
       throw new Error('Attempted to inspect a frame that is not connected to Angular DevTools.');
     }
 
-    this._selectedFrameId = null;
+    this._selectedFrameId.set(null);
     this._messageBus.emit('enableFrameConnection', [frame.id, this._inspectedWindowTabId]);
   }
 
@@ -113,11 +114,14 @@ export class FrameManager {
   }
 
   private addFrame(frame: Frame): void {
-    this._frames.set(frame.id, frame);
-    const frameUrl = frame.url.toString();
-    const frameIdSet = this._frameUrlToFrameIds.get(frameUrl) ?? new Set<number>();
-    frameIdSet.add(frame.id);
-    this._frameUrlToFrameIds.set(frameUrl, frameIdSet);
+    this._frames.update((frames) => {
+      frames.set(frame.id, frame);
+      const frameUrl = frame.url.toString();
+      const frameIdSet = this._frameUrlToFrameIds.get(frameUrl) ?? new Set<number>();
+      frameIdSet.add(frame.id);
+      this._frameUrlToFrameIds.set(frameUrl, frameIdSet);
+      return new Map(frames);
+    });
   }
 
   private removeFrame(frame: Frame): void {
@@ -128,6 +132,9 @@ export class FrameManager {
     if (urlFrameIds.size === 0) {
       this._frameUrlToFrameIds.delete(frameUrl);
     }
-    this._frames.delete(frameId);
+    this._frames.update((frames) => {
+      frames.delete(frameId);
+      return new Map(frames);
+    });
   }
 }
diff --git a/devtools/projects/ng-devtools/src/lib/frame_manager_spec.ts b/devtools/projects/ng-devtools/src/lib/frame_manager_spec.ts
index 3dd70123efd..9d983a4d75c 100644
--- a/devtools/projects/ng-devtools/src/lib/frame_manager_spec.ts
+++ b/devtools/projects/ng-devtools/src/lib/frame_manager_spec.ts
@@ -17,7 +17,7 @@ describe('FrameManager', () => {
   let topicToCallback: {[topic: string]: Function | null};
 
   function getFrameFromFrameManager(frameId: number): Frame | undefined {
-    return frameManager.frames.find((f: Frame) => f.id === frameId);
+    return frameManager.frames().find((f: Frame) => f.id === frameId);
   }
 
   function frameConnected(frameId: number): void {
@@ -66,35 +66,36 @@ describe('FrameManager', () => {
 
   it('should add frame when contentScriptConnected event is emitted', () => {
     contentScriptConnected(topLevelFrameId, 'name', 'http://localhost:4200/url');
-    expect(frameManager.frames.length).toBe(1);
-    expect(frameManager.frames[0].id).toBe(topLevelFrameId);
-    expect(frameManager.frames[0].name).toBe('name');
-    expect(frameManager.frames[0].url.toString()).toBe('http://localhost:4200/url');
+    const frames = frameManager.frames();
+    expect(frames.length).toBe(1);
+    expect(frames[0].id).toBe(topLevelFrameId);
+    expect(frames[0].name).toBe('name');
+    expect(frames[0].url.toString()).toBe('http://localhost:4200/url');
   });
 
   it('should set the selected frame to the first frame when there is only one frame', () => {
     contentScriptConnected(topLevelFrameId, 'name', 'http://localhost:4200/url');
-    expect(frameManager.selectedFrame?.id).toBe(topLevelFrameId);
+    expect(frameManager.selectedFrame()?.id).toBe(topLevelFrameId);
   });
 
   it('should set selected frame when frameConnected event is emitted', () => {
     contentScriptConnected(topLevelFrameId, 'name', 'http://localhost:4200/url');
     contentScriptConnected(otherFrameId, 'name2', 'http://localhost:4200/url2');
     frameConnected(otherFrameId);
-    expect(frameManager.selectedFrame?.id).toBe(otherFrameId);
+    expect(frameManager.selectedFrame()?.id).toBe(otherFrameId);
   });
 
   it('should remove frame when contentScriptDisconnected event is emitted', () => {
     contentScriptConnected(topLevelFrameId, 'name', 'http://localhost:4200/url');
     contentScriptConnected(otherFrameId, 'name2', 'http://localhost:4200/url2');
-    expect(frameManager.frames.length).toBe(2);
+    expect(frameManager.frames().length).toBe(2);
     contentScriptDisconnected(otherFrameId);
-    expect(frameManager.frames.length).toBe(1);
-    expect(frameManager.frames[0].id).toBe(topLevelFrameId);
+    expect(frameManager.frames().length).toBe(1);
+    expect(frameManager.frames()[0].id).toBe(topLevelFrameId);
 
     const errorSpy = spyOn(console, 'error');
     contentScriptDisconnected(topLevelFrameId);
-    expect(frameManager.frames.length).toBe(0);
+    expect(frameManager.frames().length).toBe(0);
     expect(errorSpy).toHaveBeenCalledWith('Angular DevTools is not connected to any frames.');
   });
 
@@ -102,18 +103,18 @@ describe('FrameManager', () => {
     contentScriptConnected(topLevelFrameId, 'name', 'http://localhost:4200/url');
     contentScriptConnected(otherFrameId, 'name2', 'http://localhost:4200/url2');
     frameConnected(otherFrameId);
-    expect(frameManager.selectedFrame?.id).toBe(otherFrameId);
+    expect(frameManager.selectedFrame()?.id).toBe(otherFrameId);
     contentScriptDisconnected(otherFrameId);
-    expect(frameManager.selectedFrame?.id).toBe(topLevelFrameId);
+    expect(frameManager.selectedFrame()?.id).toBe(topLevelFrameId);
   });
 
   it('should not set selected frame to top level frame when contentScriptDisconnected event is emitted for non selected frame', () => {
     contentScriptConnected(topLevelFrameId, 'name', 'http://localhost:4200/url');
     contentScriptConnected(otherFrameId, 'name2', 'http://localhost:4200/url2');
     frameConnected(topLevelFrameId);
-    expect(frameManager.selectedFrame?.id).toBe(topLevelFrameId);
+    expect(frameManager.selectedFrame()?.id).toBe(topLevelFrameId);
     contentScriptDisconnected(otherFrameId);
-    expect(frameManager.selectedFrame?.id).toBe(topLevelFrameId);
+    expect(frameManager.selectedFrame()?.id).toBe(topLevelFrameId);
   });
 
   it('should not set selected frame to top level frame when contentScriptDisconnected event is emitted for non existing frame', () => {
@@ -122,9 +123,9 @@ describe('FrameManager', () => {
     contentScriptConnected(topLevelFrameId, 'name', 'http://localhost:4200/url');
     contentScriptConnected(otherFrameId, 'name2', 'http://localhost:4200/url2');
     frameConnected(otherFrameId);
-    expect(frameManager.selectedFrame?.id).toBe(otherFrameId);
+    expect(frameManager.selectedFrame()?.id).toBe(otherFrameId);
     contentScriptDisconnected(nonExistingFrameId);
-    expect(frameManager.selectedFrame?.id).toBe(otherFrameId);
+    expect(frameManager.selectedFrame()?.id).toBe(otherFrameId);
   });
 
   it('isSelectedFrame should return true when frame matches selected frame', () => {
@@ -161,21 +162,21 @@ describe('FrameManager', () => {
     const topLevelFrame = getFrameFromFrameManager(topLevelFrameId);
     expect(topLevelFrame).toBeDefined();
     frameManager.inspectFrame(topLevelFrame!);
-    expect(frameManager.selectedFrame?.id).toBe(topLevelFrameId);
+    expect(frameManager.selectedFrame()?.id).toBe(topLevelFrameId);
   });
 
   it('frameHasUniqueUrl should return false when a two frames have the same url', () => {
     contentScriptConnected(topLevelFrameId, 'name', 'https://angular.dev/');
     contentScriptConnected(otherFrameId, 'name2', 'https://angular.dev/');
-    expect(frameManager.selectedFrame?.url.toString()).toBe('https://angular.dev/');
-    expect(frameManager.frameHasUniqueUrl(frameManager.selectedFrame!)).toBe(false);
+    expect(frameManager.selectedFrame()?.url.toString()).toBe('https://angular.dev/');
+    expect(frameManager.frameHasUniqueUrl(frameManager.selectedFrame()!)).toBe(false);
   });
 
   it('frameHasUniqueUrl should return true when only one frame has a given url', () => {
     contentScriptConnected(topLevelFrameId, 'name', 'https://angular.dev/');
     contentScriptConnected(otherFrameId, 'name', 'https://angular.dev/overview');
-    expect(frameManager.selectedFrame?.url.toString()).toBe('https://angular.dev/');
-    expect(frameManager.frameHasUniqueUrl(frameManager.selectedFrame!)).toBe(true);
+    expect(frameManager.selectedFrame()?.url.toString()).toBe('https://angular.dev/');
+    expect(frameManager.frameHasUniqueUrl(frameManager.selectedFrame()!)).toBe(true);
   });
 
   it('frameHasUniqueUrl should not consider url fragments as part of the url comparison', () => {
@@ -185,8 +186,10 @@ describe('FrameManager', () => {
       'name',
       'https://angular.dev/guide/components#using-components',
     );
-    expect(frameManager.selectedFrame?.url.toString()).toBe('https://angular.dev/guide/components');
-    expect(frameManager.frameHasUniqueUrl(frameManager.selectedFrame!)).toBe(false);
+    expect(frameManager.selectedFrame()?.url.toString()).toBe(
+      'https://angular.dev/guide/components',
+    );
+    expect(frameManager.frameHasUniqueUrl(frameManager.selectedFrame()!)).toBe(false);
   });
 
   it('frameHasUniqueUrl should return false when frame is null', () => {