|
| 1 | +--- |
| 2 | +name: qa-pr |
| 3 | +description: Use when the user wants a QA-style review of a Rocket.Chat branch, pull request, or diff range; inspect the change like a QA engineer, prioritize user-visible regressions, validate likely failures with targeted checks, and report confirmed bugs before summaries. |
| 4 | +--- |
| 5 | + |
| 6 | +# QA PR Review |
| 7 | + |
| 8 | +Use this workflow when you want a QA-style pass on the current branch or a pull request. Act like a pragmatic QA engineer: hunt for bugs a user could notice, hit, or be blocked by. Do not stop at summarizing the diff. |
| 9 | + |
| 10 | +## Quick start |
| 11 | + |
| 12 | +1. Lock the comparison range. |
| 13 | +2. Classify the risky surfaces touched by the PR. |
| 14 | +3. Turn risky changes into concrete bug hypotheses. |
| 15 | +4. Validate each hypothesis with the smallest useful proof. |
| 16 | +5. Report only confirmed bugs, strong suspicions, and targeted test suggestions. |
| 17 | + |
| 18 | +## Task framing |
| 19 | + |
| 20 | +When asked to review a branch or PR from a QA perspective: |
| 21 | + |
| 22 | +- treat the task as bug-finding, not diff summarization |
| 23 | +- prioritize user-visible regressions and broken workflows |
| 24 | +- validate with the smallest useful proof available |
| 25 | +- report confirmed bugs first, then strong suspicions, then targeted test suggestions if no bug is confirmed |
| 26 | + |
| 27 | +## Comparison range |
| 28 | + |
| 29 | +Default to the current branch against its merge base with the default branch unless the user gave a base branch, diff range, or PR number. |
| 30 | + |
| 31 | +Recommended commands: |
| 32 | + |
| 33 | +```bash |
| 34 | +git branch --show-current |
| 35 | +git remote show origin | sed -n '/HEAD branch/s/.*: //p' |
| 36 | +git merge-base HEAD origin/<base> |
| 37 | +git diff --stat <merge-base>..HEAD |
| 38 | +git log --first-parent --reverse --oneline <merge-base>..HEAD |
| 39 | +``` |
| 40 | + |
| 41 | +If the user gave a PR number and remote metadata is available, use the PR base and head explicitly. |
| 42 | + |
| 43 | +## What to optimize for |
| 44 | + |
| 45 | +Prioritize: |
| 46 | + |
| 47 | +- user-visible bugs |
| 48 | +- broken workflows |
| 49 | +- validation mistakes |
| 50 | +- missing permissions or feature-flag handling |
| 51 | +- rendering regressions |
| 52 | +- state, async, or loading regressions |
| 53 | +- API contract mismatches that break the UI or integration path |
| 54 | + |
| 55 | +Do not report: |
| 56 | + |
| 57 | +- pure refactors with no observable behavior change |
| 58 | +- style-only issues unless they block use or accessibility |
| 59 | +- speculative bugs with no concrete path to failure |
| 60 | +- intentional product changes unless the user wants them called out |
| 61 | + |
| 62 | +## Rocket.Chat risk map |
| 63 | + |
| 64 | +Start here first when these paths are touched: |
| 65 | + |
| 66 | +- `apps/meteor/client/` |
| 67 | +- `apps/meteor/app/` |
| 68 | +- `apps/meteor/server/` |
| 69 | +- `apps/meteor/ee/` |
| 70 | +- `packages/` |
| 71 | + |
| 72 | +Pay extra attention to: |
| 73 | + |
| 74 | +- forms, validation, and save flows |
| 75 | +- authentication, authorization, and permissions |
| 76 | +- navigation, routing, and deep links |
| 77 | +- message rendering, uploads, previews, or composer flows |
| 78 | +- settings, toggles, feature flags, and enterprise-only branches |
| 79 | +- loading states, optimistic updates, retries, and error handling |
| 80 | +- shared helpers whose behavior fans out to many screens |
| 81 | + |
| 82 | +## Investigation flow |
| 83 | + |
| 84 | +### 1. Scan strategically |
| 85 | + |
| 86 | +Prefer targeted reading over broad reading. |
| 87 | + |
| 88 | +Recommended commands: |
| 89 | + |
| 90 | +```bash |
| 91 | +git diff --name-only <merge-base>..HEAD |
| 92 | +git diff --diff-filter=D --name-status <merge-base>..HEAD |
| 93 | +git show --stat --summary <commit> |
| 94 | +git show <commit> -- <path> |
| 95 | +rg -n "onBlur|onChange|disabled=|useEffect|useMemo|useCallback|Promise\\.all|await |try \\{|catch \\(|feature flag|permission|can[A-Z]" . |
| 96 | +``` |
| 97 | + |
| 98 | +Review commit by commit when the PR is non-trivial. Classify each commit as: |
| 99 | + |
| 100 | +- checked, no bug found |
| 101 | +- confirmed bug |
| 102 | +- suspected bug |
| 103 | +- intentional change |
| 104 | + |
| 105 | +Do not claim a full QA pass unless the whole requested range was checked. |
| 106 | + |
| 107 | +### 2. Turn code signals into hypotheses |
| 108 | + |
| 109 | +Write concrete, falsifiable bug hypotheses before testing them. |
| 110 | + |
| 111 | +Examples: |
| 112 | + |
| 113 | +- "Submitting profile changes now fails when the field is unchanged because validation moved from blur-time to submit-time." |
| 114 | +- "The composer attachment flow can get stuck because a new disabled state is never cleared on API error." |
| 115 | +- "A permission gate now hides an action for valid roles because the fallback branch was removed." |
| 116 | + |
| 117 | +High-yield bug patterns: |
| 118 | + |
| 119 | +- deleted guards or fallback branches |
| 120 | +- changed default values |
| 121 | +- new required props or parameters without all call sites updated |
| 122 | +- server response shape changes without UI updates |
| 123 | +- state derived from stale closures or incomplete effect dependencies |
| 124 | +- loading spinners, disabled buttons, or optimistic state that never resets |
| 125 | +- feature-flag or edition checks that skip a previously valid path |
| 126 | +- helper or library migrations with slightly different semantics |
| 127 | + |
| 128 | +### 3. Validate with the smallest proof |
| 129 | + |
| 130 | +Use the cheapest proof that can support or reject the hypothesis: |
| 131 | + |
| 132 | +1. existing automated tests |
| 133 | +2. targeted unit or integration tests |
| 134 | +3. local app flow or UI repro |
| 135 | +4. API repro |
| 136 | +5. code-path proof when runtime setup is impractical |
| 137 | + |
| 138 | +Useful commands: |
| 139 | + |
| 140 | +```bash |
| 141 | +git diff <merge-base>..HEAD -- <path> |
| 142 | +git log --oneline <merge-base>..HEAD -- <path> |
| 143 | +git show <merge-base>:<path> |
| 144 | +``` |
| 145 | + |
| 146 | +Validation guidance: |
| 147 | + |
| 148 | +- Prefer targeted test commands over whole-repo suites. |
| 149 | +- If no relevant test exists, say that clearly and propose the smallest missing check. |
| 150 | +- Use manual repro steps when the issue is primarily UX or workflow-visible. |
| 151 | +- Use code-only proof only when runtime validation is too expensive for the task. |
| 152 | + |
| 153 | +## Reproduction standard |
| 154 | + |
| 155 | +Bug reports must be: |
| 156 | + |
| 157 | +- concrete |
| 158 | +- minimal |
| 159 | +- repeatable |
| 160 | +- written from the user's perspective |
| 161 | + |
| 162 | +Good: |
| 163 | + |
| 164 | +1. Open the room composer. |
| 165 | +2. Add an attachment. |
| 166 | +3. Force the upload API to fail. |
| 167 | +4. Observe that the send button remains disabled after the error toast. |
| 168 | + |
| 169 | +Bad: |
| 170 | + |
| 171 | +- "Test attachments" |
| 172 | +- "See if the composer still works" |
| 173 | + |
| 174 | +## Reporting format |
| 175 | + |
| 176 | +Use this structure: |
| 177 | + |
| 178 | +```markdown |
| 179 | +## [Bug Title] |
| 180 | + |
| 181 | +**Status**: confirmed | suspected |
| 182 | +**Impact**: [Who is affected and why it matters] |
| 183 | + |
| 184 | +**Steps to reproduce**: |
| 185 | +1. ... |
| 186 | +2. ... |
| 187 | +3. ... |
| 188 | + |
| 189 | +**Expected**: ... |
| 190 | +**Actual**: ... |
| 191 | + |
| 192 | +**Evidence**: |
| 193 | +- [test failure, code path, local repro, or API proof] |
| 194 | + |
| 195 | +**Root cause**: commit `[sha]` / PR #[number if known] |
| 196 | +- [behavior-changing code change] |
| 197 | +- [relevant files] |
| 198 | +``` |
| 199 | + |
| 200 | +If no bug is confirmed, use: |
| 201 | + |
| 202 | +```markdown |
| 203 | +## No confirmed bugs found |
| 204 | + |
| 205 | +**Highest-risk areas checked**: |
| 206 | +- ... |
| 207 | + |
| 208 | +**Targeted tests to run**: |
| 209 | +- ... |
| 210 | + |
| 211 | +**Manual QA still recommended**: |
| 212 | +1. ... |
| 213 | +2. ... |
| 214 | +``` |
| 215 | + |
| 216 | +## Output bar |
| 217 | + |
| 218 | +- Prefer one strong confirmed bug over five weak suspicions. |
| 219 | +- Use `confirmed` only after reproduction, failing test evidence, or equivalent code-path proof. |
| 220 | +- Use `suspected` when the risk is real but validation is incomplete. |
| 221 | +- Always include why the bug matters to a user, not just why the code looks wrong. |
0 commit comments