Conversation
WalkthroughCentralizes rate limiter creation by introducing utils.GetRateLimiter and replacing direct ratelimit constructions in runner, config, and multi components. The new helper returns either an unlimited limiter or a timed limiter based on inputs. No public API changes except the new exported utility function. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Runner/Lib
participant Utils as utils.GetRateLimiter
participant RL as ratelimit pkg
Caller->>Runner/Lib: initialize components
Runner/Lib->>Utils: GetRateLimiter(ctx, rate, duration)
alt rate==0 or duration==0
Utils-->>Runner/Lib: return unlimited limiter
note over Utils,Runner/Lib: Zero-value inputs yield no rate limiting
else
Utils->>RL: New(ctx, uint(rate), duration)
RL-->>Utils: limiter instance
Utils-->>Runner/Lib: return configured limiter
end
Runner/Lib-->>Caller: proceed with obtained limiter
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/multi.go (1)
56-56: Avoid drift: rely on utils.GetRateLimiter for defaultingYou still set RateLimitDuration locally when zero; if you accept centralizing defaulting in utils.GetRateLimiter (see suggested change), drop the local default to keep behavior uniform across runner/multi/config.
Follow-up (after updating utils.GetRateLimiter as proposed):
- if opts.RateLimit > 0 && opts.RateLimitDuration == 0 { - opts.RateLimitDuration = time.Second - }lib/config.go (1)
184-185: Confirm semantics: duration==0 now yields unlimited via utils.GetRateLimiterWith the new helper, passing (maxTokens > 0, duration == 0) results in an unlimited limiter, which may differ from prior behavior. If the intended policy is “default to 1s when duration is zero,” either adopt the centralized defaulting in utils.GetRateLimiter (preferred) or add the same default here for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/auto_assign.ymlis excluded by!**/*.yml
📒 Files selected for processing (4)
internal/runner/runner.go(1 hunks)lib/config.go(2 hunks)lib/multi.go(2 hunks)pkg/utils/utils.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/utils/utils.gointernal/runner/runner.golib/config.golib/multi.go
🧬 Code graph analysis (4)
pkg/utils/utils.go (1)
internal/runner/runner.go (1)
New(109-394)
internal/runner/runner.go (1)
pkg/utils/utils.go (1)
GetRateLimiter(80-85)
lib/config.go (1)
pkg/utils/utils.go (1)
GetRateLimiter(80-85)
lib/multi.go (1)
pkg/utils/utils.go (1)
GetRateLimiter(80-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (ubuntu-latest)
🔇 Additional comments (2)
pkg/utils/utils.go (1)
78-85: Harden limiter creation: guard negatives and centralize duration defaultingTreat negative/zero maxTokens as unlimited and default non-positive durations to 1s; remove duplicated per-callsite defaults.
-// GetRateLimiter returns a rate limiter with the given max tokens and duration -// if maxTokens is 0 or duration is 0, it returns an unlimited rate limiter func GetRateLimiter(ctx context.Context, maxTokens int, duration time.Duration) *ratelimit.Limiter { - if maxTokens == 0 || duration == 0 { - return ratelimit.NewUnlimited(ctx) - } - return ratelimit.New(ctx, uint(maxTokens), duration) + // Policy: + // - maxTokens <= 0 => unlimited + // - duration <= 0 => defaults to 1s + if maxTokens <= 0 { + return ratelimit.NewUnlimited(ctx) + } + if duration <= 0 { + duration = time.Second + } + return ratelimit.New(ctx, uint(maxTokens), duration) }Update/remove duplicated duration-defaulting at these call sites:
- internal/runner/runner.go — remove the opts.RateLimitDuration == 0 -> time.Second block (≈ lines 384–386).
- lib/multi.go — remove the opts.RateLimitDuration == 0 -> time.Second block (≈ lines 53–55).
- lib/sdk_private.go — replace the manual branch (currently setting RateLimitDuration and calling ratelimit.New/Unlimited, ≈ lines 224–231) with utils.GetRateLimiter(...) to keep semantics consistent.
Confirm API/SDK semantics: WithGlobalRateLimitCtx (lib/config.go:179–186) currently passes duration through; after this change duration <= 0 will be treated as 1s (not “unlimited”) — validate that this is the intended SDK behavior and update docs if necessary.
internal/runner/runner.go (1)
387-387: LGTM: centralized limiter initializationUsing utils.GetRateLimiter here simplifies the logic and keeps behavior consistent with other components.
|
@coderabbitai resolve |
🧩 Analysis chainI'll resolve the review comments and create an issue for this PR. @Mzack9999 I've created an issue to track this PR and will resolve all the review comments. 🏁 Script executed: Length of output: 54 @Mzack9999 Done! I've resolved all review comments and created issue #6474 to track this PR: #6474 The issue includes the background, proposed changes, and references this PR as requested. ✅ Actions performedComments resolved. |
Proposed changes
Supersedes #5667
Checklist
Summary by CodeRabbit