Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough本次变更为 API 令牌引入按令牌的并发请求限制:后端中间件新增并发计数/限流逻辑及错误响应(JSON 与 SSE 两种格式),代理层在令牌验证后尝试获取并发槽位并在超限时返回 429,前端设置页新增管理员可配置的并发上限(默认 5),并增加对应本地化字符串和测试覆盖。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProxyHandler as Proxy\nHandler
participant TokenAuth as TokenAuth\nMiddleware
participant Counter as Concurrency\nCounter
participant Response
Client->>ProxyHandler: HTTP 请求(含 API Token)
ProxyHandler->>ProxyHandler: 检测是否为流式请求
ProxyHandler->>TokenAuth: ValidateRequest / ResolveToken
TokenAuth->>TokenAuth: 提取并校验 Token
TokenAuth-->>ProxyHandler: Token 有效(apiToken)
ProxyHandler->>TokenAuth: AcquireConcurrency(apiToken)
TokenAuth->>Counter: 检查当前活跃数 vs 限额
alt 有可用槽位
Counter-->>TokenAuth: 成功(计数增加)
TokenAuth-->>ProxyHandler: 返回成功
ProxyHandler->>ProxyHandler: defer ReleaseConcurrency
ProxyHandler->>Response: 继续处理请求
else 超出限额
Counter-->>TokenAuth: 返回 ErrTokenConcurrentLimit
TokenAuth-->>ProxyHandler: 返回错误
ProxyHandler->>Response: 写入 429 Rate Limit(JSON 或 SSE)(含 Retry-After)
Response-->>Client: 429 响应
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
概述本次变更为 API 令牌增加了并发请求限制功能。实现了基于令牌的并发计数机制、速率限制错误响应处理、后端中间件集成,以及前端管理界面,支持自定义每令牌并发限制阈值(默认值为 5),超限返回标准 429 响应。 变更清单
庆祝诗
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/handler/token_auth_test.go (1)
155-178: 测试覆盖充分,但缺少 name-based 路径的测试。当前测试使用
ID=42的 token,仅覆盖了activeByTokenID路径。建议补充一个ID=0的测试用例以覆盖activeByTokenName回退路径。♻️ 可选:补充 name-based 并发限制测试
func TestTokenAuthConcurrentLimitByName(t *testing.T) { repo := newTokenAuthTestRepo() cachedRepo := cached.NewAPITokenRepository(repo) middleware := NewTokenAuthMiddleware(cachedRepo, tokenAuthTestSettingRepo{}) token := &domain.APIToken{ID: 0, Token: "maxx_test_token_noID", IsEnabled: true} for i := 0; i < DefaultAPITokenConcurrentLimit; i++ { if err := middleware.AcquireConcurrency(token); err != nil { t.Fatalf("AcquireConcurrency() attempt %d error = %v", i+1, err) } } if err := middleware.AcquireConcurrency(token); err != ErrTokenConcurrentLimit { t.Fatalf("AcquireConcurrency() over limit error = %v, want %v", err, ErrTokenConcurrentLimit) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handler/token_auth_test.go` around lines 155 - 178, The existing test TestTokenAuthConcurrentLimitDefaultsToFive only exercises the ID-based lookup path; add a new test (e.g., TestTokenAuthConcurrentLimitByName) that constructs a token with ID: 0 and Token set (e.g., "maxx_test_token_noID"), uses newTokenAuthTestRepo + cached.NewAPITokenRepository + NewTokenAuthMiddleware, then loop AcquireConcurrency up to DefaultAPITokenConcurrentLimit expecting success, assert the next AcquireConcurrency returns ErrTokenConcurrentLimit, and optionally ReleaseConcurrency afterward; this will exercise the activeByTokenName fallback path used inside AcquireConcurrency.web/src/pages/settings/index.tsx (1)
633-635: 小问题:使用小写<label>而非<Label>组件。其他设置区域(如
DataRetentionSection)使用的是<Label>组件,建议保持一致性。♻️ 建议修改
- <label className="text-sm font-medium text-muted-foreground shrink-0"> + <Label className="text-sm font-medium text-muted-foreground shrink-0"> {t('settings.apiTokenConcurrencyLimit')} - </label> + </Label>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/settings/index.tsx` around lines 633 - 635, Replace the lowercase HTML <label> with the shared React <Label> component to match other sections (e.g., DataRetentionSection) and ensure consistent styling; update the JSX where the text {t('settings.apiTokenConcurrencyLimit')} is rendered (replace the <label className="..."> wrapper with <Label className="...">) and add or confirm the import of Label at the top of the file if missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/handler/token_auth_test.go`:
- Around line 155-178: The existing test
TestTokenAuthConcurrentLimitDefaultsToFive only exercises the ID-based lookup
path; add a new test (e.g., TestTokenAuthConcurrentLimitByName) that constructs
a token with ID: 0 and Token set (e.g., "maxx_test_token_noID"), uses
newTokenAuthTestRepo + cached.NewAPITokenRepository + NewTokenAuthMiddleware,
then loop AcquireConcurrency up to DefaultAPITokenConcurrentLimit expecting
success, assert the next AcquireConcurrency returns ErrTokenConcurrentLimit, and
optionally ReleaseConcurrency afterward; this will exercise the
activeByTokenName fallback path used inside AcquireConcurrency.
In `@web/src/pages/settings/index.tsx`:
- Around line 633-635: Replace the lowercase HTML <label> with the shared React
<Label> component to match other sections (e.g., DataRetentionSection) and
ensure consistent styling; update the JSX where the text
{t('settings.apiTokenConcurrencyLimit')} is rendered (replace the <label
className="..."> wrapper with <Label className="...">) and add or confirm the
import of Label at the top of the file if missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86291737-3770-4436-ad88-7112b544c660
📒 Files selected for processing (7)
internal/handler/proxy.gointernal/handler/proxy_test.gointernal/handler/token_auth.gointernal/handler/token_auth_test.goweb/src/locales/en.jsonweb/src/locales/zh.jsonweb/src/pages/settings/index.tsx
📜 Review details
⏰ 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). (2)
- GitHub Check: e2e
- GitHub Check: playwright
🔇 Additional comments (13)
web/src/locales/en.json (1)
999-1004: LGTM!新增的 i18n 键遵循现有命名约定,
defaultValue使用了正确的{{value}}插值语法。提示文本清晰地解释了超过并发限制时的 429 行为。web/src/locales/zh.json (1)
999-1004: LGTM!中文翻译准确且与英文版本保持一致。
defaultValue使用了正确的默认 {{value}}格式。internal/handler/proxy_test.go (2)
35-53: LGTM!测试覆盖了 429 响应的核心场景:HTTP 状态码、
Retry-After头和错误类型。JSON 结构验证正确。
82-95: LGTM!流式响应测试正确验证了 SSE 格式的 429 响应。使用
strings.Contains检查流式响应体是合适的做法。internal/handler/token_auth_test.go (1)
14-23: LGTM!使用
switch语句使设置仓库的测试实现更清晰,正确返回了并发限制设置值。web/src/pages/settings/index.tsx (1)
575-651: 整体实现良好。组件正确实现了草稿状态管理、验证逻辑和保存流程,与其他设置区域保持了一致的模式。
internal/handler/proxy.go (3)
151-161: LGTM!并发限制的 acquire/release 模式实现正确。
defer放置位置正确——仅在AcquireConcurrency成功后才注册释放。错误处理根据stream标志正确分支到 JSON 或 SSE 响应。
310-323: LGTM!
writeRateLimitError正确设置了 429 状态码、Retry-After头和rate_limit_error类型。默认值处理(retryAfterSeconds <= 0时设为 1)是合理的防御性编程。
352-376: LGTM!SSE 格式的 429 响应实现正确,包括
text/event-streamContent-Type、Cache-Control: no-cache和正确的事件格式。与现有writeStreamError保持一致的模式。internal/handler/token_auth.go (4)
196-223: 实现正确,但有一个边缘情况需要注意。当
apiToken.ID == 0且keyName为空时(第 219-221 行),函数会返回nil(成功)但不进行任何追踪,可能允许无限请求。不过在实际场景中,validateExtractedToken已验证 token 格式,不会出现空 token,所以这是安全的。并发控制逻辑整体正确:
- 使用 mutex 保证线程安全
- ID 优先、name 回退的双重追踪策略合理
- 先检查限制再递增的顺序正确
225-252: LGTM!
ReleaseConcurrency正确处理了资源释放:
- nil token 的安全返回
- 计数器归零时删除 map 条目以防止内存泄漏
- ID 和 name 两种路径的释放逻辑一致
116-150: LGTM!
validateExtractedToken和ResolveToken的重构使代码结构更清晰,复用了验证逻辑。ResolveToken提供了不依赖 client type 的 token 解析能力。
275-282: LGTM!常量定义清晰,默认并发限制为 5 与 PR 目标一致。将设置键和默认值放在同一个
const块中便于维护。
|
@coderabbitai re-review !Stop making breaking changes, do a proper review! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 72329
|
|
@coderabbitai re-review !Stop making breaking changes, do a proper review! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 181396
Corrected Re-Review of PR
|
| Previous claim | Reality |
|---|---|
APITokenConcurrencySection missing !hasChanges guard in second useEffect |
Wrong. The guard is present: if (initialized && !hasChanges) { setLimitDraft(currentLimit); } — identical pattern to DataRetentionSection. |
AcquireConcurrency silent no-op when ID == 0 and Token == "" |
Wrong. The code explicitly returns ErrInvalidToken in that case: if apiToken.ID == 0 && keyName == "" { return ErrInvalidToken } |
ResolveToken has no test coverage |
Wrong. TestTokenAuthResolveToken (valid + invalid token sub-tests) and TestTokenAuthAcquireConcurrencyRejectsEmptyNameFallback both exist and cover these paths. |
stream moved before clientType is a behavioral regression |
Not a regression. IsStreamRequest(r, body) takes only r and body — it has no dependency on clientType. |
ResolveToken passing "" as ClientType is fragile |
Intentional and correct. Empty ClientType falls through to the generic multi-header scan in ExtractToken, which is exactly the right behavior for client-type-agnostic token resolution. |
✅ Accurate assessment
The PR is well-implemented:
token_auth.go: Concurrency maps are correctly keyed by ID (preferred) or token name (fallback), protected by a mutex, with proper acquire/release symmetry and correct edge case handling.proxy.go: The concurrency gate is correctly placed after token validation and before request processing, withdefer ReleaseConcurrencyensuring cleanup. The 429 helpers correctly setRetry-Afterand emit both JSON and SSE formats.settings/index.tsx:APITokenConcurrencySectionfollows the same correct draft/sync pattern as existing sections. The save path validates>= 1before mutating.- Tests: All new behavior is covered — default limit, name-keyed fallback, empty-name rejection, and
ResolveTokenresolution paths.
No breaking changes, no behavioral regressions, no public API contract violations detected.
✏️ Learnings added
Learnt from: ymkiux
URL: https://github.com/awsl-project/maxx/pull/466
Timestamp: 2026-03-30T09:23:52.562Z
Learning: In awsl-project/maxx PR `#466`, all 5 concerns raised in the initial review were incorrect. The `APITokenConcurrencySection` useEffect guard was present, `AcquireConcurrency` correctly returns `ErrInvalidToken` for zero-ID+empty-token, `ResolveToken` is fully tested, `IsStreamRequest` has no dependency on clientType, and passing `""` as ClientType in `ExtractToken` is intentional to scan all auth headers. Be more careful to read the actual code before flagging concerns.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ymkiux
Repo: awsl-project/maxx PR: 0
File: :0-0
Timestamp: 2026-03-11T07:29:12.071Z
Learning: Reviewer ymkiux (repo: awsl-project/maxx) requires strict attention to breaking changes in all PRs. Always thoroughly audit public API, interface, and behavioral contract changes — especially in sidebar/auth/layout components — before raising review comments. Do not flag superficial style issues; focus on correctness, regressions, and breaking changes.
Summary
Changes
Retry-Afterapi_token_concurrent_limitValidation
go test ./internal/handler/...cd web && pnpm run typecheckCloses #464
Summary by CodeRabbit
发布说明
新功能
文档 / 本地化
测试