Liquid SDK: All tests passing (84/84 unit, 125/138 cross-SDK)#3
Open
joalves wants to merge 13 commits into
Open
Liquid SDK: All tests passing (84/84 unit, 125/138 cross-SDK)#3joalves wants to merge 13 commits into
joalves wants to merge 13 commits into
Conversation
- Improved absmartly_treatment tag with better error handling - Enhanced filter implementations for treatment, peek, track, variables - Extended Drop class with custom field support and better state management - Updated gemspec and dependencies
- Added spec/support directory with shared contexts and helpers - Updated spec_helper with improved configuration
WalkthroughThis PR adds project metadata and documentation (CHANGELOG.md, LICENSE, SECURITY.md, README.md), updates .gitignore and Gemfile entries, and changes the gemspec file list to a static set. It hardens Shopify theme examples to use server-rendered JSON and safer DOM handling. The core library gains a Logging module, logger and strict_mode accessors, enhanced Drop validations and error handling, a with_ready_context helper in Filters, and safer Tag implementations. A substantial RSpec test suite and test helpers were added to cover logging, strict_mode, validation, and event publishing. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Remove accidentally committed .claude/ directory and audit report files. Fix liquid filter implementations.
…in absmartly_track filter
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
spec/support/test_data.rb (1)
1-138: Consider extracting experiment data to a separate fixture file.The module exceeds 100 lines (RuboCop Metrics/ModuleLength). For test data that rarely changes, moving to a YAML/JSON fixture file could improve maintainability:
def build_experiment_data YAML.load_file(File.expand_path('fixtures/experiment_data.yml', __dir__)) endThis is optional given this is test infrastructure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/support/test_data.rb` around lines 1 - 138, The TestDataHelpers module contains a large build_experiment_data method causing a Metrics/ModuleLength offense; extract the static experiment payload into an external fixture (YAML or JSON) and have build_experiment_data load and return that fixture instead. Update TestDataHelpers#build_experiment_data to read the fixture (e.g., YAML.load_file or JSON.parse) and return the parsed data, keeping the module lightweight and ensuring tests still call build_experiment_data for compatibility.lib/absmartly/liquid/tags.rb (1)
33-34: Use consistent error types for strict mode exceptions.Line 33 raises a plain
RuntimeErrorwith a string, whilst line 13 raises::Liquid::SyntaxError. Consider using a consistent error class for strict mode violations.♻️ Proposed fix
- raise 'ABsmartly context not available or not ready' if ABsmartly::Liquid.strict_mode + raise ::Liquid::Error, 'ABsmartly context not available or not ready' if ABsmartly::Liquid.strict_mode🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/absmartly/liquid/tags.rb` around lines 33 - 34, Replace the plain RuntimeError raised when strict mode is enabled with the same Liquid error type used elsewhere: change the `raise 'ABsmartly context not available or not ready' if ABsmartly::Liquid.strict_mode` to raise a `::Liquid::SyntaxError` (preserving the message), so strict-mode violations consistently use `::Liquid::SyntaxError` (reference symbols: ABsmartly::Liquid.strict_mode and ::Liquid::SyntaxError).lib/absmartly/liquid/drop.rb (1)
9-11: Consider callingsuperfor defensive coding against future Liquid changes.
Liquid::Drop#initializecurrently only sets@context = nil, so omittingsuperhas no functional impact today. However, callingsuperis common practice in Liquid examples and protects against future versions that may add additional setup logic.♻️ Suggested improvement
def initialize(absmartly_context) + super() `@absmartly_context` = absmartly_context end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/absmartly/liquid/drop.rb` around lines 9 - 11, The initialize method in the Drop class should call the parent initializer to preserve any setup Liquid::Drop may perform in future versions; update the initialize(absmartly_context) in the class that inherits Liquid::Drop to invoke super (e.g., call super with no args or appropriate args) before or after assigning `@absmartly_context` so the parent's initialization logic runs alongside setting `@absmartly_context`.lib/absmartly/liquid/filters.rb (1)
57-66: Inconsistency betweenabsmartly_trackfilter andTrackTagbehaviour.The filter allows tracking without checking
context.ready?, butTrackTag(lines 66-77 intags.rb) requires the context to be ready:# tags.rb - requires ready if absmartly && absmartly.respond_to?(:ready?) && absmartly.ready? absmartly.track(goal_name, properties)While the PR objectives mention this is intentional ("allow track() to run before context is ready"), users may find it confusing that
{% absmartly_track %}and{{ 'goal' | absmartly_track }}behave differently regarding context readiness.Additionally, when context is missing (line 59), no warning is logged, unlike other methods that use
with_ready_context. Consider adding a warning for consistency:♻️ Suggested improvement
def absmartly_track(goal_name, properties = {}) context = get_absmartly_context - return '' unless context + unless context + log_warning("ABsmartly context missing, event '#{goal_name}' dropped") + raise 'ABsmartly context not available' if ABsmartly::Liquid.strict_mode + return '' + end context.track(goal_name, properties) '' rescue StandardError => e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/absmartly/liquid/filters.rb` around lines 57 - 66, The absmartly_track filter currently calls get_absmartly_context and tracks without verifying context.ready?, causing inconsistent behavior with TrackTag; modify absmartly_track (and its use of get_absmartly_context) to mirror TrackTag by checking context.respond_to?(:ready?) && context.ready? before calling context.track(goal_name, properties), and when get_absmartly_context returns nil log a warning similar to with_ready_context; preserve the existing rescue behavior and respect ABsmartly::Liquid.strict_mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/shopify-theme/snippets/absmartly-init.liquid`:
- Around line 22-36: The captured Liquid variable config_data is already a JSON
string and is being JSON-encoded again when assigned to window.absmartlyConfig,
producing a string instead of an object; fix this in the snippet that sets
window.absmartlyConfig by removing the final | json filter (i.e., assign
window.absmartlyConfig = {{ config_data | strip }}), or alternatively keep the
filter but convert the value into an object in the client by calling JSON.parse,
and ensure you reference the config_data capture and the global
window.absmartlyConfig assignment when making the change.
In `@examples/shopify-theme/templates/product.liquid`:
- Around line 82-87: The call to window.absmartly.track can throw if the SDK
isn't loaded; update the add_to_cart handler to guard the call by checking
window.absmartly exists and that window.absmartly.track is a function before
invoking it (use a conditional around the existing track call that still
computes product_id/variant_id/price/quantity as before), and optionally
fallback to a no-op or local logging if the SDK is unavailable to avoid runtime
TypeError; refer to window.absmartly and its track method in the product
add_to_cart block where product.id, variantInput, product.price, and
quantityInput are used.
In `@lib/absmartly/liquid.rb`:
- Around line 14-16: The documentation and code disagree about the presence of
the global fallback accessor current_context (attr_accessor :current_context) —
either remove the accessor and all code/tests that assert its presence (update
any tests that reference current_context and remove the attr_accessor line) and
update SECURITY.md to reflect its removal, or keep the accessor and update
SECURITY.md to state it remains but is discouraged/unsafe and update tests to
reflect the intended behavior; target the attr_accessor declaration for
current_context and the tests that assert its existence when making the change.
In `@lib/absmartly/liquid/tags.rb`:
- Around line 87-90: The current parsing in properties =
`@properties_markup.scan`(/([\w\-\.]+):\s*([^,]+)/).to_h incorrectly splits values
containing commas; update the parser to split on commas that are not inside
quotes (or use Ruby's CSV parsing with a quote char) so keys/values like data:
'a,b' remain intact, then continue to call context.evaluate(value.strip) for
each value; locate the `@properties_markup` usage and replace the simple regex
scan with a split/parse that respects quoted strings (or fall back to
documenting the comma limitation in the syntax error if you deliberately keep
the simpler parser).
In `@SECURITY.md`:
- Around line 56-68: The example double-encodes JSON: you build JSON into the
Liquid capture named config_data (using shop.name and session_id) and then apply
| strip | json again when assigning var config, resulting in a JSON string
rather than an object; fix by either (A) removing the outer | json filter so var
config = {{ config_data | strip }} outputs a JS object literal, or (B) keep the
outer | json but parse it on the client with JSON.parse (e.g., var config =
JSON.parse({{ config_data | strip | json }})), updating the assignment that
references config_data/strip/json accordingly.
---
Nitpick comments:
In `@lib/absmartly/liquid/drop.rb`:
- Around line 9-11: The initialize method in the Drop class should call the
parent initializer to preserve any setup Liquid::Drop may perform in future
versions; update the initialize(absmartly_context) in the class that inherits
Liquid::Drop to invoke super (e.g., call super with no args or appropriate args)
before or after assigning `@absmartly_context` so the parent's initialization
logic runs alongside setting `@absmartly_context`.
In `@lib/absmartly/liquid/filters.rb`:
- Around line 57-66: The absmartly_track filter currently calls
get_absmartly_context and tracks without verifying context.ready?, causing
inconsistent behavior with TrackTag; modify absmartly_track (and its use of
get_absmartly_context) to mirror TrackTag by checking
context.respond_to?(:ready?) && context.ready? before calling
context.track(goal_name, properties), and when get_absmartly_context returns nil
log a warning similar to with_ready_context; preserve the existing rescue
behavior and respect ABsmartly::Liquid.strict_mode.
In `@lib/absmartly/liquid/tags.rb`:
- Around line 33-34: Replace the plain RuntimeError raised when strict mode is
enabled with the same Liquid error type used elsewhere: change the `raise
'ABsmartly context not available or not ready' if ABsmartly::Liquid.strict_mode`
to raise a `::Liquid::SyntaxError` (preserving the message), so strict-mode
violations consistently use `::Liquid::SyntaxError` (reference symbols:
ABsmartly::Liquid.strict_mode and ::Liquid::SyntaxError).
In `@spec/support/test_data.rb`:
- Around line 1-138: The TestDataHelpers module contains a large
build_experiment_data method causing a Metrics/ModuleLength offense; extract the
static experiment payload into an external fixture (YAML or JSON) and have
build_experiment_data load and return that fixture instead. Update
TestDataHelpers#build_experiment_data to read the fixture (e.g., YAML.load_file
or JSON.parse) and return the parsed data, keeping the module lightweight and
ensuring tests still call build_experiment_data for compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f87d1fe-1a51-4176-92b2-f13ed94404e0
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.gitignoreCHANGELOG.mdGemfileLICENSEREADME.mdSECURITY.mdexamples/shopify-theme/snippets/absmartly-init.liquidexamples/shopify-theme/snippets/absmartly-tracking.liquidexamples/shopify-theme/templates/cart.liquidexamples/shopify-theme/templates/product.liquidlib/absmartly/liquid.rblib/absmartly/liquid/drop.rblib/absmartly/liquid/filters.rblib/absmartly/liquid/logging.rblib/absmartly/liquid/tags.rbliquid-sdk.gemspecspec/fixes_spec.rbspec/spec_helper.rbspec/support/test_data.rb
💤 Files with no reviewable changes (1)
- Gemfile
Comment on lines
+22
to
+36
| {% capture config_data %} | ||
| { | ||
| "endpoint": {{ settings.absmartly_endpoint | json }}, | ||
| "application": {{ shop.name | json }}, | ||
| "environment": {{ settings.absmartly_environment | default: 'production' | json }}, | ||
| "units": { | ||
| {% if session_id %}"session_id": {{ session_id | json }}{% if customer_id %},{% endif %}{% endif %} | ||
| {% if customer_id %}"customer_id": {{ customer_id | json }}{% endif %} | ||
| } | ||
| } | ||
| {% endcapture %} | ||
|
|
||
| <script> | ||
| window.absmartlyConfig = { | ||
| endpoint: '{{ settings.absmartly_endpoint }}', | ||
| apiKey: '{{ settings.absmartly_api_key }}', | ||
| application: '{{ shop.name }}', | ||
| environment: '{{ settings.absmartly_environment | default: 'production' }}', | ||
| units: { | ||
| {% if session_id %}session_id: '{{ session_id }}',{% endif %} | ||
| {% if customer_id %}customer_id: {{ customer_id }}{% endif %} | ||
| } | ||
| }; | ||
| /* ABsmartly Client Configuration - Server-Side Rendered */ | ||
| window.absmartlyConfig = {{ config_data | strip | json }}; |
There was a problem hiding this comment.
Double JSON encoding produces a string instead of an object.
config_data is constructed as valid JSON. Applying | strip | json on line 36 will JSON-encode the JSON string, resulting in window.absmartlyConfig being assigned a string like "{\"endpoint\":...}" rather than a JavaScript object.
Remove the | json filter or parse on assignment:
🐛 Proposed fix
/* ABsmartly Client Configuration - Server-Side Rendered */
- window.absmartlyConfig = {{ config_data | strip | json }};
+ window.absmartlyConfig = {{ config_data | strip }};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/shopify-theme/snippets/absmartly-init.liquid` around lines 22 - 36,
The captured Liquid variable config_data is already a JSON string and is being
JSON-encoded again when assigned to window.absmartlyConfig, producing a string
instead of an object; fix this in the snippet that sets window.absmartlyConfig
by removing the final | json filter (i.e., assign window.absmartlyConfig = {{
config_data | strip }}), or alternatively keep the filter but convert the value
into an object in the client by calling JSON.parse, and ensure you reference the
config_data capture and the global window.absmartlyConfig assignment when making
the change.
Comment on lines
+82
to
+87
| window.absmartly.track('add_to_cart', { | ||
| product_id: {{ product.id | json }}, | ||
| variant_id: variantInput ? variantInput.value : null, | ||
| price: {{ product.price | divided_by: 100.0 | json }}, | ||
| quantity: quantityInput ? parseInt(quantityInput.value) || 1 : 1 | ||
| }); |
There was a problem hiding this comment.
Add null check for window.absmartly before calling track.
If the ABsmartly SDK fails to initialise or load, window.absmartly will be undefined, causing a TypeError when track is called.
🛡️ Proposed defensive fix
- window.absmartly.track('add_to_cart', {
- product_id: {{ product.id | json }},
- variant_id: variantInput ? variantInput.value : null,
- price: {{ product.price | divided_by: 100.0 | json }},
- quantity: quantityInput ? parseInt(quantityInput.value) || 1 : 1
- });
+ if (window.absmartly && typeof window.absmartly.track === 'function') {
+ window.absmartly.track('add_to_cart', {
+ product_id: {{ product.id | json }},
+ variant_id: variantInput ? variantInput.value : null,
+ price: {{ product.price | divided_by: 100.0 | json }},
+ quantity: quantityInput ? parseInt(quantityInput.value) || 1 : 1
+ });
+ }📝 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
| window.absmartly.track('add_to_cart', { | |
| product_id: {{ product.id | json }}, | |
| variant_id: variantInput ? variantInput.value : null, | |
| price: {{ product.price | divided_by: 100.0 | json }}, | |
| quantity: quantityInput ? parseInt(quantityInput.value) || 1 : 1 | |
| }); | |
| if (window.absmartly && typeof window.absmartly.track === 'function') { | |
| window.absmartly.track('add_to_cart', { | |
| product_id: {{ product.id | json }}, | |
| variant_id: variantInput ? variantInput.value : null, | |
| price: {{ product.price | divided_by: 100.0 | json }}, | |
| quantity: quantityInput ? parseInt(quantityInput.value) || 1 : 1 | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/shopify-theme/templates/product.liquid` around lines 82 - 87, The
call to window.absmartly.track can throw if the SDK isn't loaded; update the
add_to_cart handler to guard the call by checking window.absmartly exists and
that window.absmartly.track is a function before invoking it (use a conditional
around the existing track call that still computes
product_id/variant_id/price/quantity as before), and optionally fallback to a
no-op or local logging if the SDK is unavailable to avoid runtime TypeError;
refer to window.absmartly and its track method in the product add_to_cart block
where product.id, variantInput, product.price, and quantityInput are used.
Comment on lines
+14
to
16
| attr_accessor :logger | ||
| attr_accessor :strict_mode | ||
| attr_accessor :current_context |
There was a problem hiding this comment.
Documentation inconsistency: current_context accessor retained but documented as removed.
The current_context accessor remains at line 16, but SECURITY.md (line 109) states: "The global fallback (ABsmartly::Liquid.current_context) has been removed as it was thread-unsafe." The tests in spec/fixes_spec.rb (lines 59-64) also verify this accessor exists.
Please either:
- Remove the accessor if it's truly deprecated (and update tests), or
- Update SECURITY.md to reflect the accessor remains but is discouraged.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/absmartly/liquid.rb` around lines 14 - 16, The documentation and code
disagree about the presence of the global fallback accessor current_context
(attr_accessor :current_context) — either remove the accessor and all code/tests
that assert its presence (update any tests that reference current_context and
remove the attr_accessor line) and update SECURITY.md to reflect its removal, or
keep the accessor and update SECURITY.md to state it remains but is
discouraged/unsafe and update tests to reflect the intended behavior; target the
attr_accessor declaration for current_context and the tests that assert its
existence when making the change.
Comment on lines
+87
to
90
| properties = @properties_markup.scan(/([\w\-\.]+):\s*([^,]+)/).to_h do |key, value| | ||
| evaluated = context.evaluate(value.strip) | ||
| [key, evaluated] | ||
| end |
There was a problem hiding this comment.
Property parsing fails for values containing commas.
The regex [^,]+ for capturing values will incorrectly split values containing commas. For example, {% absmartly_track 'goal', data: 'a,b' %} would parse data as 'a' only.
Consider using a more robust parser or documenting this limitation.
📝 Documented limitation example
Add to the syntax error message or documentation:
Note: Property values cannot contain commas. For complex values, use Liquid variables.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/absmartly/liquid/tags.rb` around lines 87 - 90, The current parsing in
properties = `@properties_markup.scan`(/([\w\-\.]+):\s*([^,]+)/).to_h incorrectly
splits values containing commas; update the parser to split on commas that are
not inside quotes (or use Ruby's CSV parsing with a quote char) so keys/values
like data: 'a,b' remain intact, then continue to call
context.evaluate(value.strip) for each value; locate the `@properties_markup`
usage and replace the simple regex scan with a split/parse that respects quoted
strings (or fall back to documenting the comma limitation in the syntax error if
you deliberately keep the simpler parser).
Comment on lines
+56
to
+68
| Or even better, build the entire object server-side: | ||
| ```liquid | ||
| {% capture config_data %} | ||
| { | ||
| "shopName": {{ shop.name | json }}, | ||
| "sessionId": {{ session_id | json }} | ||
| } | ||
| {% endcapture %} | ||
|
|
||
| <script> | ||
| var config = {{ config_data | strip | json }}; | ||
| </script> | ||
| ``` |
There was a problem hiding this comment.
Double JSON encoding in "even better" example.
The example at lines 58-67 builds valid JSON in config_data, then applies | strip | json on line 66. This double-encodes the JSON, producing a string like "{\"shopName\":...}" rather than a usable object.
Either parse the JSON on the client side, or remove the outer | json filter:
📝 Option 1: Assign as parsed JSON
<script>
- var config = {{ config_data | strip | json }};
+ var config = JSON.parse({{ config_data | strip | json }});
</script>📝 Option 2: Direct assignment without double encoding
<script>
- var config = {{ config_data | strip | json }};
+ var config = {{ config_data | strip }};
</script>📝 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
| Or even better, build the entire object server-side: | |
| ```liquid | |
| {% capture config_data %} | |
| { | |
| "shopName": {{ shop.name | json }}, | |
| "sessionId": {{ session_id | json }} | |
| } | |
| {% endcapture %} | |
| <script> | |
| var config = {{ config_data | strip | json }}; | |
| </script> | |
| ``` | |
| <script> | |
| var config = JSON.parse({{ config_data | strip | json }}); | |
| </script> |
Suggested change
| Or even better, build the entire object server-side: | |
| ```liquid | |
| {% capture config_data %} | |
| { | |
| "shopName": {{ shop.name | json }}, | |
| "sessionId": {{ session_id | json }} | |
| } | |
| {% endcapture %} | |
| <script> | |
| var config = {{ config_data | strip | json }}; | |
| </script> | |
| ``` | |
| <script> | |
| var config = {{ config_data | strip }}; | |
| </script> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SECURITY.md` around lines 56 - 68, The example double-encodes JSON: you build
JSON into the Liquid capture named config_data (using shop.name and session_id)
and then apply | strip | json again when assigning var config, resulting in a
JSON string rather than an object; fix by either (A) removing the outer | json
filter so var config = {{ config_data | strip }} outputs a JS object literal, or
(B) keep the outer | json but parse it on the client with JSON.parse (e.g., var
config = JSON.parse({{ config_data | strip | json }})), updating the assignment
that references config_data/strip/json accordingly.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/absmartly/liquid/filters.rb (2)
7-15: Minor: potential double-logging in strict mode.When
strict_modeis enabled and context is missing/not ready,with_ready_contextlogs a warning and raises, then this rescue block logs an error before re-raising. This results in both a warning and an error being logged for the same underlying issue.If this is intentional (warning for context state, error for which filter failed), consider adding a brief comment to clarify. Otherwise, you could skip the warning in
with_ready_contextwhenstrict_modeis true to avoid log noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/absmartly/liquid/filters.rb` around lines 7 - 15, The rescue in absmartly_treatment logs an error and re-raises when ABsmartly::Liquid.strict_mode is true, which duplicates a warning already emitted by with_ready_context; to fix, update with_ready_context (the method that emits the warning and raises when context is missing) to suppress the warning when ABsmartly::Liquid.strict_mode is true (so the only log is the error in absmartly_treatment), or alternatively add a clarifying comment in absmartly_treatment noting the intentional double-logging; refer to the methods absmartly_treatment, with_ready_context and the flag ABsmartly::Liquid.strict_mode when making the change.
70-86: Consider using custom exception classes instead of raising strings.Lines 75 and 81 raise plain strings, which creates generic
RuntimeErrorinstances. Using dedicated exception classes would improve error handling and debugging:
- Easier to distinguish ABsmartly-specific errors from other runtime errors
- Enables more granular rescue clauses if needed upstream
- Provides a clearer API contract
♻️ Proposed refactor
Define exception classes (e.g., in a separate file or at module level):
module ABsmartly module Liquid class ContextMissingError < StandardError; end class ContextNotReadyError < StandardError; end end endThen update the raises:
- raise 'ABsmartly context not available' if ABsmartly::Liquid.strict_mode + raise ABsmartly::Liquid::ContextMissingError, 'ABsmartly context not available' if ABsmartly::Liquid.strict_mode return fallback end unless context.ready? log_warning('ABsmartly context not ready, returning fallback value') - raise 'ABsmartly context not ready' if ABsmartly::Liquid.strict_mode + raise ABsmartly::Liquid::ContextNotReadyError, 'ABsmartly context not ready' if ABsmartly::Liquid.strict_mode return fallback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/absmartly/liquid/filters.rb` around lines 70 - 86, Replace the string raises in with_ready_context with dedicated exception classes: define ABsmartly::Liquid::ContextMissingError and ABsmartly::Liquid::ContextNotReadyError (e.g., at module level or in a new exceptions file) and change the two raise '...' lines to raise ContextMissingError and raise ContextNotReadyError respectively when ABsmartly::Liquid.strict_mode is true; keep existing logging and fallback behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/absmartly/liquid/filters.rb`:
- Around line 7-15: The rescue in absmartly_treatment logs an error and
re-raises when ABsmartly::Liquid.strict_mode is true, which duplicates a warning
already emitted by with_ready_context; to fix, update with_ready_context (the
method that emits the warning and raises when context is missing) to suppress
the warning when ABsmartly::Liquid.strict_mode is true (so the only log is the
error in absmartly_treatment), or alternatively add a clarifying comment in
absmartly_treatment noting the intentional double-logging; refer to the methods
absmartly_treatment, with_ready_context and the flag
ABsmartly::Liquid.strict_mode when making the change.
- Around line 70-86: Replace the string raises in with_ready_context with
dedicated exception classes: define ABsmartly::Liquid::ContextMissingError and
ABsmartly::Liquid::ContextNotReadyError (e.g., at module level or in a new
exceptions file) and change the two raise '...' lines to raise
ContextMissingError and raise ContextNotReadyError respectively when
ABsmartly::Liquid.strict_mode is true; keep existing logging and fallback
behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81740f82-4dd7-4b3c-be82-9b18491bd4a2
📒 Files selected for processing (1)
lib/absmartly/liquid/filters.rb
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
Test Results
Changes
lib/absmartly/liquid.rb- Improved registration and initializationlib/absmartly/liquid/drop.rb- Extended Drop class with custom field supportlib/absmartly/liquid/filters.rb- Enhanced filter implementationslib/absmartly/liquid/tags.rb- Better error handling in treatment/track tagsliquid-sdk.gemspec- Updated dependenciesspec/support/test_data.rb- New shared test data helpersspec/spec_helper.rb- Improved test configurationexamples/- Updated Shopify theme examplesLICENSE,CHANGELOG.md,SECURITY.mdTest Command
bundle exec rspecSummary by CodeRabbit
Documentation
New Features
Bug Fixes
Tests
Chores