[mairas]

Summary

Toggling Show Background Graph on a placed Numeric widget had no effect until the widget was rebuilt (issue 594). The options dialog reconfigures the widget in place, but showMiniChart was a signal written only at construction and on a path change. It is now computed from runtime.options(), so the graph appears and disappears as the option is toggled. The writes and the never-empty miniGraphSignature guard are removed. The mini-graph effect still re-runs on option, unit and range changes, because setMiniGraph reads them inside the effect.

Issue 575 (Min/Max kept in the old unit after a live unit switch) is already fixed on main by the SI refactor: the extremes are kept in SI and formatted in the current measure. This PR adds a regression test that pins it.

UX

Classification: UI in scope: UX section only. The existing option now takes effect immediately.

Flows: open a placed Numeric widget's settings, toggle Show Background Graph, then Save. The graph appears or disappears without a reload.

Screens and states: with the graph on, the mini graph is drawn behind the value. With it off, there is no graph. Nothing else changes.

Layout intent: existing layout kept. Design system followed: unchanged widget.

Viewports and accessibility: 390x844 and 1440x900. No new controls.

Testing

• npm run ci passes: 3279 unit tests and 40 schema tests.
• New test for 594: mounts the widget, flips showMiniChart false → true → false through the runtime options signal (as applyRuntimeConfig does), and checks for the <minigraph> element after each step. It failed before the fix, at the false → true step.
• New test for 575: feeds two samples tagged celsius (Min: 11.8 Max: 13.2), then one tagged fahrenheit, and expects 54.2 with Min: 53.2 Max: 55.8 and the mini graph in fahrenheit. It passes against main's component too, which is the evidence that 575 is fixed there.

Closes #594
Closes #575

---

🤖 Generated with Claude Code

https://claude.ai/code/session_01GKFfJCv7rbzXcAYj1DZocd

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 8 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: halos-org/skip/.coderabbit.yaml
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: dceb5bf8-1e27-48c6-a2f9-7c3c77422282

---

📥 Commits

Reviewing files that changed from the base of the PR and between bcdc4ab and 3e0f1e1.

---

📒 Files selected for processing (2)

• src/app/widgets/widget-numeric/widget-numeric.component.spec.ts
• src/app/widgets/widget-numeric/widget-numeric.component.ts

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

_{Comment @coderabbitai help to get the list of available commands.}

[mairas]

Code review

Scope: bcdc4ab1..164dd867, widget-numeric.component.ts and its spec (34 changed non-test lines).
Intent: Show Background Graph takes effect live on a placed Numeric widget, and the test confirming 575 is already fixed stays in place.

Reviewers (model: Opus 5.5, inherited from session; all at once, 7d r 2.05):

• correctness, testing, maintainability, project-standards (always)
• ui: the live toggle is user-facing

Findings

Finding	File	Issue	Trigger	Reviewer	Confidence	Route
1	widget-numeric.component.ts:209 (P2)	The graph restarts on a unit or range change only because setMiniGraph reads those signals inside the tracked effect. No test drove the real effect: the 575 test calls setMiniGraph on a plain object, and the 594 test checks only that <minigraph> exists.	Every live unit switch, y-scale edit or displayScale arrival while the graph is shown.	testing, correctness, maintainability	0.95	safe_auto, applied
2	widget-numeric.component.ts:217 (P3, pre-existing)	graph.startGraph() runs tracked, so the mini graph's own theme() and viewChild become dependencies of the parent effect.	None in current use: a theme change re-runs the effect, but buildChartSignature turns it into a no-op. The old effect did the same.	maintainability	0.62	advisory, drop

Applied fixes

• Finding 1, e4260b45: three tests in the "reconfigured in place" block drive a real MinigraphComponent.
  • A yScaleMax/inverseYAxis change restarts the graph with the new values.
  • A celsius → fahrenheit sample switch restarts it in the new unit.
  • A displayName change leaves the graph's fields alone.
  • With setMiniGraph wrapped in untracked(), the first two fail, as they should. npm run ci passes: 3282 unit tests and 40 schema tests.

Residual risks

• The removed miniGraphSignature never deduplicated anything: if (!miniGraphSignature) could not fire. Restart timing is unchanged, and startGraph deduplicates through its own signature. (correctness, maintainability, testing)
• If a future graph field is passed from a plain member rather than a signal, it will not restart the graph. The inline comment states this. (maintainability)
• Pre-existing: if the numeric path is cleared while the graph is shown, startGraph returns before destroyChart() and the old chart keeps streaming. The path is required, so this is not reachable through the UI. (correctness)

Screenshots

The UI reviewer drove the real flow at 1440x900 (mat-menu) and 390x844 (bottom sheet, touch): edit mode, widget Settings, toggle, Save, through on, off, on, done editing, edit again, off. Every time the option was on, <minigraph> was present and kept updating for 3 s or more. Every time it was off, it was gone. There were no reloads, and the second cycle behaved like the first.

Coverage

• Suppressed: 0. Project standards: no findings (no version bump needed; commit subjects 46–48 characters).
• Dispatch: all at once, 7d r 2.05. No reviewer failed.

---

Verdict: Ready to merge once CI passes on e4260b45.

[mairas]

Pushed e4260b45, which applies review finding 1: three tests that drive the real mini-graph effect (scale change, unit change, unrelated change). It changes tests only. npm run ci passes: 3282 unit tests and 40 schema tests.

[mairas]

Follow-up review of the test commit (e4260b45, pushed after the fan-out; CodeRabbit was rate limited). Correctness, testing, maintainability and project-standards ran over it. Three of them found the same P3: the "unrelated option" test checked only field values, which cannot change, so it passed whether or not the chart was rebuilt. The effect re-runs startGraph() on every options change by design, and MinigraphComponent's chart signature is what makes that a no-op. I amended the commit (now 3e0f1e11, force-pushed with a lease). The test is renamed to "keeps the running chart, and its history, when only an unrelated option changes". It asserts the chart exists and is the same object after a displayName change. With the signature guard removed it fails, and with the guard in place it passes. The Numeric spec passes (35 tests), and so do lint and snc.