feat: add charts, date filters, snapshot storage, and UI improvements

[jjroelofs]

Summary

This PR adds visualization charts, historical data tracking, and various improvements to the RL module.

Features Added

• Plotly.js charting: 2D line charts (≤10 variants) and 3D surface charts (>10 variants)
• Snapshot storage system: Historical data tracking for chart visualization
• Date range filters: Presets (1 day, 5 days, 1-24 weeks, this month/quarter/year)
• X-axis toggle: Switch between Impressions, Daily, Weekly, Monthly, Quarterly views
• Adaptive lighting: 3D charts adjust lighting based on data variance
• Arm data validation service: Validates data integrity before Thompson Sampling

UI/UX Changes

• Renamed terminology: Arms→Variants, Turns→Impressions, Rewards→Conversions
• Added Conversions column to experiments overview table
• Moved RL Settings to Configuration → Web services
• Loading spinner during chart filter changes

Technical Improvements

• Migrated from drupal-check to PHPStan for static analysis
• Added hook_requirements check for rl.php accessibility and Plotly library
• Use Asset Packagist for Plotly.js (npm-asset/plotly.js-dist-min)
• DRY refactoring of ReportsController (~8% smaller) and JS (~13% smaller)
• Added Nginx configuration documentation
• Snapshot cleanup on experiment deletion

Files Changed

• New: composer.json, js/rl-plotly-charts.js, js/rl-date-filter.js, css/rl-charts.css, templates/rl-charts.html.twig
• New services: ArmDataValidator, SnapshotStorage
• Updated: ReportsController, ExperimentDataStorage, rl.install, rl.module

Testing

• Verified charts render correctly for experiments with various arm counts
• Tested date filters and axis toggles
• All PHPStan and PHPCS linters pass

[jjroelofs]

Comprehensive Code Review (Updated v2)

Overview

PR Title: fix: resolve all lint issues and drupal-check configuration
Stats: +2,210 additions / -82 deletions across 28 files / 20 commits

This PR has grown beyond its original scope. The title and description should be updated to reflect the actual changes:

• Lint fixes + PHPStan migration
• Snapshot storage system for historical data
• Plotly.js charting integration (2D line charts, 3D surface)
• Arm data validation service
• UI terminology changes (Arms→Variants, Turns→Impressions, Rewards→Conversions)
• hook_requirements check for rl.php accessibility
• Nginx configuration documentation
• New CSS/Twig templates for charts

Recommendation: Update the PR title and description to accurately reflect the full scope of changes.

---

Critical Issues

1. Missing hook_uninstall for Snapshot Table

File: rl.install:193-219

The rl_arm_snapshots table is added in hook_schema() (line 362) and created via rl_update_10001(), but is NOT dropped in hook_uninstall(). This leaves orphaned tables after module uninstall.

function rl_uninstall() {
  // Only drops: rl_experiment_totals, rl_arm_data, rl_experiment_registry
  // Missing: rl_arm_snapshots
}

2. @phpstan-ignore Suppressions Hide Real Issues

File: src/Controller/ReportsController.php:114-115, 322

// @phpstan-ignore new.static
return new static(

// @phpstan-ignore argument.unresolvableType, argument.unresolvableType
usort($arm_data, static function (array $a, array $b) use ($sort_field, $sort): int {

These suppressions hide valid static analysis warnings rather than fixing the underlying issues. The new static() warning exists because static factories in non-final classes can cause inheritance issues.

3. hook_requirements Creates Real Database Entries

File: rl.install:448-458

$response = $client->post($rl_endpoint_url, [
  'form_params' => [
    'action' => 'turns',
    'experiment_id' => 'status_check',
    'arm_ids' => '1',
  ],

Every time the status report page is viewed, this creates a real experiment entry (status_check) in the database. Use a read-only check or dedicated test action instead.

4. Direct Database Queries in Controller

File: src/Controller/ReportsController.php:146-153, 157-161, 247-262

Controllers should delegate database operations to storage services. Multiple direct $this->database->select() calls violate separation of concerns:

$query = $this->database->select('rl_experiment_registry', 'er')
  ->fields('er', ['experiment_id', 'module', 'experiment_name', 'registered_at']);

This logic belongs in ExperimentDataStorage or a dedicated repository service.

---

Major Issues

5. Large External Library Committed to Repository

File: libraries/plotly/plotly.min.js

Plotly.js (~3.5MB minified) is committed directly. This should use Asset Packagist:

{
  "require": {
    "npm-asset/plotly.js-dist-min": "^2.35"
  }
}

Or load via CDN with a fallback.

6. Potential XSS Vulnerability

File: src/Controller/ReportsController.php:295, 332

$arm_name = $arm_display ? $this->renderer->renderInIsolation($arm_display) : $arm->arm_id;
// ...
$rows[] = [
  ['data' => ['#markup' => $data['arm_name']]], // Line 332

When $arm_display is falsy, $arm->arm_id is passed directly to #markup without sanitization. If arm IDs contain user-controlled content, this is an XSS vector. Use Html::escape() or '#plain_text'.

7. Dead Code

File: src/Controller/ReportsController.php:535-648

Four methods are defined but never called:

• determineTimeGranularity() (line 535)
• sampleBeta() (line 598)
• sampleGamma() (line 614)
• sampleNormal() (line 642)

These duplicate functionality in ThompsonCalculator and should be removed.

8. JavaScript Uses Outdated Standards

File: js/rl-plotly-charts.js

Uses var throughout instead of const/let. Drupal 10+ expects modern JavaScript:

// Current:
var showLineChart = numArms >= 1 && numArms <= 10;
var showLandscape = numArms > 10;

// Should be:
const showLineChart = numArms >= 1 && numArms <= 10;
const showLandscape = numArms > 10;

---

Minor Issues

9. Magic Numbers Without Configuration

File: js/rl-plotly-charts.js:158-159

var showLineChart = numArms >= 1 && numArms <= 10;
var showLandscape = numArms > 10;

The threshold (10) should be configurable or at minimum documented with rationale.

10. Outdated Comment

File: js/rl-plotly-charts.js:172

// 1. 2D Line Chart - Conversion Rate Over Time (1-7 arms)

Comment says "1-7 arms" but the actual logic on line 158 uses 10 as the threshold. Update comment to match code.

11. Inconsistent Error Handling in Validator

File: src/Service/ArmDataValidator.php:48-95

• Throws RuntimeException for negative turns/rewards
• Silently sanitizes when rewards > turns (with logging)

The asymmetric behavior is confusing. Consider consistent handling (either always throw or always sanitize with logging).

12. Missing Return Types

File: src/Form/RlSettingsForm.php:17, 23

public function getFormId()  // Missing : string
protected function getEditableConfigNames()  // Missing : array

PHP 8+ return type declarations should be added for consistency.

13. Inconsistent Indentation in create() Method

File: src/Controller/ReportsController.php:116-124

return new static(
      $container->get('database'),  // 6 spaces
      $container->get('rl.experiment_data_storage'),

Uses 6-space indentation instead of standard 2 or 4 spaces.

---

Testing Concerns

• No test files included
• New services (ArmDataValidator, SnapshotStorage) lack unit tests
• JavaScript charting logic has no test coverage
• The hook_requirements HTTP check should be tested

---

Summary

Fix before merge:

• Add rl_arm_snapshots to hook_uninstall()
• Fix or properly document PHPStan suppressions
• Change hook_requirements to use read-only check
• Sanitize arm_id before #markup
• Remove dead code (lines 535-648)
• Remove Plotly.js from repo and use Asset Packagist or CDN
• Fix outdated comment on line 172 (says "1-7" should be "1-10")

---

Category	Rating	Notes
Drupal Standards	6/10	Direct DB calls in controller, missing uninstall cleanup
Code Quality	5/10	Dead code, magic numbers, PHPStan suppressions
Security	7/10	XSS concern, status check creates data
Performance	7/10	Reasonable bounds on snapshot storage
Maintainability	6/10	No tests, bundled library

(This review supersedes all previous reviews)

[jjroelofs]

Follow-up Review: Additional Findings

Good News

Plotly.js is now loaded from CDN - The bundled library was removed and it's now loaded externally via rl.libraries.yml:11:

https://cdn.plot.ly/plotly-2.35.2.min.js: { type: external, minified: true }

This addresses the original review concern about committing large external libraries.

---

New Issues Found

1. Duplicate Date Filter Rendering (Bug)

File: templates/rl-charts.html.twig:18-22, 32-36

The date_filter is rendered twice - once in the 2D chart box and once in the 3D chart box. Since only one chart is visible at a time (based on arm count), users see duplicate filter controls.

{# Line 18-22: Filter in 2D box #}
{% if date_filter %}
  <div class="rl-chart-filters">{{ date_filter }}</div>
{% endif %}

{# Line 32-36: Same filter duplicated in 3D box #}
{% if date_filter %}
  <div class="rl-chart-filters">{{ date_filter }}</div>
{% endif %}

Fix: Move the date filter outside both chart boxes, rendering it once at the top of the template.

2. Misleading Variable Names

Files: ReportsController.php:471-472, js/rl-plotly-charts.js:149

Ridgelines were removed but variable names still reference them:

// Prepare ridgeline data for Plotly 3D (up to 100 arms).
$ridgeline_data = ['arms' => []];

Suggestion: Rename to $chart_data / chartData or $arms_data / armsData.

3. Missing Snapshot Cleanup on Experiment Deletion

When an experiment is deleted, the corresponding rl_arm_snapshots rows should also be deleted. Verify that experiment deletion cascades to snapshot cleanup.

4. Unused CSS Selector

File: css/rl-charts.css:123-125, 159-161, 184-186

.rl-chart-box h4 {
  font-size: 15px;
}

The template doesn't have <h4> elements inside .rl-chart-box - chart titles are now set via Plotly's config. This CSS is dead code.

---

Summary

Finding	Severity
Duplicate date filter	Medium - visible UI bug
Misleading variable names	Low - code clarity
Missing snapshot cleanup	Medium - data integrity
Unused CSS selector	Low - dead code

The duplicate date filter is the most user-visible issue to address.

[jjroelofs]

DRY Suggestions for JavaScript

The PHP side is now clean. Here are remaining opportunities in js/rl-plotly-charts.js:

---

1. getResponsiveConfig() - 83 lines → ~25 lines

Lines 41-124

Five nearly identical config objects. Use a breakpoint table:

function getResponsiveConfig() {
  const width = window.innerWidth;
  const height3d = getContainerHeight('rl-plotly-3d-surface');
  const height2d = getContainerHeight('rl-plotly-2d-lines');

  // [maxWidth, fontSize, titleSize, axisTitleSize, tickSize, margins, camera, colorbarLen, colorbarThickness, maxLabelLength]
  const breakpoints = [
    [430,  10, 13, 11, 9,  [50,30,40,60],   [2.0,-2.0,1.2],  0.6,  15, 20],
    [768,  11, 14, 12, 10, [60,35,45,80],   [1.8,-1.9,1.0],  0.7,  18, 30],
    [1200, 12, 15, 13, 11, [70,40,50,90],   [1.7,-1.8,0.95], 0.75, 20, 40],
    [1920, 13, 16, 14, 11, [80,40,50,100],  [1.6,-1.8,0.9],  0.8,  20, 50],
    [Infinity, 14, 18, 15, 12, [100,50,60,120], [1.5,-1.7,0.85], 0.85, 25, 60]
  ];

  const bp = breakpoints.find(b => width <= b[0]);
  return {
    height: height3d,
    height2d: height2d,
    fontSize: bp[1],
    titleSize: bp[2],
    axisTitleSize: bp[3],
    tickSize: bp[4],
    margin: { l: bp[5][0], r: bp[5][1], t: bp[5][2], b: bp[5][3] },
    camera: { eye: { x: bp[6][0], y: bp[6][1], z: bp[6][2] } },
    colorbarLen: bp[7],
    colorbarThickness: bp[8],
    maxLabelLength: bp[9]
  };
}

Savings: ~58 lines

---

2. X-Axis Config - Extract Helper

Lines 210-223 and 328-339 (duplicated logic)

function buildXAxisConfig(data, config, includeGrid) {
  const xAxisLabel = data.xAxisLabel || 'Total Impressions';
  const axisConfig = {
    title: { text: xAxisLabel, font: { size: config.axisTitleSize } },
    tickfont: { size: config.tickSize }
  };
  if (includeGrid) {
    axisConfig.gridcolor = 'rgba(0,0,0,0.1)';
  }
  if (data.xLabels && data.timeAxis !== 'trials') {
    axisConfig.tickvals = Object.keys(data.xLabels).map(Number);
    axisConfig.ticktext = Object.values(data.xLabels);
    if (includeGrid) axisConfig.tickangle = -45;
  }
  return axisConfig;
}

// Usage:
const xAxisConfig = buildXAxisConfig(data, config, true);   // 2D
const xAxis3dConfig = buildXAxisConfig(data, config, false); // 3D

Savings: ~12 lines

---

3. Statistics Calculation - Single Pass

Lines 341-366 (two loops over same data)

Combine min/max/sum/variance into one pass:

// Single pass for all statistics
let zMin = Infinity, zMax = -Infinity, zSum = 0, zSumSq = 0, zCount = 0;
for (let ai = 0; ai < sortedZMatrix.length; ai++) {
  for (let ti = 0; ti < sortedZMatrix[ai].length; ti++) {
    const val = sortedZMatrix[ai][ti];
    if (val < zMin) zMin = val;
    if (val > zMax) zMax = val;
    zSum += val;
    zSumSq += val * val;
    zCount++;
  }
}
const zMean = zSum / zCount;
const zVariance = (zSumSq / zCount) - (zMean * zMean);
const zStdDev = Math.sqrt(Math.max(0, zVariance));

Savings: ~10 lines

---

4. armIndices Array - One-liner

Lines 287-290

// Before (4 lines):
const armIndices = [];
for (let i = 0; i < landscapeNumArms; i++) {
  armIndices.push(i);
}

// After (1 line):
const armIndices = [...Array(landscapeNumArms).keys()];

Savings: 3 lines

---

Summary

Change	Current	After	Savings
getResponsiveConfig	83	~25	~58
X-axis config helper	26	~14	~12
Single-pass stats	25	~15	~10
armIndices one-liner	4	1	3
Total	138	~55	~83 lines

Current JS file: 512 lines → Could be ~429 lines (16% reduction)