fix(model): handle string-typed ChannelInfo scan for SQLite - #6311
fix(model): handle string-typed ChannelInfo scan for SQLite#6311fux-dev wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesChannel JSON scanning
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
8cee60a to
2c29a82
Compare
📸 Proof of WorkReproduced locally with an in-memory SQLite DB ( Test (
|
2c29a82 to
89ce3a3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
web/default/src/features/channels/components/dialogs/status-code-risk-dialog.tsx (1)
56-57: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear the confirmation state when the dialog closes.
If the dialog is closed via an overlay click or the Escape key, the underlying
Dialogcomponent callsonOpenChangedirectly, bypassinghandleCancel. This leaves the high-risk checkboxes and confirmation text intact the next time the dialog is opened, which defeats the purpose of the safety checks.Add an effect to clear the state whenever the dialog is closed:
♻️ Proposed fix
const [checkedItems, setCheckedItems] = useState<Set<number>>(new Set()) const [confirmText, setConfirmText] = useState('') + + useEffect(() => { + if (!open) { + setCheckedItems(new Set()) + setConfirmText('') + } + }, [open])(Don't forget to add
useEffectto your React imports on line 20).🤖 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/status-code-risk-dialog.tsx` around lines 56 - 57, Update the status-code risk dialog’s state management to import and use useEffect, clearing checkedItems and confirmText whenever the dialog’s open state becomes false. Keep handleCancel behavior intact while ensuring overlay and Escape closures also reset the confirmation state before the next opening.
🧹 Nitpick comments (1)
web/default/src/features/channels/constants.ts (1)
379-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
MODEL_FETCHABLE_TYPESliteral across files.The exact same
Setliteral is also defined inweb/default/src/features/channels/lib/channel-form.ts(lines 379-381). Both copies were kept in sync in this PR, but duplicating this list risks future drift — one file could add a new fetchable type without the other, desyncing UI gating from settings-building logic.♻️ Suggested consolidation
-export const MODEL_FETCHABLE_TYPES = new Set([ - 1, 4, 14, 17, 20, 23, 24, 25, 26, 27, 31, 34, 35, 40, 42, 43, 47, 48, 57, 58, -]) +export { MODEL_FETCHABLE_TYPES } from './lib/channel-form'(or the inverse — keep one canonical definition and import it from the other file.)
🤖 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/constants.ts` around lines 379 - 381, Consolidate the duplicate MODEL_FETCHABLE_TYPES definitions by keeping one canonical Set and importing/reusing it from the other file. Update both the constants module and channel-form logic so UI gating and settings-building reference the same exported symbol, preserving the existing fetchable type values.
🤖 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 `@controller/channel_upstream_update.go`:
- Around line 307-333: Update getFetchModelsResponseBody to enforce a finite
deadline for the outbound request, preferably by creating a context with timeout
and attaching it to request before client.Do. Ensure the timeout is applied even
when service.NewProxyHttpClient receives an unset RELAY_TIMEOUT, and release the
context cancellation after the request completes.
---
Outside diff comments:
In
`@web/default/src/features/channels/components/dialogs/status-code-risk-dialog.tsx`:
- Around line 56-57: Update the status-code risk dialog’s state management to
import and use useEffect, clearing checkedItems and confirmText whenever the
dialog’s open state becomes false. Keep handleCancel behavior intact while
ensuring overlay and Escape closures also reset the confirmation state before
the next opening.
---
Nitpick comments:
In `@web/default/src/features/channels/constants.ts`:
- Around line 379-381: Consolidate the duplicate MODEL_FETCHABLE_TYPES
definitions by keeping one canonical Set and importing/reusing it from the other
file. Update both the constants module and channel-form logic so UI gating and
settings-building reference the same exported symbol, preserving the existing
fetchable type values.
🪄 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: c6cdcbc7-f37e-4ded-964a-f4173c192b68
📒 Files selected for processing (49)
controller/channel.gocontroller/channel_upstream_update.gocontroller/channel_upstream_update_test.gocontroller/telegram.gocontroller/telegram_test.gocontroller/user.godocs/openapi/api.jsondto/channel_settings.godto/channel_settings_test.gomodel/channel.gomodel/channel_settings_test.gomodel/pricing_default.gomodel/task_cas_test.gomodel/token.gomodel/twofa.gomodel/user.gomodel/user_authentication_test.gomodel/user_pagination_test.gorelay/channel/advancedcustom/adaptor.gorelay/channel/advancedcustom/adaptor_test.gorelay/channel/codex/constants.goservice/codex_channel_models.goservice/codex_models.goservice/relayconvert/internal/oai_responses/to_oai_chat_resp_test.goservice/relayconvert/internal/oai_responses/to_oai_chat_stream_resp.gosetting/ratio_setting/compact_suffix.goweb/default/src/components/data-table/core/data-table-row.tsxweb/default/src/components/data-table/hooks/use-data-table.tsweb/default/src/features/channels/api.tsweb/default/src/features/channels/components/dialogs/advanced-custom-editor-dialog.tsxweb/default/src/features/channels/components/dialogs/status-code-risk-dialog.tsxweb/default/src/features/channels/components/drawers/channel-mutate-drawer.tsxweb/default/src/features/channels/constants.tsweb/default/src/features/channels/lib/advanced-custom.tsweb/default/src/features/channels/lib/channel-form.tsweb/default/src/features/models/components/models-table.tsxweb/default/src/features/users/api.tsweb/default/src/features/users/components/user-quota-cell.tsxweb/default/src/features/users/components/users-columns.tsxweb/default/src/features/users/components/users-table.tsxweb/default/src/features/users/types.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-TW.jsonweb/default/src/i18n/locales/zh.jsonweb/default/src/i18n/static-keys.ts
💤 Files with no reviewable changes (1)
- web/default/src/features/models/components/models-table.tsx
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 (1)
web/default/src/features/channels/components/dialogs/status-code-risk-dialog.tsx (1)
56-57: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear the confirmation state when the dialog closes.
If the dialog is closed via an overlay click or the Escape key, the underlying
Dialogcomponent callsonOpenChangedirectly, bypassinghandleCancel. This leaves the high-risk checkboxes and confirmation text intact the next time the dialog is opened, which defeats the purpose of the safety checks.Add an effect to clear the state whenever the dialog is closed:
♻️ Proposed fix
const [checkedItems, setCheckedItems] = useState<Set<number>>(new Set()) const [confirmText, setConfirmText] = useState('') + + useEffect(() => { + if (!open) { + setCheckedItems(new Set()) + setConfirmText('') + } + }, [open])(Don't forget to add
useEffectto your React imports on line 20).🤖 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/status-code-risk-dialog.tsx` around lines 56 - 57, Update the status-code risk dialog’s state management to import and use useEffect, clearing checkedItems and confirmText whenever the dialog’s open state becomes false. Keep handleCancel behavior intact while ensuring overlay and Escape closures also reset the confirmation state before the next opening.
🧹 Nitpick comments (1)
web/default/src/features/channels/constants.ts (1)
379-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
MODEL_FETCHABLE_TYPESliteral across files.The exact same
Setliteral is also defined inweb/default/src/features/channels/lib/channel-form.ts(lines 379-381). Both copies were kept in sync in this PR, but duplicating this list risks future drift — one file could add a new fetchable type without the other, desyncing UI gating from settings-building logic.♻️ Suggested consolidation
-export const MODEL_FETCHABLE_TYPES = new Set([ - 1, 4, 14, 17, 20, 23, 24, 25, 26, 27, 31, 34, 35, 40, 42, 43, 47, 48, 57, 58, -]) +export { MODEL_FETCHABLE_TYPES } from './lib/channel-form'(or the inverse — keep one canonical definition and import it from the other file.)
🤖 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/constants.ts` around lines 379 - 381, Consolidate the duplicate MODEL_FETCHABLE_TYPES definitions by keeping one canonical Set and importing/reusing it from the other file. Update both the constants module and channel-form logic so UI gating and settings-building reference the same exported symbol, preserving the existing fetchable type values.
🤖 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 `@controller/channel_upstream_update.go`:
- Around line 307-333: Update getFetchModelsResponseBody to enforce a finite
deadline for the outbound request, preferably by creating a context with timeout
and attaching it to request before client.Do. Ensure the timeout is applied even
when service.NewProxyHttpClient receives an unset RELAY_TIMEOUT, and release the
context cancellation after the request completes.
---
Outside diff comments:
In
`@web/default/src/features/channels/components/dialogs/status-code-risk-dialog.tsx`:
- Around line 56-57: Update the status-code risk dialog’s state management to
import and use useEffect, clearing checkedItems and confirmText whenever the
dialog’s open state becomes false. Keep handleCancel behavior intact while
ensuring overlay and Escape closures also reset the confirmation state before
the next opening.
---
Nitpick comments:
In `@web/default/src/features/channels/constants.ts`:
- Around line 379-381: Consolidate the duplicate MODEL_FETCHABLE_TYPES
definitions by keeping one canonical Set and importing/reusing it from the other
file. Update both the constants module and channel-form logic so UI gating and
settings-building reference the same exported symbol, preserving the existing
fetchable type values.
🪄 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: c6cdcbc7-f37e-4ded-964a-f4173c192b68
📒 Files selected for processing (49)
controller/channel.gocontroller/channel_upstream_update.gocontroller/channel_upstream_update_test.gocontroller/telegram.gocontroller/telegram_test.gocontroller/user.godocs/openapi/api.jsondto/channel_settings.godto/channel_settings_test.gomodel/channel.gomodel/channel_settings_test.gomodel/pricing_default.gomodel/task_cas_test.gomodel/token.gomodel/twofa.gomodel/user.gomodel/user_authentication_test.gomodel/user_pagination_test.gorelay/channel/advancedcustom/adaptor.gorelay/channel/advancedcustom/adaptor_test.gorelay/channel/codex/constants.goservice/codex_channel_models.goservice/codex_models.goservice/relayconvert/internal/oai_responses/to_oai_chat_resp_test.goservice/relayconvert/internal/oai_responses/to_oai_chat_stream_resp.gosetting/ratio_setting/compact_suffix.goweb/default/src/components/data-table/core/data-table-row.tsxweb/default/src/components/data-table/hooks/use-data-table.tsweb/default/src/features/channels/api.tsweb/default/src/features/channels/components/dialogs/advanced-custom-editor-dialog.tsxweb/default/src/features/channels/components/dialogs/status-code-risk-dialog.tsxweb/default/src/features/channels/components/drawers/channel-mutate-drawer.tsxweb/default/src/features/channels/constants.tsweb/default/src/features/channels/lib/advanced-custom.tsweb/default/src/features/channels/lib/channel-form.tsweb/default/src/features/models/components/models-table.tsxweb/default/src/features/users/api.tsweb/default/src/features/users/components/user-quota-cell.tsxweb/default/src/features/users/components/users-columns.tsxweb/default/src/features/users/components/users-table.tsxweb/default/src/features/users/types.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-TW.jsonweb/default/src/i18n/locales/zh.jsonweb/default/src/i18n/static-keys.ts
💤 Files with no reviewable changes (1)
- web/default/src/features/models/components/models-table.tsx
🛑 Comments failed to post (1)
controller/channel_upstream_update.go (1)
307-333: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Check whether NewProxyHttpClient sets an http.Client Timeout. rg -n -A 25 'func NewProxyHttpClient' --type=goRepository: QuantumNous/new-api
Length of output: 157
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the proxy HTTP client implementation and related timeout settings. git ls-files | rg '(^|/)(service|controller)/.*\.(go|ts|js)$|^service/.*\.go$|^controller/channel_upstream_update\.go$|codex_channel_models\.go$' echo '---' rg -n 'NewProxyHttpClient|http\.Client|Timeout|WithTimeout|NewRequestWithContext' service controller --type=goRepository: QuantumNous/new-api
Length of output: 18211
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the proxy client implementation and the RelayTimeout default/configuration. sed -n '120,220p' service/http_client.go echo '---' rg -n 'RelayTimeout' --type=go --type=toml --type=yaml --type=yml --type=json --type=ini .Repository: QuantumNous/new-api
Length of output: 2760
🏁 Script executed:
#!/bin/bash set -euo pipefail # Check whether the fetch-models path already has a timeout elsewhere in the call chain. sed -n '300,470p' controller/channel_upstream_update.go echo '---' sed -n '1,120p' service/codex_channel_models.goRepository: QuantumNous/new-api
Length of output: 8057
🏁 Script executed:
#!/bin/bash set -euo pipefail # Check the default value and configuration source for common.RelayTimeout. rg -n 'RelayTimeout' --type=go common setting config constant . echo '---' sed -n '1,120p' service/http_client.goRepository: QuantumNous/new-api
Length of output: 1310
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect where common.RelayTimeout is defined/initialized. rg -n 'RelayTimeout\s*[:=]|type .*RelayTimeout|var .*RelayTimeout|const .*RelayTimeout' --type=go . echo '---' rg -n 'RelayTimeout' common setting constant --type=goRepository: QuantumNous/new-api
Length of output: 492
Add a deadline to this shared fetch helper
service.NewProxyHttpClientonly appliesclient.TimeoutwhenRELAY_TIMEOUTis set; the default is0, so this/api/channel/fetch_modelspath can still block indefinitely on a slow upstream. Add a per-request context timeout here or require a nonzero proxy timeout.🤖 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 `@controller/channel_upstream_update.go` around lines 307 - 333, Update getFetchModelsResponseBody to enforce a finite deadline for the outbound request, preferably by creating a context with timeout and attaching it to request before client.Do. Ensure the timeout is applied even when service.NewProxyHttpClient receives an unset RELAY_TIMEOUT, and release the context cancellation after the request completes.
1 participant
📝 Description
ChannelInfois a JSON custom type attached toChannel(model/channel.go:54,gorm:"type:json"), read/written throughdriver.Valuer/sql.Scanner. The originalScanonly asserted a single type,[]byte:But the
valuepassed toScanis determined by the underlying driver, not by GORM:[]bytefor JSON columns — the assertion succeeds and everything works.stringfor TEXT columns — the assertion fails,bytesValueisnil,common.Unmarshal(nil, c)returns immediately, andChannelInfostays at its zero value. The field is silently dropped.Trigger scenario: importing a MySQL database into a SQLite deployment. The MySQL
JSONcolumn is materialized by migration tooling as a SQLiteTEXTcolumn (SQLite has no native JSON type), still holding valid JSON text. On read, the SQLite driver returns astring, which never enters the[]bytebranch. As a result, all multi-key fields on the channel —IsMultiKey,MultiKeySize,MultiKeyStatusList,MultiKeyPollingIndex, etc. — come back empty, and every multi-key branch inmiddleware/distributor.goandcontroller/channel.go(key polling, disable state, recovery) stops taking effect.Fix: add a
stringbranch at the top ofScanand decode via the project's standardcommon.UnmarshalJsonStrwrapper; the[]bytepath is unchanged.🚀 Type of change
🔗 Related Issue
model/channel.goonmainstill has the[]byte-onlyScan, and there is no matching issue/PR. Happy to open one if the maintainers prefer.✅ Checklist
ChannelInfo.Scanstill does not handle thestringtype.string/[]bytebranches correspond to the SQLite vs. MySQL/Postgres driver return types per thedatabase/sqlScanner contract; behavior on MySQL/Postgres is unchanged.model/channel.go), +5 lines, no unrelated changes.ChannelInfofields.common.UnmarshalJsonStrwrapper per the JSON convention.📸 Proof of Work
(to be attached: before/after logs from a SQLite deployment whose DB was imported from MySQL)
Summary by CodeRabbit