PMM-15308 Trim whitespace from dashboard titles

[theTibi]

Ticket number: PMM-15308

Feature build: Percona-Lab/pmm-submodules#4525

Summary

• Bug: panel and dashboard titles carried leading/trailing whitespace — the reported "InnoDB Buffer Pool Data ", plus 38 more across 14 dashboards (e.g. Nodes_Overview.json has 8 leading-space titles like " Top Disk Latency").
• Root cause: nothing validated titles. The Dashboards → Check job already runs cleanup-dash.py --check-only, but its CLEANUPERS list only normalised editable/time/timezone/refresh/id, so untrimmed titles always passed.
• Fix: trim the 39 titles, and add a trim_titles cleanuper so the existing CI gate catches regressions — the trim-on-publish the reporter asked for.

Notes for reviewers

1. The --check-only reporter needed a title: case too. It hardcodes per-field comparisons, so adding trim_titles alone would have made CI exit 1 while printing no reason. walk_titles is shared by the cleanuper and the reporter so the two cannot drift.

2. Titles were trimmed in place, not via cleanup-dash.py write mode. --check-only compares parsed objects, while write mode re-serialises with sort_keys/indent=4. Running it over the tree would reformat 6 unrelated dashboards wholesale (~26k lines of indentation churn): MongoDB_Cluster_Summary, MongoDB_Collections_Overview, MongoDB_ReplSet_Summary, PMM_Health, PostgreSQL_Instance_Summary, PostgreSQL_Top_Queries. That drift is pre-existing and left alone here — worth its own ticket, since the CI error message points devs at exactly that command.

3. One behaviour-visible change. MySQL_User_Details.json panels[0] is a text spacer whose title is literally " "; it becomes "". Confirmed as intended.

4. First Python tests in the repo. test_cleanup_dash.py is stdlib unittest — no new dependency. Happy to drop it and the CI step if you'd rather not set the precedent.

5. The whole-tree gate, and how it is scoped. TestRepoDashboards runs --check-only over all 76 dashboards, so a dirty dashboard fails the job no matter which file the PR touched. That breadth is deliberate — it is what forces a whole-tree fix when a cleanuper is added, exactly as this PR had to do — but the workflow triggers on paths: dashboards/**, so left unscoped it would gate unrelated PRs (a pmm-app change, a yarn.lock bump) on every dashboard in the tree. The Test the cleanup script step therefore runs only when the PR changes a dashboard JSON file, cleanup-dash.py / test_cleanup_dash.py, or dashboards.yml itself — whoever edits the gate should see it run, and that is the one place the widened scope is deliberate. It runs after the per-dashboard check so a whole-tree failure never hides the step summary and ::error file= annotations for the PR's own dashboards. On failure it prints the tool's own per-file report — every dirty field as "raw" -> "cleaned" — and sorts failures into three buckets with different remedies: needs cleanup (run the cleanuper), failed without naming a field (a cleanuper has no case in the --check-only reporter; write mode still fixes the file), and could not be checked at all (the tool crashed; the cleanuper hits the same error). Each bucket is capped at 10 entries so a systemic fault does not bury the explanation. The trigger matches any dashboards/misc/test_*.py, so a PR adding a test module actually runs it. AGENTS.md and dashboards/dashboards/AGENTS.md document the new command.

6. Drive-by: the check job was fail-open. git diff … | sort -u > changed_dashboards.txt || true returns sort's exit status, so an unresolvable base SHA yielded an empty file and the job reported success with no dashboard checked at all. It now fails with an annotation. Pre-existing, fixed here because note 5 doubled the blast radius (two skipped checks instead of one). The workflow also now triggers on changes to itself — before this, a PR fixing only dashboards.yml merged without ever running it.

Test plan

• cleanup-dash.py --check-only fails on exactly 14 dashboards before the trim, passes on all 76 after
• All 76 dashboards still parse as JSON; 0 untrimmed titles remain
• Diff is title-only — 0 changed dashboard lines that are not "title":
• 20 tests pass in a clean ubuntu:24.04 / Python 3.12.3 container (mirrors CI runner), no pip installs; also green on Python 3.9.6 / 3.13.7 / 3.14.3, so the suite is not tied to the runner's minor version
• Negative-tested: reverting the tool → 12 errors + 4 failures; reverting the dashboards → fails listing all 39 titles
• Failure paths verified in throwaway repos — crash (missing time, malformed JSON, non-dict root), cleanuper with no reporter case incl. unrelated stderr noise, missing dashboard tree, mixed dirty+broken, and the printed remediation command actually runs and makes the gate pass: 19/19 assertions, plus a second harness of 24 covering the three-bucket split, exit-code reporting, output capping and per-field reporter coverage (positive and negative probes)
• The check job's changed-files step executed verbatim against a real / bogus / empty base SHA → 0 / 1 / 1, each failure carrying a ::error:: annotation; the tooling output resolves to 1 for a dashboard / cleanup-dash.py / test_*.py / dashboards.yml change and 0 for a pmm-app-only or lockfile-only change
• Write-mode round-trip clean; plugin.json's 68 dashboard paths all resolve
• Not run: live PMM server render check — change is a pure whitespace trim with no query or schema impact

Related

• Follow-up candidate: normalise the 6 reformat-prone dashboards so cleanup-dash.py <file> is safe to run as CI suggests.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 45.69%. Comparing base (31318c7) to head (af3fe4d).
⚠️ Report is 164 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5767      +/-   ##
==========================================
+ Coverage   43.59%   45.69%   +2.10%     
==========================================
  Files         415      547     +132     
  Lines       43134    46048    +2914     
  Branches        0      581     +581     
==========================================
+ Hits        18804    21042    +2238     
- Misses      22454    22953     +499     
- Partials     1876     2053     +177     

☔ 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.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: bae4ce71-11c0-4a38-b3e8-b34bd0fac551

📥 Commits

Reviewing files that changed from the base of the PR and between 17652a6 and af3fe4d.

📒 Files selected for processing (1)

• AGENTS.md

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• percona/pmm-qa (manual)
• percona/pmm (manual)

🚧 Files skipped from review as they are similar to previous changes (1)

• AGENTS.md

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

Walkthrough

The cleanup script now recursively trims dashboard titles and reports title changes. Dashboard JSON titles were normalized. Unit, CLI, repository, and workflow checks validate the cleanup behavior. CI also detects workflow and tooling changes before running the whole-tree test suite. Contributor guidance documents the required test command, and Python cache files are ignored.

Merge Risk: ⚪ Minimal · up to af3fe

Dashboard titles are normalized and the cleanup tooling adds checks to prevent future whitespace regressions. No current merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary change: trimming whitespace from dashboard titles.
Description check	✅ Passed	The description includes the ticket number, feature build, detailed summary, related work, and a complete test plan. The API documentation checkbox is not required because this PR does not alter API e…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description includes the ticket number, feature build, detailed summary, related work, and a complete test plan. The API documentation checkbox is not required because this PR does not alter API endpoints.

---

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

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[mattiasimonato]

One comment below on the repo-wide test. Not blocking, but I'd like it sorted before merge.