View your CI Pipeline Execution ↗ for commit b6b35f3.

☁️ Nx Cloud last updated this comment at 2025-05-26 00:13:47 UTC

@NathanWalker This should be good to go. Let me say that theoritically, this PR doesn't completely solve the problem because we have some setCurrent calls wrapped inside setTimeout and that can also mess the order of execution for frames.
Regardless, it should fix the suspend/resume problem mentioned in the issue.

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

#!/bin/bash
# Get PR details and information
gh pr view 10713 --json title,body,state,isDraft,files,additions,deletions,headRefName,baseRefName

# Get the diff for this PR to understand the changes
gh pr diff 10713

# Look for any related issues mentioned
gh pr view 10713 --json body | jq -r '.body' | grep -i -E '(fix|close|resolve|issue|#[0-9]+)'

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 & Solution

The 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

1. Proper Loading Sequencing: The addition of isLoadingSubviews tracking and event coordination ensures parent frames complete their setup before child frames begin processing.

2. Code Organization: Moving transition state management (_getTransitionState, _restoreTransitionState) to fragment.transitions.android.ts improves separation of concerns.

3. Type Safety: Excellent improvements replacing any types with proper BackstackEntry types.

⚠️ Areas of Concern

1. Race Conditions: The event-driven approach could introduce timing issues:

// In frame-common.ts
if (parentFrame && parentFrame.isLoadingSubviews) {
    parentFrame.once('frameEntryLoaded', () => {
        this.onFrameLoaded();
    });
}

Concern: What if parentFrame.isLoadingSubviews becomes false between the check and event registration?

2. setTimeout Dependencies: As you mentioned, there are still setTimeout calls that could affect execution order:

// In index.android.ts
this._frameCreateTimeout = setTimeout(() => {
    // Fragment recreation logic
}, 0);

3. Memory Management: Event listeners need proper cleanup to prevent memory leaks.

🔧 Specific Code Issues

1. Null Safety Enhancement Needed:

// Consider adding null check
const parentFrame = this.page?.frame;
if (parentFrame?.isLoadingSubviews) {
    // ...
}

2. Event Cleanup: The once listener should be cleaned up if the frame is destroyed before the event fires.

3. Error Handling: Missing error handling for fragment creation failures.

🧪 Testing Observations

The new test test_frame_entry_loaded_order() is good but consider adding:

• Tests for error scenarios (fragment creation failures)
• Tests for rapid suspend/resume cycles
• Tests for deeply nested frame hierarchies
• Tests for concurrent navigation in nested frames

📋 Recommendations

1. Add Timeout Safety:

let timeoutId: number;
const cleanup = () => {
    if (timeoutId) clearTimeout(timeoutId);
    // Remove any pending listeners
};

if (parentFrame?.isLoadingSubviews) {
    timeoutId = setTimeout(() => {
        // Fallback if event never fires
        this.onFrameLoaded();
    }, 5000);
    
    parentFrame.once('frameEntryLoaded', () => {
        cleanup();
        this.onFrameLoaded();
    });
}

2. Consider Using Promises for better async coordination instead of events.

3. Add Debug Logging to help diagnose timing issues in production.

🎯 Overall Assessment

This is a solid architectural improvement that addresses a real pain point. The event-driven approach is conceptually correct, but needs additional safeguards for robustness. The code quality improvements (type safety, organization) are excellent.

Recommendation: Address the race condition concerns and add timeout safeguards before moving out of draft status. Consider adding more comprehensive error handling for edge cases.

The PR is on the right track but needs these reliability improvements to handle the complex timing scenarios that can occur with nested frames in production applications.