Node "Chart" tab: overlay comparable child sensors (#1235)

[lotgon]

Summary

Adds a read-only Chart tab to a tree node that overlays its comparable child sensors' history as one multi-line time chart over an operator-chosen window (last hour / 3h / day / custom). Read-only, derived entirely from stored history — never writes or changes sensor state.

Closes #1235

What the tab does

• Groups the node's chart-comparable descendant sensors by (SensorType, effective unit) — numeric line-able types only (Integer/Double/Rate/IntegerBar/DoubleBar; excludes enum/bool/version/timespan/string + service status/alive). Bars are flattened to their Mean.
• Group ("Type") selector next to the Period selector: when a node has more than one comparable (type, unit) group, the operator picks which one to overlay (one unit at a time, so the y-axis stays meaningful). Defaults to the largest group.
• One line per child. Children with no data in the window are omitted (not zero-filled); intermittent series (top-N processes reported only while active) are drawn with gaps, never interpolated.
• 20-line cap. The endpoint reads every sensor in the group (bounded per-sensor + a 500 scan ceiling), keeps only those with data in the window, then shows the 20 highest-peak — so intermittent nodes surface their active sensors instead of collapsing to one line. A note reports "20 of N with data".
• Tab hidden when a node has no group of ≥2 comparable children.

Implementation

• Server: TreeViewModel.GetComparableChildGroups / HasComparableChildGroup; SensorHistoryController.NodeChartHistory (POST, body { nodeId, groupKey?, from, to } → { error, unit, note, selectedKey, groups[], series[] }); SelectNode sets SelectedNodeViewModel.ShowChartTab.
• Client: an inline renderer in _NodeChartTabContent.cshtml reuses the already-bundled Plotly.js (palette-cycled scattergl, shared legend) — no webpack rebuild.
• Docs: aicontext/features/site/node-children-view/feature.md.

Verified locally (server built from this branch, real + seeded data)

• Top CPU processes (52-sensor group): last hour → 15 lines, 24h → 20 (capped, peak-ranked), values match; fixed an earlier collapse-to-one-line bug where the cap picked idle sensors by tree order.
• Multi-group node → Type selector lists both groups and switching re-queries the other.
• Node with < 2 comparable children → no Chart tab.
• Period change re-queries and redraws.

Out of scope (tracked for later)

• v2 ranking leaderboard (total / average / peak).
• v3 direct-children-vs-subtree scope toggle, per-type aggregation (rate/counter delta, enum/bool availability).
• Possible follow-up: move the inline renderer into the bundled JS layer (wwwroot/src/ts) and reuse ChartHelper.initMultiChart.

🤖 Generated with Claude Code

[github-actions]

Code Review — PR #1241: Node chart tab (issue #1235)

Overall this is a clean, well-scoped feature. The code is nicely commented, the "why" behind the design choices (peak-based capping, gaps vs zero-fill, cheap-vs-full group computation) is documented, and the server/client contract is coherent. Findings below are mostly medium/low; nothing looks like a hard blocker.

---

🟠 Medium

1. Sequential fan-out of history reads — latency & DB load
SensorHistoryController.cs (NodeChartHistory / ReadNodeChartPoints)

The endpoint reads every sensor in the group sequentially with await inside the foreach:

foreach (var sensorId in group.SensorIds.Take(NodeChartMaxSensorsScanned))
{
    ...
    var points = await ReadNodeChartPoints(sensor, from, to); // one LevelDB paged read at a time
    ...
}

GetSensorValuesPage is an IAsyncEnumerable backed by LevelDB. With NodeChartMaxSensorsScanned = 500 and NodeChartMaxPointsPerSensor = -2000, a worst-case request serializes up to 500 paged DB reads (up to ~1M points) into one request's wall-clock, and the full history of every sensor is read even though only MaxSensorsPerChart = 20 are ever displayed (the peak ranking requires it). On a large same-unit group this is a slow, blocking endpoint.

Suggestions:

• Parallelize the reads (e.g. bounded Task.WhenAll / Parallel.ForEachAsync) if _cache.GetSensorValuesPage is safe for concurrent reads (worth confirming), or
• Lower the scan ceiling / points-per-sensor for the default (last-hour) window, or
• Consider a lighter aggregation read for ranking, then a full read only for the selected top-N.

At minimum, please confirm the observed latency on a realistically large group is acceptable.

---

🟡 Low

2. Odd namespace: HSMServer.Model.Model.History
Model/History/NodeChartRequest.cs

The file lives in Model/History/ but declares namespace HSMServer.Model.Model.History (double Model), and the controller has to import both HSMServer.Model.History and HSMServer.Model.Model.History. This looks like a typo. It compiles, but it's confusing and inconsistent with the sibling HSMServer.Model.History namespace. Recommend HSMServer.Model.History.

3. No client-side error handling / data.error never checked
_NodeChartTabContent.cshtml (loadNodeChart)

The AJAX call has a .done handler but no .fail. The server responses carry an error flag (always false today), but the client never inspects it. If the request fails (500, network, timeout), the user sees a stale chart with no feedback. Consider adding a .fail branch that surfaces the "No data / error" state.

4. Custom datetime parsing can produce an invalid date
_NodeChartTabContent.cshtml (nodeChartWindow)

const from = fromVal ? new Date(`${fromVal}:00Z`) : ...

datetime-local normally yields YYYY-MM-DDTHH:MM, so appending :00Z is correct. But when the control supplies seconds (a step allowing seconds, or some browsers), the value is YYYY-MM-DDTHH:MM:SS and the result becomes the invalid ...:SS:00Z → Invalid Date → toISOString() throws. Low likelihood but easy to harden (strip/normalize before appending, or build the ISO string explicitly).

5. Series labels rendered into Plotly without escaping (consistency)
_NodeChartTabContent.cshtml (renderNodeChart)

Group selector options are correctly escaped via nodeChartEscape, but series labels are injected raw into Plotly name and into the hovertemplate string. Sensor names/paths are ultimately client-supplied (via DataCollector), and Plotly text supports a limited pseudo-HTML subset. Risk is low (Plotly sanitizes hrefs, and the existing single-sensor chart does the same), but the escaping is applied inconsistently. Worth a note / follow-up.

6. Access control relies on [Authorize] only — please confirm intended
SensorHistoryController.cs

NodeChartHistory looks up nodes/sensors in the global _tree with no product-role filter, so any authenticated user can request any node's child-sensor data by node id. This is consistent with the existing ChartHistory endpoint in the same controller (no ProductRoleFilter* attributes anywhere here), so it's not a regression introduced by this PR — but since this PR adds a convenient "dump an entire node's children" surface, please confirm the product's access model treats the tree as globally readable for authenticated users. If per-product visibility is expected elsewhere, this endpoint (and ChartHistory) should honor it.

---

✅ Things done well

• HasComparableChildGroup (cheap, early-return existence check) vs GetComparableChildGroups (full, labeled, sorted) split is a good call — the expensive path only runs on the actual chart request.
• Peak-based capping after reading, with an explicit user-facing note, avoids the "first-by-tree-order is idle" trap and is well documented (no silent truncation — good, matches the repo's "no silent data loss" rule).
• TryGetScalar correctly covers RateValue (it's BaseValue<double>), int/double, and the bar means; non-scalar types are omitted rather than zero-filled.
• ToGuid() degrades safely to Guid.Empty for null/garbage NodeId, so the null-body and bad-id paths return an empty result rather than throwing.
• Stale-response guard via window._nodeChartToken prevents out-of-order renders when the user changes period/group quickly.

---

Reviewed via git diff 4da5dee...623930a (PR merge ref).

[github-actions]

Review — PR #1241: Node "Chart" tab (issue #1235)

Nice feature. The implementation is well-factored, the XML/inline comments are genuinely useful, and several sharp edges are already handled well: the stale-response token guard on the AJAX call, connectgaps: false to honour intermittent series, scattergl for many-line performance, bounded per-sensor point counts, and escaping of the group-selector <option> values. Below are the issues I found, ordered by importance.

1. Performance — history reads are fully sequential (medium)

SensorHistoryController.NodeChartHistory reads each sensor's history one at a time:

foreach (var sensorId in group.SensorIds.Take(NodeChartMaxSensorsScanned)) // up to 500
{
    ...
    var points = await ReadNodeChartPoints(sensor, from, to); // ~2000 points, hits the cache/DB
    ...
}

With NodeChartMaxSensorsScanned = 500 and NodeChartMaxPointsPerSensor = -2000, a single tab load can issue up to 500 sequential GetSensorValuesPage reads before returning. Each await blocks the whole chain, so latency is the sum of all reads, not the slowest. For a large same-unit group this is a slow, thread-holding request. Consider bounded parallelism (Task.WhenAll over batches / a SemaphoreSlim), or reading only the sensors you'll actually display. Even for typical groups this is the most likely real-world pain point.

2. Authorization — no per-user/product access check (security consideration)

The endpoint takes an arbitrary NodeId from the request body and returns the history of all of that node's comparable children, gated only by [Authorize] (authentication), reading from the global singleton TreeViewModel:

if (!_tree.Nodes.TryGetValue(nodeId, out var node)) ...

Any authenticated user can request any node id and read its children's data regardless of product permissions. This mirrors the existing ChartHistory / SelectNode pattern (also unguarded against the global tree), so it is not a regression — but this new endpoint widens the surface by returning many sensors' history in one call. If product-level access control is expected anywhere, this endpoint should participate in it. Worth confirming with the team whether the global-tree pattern is intentional.

3. Scan ceiling silently drops sensors by tree order (low)

The code carefully argues (in the MaxSensorsPerChart comment) that capping by tree order before reading is wrong because intermittent top-N series are usually idle first — yet NodeChartMaxSensorsScanned does exactly that:

foreach (var sensorId in group.SensorIds.Take(NodeChartMaxSensorsScanned)) // first 500 by tree order

For a group larger than 500, active-but-late sensors get dropped by the same tree-order logic the display cap was designed to avoid — and unlike the display cap, there is no note telling the operator that truncation happened. 500 is generous so this is edge-case, but the two caps are inconsistent in spirit; at minimum, emit a note (and/or log) when the scan ceiling truncates.

4. hovertemplate / trace name interpolate the label unescaped (low)

The group selector escapes labels with nodeChartEscape, but the trace legend name and hover template do not:

name: s.label,
hovertemplate: `${s.label}: %{y} ${unit}<br>...`

Sensor names/paths are user-influenced. Plotly restricts hover/legend HTML to a safe subset (so script execution is unlikely), but escaping here would be consistent with the option handling and removes any doubt.

5. GetNodeRelativeLabel prefix match lacks a separator boundary (low / robustness)

if (sensorPath.StartsWith(nodePath, StringComparison.Ordinal))
    var relative = sensorPath.Substring(nodePath.Length).TrimStart('/');

For a node a/b, a path a/bc/d would StartsWith("a/b") and yield c/d. In practice every id comes from GetAllNodeSensors, so sensorPath is always a true nodePath + "/…" descendant and this can't trigger — but a nodePath + "/" prefix check would make the intent explicit and future-proof.

Minor notes

• HasComparableChildGroup runs on every SelectNode. It early-exits at the second comparable sensor, so it's cheap in the common case, but on a large node with no comparable group it walks the full descendant set on every click. Acceptable, just noting the added per-selection cost.
• EffectiveUnitCode casts two different enums (RateDisplayUnit vs SelectedUnit) to int. This is only safe because SensorType is part of the group key; it's correct today but brittle if the key ever drops Type. A short comment on that dependency would help.
• Null-safety checks out: request.NodeId.ToGuid() uses the Guid.TryParse extension, so a null/garbage id resolves to Guid.Empty and returns the empty result rather than throwing. ToUtcKind uses SpecifyKind, so DateTime.MaxValue doesn't overflow. Good.

Verdict

No blocking correctness bugs found. I'd like to see #1 (sequential reads) addressed before merge given it directly affects the feature's responsiveness, and #2 (authorization) at least explicitly confirmed as intended. The rest are low-severity polish.

[github-actions]

PR #1241 Review — Node chart tab (issue #1235)

Nice feature. The overlay endpoint is well-bounded (scan/points/concurrency caps), reads are pure/read-only, and client-side escaping is applied before injecting user-influenced sensor names into Plotly and <option> markup. A few issues worth addressing before merge, one of which is a real functional bug.

🐞 Bugs / logic errors

1. ShowChartTab is stale for folders — the Chart tab wrongly renders on folder panels (Medium)

SelectNode only assigns ShowChartTab in the node branch:

// HomeController.cs:122-124
StoredUser.SelectedNode.ConnectNode(node);
StoredUser.SelectedNode.ShowChartTab = _treeViewModel.HasComparableChildGroup(id);

StoredUser.SelectedNode is a single reused instance, and neither ConnectFolder nor Subscribe reset ShowChartTab (SelectedNodeViewModel.cs:35-41). So:

1. Select a node that has comparable child groups → ShowChartTab = true.
2. Select a folder → ConnectFolder runs, ShowChartTab stays true.
3. _Folder.cshtml renders _ChildrenPanel with SelectedNode → @if (Model.ShowChartTab) is true → the Chart tab renders for the folder.
4. Clicking it calls loadNodeChart(folderId); the endpoint does _tree.Nodes.TryGetValue(folderId, …), which fails because folders live in _folderManager, not Nodes → _emptyJsonResult → a broken/empty Chart tab on a folder.

Fix: reset ShowChartTab when a non-node is connected (e.g. set it false in the folder branch of SelectNode, and ideally default it in Subscribe/ConnectFolder so it can never leak across selections).

2. A single failing per-sensor read faults the entire chart request (Medium)

The endpoint's stated design is graceful degradation ("children with no data … are omitted"), but the read fan-out has no per-sensor try/catch:

// SensorHistoryController.cs — inside the per-sensor lambda
try {
    var points = await ReadNodeChartPoints(sensor, from, to);
    ...
}
finally { readGate.Release(); }   // releases, but does NOT catch
...
var built = (await Task.WhenAll(reads))...

If any one sensor's ReadNodeChartPoints throws (e.g. RateSensorModel.ToDisplayValue throws ApplicationException for an unexpected value shape, or the underlying page read faults), Task.WhenAll rethrows and the whole node chart 500s — one bad child sensor takes down an entire group's chart. Consider catching per-sensor and treating a failed read like "no data" (return null) so one sensor can't break the overlay.

🧹 Code quality / maintainability

3. window._nodeChartToken is global across all node charts (Low–Medium)

_NodeChartTabContent.cshtml uses a single global request token:

const token = (window._nodeChartToken = (window._nodeChartToken || 0) + 1);

Multiple opened node panels each get their own _NodeChartTabContent in the DOM, but they all share this one counter. If node A's chart and node B's chart load near-simultaneously, A's successful response is discarded on arrival (token !== window._nodeChartToken) and A's chart stays blank. The token should be scoped per node (e.g. stored on the nodeChartData_${nodeId} element's dataset) so requests only cancel their own predecessors.

4. renderNodeChart dereferences noteEl without the null guard used elsewhere (Low/nit)

showNodeChartMessage null-checks noteEl before use, but renderNodeChart does noteEl.textContent = data.note unguarded. Harmless today (the element is always in the template), but the inconsistency is a latent trap — worth aligning.

🔒 Security

5. No per-user authorization on the resolved node (Low / confirm intended)

NodeChartHistory resolves any nodeId straight from the global _tree with no product-access check, so any authenticated user can read history for any node's descendant sensors by supplying a GUID — and this endpoint fans that out across up to 500 descendants at once. This matches the existing pattern (ChartHistory similarly reads any sensor by encoded id without an access check), so it's not a new escalation, but please confirm the intended access model — the broader fan-out makes it a more attractive enumeration primitive than the single-sensor endpoints.

Minor notes

• EffectiveUnitCode/grouping keys on (Type, unitCode), so an Integer "%" group and a Double "%" group are never overlaid together even when comparable. Reasonable for v1 per the comments — just flagging as a known limitation.
• Ranking dropped series by raw peak Value mixes magnitudes fine within a unit group, but if values can be negative the "highest-peak" selection picks the least-negative series; acceptable for the numeric types in scope.

Verdict

Solid, defensively-written feature. Please fix #1 (folder Chart-tab leak) and #2 (whole-request failure on one bad sensor) before merge; #3 is worth doing while nearby.

[github-actions]

PR #1241 Review — Node chart tab (issue #1235)

Reviewed git diff origin/master...feature/1235-node-chart-tab.

Overall this is a well-scoped, carefully defensive change. The new NodeChartHistory endpoint has good comments explaining the tradeoffs (peak-based capping, bounded fan-out, per-sensor exception isolation, per-node request tokens on the client). Nothing here looks like a hard blocker, but there are a few correctness edges, a security question, and some performance/maintainability items worth addressing before merge.

---

🔒 Security / access control

1. New data-exposure endpoint has no per-product authorization check. (SensorHistoryController.NodeChartHistory)

[Authorize] only enforces authentication. The node id comes straight from the request body, is looked up in the global singleton TreeViewModel (registered AddSingleton<TreeViewModel>()), and its descendant sensor history is returned. There is no check that the current user is allowed to see this product/node.

This is consistent with the existing ChartHistory endpoint (which likewise looks up any encodedId in the global tree), so it isn't a regression — but the PR adds a new surface that returns bulk history for arbitrary node ids to any logged-in user. Please confirm product-level access is enforced somewhere (or add a check), since "the UI only shows ids you can see" is not an authorization boundary for a POST endpoint. If the whole controller intentionally lacks per-product checks, a one-line note in the PR would help.

---

🐛 Correctness

2. EffectiveUnitCode and EffectiveUnitLabel diverge for Rate sensors without a DisplayUnit. (SensorNodeViewModel.cs)

public int? EffectiveUnitCode => Type is SensorType.Rate
    ? (DisplayUnit.HasValue ? (int)DisplayUnit.Value : null)      // ignores SelectedUnit
    : (SelectedUnit.HasValue ? (int)SelectedUnit.Value : null);

public string EffectiveUnitLabel => Type is SensorType.Rate && DisplayUnit.HasValue
    ? DisplayUnit.Value.GetDisplayName()
    : SelectedUnit?.GetDisplayName() ?? string.Empty;             // falls back to SelectedUnit

For a Rate sensor with DisplayUnit == null, the grouping code is null (SelectedUnit ignored) but the label falls back to SelectedUnit. Consequence: two Rate sensors that both lack DisplayUnit but have different SelectedUnit values are grouped together (same null code) yet carry different unit labels — they'd be overlaid on one axis despite representing different units, and the group's displayed unit is taken arbitrarily from ids[0]. It's an edge case (Rate sensors usually carry DisplayUnit), but the code/label paths should use the same source. Consider making the label derive from the same expression that produces the code.

3. NaN/Infinity values reach the client as JSON string literals.

_serializationsOptions sets JsonNumberHandling.AllowNamedFloatingPointLiterals, so a rate/mean that computes to NaN/Infinity serializes as "NaN"/"Infinity" strings in the values[].value array. Plotly will get a string where it expects a number for that point. Worth deciding whether such points should be filtered in TryGetScalar (e.g. double.IsFinite) rather than plotted.

---

⚡ Performance

4. Every chart load reads up to 500 sensors × 2000 points, even though only 20 are shown. (NodeChartHistory)

The design reads all scanned sensors and then caps the display to the 20 highest-peak (the comment explains why capping pre-read would drop intermittent series — that reasoning is sound). But this means each tab open and each period/group change fans out up to NodeChartMaxSensorsScanned = 500 history reads (concurrency 8). For a large same-unit group this is a meaningful DB burst per interaction. It's bounded and documented, so acceptable for v1, but flagging it since period changes re-fetch everything. A future optimization could fetch a cheap peak/aggregate first, then full history only for the top 20.

5. HasComparableChildGroup runs on every node selection. (HomeController line ~121)

It walks all descendant sensors via GetAllNodeSensors (allocates a list, recurses the subtree) on every node click. The early-return on the second matching (type,unit) keeps the common case cheap, but the worst case (a large node whose sensors are all distinct unit/type) iterates the entire subtree with no early exit on each selection. Probably fine at current scale; worth keeping in mind for very large products.

---

🧹 Maintainability

6. The entire <script> block is re-emitted per node. (_NodeChartTabContent.cshtml)

Each opened node with a chart tab renders its own copy of the IIFE that (re)assigns window.loadNodeChart, window.onNodeChartPeriodChange, etc. The functions are identical and keyed by nodeId, so it's functionally correct (last-write-wins with the same code), but it duplicates the palette and all handlers N times in the DOM. Consider moving the script to a single shared .js/bundle include and keeping only the per-node markup in the partial.

7. Reliance on global window.Plotly without a guard.

showNodeChartMessage calls window.Plotly.purge(...) and renderNodeChart calls window.Plotly.newPlot(...). If Plotly isn't loaded (or fails to load) these throw uncaught inside the ajax .done handler. A cheap if (!window.Plotly) return; guard (or ensuring the dependency is loaded before the tab activates) would fail more gracefully.

8. nodeChartEscape doesn't escape '. Currently fine because the only attribute interpolation (<option value="...">) uses double quotes and the values are numeric keys, but if the escaping helper is reused later in a single-quoted context it would be unsafe. Minor — consider escaping ' too for defensiveness, or add a comment that it's HTML-double-quote/text only.

---

✅ Things done well

• Per-sensor try/catch so one faulting child doesn't 500 the whole overlay (with logging) — nice.
• Per-node request token (dataset.reqToken) instead of a global counter — correctly avoids cross-node cancellation.
• ShowChartTab reset in SelectNode to prevent state leaking across node selections.
• Bounded concurrency via SemaphoreSlim with correct WaitAsync/finally Release and using disposal after WhenAll.
• GetNodeRelativeLabel guarding on the / boundary; ToGuid null-safety; UTC handling on both client and server is consistent and commented.
• Server-side XSS escaping of user-influenced sensor names before Plotly injection.

Nice work overall — items #1 (confirm authz) and #2 (unit code/label consistency) are the ones I'd want resolved before merge; the rest are follow-ups.

[github-actions]

Code Review — PR #1241: Node chart tab (issue #1235)

Overall this is a well-structured, well-commented change. The overlay endpoint is defensive (per-sensor try/catch, bounded scan/concurrency, non-finite point filtering), and the client escapes user-influenced labels before feeding them to Plotly. Nice touches: the per-node request token that fixes cross-node cancellation, and parseLocalAsUtc guarding toISOString(). Comments below, grouped by severity — none are blocking, but a couple are worth addressing.

🔐 Security / authorization

1. No product-level authorization on NodeChartHistory (potential IDOR). SensorHistoryController.cs
The endpoint takes an arbitrary NodeId from the body, looks it up in the global _tree.Nodes/_tree.Sensors, and returns history for every comparable descendant sensor:

if (!_tree.Nodes.TryGetValue(nodeId, out var node)) ...

There is no [ProductRoleFilter*] (the mechanism AccessKeysController/ProductController use) and no user.IsProductAvailable(...) check, so any authenticated user who guesses/knows a node GUID can read that subtree's data regardless of their product roles — and this endpoint amplifies it by returning up to 500 sensors' worth of history in one call.

Note this matches the existing ChartHistory/TableHistory endpoints in this controller (all [Authorize]-only, all reading the global tree), so it's a pre-existing pattern rather than a regression introduced here. Still worth confirming this is intended for the new bulk endpoint; if product isolation matters, this endpoint (and its siblings) should verify access to node.RootProduct.

🐞 Correctness

2. Nondeterministic default group / selector order on ties. TreeViewModel.GetComparableChildGroups
Groups are collected in a Dictionary (unordered iteration) and sorted:

return result.OrderByDescending(group => group.SensorIds.Count)
             .ThenBy(group => group.UnitLabel)
             .ToList();

Two groups with the same count and same UnitLabel but different Type (e.g. an Integer and a Double group that are both unitless → UnitLabel == "") tie on both sort keys, so their relative order — and therefore groups[0], the group overlaid by default when the client sends no GroupKey — can flip between requests. The dropdown order can likewise shuffle. Add a stable final tie-break, e.g. .ThenBy(group => group.Key).

⚡ Performance

3. Read amplification: up to 500 full-window reads to display ≤20 series.
Because the display cap is applied after reads (to rank by peak), every scanned sensor is read in full:

var scannedIds = group.SensorIds.Take(NodeChartMaxSensorsScanned).ToList(); // up to 500
...
var shown = withData > MaxSensorsPerChart ? built.OrderByDescending(...).Take(20) : built;

So for a large group the server issues up to 500 history reads (each up to 2000 points), materializes them all, then discards ~480. The rationale (top-N intermittent series) is sound, but the cost is real and per-request. Consider a cheaper pre-rank — e.g. last-value / recent-activity from the cache — to shortlist candidates before doing full-window reads, or lowering NodeChartMaxSensorsScanned toward a realistic ceiling. The NodeChartReadConcurrency/NodeChartMaxSensorsScanned guards bound a single request well; there's no per-user rate limit, so a client can still issue these back-to-back.

4. scattergl WebGL context exhaustion. _NodeChartTabContent.cshtml
Each open node's chart is a WebGL (scattergl) plot. Browsers cap live WebGL contexts (~16); with several nodes open simultaneously, older charts can be silently dropped by the browser. scatter (SVG) for these modest point counts (≤20×2000) would sidestep that, at some render cost. Minor — flagging in case multi-node dashboards are common.

🧹 Code quality / maintainability

5. Inline <script> duplicated per node. _NodeChartTabContent.cshtml
The whole IIFE (palette + all window.* handlers) is emitted inside the partial, so every opened node re-parses and re-declares window.loadNodeChart, onNodeChartPeriodChange, etc. It works (identical redefinition, DOM IDs are namespaced by nodeId), but the JS belongs in a static script file, with only per-node config passed via the existing data-* attributes. Keeps the payload small and the behavior in one place.

6. Minor

• renderNodeChart: when series.length === 0, showNodeChartMessage hides the note element, so an informational note (e.g. "only first 500 scanned") is never shown on an empty result. Edge case, low impact.
• The negative-count convention (NodeChartMaxPointsPerSensor = -2000, "negative = most recent within window") is subtle; it's documented, but a small helper/enum would read better than a magic negative int shared with LatestHistoryCount/SensorValuesCount.
• EffectiveUnitCode/EffectiveUnitLabel correctly branch on the same source and are only disambiguated together with Type in the group key — the XML comments call this out well; just make sure any future caller that uses EffectiveUnitCode without Type is aware (the two-enum overlap is a latent trap).

✅ Things done well

• Per-sensor try/catch so one faulting child can't fault Task.WhenAll and 500 the whole overlay.
• TryGetScalar drops NaN/Infinity so AllowNamedFloatingPointLiterals can't emit un-plottable "NaN" strings.
• Server- and client-side escaping of user-influenced sensor names before Plotly.
• ShowChartTab reset on every SelectNode to prevent the flag leaking across the reused instance.
• GetNodeRelativeLabel requires a / boundary to avoid prefix false-matches.

---

Automated review — please verify findings against intended behavior before acting.

[github-actions]

Code Review — PR #1241: Node Chart tab (overlay comparable child sensors, #1235)

Reviewed git diff origin/master...feature/1235-node-chart-tab. Overall this is a well-crafted, defensively-written change: bounded fan-out concurrency, per-sensor fault isolation, non-finite filtering, XSS-escaped labels, and a per-node request token are all handled thoughtfully, and the code/doc alignment is excellent. Findings below are mostly low-severity or things to confirm — I did not find a blocking correctness bug.

---

Security

1. No per-user product authorization on NodeChartHistory (IDOR) — verify intent. (Medium; pre-existing pattern, but amplified here)

NodeChartHistory resolves the node via the global _tree.Nodes/_tree.Sensors and reads all descendant sensor histories, with no check against user.IsProductAvailable(...):

if (!_tree.Nodes.TryGetValue(nodeId, out var node)) return _emptyJsonResult;
var groups = _tree.GetComparableChildGroups(nodeId);
...
_tree.Sensors.TryGetValue(sensorId, out var sensor)

The tree view is filtered per user (TreeViewModel.GetUserProducts → user.IsProductAvailable), but this endpoint bypasses that filter — the only gate is [Authorize] (authenticated, any product). An authenticated user scoped to product A who learns/guesses a node Guid in product B can read B's sensor history. The node-level fan-out amplifies the exposure versus the single-sensor endpoints: one id yields every descendant series.

Note this is consistent with the existing endpoints in the same controller — ChartHistory → TryGetSensor → _tree.Sensors.TryGetValue(...) has the identical gap. So this PR isn't introducing a new class of bug, but it does add a new, higher-leverage entry point. Please confirm whether product-level authorization is expected here; if so, gate on CurrentUser.IsProductAvailable(rootProductId(node)) before reading (ideally fixing the sibling endpoints too).

---

Correctness / UX

2. Per-sensor truncation to the newest 2000 points is silent (no note). (Low–Medium)

ReadNodeChartPoints reads NodeChartMaxPointsPerSensor = -2000, which resolves (via TreeValuesCache.GetSensorValuesPageInternal → DatabaseCore.GetSensorValues using GetValuesTo) to the 2000 most recent points in the window. For a dense sensor over a "Last day" window this can silently drop the earlier part of the window — the chart shows only the recent tail with no indication. Contrast with the sensor-count cap (MaxSensorsPerChart) and the scan cap (NodeChartMaxSensorsScanned), both of which append an explanatory note. The per-sensor point cap has no equivalent. Consider adding a note when any series hits the 2000 cap, or documenting that dense long windows are partial.

3. "Type" group-selector labels can go stale. (Low)

In _NodeChartTabContent.cshtml, updateNodeChartTypes rebuilds the <option> list only when the joined key signature changes:

const sig = groups.map(g => g.key).join('|');
if (sel.dataset.sig !== sig) { /* rebuild options */ }

But each option label embeds the group count (GetGroupLabel → "Integer bar, % (7)"). If a sensor is added/removed from a group so its count changes while the set of (type,unit) keys is unchanged, sig is identical and the displayed (N) never refreshes. The selected key still charts correctly — this is purely a stale-count display issue. Cheap fix: include counts (or the labels) in the signature.

---

Code quality / maintainability

4. 250-line inline <script> re-emitted per node panel. (Low)

_NodeChartTabContent.cshtml ships the full renderer inline, and _ChildrenPanel renders it every time a chart-capable node is selected. The IIFE reassigns window.loadNodeChart/onNodeChartPeriodChange on each partial load — functionally idempotent, but it re-parses/re-executes the whole block per selection and complicates any future CSP (script-src) tightening. Consider extracting to a bundled JS module and passing per-node config via data-* attributes (the container already carries data-url/data-node-id/data-req-token).

5. Top-20 selection ranks by Max(value) ("highest peak"). (Nit / by design)

built.OrderByDescending(s => s.Points.Max(p => p.Value)) picks the 20 largest-peak series. Series that are interesting when low (free memory, latency dips, negative rate deltas) can be dropped when a group exceeds 20. Reasonable for v1 and the note is honest ("highest-peak"), just worth keeping in mind for the ranking work in v2.

6. scattergl WebGL contexts. (Nit)

Traces use scattergl; Plotly.purge runs only on the error/no-data paths, not when navigating away. Because node selection replaces the panel, in practice only one chart is live at a time, so this is minor — but if multiple node panels ever coexist, browsers cap active WebGL contexts (~16). For the typical point counts here, scatter would also be fine and avoids the concern.

---

Things done well (kept, not asking to change)

• Bounded concurrency via SemaphoreSlim(8) with a lazy Select + Task.WhenAll; the "File is the only shared-read-state sensor and it's excluded" reasoning checks out (ToDisplayValue is pure; _fileHistoryLocks is only touched for FileSensorModel).
• Per-sensor try/catch so one faulting child is logged and dropped instead of 500-ing the whole overlay.
• TryGetScalar filters NaN/Infinity — correct given AllowNamedFloatingPointLiterals on the serializer, which would otherwise emit unplottable string literals.
• XSS: user-influenced sensor names are escaped before injection into Plotly text and <option> HTML; the unescaped unit in the hovertemplate is server-derived from a fixed enum display name, so it's safe.
• Per-node request token correctly prevents a later request for one node from blanking another node's chart (the global-counter bug called out in the comment).
• ShowChartTab reset in Subscribe prevents the flag leaking across reused view-model instances.
• Two comparability code paths (HasComparableChildGroup for tab visibility, GetComparableChildGroups for data) share IsComparableChartSensor and the same key, so they can't drift.

---

No blocking issues. Recommend confirming item 1 (authorization) before merge, and consider item 2's note. The rest are low-priority polish.

[github-actions]

Code Review — PR #1241: Node "Chart" tab (issue #1235)

Reviewed origin/master...feature/1235-node-chart-tab (12 files, +788/-54): a new node-level overlay endpoint (SensorHistoryController.NodeChartHistory), the comparable-group model + tree helpers, the ShowChartTab plumbing, and the _NodeChartTabContent view.

Overall this is a high-quality, well-reasoned PR. The code is unusually well-commented, the edge cases that are usually missed are handled deliberately (per-node request token, non-finite value filtering, bounded concurrency with correct semaphore release, per-sensor exception isolation, deterministic group ordering, XSS escaping on all user-influenced strings). I did not find any correctness-breaking bug. The notes below are mostly hardening / maintainability.

---

Security

1. No product-level authorization on the endpoint (Low–Medium — confirm tree scoping).
NodeChartHistory looks the node up in the global _tree.Nodes / _tree.Sensors (ConcurrentDictionary, all products) using only the [Authorize] class attribute — there is no ProductRoleFilter and no per-user access check. Any authenticated user who knows/guesses a node Guid can retrieve aggregated history for all of that node's descendant sensors.

This is consistent with the sibling ChartHistory/ChartHistoryLatest endpoints, which read any sensor by encoded id the same way, so the PR is not introducing a new pattern. But this endpoint widens the surface (one request → every descendant sensor), so it is worth confirming that either (a) TreeViewModel is effectively user/tenant-scoped in your deployments, or (b) an IDOR here is acceptable given the existing endpoints already expose the same data. If product ACLs are meant to be enforced, this and the existing history endpoints should apply the ProductRoleFilter* filters.

2. XSS — handled correctly. Sensor labels and group labels are escaped (nodeChartEscape) before injection into Plotly's pseudo-HTML name/hovertemplate and into sel.innerHTML; the server note is written via textContent. Good.

---

Performance

3. Heavy, uncached per-request fan-out.
A single request can read up to NodeChartMaxSensorsScanned (500) sensors × up to |NodeChartMaxPointsPerSensor| (2000) points = ~1M BaseValues materialized, converted, sorted, and serialized. The window is fully client-controlled (a "Custom" range or a crafted request can be arbitrarily wide), and the chart is re-fetched from scratch on every tab click, period change, and group change — there is no client debounce or server-side memoization. The constants do bound the worst case and concurrency is capped at 8, so this is acceptable for v1, but on a large node this is a noticeably expensive operation to trigger on a mere tab click. Consider a short client-side debounce and/or skipping the reload when the tab is re-clicked with unchanged parameters.

4. HasComparableChildGroup runs on every node selection.
It's called in HomeController.SelectNode for every node and walks the entire descendant sensor set via GetAllNodeSensors. It early-exits at the second match (good), but for a node whose first comparable pair appears only after thousands of descendants it walks them all synchronously during selection. Fine for typical trees; worth being aware of for very large nodes. (The comment already acknowledges the trade-off.)

5. Concurrency-safety claim verified. The comment justifying parallel reads ("comparable types exclude File, the only sensor kind with shared read state") is correct — I confirmed in TreeValuesCache.GetSensorValuesPageInternal that only FileSensorModel mutates shared state (_fileHistoryLocks); the comparable types iterate the DB independently. Semaphore acquisition is outside the try with Release() in finally, and the fast-path TryGetValue miss returns before acquiring — no leak, no over-release. 👍

---

Bugs / Logic

No functional bugs found. Details I checked and that hold up:

• Task.WhenAll over the lazy Select(async …) — bounded correctly by the semaphore; order is preserved so uncapped results stay in tree order.
• built.OrderByDescending(s => s.Points.Max(…)) — Points is guaranteed non-empty (empty windows return null and are filtered), so Max can't throw.
• ToGuid() on a null/invalid NodeId yields Guid.Empty → TryGetValue miss → empty result (no throw).
• ShowChartTab reset ordering is correct: ConnectNode → Subscribe sets it false, then HomeController sets the real value afterward; folder/sensor branches leave it false.
• Non-finite (NaN/Infinity) points are dropped so they can't serialize as JSON string literals that Plotly would reject.

6. Minor — "most recent" truncation depends on the shared cache path.
ReadNodeChartPoints derives truncated from rawValues.Count >= 2000 and the UI note says "their most recent N points." Whether the retained 2000 are actually the most recent depends on TreeValuesCache.GetSensorValuesPageInternal ordering for negative counts (it takes the first N of _database.GetSensorValues, which reads newest-first within a partition). This matches how the existing single-sensor chart behaves, so it's consistent — just flagging that the "most recent" wording inherits that assumption rather than being enforced locally.

---

Code quality / maintainability

7. Large inline <script> emitted per open node panel.
_NodeChartTabContent.cshtml carries ~180 lines of JS inside an IIFE that assigns window.loadNodeChart etc. Because _ChildrenPanel can render for multiple open nodes, this block is emitted once per panel, redefining identical window.* functions and re-declaring the palette each time. It's harmless (idempotent assignment), but it duplicates payload and defeats browser caching. Consider moving it to a shared static JS module (e.g. site.js/a dedicated file) and keeping only the per-node markup in the partial.

8. Nit — group-selector reset.
updateNodeChartTypes hides the "Type" selector when groups.length <= 1 but leaves its stale value/dataset.sig. A subsequent loadNodeChart would send the stale groupKey; the server falls back to groups[0] when the key isn't found, so it's harmless, but resetting the select when hiding it would be tidier.

9. Nit — doc wording. The comment in SelectedNodeViewModel.Subscribe says "The node branch of SelectNode re-enables it" — the re-enable actually happens in HomeController.SelectNode, not this class. Minor.

---

Verdict

Solid, defensive implementation with excellent inline documentation. No blocking bugs. The one item worth an explicit decision before merge is #1 (authorization) — confirm the global-tree lookup is acceptable given your access model (it matches existing history endpoints, so likely yes). The rest are optional hardening/cleanup.

[github-actions]

PR #1241 Review — Node Chart tab (issue #1235)

Reviewed origin/master...feature/1235-node-chart-tab. This adds a read-only Chart tab that overlays a node's comparable child sensors on one time chart: a new NodeChartHistory endpoint, (type, unit) grouping on TreeViewModel, a ShowChartTab flag, and an inline Plotly renderer.

Overall this is a clean, well-documented feature. The hard parts are handled well: per-node request tokens defeat response races, connectgaps: false keeps sparse series honest, labels are HTML-escaped before hitting Plotly's pseudo-HTML, non-finite points are dropped, reads are concurrency-bounded, and a single faulting child is isolated instead of 500-ing the whole overlay. Comments are unusually thorough. Findings below are mostly low/minor.

---

Security

1. No product-level authorization on NodeChartHistory (verify — likely consistent with existing endpoints).
SensorHistoryController.cs:296 looks the node up in _tree.Nodes, and _tree (TreeViewModel) is registered as a singleton (ApplicationServiceExtensions.cs:58) holding all nodes/sensors across every product. The endpoint has only [Authorize] — no IsProductAvailable/role check — so any authenticated user who knows (or guesses) a node GUID can read that node's child-sensor history regardless of their product access rights (IDOR).

This matches the rest of SensorHistoryController (ChartHistory, GetSensorPlotInfo, etc. do the same global lookup with no ACL), so it's very likely relying on an app-wide assumption rather than a new hole. But this PR extends that surface to batch, node-level history, so it's worth explicitly confirming that access control is enforced somewhere (e.g. the tree is effectively per-user, or GUIDs are treated as capabilities). If it isn't, this and the sibling endpoints are broken access control.

---

Correctness / Docs

2. Documentation contradicts the shipped behavior ("largest group only").
The method summary (SensorHistoryController.cs:194), the Swagger XML (HSMSwaggerComments.xml:481-485 and 494-499), and the NodeChartRequest XML all still say "v1 overlays the node's largest (type, unit) group only" / "fans out over the node's largest comparable child-sensor group." The actual feature ships a Type/group selector (GroupKey) that lets the operator chart any group — feature.md describes it correctly, but these three doc sites are stale and now misdescribe the API. Please update them to match the group-selector behavior.

3. truncated note can be a false positive.
ReadNodeChartPoints (SensorHistoryController.cs:412) sets truncated = rawValues.Count >= -NodeChartMaxPointsPerSensor (>= 2000). A series that has exactly 2000 points in the window (nothing actually dropped) still trips the "earlier data in the window may be omitted" note. Cosmetic, but the note claims data loss that didn't happen. Reading 2000 + 1 (and trimming) would let you distinguish "hit the cap" from "exactly filled it," if you care.

---

Maintainability / Performance (minor)

4. Palette comment is inaccurate. _NodeChartTabContent.cshtml:872 says "20 distinct colors — matches the server-side MaxSensorsPerChart cap so lines don't repeat," but MaxSensorsPerChart is 15, not 20. Functionally fine (extra colors), just a misleading comment.

5. Duplicated grouping traversal. GetComparableChildGroups and HasComparableChildGroup (TreeViewModel.cs:702 / 746) reimplement the same "walk descendants, group by (type, effectiveUnit)" loop. Also, HasComparableChildGroup's "cheap" claim is only partly true: it early-exits the comparability loop, but GetAllNodeSensors fully materializes the entire descendant list first, so the cost is dominated by the traversal either way. Consider a shared private helper (HasComparableChildGroup can short-circuit inside the same enumeration). And note this runs on every node selection (HomeController.cs:123), adding a full-subtree walk to selection latency for large trees.

6. Per-node <script> duplication. _NodeChartTabContent.cshtml embeds a full <script> that assigns global window.loadNodeChart / window.onNodeChartPeriodChange. It's rendered once per opened node panel, so opening multiple nodes re-defines those globals repeatedly. Harmless (the functions are nodeId-parameterized and identical), but the palette/handlers would be better in a shared static JS file included once.

7. Stale Type selector state on shrink. In updateNodeChartTypes (_NodeChartTabContent.cshtml:979), when a node drops from >1 group to exactly 1 group, the wrapper is hidden but the <select> keeps its old options and dataset.sig. Only relevant if a node's group set changes while its panel is open; low impact.

---

Nits

• NodeChartRequest lives in namespace HSMServer.Model.Model.History (doubled Model). Matches the pre-existing Model.Model.History namespace, so consistent, but the doubling reads like a typo.
• NodeChartRequest.NodeId being null is handled safely (ToGuid() → Guid.Empty → TryGetValue miss → empty result), good.

Nice work overall — the concurrency, race-token, and escaping details are done right. The main asks are (2) fixing the "largest group only" docs and (1) confirming the authorization assumption.

[github-actions]

PR #1241 Review — Node Chart tab (issue #1235)

Overall this is a well-crafted, carefully-commented feature. The endpoint bounds its DB fan-out, isolates per-sensor read failures instead of failing the whole overlay, escapes user-influenced labels before injecting into Plotly, and uses a per-node request token to avoid cross-node response clobbering. Nice work. A few things worth addressing before merge, none of them blockers.

Security / access control (medium)

NodeChartHistory performs no per-user authorization on the requested node. _tree is a global singleton (AddSingleton<TreeViewModel>()), and the endpoint resolves the node and all of its descendant sensors purely from a client-supplied Guid:

var nodeId = request.NodeId.ToGuid();
if (!_tree.Nodes.TryGetValue(nodeId, out var node)) ...
var groups = _tree.GetComparableChildGroups(nodeId);

Any authenticated user who supplies a node Guid gets back history and structural metadata (group list, counts, sensor-relative paths) for that node's entire subtree — regardless of whether they have product access to it. This mirrors the pre-existing pattern in ChartHistory/ExportHistory (which also look sensors up directly in the global tree with no access check), so it isn't a new class of gap — but this endpoint widens the blast radius: one node id now leaks the whole subtree's sensor data in a single call. Guids aren't trivially enumerable, which lowers the practical severity, but I'd recommend gating the node against the current user's accessible products (the same check the tree UI applies) here and, ideally, in the sibling endpoints too.

There is also no [ValidateAntiForgeryToken], but the endpoint is read-only and consistent with the other POST endpoints in the controller, so this is low priority.

Correctness / UX

Intermittent series won't actually render "with gaps". The view sets connectgaps: false and the help text promises that intermittent series (e.g. top-N processes reported only while active) are "drawn with gaps." But connectgaps only governs whether Plotly bridges null/NaN y-values — and the server never emits nulls; it simply omits idle periods. So consecutive real points on either side of an inactive stretch get joined by a straight line segment, producing a misleading diagonal/flat line rather than a visible gap. To get true gaps you'd need to insert null y-values where the sample interval exceeds some threshold. Otherwise consider softening the UI text so it doesn't over-promise.

Ranking candidates by current value is misleading for custom historical windows. The shortlist is chosen as follows (before any history read):

.OrderByDescending(s => TryGetScalar(s.LastValue, out var v) ? v : double.NegativeInfinity)
.Take(MaxSensorsPerChart)

The inline comment acknowledges this trades away "peaked-earlier-but-idle-now" sensors, which is reasonable for the default recent windows. But for a custom window in the past, "highest current value" is unrelated to what was interesting during that window — the operator may pick a window from last week and get charted the 15 sensors that happen to be hot right now. Worth at least documenting in the UI, or ranking by in-window activity for the custom case.

Maintainability

Misleading comment in SelectedNodeViewModel.Subscribe. The comment says the reset exists so the flag "can't leak across nodes/folders … folders/sensors leave it off," implying Subscribe covers the sensor case. But SelectNode's sensor branch (HomeController.cs:126) never calls ConnectNode/ConnectFolder, so Subscribe — and therefore the reset — never runs when a sensor is selected. This happens to be harmless only because the sensor view (_NodeDataPanel) doesn't render _ChildrenPanel (only _Node.cshtml/_Folder.cshtml do). That's a fragile, implicit coupling; consider either resetting ShowChartTab unconditionally in SelectNode or fixing the comment to state the real reason it's safe.

HasComparableChildGroup scans the full subtree on every node selection. It calls GetAllNodeSensors (which recurses the entire descendant tree into a list) and iterates it on each SelectNode. For large products (thousands of descendant sensors) this is an O(all-descendants) pass per click. The early-return on the second match helps in the common case, but a large group of distinct keys still walks everything. Probably fine in practice; flagging for awareness.

Things done well

• TryGetScalar correctly drops NaN/Infinity so they can't serialize as "NaN" string literals and break Plotly.
• The over-read-by-one trick to distinguish "hit the cap" from "window holds exactly N" is a nice touch and correctly implemented.
• Per-child read failures are logged and dropped rather than faulting Task.WhenAll.
• Deterministic tie-breaking on the group Key keeps the default group and selector order stable across requests.
• Label/group escaping guards against XSS via user-controlled sensor names in Plotly's pseudo-HTML.

🤖 Generated with Claude Code

[github-actions]

PR #1241 Review — Node Chart Tab (issue #1235)

Reviewed git diff origin/master...origin/feature/1235-node-chart-tab.

Overall this is a well-scoped, carefully implemented feature. The code is unusually well-commented — most non-obvious decisions (truncation over-read, semaphore bounding, gap insertion, unit-key ambiguity, request-token cancellation) are explained inline and the reasoning holds up. The XSS surface is handled correctly, the DB fan-out is bounded, and failure of a single child read is contained. Below are the issues I found, ordered by severity. Nothing here is a hard blocker, but a couple are worth addressing before merge.

---

1. No product-level authorization check on NodeChartHistory (Medium — but pre-existing pattern)

NodeChartHistory resolves the node from the global _tree and reads history for its descendants with no check that CurrentUser may access the node's product:

if (!_tree.Nodes.TryGetValue(nodeId, out var node))
    return _emptyJsonResult;
var groups = _tree.GetComparableChildGroups(nodeId);

Any authenticated user who knows/guesses a node Guid can retrieve up to 15 child sensors' recent history in one call, regardless of ProductsRoles. This matches the existing pattern in this controller (ChartHistory / GetSensorPlotInfo also skip product-level authz), so it is not newly introduced — but this endpoint makes bulk read easier than the per-sensor endpoints and is a good moment to consider whether these read endpoints should filter by CurrentUser.IsProductAvailable(...). At minimum, please confirm this is intentional and consistent with the product's threat model.

2. Candidate shortlist ranks by current value, ignoring the requested window (Low)

var candidates = group.SensorIds
    .Select(id => _tree.Sensors.TryGetValue(id, out var s) ? s : null)
    .Where(s => s is not null)
    .OrderByDescending(s => TryGetScalar(s.LastValue, out var v) ? v : double.NegativeInfinity)
    .Take(MaxSensorsPerChart)
    ...

The comment acknowledges the recent-window tradeoff, but for the Custom period the window can be entirely historical. In that case the "top 15 by current value" shortlist has no relationship to the window being charted — a node whose hot sensors have since gone idle can render an empty or misleading chart even though rich history exists. Consider noting this limitation in the UI for custom windows, or ranking historical windows differently. Not a correctness bug, but a real UX gotcha given the Custom option is exposed.

3. Ranking uses raw LastValue, chart uses display value (Low)

TryGetScalar(s.LastValue, ...) ranks candidates on the raw stored value, while ReadNodeChartPoints plots sensor.ToDisplayValue(raw). For Rate sensors (unit conversion between stored and display) the ranking scale differs from the plotted scale, so the "top 15" selection can disagree with what a user sees on the y-axis. Within a single (type, unit) group this is usually harmless, but it's an inconsistency worth a comment or aligning on the display value.

4. truncated flag is computed before non-finite filtering (Low / benign)

var truncated = rawValues.Count > maxPoints;   // counted before filtering
...
foreach (var raw in rawValues) { if (TryGetScalar(display, out var scalar)) points.Add(...); }

truncated counts raw values, but points may be smaller after dropping non-finite/non-scalar samples. So a series that returned 2001 raw values, 100 of them non-finite, ends up with 1901 plotted points yet still shows the "most recent 2000 points … earlier data omitted" note. Cosmetic only, but the note can be slightly inaccurate. Computing truncated from points.Count (or from whether the over-read extra survived) would make the note exact.

5. HasComparableChildGroup walks the full subtree on every node selection (Low / perf)

HomeController.SelectNode now calls _treeViewModel.HasComparableChildGroup(id) on every node click, and GetAllNodeSensors recurses the entire descendant subtree building a List<Guid> before HasComparableChildGroup even starts counting. For very large product trees this materializes every descendant id per selection just to answer a yes/no. The early-return in the counting loop helps, but the allocation in GetAllNodeSensors happens regardless. Consider a short-circuiting descendant walk if node selection latency matters on big trees. Given the existing code already calls GetAllNodeSensors elsewhere, this is minor.

---

Minor / nits

• renderNodeChart uses Plotly.newPlot on every reload rather than Plotly.react. newPlot tears down and rebuilds the plot each period/group change; react would be cheaper and smoother. Optional.
• Timezone display: points are emitted as UTC ISO strings and the hovertemplate labels them UTC, but Plotly renders type: 'date' axes tz-naively. Worth a quick manual check that axis ticks and hover both read as UTC to the operator (no off-by-timezone surprise).
• NodeChartRequest.From/To default to MinValue/MaxValue — if a client ever omits them, ToUtcKind just stamps the kind and the read spans everything up to MaxSensorsPerChart×2000 points. Fine in practice since the client always sends a window, but a server-side clamp/validation would harden the endpoint against direct callers.
• No From <= To validation — inverted windows silently return "no data". Acceptable, just noting.

---

Things done well (worth calling out)

• XSS handled correctly: sensor labels are escaped via nodeChartEscape before injection into Plotly's pseudo-HTML name/hovertemplate; group labels escaped in option markup; note set via textContent. Unit strings come from enum GetDisplayName(), not user input.
• DB fan-out is genuinely bounded: shortlist-before-read caps history reads at MaxSensorsPerChart, SemaphoreSlim caps concurrency, and per-series point count is capped with a clean over-read trick.
• Per-child read failures are caught and dropped rather than faulting Task.WhenAll and 500-ing the whole overlay.
• Per-node request token (vs a single global counter) correctly prevents cross-node response clobbering.
• EffectiveUnitCode/EffectiveUnitLabel branch on the same source, avoiding the null-key/mismatched-label trap that's explicitly documented.
• ShowChartTab reset in SelectNode prevents a previous node's flag leaking onto the next selection.

Nice work overall — recommend addressing #2/#4 (or documenting them) and confirming the authorization intent in #1.

[github-actions]

Review — PR #1241: Node chart tab (issue #1235)

Scope: new SensorHistory/NodeChartHistory endpoint that overlays a node's comparable child sensors on one Plotly chart, plus the tree/view-model plumbing to show a "Chart" tab and the client-side rendering.

Overall this is a well-engineered change. The comments are unusually thorough and correctly capture most of the non-obvious tradeoffs (over-read for truncation detection, per-node request token, gap insertion, non-finite filtering, group-key stability). I verified several things that looked risky and they turned out to be fine — most notably BaseValue.Time is already normalized to UTC by its getter, so the extra ToUniversalTime() in ReadNodeChartPoints is redundant but harmless, and the ShowChartTab reset in Subscribe correctly prevents a stale true leaking across node selections. No blocking bugs found. The notes below are things to consider.

---

1. Access control — endpoint returns history for any node GUID, no product-role check (Medium — please confirm)

SensorHistoryController.cs NodeChartHistory (and its helper GetSensorValues) look the node up in the global _tree/_cache and return every comparable descendant sensor's history, gated only by the controller's [Authorize] — there is no ProductRoleHelper/product-role check for the calling user.

This mirrors the existing ChartHistory/TableHistory endpoints (they also resolve an id against the global tree with no per-product check), so it's likely consistent with the app's current access model rather than a regression. Two reasons it's still worth an explicit decision here:

• The blast radius is larger: a single node GUID now fans out to many sensors' full history in one call, where ChartHistory exposed one sensor per known encodedId.
• If HSM intends product-level read isolation (users only hold roles on some products), this endpoint — like the others — does not enforce it.

Suggested: confirm whether product-level read authorization is expected for history endpoints; if so, add the same check used elsewhere for the resolved node.RootProduct against CurrentUser.ProductsRoles.

2. Full descendant traversal on every node selection (Low–Medium, performance)

HomeController.SelectNode calls HasComparableChildGroup(id) for every node click (TreeViewModel.cs:154), which calls GetAllNodeSensors(nodeId). GetAllNodeSensors eagerly builds the complete descendant-sensor list for the whole subtree before iteration, so the early return true at the second matching group saves only the dictionary work, not the traversal. For a large root product (thousands of descendant sensors) this walks the entire subtree on each selection, including for nodes that will never show the tab.

Not incorrect, and node selection already does some O(children) work, but consider short-circuiting inside the traversal (a variant of GetAllNodeSensors that stops once two comparable sensors of the same key are seen) if selection latency on big trees matters.

3. Truncation note can false-positive (Low, cosmetic)

ReadNodeChartPoints (SensorHistoryController.cs:326) sets truncated = rawValues.Count > maxPoints from the raw count, but non-finite points are dropped afterward. A series that read 2001 raw values of which many were NaN/Infinity can report truncated = true (and surface the "earlier data omitted" note) even though the displayed series is well under the cap. Low impact — the note is advisory — but computing truncated after the finite-filter would be more accurate.

4. Shortlist by current value can hide the true in-window peak (informational)

Candidates are picked by LastValue before any history read (SensorHistoryController.cs:232), then the rendered lines are re-ordered by in-window peak. The comment already documents this tradeoff (a series that peaked earlier in the window but is idle now can fall outside the top-15). Flagging only so it's a conscious product decision: for the "custom / older window" case this can silently omit the most interesting series. Consider making the note wording reflect "highest current value" clearly in the UI (it currently does — good).

5. Minor / nits

• CSRF: NodeChartHistory is [HttpPost] with no antiforgery token, consistent with the other POST history endpoints. It's read-only so impact is limited; noting for completeness.
• scattergl (WebGL): each open node chart creates a WebGL context; browsers cap live contexts (~16). With many nodes' Chart tabs opened in one session this could hit the limit. scatter (SVG) is safe below a few thousand points; scattergl is the right call for dense series, just be aware of the context ceiling.
• XSS: sensor labels and group labels are escaped via nodeChartEscape before injection into Plotly text/options — good. unit is interpolated unescaped into hovertemplate/axis title, but it derives from server-side enum GetDisplayName(), not user input, so it's safe.
• Redundant ToUniversalTime() in ReadNodeChartPoints — BaseValue.Time already returns UTC. Harmless; could drop for clarity.

---

Recommendation: Approve pending a decision on point #1 (access control posture). Points #2–#5 are non-blocking follow-ups.