Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a configurable error-message replacement system: new GORM model and migration, thread-safe cached rules with CRUD API and admin UI, startup provider registration, and runtime integration that rewrites API error messages via rule matching. Changes
Sequence Diagram(s)sequenceDiagram
participant AdminUI as "Admin UI"
participant API as "Server API (Gin)"
participant Model as "Model / DB"
participant Cache as "Rules Cache"
participant Common as "common.ReplaceLogic"
participant Client as "Client / Error Path"
AdminUI->>API: GET /api/custom_error_rule/
API->>Model: GetAllCustomErrorRules()
Model-->>API: rules
API-->>AdminUI: JSON list
AdminUI->>API: POST/PUT/DELETE /api/custom_error_rule/
API->>Model: Create/Update/Delete rule
Model->>Cache: refreshCustomErrorRulesCache()
Cache-->>Common: provider returns updated replacements
Client->>Common: ReplaceCustomErrorMessage(statusCode, message)
Common-->>Client: replacedMessage
Client-->>Consumer: final transformed error message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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. 🎉 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model/main.go (1)
296-376:⚠️ Potential issue | 🔴 CriticalMove cache initialization from dead code function to the active migration path.
The
InitCustomErrorRulesCache()andCustomErrorRulesProviderregistration (lines 363–375) are located inmigrateDBFast(), which is never called—InitDB()calls onlymigrateDB()(line 205). As a result, theCustomErrorRuletable is created (line 279), but the cache is never initialized at startup, leavingReplaceCustomErrorMessage()non-functional.Move the cache initialization block (lines 363–375) from
migrateDBFast()intomigrateDB()after theAutoMigratecalls, or remove the deadmigrateDBFast()function entirely if it is not planned for use.
🤖 Fix all issues with AI agents
In `@common/str.go`:
- Around line 192-206: ReplaceCustomErrorMessage currently treats rule.Contains
== "" as a match because strings.Contains(message, "") is always true; update
the function (which calls CustomErrorRulesProvider and iterates rules) to skip
any rule whose rule.Contains is empty (or only whitespace) before calling
strings.Contains, preserving existing statusCode checks and returning
rule.NewMessage only for non-empty Contains matches.
In `@controller/custom_error_rule.go`:
- Around line 56-83: UpdateCustomErrorRule is missing validation for the
CustomErrorRule.Contains field, allowing an admin to set it to empty which would
match every message; add the same non-empty check used in CreateCustomErrorRule
to reject rule.Contains == "" (or trimmed empty) and return a JSON error
response (e.g., "Contains不能为空") before calling model.UpdateCustomErrorRule,
referencing UpdateCustomErrorRule, model.CustomErrorRule and
CreateCustomErrorRule to mirror behavior.
In `@model/custom_error_rule.go`:
- Around line 113-122: The count check in InitCustomErrorRulesCache uses
DB.Model(&CustomErrorRule{}).Count(&count) which ignores soft-deleted rows and
causes reseeding when all rows are soft-deleted; change the count query to
include soft-deleted records by using
DB.Unscoped().Model(&CustomErrorRule{}).Count(&count) so seedDefaultErrorRules()
only runs when the physical table is empty, keeping calls to
seedDefaultErrorRules() and refreshCustomErrorRulesCache() intact.
- Around line 11-21: The Enabled field on the CustomErrorRule struct uses a
numeric default (`default:1`); change its GORM tag to use a boolean literal
(`default:true`) to match other models and ensure cross-database
compatibility—update the struct field `Enabled bool` tag in CustomErrorRule
(reference symbol: CustomErrorRule.Enabled) to `gorm:"not null;default:true"`.
In `@web/src/pages/Setting/Operation/SettingsCustomErrorRules.jsx`:
- Around line 244-326: The SideSheet keeps children mounted so the Form's
initValues (in the Form component) aren't reapplied when switching editing
targets; to fix, force the Form to remount by adding a key that changes with the
editing target (e.g., key={editingRule?.id ?? 'new'} on the Form) or enable
SideSheet's destroy-on-close behavior (destroyOnClose) and also clear formRef
(formRef.current = null) when opening a different rule so the Form always
initializes from initValues for the current editingRule.
🧹 Nitpick comments (3)
model/main.go (1)
362-375: Provider closure allocates a new slice on every call — hot path concern.
CustomErrorRulesProvideris invoked on every error response (viaReplaceCustomErrorMessageinToOpenAIError/ToClaudeError). The closure allocates a new[]common.CustomErrorReplacementslice each time, copying every cached rule. Since the cache is only refreshed on CRUD operations, consider caching the[]common.CustomErrorReplacementrepresentation itself to avoid per-request allocations.♻️ Suggested approach: cache the mapped slice
Store the
[]common.CustomErrorReplacementalongside the[]CustomErrorRulecache, rebuilding it only inrefreshCustomErrorRulesCache. The provider then just returns the pre-built slice:common.CustomErrorRulesProvider = func() []common.CustomErrorReplacement { - rules := GetCachedCustomErrorRules() - result := make([]common.CustomErrorReplacement, len(rules)) - for i, r := range rules { - result[i] = common.CustomErrorReplacement{ - Contains: r.Contains, - StatusCode: r.StatusCode, - NewMessage: r.NewMessage, - } - } - return result + return GetCachedCustomErrorReplacements() }Where
GetCachedCustomErrorReplacements()returns a pre-built[]common.CustomErrorReplacementthat is updated atomically alongside the rule cache inrefreshCustomErrorRulesCache.model/custom_error_rule.go (1)
34-50: Benign thundering-herd on uninitialized cache.Between the
RUnlock()on Line 42 andrefreshCustomErrorRulesCache()on Line 45, multiple concurrent goroutines can all observe!customErrorRulesCacheInitand trigger redundant DB queries. This is not a correctness issue sincerefreshCustomErrorRulesCacheserializes viaWLock, but it could cause a brief burst of identical queries on first access under load.web/src/pages/Setting/Operation/SettingsCustomErrorRules.jsx (1)
82-96: Toggle sends the full rule object back — acceptable but fragile.
handleToggleEnabledspreads the entireruleobject (including server-managed fields likecreated_time,updated_time) into the PUT body. This works because the backend'sUpdateCustomErrorRuleuses an explicit field map inUpdates, so extra fields are ignored. Just noting for awareness.
TAYOTOoO
force-pushed
the
feature/custom-error-rules
branch
2 times, most recently
from
8cfd554 to
e71a4d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@model/main.go`:
- Line 279: migrateDBFast runs concurrent migrations but omits
&CustomErrorRule{}, so the table may not be created when using the fast path;
update the migrations slice in migrateDBFast to include &CustomErrorRule{} (same
as in migrateDB) so both migration paths create the same set of models—locate
the migrations list inside the migrateDBFast function and add &CustomErrorRule{}
to that slice.
🧹 Nitpick comments (1)
web/src/pages/Setting/Operation/SettingsCustomErrorRules.jsx (1)
108-137: Sharedloadingstate causes table spinner during form submission.
handleSubmitsetssetLoading(true)(Line 109), which also triggers the<Spin spinning={loading}>wrapping the entire table (Line 214). Consider using a separatesubmittingstate for the form to avoid the table showing a loading overlay while submitting.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@controller/custom_error_rule.go`:
- Around line 75-85: The update path currently calls
model.UpdateCustomErrorRule(&rule) but GORM's Updates() can succeed with zero
rows affected; change the logic so non-existent IDs are detected: either (A)
modify model.UpdateCustomErrorRule to inspect the GORM result.RowsAffected and
return an error (e.g., gorm.ErrRecordNotFound or a custom error) when
RowsAffected == 0, or (B) fetch the rule first (e.g.,
model.GetCustomErrorRuleByID or similar) and return a not-found error before
calling model.UpdateCustomErrorRule; then update the controller code that calls
model.UpdateCustomErrorRule to treat that not-found error as a failure and avoid
triggering the cache refresh when the rule doesn't exist.
In `@model/custom_error_rule.go`:
- Around line 128-139: The seeding function seedDefaultErrorRules currently
ignores the result of DB.Create(&sample); capture the returned result (e.g., res
:= DB.Create(&sample)) and check res.Error; if non-nil, log the error with
contextual info (including the sample data) and return the error up to the
caller (change seedDefaultErrorRules to return error) so
refreshCustomErrorRulesCache can handle failures instead of proceeding with an
empty cache; update callers of seedDefaultErrorRules (e.g.,
refreshCustomErrorRulesCache) to handle the returned error and act accordingly.
🧹 Nitpick comments (1)
model/custom_error_rule.go (1)
38-53: Lazy init has a benign thundering-herd: multiple goroutines may callrefreshCustomErrorRulesCacheconcurrently on first access.Between the
RUnlockon line 45 and therefreshCustomErrorRulesCachecall on line 48, other goroutines can also observe!customErrorRulesCacheInitand trigger redundant DB queries. This is safe (each refresh atomically replaces the cache), but wasteful under high concurrency at startup.A
sync.Oncewould be a cleaner approach for the lazy init path:Suggested refactor
var ( customErrorRulesCache []CustomErrorRule customErrorRulesCacheLock sync.RWMutex - customErrorRulesCacheInit bool + customErrorRulesCacheOnce sync.Once ) func GetCachedCustomErrorRules() []CustomErrorRule { + customErrorRulesCacheOnce.Do(func() { + refreshCustomErrorRulesCache() + }) customErrorRulesCacheLock.RLock() - if customErrorRulesCacheInit { - rules := customErrorRulesCache - customErrorRulesCacheLock.RUnlock() - return rules - } - customErrorRulesCacheLock.RUnlock() - - // Initialize cache - refreshCustomErrorRulesCache() - - customErrorRulesCacheLock.RLock() defer customErrorRulesCacheLock.RUnlock() return customErrorRulesCache }Note:
refreshCustomErrorRulesCacheis also called from CRUD operations, so theRWMutexis still needed.sync.Onceonly guards the lazy-init path.
TAYOTOoO
force-pushed
the
feature/custom-error-rules
branch
from
e71a4d4 to
bbdc293
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@model/custom_error_rule.go`:
- Around line 20-21: The gorm struct tags on CreatedTime and UpdatedTime use
gorm:"bigint" which GORM v2 ignores; update the struct field tags for
CreatedTime and UpdatedTime to use the explicit type key (gorm:"type:bigint") so
GORM recognizes the column type; locate the CreatedTime and UpdatedTime fields
in the struct and replace their gorm tag values accordingly.
🧹 Nitpick comments (3)
web/src/pages/Setting/Operation/SettingsCustomErrorRules.jsx (3)
43-43: Sharedloadingstate causes the table spinner to activate during form submission.A single
loadingstate controls both the<Spin>wrapping the table (Line 214) and the submit button'sloadingprop (Line 320). Submitting the form will overlay the table with a spinner unnecessarily. Consider a separatesubmittingstate for the form.Proposed fix
const [loading, setLoading] = useState(false); + const [submitting, setSubmitting] = useState(false);In
handleSubmit:const handleSubmit = async (values) => { - setLoading(true); + setSubmitting(true); ... - setLoading(false); + setSubmitting(false); };On the submit button:
- loading={loading} + loading={submitting}Also applies to: 108-137
64-66: Missing dependency inuseEffect.
loadRulesis not listed in the dependency array, which will trigger an ESLintreact-hooks/exhaustive-depswarning. SinceloadRulesis stable in behavior (only depends on state setters andt), the simplest fix is to either wraploadRulesinuseCallbackor inline the fetch inside the effect.
82-96: Toggle sends all server-side fields back, including soft-delete metadata.
...rulespreads every property from the API response (includingcreated_time,deleted_at, etc.) into the PUT payload. This works today because the backend'sUpdateCustomErrorRuleuses a field map, but it couples the frontend to the exact shape of the response and sends unnecessary data. Consider sending only the fields the update endpoint needs.
支持通过后台管理界面增删改查错误消息替换规则，取代硬编码方式。 - 新增 CustomErrorRule 模型，带内存缓存和自动刷新 - 新增 CRUD 接口 /api/custom_error_rule/（管理员权限） - common/str.go 新增 CustomErrorRulesProvider 和 ReplaceCustomErrorMessage - types/error.go 在 ToOpenAIError/ToClaudeError 中调用替换逻辑 - model/main.go 启动时初始化缓存并注册 Provider - 首次启动时自动插入一条示例规则 - 新增前端管理页面 SettingsCustomErrorRules.jsx Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TAYOTOoO
force-pushed
the
feature/custom-error-rules
branch
from
bbdc293 to
f3451b3
Compare
1 participant
支持通过后台管理界面增删改查错误消息替换规则，取代硬编码方式。
Summary by CodeRabbit