Fix/jaco/fix right click menu bug#107
Conversation
📝 WalkthroughWalkthroughThis PR introduces granular control over right-click behavior by adding an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/flites/lib/ui/panel/controls/panel_list_item.dart (1)
46-67:⚠️ Potential issue | 🟡 Minor
copyWithis missing theenableRightClickparameter.The new
enableRightClickfield is not included in thecopyWithmethod. IfcopyWithis called on aPanelListItemwithenableRightClick: false, the returned copy will revert totrue(the default), potentially re-enabling right-click unintentionally.🔧 Proposed fix to include enableRightClick in copyWith
PanelListItem copyWith({ bool? hoverSelected, bool? isSelected, VoidCallback? onTap, List<IconBtn> Function({required bool isHovered, required bool isActive})? actionButtons, String? title, String? subtitle, FlitesImage? image, String? value, IconData? icon, + bool? enableRightClick, }) => PanelListItem( title: title ?? this.title, subtitle: subtitle ?? this.subtitle, image: image ?? this.image, actionButtons: actionButtons ?? this.actionButtons, isSelected: isSelected ?? this.isSelected, onTap: onTap ?? this.onTap, hoverSelected: hoverSelected ?? this.hoverSelected, value: value ?? this.value, icon: icon ?? this.icon, + enableRightClick: enableRightClick ?? this.enableRightClick, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/flites/lib/ui/panel/controls/panel_list_item.dart` around lines 46 - 67, The copyWith implementation for PanelListItem omits the enableRightClick field, causing copies to revert to the default value; add a nullable parameter bool? enableRightClick to the PanelListItem.copyWith signature and pass it through to the PanelListItem constructor as enableRightClick: enableRightClick ?? this.enableRightClick so the original setting is preserved when not overridden.
🧹 Nitpick comments (1)
apps/flites/lib/widgets/right_click_menu/right_click_menu_handler.dart (1)
10-16: Stale documentation: comments no longer match behavior.The doc comment describes this widget as listening for right-click events and showing a context menu, but after this change,
build()simply returns the child and no longer intercepts right-clicks. Consider updating the documentation to reflect that this widget now serves as an ancestor state holder forRightClickableItemWrapperto access viafindAncestorStateOfType.Also applies to: 152-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/flites/lib/widgets/right_click_menu/right_click_menu_handler.dart` around lines 10 - 16, Update the stale doc comment on the RightClickMenuHandler class and its build() method to reflect current behavior: it no longer intercepts right-clicks or shows context menus but instead acts as an ancestor StatefulWidget/state holder that RightClickableItemWrapper obtains via findAncestorStateOfType to coordinate context-menu actions; also update any duplicate comment associated with RightClickMenuHandler (the second doc block) to the same description so docs match the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/flites/lib/ui/panel/controls/panel_list_item.dart`:
- Around line 46-67: The copyWith implementation for PanelListItem omits the
enableRightClick field, causing copies to revert to the default value; add a
nullable parameter bool? enableRightClick to the PanelListItem.copyWith
signature and pass it through to the PanelListItem constructor as
enableRightClick: enableRightClick ?? this.enableRightClick so the original
setting is preserved when not overridden.
---
Nitpick comments:
In `@apps/flites/lib/widgets/right_click_menu/right_click_menu_handler.dart`:
- Around line 10-16: Update the stale doc comment on the RightClickMenuHandler
class and its build() method to reflect current behavior: it no longer
intercepts right-clicks or shows context menus but instead acts as an ancestor
StatefulWidget/state holder that RightClickableItemWrapper obtains via
findAncestorStateOfType to coordinate context-menu actions; also update any
duplicate comment associated with RightClickMenuHandler (the second doc block)
to the same description so docs match the implementation.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/flites/lib/ui/panel/controls/panel_list_item.dartapps/flites/lib/widgets/project_file_list/widgets/main_frame_list.dartapps/flites/lib/widgets/right_click_menu/right_click_menu_handler.dartapps/flites/lib/widgets/right_click_menu/right_clickable_item_wrapper.dart
BenAuerDev
left a comment
BenAuerDev
left a comment
There was a problem hiding this comment.
LGTM, please check if the enableRightClick should be passed to copyWith and maybe update the doc comment
| }) => PanelListItem( | ||
| title: title ?? this.title, | ||
| subtitle: subtitle ?? this.subtitle, | ||
| image: image ?? this.image, | ||
| actionButtons: actionButtons ?? this.actionButtons, | ||
| isSelected: isSelected ?? this.isSelected, | ||
| onTap: onTap ?? this.onTap, | ||
| hoverSelected: hoverSelected ?? this.hoverSelected, | ||
| value: value ?? this.value, | ||
| icon: icon ?? this.icon, | ||
| ); |
There was a problem hiding this comment.
should enableRightClick be included in the copyWIth?
| }) => PanelListItem( | |
| title: title ?? this.title, | |
| subtitle: subtitle ?? this.subtitle, | |
| image: image ?? this.image, | |
| actionButtons: actionButtons ?? this.actionButtons, | |
| isSelected: isSelected ?? this.isSelected, | |
| onTap: onTap ?? this.onTap, | |
| hoverSelected: hoverSelected ?? this.hoverSelected, | |
| value: value ?? this.value, | |
| icon: icon ?? this.icon, | |
| ); | |
| }) => PanelListItem( | |
| title: title ?? this.title, | |
| subtitle: subtitle ?? this.subtitle, | |
| image: image ?? this.image, | |
| actionButtons: actionButtons ?? this.actionButtons, | |
| isSelected: isSelected ?? this.isSelected, | |
| onTap: onTap ?? this.onTap, | |
| hoverSelected: hoverSelected ?? this.hoverSelected, | |
| value: value ?? this.value, | |
| icon: icon ?? this.icon, | |
| enableRightClick: enableRightClick ?? this.enableRightClick, | |
| ); |
|
|
||
| /// A widget that wraps around the whole application to handle | ||
| /// right-click context menus. | ||
| /// It listens for right-click events and shows a context menu with the following options: |
There was a problem hiding this comment.
This line in the doc needs to be updated, since now only wrapped items listen for right-click events
2 participants
Description
Fixes the bug where sometimes right-click menus would be impossible to dismiss, causing the app to break until restart.
I believe the issue was that both the menu handler AND the item wrapper would add an overlay in the corresponding gesture detectors, and the overlay state in the handler would end up to be null but not removed.
My changes don't wrap the whole app in a right click menu - which didn't do anything anyways, and instead relies on the specific wrapped items (the only place where it is actionable. Fixes the bug and improves UX.
Type of Change
Summary by CodeRabbit
Features
Improvements