Conversation
改进错误规则同步逻辑，避免重复插入导致的冲突： - 使用 upsert 策略替代删除重建 - 保留用户自定义规则不被覆盖 - 清理过期的默认规则
Summary of ChangesHello @sususu98, 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! 此拉取请求主要解决了点击同步错误规则时遇到的问题，并显著增强了底层的同步机制。它将同步方法从破坏性的“删除重建”转变为更健壮的“用户优先”更新策略。这确保了用户自定义规则在同步过程中得以保留，同时默认规则能够按需更新或插入，并且过时的默认规则会被自动移除，从而实现了一个更稳定、更用户友好的规则管理系统。 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
![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
此拉取请求修复了同步错误规则时的错误，并对同步逻辑进行了重要改进。旧的“先删后建”策略被替换为更智能的“更新插入”（upsert）策略，此举不仅能保留用户的自定义规则不被覆盖，还能清理已过期的默认规则，显著提升了功能和用户体验。相关更改清晰地体现在了初始化脚本、服务器动作和启动挂钩中，保持了代码的一致性。
我的审查意见主要集中在新同步逻辑的性能优化上。我发现有两处在循环中执行数据库操作（N+1查询问题），这可能导致性能下降。我建议通过批量处理删除和插入操作，并使用 Promise.all 并行执行更新，来提升执行效率，这对于未来默认规则数量增加的场景尤其重要。
总体而言，这是一个高质量的拉取请求，大大改善了核心功能。采纳我提出的优化建议将使实现更加健壮和可扩展。
| // 删除不再存在于 DEFAULT_ERROR_RULES 中的默认规则 | ||
| for (const rule of allDefaultRulesInDb) { | ||
| if (!defaultPatternSet.has(rule.pattern)) { | ||
| await tx.delete(errorRules).where(eq(errorRules.id, rule.id)); | ||
| counters.deleted += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
为了提升性能，建议将循环内的删除操作批量化。当前实现在循环中逐条删除过期的规则，这会导致多次数据库请求（N+1问题）。
您可以先收集所有需要删除的规则ID，然后使用 inArray 操作符在一次查询中将它们全部删除。这样可以显著减少数据库的负载，尤其是在有过时规则需要清理时。
| // 删除不再存在于 DEFAULT_ERROR_RULES 中的默认规则 | |
| for (const rule of allDefaultRulesInDb) { | |
| if (!defaultPatternSet.has(rule.pattern)) { | |
| await tx.delete(errorRules).where(eq(errorRules.id, rule.id)); | |
| counters.deleted += 1; | |
| } | |
| } | |
| // 删除不再存在于 DEFAULT_ERROR_RULES 中的默认规则 | |
| const obsoleteRuleIds = allDefaultRulesInDb | |
| .filter((rule) => !defaultPatternSet.has(rule.pattern)) | |
| .map((rule) => rule.id); | |
| if (obsoleteRuleIds.length > 0) { | |
| await tx.delete(errorRules).where(inArray(errorRules.id, obsoleteRuleIds)); | |
| counters.deleted = obsoleteRuleIds.length; | |
| } |
| for (const rule of DEFAULT_ERROR_RULES) { | ||
| await tx.insert(errorRules).values(rule); | ||
| const existingIsDefault = existingMap.get(rule.pattern); | ||
|
|
||
| if (existingIsDefault === undefined) { | ||
| // pattern 不存在，直接插入 | ||
| await tx.insert(errorRules).values(rule); | ||
| counters.inserted += 1; | ||
| continue; | ||
| } | ||
|
|
||
| if (existingIsDefault === true) { | ||
| // pattern 存在且是默认规则，更新它 | ||
| await tx | ||
| .update(errorRules) | ||
| .set({ | ||
| matchType: rule.matchType, | ||
| category: rule.category, | ||
| description: rule.description, | ||
| isEnabled: rule.isEnabled, | ||
| isDefault: true, | ||
| priority: rule.priority, | ||
| updatedAt: new Date(), | ||
| }) | ||
| .where(eq(errorRules.pattern, rule.pattern)); | ||
| counters.updated += 1; | ||
| continue; | ||
| } | ||
|
|
||
| // pattern 存在但已被用户自定义（isDefault=false），跳过 | ||
| counters.skipped += 1; | ||
| } |
There was a problem hiding this comment.
这段代码在循环中执行插入和更新操作，同样存在N+1查询问题，当默认规则数量增多时可能会影响性能。
建议进行优化：
- 将需要插入和更新的规则分别收集到两个数组中。
- 使用Drizzle的批量插入功能一次性插入所有新规则。
- 使用
Promise.all并行执行所有更新操作。
这样可以将多次数据库往返合并为少数几次，从而提高同步效率。
const rulesToInsert: (typeof DEFAULT_ERROR_RULES)[number][] = [];
const rulesToUpdate: (typeof DEFAULT_ERROR_RULES)[number][] = [];
for (const rule of DEFAULT_ERROR_RULES) {
const existingIsDefault = existingMap.get(rule.pattern);
if (existingIsDefault === undefined) {
// pattern 不存在，准备插入
rulesToInsert.push(rule);
} else if (existingIsDefault === true) {
// pattern 存在且是默认规则，准备更新
rulesToUpdate.push(rule);
} else {
// pattern 存在但已被用户自定义（isDefault=false），跳过
counters.skipped += 1;
}
}
// 批量插入新规则
if (rulesToInsert.length > 0) {
await tx.insert(errorRules).values(rulesToInsert);
counters.inserted = rulesToInsert.length;
}
// 并行更新现有规则
if (rulesToUpdate.length > 0) {
await Promise.all(
rulesToUpdate.map((rule) =>
tx
.update(errorRules)
.set({
matchType: rule.matchType,
category: rule.category,
description: rule.description,
isEnabled: rule.isEnabled,
isDefault: true,
priority: rule.priority,
updatedAt: new Date(),
})
.where(eq(errorRules.pattern, rule.pattern))
)
);
counters.updated = rulesToUpdate.length;
}![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
This PR successfully refactors the error rule synchronization logic from a destructive delete-and-recreate approach to a more intelligent upsert strategy that preserves user customizations. The implementation is solid and the logic is correct.
PR Size: S
- Lines changed: 168 (136 additions, 32 deletions)
- Files changed: 5
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 1 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 1 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
High Priority Issues (Should Fix)
-
Transaction Error Handling (src/repository/error-rules.ts:476-548): Database transaction lacks explicit error handling. If any query fails mid-transaction, the error propagates without detailed context logging, making debugging difficult in production.
-
Test Coverage (Missing): The critical sync logic now has three code paths (insert/update/skip) plus a deletion path, but no unit tests verify this behavior. This is a regression risk for future changes.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - One issue identified
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Missing tests
- Code clarity - Good
Automated review by Claude AI
Detailed Review Findings1. Transaction Error Handling (High Priority)File: Issue: The Why this matters: In production environments, when this transaction fails (e.g., due to constraint violations, network issues, or data inconsistencies), the generic error makes it extremely difficult to diagnose the root cause. Operations teams need to know:
Suggested fix: export async function syncDefaultErrorRules(): Promise<{
inserted: number;
updated: number;
skipped: number;
deleted: number;
}> {
const counters = { inserted: 0, updated: 0, skipped: 0, deleted: 0 };
try {
await db.transaction(async (tx) => {
// ... existing transaction logic ...
});
} catch (error) {
logger.error('[ErrorRulesRepository] syncDefaultErrorRules transaction failed', {
error,
counters, // Shows progress before failure
totalDefaultRules: DEFAULT_ERROR_RULES.length,
});
throw error; // Re-throw to let caller handle
}
return counters;
}2. Missing Test Coverage (High Priority)Scope: New Issue: The refactored
Why this matters:
Suggested test cases: describe('syncDefaultErrorRules', () => {
it('should insert new patterns that do not exist', async () => {
// Test path #1
});
it('should update existing default rules (isDefault=true)', async () => {
// Test path #2
});
it('should skip user-customized rules (isDefault=false)', async () => {
// Test path #3 - CRITICAL: validates PR promise
});
it('should delete obsolete default rules no longer in DEFAULT_ERROR_RULES', async () => {
// Test path #4 - NEW FUNCTIONALITY
});
it('should handle empty DEFAULT_ERROR_RULES array', async () => {
// Edge case
});
it('should rollback transaction on error', async () => {
// Error handling validation
});
it('should return accurate counters', async () => {
// Validates the new return type
});
});Validation ChecklistI performed full-context validation before reporting these issues: Transaction Error Handling:
Test Coverage:
Both issues passed the 80-point confidence threshold and validation checks. |
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
Summary
Problem
在点击错误规则页面的"刷新缓存"按钮时，由于采用"删除所有默认规则 + 重新插入"的策略，可能导致重复插入冲突错误。此外，这种策略无法优雅处理用户对默认规则的自定义修改。
Related PRs:
Solution
改用智能 upsert 策略（"用户自定义优先"）：
Changes
Core Changes
src/repository/error-rules.ts: 重构syncDefaultErrorRules()，使用批量查询 + upsert 逻辑，返回详细统计（inserted/updated/skipped/deleted）src/actions/error-rules.ts: 更新refreshCacheAction()返回类型，包含syncResult统计信息src/instrumentation.ts: 启动时同步逻辑适配新的返回格式Supporting Changes
scripts/init-error-rules.ts: 更新脚本日志输出格式CHANGELOG.md: 更新 v0.3.26 变更日志Test plan
Description enhanced by Claude AI - Added related PR context and detailed solution explanation