Skip to content

Commit 98f833a

Browse files
authored
Upgrade React on Rails/Shakapacker and standardize on Rspack v2 (#702)
* Upgrade to react_on_rails 16.4.0, shakapacker 9.7, and rspack v2 * Fix review-app deploy by using Node 22.12.0 in Control Plane image * Address review feedback and port address-review command * Make isRspack strict and align tests * Add language tag to README process management block * Address remaining review nits for merge readiness * Address remaining review comments for merge * Tighten Rspack runtime setup and config safety
1 parent 1e191ab commit 98f833a

File tree

17 files changed

+919
-957
lines changed

17 files changed

+919
-957
lines changed

.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

.controlplane/Dockerfile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ RUN apt-get update -qq && \
1212

1313
# Install JavaScript dependencies
1414
# Make sure NODE_VERSION matches the node version in .nvmrc and package.json
15-
ARG NODE_VERSION=22.3.0
15+
ARG NODE_VERSION=22.12.0
1616
ARG YARN_VERSION=1.22.19
1717
ENV PATH=/usr/local/node/bin:$PATH
1818
RUN curl -sL https://github.com/nodenv/node-build/archive/master.tar.gz | tar xz -C /tmp/ && \

.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/

Gemfile

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,8 @@ git_source(:github) { |repo| "https://github.com/#{repo}.git" }
55

66
ruby "3.4.6"
77

8-
gem "react_on_rails", "16.4.0.rc.9"
9-
gem "shakapacker", "9.6.1"
8+
gem "react_on_rails", "16.4.0"
9+
gem "shakapacker", "9.7.0"
1010

1111
# Bundle edge Rails instead: gem "rails", github: "rails/rails"
1212
gem "listen"

Gemfile.lock

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,7 @@ GEM
293293
erb
294294
psych (>= 4.0.0)
295295
tsort
296-
react_on_rails (16.4.0.rc.9)
296+
react_on_rails (16.4.0)
297297
addressable
298298
connection_pool
299299
execjs (~> 2.5)
@@ -384,7 +384,7 @@ GEM
384384
websocket (~> 1.0)
385385
semantic_range (3.1.1)
386386
sexp_processor (4.17.1)
387-
shakapacker (9.6.1)
387+
shakapacker (9.7.0)
388388
activesupport (>= 5.2)
389389
package_json
390390
rack-proxy (>= 0.6.1)
@@ -483,7 +483,7 @@ DEPENDENCIES
483483
rails-html-sanitizer
484484
rails_best_practices
485485
rainbow
486-
react_on_rails (= 16.4.0.rc.9)
486+
react_on_rails (= 16.4.0)
487487
redcarpet
488488
redis (~> 5.0)
489489
rspec-rails (~> 6.0.0)
@@ -495,7 +495,7 @@ DEPENDENCIES
495495
scss_lint
496496
sdoc
497497
selenium-webdriver (~> 4)
498-
shakapacker (= 9.6.1)
498+
shakapacker (= 9.7.0)
499499
spring
500500
spring-commands-rspec
501501
stimulus-rails (~> 1.3)

Procfile.dev

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,13 @@
22
# You can run these commands in separate shells
33
#
44
# Note: bin/dev runs precompile tasks (rescript + locale) BEFORE starting these processes.
5-
# This ensures all generated files exist before webpack starts watching.
5+
# This ensures all generated files exist before Rspack starts watching.
66
#
77
# ReScript watch mode (no clean - bin/dev already did the clean build)
88
rescript: yarn res:watch
99
# redis: redis-server # Run Redis as a system service instead (brew services start redis)
1010
rails: bundle exec thrust bin/rails server -p 3000
11-
# Client webpack dev server with HMR
11+
# Client Rspack dev server with HMR
1212
wp-client: RAILS_ENV=development NODE_ENV=development bin/shakapacker-dev-server
13-
# Server webpack watcher for SSR bundle
13+
# Server Rspack watcher for SSR bundle
1414
wp-server: SERVER_BUNDLE_ONLY=yes bin/shakapacker --watch

Procfile.dev-prod-assets

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@
22
web: bundle exec thrust bin/rails server -p 3001
33
redis: redis-server
44

5-
# Next line runs a watch process with webpack to compile the changed files.
6-
# When making frequent changes to client side assets, you will prefer building webpack assets
5+
# Next line runs a watch process with Rspack to compile changed files.
6+
# When making frequent changes to client side assets, you will prefer building Rspack assets
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-
webpack: 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/* || true) && bin/shakapacker -w'

Procfile.dev-static

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,10 @@
22
web: bundle exec thrust bin/rails server -p 3000
33
redis: redis-server
44

5-
# Next line runs a watch process with webpack to compile the changed files.
6-
# When making frequent changes to client side assets, you will prefer building webpack assets
5+
# Next line runs a watch process with Rspack to compile changed files.
6+
# When making frequent changes to client side assets, you will prefer building Rspack assets
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
1010
# Sleep 5 to allow other rescript files to build
11-
webpack: 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/* || true) && bin/shakapacker -w'

Procfile.dev-static-assets

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@
22
web: bundle exec thrust bin/rails server -p 3000
33
redis: redis-server
44

5-
# Next line runs a watch process with webpack to compile the changed files.
6-
# When making frequent changes to client side assets, you will prefer building webpack assets
5+
# Next line runs a watch process with Rspack to compile changed files.
6+
# When making frequent changes to client side assets, you will prefer building Rspack assets
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-
webpack: 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/* || true) && bin/shakapacker -w'

0 commit comments

Comments
 (0)