Skip to content

Commit d735414

Browse files
committed
fix: address PR review feedback on stale thread resolution
- Check GraphQL response JSON for `.errors` field (HTTP 200 can still contain errors) on both the query and mutation calls - Add error handling around jq parsing/merging of thread data - Replace world-writable /tmp with mktemp + trap cleanup - Validate mutation success before incrementing resolved count
1 parent dd8a873 commit d735414

File tree

1 file changed

+30
-9
lines changed

1 file changed

+30
-9
lines changed

review-pr/action.yml

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,9 @@ runs:
182182
exit 0
183183
fi
184184
185+
DIFF_LINES_FILE=$(mktemp)
186+
trap "rm -f '$DIFF_LINES_FILE'" EXIT
187+
185188
awk '
186189
/^\+\+\+ b\// {
187190
# Extract file path from "+++ b/foo" (unambiguous, unlike diff --git header)
@@ -202,9 +205,9 @@ runs:
202205
}
203206
/^ / { line++ }
204207
/^-/ { next }
205-
' pr.diff | sort -u > /tmp/diff_lines.txt
208+
' pr.diff | sort -u > $DIFF_LINES_FILE
206209
207-
DIFF_LINE_COUNT=$(wc -l < /tmp/diff_lines.txt | tr -d ' ')
210+
DIFF_LINE_COUNT=$(wc -l < $DIFF_LINES_FILE | tr -d ' ')
208211
echo "📄 Found $DIFF_LINE_COUNT unique file:line pairs in diff"
209212
210213
# B. Fetch all review threads via GraphQL (paginated)
@@ -248,8 +251,20 @@ runs:
248251
exit 0
249252
}
250253
251-
THREADS=$(echo "$RESULT" | jq '.data.repository.pullRequest.reviewThreads.nodes // []')
252-
ALL_THREADS=$(echo "$ALL_THREADS" "$THREADS" | jq -s '.[0] + .[1]')
254+
# Check for GraphQL errors in response (HTTP 200 can still contain errors)
255+
if echo "$RESULT" | jq -e '.errors' > /dev/null 2>&1; then
256+
echo "::warning::GraphQL returned errors: $(echo "$RESULT" | jq -c '.errors')"
257+
exit 0
258+
fi
259+
260+
THREADS=$(echo "$RESULT" | jq '.data.repository.pullRequest.reviewThreads.nodes // []') || {
261+
echo "::warning::Failed to parse threads on page $PAGE"
262+
break
263+
}
264+
ALL_THREADS=$(echo "$ALL_THREADS" "$THREADS" | jq -s '.[0] + .[1]') || {
265+
echo "::warning::Failed to merge threads on page $PAGE"
266+
break
267+
}
253268
254269
HAS_NEXT=$(echo "$RESULT" | jq -r '.data.repository.pullRequest.reviewThreads.pageInfo.hasNextPage')
255270
if [ "$HAS_NEXT" != "true" ]; then
@@ -296,21 +311,27 @@ runs:
296311
297312
FILE_LINE="${THREAD_PATH}:${THREAD_LINE}"
298313
299-
if grep -qF "$FILE_LINE" /tmp/diff_lines.txt; then
314+
if grep -qF "$FILE_LINE" $DIFF_LINES_FILE; then
300315
echo " ⏭️ Keeping open: $FILE_LINE (still in diff)"
301316
KEPT=$((KEPT + 1))
302317
else
303-
echo " ✅ Resolving: $FILE_LINE (no longer in diff)"
304-
gh api graphql -f query='
318+
MUTATION_RESULT=$(gh api graphql -f query='
305319
mutation($threadId: ID!) {
306320
resolveReviewThread(input: { threadId: $threadId }) {
307321
thread { id isResolved }
308322
}
309323
}
310-
' -f threadId="$THREAD_ID" > /dev/null 2>&1 || {
311-
echo " ⚠️ Failed to resolve thread $THREAD_ID"
324+
' -f threadId="$THREAD_ID" 2>&1) || {
325+
echo " ⚠️ Failed to resolve thread $THREAD_ID (API error)"
312326
continue
313327
}
328+
329+
if echo "$MUTATION_RESULT" | jq -e '.errors' > /dev/null 2>&1; then
330+
echo " ⚠️ Failed to resolve thread $THREAD_ID: $(echo "$MUTATION_RESULT" | jq -c '.errors')"
331+
continue
332+
fi
333+
334+
echo " ✅ Resolved: $FILE_LINE (no longer in diff)"
314335
RESOLVED=$((RESOLVED + 1))
315336
fi
316337
done

0 commit comments

Comments
 (0)