fix: allow http client configuration from configure_client #33
Conversation
WalkthroughThis PR renames the SDK option Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/client_config.rb`:
- Around line 47-53: The http_client_config method currently uses truthy checks
which ignore valid zero values; change the conditional assignments to explicit
nil checks so zeros are preserved (e.g., use unless `@connect_timeout.nil`? or if
!@connect_timeout.nil? for each of `@connect_timeout`,
`@connection_request_timeout`, `@retry_interval`, and `@max_retries`) when setting
properties on DefaultHttpClientConfig in http_client_config so 0 is propagated
correctly and the method still returns http_config.
🧹 Nitpick comments (1)
spec/absmartly_spec.rb (1)
8-41: LGTM!Good test coverage for the new configuration options. The
afterblock properly resets singleton state to ensure test isolation.Consider adding tests for edge cases such as negative values or zero for timeouts/retries, and verifying default values when options are not explicitly set.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.mdlib/absmartly.rblib/absmartly/version.rblib/client.rblib/client_config.rbspec/absmartly_spec.rbspec/client_config_spec.rb
🧰 Additional context used
🧬 Code graph analysis (5)
lib/absmartly.rb (2)
lib/client_config.rb (1)
attr_accessor(5-55)lib/default_http_client.rb (1)
attr_accessor(8-62)
lib/client_config.rb (2)
lib/client.rb (2)
attr_accessor(7-79)create(11-13)lib/default_http_client.rb (2)
attr_accessor(8-62)create(11-13)
spec/client_config_spec.rb (4)
lib/client.rb (1)
create(11-13)lib/client_config.rb (2)
create(10-12)http_client_config(47-54)lib/context_config.rb (1)
create(7-9)lib/default_http_client.rb (1)
create(11-13)
spec/absmartly_spec.rb (1)
lib/absmartly.rb (1)
configure_client(18-20)
lib/client.rb (8)
lib/absmartly.rb (1)
create(22-24)lib/client_config.rb (2)
create(10-12)http_client_config(47-54)lib/a_b_smartly_config.rb (1)
create(7-9)lib/context_config.rb (1)
create(7-9)lib/a_b_smartly.rb (1)
create(17-19)lib/default_http_client.rb (1)
create(11-13)lib/context.rb (1)
create(15-19)lib/default_http_client_config.rb (1)
create(9-11)
🔇 Additional comments (9)
lib/absmartly/version.rb (1)
4-4: LGTM!Version bump to 1.2.3 appropriately reflects the new HTTP client configuration features.
README.md (2)
32-35: LGTM!The configuration example clearly demonstrates the new HTTP client options with sensible default values.
44-50: LGTM!The SDK options table is well-documented with the renamed
api_keyand the new timeout/retry configuration fields. The defaults (3.0s for timeouts, 0.5s retry interval, 5 max retries) are reasonable for HTTP client configuration.lib/client.rb (1)
11-12: LGTM!The refactoring correctly delegates HTTP client configuration to
config.http_client_config, enabling the configuration flow fromconfigure_clientthrough to the underlying HTTP client. This is a clean approach that maintains separation of concerns.spec/client_config_spec.rb (2)
85-107: LGTM!Good test coverage for the new configuration properties. Each property has a dedicated test verifying its getter/setter behaviour.
109-135: LGTM!Excellent test coverage for
http_client_config. The tests verify both:
- Custom values are correctly propagated to the
DefaultHttpClientConfig- Default values are applied when options are not explicitly set
The default values tested (3.0, 3.0, 0.5, 5) align with the documentation in README.md.
lib/client_config.rb (1)
3-8: LGTM for the new client HTTP accessors.
The additions are cleanly scoped and align with the new configuration surface.lib/absmartly.rb (2)
15-16: LGTM for exposing the new HTTP configuration fields.
This aligns the singleton API with the updated client configuration surface.
49-52: Configuration wiring looks correct.
The new fields are propagated intoClientConfigas expected.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
calthejuggler
force-pushed
the
feature/timeout-and-retry-configuration
branch
from
1b1b911 to
fad915d
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
| | connection_request_timeout | `number` | ❌ | `3.0` | The request timeout in seconds. | | ||
| | retry_interval | `number` | ❌ | `0.5` | The initial retry interval in seconds (uses exponential backoff). | | ||
| | max_retries | `number` | ❌ | `5` | The maximum number of retries before giving up. | | ||
| | eventLogger | `(context, eventName, data) => void` | ❌ | See "Using a Custom Event Logger" below | A callback function which runs after SDK events. | |
There was a problem hiding this comment.
I don't think this event logger is exposed, so we can also remove it from the README
This PR allows configuration of the HTTP client as documented in the README.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.