Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.7k
fix(android): Better handling for nested frames#10713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
7cca393 toc55ab1fCompare5f86bc7 to6e86af2Comparenx-cloudbot commentedMay 11, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
View yourCI Pipeline Execution ↗ for commit211f28f.
☁️Nx Cloud last updated this comment at |
a9d0902 to67ce09aCompareCatchABus commentedMay 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@NathanWalker This should be good to go. Let me say that theoritically, this PR doesn't completely solve the problem because we have some UPDATED: This has actually got more room for improvements. |
It seems the problem goes a bit deeper so I'll keep this one as draft for just a little. |
@coderabbitai please review |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@NathanWalker I'll review this PR for you. Let me gather the details about the changes first. 🏁 Script executed: Length of output: 23664 Review of PR#10713: Better handling for nested frames@NathanWalker This PR addresses a critical issue with nested frame management in NativeScript core. Here's my comprehensive review: 🎯Core Problem & SolutionThe PR tackles the "No view found for id" exception that occurs when nested frames are destroyed and reused, particularly evident in Vue applications. The solution introduces proper sequencing of frame loading through an event-driven approach. ✅Architectural Improvements
|
This comment was marked as resolved.
This comment was marked as resolved.
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 5
🧹 Nitpick comments (4)
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:
- Creates root and nested frames in proper hierarchy
- Overrides
onFrameLoadedto track execution order- Validates root frame loads before nested frame
- 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 isinternal and avoid subscribing from user code.packages/core/ui/frame/fragment.transitions.android.ts (2)
436-444:Cloned transition listeners keep strong reference to originalentry
cloneExpandedTransitionListenerreusesexpandedTransitionListener.entry, so every clone still references theold 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
entryin aWeakRefinside the listener, or ensure_restoreTransitionStateexecutes quickly and the snapshot is nulled afterwards (see previous comment).
155-156:String slicing duplicated & fragile
'flip'.length/'slide'.lengthstring-math is repeated three times.
A simple helper improves readability and avoids mismatched literals:functionstripPrefix(value:string,prefix:string){returnvalue.startsWith(prefix) ?value.substring(prefix.length) :value;}constdirection=stripPrefix(name,'flip')||'right';Also applies to: 628-629, 653-654
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/automated/src/ui/frame/frame-tests-common.ts(2 hunks)packages/core/ui/core/view-base/index.ts(4 hunks)packages/core/ui/frame/callbacks/activity-callbacks.ts(0 hunks)packages/core/ui/frame/fragment.transitions.android.ts(7 hunks)packages/core/ui/frame/fragment.transitions.d.ts(2 hunks)packages/core/ui/frame/frame-common.ts(4 hunks)packages/core/ui/frame/index.android.ts(7 hunks)packages/core/ui/frame/index.ios.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/core/ui/frame/callbacks/activity-callbacks.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/core/ui/frame/fragment.transitions.d.ts (1)
packages/core/ui/frame/fragment.transitions.android.ts (2)
_getTransitionState(487-504)_restoreTransitionState(506-526)
packages/core/ui/frame/index.ios.ts (1)
packages/core/ui/frame/index.d.ts (2)
BackstackEntry(454-484)NavigationTransition(415-449)
packages/core/ui/frame/index.android.ts (3)
packages/core/ui/frame/fragment.transitions.android.ts (3)
_getTransitionState(487-504)_updateTransitions(283-309)_restoreTransitionState(506-526)packages/core/ui/frame/frame-common.ts (2)
currentEntry(559-565)BackstackEntry(18-18)packages/core/ui/frame/index.d.ts (1)
BackstackEntry(454-484)
🪛 Biome (1.9.4)
packages/core/ui/frame/frame-common.ts
[error] 84-84: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (11)
packages/core/ui/core/view-base/index.ts (4)
120-127:LGTM! Excellent type safety improvement.The generic enhancement to
getAncestorprovides better type safety by allowing the return type to be inferred from the criterion parameter. The type guard approach ensures proper type narrowing.
349-349:Good addition for subview loading coordination.The private
_isLoadingSubviewsfield supports the new event-driven frame loading mechanism described in the PR objectives.
642-644:Proper encapsulation of loading state.The public getter provides read-only access to the internal loading state, which is appropriate for the frame coordination mechanism.
706-714:Verify thread safety and exception handling.The loading state tracking implementation looks correct, but consider potential edge cases:
- Exception handling: If an exception occurs while loading child views, the flag might remain
trueindefinitely- Re-entrancy: Consider what happens if
onLoadedis called recursively during child loadingConsider wrapping the child loading logic in a try-finally block to ensure the flag is always reset:
this._isLoadingSubviews = true;--this.eachChild((child) => {-this.loadView(child);--return true;-});--this._isLoadingSubviews = false;+try {+this.eachChild((child) => {+this.loadView(child);+return true;+});+} finally {+this._isLoadingSubviews = false;+}packages/core/ui/frame/fragment.transitions.d.ts (2)
1-1:LGTM! Import added for new transition state functionality.The
TransitionStateimport is correctly added to support the new transition state management functions.
23-30:Good modularization of transition state management.The new function declarations provide a clean API for saving and restoring transition states, which should help with the nested frame lifecycle issues mentioned in the PR objectives.
The function signatures are appropriate:
_getTransitionStatereturns a snapshot of the current state_restoreTransitionStateaccepts a snapshot to restoreapps/automated/src/ui/frame/frame-tests-common.ts (1)
8-8:LGTM! Import added for test requirements.The
StackLayoutimport is correctly added to support the new nested frame test.packages/core/ui/frame/index.ios.ts (2)
91-91:LGTM! Type safety improvement for navigation core.Changing the parameter type from
anytoBackstackEntryprovides better type safety and aligns with the Android implementation improvements in this PR.
520-520:LGTM! Refined callback type annotation.The change from generic
Functionto specific() => voidimproves type safety by explicitly indicating the callback takes no parameters and returns void. This is more precise than the genericFunctiontype.packages/core/ui/frame/index.android.ts (1)
541-565:Dispose transition snapshot eagerly
disposeNativeViewsets_cachedTransitionState = nullbutdoes not remove any listeners that might have been cloned inside the snapshot.
IfdisposeNativeViewfires before the simulated navigation completes, the cloned transitions may still point to disposed fragments.Consider invoking
_clearEntry(_cachedTransitionState?.entry)before nullifying the snapshot.packages/core/ui/frame/fragment.transitions.android.ts (1)
283-289:Good safeguardEarly-returning when
fragmentis falsy prevents NPEs during fragment restoration. Nice catch.
Uh oh!
There was an error while loading.Please reload this page.
| @profile | ||
| public onLoaded() { | ||
| // Property page refers to the page this frame is nested into | ||
| const parentFrame = this.page?.frame; | ||
| super.onLoaded(); | ||
| if (parentFrame?.isLoadingSubviews) { | ||
| const frameRef = new WeakRef(this); | ||
| parentFrame.once(FRAME_ENTRY_LOADED_EVENT, () => { | ||
| const frame = frameRef.deref(); | ||
| if (frame) { | ||
| frame.onFrameLoaded(); | ||
| } | ||
| }); | ||
| } else { | ||
| this.onFrameLoaded(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
💡 Verification agent
🧩 Analysis chain
Potential dangling listener on parent frame
onLoaded registers aonce handler onparentFrame but never stores a reference to that frame.
If the child frame is unloaded before the parent fires_frameEntryLoaded, that handler remains attached to the parent for the lifetime of the app, causing a memory leak.
Consider capturingparentFrame in a weak ref and detaching the listener from theparent insideonUnloaded (or when the child is destroyed).
// captureconst parentFrameListener = () => { … }parentFrame.once(FRAME_ENTRY_LOADED_EVENT, parentFrameListener);this._parentFrameRef = new WeakRef(parentFrame);this._parentFrameListener = parentFrameListener;and later:
public override onUnloaded() { …- if (this.hasListeners(FRAME_ENTRY_LOADED_EVENT)) {- this.off(FRAME_ENTRY_LOADED_EVENT);- }+ const parent = this._parentFrameRef?.deref?.();+ if (parent && this._parentFrameListener) {+ parent.off(FRAME_ENTRY_LOADED_EVENT, this._parentFrameListener);+ }}I’d like to confirm whetheronUnloaded is implemented in this class (and thatparent.off is available), so we can determine if the suggested cleanup hook is feasible:
🏁 Script executed:
#!/bin/bash# Locate frame-common.ts and inspect for onUnloaded overridefile=$(fd frame-common.ts)echo"Inspecting:$file"rg -n"onUnloaded" -C3"$file"# Check the FRAME_ENTRY_LOADED_EVENT declaration for contextrg -n"FRAME_ENTRY_LOADED_EVENT" -C3"$file"
Length of output: 1658
Fix dangling listener on parent frame
TheonLoaded method registers aonce listener onparentFrame, but the existingonUnloaded only callsthis.off on the child frame—leaving the parent-attached callback alive if the child is unloaded first. To prevent this leak, capture both the parent reference and the listener callback, then remove it from the parent inonUnloaded.
• File:packages/core/ui/frame/frame-common.ts
– onLoaded (lines ~131–139): storeparentFrame and the listener in class fields
– onUnloaded (lines 146–153): callparent.off(FRAME_ENTRY_LOADED_EVENT, listener)
Suggested diff:
diff --git a/packages/core/ui/frame/frame-common.ts b/packages/core/ui/frame/frame-common.tsindex …--- a/packages/core/ui/frame/frame-common.ts+++ b/packages/core/ui/frame/frame-common.ts@@ class FrameCommon {+ private _parentFrameRef?: WeakRef<FrameCommon>;+ private _frameLoadListener?: () => void; @profile public onLoaded() { super.onLoaded(); const parentFrame = this.page?.frame; if (parentFrame?.isLoadingSubviews) {- const frameRef = new WeakRef(this);- parentFrame.once(FRAME_ENTRY_LOADED_EVENT, () => {- const frame = frameRef.deref();- if (frame) {- frame.onFrameLoaded();- }- });+ this._parentFrameRef = new WeakRef(parentFrame);+ const frameRef = new WeakRef(this);+ this._frameLoadListener = () => {+ const frame = frameRef.deref();+ if (frame) {+ frame.onFrameLoaded();+ }+ };+ parentFrame.once(FRAME_ENTRY_LOADED_EVENT, this._frameLoadListener); } else { this.onFrameLoaded(); }@@ @profile public onUnloaded() { super.onUnloaded();- // This is a precaution in case the method is called asynchronously during the loading procedure- if (this.hasListeners(FRAME_ENTRY_LOADED_EVENT)) {- this.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;+ } }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @profile | |
| publiconLoaded(){ | |
| // Property page refers to the page this frame is nested into | |
| constparentFrame=this.page?.frame; | |
| super.onLoaded(); | |
| if(parentFrame?.isLoadingSubviews){ | |
| constframeRef=newWeakRef(this); | |
| parentFrame.once(FRAME_ENTRY_LOADED_EVENT,()=>{ | |
| constframe=frameRef.deref(); | |
| if(frame){ | |
| frame.onFrameLoaded(); | |
| } | |
| }); | |
| }else{ | |
| this.onFrameLoaded(); | |
| } | |
| } | |
| ---a/packages/core/ui/frame/frame-common.ts | |
| +++b/packages/core/ui/frame/frame-common.ts | |
| @@classFrameCommon{ | |
| +private_parentFrameRef?:WeakRef<FrameCommon>; | |
| +private_frameLoadListener?:()=>void; | |
| @profile | |
| publiconLoaded(){ | |
| -// Property page refers to the page this frame is nested into | |
| -constparentFrame=this.page?.frame; | |
| - | |
| -super.onLoaded(); | |
| +super.onLoaded(); | |
| +// Property page refers to the page this frame is nested into | |
| +constparentFrame=this.page?.frame; | |
| if(parentFrame?.isLoadingSubviews){ | |
| -constframeRef=newWeakRef(this); | |
| - | |
| -parentFrame.once(FRAME_ENTRY_LOADED_EVENT,()=>{ | |
| -constframe=frameRef.deref(); | |
| -if(frame){ | |
| -frame.onFrameLoaded(); | |
| -} | |
| -}); | |
| +this._parentFrameRef=newWeakRef(parentFrame); | |
| +constframeRef=newWeakRef(this); | |
| +this._frameLoadListener=()=>{ | |
| +constframe=frameRef.deref(); | |
| +if(frame){ | |
| +frame.onFrameLoaded(); | |
| +} | |
| +}; | |
| +parentFrame.once(FRAME_ENTRY_LOADED_EVENT,this._frameLoadListener); | |
| }else{ | |
| this.onFrameLoaded(); | |
| } | |
| @@ | |
| @profile | |
| publiconUnloaded(){ | |
| super.onUnloaded(); | |
| - | |
| -// This is a precaution in case the method is called asynchronously during the loading procedure | |
| -if(this.hasListeners(FRAME_ENTRY_LOADED_EVENT)){ | |
| -this.off(FRAME_ENTRY_LOADED_EVENT); | |
| -} | |
| +constparent=this._parentFrameRef?.deref(); | |
| +if(parent&&this._frameLoadListener){ | |
| +parent.off(FRAME_ENTRY_LOADED_EVENT,this._frameLoadListener); | |
| +this._frameLoadListener=undefined; | |
| +this._parentFrameRef=undefined; | |
| +} | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/core/ui/frame/frame-common.ts around lines 124 to 144, the onLoadedmethod adds a once listener to parentFrame without storing references, causing apotential memory leak if the child unloads first. Fix this by saving parentFrameand the listener callback as class fields in onLoaded, then override onUnloaded(around lines 146 to 153) to remove the listener from parentFrame usingparent.off(FRAME_ENTRY_LOADED_EVENT, listener), ensuring proper cleanup of theevent listener.| protected override _notifyFrameEntryLoaded(): void { | ||
| const currentEntry = this._currentEntry || this._executingContext?.entry; | ||
| // Note: This is kept as a precaution and must execute before emitting the frame entry event. | ||
| // There's a bug with nested frames where sometimes the nested fragment is not recreated at all | ||
| // so we manually check on loaded event if the fragment is not recreated and recreate it | ||
| if (currentEntry && !currentEntry.fragment) { | ||
| this._frameCreateTimeout = setTimeout(() => { | ||
| const manager = this._getFragmentManager(); | ||
| const transaction = manager.beginTransaction(); | ||
| currentEntry.fragment = this.createFragment(currentEntry, currentEntry.fragmentTag); | ||
| _updateTransitions(currentEntry); | ||
| transaction.replace(this.containerViewId, currentEntry.fragment, currentEntry.fragmentTag); | ||
| transaction.commitAllowingStateLoss(); | ||
| this._frameCreateTimeout = null; | ||
| }, 0); | ||
| } | ||
| super._notifyFrameEntryLoaded(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Timeout cleanup is broken & may cause leaks
setTimeout returns aNodeJS.Timeout object (on NativeScript’s global typings) –not a number.typeof this._frameCreateTimeout === 'number' will therefore befalse, preventingclearTimeout from running and leaking both the timer and its closure.
Apply the fix below and also clear any pending timeout before scheduling a new one to avoid multiple queued fragment creations:
- if (currentEntry && !currentEntry.fragment) {- this._frameCreateTimeout = setTimeout(() => {+ if (currentEntry && !currentEntry.fragment) {+ if (this._frameCreateTimeout) {+ clearTimeout(this._frameCreateTimeout as any);+ }+ this._frameCreateTimeout = setTimeout(() => { …- this._frameCreateTimeout = null;+ this._frameCreateTimeout = null; }, 0); }
and inonUnloaded:
- if (typeof this._frameCreateTimeout === 'number') {+ if (this._frameCreateTimeout) { clearTimeout(this._frameCreateTimeout as any); this._frameCreateTimeout = null; }
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/core/ui/frame/index.android.ts around lines 248 to 269, the_frameCreateTimeout is assigned the result of setTimeout, which is aNodeJS.Timeout object, not a number. This causes typeof checks against 'number'to fail and prevents clearTimeout from clearing the timer, leading to leaks. Tofix this, ensure _frameCreateTimeout is typed correctly to acceptNodeJS.Timeout, clear any existing timeout before setting a new one to avoidmultiple queued fragment creations, and call clearTimeout properly in onUnloadedto clean up the timer.Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/core/ui/frame/frame-common.ts (2)
125-143:Critical issue: Event listener memory leak persistsThe event-driven coordination logic is sound, but this implementation still has the memory leak issue identified in previous reviews. The listener registered on
parentFrameis 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 cleanupThe cleanup logic has two fundamental problems identified in previous reviews:
parentFrame.off(FRAME_ENTRY_LOADED_EVENT)removes ALL listeners for this event from the parent, not just this frame's listener- The condition
parentFrame.hasListeners(FRAME_ENTRY_LOADED_EVENT)checks the wrong frameApply 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)
🧹 Nitpick comments (1)
packages/core/ui/frame/frame-common.ts (1)
84-84:Implement optional chaining for better safetyThe 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/ui/frame/frame-common.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/ui/frame/frame-common.ts
[error] 84-84: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 153-153: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-android
🔇 Additional comments (5)
packages/core/ui/frame/frame-common.ts (5)
20-21:LGTM: Well-defined event constantThe introduction of
FRAME_ENTRY_LOADED_EVENTconstant provides a clear, centralized definition for the frame coordination event.
81-81:Good improvement: Cleaner type handlingRemoving the explicit type casts from
getAncestorcalls improves code readability and suggests better typing has been implemented upstream.Also applies to: 87-87
158-161:LGTM: Clean separation of concernsThe extraction of frame loading logic into
onFrameLoaded()with clear responsibilities (process navigation and notify) improves code organization and maintainability.
359-365:LGTM: Well-implemented event notificationThe
_notifyFrameEntryLoadedmethod provides a clean, protected interface for emitting the frame coordination event.
367-367:Good improvement: Cleaner type handling in frame ancestryRemoving the explicit type casts from
getAncestorcalls in theisNestedWithinmethod improves code readability and consistency.Also applies to: 370-370
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/ui/frame/frame-common.ts (2)
125-148:Excellent implementation of nested frame coordination.The event-driven approach properly addresses the nested frame loading order issue. The use of weak references prevents memory leaks, and storing the callback reference enables proper cleanup.
Consider a minor improvement to make the callback cleanup more robust:
const callback = () => { const frame = frameRef.deref(); if (frame) { frame.onFrameLoaded(); }-- this._frameEntryLoadedCallback = null; };+this._frameEntryLoadedCallback = callback; parentFrame.once(FRAME_ENTRY_LOADED_EVENT, callback);-this._frameEntryLoadedCallback = callback;This ensures the callback reference is stored before event registration and removes the cleanup from within the callback itself (since
onUnloadedhandles cleanup).
82-95:Type safety improvement with opportunity for optional chaining.The removal of explicit type casts improves type safety. Consider applying the static analysis suggestion to use optional chaining:
-if (parentFrame && parentFrame.canGoBack()) {+if (parentFrame?.canGoBack()) {This simplifies the null check and improves readability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 85-85: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/ui/frame/frame-common.ts(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/ui/frame/frame-common.ts
[error] 85-85: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test-ios
- GitHub Check: test-android
🔇 Additional comments (6)
packages/core/ui/frame/frame-common.ts (6)
20-20:Good addition for event coordination.The new constant provides a centralized name for the frame entry loaded event, which improves maintainability and reduces the risk of typos in event handling.
48-48:Essential field for proper event listener cleanup.This field enables storing the callback reference for proper removal from the parent frame, addressing the memory leak concerns from previous reviews.
150-167:Perfect implementation of event listener cleanup.This method correctly addresses the memory leak concerns from previous reviews by:
- Removing the listener from the parent frame (where it was actually attached)
- Properly nullifying the callback reference
- Following the established pattern for cleanup
169-172:Good separation of concerns.Extracting the frame loading logic into a dedicated method improves code organization and makes the lifecycle flow more explicit.
370-375:Clean event emission implementation.The protected method provides a centralized way to emit the frame entry loaded event and allows for platform-specific overrides if needed.
378-388:Improved type safety with generic ancestor detection.Removing the explicit type casts indicates that
getAncestornow provides proper generic typing, which improves type safety and reduces boilerplate code.
4987961 to417c6d6CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/ui/frame/frame-common.ts (1)
325-329:Tiny clean-up: unnecessary cast onthis
thisis already aFrameBaseinside the class; the manual cast is redundant.
(Optional) tighten the declaration and avoid the extra blank line:-let frameAncestor = this as FrameBase;-+let frameAncestor: FrameBase | null = this;Pure readability; no functional difference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/core/ui/core/view-base/index.ts(1 hunks)packages/core/ui/frame/callbacks/activity-callbacks.ts(0 hunks)packages/core/ui/frame/fragment.transitions.android.ts(7 hunks)packages/core/ui/frame/fragment.transitions.d.ts(2 hunks)packages/core/ui/frame/frame-common.ts(2 hunks)packages/core/ui/frame/index.android.ts(14 hunks)packages/core/ui/frame/index.ios.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/core/ui/frame/callbacks/activity-callbacks.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/ui/frame/fragment.transitions.d.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/ui/core/view-base/index.ts
- packages/core/ui/frame/index.ios.ts
- packages/core/ui/frame/fragment.transitions.android.ts
- packages/core/ui/frame/index.android.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/ui/frame/frame-common.ts
[error] 82-82: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| let parentFrame = getAncestor(top, 'Frame'); | ||
| while (parentFrame && !parentFrameCanGoBack) { | ||
| if (parentFrame && parentFrame.canGoBack()) { | ||
| parentFrameCanGoBack = true; | ||
| } else { | ||
| parentFrame =<FrameBase>getAncestor(parentFrame, 'Frame'); | ||
| parentFrame = getAncestor(parentFrame, 'Frame'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🛠️ Refactor suggestion
UseFrameBase instead of string literal to get proper type-inference
getAncestor is now generic and will only inferFrameBase (and thus exposecanGoBack,goBack, etc.) when the second argument is the class reference, not the string literal.
Passing'Frame' collapses the inferred type toViewBase | undefined, forcing the compiler to fall back toany and silently losing type-safety.
-let parentFrame = getAncestor(top, 'Frame');+let parentFrame: FrameBase | undefined = getAncestor(top, FrameBase);…-parentFrame = getAncestor(parentFrame, 'Frame');+parentFrame = getAncestor(parentFrame, FrameBase);
Besides safer autocompletion, this change will surface misspelled members at compile-time instead of runtime.
(No behavioural impact.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| letparentFrame=getAncestor(top,'Frame'); | |
| while(parentFrame&&!parentFrameCanGoBack){ | |
| if(parentFrame&&parentFrame.canGoBack()){ | |
| parentFrameCanGoBack=true; | |
| }else{ | |
| parentFrame=<FrameBase>getAncestor(parentFrame,'Frame'); | |
| parentFrame=getAncestor(parentFrame,'Frame'); | |
| } | |
| // Before: let parentFrame = getAncestor(top, 'Frame'); | |
| letparentFrame:FrameBase|undefined=getAncestor(top,FrameBase); | |
| while(parentFrame&&!parentFrameCanGoBack){ | |
| if(parentFrame&&parentFrame.canGoBack()){ | |
| parentFrameCanGoBack=true; | |
| }else{ | |
| // Before: parentFrame = getAncestor(parentFrame, 'Frame'); | |
| parentFrame=getAncestor(parentFrame,FrameBase); | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🤖 Prompt for AI Agents
In packages/core/ui/frame/frame-common.ts around lines 79 to 86, replace thestring literal 'Frame' passed to getAncestor with the class reference FrameBaseto enable proper type inference. This change ensures getAncestor returns aFrameBase type, exposing methods like canGoBack with full type safety and betterautocompletion. Update all occurrences of 'Frame' in getAncestor calls withinthis block to FrameBase accordingly.| if (this._isReset && !this._attachedToWindow) { | ||
| return; | ||
| } | ||
| this._frameCreateTimeout = setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@NathanWalker@edusperoni I kept this one in hope of still using it as a precaution but it might no longer be needed. I wonder if there's still a sample we could retry the nested frame bug that the check was intended for.
// there's a bug with nested frames where sometimes the nested fragment is not recreated at all// so we manually check on loaded event if the fragment is not recreated and recreate it
Uh oh!
There was an error while loading.Please reload this page.
PR Checklist
What is the current behavior?
Right now, core has inconsistencies with nested Frames when they get destroyed and reused.
This results in errors like the
No view found for idexception users have been getting lately.The problem is more apparent in flavors like Vue where the entire view tree is being reused after application exit and resume.
What is the new behavior?
This PR ensures attach listener is responsible for creating and attaching entry fragments in the correct order to prevent errors.
Previously, we had a workaround included in#9725 to take care of similar problems but unfortunately the error still persists.
Few things to note:
anywith needed typessubstrcalls withsubstringFixes/Implements/Closes#8126.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes