feat: bring the redesigned indicator label to web - #496
Conversation
Reviewer's GuideBrings the redesigned indicator labels to web by sharing the mobile implementation and state, adding overlay labels and hide/reorder/actions to repository-driven web panels, and replacing fullscreen expansion with persistent resizable layouts. Also wires panel persistence and custom label icons through DerivChartLite and moves label styling into ChartTheme while preserving defaults. Sequence diagram for web indicator label actionssequenceDiagram
actor User
participant Label as IndicatorLabel
participant State as _ChartStateWeb
participant Repo as IndicatorConfig repository
participant Host as Host callbacks
User->>Label: expand or collapse
Label->>State: _toggleLabelExpanded
State-->>Label: rebuild action row
User->>Label: hide or unhide
Label->>State: _onIndicatorHideToggleTapped
State->>Repo: updateHiddenStatus
Repo-->>State: hidden status changes
State-->>Label: rebuild label or panel
User->>Label: edit or delete
Label->>Host: onEdit or onRemove
User->>Label: reorder
Label->>State: onSwap
State->>Host: reorder indicator config
Flow diagram for persistent web panel sizingflowchart TD
Repo[IndicatorConfig repository] --> Keys[Build ordered panel keys]
Keys --> Sync[_syncPanelFractions]
Sync --> Layout[Render main chart and bottom panels]
Layout --> Divider[ResizableChartDivider]
Divider --> Resize[_resizeCascadingPanels]
Resize --> Persist[_persistPanelFractions]
Persist --> Storage[PanelSizeRepository]
Storage --> Layout
Hidden[Hidden bottom indicator] --> Layout
Layout --> Restore[Unhide restores stored panel fraction]
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 Code is working… 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 found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="lib/src/deriv_chart/chart/chart_state_web.dart" line_range="54-84" />
<code_context>
+ final List<Series> visibleOverlaySeries = <Series>[];
</code_context>
<issue_to_address>
**issue (broader_impact):** The web layout ignores the `overlaySeries` argument built from `widget.overlayConfigs` and passes only repository-backed overlays to `MainChart`, so overlays supplied through `Chart.overlayConfigs` disappear on web.
**Triggers:** When a consumer supplies overlay indicators through `Chart.overlayConfigs` rather than `indicatorsRepo`.
**Suggested fix:** Include the existing `overlaySeries` in the visible overlay list, while applying repository hidden-status filtering to repository-backed overlays.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Claude finished @behnam-deriv's task in 8m 59s —— View job I'll analyze this and get back to you. |
Web fell back to reading only `indicatorsRepo`, so a host passing plain `overlayConfigs`/`bottomConfigs` to `Chart` - both public and independent of the repository, which is optional - got no indicators at all. The repository stays the source of truth when present, since it is what carries hidden status, and the passed-in configs and series are used when it is absent. The two are deliberately not merged, as the review suggested: `DerivChart` derives `overlayConfigs` *from* the repository it also passes, so combining them would draw every overlay twice on the path that matters most. Adds tests pinning `DerivChartLite`'s forwarding of `panelSizeRepo` and `indicatorLabelIcons` - the two parameters it was missing - since neither has a visible effect until the chart renders on web. Corrects two doc comments the rename left stale: `BottomChartWithLabel` no longer calls itself mobile-only, and `DerivChartLite` no longer claims to avoid `shared_preferences`, which it now uses for default panel-size persistence.
|
Claude finished @behnam-deriv's task in 2m 10s —— View job I'll analyze this and get back to you. |
|
Claude finished @behnam-deriv's task in 1m 8s —— View job I'll analyze this and get back to you. |
🤖 Claude PR Review CompleteModel: Summary3 of 4 issues from the previous review have been resolved. This push ( Recommendation: APPROVE (one non-blocking follow-up suggested) 🟡 Medium Priority Issues🟡 1. Web layout integration (
|
| Severity | File | Lines |
|---|---|---|
| MEDIUM | lib/src/deriv_chart/chart/chart_state_web.dart |
whole file |
❌ Problematic Code:
test/deriv_chart/deriv_chart_lite_test.dart only asserts that DerivChartLite forwards its new parameters to the Chart widget it builds:
final Chart chart =
await pumpAndFindChart(tester, app(panelSizeRepo: repo));
expect(chart.panelSizeRepo, same(repo));No test pumps _ChartStateWeb (or forces Chart onto the web code path) to assert that overlay/bottom panels actually render from overlayConfigs/bottomConfigs when no indicatorsRepo is supplied, that hide/unhide toggles a panel's visibility, or that reordering swaps panels correctly.
📋 Issue: This commit is formatting-only (dart format), so it doesn't touch test coverage. The gap that would have caught the now-fixed regression in bottomPanelConfigs/visibleOverlaySeries branching (Issue #1 from the previous review) is still untested at the widget level.
✅ Fix:
Add a widget test that pumps Chart (with debugDefaultTargetPlatformOverride or an equivalent web-forcing shim) covering: (a) overlayConfigs/bottomConfigs supplied with no indicatorsRepo still renders panels, (b) an indicatorsRepo with one overlay + one bottom indicator renders both labels, (c) hiding one collapses its panel while keeping the label.
💡 Explanation: Widget-level coverage of buildChartsLayout's actual output — not just DerivChartLite's forwarding — is what guards against a repeat of the issue that was just fixed.
Summary Table
| Priority | Count | Categories |
|---|---|---|
| 🔴 Critical | 0 | — |
| 🟠 High | 0 | — |
| 🟡 Medium | 1 | Missing _ChartStateWeb rendering/hide-unhide test coverage |
| 🟢 Low | 0 | — |
Recommendations
- Non-blocking: add a widget test exercising
_ChartStateWeb's actual rendering path (with and withoutindicatorsRepo, plus hide/unhide) to fully close the coverage gap.
Auto Fix Claude Reviews
| Action | Open Dashboard |
|---|
1 participant
What
The on-chart indicator label added in #487 shipped for mobile only —
Chartpicks its state class withkIsWeb, and only_ChartStateMobilebuilt the label. Web kept the older always-visible icon row, had no labels at all for overlay indicators, and no way to hide one.Web now renders the same label, with the same expand/collapse, hide/unhide, reorder, settings and delete actions as mobile.
How
_ChartState— the expanded-label set, icon accessors, hide toggle, overlay-label column, title composer andreferenceIndexOf— so there is one implementation rather than two._ChartStateWebiterates the repository instead ofbottomConfigs, keeping each panel's true index so hidden status can be looked up. Hidden panels collapse to their label while holding their size, as on mobile.expandedIndex. It was web-only, has no equivalent in the design, and the resizable dividers from feat: indicators panel resize and toolbar #487 cover the need.BottomChartMobile→BottomChartWithLabelandIndicatorLabelMobile→IndicatorLabel, now that both platforms use them. Neither is exported, so nothing outside the package is affected.Also in here
DerivChartLitenever acceptedpanelSizeRepoorindicatorLabelIcons— feat: indicators panel resize and toolbar #487 wired only the fullDerivChart. Consumers on the lite variant therefore got neither persisted panel sizes nor custom label icons. Both are now passed through, with the same ownership rules asDerivChart(it creates and disposes its own repo only when the host doesn't supply one).ChartTheme(indicatorLabelIconSize,indicatorLabelTextStyle), alongside the component-level styling it already carries. Defaults are unchanged, so mobile renders exactly as before; a host on a wider canvas — where the same absolute size reads much smaller — scales them by overriding the theme rather than threading parameters through the widget tree.Compatibility
Defaults are preserved throughout, so deriv-mobile-app is visually unchanged. Two notes for reviewers:
ChartThemeinterface is breaking for anyone implementing it directly rather than extendingChartDefaultTheme. The concrete defaults mean the normal path is unaffected.BottomChartis now unused by the layout but kept, since it still owns theSwapCallbacktypedef.Testing
flutter testpasses in full (133 tests intest/deriv_chart/chart/, including the label, panel-fraction, panel-size-repository and divider suites from #487).flutter analyze lib/reports no new issues.Verified end to end through deriv-smartcharts on a desktop-width chart: collapsed pills on bottom panels and overlays; chevron expands the action row; eye hides the series and collapses the panel, restoring its exact size on unhide; gear and delete route to the host; reorder arrows reorder; dividers resize and persist across a reload; light and dark themes.
Summary by Sourcery
Bring the redesigned indicator labels and consistent panel interactions to web while aligning lite-chart configuration, persistence, and theming with the full chart.
New Features:
Bug Fixes:
Enhancements:
Tests: