fix(core): signals should be tracked when embeddedViewRef.detectChang…

…es is called

This commit fixes an issue where signals in embedded views are not
tracked if they are refreshed with `EmbeddedViewRef.detectChanges`
directly. We had previously assumed that embedded views were always
refreshed along with their hosts.

packages/core/src/render3/instructions/change_detection.ts

@@ -9,20 +9,16 @@
 import {
   consumerAfterComputation,
   consumerBeforeComputation,
+  consumerDestroy,
   consumerPollProducersForChange,
+  getActiveConsumer,
   ReactiveNode,
 } from '@angular/core/primitives/signals';
 
 import {RuntimeError, RuntimeErrorCode} from '../../errors';
 import {assertDefined, assertEqual} from '../../util/assert';
-import {assertLContainer} from '../assert';
 import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} from '../hooks';
-import {
-  CONTAINER_HEADER_OFFSET,
-  LContainer,
-  LContainerFlags,
-  MOVED_VIEWS,
-} from '../interfaces/container';
+import {CONTAINER_HEADER_OFFSET, LContainerFlags, MOVED_VIEWS} from '../interfaces/container';
 import {ComponentTemplate, RenderFlags} from '../interfaces/definition';
 import {
   CONTEXT,
@@ -32,16 +28,16 @@ import {
   InitPhaseState,
   LView,
   LViewFlags,
-  PARENT,
   REACTIVE_TEMPLATE_CONSUMER,
   TVIEW,
   TView,
-  TViewType,
 } from '../interfaces/view';
 import {
+  getOrCreateTemporaryConsumer,
   getOrBorrowReactiveLViewConsumer,
   maybeReturnReactiveLViewConsumer,
   ReactiveLViewConsumer,
+  viewShouldHaveReactiveConsumer,
 } from '../reactive_lview_consumer';
 import {
   enterView,
@@ -200,11 +196,27 @@ export function refreshView<T>(
   // - We might already be in a reactive context if this is an embedded view of the host.
   // - We might be descending into a view that needs a consumer.
   enterView(lView);
+  let returnConsumerToPool = true;
   let prevConsumer: ReactiveNode | null = null;
   let currentConsumer: ReactiveLViewConsumer | null = null;
-  if (!isInCheckNoChangesPass && viewShouldHaveReactiveConsumer(tView)) {
-    currentConsumer = getOrBorrowReactiveLViewConsumer(lView);
-    prevConsumer = consumerBeforeComputation(currentConsumer);
+  if (!isInCheckNoChangesPass) {
+    if (viewShouldHaveReactiveConsumer(tView)) {
+      currentConsumer = getOrBorrowReactiveLViewConsumer(lView);
+      prevConsumer = consumerBeforeComputation(currentConsumer);
+    } else if (getActiveConsumer() === null) {
+      // If the current view should not have a reactive consumer but we don't have an active consumer,
+      // we still need to create a temporary consumer to track any signal reads in this template.
+      // This is a rare case that can happen with `viewContainerRef.createEmbeddedView(...).detectChanges()`.
+      // This temporary consumer marks the first parent that _should_ have a consumer for refresh.
+      // Once that refresh happens, the signals will be tracked in the parent consumer and we can destroy
+      // the temporary one.
+      returnConsumerToPool = false;
+      currentConsumer = getOrCreateTemporaryConsumer(lView);
+      prevConsumer = consumerBeforeComputation(currentConsumer);
+    } else if (lView[REACTIVE_TEMPLATE_CONSUMER]) {
+      consumerDestroy(lView[REACTIVE_TEMPLATE_CONSUMER]);
+      lView[REACTIVE_TEMPLATE_CONSUMER] = null;
+    }
   }
 
   try {
@@ -339,30 +351,14 @@ export function refreshView<T>(
   } finally {
     if (currentConsumer !== null) {
       consumerAfterComputation(currentConsumer, prevConsumer);
-      maybeReturnReactiveLViewConsumer(currentConsumer);
+      if (returnConsumerToPool) {
+        maybeReturnReactiveLViewConsumer(currentConsumer);
+      }
     }
     leaveView();
   }
 }
 
-/**
- * Indicates if the view should get its own reactive consumer node.
- *
- * In the current design, all embedded views share a consumer with the component view. This allows
- * us to refresh at the component level rather than at a per-view level. In addition, root views get
- * their own reactive node because root component will have a host view that executes the
- * component's host bindings. This needs to be tracked in a consumer as well.
- *
- * To get a more granular change detection than per-component, all we would just need to update the
- * condition here so that a given view gets a reactive consumer which can become dirty independently
- * from its parent component. For example embedded views for signal components could be created with
- * a new type "SignalEmbeddedView" and the condition here wouldn't even need updating in order to
- * get granular per-view change detection for signal components.
- */
-function viewShouldHaveReactiveConsumer(tView: TView) {
-  return tView.type !== TViewType.Embedded;
-}
-
 /**
  * Goes over embedded views (ones created through ViewContainerRef APIs) and refreshes
  * them by executing an associated template function.

packages/core/src/render3/reactive_lview_consumer.ts

@@ -8,13 +8,20 @@
 
 import {REACTIVE_NODE, ReactiveNode} from '@angular/core/primitives/signals';
 
-import {LView, REACTIVE_TEMPLATE_CONSUMER} from './interfaces/view';
-import {markAncestorsForTraversal} from './util/view_utils';
+import {
+  LView,
+  PARENT,
+  REACTIVE_TEMPLATE_CONSUMER,
+  TVIEW,
+  TView,
+  TViewType,
+} from './interfaces/view';
+import {getLViewParent, markAncestorsForTraversal, markViewForRefresh} from './util/view_utils';
+import {assertDefined} from '../util/assert';
 
-let freeConsumers: ReactiveLViewConsumer[] = [];
+let freeConsumers: ReactiveNode[] = [];
 export interface ReactiveLViewConsumer extends ReactiveNode {
   lView: LView | null;
-  slot: typeof REACTIVE_TEMPLATE_CONSUMER;
 }
 
 /**
@@ -27,8 +34,7 @@ export function getOrBorrowReactiveLViewConsumer(lView: LView): ReactiveLViewCon
 }
 
 function borrowReactiveLViewConsumer(lView: LView): ReactiveLViewConsumer {
-  const consumer: ReactiveLViewConsumer =
-    freeConsumers.pop() ?? Object.create(REACTIVE_LVIEW_CONSUMER_NODE);
+  const consumer = freeConsumers.pop() ?? Object.create(REACTIVE_LVIEW_CONSUMER_NODE);
   consumer.lView = lView;
   return consumer;
 }
@@ -42,7 +48,7 @@ export function maybeReturnReactiveLViewConsumer(consumer: ReactiveLViewConsumer
   freeConsumers.push(consumer);
 }
 
-const REACTIVE_LVIEW_CONSUMER_NODE: Omit<ReactiveLViewConsumer, 'lView' | 'slot'> = {
+const REACTIVE_LVIEW_CONSUMER_NODE: Omit<ReactiveLViewConsumer, 'lView'> = {
   ...REACTIVE_NODE,
   consumerIsAlwaysLive: true,
   consumerMarkedDirty: (node: ReactiveLViewConsumer) => {
@@ -52,3 +58,59 @@ const REACTIVE_LVIEW_CONSUMER_NODE: Omit<ReactiveLViewConsumer, 'lView' | 'slot'
     this.lView![REACTIVE_TEMPLATE_CONSUMER] = this;
   },
 };
+
+/**
+ * Creates a temporary consumer for use with `LView`s that should not have consumers.
+ * If the LView already has a consumer, returns the existing one instead.
+ *
+ * This is necessary because some APIs may cause change detection directly on an LView
+ * that we do not want to have a consumer (Embedded views today). As a result, there
+ * would be no active consumer from running change detection on its host component
+ * and any signals in the LView template would be untracked. Instead, we create
+ * this temporary consumer that marks the first parent that _should_ have a consumer
+ * for refresh. Once change detection runs as part of that refresh, we throw away
+ * this consumer because its signals will then be tracked by the parent's consumer.
+ */
+export function getOrCreateTemporaryConsumer(lView: LView): ReactiveLViewConsumer {
+  return (
+    lView[REACTIVE_TEMPLATE_CONSUMER] ?? {
+      ...REACTIVE_NODE,
+      consumerIsAlwaysLive: true,
+      consumerMarkedDirty: (node: ReactiveLViewConsumer) => {
+        let parent = getLViewParent(node.lView!);
+        while (parent && !viewShouldHaveReactiveConsumer(parent[TVIEW])) {
+          parent = getLViewParent(parent);
+        }
+        if (!parent) {
+          // If we can't find an appropriate parent that should have a consumer, we
+          // don't have a way of appropriately refreshing this LView as part of application synchronization.
+          return;
+        }
+
+        markViewForRefresh(parent);
+      },
+      consumerOnSignalRead(this: ReactiveLViewConsumer): void {
+        this.lView![REACTIVE_TEMPLATE_CONSUMER] = this;
+      },
+      lView,
+    }
+  );
+}
+
+/**
+ * Indicates if the view should get its own reactive consumer node.
+ *
+ * In the current design, all embedded views share a consumer with the component view. This allows
+ * us to refresh at the component level rather than at a per-view level. In addition, root views get
+ * their own reactive node because root component will have a host view that executes the
+ * component's host bindings. This needs to be tracked in a consumer as well.
+ *
+ * To get a more granular change detection than per-component, all we would just need to update the
+ * condition here so that a given view gets a reactive consumer which can become dirty independently
+ * from its parent component. For example embedded views for signal components could be created with
+ * a new type "SignalEmbeddedView" and the condition here wouldn't even need updating in order to
+ * get granular per-view change detection for signal components.
+ */
+export function viewShouldHaveReactiveConsumer(tView: TView) {
+  return tView.type !== TViewType.Embedded;
+}

packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts

@@ -7,19 +7,15 @@
  */
 
 import {NgFor, NgIf} from '@angular/common';
-import {PLATFORM_BROWSER_ID} from '@angular/common/src/platform_id';
 import {
-  afterNextRender,
   ApplicationRef,
   ChangeDetectionStrategy,
   ChangeDetectorRef,
   Component,
   computed,
   Directive,
-  EnvironmentInjector,
   inject,
   Input,
-  PLATFORM_ID,
   signal,
   TemplateRef,
   ViewChild,
@@ -803,6 +799,38 @@ describe('OnPush components with signals', () => {
       fixture.detectChanges();
       expect(trim(fixture.nativeElement.textContent)).toEqual('new');
     });
+
+    it('tracks signal updates if embedded view is change detected directly', () => {
+      @Component({
+        changeDetection: ChangeDetectionStrategy.OnPush,
+        template: `
+            <ng-template #template>{{value()}}</ng-template>
+          `,
+        standalone: true,
+      })
+      class Test {
+        value = signal('initial');
+        @ViewChild('template', {static: true, read: TemplateRef})
+        template!: TemplateRef<{}>;
+        @ViewChild('template', {static: true, read: ViewContainerRef})
+        vcr!: ViewContainerRef;
+      }
+
+      const fixture = TestBed.createComponent(Test);
+      const appRef = TestBed.inject(ApplicationRef);
+      appRef.attachView(fixture.componentRef.hostView);
+      appRef.tick();
+
+      const viewRef = fixture.componentInstance.vcr.createEmbeddedView(
+        fixture.componentInstance.template,
+      );
+      viewRef.detectChanges();
+      expect(fixture.nativeElement.innerText).toContain('initial');
+
+      fixture.componentInstance.value.set('new');
+      appRef.tick();
+      expect(fixture.nativeElement.innerText).toContain('new');
+    });
   });
 
   describe('shielded by non-dirty OnPush', () => {

packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json

@@ -434,6 +434,9 @@
   {
     "name": "REACTIVE_LVIEW_CONSUMER_NODE"
   },
+  {
+    "name": "REACTIVE_NODE"
+  },
   {
     "name": "REMOVE_STYLES_ON_COMPONENT_DESTROY"
   },
@@ -746,6 +749,12 @@
   {
     "name": "configureViewWithDirective"
   },
+  {
+    "name": "consumerBeforeComputation"
+  },
+  {
+    "name": "consumerDestroy"
+  },
   {
     "name": "consumerIsLive"
   },
@@ -1454,6 +1463,9 @@
   {
     "name": "viewAttachedToChangeDetector"
   },
+  {
+    "name": "viewShouldHaveReactiveConsumer"
+  },
   {
     "name": "visitDslNode"
   },

packages/core/test/bundling/animations/bundle.golden_symbols.json

@@ -473,6 +473,9 @@
   {
     "name": "REACTIVE_LVIEW_CONSUMER_NODE"
   },
+  {
+    "name": "REACTIVE_NODE"
+  },
   {
     "name": "REMOVE_STYLES_ON_COMPONENT_DESTROY"
   },
@@ -803,6 +806,12 @@
   {
     "name": "configureViewWithDirective"
   },
+  {
+    "name": "consumerBeforeComputation"
+  },
+  {
+    "name": "consumerDestroy"
+  },
   {
     "name": "consumerIsLive"
   },
@@ -1523,6 +1532,9 @@
   {
     "name": "viewAttachedToChangeDetector"
   },
+  {
+    "name": "viewShouldHaveReactiveConsumer"
+  },
   {
     "name": "visitDslNode"
   },

packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json

@@ -359,6 +359,9 @@
   {
     "name": "REACTIVE_LVIEW_CONSUMER_NODE"
   },
+  {
+    "name": "REACTIVE_NODE"
+  },
   {
     "name": "REMOVE_STYLES_ON_COMPONENT_DESTROY"
   },
@@ -602,6 +605,12 @@
   {
     "name": "configureViewWithDirective"
   },
+  {
+    "name": "consumerBeforeComputation"
+  },
+  {
+    "name": "consumerDestroy"
+  },
   {
     "name": "consumerIsLive"
   },
@@ -1223,6 +1232,9 @@
   {
     "name": "viewAttachedToChangeDetector"
   },
+  {
+    "name": "viewShouldHaveReactiveConsumer"
+  },
   {
     "name": "walkProviderTree"
   },

packages/core/test/bundling/defer/bundle.golden_symbols.json

@@ -683,6 +683,12 @@
   {
     "name": "configureViewWithDirective"
   },
+  {
+    "name": "consumerBeforeComputation"
+  },
+  {
+    "name": "consumerDestroy"
+  },
   {
     "name": "consumerIsLive"
   },
@@ -2468,6 +2474,9 @@
   {
     "name": "viewAttachedToChangeDetector"
   },
+  {
+    "name": "viewShouldHaveReactiveConsumer"
+  },
   {
     "name": "walkProviderTree"
   },