Skip to content

Commit 7f3efdf

Browse files
emaneman
committed
Merge branch 'main' of github.com:eman/nwp500-python
2 parents 5f0d7ce + 0c12fa6 commit 7f3efdf

3 files changed

Lines changed: 443 additions & 2 deletions

File tree

‎.github/RESOLVING_PR_COMMENTS.md‎

Lines changed: 376 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,376 @@
1+
# How to Resolve GitHub Review Comments
2+
3+
This guide documents how to mark review comment conversations as resolved after addressing reviewer feedback in pull requests.
4+
5+
## Quick Reference
6+
7+
```bash
8+
# 1. Get all review threads for PR #74
9+
gh api graphql -f query='
10+
query {
11+
repository(owner: "eman", name: "nwp500-python") {
12+
pullRequest(number: 74) {
13+
reviewThreads(first: 10) {
14+
nodes {
15+
id
16+
path
17+
line
18+
isResolved
19+
}
20+
}
21+
}
22+
}
23+
}
24+
' | jq '.data.repository.pullRequest.reviewThreads.nodes'
25+
26+
# 2. Resolve a specific thread
27+
gh api graphql -f query='
28+
mutation {
29+
resolveReviewThread(input: {threadId: "PRRT_kwDOP_hNvM5ukIVT"}) {
30+
thread { isResolved }
31+
}
32+
}
33+
'
34+
```
35+
36+
## Why Resolve Comments?
37+
38+
- **Clarity**: Shows reviewers their feedback has been acted upon
39+
- **Workflow**: Prevents reviewers from re-reading already-addressed comments
40+
- **Signal Readiness**: Indicates PR is ready for another review pass
41+
- **Clean PR Interface**: Makes GitHub's PR conversation cleaner and easier to follow
42+
43+
## Workflow: Address and Resolve
44+
45+
### 1. Address the Comment
46+
47+
Make the code changes requested by the reviewer:
48+
- Fix bugs
49+
- Update documentation
50+
- Refactor code
51+
- Add tests
52+
53+
**Example:**
54+
```bash
55+
# Edit file based on comment
56+
nano src/nwp500/converters.py
57+
58+
# Commit the change
59+
git add src/nwp500/converters.py
60+
git commit -m "Fix div_10 converter to handle string inputs"
61+
62+
# Push to remote
63+
git push
64+
```
65+
66+
### 2. Verify Tests Pass
67+
68+
Run all validation before marking comments as resolved:
69+
70+
```bash
71+
# Run tests
72+
pytest --ignore=tests/test_mqtt_hypothesis.py
73+
74+
# Run linting
75+
make ci-lint
76+
77+
# Run type checking
78+
python3 -m mypy src/nwp500 --config-file pyproject.toml
79+
```
80+
81+
All must pass before proceeding.
82+
83+
### 3. Get Review Thread IDs
84+
85+
Query GraphQL to find threads that need resolving:
86+
87+
```bash
88+
gh api graphql -f query='
89+
query {
90+
repository(owner: "eman", name: "nwp500-python") {
91+
pullRequest(number: 74) {
92+
reviewThreads(first: 10) {
93+
nodes {
94+
id
95+
path
96+
line
97+
isResolved
98+
}
99+
}
100+
}
101+
}
102+
}
103+
' | jq '.data.repository.pullRequest.reviewThreads.nodes'
104+
```
105+
106+
**Output example:**
107+
```json
108+
[
109+
{
110+
"id": "PRRT_kwDOP_hNvM5ukIVT",
111+
"path": "src/nwp500/converters.py",
112+
"line": 125,
113+
"isResolved": false
114+
},
115+
{
116+
"id": "PRRT_kwDOP_hNvM5ukIVo",
117+
"path": "tests/test_model_converters.py",
118+
"line": 212,
119+
"isResolved": false
120+
}
121+
]
122+
```
123+
124+
### 4. Identify Which Threads to Resolve
125+
126+
Cross-reference the output with:
127+
1. Which file you modified
128+
2. Which line the comment was on (approximately)
129+
3. Whether `isResolved` is `false`
130+
131+
### 5. Resolve the Threads
132+
133+
For each thread you addressed:
134+
135+
```bash
136+
gh api graphql -f query='
137+
mutation {
138+
resolveReviewThread(input: {threadId: "PRRT_kwDOP_hNvM5ukIVT"}) {
139+
thread {
140+
isResolved
141+
}
142+
}
143+
}
144+
'
145+
```
146+
147+
Success response:
148+
```json
149+
{
150+
"data": {
151+
"resolveReviewThread": {
152+
"thread": {
153+
"isResolved": true
154+
}
155+
}
156+
}
157+
}
158+
```
159+
160+
### 6. Verify All Are Resolved
161+
162+
Re-run the query from Step 3 to confirm all addressed threads now show `"isResolved": true`:
163+
164+
```bash
165+
gh api graphql -f query='
166+
query {
167+
repository(owner: "eman", name: "nwp500-python") {
168+
pullRequest(number: 74) {
169+
reviewThreads(first: 10) {
170+
nodes {
171+
path
172+
isResolved
173+
}
174+
}
175+
}
176+
}
177+
}
178+
' | jq '.data.repository.pullRequest.reviewThreads.nodes'
179+
```
180+
181+
## Batch Resolving Multiple Threads
182+
183+
If you have many threads to resolve, use a loop:
184+
185+
```bash
186+
#!/bin/bash
187+
188+
# Define thread IDs
189+
THREAD_IDS=(
190+
"PRRT_kwDOP_hNvM5ukIVT"
191+
"PRRT_kwDOP_hNvM5ukIVo"
192+
"PRRT_kwDOP_hNvM5ukIVx"
193+
)
194+
195+
# Resolve each one
196+
for thread_id in "${THREAD_IDS[@]}"; do
197+
gh api graphql -f query="
198+
mutation {
199+
resolveReviewThread(input: {threadId: \"$thread_id\"}) {
200+
thread { isResolved }
201+
}
202+
}
203+
" && echo "✓ $thread_id resolved"
204+
done
205+
206+
echo "All threads resolved!"
207+
```
208+
209+
Or as a one-liner:
210+
211+
```bash
212+
for id in PRRT_kwDOP_hNvM5ukIVT PRRT_kwDOP_hNvM5ukIVo PRRT_kwDOP_hNvM5ukIVx; do
213+
gh api graphql -f query="mutation { resolveReviewThread(input: {threadId: \"$id\"}) { thread { isResolved } } }" && echo "✓ $id resolved"
214+
done
215+
```
216+
217+
## Shell Function (Optional)
218+
219+
Add this to your shell profile (`~/.bashrc`, `~/.zshrc`, etc.):
220+
221+
```bash
222+
# Resolve a single GitHub PR review thread
223+
resolve-pr-thread() {
224+
local thread_id="$1"
225+
if [[ -z "$thread_id" ]]; then
226+
echo "Usage: resolve-pr-thread THREAD_ID"
227+
echo "Example: resolve-pr-thread PRRT_kwDOP_hNvM5ukIVT"
228+
return 1
229+
fi
230+
231+
gh api graphql -f query="
232+
mutation {
233+
resolveReviewThread(input: {threadId: \"$thread_id\"}) {
234+
thread { isResolved }
235+
}
236+
}
237+
" | jq '.data.resolveReviewThread.thread.isResolved'
238+
}
239+
240+
# Get all unresolved threads for a PR
241+
pr-threads() {
242+
local pr_num="${1:?PR number required}"
243+
gh api graphql -f query="
244+
query {
245+
repository(owner: \"eman\", name: \"nwp500-python\") {
246+
pullRequest(number: $pr_num) {
247+
reviewThreads(first: 10) {
248+
nodes {
249+
id
250+
path
251+
line
252+
isResolved
253+
}
254+
}
255+
}
256+
}
257+
}
258+
" | jq '.data.repository.pullRequest.reviewThreads.nodes'
259+
}
260+
```
261+
262+
Usage:
263+
```bash
264+
pr-threads 74 # List all threads for PR #74
265+
resolve-pr-thread PRRT_kwDOP_hNvM5ukIVT # Resolve one thread
266+
```
267+
268+
## Special Cases
269+
270+
### Unresolving a Thread
271+
272+
If a reviewer asks for changes after you marked it resolved, unresolve it:
273+
274+
```bash
275+
gh api graphql -f query='
276+
mutation {
277+
unresolveReviewThread(input: {threadId: "PRRT_kwDOP_hNvM5ukIVT"}) {
278+
thread {
279+
isResolved
280+
}
281+
}
282+
}
283+
'
284+
```
285+
286+
### Force-Pushed Commits
287+
288+
When you amend and force-push commits:
289+
1. Old review threads remain resolvable
290+
2. Thread IDs don't change
291+
3. Line numbers may be different in new commits
292+
4. Comments still point to old code but threads can be resolved
293+
5. This is normal GitHub behavior - resolve all threads once your changes are complete
294+
295+
### Multiple Changes to Same File
296+
297+
If a reviewer left multiple comments on the same file and you addressed them all:
298+
1. Make all changes to the file
299+
2. Commit and push
300+
3. Get all thread IDs for that file
301+
4. Resolve each one individually
302+
303+
## Troubleshooting
304+
305+
### "Not Found" Error
306+
307+
**Problem:**
308+
```
309+
{
310+
"message": "Not Found",
311+
"documentation_url": "https://docs.github.com/rest",
312+
"status": 404
313+
}
314+
```
315+
316+
**Solutions:**
317+
- Verify PR number is correct
318+
- Check you have access to the repository
319+
- Verify `gh auth status` shows you're authenticated
320+
- Try running `gh auth login` again
321+
322+
### No Threads Returned
323+
324+
**Problem:**
325+
```
326+
{
327+
"data": {
328+
"repository": {
329+
"pullRequest": {
330+
"reviewThreads": {
331+
"nodes": []
332+
}
333+
}
334+
}
335+
}
336+
}
337+
```
338+
339+
**Solutions:**
340+
- Verify PR number is correct
341+
- The PR may have only general comments (not review comments)
342+
- Try increasing `first: 10` to `first: 100` if there are many threads
343+
- Verify reviewers left inline code comments, not just PR comments
344+
345+
### Thread Won't Resolve
346+
347+
**Problem:**
348+
Mutation succeeds but `isResolved` still returns `false` on next check
349+
350+
**Solutions:**
351+
- Wait a moment and query again (GitHub API may have delay)
352+
- Verify you're using the exact same thread ID
353+
- Check that you have write permissions on the repository
354+
- Try running `gh auth refresh` to refresh your token
355+
356+
### Can't Find Thread ID for Comment I Fixed
357+
358+
**Problem:**
359+
You fixed the code but can't find the matching thread ID
360+
361+
**Possible Causes:**
362+
1. The comment was a general PR comment, not an inline code comment (can't be resolved)
363+
2. The thread was already resolved by someone else
364+
3. You're looking at the wrong PR number
365+
4. The comment was deleted by the reviewer
366+
367+
**Solution:**
368+
Go to the PR on GitHub.com and manually verify the comment still exists and is an inline review comment (not a general comment).
369+
370+
## References
371+
372+
- [GitHub GraphQL API: resolveReviewThread](https://docs.github.com/en/graphql/reference/mutations#resolvereviewthread)
373+
- [GitHub GraphQL API: unresolveReviewThread](https://docs.github.com/en/graphql/reference/mutations#unresolvereviewthread)
374+
- [GitHub GraphQL API: Review Threads](https://docs.github.com/en/graphql/reference/objects#reviewthread)
375+
- [GitHub CLI: gh api](https://cli.github.com/manual/gh_api)
376+
- [GitHub: Review conversations](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#about-pull-request-reviews)

0 commit comments

Comments
 (0)