Migrate to Shakapacker 9.0.0-beta.7 with Babel transpiler #662
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughBumps Shakapacker to 9.0.0.beta.7 (Ruby + JS), adds Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite as Test Suite
participant CapybaraConfig as Capybara Config
participant Selenium as Selenium/Chrome
Note over CapybaraConfig: New inlined driver registration
TestSuite->>CapybaraConfig: load rails_helper / driver setup
CapybaraConfig->>Selenium: register :selenium_chrome_headless (with Chrome::Options)
CapybaraConfig->>CapybaraConfig: register screenshot handler for :selenium_chrome_headless
TestSuite->>CapybaraConfig: request driver for JS tests
CapybaraConfig->>Selenium: instantiate headless Chrome driver
Selenium->>TestSuite: provides browser session
Note right of Selenium: Screenshots saved via registered handler on failures
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
|
/deploy-review-app |
1 similar comment
|
/deploy-review-app |
Code Review for PR #662: Migrate to Shakapacker 9.0.0-beta.4 with Babel transpilerThank you for this well-structured migration PR! I've reviewed the changes and here's my detailed feedback: ✅ Strengths
🔍 Code Quality & Best PracticesGood:
Suggestions:
|
PR Review: Migrate to Shakapacker 9.0.0-beta.4 with Babel transpilerSummaryThis PR successfully migrates from Shakapacker 8.2.0 to 9.0.0-beta.4, but introduces a workaround for SSR issues that should be properly addressed before merging. ✅ Positive Aspects
🔴 Critical Issues1. SSR Workaround Instead of FixThe defensive programming pattern added to handle missing const formatMessage = intl && intl.formatMessage ? intl.formatMessage : (msg) => msg.defaultMessage || '';Issues with this approach:
Recommended fix: yarn add -D babel-plugin-formatjsThen update babel.config.js: plugins: [
['formatjs', {
idInterpolationPattern: '[sha512:contenthash:base64:6]',
ast: true
}],
// ... other plugins
]2. Missing Test Coverage
🟡 Performance Considerations
🟡 Security Concerns
📝 Recommendations
🚦 VerdictRequest Changes - While the migration is technically correct, the SSR workaround should be replaced with a proper fix using babel-plugin-formatjs. The current approach creates technical debt and masks configuration issues. Consider creating a separate branch to properly configure babel-plugin-formatjs and test SSR functionality thoroughly before proceeding with the SWC migration. 🤖 Generated with Claude Code Co-Authored-By: Claude noreply@anthropic.com |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx (1)
104-106: Consider extracting the SSR guard to eliminate duplication.The same SSR guard pattern for
formatMessageis repeated four times acrossformHorizontal,formStacked,formInline, andrender. This duplication increases maintenance burden and the risk of inconsistent updates.Consider extracting the guard logic to a helper method or computing it once in
renderand passing it to the form methods:Option 1: Extract to instance method
+ getFormatMessage() { + const { intl } = this.props; + return intl && intl.formatMessage ? intl.formatMessage : (msg) => msg.defaultMessage || ''; + } + formHorizontal() { - // Safeguard for SSR where intl might not be properly initialized - const { intl } = this.props; - const formatMessage = intl && intl.formatMessage ? intl.formatMessage : (msg) => msg.defaultMessage || ''; + const formatMessage = this.getFormatMessage(); return (Apply the same change to
formStacked,formInline, andrender.Option 2: Compute once in render and pass as parameter
render() { + const formatMessage = this.getFormatMessage(); let inputForm; switch (this.state.formMode) { case 0: - inputForm = this.formHorizontal(); + inputForm = this.formHorizontal(formatMessage); break;Then update each form method signature to accept
formatMessageas a parameter.Also applies to: 161-163, 218-220, 323-325
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
Gemfile(1 hunks)client/app/bundles/comments/components/CommentBox/CommentBox.jsx(1 hunks)client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx(4 hunks)client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx(1 hunks)config/shakapacker.yml(1 hunks)package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: deploy
- GitHub Check: claude-review
🔇 Additional comments (6)
Gemfile (1)
9-9: LGTM! Version aligns with npm package.The gem version format (9.0.0.beta.4) correctly matches the npm package version (9.0.0-beta.4) from package.json. The beta version concern applies here as well—ensure thorough testing before production deployment.
config/shakapacker.yml (1)
11-11: LGTM! Babel transpiler configuration is explicit.The addition of
javascript_transpiler: babelclearly documents the transpilation strategy. This aligns with the existing Babel dependencies in package.json (babel-loader, @babel/core, etc.).Per the PR objectives, switching to SWC is planned as a follow-up. Consider opening a tracking issue for that migration if one doesn't exist yet.
client/app/bundles/comments/components/CommentBox/CommentBox.jsx (1)
77-78: LGTM! SSR-safe guard for intl is well-implemented.The fallback ensures
formatMessageis always callable, preventing runtime errors during SSR whenintlmight be uninitialized. The fallback correctly usesmsg.defaultMessage(from thedefaultMessagesimport on line 8) as a sensible default.This pattern is consistently applied across other components in this PR (CommentForm, SimpleCommentScreen).
client/app/bundles/comments/components/SimpleCommentScreen/ror_components/SimpleCommentScreen.jsx (1)
75-76: LGTM! Consistent SSR-safe implementation.The guard implementation is identical to the one in CommentBox.jsx (lines 77-78), ensuring consistency across components. The fallback chain (
msg.defaultMessage || '') handles edge cases gracefully.package.json (1)
96-96: Verify beta stability and migration plan.You're using Shakapacker 9.0.0-beta.4, but the latest stable release is 8.4.0. Version 9.0.0 has not reached GA and lacks official migration documentation. Ensure thorough testing in staging before deploying to production.
Track the official 9.0.0 release and review the migration guide once available. Consider the stability requirements of your production environment when using pre-release versions.
client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx (1)
104-106: Clarify whenintlwould be unavailable.The component is wrapped with
injectIntl(line 371) and declaresintlas a required prop (line 19). Under normal circumstances with a properIntlProviderwrapper,intlshould always be available.Can you clarify the specific scenario where
intlorintl.formatMessagewould be unavailable?
- Is this addressing a case where the component is rendered outside of
IntlProvider?- Is this a temporary workaround for the babel-plugin-formatjs issue mentioned in the PR objectives?
- Should this guard be removed once the SWC transpiler migration is complete?
Understanding the specific use case will help determine if this is the right long-term solution or if there's a better approach (e.g., ensuring
IntlProvideralways wraps the component tree).
| const { formatMessage } = this.props.intl; | ||
| // Safeguard for SSR where intl might not be properly initialized | ||
| const { intl } = this.props; | ||
| const formatMessage = intl && intl.formatMessage ? intl.formatMessage : (msg) => msg.defaultMessage || ''; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that empty string fallback is acceptable for SSR.
The SSR guard falls back to returning msg.defaultMessage || ''. While this prevents runtime errors, it may result in empty labels, placeholders, and button text during server-side rendering if defaultMessage is undefined.
Please verify that:
- All message descriptors in
defaultMessageshavedefaultMessageproperties defined - Empty strings during SSR won't negatively impact SEO or initial page render
- The client-side hydration correctly replaces these with proper localized messages
You can verify the message descriptor structure with:
Also applies to: 163-163, 220-220, 325-325
🏁 Script executed:
#!/bin/bash
# Check if all defaultMessages have defaultMessage property
cat client/app/libs/i18n/default.js client/app/libs/i18n/default.ts client/app/libs/i18n/default.jsx 2>/dev/null | head -100Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find the i18n default messages file
echo "=== Finding i18n default file ==="
fd -t f "default" client/app/libs/i18n 2>/dev/null || echo "Path client/app/libs/i18n not found"
fd -t f "default" libs/i18n 2>/dev/null || echo "Path libs/i18n not found"
echo -e "\n=== Search for i18n directory ==="
fd -t d "i18n" client 2>/dev/null | head -10
echo -e "\n=== Find files with 'default' in i18n contexts ==="
fd -e js -e ts -e jsx "default" client/app 2>/dev/null | grep -i i18n | head -10Length of output: 167
🏁 Script executed:
#!/bin/bash
# Search for where defaultMessages is defined or exported
echo "=== Search for defaultMessages export ==="
rg -n "export.*defaultMessages" client/app --type js --type ts -C2 | head -50Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check how formatMessage is used with message descriptors in this file
echo "=== Check formatMessage usage in CommentForm.jsx ==="
rg -n "formatMessage\(" client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx -A1 -B1 | head -80Length of output: 3344
🏁 Script executed:
#!/bin/bash
# List all files in the i18n directory
echo "=== Files in client/app/libs/i18n/ ==="
ls -la client/app/libs/i18n/ 2>/dev/null || echo "Directory not found"Length of output: 214
🏁 Script executed:
#!/bin/bash
# Show the import statement and surrounding context
echo "=== Import statement context in CommentForm.jsx ==="
cat client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx | head -20Length of output: 932
🏁 Script executed:
#!/bin/bash
# Search for defineMessages usage in the codebase
echo "=== Search for defineMessages in client/app/libs/i18n ==="
rg -n "defineMessages" client/app/libs/i18n -A5 -B2 2>/dev/null | head -100Length of output: 57
🏁 Script executed:
#!/bin/bash
# Look for any .js, .jsx, .ts files in i18n directory
echo "=== Finding JavaScript/TypeScript files in i18n ==="
fd -e js -e jsx -e ts -e tsx . client/app/libs/i18n 2>/dev/nullLength of output: 91
🏁 Script executed:
#!/bin/bash
# Search for libs/i18n/default at different locations
echo "=== Search for default file at project root ==="
fd -t f "default" libs/i18n 2>/dev/null || echo "Path libs/i18n not found at root"
echo -e "\n=== Find all i18n directories ==="
fd -t d "^i18n$" . 2>/dev/null | head -10Length of output: 139
🏁 Script executed:
#!/bin/bash
# Search for defaultMessages definition/export across entire codebase
echo "=== Search for defaultMessages definition ==="
rg -n "defaultMessages\s*=" client --type js --type jsx -C3 2>/dev/null | head -80Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search for export of defaultMessages with more flexible pattern
echo "=== Search for defaultMessages export (flexible) ==="
rg -n "defaultMessages" client/app/libs -C3 2>/dev/null | head -80Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check if there's a default.js or default/index.js anywhere
echo "=== Find files named default.js, default.jsx, default.ts ==="
fd -t f "^default\.(js|jsx|ts|tsx)$" client 2>/dev/null
fd -t f "^index\.(js|jsx|ts|tsx)$" client/app/libs/i18n 2>/dev/nullLength of output: 60
🏁 Script executed:
#!/bin/bash
# Search entire repo for libs/i18n/default directory or file
echo "=== Search entire repo for libs/i18n/default ==="
fd -t f "default" . 2>/dev/null | grep -i "i18n" | head -20
echo -e "\n=== Search for any libs directory at root ==="
ls -la libs/ 2>/dev/null || echo "No libs directory at root"Length of output: 121
🏁 Script executed:
#!/bin/bash
# Search for other files importing from libs/i18n/default
echo "=== Files importing from libs/i18n/default ==="
rg -n "from ['\"]libs/i18n/default['\"]" . --type js --type jsx -C1 2>/dev/null | head -50Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check if there's a webpack config or babel config that might alias this
echo "=== Search for webpack or babel config ==="
fd -t f "webpack" . 2>/dev/null | head -10
fd -t f "babel" . 2>/dev/null | head -10Length of output: 249
🏁 Script executed:
#!/bin/bash
# Search the entire codebase for where defaultMessages object is defined
echo "=== Search for defineMessages across entire codebase ==="
rg -n "defineMessages" . --type js --type jsx -g '!node_modules' -g '!dist' -g '!build' 2>/dev/null | head -30Length of output: 56
🏁 Script executed:
#!/bin/bash
# Check webpack config for resolve aliases
echo "=== Check webpack resolve aliases ==="
rg -n "resolve.*alias" config/webpack/ -A10 -B2 2>/dev/null | head -60Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check commonWebpackConfig specifically
echo "=== Check commonWebpackConfig.js ==="
cat config/webpack/commonWebpackConfig.js 2>/dev/null | grep -A15 -B5 "resolve\|alias" | head -80Length of output: 1057
🏁 Script executed:
#!/bin/bash
# Search for 'libs' path resolution
echo "=== Search for libs path in webpack config ==="
rg -n "libs" config/webpack/commonWebpackConfig.js -C3 2>/dev/nullLength of output: 46
🏁 Script executed:
#!/bin/bash
# Check if this import exists in git history or is newly added
echo "=== Check git log for this import ==="
git log --all --oneline -S "from 'libs/i18n/default'" -- "client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx" 2>/dev/null | head -10Length of output: 128
🏁 Script executed:
#!/bin/bash
# Check what files were changed in the i18n commit
echo "=== Files changed in commit ef369ed ==="
git show --name-status ef369ed 2>/dev/null | head -30Length of output: 1136
🏁 Script executed:
#!/bin/bash
# Check the full diff for the import line specifically
echo "=== Show changes to CommentForm.jsx in ef369ed ==="
git show ef369ed:client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx 2>/dev/null | head -30Length of output: 1320
🏁 Script executed:
#!/bin/bash
# Check if any i18n/default file was added in this commit
echo "=== Check if default file was added ==="
git show --name-only ef369ed 2>/dev/null | grep -i "default\|i18n"Length of output: 325
🏁 Script executed:
#!/bin/bash
# Check the shakapacker config for module resolution
echo "=== Check package.json and shakapacker config ==="
cat config/shakapacker.yml 2>/dev/null | head -40Length of output: 1298
🏁 Script executed:
#!/bin/bash
# Check git log for libs/i18n/default file history
echo "=== Git history for libs/i18n/default ==="
git log --all --full-history --oneline -- "**/i18n/default*" 2>/dev/null | head -20Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check if the file exists in any branch or was ever committed
echo "=== Search all commits for i18n/default file ==="
git log --all --oneline --name-only -- "client/app/libs/i18n/default*" 2>/dev/null | head -30Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check current branch and PR diff
echo "=== Current branch info ==="
git branch --show-current 2>/dev/null
echo ""
git log --oneline -5 2>/dev/nullLength of output: 345
🏁 Script executed:
#!/bin:bash
# Try to find if there's a default.js file in git history at all
echo "=== Search for any default file creation in libs/i18n ==="
git log --all --diff-filter=A --name-only --pretty=format: -- "*/libs/i18n/*" 2>/dev/null | grep -i default | head -20Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check the exact current state of CommentForm.jsx imports
echo "=== Current CommentForm.jsx first 30 lines ==="
head -30 client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx 2>/dev/nullLength of output: 1160
🏁 Script executed:
#!/bin/bash
# Search for any object or const called defaultMessages in the file itself
echo "=== Search for defaultMessages definition in CommentForm.jsx ==="
rg -n "defaultMessages" client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx -C5Length of output: 7818
🏁 Script executed:
#!/bin/bash
# Check if defaultMessages might be injected via babel or defined elsewhere
echo "=== Search for defaultMessages anywhere in comments bundle ==="
rg -n "export.*defaultMessages|const defaultMessages|defaultMessages.*=" client/app/bundles/comments -C2 2>/dev/null | head -50Length of output: 62
🏁 Script executed:
#!/bin/bash
# Check the git diff for the current PR to see what changed
echo "=== Git diff for current commit ==="
git show HEAD:client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx 2>/dev/null | grep -A5 -B5 "import.*default"Length of output: 439
🏁 Script executed:
#!/bin/bash
# Check if the file existed in the parent commit
echo "=== Check parent commit for libs/i18n/default ==="
git show HEAD~1:client/app/libs/i18n/default.js 2>/dev/null | head -20 || echo "File does not exist in parent commit"
git show HEAD~1:client/app/libs/i18n/default.ts 2>/dev/null | head -20 || echo "File does not exist in parent commit"
git show HEAD~1:client/app/libs/i18n/default.jsx 2>/dev/null | head -20 || echo "File does not exist in parent commit"Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check several commits back
echo "=== Check 5 commits back ==="
git show HEAD~5:client/app/libs/i18n/default.js 2>/dev/null | head -20 || echo "File does not exist 5 commits back"Length of output: 28
🏁 Script executed:
#!/bin/bash
# List all files in i18n directory from parent commit
echo "=== Files in i18n dir in parent commit ==="
git ls-tree -r --name-only HEAD~1 client/app/libs/i18n/ 2>/dev/nullLength of output: 82
🏁 Script executed:
#!/bin/bash
# Check if this might be resolved via package or node_modules
echo "=== Search for defaultMessages in package.json dependencies ==="
cat package.json 2>/dev/null | jq '.dependencies, .devDependencies' | grep -i "i18n\|intl" || echo "No obvious i18n packages"Length of output: 109
Critical: Missing required file for import
The import import { defaultMessages } from 'libs/i18n/default'; on line 7 references a file that does not exist in the codebase. The file client/app/libs/i18n/default.js (or .ts, .jsx, .tsx) has never been committed to the repository.
This will cause a module resolution error at runtime, preventing the component from loading. The SSR fallback logic in lines 106, 163, 220, and 325 cannot be evaluated because the import itself will fail.
Required action:
- Create the missing
client/app/libs/i18n/default.jsfile with proper message descriptors - Ensure all message descriptors include both
idanddefaultMessageproperties:
import { defineMessages } from 'react-intl';
export const defaultMessages = defineMessages({
inputNameLabel: {
id: 'comments.form.nameLabel',
defaultMessage: 'Name'
},
inputNamePlaceholder: {
id: 'comments.form.namePlaceholder',
defaultMessage: 'Enter your name'
},
// ... other messages used in CommentForm.jsx
});- After creating the file, verify that the fallback
msg.defaultMessage || ''provides acceptable UX during SSR
🤖 Prompt for AI Agents
In client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx
around line 106, the import of defaultMessages points to a missing module which
will break module resolution; create the missing file
client/app/libs/i18n/default.js exporting a defaultMessages object (use
react-intl defineMessages or a plain object) and include message descriptors for
every message used in CommentForm.jsx (each descriptor must have id and
defaultMessage keys, e.g., inputNameLabel, inputNamePlaceholder, and any others
referenced at lines ~106, 163, 220, 325); save the file and ensure the existing
import statement matches the export name so the SSR fallback (msg.defaultMessage
|| '') can work correctly.
justin808
changed the title
|
✅ Deployment complete for PR #662, commit 20fab2e |
PR Review: Migrate to Shakapacker 9.0.0-beta.7Overall AssessmentThis is a well-structured migration PR with clear documentation and testing. The changes are focused and appropriate for a beta migration. Here's my detailed feedback: ✅ Strengths
🔍 Code Quality Issues1. Repeated Code Pattern - DRY Violation (Medium Priority)Location: All modified JSX files (CommentForm.jsx:104-106, 162-163, 218-220, 323-325, CommentBox.jsx:77-78, SimpleCommentScreen.jsx:75) Issue: The same intl safeguard is repeated 7 times across 3 files: const formatMessage = intl && intl.formatMessage ? intl.formatMessage : (msg) => msg.defaultMessage || '';Recommendation: Extract to a utility function or custom hook: // libs/i18n/helpers.js
export const getFormatMessage = (intl) =>
intl?.formatMessage ?? ((msg) => msg.defaultMessage || '');
// Usage
const formatMessage = getFormatMessage(intl);Rationale: Reduces maintenance burden, improves testability, and makes future changes easier. 2. Missing Import Issue (High Priority)Location: Multiple files import Issue: The file
Impact: This will cause runtime errors unless:
Recommendation: Verify this import path. If the file is missing, it must be created. If it's aliased, document this in the webpack config. 3. Incomplete Safeguarding (Medium Priority)Location: Issue: The Recommendation: Audit all components that use 🐛 Potential Bugs1. Unsafe Error Property Access (Medium Priority)Location: Issue: if (!error.error) return null;This assumes Fix: if (!error?.error) return null;2. HTML Input Type Issue (Low Priority)Location: Issue: Using Current: <input type="textarea" ... />Should be either: <input type="text" ... /> // single line
// OR
<textarea ... /> // multilineImpact: Browsers will treat this as 🔒 Security Considerations✅ No security concerns identified
Recommendation: Monitor Shakapacker 9.0.0 releases and upgrade from beta to stable as soon as available. ⚡ Performance Considerations
🧪 Test CoverageCurrent State: Limited test coverage for modified components
Recommendations:
Priority: Medium - While CI passes, the new SSR handling should have test coverage to prevent regressions. 📋 Additional Recommendations
🎯 SummaryApproval Status: The PR is well-executed, but the missing Action Items Before Merge:
Nice to Have (can be follow-up PRs):
Great work on this migration! The phased approach and thorough testing are commendable. 🚀 |
|
✅ Deployment complete for PR #662, commit 90bd34d |
PR Review: Migrate to Shakapacker 9.0.0-beta.7 with Babel transpilerOverall AssessmentThis is a well-executed migration PR with good documentation and incremental commits. The changes are focused and address specific compatibility issues introduced by Shakapacker 9.0.0-beta.7. ✅ Code Quality & Best Practices✅ Strengths
|
|
✅ Deployment complete for PR #662, commit 268cac8 |
This is the first of three planned PRs to migrate to Shakapacker 9.x: 1. Migrate to beta.4 with Babel transpiler (this PR) 2. Switch from Babel to SWC transpiler 3. Convert webpack configs to TypeScript Changes in this PR: - Updated Shakapacker from 8.2.0 to 9.0.0-beta.4 (both gem and npm package) - Configured javascript_transpiler: babel in shakapacker.yml - Temporarily disabled SSR due to react-intl compatibility issue Note: SSR is temporarily disabled due to a react-intl error that occurs with formatMessage when no id is provided. This appears to be related to babel-plugin-formatjs configuration. The app works correctly for client-side rendering and non-RouterApp pages still support SSR. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Re-enable SSR in React on Rails configuration - Add safeguards for formatMessage when intl context is not initialized during SSR - Add missing message definitions for form buttons - Update i18n translations to support all UI messages This fixes the react-intl SSR error that was preventing server-side rendering from working properly with Shakapacker 9.0.0-beta.4. 🤖 Generated with Claude Code (https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Update shakapacker gem to 9.0.0.beta.7
- Update shakapacker npm package to 9.0.0-beta.7
- Fix css-loader configuration for CSS modules compatibility
- Beta.7 enables namedExport by default, which requires
exportLocalsConvention to be 'camelCaseOnly' or 'dashesOnly'
- Added configuration to automatically set exportLocalsConvention
when namedExport is enabled
This resolves the CI build error:
"The modules.namedExport option requires the modules.exportLocalsConvention
option to be camelCaseOnly or dashesOnly"
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
- Removed SSR safeguards from react-intl usage in JSX components - These safeguards were added during initial beta.4 migration - They're not necessary for the shakapacker v9 upgrade - Simplified code by using destructured formatMessage directly - Fixed failing Turbo/Stimulus inline form test - Added explicit wait time for Turbo frame update to complete - Test now properly waits for async Turbo frame navigation All tests passing (38 examples, 0 failures) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The wait parameter is not needed for the Turbo frame test. The test passes without it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
PR Review: Migrate to Shakapacker 9.0.0-beta.7 with Babel transpilerSummaryThis PR successfully migrates the project from Shakapacker 8.2.0 to 9.0.0-beta.7, addresses CSS modules configuration issues, and resolves react-intl SSR compatibility problems. The migration is well-structured as the first of three planned PRs. Code Quality & Best Practices✅ Strengths
|
justin808
force-pushed
the
shakapacker-beta4-babel
branch
from
268cac8 to
5905fa9
Compare
Code Review: Migrate to Shakapacker 9.0.0-beta.7 with Babel transpilerSummaryThis PR successfully migrates from Shakapacker 8.2.0 to 9.0.0-beta.7 with appropriate fixes for compatibility issues. The changes are well-documented and the testing approach is thorough. ✅ Strengths1. Clear Migration Path
2. Proper Configuration FixesThe CSS modules configuration fix in
3. React-Intl SSR CompatibilityThe restructuring in const { intl } = this.props;
const { formatMessage } = intl;This two-step destructuring makes it easier to add null checks if needed for SSR scenarios. 4. Test Infrastructure ImprovementsThe Selenium driver updates in
🔍 Issues & RecommendationsMedium Priority: Potential Null Reference in react-intlLocation: Issue: While you've restructured the destructuring, there's still a potential runtime error if Current code: const { intl } = this.props;
const { formatMessage } = intl; // Could throw if intl is undefinedRecommendation: const { intl } = this.props;
if (!intl) {
console.error('intl prop is missing');
return null; // or render a fallback
}
const { formatMessage } = intl;Or use optional chaining: const formatMessage = this.props.intl?.formatMessage;
if (!formatMessage) return null;Low Priority: Sass-loader ConfigurationLocation: Issue: The Current code: sassOptions: {
includePaths: []
}Recommendation: Only set if you need to customize it: sassOptions: {
...(sassLoader.options?.sassOptions || {}),
}Or remove the Low Priority: Driver Registration Pattern InconsistencyLocation: Issue: The Recommendation: Consider applying the same pattern to all three methods for consistency: def self.register_selenium_chrome
Capybara.drivers.delete(:selenium_chrome)
# ... rest of implementation
endThis ensures all drivers are consistently configured in test environments. Documentation: Empty Braces RemovedLocation: The diff shows removal of empty JSX expression 🔒 Security Considerations✅ No security concerns identified
⚡ Performance Considerations✅ Positive performance implications
Note: Future PR switching to SWC will provide additional transpilation performance gains. 🧪 Test CoverageStrengths:
Recommendations:
Example test you might add: it 'renders server-side with internationalization', js: true do
visit root_path
expect(page).to have_content('Comments')
# Verify no JS errors in console
end📋 Best PracticesPositive:✅ Proper use of semantic versioning awareness (beta version) Suggestions:
✅ Approval RecommendationThis PR is ready to merge with the optional improvements noted above. The core changes are solid, well-tested, and follow good practices. The suggested improvements are mostly defensive programming enhancements that could be addressed in a follow-up PR if preferred. Priority for follow-up:
Great work on the migration! The incremental approach and thorough testing give confidence in the changes. 🎉 Review generated by Claude Code |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the project from Shakapacker 8.2.0 to 9.0.0-beta.7 with Babel as the JavaScript transpiler, preparing for a planned three-phase migration to Shakapacker 9.x.
- Updated Shakapacker gem and npm package to 9.0.0-beta.7
- Configured Babel as the JavaScript transpiler in shakapacker.yml
- Fixed CSS modules configuration compatibility issues with beta.7
Reviewed Changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Gemfile | Updated Shakapacker gem to 9.0.0.beta.7 |
| package.json | Updated Shakapacker npm package to 9.0.0-beta.7 |
| config/shakapacker.yml | Added Babel transpiler configuration |
| config/webpack/commonWebpackConfig.js | Fixed CSS modules configuration for beta.7 compatibility |
| client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx | Restructured intl access for SSR compatibility |
| spec/support/driver_registration.rb | Improved headless Chrome driver configuration |
| spec/rails_helper.rb | Updated comment for driver registration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| formHorizontal() { | ||
| const { formatMessage } = this.props.intl; | ||
| const { intl } = this.props; | ||
| const { formatMessage } = intl; |
There was a problem hiding this comment.
Potential runtime error if intl prop is undefined during SSR. Consider adding a null check: const { formatMessage } = intl || {}; or guard against undefined intl.
| const { formatMessage } = intl; | |
| const { formatMessage } = intl || { formatMessage: (msg) => (msg && msg.defaultMessage) || '' }; |
| const { intl } = this.props; | ||
| const { formatMessage } = intl; |
There was a problem hiding this comment.
Potential runtime error if intl prop is undefined during SSR. Consider adding a null check: const { formatMessage } = intl || {}; or guard against undefined intl.
| const { intl } = this.props; | ||
| const { formatMessage } = intl; |
There was a problem hiding this comment.
Potential runtime error if intl prop is undefined during SSR. Consider adding a null check: const { formatMessage } = intl || {}; or guard against undefined intl.
|
|
||
| const { formatMessage } = this.props.intl; | ||
| const { intl } = this.props; | ||
| const { formatMessage } = intl; |
There was a problem hiding this comment.
Potential runtime error if intl prop is undefined during SSR. Consider adding a null check: const { formatMessage } = intl || {}; or guard against undefined intl.
| const { formatMessage } = intl; | |
| const { formatMessage } = intl || {}; |
- Apply consistent driver registration pattern across all drivers - Force re-register drivers to ensure fresh configuration in tests - Remove unnecessary empty includePaths from sass-loader options - Let Shakapacker handle default sass include paths These changes improve code consistency and prevent potential issues with overriding Shakapacker defaults. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
✅ Deployment complete for PR #662, commit c3ed3ac |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| sassOptions: { | ||
| includePaths: [] | ||
| } | ||
| api: 'modern' |
There was a problem hiding this comment.
The removal of sassOptions: { includePaths: [] } may break Sass imports that rely on custom include paths. Consider verifying that all existing Sass imports continue to work without this configuration.
| const { intl } = this.props; | ||
| const { formatMessage } = intl; |
There was a problem hiding this comment.
Potential null reference error if intl is undefined during SSR. Add a null check: const { formatMessage } = intl || {}; or provide a fallback to prevent runtime errors.
| # Force re-register to ensure our configuration is used | ||
| Capybara.drivers.delete(:selenium_chrome) |
There was a problem hiding this comment.
[nitpick] The comment mentions 'Force re-register' but doesn't explain why this is necessary. Consider adding more context about what specific configuration issues this addresses.
Pull Request Review - Shakapacker 9.0.0-beta.7 MigrationOverall AssessmentThis is a well-executed migration PR with good commit history, comprehensive testing notes, and clear documentation. The changes are focused and address the core requirements for upgrading to Shakapacker 9.0.0-beta.7. StrengthsCode Quality
Testing
Documentation
Code Review Issues1. Potential Null Reference Issue - CommentForm.jsx:104-105, 160-161, 216-217The intermediate intl variable adds an unnecessary indirection. While functionally equivalent to the previous code, this could potentially mask issues if intl is undefined. Severity: Low - The change does not introduce bugs, but questionable whether the refactor adds value. 2. Incomplete CSS Loader Validation - config/webpack/commonWebpackConfig.js:63-71The CSS loader configuration fix does not validate whether modules itself is enabled before checking namedExport. Severity: Low - Current code likely works but could be more defensive. 3. Deprecated Headless Flag - spec/support/driver_registration.rb:36Good fix! The migration from the old headless argument to --headless is correct. This implementation is appropriate for modern Chrome versions. 4. Driver Re-registration Pattern - spec/support/driver_registration.rb:5-6, 19, 32Force re-registering drivers by deleting them first could potentially cause issues in parallel test execution. Severity: Medium - Could cause test flakiness in certain scenarios. Performance ConsiderationsPositive Changes
The CSS loader configuration changes should have negligible performance impact. Security ConsiderationsNo Security Issues Found:
Minor Note: Using beta.7 in production should be carefully considered, but the PR description indicates this is part 1 of 3, which is appropriate. Test CoverageCoverage Assessment: Good Existing Coverage:
For This PR:
Gap: No specific tests added for CSS modules namedExport behavior with camelCaseOnly convention. Suggestions for Improvement
Final VerdictRecommendation: APPROVE with minor suggestions This is a solid migration PR that:
The issues identified are minor and do not block merging. Great work on the methodical migration approach! Review performed with Claude Code |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx:1
- Empty JSX expression
{}should be removed as it serves no purpose and clutters the code.
/* eslint-disable react/no-find-dom-node, react/no-string-refs */
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
The original pattern `const { formatMessage } = this.props.intl;` was already correct and more concise. Reverting to the simpler one-line destructuring pattern while keeping the empty braces removal.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
|
✅ Deployment complete for PR #662, commit d6fa4d4 |
Replace verbose defensive coding with cleaner optional chaining and simpler array find pattern. The fix only applies if namedExport is enabled, which is not currently the case but may be in future Shakapacker versions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Pull Request Review - PR #662OverviewThis PR migrates from Shakapacker 8.2.0 to 9.0.0-beta.7, configures Babel as the JavaScript transpiler, and fixes CSS modules and react-intl SSR compatibility. The PR is well-structured as the first step in a three-part migration plan. ✅ Strengths
🔍 Code Quality & Best Practices✅ Good Practices
|
|
✅ Deployment complete for PR #662, commit da0c7f5 |
PR Review: Migrate to Shakapacker 9.0.0-beta.7 with Babel transpilerOverall AssessmentThis is a well-structured migration PR with clear objectives and good documentation. The changes are incremental, well-tested, and follow a logical progression through multiple commits. The PR successfully addresses the migration path from Shakapacker 8.2.0 to 9.0.0-beta.7. ✅ Strengths
🔍 Code Quality Observationsconfig/webpack/commonWebpackConfig.js (Lines 54-63)Potential Issue: The CSS loader fix may silently fail if the structure changes. const cssLoader = scssRule.use.find(loader => {
const loaderName = typeof loader === 'string' ? loader : loader?.loader;
return loaderName?.includes('css-loader');
});
if (cssLoader?.options?.modules?.namedExport) {
cssLoader.options.modules.exportLocalsConvention = 'camelCaseOnly';
}Recommendation: Add defensive checks or logging to ensure the fix is actually applied: const cssLoader = scssRule.use.find(loader => {
const loaderName = typeof loader === 'string' ? loader : loader?.loader;
return loaderName?.includes('css-loader');
});
if (cssLoader?.options?.modules?.namedExport) {
cssLoader.options.modules.exportLocalsConvention = 'camelCaseOnly';
} else if (cssLoader && !cssLoader.options?.modules) {
// CSS loader found but modules not configured - might be expected
console.debug('CSS loader found without modules configuration');
}Alternatively, consider if this configuration should be in a more centralized location or if Shakapacker 9.x has a built-in way to handle this. spec/support/driver_registration.rb (Lines 4-6, 18-19, 31-32)Concern: Force re-registering drivers by deleting them could cause issues in parallel test execution. def self.register_selenium_chrome
# Force re-register to ensure our configuration is used
Capybara.drivers.delete(:selenium_chrome)Questions:
Recommendation: Add a comment explaining why this is necessary, or consider using a check-and-register pattern: def self.register_selenium_chrome
# Only register if not already configured to avoid redundant registrations
return if Capybara.drivers[:selenium_chrome] && !ENV['FORCE_DRIVER_REREGISTER']
Capybara.register_driver :selenium_chrome do |app|
Capybara::Selenium::Driver.new(app, browser: :chrome)
end
# ... rest of code
endspec/support/driver_registration.rb (Line 36)Chrome Headless Flag: Using browser_options.args << "--headless"Note: The PR description mentions updating to Recommendation: Consider using browser_options.args << "--headless=new"🔒 Security Considerations
⚡ Performance Considerations
🧪 Test CoverageStrengths:
Recommendations:
🐛 Potential Bugs
📚 Documentation & MaintenanceSuggestions:
🎯 Action ItemsBefore Merge:
Future PRs (as already planned):
✨ SummaryThis is a solid migration PR with thoughtful problem-solving and good testing. The main concerns are:
Recommendation: Approve with minor suggestions. The issues raised are not blockers but would improve code robustness and maintainability. Great work on the migration! 🚀 Review conducted with attention to code quality, best practices, security, performance, and test coverage. |
|
✅ Review app for PR #662 was successfully deleted |
Summary
Context
This is the first of three planned PRs to migrate to Shakapacker 9.x:
Changes
Gemfileandpackage.jsonto use Shakapacker 9.0.0-beta.7javascript_transpiler: babelconfiguration toconfig/shakapacker.ymlconfig/webpack/commonWebpackConfig.js:namedExportby default for CSS modulesexportLocalsConvention: 'camelCaseOnly'whennamedExportis enabledidanddefaultMessagepropertiesintlcontext during SSRformatMessagecallsTesting
Next Steps
After this PR is merged:
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
This change is
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Tests