chore: keep marker label visible till startTimeCollapsed marker exists - #494
Conversation
Reviewer's GuideRefactors tick marker radius calculation and changes how the dashed connector line and label are drawn so that the label tied to the startTimeCollapsed marker remains visible even after the contractMarker disappears, optionally without the connector line. 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 in 3m 42s —— 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:
- In the block that draws the horizontal dashed line/label, when
_startCollapsedOffsetis non-null but both_contractMarkerOffsetandlabelMarkerare null, nothing is rendered; consider explicitly handling this case (or documenting why it's intentionally a no-op) to make the behaviour clearer. - Within
paintyou repeatedly call_contractOuterRadius(painterProps)for the samepainterProps; consider computing this once per invocation and reusing the value to avoid unnecessary recalculation and make the code slightly clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the block that draws the horizontal dashed line/label, when `_startCollapsedOffset` is non-null but both `_contractMarkerOffset` and `labelMarker` are null, nothing is rendered; consider explicitly handling this case (or documenting why it's intentionally a no-op) to make the behaviour clearer.
- Within `paint` you repeatedly call `_contractOuterRadius(painterProps)` for the same `painterProps`; consider computing this once per invocation and reusing the value to avoid unnecessary recalculation and make the code slightly clearer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
🤖 Claude PR Review CompleteModel: SummaryThis PR refactors Recommendation: REQUEST CHANGES 🟠 High Priority Issues🟠 1. Label text source moved from
|
| Severity | File | Lines |
|---|---|---|
| HIGH | lib/src/.../tick_marker_icon_painter.dart |
204-229 |
❌ Problematic Code:
final ChartMarker? labelMarker = markerGroup.markers.firstWhereOrNull(
(ChartMarker marker) =>
marker.markerType == MarkerType.startTimeCollapsed &&
(marker.text?.isNotEmpty ?? false),
);
if (_contractMarkerOffset != null || labelMarker != null) {
...
if (labelMarker != null) {
_paintDashedLineWithText(
canvas,
lineStart,
_startCollapsedOffset,
finalLineColor,
labelMarker.text!,
labelMarker.textType,
theme,
opacity,
paintConnectorLine: _contractMarkerOffset != null,
);
}
...
}📋 Issue: Before this PR, the label text/type came exclusively from the contractMarker (contractMarker?.text). After this PR, it is read exclusively from a marker with markerType == MarkerType.startTimeCollapsed. This package itself never constructs ChartMarker instances — they are built by the consuming application (e.g. deriv-app) and passed in via MarkerGroup. If the producer of these markers is not updated in lock-step to also (or instead) populate text/textType on the startTimeCollapsed marker, the label will simply stop rendering for every contract once contractMarker disappears — and, depending on producer timing, potentially even while contractMarker is still present, since the new code path only reads startTimeCollapsed.text and no longer falls back to contractMarker.text at all.
text on the startTimeCollapsed marker. Since this is a UI library consumed by another repo, there's nothing in this diff (or in CI here) that would catch the mismatch.
✅ Fix:
final ChartMarker? contractMarker = markerGroup.markers.firstWhereOrNull(
(ChartMarker marker) => marker.markerType == MarkerType.contractMarker,
);
final ChartMarker? labelMarker = markerGroup.markers.firstWhereOrNull(
(ChartMarker marker) =>
marker.markerType == MarkerType.startTimeCollapsed &&
(marker.text?.isNotEmpty ?? false),
) ??
(contractMarker?.text?.isNotEmpty ?? false ? contractMarker : null);💡 Explanation: Falling back to contractMarker.text when startTimeCollapsed.text isn't populated preserves backward compatibility with existing callers during the transition, while still allowing the new source to take priority once callers are updated. At minimum, please confirm with the author/reviewers that the corresponding change to populate text on startTimeCollapsed markers has already landed (or is landing together) in the consuming application(s), since this repo has no way to verify that from the diff alone.
🟡 Medium Priority Issues
🟡 2. New branching logic has no test coverage — lib/src/deriv_chart/chart/data_visualization/markers/marker_icon_painters/tick_marker_icon_painter.dart:203-243,331-405
Details
| Severity | File | Lines |
|---|---|---|
| MEDIUM | lib/src/.../tick_marker_icon_painter.dart |
203-243, 331-405 |
❌ Problematic Code:
if (_startCollapsedOffset != null) {
final ChartMarker? labelMarker = markerGroup.markers.firstWhereOrNull(...);
if (_contractMarkerOffset != null || labelMarker != null) {
final Offset lineStart = _contractMarkerOffset ?? Offset(...);
if (labelMarker != null) {
_paintDashedLineWithText(..., paintConnectorLine: _contractMarkerOffset != null);
} else {
paintHorizontalDashedLine(...);
}
}
}📋 Issue: There is no test file for tick_marker_icon_painter.dart in the repo (pre-existing gap), so this PR adds several new conditional paths — (a) contract marker present + label present, (b) contract marker absent + label present (the new "label outlives contractMarker" case this PR is meant to fix), (c) contract marker present + label absent, (d) both absent — without any automated regression test to lock in the intended behavior (e.g. verifying paintConnectorLine is false and the fallback anchor position is used when _contractMarkerOffset is null).
✅ Fix:
testWidgets('renders label anchored at fixed offset when contractMarker is absent but startTimeCollapsed has text', (tester) async {
// Build a MarkerGroup with only a startTimeCollapsed marker (text: '3/10')
// and no contractMarker; verify paintHorizontalDashedLine/text painter
// is invoked with paintConnectorLine == false and the expected anchor.
});💡 Explanation: A focused unit/widget test (using a TestCanvas/RecordingCanvas or golden test, consistent with other painter tests in this repo if any exist for sibling painters) would pin down this new behavior and prevent regressions.
Summary Table
| Priority | Count | Categories |
|---|---|---|
| 🔴 Critical | 0 | — |
| 🟠 High | 1 | Regression risk / data contract change |
| 🟡 Medium | 1 | Missing test coverage |
| 🟢 Low | 0 | — |
Recommendations
- Confirm (in the PR description or with the consuming app's maintainers) that
text/textTypewill be populated onstartTimeCollapsedmarkers going forward, or add a fallback tocontractMarker.textto avoid a silent feature regression for existing callers. - Add at least one test exercising the new "contractMarker absent, label present" branch to lock in the intended fix and guard against future regressions.
- The
_contractOuterRadiusextraction (tick_marker_icon_painter.dart:322-323) is a good deduplication and does not need changes.
Auto Fix Claude Reviews
| Action | Open Dashboard |
|---|
1 participant
This PR contains the following changes:
The marker label before a contract's start time (e.g. the
2/10tick counter) is now read fromstartTimeCollapsedinstead ofcontractMarker.Consumers drop the
contractMarkerwhen a contract finishes, so the label — read off that marker and painted as part of its dashed connector line — vanished on settlement. It now stays as long as the finished contract's other markers do, while the connector line still goes with thecontractMarkerit belongs to.contractMarkerstartTimeCollapsed.textWithout the
contractMarker, the label anchors to the same fixed left-edge offset that marker would have occupied, so it doesn't shift on settlement.text/textTypeon thecontractMarkerare no longer rendered, with no fallback and no warning. Send them onstartTimeCollapsedinstead, in both the running and finished payloads. That marker must also be present, since the label is positioned relative to it.