Upgrade ruby #2038
Upgrade ruby #2038
Conversation
📝 WalkthroughWalkthroughUpgrade Ruby/Rails and container images; migrate Webpacker → Shakapacker; switch Chrome → Chromium; add lazy, container-aware Ferrum PDF initialization and test mocks; add AWS S3 checksum config; CI asset precompile; add SolidQueue recurring tasks schema and queue configs; numerous enum/model, initializer, script, and test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RailsApp as Rails App
participant PdfSvc as PdfGeneration::InvoiceService
participant Ferrum as FerrumPdf (lazy init)
participant Browser as Chromium
Client->>RailsApp: request PDF (download/send)
RailsApp->>PdfSvc: PdfGeneration::InvoiceService.new(...).process
PdfSvc->>Ferrum: request render (calls FerrumPdf.browser)
alt browser not initialized
Ferrum->>Ferrum: resolve browser_path (ENV CHROME_BIN / BROWSER_PATH / common paths)
Ferrum->>Browser: spawn Chromium (headless, options)
end
Ferrum->>Browser: render page -> produce PDF bytes
Browser-->>Ferrum: return PDF bytes
Ferrum-->>PdfSvc: PDF bytes
PdfSvc-->>RailsApp: PDF bytes
RailsApp-->>Client: respond with PDF
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/initializers/ferrum_pdf.rb (1)
32-49: Browser path mismatch will break PDF generation in local Docker containers.The
browser_pathis hardcoded to/usr/bin/google-chrome-stable, butDockerfile.localinstalls Chromium at/usr/bin/chromiumand setsCHROME_BIN=/usr/bin/chromium. WhenDOCKER_CONTAINERis set in local development, FerrumPdf will fail to find the browser.Consider using
ENV["CHROME_BIN"]to allow the browser path to be configured per environment:🐛 Proposed fix
# Configure browser for production and containerized environments if Rails.env.production? || ENV["DOCKER_CONTAINER"] + chrome_path = ENV.fetch("CHROME_BIN", "/usr/bin/google-chrome-stable") FerrumPdf.browser = Ferrum::Browser.new( headless: true, # Run browser in headless mode (no GUI) timeout: 60, # Maximum time to wait for browser operations process_timeout: 60, # Maximum time to wait for browser process to start - browser_path: "/usr/bin/google-chrome-stable", # Path to Chrome binary + browser_path: chrome_path, # Path to Chrome/Chromium binary browser_options: {
🤖 Fix all issues with AI agents
In @.devcontainer/Dockerfile:
- Around line 2-3: Update the Docker ARG default from the patch-level value to a
supported tag: change ARG VARIANT from "3.4.8" to "3.4" (or to a specific
variant like "3.4-bookworm"/"3.4-bullseye"/"3.4-trixie") so the FROM
mcr.microsoft.com/vscode/devcontainers/ruby:${VARIANT} line can successfully
pull the image; modify the ARG VARIANT declaration accordingly.
In `@deployment/fly/Dockerfile`:
- Around line 2-4: The Dockerfile references a non-published base image via ARG
RUBY_VERSION=3.4.8 and FROM
quay.io/evl.ms/fullstaq-ruby:${RUBY_VERSION}-${VARIANT}; change the RUBY_VERSION
default to a published release (e.g., 3.4.7) or otherwise pin the FROM image to
an available tag so the build succeeds; update the ARG RUBY_VERSION value and
ensure any CI/build configs that override it use the published tag as well.
In `@Gemfile`:
- Line 82: The Gemfile bump to aws-sdk-s3 ">= 1.208.0" introduces automatic
checksum behavior that may break current S3 usage; either pin or constrain the
gem to a pre-1.208.x version or explicitly set SDK checksum settings in your S3
client initialization (e.g., in whatever code constructs Aws::S3::Client or
Aws::S3::Resource) by configuring request_checksum_calculation and
response_checksum_validation to WHEN_REQUIRED; update the Gemfile requirement or
add the client config in the S3 initializer (or code that calls
Aws::S3::Client.new / Aws::S3::Resource.new) accordingly and run S3 integration
tests.
🧹 Nitpick comments (2)
.devcontainer/Dockerfile (1)
53-53: Consider updating the pinned Bundler version.Bundler 2.3.11 is from 2022. Ruby 3.4 ships with Bundler 2.5+, so you may want to update this to a more recent version for better compatibility and performance.
Suggested update
-RUN gem install bundler -v 2.3.11 +RUN gem install bundler -v 2.5.23Dockerfile.test (1)
31-31: Same Bundler version consideration as in the devcontainer Dockerfile.Consider updating to a more recent Bundler version for consistency with Ruby 3.4.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.devcontainer/Dockerfile.ruby-versionDockerfile.localDockerfile.testGemfileconfig/initializers/ferrum_pdf.rbdeployment/fly/Dockerfiledocker-compose.ymldocs/docs/contributing-guide/setup/macos.md
⏰ 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). (1)
- GitHub Check: Running tests
🔇 Additional comments (9)
docker-compose.yml (2)
32-32: Environment variable addition for container detection looks good.This enables the FerrumPdf browser initialization in containerized environments, which is necessary for PDF generation functionality.
54-54: Consistent with the app service configuration.Gemfile (2)
6-6: Ruby version declaration is consistent with.ruby-versionand Dockerfiles.
142-144: Required gem additions for Ruby 3.4 compatibility.Both
csvandobserverwere extracted from Ruby's standard library in Ruby 3.4 and now require explicit gem declarations. These additions are necessary for the upgrade..ruby-version (1)
1-1: Ruby version update is consistent across all configuration files.The version 3.4.8 is correctly specified in
.ruby-version,Gemfile(ruby "3.4.8"),.devcontainer/Dockerfile(ARG VARIANT=3.4.8), and bothDockerfile.testandDockerfile.local(FROM ruby:3.4.8-slim). Ruby 3.4.8 is a stable release.Dockerfile.test (1)
2-2: Base image update is consistent and appropriate.The
ruby:3.4.8-slimimage is available on Docker Hub as an official Ruby release. Bundler version 2.3.11 is properly pinned on line 31, ensuring consistent dependency resolution.docs/docs/contributing-guide/setup/macos.md (1)
22-26: LGTM!The documentation correctly reflects the Ruby version update. Using
$(cat .ruby-version)for the install command is a good practice as it ensures the installed version stays in sync with the project's.ruby-versionfile.Dockerfile.local (2)
2-2: LGTM!Ruby version update to 3.4.8-slim is consistent with the PR objective.
25-28: Good switch to Chromium, but ensure integration with ferrum_pdf.rb.The switch from Google Chrome to Chromium is a good choice for local development—it's lighter and easier to install in Debian-based containers. The
CHROME_BINenvironment variable is correctly set.However, as noted in the
ferrum_pdf.rbreview, the initializer doesn't currently use this environment variable, which will cause a path mismatch. Once that fix is applied, this configuration will work correctly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Upgrade httparty to 0.24.0 (fixes CVE-2025-68696 SSRF vulnerability) - Upgrade shakapacker to 9.5.0 (fixes environment variable leak) - Update .devcontainer/Dockerfile to use ARG VARIANT=3.4 for image compatibility - Update deployment/fly/Dockerfile to use correct fullstaq-ruby tag format (3.4.8-jemalloc-slim) - Add AWS SDK checksum configuration for aws-sdk-s3 >= 1.208.0 compatibility
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@config/initializers/aws.rb`:
- Around line 1-14: The CHANGELOG link in the comment is pointing to the wrong
version; update the comment above Aws.config.update to reference the AWS SDK S3
CHANGELOG entry for version 1.208.0 (the release that introduced the breaking
changes mentioned) by replacing the current URL with the correct changelog
anchor for v1.208.0 and adjusting the date text to 2025-12-16 so the comment
accurately cites the relevant release.
In `@Gemfile`:
- Line 57: Update the JavaScript shakapacker dependency to match the Gemfile by
changing "shakapacker" in package.json to "^9.5.0" (or a compatible ^9.x range),
then run yarn install (or npm install) to update yarn.lock/package-lock.json;
ensure the updated package resolves to v9.x so config/webpack/webpack.config.js
and config/webpack/base.js use the matching v9 API.
♻️ Duplicate comments (1)
Gemfile (1)
82-82: AWS SDK S3 checksum configuration addressed.The version constraint aligns with the new
config/initializers/aws.rbthat configures checksum behavior toWHEN_REQUIREDfor backward compatibility.
🧹 Nitpick comments (1)
deployment/fly/Dockerfile (1)
2-4: Consider cleaner ARG naming for clarity.The current approach embeds
-jemallocinRUBY_VERSIONand-sliminVARIANT, which works but conflates version with build variant. A cleaner pattern would separate these concerns:-ARG RUBY_VERSION=3.4.8-jemalloc -ARG VARIANT=slim +ARG RUBY_VERSION=3.4.8 +ARG VARIANT=jemalloc-slimThis keeps
RUBY_VERSIONas a pure version number andVARIANTas the complete build variant, improving readability and making future version bumps more intuitive.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.devcontainer/DockerfileGemfileconfig/initializers/aws.rbdeployment/fly/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (1)
- .devcontainer/Dockerfile
⏰ 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). (1)
- GitHub Check: Running tests
🔇 Additional comments (3)
Gemfile (3)
6-6: Ruby 3.4.8 upgrade looks good.The Ruby version bump aligns with updates across the repository (
.ruby-version, Dockerfiles). Ensure CI pipelines pass with the new version.
140-140: HTTParty upgrade addresses CVE-2025-68696 (SSRF).Good security practice to pin the minimum version that includes the fix.
142-143: Required additions for Ruby 3.4.x compatibility.In Ruby 3.4.0,
csvandobserverwere removed from the default gems and became bundled gems. Adding them explicitly is necessary for the Ruby upgrade to work correctly.The web search confirms the additions are correct.
csvwas loaded from the standard library, but will no longer be part of the default gems since Ruby 3.4.0. Add csv to your Gemfile or gemspec. The observer gem was gemified in Ruby 3.4, 3.3, and 3.2. These explicit additions are necessary for Ruby 3.4.8 compatibility.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Update package.json shakapacker dependency to ^9.5.0 - Create config/shakapacker.yml for v9 compatibility - Update yarn.lock with new shakapacker version - Fix AWS initializer changelog reference to v1.208.0
React-rails 2.6.2 is incompatible with connection_pool 3.x due to API changes. Pin connection_pool to ~> 2.4 to resolve ArgumentError.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 79: Remove all legacy webpacker artifacts and regenerate Shakapacker v9
binaries: delete or replace the old bin scripts named "bin/webpacker" and
"bin/webpacker-dev-server" and ensure the new "bin/shakapacker" and
"bin/shakapacker-dev-server" are present (regenerate by running rails
shakapacker:install). Update any references to the old binaries in
docker-compose.yml and docker-compose.test.yml to call "bin/shakapacker" and
update Procfile.dev to use "bin/shakapacker-dev-server". Remove the obsolete
configuration file "config/webpacker.yml" since it is replaced by
"config/shakapacker.yml". Finally, run the installer and verify the generated
bin scripts are executable and referenced correctly across your compose files
and Procfile.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
config/initializers/aws.rbconfig/shakapacker.ymlpackage.json
🚧 Files skipped from review as they are similar to previous changes (1)
- config/initializers/aws.rb
⏰ 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). (1)
- GitHub Check: Running tests
🔇 Additional comments (4)
config/shakapacker.yml (4)
3-25: LGTM!The default configuration section follows Shakapacker v9 conventions with appropriate settings for paths, caching, and versioning. The
ensure_consistent_versioning: trueis a good practice to catch gem/npm version mismatches early.
48-52: Permissive dev server settings are acceptable for local development.The
allowed_hosts: "all"combined with the wildcard CORS header is permissive, but this is a common pattern for local development environments. The inline comment appropriately warns about DNS rebinding risks. These settings are isolated to the development environment and don't affect production.
57-62: LGTM!Separating test packs to
packs-testis a good practice to isolate test builds from development builds.
64-71: LGTM!Production configuration correctly disables runtime compilation (relying on precompiled assets) and enables manifest caching for performance.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Remove deprecated cache_path and webpack_compile_output options - Add required javascript_transpiler: babel configuration - Keep all other v9-compatible options (compiler_strategy, useContentHash, etc.)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Gemfile`:
- Line 6: You bumped Ruby to "3.4.8" in the Gemfile; verify all gem dependencies
and native extensions are compatible with Ruby 3.4 by running the full test
suite and bundler checks, update or pin incompatible gems (adjust Gemfile
entries and version constraints for gems with known 3.4 issues), rebuild any
C-extensions and run bundle install/build to catch compilation errors, and add a
CI job to test against ruby-3.4.8 (and optionally earlier supported rubies) so
regressions are caught automatically.
♻️ Duplicate comments (1)
Gemfile (1)
85-85: Acknowledged - checksum handling addressed.The aws-sdk-s3 >= 1.208.0 upgrade and its checksum behavior change have been addressed with the AWS initializer configuration.
🧹 Nitpick comments (1)
Gemfile (1)
57-57: Consider using a tighter version constraint.The
>= 9.5.0constraint allows any future major version (e.g., 10.x, 11.x), which could introduce breaking changes. Consider using~> 9.5to restrict to 9.x versions while still receiving patch updates.🔧 Suggested constraint
-gem "shakapacker", ">= 9.5.0" +gem "shakapacker", "~> 9.5"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Gemfileconfig/shakapacker.yml
⏰ 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). (1)
- GitHub Check: Running tests
🔇 Additional comments (6)
Gemfile (2)
62-63: LGTM!Good practice to pin
connection_poolto 2.x with a clear comment explaining the react-rails 2.6.2 compatibility requirement.
143-146: Good addition of extracted stdlib gems for Ruby 3.4 compatibility.The
csvandobservergems were extracted from Ruby's standard library into bundled gems starting in Ruby 3.4, requiring explicit declaration in the Gemfile. This is necessary for the Ruby 3.4.8 upgrade.For
httparty, consider using a tighter constraint (e.g.,~> 0.24) similar to the suggestion for shakapacker.config/shakapacker.yml (4)
3-27: Configuration aligns with Shakapacker v9 requirements.The deprecated legacy keys (
cache_path,webpack_compile_output) have been removed, and the requiredjavascript_transpiler: babelis properly configured.Note:
source_entry_path: /means entry points are directly inapp/javascript/rather than a subdirectory. Verify this matches your webpack entry point structure.
29-57: Development configuration looks appropriate.Good choices for development:
compiler_strategy: mtimeanduseContentHash: falseimprove rebuild speed. Theallowed_hosts: "all"is acceptable for local development, and the inline comment correctly warns about DNS rebinding risks.
59-64: LGTM!Compiling test packs to a separate
packs-testdirectory prevents conflicts with development assets.
66-73: LGTM!Production configuration correctly disables runtime compilation and enables manifest caching for performance.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Remove old bin/webpacker and bin/webpacker-dev-server binaries - Create new bin/shakapacker and bin/shakapacker-dev-server binaries - Remove config/webpacker.yml (replaced by config/shakapacker.yml) - Update docker-compose.yml to use bin/shakapacker-dev-server - Update docker-compose.test.yml to use bin/shakapacker-dev-server - Update Procfile.dev to use bin/shakapacker-dev-server This completes the migration from webpacker to shakapacker v9.5.0
Apply fix from PR #2028 - when net_days is 0, show only hours instead of '0 days X hours' format.
Shakapacker requires assets to be precompiled before running tests. Add assets:precompile step after yarn install to generate manifest.json and compile webpack bundles for the test environment.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
spec/services/timeoff_entries/index_service_spec.rb (1)
201-224: This doesn’t guarantee thenet_days.zero?formatting is actually tested.Right now the expectation adapts to whatever
net_dayscomputes to; ifnet_daysnever becomes0in this fixture, the “hours-only” branch is never exercised and the change isn’t validated.Also, consider extracting a helper for label formatting to avoid repeating string rules across examples (and to apply consistently to the other leave types).
Proposed spec helper + usage
@@ private + + def expected_balance_label(net_days, extra_hours) + net_days.zero? ? "#{extra_hours} hours" : "#{net_days} days #{extra_hours} hours" + end @@ summary_object = { @@ - label: net_days.zero? ? "#{extra_hours} hours" : "#{net_days} days #{extra_hours} hours", + label: expected_balance_label(net_days, extra_hours), type: "leave" }docker-compose.test.yml (1)
59-71: Remove unusedWEBPACKER_DEV_SERVER_HOSTenv var or make dev server host configurable.The
WEBPACKER_DEV_SERVER_HOSTenvironment variable in docker-compose.test.yml is ignored by Shakapacker. The dev server host is hardcoded tolocalhostinconfig/shakapacker.yml(line 39), which means the dev server only binds to127.0.0.1inside the container. This will cause connectivity issues in Docker—other services cannot reachlocalhost, and the dev server won't be accessible from the host machine.Either remove the unused
WEBPACKER_DEV_SERVER_HOSTvariable, or modifyconfig/shakapacker.ymlto make thehostsetting configurable via an environment variable so it can be set to0.0.0.0in Docker.
🤖 Fix all issues with AI agents
In `@Procfile.dev`:
- Line 2: Rename the Procfile.dev process from "webpacker" to "webpack" so it
matches the docker-compose service names; keep the command unchanged
(bin/shakapacker-dev-server) and update any references or scripts that expect
the Procfile.dev process name "webpacker" to use "webpack" instead to maintain
consistency across docker-compose.yml, docker-compose.test.yml, and
.devcontainer/docker-compose.yml.
🧹 Nitpick comments (2)
spec/services/timeoff_entries/index_service_spec.rb (1)
226-249: Keep label expectations consistent across leave types and add a forcednet_days == 0case.Same issue as above: unless you force a scenario where
net_days == 0, this won’t catch regressions in the new formatting. Also, only month/week current-year cases use the new formatting logic—if the production code applies the rule generally, the quarter/year examples (and/or the previous-year block) should use the same helper/expectation pattern.bin/shakapacker-dev-server (1)
6-9: Consider simplifyingBUNDLE_GEMFILEpath base (optional).
CurrentFile.expand_path("../../Gemfile", Pathname.new(__FILE__).realpath)should work, but you can simplify and avoidPathnameentirely by basing off__dir__to reduce indirection.Proposed simplification
require "pathname" -ENV["BUNDLE_GEMFILE"] ||= File.expand_path("../../Gemfile", - Pathname.new(__FILE__).realpath) +ENV["BUNDLE_GEMFILE"] ||= File.expand_path("../Gemfile", __dir__)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.github/workflows/validations.ymlProcfile.devbin/shakapackerbin/shakapacker-dev-serverdocker-compose.test.ymldocker-compose.ymlspec/services/timeoff_entries/index_service_spec.rb
⏰ 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). (1)
- GitHub Check: Running tests
🔇 Additional comments (7)
docker-compose.yml (2)
39-55: Shakapacker dev-server command swap OK; double-check env var compatibility.
Same note as test compose: ensureWEBPACKER_DEV_SERVER_HOSTis still honored by your Shakapacker setup (or rename accordingly).
26-33:DOCKER_CONTAINER=trueis actively consumed. It's used inconfig/initializers/ferrum_pdf.rbto conditionally configure Ferrum for PDF generation in Docker environments. No action needed.bin/shakapacker (1)
3-18: Looks correct; consider defaultingNODE_ENVfromRAILS_ENV.
This avoids accidental “RAILS_ENV=production but NODE_ENV=development” runs.Optional tweak
ENV["RAILS_ENV"] ||= ENV["RACK_ENV"] || "development" -ENV["NODE_ENV"] ||= "development" +ENV["NODE_ENV"] ||= ENV["RAILS_ENV"].github/workflows/validations.yml (2)
76-81: No changes needed. The current setup usingbundle exec rails assets:precompileis the correct approach for Shakapacker 9.5.0 in Rails. The configuration is properly set up with a separatepacks-testdirectory for the test environment, andassets:precompileis the standard Rails task for compiling Shakapacker bundles.Likely an incorrect or invalid review comment.
76-81: Asset precompile requires SECRET_KEY_BASE; step order and Rails version matter.
Rails loads the app duringassets:precompile, which requiressecret_key_base. Since this step runs before "Setup Env's" (lines 85–88), it will fail if the app needs.envor encrypted credentials at boot.For Rails 7.1+, set
SECRET_KEY_BASE_DUMMY: "1"to auto-generate a temporary secret for the build step:- name: Compile webpack assets env: RAILS_ENV: test NODE_ENV: test + SECRET_KEY_BASE_DUMMY: "1" run: | bundle exec rails assets:precompileFor Rails 7.0 or earlier, this variable is unavailable—provide
SECRET_KEY_BASEdirectly or move the step after "Setup Env's" (if.envdefines it). If using encrypted credentials, supplyRAILS_MASTER_KEY.Also consider caching compiled assets (
public/packs*,tmp/shakapacker/tmp/webpacker) and Node dependencies by lockfile hash to reduce precompile time across workflow runs.bin/shakapacker-dev-server (2)
3-5:NODE_ENVdefault looks fine; minor whitespace is non-issue.
No functional concerns here;ENV["NODE_ENV"] ||= "development"matches typical dev-server expectations.
12-14: The require pathrequire "shakapacker/dev_server_runner"and API callShakapacker::DevServerRunner.run(ARGV)are correctly implemented for shakapacker v9.5.0.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
… 3.4 compatibility - Upgrade database_cleaner-active_record from 2.1.0 to 2.2.2 to fix NoMethodError with schema_migration in Rails 7.2 - Add test environment support to Ferrum PDF initializer to enable Chrome auto-detection during test runs - Fixes browser timeout errors in PDF generation tests
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config/initializers/ferrum_pdf.rb`:
- Around line 36-52: The Ferrum/Chrome path is hardcoded when DOCKER_CONTAINER
is set, causing FerrumPdf.browser (Ferrum::Browser.new) to fail in local Docker
where Chromium lives at /usr/bin/chromium; update the
DOCKER_CONTAINER/production branch to reuse the same auto-detection logic used
in the dev/test branch (or at minimum check for /usr/bin/chromium before falling
back to /usr/bin/google-chrome-stable) so Ferrum::Browser.new is initialized
with the correct browser_path in Docker; alternatively ensure Dockerfile.local
installs google-chrome-stable if you prefer the fixed path.
♻️ Duplicate comments (3)
Gemfile (3)
6-6: Ruby 3.4.8 compatibility previously addressed.The Ruby 3.4 compatibility concerns (frozen-string behavior,
itblock parameter, stdlib removals, C-extension compatibility) were flagged in a prior review and confirmed addressed.
57-57: Shakapacker version alignment previously addressed.The package.json/Gemfile version mismatch was flagged and confirmed resolved by the author.
85-85: aws-sdk-s3 checksum handling previously addressed.The breaking change in checksum behavior was flagged and confirmed resolved by the author with the AWS initializer configuration.
🧹 Nitpick comments (1)
config/initializers/ferrum_pdf.rb (1)
65-73: Silent failure if no browser is found could cause confusing runtime errors.When no browser path is detected,
FerrumPdf.browserremains unset, leading to obscure errors during PDF generation. Consider logging a warning to help developers diagnose the issue.♻️ Suggested improvement
# Configure browser if a valid Chrome/Chromium installation was found if browser_path FerrumPdf.browser = Ferrum::Browser.new( headless: true, # Run browser in headless mode timeout: 60, # Maximum time to wait for browser operations process_timeout: 60, # Maximum time to wait for browser process to start browser_path: # Use the auto-detected browser path ) + else + Rails.logger.warn( + "[FerrumPdf] No Chrome/Chromium installation found. " \ + "PDF generation will not work. Set BROWSER_PATH environment variable " \ + "or install Chrome/Chromium." + ) end
Shakapacker's nested_entries feature was enabled by default with source_entry_path set to '/', which would create webpack entry points for every file in app/javascript. This caused the build to fail in CI. Set nested_entries: false since the project uses a flat structure for entry points (application.js, modal.js, etc. at the root level). Fixes webpack compilation error: "Invalid Shakapacker configuration detected! You have set source_entry_path to '/' with nested_entries enabled."
- Skip browser initialization in test environment to avoid Chrome startup timeouts during Rails boot - Add PDF generation mocks to tests that trigger PDF creation: - invoice_mailer_spec.rb - send_reminder_mailer_spec.rb - invoices/download_spec.rb - invoices/send_invoice_spec.rb - reports/time_entry/download_spec.rb - bulk_invoice_download_service_spec.rb - Add spec/support/ferrum_pdf.rb for tests needing real PDF generation - Improve browser path detection for production/Docker environments
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spec/requests/internal_api/v1/reports/time_entry/download_spec.rb (1)
87-92: Pre-existing bug: Auth headers mismatch in book_keeper context tests.Lines 88 and 99 use
auth_headers(admin)instead ofauth_headers(book_keeper). The book_keeper context signs in asbook_keeper(line 72) but then authenticates requests with admin credentials, meaning these tests validate admin access rather than book_keeper authorization. The pattern contradicts the admin context (lines 43, 54) and employee context (line 65), which correctly match signed-in user with request auth headers.This is not introduced by this PR, but should be corrected to properly test book_keeper access control.
🧹 Nitpick comments (1)
spec/support/ferrum_pdf.rb (1)
9-9: Class variable access relies on internal implementation details.Accessing
@@browserdirectly viaclass_variable_defined?andclass_variable_getcouples this code to FerrumPdf's internal implementation. If FerrumPdf changes how it stores the browser instance, this will break silently.Consider wrapping this in a helper method or checking if FerrumPdf exposes a public API for this check.
♻️ Suggested approach
- next if FerrumPdf.class_variable_defined?(:@@browser) && FerrumPdf.class_variable_get(:@@browser).present? + next if FerrumPdf.respond_to?(:browser) && FerrumPdf.browser.present? + rescue Ferrum::Error + # Browser not initialized yet, continue with initializationNote: Verify that
FerrumPdf.browserdoesn't raise when uninitialized, or wrap appropriately.
Update base.js to use generateWebpackConfig() instead of webpackConfig export, which was removed in shakapacker 9.x. This fixes the CI error: "TypeError: Merging undefined is not supported"
This babel plugin is required in test environment (per babel.config.js) but was missing from package.json, causing CI webpack builds to fail.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@app/controllers/internal_api/v1/users/confirmations_controller.rb`:
- Around line 17-23: respond_with_error currently renders inside a loop causing
DoubleRenderError; change it to aggregate errors and render once (e.g., use
resource.errors.full_messages and render json: { errors:
resource.errors.full_messages } with a single status like :unprocessable_entity)
instead of calling render for each message; mirror the fix applied in
passwords_controller.rb and update the respond_with_error method accordingly.
In `@app/controllers/internal_api/v1/users/passwords_controller.rb`:
- Around line 31-37: The respond_with_error method currently renders inside a
loop causing DoubleRenderError when resource.errors.full_messages has multiple
items; instead consolidate and render once by returning all messages (e.g., use
resource.errors.full_messages as an array) with the existing status
(:unprocessable_content) so replace the per-message render loop in
respond_with_error with a single render that includes errors:
resource.errors.full_messages.
In `@db/migrate/20260120064206_add_solid_queue_recurring_tasks.rb`:
- Around line 6-8: The migration is unsafe because adding name with default ""
then creating a unique index on [:name, :supervisor_id] can fail for existing
rows; change the plan in add_column :solid_queue_processes, :name to add a
nullable column with no default, backfill unique values for each row (e.g.,
generate per-row names or use "#{id}"-based temporary names) for the existing
records in a reversible way, then add the unique index on [:name,
:supervisor_id], and finally change the column to NOT NULL and (if desired) set
the default to "" using change_column; update references to add_column/add_index
in the migration to follow that sequence and ensure the backfill and schema
changes are done within the migration.
In `@Gemfile`:
- Around line 6-10: Update the bundler pin in both Dockerfile.local and
.devcontainer/Dockerfile from 2.3.11 to 2.4.10 so the Docker images match
Gemfile.lock, and in .devcontainer/Dockerfile change the VARIANT value from the
loose "3.4" to the exact "3.4.8"; ensure the lines that install bundler (e.g.,
gem install bundler or any ENV BUNDLER_VERSION) reference 2.4.10 so bundle
install will succeed.
🧹 Nitpick comments (5)
config/database.yml (1)
5-5: LGTM! Pool size increase is consistent across environments.The increase from 5 to 10 connections is reasonable, especially with the addition of SolidQueue for background jobs. Ensure the PostgreSQL server's
max_connectionssetting can accommodate the increased pool across all application instances (e.g., web dynos, workers) to avoid connection exhaustion under load.Also applies to: 20-20
spec/requests/internal_api/v1/timeoff_entries/update_spec.rb (2)
49-49: Remove debug statement.This
putsstatement appears to be a leftover debug artifact and should be removed from the test.Suggested fix
}, headers: auth_headers(user) - puts json_response["errors"] - expect(response).to have_http_status(:unprocessable_content)
91-91: Remove debug statement.Same issue as above — this
putsstatement should be removed.Suggested fix
}, headers: auth_headers(user) - puts json_response["errors"] - expect(response).to have_http_status(:unprocessable_content)config/queue.yml (1)
1-17: Consider isolating test concurrency for determinism.
Using 5 worker threads intestcan make specs flaky. A test-specific override (threads: 1, slightly higher polling_interval) keeps behavior predictable.♻️ Suggested tweak for test env
test: <<: *default + workers: + - queues: "*" + threads: 1 + polling_interval: 0.5db/queue_schema.rb (1)
3-3: Align queue schema version with Rails 7.2 if applicable.
If the app is now on Rails 7.2, keepingActiveRecord::Schema[7.1]can emit schema version warnings.♻️ Possible adjustment
-ActiveRecord::Schema[7.1].define(version: 1) do +ActiveRecord::Schema[7.2].define(version: 1) do
…mismatches - Fix DoubleRenderError in confirmations_controller and passwords_controller by rendering all error messages in a single response instead of looping - Make solid_queue migration safe by adding nullable column first, backfilling unique values, then adding NOT NULL constraint and unique index - Update bundler version from 2.3.11 to 2.4.10 in Dockerfile.local and .devcontainer/Dockerfile to match Gemfile.lock - Pin Ruby version to exact 3.4.8 in .devcontainer/Dockerfile - Add defensive checks for Elasticsearch index existence in Client and Invoice - Add searchkick:reindex:all step to CI workflow before running tests
This commit addresses multiple issues encountered during the Ruby 3.4.8 upgrade: 1. **Webpack Configuration Updates** - Updated webpack base.js to use Shakapacker 9.x API (baseConfig instead of generateWebpackConfig) - Fixed webpack dev server port configuration (8080) - Added DefinePlugin override to prevent NODE_ENV conflict warnings - Disabled performance warnings in development 2. **Database Migration Safety** - Wrapped strong_migrations flagged operations in safety_assured blocks - Added disable_ddl_transaction! for concurrent index creation - Migration: AddSolidQueueRecurringTasks 3. **Gem Updates** - Added msgpack >= 1.7.0 (required for Rails 7.2+ with Ruby 3.4) - Updated web-console to >= 4.2.1 (Ruby 3.4 compatibility) - Updated searchkick to >= 5.3.0 (fixes compact_blank error)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/webpack/base.js (1)
6-23: Avoid defaulting NODE_ENV to development; gate performance hints to development only.If NODE_ENV is not explicitly set during
assets:precompile, defaulting to development will inline dev-only code paths (e.g.,if (process.env.NODE_ENV === 'development')) into production bundles, preventing minifiers from stripping them. Additionally,performance.hintsshould warn in development and error in production, not be disabled universally.Recommended NODE_ENV + environment-specific performance hints
+const nodeEnv = process.env.NODE_ENV || process.env.RAILS_ENV || "development"; + let customConfig = { plugins: [ new ForkTSCheckerWebpackPlugin(), new webpack.ProvidePlugin({ $: "jquery/src/jquery", jQuery: "jquery/src/jquery", }), // Override DefinePlugin to prevent NODE_ENV conflict new webpack.DefinePlugin({ - 'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV || 'development') - }), + "process.env.NODE_ENV": JSON.stringify(nodeEnv), + }), ], // Disable performance warnings in development - performance: { - hints: false, - maxEntrypointSize: 512000, - maxAssetSize: 512000, - }, + ...(nodeEnv === "development" + ? { + performance: { + hints: false, + maxEntrypointSize: 512000, + maxAssetSize: 512000, + }, + } + : {}), };
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.