You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Review the changes on the current branch against `dev` for correctness, vanilla JS best practices, and performance. Focus on what the code does, whether it does it efficiently, and flag anything that would hurt the reader or the runtime.
2
+
3
+
## 1. Collect the diff
4
+
5
+
```bash
6
+
git diff dev...HEAD
7
+
```
8
+
9
+
Read every changed source file in full (not just the diff lines) so you have complete context.
10
+
11
+
## 2. For each changed file, produce a section
12
+
13
+
Use this structure per file:
14
+
15
+
### `path/to/file.js`
16
+
17
+
**What changed** — one paragraph. Describe the intent of the change in plain English (e.g. "Adds a helper that strips HTML tags and decodes entities before using a string as modal ARIA label text, so screen readers receive clean text instead of raw markup.").
18
+
19
+
**Concrete example** — show a before/after pair that makes the change tangible:
**Issues found** — one bullet per issue, with severity (`critical` / `warn` / `nit`) and the exact line or pattern:
27
+
28
+
-`[warn] line 42` — description + a corrected snippet
29
+
30
+
If there are no issues, write `No issues found.`
31
+
32
+
## 3. Review criteria
33
+
34
+
Apply every check below. Quote the offending code for any failure.
35
+
36
+
### Correctness
37
+
- Does the logic handle `null`, `undefined`, and empty strings without throwing?
38
+
- Are all edge cases covered (empty arrays, missing DOM nodes, async races)?
39
+
- Are regular expressions anchored or guarded correctly — no catastrophic backtracking, no runaway `.+` on untrusted input?
40
+
41
+
### Vanilla JS best practices
42
+
- No unnecessary `innerHTML` assignments where `textContent` suffices (avoids XSS, faster parse)
43
+
- Prefer `el.textContent = value` over `el.innerHTML = value` when the value is plain text
44
+
- Use `document.createDocumentFragment()` or a single `append()` call when inserting multiple nodes — avoid repeated DOM mutations in a loop
45
+
- Use `const` / `let`; never `var`
46
+
- Arrow functions for closures; named `function` declarations for top-level exports
47
+
- Destructuring and default parameters where they simplify without obscuring
48
+
- No `==` — use `===` for all comparisons
49
+
- Avoid `try/catch` inside hot loops; guard with a conditional check before entering
50
+
- No dead code, no commented-out blocks left in
51
+
52
+
### Performance & optimisation
53
+
-**DOM reads/writes**: never interleave reads and writes in a loop (causes layout thrashing). Batch all reads, then all writes.
54
+
-**Regex**: compile once at module scope (`const RE = /pattern/`) rather than inside a called function if the regex is invariant
55
+
-**String building**: prefer a single `replace` chain on a string rather than splitting into an array, mapping, and rejoining when the operation is purely substitution
56
+
-**Event listeners**: confirm `removeEventListener` or `AbortController` is used if listeners are attached inside `init()` — leaks on re-init
57
+
-**Selector cost**: `querySelector` is fine; avoid `querySelectorAll` + `forEach` when a targeted `querySelector` on a known container is possible
58
+
-**Lazy work**: heavy computation (entity decoding, full DOM traversal) should be guarded so it only runs when the input actually needs it — not on every call
59
+
60
+
### Code size & readability
61
+
- No abstraction that is only used once and is no simpler than its inline equivalent
62
+
- Helper names should say *what* they return, not *how* (e.g. `plainText(title)` not `processAndCleanTitle`)
63
+
- Functions longer than ~30 lines should be split unless the logic is a single clear pipeline
64
+
- No multi-line comment blocks; inline comments only for non-obvious invariants
65
+
66
+
## 4. Summary table
67
+
68
+
End with a compact table:
69
+
70
+
| File | Critical | Warn | Nit |
71
+
|------|----------|------|-----|
72
+
| path/to/file.js | 0 | 2 | 1 |
73
+
74
+
If all counts are 0, say **"Ready to merge — no issues found."**
75
+
If any `critical` issues exist, say **"Block on critical issues before merging."**
76
+
If only `warn`/`nit`, say **"Good to merge after addressing warnings."**
-`/find-util <description>` — search utils/ and features/ for existing code before writing new code
139
139
-`/coverage-gaps` — inventory every block/utility and flag missing or shallow tests
140
140
-`/pr-ready` — lint + test + changed-file review + commit message check before opening a PR
141
+
-`/pr-review` — review branch changes for correctness, JS best practices, and performance; produces a per-file report with before/after examples and a severity summary table
0 commit comments