fix(provider-endpoints): close #742 endpoint update regressions#746
fix(provider-endpoints): close #742 endpoint update regressions#746
Conversation
…Rules The update path for isDefault=true rules was missing overrideResponse and overrideStatusCode fields, causing code-level changes to default error override messages to never propagate to the database on restart.
- Fix UI consistency: ensure operations return single authoritative result (success/error) - Fix sync logic: prevent aggressive soft-deletion of sibling endpoints - Fix unique constraints: align constraints to ignore soft-deleted rows - Add comprehensive regression tests for direct edit and sync scenarios - Add data repair tool and documentation for troubleshooting
📝 WalkthroughWalkthrough新增数据库迁移与部分索引更改；在仓库层引入冲突检测、写后读一致性与回填/修复流程；前端对端点增删改流程的异步与错误处理进行调整；添加大量单元与集成测试以及故障排查文档以覆盖 Changes
预期代码审查工作量🎯 4 (Complex) | ⏱️ ~60 分钟 可能相关的 PR
🚥 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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 addresses several regressions related to provider endpoint management, significantly enhancing the system's reliability and user experience. It ensures that endpoint updates are deterministic and consistent, provides clear feedback for conflicts, and prevents accidental data loss during provider synchronization. Additionally, a new diagnostic and repair tool has been added to help identify and resolve historical data inconsistencies. Highlights
Changelog
Activity
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
|
![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 successfully addresses the endpoint update regressions and sibling disappearance issues identified in #742. The implementation of a more conservative sync strategy, deterministic edit outcomes with read-after-write consistency checks, and a robust repair tool significantly improves the reliability of provider endpoint management. The troubleshooting guide and integration tests provide excellent support for maintaining these changes. I have identified a few minor areas for improvement regarding consistency in field usage and promise handling in the UI components.
| // Use void to catch promise errors silently to avoid double toast | ||
| void queryClient.invalidateQueries({ queryKey: ["provider-endpoints", vendorId] }); |
There was a problem hiding this comment.
The comment states that void is used to 'catch promise errors silently', but void only informs the linter that the promise is intentionally not being awaited; it does not catch rejections. To truly avoid unhandled promise rejections and ensure errors are silenced as intended, you should append a .catch(() => {}) to the promise.
| // Use void to catch promise errors silently to avoid double toast | |
| void queryClient.invalidateQueries({ queryKey: ["provider-endpoints", vendorId] }); | |
| // Use void and .catch() to ignore promise outcomes and avoid double toast | |
| void queryClient.invalidateQueries({ queryKey: ["provider-endpoints", vendorId] }).catch(() => {}); |
src/repository/provider-endpoints.ts
Outdated
| if (expectedEditableFields.url !== undefined) updates.url = expectedEditableFields.url; | ||
| if (payload.label !== undefined) updates.label = payload.label; | ||
| if (payload.sortOrder !== undefined) updates.sortOrder = payload.sortOrder; | ||
| if (payload.isEnabled !== undefined) updates.isEnabled = payload.isEnabled; |
There was a problem hiding this comment.
For better consistency and to ensure the updates object perfectly aligns with the expectedEditableFields used in the subsequent consistency check, it is recommended to use the expectedEditableFields object for all fields in the update payload.
| if (expectedEditableFields.url !== undefined) updates.url = expectedEditableFields.url; | |
| if (payload.label !== undefined) updates.label = payload.label; | |
| if (payload.sortOrder !== undefined) updates.sortOrder = payload.sortOrder; | |
| if (payload.isEnabled !== undefined) updates.isEnabled = payload.isEnabled; | |
| if (expectedEditableFields.url !== undefined) updates.url = expectedEditableFields.url; | |
| if (expectedEditableFields.label !== undefined) updates.label = expectedEditableFields.label; | |
| if (expectedEditableFields.sortOrder !== undefined) updates.sortOrder = expectedEditableFields.sortOrder; | |
| if (expectedEditableFields.isEnabled !== undefined) updates.isEnabled = expectedEditableFields.isEnabled; |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/repository/provider-endpoints.ts
Outdated
| if (payload.label !== undefined) updates.label = payload.label; | ||
| if (payload.sortOrder !== undefined) updates.sortOrder = payload.sortOrder; | ||
| if (payload.isEnabled !== undefined) updates.isEnabled = payload.isEnabled; |
There was a problem hiding this comment.
Inconsistency: URL uses expectedEditableFields.url (trimmed) but label/sortOrder/isEnabled use raw payload values. For consistency, all fields should go through the expectations picking logic.
| if (payload.label !== undefined) updates.label = payload.label; | |
| if (payload.sortOrder !== undefined) updates.sortOrder = payload.sortOrder; | |
| if (payload.isEnabled !== undefined) updates.isEnabled = payload.isEnabled; | |
| if (expectedEditableFields.url !== undefined) updates.url = expectedEditableFields.url; | |
| if (expectedEditableFields.label !== undefined) updates.label = expectedEditableFields.label; | |
| if (expectedEditableFields.sortOrder !== undefined) updates.sortOrder = expectedEditableFields.sortOrder; | |
| if (expectedEditableFields.isEnabled !== undefined) updates.isEnabled = expectedEditableFields.isEnabled; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/provider-endpoints.ts
Line: 1746:1748
Comment:
Inconsistency: URL uses `expectedEditableFields.url` (trimmed) but label/sortOrder/isEnabled use raw `payload` values. For consistency, all fields should go through the expectations picking logic.
```suggestion
if (expectedEditableFields.url !== undefined) updates.url = expectedEditableFields.url;
if (expectedEditableFields.label !== undefined) updates.label = expectedEditableFields.label;
if (expectedEditableFields.sortOrder !== undefined) updates.sortOrder = expectedEditableFields.sortOrder;
if (expectedEditableFields.isEnabled !== undefined) updates.isEnabled = expectedEditableFields.isEnabled;
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
github-actions
bot
added
bug
src/actions/provider-endpoints.ts
Outdated
| if (candidate.code === "PROVIDER_ENDPOINT_CONFLICT" || candidate.code === "23505") { | ||
| return true; | ||
| } | ||
|
|
||
| if (typeof candidate.message === "string") { | ||
| if (candidate.message.includes("[ProviderEndpointEdit] endpoint conflict")) { | ||
| return true; | ||
| } | ||
|
|
||
| if (candidate.message.includes("duplicate key value")) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| if (candidate.cause?.code === "23505") { | ||
| return true; | ||
| } | ||
|
|
||
| return ( | ||
| typeof candidate.cause?.message === "string" && | ||
| candidate.cause.message.includes("duplicate key value") | ||
| ); |
There was a problem hiding this comment.
Over-broad conflict detection
isDirectEndpointEditConflictError() treats any Postgres unique violation (code === "23505") or message containing "duplicate key value" as an endpoint-URL edit conflict. That will misreport unrelated unique constraint failures inside editProviderEndpoint() (e.g., if another unique index is violated during the same action) as ERROR_CODES.CONFLICT with the wrong user message. This should be constrained to the specific PROVIDER_ENDPOINT_CONFLICT code you throw from the repository, or at least check the constraint name (uniq_provider_endpoints_vendor_type_url) before mapping to CONFLICT.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/provider-endpoints.ts
Line: 144:165
Comment:
**Over-broad conflict detection**
`isDirectEndpointEditConflictError()` treats any Postgres unique violation (`code === "23505"`) or message containing `"duplicate key value"` as an endpoint-URL edit conflict. That will misreport *unrelated* unique constraint failures inside `editProviderEndpoint()` (e.g., if another unique index is violated during the same action) as `ERROR_CODES.CONFLICT` with the wrong user message. This should be constrained to the specific `PROVIDER_ENDPOINT_CONFLICT` code you throw from the repository, or at least check the constraint name (`uniq_provider_endpoints_vendor_type_url`) before mapping to CONFLICT.
How can I resolve this? If you propose a fix, please make it concise.| DROP INDEX IF EXISTS "uniq_provider_endpoints_vendor_type_url";--> statement-breakpoint | ||
| CREATE UNIQUE INDEX IF NOT EXISTS "uniq_provider_endpoints_vendor_type_url" ON "provider_endpoints" USING btree ("vendor_id","provider_type","url") WHERE "provider_endpoints"."deleted_at" IS NULL; |
There was a problem hiding this comment.
Migration numbering mismatch
PR description and docs refer to migration 0064 for the partial unique index, but the actual migration added here is 0065_stale_vertigo.sql (and journal updated accordingly). If downstream environments expect 0064_* or if troubleshooting docs/scripts reference 0064, this will break operator instructions and could leave the old index in place. Update the PR description/docs (or rename/regenerate the migration) so the migration number/tag referenced externally matches what ships.
Prompt To Fix With AI
This is a comment left during a code review.
Path: drizzle/0065_stale_vertigo.sql
Line: 1:2
Comment:
**Migration numbering mismatch**
PR description and docs refer to migration `0064` for the partial unique index, but the actual migration added here is `0065_stale_vertigo.sql` (and journal updated accordingly). If downstream environments expect `0064_*` or if troubleshooting docs/scripts reference 0064, this will break operator instructions and could leave the old index in place. Update the PR description/docs (or rename/regenerate the migration) so the migration number/tag referenced externally matches what ships.
How can I resolve this? If you propose a fix, please make it concise.| vi.resetModules(); | ||
|
|
||
| const duplicateKeyError = Object.assign( | ||
| new Error("duplicate key value violates unique constraint"), | ||
| { | ||
| code: "23505", | ||
| } | ||
| ); | ||
|
|
||
| const endpointRow = { | ||
| id: 42, | ||
| vendorId: 7, | ||
| providerType: "claude", | ||
| url: "https://next.example.com/v1/messages", | ||
| label: null, | ||
| sortOrder: 0, | ||
| isEnabled: true, | ||
| lastProbedAt: null, | ||
| lastProbeOk: null, | ||
| lastProbeStatusCode: null, | ||
| lastProbeLatencyMs: null, | ||
| lastProbeErrorType: null, | ||
| lastProbeErrorMessage: null, | ||
| createdAt: new Date("2026-01-01T00:00:00.000Z"), | ||
| updatedAt: new Date("2026-01-01T00:00:00.000Z"), | ||
| deletedAt: null, | ||
| }; | ||
|
|
||
| const updateReturningMock = vi.fn(async () => { | ||
| throw duplicateKeyError; | ||
| }); | ||
| const updateWhereMock = vi.fn(() => ({ returning: updateReturningMock })); | ||
| const updateSetMock = vi.fn(() => ({ where: updateWhereMock })); | ||
| const updateMock = vi.fn(() => ({ set: updateSetMock })); | ||
|
|
||
| const selectLimitMock = vi.fn(async () => [endpointRow]); | ||
| const selectWhereMock = vi.fn(() => ({ limit: selectLimitMock })); | ||
| const selectFromMock = vi.fn(() => ({ where: selectWhereMock })); | ||
| const selectMock = vi.fn(() => ({ from: selectFromMock })); | ||
|
|
||
| vi.doMock("@/drizzle/db", () => ({ | ||
| db: { | ||
| update: updateMock, | ||
| select: selectMock, | ||
| }, | ||
| })); | ||
|
|
||
| const { updateProviderEndpoint } = await import("@/repository/provider-endpoints"); |
There was a problem hiding this comment.
Mock leakage across tests
This test uses vi.resetModules() + vi.doMock("@/drizzle/db", ...) but never vi.unmock()/vi.doUnmock() or vi.restoreAllMocks() afterward. In Vitest, that can leak the mocked module into later tests that import @/drizzle/db (especially if ordering changes), causing unrelated unit tests to run against the mock DB. Add cleanup (e.g. afterEach(() => { vi.restoreAllMocks(); vi.doUnmock("@/drizzle/db"); })) or scope the mock using vi.mock at file level with proper teardown.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/repository/provider-endpoint-742-direct-edit.test.ts
Line: 5:52
Comment:
**Mock leakage across tests**
This test uses `vi.resetModules()` + `vi.doMock("@/drizzle/db", ...)` but never `vi.unmock()`/`vi.doUnmock()` or `vi.restoreAllMocks()` afterward. In Vitest, that can leak the mocked module into later tests that import `@/drizzle/db` (especially if ordering changes), causing unrelated unit tests to run against the mock DB. Add cleanup (e.g. `afterEach(() => { vi.restoreAllMocks(); vi.doUnmock("@/drizzle/db"); })`) or scope the mock using `vi.mock` at file level with proper teardown.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/repository/provider-endpoints.ts`:
- Around line 1752-1796: The write-after-read consistency branch in
updateProviderEndpoint throws a plain Error when
readConsistentProviderEndpointAfterWrite returns falsy, which lacks a structured
code like the unique-violation path; change that throw to produce a structured
error with a code (e.g. PROVIDER_ENDPOINT_WRITE_READ_INCONSISTENCY) so callers
can dispatch uniformly—create the error similarly to the conflict branch (e.g.
Object.assign(new Error("[ProviderEndpointEdit] write-read consistency check
failed"), { code: PROVIDER_ENDPOINT_WRITE_READ_INCONSISTENCY })) and reference
the same symbols: updateProviderEndpoint,
readConsistentProviderEndpointAfterWrite, and the existing
DIRECT_ENDPOINT_EDIT_CONFLICT_CODE pattern.
- Around line 63-64: The constant DIRECT_ENDPOINT_EDIT_CONFLICT_CODE is defined
but not exported, causing the actions layer to hardcode
"PROVIDER_ENDPOINT_CONFLICT"; export DIRECT_ENDPOINT_EDIT_CONFLICT_CODE from
provider-endpoints.ts and update the actions code that checks the literal
"PROVIDER_ENDPOINT_CONFLICT" to import and use
DIRECT_ENDPOINT_EDIT_CONFLICT_CODE instead so the error code is maintained in
one place.
🧹 Nitpick comments (5)
src/repository/provider-endpoints.ts (5)
1351-1356:clampSampleLimit参数类型与实际使用不一致，且存在死代码参数声明为
number | undefined（来自可选属性），但!Number.isFinite(value)在value为undefined时已返回true。第 1355 行的value ?? 20永远不会走到?? 20分支，因为执行到此处时value一定是有限正数。建议简化
function clampSampleLimit(value: number | undefined): number { - if (!Number.isFinite(value) || (value ?? 0) <= 0) { + if (value === undefined || !Number.isFinite(value) || value <= 0) { return 20; } - return Math.min(Math.trunc(value ?? 20), 2000); + return Math.min(Math.trunc(value), 2000); }
615-642: 多处查询重复展开字段选择，可复用providerEndpointSelectFields
findProviderEndpointById（615-642）、findProviderEndpointsByVendorAndType（751-784）、findProviderEndpointsByVendor（787-812）等函数仍然手动列举所有列，而第 207-224 行已定义了providerEndpointSelectFields。updateProviderEndpoint的.returning()已使用它（第 1757 行），建议其他查询函数也统一使用以减少重复。示例（以 findProviderEndpointById 为例）
export async function findProviderEndpointById( endpointId: number ): Promise<ProviderEndpoint | null> { const rows = await db - .select({ - id: providerEndpoints.id, - vendorId: providerEndpoints.vendorId, - providerType: providerEndpoints.providerType, - url: providerEndpoints.url, - label: providerEndpoints.label, - sortOrder: providerEndpoints.sortOrder, - isEnabled: providerEndpoints.isEnabled, - lastProbedAt: providerEndpoints.lastProbedAt, - lastProbeOk: providerEndpoints.lastProbeOk, - lastProbeStatusCode: providerEndpoints.lastProbeStatusCode, - lastProbeLatencyMs: providerEndpoints.lastProbeLatencyMs, - lastProbeErrorType: providerEndpoints.lastProbeErrorType, - lastProbeErrorMessage: providerEndpoints.lastProbeErrorMessage, - createdAt: providerEndpoints.createdAt, - updatedAt: providerEndpoints.updatedAt, - deletedAt: providerEndpoints.deletedAt, - }) + .select(providerEndpointSelectFields) .from(providerEndpoints)Also applies to: 751-784, 787-812
1662-1696: backfill apply 模式下批量插入无事务保护
flush()中的db.insert()在事务外逐批执行。如果中途出错，已插入的批次无法回滚，导致部分修复状态。对于修复工具这可能是可接受的（幂等onConflictDoNothing），但建议在日志或报告中明确记录实际完成的批次数，以便排查中断情况。
446-448: 注释使用了中文根据编码规范，用户可见字符串需要 i18n。虽然这些是代码注释而非用户可见字符串，但在多语言团队中建议保持注释语言统一为英文，便于所有贡献者理解。
Also applies to: 472-474
157-170:computeVendorKey和deriveDisplayNameFromDomain无需async两个函数都声明为
async但不含任何异步操作，返回Promise包装不必要。这增加微任务开销，且会误导调用方认为存在 I/O 操作。建议同时移除两个函数的
async：
computeVendorKey: 改为string | nullderiveDisplayNameFromDomain: 改为string更新所有调用点移除
await（src/repository/provider-endpoints.ts:400, 520; src/actions/providers.ts:3666; 测试文件）。
🧪 测试结果
总体结果: ✅ 所有测试通过 |
| const updates: Partial<typeof providerEndpoints.$inferInsert> = { updatedAt: now }; | ||
| if (payload.url !== undefined) updates.url = payload.url; | ||
| if (payload.label !== undefined) updates.label = payload.label; | ||
| if (payload.sortOrder !== undefined) updates.sortOrder = payload.sortOrder; | ||
| if (payload.isEnabled !== undefined) updates.isEnabled = payload.isEnabled; | ||
| if (expectedEditableFields.url !== undefined) updates.url = expectedEditableFields.url; | ||
| if (expectedEditableFields.label !== undefined) updates.label = expectedEditableFields.label; | ||
| if (expectedEditableFields.sortOrder !== undefined) | ||
| updates.sortOrder = expectedEditableFields.sortOrder; | ||
| if (expectedEditableFields.isEnabled !== undefined) | ||
| updates.isEnabled = expectedEditableFields.isEnabled; |
There was a problem hiding this comment.
Fields picked inconsistently: url uses expectedEditableFields.url (already trimmed), but label, sortOrder, and isEnabled use expectedEditableFields.* which go through the expectations picking logic. For true consistency, all fields should use the expected values or all should use raw payload.
| const updates: Partial<typeof providerEndpoints.$inferInsert> = { updatedAt: now }; | |
| if (payload.url !== undefined) updates.url = payload.url; | |
| if (payload.label !== undefined) updates.label = payload.label; | |
| if (payload.sortOrder !== undefined) updates.sortOrder = payload.sortOrder; | |
| if (payload.isEnabled !== undefined) updates.isEnabled = payload.isEnabled; | |
| if (expectedEditableFields.url !== undefined) updates.url = expectedEditableFields.url; | |
| if (expectedEditableFields.label !== undefined) updates.label = expectedEditableFields.label; | |
| if (expectedEditableFields.sortOrder !== undefined) | |
| updates.sortOrder = expectedEditableFields.sortOrder; | |
| if (expectedEditableFields.isEnabled !== undefined) | |
| updates.isEnabled = expectedEditableFields.isEnabled; | |
| const updates: Partial<typeof providerEndpoints.$inferInsert> = { updatedAt: now }; | |
| if (expectedEditableFields.url !== undefined) updates.url = expectedEditableFields.url; | |
| if (payload.label !== undefined) updates.label = payload.label; | |
| if (payload.sortOrder !== undefined) updates.sortOrder = payload.sortOrder; | |
| if (payload.isEnabled !== undefined) updates.isEnabled = payload.isEnabled; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/provider-endpoints.ts
Line: 1744:1750
Comment:
Fields picked inconsistently: `url` uses `expectedEditableFields.url` (already trimmed), but `label`, `sortOrder`, and `isEnabled` use `expectedEditableFields.*` which go through the expectations picking logic. For true consistency, all fields should use the expected values or all should use raw payload.
```suggestion
const updates: Partial<typeof providerEndpoints.$inferInsert> = { updatedAt: now };
if (expectedEditableFields.url !== undefined) updates.url = expectedEditableFields.url;
if (payload.label !== undefined) updates.label = payload.label;
if (payload.sortOrder !== undefined) updates.sortOrder = payload.sortOrder;
if (payload.isEnabled !== undefined) updates.isEnabled = payload.isEnabled;
```
How can I resolve this? If you propose a fix, please make it concise.
src/actions/provider-endpoints.ts
Outdated
| if (candidate.code === "PROVIDER_ENDPOINT_CONFLICT" || candidate.code === "23505") { | ||
| return true; | ||
| } | ||
|
|
||
| if (typeof candidate.message === "string") { | ||
| if (candidate.message.includes("[ProviderEndpointEdit] endpoint conflict")) { | ||
| return true; | ||
| } | ||
|
|
||
| if (candidate.message.includes("duplicate key value")) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| if (candidate.cause?.code === "23505") { | ||
| return true; | ||
| } | ||
|
|
||
| return ( | ||
| typeof candidate.cause?.message === "string" && | ||
| candidate.cause.message.includes("duplicate key value") | ||
| ); |
There was a problem hiding this comment.
Over-broad conflict detection: any Postgres unique violation (code === "23505") or "duplicate key value" message gets mapped to endpoint conflict. This will misreport unrelated unique constraint failures (e.g., on different tables or indexes) as ERROR_CODES.CONFLICT with the wrong user message.
Check for the specific PROVIDER_ENDPOINT_CONFLICT code first, or verify the constraint name matches uniq_provider_endpoints_vendor_type_url before mapping to CONFLICT.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/provider-endpoints.ts
Line: 144:165
Comment:
Over-broad conflict detection: any Postgres unique violation (`code === "23505"`) or `"duplicate key value"` message gets mapped to endpoint conflict. This will misreport unrelated unique constraint failures (e.g., on different tables or indexes) as `ERROR_CODES.CONFLICT` with the wrong user message.
Check for the specific `PROVIDER_ENDPOINT_CONFLICT` code first, or verify the constraint name matches `uniq_provider_endpoints_vendor_type_url` before mapping to CONFLICT.
How can I resolve this? If you propose a fix, please make it concise.| vi.resetModules(); | ||
|
|
||
| const duplicateKeyError = Object.assign( | ||
| new Error("duplicate key value violates unique constraint"), | ||
| { | ||
| code: "23505", |
There was a problem hiding this comment.
Mock cleanup missing: vi.doMock("@/drizzle/db", ...) without vi.doUnmock() or vi.restoreAllMocks() in afterEach can leak the mocked module to other tests that import @/drizzle/db, especially if test ordering changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/repository/provider-endpoint-742-direct-edit.test.ts
Line: 5:10
Comment:
Mock cleanup missing: `vi.doMock("@/drizzle/db", ...)` without `vi.doUnmock()` or `vi.restoreAllMocks()` in `afterEach` can leak the mocked module to other tests that import `@/drizzle/db`, especially if test ordering changes.
How can I resolve this? If you propose a fix, please make it concise.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
| if (res.ok) { | ||
| toast.success(t("endpointUpdateSuccess")); | ||
| setOpen(false); | ||
| queryClient.invalidateQueries({ queryKey: ["provider-endpoints"] }); | ||
| } else { | ||
| toast.error( | ||
| res.errorCode | ||
| ? getErrorMessage(tErrors, res.errorCode, res.errorParams) | ||
| : t("endpointUpdateFailed") | ||
| ); | ||
| queryClient.invalidateQueries({ queryKey: ["provider-endpoints"] }).catch(() => undefined); |
There was a problem hiding this comment.
Unhandled rejection from invalidation
In EditEndpointDialog, invalidateQueries(...).catch(...) is used, but other invalidateQueries calls in this same file (probe/delete/toggle success handlers) are still fire-and-forget without awaiting/voiding/catching. If invalidateQueries rejects (e.g., cancelled query function throwing), this can surface as an unhandled promise rejection at runtime.
Also appears at src/app/[locale]/settings/providers/_components/provider-endpoints-table.tsx:206,229-230,250.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/provider-endpoints-table.tsx
Line: 597:600
Comment:
**Unhandled rejection from invalidation**
In `EditEndpointDialog`, `invalidateQueries(...).catch(...)` is used, but other `invalidateQueries` calls in this same file (probe/delete/toggle success handlers) are still fire-and-forget without awaiting/voiding/catching. If `invalidateQueries` rejects (e.g., cancelled query function throwing), this can surface as an unhandled promise rejection at runtime.
Also appears at `src/app/[locale]/settings/providers/_components/provider-endpoints-table.tsx:206,229-230,250`.
How can I resolve this? If you propose a fix, please make it concise.| if (options === undefined) { | ||
| return { | ||
| inserted: report.inserted, | ||
| uniqueCandidates: report.uniqueCandidates, | ||
| skippedInvalid: report.skippedInvalid, | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Overload return shape bug
backfillProviderEndpointsFromProviders() returns the summary shape only when options === undefined, but the declared overload says “calling with an options object returns the full report.” Passing an empty object (e.g. {}) will currently return the full report, but passing { mode: undefined } / other fields explicitly undefined will also return the full report, which can break callers that treat “options present but effectively empty” as the legacy call. If you intend “no options or empty options” to keep the legacy shape, the runtime check needs to match that contract.
Location: src/repository/provider-endpoints.ts:1724-1732.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/provider-endpoints.ts
Line: 1724:1731
Comment:
**Overload return shape bug**
`backfillProviderEndpointsFromProviders()` returns the *summary shape* only when `options === undefined`, but the declared overload says “calling with an options object returns the full report.” Passing an empty object (e.g. `{}`) will currently return the full report, but passing `{ mode: undefined }` / other fields explicitly undefined will also return the full report, which can break callers that treat “options present but effectively empty” as the legacy call. If you intend “no options or empty options” to keep the legacy shape, the runtime check needs to match that contract.
Location: `src/repository/provider-endpoints.ts:1724-1732`.
How can I resolve this? If you propose a fix, please make it concise.| }); | ||
|
|
||
| it("editProviderEndpoint: conflict maps to CONFLICT errorCode", async () => { | ||
| getSessionMock.mockResolvedValue({ user: { role: "admin" } }); | ||
| updateProviderEndpointMock.mockRejectedValue( | ||
| Object.assign(new Error("[ProviderEndpointEdit] endpoint conflict"), { | ||
| code: "PROVIDER_ENDPOINT_CONFLICT", | ||
| }) |
There was a problem hiding this comment.
Mock state leaks between tests
New tests introduce updateProviderEndpointMock but don’t reset its call history/implementations between test cases. Since the module is mocked once at file scope, earlier .mockRejectedValue / .mockResolvedValue can affect later tests depending on ordering.
Add an afterEach(() => vi.clearAllMocks()) (or at least reset updateProviderEndpointMock) to keep tests isolated.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/actions/provider-endpoints.test.ts
Line: 141:148
Comment:
**Mock state leaks between tests**
New tests introduce `updateProviderEndpointMock` but don’t reset its call history/implementations between test cases. Since the module is mocked once at file scope, earlier `.mockRejectedValue` / `.mockResolvedValue` can affect later tests depending on ordering.
Add an `afterEach(() => vi.clearAllMocks())` (or at least reset `updateProviderEndpointMock`) to keep tests isolated.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Add it to the include list or broaden the include pattern to cover new integration tests. Prompt To Fix With AIThis is a comment left during a code review.
Path: vitest.integration.config.ts
Line: 21:32
Comment:
**New integration test not included**
`tests/integration/provider-endpoint-index-and-repair.test.ts` was added in this PR, but it isn’t listed in `vitest.integration.config.ts`’s `test.include`. As a result, it won’t run in the integration suite even when `DSN` is set, which defeats the purpose of guarding the new partial-index/repair behavior.
Add it to the include list or broaden the include pattern to cover new integration tests.
How can I resolve this? If you propose a fix, please make it concise. |
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Fixes deterministic endpoint edit outcomes, prevents unintended endpoint soft-deletion during provider sync, adds a partial unique index on
provider_endpointsto exclude soft-deleted rows, and introduces a dry-run repair/diagnostic tool for affected data.Problem
When editing a provider endpoint URL in the settings UI, three regressions were observed (reported in #742):
duplicate key value) that was swallowed by a generic error handler.syncProviderEndpointOnProviderEditlogic.0:...and1:...), which is actually the expected React Flight protocol but was undocumented.Root cause: The unique index
uniq_provider_endpoints_vendor_type_urlincluded soft-deleted rows, so reviving or re-creating an endpoint for the same(vendor_id, provider_type, url)tuple would conflict with a zombie row. Additionally, the sync logic was too aggressive in soft-deleting sibling endpoints, and the UI error path could show both a success and error toast simultaneously.Related Issues:
provider_endpointstable accumulating stale records (the partial index and repair tool further address this)Solution
1. Partial unique index (migration 0064)
Replace the full unique index on
(vendor_id, provider_type, url)with a partial unique index that only covers rows wheredeleted_at IS NULL. This allows soft-deleted rows to coexist without blocking active endpoint operations.2. Deterministic endpoint edit with read-after-write verification
updateProviderEndpoint()now:PROVIDER_ENDPOINT_CONFLICTerror3. Conservative sync strategy
syncProviderEndpointOnProviderEdit()now preserves sibling endpoints whenkeepPreviousWhenReferencedis true. Two new code paths returnkept-previous-and-created-next/kept-previous-and-revived-next/kept-previous-and-kept-nextinstead of soft-deleting the previous endpoint.4. UI toast fix
Restructured the success/error branching in
EditEndpointDialogandAddEndpointButtonto use early-return on success, ensuring only one toast is ever displayed. Addedvoidprefix oninvalidateQueriespromises to suppress unhandled promise warnings.5. Conflict error surfacing
New
isDirectEndpointEditConflictError()detector in the actions layer maps unique violation errors to a user-facingCONFLICTerror code with a clear message.6. Error rules sync fix
syncDefaultErrorRules()now correctly persistsoverrideResponseandoverrideStatusCodefields that were previously dropped during upsert.7. Repair & diagnostic tooling
backfillProviderEndpointsFromProviders()is enhanced with:dry-run/applymodesdeterministic-safe-insert,historical-ambiguous-report-only,invalid-provider-rowChanges
Core Changes
src/drizzle/schema.ts.where(deleted_at IS NULL)clause to unique indexdrizzle/0064_stale_vertigo.sqlsrc/repository/provider-endpoints.tssrc/actions/provider-endpoints.tsCONFLICTerror codesrc/repository/error-rules.tsoverrideResponse/overrideStatusCodein default error rulesUI Changes
src/app/[locale]/settings/providers/_components/provider-endpoints-table.tsxsrc/app/[locale]/settings/providers/_components/forms/provider-form/index.tsxonSuccesscallback in try-catch, void promise prefixesDocumentation
docs/troubleshooting/provider-endpoint-742.mdTests
tests/unit/repository/provider-endpoint-742-direct-edit.test.tstests/unit/actions/provider-endpoints.test.tstests/unit/settings/providers/provider-endpoints-table.test.tsxtests/unit/repository/provider-endpoint-sync-helper.test.tstests/unit/repository/provider-endpoints.test.tstests/integration/provider-endpoint-regression-742.test.tstests/integration/provider-endpoint-index-and-repair.test.tstests/integration/provider-endpoint-sync-race.test.tsConfiguration
vitest.integration.config.tsBreaking Changes
The
backfillProviderEndpointsFromProviders()function signature now uses overloads: calling without arguments returns the original summary shape, but calling with an options object returns the full report. Existing callers with no arguments are unaffected.Database migration required: Migration 0064 drops and recreates the unique index as a partial index. This is a non-destructive DDL change but requires
bun run db:migrateorAUTO_MIGRATE=true.Testing
Automated Tests
Manual Testing
bunx vitest run tests/integration/provider-endpoint-index-and-repair.test.ts -t "dry-run"to verify repair diagnosticsChecklist
bun run db:generateDescription enhanced by Claude AI
Greptile Overview
Greptile Summary
This PR fixes provider endpoint edit regressions by (1) changing the
provider_endpointsuniqueness constraint to exclude soft-deleted rows, (2) makingupdateProviderEndpoint()deterministic via expected-field snapshots + read-after-write verification + typed conflict errors, and (3) makingsyncProviderEndpointOnProviderEdit()more conservative to avoid soft-deleting unrelated sibling endpoints during provider edits. It also updates UI flows to avoid showing both success and error toasts on endpoint add/edit, persists additional fields during default error-rule sync, and adds integration/unit coverage plus a dry-run/apply diagnostic backfill tool.Key areas affected: database uniqueness semantics (
drizzle/0065_*,src/drizzle/schema.ts), endpoint repository logic (src/repository/provider-endpoints.ts), server action error mapping (src/actions/provider-endpoints.ts), and settings UI cache invalidation/toast behavior (src/app/[locale]/settings/providers/_components/provider-endpoints-table.tsx).Confidence Score: 2/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as Settings UI participant Action as Server Action participant Repo as Repository participant DB as Postgres UI->>Action: editProviderEndpoint({endpointId, fields...}) Action->>Repo: updateProviderEndpoint(endpointId, payload) Repo->>DB: UPDATE provider_endpoints ... RETURNING alt Update applied and matches expected DB-->>Repo: updated row Repo-->>Action: ProviderEndpoint Action-->>UI: {ok:true, endpoint} UI->>UI: toast.success + close dialog UI->>UI: invalidateQueries(["provider-endpoints", ...]) else Unique violation (23505) DB-->>Repo: error 23505 Repo->>DB: SELECT endpoint by id (read-after-write) alt Row now matches expected (idempotent) DB-->>Repo: matching row Repo-->>Action: ProviderEndpoint Action-->>UI: {ok:true, endpoint} else Row does not match Repo-->>Action: throw {code: PROVIDER_ENDPOINT_CONFLICT} Action-->>UI: {ok:false, errorCode: CONFLICT} UI->>UI: toast.error(conflict) end else Write/read inconsistency Repo-->>Action: throw {code: PROVIDER_ENDPOINT_WRITE_READ_INCONSISTENCY} Action-->>UI: {ok:false, errorCode: UPDATE_FAILED} end Note over Repo,DB: Migration 0065 makes unique index partial where deleted_at IS NULL