QC plots ignore date range when showing guide set with many replicates

[ankurjuneja]

Rationale

Related Pull Requests

Changes

[ankurjuneja]

this is the most important change that fixes the issue, one of the operands is string and other is number

[labkey-jeckels]

Please change to do an explicit type conversion via parseInt with a comment about the one side being a string. Otherwise we're at high risk of regression.

[labkey-jeckels]

I did review with Claude. Can you can check if any of these findings are worth addressing? I didn't dive deep and try to repro them, but I find them plausible given my reading of the code. (Note that I already added finding #3 based on my own review, hence the jump from 2 to 4 in the list).

Finding #1 - [Severity: Medium] — Logic — webapp/TargetedMS/js/QCPlotHelperBase.js:333-344

Issue: The == fix at line 335 makes the if branch reachable for the first time, but the Ext4.Object.each loop over this.guideSetDataMap sets plotData['ReferenceRangeSeries'] = "InRange" in the else branch on every non-matching guide set. When the map contains more than one guide set, any point labeled "GuideSet" by an earlier key is clobbered back to "InRange" by a later non-matching key (integer-like keys iterate in ascending order, and the qualifying guide set — the one with TrainingEnd <= startDate — is typically an older, lower ID). The label survives only if the matching guide set happens to be the last key.
Why it matters: ReferenceRangeSeries is used as the groupBy to split the trend line (QCPlotHelperBase.js:958-961). In any folder with multiple guide sets — common in long-running QC folders — the separation breaks and the line draws straight across the truncation gap, which is part of what this PR is trying to render correctly.
Suggestion: Stop iterating once a match is found (return false from the each callback), or initialize plotData['ReferenceRangeSeries'] = "InRange" before the loop and only ever set "GuideSet" inside it.

Finding #2 - [Severity: Medium] — Logic — webapp/TargetedMS/js/QCPlotHelperBase.js:367-392

Issue: The outer loop iterates plotDataRows, and for each row runs Ext4.Object.each over all metric entries in this.filterPoints[SeriesLabel]. In multi-metric mode, two plotDataRows share the same SeriesLabel (one per metric), so each filterPointsData entry gets processed twice. The processing is not idempotent: the else branch mutates filterPointsLastIndex -= 6 each pass. On the second pass, an entry that legitimately had ≥6 out-of-range points (e.g., 8) now shows a gap of 2 and gets skipTruncation = true — so it is never truncated. With the old code the same double-processing set filterQCPoints = false globally, which may be exactly the reported bug; the new code narrows the blast radius but the multi-metric ("Show metric values"/two-plot-type) case still fails to truncate.
Why it matters: The bug this PR fixes can still reproduce when two metrics are plotted together, and filterPointsLastIndex can be decremented by 12 instead of 6, truncating 6 extra points in cases that do truncate.
Suggestion: Process each (SeriesLabel, metricId) pair exactly once — e.g., iterate this.filterPoints directly instead of via plotDataRows, or short-circuit entries that already have skipTruncation set / have already been adjusted (a simple processed flag on filterPointsData works).

Finding #4 - [Severity: Low] — Pre-existing edge case, adjacent to changed code — webapp/TargetedMS/js/QCPlotHelperBase.js:374,382

Issue: Two pre-existing issues now sit on the newly-live code path: (a) the truthiness checks if (filterPointsData['filterPointsFirstIndex'] && filterPointsData['filterPointsLastIndex']) (and the !...['filterPointsLastIndex'] guard
at line 348) treat a legitimate index of 0 as "unset" — filterPointsFirstIndex is always ≥ 1 (j + 1), but filterPointsLastIndex can be 0 when the first data point is already in range; (b) the indices are computed against this.fragmentPlotData[frag].data (which includes injected type: 'missing' date entries, and in multi-metric mode both metrics' points concatenated), but lines 382/388 use them to index plotDataRows[i].data (raw, per-metric, no missing entries) — so the AcquiredTime used to update the start-date field can come from the wrong point, or be a TypeError on undefined.AcquiredTime if the index lands past the raw array's end.
Why it matters: These were unreachable before the ===→== fix; now they execute on every truncation render.
Suggestion: Use !== undefined checks for the index guards, and look up the start-date point from the same array the indices were computed from (this.fragmentPlotData[label].data), skipping type === 'missing' entries.

Finding #5 - [Severity: Low] — Logic — webapp/TargetedMS/js/QCTrendPlotPanel.js:2407-2412

Issue: The new seqValue-range matching is correct for single-metric plots, but in multi-series mode precursorInfo.data holds both metrics' points with repeated seqValues, so expDataArr mixes values from two different metrics into a single mean/std-dev/%CV for the experiment-range tooltip. (The old index-based slice was differently wrong, so this is not a regression — but the new comment makes the code look intentional and correct.)
Suggestion: If multi-series is reachable with showExpRunRange, also match on the metric (e.g., pointsData[i].MetricId) when collecting values, or restrict the stats to the primary metric.

[labkey-jeckels]

Please change to do an explicit type conversion via parseInt with a comment about the one side being a string. Otherwise we're at high risk of regression.

[ankurjuneja]

Confirmed — Finding #1 is a real bug. ReferenceRangeSeries (set in that loop) is used as the groupBy that splits the trend line (line 960). In the loop, the else branch unconditionally writes "InRange" on every non-matching guide set, so when guideSetDataMap holds more than one guide set, a later non-matching key overwrites the "GuideSet" label set by the matching key. The label only survives if the matching guide set is the last key iterated.

Fixing it minimally — default to "InRange" before the loop, promote to "GuideSet" only on a match, and stop iterating once matched.

Finding #2 is not an actual bug in this codebase. Its premise is incorrect.

The premise: "In multi-metric mode, two plotDataRows share the same SeriesLabel (one per metric), so each filterPointsData entry gets processed twice."

What the server actually returns (OutlierGenerator.getQCPlotFragment, lines 545-578): fragments are grouped into a Map<String, List> keyed by SeriesLabel, and exactly one QCPlotFragment is emitted per map entry (response.put("plotDataRows", qcPlotFragments…)). So plotDataRows has unique SeriesLabel values — no two rows share one.

In multi-metric mode the two metrics don't become two rows; they live inside a single fragment's data array, each point carrying its own MetricId (QCPlotFragment.toJSON line 153). That's exactly what the "repeated dates… all in the same array" comment at line 257 describes — one row, both metrics concatenated.

Consequence for the loop (367-392):

• Outer loop visits each SeriesLabel once.
• Inner Ext4.Object.each(seriesPoints, …) visits each metricId under that label once.
• So each (SeriesLabel, metricId) pair — i.e. each filterPointsData — is processed exactly once. filterPointsLastIndex -= 6 runs a single time; there's no path to -= 12 or to a spuriously-set skipTruncation from re-entry within one processPlotData call.

Finding #4(a) — index 0 treated as "unset": real but functionally harmless

The truthiness observation is correct: filterPointsFirstIndex is always j + 1 so it's ≥ 1 (the && guard at line 374 is fine for it), while filterPointsLastIndex is assigned raw j and can be 0, which !... and && read as unset.

But trace when filterPointsLastIndex === 0 actually happens: only when the first data point (j = 0) already satisfies fullDate >= startDate — i.e. there are no points before startDate. Since data is date-sorted ascending and a qualifying guide-set training point requires TrainingEnd <= startDate (so it sorts before startDate), that case means no training point precedes the range, so filterPointsFirstIndex is never set. The combined if (firstIndex && lastIndex) then short-circuits on the missing firstIndex and the branch is skipped. So lastIndex === 0 and a set firstIndex cannot co-occur. No observable bug today — !== undefined guards would be correct hygiene but change no behavior.

Finding #4(b) — index-space mismatch: real, and the meaningful half — but the proposed fix is itself wrong

The mismatch is genuine:

• Indices are computed in the loop at lines 320-352 against precursorInfo.data = this.fragmentPlotData[frag].data. That array has type:'missing' placeholders spliced in (lines 280-307), and crucially j counts those missing entries even though line 328 continues past them before labeling.
• Lines 382/388 then apply those indices to plotDataRows[i].data — the raw server array, which has no missing entries.

Since fragmentPlotData[frag].data is a superset (raw + injected missing), its indices run ahead of the raw array. Result: wrong AcquiredTime, or plotDataRows[i].data[idx] is undefined → undefined.AcquiredTime TypeError, whenever any dates were injected. And yes — this path was dead before the == fix and is now live on every showExpRunRange truncation render.

Two corrections to the finding, though:

1. The multi-metric concern is a non-issue for the mismatch. Both arrays carry the two metrics concatenated identically (the raw server data already contains both metrics; fragmentPlotData is built from it in the same order — and there's exactly one fragment per SeriesLabel, as established earlier). The only divergence between the two arrays is the injected missing dates. So the trigger is missing dates, not multi-metric.
2. The suggested fix — "look up from this.fragmentPlotData[label].data" — would break. Those processed objects don't carry AcquiredTime; processPlotDataRow (QCPlotHelperWrapper.js:312-368) only stores fullDate/date (formatted strings), never AcquiredTime. Reading .AcquiredTime off them yields undefined. The current code uses plotDataRows[i].data precisely because the raw rows are where AcquiredTime lives (set at QCPlotHelperBase.js:216).

So the correct fix is not to switch arrays, but to translate the index into raw space (subtract the count of type:'missing' entries at/before the index) — or, equivalently, store AcquiredTime on the processed object and then index fragmentPlotData[frag].data while skipping missing entries.

Verdict: (a) accept as cosmetic/no-op; (b) accept as a real latent crash on the now-live path, but the proposed remedy is incorrect.

What changed (QCPlotHelperBase.js):

• The two start-date lookups in the truncation block no longer index the raw plotDataRows[i].data array with a fragmentPlotData-space index. Both now call a new helper, setStartDateFromFilterIndex(plotDataRow, fragIndex).
• The helper:
  a. Pulls the same array the indices were computed against — this.fragmentPlotData[label].data.
  b. Clamps the index into bounds, then walks back past any type:'missing' placeholder so it lands on a real point.
  c. Translates to raw-space by counting non-missing entries before that point (missing entries exist only in fragmentPlotData, so this count is exactly the corresponding raw index).
  d. Looks up plotDataRow.data[rawIndex] and only calls setValue if a real point with an AcquiredTime exists.

Finding #5 is valid and reachable (experimental-run-link mode + user-added second metric), and the blended statistic is real.

Fix applied (QCTrendPlotPanel.js:2407-2415): added && pointsData[i].MetricId === this.metric to the experiment-range collection loop, with a comment explaining why. Type note: both sides are numbers (pointsData[i].MetricId from the server
JSON via QCPlotHelperWrapper:327; this.metric from validateMetricId's parseInt), so === is correct here — unlike the guide-set-map case in Finding #3 where one side was forced to a String by JS object-key semantics.