feat: add global cutom event logger #34
Conversation
WalkthroughThis pull request adds a class-level Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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
Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
calthejuggler
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
41-52: Config table key should matchevent_logger.The table lists
eventLogger, but Ruby config isevent_logger. This could confuse users.Suggested fix
-| eventLogger | `(context, eventName, data) => void` | ❌ | See "Using a Custom Event Logger" below | A callback function which runs after SDK events. | +| event_logger | `(context, eventName, data) => void` | ❌ | See "Using a Custom Event Logger" below | A callback function which runs after SDK events. |
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 59-87: The README's fenced Ruby code block triggers MD046; change
the block to the project's expected style or update the lint rule: either
convert the fenced ```ruby block (showing MyEventLogger, handle_event, and the
Absmartly.configure_client usage that sets config.event_logger) into the
preferred indented code block style, or update the markdownlint configuration to
allow fenced code blocks for Ruby so the example remains unchanged.
In `@spec/absmartly_spec.rb`:
- Around line 185-189: The test assigns an unused local `context` from the
helper `create_ready_context`; remove the unused local by invoking
`create_ready_context` without assignment (keep the preceding
`mock_logger.clear`), so the spec calls `create_ready_context` to trigger the
READY event and still asserts `mock_logger` received `handle_event` with
`ContextEventLogger::EVENT_TYPE::READY` and `data` once.
- Around line 170-182: The after hook currently only clears
Absmartly.event_logger causing global SDK fields to leak between examples;
update the cleanup to reset all mutated global config fields set in the before
block (endpoint, api_key, application, environment and event_logger). For
example, in the after block reset Absmartly.endpoint, Absmartly.api_key,
Absmartly.application, Absmartly.environment and Absmartly.event_logger (or call
Absmartly.configure_client and nil out those attributes) so the Absmartly global
state modified by Absmartly.configure_client is fully cleared.
- Around line 162-168: create_ready_context currently injects mock_logger
directly into Context.create, bypassing the global configuration path; change
the test to use the globally configured logger by wiring Absmartly.event_logger
(or calling Context.create so it picks up Absmartly.event_logger) instead of
mock_logger and ensure Absmartly.event_logger is configured before calling
create_ready_context; update references in the test so create_ready_context uses
Absmartly.event_logger rather than mock_logger to validate the full global
logger path (symbols: create_ready_context, Context.create, mock_logger,
Absmartly.event_logger).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mdlib/absmartly.rbspec/absmartly_spec.rb
🧰 Additional context used
🧬 Code graph analysis (1)
lib/absmartly.rb (1)
lib/a_b_smartly_config.rb (1)
context_event_logger(31-34)
🪛 markdownlint-cli2 (0.18.1)
README.md
59-59: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🪛 RuboCop (1.82.1)
spec/absmartly_spec.rb
[warning] 187-187: Useless assignment to variable - context.
(Lint/UselessAssignment)
🔇 Additional comments (4)
lib/absmartly.rb (2)
15-17: Public accessor forevent_loggerlooks solid.Fits the existing configuration surface neatly.
57-61: Propagation intoABSmartlyConfigis correctly wired.The new logger is now passed through to the SDK config as expected.
spec/absmartly_spec.rb (2)
16-35: Test helper is clear and focused.
MockContextEventLoggerProxyis a neat, minimal harness for capturing events.
77-110: Accessor tests look good.These cover the public surface and propagation into
sdk_config.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Line 51: The README entry for event_logger currently shows a JS-style
signature and should be updated to reflect the Ruby API: replace the `(context,
eventName, data) => void` signature with a description that event_logger expects
a ContextEventLogger object implementing handle_event(event, data), and update
the notes (e.g., the "Using a Custom Event Logger" section) to show usage via
ContextEventLogger#handle_event rather than a JS callback; reference symbol
names: event_logger, ContextEventLogger, handle_event.
♻️ Duplicate comments (3)
spec/absmartly_spec.rb (2)
78-80: Reset all global config mutated in this describe.Line 100–105 sets endpoint/api_key/application/environment, but the after hook only clears
event_logger, so global state can leak into later specs.♻️ Suggested cleanup
after do Absmartly.event_logger = nil + Absmartly.endpoint = nil + Absmartly.api_key = nil + Absmartly.application = nil + Absmartly.environment = nil end
162-168: Integration helper bypasses the global logger path.Line 166–167 injects
mock_loggerdirectly, so the “configured globally” path isn’t actually exercised.✅ Suggested adjustment
Context.create(clock, config, data_future_ready, data_provider, - event_handler, mock_logger, variable_parser, audience_matcher) + event_handler, Absmartly.event_logger, variable_parser, audience_matcher)README.md (1)
59-88: MD046: fenced block conflicts with configured markdownlint style.Line 59 uses a fenced block; markdownlint expects indented blocks here.
📝 One possible fix (convert to indented block)
-```ruby -class MyEventLogger < ContextEventLogger - def handle_event(event, data) - case event - when EVENT_TYPE::EXPOSURE - puts "Exposure: #{data}" - when EVENT_TYPE::GOAL - puts "Goal: #{data}" - when EVENT_TYPE::ERROR - puts "Error: #{data}" - when EVENT_TYPE::PUBLISH - puts "Publish: #{data}" - when EVENT_TYPE::READY - puts "Ready: #{data}" - when EVENT_TYPE::REFRESH - puts "Refresh: #{data}" - when EVENT_TYPE::CLOSE - puts "Close" - end - end -end - -Absmartly.configure_client do |config| - config.endpoint = "https://your-company.absmartly.io/v1" - config.api_key = "YOUR-API-KEY" - config.application = "website" - config.environment = "development" - config.event_logger = MyEventLogger.new -end -``` + class MyEventLogger < ContextEventLogger + def handle_event(event, data) + case event + when EVENT_TYPE::EXPOSURE + puts "Exposure: #{data}" + when EVENT_TYPE::GOAL + puts "Goal: #{data}" + when EVENT_TYPE::ERROR + puts "Error: #{data}" + when EVENT_TYPE::PUBLISH + puts "Publish: #{data}" + when EVENT_TYPE::READY + puts "Ready: #{data}" + when EVENT_TYPE::REFRESH + puts "Refresh: #{data}" + when EVENT_TYPE::CLOSE + puts "Close" + end + end + end + + Absmartly.configure_client do |config| + config.endpoint = "https://your-company.absmartly.io/v1" + config.api_key = "YOUR-API-KEY" + config.application = "website" + config.environment = "development" + config.event_logger = MyEventLogger.new + end
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdspec/absmartly_spec.rb
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
59-59: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🔇 Additional comments (4)
spec/absmartly_spec.rb (2)
16-34: Test helper looks solid and purpose-built.The proxy captures events cleanly and provides a clear reset hook.
189-245: Comprehensive event-flow coverage.The READY/EXPOSURE/GOAL/PUBLISH/CLOSE assertions provide a strong integration signal.
README.md (2)
55-57: Nice clarification about per-context loggers.
101-101: Good addition to document CLOSE event data.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 59-78: Update the example in MyEventLogger#handle_event to
reference the fully qualified constants: replace bare
EVENT_TYPE::EXPOSURE/GOAL/ERROR/PUBLISH/READY/REFRESH/CLOSE with
ContextEventLogger::EVENT_TYPE::EXPOSURE, ContextEventLogger::EVENT_TYPE::GOAL,
ContextEventLogger::EVENT_TYPE::ERROR, ContextEventLogger::EVENT_TYPE::PUBLISH,
ContextEventLogger::EVENT_TYPE::READY, ContextEventLogger::EVENT_TYPE::REFRESH,
and ContextEventLogger::EVENT_TYPE::CLOSE so the subclass MyEventLogger
correctly resolves the nested constants used in handle_event.
♻️ Duplicate comments (1)
README.md (1)
59-87: MD046 code-block style warning persists.This was flagged previously. Either adjust the markdownlint configuration to allow fenced blocks or convert to indented style for consistency.
🧹 Nitpick comments (2)
spec/absmartly_spec.rb (2)
16-35: Unused logger instance in MockContextEventLoggerProxy.The
@loggerattribute (lines 22-23) is initialised but never used within the class. Consider removing it to avoid confusion.♻️ Suggested cleanup
class MockContextEventLoggerProxy < ContextEventLogger - attr_accessor :called, :events, :logger + attr_accessor :called, :events def initialize `@called` = 0 `@events` = [] - `@logger` = Logger.new(STDOUT) - `@logger.level` = Logger::WARN end
135-138: Unused let blocks.
data_future(line 135) anddata_future_ready(line 138) are defined but never referenced in the integration tests. Consider removing them to reduce clutter.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdspec/absmartly_spec.rb
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
59-59: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🔇 Additional comments (5)
spec/absmartly_spec.rb (3)
163-168: LGTM!The helper now correctly uses
Absmartly.create_context(config), which should pick up the globally configuredevent_loggerfrom thebeforeblock. This properly exercises the global configuration path.
170-178: LGTM!The cleanup now comprehensively resets all global configuration fields including the SDK instance variables, preventing cross-test leakage.
193-259: LGTM!The integration tests provide good coverage for all key event types (READY, EXPOSURE, GOAL, PUBLISH, CLOSE) with proper isolation using the
mock_logger.clearpattern.README.md (2)
51-51: LGTM!The
event_loggerdocumentation now correctly specifies the type asContextEventLoggerwith thehandle_event(event, data)method signature.
90-101: LGTM!The event table provides clear documentation for all event types with accurate descriptions of when they fire and what data they provide.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
This PR allows the user to add a global custom event logger which applies to all contexts.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.