Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 40 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request enhances session usage analytics by expanding Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
🧹 Nitpick comments (2)
tests/unit/provider-share-command.test.mjs (1)
163-165: Add a focused unit case forisDefaultLocalProvider() === true.These new bindings force every config-template unit test down the non-local branch, so the local-provider path added in this PR is still only exercised by e2e. A small unit case for
getConfigTemplate(...)/applyConfigTemplate(...)withisDefaultLocalProvider()returningtruewould catch local template regressions much faster.Also applies to: 256-258, 309-311, 367-369
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/provider-share-command.test.mjs` around lines 163 - 165, Add a focused unit test that covers the local-provider branch by creating a test case where the mocked provider's isDefaultLocalProvider() returns true and then invoking getConfigTemplate(...) and applyConfigTemplate(...) to verify the local-template behavior; modify or add a test block in the provider-share-command unit tests to stub isDefaultLocalProvider() => true (alongside the existing non-local cases), call getConfigTemplate and applyConfigTemplate with the same inputs used by other tests, and assert the expected local-path outputs/side-effects so local-template regressions are exercised in unit tests rather than only in e2e.tests/unit/providers-validation.test.mjs (1)
131-151: Cover the blankauthStatusTextfallback too.The production branch also trims
authStatusTextand falls back to'已登录'/'未登录'when the field is missing or whitespace, but this test only exercises the explicit-string path. One extra assertion withauthStatusText: ' 'would lock down the new fallback behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/providers-validation.test.mjs` around lines 131 - 151, Add a test asserting that providerPillState falls back to the default labels when authStatusText is blank/whitespace: call context.providerPillState with name: 'local', authConfigured: true, authStatusText: ' ' and assert it returns { configured: true, text: '已登录' } (this ensures providerPillState's trim-and-fallback logic is covered).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 3841-3842: The code is persisting the reserved provider name
"local" which breaks import/export round-tripping; when calling
writeConfig(finalTemplate + '\n') and
updateAuthJson(activeProviderBlock.preferred_auth_method || ''), ensure you do
not emit or persist a [model_providers.local] entry or set preferred_auth_method
to 'local'. Modify the write path (around writeConfig and updateAuthJson) to
detect if the active provider block or preferred_auth_method equals 'local' and
skip writing that provider or replacing it with a non-reserved placeholder;
alternatively strip any "local" provider stanza from finalTemplate before
calling writeConfig and avoid calling updateAuthJson with 'local' so
cmdAdd/cmdUpdate (which reject 'local') are not required to import it.
- Around line 6659-6661: The local provider proxy must not be run through the
persisted generic proxy path because
ensureBuiltinProxyRuntime(buildLocalProviderProxySettings()) writes
codexmate-proxy.json and can call saveBuiltinProxySettings(), which overwrites
settings.provider away from "local" and breaks reuse; change
ensureLocalProviderProxyRuntime to bypass the generic persistence flow — either
invoke a dedicated non-persisting startup for the local runtime or call
ensureBuiltinProxyRuntime with a no-persist flag so it does not write
codexmate-proxy.json or call saveBuiltinProxySettings(), ensuring the local
runtime is started/reused without touching persisted settings.
- Around line 6066-6077: The base_url in buildLocalProviderConfigEntry is being
built from the proxy listen host verbatim, which yields unconnectable wildcard
addresses like "0.0.0.0" or "::"; update buildLocalProviderConfigEntry (or the
helper buildBuiltinProxyProviderBaseUrl call) to normalize wildcard listen hosts
into a connectable host (e.g., replace '0.0.0.0' and '::' with '127.0.0.1' or
'localhost') before generating the URL so clients get a usable address; use
buildLocalProviderProxySettings to read the original settings, detect wildcard
hosts, substitute a safe loopback name, then pass the normalized settings into
buildBuiltinProxyProviderBaseUrl when constructing base_url.
In `@tests/e2e/test-config.js`:
- Around line 289-291: The test is removing importPayload.providers.local which
masks a regression; instead restore the test to use the raw exported payload and
fix the product path: update the export-config path (the export-config
routine/function) to filter out reserved/system providers (e.g., the
always-present "local") before emitting the exported JSON, or alternatively
update the import-config logic to ignore reserved provider keys on import;
locate the code that builds/returns providers in export-config or the importer
that consumes importPayload.providers and add a RESERVED_PROVIDERS set
(including "local") and skip those keys so the exported payload remains directly
consumable and the test can keep the unmodified importPayload.providers.local
reference.
---
Nitpick comments:
In `@tests/unit/provider-share-command.test.mjs`:
- Around line 163-165: Add a focused unit test that covers the local-provider
branch by creating a test case where the mocked provider's
isDefaultLocalProvider() returns true and then invoking getConfigTemplate(...)
and applyConfigTemplate(...) to verify the local-template behavior; modify or
add a test block in the provider-share-command unit tests to stub
isDefaultLocalProvider() => true (alongside the existing non-local cases), call
getConfigTemplate and applyConfigTemplate with the same inputs used by other
tests, and assert the expected local-path outputs/side-effects so local-template
regressions are exercised in unit tests rather than only in e2e.
In `@tests/unit/providers-validation.test.mjs`:
- Around line 131-151: Add a test asserting that providerPillState falls back to
the default labels when authStatusText is blank/whitespace: call
context.providerPillState with name: 'local', authConfigured: true,
authStatusText: ' ' and assert it returns { configured: true, text: '已登录' }
(this ensures providerPillState's trim-and-fallback logic is covered).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ad1883d-38dc-449e-ada6-5a5e3ffae72d
📒 Files selected for processing (6)
cli.jstests/e2e/test-config.jstests/unit/provider-share-command.test.mjstests/unit/providers-validation.test.mjsweb-ui/modules/app.methods.providers.mjsweb-ui/partials/index/panel-config-codex.html
cli.js
Outdated
| function buildLocalProviderConfigEntry(overrides = {}) { | ||
| const settings = buildLocalProviderProxySettings(overrides); | ||
| return { | ||
| name: DEFAULT_LOCAL_PROVIDER_NAME, | ||
| base_url: buildBuiltinProxyProviderBaseUrl(settings), | ||
| wire_api: 'responses', | ||
| requires_openai_auth: false, | ||
| preferred_auth_method: '', | ||
| request_max_retries: 4, | ||
| stream_max_retries: 10, | ||
| stream_idle_timeout_ms: 300000 | ||
| }; |
There was a problem hiding this comment.
Use a connectable host when generating the local provider URL.
This builds base_url from the proxy listen host verbatim. If the persisted proxy host is 0.0.0.0 or ::, the generated URL becomes a wildcard bind address, which clients cannot reliably connect to. That breaks the template, provider list, and local model discovery for anyone who configured the proxy to listen on all interfaces.
💡 Proposed fix
function buildLocalProviderConfigEntry(overrides = {}) {
const settings = buildLocalProviderProxySettings(overrides);
+ const clientSettings = {
+ ...settings,
+ host: settings.host === '0.0.0.0'
+ ? '127.0.0.1'
+ : (settings.host === '::' ? '::1' : settings.host)
+ };
return {
name: DEFAULT_LOCAL_PROVIDER_NAME,
- base_url: buildBuiltinProxyProviderBaseUrl(settings),
+ base_url: buildBuiltinProxyProviderBaseUrl(clientSettings),
wire_api: 'responses',
requires_openai_auth: false,
preferred_auth_method: '',
request_max_retries: 4,📝 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.
| function buildLocalProviderConfigEntry(overrides = {}) { | |
| const settings = buildLocalProviderProxySettings(overrides); | |
| return { | |
| name: DEFAULT_LOCAL_PROVIDER_NAME, | |
| base_url: buildBuiltinProxyProviderBaseUrl(settings), | |
| wire_api: 'responses', | |
| requires_openai_auth: false, | |
| preferred_auth_method: '', | |
| request_max_retries: 4, | |
| stream_max_retries: 10, | |
| stream_idle_timeout_ms: 300000 | |
| }; | |
| function buildLocalProviderConfigEntry(overrides = {}) { | |
| const settings = buildLocalProviderProxySettings(overrides); | |
| const clientSettings = { | |
| ...settings, | |
| host: settings.host === '0.0.0.0' | |
| ? '127.0.0.1' | |
| : (settings.host === '::' ? '::1' : settings.host) | |
| }; | |
| return { | |
| name: DEFAULT_LOCAL_PROVIDER_NAME, | |
| base_url: buildBuiltinProxyProviderBaseUrl(clientSettings), | |
| wire_api: 'responses', | |
| requires_openai_auth: false, | |
| preferred_auth_method: '', | |
| request_max_retries: 4, | |
| stream_max_retries: 10, | |
| stream_idle_timeout_ms: 300000 | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.js` around lines 6066 - 6077, The base_url in
buildLocalProviderConfigEntry is being built from the proxy listen host
verbatim, which yields unconnectable wildcard addresses like "0.0.0.0" or "::";
update buildLocalProviderConfigEntry (or the helper
buildBuiltinProxyProviderBaseUrl call) to normalize wildcard listen hosts into a
connectable host (e.g., replace '0.0.0.0' and '::' with '127.0.0.1' or
'localhost') before generating the URL so clients get a usable address; use
buildLocalProviderProxySettings to read the original settings, detect wildcard
hosts, substitute a safe loopback name, then pass the normalized settings into
buildBuiltinProxyProviderBaseUrl when constructing base_url.
tests/e2e/test-config.js
Outdated
| if (importPayload.providers && importPayload.providers.local) { | ||
| delete importPayload.providers.local; | ||
| } |
There was a problem hiding this comment.
This test is masking an export/import round-trip regression.
Deleting importPayload.providers.local here means the raw output of export-config is no longer directly consumable by import-config after introducing the always-present reserved local provider. That breaks real backup/share flows. Please fix the product path by filtering reserved/system providers during export or ignoring them during import, then keep this test using the unmodified exported payload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test-config.js` around lines 289 - 291, The test is removing
importPayload.providers.local which masks a regression; instead restore the test
to use the raw exported payload and fix the product path: update the
export-config path (the export-config routine/function) to filter out
reserved/system providers (e.g., the always-present "local") before emitting the
exported JSON, or alternatively update the import-config logic to ignore
reserved provider keys on import; locate the code that builds/returns providers
in export-config or the importer that consumes importPayload.providers and add a
RESERVED_PROVIDERS set (including "local") and skip those keys so the exported
payload remains directly consumable and the test can keep the unmodified
importPayload.providers.local reference.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web-ui/partials/index/modals-basic.html (1)
144-161: Consider using a computed property for the selected item.The
v-for="selectedItem in [getSelectedLocalProviderItem()]"pattern is unconventional. While functional, it could be clearer as a computed property that returns the selected item, then usev-if="selectedLocalProviderItem"with direct property access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/partials/index/modals-basic.html` around lines 144 - 161, Replace the unconventional v-for wrapper that calls getSelectedLocalProviderItem() with a computed property and direct v-if binding: add a computed property (e.g., selectedLocalProviderItem) that returns getSelectedLocalProviderItem(), then change the template to use v-if="selectedLocalProviderItem" (and :key="selectedLocalProviderItem ? selectedLocalProviderItem.id : 'local-empty'") and update all references from selectedItem to selectedLocalProviderItem so v-model bindings (selectedLocalProviderItem.name, .model, .baseUrl) and the `@blur` handler normalizeLocalProviderDraft continue to work seamlessly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-ui/partials/index/modals-basic.html`:
- Around line 144-161: Replace the unconventional v-for wrapper that calls
getSelectedLocalProviderItem() with a computed property and direct v-if binding:
add a computed property (e.g., selectedLocalProviderItem) that returns
getSelectedLocalProviderItem(), then change the template to use
v-if="selectedLocalProviderItem" (and :key="selectedLocalProviderItem ?
selectedLocalProviderItem.id : 'local-empty'") and update all references from
selectedItem to selectedLocalProviderItem so v-model bindings
(selectedLocalProviderItem.name, .model, .baseUrl) and the `@blur` handler
normalizeLocalProviderDraft continue to work seamlessly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0deac6bd-9df8-4afc-94f4-a702a80d4dfa
📒 Files selected for processing (8)
cli.jstests/e2e/test-config.jstests/unit/providers-validation.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/modules/app.methods.providers.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-codex.html
✅ Files skipped from review due to trivial changes (1)
- cli.js
📜 Review details
🔇 Additional comments (23)
tests/e2e/test-config.js (3)
379-381: This workaround should be addressed in the product code.As flagged in a previous review, deleting
importPayload.providers.localhere masks an export/import round-trip issue. The exported payload contains the reservedlocalprovider, but importing it would fail or cause conflicts.The proper fix is to either:
- Filter reserved/system providers (like
local) duringexport-config, or- Ignore reserved provider keys during
import-configThis keeps the test using the raw exported payload and ensures real backup/share flows work correctly.
6-45: LGTM!The
requestJsonhelper is well-implemented for direct HTTP requests needed to test proxy forwarding behavior. It properly handles JSON encoding/decoding and exposes both parsed and raw response data.
77-187: LGTM!Excellent end-to-end test coverage for the local provider flow:
- Validates provider listing invariants (
readOnly,nonDeletable,authConfigured)- Tests local config CRUD operations
- Verifies template generation with correct TOML fragments
- Tests OAuth credential flow by writing mock credentials and verifying state changes
- Validates proxy forwarding with proper header assertions (Bearer token,
x-dashscope-authtype)- Properly restores the primary provider after tests
The tests thoroughly exercise the integration between the UI, API, and proxy components.
web-ui/partials/index/panel-config-codex.html (2)
174-174: LGTM!The subtitle fallback chain (
authHint || provider.url || '未设 URL') correctly prioritizes the OAuth status hint for local providers while maintaining backward compatibility for other providers.
220-230: LGTM!The new local provider configuration button is properly implemented with appropriate accessibility attributes (
aria-labelandtitle), and thev-if="isLocalLikeProvider(provider)"condition ensures it only appears for local-like providers.web-ui/app.js (2)
57-57: LGTM!The
showLocalProviderModalflag is correctly placed alongside other modal visibility flags.
243-262: LGTM!The local provider state structure is well-designed:
- Loading/saving flags for proper UI feedback
- Config structure with
selectedItemIdanditemsmatches the backend schema- OAuth status fields capture all relevant credential information
- Runtime state tracks proxy status appropriately
The default values are sensible and align with the backend's
buildLocalProviderConfigState()response structure.web-ui/partials/index/modals-basic.html (3)
73-105: LGTM!The Qwen OAuth status section is well-structured with:
- Clear visual status indicator using the existing pill component
- Informative subtitle showing email, resource URL, or error state
- Helpful hints for credential file path and login command
- Action buttons for copying the command and refreshing status
107-142: LGTM!The local CPA items list implementation is solid:
- Proper selection handling with visual feedback
- Delete button correctly disabled when only one item remains (enforcing min-items constraint)
- Add button for creating new configuration items
169-172: LGTM!The modal action buttons properly disable during loading/saving operations to prevent race conditions and duplicate submissions.
tests/unit/web-ui-behavior-parity.test.mjs (2)
322-349: LGTM!The parity test allowlists are correctly updated to include all new local provider data keys for both HEAD and non-HEAD comparison branches.
378-409: LGTM!The method allowlist correctly includes all new local provider methods, maintaining consistency with the implementation in
app.methods.providers.mjs.tests/unit/providers-validation.test.mjs (3)
13-25: LGTM!The test context is properly extended with all required local provider state fields and the
fallbackCopyTextstub needed for clipboard operations.
139-170: LGTM!Good test coverage for local provider UI state:
providerPillStatecorrectly mapsauthConfigured/authStatusTextfrom the provider payloadshouldShowProviderEditcorrectly returnstruefor local providers even when marked asreadOnlyandnonDeletable
172-198: LGTM!The
openEditModalrouting test effectively verifies:
- Local providers trigger the
get-local-provider-configAPI call- The local provider modal opens (
showLocalProviderModal = true)- The standard edit modal stays closed (
showEditModal = false)- Config state is properly populated from the API response
web-ui/modules/app.methods.providers.mjs (8)
4-4: LGTM!The constant
DEFAULT_LOCAL_PROVIDER_MODEL = 'coder-model'correctly matches the backend'sDEFAULT_QWEN_LOCAL_MODELdefined incli.js.
53-79: LGTM!The normalization function properly handles all item fields:
- Generates unique IDs when missing (using timestamp + index for collision avoidance)
- Provides sensible defaults for name and model
- Forces
runtimeto'qwen-oauth'to match the backend expectation- Correctly handles the
enabledboolean default
208-221: LGTM!The
providerPillStatecorrectly handles local providers by deriving status fromauthConfiguredandauthStatusTextfields, with appropriate fallback text.
265-270: LGTM!Correctly overrides the edit visibility check for local-like providers, allowing them to show the config button despite being marked as
readOnly.
276-328: LGTM!The local provider item management methods are well-implemented:
createDefaultLocalProviderItemgenerates proper default itemsgetSelectedLocalProviderItemhandles missing/empty config gracefullyselectLocalProviderItem,addLocalProviderItem,removeLocalProviderItemmaintain proper state invariantsnormalizeLocalProviderDraftensures all items have required fields
330-377: LGTM!The
loadLocalProviderConfigmethod follows good async patterns:
- Sets loading flag before async operation
- Proper error handling with user feedback
- Normalizes response data defensively
- Uses
finallyto ensure loading flag is cleared- Returns boolean success indicator for callers
379-431: LGTM!The modal and save methods are solid:
openLocalProviderModalwaits for config load before showing modalsaveLocalProviderConfigguards against concurrent saves and normalizes draft before API callcopyLocalProviderTextproperly handles clipboard API with secure context check and fallback
455-472: LGTM!The
openEditModalmethod correctly routes local-like providers to the dedicated local provider modal while maintaining the standard edit flow for other providers.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
cli.js (3)
6264-6281:⚠️ Potential issue | 🔴 CriticalForce the managed local proxy onto loopback.
Line 6266 inherits the generic proxy bind host verbatim. If the saved host is
0.0.0.0or::, the generatedbase_urlis not client-connectable, and the local Qwen OAuth proxy can end up exposed on all interfaces without auth while injecting the user's bearer token upstream. The managedlocalprovider should use a loopback host instead of reusing the generic bind address.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 6264 - 6281, The buildLocalProviderProxySettings/buildLocalProviderConfigEntry flow currently inherits the generic bind host which can be 0.0.0.0 or :: and produce a non-client-connectable or exposed base_url; force the managed local proxy to use a loopback address by overriding the host in the settings returned by buildLocalProviderProxySettings (e.g., set host to a loopback like 127.0.0.1 or ::1) before passing settings into buildBuiltinProxyProviderBaseUrl, ensuring the local provider always binds/advertises a loopback-only base_url rather than reusing the generic bind host.
4037-4050:⚠️ Potential issue | 🟠 MajorKeep the managed
localprovider out of persisted Codex config.Writing
[model_providers.local]here still breaks config round-tripping:buildExportPayload()exports it, butimportConfigData()routes reserved providers throughcmdAdd()/cmdUpdate(), which rejectlocal. Line 4050 also clearsauth.jsonwhenlocalis active because its managed block carries an emptypreferred_auth_method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 4037 - 4050, The code is persisting the managed local provider block; when isDefaultLocalProvider(activeProvider) is true you must NOT append the managed local block to the exported config. Change the flow in the isDefaultLocalProvider branch so that ensureLocalProviderTemplate is used only at runtime (for in-memory finalTemplate) but is NOT added to the string passed to writeConfig; alternatively strip any [model_providers.local] section before calling writeConfig and ensure updateAuthJson uses a non-managed preferred_auth_method (use the original activeProviderBlock value rather than the managed block). Update references: isDefaultLocalProvider, ensureLocalProviderTemplate, writeConfig, updateAuthJson, and activeProviderBlock.
7545-7574:⚠️ Potential issue | 🟠 MajorStart the local runtime without touching
codexmate-proxy.json.
ensureLocalProviderProxyRuntime()still goes throughensureBuiltinProxyRuntime(), which persists settings viastartBuiltinProxyRuntime(). Whenlocalis not yet present inconfig.model_providers,saveBuiltinProxySettings()rewritesprovideraway fromlocal; on the next ensure, the runtime no longer matches and gets restarted instead of reused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli.js` around lines 7545 - 7574, ensureLocalProviderProxyRuntime should start the local proxy without going through ensureBuiltinProxyRuntime (which persists settings); change ensureLocalProviderProxyRuntime to build and normalize the local settings via buildLocalProviderProxySettings() and normalizeBuiltinProxySettings(), then call startBuiltinProxyRuntime() directly with those normalized settings (or add an options flag to startBuiltinProxyRuntime like { persist: false } and pass it) so that saveBuiltinProxySettings() is not invoked and codexmate-proxy.json is not modified; reference functions: ensureLocalProviderProxyRuntime, ensureBuiltinProxyRuntime, startBuiltinProxyRuntime, saveBuiltinProxySettings, buildLocalProviderProxySettings, normalizeBuiltinProxySettings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli.js`:
- Around line 6715-6721: The readRequestBody function currently buffers the
entire request with no limit; update it to enforce a maximum by using the
existing MAX_API_BODY_SIZE constant (or define a clear ceiling) — track total
bytes as you receive chunks in readRequestBody, and if totalBytes >
MAX_API_BODY_SIZE immediately destroy the incoming req, reject the Promise with
a clear "Payload Too Large" / HTTP 413 style error, and avoid concatenating
further chunks; reference the readRequestBody function and the MAX_API_BODY_SIZE
symbol when making this change so large POST bodies fail fast instead of causing
OOM.
- Around line 6689-6708: The proxy headers currently allow upstream to send
compressed responses which the downstream parsing (in handleLocalResponsesProxy
and safeJsonParseBuffer) doesn't decode; update buildQwenProxyHeaders to not
advertise or forward Accept-Encoding (i.e., remove any incoming Accept-Encoding
and do not set one from QWEN_PROXY_DEFAULT_HEADERS) so upstream returns
uncompressed bodies, or alternatively detect content-encoding in
handleLocalResponsesProxy and decompress before calling
safeJsonParseBuffer/setEncoding — reference buildQwenProxyHeaders (and the
parsing code paths in handleLocalResponsesProxy / safeJsonParseBuffer) and
implement the simpler fix by dropping/omitting Accept-Encoding from the headers
you send upstream.
---
Duplicate comments:
In `@cli.js`:
- Around line 6264-6281: The
buildLocalProviderProxySettings/buildLocalProviderConfigEntry flow currently
inherits the generic bind host which can be 0.0.0.0 or :: and produce a
non-client-connectable or exposed base_url; force the managed local proxy to use
a loopback address by overriding the host in the settings returned by
buildLocalProviderProxySettings (e.g., set host to a loopback like 127.0.0.1 or
::1) before passing settings into buildBuiltinProxyProviderBaseUrl, ensuring the
local provider always binds/advertises a loopback-only base_url rather than
reusing the generic bind host.
- Around line 4037-4050: The code is persisting the managed local provider
block; when isDefaultLocalProvider(activeProvider) is true you must NOT append
the managed local block to the exported config. Change the flow in the
isDefaultLocalProvider branch so that ensureLocalProviderTemplate is used only
at runtime (for in-memory finalTemplate) but is NOT added to the string passed
to writeConfig; alternatively strip any [model_providers.local] section before
calling writeConfig and ensure updateAuthJson uses a non-managed
preferred_auth_method (use the original activeProviderBlock value rather than
the managed block). Update references: isDefaultLocalProvider,
ensureLocalProviderTemplate, writeConfig, updateAuthJson, and
activeProviderBlock.
- Around line 7545-7574: ensureLocalProviderProxyRuntime should start the local
proxy without going through ensureBuiltinProxyRuntime (which persists settings);
change ensureLocalProviderProxyRuntime to build and normalize the local settings
via buildLocalProviderProxySettings() and normalizeBuiltinProxySettings(), then
call startBuiltinProxyRuntime() directly with those normalized settings (or add
an options flag to startBuiltinProxyRuntime like { persist: false } and pass it)
so that saveBuiltinProxySettings() is not invoked and codexmate-proxy.json is
not modified; reference functions: ensureLocalProviderProxyRuntime,
ensureBuiltinProxyRuntime, startBuiltinProxyRuntime, saveBuiltinProxySettings,
buildLocalProviderProxySettings, normalizeBuiltinProxySettings.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6443369-47e0-4aa9-b8fc-e10065375aef
📒 Files selected for processing (3)
cli.jstests/e2e/run.jstests/e2e/test-config.js
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/test-config.js
📜 Review details
🔇 Additional comments (2)
tests/e2e/run.js (2)
61-83: Good move to use an explicit chat-completions fixture for the mock provider.This makes the E2E path more realistic and deterministic for proxy translation behavior instead of relying on implicit defaults.
108-109: Nice context wiring for request-level proxy assertions.Exposing
mockProviderRequestsandmockProviderRequestDetailsenables precise downstream checks for forwarded path and headers.
cli.js
Outdated
| function buildQwenProxyHeaders(req, token) { | ||
| const incoming = req && req.headers && typeof req.headers === 'object' ? req.headers : {}; | ||
| const headers = { ...incoming }; | ||
| delete headers.host; | ||
| delete headers.connection; | ||
| delete headers['content-length']; | ||
| delete headers.authorization; | ||
| delete headers['x-api-key']; | ||
|
|
||
| const acceptHeader = typeof incoming.accept === 'string' ? incoming.accept.toLowerCase() : ''; | ||
| const wantsStream = acceptHeader.includes('text/event-stream'); | ||
| for (const [key, value] of Object.entries(QWEN_PROXY_DEFAULT_HEADERS)) { | ||
| headers[key] = value; | ||
| } | ||
| headers.Authorization = /^bearer\s+/i.test(token || '') ? token : `Bearer ${token}`; | ||
| headers.Accept = wantsStream ? 'text/event-stream' : 'application/json'; | ||
| headers['Content-Type'] = typeof incoming['content-type'] === 'string' && incoming['content-type'].trim() | ||
| ? incoming['content-type'] | ||
| : 'application/json'; | ||
| headers['x-codexmate-proxy'] = '1'; |
There was a problem hiding this comment.
Don’t request compressed upstream responses unless you decode them.
handleLocalResponsesProxy() parses the upstream body as plain JSON/SSE, but these headers still advertise gzip, deflate. If the upstream compresses either the normal or streaming response, safeJsonParseBuffer() / setEncoding('utf-8') will read compressed bytes and the adapter will fail or emit garbage. Drop Accept-Encoding here, or branch on content-encoding and decompress before parsing.
💡 Minimal safe fix
const QWEN_PROXY_DEFAULT_HEADERS = Object.freeze({
'X-Stainless-Runtime-Version': 'v22.17.0',
'User-Agent': 'QwenCode/0.0.0 codexmate-local-proxy',
'X-Stainless-Lang': 'js',
'Accept-Language': '*',
@@
- 'Accept-Encoding': 'gzip, deflate',
'X-Stainless-Package-Version': '5.11.0',
'Sec-Fetch-Mode': 'cors',
'Connection': 'keep-alive',
'X-Dashscope-Useragent': 'QwenCode/0.0.0 codexmate-local-proxy'
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.js` around lines 6689 - 6708, The proxy headers currently allow upstream
to send compressed responses which the downstream parsing (in
handleLocalResponsesProxy and safeJsonParseBuffer) doesn't decode; update
buildQwenProxyHeaders to not advertise or forward Accept-Encoding (i.e., remove
any incoming Accept-Encoding and do not set one from QWEN_PROXY_DEFAULT_HEADERS)
so upstream returns uncompressed bodies, or alternatively detect
content-encoding in handleLocalResponsesProxy and decompress before calling
safeJsonParseBuffer/setEncoding — reference buildQwenProxyHeaders (and the
parsing code paths in handleLocalResponsesProxy / safeJsonParseBuffer) and
implement the simpler fix by dropping/omitting Accept-Encoding from the headers
you send upstream.
cli.js
Outdated
| function readRequestBody(req) { | ||
| return new Promise((resolve, reject) => { | ||
| const chunks = []; | ||
| req.on('data', (chunk) => chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk))); | ||
| req.on('end', () => resolve(Buffer.concat(chunks))); | ||
| req.on('error', reject); | ||
| }); |
There was a problem hiding this comment.
Cap buffered /v1/responses bodies.
readRequestBody() accumulates the entire request in memory with no limit. This adapter is the only path that fully buffers requests, so a large POST can OOM the process instead of failing fast. Reuse MAX_API_BODY_SIZE or another explicit ceiling and destroy the request once it is exceeded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli.js` around lines 6715 - 6721, The readRequestBody function currently
buffers the entire request with no limit; update it to enforce a maximum by
using the existing MAX_API_BODY_SIZE constant (or define a clear ceiling) —
track total bytes as you receive chunks in readRequestBody, and if totalBytes >
MAX_API_BODY_SIZE immediately destroy the incoming req, reject the Promise with
a clear "Payload Too Large" / HTTP 413 style error, and avoid concatenating
further chunks; reference the readRequestBody function and the MAX_API_BODY_SIZE
symbol when making this change so large POST bodies fail fast instead of causing
OOM.
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-ui/logic.sessions.mjs (1)
163-223:⚠️ Potential issue | 🟠 MajorEnsure the derived session keys are actually unique.
When
sessionIdis missing, repeated unnamed sessions from the same source and path/title collapse to the same key. Both newv-forlists depend on this field, so Vue can recycle the wrong row and render stale session metadata.🛠️ Proposed fix
- for (const session of list) { + for (const [sessionIndex, session] of list.entries()) { if (!session || typeof session !== 'object') continue; const source = normalizeSessionSource(session.source, ''); if (source !== 'codex' && source !== 'claude') continue; @@ const sessionEntry = { - key: `${source}:${session.sessionId || ''}:${session.filePath || normalizedTitle}`, + key: typeof session.sessionId === 'string' && session.sessionId.trim() + ? `${source}:${session.sessionId.trim()}` + : `${source}:${session.filePath || normalizedTitle}:${updatedAtMs}:${sessionIndex}`, title: normalizedTitle,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/logic.sessions.mjs` around lines 163 - 223, The generated session key `${source}:${session.sessionId || ''}:${session.filePath || normalizedTitle}` can collide when session.sessionId is missing, causing Vue v-for to recycle rows; update the key generation in the block that builds sessionEntry (where sessionEntry.key is set, used by recentSessions and topSessionsByMessages) to include an additional unique discriminator such as the updatedAtMs (e.g., `${source}:${session.sessionId || ''}:${session.filePath || normalizedTitle}:${updatedAtMs}`) or a per-session fallback UID (derived from updatedAtMs + messageCount or a short hash) so that unnamed sessions from the same source/path/title produce distinct keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/logic.sessions.mjs`:
- Around line 188-195: The code is using UTC-based methods (stamp.getUTCHours()
and stamp.getUTCDay()) which mis-buckets local sessions; change to local-time
methods (stamp.getHours() and stamp.getDay()) so hourCounts and weekdayCounts
are incremented using local hours/weekday; update any related variables or
calculations that reference utcHour/weekday index (e.g., utcHour, dayIndex,
hourCounts, weekdayCounts, and the busiest-hour summary logic) to use the
local-derived values and preserve the existing weekday indexing ((stamp.getDay()
+ 6) % 7) if that ordering is required.
---
Outside diff comments:
In `@web-ui/logic.sessions.mjs`:
- Around line 163-223: The generated session key `${source}:${session.sessionId
|| ''}:${session.filePath || normalizedTitle}` can collide when
session.sessionId is missing, causing Vue v-for to recycle rows; update the key
generation in the block that builds sessionEntry (where sessionEntry.key is set,
used by recentSessions and topSessionsByMessages) to include an additional
unique discriminator such as the updatedAtMs (e.g.,
`${source}:${session.sessionId || ''}:${session.filePath ||
normalizedTitle}:${updatedAtMs}`) or a per-session fallback UID (derived from
updatedAtMs + messageCount or a short hash) so that unnamed sessions from the
same source/path/title produce distinct keys.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b84cbc47-16c3-491f-82f4-352e889ac87e
📒 Files selected for processing (5)
tests/unit/session-usage.test.mjsweb-ui/logic.sessions.mjsweb-ui/modules/app.computed.session.mjsweb-ui/partials/index/panel-usage.htmlweb-ui/styles/sessions-usage.css
✅ Files skipped from review due to trivial changes (1)
- web-ui/styles/sessions-usage.css
awsl233777
force-pushed
the
feat/local-qwen-oauth-provider
branch
from
5d39ace to
d18b384
Compare
awsl233777
changed the title
|
@coderabbitai review Please review the current state of PR #82 only. Focus specifically on:
Please ignore stale findings from earlier force-pushed revisions that are no longer in this PR. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 46073
🔴 Issue 1 — Hour/weekday activity uses local time; day buckets use UTC (Logic Bug)File: All day-bucket logic consistently uses UTC ( // ❌ Local time — inconsistent with UTC bucket keys
const localHour = stamp.getHours();
...
const dayIndex = (stamp.getDay() + 6) % 7;Regression risk: A session whose UTC date falls on 04-06 (and therefore lands in the 04-06 bucket) may be counted under a different hour or weekday in the "活跃时段" / "工作日分布" cards depending on the user's timezone. The two charts will be inconsistent with each other and with the main trend bars. Unit test regression: assert.strictEqual(result.hourActivity.find(item => item.key === '09').count, 2);The three sessions have UTC timestamps at hours 08, 09, and 09. In UTC this gives key Fix: Replace local-time calls with their UTC equivalents: // In buildUsageChartGroups
- const localHour = stamp.getHours();
+ const localHour = stamp.getUTCHours();
if (hourCounts[localHour]) {
hourCounts[localHour].count += 1;
}
- const dayIndex = (stamp.getDay() + 6) % 7;
+ const dayIndex = (stamp.getUTCDay() + 6) % 7;🔴 Issue 2 —
|
| Severity | Issue | File |
|---|---|---|
| 🔴 High | Hour/weekday activity uses getHours()/getDay() (local) instead of UTC — data mismatch + timezone-dependent unit test failure |
logic.sessions.mjs, session-usage.test.mjs |
| 🔴 High | 24-item hourly bar chart rendered into 12-column grid → wraps to 2 rows, chart visually broken | sessions-usage.css |
| 🟡 Minor | Shared object refs in recentSessions/topSessionsByMessages |
logic.sessions.mjs |
🧠 Learnings used
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-04T01:36:26.964Z
Learning: In PR `#68` for SakuraByteCore/codexmate, github-actions[bot] again flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`, `#68`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-31T12:37:51.023Z
Learning: In PR `#59` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-03T10:42:22.368Z
Learning: In PR `#67` for SakuraByteCore/codexmate, github-actions[bot] flagged that CodeRabbit was again introducing breaking changes. The maintainer's absolute constraint (reinforced across PRs `#55`, `#59`, `#67`) is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-30T04:47:15.352Z
Learning: In PR `#55` for SakuraByteCore/codexmate, github-actions[bot] repeatedly flagged that CodeRabbit was introducing breaking changes. The maintainer's absolute constraint is: no breaking changes in any review suggestion; limit all feedback to correctness, safety, and quality only within the existing design, without refactoring or altering APIs, interfaces, or behavior.
Learnt from: SurviveM
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-03-29T02:42:32.785Z
Learning: The repository maintainer SurviveM (GitHub: SurviveM, repo: SakuraByteCore/codexmate) prefers that CodeRabbit review suggestions do not introduce breaking changes. Reviews should focus on correctness, safety, and quality without recommending refactors or changes that would break existing APIs, interfaces, or behavior.
Learnt from: github-actions[bot]
Repo: SakuraByteCore/codexmate PR: 0
File: :0-0
Timestamp: 2026-04-05T03:58:26.778Z
Learning: For SakuraByteCore/codexmate reviews, only provide non-breaking feedback (correctness, safety, quality). Avoid API, behavior, or refactor suggestions. This rule applies across PRs `#55`, `#59`, `#67`, and `#68`.
|
@coderabbitai review Please review the latest state of PR #82 after commit Context:
Please answer specifically:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 47035
1. ✅ 24-column grid — Fixed
.usage-mini-bars {
grid-template-columns: repeat(24, minmax(0, 1fr));
}
/* 640px breakpoint: */
.usage-mini-bars {
grid-template-columns: repeat(12, minmax(0, 1fr));
}All 24 hourly bars render in a single row on desktop; they wrap into 2 clean rows on mobile. ✅ 2. ✅ Shared object references — FixedtopSessionsByMessages.push({ ...sessionEntry }); // spread copy now usedThe two session lists now hold independent objects. ✅ 3. 🔴 Local-time activity buckets — Not yet fixedDespite the claim, the current code in // Lines as they appear now — still local time:
const localHour = stamp.getHours(); // ← should be getUTCHours()
const dayIndex = (stamp.getDay() + 6) % 7; // ← should be getUTCDay()The unit test asserts: assert.strictEqual(result.hourActivity.find(item => item.key === '09').count, 2);
assert.strictEqual(result.weekdayActivity.find(item => item.label === '周一').count, 2);The three fixture sessions have UTC timestamps at 08:00Z, 09:00Z, 09:00Z. In UTC those give key The fix is two lines: - const localHour = stamp.getHours();
+ const localHour = stamp.getUTCHours();
if (hourCounts[localHour]) {
hourCounts[localHour].count += 1;
}
- const dayIndex = (stamp.getDay() + 6) % 7;
+ const dayIndex = (stamp.getUTCDay() + 6) % 7;Answers to your four questions
🧠 Learnings used |
|
@coderabbitai review Workflow checkpoint for the current PR #82 state after commit Please review only the current Usage-tab PR content and answer in terms of merge-readiness:
Please ignore stale findings from earlier force-pushed revisions that are no longer part of this PR. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 21656
✅ Issue 1 — UTC local-time mismatch — Fixed
const utcHour = stamp.getUTCHours(); // line 188
const dayIndex = (stamp.getUTCDay() + 6) % 7; // line 192Both the hour and weekday accumulators are now consistent with the UTC day-bucket keys. The unit test assertions ( ✅ Issue 2 — 24-bar mini-bar grid — Fixed
✅ Issue 3 — Shared object references — Fixed
Merge-readiness verdictNo remaining regression issues. No remaining UX regressions. All three previously flagged issues are fully resolved in the current state. The PR is clear to merge from a regression and UX standpoint. 🧠 Learnings used |
2 participants
Summary
Validation
npm run test:unitnpm run test:e2eSummary by CodeRabbit
Release Notes
New Features
Tests