fix: drawing tools sync issue in mobile app#484
Conversation
Reviewer's GuideRefactors the interactive drawing layer to keep the painted drawings, interaction state machine, and drawing tools repository strictly in sync across remounts and repo changes, adds guarded animation/state transitions, wires a live drawings notifier into the gesture handler, and introduces diagnostics plus regression tests for the previously flaky mobile drawing-tools flows. Sequence diagram for guarded interactive state transitionssequenceDiagram
participant Behaviour as InteractiveLayerBehaviour
participant Layer as _InteractiveLayerGestureHandlerState
participant Controller as AnimationController
Behaviour->>Behaviour: updateStateTo(newState, direction, waitForAnimation, animate)
Behaviour->>Behaviour: increment _stateTransitionSeq
alt waitForAnimation
Behaviour->>Layer: animateStateChange(direction, animate)
opt [layer is not mounted]
Layer-->>Behaviour: return (no animation)
end
alt direction == forward
Layer->>Controller: reset()
alt animate
Layer->>Controller: forward().orCancel
Controller-->>Layer: TickerFuture completes
else [animate == false]
Layer->>Controller: value = 1.0
end
else direction == reverse
alt animate
Layer->>Controller: reverse().orCancel
Controller-->>Layer: TickerFuture completes
else [animate == false]
Layer->>Controller: value = 0.0
end
end
Layer-->>Behaviour: animation completed
alt [transition superseded]
Behaviour-->>Behaviour: return (drop stale transition)
else [still current]
Behaviour->>Behaviour: _controller.currentState = newState
Behaviour->>Behaviour: onUpdate()
end
else [waitForAnimation == false]
Behaviour->>Layer: animateStateChange(direction, animate) (unawaited)
Behaviour->>Behaviour: _controller.currentState = newState
Behaviour->>Behaviour: onUpdate()
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Claude finished @behnam-deriv's task in 2m 19s —— View job 🤖 Claude PR Review CompleteModel: Summary1 of 3 issues from the previous review have been resolved. The latest commit addressed two regressions in Recommendation: REQUEST CHANGES 🟠 High Priority Issues🟠 1. Silent exception swallowing with debug-only diagnostics in release builds —
|
| Severity | File | Lines |
|---|---|---|
| HIGH | lib/src/deriv_chart/interactive_layer/interactive_layer.dart |
208-218 |
❌ Problematic Code:
try {
_reconcileDrawings();
} on Object catch (error, stackTrace) {
// Diagnostics: a throw between the map mutation and setState would
// leave the painted drawings stale with no visible error in release.
// Log it and still rebuild from whatever state the map is in.
if (kChartDiagnosticsEnabled) {
chartDiag('layer#$hashCode reconcile threw: $error\n$stackTrace');
}
}
setState(() {});📋 Issue: kChartDiagnosticsEnabled is kDebugMode, which is false in release builds (a compile-time constant). Any exception thrown inside _reconcileDrawings() — including null-assertion failures on config.configId! for legacy stored configs — is completely silently swallowed in production. The only consequence is broken or frozen drawing state with no crash, no log, and no signal reaching any error-monitoring service.
✅ Fix:
try {
_reconcileDrawings();
} on Object catch (error, stackTrace) {
// Continue with whatever partial state the map is in; a throw mid-mutation
// would otherwise leave painted drawings stale. Report the error so it
// surfaces in production monitoring even though we handle it gracefully.
FlutterError.reportError(FlutterErrorDetails(
exception: error,
stack: stackTrace,
library: 'deriv_chart',
context: ErrorDescription('reconciling drawing tools with repository'),
));
}
setState(() {});💡 Explanation: FlutterError.reportError is visible in all build modes and integrates with Flutter's error reporting pipeline (Crashlytics, Sentry, etc. via FlutterError.onError). It preserves the same graceful-recovery behavior (no rethrow, state rebuilt from whatever the map has) while making the failure observable in production.
🟢 Low Priority Issues
🟢 2. Missing deduplication guard for zero-size post-frame retry in _reconcileDrawings — lib/src/deriv_chart/interactive_layer/interactive_layer.dart:249-260
Details
| Severity | File | Lines |
|---|---|---|
| LOW | lib/src/deriv_chart/interactive_layer/interactive_layer.dart |
249-260 |
❌ Problematic Code:
if (drawingContext.fullSize == Size.zero) {
if (kChartDiagnosticsEnabled) {
chartDiag('layer#$hashCode reconcile skipped: drawing context has zero size...');
}
// Drawing context not ready yet, schedule retry after next frame
WidgetsBinding.instance.addPostFrameCallback((_) {
if (mounted) {
syncDrawingsWithConfigs();
}
});
return;
}📋 Issue: _reconcileDrawings() is called on every build() (line ~467). If drawingContext.fullSize is Size.zero during multiple consecutive builds (rapid rebuilds before first layout), a new addPostFrameCallback is registered on each build. Each callback calls syncDrawingsWithConfigs() → setState() → another rebuild. Unlike _scheduleStateReset() — which guards with a _stateResetScheduled boolean to prevent duplicate scheduling — the zero-size retry path has no such guard, so redundant callbacks can accumulate.
✅ Fix:
bool _zeroSizeRetryScheduled = false;
// In _reconcileDrawings:
if (drawingContext.fullSize == Size.zero) {
if (!_zeroSizeRetryScheduled) {
_zeroSizeRetryScheduled = true;
WidgetsBinding.instance.addPostFrameCallback((_) {
_zeroSizeRetryScheduled = false;
if (mounted) {
syncDrawingsWithConfigs();
}
});
}
return;
}💡 Explanation: Consistent with the _stateResetScheduled pattern already used for _scheduleStateReset() in the same class. Prevents multiple pending retry callbacks from stacking up during multi-build sequences before first layout.
Summary Table
| Priority | Count | Categories |
|---|---|---|
| 🔴 Critical | 0 | — |
| 🟠 High | 1 | Silent production exception swallowing |
| 🟡 Medium | 0 | — |
| 🟢 Low | 1 | Missing retry deduplication guard |
Recommendations
-
Must fix before merge: Replace the debug-only
chartDiagin thesyncDrawingsWithConfigscatch block withFlutterError.reportErrorso production exceptions remain observable (Issue Phase 1 initial #1). This is a single targeted change in one location. -
Low-risk follow-up: Add
_zeroSizeRetryScheduledguard to match the_stateResetScheduledpattern (Issue Ramin/animate_to_current_tick #2).
The main_chart.dart changes in the latest commit are correct and welcome: the null-guard on markerSeries prevents the release-mode ErrorWidget crash, and reading XAxisModel via context.watch<XAxisModel>() inside a LayoutBuilder ensures annotations track scroll/zoom changes with live closures rather than a stale capture.
Auto Fix Claude Reviews
| Action | Open Dashboard |
|---|
🤖 Claude PR Review CompleteModel: Summary1 of 3 issues from the previous review have been resolved. The latest commit addressed two regressions in Recommendation: REQUEST CHANGES 🟠 High Priority Issues🟠 1. Silent exception swallowing with debug-only diagnostics in release builds —
|
| Severity | File | Lines |
|---|---|---|
| HIGH | lib/src/deriv_chart/interactive_layer/interactive_layer.dart |
208-218 |
❌ Problematic Code:
try {
_reconcileDrawings();
} on Object catch (error, stackTrace) {
// Diagnostics: a throw between the map mutation and setState would
// leave the painted drawings stale with no visible error in release.
// Log it and still rebuild from whatever state the map is in.
if (kChartDiagnosticsEnabled) {
chartDiag('layer#$hashCode reconcile threw: $error\n$stackTrace');
}
}
setState(() {});📋 Issue: kChartDiagnosticsEnabled is kDebugMode, which is false in release builds (a compile-time constant). Any exception thrown inside _reconcileDrawings() — including null-assertion failures on config.configId! for legacy stored configs — is completely silently swallowed in production. The only consequence is broken or frozen drawing state with no crash, no log, and no signal reaching any error-monitoring service.
✅ Fix:
try {
_reconcileDrawings();
} on Object catch (error, stackTrace) {
// Continue with whatever partial state the map is in; a throw mid-mutation
// would otherwise leave painted drawings stale. Report the error so it
// surfaces in production monitoring even though we handle it gracefully.
FlutterError.reportError(FlutterErrorDetails(
exception: error,
stack: stackTrace,
library: 'deriv_chart',
context: ErrorDescription('reconciling drawing tools with repository'),
));
}
setState(() {});💡 Explanation: FlutterError.reportError is visible in all build modes and integrates with Flutter's error reporting pipeline (Crashlytics, Sentry, etc. via FlutterError.onError). It preserves the same graceful-recovery behavior (no rethrow, state rebuilt from whatever the map has) while making the failure observable in production.
🟢 Low Priority Issues
🟢 2. Missing deduplication guard for zero-size post-frame retry in _reconcileDrawings — lib/src/deriv_chart/interactive_layer/interactive_layer.dart:249-260
Details
| Severity | File | Lines |
|---|---|---|
| LOW | lib/src/deriv_chart/interactive_layer/interactive_layer.dart |
249-260 |
❌ Problematic Code:
if (drawingContext.fullSize == Size.zero) {
if (kChartDiagnosticsEnabled) {
chartDiag('layer#$hashCode reconcile skipped: drawing context has zero size...');
}
// Drawing context not ready yet, schedule retry after next frame
WidgetsBinding.instance.addPostFrameCallback((_) {
if (mounted) {
syncDrawingsWithConfigs();
}
});
return;
}📋 Issue: _reconcileDrawings() is called on every build() (line ~467). If drawingContext.fullSize is Size.zero during multiple consecutive builds (rapid rebuilds before first layout), a new addPostFrameCallback is registered on each build. Each callback calls syncDrawingsWithConfigs() → setState() → another rebuild. Unlike _scheduleStateReset() — which guards with a _stateResetScheduled boolean to prevent duplicate scheduling — the zero-size retry path has no such guard, so redundant callbacks can accumulate.
✅ Fix:
bool _zeroSizeRetryScheduled = false;
// In _reconcileDrawings:
if (drawingContext.fullSize == Size.zero) {
if (!_zeroSizeRetryScheduled) {
_zeroSizeRetryScheduled = true;
WidgetsBinding.instance.addPostFrameCallback((_) {
_zeroSizeRetryScheduled = false;
if (mounted) {
syncDrawingsWithConfigs();
}
});
}
return;
}💡 Explanation: Consistent with the _stateResetScheduled pattern already used for _scheduleStateReset() in the same class. Prevents multiple pending retry callbacks from stacking up during multi-build sequences before first layout.
Summary Table
| Priority | Count | Categories |
|---|---|---|
| 🔴 Critical | 0 | — |
| 🟠 High | 1 | Silent production exception swallowing |
| 🟡 Medium | 0 | — |
| 🟢 Low | 1 | Missing retry deduplication guard |
Recommendations
-
Must fix before merge: Replace the debug-only
chartDiagin thesyncDrawingsWithConfigscatch block withFlutterError.reportErrorso production exceptions remain observable (Issue Phase 1 initial #1). This is a single targeted change in one location. -
Low-risk follow-up: Add
_zeroSizeRetryScheduledguard to match the_stateResetScheduledpattern (Issue Ramin/animate_to_current_tick #2).
The main_chart.dart changes in the latest commit are correct and welcome: the null-guard on markerSeries prevents the release-mode ErrorWidget crash, and reading XAxisModel via context.watch<XAxisModel>() inside a LayoutBuilder ensures annotations track scroll/zoom changes with live closures rather than a stale capture.
Auto Fix Claude Reviews
| Action | Open Dashboard |
|---|
1 participant
Clickup link:
Fixes issue: #
This PR contains the following changes:
Release-mode timing exposed several sync failures between the drawing
tools repository, the interactive layer state machine, and the painted
layer:
keyed remount disposes the AnimationController mid-animation, leaving
the state machine stuck; awaited animations now use .orCancel with
mounted guards, and a sequence guard drops superseded transitions.
persisted configId, breaking later edits/deletes; the repo-backed
config is now bound to the selected instance and reconciliation
unifies map/selected instances sharing the same id.
that could go stale when a rebuild failed to propagate (bottom-sheet
"clear all"); it now reads the reconciled map live and repaints via a
drawings-changed notifier independent of rebuild delivery.
Adds debug-only [chart-diag] logging and regression tests covering
remount-cancelled transitions, superseded transitions, instance
unification, and clear-all repaint.
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
Improve synchronization between drawing tools repository, interactive layer state machine, and painted layer to prevent stuck states and ghost drawings in mobile charts.
Bug Fixes:
Enhancements:
Tests: