fix(probe): remove success log filtering to enable latency curve rendering#636
Conversation
When upgrading from older versions, existing providers were incorrectly grouped under "Unknown Vendor #0" instead of being auto-aggregated by their website domain. Changes: - Add backfillProviderVendorsFromProviders() to auto-create vendors from provider URLs during startup - Add deriveDisplayNameFromDomain() to generate display names from domains - Integrate vendor backfill into instrumentation.ts startup flow - Fix providerVendorId type to allow null (Schema, Type, Transformer) - Add vendorId=-1 protection for orphaned providers in frontend - Add i18n keys for orphanedProviders in 5 languages The backfill runs idempotently on startup, processing providers with null or 0 vendorId, extracting domains from websiteUrl (preferred) or url, and creating/associating vendors accordingly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ering Problem: - Endpoint probe scheduler runs correctly every 10s - But successful probes were filtered by 60s sampling threshold - ~5/6 success probes not recorded to history table - Frontend latency curve could not be rendered Solution: - Remove shouldLogScheduledSuccess() filtering function - Remove SUCCESS_LOG_MIN_INTERVAL_MS constant - Always call recordProviderEndpointProbeResult() for all probes - Change default retention from 7 days to 1 day (storage: ~40MB/day) - Update .env.example to reflect new behavior Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 Walkthrough总体概述该 PR 引入对孤立提供商（缺少有效供应商 ID 的提供商）的支持。将 providerVendorId 在数据库架构和类型定义中改为可空，添加孤立提供商的本地化字符串，实现启动时的供应商回填机制，更新 UI 组件处理孤立提供商分组，并移除基于快照的探测日志记录改为始终记录结果。探测日志保留期从 7 天减少到 1 天。 变更清单
代码审查工作量估计🎯 3 (中等) | ⏱️ ~30 分钟 可能相关的 PR
🚥 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
🧪 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 |
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the reliability and completeness of endpoint probe data by removing previous filtering mechanisms that led to incomplete logging of successful probes. This change directly improves the accuracy of frontend latency visualizations and ensures a more robust historical record. Additionally, it introduces better handling for providers not associated with a vendor, including a backfill mechanism and UI adjustments, while also optimizing log storage by reducing the default retention period. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
Bot
added
bug
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![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 `@src/drizzle/schema.ts`:
- Around line 154-156: The schema change to providerVendorId (the
integer('provider_vendor_id') column referencing providerVendors.id) removed the
notNull() constraint but you did not generate the migration; run the migration
generator (bun run db:generate) to produce the corresponding migration that
drops the NOT NULL constraint for providerVendorId, verify the generated
migration reflects the change (alter column to nullable) and add/commit that
migration alongside the schema change before merging.
🧹 Nitpick comments (3)
src/app/[locale]/settings/providers/_components/provider-vendor-view.tsx (1)
133-137: 排序行为可能需要调整。当前排序
(a, b) => a - b会将孤立供应商组（vendorId = -1）置于列表最前。从用户体验角度，孤立供应商通常应显示在最后，便于用户优先关注正常关联的供应商。可选：将孤立供应商排序至末尾
const allVendorIds = useMemo(() => { const ids = new Set<number>(vendors.map((v) => v.id)); Object.keys(providersByVendor).forEach((id) => ids.add(Number(id))); - return Array.from(ids).sort((a, b) => a - b); + return Array.from(ids).sort((a, b) => { + // 将 -1（孤立供应商）排到最后 + if (a === -1) return 1; + if (b === -1) return -1; + return a - b; + }); }, [vendors, providersByVendor]);src/instrumentation.ts (1)
209-225: 建议提取重复的 backfill 逻辑到辅助函数生产环境 (209-225) 和开发环境 (309-325) 的 provider vendors backfill 代码块完全相同。考虑提取到一个类似于
syncErrorRulesAndInitializeDetector()的独立辅助函数，减少重复并便于维护。♻️ 建议的重构方案
+/** + * 回填 provider_vendors（按域名自动聚合旧 providers） + * 非关键功能，失败不阻塞启动 + */ +async function backfillProviderVendors(): Promise<void> { + try { + const { backfillProviderVendorsFromProviders } = await import( + "@/repository/provider-endpoints" + ); + const vendorResult = await backfillProviderVendorsFromProviders(); + logger.info("[Instrumentation] Provider vendors backfill completed", { + processed: vendorResult.processed, + providersUpdated: vendorResult.providersUpdated, + vendorsCreatedCount: vendorResult.vendorsCreated.size, + skippedInvalidUrl: vendorResult.skippedInvalidUrl, + }); + } catch (error) { + logger.warn("[Instrumentation] Failed to backfill provider vendors", { + error: error instanceof Error ? error.message : String(error), + }); + } +}然后在生产和开发环境中调用
await backfillProviderVendors();Also applies to: 309-325
src/repository/provider-endpoints.ts (1)
310-310:vendorsCreated命名可能产生误导
vendorsCreatedSet 实际上收集的是所有被关联的 vendor ID（包括已存在的和新创建的），但命名暗示这些都是新创建的。这可能导致日志中vendorsCreatedCount的含义不够准确。考虑重命名为
vendorsReferenced或uniqueVendorIds，或者在getOrCreateProviderVendorIdFromUrls返回时区分是否为新创建。
| providerVendorId: integer('provider_vendor_id').references(() => providerVendors.id, { | ||
| onDelete: 'restrict', | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the schema.ts file around lines 154-156
head -160 src/drizzle/schema.ts | tail -20Repository: ding113/claude-code-hub
Length of output: 771
🏁 Script executed:
# List migration files in the drizzle directory
ls -la src/drizzle/ 2>/dev/null || echo "drizzle directory not found"Repository: ding113/claude-code-hub
Length of output: 360
🏁 Script executed:
# Check if migrations directory exists and list recent migrations
fd -e sql . src/drizzle/migrations 2>/dev/null | sort -r | head -10Repository: ding113/claude-code-hub
Length of output: 49
🏁 Script executed:
# Search for provider_vendor_id in migration files to find the relevant migration
rg "provider_vendor_id" src/drizzle/migrations/ 2>/dev/null | head -20Repository: ding113/claude-code-hub
Length of output: 49
必须执行 bun run db:generate 生成迁移文件。
providerVendorId 字段的 schema 变更已确认（移除 notNull() 约束），但未发现对应的迁移文件。根据编码规范，修改 src/drizzle/schema.ts 后必须运行 bun run db:generate 来生成迁移文件。请确保在合并前完成此步骤。
🤖 Prompt for AI Agents
In `@src/drizzle/schema.ts` around lines 154 - 156, The schema change to
providerVendorId (the integer('provider_vendor_id') column referencing
providerVendors.id) removed the notNull() constraint but you did not generate
the migration; run the migration generator (bun run db:generate) to produce the
corresponding migration that drops the NOT NULL constraint for providerVendorId,
verify the generated migration reflects the change (alter column to nullable)
and add/commit that migration alongside the schema change before merging.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of incomplete probe history data by removing the success log filtering mechanism and adjusting the default log retention. The changes to allow providerVendorId to be nullable are well-integrated across the database schema, API actions, and UI components, including a crucial backfill script for existing data. The updated tests accurately reflect the new probe logging behavior. Overall, the changes significantly improve the accuracy of probe data and enable proper rendering of the latency curve.
I am having trouble creating individual review comments. Click here to see my feedback.
src/drizzle/schema.ts (154-156)
Removing the .notNull() constraint for providerVendorId is a critical schema change that enables providers to exist without an explicit vendor association. This is fundamental to supporting 'orphaned' providers and the new backfill logic.
src/types/provider.ts (45)
Changing providerVendorId: number; to providerVendorId: number | null; is a fundamental type definition update that reflects the database schema change. This is crucial for type safety and correctly representing the new nullable state of vendor associations.
src/repository/provider-endpoints.ts (230-337)
The deriveDisplayNameFromDomain and backfillProviderVendorsFromProviders functions are critical for data migration. backfillProviderVendorsFromProviders ensures that existing providers are correctly associated with vendors based on their URLs, or new vendors are created. This is a robust solution for handling the new nullable providerVendorId field.
src/repository/provider.ts (565-585)
This conditional logic for ensureProviderEndpointExistsForUrl in updateProvider is well-designed. It ensures that endpoints are correctly managed when a provider's URL or vendor association changes, or when a provider is newly assigned to a vendor. The check for transformed.providerVendorId !== previousVendorId || previousVendorId === null is particularly important for handling initial vendor assignments.
src/lib/provider-endpoints/probe.ts (142-152)
The removal of the shouldLogScheduledSuccess function is a key change. This function was responsible for sampling successful probe logs, and its removal ensures that all successful probe results are now recorded, enabling more accurate latency curve rendering.
src/instrumentation.ts (209-225)
The addition of backfillProviderVendorsFromProviders() in the instrumentation is a high-priority change. This ensures that existing providers are correctly associated with vendors (or new ones created) after the providerVendorId column becomes nullable, maintaining data integrity and enabling the new UI features.
src/app/v1/_lib/proxy/forwarder.ts (269)
This additional check for currentProvider.providerVendorId being truthy is important. It ensures that isVendorTypeCircuitOpen is only called when a valid providerVendorId exists, preventing potential issues with null values.
src/lib/provider-endpoints/probe.ts (5)
Removing updateProviderEndpointProbeSnapshot from the imports is correct, as this function is no longer used after the refactoring of probe result logging.
src/app/v1/_lib/proxy/forwarder.ts (916-917)
Adding currentProvider.providerVendorId to this condition is a necessary safety check. recordVendorTypeAllEndpointsTimeout should only be invoked if a valid providerVendorId is present, preventing errors when dealing with providers that might not have a vendor association.
src/app/v1/_lib/proxy/provider-selector.ts (534)
Adding provider.providerVendorId && here is a crucial defensive check. It ensures that isVendorTypeCircuitOpen is only called when providerVendorId is not null, preventing potential runtime errors and improving the robustness of the circuit breaker logic.
src/app/v1/_lib/proxy/provider-selector.ts (877)
This check is correctly added to ensure isVendorTypeCircuitOpen is only invoked when p.providerVendorId is a valid, non-null value, aligning with the updated schema.
src/app/v1/_lib/proxy/provider-selector.ts (971)
Adding p.providerVendorId && here is consistent with other changes, ensuring that isVendorTypeCircuitOpen is only called with a valid providerVendorId.
src/app/v1/_lib/proxy/session.ts (484)
Changing vendorId: provider.providerVendorId, to vendorId: provider.providerVendorId ?? undefined, correctly maps a null providerVendorId to undefined in the ProviderChainItem. This is a more idiomatic way to represent an optional field that is not present, aligning with the updated type definition.
src/app/v1/_lib/proxy/forwarder.ts (231)
Adding currentProvider.providerVendorId && is a good defensive check. Since providerVendorId can now be null, this prevents potential TypeError if currentProvider.providerVendorId is null before accessing its value.
src/app/[locale]/settings/providers/_components/vendor-keys-compact-list.tsx (120)
Adding this check ensures that getProviderEndpoints is not called for the virtual orphaned vendor group (where vendorId <= 0), preventing unnecessary API calls and potential errors.
src/instrumentation.ts (309-325)
Including the backfillProviderVendorsFromProviders() call in the development environment is also important. This ensures that developers working with local databases will have their data correctly migrated and aligned with the new schema, preventing inconsistencies during development.
src/lib/provider-endpoints/probe-log-cleanup.ts (18)
Changing the default ENDPOINT_PROBE_LOG_RETENTION_DAYS from 7 to 1 directly implements the specified change in the PR description. This reduces storage requirements for probe logs.
src/actions/providers.ts (759)
The change from if (provider) to if (provider?.providerVendorId) is a good defensive measure. With providerVendorId now being nullable, explicitly checking for its existence before attempting to delete the vendor prevents potential runtime errors if providerVendorId is null or 0.
src/lib/provider-endpoints/probe.ts (32-35)
The removal of SUCCESS_LOG_MIN_INTERVAL_MS is a direct consequence of eliminating the filtering logic for successful probe logs, which is a core change in this PR.
src/app/[locale]/settings/providers/_components/provider-vendor-view.tsx (248)
Similarly, preventing the VendorEndpointsSection from rendering for the virtual orphaned group (vendorId > 0) is correct, as orphaned providers do not have associated endpoints in the same way real vendors do.
src/repository/_shared/transformers.ts (69)
Changing the default value for providerVendorId from 0 to null aligns the transformation logic with the updated database schema, correctly representing providers without an assigned vendor.
src/repository/provider-endpoints.ts (3)
Adding or to the drizzle-orm imports is necessary for the new backfillProviderVendorsFromProviders function, which uses or(isNull(providers.providerVendorId), eq(providers.providerVendorId, 0)) to identify providers needing a vendor association.
src/app/[locale]/settings/providers/_components/provider-vendor-view.tsx (230)
Restricting the DeleteVendorDialog to vendorId > 0 is appropriate. The virtual 'orphaned' vendor group should not be deletable, as it's a logical grouping rather than a physical entity.
src/repository/provider.ts (136-150)
Adding the if (created.providerVendorId) check before calling ensureProviderEndpointExistsForUrl is a good correctness improvement. It prevents attempts to create endpoints for providers that do not have a valid vendor ID, which is now possible with the nullable providerVendorId.
src/app/[locale]/settings/providers/_components/provider-vendor-view.tsx (195-197)
This conditional rendering for displayName correctly displays 'Unknown Vendor' for the virtual orphaned group (vendorId === -1), improving the user experience and clarity for providers without an assigned vendor.
src/app/[locale]/settings/providers/_components/provider-vendor-view.tsx (115-128)
The updated logic for grouping providers correctly handles cases where providerVendorId is null or 0 by assigning them to an 'orphaned' group with vendorId: -1. This ensures that all providers are displayed, even those not yet associated with a vendor, and aligns with the database schema change.
src/types/provider.ts (161)
Updating providerVendorId to number | null in the ProviderDisplay interface is consistent with the schema and Provider interface changes, ensuring type consistency across the application.
tests/unit/lib/provider-endpoints/probe.test.ts (332)
The test case name has been updated to reflect the new behavior where successful scheduled probes are always logged, which is consistent with the removal of the sampling logic.
tests/unit/lib/provider-endpoints/probe.test.ts (339)
Removing snapshotMock is correct as the updateProviderEndpointProbeSnapshot function is no longer used in the probe logic.
tests/unit/lib/provider-endpoints/probe.test.ts (375)
This assertion correctly verifies that recordMock (for recordProviderEndpointProbeResult) is now called for successful scheduled probes, confirming the removal of the filtering logic.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR. The changes properly address the probe logging issue while maintaining code quality standards.
PR Size: L
- Lines changed: 344 (237 additions, 107 deletions)
- Files changed: 21
Split Suggestion: This PR could be split into two logical commits:
- Core probe logging fix (probe.ts, probe-log-cleanup.ts, tests, .env.example)
- Vendor backfill support for legacy data (schema.ts, repository files, UI components, i18n)
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean (all catch blocks properly log errors)
- Type safety - Clean (nullable providerVendorId properly handled)
- Documentation accuracy - Clean (comments match code, i18n properly implemented)
- Test coverage - Adequate (probe tests updated to reflect new behavior)
- Code clarity - Good
Key Changes Validated
- Probe logging: Removed filtering logic to record all probe results - correctly addresses the issue
- Error handling: All catch blocks use logger.warn/error appropriately
- Null safety: providerVendorId null checks added consistently across proxy logic
- Backfill logic: Vendor auto-creation from legacy providers is properly paginated and error-handled
- UI handling: Orphaned providers (vendorId = -1) are properly grouped and displayed
- i18n compliance: All user-facing strings use i18n (5 languages)
Automated review by Claude AI
| stats.processed++; | ||
|
|
||
| const domainSource = row.websiteUrl?.trim() || row.url; | ||
| const domain = normalizeWebsiteDomainFromUrl(domainSource); |
There was a problem hiding this comment.
[High] [LOGIC-BUG] Backfill treats invalid websiteUrl as fatal (even when url is usable)
src/repository/provider-endpoints.ts:288
Why this is a problem: The current code prefers websiteUrl unconditionally:
const domainSource = row.websiteUrl?.trim() || row.url;
If websiteUrl is non-empty but malformed, normalizeWebsiteDomainFromUrl(domainSource) returns null and the provider is counted as skippedInvalidUrl, leaving provider_vendor_id unset even though row.url may be a valid fallback. Additionally, getOrCreateProviderVendorIdFromUrls({ websiteUrl: row.websiteUrl }) will still throw for an invalid websiteUrl, preventing backfill.
Suggested fix:
const websiteDomain = row.websiteUrl ? normalizeWebsiteDomainFromUrl(row.websiteUrl) : null;
const providerDomain = normalizeWebsiteDomainFromUrl(row.url);
const domain = websiteDomain ?? providerDomain;
if (!domain) {
// log + skippedInvalidUrl++
...
continue;
}
const vendorId = await getOrCreateProviderVendorIdFromUrls({
providerUrl: row.url,
websiteUrl: websiteDomain ? row.websiteUrl : null,
faviconUrl: row.faviconUrl ?? null,
displayName: deriveDisplayNameFromDomain(domain),
});
| * 按照 website_url（优先）或 url 的域名进行自动聚合 | ||
| */ | ||
| export async function backfillProviderVendorsFromProviders(): Promise<{ | ||
| processed: number; |
There was a problem hiding this comment.
[High] [TEST-MISSING-CRITICAL] No unit coverage for backfillProviderVendorsFromProviders()
src/repository/provider-endpoints.ts:244
Why this is a problem: This PR adds a new startup backfill that reads/writes DB state (backfillProviderVendorsFromProviders() is invoked from src/instrumentation.ts). CLAUDE.md:13 requires: All new features must have unit test coverage of at least 80%. There are existing repository tests in tests/unit/repository/provider-endpoints.test.ts, but none cover this new backfill path (success update, invalid URL skip, error handling).
Suggested fix:
// tests/unit/repository/provider-endpoints.test.ts
test("backfillProviderVendorsFromProviders: updates providers with NULL vendor id", async () => {
vi.resetModules();
const providerRow = {
id: 1,
name: "OpenAI",
url: "https://api.openai.com",
websiteUrl: null,
faviconUrl: null,
providerVendorId: null,
};
// providers page -> vendor lookup (empty) -> providers end
const selectPages = [[providerRow], [], []];
const selectMock = vi.fn(() => createThenableQuery(selectPages.shift() ?? []));
const returning = vi.fn(async () => [{ id: 99 }]);
const onConflictDoNothing = vi.fn(() => ({ returning }));
const values = vi.fn(() => ({ onConflictDoNothing }));
const insertMock = vi.fn(() => ({ values }));
const where = vi.fn(async () => []);
const set = vi.fn(() => ({ where }));
const updateMock = vi.fn(() => ({ set }));
vi.doMock("@/drizzle/db", () => ({
db: { select: selectMock, insert: insertMock, update: updateMock },
}));
const { backfillProviderVendorsFromProviders } = await import("@/repository/provider-endpoints");
const result = await backfillProviderVendorsFromProviders();
expect(result.providersUpdated).toBe(1);
expect(set).toHaveBeenCalledWith(expect.objectContaining({ providerVendorId: 99 }));
});
There was a problem hiding this comment.
Code Review Summary
This PR changes endpoint probe logging behavior and also introduces provider-vendor backfill + nullable providerVendorId handling across the UI/proxy layers. The probe change is straightforward, but the new vendor backfill path has a correctness gap and lacks unit coverage.
PR Size: L
- Lines changed: 344
- Files changed: 21
- Split suggestion: Split into (1) probe logging + retention changes, (2) provider vendor nullability/backfill + UI/i18n updates.
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 | 1 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
None.
High Priority Issues (Should Fix)
src/repository/provider-endpoints.ts: backfill skips valid providers whenwebsiteUrlis malformed (Confidence: 90)src/repository/provider-endpoints.ts: missing unit coverage for new vendor backfill invoked on startup (Confidence: 85)
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
#636and applied labelsize/L(344 lines changed across 21 files). - Posted 2 inline review comments on
src/repository/provider-endpoints.ts:- Backfill logic bug: malformed
websiteUrlcan block backfill even whenurlis usable. - Missing unit coverage for the new
backfillProviderVendorsFromProviders()startup behavior (perCLAUDE.md:13).
- Backfill logic bug: malformed
- Submitted the required PR review summary via
gh pr review --comment.
1 participant
Summary
shouldLogScheduledSuccess()filtering function that was causing ~5/6 of successful probe results to be discardedprovider_vendor_idfor legacy providers without vendor associationRelated PRs
Problem Analysis
Endpoint probe scheduler was running correctly every 10s, but successful probes were filtered by a 60s sampling threshold (
SUCCESS_LOG_MIN_INTERVAL_MS). This caused:provider_endpoint_probe_logstableAdditionally, legacy providers created before PR #608 had
NULLor0values forprovider_vendor_id, causing null pointer issues in vendor-type circuit breaker logic.Changes
Core Changes
src/lib/provider-endpoints/probe.tssrc/lib/provider-endpoints/probe-log-cleanup.tstests/unit/lib/provider-endpoints/probe.test.ts.env.exampleSupporting Changes
src/repository/provider-endpoints.tsbackfillProviderVendorsFromProviders()to auto-create vendors for legacy providerssrc/instrumentation.tssrc/drizzle/schema.tsproviderVendorIdnullable to support legacy datasrc/types/provider.tssrc/repository/provider.tssrc/app/v1/_lib/proxy/*.tsproviderVendorIdin circuit breaker logicsrc/app/[locale]/settings/providers/_components/*.tsxmessages/*/settings/providers/strings.jsonStorage Impact
Test Plan
Checklist
🤖 Generated with Claude Code