feat: add preservePage property to navigation bar props and improve n… #482
Conversation
…avigation handling and fixed alignment conversion
WalkthroughAdds optional Changes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (4)
lib/src/framework/utils/flutter_type_converters.dart (1)
441-451: Consider adding symmetric aliases for topLeft/bottomLeft.The new aliases map
topRight→topEndandbottomRight→bottomEnd, but there's no correspondingtopLeft→topStartorbottomLeft→bottomStart. This asymmetry may confuse users expecting consistent behavior.static AlignmentDirectional stackChildAlignment(String? fit) => switch (fit) { 'center' => AlignmentDirectional.center, 'topEnd' || 'topRight' => AlignmentDirectional.topEnd, 'topCenter' => AlignmentDirectional.topCenter, 'centerEnd' => AlignmentDirectional.centerEnd, 'centerStart' => AlignmentDirectional.centerStart, - 'bottomStart' => AlignmentDirectional.bottomStart, + 'bottomStart' || 'bottomLeft' => AlignmentDirectional.bottomStart, 'bottomCenter' => AlignmentDirectional.bottomCenter, 'bottomEnd' || 'bottomRight' => AlignmentDirectional.bottomEnd, - _ => AlignmentDirectional.topStart + 'topStart' || 'topLeft' || _ => AlignmentDirectional.topStart };lib/src/framework/widgets/scaffold.dart (3)
336-347: Consider resetting_currentIndexonly when valid.When the bottom navigation bar changes,
_currentIndexis reset to0. If the new navigation bar has fewer items and the current index is still valid, this could cause an unnecessary state reset.if (oldNav != newNav) { _navItems = _extractNavigationItems(); _pageCache.clear(); - _currentIndex = 0; + if (_currentIndex >= _navItems.length) { + _currentIndex = 0; + } }
408-411: Simplify type access with pattern matching.The conditional access pattern is correct but could be cleaner. Since
_extractNavigationItemsalready filters to specific types, this is safe, but consider using pattern matching for clarity.final item = _navItems[index]; -final onSelect = item is VWNavigationBarItemDefault - ? item.props.onSelect - : (item as VWNavigationBarItemCustom).props.onSelect; +final onSelect = switch (item) { + VWNavigationBarItemDefault(:final props) => props.onSelect, + VWNavigationBarItemCustom(:final props) => props.onSelect, + _ => null, +};
475-482: IndexedStack builds all pages eagerly, defeating lazy loading.When
preserveis true,List.generatecalls_buildPage(i)for every index immediately, constructing all pages on first render. This negates the lazy loading benefit and may cause performance issues with many navigation items or complex pages.Consider building only visited pages:
if (preserve) { return IndexedStack( index: _currentIndex, children: List.generate( _navItems.length, - (i) => _buildPage(i), + (i) => _pageCache.containsKey(i) || i == _currentIndex + ? _buildPage(i) + : const SizedBox.shrink(), ), ); }This ensures only the current page and previously visited pages are built, while maintaining placeholder slots for unvisited pages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/src/framework/utils/flutter_type_converters.dart(1 hunks)lib/src/framework/widget_props/navigation_bar_custom_props.dart(3 hunks)lib/src/framework/widget_props/navigation_bar_props.dart(3 hunks)lib/src/framework/widgets/navigation_bar.dart(0 hunks)lib/src/framework/widgets/navigation_bar_custom.dart(1 hunks)lib/src/framework/widgets/scaffold.dart(2 hunks)
💤 Files with no reviewable changes (1)
- lib/src/framework/widgets/navigation_bar.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-format-and-analyse
🔇 Additional comments (7)
lib/src/framework/widget_props/navigation_bar_custom_props.dart (1)
13-13: LGTM!The
preservePageproperty is added consistently with the existing property patterns and follows the established JSON parsing convention usingExprOr.fromJson<bool>().Also applies to: 27-27, 39-39
lib/src/framework/widgets/navigation_bar_custom.dart (2)
43-47: LGTM!Wrapping each navigation item with
Builderto provide a per-itemBuildContextis the correct pattern for ensuring proper widget tree context propagation. Thepayload.copyWith(buildContext: navItemContext)correctly passes this context to child widgets.
30-32: LGTM!The simplification of
handleDestinationSelectedto just delegate to the callback aligns with the architectural change that moves action execution responsibility to the scaffold level.lib/src/framework/widget_props/navigation_bar_props.dart (1)
15-15: LGTM!The
preservePageproperty addition is consistent withNavigationBarCustomPropsand follows the established property definition and JSON parsing patterns.Also applies to: 31-31, 45-45
lib/src/framework/widgets/scaffold.dart (3)
450-466: LGTM!The navigation handler correctly prioritizes action execution over index updates, and the early return for same-index selection (when no action) prevents unnecessary state updates.
492-524: LGTM!The unified bottom navigation bar builder correctly handles both
VWNavigationBarandVWNavigationBarCustomtypes, propagatingselectedIndexandonDestinationSelectedwhile usingBuilderfor proper context isolation.
546-551: Verify:preservePagenot applied with collapsible app bar.When
isCollapsibleAppBaris true, the scaffold uses_buildCollapsibleAppBarBodyinstead of_buildBody(), bypassing thepreservePagelogic. If this is intentional, consider documenting this limitation.
| int _currentIndex = 0; | ||
|
|
||
| late List<VirtualWidget> _navItems; | ||
| final Map<int, Widget> _pageCache = {}; |
There was a problem hiding this comment.
This doesn't seem right. Widget caching isn't a recommended way of building this feture. Check some other approach.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/src/framework/widgets/scaffold.dart (1)
320-323: Widget caching is not the recommended pattern for preserving page state.Caching
Widgetinstances doesn't preserve widget state—Flutter's framework manages state separately. Consider usingAutomaticKeepAliveClientMixinon child pages orPageStorage/PageStorageKeyfor scroll positions and form data.
🧹 Nitpick comments (1)
lib/src/framework/widgets/scaffold.dart (1)
474-488: IndexedStack eagerly builds all pages, negating lazy loading.When
preserveis true,List.generatebuilds all pages upfront. For navigation bars with many items or expensive pages, this can cause performance issues on first render.Consider lazy-building only visited pages:
Widget _buildBody() { final preserve = _getPreservePageState(); if (preserve) { return IndexedStack( index: _currentIndex, children: List.generate( _navItems.length, - (i) => _buildPage(i), + (i) => _pageCache.containsKey(i) || i == _currentIndex + ? _buildPage(i) + : const SizedBox.shrink(), ), ); } return _buildPage(_currentIndex); }This ensures only visited tabs are built while maintaining IndexedStack's state preservation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/src/framework/widget_props/navigation_bar_custom_props.dart(3 hunks)lib/src/framework/widget_props/navigation_bar_props.dart(3 hunks)lib/src/framework/widgets/scaffold.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/framework/widget_props/navigation_bar_custom_props.dart
- lib/src/framework/widget_props/navigation_bar_props.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-format-and-analyse
🔇 Additional comments (6)
lib/src/framework/widgets/scaffold.dart (6)
303-313: LGTM!Constructor is clean and properly passes all required parameters.
353-373: LGTM!Extraction logic correctly handles both navigation bar types and safely returns an empty list for unrecognized types.
401-429: LGTM!Page building has proper null safety with fallback to
SizedBox.shrink. The cache is bounded by the number of navigation items.
435-446: LGTM!Action detection correctly distinguishes between action-type and entity-type selections.
452-468: LGTM!Handler correctly separates action execution from navigation. The early return optimization when index is unchanged is a good practice.
548-553: Collapsible app bar body bypasses preserve-page logic.When
isCollapsibleAppBaris true, the body uses_buildCollapsibleAppBarBodywhich doesn't utilize the page caching or preserve-page behavior implemented in_buildBody. IfpreservePageshould work with collapsible app bars, this path needs to integrate the same logic.Is this intentional? If collapsible app bars should also support page preservation, the body construction will need adjustment.
| void didUpdateWidget(covariant _ScaffoldWithBottomNav oldWidget) { | ||
| super.didUpdateWidget(oldWidget); | ||
|
|
||
| final oldNav = oldWidget.parent.childOf('bottomNavigationBar'); | ||
| final newNav = widget.parent.childOf('bottomNavigationBar'); | ||
|
|
||
| if (oldNav != newNav) { | ||
| _navItems = _extractNavigationItems(); | ||
| _pageCache.clear(); | ||
| _currentIndex = 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
Reference comparison may not reliably detect navigation bar changes.
Comparing oldNav != newNav relies on object identity. If the parent rebuilds and creates new widget instances with the same configuration, the cache will be unnecessarily cleared. Consider comparing a stable identifier (e.g., a key or hash of navigation item IDs) instead.
Also, unconditionally resetting _currentIndex = 0 when navigation changes could cause unexpected tab jumps for users.
| bool _getPreservePageState() { | ||
| final navBar = widget.parent.childOf('bottomNavigationBar'); | ||
|
|
||
| if (navBar is VWNavigationBar) { | ||
| return navBar.props.rebuildOnEveryLoad | ||
| ?.evaluate(widget.payload.scopeContext) ?? | ||
| false; | ||
| } | ||
|
|
||
| if (navBar is VWNavigationBarCustom) { | ||
| return navBar.props.rebuildOnEveryLoad | ||
| ?.evaluate(widget.payload.scopeContext) ?? | ||
| false; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Semantic inversion: rebuildOnEveryLoad logic appears inverted.
The method _getPreservePageState returns rebuildOnEveryLoad, but the semantics seem backwards:
- If
rebuildOnEveryLoad = true, you'd expect pages to not be preserved (rebuild each time) - If
rebuildOnEveryLoad = false, you'd expect pages to be preserved
However, _buildBody uses preserve = true to enable IndexedStack (which preserves pages). This means when rebuildOnEveryLoad = true, pages are preserved—opposite of what the name suggests.
Either rename the property to preservePageState or invert the logic:
bool _getPreservePageState() {
final navBar = widget.parent.childOf('bottomNavigationBar');
if (navBar is VWNavigationBar) {
- return navBar.props.rebuildOnEveryLoad
+ return !(navBar.props.rebuildOnEveryLoad
?.evaluate(widget.payload.scopeContext) ??
- false;
+ true);
}
// ... similar for VWNavigationBarCustom
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/src/framework/widgets/scaffold.dart around lines 379 to 395, the method
_getPreservePageState currently returns the raw value of rebuildOnEveryLoad
which inverts semantics (true means rebuild, so pages should NOT be preserved);
change the method to return the negation of rebuildOnEveryLoad (i.e., preserve =
!(rebuildOnEveryLoad?.evaluate(...) ?? false)) for both VWNavigationBar and
VWNavigationBarCustom branches so preservePageState correctly reflects that
rebuildOnEveryLoad = true disables preservation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
lib/src/framework/widgets/scaffold.dart (4)
320-333: Widget caching concern already flagged.The
_pageCacheapproach for widget caching has been flagged in a previous review. Widget caching can lead to stale state, incorrect context, or lifecycle issues when the cached widgets don't rebuild with updated data.
336-347: Reference comparison concern already flagged.The concern about
oldNav != newNavusing object identity comparison has been raised in a previous review. This can cause unnecessary cache clears or miss actual changes depending on how parent widgets rebuild.
379-395: Semantic inversion concern already flagged.The inverted semantics between
rebuildOnEveryLoadand_getPreservePageStatehas been raised in a previous review. WhenrebuildOnEveryLoad = true, the current logic preserves pages (via IndexedStack), which is the opposite of what the property name implies.
401-431: Cached pages may use stale BuildContext.Pages are built using
widget.payload.buildContextand then cached. When the widget rebuilds, the cached pages retain references to the old context, which can become invalid and causelooking up a deactivated widget's ancestorerrors.Consider either:
- Not caching widget instances, but caching the data/state needed to build them
- Using
contextfrom the current build method via a Builder patternWidget _buildPage(int index) { - if (_pageCache.containsKey(index)) { - return _pageCache[index]!; - } - if (index >= _navItems.length) { return const SizedBox.shrink(); } final item = _navItems[index]; final onSelect = item is VWNavigationBarItemDefault ? item.props.onSelect : (item as VWNavigationBarItemCustom).props.onSelect; final entity = (onSelect != null && onSelect['type'] == 'loadEntity') ? onSelect['entity'] as JsonLike? : null; final entityId = as$<String?>(entity?['id']); final args = entity?['args'] as JsonLike?; - final page = entityId != null + return entityId != null ? DefaultActionExecutor.of(widget.payload.buildContext).viewBuilder( widget.payload.buildContext, entityId, args, ) : const SizedBox.shrink(); - - _pageCache[index] = page; - return page; }Note: This removes caching entirely. If caching is required for performance, consider caching entity metadata instead and rebuilding widgets each time, or use
AutomaticKeepAliveClientMixinwith aPageView/TabBarViewapproach.
🧹 Nitpick comments (1)
lib/src/framework/widgets/scaffold.dart (1)
476-490: IndexedStack builds all pages upfront.When
preserveis true,IndexedStackinstantiates all page widgets regardless of which tab is selected. With many navigation items or heavy pages, this can cause:
- Slower initial load time
- Increased memory usage
- All pages running their lifecycle simultaneously
Consider lazy initialization where pages are only built when first visited:
Widget _buildBody() { final preserve = _getPreservePageState(); if (preserve) { return IndexedStack( index: _currentIndex, children: List.generate( _navItems.length, - (i) => _buildPage(i), + (i) => _visitedIndices.contains(i) ? _buildPage(i) : const SizedBox.shrink(), ), ); } return _buildPage(_currentIndex); }You'd need to track visited indices and add the current index to
_visitedIndicesinonDestinationSelected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/src/framework/widgets/scaffold.dart(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-format-and-analyse
🔇 Additional comments (5)
lib/src/framework/widgets/scaffold.dart (5)
353-373: LGTM!The navigation item extraction logic correctly handles both
VWNavigationBarandVWNavigationBarCustomtypes with appropriate type filtering.
437-448: LGTM!The action detection logic correctly identifies action-type navigation items and returns
nullfor entity-based navigation.
454-470: LGTM!The tap handler correctly separates action execution from page navigation. Action items execute without changing the selected index, while entity items update the index only when changed.
496-528: LGTM!The navigation bar builder correctly creates new instances with the current index and callback, using a
Builderto ensure fresh context for the navigation bar widget.
550-555: Collapsible app bar bypasses navigation-aware body building.When
isCollapsibleAppBaris true, the body uses_buildCollapsibleAppBarBodyfrom the parent widget, which doesn't use the new_buildBody()logic with preserve-page-state or page caching. This means:
- Tabs won't preserve state when switching in collapsible mode
- The
rebuildOnEveryLoadsetting has no effectIf this is intentional, consider documenting the limitation. Otherwise, the collapsible body should integrate with the navigation-aware body building.
2 participants
…avigation handling and fixed alignment conversion