feat: add group monitoring module#2951
Conversation
Add a group-level health monitoring system that periodically tests channels per group, records latency/availability, and provides admin config + user status pages with trend charts. New module files in modules/group_monitor/ and web/src/modules/group-monitor/ with minimal changes to core files (~50 lines across 8 files).
WalkthroughThis PR introduces a comprehensive group monitoring system that enables tracking channel health across logical groups. It includes backend services for periodic health checks, database models for storing monitoring logs and configurations, HTTP APIs for data retrieval and management, and frontend UI components for admins to configure monitoring and for users to view group status and latency trends. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Scheduler
participant Service as AutomaticallyGroupMonitor
participant DB as Database
participant Config as GroupMonitorConfig
participant Test as TestChannelForMonitor
participant Log as CreateGroupMonitorLog
Scheduler->>Service: Trigger background loop
Service->>DB: GetEnabledGroupMonitorConfigs()
DB-->>Service: [config1, config2, ...]
loop For each config
Service->>Config: Resolve testModel & validate channel
Config-->>Service: actualModel, channel
Service->>Test: TestChannelForMonitor(channel, model)
Test-->>Service: MonitorTestResult{latency, success, error}
Service->>Log: CreateGroupMonitorLog(GroupMonitorLog)
Log->>DB: INSERT INTO group_monitor_logs
end
Service->>DB: CleanupGroupMonitorLogs(retainDays)
DB-->>Service: Cleanup complete
sequenceDiagram
participant User as User/Admin
participant API as API Handler
participant Service as Database Service
participant DB as GORM/Database
User->>API: GET /group/monitor/status
API->>Service: GetGroupMonitorLatest()
Service->>DB: Query latest logs per group
DB-->>Service: [GroupMonitorLog, ...]
API->>Service: GetGroupMonitorStats(startTs, endTs)
Service->>DB: Query aggregated stats
DB-->>Service: [GroupMonitorStat, ...]
API-->>User: Compose & return GroupStatus[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@main.go`:
- Around line 304-306: The call to group_monitor.Migrate() currently ignores its
returned error; capture its error and handle it the same way as the other
migrations (e.g., if err := group_monitor.Migrate(); err != nil { return
fmt.Errorf("group_monitor.Migrate: %w", err) } or log/exit consistently) so
failures are propagated before starting the background monitor goroutine (which
queries those tables) and prevent runtime panics; update the invocation in
main.go to check the error and propagate or fail fast (matching the existing
error handling pattern in this function).
In `@modules/group_monitor/model.go`:
- Around line 74-93: GetGroupMonitorLatest currently swallows all errors from
model.DB.Where(...).Order(...).First(&log).Error by doing continue; update this
loop to only ignore gorm.ErrRecordNotFound and to handle any other error by
returning it (or collecting and returning a combined error) so callers aren’t
given partial results on DB failures; modify the error check in the per-group
loop inside GetGroupMonitorLatest to compare against gorm.ErrRecordNotFound (or
use errors.Is) and continue only in that case, otherwise propagate or accumulate
the error and stop.
- Around line 163-179: SaveGroupMonitorConfig currently does a separate First
then Create which creates a TOCTOU race; modify SaveGroupMonitorConfig (and the
Create path for GroupMonitorConfig) to handle duplicate-key errors by falling
back to an update (e.g., attempt model.DB.Create(cfg) and if it returns a
unique-constraint error for group_name then perform
model.DB.Model(&GroupMonitorConfig{}).Where("group_name = ?",
cfg.GroupName).Updates(...) ), or replace the upsert with GORM's
clause.OnConflict to do an atomic upsert for GroupMonitorConfig (using
Clauses(clause.OnConflict{UpdateAll: true} or a specific DoUpdates map) to avoid
the race. Ensure you reference SaveGroupMonitorConfig, GroupMonitorConfig,
Create, and Updates in the change.
- Around line 126-137: The loop currently issues an N+1 query per group to
populate SuccessCount using
model.DB.Model(&GroupMonitorLog{}).Where(...).Count(&count) and also ignores the
returned error; replace this with a single aggregated query that SELECTs
group_name, COUNT(*) AS cnt FROM group_monitor_logs WHERE success = true [AND
created_at >= startTs] [AND created_at <= endTs] GROUP BY group_name (using GORM
on model.DB with Model(&GroupMonitorLog{}) and Group("group_name")) and load
results into a temporary struct, then map counts back into the existing stats
slice by matching stat.GroupName to set stats[i].SuccessCount; also capture and
handle the error returned by the DB call (propagate or log/return) instead of
discarding it so failures no longer silently leave SuccessCount as zero.
- Around line 49-71: GetGroupMonitorLogs currently reuses the same GORM query
for Count and Find which causes SQL corruption; update the code to call Count on
a fresh session (e.g., query.Session(&gorm.Session{}) or create a copy of the
query before Count) so the subsequent query.Order(...).Find(...) is not
affected, and add the import "gorm.io/gorm" at the top of the file; ensure you
still build the filters on groupName/startTs/endTs and use the new session when
calling Count to populate total, then run Order/Offset/Limit/Find on the
original query.
In `@modules/group_monitor/router.go`:
- Around line 24-29: The user UI fails because the time-series endpoint is only
registered for admins; add the same GET "/time_series" handler to the user route
group so authenticated users can fetch it: inside the userRoute block (variable
userRoute) register userRoute.GET("/time_series",
GetGroupMonitorTimeSeriesHandler) (the handler used for admins), ensuring it
uses middleware.UserAuth(); alternatively remove the chart from the
GroupMonitorUser UI if you intentionally want it admin-only.
In `@modules/group_monitor/service.go`:
- Around line 31-34: The loop in GetGroupMonitorSetting() processing can produce
a zero sleep because interval is computed from setting.IntervalMins and may
round to 0; update the loop to enforce a sane minimum (e.g., if computed
interval <= 0 or < time.Minute then set interval = time.Minute or another chosen
MIN_INTERVAL) before calling time.Sleep; reference the variables and functions
from the diff (setting, IntervalMins, interval, GetGroupMonitorSetting) and
handle negative/zero values so the goroutine never spins with a zero-duration
sleep.
- Around line 109-127: The GroupMonitorLog.CachedModel field is never set in
testAndRecord because controller.TestChannelForMonitor returns a
MonitorTestResult with no cached-model info; update testAndRecord to populate
CachedModel (or remove the field if unused). Locate the testAndRecord function
and either: 1) read the cached model identifier from the Channel object
(channel.Model or similar) or from the service/config where channels are defined
and assign it to log.CachedModel before calling CreateGroupMonitorLog, or 2) if
CachedModel is not meaningful, remove the CachedModel field from the
GroupMonitorLog struct and related storage code; ensure changes reference
GroupMonitorLog, testAndRecord, controller.TestChannelForMonitor,
MonitorTestResult, and model.Channel so callers and storage remain consistent.
In `@web/src/modules/group-monitor/GroupMonitorAdmin.jsx`:
- Around line 93-99: The channel loader loadChannels currently requests only
page_size: 100 which truncates results; update loadChannels (and the API.get
call) to retrieve all channels by either removing the hard cap or implementing
pagination: loop calling API.get('/api/channel/') with incrementing p until
res.data.data.items is empty (accumulate items into a single array) and then
call setChannels(accumulated) — alternatively wire up a search/autocomplete UI
and query the backend for matches instead of fetching all when appropriate.
- Around line 456-464: The UI shows "-%" and "-ms" when availability or
avgLatency is the string '-' — update the JSX around the availability and
avgLatency spans in GroupMonitorAdmin.jsx to only append the '%' and 'ms'
suffixes when the values are numeric (e.g. use parseFloat/Number and isNaN
checks or a utility like isNumeric) and otherwise render the raw '-' (or a
placeholder) without the suffix; specifically change the span that renders
availability (variable availability) and the span that renders avgLatency
(variable avgLatency) to conditionally include the suffixes based on numeric
checks.
🧹 Nitpick comments (8)
web/src/modules/group-monitor/GroupMonitorAdmin.jsx (3)
40-61: Stale closure:globalSettingsdefaults reference is safe but fragile.The
useCallbackhas an empty dependency array, soglobalSettingsinside the callback always refers to the initial state object. This works correctly for reading the keys and default values, but if the default keys ever change dynamically this will silently break. Consider extracting the default settings keys/values into a module-level constant to make the intent explicit and avoid future confusion.
72-72: Redundant ternary — both branches are identical.The
typeof ... === 'boolean'check producesString(globalSettings[item.key])in both branches.Suggested fix
- const value = typeof globalSettings[item.key] === 'boolean' ? String(globalSettings[item.key]) : String(globalSettings[item.key]); + const value = String(globalSettings[item.key]);
177-185: Missing dependency array entries foruseEffect.The effect calls
loadGlobalSettings,loadGroups, etc., but the dependency array is empty. While these are stableuseCallbackrefs, the React exhaustive-deps lint rule will warn. Adding the callbacks to the array (they're stable) silences the warning and is more correct.web/src/App.jsx (1)
48-49: Consider lazy-loading these new module components.Several other pages (Home, Dashboard, About) use
lazy()imports to reduce the initial bundle size. Since group monitoring is an admin/niche feature, lazy loading would be beneficial. That said, the current pattern is consistent with other eagerly imported pages likeChannelandToken.main.go (1)
23-23: Import grouping nit.The
modules/group_monitorimport is placed between other first-party imports and third-party imports. Consider grouping it with the other first-partyQuantumNous/new-api/...imports for consistency (e.g., alphabetically aftermodel).modules/group_monitor/controller.go (2)
86-110: Consider validatingchannel_idon the server side as well.The handler validates
group_namebut doesn't check thatchannel_idis positive. The frontend validates this, but a direct API call could save a config withchannel_id: 0, which would then be skipped silently during monitoring (service.go Line 61).Suggested fix
if cfg.GroupName == "" { c.JSON(http.StatusBadRequest, gin.H{ "success": false, "message": "group_name is required", }) return } + + if cfg.ChannelId <= 0 { + c.JSON(http.StatusBadRequest, gin.H{ + "success": false, + "message": "channel_id must be positive", + }) + return + }
163-180:resultwill benil(JSONnull) when there are no groups — consider initializing the slice.If
latestis empty,resultis never appended to and remainsnil, which marshals as JSONnullrather than[]. Some frontend code may not handle this gracefully.Suggested fix
- var result []GroupStatus + result := make([]GroupStatus, 0, len(latest))modules/group_monitor/model.go (1)
186-201: No limit on time-series query results — potential OOM for wide time ranges.
GetGroupMonitorTimeSeriesreturns all matching logs in the range with no cap. If the monitoring interval is short and the time window is wide, this could return a very large result set, causing high memory usage and slow API responses.Consider adding a reasonable
Limit(e.g., 1000 or configurable) or requiring the caller to enforce bounded time ranges.
| // Migrate group monitor tables | ||
| group_monitor.Migrate() | ||
|
|
There was a problem hiding this comment.
Migration error is silently discarded.
group_monitor.Migrate() returns an error that is ignored. If the migration fails, the background monitor goroutine (Line 108) will encounter runtime errors querying non-existent tables. Other migrations in this function propagate errors.
Suggested fix
// Migrate group monitor tables
- group_monitor.Migrate()
-
+ if err = group_monitor.Migrate(); err != nil {
+ common.SysError("failed to migrate group monitor tables: " + err.Error())
+ // Non-fatal: log but don't block startup
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Migrate group monitor tables | |
| group_monitor.Migrate() | |
| // Migrate group monitor tables | |
| if err = group_monitor.Migrate(); err != nil { | |
| common.SysError("failed to migrate group monitor tables: " + err.Error()) | |
| // Non-fatal: log but don't block startup | |
| } |
🤖 Prompt for AI Agents
In `@main.go` around lines 304 - 306, The call to group_monitor.Migrate()
currently ignores its returned error; capture its error and handle it the same
way as the other migrations (e.g., if err := group_monitor.Migrate(); err != nil
{ return fmt.Errorf("group_monitor.Migrate: %w", err) } or log/exit
consistently) so failures are propagated before starting the background monitor
goroutine (which queries those tables) and prevent runtime panics; update the
invocation in main.go to check the error and propagate or fail fast (matching
the existing error handling pattern in this function).
| func GetGroupMonitorLogs(groupName string, startTs, endTs int64, startIdx, pageSize int) ([]*GroupMonitorLog, int64, error) { | ||
| var logs []*GroupMonitorLog | ||
| var total int64 | ||
|
|
||
| query := model.DB.Model(&GroupMonitorLog{}) | ||
| if groupName != "" { | ||
| query = query.Where("group_name = ?", groupName) | ||
| } | ||
| if startTs > 0 { | ||
| query = query.Where("created_at >= ?", startTs) | ||
| } | ||
| if endTs > 0 { | ||
| query = query.Where("created_at <= ?", endTs) | ||
| } | ||
|
|
||
| err := query.Count(&total).Error | ||
| if err != nil { | ||
| return nil, 0, err | ||
| } | ||
|
|
||
| err = query.Order("created_at DESC").Offset(startIdx).Limit(pageSize).Find(&logs).Error | ||
| return logs, total, err | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GORM v2 Count query mutation Find same query object
💡 Result:
In GORM v2, Count is a finisher method, and running a finisher on a *gorm.DB initializes/mutates its internal Statement, so reusing the same query object for Find can get “polluted” (e.g., leftover SELECT count(*), LIMIT/OFFSET, etc.). GORM’s own method-chaining docs explicitly warn that an initialized *gorm.DB is not safe to reuse unless you create a new session for reuse. [1]
Fix: clone the query before calling Count
Create a “base” query, then run Count on a cloned session, and Find on the original (or another clone):
base := db.Model(&User{}).
Where("status = ?", "active").
Session(&gorm.Session{}) // makes it shareable for reuse
var total int64
base.Session(&gorm.Session{Initialized: true}).Count(&total)
var users []User
base.Find(&users)This pattern (new session for the count query) is also a commonly recommended workaround in practice. [2]
Alternative (common) approach
Build the filters once (e.g., with Scopes) and apply them separately to a count query and a data query, rather than trying to reuse the same initialized *gorm.DB. [1]
Sources: GORM method chaining / “shareable session” note [1], example of new-session-for-count workaround [2].
References:
[1] https://gist.github.com/versionsix/21c66475ed5249801535e30ea7987a2b
[2] https://stackoverflow.com/questions/72666748/golang-gorm-scope-broken-after-upgrade-from-v1-to-v2
🌐 Web query:
GORM v2 Count finisher behavior documentation
💡 Result:
-
Where it’s documented that finishers execute immediately and can “pollute” a reused
*gorm.DB: GORM’s Method Chaining doc explains Finisher Methods “execute registered callbacks” (i.e., run SQL immediately) and warns that the returned*gorm.DBis not safe to reuse unless you start a new session (Session,WithContext,Debug) to avoid carrying conditions into later queries. This is the key doc for “finisher behavior” (and it applies toCountin practice, sinceCountexecutes a SQL query). [1] -
Countusage/behavior examples (includingDistinct/Groupsemantics): The Advanced Query → Count section documents howCountgenerates SQL likeSELECT count(1) ..., and how it behaves withDistinctandGroup(e.g., afterGroup("name"),Countreturns the number of groups). [2] -
V2 signature requirement: The GORM v2 release notes specify that
Countonly accepts a*int64. [3]
Sources: [1] [2] [3]
🌐 Web query:
GORM v2 query reuse Count then Find best practice
💡 Result:
In GORM v2, treat Count and Find as separate executions: don’t reuse the same “initialized” *gorm.DB returned after a chain/finisher, or you can get “condition pollution”. Use a New Session Method (Session/WithContext/Debug) to make a query safely reusable. [1]
Recommended pattern (reusable base query + separate executions)
base := db.Model(&User{}).
Where("status = ?", "active").
Session(&gorm.Session{}) // makes base safely reusable
var total int64
if err := base.Session(&gorm.Session{}).Count(&total).Error; err != nil {
return err
}
var rows []User
if err := base.Session(&gorm.Session{}).
Order("created_at DESC").
Limit(pageSize).
Offset(offset).
Find(&rows).Error; err != nil {
return err
}Why:
Sessionis the “new session” mechanism for safe reuse. [1][2]Countis a query execution (finisher) and should be run independently fromFind. [3]
Pagination gotcha: Count can include Limit/Offset if you already applied them
So either Count first, then apply pagination, or reset pagination before counting (e.g., Limit(-1).Offset(-1) patterns exist, but “Count first” is usually clearer). [4]
Sources:
[1] GORM Method Chaining (reusability/safety) https://gorm.io/docs/method_chaining.html
[2] GORM Session (New Session Methods, Initialized, etc.) https://gorm.io/docs/session.html
[3] GORM Advanced Query – Count examples https://gorm.io/docs/advanced_query.html
[4] Note on Limit/Offset affecting Count (example discussion) https://www.cnblogs.com/beatle-go/p/17463713.html
🏁 Script executed:
head -20 modules/group_monitor/model.goRepository: QuantumNous/new-api
Length of output: 745
🏁 Script executed:
grep -n "gorm.io/gorm" modules/group_monitor/model.goRepository: QuantumNous/new-api
Length of output: 45
Bug: GORM Count mutates the query, corrupting the subsequent Find.
In GORM v2, Count is a finisher method that initializes and mutates the query's internal Statement, making the same query object unsafe for reuse. Reusing it for Find produces incorrect SQL due to leftover state (e.g., SELECT count(*) clause). Use Session(&gorm.Session{}) to create a new session before Count:
- err := query.Count(&total).Error
+ err := query.Session(&gorm.Session{}).Count(&total).Error
if err != nil {
return nil, 0, err
}Add the import at the top of the file:
import "gorm.io/gorm"🤖 Prompt for AI Agents
In `@modules/group_monitor/model.go` around lines 49 - 71, GetGroupMonitorLogs
currently reuses the same GORM query for Count and Find which causes SQL
corruption; update the code to call Count on a fresh session (e.g.,
query.Session(&gorm.Session{}) or create a copy of the query before Count) so
the subsequent query.Order(...).Find(...) is not affected, and add the import
"gorm.io/gorm" at the top of the file; ensure you still build the filters on
groupName/startTs/endTs and use the new session when calling Count to populate
total, then run Order/Offset/Limit/Find on the original query.
| func GetGroupMonitorLatest() ([]*GroupMonitorLog, error) { | ||
| var logs []*GroupMonitorLog | ||
|
|
||
| // 先查所有 distinct group_name，再逐个查最新记录（兼容三种数据库） | ||
| var groupNames []string | ||
| err := model.DB.Model(&GroupMonitorLog{}).Distinct().Pluck("group_name", &groupNames).Error | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| for _, gn := range groupNames { | ||
| var log GroupMonitorLog | ||
| err := model.DB.Where("group_name = ?", gn).Order("created_at DESC").First(&log).Error | ||
| if err != nil { | ||
| continue | ||
| } | ||
| logs = append(logs, &log) | ||
| } | ||
| return logs, nil | ||
| } |
There was a problem hiding this comment.
Silent error swallowing in per-group loop.
Line 88: errors from First are silently discarded via continue. If the error is something other than "record not found" (e.g., a connection failure), the function returns a partial result with no indication of failure. Consider logging or accumulating errors.
🤖 Prompt for AI Agents
In `@modules/group_monitor/model.go` around lines 74 - 93, GetGroupMonitorLatest
currently swallows all errors from
model.DB.Where(...).Order(...).First(&log).Error by doing continue; update this
loop to only ignore gorm.ErrRecordNotFound and to handle any other error by
returning it (or collecting and returning a combined error) so callers aren’t
given partial results on DB failures; modify the error check in the per-group
loop inside GetGroupMonitorLatest to compare against gorm.ErrRecordNotFound (or
use errors.Is) and continue only in that case, otherwise propagate or accumulate
the error and stop.
| for i, stat := range stats { | ||
| var count int64 | ||
| q := model.DB.Model(&GroupMonitorLog{}).Where("group_name = ? AND success = ?", stat.GroupName, true) | ||
| if startTs > 0 { | ||
| q = q.Where("created_at >= ?", startTs) | ||
| } | ||
| if endTs > 0 { | ||
| q = q.Where("created_at <= ?", endTs) | ||
| } | ||
| q.Count(&count) | ||
| stats[i].SuccessCount = count | ||
| } |
There was a problem hiding this comment.
N+1 queries for success count; unchecked error on Count.
Two concerns:
- The loop issues one extra query per group to get
SuccessCount. For a small number of groups this is acceptable, but it scales linearly. - Line 135:
q.Count(&count)discards the error. If the query fails,SuccessCountsilently stays 0.
At minimum, check the error:
Proposed fix: check error on Count
- q.Count(&count)
+ if err := q.Count(&count).Error; err != nil {
+ return nil, err
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i, stat := range stats { | |
| var count int64 | |
| q := model.DB.Model(&GroupMonitorLog{}).Where("group_name = ? AND success = ?", stat.GroupName, true) | |
| if startTs > 0 { | |
| q = q.Where("created_at >= ?", startTs) | |
| } | |
| if endTs > 0 { | |
| q = q.Where("created_at <= ?", endTs) | |
| } | |
| q.Count(&count) | |
| stats[i].SuccessCount = count | |
| } | |
| for i, stat := range stats { | |
| var count int64 | |
| q := model.DB.Model(&GroupMonitorLog{}).Where("group_name = ? AND success = ?", stat.GroupName, true) | |
| if startTs > 0 { | |
| q = q.Where("created_at >= ?", startTs) | |
| } | |
| if endTs > 0 { | |
| q = q.Where("created_at <= ?", endTs) | |
| } | |
| if err := q.Count(&count).Error; err != nil { | |
| return nil, err | |
| } | |
| stats[i].SuccessCount = count | |
| } |
🤖 Prompt for AI Agents
In `@modules/group_monitor/model.go` around lines 126 - 137, The loop currently
issues an N+1 query per group to populate SuccessCount using
model.DB.Model(&GroupMonitorLog{}).Where(...).Count(&count) and also ignores the
returned error; replace this with a single aggregated query that SELECTs
group_name, COUNT(*) AS cnt FROM group_monitor_logs WHERE success = true [AND
created_at >= startTs] [AND created_at <= endTs] GROUP BY group_name (using GORM
on model.DB with Model(&GroupMonitorLog{}) and Group("group_name")) and load
results into a temporary struct, then map counts back into the existing stats
slice by matching stat.GroupName to set stats[i].SuccessCount; also capture and
handle the error returned by the DB call (propagate or log/return) instead of
discarding it so failures no longer silently leave SuccessCount as zero.
| func SaveGroupMonitorConfig(cfg *GroupMonitorConfig) error { | ||
| cfg.UpdatedAt = common.GetTimestamp() | ||
| // Upsert by group_name | ||
| var existing GroupMonitorConfig | ||
| err := model.DB.Where("group_name = ?", cfg.GroupName).First(&existing).Error | ||
| if err != nil { | ||
| // 不存在，创建 | ||
| return model.DB.Create(cfg).Error | ||
| } | ||
| // 已存在，更新 | ||
| return model.DB.Model(&existing).Updates(map[string]interface{}{ | ||
| "channel_id": cfg.ChannelId, | ||
| "test_model": cfg.TestModel, | ||
| "enabled": cfg.Enabled, | ||
| "updated_at": cfg.UpdatedAt, | ||
| }).Error | ||
| } |
There was a problem hiding this comment.
TOCTOU race in upsert logic.
Between the First check (line 167) and Create (line 170), a concurrent request could insert the same group_name, causing a unique constraint violation on the uniqueIndex. Since this is an admin-only operation the risk is low, but be aware it's not atomic.
Consider handling the duplicate-key error from Create by falling back to an update, or use GORM's Clauses(clause.OnConflict{...}) if cross-DB compatibility allows.
🤖 Prompt for AI Agents
In `@modules/group_monitor/model.go` around lines 163 - 179,
SaveGroupMonitorConfig currently does a separate First then Create which creates
a TOCTOU race; modify SaveGroupMonitorConfig (and the Create path for
GroupMonitorConfig) to handle duplicate-key errors by falling back to an update
(e.g., attempt model.DB.Create(cfg) and if it returns a unique-constraint error
for group_name then perform
model.DB.Model(&GroupMonitorConfig{}).Where("group_name = ?",
cfg.GroupName).Updates(...) ), or replace the upsert with GORM's
clause.OnConflict to do an atomic upsert for GroupMonitorConfig (using
Clauses(clause.OnConflict{UpdateAll: true} or a specific DoUpdates map) to avoid
the race. Ensure you reference SaveGroupMonitorConfig, GroupMonitorConfig,
Create, and Updates in the change.
| // 用户接口 | ||
| userRoute := apiRouter.Group("/group/monitor") | ||
| userRoute.Use(middleware.UserAuth()) | ||
| { | ||
| userRoute.GET("/status", GetGroupMonitorStatusHandler) | ||
| } |
There was a problem hiding this comment.
User UI will fail to load the latency trend chart — /time_series is admin-only.
The GroupMonitorUser component (from the relevant code snippets) fetches /api/group/monitor/time_series, but this endpoint is only registered under the admin route group (Line 18). Regular users will receive a 403 response, and the chart won't render.
Either add GET /time_series to the user route group, or remove the time-series chart from the user UI.
Suggested fix — expose time_series to users
// 用户接口
userRoute := apiRouter.Group("/group/monitor")
userRoute.Use(middleware.UserAuth())
{
userRoute.GET("/status", GetGroupMonitorStatusHandler)
+ userRoute.GET("/time_series", GetGroupMonitorTimeSeriesHandler)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 用户接口 | |
| userRoute := apiRouter.Group("/group/monitor") | |
| userRoute.Use(middleware.UserAuth()) | |
| { | |
| userRoute.GET("/status", GetGroupMonitorStatusHandler) | |
| } | |
| // 用户接口 | |
| userRoute := apiRouter.Group("/group/monitor") | |
| userRoute.Use(middleware.UserAuth()) | |
| { | |
| userRoute.GET("/status", GetGroupMonitorStatusHandler) | |
| userRoute.GET("/time_series", GetGroupMonitorTimeSeriesHandler) | |
| } |
🤖 Prompt for AI Agents
In `@modules/group_monitor/router.go` around lines 24 - 29, The user UI fails
because the time-series endpoint is only registered for admins; add the same GET
"/time_series" handler to the user route group so authenticated users can fetch
it: inside the userRoute block (variable userRoute) register
userRoute.GET("/time_series", GetGroupMonitorTimeSeriesHandler) (the handler
used for admins), ensuring it uses middleware.UserAuth(); alternatively remove
the chart from the GroupMonitorUser UI if you intentionally want it admin-only.
| for { | ||
| setting = GetGroupMonitorSetting() | ||
| interval := time.Duration(int(math.Round(setting.IntervalMins))) * time.Minute | ||
| time.Sleep(interval) |
There was a problem hiding this comment.
Zero or near-zero IntervalMins causes a tight infinite loop.
If setting.IntervalMins rounds to 0 (or is very small), time.Duration(0) * time.Minute equals zero, and the goroutine will spin without sleeping, hammering the database and test endpoints. Enforce a minimum interval.
Suggested fix
setting = GetGroupMonitorSetting()
- interval := time.Duration(int(math.Round(setting.IntervalMins))) * time.Minute
+ intervalMins := int(math.Round(setting.IntervalMins))
+ if intervalMins < 1 {
+ intervalMins = 1
+ }
+ interval := time.Duration(intervalMins) * time.Minute
time.Sleep(interval)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for { | |
| setting = GetGroupMonitorSetting() | |
| interval := time.Duration(int(math.Round(setting.IntervalMins))) * time.Minute | |
| time.Sleep(interval) | |
| for { | |
| setting = GetGroupMonitorSetting() | |
| intervalMins := int(math.Round(setting.IntervalMins)) | |
| if intervalMins < 1 { | |
| intervalMins = 1 | |
| } | |
| interval := time.Duration(intervalMins) * time.Minute | |
| time.Sleep(interval) |
🤖 Prompt for AI Agents
In `@modules/group_monitor/service.go` around lines 31 - 34, The loop in
GetGroupMonitorSetting() processing can produce a zero sleep because interval is
computed from setting.IntervalMins and may round to 0; update the loop to
enforce a sane minimum (e.g., if computed interval <= 0 or < time.Minute then
set interval = time.Minute or another chosen MIN_INTERVAL) before calling
time.Sleep; reference the variables and functions from the diff (setting,
IntervalMins, interval, GetGroupMonitorSetting) and handle negative/zero values
so the goroutine never spins with a zero-duration sleep.
| func testAndRecord(groupName string, channel *model.Channel, testModel string) { | ||
| result := controller.TestChannelForMonitor(channel, testModel) | ||
|
|
||
| log := &GroupMonitorLog{ | ||
| GroupName: groupName, | ||
| ChannelId: channel.Id, | ||
| ChannelName: channel.Name, | ||
| ModelName: testModel, | ||
| LatencyMs: result.LatencyMs, | ||
| Success: result.Success, | ||
| ErrorMsg: result.ErrorMsg, | ||
| CreatedAt: common.GetTimestamp(), | ||
| } | ||
|
|
||
| err := CreateGroupMonitorLog(log) | ||
| if err != nil { | ||
| common.SysError(fmt.Sprintf("group monitor: failed to save log for group %s: %v", groupName, err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the GroupMonitorLog struct definition to verify CachedModel field exists
rg -n 'type GroupMonitorLog struct' --type=go -A 20Repository: QuantumNous/new-api
Length of output: 1921
🏁 Script executed:
#!/bin/bash
# Find the TestChannelForMonitor function signature to see what it returns
rg -n 'func TestChannelForMonitor' --type=go -A 5Repository: QuantumNous/new-api
Length of output: 530
🏁 Script executed:
#!/bin/bash
# Search for CachedModel references in the codebase
rg -n 'CachedModel' --type=goRepository: QuantumNous/new-api
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Get the full TestChannelForMonitor function
rg -n 'func TestChannelForMonitor' --type=go -A 40Repository: QuantumNous/new-api
Length of output: 2575
🏁 Script executed:
#!/bin/bash
# Find the MonitorTestResult struct definition
rg -n 'type MonitorTestResult struct' --type=go -A 10Repository: QuantumNous/new-api
Length of output: 801
🏁 Script executed:
#!/bin/bash
# Also check if there's any admin UI code that uses CachedModel
rg -n '缓存模型|cached.model|CachedModel' --type=go --type=vue --type=jsRepository: QuantumNous/new-api
Length of output: 90
CachedModel field is never populated in the log record.
The GroupMonitorLog struct defines a CachedModel field (line 21, modules/group_monitor/model.go), but testAndRecord never sets it. TestChannelForMonitor does not return CachedModel data (the MonitorTestResult struct only contains Success, LatencyMs, and ErrorMsg), so the field will always be empty. Consider either removing the field or sourcing CachedModel from the channel model or configuration if it should be tracked.
🤖 Prompt for AI Agents
In `@modules/group_monitor/service.go` around lines 109 - 127, The
GroupMonitorLog.CachedModel field is never set in testAndRecord because
controller.TestChannelForMonitor returns a MonitorTestResult with no
cached-model info; update testAndRecord to populate CachedModel (or remove the
field if unused). Locate the testAndRecord function and either: 1) read the
cached model identifier from the Channel object (channel.Model or similar) or
from the service/config where channels are defined and assign it to
log.CachedModel before calling CreateGroupMonitorLog, or 2) if CachedModel is
not meaningful, remove the CachedModel field from the GroupMonitorLog struct and
related storage code; ensure changes reference GroupMonitorLog, testAndRecord,
controller.TestChannelForMonitor, MonitorTestResult, and model.Channel so
callers and storage remain consistent.
| // 加载渠道列表 | ||
| const loadChannels = useCallback(async () => { | ||
| const res = await API.get('/api/channel/', { params: { p: 1, page_size: 100 } }); | ||
| if (res.data.success) { | ||
| setChannels(res.data.data?.items || res.data.data || []); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Channel list is hard-capped at 100 — larger deployments will silently miss channels.
The page_size: 100 limit means admins with more than 100 channels won't see all options in the channel selector. Consider fetching all channels or implementing a search/autocomplete approach.
🤖 Prompt for AI Agents
In `@web/src/modules/group-monitor/GroupMonitorAdmin.jsx` around lines 93 - 99,
The channel loader loadChannels currently requests only page_size: 100 which
truncates results; update loadChannels (and the API.get call) to retrieve all
channels by either removing the hard cap or implementing pagination: loop
calling API.get('/api/channel/') with incrementing p until res.data.data.items
is empty (accumulate items into a single array) and then call
setChannels(accumulated) — alternatively wire up a search/autocomplete UI and
query the backend for matches instead of fetching all when appropriate.
| <span className='text-gray-500'>{t('可用率')}</span> | ||
| <span style={{ color: availability !== '-' && parseFloat(availability) >= 95 ? '#52c41a' : '#ff4d4f' }}> | ||
| {availability}% | ||
| </span> | ||
| </div> | ||
| <div className='flex justify-between mb-1'> | ||
| <span className='text-gray-500'>{t('平均延迟')}</span> | ||
| <span>{avgLatency}ms</span> | ||
| </div> |
There was a problem hiding this comment.
Display artifacts: "-%" and "-ms" when no data is available.
When availability or avgLatency is '-', the template literals produce -% and -ms. Gate the suffix on the value being numeric.
Suggested fix
- {availability}%
+ {availability !== '-' ? `${availability}%` : '-'}- <span>{avgLatency}ms</span>
+ <span>{avgLatency !== '-' ? `${avgLatency}ms` : '-'}</span>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <span className='text-gray-500'>{t('可用率')}</span> | |
| <span style={{ color: availability !== '-' && parseFloat(availability) >= 95 ? '#52c41a' : '#ff4d4f' }}> | |
| {availability}% | |
| </span> | |
| </div> | |
| <div className='flex justify-between mb-1'> | |
| <span className='text-gray-500'>{t('平均延迟')}</span> | |
| <span>{avgLatency}ms</span> | |
| </div> | |
| <span className='text-gray-500'>{t('可用率')}</span> | |
| <span style={{ color: availability !== '-' && parseFloat(availability) >= 95 ? '#52c41a' : '#ff4d4f' }}> | |
| {availability !== '-' ? `${availability}%` : '-'} | |
| </span> | |
| </div> | |
| <div className='flex justify-between mb-1'> | |
| <span className='text-gray-500'>{t('平均延迟')}</span> | |
| <span>{avgLatency !== '-' ? `${avgLatency}ms` : '-'}</span> | |
| </div> |
🤖 Prompt for AI Agents
In `@web/src/modules/group-monitor/GroupMonitorAdmin.jsx` around lines 456 - 464,
The UI shows "-%" and "-ms" when availability or avgLatency is the string '-' —
update the JSX around the availability and avgLatency spans in
GroupMonitorAdmin.jsx to only append the '%' and 'ms' suffixes when the values
are numeric (e.g. use parseFloat/Number and isNaN checks or a utility like
isNumeric) and otherwise render the raw '-' (or a placeholder) without the
suffix; specifically change the span that renders availability (variable
availability) and the span that renders avgLatency (variable avgLatency) to
conditionally include the suffixes based on numeric checks.
- Add user_time_series endpoint for user access - Change default test model to gpt-4o-mini - Optimize GetGroupMonitorLatest with subquery - Fix React useCallback/useEffect dependencies
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@modules/group_monitor/model.go`:
- Around line 196-211: GetGroupMonitorTimeSeries currently can return an
unbounded number of rows and should enforce a cap; modify the function
(GetGroupMonitorTimeSeries) to apply a sensible Limit on the query (e.g., 1000
or a const like maxTimeSeriesRows) and/or validate the time window (reject or
require a tighter startTs/endTs if the range exceeds a threshold) before
executing model.DB.Model(&GroupMonitorLog{}) query; ensure the query uses query
= query.Limit(maxTimeSeriesRows) and return a clear error when the caller
supplies too large a range or when rows are truncated so callers can handle
pagination or narrower queries.
In `@web/src/modules/group-monitor/GroupMonitorAdmin.jsx`:
- Around line 42-63: Wrap the async bodies of loadGlobalSettings,
saveGlobalSettings, saveConfig, and deleteConfig in try/catch blocks so any
thrown API errors are handled; inside each try keep the existing API calls and
state updates, and in the catch log or surface the error (console.error or
showError/toast) and ensure any loading/spinner state (e.g., isLoading, saving
flags) is reset before returning; also consider rethrowing or returning a
failure value if callers depend on the result.
In `@web/src/modules/group-monitor/GroupMonitorUser.jsx`:
- Around line 15-30: The async fetches in loadStatus and loadTimeSeries can
throw and currently have no try/catch, causing unhandled promise rejections;
wrap the body of both loadStatus and loadTimeSeries in try/catch blocks, call
API.get as before, setStatusData and setTimeSeriesData on success, and in the
catch log the error (e.g., console.error or process/logger) and optionally set
an error state or leave data unchanged to avoid crashing the interval refresh.
Ensure the error handling is added around the await API.get calls inside the
existing useCallback functions so the interval refresh doesn't produce silent
rejections.
🧹 Nitpick comments (2)
web/src/modules/group-monitor/GroupMonitorAdmin.jsx (1)
73-75: Dead ternary — both branches are identical.const value = typeof globalSettings[item.key] === 'boolean' ? String(globalSettings[item.key]) : String(globalSettings[item.key]);The condition has no effect; both branches call
String(...). Simplify:Proposed fix
- const value = typeof globalSettings[item.key] === 'boolean' ? String(globalSettings[item.key]) : String(globalSettings[item.key]); + const value = String(globalSettings[item.key]);web/src/modules/group-monitor/GroupMonitorUser.jsx (1)
43-83: Chart spec is largely duplicated fromGroupMonitorAdmin.jsx.The
chartSpecconstruction (data mapping, axes, line/point styles) is nearly identical between both components. Consider extracting a shared helper (e.g.,buildLatencyChartSpec(timeSeriesData, t)) into a common module to avoid divergence.
| // GetGroupMonitorTimeSeries 获取时间序列数据（用于趋势图） | ||
| func GetGroupMonitorTimeSeries(groupName string, startTs, endTs int64) ([]*GroupMonitorLog, error) { | ||
| var logs []*GroupMonitorLog | ||
| query := model.DB.Model(&GroupMonitorLog{}) | ||
| if groupName != "" { | ||
| query = query.Where("group_name = ?", groupName) | ||
| } | ||
| if startTs > 0 { | ||
| query = query.Where("created_at >= ?", startTs) | ||
| } | ||
| if endTs > 0 { | ||
| query = query.Where("created_at <= ?", endTs) | ||
| } | ||
| err := query.Order("created_at ASC").Find(&logs).Error | ||
| return logs, err | ||
| } |
There was a problem hiding this comment.
Unbounded result set on time-series query.
GetGroupMonitorTimeSeries has no Limit clause. For a wide time range or many groups, this could return a very large number of rows, causing high memory usage and slow responses. Consider adding a reasonable cap (e.g., 1000–5000 rows) or requiring a tighter time window.
Suggested fix
- err := query.Order("created_at ASC").Find(&logs).Error
+ err := query.Order("created_at ASC").Limit(5000).Find(&logs).Error🤖 Prompt for AI Agents
In `@modules/group_monitor/model.go` around lines 196 - 211,
GetGroupMonitorTimeSeries currently can return an unbounded number of rows and
should enforce a cap; modify the function (GetGroupMonitorTimeSeries) to apply a
sensible Limit on the query (e.g., 1000 or a const like maxTimeSeriesRows)
and/or validate the time window (reject or require a tighter startTs/endTs if
the range exceeds a threshold) before executing
model.DB.Model(&GroupMonitorLog{}) query; ensure the query uses query =
query.Limit(maxTimeSeriesRows) and return a clear error when the caller supplies
too large a range or when rows are truncated so callers can handle pagination or
narrower queries.
| const loadGlobalSettings = useCallback(async () => { | ||
| const res = await API.get('/api/option/'); | ||
| if (res.data.success) { | ||
| const opts = res.data.data; | ||
| const current = {}; | ||
| for (const key of Object.keys(DEFAULT_SETTINGS)) { | ||
| if (opts[key] !== undefined) { | ||
| if (key.includes('enabled')) { | ||
| current[key] = opts[key] === 'true'; | ||
| } else if (key.includes('interval') || key.includes('retain') || key.includes('days')) { | ||
| current[key] = parseInt(opts[key]); | ||
| } else { | ||
| current[key] = opts[key]; | ||
| } | ||
| } else { | ||
| current[key] = DEFAULT_SETTINGS[key]; | ||
| } | ||
| } | ||
| setGlobalSettings(current); | ||
| setSavedSettings(structuredClone(current)); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Missing error handling on all async API calls.
None of the load* functions (or saveGlobalSettings, saveConfig, deleteConfig) have try/catch blocks. If any API call throws (network error, 5xx, etc.), the promise rejection goes unhandled, which can leave loading spinners stuck and produce console errors.
Wrap each async function body in try/catch, e.g.:
Example fix for loadGlobalSettings
const loadGlobalSettings = useCallback(async () => {
+ try {
const res = await API.get('/api/option/');
if (res.data.success) {
// ...
}
+ } catch (err) {
+ showError(err.message || 'Failed to load settings');
+ }
}, []);Apply the same pattern to all other async functions in this component.
🤖 Prompt for AI Agents
In `@web/src/modules/group-monitor/GroupMonitorAdmin.jsx` around lines 42 - 63,
Wrap the async bodies of loadGlobalSettings, saveGlobalSettings, saveConfig, and
deleteConfig in try/catch blocks so any thrown API errors are handled; inside
each try keep the existing API calls and state updates, and in the catch log or
surface the error (console.error or showError/toast) and ensure any
loading/spinner state (e.g., isLoading, saving flags) is reset before returning;
also consider rethrowing or returning a failure value if callers depend on the
result.
| const loadStatus = useCallback(async () => { | ||
| const res = await API.get('/api/group/monitor/status'); | ||
| if (res.data.success) { | ||
| setStatusData(res.data.data || []); | ||
| } | ||
| }, []); | ||
|
|
||
| const loadTimeSeries = useCallback(async () => { | ||
| const now = Math.floor(Date.now() / 1000); | ||
| const res = await API.get('/api/group/monitor/user_time_series', { | ||
| params: { start_timestamp: now - 3600 }, | ||
| }); | ||
| if (res.data.success) { | ||
| setTimeSeriesData(res.data.data || []); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Missing error handling on async API calls.
Same issue as in GroupMonitorAdmin.jsx: loadStatus and loadTimeSeries lack try/catch. A failed fetch during the 60-second interval refresh will produce unhandled promise rejections silently.
Example fix
const loadStatus = useCallback(async () => {
+ try {
const res = await API.get('/api/group/monitor/status');
if (res.data.success) {
setStatusData(res.data.data || []);
}
+ } catch (err) {
+ console.error('Failed to load status', err);
+ }
}, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const loadStatus = useCallback(async () => { | |
| const res = await API.get('/api/group/monitor/status'); | |
| if (res.data.success) { | |
| setStatusData(res.data.data || []); | |
| } | |
| }, []); | |
| const loadTimeSeries = useCallback(async () => { | |
| const now = Math.floor(Date.now() / 1000); | |
| const res = await API.get('/api/group/monitor/user_time_series', { | |
| params: { start_timestamp: now - 3600 }, | |
| }); | |
| if (res.data.success) { | |
| setTimeSeriesData(res.data.data || []); | |
| } | |
| }, []); | |
| const loadStatus = useCallback(async () => { | |
| try { | |
| const res = await API.get('/api/group/monitor/status'); | |
| if (res.data.success) { | |
| setStatusData(res.data.data || []); | |
| } | |
| } catch (err) { | |
| console.error('Failed to load status', err); | |
| } | |
| }, []); | |
| const loadTimeSeries = useCallback(async () => { | |
| const now = Math.floor(Date.now() / 1000); | |
| const res = await API.get('/api/group/monitor/user_time_series', { | |
| params: { start_timestamp: now - 3600 }, | |
| }); | |
| if (res.data.success) { | |
| setTimeSeriesData(res.data.data || []); | |
| } | |
| }, []); |
🤖 Prompt for AI Agents
In `@web/src/modules/group-monitor/GroupMonitorUser.jsx` around lines 15 - 30, The
async fetches in loadStatus and loadTimeSeries can throw and currently have no
try/catch, causing unhandled promise rejections; wrap the body of both
loadStatus and loadTimeSeries in try/catch blocks, call API.get as before,
setStatusData and setTimeSeriesData on success, and in the catch log the error
(e.g., console.error or process/logger) and optionally set an error state or
leave data unchanged to avoid crashing the interval refresh. Ensure the error
handling is added around the await API.get calls inside the existing useCallback
functions so the interval refresh doesn't produce silent rejections.
- Fix closure variable capture bug in goroutine (service.go) - Remove unused GetChannelIdsByGroupAndModel function (ability.go) - Remove unused ParseChannelIds function (model.go) - Remove unused isAdmin import (GroupMonitorAdmin.jsx)
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@modules/group_monitor/service.go`:
- Around line 90-95: The local variable named model shadows the imported model
package; rename the local variable (e.g., testModel or mdl) in the closure where
you assign actualModel so future references to the model package don't get
shadowed—update the assignment (model := actualModel) and any use inside the
gopool.Go closure (which calls testAndRecord) to use the new name (e.g.,
testModel) instead.
- Around line 101-109: The comparison in channelSupportsModel does not normalize
testModel, so leading/trailing whitespace in testModel can cause false
negatives; update channelSupportsModel to trim testModel (e.g., assign testModel
= strings.TrimSpace(testModel) at the top) before iterating over
channel.GetModels(), or otherwise normalize both sides (trim each m and the
testModel) so comparisons are consistent; modify the function
channelSupportsModel and its use sites if needed to ensure testModel is
normalized.
In `@web/src/modules/group-monitor/GroupMonitorAdmin.jsx`:
- Line 19: The shared loading state causes UI cross-contamination; replace the
single useState(false) "loading" with two dedicated booleans (e.g.
savingSettings and logsLoading) in GroupMonitorAdmin.jsx, update all callers
that set or read loading (notably saveGlobalSettings and loadLogs) to set the
appropriate flag instead of loading, and change consumers (the global-settings
<Spin> and the logs <Table loading={...}>) to use savingSettings and logsLoading
respectively; ensure every setLoading(false)/setLoading(true) in functions
referenced in lines around saveGlobalSettings and loadLogs is converted to the
corresponding flag to avoid races and incorrect spinners.
- Line 346: The onChange handler for the InputNumber currently does
setGlobalSettings({ ...globalSettings, 'group_monitor_setting.interval_mins':
parseInt(v) }) which yields NaN when the control is cleared; change it to guard
empty/undefined values and use an explicit radix: compute let val = (v ===
undefined || v === "") ? undefined : parseInt(v, 10) and then call
setGlobalSettings({ ...globalSettings, 'group_monitor_setting.interval_mins':
val }); apply the same guarded parsing change to the other occurrence handling
the related setting on lines around 362 so cleared inputs store undefined
instead of "NaN".
🧹 Nitpick comments (2)
web/src/modules/group-monitor/GroupMonitorAdmin.jsx (2)
73-75: Redundant ternary — both branches are identical.The ternary on line 74 evaluates to
String(globalSettings[item.key])regardless of the type check. Simplify:Proposed fix
- const value = typeof globalSettings[item.key] === 'boolean' ? String(globalSettings[item.key]) : String(globalSettings[item.key]); + const value = String(globalSettings[item.key]);
317-321:statsMapis rebuilt on every render — consideruseMemo.This object is derived from
statsDataand consumed during render. Wrapping it inuseMemoavoids unnecessary recomputation on unrelated re-renders.Proposed fix
- const statsMap = {}; - statsData.forEach((s) => { - statsMap[s.group_name] = s; - }); + const statsMap = useMemo(() => { + const map = {}; + statsData.forEach((s) => { map[s.group_name] = s; }); + return map; + }, [statsData]);
| groupName := cfg.GroupName | ||
| ch := channel | ||
| model := actualModel | ||
| gopool.Go(func() { | ||
| testAndRecord(groupName, ch, model) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Variable model shadows the imported model package.
On line 92, model := actualModel shadows the model package imported at line 12. While the current closure body doesn't reference the package, any future edit within this scope that tries to call model.SomeFunc(...) will silently bind to the string variable and fail to compile (or worse, cause confusion). Rename to something like testModel or mdl.
Suggested fix
groupName := cfg.GroupName
ch := channel
- model := actualModel
+ mdl := actualModel
gopool.Go(func() {
- testAndRecord(groupName, ch, model)
+ testAndRecord(groupName, ch, mdl)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| groupName := cfg.GroupName | |
| ch := channel | |
| model := actualModel | |
| gopool.Go(func() { | |
| testAndRecord(groupName, ch, model) | |
| }) | |
| groupName := cfg.GroupName | |
| ch := channel | |
| mdl := actualModel | |
| gopool.Go(func() { | |
| testAndRecord(groupName, ch, mdl) | |
| }) |
🤖 Prompt for AI Agents
In `@modules/group_monitor/service.go` around lines 90 - 95, The local variable
named model shadows the imported model package; rename the local variable (e.g.,
testModel or mdl) in the closure where you assign actualModel so future
references to the model package don't get shadowed—update the assignment (model
:= actualModel) and any use inside the gopool.Go closure (which calls
testAndRecord) to use the new name (e.g., testModel) instead.
| func channelSupportsModel(channel *model.Channel, testModel string) bool { | ||
| models := channel.GetModels() | ||
| for _, m := range models { | ||
| if strings.TrimSpace(m) == testModel { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Inconsistent whitespace handling — testModel is not trimmed.
TrimSpace is applied to each channel model but not to testModel. If the configured test model contains leading/trailing whitespace (e.g., from user input in the admin UI), the comparison will always fail, silently triggering the fallback path for every channel.
Trim testModel as well, or normalize both inputs at the call site.
Suggested fix
func channelSupportsModel(channel *model.Channel, testModel string) bool {
+ testModel = strings.TrimSpace(testModel)
models := channel.GetModels()
for _, m := range models {
if strings.TrimSpace(m) == testModel {🤖 Prompt for AI Agents
In `@modules/group_monitor/service.go` around lines 101 - 109, The comparison in
channelSupportsModel does not normalize testModel, so leading/trailing
whitespace in testModel can cause false negatives; update channelSupportsModel
to trim testModel (e.g., assign testModel = strings.TrimSpace(testModel) at the
top) before iterating over channel.GetModels(), or otherwise normalize both
sides (trim each m and the testModel) so comparisons are consistent; modify the
function channelSupportsModel and its use sites if needed to ensure testModel is
normalized.
|
|
||
| const GroupMonitorAdmin = () => { | ||
| const { t } = useTranslation(); | ||
| const [loading, setLoading] = useState(false); |
There was a problem hiding this comment.
Shared loading state causes cross-contamination between unrelated UI sections.
A single loading boolean drives both the global-settings <Spin> (line 329) and the logs <Table loading> (line 518). When loadLogs runs, the global-settings card shows a spinner; when saveGlobalSettings runs, the logs table shows loading. Concurrent calls also race on setLoading(false).
Split into dedicated flags (e.g. savingSettings and logsLoading).
Proposed fix
- const [loading, setLoading] = useState(false);
+ const [savingSettings, setSavingSettings] = useState(false);
+ const [logsLoading, setLogsLoading] = useState(false);Then in saveGlobalSettings:
- setLoading(true);
+ setSavingSettings(true);
...
- setLoading(false);
+ setSavingSettings(false);In loadLogs:
- setLoading(true);
+ setLogsLoading(true);
...
- setLoading(false);
+ setLogsLoading(false);And update the consumers:
- <Spin spinning={loading}>
+ <Spin spinning={savingSettings}>- loading={loading}
+ loading={logsLoading}Also applies to: 72-84, 167-176
🤖 Prompt for AI Agents
In `@web/src/modules/group-monitor/GroupMonitorAdmin.jsx` at line 19, The shared
loading state causes UI cross-contamination; replace the single useState(false)
"loading" with two dedicated booleans (e.g. savingSettings and logsLoading) in
GroupMonitorAdmin.jsx, update all callers that set or read loading (notably
saveGlobalSettings and loadLogs) to set the appropriate flag instead of loading,
and change consumers (the global-settings <Spin> and the logs <Table
loading={...}>) to use savingSettings and logsLoading respectively; ensure every
setLoading(false)/setLoading(true) in functions referenced in lines around
saveGlobalSettings and loadLogs is converted to the corresponding flag to avoid
races and incorrect spinners.
| value={globalSettings['group_monitor_setting.interval_mins']} | ||
| min={1} | ||
| suffix={t('分钟')} | ||
| onChange={(v) => setGlobalSettings({ ...globalSettings, 'group_monitor_setting.interval_mins': parseInt(v) })} |
There was a problem hiding this comment.
parseInt(v) produces NaN when the InputNumber is cleared.
Semi's InputNumber passes undefined or "" on clear. parseInt(undefined) → NaN, which would be saved to state and serialized as "NaN" on the API call.
Guard with a fallback:
Proposed fix
- onChange={(v) => setGlobalSettings({ ...globalSettings, 'group_monitor_setting.interval_mins': parseInt(v) })}
+ onChange={(v) => setGlobalSettings({ ...globalSettings, 'group_monitor_setting.interval_mins': Number(v) || 1 })}- onChange={(v) => setGlobalSettings({ ...globalSettings, 'group_monitor_setting.retain_days': parseInt(v) })}
+ onChange={(v) => setGlobalSettings({ ...globalSettings, 'group_monitor_setting.retain_days': Number(v) || 1 })}Also applies to: 362-362
🤖 Prompt for AI Agents
In `@web/src/modules/group-monitor/GroupMonitorAdmin.jsx` at line 346, The
onChange handler for the InputNumber currently does setGlobalSettings({
...globalSettings, 'group_monitor_setting.interval_mins': parseInt(v) }) which
yields NaN when the control is cleared; change it to guard empty/undefined
values and use an explicit radix: compute let val = (v === undefined || v ===
"") ? undefined : parseInt(v, 10) and then call setGlobalSettings({
...globalSettings, 'group_monitor_setting.interval_mins': val }); apply the same
guarded parsing change to the other occurrence handling the related setting on
lines around 362 so cleared inputs store undefined instead of "NaN".
1 participant
Summary
/console/group-monitor): global settings, per-group channel config, status cards, latency trend chart,monitor logs table
/console/group-status): simplified status cards and latency trend chartArchitecture
Independent module design - only ~50 lines of changes across 8 core files, all business logic in
modules/directory.Core file changes (minimal, easy to restore after update):
main.go- import + migration + goroutine startrouter/api-router.go- route registrationcontroller/channel-test.go- exported TestChannelForMonitor()model/ability.go- exported GetChannelIdsByGroupAndModel()web/src/App.jsx- 2 routesweb/src/components/layout/SiderBar.jsx- 2 menu itemsweb/src/helpers/render.jsx- 2 iconsweb/src/i18n/locales/en.json- translationsSummary by CodeRabbit
Release Notes