bug fix for signal form

packages/angular-render-scan/src/infrastructure/angular/auto-instrumentation.spec.ts

@@ -0,0 +1,131 @@
+import { describe, expect, it } from 'vitest';
+import { patchSignalsOnInstance } from './auto-instrumentation';
+
+// A minimal stand-in for an Angular signal: a callable with a [SIGNAL] symbol
+// node plus set/update/asReadonly, matching what the instrumentation detects.
+const SIGNAL = Symbol('SIGNAL');
+function makeSignal<T>(initial: T) {
+  const node = { value: initial, equal: (a: T, b: T) => a === b, debugName: undefined };
+  const fn: any = function () {
+    return node.value;
+  };
+  fn[SIGNAL] = node;
+  fn.set = (v: T) => {
+    node.value = v;
+  };
+  fn.update = (f: (v: T) => T) => {
+    node.value = f(node.value);
+  };
+  fn.asReadonly = () => fn;
+  return fn;
+}
+
+// Stand-ins that mirror the real framework shapes from the bug reports: a
+// reactive-forms control tree and a router URL segment tree, both of which keep
+// their children in plain-object dictionaries that the framework walks with
+// Object.keys(). The original instrumentation injected an enumerable marker into
+// those dictionaries, so iteration picked up a `true` and called methods on it.
+class AbstractControl {
+  _updateTreeValidity(): void {
+    /* no-op */
+  }
+}
+class FormControl extends AbstractControl {}
+class FormGroup extends AbstractControl {
+  controls: Record<string, AbstractControl>;
+  constructor() {
+    super();
+    this.controls = { name: new FormControl(), email: new FormControl() };
+  }
+  _forEachChild(cb: (c: AbstractControl) => void): void {
+    Object.keys(this.controls).forEach(k => cb(this.controls[k]));
+  }
+  override _updateTreeValidity(): void {
+    this._forEachChild(ctrl => ctrl._updateTreeValidity());
+  }
+}
+
+class UrlSegmentGroup {
+  children: Record<string, UrlSegmentGroup> = {};
+  hasChildren(): boolean {
+    return Object.keys(this.children).length > 0;
+  }
+}
+
+describe('patchSignalsOnInstance', () => {
+  it('does not corrupt reactive-forms control trees', () => {
+    const form = new FormGroup();
+    const component = { name: 'FormComponent', form };
+
+    patchSignalsOnInstance(component, 'FormComponent');
+
+    // The controls dictionary must not gain an injected enumerable key...
+    expect(Object.keys(form.controls)).toEqual(['name', 'email']);
+    // ...so framework iteration over it stays valid (this threw the original
+    // "ctrl._updateTreeValidity is not a function").
+    expect(() => form._updateTreeValidity()).not.toThrow();
+  });
+
+  it('does not corrupt router URL segment trees', () => {
+    const root = new UrlSegmentGroup();
+    root.children = { primary: new UrlSegmentGroup() };
+    const component = { name: 'NavComponent', tree: root };
+
+    patchSignalsOnInstance(component, 'NavComponent');
+
+    expect(Object.keys(root.children)).toEqual(['primary']);
+    // The original bug surfaced as "segment.hasChildren is not a function" when
+    // serializing children — emulate that walk.
+    expect(() =>
+      Object.keys(root.children).forEach(k => root.children[k].hasChildren())
+    ).not.toThrow();
+  });
+
+  it('never writes enumerable markers onto any walked object', () => {
+    class ViewModel {
+      nested = { deep: { leaf: 1 } };
+    }
+    const component = { name: 'C', vm: new ViewModel() };
+
+    patchSignalsOnInstance(component, 'C');
+
+    const polluted = (obj: object): boolean =>
+      Object.keys(obj).some(k => k.includes('angularRenderScan'));
+    expect(polluted(component)).toBe(false);
+    expect(polluted(component.vm)).toBe(false);
+    expect(polluted(component.vm.nested)).toBe(false);
+  });
+
+  it('wraps real signals but leaves plain methods untouched', () => {
+    const realSignal = makeSignal(1);
+    const plainMethod = function () {
+      return 'hello';
+    };
+    const component: any = {
+      name: 'C',
+      count: realSignal,
+      doThing: plainMethod
+    };
+
+    patchSignalsOnInstance(component, 'C');
+
+    // Real signal is wrapped (new function) but still reads through and is settable.
+    expect(component.count).not.toBe(realSignal);
+    expect(component.count()).toBe(1);
+    component.count.set(5);
+    expect(component.count()).toBe(5);
+
+    // The cache-bug regression: a plain function property next to a signal must
+    // NOT be wrapped as a fake signal — identity is preserved.
+    expect(component.doThing).toBe(plainMethod);
+  });
+
+  it('is safe against cyclic object graphs', () => {
+    const a: any = { name: 'A' };
+    const b: any = { name: 'B', a };
+    a.b = b;
+    const component = { name: 'C', a };
+
+    expect(() => patchSignalsOnInstance(component, 'C')).not.toThrow();
+  });
+});

packages/angular-render-scan/src/infrastructure/angular/auto-instrumentation.ts

@@ -16,19 +16,53 @@ import type { AngularRenderChangedInput, AngularRenderScanRenderDetails } from '
 const ProfilerEventTemplateUpdateStart = 2;
 const ProfilerEventTemplateUpdateEnd = 3;
 
-function patchSignalsOnInstance(instance: any, compName: string): void {
+// How deep into a component's object graph we walk looking for nested signals.
+const MAX_SIGNAL_PATCH_DEPTH = 3;
+
+// Constructor names we must never recurse into. Walking these mutates / wraps
+// Angular framework internals and corrupts them — e.g. reactive-forms controls
+// and router URL trees, whose children are plain-object containers that Angular
+// iterates with Object.keys(). See exclusions in the recursion branch below.
+const FRAMEWORK_CTOR_EXCLUSIONS = [
+  'ElementRef',
+  'ViewContainerRef',
+  'ChangeDetectorRef',
+  'NgZone',
+  'ApplicationRef',
+  'Router',
+  'ActivatedRoute',
+  'UrlTree',
+  'UrlSegment',
+  'UrlSegmentGroup',
+  'AbstractControl',
+  'FormControl',
+  'FormGroup',
+  'FormArray',
+  'FormRecord',
+  'NgControl',
+  'EventEmitter'
+];
+
+// Per-prop scan for the signal symbol. Must NOT be cached across props: a cached
+// symbol would make us treat any later function-valued property as a signal.
+function getSignalSymbol(obj: any): symbol | undefined {
+  if (!obj || (typeof obj !== 'object' && typeof obj !== 'function')) return undefined;
+  const sym = Reflect.ownKeys(obj).find(k => typeof k === 'symbol' && String(k).includes('SIGNAL'));
+  return sym as symbol | undefined;
+}
+
+export function patchSignalsOnInstance(
+  instance: any,
+  compName: string,
+  visited: WeakSet<object> = new WeakSet(),
+  depth = 0
+): void {
   if (!instance || typeof instance !== 'object') return;
-  if (instance.__angularRenderScanPatchedSignals) return;
-  instance.__angularRenderScanPatchedSignals = true;
-
-  let signalSymbol: symbol | undefined;
-  const getSignalSymbol = (obj: any) => {
-    if (!obj || (typeof obj !== 'object' && typeof obj !== 'function')) return undefined;
-    if (signalSymbol) return signalSymbol;
-    const sym = Reflect.ownKeys(obj).find(k => typeof k === 'symbol' && String(k).includes('SIGNAL'));
-    if (sym) signalSymbol = sym as symbol;
-    return signalSymbol;
-  };
+  // Track visited objects in a side WeakSet instead of writing a marker onto the
+  // instance. Writing an enumerable property pollutes Object.keys() of any object
+  // we touch, which breaks Angular containers (form controls, URL segments).
+  if (visited.has(instance)) return;
+  visited.add(instance);
 
   const keys = Object.getOwnPropertyNames(instance);
   for (const key of keys) {
@@ -68,7 +102,12 @@ function patchSignalsOnInstance(instance: any, compName: string): void {
           }
         } as any;
 
-        wrapped.__angularRenderScanPatched = true;
+        Object.defineProperty(wrapped, '__angularRenderScanPatched', {
+          value: true,
+          enumerable: false,
+          configurable: true,
+          writable: true
+        });
         wrapped[sym] = internalNode;
         wrapped.toString = originalSignal.toString.bind(originalSignal);
 
@@ -121,21 +160,23 @@ function patchSignalsOnInstance(instance: any, compName: string): void {
 
         instance[key] = wrapped;
       }
-    } else if (typeof prop === 'object' && prop !== null && !Array.isArray(prop)) {
+    } else if (depth < MAX_SIGNAL_PATCH_DEPTH && typeof prop === 'object' && prop !== null && !Array.isArray(prop)) {
+      // Only recurse into class instances that look like view models. Skip
+      // framework internals and plain-object containers (constructor `Object`),
+      // since those are the dictionaries Angular iterates with Object.keys().
       const ctorName = prop.constructor?.name;
       if (
         ctorName &&
+        ctorName !== 'Object' &&
         !ctorName.startsWith('ɵ') &&
-        !ctorName.startsWith('ElementRef') &&
-        !ctorName.startsWith('ViewContainerRef') &&
-        !ctorName.startsWith('ChangeDetectorRef') &&
-        !ctorName.startsWith('NgZone') &&
-        !ctorName.startsWith('ApplicationRef') &&
-        !ctorName.startsWith('Router') &&
-        !ctorName.startsWith('ActivatedRoute') &&
-        !(prop instanceof HTMLElement)
+        !FRAMEWORK_CTOR_EXCLUSIONS.some(name => ctorName.startsWith(name)) &&
+        !(prop instanceof HTMLElement) &&
+        !(prop instanceof Date) &&
+        !(prop instanceof RegExp) &&
+        !(prop instanceof Map) &&
+        !(prop instanceof Set)
       ) {
-        patchSignalsOnInstance(prop, ctorName);
+        patchSignalsOnInstance(prop, ctorName, visited, depth + 1);
       }
     }
   }