fix(navigation): preserve parent region during HR to prevent orphaned regions #3000
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the navigation region system during Hot Reload where regions could become orphaned when their parent cannot be found in the visual tree. The fix preserves a reference to the previous parent region and restores it if a new parent cannot be located during reassignment.
Key Changes:
- Introduced a
_previousParentfield to track the last known parent region - Refactored
ReassignParent()method with early-return guards and improved logic for Hot Reload scenarios - Added logging for various reassignment scenarios to aid debugging
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
|
|
||
| if (parent is not null) | ||
| { | ||
| Name = routeName; |
There was a problem hiding this comment.
How come we needed to add this? Is it necessary?
There was a problem hiding this comment.
This line existed before but was outside the parent check, I moved it inside if (parent is not null)
As the old code set Name = null unconditionally, which broke region identity and caused the test failures
| if (View is null || _isRoot || !View.GetAttached()) | ||
| { | ||
| if (_logger.IsEnabled(LogLevel.Trace)) | ||
| { | ||
| _logger.LogTraceMessage($"(Name: {Name}) Cannot reassign parent: View is null ({View is null}), IsRoot ({_isRoot}), or not attached ({!(View?.GetAttached() ?? false)})"); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
This is changing the logic since we would no longer set the Parent to null if we fall into this if because it returns early. Are we sure that's what we want to do?
There was a problem hiding this comment.
Yes, intentional
Original code did Parent = null then AssignParent(), which orphaned regions even when AssignParent would fail (View null/root/not attached)
The early return prevents unnecessary orphaning when we know reassignment won't work
if (View is null || _isRoot || !View.GetAttached())
return;
IRegion? fallbackParent = null;
_previousParent?.TryGetTarget(out fallbackParent);
AssignParent(fallbackParent); // If FindParentRegion fails, does Parent = fallbackParent
|
|
GitHub Issue (If applicable): closes #2912
PR Type
What kind of change does this PR introduce?
What is the current behavior?
What is the new behavior?
PR Checklist
Please check if your PR fulfills the following requirements:
Screenshots Compare Test Runresults.