Add local mail API rate limiting#1219
Conversation
|
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:
📝 WalkthroughWalkthroughAdds on-device mail API rate limiting: canonical path matching, per-key persisted state with locking and GC, enforcement before token/HTTP execution, preservation of local rate-limit errors through API wrappers and retry loops, and extensive unit and integration tests. ChangesMail rate limiting feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/mailratelimit/limiter.go (1)
100-116: ⚡ Quick winCache compiled regex rules per
Limiter(avoid recompiling inmatch).
Limiter.matchcallscompileRules(l.rules)on every request;compileRulesrunsregexp.MustCompilefor each rule, so this recompiles all patterns on the hot path. Precompute/store the compiled rules when creating the limiter (e.g., innewLimiter) and reuse them inmatch—*regexp.Regexpsupports concurrentMatchStringcalls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/mailratelimit/limiter.go` around lines 100 - 116, The match method calls compileRules on every invocation, causing repeated regex compilation and performance overhead. To fix this, modify the Limiter struct to store compiled rules as a field initialized once (e.g., during newLimiter construction) and change match to use this precompiled data instead of recompiling on each call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/mailratelimit/limiter.go`:
- Around line 100-116: The match method calls compileRules on every invocation,
causing repeated regex compilation and performance overhead. To fix this, modify
the Limiter struct to store compiled rules as a field initialized once (e.g.,
during newLimiter construction) and change match to use this precompiled data
instead of recompiling on each call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6debcfed-910c-4042-a11f-4957beb99f7e
📒 Files selected for processing (18)
cmd/api/api.gocmd/api/api_test.gocmd/service/service.gocmd/service/service_test.gointernal/client/api_errors.gointernal/client/api_errors_test.gointernal/client/client.gointernal/client/client_test.gointernal/client/pagination.gointernal/mailratelimit/canonical.gointernal/mailratelimit/canonical_test.gointernal/mailratelimit/errors.gointernal/mailratelimit/limiter.gointernal/mailratelimit/limiter_test.gointernal/mailratelimit/process_test.gointernal/mailratelimit/rules.gointernal/mailratelimit/store.gointernal/mailratelimit/store_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/client/client_test.go (1)
275-290: ⚡ Quick winTest can hang indefinitely on a regression instead of failing cleanly.
PageLimit: 0(unlimited) combined with a stub that always returnshas_more: true+page_token: "next"means the only thing terminatingPaginateAllis the local rate limit. If a regression causes the rule to stop matching (e.g., canonicalization or keying change), this loops forever and the test hangs rather than producing an actionable failure. Consider bounding the loop (e.g., a generousPageLimit) so a non-firing limiter fails fast.Proposed safeguard
- }, PaginationOptions{PageLimit: 0, PageDelay: -1}) + }, PaginationOptions{PageLimit: 10, PageDelay: -1})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/client_test.go` around lines 275 - 290, The test uses PaginateAll with PaginationOptions{PageLimit: 0} which allows an infinite loop when the stub always returns has_more:true; change the test to use a finite, generous PageLimit (e.g., 10 or 100) instead of 0 so a missing local rate limiter will cause the test to fail fast rather than hang; update the call site passing PaginationOptions to PaginateAll and keep assertions on err being a rate_limit ExitError and apiCalls expectations adjusted if needed.internal/client/api_errors_test.go (1)
52-65: 💤 Low valueReduce drift by reusing the local mail rate-limit error payload in this test
output.ErrAPI(LarkErrRateLimit, ...)already setsExitError.Detail.Type == "rate_limit"(vialark_errors.go), so theTypepart of this assertion isn’t brittle. The drift risk is the local error payload you hand-roll inExitError.Detail.Detail—especially keys likesource: "local_mailratelimit"andretry_after_ms(used byinternal/mailratelimit). Reuse/export theinternal/mailratelimitconstructor (or a small helper) so the test stays coupled to the real contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/api_errors_test.go` around lines 52 - 65, The test TestWrapDoAPIError_PreservesLocalMailRateLimit is hand-constructing the local mail ratelimit payload (map with "source" and "retry_after_ms") which risks drift; instead reuse the real constructor from internal/mailratelimit (or export a small helper) to build the payload passed into output.ErrAPI so the test stays coupled to the real contract. Update the test to call the mailratelimit payload constructor (rather than embedding the map) when creating original, keep using WrapDoAPIError and the same assertions to verify ExitError.Detail is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/client/api_errors_test.go`:
- Around line 52-65: The test TestWrapDoAPIError_PreservesLocalMailRateLimit is
hand-constructing the local mail ratelimit payload (map with "source" and
"retry_after_ms") which risks drift; instead reuse the real constructor from
internal/mailratelimit (or export a small helper) to build the payload passed
into output.ErrAPI so the test stays coupled to the real contract. Update the
test to call the mailratelimit payload constructor (rather than embedding the
map) when creating original, keep using WrapDoAPIError and the same assertions
to verify ExitError.Detail is preserved.
In `@internal/client/client_test.go`:
- Around line 275-290: The test uses PaginateAll with
PaginationOptions{PageLimit: 0} which allows an infinite loop when the stub
always returns has_more:true; change the test to use a finite, generous
PageLimit (e.g., 10 or 100) instead of 0 so a missing local rate limiter will
cause the test to fail fast rather than hang; update the call site passing
PaginationOptions to PaginateAll and keep assertions on err being a rate_limit
ExitError and apiCalls expectations adjusted if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ea72ab5-b5ae-4673-9dd5-ecbe10ee9ec7
📒 Files selected for processing (15)
internal/client/api_errors.gointernal/client/api_errors_test.gointernal/client/client.gointernal/client/client_test.gointernal/mailratelimit/canonical.gointernal/mailratelimit/canonical_test.gointernal/mailratelimit/errors.gointernal/mailratelimit/limiter.gointernal/mailratelimit/limiter_test.gointernal/mailratelimit/rules.gointernal/mailratelimit/store.gointernal/mailratelimit/store_test.goshortcuts/common/common.goshortcuts/mail/mail_shortcut_test.goshortcuts/mail/mail_triage.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/client/client.go
- internal/mailratelimit/canonical.go
bubbmon233
force-pushed
the
harness/01kt0nkhxkc5595f6cm6wp00s6
branch
from
d0e49e1 to
60c5e03
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1219 +/- ##
==========================================
+ Coverage 69.19% 69.40% +0.21%
==========================================
Files 637 644 +7
Lines 59753 60107 +354
==========================================
+ Hits 41345 41718 +373
+ Misses 15067 15018 -49
- Partials 3341 3371 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@c7c0a9757bf1f2595cafefd3a258a99144553750🧩 Skill updatenpx skills add bubbmon233/cli#harness/01kt0nkhxkc5595f6cm6wp00s6 -y -g |
bubbmon233
force-pushed
the
harness/01kt0nkhxkc5595f6cm6wp00s6
branch
8 times, most recently
from
495a596 to
d16bf6b
Compare
bubbmon233
force-pushed
the
harness/01kt0nkhxkc5595f6cm6wp00s6
branch
from
d16bf6b to
c7c0a97
Compare
1 participant
Generated by the harness-coding skill.
Sprints
Source specs
This MR was created autonomously. Quality gates were enforced by the repo's own pre-commit hooks.
Summary by CodeRabbit
New Features
Bug Fixes
Tests