fix: marker rebuild issue - #482
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a LayoutBuilder with an ObjectKey around the marker series area in the main chart to force targeted rebuilds when markerSeries changes, preventing unnecessary full chart rebuilds and fixing marker-related issues on web. Sequence diagram for markerSeries rebuild with LayoutBuildersequenceDiagram
participant FlutterFramework
participant ChartImplementationState
participant LayoutBuilder
participant MarkerArea
FlutterFramework->>ChartImplementationState: build(context)
ChartImplementationState->>LayoutBuilder: create with ObjectKey(markerSeries)
FlutterFramework->>LayoutBuilder: builder(context, constraints)
alt [markerSeries != null]
LayoutBuilder->>ChartImplementationState: _buildMarkerArea()
ChartImplementationState-->>MarkerArea: build marker widgets
else [markerSeries == null]
LayoutBuilder-->>LayoutBuilder: return SizedBox.shrink()
end
alt [markerSeries changes]
FlutterFramework->>LayoutBuilder: rebuild subtree (ObjectKey changed)
LayoutBuilder->>ChartImplementationState: _buildMarkerArea() or SizedBox.shrink()
else [other chart parts unchanged]
FlutterFramework-->>ChartImplementationState: no full chart rebuild
end
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 encountered an error —— View job I'll analyze this and get back to you. |
![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:
- Using a
LayoutBuildersolely to attach anObjectKeyfeels indirect; consider extracting_buildMarkerAreainto its ownWidgetand giving that widget aKeyinstead so the intent (scoped rebuild of markers) is clearer and avoids an extra layout pass. - Relying on
ObjectKey(widget.markerSeries)assumesmarkerSeries’s identity changes when its contents change; if markerSeries is mutated in place, a more explicitValueKey(e.g. based on a version or hash) might be safer to ensure rebuilds happen when expected.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using a `LayoutBuilder` solely to attach an `ObjectKey` feels indirect; consider extracting `_buildMarkerArea` into its own `Widget` and giving that widget a `Key` instead so the intent (scoped rebuild of markers) is clearer and avoids an extra layout pass.
- Relying on `ObjectKey(widget.markerSeries)` assumes `markerSeries`’s identity changes when its contents change; if markerSeries is mutated in place, a more explicit `ValueKey` (e.g. based on a version or hash) might be safer to ensure rebuilds happen when expected.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
3 similar comments
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
…nto fix-marker-rebuild-issue-scoped
|
Claude finished @behnam-deriv's task in 1m 50s —— View job 🤖 Claude PR Review CompleteModel: SummaryThis PR makes a targeted fix to force marker area rebuilds when Recommendation: REQUEST CHANGES — the fix is conceptually right but uses the wrong Flutter primitive, adding unnecessary layout cost in a high-frequency rendering context. 🟡 Medium Priority Issues🟡 1. LayoutBuilder used solely as a key holder — use KeyedSubtree instead —
|
| Severity | File | Lines |
|---|---|---|
| MEDIUM | lib/src/deriv_chart/chart/main_chart.dart |
429-434 |
❌ Problematic Code:
LayoutBuilder(
key: ObjectKey(widget.markerSeries),
builder: (_, __) => widget.markerSeries != null
? _buildMarkerArea()
: const SizedBox.shrink(),
),📋 Issue: LayoutBuilder triggers a full layout pass and exposes BoxConstraints to its builder. Here the constraints are discarded entirely (_, __), which means the only purpose of LayoutBuilder is to be a container for the ObjectKey. Flutter provides KeyedSubtree specifically for this pattern — it associates a key with a subtree without any layout overhead.
✅ Fix:
KeyedSubtree(
key: ObjectKey(widget.markerSeries),
child: widget.markerSeries != null
? _buildMarkerArea()
: const SizedBox.shrink(),
),💡 Explanation: KeyedSubtree is the canonical Flutter widget for attaching a key to a subtree. It has no layout behavior — it simply forwards the child — so it achieves the same forced-recreate semantics on key change without the extra layout pass. This is the correct primitive here.
🟢 Low Priority Issues
🟢 2. Marker placeholder always occupies a Stack slot when markerSeries is null — lib/src/deriv_chart/chart/main_chart.dart:429-434
Details
| Severity | File | Lines |
|---|---|---|
| LOW | lib/src/deriv_chart/chart/main_chart.dart |
429-434 |
❌ Problematic Code:
LayoutBuilder(
key: ObjectKey(widget.markerSeries),
builder: (_, __) => widget.markerSeries != null
? _buildMarkerArea()
: const SizedBox.shrink(),
),📋 Issue: The previous code used if (widget.markerSeries != null) _buildMarkerArea(), which means when markerSeries is null, no widget was added to the Stack's children at all. The new code always adds a widget (a SizedBox.shrink() placeholder) to the Stack's children list when null. This is a subtle behavioral change — a zero-size widget is still a node in the widget tree that participates in hit testing and layout.
SizedBox.shrink() node in the Stack when markers are not used. The impact is negligible in most cases, but worth being intentional about it.
✅ Fix:
If you still want to key the subtree, you can preserve the if-guard:
if (widget.markerSeries != null)
KeyedSubtree(
key: ObjectKey(widget.markerSeries),
child: _buildMarkerArea(),
),This removes the null branch entirely while still keying the subtree when markers exist.
💡 Explanation: Keeping the if-guard from before means no widget is added when there are no markers, preserving the original behavior. The key on KeyedSubtree still forces recreation when markerSeries changes to a new object.
Summary Table
| Priority | Count | Categories |
|---|---|---|
| 🔴 Critical | 0 | — |
| 🟠 High | 0 | — |
| 🟡 Medium | 1 | Wrong Flutter primitive (LayoutBuilder vs KeyedSubtree) |
| 🟢 Low | 1 | Unnecessary placeholder in Stack when markerSeries is null |
Recommendations
- Replace
LayoutBuilderwithKeyedSubtree(Issue Phase 1 initial #1) — this is a one-line change and the semantically correct Flutter primitive for this use case. - Consider combining the
if-guard withKeyedSubtree(Issue Ramin/animate_to_current_tick #2) to preserve the original "no widget in Stack when null" behavior. - The
ObjectKey(widget.markerSeries)strategy is correct as long asmarkerSeriesis replaced with a new object (not mutated in place) when its data changes — the existing immutability convention in this codebase supports this, so no change needed there.
Auto Fix Claude Reviews
| Action | Open Dashboard |
|---|
🤖 Claude PR Review CompleteModel: SummaryThis PR makes a targeted fix to force marker area rebuilds when Recommendation: REQUEST CHANGES — the fix is conceptually right but uses the wrong Flutter primitive, adding unnecessary layout cost in a high-frequency rendering context. 🟡 Medium Priority Issues🟡 1. LayoutBuilder used solely as a key holder — use KeyedSubtree instead —
|
| Severity | File | Lines |
|---|---|---|
| MEDIUM | lib/src/deriv_chart/chart/main_chart.dart |
429-434 |
❌ Problematic Code:
LayoutBuilder(
key: ObjectKey(widget.markerSeries),
builder: (_, __) => widget.markerSeries != null
? _buildMarkerArea()
: const SizedBox.shrink(),
),📋 Issue: LayoutBuilder triggers a full layout pass and exposes BoxConstraints to its builder. Here the constraints are discarded entirely (_, __), which means the only purpose of LayoutBuilder is to be a container for the ObjectKey. Flutter provides KeyedSubtree specifically for this pattern — it associates a key with a subtree without any layout overhead.
✅ Fix:
KeyedSubtree(
key: ObjectKey(widget.markerSeries),
child: widget.markerSeries != null
? _buildMarkerArea()
: const SizedBox.shrink(),
),💡 Explanation: KeyedSubtree is the canonical Flutter widget for attaching a key to a subtree. It has no layout behavior — it simply forwards the child — so it achieves the same forced-recreate semantics on key change without the extra layout pass. This is the correct primitive here.
🟢 Low Priority Issues
🟢 2. Marker placeholder always occupies a Stack slot when markerSeries is null — lib/src/deriv_chart/chart/main_chart.dart:429-434
Details
| Severity | File | Lines |
|---|---|---|
| LOW | lib/src/deriv_chart/chart/main_chart.dart |
429-434 |
❌ Problematic Code:
LayoutBuilder(
key: ObjectKey(widget.markerSeries),
builder: (_, __) => widget.markerSeries != null
? _buildMarkerArea()
: const SizedBox.shrink(),
),📋 Issue: The previous code used if (widget.markerSeries != null) _buildMarkerArea(), which means when markerSeries is null, no widget was added to the Stack's children at all. The new code always adds a widget (a SizedBox.shrink() placeholder) to the Stack's children list when null. This is a subtle behavioral change — a zero-size widget is still a node in the widget tree that participates in hit testing and layout.
SizedBox.shrink() node in the Stack when markers are not used. The impact is negligible in most cases, but worth being intentional about it.
✅ Fix:
If the keyed subtree fix from Issue #1 is adopted, this concern is moot if you still need the key to be stable. But if markers are never present for some chart instances, the if-guard can be preserved while still using the key:
if (widget.markerSeries != null)
KeyedSubtree(
key: ObjectKey(widget.markerSeries),
child: _buildMarkerArea(),
),This removes the null branch entirely while still keying the subtree when markers exist.
💡 Explanation: Keeping the if-guard from before means no widget is added when there are no markers, preserving the original behavior. The key on KeyedSubtree still forces recreation when markerSeries changes to a new object.
Summary Table
| Priority | Count | Categories |
|---|---|---|
| 🔴 Critical | 0 | — |
| 🟠 High | 0 | — |
| 🟡 Medium | 1 | Wrong Flutter primitive (LayoutBuilder vs KeyedSubtree) |
| 🟢 Low | 1 | Unnecessary placeholder in Stack when markerSeries is null |
Recommendations
- Replace
LayoutBuilderwithKeyedSubtree(Issue Phase 1 initial #1) — this is a one-line change and the semantically correct Flutter primitive for this use case. - Consider combining the
if-guard withKeyedSubtree(Issue Ramin/animate_to_current_tick #2) to preserve the original "no widget in Stack when null" behavior. - The
ObjectKey(widget.markerSeries)strategy is correct as long asmarkerSeriesis replaced with a new object (not mutated in place) when its data changes — the existing immutability convention in this codebase (@immutable/ const constructors) supports this, so no change needed there.
Auto Fix Claude Reviews
| Action | Open Dashboard |
|---|
1 participant
Clickup link:
Fixes issue: #
This PR contains the following changes:
This fixes the marker rebuild issue in deriv mobile app , similar to approach in this branch in main_chart.dart , but with a targetted and scoped solution that only affects markers and avoid exccessive rebuild of the whole chart which was causing drawing tools feature to not function properly on web.
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
Bug Fixes: