Skip to content

Commit 29ad680

Browse files
committed
Address review feedback and port address-review command
1 parent 5945fe5 commit 29ad680

10 files changed

Lines changed: 214 additions & 39 deletions

File tree

.claude/commands/address-review.md

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
---
2+
description: Fetch GitHub PR review comments, triage them, create todos for must-fix items, reply to comments, and resolve addressed threads
3+
---
4+
5+
Fetch review comments from a GitHub PR in this repository, triage them, and create a todo list only for items worth addressing.
6+
7+
# Instructions
8+
9+
## Step 1: Determine the Repository
10+
11+
```bash
12+
REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner)
13+
```
14+
15+
If this command fails, ensure `gh` CLI is installed and authenticated (`gh auth status`).
16+
17+
## Step 2: Parse User Input
18+
19+
Extract the PR number and optional review/comment ID from the user's message:
20+
21+
**Supported formats:**
22+
23+
- PR number only: `12345`
24+
- PR URL: `https://github.com/org/repo/pull/12345`
25+
- Specific PR review: `https://github.com/org/repo/pull/12345#pullrequestreview-123456789`
26+
- Specific issue comment: `https://github.com/org/repo/pull/12345#issuecomment-123456789`
27+
28+
**URL parsing:**
29+
30+
- Extract org/repo from URL path: `github.com/{org}/{repo}/pull/{PR_NUMBER}`
31+
- Extract fragment ID after `#` (e.g., `pullrequestreview-123456789``123456789`)
32+
- If a full GitHub URL is provided, use the org/repo from the URL instead of the current repo
33+
34+
## Step 3: Fetch Review Comments
35+
36+
**If a specific issue comment ID is provided (`#issuecomment-...`):**
37+
38+
```bash
39+
gh api repos/${REPO}/issues/comments/{COMMENT_ID} | jq '{body: .body, user: .user.login, html_url: .html_url}'
40+
```
41+
42+
**If a specific review ID is provided (`#pullrequestreview-...`):**
43+
44+
```bash
45+
gh api repos/${REPO}/pulls/{PR_NUMBER}/reviews/{REVIEW_ID}/comments | jq '[.[] | {id: .id, path: .path, body: .body, line: .line, start_line: .start_line, user: .user.login}]'
46+
```
47+
48+
**If only PR number is provided (fetch all PR review comments):**
49+
50+
```bash
51+
gh api repos/${REPO}/pulls/{PR_NUMBER}/comments | jq '[.[] | {id: .id, path: .path, body: .body, line: .line, start_line: .start_line, user: .user.login, in_reply_to_id: .in_reply_to_id}]'
52+
```
53+
54+
**Filtering comments:**
55+
56+
- Skip comments where `in_reply_to_id` is set (these are replies, not top-level comments)
57+
- Do not skip bot-generated comments by default. Many actionable review comments in this repository come from bots.
58+
- Deduplicate repeated bot comments and skip bot status posts, summaries, and acknowledgments that do not require a code or documentation change
59+
- Treat as actionable by default only: correctness bugs, regressions, missing tests, and clear inconsistencies with adjacent code
60+
- Treat as non-actionable by default: style nits, speculative suggestions, changelog wording, duplicate bot comments, and "could consider" feedback unless the user explicitly asks for polish work
61+
- Focus on actionable feedback, not acknowledgments or thank-you messages
62+
63+
**Error handling:**
64+
65+
- If the API returns 404, the PR/comment doesn't exist - inform the user
66+
- If the API returns 403, check authentication with `gh auth status`
67+
- If the response is empty, inform the user no review comments were found
68+
69+
## Step 4: Triage Comments
70+
71+
Before creating any todos, classify every review comment into one of three categories:
72+
73+
- `MUST-FIX`: correctness bugs, regressions, security issues, missing tests that could hide a real bug, and clear inconsistencies with adjacent code that would likely block merge
74+
- `DISCUSS`: reasonable suggestions that expand scope, architectural opinions that are not clearly right or wrong, and comments where the reviewer claim may be correct but needs a user decision
75+
- `SKIPPED`: style preferences, documentation nits, comment requests, test-shape preferences, speculative suggestions, changelog wording, duplicate comments, status posts, summaries, and factually incorrect suggestions
76+
77+
Triage rules:
78+
79+
- Deduplicate overlapping comments before classifying them. Keep one representative item for the underlying issue.
80+
- Verify factual claims locally before classifying a comment as `MUST-FIX`.
81+
- If a claim appears wrong, classify it as `SKIPPED` and note briefly why.
82+
- Preserve the original review comment ID and thread ID when available so the command can reply to the correct place and resolve the correct thread later.
83+
84+
## Step 5: Create Todo List
85+
86+
Create a todo list with TodoWrite containing **only the `MUST-FIX` items**:
87+
88+
- One todo per must-fix comment or deduplicated issue
89+
- For file-specific comments: `"{file}:{line} - {comment_summary} (@{username})"` (content)
90+
- For general comments: Parse the comment body and extract the must-fix action
91+
- Format activeForm: `"Addressing {brief description}"`
92+
- All todos should start with status: `"pending"`
93+
94+
## Step 6: Present Triage to User
95+
96+
Present the triage to the user - **DO NOT automatically start addressing items**:
97+
98+
- `MUST-FIX ({count})`: list the todos created
99+
- `DISCUSS ({count})`: list items needing user choice, with a short reason
100+
- `SKIPPED ({count})`: list skipped comments with a short reason, including duplicates and factually incorrect suggestions
101+
- Wait for the user to tell you which items to address
102+
- If useful, suggest replying with a brief rationale on declined items, but do not do so unless the user asks
103+
104+
## Step 7: Address Items, Reply, and Resolve
105+
106+
When addressing items, after completing each selected todo item, reply to the original review comment explaining how it was addressed.
107+
108+
**For issue comments (general PR comments):**
109+
110+
```bash
111+
gh api repos/${REPO}/issues/{PR_NUMBER}/comments -X POST -f body="<response>"
112+
```
113+
114+
**For PR review comments (file-specific, replying to a thread):**
115+
116+
```bash
117+
gh api repos/${REPO}/pulls/{PR_NUMBER}/comments/{COMMENT_ID}/replies -X POST -f body="<response>"
118+
```
119+
120+
**For standalone review comments (not in a thread):**
121+
122+
```bash
123+
gh api repos/${REPO}/pulls/{PR_NUMBER}/comments -X POST -f body="<response>" -f commit_id="<COMMIT_SHA>" -f path="<FILE_PATH>" -f line=<LINE_NUMBER> -f side="RIGHT"
124+
```
125+
126+
Note: `side` is required when using `line`. Use `"RIGHT"` for the PR commit side (most common) or `"LEFT"` for the base commit side.
127+
128+
The response should briefly explain:
129+
130+
- What was changed
131+
- Which commit(s) contain the fix
132+
- Any relevant details or decisions made
133+
134+
After posting the reply, resolve the review thread when all of the following are true:
135+
136+
- The comment belongs to a review thread and you have the thread ID
137+
- The concern was actually addressed in code, tests, or documentation, or it was explicitly declined with a clear explanation approved by the user
138+
- The thread is not already resolved
139+
140+
Use GitHub GraphQL to resolve the thread:
141+
142+
```bash
143+
gh api graphql -f query='mutation($threadId:ID!) { resolveReviewThread(input:{threadId:$threadId}) { thread { id isResolved } } }' -f threadId="<THREAD_ID>"
144+
```
145+
146+
Do not resolve a thread if the fix is still pending, if you are unsure whether the reviewer concern is satisfied, or if the user asked to leave the thread open.
147+
148+
If the user explicitly asks to close out a `DISCUSS` or `SKIPPED` item, reply with the rationale and resolve the thread only when the conversation is actually complete.
149+
150+
# Example Usage
151+
152+
```text
153+
/address-review https://github.com/org/repo/pull/12345#pullrequestreview-123456789
154+
/address-review https://github.com/org/repo/pull/12345#issuecomment-123456789
155+
/address-review 12345
156+
/address-review https://github.com/org/repo/pull/12345
157+
```
158+
159+
# Example Output
160+
161+
After fetching and triaging comments, present them like this:
162+
163+
```text
164+
Found 5 review comments. Triage:
165+
166+
MUST-FIX (1):
167+
1. ⬜ src/helper.rb:45 - Missing nil guard causes a crash on empty input (@reviewer1)
168+
169+
DISCUSS (1):
170+
2. src/config.rb:12 - Extract this to a shared config constant (@reviewer1)
171+
Reason: reasonable suggestion, but it expands scope
172+
173+
SKIPPED (3):
174+
3. src/helper.rb:50 - "Consider adding a comment" (@claude[bot]) - documentation nit
175+
4. src/helper.rb:45 - Same nil guard issue (@greptile-apps[bot]) - duplicate of #1
176+
5. spec/helper_spec.rb:20 - "Consolidate assertions" (@claude[bot]) - test style preference
177+
178+
Which items would you like me to address? (e.g., "1", "1,2", or "all must-fix")
179+
```
180+
181+
# Important Notes
182+
183+
- Automatically detect the repository using `gh repo view` for the current working directory
184+
- If a GitHub URL is provided, extract the org/repo from the URL
185+
- Include file path and line number in each todo for easy navigation (when available)
186+
- Include the reviewer's username in the todo text
187+
- If a comment doesn't have a specific line number, note it as "general comment"
188+
- **NEVER automatically address all review comments** - always wait for user direction
189+
- When given a specific review URL, no need to ask for more information
190+
- **ALWAYS reply to comments after addressing them** to close the feedback loop
191+
- Resolve the review thread after replying when the concern is actually addressed and a thread ID is available
192+
- Default to real issues only. Do not spend a review cycle on optional polish unless the user explicitly asks for it
193+
- Triage comments before creating todos. Only `MUST-FIX` items should become todos by default
194+
- For large review comments (like detailed code reviews), parse and extract the actionable items into separate todos
195+
196+
# Known Limitations
197+
198+
- Rate limiting: GitHub API has rate limits; if you hit them, wait a few minutes
199+
- Private repos: Requires appropriate `gh` authentication scope

.gitignore

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,5 +65,7 @@ client/app/bundles/comments/rescript/**/*.bs.js
6565
**/*.res.mjs
6666
**/*.bs.js
6767

68-
.claude/
68+
.claude/*
69+
!.claude/commands/
70+
!.claude/commands/address-review.md
6971
.context/

Procfile.dev-prod-assets

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,4 +7,4 @@ redis: redis-server
77
# upon saving rather than when you refresh your browser page.
88
# Note, if using React on Rails localization you will need to run
99
# `bundle exec rake react_on_rails:locale` before you run bin/shakapacker
10-
rspack: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w'
10+
rspack: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* && bin/shakapacker -w'

Procfile.dev-static

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,4 +8,4 @@ redis: redis-server
88
# Note, if using React on Rails localization you will need to run
99
# `bundle exec rake react_on_rails:locale` before you run bin/shakapacker
1010
# Sleep 5 to allow other rescript files to build
11-
rspack: sleep 5 && sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w'
11+
rspack: sleep 5 && sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* && bin/shakapacker -w'

Procfile.dev-static-assets

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,4 +7,4 @@ redis: redis-server
77
# upon saving rather than when you refresh your browser page.
88
# Note, if using React on Rails localization you will need to run
99
# `bundle exec rake react_on_rails:locale` before you run bin/shakapacker
10-
rspack: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* || true && bin/shakapacker -w'
10+
rspack: sh -c 'bundle exec rake react_on_rails:locale && rm -rf public/packs/* && bin/shakapacker -w'

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ React on Rails Pro provides Node server rendering and other performance enhancem
3030

3131
For more information, see the [React on Rails Pro Docs](https://www.shakacode.com/react-on-rails-pro/).
3232

33-
* Optimizing your front end setup with Rspack + Shakapacker for React on Rails, including SSR and code splitting.
33+
* Optimizing your front-end setup with Rspack + Shakapacker for React on Rails, including SSR and code splitting.
3434
* Upgrading your app to the current React on Rails 16.4 / Shakapacker 9.7 stack with modern asset builds.
3535
* Better performance client and server side.
3636

config/webpack/bundlerUtils.js

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ const ensureRspack = () => {
1313
if (config.assets_bundler !== 'rspack') {
1414
throw new Error(
1515
`Invalid assets_bundler: "${config.assets_bundler}". ` +
16-
'This project is configured for Rspack only. ' +
17-
'Set assets_bundler: rspack in config/shakapacker.yml',
16+
'This project is configured for Rspack only. ' +
17+
'Set assets_bundler: rspack in config/shakapacker.yml',
1818
);
1919
}
2020
};
@@ -37,13 +37,6 @@ const getBundler = () => {
3737
return _cachedBundler;
3838
};
3939

40-
/**
41-
* Checks if the current bundler is Rspack.
42-
*
43-
* @returns {boolean} True if using Rspack
44-
*/
45-
const isRspack = () => config.assets_bundler === 'rspack';
46-
4740
/**
4841
* Gets the CSS extraction plugin for Rspack.
4942
*
@@ -53,6 +46,5 @@ const getCssExtractPlugin = () => getBundler().CssExtractRspackPlugin;
5346

5447
module.exports = {
5548
getBundler,
56-
isRspack,
5749
getCssExtractPlugin,
5850
};

config/webpack/client.js

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,9 @@
11
const devBuild = process.env.NODE_ENV === 'development';
22
const isHMR = process.env.WEBPACK_DEV_SERVER === 'TRUE';
3-
const { config } = require('shakapacker');
3+
const { getBundler } = require('./bundlerUtils');
44
const environment = require('./environment');
55

6-
// Auto-detect bundler from shakapacker config and load the appropriate library
7-
const bundler = config.assets_bundler === 'rspack'
8-
? require('@rspack/core')
9-
: require('webpack');
6+
const bundler = getBundler();
107

118
if (devBuild && !isHMR) {
129
environment.loaders.get('sass').use.find((item) => item.loader === 'sass-loader').options.sourceMap = false;
@@ -23,8 +20,4 @@ environment.plugins.append(
2320
}),
2421
);
2522

26-
if (devBuild && isHMR && config.assets_bundler !== 'rspack') {
27-
// Rspack is the only supported bundler for this repo; no webpack refresh plugin wiring.
28-
}
29-
3023
module.exports = environment;

config/webpack/development.js

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,7 @@
33

44
process.env.NODE_ENV = process.env.NODE_ENV || 'development';
55

6-
const { inliningCss } = require('shakapacker');
7-
86
const webpackConfig = require('./webpackConfig');
97

10-
const developmentEnvOnly = (_clientWebpackConfig, _serverWebpackConfig) => {
11-
if (inliningCss) {
12-
// Rspack HMR/refresh is handled by Shakapacker + @rspack/plugin-react-refresh.
13-
// No extra development-only plugin wiring is needed here.
14-
}
15-
};
16-
17-
module.exports = webpackConfig(developmentEnvOnly);
8+
// Rspack HMR/refresh is handled by Shakapacker + @rspack/plugin-react-refresh.
9+
module.exports = webpackConfig();

config/webpack/server.js

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,10 @@
11
const merge = require('webpack-merge');
22

33
const devBuild = process.env.NODE_ENV === 'production' ? 'production' : 'development';
4-
const { config } = require('shakapacker');
4+
const { getBundler } = require('./bundlerUtils');
55
const environment = require('./environment');
66

7-
// Auto-detect bundler from shakapacker config and load the appropriate library
8-
const bundler = config.assets_bundler === 'rspack'
9-
? require('@rspack/core')
10-
: require('webpack');
7+
const bundler = getBundler();
118

129
// React Server Side Rendering shakapacker config
1310
// Builds a Node compatible file that React on Rails can load, never served to the client.

0 commit comments

Comments
 (0)