feat: support multi-key channel key testing#5646
Closed
SwillerDawn wants to merge 2 commits into
Closed
Conversation
Contributor
WalkthroughAdds per-key targeting to the channel test flow: a new ChangesMulti-key channel test targeting
Sequence Diagram(s)sequenceDiagram
participant User
participant TestDialog
participant handleTestChannel
participant GoTestChannel
participant SetupContextForSelectedChannel
participant GetEnabledKeyByIndex
User->>TestDialog: Open dialog (multi-key channel)
TestDialog->>GoTestChannel: GET /api/channel/multi-keys (getMultiKeyStatus)
GoTestChannel-->>TestDialog: KeyStatus[] with masked KeyPreview
User->>TestDialog: Select key from Test Key dropdown
User->>TestDialog: Click test model
TestDialog->>handleTestChannel: { keyIndex: selectedKeyIndex }
handleTestChannel->>GoTestChannel: POST /api/channel/test?key_index=N
GoTestChannel->>SetupContextForSelectedChannel: inject key index via SetChannelTestMultiKeyIndex
SetupContextForSelectedChannel->>GetEnabledKeyByIndex: index N
GetEnabledKeyByIndex-->>SetupContextForSelectedChannel: key string or error
SetupContextForSelectedChannel-->>GoTestChannel: selected key
GoTestChannel-->>handleTestChannel: test result
handleTestChannel-->>TestDialog: testResults[composite key]
TestDialog->>User: Show result + optional Disable current key button
User->>TestDialog: Click Disable current key
TestDialog->>GoTestChannel: disableMultiKey(channelId, keyIndex)
GoTestChannel-->>TestDialog: updated key statuses
TestDialog->>TestDialog: Refresh key list + reset selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/default/src/features/channels/components/dialogs/channel-test-dialog.tsx (2)
849-906:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not delete channel models from a specific-key failure set.
failedis now scoped to the selected key viagetModelTestResultKey(model), butupdateChannel(... { models })removes those models globally from the channel. In specific-key mode, a failure usually means the key is bad, not the model; hide/guard this destructive path unless the selector is in auto mode.Proposed guard
const handleDeleteFailedModels = useCallback(async () => { + if (typeof selectedKeyIndex === 'number') { + setIsDeleteFailedDialogOpen(false) + return + } + const failed = models.filter( (model) => testResults[getModelTestResultKey(model)]?.status === 'error' ) @@ }, [ currentRow.id, getModelTestResultKey, models, refreshChannelLists, + selectedKeyIndex, t, testResults, ])Also hide the toolbar action in specific-key mode:
- {failedModels.length > 0 && ( + {failedModels.length > 0 && + typeof selectedKeyIndex !== 'number' && ( <Button variant='outline' size='sm' onClick={() => setIsDeleteFailedDialogOpen(true)} @@ })} </Button> - )} + )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/channels/components/dialogs/channel-test-dialog.tsx` around lines 849 - 906, The handleDeleteFailedModels function currently allows deletion of models globally based on test failures for a specific key, but in specific-key mode this is problematic since a key failure doesn't mean the model is bad. Add a guard condition at the beginning of handleDeleteFailedModels to check if the selector is in auto mode before allowing the deletion to proceed; if in specific-key mode, exit early by setting the dialog state and returning, or consider disabling this action entirely in the UI when in specific-key mode.
981-1036:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not offer key disabling for model-price/configuration failures.
canDisableTestKeyis true for every failed result, includingMODEL_PRICE_ERROR_CODE, where the existing UI correctly points users to pricing settings. Showing “Disable current key” there can disable a healthy key for a non-key-specific configuration error.Proposed gate
canDisableTestKey={ isMultiKeyChannel && typeof selectedKeyIndex === 'number' && - result?.status === 'error' + result?.status === 'error' && + result.errorCode !== MODEL_PRICE_ERROR_CODE }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/channels/components/dialogs/channel-test-dialog.tsx` around lines 981 - 1036, The `canDisableTestKey` property is currently enabled for all error results, but it should exclude model-price and configuration-specific failures where disabling the key is not the correct resolution. Modify the condition for `canDisableTestKey` in the TestStatusCell configuration to additionally check that the error result is not a pricing-related error code (such as MODEL_PRICE_ERROR_CODE). You should gate the key disabling option by verifying the specific error type in the result object, ensuring users are not offered to disable a healthy key when the actual issue is a configuration or pricing problem unrelated to the key itself.
🧹 Nitpick comments (1)
web/default/src/features/channels/components/dialogs/channel-test-dialog.tsx (1)
553-621: Refactor the multi-key fetch and disable operations to use React Query hooks for consistency with the project pattern.The
loadModelTestKeyStatusesanddisableSelectedTestKeyfunctions should be wrapped withuseQueryanduseMutationrespectively, using the establishedchannelsQueryKeyspattern. This aligns with the project's React Query implementation elsewhere in the channels feature and provides automatic loading, error handling, and cache invalidation management per the coding guidelines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/channels/components/dialogs/channel-test-dialog.tsx` around lines 553 - 621, The loadModelTestKeyStatuses and disableSelectedTestKey functions currently use manual state management with setModelTestKeyStatusLoading and setDisableTestKeyLoading instead of React Query hooks. Refactor loadModelTestKeyStatuses to use the useQuery hook with the channelsQueryKeys pattern (following the project's React Query implementation), and refactor disableSelectedTestKey to use the useMutation hook with appropriate onSuccess callback for cache invalidation. This will align with the established patterns in the channels feature and provide consistent loading state, error handling, and automatic cache management without manual state management.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model/channel.go`:
- Around line 295-300: The code at line 295 reads
channel.ChannelInfo.MultiKeyStatusList without acquiring the channel polling
lock, while other operations like GetNextEnabledKey and ManageMultiKeys use
GetChannelPollingLock(channel.Id) to protect concurrent access to this map. Wrap
the statusList access and the subsequent if block with
GetChannelPollingLock(channel.Id) to synchronize this read with other status
reads and writes, preventing potential race conditions and panic on concurrent
map access.
---
Outside diff comments:
In
`@web/default/src/features/channels/components/dialogs/channel-test-dialog.tsx`:
- Around line 849-906: The handleDeleteFailedModels function currently allows
deletion of models globally based on test failures for a specific key, but in
specific-key mode this is problematic since a key failure doesn't mean the model
is bad. Add a guard condition at the beginning of handleDeleteFailedModels to
check if the selector is in auto mode before allowing the deletion to proceed;
if in specific-key mode, exit early by setting the dialog state and returning,
or consider disabling this action entirely in the UI when in specific-key mode.
- Around line 981-1036: The `canDisableTestKey` property is currently enabled
for all error results, but it should exclude model-price and
configuration-specific failures where disabling the key is not the correct
resolution. Modify the condition for `canDisableTestKey` in the TestStatusCell
configuration to additionally check that the error result is not a
pricing-related error code (such as MODEL_PRICE_ERROR_CODE). You should gate the
key disabling option by verifying the specific error type in the result object,
ensuring users are not offered to disable a healthy key when the actual issue is
a configuration or pricing problem unrelated to the key itself.
---
Nitpick comments:
In
`@web/default/src/features/channels/components/dialogs/channel-test-dialog.tsx`:
- Around line 553-621: The loadModelTestKeyStatuses and disableSelectedTestKey
functions currently use manual state management with
setModelTestKeyStatusLoading and setDisableTestKeyLoading instead of React Query
hooks. Refactor loadModelTestKeyStatuses to use the useQuery hook with the
channelsQueryKeys pattern (following the project's React Query implementation),
and refactor disableSelectedTestKey to use the useMutation hook with appropriate
onSuccess callback for cache invalidation. This will align with the established
patterns in the channels feature and provide consistent loading state, error
handling, and automatic cache management without manual state management.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2a5033c-b672-4e2e-9741-cfb0cfa82bf2
📒 Files selected for processing (13)
controller/channel-test.gocontroller/channel.gomiddleware/distributor.gomodel/channel.goweb/default/src/features/channels/api.tsweb/default/src/features/channels/components/dialogs/channel-test-dialog.tsxweb/default/src/features/channels/lib/channel-actions.tsweb/default/src/i18n/locales/en.jsonweb/default/src/i18n/locales/fr.jsonweb/default/src/i18n/locales/ja.jsonweb/default/src/i18n/locales/ru.jsonweb/default/src/i18n/locales/vi.jsonweb/default/src/i18n/locales/zh.json
Comment on lines
+295
to
+300
| statusList := channel.ChannelInfo.MultiKeyStatusList | ||
| if statusList != nil { | ||
| if status, ok := statusList[index]; ok && status != common.ChannelStatusEnabled { | ||
| return "", index, fmt.Sprintf("key_status_%d", status), false | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Synchronize forced-index key-status reads with the channel lock.
Line 295 reads MultiKeyStatusList without GetChannelPollingLock(channel.Id). Other status reads/writes (GetNextEnabledKey, ManageMultiKeys) use this lock, so this path can race and panic on concurrent map access.
Proposed fix
func (channel *Channel) GetEnabledKeyByIndex(index int) (string, int, string, bool) {
if index < 0 {
return "", index, "invalid_key_index", false
}
+
+ lock := GetChannelPollingLock(channel.Id)
+ lock.Lock()
+ defer lock.Unlock()
keys := channel.GetKeys()
if index >= len(keys) {
return "", index, "key_index_out_of_range", false
}📝 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.
Suggested change
| statusList := channel.ChannelInfo.MultiKeyStatusList | |
| if statusList != nil { | |
| if status, ok := statusList[index]; ok && status != common.ChannelStatusEnabled { | |
| return "", index, fmt.Sprintf("key_status_%d", status), false | |
| } | |
| } | |
| func (channel *Channel) GetEnabledKeyByIndex(index int) (string, int, string, bool) { | |
| if index < 0 { | |
| return "", index, "invalid_key_index", false | |
| } | |
| lock := GetChannelPollingLock(channel.Id) | |
| lock.Lock() | |
| defer lock.Unlock() | |
| keys := channel.GetKeys() | |
| if index >= len(keys) { | |
| return "", index, "key_index_out_of_range", false | |
| } | |
| statusList := channel.ChannelInfo.MultiKeyStatusList | |
| if statusList != nil { | |
| if status, ok := statusList[index]; ok && status != common.ChannelStatusEnabled { | |
| return "", index, fmt.Sprintf("key_status_%d", status), false | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@model/channel.go` around lines 295 - 300, The code at line 295 reads
channel.ChannelInfo.MultiKeyStatusList without acquiring the channel polling
lock, while other operations like GetNextEnabledKey and ManageMultiKeys use
GetChannelPollingLock(channel.Id) to protect concurrent access to this map. Wrap
the statusList access and the subsequent if block with
GetChannelPollingLock(channel.Id) to synchronize this read with other status
reads and writes, preventing potential race conditions and panic on concurrent
map access.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
📝 变更描述 / Description
为多 Key 渠道的模型测试增加 Key 级别测试能力。
本次变更允许前端在渠道测试弹窗中为 multi-key 渠道选择指定 Key 进行模型测试；后端
/api/channel/test/:id支持key_index参数，并在测试上下文中优先使用指定的启用 Key。若指定 Key 不可用或渠道不是 multi-key，会返回明确错误。同时，测试失败且选择了具体 Key 时，前端提供“禁用当前密钥”操作，便于管理员快速处理异常 Key。默认“自动选择”仍保持原有随机/轮询选择逻辑，不影响普通渠道和自动测试流程。
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
(请在此粘贴截图、关键日志或测试报告，以证明变更生效)
Summary by CodeRabbit
New Features
Localization