Add missing files from spree core and fix CI#14
Open
damianlegawiec wants to merge 10 commits intomainfrom
Open
Add missing files from spree core and fix CI#14damianlegawiec wants to merge 10 commits intomainfrom
damianlegawiec wants to merge 10 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoves CI's check_skip gate and adds libvips installs; standardizes Gemfiles/gemspecs and adds spree_* gems; modernizes Rakefiles to Bundler/extension_rake; adds three Rails concerns and a screenshot_api_token; enables controller testing helpers in specs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
page_builder/spec/jobs/spree/themes/screenshot_job_spec.rb (1)
14-16:⚠️ Potential issue | 🟡 MinorTest never invokes
subject, so the expectation is not verified.This test sets up a message expectation but never calls
subject. The expectation will vacuously pass without actually verifying that the job skips the HTTP call when the token is nil.🧪 Proposed fix
it 'does not take a screenshot' do + subject expect(Net::HTTP).not_to receive(:get_response) endOr restructure to use
expect { subject }.not_to ...pattern:it 'does not take a screenshot' do - expect(Net::HTTP).not_to receive(:get_response) + expect(Net::HTTP).not_to receive(:get_response) + subject end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@page_builder/spec/jobs/spree/themes/screenshot_job_spec.rb` around lines 14 - 16, The test sets an expectation on Net::HTTP.get_response but never invokes subject, so it passes vacuously; fix by invoking the job in the example (call subject or perform the job) so the expectation is exercised — e.g. wrap the invocation as expect { subject }.not_to receive(:get_response) or set the expectation on Net::HTTP and then call subject (or described_class.perform_now/perform) to verify that Net::HTTP.get_response is not called when the token is nil; ensure you reference the existing subject in screenshot_job_spec.rb and the Net::HTTP.get_response expectation.
🧹 Nitpick comments (2)
page_builder/app/models/concerns/spree/has_one_link.rb (1)
10-17: Add trailing commas for consistency.Per RuboCop style guidelines, add trailing commas after the last items in multiline arrays to maintain consistency and cleaner diffs.
🧹 Proposed fix
def allowed_linkable_types [ [Spree.t(:page), 'Spree::Page'], [Spree.t(:product), 'Spree::Product'], [Spree.t(:post), 'Spree::Post'], [Spree.t(:taxon), 'Spree::Taxon'], [Spree.t(:policy), 'Spree::Policy'], - [Spree.t(:url), nil] + [Spree.t(:url), nil], ] end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@page_builder/app/models/concerns/spree/has_one_link.rb` around lines 10 - 17, The multiline array of link options (the array containing [Spree.t(:page), 'Spree::Page'] … [Spree.t(:url), nil]) is missing a trailing comma on the final element; update that array literal in has_one_link.rb to include a trailing comma after the last item to satisfy RuboCop and keep diffs clean (i.e., add a comma after [Spree.t(:url), nil]).page_builder/lib/spree/page_builder/engine.rb (1)
139-143: Remove redundant getter method.
mattr_accessor :screenshot_api_tokenalready generates both getter and setter methods. The explicitdef self.screenshot_api_tokendefinition on lines 141-143 is redundant and could cause confusion.♻️ Proposed fix
mattr_accessor :screenshot_api_token - - def self.screenshot_api_token - @@screenshot_api_token - end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@page_builder/lib/spree/page_builder/engine.rb` around lines 139 - 143, The explicit getter `def self.screenshot_api_token` duplicates the accessor already created by `mattr_accessor :screenshot_api_token`; remove the redundant `def self.screenshot_api_token ... end` method so the module uses the automatically generated getter/setter from `mattr_accessor` (leave `mattr_accessor :screenshot_api_token` intact and delete the manual method).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 117-118: In the GitHub Actions workflow step named "Install
libvips" update the apt package being installed from libvips to libvips-dev:
modify the run command that currently does "sudo apt-get update && sudo apt-get
install -y libvips" so it installs libvips-dev instead, ensuring the step name
remains the same and the rest of the command structure (update && install -y) is
preserved.
- Around line 61-62: Update the CI workflow steps named "Install libvips" in
both the page_builder and storefront jobs to install the development package
instead of the runtime-only one: replace the apt package name libvips with
libvips-dev in the run commands so gems like ruby-vips and image_processing can
compile against the headers.
In `@page_builder/app/models/concerns/spree/has_one_link.rb`:
- Around line 28-35: The `@default_links` instance variable is never assigned so
the `@default_links.presence` check in default_links is dead; either allow
overriding by setting `@default_links` elsewhere or simplify the method by
removing the presence check and always returning the array. Update the
default_links method (referencing default_links, `@default_links`,
Spree::PageLink, and theme_or_parent.pages.find_by) to either (A) initialize
`@default_links` with the array when nil or (B) drop the `@default_links.presence`
branch and return the literal array directly; if keeping the override pattern,
add a comment documenting that including classes should set `@default_links`.
In `@page_builder/Gemfile`:
- Around line 24-25: Reorder the two gem declarations in the Gemfile so that gem
'spree_dev_tools', '>= 0.6.0.rc1' appears before gem 'spree_multi_store',
github: 'spree/spree-multi-store', branch: 'main'; this resolves the
Bundler/OrderedGems warning by ensuring the gems are declared in the expected
alphabetical/order-required sequence.
- Around line 16-20: The Gemfile currently sets spree_opts to use a floating
branch reference (branch: 'main') for the Spree dependency which makes CI
nondeterministic; update spree_opts to reference an immutable ref instead
(replace branch: 'main' with ref: '<commit-sha-or-tag>' or use ref:
ENV.fetch('SPREE_REF', '<default-immutable-ref>') so CI can pin to a specific
commit or tag) while preserving the existing ENV['SPREE_PATH'] path branch logic
in the spree_opts assignment.
In `@page_builder/Rakefile`:
- Around line 9-14: The default Rake task uses a fragile relative
Dir.chdir('../../') after invoking :test_app; replace that with an absolute,
deterministic reset to the Rakefile’s directory (use __dir__ or
File.expand_path(File.dirname(__FILE__)) as the project root) so the working
directory is restored before calling Rake::Task[:spec].invoke; update the task
:default to compute the Rakefile directory and call Dir.chdir(project_root)
instead of the '../../' relative change.
In `@storefront/Gemfile`:
- Around line 25-26: The Gemfile has gems out of alphabetical order causing
Bundler/OrderedGems lint failures: move the gem declaration for spree_dev_tools
so it appears before spree_multi_store; specifically ensure "gem
'spree_dev_tools', '>= 0.6.0.rc1'" is listed above "gem 'spree_multi_store',
github: 'spree/spree-multi-store', branch: 'main'". Save the Gemfile and run the
lint (or bundle exec rubocop) to verify the ordering issue is resolved.
- Around line 16-20: The Gemfile uses a mutable git ref for the Spree dependency
in the spree_opts assignment (branch: 'main'), causing non-deterministic
installs; update the spree_opts git spec to accept and use a fixed ref (for
example from an env var like ENV['SPREE_REF'] or a CI-provided value) instead of
branch: 'main' so bundler resolves a specific commit SHA in CI, and ensure the
code falls back to the previous branch behavior only when no ref env var is
provided.
In `@storefront/Rakefile`:
- Around line 9-14: The default Rake task uses a brittle Dir.chdir('../../')
after invoking Rake::Task[:test_app], which can leave the process in the wrong
working directory; instead ensure the current directory is restored
deterministically by capturing the original Dir.pwd before invoking
Rake::Task[:test_app] and then returning to it (or use Dir.chdir with a block)
so that Rake::Task[:spec].invoke always runs from the intended directory; update
the task :default to remove the hardcoded relative chdir and use a safe restore
pattern around Rake::Task[:test_app].invoke.
- Around line 20-22: The Rake task invocation uses hard-coded class names when
calling Rake::Task['extension:test_app'].execute; update the call to pass the
configured Spree user classes instead by replacing the literal 'Spree::User' and
'Spree::AdminUser' arguments with Spree.user_class and Spree.admin_user_class so
the task respects the app's configured user/admin classes.
---
Outside diff comments:
In `@page_builder/spec/jobs/spree/themes/screenshot_job_spec.rb`:
- Around line 14-16: The test sets an expectation on Net::HTTP.get_response but
never invokes subject, so it passes vacuously; fix by invoking the job in the
example (call subject or perform the job) so the expectation is exercised — e.g.
wrap the invocation as expect { subject }.not_to receive(:get_response) or set
the expectation on Net::HTTP and then call subject (or
described_class.perform_now/perform) to verify that Net::HTTP.get_response is
not called when the token is nil; ensure you reference the existing subject in
screenshot_job_spec.rb and the Net::HTTP.get_response expectation.
---
Nitpick comments:
In `@page_builder/app/models/concerns/spree/has_one_link.rb`:
- Around line 10-17: The multiline array of link options (the array containing
[Spree.t(:page), 'Spree::Page'] … [Spree.t(:url), nil]) is missing a trailing
comma on the final element; update that array literal in has_one_link.rb to
include a trailing comma after the last item to satisfy RuboCop and keep diffs
clean (i.e., add a comma after [Spree.t(:url), nil]).
In `@page_builder/lib/spree/page_builder/engine.rb`:
- Around line 139-143: The explicit getter `def self.screenshot_api_token`
duplicates the accessor already created by `mattr_accessor
:screenshot_api_token`; remove the redundant `def self.screenshot_api_token ...
end` method so the module uses the automatically generated getter/setter from
`mattr_accessor` (leave `mattr_accessor :screenshot_api_token` intact and delete
the manual method).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/ci.ymlpage_builder/Gemfilepage_builder/Rakefilepage_builder/app/models/concerns/spree/has_one_link.rbpage_builder/app/models/concerns/spree/linkable.rbpage_builder/app/models/concerns/spree/previewable.rbpage_builder/lib/spree/page_builder/engine.rbpage_builder/spec/jobs/spree/themes/screenshot_job_spec.rbpage_builder/spec/spec_helper.rbpage_builder/spree_page_builder.gemspecstorefront/Gemfilestorefront/Rakefilestorefront/spec/spec_helper.rbstorefront/spree_storefront.gemspec
Comment on lines
+61
to
+62
| - name: Install libvips | ||
| run: sudo apt-get update && sudo apt-get install -y libvips |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check available libvips packages on Ubuntu/Debian
# Update package cache first
apt-get update -qq 2>/dev/null
# Check if 'libvips' package exists
echo "=== Checking for libvips package ==="
apt-cache search '^libvips$' 2>/dev/null || echo "Package 'libvips' not found in cache"
echo -e "\n=== All libvips-related packages ==="
apt-cache search libvips 2>/dev/null
echo -e "\n=== Package details for libvips-dev ==="
apt-cache show libvips-dev 2>/dev/null | head -20Repository: spree/spree-rails-storefront
Length of output: 224
🌐 Web query:
libvips Ubuntu package libvips-dev development headers
💡 Result:
On Ubuntu, the development headers for libvips are provided by the package libvips-dev (it “includes all development files needed to compile applications that use the vips library”). [1]
Install it with:
sudo apt update
sudo apt install libvips-devThis is also the package you want for C/C++ (and commonly Python build) development on Ubuntu. [2]
What you get (key dev artifacts):
- C header:
#include <vips/vips.h>[3] pkg-configmetadata:pkg-config --cflags --libs vips(thevips.pcfile is included inlibvips-dev). [4]
Related packages you may also want:
sudo apt install libvips # runtime library
sudo apt install libvips-tools # CLI tools like vips, vipsthumbnail[2][5]
Sources: [1] [2] [3] [4] [5]
🏁 Script executed:
#!/bin/bash
# Check the CI workflow file for libvips install commands
cat -n .github/workflows/ci.yml | grep -A 1 -B 1 "libvips"Repository: spree/spree-rails-storefront
Length of output: 467
Change libvips to libvips-dev in both install commands.
On Ubuntu, the package libvips contains only runtime libraries. Ruby gems like ruby-vips and image_processing require development headers for compilation, which are provided by the libvips-dev package. Both occurrences need this fix:
- Lines 61-62 (page_builder job)
- Lines 117-118 (storefront job)
🔧 Proposed fix
- name: Install libvips
- run: sudo apt-get update && sudo apt-get install -y libvips
+ run: sudo apt-get update && sudo apt-get install -y libvips-devApply this change to both lines 62 and 118.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Install libvips | |
| run: sudo apt-get update && sudo apt-get install -y libvips | |
| - name: Install libvips | |
| run: sudo apt-get update && sudo apt-get install -y libvips-dev |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 61 - 62, Update the CI workflow steps
named "Install libvips" in both the page_builder and storefront jobs to install
the development package instead of the runtime-only one: replace the apt package
name libvips with libvips-dev in the run commands so gems like ruby-vips and
image_processing can compile against the headers.
Comment on lines
+117
to
+118
| - name: Install libvips | ||
| run: sudo apt-get update && sudo apt-get install -y libvips |
Contributor
There was a problem hiding this comment.
Same libvips package name issue as above.
Apply the same fix here — use libvips-dev instead of libvips.
🔧 Proposed fix
- name: Install libvips
- run: sudo apt-get update && sudo apt-get install -y libvips
+ run: sudo apt-get update && sudo apt-get install -y libvips-dev📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Install libvips | |
| run: sudo apt-get update && sudo apt-get install -y libvips | |
| - name: Install libvips | |
| run: sudo apt-get update && sudo apt-get install -y libvips-dev |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 117 - 118, In the GitHub Actions
workflow step named "Install libvips" update the apt package being installed
from libvips to libvips-dev: modify the run command that currently does "sudo
apt-get update && sudo apt-get install -y libvips" so it installs libvips-dev
instead, ensuring the step name remains the same and the rest of the command
structure (update && install -y) is preserved.
Comment on lines
+28
to
+35
| def default_links | ||
| @default_links.presence || [ | ||
| Spree::PageLink.new( | ||
| label: Spree.t(:shop_all), | ||
| linkable: theme_or_parent.pages.find_by(type: 'Spree::Pages::ShopAll') | ||
| ) | ||
| ] | ||
| end |
Contributor
There was a problem hiding this comment.
@default_links instance variable is never set in this concern.
The @default_links.presence check on line 29 will always evaluate to nil since this instance variable is never assigned within the concern. This makes the .presence check dead code that always falls through to the array.
If this is intentional (allowing including classes to override by setting @default_links), consider documenting this pattern. Otherwise, simplify by removing the check.
🔧 Proposed fix if override pattern is not needed
def default_links
- `@default_links.presence` || [
+ [
Spree::PageLink.new(
label: Spree.t(:shop_all),
linkable: theme_or_parent.pages.find_by(type: 'Spree::Pages::ShopAll')
)
]
end🧰 Tools
🪛 RuboCop (1.84.2)
[convention] 33-33: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@page_builder/app/models/concerns/spree/has_one_link.rb` around lines 28 - 35,
The `@default_links` instance variable is never assigned so the
`@default_links.presence` check in default_links is dead; either allow overriding
by setting `@default_links` elsewhere or simplify the method by removing the
presence check and always returning the array. Update the default_links method
(referencing default_links, `@default_links`, Spree::PageLink, and
theme_or_parent.pages.find_by) to either (A) initialize `@default_links` with the
array when nil or (B) drop the `@default_links.presence` branch and return the
literal array directly; if keeping the override pattern, add a comment
documenting that including classes should set `@default_links`.
Comment on lines
+16
to
+20
| spree_opts = if ENV['SPREE_PATH'] | ||
| { path: ENV['SPREE_PATH'] } | ||
| else | ||
| { github: 'spree/spree', branch: 'main', glob: 'spree/**/*.gemspec' } | ||
| end |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n page_builder/Gemfile | head -30Repository: spree/spree-rails-storefront
Length of output: 979
🏁 Script executed:
# Check the exact lines mentioned
sed -n '16,26p' page_builder/GemfileRepository: spree/spree-rails-storefront
Length of output: 437
🏁 Script executed:
# Run the script from the review to find floating branches
echo "== Floating git branches in Gemfiles =="
rg -n "github: .*branch:" page_builder/Gemfile storefront/Gemfile 2>/dev/null || echo "No matches found"Repository: spree/spree-rails-storefront
Length of output: 523
🏁 Script executed:
# Check if these ENV variables are used anywhere in the project
echo "=== Searching for SPREE_REF usage ==="
rg -i "SPREE_REF|SPREE_PATH|SPREE_MULTI_STORE_REF" --type-list | head -20
rg "SPREE_REF" . 2>/dev/null || trueRepository: spree/spree-rails-storefront
Length of output: 516
🏁 Script executed:
# Check CI/build configuration to understand how this Gemfile is used
fd -t f "(\.github|\.circleci|\.travis|Jenkinsfile|\.gitlab-ci)" 2>/dev/null | head -10
git ls-files | grep -E "(github|circle|travis|jenkins|gitlab)" | head -10Repository: spree/spree-rails-storefront
Length of output: 96
🏁 Script executed:
cat -n .github/workflows/ci.ymlRepository: spree/spree-rails-storefront
Length of output: 4708
🏁 Script executed:
# Check for environment variables setup in the project
fd -t f "\.env" .
git ls-files | grep -E "(\.(env|yml|yaml|sh)$)" | head -20
rg "SPREE_PATH|SPREE_REF" .Repository: spree/spree-rails-storefront
Length of output: 439
🏁 Script executed:
# Check if there's any documentation about CI variables
fd -t f "README|CONTRIBUTING|\.md$" | xargs grep -l "SPREE\|ENV\|Gemfile" 2>/dev/null || trueRepository: spree/spree-rails-storefront
Length of output: 94
🏁 Script executed:
# Check if there's a Gemfile.lock to see what the actual resolved refs are
head -50 page_builder/Gemfile.lock | grep -A10 "spree/spree\|spree-multi-store" || trueRepository: spree/spree-rails-storefront
Length of output: 156
🏁 Script executed:
# Check Ruby/Bundler documentation behavior: does ENV.fetch without default crash?
# Let's also check if there are any setup/install instructions
cat page_builder/README.md | head -100Repository: spree/spree-rails-storefront
Length of output: 1171
Use immutable refs instead of branch: 'main' for CI predictability.
Lines 19 and 24 pull moving targets from GitHub. Floating branches cause dependency resolution drift over time and can reintroduce flaky or broken CI runs.
To fix this, pin to specific commits or tags instead of branches. For example, use a commit SHA:
{ github: 'spree/spree', ref: 'abc123def456...', glob: 'spree/**/*.gemspec' }Or use environment variables with explicit fallback values:
{ github: 'spree/spree', ref: ENV.fetch('SPREE_REF', 'main'), glob: 'spree/**/*.gemspec' }Ensure the CI workflow defines or documents these reference variables appropriately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@page_builder/Gemfile` around lines 16 - 20, The Gemfile currently sets
spree_opts to use a floating branch reference (branch: 'main') for the Spree
dependency which makes CI nondeterministic; update spree_opts to reference an
immutable ref instead (replace branch: 'main' with ref: '<commit-sha-or-tag>' or
use ref: ENV.fetch('SPREE_REF', '<default-immutable-ref>') so CI can pin to a
specific commit or tag) while preserving the existing ENV['SPREE_PATH'] path
branch logic in the spree_opts assignment.
Comment on lines
+24
to
+25
| gem 'spree_multi_store', github: 'spree/spree-multi-store', branch: 'main' | ||
| gem 'spree_dev_tools', '>= 0.6.0.rc1' |
Contributor
There was a problem hiding this comment.
Resolve Bundler/OrderedGems warning by reordering these declarations.
Place spree_dev_tools before spree_multi_store.
Suggested change
-gem 'spree_multi_store', github: 'spree/spree-multi-store', branch: 'main'
-gem 'spree_dev_tools', '>= 0.6.0.rc1'
+gem 'spree_dev_tools', '>= 0.6.0.rc1'
+gem 'spree_multi_store', github: 'spree/spree-multi-store', branch: 'main'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| gem 'spree_multi_store', github: 'spree/spree-multi-store', branch: 'main' | |
| gem 'spree_dev_tools', '>= 0.6.0.rc1' | |
| gem 'spree_dev_tools', '>= 0.6.0.rc1' | |
| gem 'spree_multi_store', github: 'spree/spree-multi-store', branch: 'main' |
🧰 Tools
🪛 RuboCop (1.84.2)
[convention] 25-25: Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem spree_dev_tools should appear before spree_multi_store.
(Bundler/OrderedGems)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@page_builder/Gemfile` around lines 24 - 25, Reorder the two gem declarations
in the Gemfile so that gem 'spree_dev_tools', '>= 0.6.0.rc1' appears before gem
'spree_multi_store', github: 'spree/spree-multi-store', branch: 'main'; this
resolves the Bundler/OrderedGems warning by ensuring the gems are declared in
the expected alphabetical/order-required sequence.
Comment on lines
+9
to
+14
| task :default do | ||
| if Dir['spec/dummy'].empty? | ||
| Rake::Task[:test_app].invoke | ||
| Dir.chdir('../../') | ||
| end | ||
| Rake::Task[:spec].invoke |
Contributor
There was a problem hiding this comment.
Replace relative chdir with a deterministic project-root reset.
On Line 12, Dir.chdir('../../') is fragile and tied to internal cwd side effects of :test_app. Use an absolute reset to this Rakefile’s directory before running specs.
Proposed fix
task :default do
+ project_root = File.expand_path(__dir__)
if Dir['spec/dummy'].empty?
Rake::Task[:test_app].invoke
- Dir.chdir('../../')
end
+ Dir.chdir(project_root)
Rake::Task[:spec].invoke
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| task :default do | |
| if Dir['spec/dummy'].empty? | |
| Rake::Task[:test_app].invoke | |
| Dir.chdir('../../') | |
| end | |
| Rake::Task[:spec].invoke | |
| task :default do | |
| project_root = File.expand_path(__dir__) | |
| if Dir['spec/dummy'].empty? | |
| Rake::Task[:test_app].invoke | |
| end | |
| Dir.chdir(project_root) | |
| Rake::Task[:spec].invoke | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@page_builder/Rakefile` around lines 9 - 14, The default Rake task uses a
fragile relative Dir.chdir('../../') after invoking :test_app; replace that with
an absolute, deterministic reset to the Rakefile’s directory (use __dir__ or
File.expand_path(File.dirname(__FILE__)) as the project root) so the working
directory is restored before calling Rake::Task[:spec].invoke; update the task
:default to compute the Rakefile directory and call Dir.chdir(project_root)
instead of the '../../' relative change.
Comment on lines
+9
to
+14
| task :default do | ||
| if Dir['spec/dummy'].empty? | ||
| Rake::Task[:test_app].invoke | ||
| Dir.chdir('../../') | ||
| end | ||
| Rake::Task[:spec].invoke |
Contributor
There was a problem hiding this comment.
Avoid brittle relative chdir in the default task.
On Line 12, Dir.chdir('../../') depends on where :test_app leaves the process. That can run specs from the wrong directory if cwd behavior changes.
Proposed fix
task :default do
+ project_root = File.expand_path(__dir__)
if Dir['spec/dummy'].empty?
Rake::Task[:test_app].invoke
- Dir.chdir('../../')
end
+ Dir.chdir(project_root)
Rake::Task[:spec].invoke
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| task :default do | |
| if Dir['spec/dummy'].empty? | |
| Rake::Task[:test_app].invoke | |
| Dir.chdir('../../') | |
| end | |
| Rake::Task[:spec].invoke | |
| task :default do | |
| project_root = File.expand_path(__dir__) | |
| if Dir['spec/dummy'].empty? | |
| Rake::Task[:test_app].invoke | |
| end | |
| Dir.chdir(project_root) | |
| Rake::Task[:spec].invoke | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@storefront/Rakefile` around lines 9 - 14, The default Rake task uses a
brittle Dir.chdir('../../') after invoking Rake::Task[:test_app], which can
leave the process in the wrong working directory; instead ensure the current
directory is restored deterministically by capturing the original Dir.pwd before
invoking Rake::Task[:test_app] and then returning to it (or use Dir.chdir with a
block) so that Rake::Task[:spec].invoke always runs from the intended directory;
update the task :default to remove the hardcoded relative chdir and use a safe
restore pattern around Rake::Task[:test_app].invoke.
Spree::Post was extracted from spree_core into the spree_posts gem. Both page_builder (post_decorator) and storefront need it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
storefront/Gemfile (2)
16-20:⚠️ Potential issue | 🟠 MajorPin git-based Spree dependencies to immutable refs for deterministic CI.
Using
branch: 'main'for git dependencies keeps bundle resolution mutable over time.Suggested change
spree_opts = if ENV['SPREE_PATH'] { path: ENV['SPREE_PATH'] } + elsif ENV['SPREE_REF'] + { github: 'spree/spree', ref: ENV.fetch('SPREE_REF'), glob: 'spree/**/*.gemspec' } else { github: 'spree/spree', branch: 'main', glob: 'spree/**/*.gemspec' } end @@ -gem 'spree_multi_store', github: 'spree/spree-multi-store', branch: 'main' -gem 'spree_posts', github: 'spree/spree-posts', branch: 'main' +gem 'spree_multi_store', github: 'spree/spree-multi-store', ref: ENV.fetch('SPREE_MULTI_STORE_REF', 'main') +gem 'spree_posts', github: 'spree/spree-posts', ref: ENV.fetch('SPREE_POSTS_REF', 'main')Also applies to: 25-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storefront/Gemfile` around lines 16 - 20, The git-based Spree dependency uses a mutable branch spec in spree_opts ({ github: 'spree/spree', branch: 'main', glob: 'spree/**/*.gemspec' }); change this to pin an immutable ref by replacing branch: 'main' with a commit SHA (e.g., ref: ENV.fetch('SPREE_REF', '<commit-sha>')) or ENV-driven ref, and apply the same change to the other git dependency occurrences (the similar entries around the later spree gem declarations) so CI/bundle resolution becomes deterministic; update spree_opts and the other git entries to use ref: instead of branch:.
25-27:⚠️ Potential issue | 🟡 MinorReorder
spree_*gem declarations to satisfy Bundler/OrderedGems.Suggested change
-gem 'spree_multi_store', github: 'spree/spree-multi-store', branch: 'main' -gem 'spree_posts', github: 'spree/spree-posts', branch: 'main' -gem 'spree_dev_tools', '>= 0.6.0.rc1' +gem 'spree_dev_tools', '>= 0.6.0.rc1' +gem 'spree_multi_store', github: 'spree/spree-multi-store', branch: 'main' +gem 'spree_posts', github: 'spree/spree-posts', branch: 'main'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storefront/Gemfile` around lines 25 - 27, The spree_* gem declarations are out of order and trigger Bundler/OrderedGems; reorder the three gem lines so they are alphabetically sorted by gem name (place gem 'spree_dev_tools', '>= 0.6.0.rc1' before gem 'spree_multi_store', github: 'spree/spree-multi-store', branch: 'main', and then gem 'spree_posts', github: 'spree/spree-posts', branch: 'main') to satisfy the linter.page_builder/Gemfile (2)
24-27:⚠️ Potential issue | 🟡 MinorReorder
spree_*gems to satisfy Bundler/OrderedGems.Suggested change
-gem 'spree_multi_store', github: 'spree/spree-multi-store', branch: 'main' -gem 'spree_posts', github: 'spree/spree-posts', branch: 'main' -gem 'spree_dev_tools', '>= 0.6.0.rc1' +gem 'spree_dev_tools', '>= 0.6.0.rc1' +gem 'spree_multi_store', github: 'spree/spree-multi-store', branch: 'main' +gem 'spree_posts', github: 'spree/spree-posts', branch: 'main'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@page_builder/Gemfile` around lines 24 - 27, Bundler/OrderedGems complaint: reorder the spree_* gem lines so they are alphabetized; update the Gemfile lines for spree_dev_tools, spree_multi_store, and spree_posts so the gems appear in alphabetical order (spree_dev_tools, spree_multi_store, spree_posts) while preserving versions/branches (keep github and branch entries and the '>= 0.6.0.rc1' constraint).
16-20:⚠️ Potential issue | 🟠 MajorPin git-based Spree dependencies to immutable refs for reproducible installs.
branch: 'main'is still used for git sources, so dependency resolution can drift between identical commits.Suggested change
spree_opts = if ENV['SPREE_PATH'] { path: ENV['SPREE_PATH'] } + elsif ENV['SPREE_REF'] + { github: 'spree/spree', ref: ENV.fetch('SPREE_REF'), glob: 'spree/**/*.gemspec' } else { github: 'spree/spree', branch: 'main', glob: 'spree/**/*.gemspec' } end gem 'spree', spree_opts gem 'spree_admin', spree_opts -gem 'spree_multi_store', github: 'spree/spree-multi-store', branch: 'main' -gem 'spree_posts', github: 'spree/spree-posts', branch: 'main' +gem 'spree_multi_store', github: 'spree/spree-multi-store', ref: ENV.fetch('SPREE_MULTI_STORE_REF', 'main') +gem 'spree_posts', github: 'spree/spree-posts', ref: ENV.fetch('SPREE_POSTS_REF', 'main')Also applies to: 24-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@page_builder/Gemfile` around lines 16 - 20, The Gemfile uses a mutable git branch for spree sources (spree_opts) which can cause non-reproducible installs; change the git source spec in spree_opts to pin to an immutable ref (use ref: '<commit-sha>' or a released tag) instead of branch: 'main' and apply the same change to the other git-based Spree entries referenced later (the other occurrences of spree_opts). Locate the spree_opts assignment and replace branch: 'main' with a ref value (or wire to an env var like ENV['SPREE_REF'] that contains a commit SHA or tag) so dependency resolution is deterministic.
🧹 Nitpick comments (2)
storefront/spree_storefront.gemspec (1)
26-29: Use consistent version constraints across Spree ecosystem dependencies.
spree_postsis currently unconstrained whilespreeandspree_page_builderare tied tospree_version. Aligning these reduces compatibility drift.Suggested change
spree_version = ">= #{s.version}" s.add_dependency 'spree', spree_version s.add_dependency 'spree_page_builder', spree_version - s.add_dependency 'spree_posts' + s.add_dependency 'spree_posts', spree_version @@ - s.add_development_dependency 'spree_dev_tools' + s.add_development_dependency 'spree_dev_tools', '>= 0.6.0.rc1'Also applies to: 43-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storefront/spree_storefront.gemspec` around lines 26 - 29, s.add_dependency 'spree_posts' is left unconstrained while spree and spree_page_builder use the spree_version variable, causing inconsistent version constraints; update the spree_posts dependency to use the same spree_version variable (spree_version) so all three dependencies use a consistent constraint, e.g. replace the unconstrained s.add_dependency 'spree_posts' with a call that uses spree_version to align its version constraint with s.add_dependency 'spree' and s.add_dependency 'spree_page_builder'.page_builder/spree_page_builder.gemspec (1)
26-31: Alignspree_posts(and dev tools) constraints with the rest of the Spree stack.
spreeandspree_adminare constrained viaspree_version, butspree_postsis unconstrained. Keeping these aligned reduces dependency skew risk.Suggested change
spree_version = ">= #{s.version}" s.add_dependency 'spree', spree_version s.add_dependency 'spree_admin', spree_version - s.add_dependency 'spree_posts' + s.add_dependency 'spree_posts', spree_version - s.add_development_dependency 'spree_dev_tools' + s.add_development_dependency 'spree_dev_tools', '>= 0.6.0.rc1'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@page_builder/spree_page_builder.gemspec` around lines 26 - 31, The gemspec currently applies the spree_version constraint to 'spree' and 'spree_admin' but not to 'spree_posts' or the dev dependency 'spree_dev_tools'; update the s.add_dependency call for 'spree_posts' and the s.add_development_dependency call for 'spree_dev_tools' to pass the existing spree_version variable (i.e., change s.add_dependency 'spree_posts' to s.add_dependency 'spree_posts', spree_version and s.add_development_dependency 'spree_dev_tools' to s.add_development_dependency 'spree_dev_tools', spree_version) so all Spree-related gems use the same constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@page_builder/Gemfile`:
- Around line 24-27: Bundler/OrderedGems complaint: reorder the spree_* gem
lines so they are alphabetized; update the Gemfile lines for spree_dev_tools,
spree_multi_store, and spree_posts so the gems appear in alphabetical order
(spree_dev_tools, spree_multi_store, spree_posts) while preserving
versions/branches (keep github and branch entries and the '>= 0.6.0.rc1'
constraint).
- Around line 16-20: The Gemfile uses a mutable git branch for spree sources
(spree_opts) which can cause non-reproducible installs; change the git source
spec in spree_opts to pin to an immutable ref (use ref: '<commit-sha>' or a
released tag) instead of branch: 'main' and apply the same change to the other
git-based Spree entries referenced later (the other occurrences of spree_opts).
Locate the spree_opts assignment and replace branch: 'main' with a ref value (or
wire to an env var like ENV['SPREE_REF'] that contains a commit SHA or tag) so
dependency resolution is deterministic.
In `@storefront/Gemfile`:
- Around line 16-20: The git-based Spree dependency uses a mutable branch spec
in spree_opts ({ github: 'spree/spree', branch: 'main', glob:
'spree/**/*.gemspec' }); change this to pin an immutable ref by replacing
branch: 'main' with a commit SHA (e.g., ref: ENV.fetch('SPREE_REF',
'<commit-sha>')) or ENV-driven ref, and apply the same change to the other git
dependency occurrences (the similar entries around the later spree gem
declarations) so CI/bundle resolution becomes deterministic; update spree_opts
and the other git entries to use ref: instead of branch:.
- Around line 25-27: The spree_* gem declarations are out of order and trigger
Bundler/OrderedGems; reorder the three gem lines so they are alphabetically
sorted by gem name (place gem 'spree_dev_tools', '>= 0.6.0.rc1' before gem
'spree_multi_store', github: 'spree/spree-multi-store', branch: 'main', and then
gem 'spree_posts', github: 'spree/spree-posts', branch: 'main') to satisfy the
linter.
---
Nitpick comments:
In `@page_builder/spree_page_builder.gemspec`:
- Around line 26-31: The gemspec currently applies the spree_version constraint
to 'spree' and 'spree_admin' but not to 'spree_posts' or the dev dependency
'spree_dev_tools'; update the s.add_dependency call for 'spree_posts' and the
s.add_development_dependency call for 'spree_dev_tools' to pass the existing
spree_version variable (i.e., change s.add_dependency 'spree_posts' to
s.add_dependency 'spree_posts', spree_version and s.add_development_dependency
'spree_dev_tools' to s.add_development_dependency 'spree_dev_tools',
spree_version) so all Spree-related gems use the same constraint.
In `@storefront/spree_storefront.gemspec`:
- Around line 26-29: s.add_dependency 'spree_posts' is left unconstrained while
spree and spree_page_builder use the spree_version variable, causing
inconsistent version constraints; update the spree_posts dependency to use the
same spree_version variable (spree_version) so all three dependencies use a
consistent constraint, e.g. replace the unconstrained s.add_dependency
'spree_posts' with a call that uses spree_version to align its version
constraint with s.add_dependency 'spree' and s.add_dependency
'spree_page_builder'.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
page_builder/Gemfilepage_builder/spree_page_builder.gemspecstorefront/Gemfilestorefront/spree_storefront.gemspec
Ensures spree_posts migrations are available in the dummy app. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
storefront/Rakefile (2)
20-22:⚠️ Potential issue | 🟠 MajorUse configured Spree user classes instead of hard-coded class names.
Lines 21-22 hard-code user classes and bypass app-level configuration.
Proposed fix
Rake::Task['extension:test_app'].execute( - user_class: 'Spree::User', - admin_user_class: 'Spree::AdminUser', + user_class: Spree.user_class.to_s, + admin_user_class: Spree.admin_user_class.to_s, authentication: 'devise',Based on learnings: Do not call
Spree::Userdirectly, useSpree.user_classinstead; and do not callSpree::AdminUserdirectly, useSpree.admin_user_classinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storefront/Rakefile` around lines 20 - 22, The task invocation currently hard-codes user class names in Rake::Task['extension:test_app'].execute by passing user_class: 'Spree::User' and admin_user_class: 'Spree::AdminUser'; change these to use the configured classes instead by passing the values from Spree.user_class and Spree.admin_user_class (i.e., replace the string literals with calls to Spree.user_class and Spree.admin_user_class) so the app-level configuration is respected.
9-14:⚠️ Potential issue | 🟠 MajorAvoid brittle relative
chdirbefore running specs.Line 12 uses
Dir.chdir('../../'), which is fragile and can leave the process in the wrong directory.Proposed fix
task :default do + project_root = File.expand_path(__dir__) if Dir['spec/dummy'].empty? Rake::Task[:test_app].invoke - Dir.chdir('../../') end + Dir.chdir(project_root) Rake::Task[:spec].invoke end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storefront/Rakefile` around lines 9 - 14, The task :default currently uses a brittle Dir.chdir('../../') after invoking Rake::Task[:test_app], which can leave the process in the wrong directory; change this to use a safe working-directory switch (either the block form Dir.chdir(target) do ... end or save Dir.pwd and restore it in an ensure) so the directory is reset after running Rake::Task[:test_app].invoke and before invoking Rake::Task[:spec].invoke; update the code around task :default to perform the needed actions inside the temporary chdir block or restore the original Dir.pwd in an ensure to avoid global side-effects from Dir.chdir('../../').page_builder/Rakefile (1)
9-14:⚠️ Potential issue | 🟠 MajorUse a deterministic cwd reset in the default task.
Line 12 uses a brittle relative
Dir.chdir('../../'). If:test_appchanges cwd behavior,:speccan run from the wrong directory.Proposed fix
task :default do + project_root = File.expand_path(__dir__) if Dir['spec/dummy'].empty? Rake::Task[:test_app].invoke - Dir.chdir('../../') end + Dir.chdir(project_root) Rake::Task[:spec].invoke end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@page_builder/Rakefile` around lines 9 - 14, The default Rake task uses a brittle relative Dir.chdir('../../') after invoking Rake::Task[:test_app]; capture the current working directory (e.g. original_dir = Dir.pwd) before invoking Rake::Task[:test_app] and ensure you restore it (Dir.chdir(original_dir)) in an ensure block so the working directory is deterministically reset before running Rake::Task[:spec]; update the default task code around Rake::Task[:test_app] and Dir.chdir to use this pattern so changes in :test_app cannot leave the process in the wrong cwd.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@page_builder/Rakefile`:
- Line 23: The system invocation on the Rake task (the call to system({
'BUNDLE_GEMFILE' => File.expand_path('Gemfile', __dir__) }, 'bundle exec rails g
spree_posts:install')) currently ignores its return value; change it to fail
fast by checking the boolean result and raising/aborting when it is false (use
the process exit status from $?.exitstatus or $?.to_s in the error message) so
the task stops immediately on generator failure and provides a clear error
message.
In `@storefront/Rakefile`:
- Line 29: The call to system({ 'BUNDLE_GEMFILE' => File.expand_path('Gemfile',
__dir__) }, 'bundle exec rails g spree_posts:install') in the Rakefile ignores
the command exit status; update the task to check the boolean return value of
system and abort/raise on failure (e.g., unless system(...); abort "Failed to
run spree_posts installer"; end) so setup failures stop the rake run and provide
a clear error message referencing the failing command.
---
Duplicate comments:
In `@page_builder/Rakefile`:
- Around line 9-14: The default Rake task uses a brittle relative
Dir.chdir('../../') after invoking Rake::Task[:test_app]; capture the current
working directory (e.g. original_dir = Dir.pwd) before invoking
Rake::Task[:test_app] and ensure you restore it (Dir.chdir(original_dir)) in an
ensure block so the working directory is deterministically reset before running
Rake::Task[:spec]; update the default task code around Rake::Task[:test_app] and
Dir.chdir to use this pattern so changes in :test_app cannot leave the process
in the wrong cwd.
In `@storefront/Rakefile`:
- Around line 20-22: The task invocation currently hard-codes user class names
in Rake::Task['extension:test_app'].execute by passing user_class: 'Spree::User'
and admin_user_class: 'Spree::AdminUser'; change these to use the configured
classes instead by passing the values from Spree.user_class and
Spree.admin_user_class (i.e., replace the string literals with calls to
Spree.user_class and Spree.admin_user_class) so the app-level configuration is
respected.
- Around line 9-14: The task :default currently uses a brittle
Dir.chdir('../../') after invoking Rake::Task[:test_app], which can leave the
process in the wrong directory; change this to use a safe working-directory
switch (either the block form Dir.chdir(target) do ... end or save Dir.pwd and
restore it in an ensure) so the directory is reset after running
Rake::Task[:test_app].invoke and before invoking Rake::Task[:spec].invoke;
update the code around task :default to perform the needed actions inside the
temporary chdir block or restore the original Dir.pwd in an ensure to avoid
global side-effects from Dir.chdir('../../').
| Rake::Task['extension:test_app'].execute( | ||
| install_admin: true | ||
| ) | ||
| system({ 'BUNDLE_GEMFILE' => File.expand_path('Gemfile', __dir__) }, 'bundle exec rails g spree_posts:install') |
Contributor
There was a problem hiding this comment.
Fail fast when the generator command fails.
Line 23 ignores the system return value. If the generator fails, the task should raise immediately to avoid partial setup.
Proposed fix
- system({ 'BUNDLE_GEMFILE' => File.expand_path('Gemfile', __dir__) }, 'bundle exec rails g spree_posts:install')
+ ok = system(
+ { 'BUNDLE_GEMFILE' => File.expand_path('Gemfile', __dir__) },
+ 'bundle', 'exec', 'rails', 'g', 'spree_posts:install'
+ )
+ raise 'Failed to run spree_posts:install generator' unless ok📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| system({ 'BUNDLE_GEMFILE' => File.expand_path('Gemfile', __dir__) }, 'bundle exec rails g spree_posts:install') | |
| ok = system( | |
| { 'BUNDLE_GEMFILE' => File.expand_path('Gemfile', __dir__) }, | |
| 'bundle', 'exec', 'rails', 'g', 'spree_posts:install' | |
| ) | |
| raise 'Failed to run spree_posts:install generator' unless ok |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@page_builder/Rakefile` at line 23, The system invocation on the Rake task
(the call to system({ 'BUNDLE_GEMFILE' => File.expand_path('Gemfile', __dir__)
}, 'bundle exec rails g spree_posts:install')) currently ignores its return
value; change it to fail fast by checking the boolean result and
raising/aborting when it is false (use the process exit status from
$?.exitstatus or $?.to_s in the error message) so the task stops immediately on
generator failure and provides a clear error message.
| css: true, | ||
| javascript: true | ||
| ) | ||
| system({ 'BUNDLE_GEMFILE' => File.expand_path('Gemfile', __dir__) }, 'bundle exec rails g spree_posts:install') |
Contributor
There was a problem hiding this comment.
Check and enforce generator command success.
Line 29 ignores the result of system(...); setup failures should abort the task.
Proposed fix
- system({ 'BUNDLE_GEMFILE' => File.expand_path('Gemfile', __dir__) }, 'bundle exec rails g spree_posts:install')
+ ok = system(
+ { 'BUNDLE_GEMFILE' => File.expand_path('Gemfile', __dir__) },
+ 'bundle', 'exec', 'rails', 'g', 'spree_posts:install'
+ )
+ raise 'Failed to run spree_posts:install generator' unless ok📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| system({ 'BUNDLE_GEMFILE' => File.expand_path('Gemfile', __dir__) }, 'bundle exec rails g spree_posts:install') | |
| ok = system( | |
| { 'BUNDLE_GEMFILE' => File.expand_path('Gemfile', __dir__) }, | |
| 'bundle', 'exec', 'rails', 'g', 'spree_posts:install' | |
| ) | |
| raise 'Failed to run spree_posts:install generator' unless ok |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@storefront/Rakefile` at line 29, The call to system({ 'BUNDLE_GEMFILE' =>
File.expand_path('Gemfile', __dir__) }, 'bundle exec rails g
spree_posts:install') in the Rakefile ignores the command exit status; update
the task to check the boolean return value of system and abort/raise on failure
(e.g., unless system(...); abort "Failed to run spree_posts installer"; end) so
setup failures stop the rake run and provide a clear error message referencing
the failing command.
Posts were extracted from spree_core into spree_posts gem, so factories need to be loaded separately. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add kaminari gem for pagination tests - Add missing i18n keys for posts (all_posts, all_posts_with_tag, blog) - Normalize i18n locale file - Add missing test requires: capybara/rspec, selenium-webdriver, timecop, rspec-activemodel-mocks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@storefront/spec/spec_helper.rb`:
- Around line 37-42: spec_helper.rb requires several test gems that are not
declared as development dependencies; update the spree_storefront.gemspec to add
add_development_dependency entries for capybara, selenium-webdriver,
database_cleaner (or database_cleaner-active_record if you prefer the namespaced
gem), ffaker, timecop, and rspec-activemodel-mocks so bundler can load them
during test runs; ensure each dependency includes an appropriate version
constraint (or leave unconstrained) and run bundle install to verify the
LoadError is resolved.
Comment on lines
+37
to
+42
| require 'capybara/rspec' | ||
| require 'selenium-webdriver' | ||
| require 'database_cleaner/active_record' | ||
| require 'ffaker' | ||
| require 'timecop' | ||
| require 'rspec-activemodel-mocks' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Dependency manifests =="
fd -HI 'Gemfile(\..*)?$|.*\.gemspec$|.*lock$'
echo
echo "== Gem declarations for new requires =="
rg -n --type ruby "gem ['\"](rails-controller-testing|capybara|selenium-webdriver|database-cleaner|ffaker|timecop|rspec-activemodel-mocks)['\"]"
echo
echo "== Require usage in specs =="
rg -n --type ruby "require ['\"](capybara/rspec|selenium-webdriver|database_cleaner/active_record|ffaker|timecop|rspec-activemodel-mocks)['\"]"Repository: spree/spree-rails-storefront
Length of output: 255
🏁 Script executed:
#!/bin/bash
echo "== Storefront Gemfile =="
cat -n storefront/Gemfile
echo
echo "== Storefront gemspec =="
cat -n storefront/spree_storefront.gemspec
echo
echo "== spec_helper.rb context (lines 35-65) =="
sed -n '35,65p' storefront/spec/spec_helper.rb | cat -nRepository: spree/spree-rails-storefront
Length of output: 4577
Add missing test gem dependencies to the storefront gemspec.
The require calls in lines 37-42 and line 59 of spec_helper.rb will raise LoadError at startup if these gems are missing from the storefront development dependency set. None of the following are currently declared in storefront/spree_storefront.gemspec:
- capybara
- selenium-webdriver
- database_cleaner
- ffaker
- timecop
- rspec-activemodel-mocks
These should be added as add_development_dependency entries in the gemspec.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@storefront/spec/spec_helper.rb` around lines 37 - 42, spec_helper.rb requires
several test gems that are not declared as development dependencies; update the
spree_storefront.gemspec to add add_development_dependency entries for capybara,
selenium-webdriver, database_cleaner (or database_cleaner-active_record if you
prefer the namespaced gem), ffaker, timecop, and rspec-activemodel-mocks so
bundler can load them during test runs; ensure each dependency includes an
appropriate version constraint (or leave unconstrained) and run bundle install
to verify the LoadError is resolved.
Add explicit waits for product cards to be present in the DOM before
asserting their order. The `within('.page-contents')` + `page.all` pattern
was racy because Turbo navigation could remove the old `.page-contents`
before the new one appeared. Using `have_css` with a count leverages
Capybara's built-in waiting to ensure the page is fully loaded.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The root cause: after visit and after Turbo sort navigation, the test interacted with the page before it was fully rendered. The progress bar check in wait_for_turbo returns immediately when Turbo responds fast enough to skip showing the bar, but the DOM swap isn't complete yet. Fixes: - Add explicit wait for 5 product cards in the before block to ensure the initial page is fully loaded before clicking sort - Extract sort_by helper to DRY up the sort-and-wait pattern - Wait for product cards after each sort to confirm navigation completed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The by_best_selling scope in Spree core uses correlated subqueries in ORDER BY, which is incompatible with SELECT DISTINCT on PostgreSQL (PG requires ORDER BY expressions to appear in the SELECT list). Override the scope via a decorator to use LEFT JOIN + column aliases instead, which works with DISTINCT on both PostgreSQL and MySQL. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The by_best_selling scope in Spree core uses correlated subqueries in ORDER BY, which is incompatible with SELECT DISTINCT on PostgreSQL (PG requires ORDER BY expressions to appear in the SELECT list). Override the scope via a decorator to use LEFT JOIN with a table alias (sps_best_selling) to avoid ambiguity with existing store_products joins. The aliased columns appear in SELECT, making it compatible with DISTINCT on PostgreSQL, MySQL, and SQLite. Also add decorator loading support to the storefront engine via config.to_prepare, matching the page_builder engine pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores
Bug Fixes