From 871ea4636fe404e28d253a681d55148ebc5857ba Mon Sep 17 00:00:00 2001 From: Daniel Frett Date: Tue, 12 May 2026 15:53:18 -0600 Subject: [PATCH 1/2] Port inline PR comments and thread resolution from parser repo skill Co-Authored-By: Claude Sonnet 4.6 --- .claude/skills/pr-review/SKILL.md | 77 ++++++++++++++++++++++++++++++- 1 file changed, 75 insertions(+), 2 deletions(-) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index b721c987c2..455e1f0e53 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -38,7 +38,34 @@ If it fails, report as **Must Fix** before reviewing anything else. 7. Output a structured review (format below). -8. After the review output, print: +8. Post inline comments to the PR for every āš ļø and āŒ finding that references a specific file and line number. Before posting, deduplicate against all existing comments (resolved or not) to avoid re-posting anything already raised: + +```bash +# Get the head SHA, repo, and all existing review comments (resolved and unresolved) +HEAD_SHA=$(gh pr view $ARGUMENTS --json headRefOid -q .headRefOid) +REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner) +EXISTING=$(gh api repos/$REPO/pulls/$ARGUMENTS/comments --jq '[.[] | select(.in_reply_to_id == null) | {path:.path, line:.line, body:.body}]') +``` + +For each finding, check whether any existing comment (resolved or not) already covers the same file + line (or contains substantially the same text). Skip any finding that is already covered. Then bundle the remaining new comments into a single review submission: + +```bash +gh api repos/$REPO/pulls/$ARGUMENTS/reviews \ + --method POST \ + --field commit_id="$HEAD_SHA" \ + --field event="COMMENT" \ + --field "comments[][path]=" \ + --field "comments[][line]=" \ + --field "comments[][side]=RIGHT" \ + --field "comments[][body]= + +šŸ¤– Posted by [Claude Code](https://claude.ai/code)" \ + # repeat --field "comments[]..." for each new finding +``` + +Use the exact file path from the diff and the line number in the current version of the file (RIGHT side). Each comment body should contain the full finding description. Always append the attribution footer `\n\nšŸ¤– Posted by [Claude Code](https://claude.ai/code)` to each comment. If no new actionable findings exist (only āœ… items or all already commented), skip this step. + +9. After the review output, print: ``` --- @@ -190,4 +217,50 @@ When the user says `dismiss: ā€” <reason>` (in any form ā€” "dismiss the **Dismissed by**: <git user.name> ``` -4. Confirm to the user what was added and that it will be suppressed in future reviews. +4. If the current session reviewed a PR, find any open (unresolved) comment thread on that PR matching the dismissed issue. Use the GraphQL API to locate threads and resolve the matching one, replying with the dismissal reason first: + +```bash +REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner) +OWNER=${REPO%%/*} +REPONAME=${REPO##*/} + +# Find unresolved review threads +gh api graphql -f query=" +{ + repository(owner: \"$OWNER\", name: \"$REPONAME\") { + pullRequest(number: $PR_NUMBER) { + reviewThreads(first: 100) { + nodes { + id + isResolved + comments(first: 1) { + nodes { id body path line } + } + } + } + } + } +}" +``` + +Match the thread by file path, line number, or substantial text overlap with the dismissed finding. Then reply to the thread and resolve it: + +```bash +# Reply to the thread's first comment explaining the dismissal +gh api repos/$REPO/pulls/$PR_NUMBER/comments \ + --method POST \ + --field in_reply_to=<comment_id> \ + --field body="Dismissed: <reason given by user> + +šŸ¤– [Claude Code](https://claude.ai/code)" + +# Resolve the thread via GraphQL +gh api graphql -f query=" +mutation { + resolveReviewThread(input: { threadId: \"<thread_node_id>\" }) { + thread { id isResolved } + } +}" +``` + +5. Confirm to the user what was added and that it will be suppressed in future reviews. From e189f884eeb486851585419e6e5d508aca17b600 Mon Sep 17 00:00:00 2001 From: Daniel Frett <daniel.frett@cru.org> Date: Tue, 12 May 2026 16:32:24 -0600 Subject: [PATCH 2/2] Add three checklist items to pr-review skill from PR comment analysis - eventSink must be passed as a direct lambda, not a local variable - navigator.pop() must be inside the launch block after async operations - Use Fakes instead of mockk for interfaces with @Composable functions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --- .claude/skills/pr-review/SKILL.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 455e1f0e53..a641bbcc86 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -119,6 +119,15 @@ To dismiss a finding so it won't appear in future reviews, say: - [ ] `UiEvent` is a `sealed interface` implementing `CircuitUiEvent`, marked `internal` - [ ] `UiState` and `UiEvent` defined as nested types inside the Presenter - [ ] `UiState` exposes `val eventSink: (UiEvent) -> Unit` +- [ ] `eventSink` lambda passed directly when constructing `UiState` ā€” not extracted to a local variable first: + ```kotlin + // Correct + return UiState(items = items) { event -> when (event) { ā€¦ } } + // Wrong + val eventSink: (UiEvent) -> Unit = { ā€¦ } + return UiState(items = items, eventSink = eventSink) + ``` +- [ ] `navigator.pop()` (or any navigation call) placed *inside* the `launch { }` block when it follows an async operation ā€” `rememberCoroutineScope()` is canceled on composition disposal and can cancel an in-flight write before it completes - [ ] Presenter contains no UI logic ā€” pure state/event handling **UI Composable** @@ -157,6 +166,7 @@ To dismiss a finding so it won't appear in future reviews, say: ### Testing - [ ] Presenter tests use `presenterTestOf { }` (Circuit test API) +- [ ] Interfaces with `@Composable` functions use a hand-written `Fake*` class in tests, not `mockk<>()` ā€” mockposable delays Kotlin version uptake and is incompatible with newer compiler versions - [ ] Paparazzi tests extend `BasePaparazziTest` from `:ui:base` testFixtures with `@TestParameter` night/accessibility matrix - [ ] Snapshots not recorded locally ā€” triggered via GitHub Actions workflow on the feature branch