Skip to content

Commit affff2d

Browse files
committed
Merge branch 'quality/require-visual-mr-screenshots' into 'main'
quality: require before/after screenshots on visual MRs Closes #218 See merge request postgres-ai/postgresai!282
2 parents fe20335 + 81ca482 commit affff2d

2 files changed

Lines changed: 96 additions & 0 deletions

File tree

quality/QUALITY_ENGINEERING_GUIDE.md

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -268,6 +268,82 @@ Before approving any PR, verify:
268268
- [ ] Error messages are actionable (not just "something went wrong")
269269
- [ ] PostgreSQL version-specific behavior is handled
270270
- [ ] No hardcoded credentials, tokens, or connection strings
271+
- [ ] **Visual changes**: if the MR touches any user-visible surface (Grafana
272+
dashboards, Console UI, Joe UI, marketing or landing pages), the
273+
description contains **before/after screenshots** for each visual fix.
274+
275+
See the [Visual Change Verification](#visual-change-verification) section
276+
below for the full standard, and rule 11 in
277+
[`quality/pr-review-prompt.md`](./pr-review-prompt.md) for the canonical
278+
file-path globs the AI reviewer uses.
279+
280+
### Visual Change Verification
281+
282+
For every MR that modifies a user-visible surface, the description must include
283+
a **Visual changes** section with one before/after pair per fix:
284+
285+
```markdown
286+
## Visual changes
287+
288+
### Fix 1 — Dashboard 3 first panel "No data"
289+
**Before:**
290+
![before](before-d3-panel.png)
291+
**After:**
292+
![after](after-d3-panel.png)
293+
```
294+
295+
This is the visual analogue of red/green TDD:
296+
297+
- A failing unit test proves the bug existed and is now fixed *in code*.
298+
- A before/after screenshot pair proves the bug existed and is now fixed
299+
*in the rendered output*.
300+
301+
Text descriptions like "I fixed the layout" or "I changed the color" are not
302+
sufficient — reviewers should not have to spin up the demo to confirm a visual
303+
claim.
304+
305+
**Scope (which surfaces trigger this rule):** Grafana dashboards
306+
(`config/grafana/dashboards/*.json`,
307+
`postgres_ai_helm/.../dashboards/*.json`), the Console UI (`console/` or
308+
similar frontend directories), Joe UI, and marketing or landing pages. See
309+
rule 11 in [`quality/pr-review-prompt.md`](./pr-review-prompt.md) for the
310+
authoritative list of path globs the AI reviewer uses.
311+
312+
**Capture mechanics:**
313+
314+
- `before` shots from the deployed staging/demo environment (the broken state
315+
users currently see).
316+
- `after` shots from either the redeployed environment OR a local stack
317+
running the patched JSON / build.
318+
- Upload to GitLab using a multipart `POST` to the project's `/uploads`
319+
endpoint (the `%2F` between `<namespace>` and `<project>` is the
320+
URL-encoded slash GitLab requires in the project identifier):
321+
322+
```bash
323+
curl --request POST \
324+
--header "PRIVATE-TOKEN: $GITLAB_TOKEN" \
325+
--form "file=@shot.png" \
326+
"https://gitlab.com/api/v4/projects/<namespace>%2F<project>/uploads"
327+
```
328+
329+
The response JSON contains a `markdown` field with the ready-to-paste
330+
`![](/uploads/...)` reference — copy that string into the MR description.
331+
332+
**Exception — render too expensive:**
333+
334+
If running the AFTER state would take more than ~10 minutes of setup (e.g., a
335+
fix that requires the full monitoring stack with live data), the AFTER may be
336+
replaced by a citation of the lint/unit test that proves the change. The
337+
citation must name the test file path and the asserted invariant:
338+
339+
```markdown
340+
**After (verified by test):** `tests/grafana_dashboards/test_xyz.py`
341+
asserts the invariant; see line 42.
342+
```
343+
344+
Use this exception only when (a) the AFTER state cannot be rendered in
345+
under ~10 minutes, and (b) the cited test directly asserts the user-visible
346+
invariant the fix establishes. The default is screenshots.
271347

272348
---
273349

quality/pr-review-prompt.md

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,26 @@ wrong decisions, or silent monitoring failures that erode customer trust.
9393
- Check for potential deadlock patterns (acquiring multiple locks in
9494
inconsistent order).
9595
96+
### 11. Visual Change Verification (HIGH)
97+
- If the diff touches user-visible surfaces — Grafana dashboards
98+
(`config/grafana/dashboards/*.json`, `postgres_ai_helm/.../dashboards/*.json`),
99+
the Console UI (`console/` or similar frontend dirs), Joe UI, or marketing /
100+
landing pages — the MR description must contain a **Visual changes** section
101+
with one before/after screenshot pair per visual fix.
102+
- Flag any MR that modifies dashboard JSON or other rendered surfaces but
103+
lacks the "Visual changes" section.
104+
- Text descriptions of the fix ("removed the TODO marker", "fixed the legend")
105+
are not acceptable as the only proof — reviewers cannot verify the rendered
106+
output without screenshots.
107+
- An exception is acceptable only when running the AFTER state would take
108+
more than ~10 minutes of setup. In that case the AFTER may be replaced by
109+
an explicit citation of the lint or unit test that proves the change
110+
(example — substitute the actual test path:
111+
`tests/grafana_dashboards/test_no_duplicate_targets_in_panel.py`). The
112+
citation must name the test file and the asserted invariant.
113+
- See [Visual Change Verification](./QUALITY_ENGINEERING_GUIDE.md#visual-change-verification)
114+
in the Quality Engineering Guide for the full standard.
115+
96116
## Review Output Format
97117
98118
For each issue found, provide:

0 commit comments

Comments
 (0)