fix(core): rebuild nested-frame fragment when its view was destroyed on reattach#11214
fix(core): rebuild nested-frame fragment when its view was destroyed on reattach#11214dangrima90 wants to merge 1 commit into
Conversation
…on reattach Extends the _ensureEntryFragment recovery from NativeScript#10713 to cover the case where the fragment object survives but its view has been destroyed. Without this, the nested frame renders blank after the parent is briefly covered and dismissed. Also guards entry.fragment = null in onDestroy so the old fragment's teardown doesn't overwrite the new reference assigned by the rebuild path.
|
View your CI Pipeline Execution ↗ for commit 5df98bd
☁️ Nx Cloud last updated this comment at |
|
I paste the video indicating the problem here: |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes a bug where a nested frame renders blank after its parent is briefly covered and dismissed by rebuilding the entry fragment when its view has been destroyed during reattach.
Changes:
- Extend
_ensureEntryFragmentto rebuild whenfragment.getView() == null, gated by a new_pendingFragmentCheckflag set inonLoaded()to distinguish incoming reattach from outgoing transitions. - Guard
entry.fragment = nullinonDestroyso it only nulls when the entry still references the destroying fragment, preserving any newer fragment assigned by the rebuild path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/core/ui/frame/index.android.ts | Adds _pendingFragmentCheck flag and broadens fragment-rebuild condition to cover destroyed-view case. |
| packages/core/ui/frame/frame-helper-for-android.ts | Only clears entry.fragment in onDestroy if it still references the destroying fragment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughAndroid Frame navigation recovery during nested transitions now includes two guarded checks: fragment cleanup only nullifies stale references, and fragment rebuild only occurs during ChangesAndroid Fragment Navigation Recovery
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/ui/frame/index.android.ts`:
- Around line 87-92: The delayed callback in _ensureEntryFragment reads the
instance field _pendingFragmentCheck which may change before the timeout fires;
capture the current value into a local constant (e.g., const pendingAtSchedule =
this._pendingFragmentCheck) immediately before scheduling the timeout and use
that local within the timeout to decide whether to rebuild the fragment; update
every similar scheduling site (the calls around lines with
_ensureEntryFragment/_onAttachedToWindow handling and the other occurrences you
noted) to use the captured local instead of reading this._pendingFragmentCheck
inside the async callback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3082298b-167d-4c3c-86fe-4f4b1bd88f5e
📒 Files selected for processing (2)
packages/core/ui/frame/frame-helper-for-android.tspackages/core/ui/frame/index.android.ts
| // True while inside an onLoaded()-driven _ensureEntryFragment call. _ensureEntryFragment | ||
| // also runs from _onAttachedToWindow during outgoing navigations, where the fragment's | ||
| // view is null because it's being torn down on purpose — we must not rebuild it then. | ||
| // This flag lets _ensureEntryFragment tell the two cases apart. | ||
| private _pendingFragmentCheck = false; | ||
|
|
There was a problem hiding this comment.
Capture the rebuild-origin flag before scheduling the timeout.
_pendingFragmentCheck is read inside the delayed callback, but it can be changed by later lifecycle calls before that callback runs. That can incorrectly enable/disable fragment rebuilds across overlapping _ensureEntryFragment() invocations.
Proposed fix
onUnloaded() {
super.onUnloaded();
+ this._pendingFragmentCheck = false;
if (typeof this._frameCreateTimeout === 'number') {
clearTimeout(this._frameCreateTimeout);
this._frameCreateTimeout = null;
}
}
private _ensureEntryFragment(): void {
+ const shouldRebuildMissingView = this._pendingFragmentCheck;
+ this._pendingFragmentCheck = false;
+
// in case the activity is "reset" using resetRootView or disposed we must wait for
// the attachedToWindow event to make the first navigation or it will crash
// https://github.com/NativeScript/NativeScript/commit/9dd3e1a8076e5022e411f2f2eeba34aabc68d112
// though we should not do it on app "start"
// or it will create a "flash" to activity background color
if (this._isReset && !this._attachedToWindow) {
- this._pendingFragmentCheck = false;
return;
}
this._frameCreateTimeout = setTimeout(() => {
// 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
const currentEntry = this._currentEntry || this._executingContext?.entry;
if (currentEntry) {
const fragmentNeedsRebuild =
- !currentEntry.fragment || (currentEntry.fragment.getView() == null && this._pendingFragmentCheck);
+ !currentEntry.fragment || (currentEntry.fragment.getView() == null && shouldRebuildMissingView);
if (fragmentNeedsRebuild) {
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._pendingFragmentCheck = false;
this._frameCreateTimeout = null;
}, 0);
}Also applies to: 254-257, 287-288, 291-313
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/ui/frame/index.android.ts` around lines 87 - 92, The delayed
callback in _ensureEntryFragment reads the instance field _pendingFragmentCheck
which may change before the timeout fires; capture the current value into a
local constant (e.g., const pendingAtSchedule = this._pendingFragmentCheck)
immediately before scheduling the timeout and use that local within the timeout
to decide whether to rebuild the fragment; update every similar scheduling site
(the calls around lines with _ensureEntryFragment/_onAttachedToWindow handling
and the other occurrences you noted) to use the captured local instead of
reading this._pendingFragmentCheck inside the async callback.
farfromrefug
left a comment
farfromrefug
left a comment
There was a problem hiding this comment.
It seems fine to me. I am not sure about code rabbit proposed changr though. I am having a hard time wrapping my head around the flow
The author and I are doing a few last tests to see if an old PR has caused the regression. |
PR Checklist
What is the current behavior?
As described on discord I have encountered an issue where a nested frame results in having no content. As stated I have investigated this via claude, not mentioning it to not take any responsibility 😄 but to highlight that code changes and comments in the code were added by claude. I did direct it to simplify certain comments.
As I mentioned on discord I did try replicating this issue in a clean application but I didn't manage. My suspicion was that there's was minor issue in NativeScript that's being triggered by as specific use case in my application.
What is the new behavior?
Short description via claude of this update:
The update here extends the
_ensureEntryFragmentrecovery from #10713 to cover the case where the fragment object survives but its view has been destroyed. Without this, the nested frame renders blank after the parent is briefly covered and dismissed. Also guards entry.fragment = null in onDestroy so the old fragment's teardown doesn't overwrite the new reference assigned by the rebuild path.Two changes under
packages/core/ui/frame:index.android.ts—_ensureEntryFragmentalso rebuilds whenfragment.getView() == null, gated by a new_pendingFragmentCheckflag set byonLoaded(). The flag is needed because_onAttachedToWindowalso calls_ensureEntryFragmentduring outgoing transitions, where a null view is expected and a rebuild would be wrong.frame-helper-for-android.ts—onDestroyonly nulls outentry.fragmentif it still points to the destroying fragment. The rebuild above assigns a new fragment toentry.fragment, and without this guard the old fragment's destruction would overwrite that new reference.