Skip to content

[O2B-1463] Refactor logic of detector fetching for RCT and hide legacy detector for periods passes 25 onwards - #2211

Draft
graduta wants to merge 10 commits into
mainfrom
feature/O2B-1463/hide-legacy-detector-for-periods-passes-25-onwards
Draft

[O2B-1463] Refactor logic of detector fetching for RCT and hide legacy detector for periods passes 25 onwards#2211
graduta wants to merge 10 commits into
mainfrom
feature/O2B-1463/hide-legacy-detector-for-periods-passes-25-onwards

Conversation

@graduta

@graduta graduta commented Aug 10, 2026

Copy link
Copy Markdown
Member

I have a JIRA ticket

  • branch and/or PR name(s) include(s) JIRA ID
  • issue has "Fix version" assigned
  • issue "Status" is set to "In review"
  • PR labels are selected

Notable changes for users:

  • CPV & PHS detectors are not to be displayed for data passes or lhc periods from LHC25 onwards.

Notable changes for developers:

  • refactor the fetching of detectors and conditional so that it is reused across the 3 classes extending RunsWithQcModel

Changes made to the database:

@graduta
graduta requested a lite review from Copilot August 10, 2026 12:27
@graduta graduta self-assigned this Aug 10, 2026
@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.00%. Comparing base (4391066) to head (f634769).

Files with missing lines Patch % Lines
lib/public/views/Runs/Overview/RunsWithQcModel.js 0.00% 21 Missing ⚠️
...uns/RunPerDataPass/RunsPerDataPassOverviewModel.js 0.00% 6 Missing ⚠️
...Runs/RunPerPeriod/RunsPerLhcPeriodOverviewModel.js 0.00% 5 Missing ⚠️
...ilities/filterOutLegacyDetectorsForNewerPeriods.js 0.00% 4 Missing ⚠️
...mulationPass/RunsPerSimulationPassOverviewModel.js 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2211      +/-   ##
==========================================
- Coverage   46.23%   46.00%   -0.24%     
==========================================
  Files        1039     1040       +1     
  Lines       17142    17163      +21     
  Branches     3124     3133       +9     
==========================================
- Hits         7925     7895      -30     
- Misses       9217     9268      +51     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors how RCT detectors are fetched/derived across runs overview models and introduces logic intended to hide legacy detectors (CPV, PHS) starting from LHC25-period/data-pass labels.

Changes:

  • Added filterOutLegacyDetectorsForNewerPeriods utility (and tests) to remove CPV/PHS for labels matching LHC25+.
  • Centralized detectors-observable construction in RunsWithQcModel via _setDetectorsObservable/_buildDetectorsObservable, and updated the three overview models to reuse it.
  • Updated utilities index exports to surface the new filtering helper.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
test/lib/utilities/stringUtils.test.js Adds unit test coverage for the new legacy-detector filtering helper.
lib/utilities/stringUtils.js Implements filterOutLegacyDetectorsForNewerPeriods and exports it.
lib/utilities/index.js Re-exports the new helper from the utilities index.
lib/public/views/Runs/RunsPerSimulationPass/RunsPerSimulationPassOverviewModel.js Switches detectors handling to the shared observable builder + applies legacy-detector filtering.
lib/public/views/Runs/RunPerPeriod/RunsPerLhcPeriodOverviewModel.js Reuses the shared observable builder + applies legacy-detector filtering before type filtering.
lib/public/views/Runs/RunPerDataPass/RunsPerDataPassOverviewModel.js Reuses the shared observable builder + applies legacy-detector filtering alongside existing CPASS filtering.
lib/public/views/Runs/Overview/RunsWithQcModel.js Introduces shared detectors observable builder and makes detectors getter concrete.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/public/views/Runs/RunPerDataPass/RunsPerDataPassOverviewModel.js Outdated
Comment thread lib/public/views/Runs/RunPerPeriod/RunsPerLhcPeriodOverviewModel.js Outdated
Comment thread lib/public/views/Runs/RunsPerSimulationPass/RunsPerSimulationPassOverviewModel.js Outdated
Comment thread lib/public/views/Runs/Overview/RunsWithQcModel.js Outdated
Comment thread lib/public/views/Runs/Overview/RunsWithQcModel.js

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

lib/public/views/Runs/Overview/RunsWithQcModel.js:273

  • RunsWithQcModel.detectors now silently initializes _detectors$ to RemoteData.notAsked() when it hasn't been configured. This masks misconfiguration and prevents the default detector source (rctDetectorsProvider.qc$) from ever loading. Consider initializing via _setDetectorsObservable() so the default behavior is consistent and detectors will load when first accessed.
    get detectors() {
        if (!this._detectors$) {
            this._detectors$ = new ObservableData(RemoteData.notAsked());
            this._detectors$.bubbleTo(this);
        }

        return this._detectors$.getCurrent();
    }

lib/public/utilities/filterOutLegacyDetectorsForNewerPeriods.js:22

  • The JSDoc lists a legacyDetectorNames parameter, but the function signature does not accept it (the list is currently hard-coded in LEGACY_DETECTOR_NAMES_FOR_AFTER_25). This mismatch can mislead callers and tooling.
 * @param {Detector[]} detectors detectors to filter
 * @param {string} [label=''] name of the period or data pass
 * @param {string[]} [legacyDetectorNames=['CPV', 'PHS']] detectors to remove for newer periods after LHC25
 * @return {Detector[]} filtered detectors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants