Add action-based removal methods for various providers#49
Conversation
- Implemented `removeAbilityAsAction`, `removeAgentAsAction`, `removeImageAsAction`, `removeTextAsAction`, and `removeUtilityAsAction` methods in their respective providers to handle deletions with action logging. - Updated the `deleteHoveredTarget` and `deletePlacedWidget` functions to utilize the new action-based removal methods, enhancing the consistency of deletion operations across the application. - Improved the debug logging system by adding repeat handling and a badge for repeated log entries in the debug dialog. These changes enhance the application's ability to track user actions and improve the overall debugging experience.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR refactors deletion operations across all widget providers by introducing Key changes:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as UI / placed_widget_builder
participant DH as delete_helpers
participant XP as XxxProvider (ability/agent/text/image/utility)
participant AP as ActionProvider
participant LP as LineUpProvider
Note over UI,LP: deleteHoveredTarget / deletePlacedWidget
UI->>DH: deleteHoveredTarget(ref, target)
alt target.type != lineup
DH->>XP: removeXAsAction(id)
XP-->>XP: guard: state.any(id)?
XP->>AP: addAction(UserAction deletion)
XP->>XP: removeX(id)
else target.type == lineup (no guard ⚠️)
DH->>LP: deleteLineUpById(id)
LP->>AP: addAction(UserAction deletion)
LP-->>LP: firstWhere(id) [throws if missing]
LP->>LP: remove from state
end
Note over UI,LP: AppErrorReporter toast deduplication
UI->>AppErrorReporter: reportWarning/reportError(msg)
AppErrorReporter->>AppErrorReporter: addDebugLog → DebugLogEntry
AppErrorReporter->>InAppDebugProvider: addEntry(entry)
InAppDebugProvider-->>InAppDebugProvider: canMergeWith(last)?
alt merge
InAppDebugProvider->>InAppDebugProvider: mergeRepeat → update repeatCount & lastOccurredAt
else new entry
InAppDebugProvider->>InAppDebugProvider: append (cap at 500)
end
AppErrorReporter->>AppErrorReporter: _showInteractiveNotificationIfNeeded
AppErrorReporter-->>AppErrorReporter: cooldown check (3s per dedupeKey)
opt cooldown not active
AppErrorReporter->>UI: Settings.showToast(message)
end
Last reviewed commit: 678c57b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case DeleteTargetType.lineup: | ||
| if (!_lineUpExists(ref, target.id)) return; | ||
| ref.read(lineUpProvider.notifier).deleteLineUpById(target.id); | ||
| return; |
There was a problem hiding this comment.
Missing existence guard for lineup deletion
The old code protected this branch with if (!_lineUpExists(ref, target.id)) return; before calling deleteLineUpById. That guard was removed here, but unlike the other five types, no equivalent check was moved inside deleteLineUpById.
Looking at line_provider.dart line 319, deleteLineUpById calls state.lineUps.firstWhere((lineUp) => lineUp.id == id) with no orElse callback. When a non-existent ID is passed, Dart's firstWhere throws a StateError, which will crash the function call and can bubble up as an unhandled exception in the UI.
All other cases (agent, ability, text, image, utility) moved their guard into their new removeXAsAction methods. The lineup case skipped this pattern, making it inconsistent and unsafe.
| case DeleteTargetType.lineup: | |
| if (!_lineUpExists(ref, target.id)) return; | |
| ref.read(lineUpProvider.notifier).deleteLineUpById(target.id); | |
| return; | |
| case DeleteTargetType.lineup: | |
| if (!state.lineUps.any((l) => l.id == target.id)) return; | |
| ref.read(lineUpProvider.notifier).deleteLineUpById(target.id); | |
| return; |
Or, alternatively, add an early-return guard inside deleteLineUpById itself (parallel to the other providers).
There was a problem hiding this comment.
@greptile The thing is that you can't really remove a lineup by dragging, so it's literally impossible to get into that scenario.
There was a problem hiding this comment.
Fair point—if lineups can't be dragged to delete in the UI, then deleteHoveredTarget won't be called for them through the normal drag-and-drop flow, making the guard unnecessary in practice.
Since this is non-obvious from the code alone, you might consider adding a brief comment to document this architectural constraint:
| case DeleteTargetType.lineup: | |
| if (!_lineUpExists(ref, target.id)) return; | |
| ref.read(lineUpProvider.notifier).deleteLineUpById(target.id); | |
| return; | |
| case DeleteTargetType.lineup: | |
| // Note: lineups cannot be dragged to delete zones, so this case | |
| // only executes in controlled scenarios where existence is guaranteed | |
| ref.read(lineUpProvider.notifier).deleteLineUpById(target.id); |
But if you'd rather keep the code clean without explanatory comments, that works too—just good to know the constraint exists.
lib/services/app_error_reporter.dart
Outdated
| static String _buildNotificationFingerprint({ | ||
| required DebugLogLevel level, | ||
| required String message, | ||
| String? source, | ||
| String? errorText, | ||
| String? stackTrace, | ||
| }) { | ||
| return [ | ||
| level.name, | ||
| _normalizeFingerprintFragment(source), | ||
| _normalizeFingerprintFragment(message), | ||
| _normalizeFingerprintFragment(errorText), | ||
| _normalizeFingerprintFragment(_firstStackTraceLine(stackTrace)), | ||
| ].join('|'); | ||
| } | ||
|
|
||
| static String _normalizeFingerprintFragment(String? value) { | ||
| if (value == null) return ''; | ||
|
|
||
| return value.replaceAll(RegExp(r'\s+'), ' ').trim(); | ||
| } | ||
|
|
||
| static String? _firstStackTraceLine(String? stackTrace) { | ||
| if (stackTrace == null) return null; | ||
|
|
||
| for (final line in stackTrace.split('\n')) { | ||
| final trimmed = line.trim(); | ||
| if (trimmed.isNotEmpty) { | ||
| return trimmed; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Duplicated deduplication key logic
_buildNotificationFingerprint, _normalizeFingerprintFragment, and _firstStackTraceLine here are byte-for-byte copies of _buildDebugLogDedupeKey, _normalizeDebugLogFragment, and _firstStackTraceLine in in_app_debug_provider.dart. Since AppErrorReporter.addDebugLog already constructs a DebugLogEntry (which auto-computes its own dedupeKey using the provider-side helpers), the explicit dedupeKey: argument on lines 117–123 is redundant — the constructor will derive the same key if it's omitted.
Consider removing the three private helpers from AppErrorReporter and dropping the dedupeKey: argument from the DebugLogEntry constructor call. That would eliminate the duplication and guarantee the two code paths can never drift apart if the hashing logic changes.
There was a problem hiding this comment.
Pull request overview
This PR improves UX around strategy tiles and in-app debugging, and standardizes deletion flows by routing removals through provider helpers that record undo/redo actions.
Changes:
- Make strategy tile agent icons responsive to available width via a
LayoutBuilder-driven row. - Deduplicate repeated debug log entries (repeat count + latest occurrence) and rate-limit “Open Logs” toasts.
- Introduce
remove*AsActionhelpers across providers and update deletion call sites to consistently record deletion actions.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/widgets/strategy_tile/strategy_tile_sections.dart | Adds a dynamic agent icon row and refactors layout constants. |
| lib/widgets/draggable_widgets/placed_widget_builder.dart | Switches out-of-bounds removals to remove*AsAction variants. |
| lib/widgets/dialogs/in_app_debug_dialog.dart | Displays repeat badge + latest occurrence for deduped log entries. |
| lib/widgets/delete_helpers.dart | Simplifies deletion helpers to call provider remove*AsAction. |
| lib/services/app_error_reporter.dart | Returns DebugLogEntry from logging, adds toast dedupe/rate limiting. |
| lib/providers/utility_provider.dart | Adds removeUtilityAsAction helper. |
| lib/providers/text_provider.dart | Adds removeTextAsAction helper. |
| lib/providers/in_app_debug_provider.dart | Adds log dedupe key + repeat merge behavior. |
| lib/providers/image_provider.dart | Adds removeImageAsAction helper. |
| lib/providers/agent_provider.dart | Adds removeAgentAsAction helper and uses it for OOB removal. |
| lib/providers/ability_provider.dart | Adds removeAbilityAsAction helper and uses it for OOB removal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
lib/services/app_error_reporter.dart
Outdated
| dedupeKey: _buildNotificationFingerprint( | ||
| level: level, | ||
| message: message, | ||
| source: source, | ||
| errorText: errorText, | ||
| stackTrace: stackTrace?.toString(), | ||
| ), |
| child: Row( | ||
| child: Column( | ||
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| // mainAxisAlignment: MainAxisAlignment.spaceBetween, |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…n and associated helper methods. This simplifies error reporting and reduces code complexity, enhancing maintainability.
removeAbilityAsAction,removeAgentAsAction,removeImageAsAction,removeTextAsAction, andremoveUtilityAsActionmethods in their respective providers to handle deletions with action logging.deleteHoveredTargetanddeletePlacedWidgetfunctions to utilize the new action-based removal methods, enhancing the consistency of deletion operations across the application.These changes enhance the application's ability to track user actions and improve the overall debugging experience.