apps/automated/src/ui/frame/frame-tests-common.ts (1)

272-320: Well-structured test for frame loading coordination.

The test effectively validates the nested frame loading order, which is crucial for the PR's objectives. The test methodology is sound:

1. Creates root and nested frames in proper hierarchy
2. Overrides onFrameLoaded to track execution order
3. Validates root frame loads before nested frame
4. Proper cleanup of overrides and frame stack

However, consider a few improvements:

Consider adding error handling and more detailed assertions:

+	// Ensure we have exactly 2 frames loaded
+	TKUnit.assertEqual(loadedFrames.length, 2, "Expected exactly 2 frames to be loaded");
+	
 	TKUnit.assertEqual(rootFrame, loadedFrames[0]);
 	TKUnit.assertEqual(nestedFrame, loadedFrames[1]);
+	
+	// Verify frames are in expected loaded state
+	TKUnit.assertTrue(rootFrame.isLoaded, "Root frame should be loaded");
+	TKUnit.assertTrue(nestedFrame.isLoaded, "Nested frame should be loaded");

Also consider testing error scenarios where frames might fail to load properly.

packages/core/ui/frame/frame-common.ts (1)

356-361: Missing documentation for new internal event

_notifyFrameEntryLoaded() emits _frameEntryLoaded, but neither the JSDoc nor public typings mention the event.
Please add a short comment so maintainers know this is internal and avoid subscribing from user code.

packages/core/ui/frame/fragment.transitions.android.ts (2)

436-444: Cloned transition listeners keep strong reference to original entry

cloneExpandedTransitionListener reuses expandedTransitionListener.entry, so every clone still references the old entry object.
That is intended for restore, but if the snapshot outlives the entry’s fragment (e.g. orientation change) the listener keeps the fragment from GC.

If unavoidable, at least wrap entry in a WeakRef inside the listener, or ensure _restoreTransitionState executes quickly and the snapshot is nulled afterwards (see previous comment).

---

155-156: String slicing duplicated & fragile

'flip'.length / 'slide'.length string-math is repeated three times.
A simple helper improves readability and avoids mismatched literals:

function stripPrefix(value: string, prefix: string) {
  return value.startsWith(prefix) ? value.substring(prefix.length) : value;
}
const direction = stripPrefix(name, 'flip') || 'right';

Also applies to: 628-629, 653-654

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/core/ui/frame/frame-common.ts (2)

125-143: Critical issue: Event listener memory leak persists

The event-driven coordination logic is sound, but this implementation still has the memory leak issue identified in previous reviews. The listener registered on parentFrame is not properly tracked for cleanup.

The solution requires storing both the parent frame reference and the listener function for proper cleanup:

+private _parentFrameRef?: WeakRef<FrameBase>;
+private _frameLoadListener?: () => void;

 @profile
 public onLoaded() {
   const parentFrame = this.page?.frame;
   super.onLoaded();

   if (parentFrame?.isLoadingSubviews) {
+    this._parentFrameRef = new WeakRef(parentFrame);
     const frameRef = new WeakRef(this);
-    parentFrame.once(FRAME_ENTRY_LOADED_EVENT, () => {
+    this._frameLoadListener = () => {
       const frame = frameRef.deref();
       if (frame) {
         frame.onFrameLoaded();
       }
-    });
+    };
+    parentFrame.once(FRAME_ENTRY_LOADED_EVENT, this._frameLoadListener);
   } else {
     this.onFrameLoaded();
   }
 }

---

145-156: Critical issue: Incorrect event listener cleanup

The cleanup logic has two fundamental problems identified in previous reviews:

1. parentFrame.off(FRAME_ENTRY_LOADED_EVENT) removes ALL listeners for this event from the parent, not just this frame's listener
2. The condition parentFrame.hasListeners(FRAME_ENTRY_LOADED_EVENT) checks the wrong frame

Apply this fix to properly clean up the specific listener:

 @profile
 public onUnloaded() {
-  const parentFrame = this.page?.frame;
   super.onUnloaded();

-  // This is a precaution in case the method is called asynchronously during the loading procedure
-  if (parentFrame && parentFrame.hasListeners(FRAME_ENTRY_LOADED_EVENT)) {
-    parentFrame.off(FRAME_ENTRY_LOADED_EVENT);
-  }
+  const parent = this._parentFrameRef?.deref();
+  if (parent && this._frameLoadListener) {
+    parent.off(FRAME_ENTRY_LOADED_EVENT, this._frameLoadListener);
+    this._frameLoadListener = undefined;
+    this._parentFrameRef = undefined;
+  }
 }

🧰 Tools

🪛 Biome (1.9.4)

[error] 153-153: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

packages/core/ui/frame/frame-common.ts (1)

84-84: Implement optional chaining for better safety

The static analysis correctly identifies opportunities to use optional chaining for cleaner and safer code.

Apply these improvements:

-if (parentFrame && parentFrame.canGoBack()) {
+if (parentFrame?.canGoBack()) {

-if (parentFrame && parentFrame.hasListeners(FRAME_ENTRY_LOADED_EVENT)) {
+if (parentFrame?.hasListeners?.(FRAME_ENTRY_LOADED_EVENT)) {

Also applies to: 153-153

🧰 Tools

🪛 Biome (1.9.4)

[error] 84-84: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro