Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughNavigation calls switched from navigate to pushNamed; several slot screens became stateful and set title-bar trailing widgets. New widgets (MappingWidget, SlotDataPopOver, SlotsViewSwitcher) and exports added. Repositories gained getById/query and extra dependencies. Layout tweaks (greedy(), flexible, removed carousels), date normalization, and a dropdown border were applied. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Switcher as SlotsViewSwitcher
participant UserRepo as UserRepository
participant Router as Modular.to
User->>Switcher: Open dropdown
Switcher->>UserRepo: getCapabilities()
Switcher-->>User: Show options (icons, labels)
User->>Switcher: Select capability
Switcher->>Router: pushNamed(selectedCapability.slotRoute)
sequenceDiagram
participant UI as Slot UI
participant SlotsRepo as SlotMasterSlotsRepository
participant Courses as SlotMasterCoursesRepository
participant Users as UsersRepository
UI->>SlotsRepo: query("text", {slots?})
alt slots provided
SlotsRepo-->>SlotsRepo: use provided slots
else no slots
SlotsRepo-->>SlotsRepo: use current state data
end
SlotsRepo->>Courses: getById(...) / course names
SlotsRepo->>Users: supervisor names
SlotsRepo-->>UI: filtered list of slots
sequenceDiagram
actor User
participant Item as SlotDataPopOver
participant Pop as Popover
User->>Item: Hover / Click
alt Multiple items
Item->>Pop: showPopover(remaining items)
User-->>Pop: Hover content
Pop-->>Item: Close on delayed mouseout if not hovered
else Single item
Item-->>User: Render primary only
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
Analysis Report for db9b314
Click to see the full report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
lib/src/app/presentation/widgets/title_bar.dart (1)
371-371: Verify the intended navigation behavior for the back button.Same concern as in the desktop build path (line 218): the back button uses
pushNamed(_parentRoute!), which may not be the intended behavior for a back action. Please verify whether pushing or replacing is the correct navigation pattern here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
lib/main.dart(1 hunks)lib/src/app/presentation/widgets/title_bar.dart(3 hunks)lib/src/app/utils/no_mobile_utils.dart(1 hunks)lib/src/course_overview/presentation/screens/course_overview_screen.dart(1 hunks)lib/src/course_overview/presentation/screens/courses_overview_screen.dart(2 hunks)lib/src/moodle/presentation/widgets/user_widget.dart(2 hunks)lib/src/slots/domain/models/slot.dart(1 hunks)lib/src/slots/presentation/repositories/slot_master_courses_repository.dart(1 hunks)lib/src/slots/presentation/repositories/slot_master_slots_repository.dart(3 hunks)lib/src/slots/presentation/screens/slot_details_screen.dart(5 hunks)lib/src/slots/presentation/screens/slot_master_screen.dart(3 hunks)lib/src/slots/presentation/screens/slot_overview_screen.dart(2 hunks)lib/src/slots/presentation/screens/slot_reservation_screen.dart(4 hunks)lib/src/slots/presentation/widgets/mapping_widget.dart(1 hunks)lib/src/slots/presentation/widgets/slot_data_pop_over.dart(1 hunks)lib/src/slots/presentation/widgets/slot_master_widget.dart(3 hunks)lib/src/slots/presentation/widgets/slot_overview_widget.dart(2 hunks)lib/src/slots/presentation/widgets/slot_widget.dart(1 hunks)lib/src/slots/presentation/widgets/slots_view_switcher.dart(1 hunks)lib/src/slots/presentation/widgets/widgets.dart(1 hunks)lib/src/theming/infra/services/material_theme_generator_service.dart(1 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). (2)
- GitHub Check: Testing 🧪
- GitHub Check: Analysis 🔍
🔇 Additional comments (11)
lib/src/course_overview/presentation/screens/courses_overview_screen.dart (1)
42-55: Verify the layout behavior withclipBehavior: Clip.noneandgreedy().The combination of
clipBehavior: Clip.noneon theSingleChildScrollViewand the.greedy()extension on the outerPaddingis unusual:
Clip.noneallows content to render outside the scroll view's bounds, which can cause visual overflow issues if child widgets extend beyond the viewport..greedy()forces thePaddingto consume all available space from its parent, which may conflict withSingleChildScrollView's intrinsic content sizing, potentially leading to layout warnings or unexpected scroll behavior.Please verify that this combination produces the intended layout without overflow or scroll issues across different screen sizes and content amounts. If this is a workaround for a specific layout problem, consider documenting the reasoning with a comment.
lib/src/slots/presentation/widgets/slot_widget.dart (1)
286-289: LGTM! Parameter rename aligns with UserWidget API update.The parameter name change from
expand: truetoflexible: truecorrectly reflects the updatedUserWidgetAPI. This change maintains the same layout behavior while using the new parameter name.lib/src/course_overview/presentation/screens/course_overview_screen.dart (1)
76-166: Verify the layout behavior withgreedy().The
.greedy()extension forces the outerPaddingto consume all available space. While this may be intentional to ensure theCardandDataTablefill the screen, verify that this doesn't cause layout issues on smaller screens or when the table content is minimal.lib/src/slots/presentation/screens/slot_master_screen.dart (2)
44-46: LGTM! Trailing widget enhancement improves UI flexibility.Setting the trailing widget to
SlotsViewSwitcherindidChangeDependenciesenhances the title bar with dynamic controls, aligning with the broader pattern of stateful title bar customization introduced in this PR.
93-136: LGTM! Refactoring filtering logic to the repository improves separation of concerns.The updated
slotTimeTablesignature now acceptsSlotMasterSlotsRepository repoand delegates filtering torepo.query(...)instead of performing inline filtering. This refactor:
- Centralizes filtering logic in the repository, making it reusable and testable.
- Reduces code duplication and improves maintainability.
- Aligns with the new repository query capabilities introduced in this PR.
lib/src/slots/presentation/widgets/slot_overview_widget.dart (2)
27-35: LGTM! UsinggetByIdimproves clarity and performance.Switching from
courseRepository.filter(id: m.courseId).firstOrNulltocourseRepository.getById(m.courseId)is a cleaner and more efficient approach for single-object retrieval. The subsequent.where((cv) => cv.$1 != null)filter ensures only valid courses are processed, preventing null reference issues downstream.
75-77: LGTM! Replacing carousel with SlotDataPopOver improves UI consistency.The new
SlotDataPopOverwidget withMappingWidgetprovides a more consistent and reusable approach for displaying slot mappings, aligning with the PR's goal to move away fromCarouselSlider-based presentation. The non-null assertioncourse!is safe because the preceding filter removes null courses.lib/src/app/presentation/widgets/title_bar.dart (3)
218-218: Verify the intended navigation behavior for the back button.The back button uses
pushNamed(_parentRoute!), which pushes a new route onto the navigation stack. Typical back button behavior is to pop the current route or navigate to the parent route without adding to the history. UsingpushNamedmay result in users navigating through multiple instances of the same parent route.If the intent is to replace the current route (standard back button behavior), consider using
Modular.to.navigate(_parentRoute!)(which replaces) orModular.to.pop()with conditional logic. If pushing is intentional, please confirm the reasoning.This is the same concern raised in
lib/src/app/utils/no_mobile_utils.dart(line 22).
318-318: Verify the layout impact of displaying all user capabilities.The change from displaying only the highest capability (
user.capabilities.highest.translate(context)) to displaying all capabilities (user.capabilities.map((c) => c.translate(context)).join(', ')) provides more information to the user, which is generally beneficial. However, if a user has multiple capabilities, the resulting string may become quite long and affect the layout or cause overflow in the title bar.Please verify that the title bar layout handles longer capability strings gracefully across different screen sizes and capability counts. Consider adding overflow handling (e.g.,
TextOverflow.ellipsis) or a tooltip to display the full list if it's too long.
213-213: LGTM! Spacing enhancement improves visual layout.Adding
spacing: Spacing.mediumSpacingto the leading Row improves the horizontal spacing between elements in the title bar, enhancing readability and visual balance.lib/src/app/utils/no_mobile_utils.dart (1)
22-22: Verify "Go Back" navigation behavior (no_mobile_utils.dart:22)
The button label implies returning to the previous screen, but it currently usesModular.to.pushNamed('/dashboard/'), which pushes a new route. If you intended to replace or pop, useModular.to.navigate('/dashboard/')orModular.to.pop(). Otherwise confirm that pushing is desired.
| Text( | ||
| user.fullname, | ||
| style: style, | ||
| overflow: TextOverflow.ellipsis, | ||
| ).flexible(), | ||
| ], |
There was a problem hiding this comment.
Restore optional flex behavior
flexible is never read now—the text is always wrapped in .flexible(), so callers setting flexible: false still get a Flexible child, altering layouts (and regressing previous behavior). Gate the modifier on the flag instead of forcing it.
- Text(
- user.fullname,
- style: style,
- overflow: TextOverflow.ellipsis,
- ).flexible(),
+ final name = Text(
+ user.fullname,
+ style: style,
+ overflow: TextOverflow.ellipsis,
+ );
+
+ return Row(
+ mainAxisSize: MainAxisSize.min,
+ children: [
+ UserProfileImage(
+ userId: user.id,
+ size: size,
+ ),
+ Spacing.smallHorizontal(),
+ flexible ? name.flexible() : name,
+ ],
+ );Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/src/moodle/presentation/widgets/user_widget.dart around lines 32 to 37,
the Text widget is always wrapped with .flexible() which ignores the flexible
flag and alters layouts; change the code to conditionally wrap the Text: if the
widget's flexible flag is true wrap the Text with .flexible(), otherwise return
the plain Text widget so callers passing flexible: false get an unwrapped child.
lib/src/slots/presentation/repositories/slot_master_courses_repository.dart
Show resolved
Hide resolved
| class _SlotOverviewScreenState extends State<SlotOverviewScreen> with AdaptiveState, NoMobile { | ||
| @override | ||
| void didChangeDependencies() { | ||
| super.didChangeDependencies(); | ||
|
|
||
| Data.of<TitleBarState>(context).setTrailingWidget(const SlotsViewSwitcher()); | ||
| } |
There was a problem hiding this comment.
Clear the TitleBar trailing widget when this screen disposes
We set a SlotsViewSwitcher on the shared TitleBarState, but never clear it. After navigating away, any screen that doesn’t explicitly override the trailing slot inherits the leftover switcher, leading to a stale UI. Cache the TitleBarState and reset the trailing widget (e.g. setTrailingWidget(null) or the dedicated clear API) in dispose() so the header returns to its default state when this screen is popped.
class _SlotOverviewScreenState extends State<SlotOverviewScreen> with AdaptiveState, NoMobile {
+ TitleBarState? _titleBarState;
+
@override
void didChangeDependencies() {
super.didChangeDependencies();
-
- Data.of<TitleBarState>(context).setTrailingWidget(const SlotsViewSwitcher());
+ _titleBarState = Data.of<TitleBarState>(context);
+ _titleBarState!.setTrailingWidget(const SlotsViewSwitcher());
}
+
+ @override
+ void dispose() {
+ _titleBarState?.setTrailingWidget(null);
+ super.dispose();
+ }
}🤖 Prompt for AI Agents
In lib/src/slots/presentation/screens/slot_overview_screen.dart around lines 22
to 28, the TitleBarState trailing widget is set to SlotsViewSwitcher in
didChangeDependencies but never cleared; cache the TitleBarState instance when
you call Data.of<TitleBarState>(context) and then override dispose() to call the
TitleBarState clear API (e.g. setTrailingWidget(null) or the dedicated clear
method) to reset the header before calling super.dispose().
| final currentRoute = Modular.to.path; | ||
| final capabilities = user.state.data?.capabilities.toList() ?? []; | ||
|
|
||
| if (capabilities.length <= 1) return const SizedBox.shrink(); | ||
|
|
||
| return DropdownMenu<UserCapability>( | ||
| inputDecorationTheme: context.theme.dropdownMenuTheme.inputDecorationTheme | ||
| ?.copyWith(fillColor: context.theme.cardColor, contentPadding: PaddingHorizontal(Spacing.smallSpacing)), | ||
| dropdownMenuEntries: [ | ||
| for (final r in capabilities) | ||
| DropdownMenuEntry( | ||
| value: r, | ||
| label: r.translateSlotRoute(context), | ||
| labelWidget: Text(r.translateSlotRoute(context)), | ||
| leadingIcon: Icon(r.slotIcon, size: 16), | ||
| ), | ||
| ], | ||
|
|
||
| trailingIcon: const Icon( | ||
| FontAwesome5Solid.chevron_down, | ||
| size: 13, | ||
| ), | ||
| leadingIcon: Icon( | ||
| SlotUserCapabilityX.capabilityFromRoute(currentRoute).slotIcon, | ||
| size: 16, | ||
| ), | ||
| requestFocusOnTap: false, // disable text input | ||
| initialSelection: SlotUserCapabilityX.capabilityFromRoute(currentRoute), | ||
| onSelected: (r) { |
There was a problem hiding this comment.
Guard against unmatched current routes
SlotUserCapabilityX.capabilityFromRoute throws when the current path is empty or '/'. Modular.to.path is known to briefly expose those values (see _preventEmptyRoute in AppWidget), so the switcher can crash while building. Capture the capability via a nullable lookup and bail out early when it’s not available before wiring it into leadingIcon/initialSelection.
- final currentRoute = Modular.to.path;
- final capabilities = user.state.data?.capabilities.toList() ?? [];
-
- if (capabilities.length <= 1) return const SizedBox.shrink();
+ final currentRoute = Modular.to.path;
+ final capabilities = user.state.data?.capabilities.toList() ?? [];
+ final currentCapability = SlotUserCapabilityX.maybeCapabilityFromRoute(currentRoute);
+
+ if (capabilities.length <= 1 || currentCapability == null) return const SizedBox.shrink();
...
- leadingIcon: Icon(
- SlotUserCapabilityX.capabilityFromRoute(currentRoute).slotIcon,
+ leadingIcon: Icon(
+ currentCapability.slotIcon,
size: 16,
),
...
- initialSelection: SlotUserCapabilityX.capabilityFromRoute(currentRoute),
+ initialSelection: currentCapability,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/src/slots/presentation/widgets/slots_view_switcher.dart around lines 20
to 48, calling SlotUserCapabilityX.capabilityFromRoute(currentRoute) can throw
for empty or '/' paths; obtain the capability via a nullable-safe lookup (or
wrap the call in a try/catch returning null) into a local variable, and if that
variable is null return SizedBox.shrink() before building the DropdownMenu so
you don't wire a thrown value into leadingIcon or initialSelection; then use the
safely-obtained non-null capability for leadingIcon and initialSelection.
| /// | ||
| /// If no matching capability is found this throws. | ||
| static UserCapability capabilityFromRoute(String route) { | ||
| if (route.startsWith(UserCapability.teacher.slotRoute)) { | ||
| return UserCapability.teacher; | ||
| } | ||
| if (route.startsWith(UserCapability.student.slotRoute)) { | ||
| return UserCapability.student; | ||
| } | ||
| if (route.startsWith(UserCapability.slotMaster.slotRoute)) { | ||
| return UserCapability.slotMaster; | ||
| } | ||
|
|
||
| throw ArgumentError.value(route, 'route', 'Unkown route'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Expose a safe route→capability lookup
Provide a non-throwing helper so the widget can avoid crashing when the path is temporarily empty or unrecognised, and have the throwing variant delegate to it. Also fix the typo in the error message.
+ /// Returns the [UserCapability] linked to the given [route] or `null` if none matches.
+ static UserCapability? maybeCapabilityFromRoute(String route) {
+ if (route.startsWith(UserCapability.teacher.slotRoute)) {
+ return UserCapability.teacher;
+ }
+ if (route.startsWith(UserCapability.student.slotRoute)) {
+ return UserCapability.student;
+ }
+ if (route.startsWith(UserCapability.slotMaster.slotRoute)) {
+ return UserCapability.slotMaster;
+ }
+
+ return null;
+ }
+
/// Returns the [UserCapability] linked to the given [route].
///
/// If no matching capability is found this throws.
static UserCapability capabilityFromRoute(String route) {
- if (route.startsWith(UserCapability.teacher.slotRoute)) {
- return UserCapability.teacher;
- }
- if (route.startsWith(UserCapability.student.slotRoute)) {
- return UserCapability.student;
- }
- if (route.startsWith(UserCapability.slotMaster.slotRoute)) {
- return UserCapability.slotMaster;
- }
-
- throw ArgumentError.value(route, 'route', 'Unkown route');
+ return maybeCapabilityFromRoute(route) ?? (throw ArgumentError.value(route, 'route', 'Unknown route'));
}📝 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.
| /// | |
| /// If no matching capability is found this throws. | |
| static UserCapability capabilityFromRoute(String route) { | |
| if (route.startsWith(UserCapability.teacher.slotRoute)) { | |
| return UserCapability.teacher; | |
| } | |
| if (route.startsWith(UserCapability.student.slotRoute)) { | |
| return UserCapability.student; | |
| } | |
| if (route.startsWith(UserCapability.slotMaster.slotRoute)) { | |
| return UserCapability.slotMaster; | |
| } | |
| throw ArgumentError.value(route, 'route', 'Unkown route'); | |
| } | |
| /// Returns the [UserCapability] linked to the given [route] or `null` if none matches. | |
| static UserCapability? maybeCapabilityFromRoute(String route) { | |
| if (route.startsWith(UserCapability.teacher.slotRoute)) { | |
| return UserCapability.teacher; | |
| } | |
| if (route.startsWith(UserCapability.student.slotRoute)) { | |
| return UserCapability.student; | |
| } | |
| if (route.startsWith(UserCapability.slotMaster.slotRoute)) { | |
| return UserCapability.slotMaster; | |
| } | |
| return null; | |
| } | |
| /// Returns the [UserCapability] linked to the given [route]. | |
| /// | |
| /// If no matching capability is found this throws. | |
| static UserCapability capabilityFromRoute(String route) { | |
| return maybeCapabilityFromRoute(route) | |
| ?? (throw ArgumentError.value(route, 'route', 'Unknown route')); | |
| } |
🤖 Prompt for AI Agents
In lib/src/slots/presentation/widgets/slots_view_switcher.dart around lines 70
to 85, add a non-throwing helper (e.g. UserCapability?
tryCapabilityFromRoute(String? route)) that returns null for
null/empty/unrecognised routes and keeps the same prefix checks, then change the
existing static capabilityFromRoute to call that helper and throw
ArgumentError.value(...) only if the helper returned null; also fix the typo in
the error message from 'Unkown route' to 'Unknown route'.
MasterMarcoHD
force-pushed
the
lpa-43RR
branch
from
db9b314 to
e96b07d
Compare
Analysis Report for e96b07d
Click to see the full report |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
lib/src/course_overview/presentation/screens/course_overview_screen.dart(1 hunks)lib/src/course_overview/presentation/screens/courses_overview_screen.dart(2 hunks)lib/src/slots/presentation/widgets/slot_data_pop_over.dart(1 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). (2)
- GitHub Check: Analysis 🔍
- GitHub Check: Testing 🧪
🔇 Additional comments (3)
lib/src/course_overview/presentation/screens/course_overview_screen.dart (1)
165-165: LGTM! Layout expansion applied correctly.The
.greedy()extension (fromawesome_extensions) makes thePaddingwidget expand to fill all available space in its parent, which is appropriate for a desktop layout where the course overview table should occupy the full viewport. This change preserves all existing functionality while improving the layout behavior.lib/src/course_overview/presentation/screens/courses_overview_screen.dart (2)
43-43: Clarify the need forClip.noneon the scroll view.Setting
clipBehavior: Clip.noneon aSingleChildScrollViewallows child content to render outside the scroll viewport, which is unusual and can cause visual artifacts. Course cards may render outside the intended scroll area and interfere with parent layout constraints.Unless there's a specific requirement for content to overflow the scroll bounds (e.g., shadows, tooltips, or popovers that need to escape), the default
Clip.hardEdgeis typically preferred for scroll views to ensure content is properly clipped.Please clarify:
- Is there a specific visual element (shadow, popover, tooltip) that requires rendering outside the scroll bounds?
- Have you tested this on different screen sizes to ensure no unintended overflow occurs?
If overflow is not required, consider removing this property to use the default clipping behavior.
55-55: LGTM! Appropriate use of.greedy()for main content.The
.greedy()extension (fromawesome_extensions) makes the scroll view expand to fill available space, which is the correct pattern for a main content area. This ensures the course overview takes up the full viewport even when content is minimal.Based on learnings.
| void closePopUp({bool forceClose = false}) { | ||
| if (popupContext == null) return; | ||
|
|
||
| if ((parentHover || childHover) && !forceClose) return; | ||
|
|
||
| Navigator.of(popupContext!).pop(); | ||
|
|
||
| popupContext = null; | ||
| } | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| return HoverBuilder( | ||
| cursor: SystemMouseCursors.basic, | ||
| builder: (context, isHovering) { | ||
| parentHover = isHovering; | ||
| Future.delayed(const Duration(milliseconds: 300), closePopUp); | ||
| if (widget.contentList.isEmpty) { | ||
| closePopUp(forceClose: true); | ||
| return const SizedBox.shrink(); | ||
| } | ||
| return Row( | ||
| children: [ | ||
| Expanded( | ||
| child: widget.contentList[0], | ||
| ), | ||
| if (widget.contentList.length > 1) ...[ | ||
| Spacing.xsHorizontal(), | ||
| GestureDetector( | ||
| // ignore: sort_child_properties_last | ||
| child: Chip( | ||
| label: Text('+${widget.contentList.length - 1}'), | ||
| ), | ||
| onTap: widget.contentList.length > 1 | ||
| ? () { | ||
| if (popupContext != null) { | ||
| closePopUp(forceClose: true); | ||
| return; | ||
| } | ||
| showPopover( | ||
| context: context, | ||
| bodyBuilder: (context) { | ||
| popupContext = context; | ||
| return HoverBuilder( | ||
| cursor: SystemMouseCursors.basic, | ||
| builder: (context, isHovering) { | ||
| childHover = isHovering; | ||
| Future.delayed(const Duration(milliseconds: 300), closePopUp); | ||
| return Card( | ||
| elevation: 16, | ||
| shape: squircle(side: BorderSide(color: context.theme.dividerColor)), | ||
| color: context.theme.cardColor, | ||
| child: SingleChildScrollView( | ||
| child: Padding( | ||
| padding: const EdgeInsets.all(8), | ||
| child: Column( | ||
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: widget.contentList.spaced(Spacing.smallSpacing), | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| }, | ||
| ); | ||
| }, | ||
| arrowHeight: 0, | ||
| arrowWidth: 0, | ||
| allowClicksOnBackground: true, | ||
| direction: PopoverDirection.bottom, | ||
| transition: PopoverTransition.other, | ||
| // contentDxOffset: -width + (context.size?.width ?? 0), | ||
| barrierDismissible: true, | ||
| barrierColor: Colors.transparent, | ||
| backgroundColor: Colors.transparent, | ||
| transitionDuration: const Duration(milliseconds: 300), | ||
| popoverTransitionBuilder: (animation, child) { | ||
| return FadeTransition( | ||
| opacity: animation, | ||
| child: SlideTransition( | ||
| position: animation.drive( | ||
| Tween<Offset>( | ||
| begin: const Offset(0, -0.02), | ||
| end: Offset.zero, | ||
| ), | ||
| ), | ||
| child: child, | ||
| ), | ||
| ); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Handle popover lifecycle when dismissed externally
When the popover is closed via the barrier/back navigation, popupContext stays set. The next closePopUp (from the delayed timer or the guard on tap) runs against a deactivated context, throwing FlutterError: Looking up a deactivated widget's ancestor… and preventing the popover from opening again. Reset the reference once the route completes and guard against unmounted contexts before calling Navigator.pop().
void closePopUp({bool forceClose = false}) {
- if (popupContext == null) return;
+ final localPopupContext = popupContext;
+ if (localPopupContext == null) return;
+
+ if (localPopupContext is Element && !localPopupContext.mounted) {
+ popupContext = null;
+ return;
+ }
- if ((parentHover || childHover) && !forceClose) return;
+ if ((parentHover || childHover) && !forceClose) return;
- Navigator.of(popupContext!).pop();
+ Navigator.of(localPopupContext).pop();
popupContext = null;
}
@@
- showPopover(
+ showPopover(
context: context,
bodyBuilder: (context) {
popupContext = context;
@@
- );
+ ).whenComplete(() {
+ popupContext = null;
+ });
}
: null,
),📝 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.
| void closePopUp({bool forceClose = false}) { | |
| if (popupContext == null) return; | |
| if ((parentHover || childHover) && !forceClose) return; | |
| Navigator.of(popupContext!).pop(); | |
| popupContext = null; | |
| } | |
| @override | |
| Widget build(BuildContext context) { | |
| return HoverBuilder( | |
| cursor: SystemMouseCursors.basic, | |
| builder: (context, isHovering) { | |
| parentHover = isHovering; | |
| Future.delayed(const Duration(milliseconds: 300), closePopUp); | |
| if (widget.contentList.isEmpty) { | |
| closePopUp(forceClose: true); | |
| return const SizedBox.shrink(); | |
| } | |
| return Row( | |
| children: [ | |
| Expanded( | |
| child: widget.contentList[0], | |
| ), | |
| if (widget.contentList.length > 1) ...[ | |
| Spacing.xsHorizontal(), | |
| GestureDetector( | |
| // ignore: sort_child_properties_last | |
| child: Chip( | |
| label: Text('+${widget.contentList.length - 1}'), | |
| ), | |
| onTap: widget.contentList.length > 1 | |
| ? () { | |
| if (popupContext != null) { | |
| closePopUp(forceClose: true); | |
| return; | |
| } | |
| showPopover( | |
| context: context, | |
| bodyBuilder: (context) { | |
| popupContext = context; | |
| return HoverBuilder( | |
| cursor: SystemMouseCursors.basic, | |
| builder: (context, isHovering) { | |
| childHover = isHovering; | |
| Future.delayed(const Duration(milliseconds: 300), closePopUp); | |
| return Card( | |
| elevation: 16, | |
| shape: squircle(side: BorderSide(color: context.theme.dividerColor)), | |
| color: context.theme.cardColor, | |
| child: SingleChildScrollView( | |
| child: Padding( | |
| padding: const EdgeInsets.all(8), | |
| child: Column( | |
| crossAxisAlignment: CrossAxisAlignment.start, | |
| children: widget.contentList.spaced(Spacing.smallSpacing), | |
| ), | |
| ), | |
| ), | |
| ); | |
| }, | |
| ); | |
| }, | |
| arrowHeight: 0, | |
| arrowWidth: 0, | |
| allowClicksOnBackground: true, | |
| direction: PopoverDirection.bottom, | |
| transition: PopoverTransition.other, | |
| // contentDxOffset: -width + (context.size?.width ?? 0), | |
| barrierDismissible: true, | |
| barrierColor: Colors.transparent, | |
| backgroundColor: Colors.transparent, | |
| transitionDuration: const Duration(milliseconds: 300), | |
| popoverTransitionBuilder: (animation, child) { | |
| return FadeTransition( | |
| opacity: animation, | |
| child: SlideTransition( | |
| position: animation.drive( | |
| Tween<Offset>( | |
| begin: const Offset(0, -0.02), | |
| end: Offset.zero, | |
| ), | |
| ), | |
| child: child, | |
| ), | |
| ); | |
| }, | |
| ); | |
| // lib/src/slots/presentation/widgets/slot_data_pop_over.dart | |
| void closePopUp({bool forceClose = false}) { | |
| final localPopupContext = popupContext; | |
| if (localPopupContext == null) return; | |
| // If the route was dismissed externally, the context is unmounted – clear it. | |
| if (localPopupContext is Element && !localPopupContext.mounted) { | |
| popupContext = null; | |
| return; | |
| } | |
| if ((parentHover || childHover) && !forceClose) return; | |
| Navigator.of(localPopupContext).pop(); | |
| popupContext = null; | |
| } | |
| @override | |
| Widget build(BuildContext context) { | |
| return HoverBuilder( | |
| cursor: SystemMouseCursors.basic, | |
| builder: (context, isHovering) { | |
| parentHover = isHovering; | |
| Future.delayed(const Duration(milliseconds: 300), closePopUp); | |
| if (widget.contentList.isEmpty) { | |
| closePopUp(forceClose: true); | |
| return const SizedBox.shrink(); | |
| } | |
| return Row( | |
| children: [ | |
| Expanded( | |
| child: widget.contentList[0], | |
| ), | |
| if (widget.contentList.length > 1) ...[ | |
| Spacing.xsHorizontal(), | |
| GestureDetector( | |
| // ignore: sort_child_properties_last | |
| child: Chip( | |
| label: Text('+${widget.contentList.length - 1}'), | |
| ), | |
| onTap: widget.contentList.length > 1 | |
| ? () { | |
| if (popupContext != null) { | |
| closePopUp(forceClose: true); | |
| return; | |
| } | |
| showPopover( | |
| context: context, | |
| bodyBuilder: (context) { | |
| popupContext = context; | |
| return HoverBuilder( | |
| cursor: SystemMouseCursors.basic, | |
| builder: (context, isHovering) { | |
| childHover = isHovering; | |
| Future.delayed(const Duration(milliseconds: 300), closePopUp); | |
| return Card( | |
| elevation: 16, | |
| shape: squircle(side: BorderSide(color: context.theme.dividerColor)), | |
| color: context.theme.cardColor, | |
| child: SingleChildScrollView( | |
| child: Padding( | |
| padding: const EdgeInsets.all(8), | |
| child: Column( | |
| crossAxisAlignment: CrossAxisAlignment.start, | |
| children: widget.contentList.spaced(Spacing.smallSpacing), | |
| ), | |
| ), | |
| ), | |
| ); | |
| }, | |
| ); | |
| }, | |
| ).whenComplete(() { | |
| // Ensure we clear the context when the popover route finishes. | |
| popupContext = null; | |
| }); | |
| } | |
| : null, | |
| ), |
| ); | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Dispose must close any open popover
If this widget is removed from the tree while the popover is open, the overlay route keeps hanging around because nothing calls closePopUp. Cleaning it up in dispose avoids orphaned UI and stale references.
}
+ @override
+ void dispose() {
+ closePopUp(forceClose: true);
+ super.dispose();
+ }
}📝 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.
| ); | |
| }, | |
| ); | |
| } | |
| ); | |
| }, | |
| ); | |
| } | |
| @override | |
| void dispose() { | |
| closePopUp(forceClose: true); | |
| super.dispose(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/src/slots/presentation/widgets/slot_data_pop_over.dart around lines 115
to 118, the widget never closes the popover when removed which leaves the
overlay route orphaned; add an override of dispose that calls closePopUp()
(guarded if necessary, e.g. if the popover/controller is non-null or if mounted
checks are required) before calling super.dispose() so any open popover is
closed and resources cleaned up.
2 participants
No description provided.