fix: marker series issue rebuild#476
Conversation
Reviewer's GuideForces the chart widget tree to rebuild when the marker series changes by keying both the LayoutBuilder and the marker area subtree, and updates intl dependencies to 0.20.2 (including an override for the example app) for compatibility with newer Flutter versions. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Claude finished @ahrar-deriv's task —— View job PR Review: fix: marker series issue rebuildI've reviewed this PR that fixes the marker series rebuild issue. Here's my comprehensive analysis: SummaryApproach: ✅ APPROVED with minor suggestions The fix correctly addresses the marker persistence issue after trade-type switches by using Detailed Review🟢 High Priority - Correctness1. Key-based remounting strategy (main_chart.dart:406, 587-588) ✅ Good approach - Using
Why this works:
Comment on implementation: The double-keying (both 🟡 Medium Priority - Code Quality2. Dependency update (pubspec.yaml:20, example/pubspec.yaml:31) ✅ Acceptable - Updating
Suggestion: Add a note in the PR description about why this 🟢 Low Priority - Edge Cases & Testing3. Null marker series handling ✅ Safe - The code already has null-safety checks:
4. Test coverage
Recommendation:
🔵 Performance Considerations5. Rebuild efficiency ✅ Acceptable - The key-based approach triggers full rebuilds, but:
6. GestureManager callback lifecycle (marker_area.dart:49, 55) ✅ Proper cleanup -
Specific Code Commentslib/src/deriv_chart/chart/main_chart.dart:402-406@override
Widget build(BuildContext context) => LayoutBuilder(
// Force remount when markerSeries changes.
key: ObjectKey(widget.markerSeries),✅ Excellent documentation in the comment. Consider mentioning this is for preventing gesture callback issues: // Force remount when markerSeries changes.
// This ensures MarkerArea's initState() re-runs, properly re-registering
// gesture callbacks and preventing stale markers after trade-type switches.lib/src/deriv_chart/chart/main_chart.dart:587-603The Current: Widget _buildMarkerArea() => KeyedSubtree(
key: ObjectKey(widget.markerSeries),
child: MultipleAnimatedBuilder(
// ...
),
);Alternative (simpler): Widget _buildMarkerArea() => MultipleAnimatedBuilder(
animations: <Listenable>[
currentTickAnimation,
topBoundQuoteAnimationController,
bottomBoundQuoteAnimationController
],
builder: (BuildContext context, _) => MarkerArea(
markerSeries: widget.markerSeries!,
quoteToCanvasY: chartQuoteToCanvasY,
animationInfo: AnimationInfo(
currentTickPercent: currentTickAnimation.value,
),
),
);However, keeping both keys is defensive and won't hurt performance. Security✅ No security concerns - This is a UI rendering fix with no security implications. Checklist ReviewBased on the PR template: Missing items:
Recommendation: Update the PR checklist before merging. RecommendationsMust-have before merge:
Nice-to-have:
Final VerdictStatus: ✅ Approve with minor suggestions This is a well-targeted bug fix with proper documentation. The approach is sound and follows Flutter best practices for forcing widget remounts. The main concerns are around test coverage verification and completing the PR checklist. Priority breakdown:
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
![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 found 1 issue, and left some high level feedback:
- You’re using
ObjectKey(widget.markerSeries)both on theLayoutBuilderand inside_buildMarkerArea; consider whether you really need to force a fullMainChartremount on markerSeries changes or if scoping the key just to the marker subtree is sufficient to avoid unnecessary rebuild cost. - If
markerSerieshas a stable identifier (e.g. an ID or value-based equality), switching fromObjectKeyto aValueKeybased on that identifier would make the remount behavior more predictable and less tied to object identity. - Now that the root
pubspec.yamlhasintl: ^0.20.2, verify whether thedependency_overridesforintlin the example app is still needed, as keeping it may hide version conflicts that would otherwise surface.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You’re using `ObjectKey(widget.markerSeries)` both on the `LayoutBuilder` and inside `_buildMarkerArea`; consider whether you really need to force a full `MainChart` remount on markerSeries changes or if scoping the key just to the marker subtree is sufficient to avoid unnecessary rebuild cost.
- If `markerSeries` has a stable identifier (e.g. an ID or value-based equality), switching from `ObjectKey` to a `ValueKey` based on that identifier would make the remount behavior more predictable and less tied to object identity.
- Now that the root `pubspec.yaml` has `intl: ^0.20.2`, verify whether the `dependency_overrides` for `intl` in the example app is still needed, as keeping it may hide version conflicts that would otherwise surface.
## Individual Comments
### Comment 1
<location path="lib/src/deriv_chart/chart/main_chart.dart" line_range="587-589" />
<code_context>
- quoteToCanvasY: chartQuoteToCanvasY,
- animationInfo: AnimationInfo(
- currentTickPercent: currentTickAnimation.value,
+ Widget _buildMarkerArea() => KeyedSubtree(
+ key: ObjectKey(widget.markerSeries),
+ child: MultipleAnimatedBuilder(
+ animations: <Listenable>[
+ currentTickAnimation,
</code_context>
<issue_to_address>
**suggestion:** Re-evaluate the need for both LayoutBuilder key and KeyedSubtree keyed by markerSeries.
Both the `LayoutBuilder` (in `build`) and this `KeyedSubtree` use `ObjectKey(widget.markerSeries)`, causing the same remount trigger at two levels. This can cause unnecessary state resets higher in the tree and makes remount behavior harder to understand. Consider keying only the minimal subtree that truly depends on `markerSeries` (likely this `MarkerArea`/`KeyedSubtree`) and leaving the outer `LayoutBuilder` unkeyed unless you explicitly need to rebuild the entire layout on `markerSeries` changes.
Suggested implementation:
```
return LayoutBuilder(
builder: (BuildContext context, BoxConstraints constraints) {
```
- The `_buildMarkerArea` method that wraps the marker area in a `KeyedSubtree(key: ObjectKey(widget.markerSeries), ...)` should remain as it is; that subtree is now solely responsible for remounting on `markerSeries` changes.
- If the exact `LayoutBuilder` snippet differs (extra parameters, different formatting), apply the same change conceptually: remove only the `key: ObjectKey(widget.markerSeries),` argument from the `LayoutBuilder` constructor, keeping the rest of the arguments and formatting intact.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| Widget _buildMarkerArea() => KeyedSubtree( | ||
| key: ObjectKey(widget.markerSeries), | ||
| child: MultipleAnimatedBuilder( |
There was a problem hiding this comment.
suggestion: Re-evaluate the need for both LayoutBuilder key and KeyedSubtree keyed by markerSeries.
Both the LayoutBuilder (in build) and this KeyedSubtree use ObjectKey(widget.markerSeries), causing the same remount trigger at two levels. This can cause unnecessary state resets higher in the tree and makes remount behavior harder to understand. Consider keying only the minimal subtree that truly depends on markerSeries (likely this MarkerArea/KeyedSubtree) and leaving the outer LayoutBuilder unkeyed unless you explicitly need to rebuild the entire layout on markerSeries changes.
Suggested implementation:
return LayoutBuilder(
builder: (BuildContext context, BoxConstraints constraints) {
- The
_buildMarkerAreamethod that wraps the marker area in aKeyedSubtree(key: ObjectKey(widget.markerSeries), ...)should remain as it is; that subtree is now solely responsible for remounting onmarkerSerieschanges. - If the exact
LayoutBuildersnippet differs (extra parameters, different formatting), apply the same change conceptually: remove only thekey: ObjectKey(widget.markerSeries),argument from theLayoutBuilderconstructor, keeping the rest of the arguments and formatting intact.
|
Claude encountered an error —— View job PR Review: fix: marker series issue rebuild
|
|
Claude finished @ahrar-deriv's task —— View job PR Review: fix: marker series issue rebuildStarting review of the marker rebuild fix... Tasks:
SummaryThis PR fixes a marker persistence bug where markers weren't properly updating after trade-type switches. The core fix adds Current state: The intl dependency changes mentioned in the PR description were reverted in commit 4082970, so only the marker fix remains in this PR. High Priority Issues1. Redundant Double-Keying (main_chart.dart:406, 588)Issue: The PR adds
Problem: When
Why it still works: Sourcery-ai's recommendation is correct: Keep ONLY the Suggested fix: @override
Widget build(BuildContext context) => LayoutBuilder(
// Remove the key from here
builder: (BuildContext context, BoxConstraints constraints) {
// ... rest of the codeThe Medium Priority Issues2. ObjectKey vs ValueKey ConsiderationObservation: The fix uses Potential issue: If Question: Does If yes: Consider using If no: 3. Missing Test CoverageIssue: No tests found for marker series rebuilding behavior. Risk: Without tests, this bug could regress in future refactorings. Recommended test scenarios: testWidgets('MarkerArea remounts when markerSeries changes', (tester) async {
// 1. Render chart with markerSeries A
// 2. Switch to markerSeries B
// 3. Verify old markers don't persist
// 4. Verify gesture callbacks work correctly with new series
});
testWidgets('MarkerArea handles null markerSeries', (tester) async {
// Verify ObjectKey(null) works correctly
});Low Priority / Informational4. Comment Quality ✅The comment at lines 402-405 is excellent: // Force remount when markerSeries changes.
// LayoutBuilder only re-invokes its builder on constraint changes;
// without a key that tracks markerSeries identity, the old
// MarkerArea subtree persists after trade-type switches.Suggestion: If keeping only the 5. Null Safety ✅The code properly handles
6. Performance Impact ✅Rebuild cost: The remount only triggers when Optimization: Using only the 7. Intl Dependency Changes ✅ (No longer in PR)The intl version bump from ^0.19.0 to ^0.20.2 mentioned in the PR description was reverted in commit 4082970, so the current PR only contains the marker fix. The PR description should be updated to reflect this. Detailed Code AnalysisMarkerArea Lifecycle (marker_area.dart:47-56)The fix correctly addresses this lifecycle issue: Problem scenario:
How the fix works: // In initState:
gestureManager = context.read<GestureManagerState>()
..registerCallback(_onTap); // line 49-50
// In dispose:
gestureManager.removeCallback(_onTap); // line 55When
RecommendationsMust Address Before Merge:
Should Address:
Nice to Have:
Final VerdictStatus: ✅ Approve with changes recommended The fix correctly solves the marker persistence bug, but the implementation can be improved by removing the redundant outer key. The core approach of using keyed widgets to force remounting is sound Flutter practice. Priority Breakdown:
The PR is functionally correct but could be more idiomatic. The suggested changes improve code quality without changing the fix's effectiveness. |
2 participants
Clickup link:
Fixes issue: #
This PR contains the following changes:
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
Ensure chart marker areas fully rebuild when marker series change and update intl dependencies for compatibility with latest Flutter.
Bug Fixes:
Enhancements:
Build: