chore: add reset to normal state method for interactive layer#480
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a programmatic API on the interactive layer behaviour to reset the interactive state back to the normal (unselected) state without requiring the widget to be mounted, by directly updating the controller’s currentState when needed. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
|
Claude finished @behnam-deriv's task —— View job Code Review: Add reset to normal state method for interactive layerI've reviewed the PR and analyzed the implementation. Here's my feedback: SummaryThis PR adds a FindingsHigh Priority1. Missing notification to listeners after state change Location: Issue: The void resetToNormalState() {
if (currentState is! InteractiveNormalState) {
_controller.currentState =
InteractiveNormalState(interactiveLayerBehaviour: this);
}
}Why this matters: Widgets listening to the controller won't be notified of the state change, potentially causing UI inconsistencies. Recommendation: Use the public setter instead of directly accessing the private field: void resetToNormalState() {
if (currentState is! InteractiveNormalState) {
_controller.currentState =
InteractiveNormalState(interactiveLayerBehaviour: this);
}
}Wait - looking at the code again, you're already using Medium Priority2. Missing test coverage Issue: No unit tests were added for the new Why this matters: This is a public API that will be used by consumer applications. Tests would help ensure:
Recommendation: Add unit tests covering these scenarios. Example test cases: test('resetToNormalState transitions from selected state to normal state', () { ... });
test('resetToNormalState transitions from adding state to normal state', () { ... });
test('resetToNormalState is no-op when already in normal state', () { ... });
test('resetToNormalState works before initialization', () { ... });3. Potential cleanup needed for AddingToolState Location: Issue: When transitioning from Why this matters: If a user is mid-way through adding a drawing tool and the app calls Recommendation: Consider calling Low Priority4. Documentation could be more specific about the use case Issue: The doc comment mentions "Safe to call before the layer has mounted" but doesn't explain why this is important or when you'd actually want to do this. Why this matters: Better documentation helps consumers understand when and how to use this method. Recommendation: Enhance the documentation to include a concrete example: /// Resets the interactive layer to [InteractiveNormalState], dropping any
/// current selection or in-progress adding state.
///
/// This is useful when reloading the chart or switching symbols while a
/// drawing tool is active. For example:
///
/// ```dart
/// // Before reloading chart data
/// interactiveLayerBehaviour.resetToNormalState();
/// reloadChartData();
/// ```
///
/// Safe to call before the layer has mounted — bypasses [updateStateTo]
/// which requires [interactiveLayer] to be initialised.
/// No-op if already in normal state.5. Consider adding a callback for state changes Issue: The method silently changes state without any callback mechanism. Consumer apps might want to know when this reset occurs to update their UI accordingly (e.g., deselecting a tool button in the toolbar). Why this matters: While the Recommendation: Consider adding an optional callback parameter or ensuring the existing Code Quality✅ Follows Dart conventions: PascalCase for class names, camelCase for methods RecommendationApprove with suggestions:
The implementation itself is clean and functional. The main concern is the lack of test coverage and potential edge case with |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Bypassing
updateStateToand assigning_controller.currentStatedirectly may skip any cleanup or side effects normally performed during state transitions; consider reusingupdateStateTowhen the layer is mounted and only falling back to direct assignment when it is not. - The doc comment says this is safe to call before the layer has mounted, but
_controllerislate final; ensure_controlleris always initialized beforeresetToNormalStatecan be called, or guard the method so it no-ops if the controller/state is not yet ready.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Bypassing `updateStateTo` and assigning `_controller.currentState` directly may skip any cleanup or side effects normally performed during state transitions; consider reusing `updateStateTo` when the layer is mounted and only falling back to direct assignment when it is not.
- The doc comment says this is safe to call before the layer has mounted, but `_controller` is `late final`; ensure `_controller` is always initialized before `resetToNormalState` can be called, or guard the method so it no-ops if the controller/state is not yet ready.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1 participant
Clickup link:
Fixes issue: #
This PR contains the following changes:
Added a
resetToNormalStatemethod to the interactive layer. This allows consumer apps to programmatically reset drawing tools from a selected state back to normal. For example, if a user reloads the chart while a drawing tool is active, the consumer app can call this method to ensure all tools are unselected upon reload.✨ New feature (non-breaking change which adds functionality)
🛠️ Bug fix (non-breaking change which fixes an issue)
❌ Breaking change (fix or feature that would cause existing functionality to change)
🧹 Code refactor
✅ Build configuration change
📝 Documentation
🗑️ Chore
Developers Note (Optional)
Pre-launch Checklist (For PR creator)
As a creator of this PR:
Reviewers
Pre-launch Checklist (For Reviewers)
As a reviewer I ensure that:
Pre-launch Checklist (For QA)
Pre-launch Checklist (For Maintainer)
Summary by Sourcery
New Features: