feat(logs): show live upstream providers - #1379
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughPR 新增实时活跃供应商跟踪和快照持久化。日志表显示当前活跃 provider 堆叠，并支持 fallback 切换。思考强度内容改为允许溢出并提升堆叠层级。相关测试已更新。 Changes实时活跃供应商展示
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (event.type === "attempt_started" && event.attemptId && event.provider) { | ||
| sawAttemptLifecycle = true; | ||
| activeAttempts.set(event.attemptId, { | ||
| id: event.provider.id, | ||
| name: event.provider.name ?? String(event.provider.id), | ||
| }); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
When a Discovery sticky probe fails or enters sticky-timeout fallback, the forwarder removes the attempt without immediately emitting attempt_finished; this replay therefore retains the disconnected provider until the request settles or commits a winner, causing live logs to show it alongside the replacement round.
Knowledge Base Used: Proxy request pipeline
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/redis/live-chain-store.ts
Line: 92-99
Comment:
**Unmatched attempts remain active**
When a Discovery sticky probe fails or enters sticky-timeout fallback, the forwarder removes the attempt without immediately emitting `attempt_finished`; this replay therefore retains the disconnected provider until the request settles or commits a winner, causing live logs to show it alongside the replacement round.
**Knowledge Base Used:** [Proxy request pipeline](https://app.greptile.com/ygxz/-/custom-context/knowledge-base/ding113/claude-code-hub/-/docs/proxy-pipeline.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69f164feb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.liveActiveProviders.clear(); | ||
| this.liveActiveProviderCounts.clear(); |
There was a problem hiding this comment.
When a second streaming-hedge attempt is created, createStreamingShadowSession shallow-copies the original ProxySession with Object.assign, so the shadow and original share these Map instances. The shadow's existing setProvider(provider) call now executes these clear() operations and removes the still-connected primary from the original session's live state; consequently, during an actual multi-provider race, the dashboard shows only the newly launched provider instead of all connected providers. Clone the maps for shadow sessions or restrict live-provider mutations to the owning session.
Useful? React with 👍 / 👎.
| }; | ||
|
|
||
| attempts.add(attempt); | ||
| session.addLiveActiveProvider(provider); |
There was a problem hiding this comment.
For the initial streaming-hedge attempt, attemptSession is the original session, and the immediately preceding attemptSession.setProvider(provider) already records the provider with count 1. This additional registration raises its count to 2, while a normal attempt failure calls removeLiveActiveProvider only once, leaving the failed provider displayed as active while fallback selection and related failure handling run. Register launches only when setProvider did not already register that attempt, or remove the count-based duplicate.
Useful? React with 👍 / 👎.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/redis/live-chain-store.ts (1)
129-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
deriveLegacyActiveProviders未覆盖全部终止性reason，且与生产环境的引用计数逻辑脱节。
deriveLegacyActiveProviders的switch语句只处理部分reason值。client_error_non_retryable、endpoint_pool_exhausted、vendor_type_all_timeout等终止性失败原因未列入delete分支，一旦命中这些原因，该 provider 会一直留在返回的 active 列表中。当前生产代码路径（
session.ts的flushLiveObservability）始终显式传入activeProviders参数，因此该函数目前只在测试中被调用。但作为writeLiveChain的默认参数，它与ProxySession的引用计数机制（liveActiveProviders/liveActiveProviderCounts）语义不一致。如果未来有代码调用writeLiveChain时省略第四个参数，会得到与真实会话状态不一致的结果。建议为该函数补全终止性
reason的delete分支，或添加注释说明它仅作为兜底/测试路径使用，避免未来误用。🤖 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 `@src/lib/redis/live-chain-store.ts` around lines 129 - 159, Update deriveLegacyActiveProviders so every terminal failure reason, including client_error_non_retryable, endpoint_pool_exhausted, and vendor_type_all_timeout, removes the provider from activeProviders. Align its switch behavior with the production liveActiveProviders/liveActiveProviderCounts semantics while preserving existing success and winner handling.
🤖 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.
Inline comments:
In `@src/app/`[locale]/dashboard/logs/_components/virtualized-logs-table.tsx:
- Around line 105-127: Update the TooltipTrigger child in the live provider
stack to a native focusable button with type="button", preserving the existing
provider badges and tooltip behavior. Add a translated aria-label using the
component’s next-intl translation mechanism so keyboard users can identify and
activate it, and ensure the focused trigger reveals the full provider list.
In `@src/app/v1/_lib/proxy/session.ts`:
- Around line 210-211: 修复 active provider 状态的生命周期管理：在
src/app/v1/_lib/proxy/session.ts 的 210-211 行，将活动 attempt 映射设计为仅由根 session
持有和管理的独立状态；在 session.ts 429-467 行拆分 setProvider 的 provider 选择与 attempt 引用计数职责，并在
907-919 行只持久化根 session 管理的活动 attempt 快照。更新 src/app/v1/_lib/proxy/forwarder.ts
4561 行仅注销根 session 注册的 attempt，在 4859-4861 行统一失败与取消的幂等注销路径，并在 5351 行确保每个 Hedge
attempt 只注册一次且 shadow session 不重置根 session 的活动列表。
---
Nitpick comments:
In `@src/lib/redis/live-chain-store.ts`:
- Around line 129-159: Update deriveLegacyActiveProviders so every terminal
failure reason, including client_error_non_retryable, endpoint_pool_exhausted,
and vendor_type_all_timeout, removes the provider from activeProviders. Align
its switch behavior with the production
liveActiveProviders/liveActiveProviderCounts semantics while preserving existing
success and winner handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c5d0668-5241-497f-8f31-c4b05741ae4b
📒 Files selected for processing (11)
src/app/[locale]/dashboard/logs/_components/thinking-effort-display.test.tsxsrc/app/[locale]/dashboard/logs/_components/thinking-effort-display.tsxsrc/app/[locale]/dashboard/logs/_components/usage-logs-table.test.tsxsrc/app/[locale]/dashboard/logs/_components/usage-logs-table.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.test.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsxsrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/session.tssrc/lib/redis/live-chain-store.storage.test.tssrc/lib/redis/live-chain-store.tssrc/repository/usage-logs.ts
| <TooltipProvider> | ||
| <Tooltip delayDuration={250}> | ||
| <TooltipTrigger asChild> | ||
| <span | ||
| className="flex min-w-0 items-center -space-x-2 cursor-help" | ||
| data-slot="live-provider-stack" | ||
| > | ||
| {visibleProviders.map((provider) => ( | ||
| <span | ||
| key={provider.id} | ||
| className="relative max-w-[68px] truncate rounded-md border bg-background px-1.5 py-0.5 text-xs text-foreground shadow-sm" | ||
| > | ||
| {provider.name} | ||
| </span> | ||
| ))} | ||
| {hiddenProviderCount > 0 && ( | ||
| <span className="relative rounded-md border bg-muted px-1.5 py-0.5 text-xs text-muted-foreground shadow-sm"> | ||
| +{hiddenProviderCount} | ||
| </span> | ||
| )} | ||
| </span> | ||
| </TooltipTrigger> | ||
| <TooltipContent className="max-w-[320px]" side="bottom" align="start"> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg 'src/app/.*/dashboard/logs/_components/virtualized-logs-table\.tsx|package\.json|package-lock\.json|pnpm-lock\.yaml|yarn\.lock' || true
echo "== relevant source =="
if [ -f src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx ]; then
wc -l src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx
sed -n '1,190p' 'src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx'
fi
echo "== search LiveProviderStack/usages =="
rg -n "LiveProviderStack|visibleProviders|hiddenProviderCount|TooltipTrigger" src || true
echo "== tooltip imports/usages in file =="
rg -n "from ['\"]`@radix-ui/react-tooltip`['\"]|TooltipTrigger|TooltipContent" src/app || true
echo "== dependencies =="
if [ -f package.json ]; then
node -e "const p=require('./package.json'); console.log(JSON.stringify({deps:p.dependencies, devDeps:p.devDependencies}, null, 2))"
fiRepository: ding113/claude-code-hub
Length of output: 2038
🌐 Web query:
Radix UI TooltipTrigger asChild non-focusable element span keyboard accessibility button type button
💡 Result:
When using Radix UI TooltipTrigger with the asChild prop, the child element must be a focusable element (such as a button, anchor, or input) to maintain accessibility [1][2]. If you replace the default trigger with a non-focusable element like a span or div, you break the component's keyboard accessibility [1][2]. Specifically: 1. Keyboard users will be unable to focus the trigger to activate the tooltip [2]. 2. Screen readers may not correctly identify the element as an interactive trigger, and accessibility attributes like aria-describedby may not be attached or announced as expected [3][2]. 3. Radix UI documentation explicitly states that when using asChild, it is the developer's responsibility to ensure the resulting element remains accessible and functional, which requires it to be able to respond to pointer and keyboard events [1]. If you find yourself needing to wrap a non-focusable element (like text in a span) to avoid nested button issues (e.g., placing a tooltip inside another button), simply changing it to a span will cause a loss of keyboard functionality [3][4]. To maintain accessibility in such cases, you must manually ensure the element is focusable and interactive, for example by adding tabIndex={0} and appropriate keyboard event listeners to the span, although this is generally discouraged in favor of using natively focusable elements like buttons or links [5][2]. When using asChild with a properly focusable element (like your own button component), ensure your component correctly forwards refs and spreads props so that Radix can attach the necessary aria attributes and event handlers [1][6][2].
Citations:
- 1: https://www.radix-ui.com/primitives/docs/guides/composition
- 2: https://eastondev.com/blog/en/posts/dev/20260330-shadcn-radix-accessibility/
- 3: Tooltip on Trigger radix-ui/primitives#560
- 4: Tooltip gives hydration error when placed into Accordion trigger radix-ui/primitives#3212
- 5: [Tooltip] wrapping
inputelement defaults its type tobuttonradix-ui/primitives#993 - 6: Tooltip does not work with TooltipTrigger asChild property radix-ui/primitives#2959
使完整 provider 列表可通过键盘访问。
TooltipTrigger asChild 下的 <span> 不是焦点元素，键盘用户无法聚焦或激活 Tooltip。将触发器改为原生的可聚焦元素，例如 <button type="button">，并使用 next-intl 提供翻译后的 aria-label。验证 Tab 聚焦触发器后可以显示完整 provider 列表。
🤖 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 `@src/app/`[locale]/dashboard/logs/_components/virtualized-logs-table.tsx
around lines 105 - 127, Update the TooltipTrigger child in the live provider
stack to a native focusable button with type="button", preserving the existing
provider badges and tooltip behavior. Add a translated aria-label using the
component’s next-intl translation mechanism so keyboard users can identify and
activate it, and ensure the focused trigger reveals the full provider list.
Source: Coding guidelines
| private liveActiveProviders = new Map<number, LiveProviderSnapshot>(); | ||
| private liveActiveProviderCounts = new Map<number, number>(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
修复 active provider 状态的生命周期模型。 provider 选择和运行 attempt 共用同一个引用计数。初始 Hedge attempt 被重复计数。shadow session 会重置根 session 的共享映射。
src/app/v1/_lib/proxy/session.ts#L210-L211: 将活动 attempt 状态设计为仅由根 session 管理的独立状态。src/app/v1/_lib/proxy/session.ts#L429-L467: 不要让setProvider同时承担 provider 选择和 attempt 引用计数。src/app/v1/_lib/proxy/session.ts#L907-L919: 只持久化经过根 session 生命周期管理的活动 attempt 快照。src/app/v1/_lib/proxy/forwarder.ts#L4561-L4561: 只注销此前由根 session 注册的 attempt。src/app/v1/_lib/proxy/forwarder.ts#L4859-L4861: 使失败清理与取消清理共享幂等的注销路径。src/app/v1/_lib/proxy/forwarder.ts#L5351-L5351: 每个 Hedge attempt 只能注册一次，且 shadow session 不得重置根 session 的活动列表。
📍 Affects 2 files
src/app/v1/_lib/proxy/session.ts#L210-L211(this comment)src/app/v1/_lib/proxy/session.ts#L429-L467src/app/v1/_lib/proxy/session.ts#L907-L919src/app/v1/_lib/proxy/forwarder.ts#L4561-L4561src/app/v1/_lib/proxy/forwarder.ts#L4859-L4861src/app/v1/_lib/proxy/forwarder.ts#L5351-L5351
🤖 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 `@src/app/v1/_lib/proxy/session.ts` around lines 210 - 211, 修复 active provider
状态的生命周期管理：在 src/app/v1/_lib/proxy/session.ts 的 210-211 行，将活动 attempt 映射设计为仅由根
session 持有和管理的独立状态；在 session.ts 429-467 行拆分 setProvider 的 provider 选择与 attempt
引用计数职责，并在 907-919 行只持久化根 session 管理的活动 attempt 快照。更新
src/app/v1/_lib/proxy/forwarder.ts 4561 行仅注销根 session 注册的 attempt，在 4859-4861
行统一失败与取消的幂等注销路径，并在 5351 行确保每个 Hedge attempt 只注册一次且 shadow session 不重置根 session
的活动列表。
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }; | ||
|
|
||
| attempts.add(attempt); | ||
| session.addLiveActiveProvider(provider); |
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] Initial hedge participant stays in the live provider stack after it fails
Why this is a problem: startAttempt(initialProvider, true) already routes through attemptSession.setProvider(provider), and the new setProvider() bookkeeping seeds liveActiveProviderCounts with 1. Adding the same provider again here bumps the initial participant to 2. When that first attempt later fails, handleAttemptFailure() / abortAttempt() only removes one count, so the failed provider remains in _liveChain.activeProviders and the logs UI continues to show it as an active upstream even though the connection is already gone.
Suggested fix:
attempts.add(attempt);
if (!useOriginalSession) {
session.addLiveActiveProvider(provider);
}
There was a problem hiding this comment.
Code Review Summary
One hedge-path state-tracking regression needs attention before merge. The new live-provider plumbing is localized, but the initial participant bookkeeping can leave a failed upstream displayed as still active in the logs UI.
PR Size: M
- Lines changed: 341
- Files changed: 11
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
- None.
High Priority Issues (Should Fix)
src/app/v1/_lib/proxy/forwarder.ts:5351—session.addLiveActiveProvider(provider)double-counts the initial hedge participant becausesetProvider()has already seeded the same provider on the original session. When that first attempt fails, only one count is removed, so_liveChain.activeProviderscan still show a dead upstream as active.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
- Reviewed PR
#1379, applied thesize/Mlabel, and posted the required summary review on GitHub. - Filed 1 high-priority inline comment at
src/app/v1/_lib/proxy/forwarder.ts:5351. - Issue: the new live-provider bookkeeping double-counts the initial hedge participant, so when that first attempt fails it can remain in
_liveChain.activeProvidersand still appear active in the logs UI. - Suggested fix in the review: only call
session.addLiveActiveProvider(provider)for non-original/shadow hedge attempts, or otherwise avoid seeding the same provider twice. - No other changed-line issues survived the full-context validation threshold.
bunxis not installed in this runner, so test execution was not available from this environment.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Verification
bun run typecheckbun run lintbun run build(passes; existing Edge/Node compatibility warnings remain)git diff --checkGreptile Summary
This PR adds live upstream-provider snapshots to Redis and renders active racing or fallback providers in the virtualized logs table.
Confidence Score: 4/5
The Discovery lifecycle gap should be fixed before merging because live logs can identify a failed sticky provider as still connected during the replacement race.
The new event replay assumes every started Discovery attempt receives an immediate terminal event, but two reachable sticky fallback branches continue routing after removing an attempt without recording its completion.
Files Needing Attention: src/lib/redis/live-chain-store.ts and the Discovery cleanup branches in src/app/v1/_lib/proxy/forwarder.ts
Important Files Changed
Sequence Diagram
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(logs): show live upstream providers" | Re-trigger Greptile
Context used (3)