feat(web-ui): claude local bridge, share command, session utilization & UI improvements#166
Merged
Merged
Conversation
- Remove disabled attribute from share command button - Update i18n translations to remove disabled state text - Allow users to copy share commands for Claude configurations
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (2)
📝 WalkthroughWalkthroughImplements a Claude local bridge and server APIs, provider-key retrieval and masked-edit UX, enabled share-command UI/i18n tweaks, side-rail restyle, session context utilization (computed, UI, styles), Windows tray support, update tooling, tests adjustments, and multiple robustness cleanups. ChangesClaude Local Bridge & server wiring
Provider key retrieval & edit UX
Claude config UI & share-button enablement
Side-rail brand restyle & scrolling
Session context utilization
Windows tray support & test harness updates
Misc fixes and small behavior tweaks
Sequence Diagram(s) sequenceDiagram
participant Browser
participant WebUI
participant Server
participant ClaudeUpstream
Browser->>WebUI: select 'claude-local' / copy/share command
WebUI->>Server: /api/claude-local-bridge-* or POST /bridge/claude-local/...
Server->>Server: authorize, read settings/providers, apply excluded list
Server->>ClaudeUpstream: forward request (stream or JSON)
ClaudeUpstream-->>Server: SSE stream or JSON response
Server-->>WebUI: proxied response
WebUI-->>Browser: render results / stream
Estimated code review effort 🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 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
🤖 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 `@web-ui/partials/index/panel-config-claude.html`:
- Line 125: The share button currently only toggles a loading class but remains
clickable; prevent duplicate actions by disabling it while loading: add a
disabled binding (e.g., :disabled="claudeShareLoading[name]") to the button and
guard the click handler so it only invokes copyClaudeShareCommand(name) when
claudeShareLoading[name] is false (for example change the `@click` to a short
conditional call or add an early-return in copyClaudeShareCommand to ignore
calls when claudeShareLoading[name] is true); also consider setting
aria-disabled for accessibility.
🪄 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: 61d44463-152b-41a5-8d56-e125863dd8f7
📒 Files selected for processing (2)
web-ui/modules/i18n.dict.mjsweb-ui/partials/index/panel-config-claude.html
📜 Review details
🔇 Additional comments (1)
web-ui/modules/i18n.dict.mjs (1)
1040-1040: LGTM!Also applies to: 2090-2090, 3150-3150
| <svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2"><rect x="9" y="9" width="13" height="13" rx="2"/><path d="M5 15H4a2 2 0 0 1-2-2V4a2 2 0 0 1 2-2h9a2 2 0 0 1 2 2v1"/></svg> | ||
| </button> | ||
| <button class="card-action-btn" :class="{ loading: claudeShareLoading[name] }" @click="copyClaudeShareCommand(name)" disabled :title="t('claude.action.shareDisabled')" :aria-label="t('config.shareCommand.aria')"> | ||
| <button class="card-action-btn" :class="{ loading: claudeShareLoading[name] }" @click="copyClaudeShareCommand(name)" :title="t('config.shareCommand')" :aria-label="t('config.shareCommand.aria')"> |
Contributor
There was a problem hiding this comment.
Disable the share button while loading to prevent duplicate actions.
Line 125 applies a loading class but still allows repeated clicks while claudeShareLoading[name] is true, which can trigger duplicate copy attempts.
Proposed fix
-<button class="card-action-btn" :class="{ loading: claudeShareLoading[name] }" `@click`="copyClaudeShareCommand(name)" :title="t('config.shareCommand')" :aria-label="t('config.shareCommand.aria')">
+<button
+ class="card-action-btn"
+ :class="{ loading: claudeShareLoading[name] }"
+ :disabled="claudeShareLoading[name]"
+ `@click`="copyClaudeShareCommand(name)"
+ :title="t('config.shareCommand')"
+ :aria-label="t('config.shareCommand.aria')">📝 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
| <button class="card-action-btn" :class="{ loading: claudeShareLoading[name] }" @click="copyClaudeShareCommand(name)" :title="t('config.shareCommand')" :aria-label="t('config.shareCommand.aria')"> | |
| <button | |
| class="card-action-btn" | |
| :class="{ loading: claudeShareLoading[name] }" | |
| :disabled="claudeShareLoading[name]" | |
| `@click`="copyClaudeShareCommand(name)" | |
| :title="t('config.shareCommand')" | |
| :aria-label="t('config.shareCommand.aria')"> |
🤖 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-ui/partials/index/panel-config-claude.html` at line 125, The share button
currently only toggles a loading class but remains clickable; prevent duplicate
actions by disabling it while loading: add a disabled binding (e.g.,
:disabled="claudeShareLoading[name]") to the button and guard the click handler
so it only invokes copyClaudeShareCommand(name) when claudeShareLoading[name] is
false (for example change the `@click` to a short conditional call or add an
early-return in copyClaudeShareCommand to ignore calls when
claudeShareLoading[name] is true); also consider setting aria-disabled for
accessibility.
- Move version number from sidebar footer to brand header next to title - Replace workspace kicker with Codex Mate title and version - Remove unused brand-title and side-rail-version styles - Add brand-version style for inline version display
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web-ui/partials/index/layout-header.html (1)
125-125: 💤 Low valueBrand name hardcoding is intentional but breaks i18n pattern for consistency.
Line 125 hardcodes "Codex Mate" rather than using the
t('...')pattern used consistently throughout the file (e.g., line 128 witht('brand.subtitle.localConfigSessionsWorkspace')). This appears intentional per the recent commit that introduced this change ("Replace workspace kicker with Codex Mate title"), as brand names typically aren't localized. However, if architectural consistency is desired, consider definingt('brand.name')that returns "Codex Mate" in all locales—this maintains the current non-translated behavior while keeping the i18n pattern uniform.🤖 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-ui/partials/index/layout-header.html` at line 125, The template currently hardcodes the brand text "Codex Mate" in the brand kicker div; replace that literal with the i18n key (e.g., use t('brand.name')) to preserve the file's i18n pattern while keeping the non-translated value by adding "Codex Mate" to the locales as brand.name; update the brand kicker line that currently contains "Codex Mate <span v-if=\"appVersion\" class=\"brand-version\">v{{ appVersion }}</span>" to use t('brand.name') alongside the existing appVersion span, and add brand.name = "Codex Mate" to your locale files so behavior remains identical.
🤖 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.
Nitpick comments:
In `@web-ui/partials/index/layout-header.html`:
- Line 125: The template currently hardcodes the brand text "Codex Mate" in the
brand kicker div; replace that literal with the i18n key (e.g., use
t('brand.name')) to preserve the file's i18n pattern while keeping the
non-translated value by adding "Codex Mate" to the locales as brand.name; update
the brand kicker line that currently contains "Codex Mate <span
v-if=\"appVersion\" class=\"brand-version\">v{{ appVersion }}</span>" to use
t('brand.name') alongside the existing appVersion span, and add brand.name =
"Codex Mate" to your locale files so behavior remains identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 623935f2-2aa7-4d3e-af15-3f8af3ccd23c
📒 Files selected for processing (2)
web-ui/partials/index/layout-header.htmlweb-ui/styles/layout-shell.css
📜 Review details
🔇 Additional comments (1)
web-ui/styles/layout-shell.css (1)
451-466: LGTM!
- Add flex: 1 1 auto to side-rail-nav to fill remaining space - Language switcher now stays pinned at bottom of sidebar
- Change side-rail-lang from sticky to absolute positioning - Move overflow-y from side-rail to side-rail-nav for proper scrolling - Language switcher now stays fixed at bottom regardless of scroll position
- Calculate token utilization percentage per session - Display visual indicator with color coding: - Green: normal (<80%) - Yellow/Orange: warning (80-95%) - Red: critical (≥95%) - Add pulse animation for warning and critical states - Show percentage badge in session list header Implements #141
- Remove disabled attribute expectation from copyClaudeShareCommand button test - Aligns test with actual behavior after enabling share command feature
- Add sessionContextUtilization to allowedExtraCurrentComputedKeys - Aligns test with new context window utilization feature
- lib/cli-sessions.js: CJK tokens now use indexOf instead of boundary regex (Chinese text has no ASCII word boundaries) - web-ui: add missing formatSignedUsageSummaryNumber and prevCostTitle - tests/e2e: increase install-status API timeout to 10s (spawnSync blocks event loop on slow environments)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web-ui/partials/index/panel-sessions.html (1)
158-163: ⚡ Quick winConsider extracting repeated
getSessionExportKey(session)calls.The expression
getSessionExportKey(session)is invoked 5 times within this single badge element (twice in the v-if condition, and three times in attributes). This reduces performance and maintainability.Consider extracting the key to a v-memo dependency or using a template-level helper to compute it once:
<template v-for="session in visibleSessionsList"> <!-- Define key once --> <component :is="'span'" v-memo="[sessionKey = getSessionExportKey(session)]"> <!-- Use sessionKey throughout --> </component> </template>Or add the utilization data directly to the session object in the computed property.
🤖 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-ui/partials/index/panel-sessions.html` around lines 158 - 163, The span element repeatedly calls getSessionExportKey(session) causing redundant work; compute the key once and reuse it: inside the v-for over visibleSessionsList (or in the badge's template scope) assign const/sessionKey = getSessionExportKey(session) via a v-memo dependency or by adding the export key/ utilization onto the session in the computed that builds visibleSessionsList, then replace all occurrences of getSessionExportKey(session) with sessionKey and reference sessionContextUtilization[sessionKey] for the v-if, :class and :title usages.
🤖 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 `@tests/unit/config-tabs-ui.test.mjs`:
- Line 331: The current assertion using assert.match(html, /<button
class="card-action-btn"[^>]*`@click`="copyClaudeShareCommand\(name\)"[^>]*>/) only
checks for presence but not that the button is enabled; update the test in
tests/unit/config-tabs-ui.test.mjs to (1) assert the matched button markup does
NOT contain disabled or :disabled (e.g., assert.notMatch against
/\s(?:disabled|:disabled)(?:=|>)/), (2) assert aria-disabled is not "true"
(e.g., assert.notMatch html against /aria-disabled=["']\s*true\s*["']/), and (3)
assert the title/aria-label used for tooltip does not indicate a disabled state
(e.g., assert.notMatch html against
/(title|aria-label)=["'][^"']*disabled[^"']*["']/); keep the original
`@click/name` presence check (copyClaudeShareCommand and name) to ensure
interactivity.
In `@web-ui/modules/app.computed.session.mjs`:
- Line 586: The field prevCostTitle duplicates prevTokenTitle and misleads by
implying cost; either remove prevCostTitle entirely from the computed object or
rename it to prevTokenLabel and keep the same logic, i.e. replace prevCostTitle:
... with prevTokenLabel: row.compareEnabled ?
formatUsageSummaryNumber(row.prevTokenTotal || 0) : ''; if you actually intend
to show cost instead, implement the proper cost calculation and formatting in
prevCostTitle using the appropriate cost-calculation helper rather than
formatUsageSummaryNumber (update any template references from prevCostTitle to
the new name as needed).
---
Nitpick comments:
In `@web-ui/partials/index/panel-sessions.html`:
- Around line 158-163: The span element repeatedly calls
getSessionExportKey(session) causing redundant work; compute the key once and
reuse it: inside the v-for over visibleSessionsList (or in the badge's template
scope) assign const/sessionKey = getSessionExportKey(session) via a v-memo
dependency or by adding the export key/ utilization onto the session in the
computed that builds visibleSessionsList, then replace all occurrences of
getSessionExportKey(session) with sessionKey and reference
sessionContextUtilization[sessionKey] for the v-if, :class and :title usages.
🪄 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: f68c33f5-0198-4ceb-94fc-988c68ace026
📒 Files selected for processing (8)
lib/cli-sessions.jstests/e2e/test-install-status.jstests/unit/config-tabs-ui.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/modules/app.computed.session.mjsweb-ui/partials/index/panel-sessions.htmlweb-ui/styles/layout-shell.cssweb-ui/styles/sessions-list.css
📜 Review details
🔇 Additional comments (11)
tests/e2e/test-install-status.js (1)
5-5: LGTM!lib/cli-sessions.js (2)
168-169: LGTM!
166-166: ⚡ Quick winThe CJK range is appropriate for the tool's intended scope.
The regex
/[⺀-鿿\u{20000}-\u{2fa1f}]/ucorrectly covers Chinese ideographs, which aligns with the tool's language support (Chinese + English, as evidenced by the "README.zh.md" and Chinese description in package.json). There is no evidence in the codebase that Japanese or Korean language support is required, and thehasCjkfunction is used specifically for matching Chinese characters in session tokens and queries. The range does not need to include Hiragana, Katakana, Hangul, or CJK Compatibility ranges.> Likely an incorrect or invalid review comment.web-ui/modules/app.computed.session.mjs (2)
30-40: LGTM!
706-733: LGTM!web-ui/styles/sessions-list.css (1)
649-683: LGTM!tests/unit/web-ui-behavior-parity.test.mjs (1)
693-694: LGTM!web-ui/styles/layout-shell.css (4)
82-90: LGTM!
209-213: LGTM!
459-465: LGTM!
467-473: LGTM!
| assert.match(html, /<button class="card-action-btn"[^>]*@click="openEditConfigModal\(name\)"[^>]*:aria-label="t\('claude\.action\.editAria', \{ name \}\)"[^>]*:title="t\('claude\.action\.edit'\)">/); | ||
| assert.match(html, /<button class="card-action-btn delete"[^>]*@click="deleteClaudeConfig\(name\)"[^>]*:aria-label="t\('claude\.action\.deleteAria', \{ name \}\)"[^>]*:title="t\('claude\.action\.delete'\)">/); | ||
| assert.match(html, /<button class="card-action-btn"[^>]*@click="copyClaudeShareCommand\(name\)"[^>]*disabled[^>]*>/); | ||
| assert.match(html, /<button class="card-action-btn"[^>]*@click="copyClaudeShareCommand\(name\)"[^>]*>/); |
Contributor
There was a problem hiding this comment.
Strengthen this assertion to prove the button is actually enabled.
This regex still passes when disabled/:disabled is present, so it doesn’t validate the PR’s core behavior (interactive share button). Add a targeted negative check on the matched markup (and ideally assert updated aria/title bindings too).
Suggested test hardening
- assert.match(html, /<button class="card-action-btn"[^>]*`@click`="copyClaudeShareCommand\(name\)"[^>]*>/);
+ const claudeShareButton = html.match(
+ /<button class="card-action-btn"[^>]*`@click`="copyClaudeShareCommand\(name\)"[^>]*>/
+ );
+ assert(claudeShareButton, 'Claude share button should exist');
+ assert.doesNotMatch(claudeShareButton[0], /\s(?:disabled|:disabled)=/);
+ assert.match(claudeShareButton[0], /:aria-label="t\('config\.shareCommand\.aria'\)"/);
+ assert.match(claudeShareButton[0], /:title="t\('config\.shareCommand'\)"/);📝 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
| assert.match(html, /<button class="card-action-btn"[^>]*@click="copyClaudeShareCommand\(name\)"[^>]*>/); | |
| const claudeShareButton = html.match( | |
| /<button class="card-action-btn"[^>]*`@click`="copyClaudeShareCommand\(name\)"[^>]*>/ | |
| ); | |
| assert(claudeShareButton, 'Claude share button should exist'); | |
| assert.doesNotMatch(claudeShareButton[0], /\s(?:disabled|:disabled)=/); | |
| assert.match(claudeShareButton[0], /:aria-label="t\('config\.shareCommand\.aria'\)"/); | |
| assert.match(claudeShareButton[0], /:title="t\('config\.shareCommand'\)"/); |
🤖 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 `@tests/unit/config-tabs-ui.test.mjs` at line 331, The current assertion using
assert.match(html, /<button
class="card-action-btn"[^>]*`@click`="copyClaudeShareCommand\(name\)"[^>]*>/) only
checks for presence but not that the button is enabled; update the test in
tests/unit/config-tabs-ui.test.mjs to (1) assert the matched button markup does
NOT contain disabled or :disabled (e.g., assert.notMatch against
/\s(?:disabled|:disabled)(?:=|>)/), (2) assert aria-disabled is not "true"
(e.g., assert.notMatch html against /aria-disabled=["']\s*true\s*["']/), and (3)
assert the title/aria-label used for tooltip does not indicate a disabled state
(e.g., assert.notMatch html against
/(title|aria-label)=["'][^"']*disabled[^"']*["']/); keep the original
`@click/name` presence check (copyClaudeShareCommand and name) to ensure
interactivity.
| tokenPercent: maxTokens > 0 ? Math.round((row.tokenTotal / maxTokens) * 1000) / 10 : 0, | ||
| prevTokenPercent: row.compareEnabled && maxTokens > 0 ? Math.round(((row.prevTokenTotal || 0) / maxTokens) * 1000) / 10 : 0, | ||
| prevTokenTitle: row.compareEnabled ? formatUsageSummaryNumber(row.prevTokenTotal || 0) : '', | ||
| prevCostTitle: row.compareEnabled ? formatUsageSummaryNumber(row.prevTokenTotal || 0) : '', |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for usage of prevCostTitle to understand its purpose
# Search for prevCostTitle usage in templates and code
rg -n 'prevCostTitle' --type=html --type=js --type=vue -C3Repository: SakuraByteCore/codexmate
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Search for prevCostTitle without vue type
rg -n 'prevCostTitle' -C3
# Also search for the file to confirm path
find . -name "app.computed.session.mjs" -type fRepository: SakuraByteCore/codexmate
Length of output: 1909
🏁 Script executed:
#!/bin/bash
# Read the file around line 586 to see context
# First, get file info
file_path=$(find . -name "app.computed.session.mjs" -type f)
if [ -n "$file_path" ]; then
# Show lines 570-600 to get context
sed -n '570,600p' "$file_path" | cat -n
fiRepository: SakuraByteCore/codexmate
Length of output: 1887
🏁 Script executed:
#!/bin/bash
# Search for how prevCostTitle and prevTokenTitle are used in templates
rg -n 'prevToken|prevCost' -t html -t js -C2Repository: SakuraByteCore/codexmate
Length of output: 2888
🏁 Script executed:
#!/bin/bash
# Search for prevCostTitle in all files, especially templates
rg -n 'prevCostTitle' -C5
# Also search for cost-related fields to see if actual cost calculation exists
rg -n 'prevCost|costTotal|cost' --type=js --type=html -i | head -50Repository: SakuraByteCore/codexmate
Length of output: 3298
🏁 Script executed:
#!/bin/bash
# Search for tooltip usage patterns to understand how prevCostTitle is consumed
rg -n 'tooltip|title=' web-ui/partials/ -C2 | head -80Repository: SakuraByteCore/codexmate
Length of output: 11257
🏁 Script executed:
#!/bin/bash
# Search for where prevTokenTitle is referenced/used
rg -n 'prevTokenTitle|prevCostTitle' -t js -t html -A2 -B2
# Also search for usage of day.tokenTitle or similar to understand the pattern
rg -n '\.tokenTitle|day\.token' web-ui/partials/ -C3Repository: SakuraByteCore/codexmate
Length of output: 2017
Remove or rename prevCostTitle—field duplicates prevTokenTitle and has misleading naming.
Line 586 defines prevCostTitle with identical logic to prevTokenTitle (line 585), formatting prevTokenTotal using formatUsageSummaryNumber. The field name suggests cost data, but it actually formats token counts. This appears to be either:
- Unused duplicate code (field is tested but not referenced in templates)
- Incomplete implementation where actual cost calculation should replace token formatting
- Placeholder for future tooltip support never completed
Either remove the field or correct the naming to prevTokenLabel if token display is intended.
🤖 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-ui/modules/app.computed.session.mjs` at line 586, The field prevCostTitle
duplicates prevTokenTitle and misleads by implying cost; either remove
prevCostTitle entirely from the computed object or rename it to prevTokenLabel
and keep the same logic, i.e. replace prevCostTitle: ... with prevTokenLabel:
row.compareEnabled ? formatUsageSummaryNumber(row.prevTokenTotal || 0) : ''; if
you actually intend to show cost instead, implement the proper cost calculation
and formatting in prevCostTitle using the appropriate cost-calculation helper
rather than formatUsageSummaryNumber (update any template references from
prevCostTitle to the new name as needed).
- fix share command shell quoting for URLs - enable clone button for Codex providers - fix clone showing bridge URL instead of upstream for transform providers - add eye toggle to show/hide full API key in Codex edit modal - add get-provider-key backend endpoint (reads bridge config for transform providers)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@cli.js`:
- Around line 2205-2227: Add an explicit anti-rebinding gate at the start of
getProviderKey so raw provider secrets are never returned unless the request
context is validated: implement or call a helper like
isRequestFromLocalTrustedSource(params.request, params.headers) (or check
Host/Origin and that the connection is loopback + matches an allowlist or an
explicit env/flag such as ALLOW_SECRET_READ) and return { error: 'not allowed' }
when the check fails; then proceed to use
readConfig/readOpenaiBridgeSettings/OPENAI_BRIDGE_SETTINGS_FILE only after the
gate passes. Apply the same gate to the other plaintext-secret readers
referenced (the same logic used around the code at the other provider-secret
reads).
- Around line 2213-2219: The detection for a transform provider is too broad;
instead of treating any base_url that contains '/bridge/openai/' as a transform,
parse provider.base_url (use new URL(...)) and only mark isTransform true when
either provider.codexmate_bridge === 'openai' OR the parsed URL's pathname
strictly startsWith '/bridge/openai/' AND its hostname matches the bridge host
from readOpenaiBridgeSettings(OPENAI_BRIDGE_SETTINGS_FILE) (e.g., settings.host
or settings.bridge_host); update the isTransform logic and then continue to read
settings.providers[name].apiKey only when that narrower isTransform is true so
preferred_auth_method from the real provider is not overridden.
In `@web-ui/modules/app.methods.providers.mjs`:
- Around line 295-303: The current clone logic sets isTransform true for any
non-empty provider.codexmate_bridge, which can misclassify non-OpenAI bridges;
update the isTransform determination in the block that computes
isTransform/cloneUrl/newProvider so it only flags OpenAI bridges (e.g., require
provider.codexmate_bridge to explicitly indicate OpenAI, or match the
provider.url against /\/bridge\/openai\//) before using provider.upstreamUrl;
keep using normalizeProviderUrl for cloneUrl and ensure newProvider.useTransform
and url are computed from this narrowed isTransform check (refer to isTransform,
provider.codexmate_bridge, provider.url, provider.upstreamUrl,
normalizeProviderUrl, and newProvider).
- Around line 394-400: The current logic only assigns params.key when
_editProviderRealKeyLoaded is true, which drops user-entered keys if
get-provider-key failed; change the logic in the save flow that builds
params.key so it does not gate on _editProviderRealKeyLoaded — always compute
currentKey from editingProvider.key and originalKey from
_editProviderOriginalKey (using the existing typeof checks/defaults) and set
params.key whenever currentKey !== originalKey (or when currentKey is non-empty
and originalKey is missing), or alternatively mark _editProviderRealKeyLoaded
true on prefetch failure; update the block around params.key,
_editProviderRealKeyLoaded, editingProvider.key and _editProviderOriginalKey
accordingly so user edits are never silently dropped.
In `@web-ui/partials/index/modals-basic.html`:
- Around line 70-73: The eye-toggle button that calls toggleEditProviderKey and
uses showEditProviderKey currently only sets title; add explicit accessible
attributes: set aria-pressed to the boolean showEditProviderKey and set
aria-label to the dynamic label (e.g., use the same expression as title:
showEditProviderKey ? t('common.hide') : t('common.show')), and apply the
identical change to the other icon-only toggle button (the one using the same
pattern around toggleEditProviderKey/showEditProviderKey at the later
occurrence).
🪄 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: c13f71de-40c4-45b6-add6-51b0a49c3ced
📒 Files selected for processing (8)
cli.jsweb-ui/app.jsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.providers.mjsweb-ui/modules/app.methods.session-actions.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-codex.htmlweb-ui/styles/modals-core.css
✅ Files skipped from review due to trivial changes (1)
- web-ui/styles/modals-core.css
📜 Review details
🔇 Additional comments (4)
web-ui/modules/app.methods.session-actions.mjs (1)
203-209: LGTM!web-ui/app.js (1)
62-67: LGTM!web-ui/modules/app.methods.claude-config.mjs (1)
64-65: LGTM!Also applies to: 81-87
web-ui/partials/index/panel-config-codex.html (1)
151-151: LGTM!
Comment on lines
+2205
to
+2227
| function getProviderKey(params = {}) { | ||
| const name = typeof params.name === 'string' ? params.name.trim() : ''; | ||
| if (!name) return { error: '名称不能为空' }; | ||
| try { | ||
| const config = readConfig(); | ||
| const provider = config.model_providers && config.model_providers[name]; | ||
| if (!provider) return { error: '提供商不存在' }; | ||
|
|
||
| const bridge = typeof provider.codexmate_bridge === 'string' ? provider.codexmate_bridge.trim() : ''; | ||
| const isTransform = bridge === 'openai' || String(provider.base_url || '').includes('/bridge/openai/'); | ||
| if (isTransform) { | ||
| const settings = readOpenaiBridgeSettings(OPENAI_BRIDGE_SETTINGS_FILE); | ||
| const entry = settings.providers ? settings.providers[name] : null; | ||
| const key = entry && typeof entry.apiKey === 'string' ? entry.apiKey : ''; | ||
| return { key }; | ||
| } | ||
|
|
||
| const key = typeof provider.preferred_auth_method === 'string' ? provider.preferred_auth_method : ''; | ||
| return { key }; | ||
| } catch (e) { | ||
| return { error: e.message || '读取失败' }; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Lock down plaintext key reads before exposing them over /api.
This new action returns raw provider secrets, but /api still fully trusts loopback requests and does not validate Host/Origin. That means a DNS-rebinding page can read local API keys once the Web UI is running. Please add an explicit anti-rebinding/secret-read gate here instead of relying on the current loopback shortcut.
Also applies to: 10632-10634
🤖 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 `@cli.js` around lines 2205 - 2227, Add an explicit anti-rebinding gate at the
start of getProviderKey so raw provider secrets are never returned unless the
request context is validated: implement or call a helper like
isRequestFromLocalTrustedSource(params.request, params.headers) (or check
Host/Origin and that the connection is loopback + matches an allowlist or an
explicit env/flag such as ALLOW_SECRET_READ) and return { error: 'not allowed' }
when the check fails; then proceed to use
readConfig/readOpenaiBridgeSettings/OPENAI_BRIDGE_SETTINGS_FILE only after the
gate passes. Apply the same gate to the other plaintext-secret readers
referenced (the same logic used around the code at the other provider-secret
reads).
Comment on lines
+2213
to
+2219
| const bridge = typeof provider.codexmate_bridge === 'string' ? provider.codexmate_bridge.trim() : ''; | ||
| const isTransform = bridge === 'openai' || String(provider.base_url || '').includes('/bridge/openai/'); | ||
| if (isTransform) { | ||
| const settings = readOpenaiBridgeSettings(OPENAI_BRIDGE_SETTINGS_FILE); | ||
| const entry = settings.providers ? settings.providers[name] : null; | ||
| const key = entry && typeof entry.apiKey === 'string' ? entry.apiKey : ''; | ||
| return { key }; |
Contributor
There was a problem hiding this comment.
Narrow the transform-provider detection.
Line 2214 treats any base_url containing /bridge/openai/ as a managed transform provider. That can misclassify a normal upstream URL and make this endpoint return settings.providers[name].apiKey instead of the provider’s real preferred_auth_method, so the edit modal loads the wrong secret.
Suggested fix
- const bridge = typeof provider.codexmate_bridge === 'string' ? provider.codexmate_bridge.trim() : '';
- const isTransform = bridge === 'openai' || String(provider.base_url || '').includes('/bridge/openai/');
+ const bridge = typeof provider.codexmate_bridge === 'string' ? provider.codexmate_bridge.trim() : '';
+ const baseUrl = typeof provider.base_url === 'string' ? provider.base_url.trim() : '';
+ const isLocalOpenaiBridgeUrl = /^https?:\/\/(?:127\.0\.0\.1|localhost|\[::1\])(?::\d+)?\/bridge\/openai\/[^/]+\/v1\/?$/i.test(baseUrl);
+ const isTransform = bridge === 'openai' || isLocalOpenaiBridgeUrl;📝 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
| const bridge = typeof provider.codexmate_bridge === 'string' ? provider.codexmate_bridge.trim() : ''; | |
| const isTransform = bridge === 'openai' || String(provider.base_url || '').includes('/bridge/openai/'); | |
| if (isTransform) { | |
| const settings = readOpenaiBridgeSettings(OPENAI_BRIDGE_SETTINGS_FILE); | |
| const entry = settings.providers ? settings.providers[name] : null; | |
| const key = entry && typeof entry.apiKey === 'string' ? entry.apiKey : ''; | |
| return { key }; | |
| const bridge = typeof provider.codexmate_bridge === 'string' ? provider.codexmate_bridge.trim() : ''; | |
| const baseUrl = typeof provider.base_url === 'string' ? provider.base_url.trim() : ''; | |
| const isLocalOpenaiBridgeUrl = /^https?:\/\/(?:127\.0\.0\.1|localhost|\[::1\])(?::\d+)?\/bridge\/openai\/[^/]+\/v1\/?$/i.test(baseUrl); | |
| const isTransform = bridge === 'openai' || isLocalOpenaiBridgeUrl; | |
| if (isTransform) { | |
| const settings = readOpenaiBridgeSettings(OPENAI_BRIDGE_SETTINGS_FILE); | |
| const entry = settings.providers ? settings.providers[name] : null; | |
| const key = entry && typeof entry.apiKey === 'string' ? entry.apiKey : ''; | |
| return { key }; |
🤖 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 `@cli.js` around lines 2213 - 2219, The detection for a transform provider is
too broad; instead of treating any base_url that contains '/bridge/openai/' as a
transform, parse provider.base_url (use new URL(...)) and only mark isTransform
true when either provider.codexmate_bridge === 'openai' OR the parsed URL's
pathname strictly startsWith '/bridge/openai/' AND its hostname matches the
bridge host from readOpenaiBridgeSettings(OPENAI_BRIDGE_SETTINGS_FILE) (e.g.,
settings.host or settings.bridge_host); update the isTransform logic and then
continue to read settings.providers[name].apiKey only when that narrower
isTransform is true so preferred_auth_method from the real provider is not
overridden.
Comment on lines
+295
to
+303
| const isTransform = !!(provider.codexmate_bridge || '').trim() || /\/bridge\/openai\//.test(provider.url || ''); | ||
| const cloneUrl = isTransform && provider.upstreamUrl | ||
| ? normalizeProviderUrl(provider.upstreamUrl) | ||
| : normalizeProviderUrl(provider.url || ''); | ||
| this.newProvider = { | ||
| name: '', | ||
| url: normalizeProviderUrl(provider.url || ''), | ||
| url: cloneUrl, | ||
| key: '', | ||
| useTransform: !!(provider.codexmate_bridge || '').trim() || /\/bridge\/openai\//.test(provider.url || '') | ||
| useTransform: isTransform |
Contributor
There was a problem hiding this comment.
Narrow clone transform detection to OpenAI bridge only.
This branch treats any non-empty codexmate_bridge as transform. That can clone non-OpenAI bridge providers with incorrect url/useTransform values.
Suggested fix
-const isTransform = !!(provider.codexmate_bridge || '').trim() || /\/bridge\/openai\//.test(provider.url || '');
+const bridge = typeof provider.codexmate_bridge === 'string'
+ ? provider.codexmate_bridge.trim()
+ : '';
+const isTransform = bridge === 'openai' || /\/bridge\/openai\//.test(provider.url || '');🤖 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-ui/modules/app.methods.providers.mjs` around lines 295 - 303, The current
clone logic sets isTransform true for any non-empty provider.codexmate_bridge,
which can misclassify non-OpenAI bridges; update the isTransform determination
in the block that computes isTransform/cloneUrl/newProvider so it only flags
OpenAI bridges (e.g., require provider.codexmate_bridge to explicitly indicate
OpenAI, or match the provider.url against /\/bridge\/openai\//) before using
provider.upstreamUrl; keep using normalizeProviderUrl for cloneUrl and ensure
newProvider.useTransform and url are computed from this narrowed isTransform
check (refer to isTransform, provider.codexmate_bridge, provider.url,
provider.upstreamUrl, normalizeProviderUrl, and newProvider).
Comment on lines
+394
to
400
| if (this._editProviderRealKeyLoaded) { | ||
| const currentKey = typeof this.editingProvider.key === 'string' ? this.editingProvider.key : ''; | ||
| const originalKey = typeof this._editProviderOriginalKey === 'string' ? this._editProviderOriginalKey : ''; | ||
| if (currentKey !== originalKey) { | ||
| params.key = currentKey; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent silent no-op when key prefetch fails.
params.key is only sent when _editProviderRealKeyLoaded is true. If get-provider-key fails, user-edited key input is dropped while save still reports success.
Suggested fix
// in openEditModal (after setting editingProvider.key)
+this._editProviderMaskedKeyAtOpen = this.editingProvider.key;
// in updateProvider (before conditional key logic)
+const currentKey = typeof this.editingProvider.key === 'string' ? this.editingProvider.key : '';
+const maskedAtOpen = typeof this._editProviderMaskedKeyAtOpen === 'string' ? this._editProviderMaskedKeyAtOpen : '';
+if (!this._editProviderRealKeyLoaded && currentKey !== maskedAtOpen) {
+ return this.showMessage('密钥加载失败，请稍后重试后再保存', 'error');
+}
-if (this._editProviderRealKeyLoaded) {
- const currentKey = typeof this.editingProvider.key === 'string' ? this.editingProvider.key : '';
+if (this._editProviderRealKeyLoaded) {
const originalKey = typeof this._editProviderOriginalKey === 'string' ? this._editProviderOriginalKey : '';
if (currentKey !== originalKey) {
params.key = currentKey;
}
}
// in closeEditModal
+this._editProviderMaskedKeyAtOpen = '';🤖 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-ui/modules/app.methods.providers.mjs` around lines 394 - 400, The current
logic only assigns params.key when _editProviderRealKeyLoaded is true, which
drops user-entered keys if get-provider-key failed; change the logic in the save
flow that builds params.key so it does not gate on _editProviderRealKeyLoaded —
always compute currentKey from editingProvider.key and originalKey from
_editProviderOriginalKey (using the existing typeof checks/defaults) and set
params.key whenever currentKey !== originalKey (or when currentKey is non-empty
and originalKey is missing), or alternatively mark _editProviderRealKeyLoaded
true on prefetch failure; update the block around params.key,
_editProviderRealKeyLoaded, editingProvider.key and _editProviderOriginalKey
accordingly so user edits are never silently dropped.
Comment on lines
+70
to
+73
| <button type="button" class="input-toggle-btn" @click="toggleEditProviderKey" :title="showEditProviderKey ? t('common.hide') : t('common.show')"> | ||
| <svg v-if="!showEditProviderKey" viewBox="0 0 20 20" fill="none" stroke="currentColor" stroke-width="1.5" width="16" height="16"><path d="M10 4C5 4 1.73 8.11 1 10c.73 1.89 4 6 9 6s8.27-4.11 9-6c-.73-1.89-4-6-9-6z"/><circle cx="10" cy="10" r="3"/></svg> | ||
| <svg v-else viewBox="0 0 20 20" fill="none" stroke="currentColor" stroke-width="1.5" width="16" height="16"><path d="M2 2l16 16M8.2 4.2A9.9 9.9 0 0 1 10 4c5 0 8.27 4.11 9 6-.44.94-1.5 2.7-3.2 4.2M14.5 14.5A5.9 5.9 0 0 1 10 16c-5 0-8.27-4.11-9-6 .76-1.66 2.2-3.6 4.3-5"/></svg> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add accessible names to icon-only key toggle buttons.
These eye-toggle buttons currently rely on title only; add explicit aria-label (and aria-pressed) so screen readers can identify the action state.
Suggested fix
-<button type="button" class="input-toggle-btn" `@click`="toggleEditProviderKey" :title="showEditProviderKey ? t('common.hide') : t('common.show')">
+<button
+ type="button"
+ class="input-toggle-btn"
+ `@click`="toggleEditProviderKey"
+ :title="showEditProviderKey ? t('common.hide') : t('common.show')"
+ :aria-label="showEditProviderKey ? t('common.hide') : t('common.show')"
+ :aria-pressed="showEditProviderKey ? 'true' : 'false'">
-<button type="button" class="input-toggle-btn" `@click`="toggleEditClaudeConfigKey" :title="showEditClaudeConfigKey ? t('common.hide') : t('common.show')">
+<button
+ type="button"
+ class="input-toggle-btn"
+ `@click`="toggleEditClaudeConfigKey"
+ :title="showEditClaudeConfigKey ? t('common.hide') : t('common.show')"
+ :aria-label="showEditClaudeConfigKey ? t('common.hide') : t('common.show')"
+ :aria-pressed="showEditClaudeConfigKey ? 'true' : 'false'">Also applies to: 157-160
🤖 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-ui/partials/index/modals-basic.html` around lines 70 - 73, The eye-toggle
button that calls toggleEditProviderKey and uses showEditProviderKey currently
only sets title; add explicit accessible attributes: set aria-pressed to the
boolean showEditProviderKey and set aria-label to the dynamic label (e.g., use
the same expression as title: showEditProviderKey ? t('common.hide') :
t('common.show')), and apply the identical change to the other icon-only toggle
button (the one using the same pattern around
toggleEditProviderKey/showEditProviderKey at the later occurrence).
ymkiux
force-pushed
the
feat/enable-claude-share-command
branch
from
8a93284 to
cde92c3
Compare
Extend local-bridge.js with Claude provider round-robin support, add backend API routes for bridge toggle/status/excluded/providers sync, and wire up UI controls (toggle, pool panel, i18n) in the Claude config panel. Parity test updated for new data/method keys.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
cli.js (2)
2208-2226:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftProtect plaintext key reads behind a trusted-request gate.
This action returns raw provider secrets, but the
/apipath still treats loopback requests as trusted. That keeps the DNS-rebinding secret-read hole open forget-provider-key. Passreq/headers intogetProviderKey()and reject secret reads unless the request passes an explicit anti-rebinding check before touchingconfig.tomlor bridge settings.Also applies to: 10885-10887
🤖 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 `@cli.js` around lines 2208 - 2226, The getProviderKey function currently returns plaintext secrets by reading config.toml and OPENAI_BRIDGE_SETTINGS_FILE without verifying the request is trusted; change getProviderKey(params = {}, reqOrHeaders) to accept the request/headers, perform the existing anti-rebinding/trusted-request check (the same check used elsewhere in the API gateway) before calling readConfig() or readOpenaiBridgeSettings(), and if the anti-rebinding check fails return an error (e.g., { error: 'unauthorized' }) rather than the key. Ensure you apply the same guard to both the code path that reads provider.preferred_auth_method and the bridge path that reads settings.providers[name].apiKey so no secret is read unless the request passes the anti-rebinding check.
2216-2218:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNarrow transform-provider detection.
includes('/bridge/openai/')will misclassify any upstream URL containing that substring as a managed bridge provider, so the modal can load the bridge key instead of the provider’s realpreferred_auth_method.Suggested fix
- const bridge = typeof provider.codexmate_bridge === 'string' ? provider.codexmate_bridge.trim() : ''; - const isTransform = bridge === 'openai' || String(provider.base_url || '').includes('/bridge/openai/'); + const bridge = typeof provider.codexmate_bridge === 'string' ? provider.codexmate_bridge.trim() : ''; + let isTransform = bridge === 'openai'; + if (!isTransform) { + try { + const parsed = new URL(String(provider.base_url || '').trim()); + isTransform = /^(127\.0\.0\.1|localhost|::1)$/i.test(parsed.hostname) + && parsed.pathname.startsWith('/bridge/openai/'); + } catch (_) {} + }🤖 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 `@cli.js` around lines 2216 - 2218, The current detection uses String(provider.base_url || '').includes('/bridge/openai/') which can false-positive on any URL containing that substring; update the isTransform check to robustly parse provider.base_url (guarding for falsy values) and test the pathname specifically (e.g., new URL(provider.base_url).pathname startsWith('/bridge/openai/')) or compare exact pathname segments instead of includes, keeping the existing codexmate_bridge check (provider.codexmate_bridge === 'openai') and ensuring exceptions from new URL are caught so managed-bridge detection only matches real bridge paths and doesn't override provider.preferred_auth_method.
🤖 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 `@cli.js`:
- Around line 5603-5608: The readClaudeLocalProvidersFile function currently
returns only a providers object and drops the excludedProviders list, causing
syncs to wipe exclusions; modify readClaudeLocalProvidersFile to also read and
return excludedProviders from the CLAUDE_LOCAL_PROVIDERS_FILE (defaulting to an
empty array only if the file value is missing or not an array) so callers (the
Claude bridge provider sync logic) receive and preserve excludedProviders
instead of falling back to [] and losing the opt-out state.
- Around line 5664-5679: The branch that handles disabling when restoreBaseUrl
is empty returns early without updating the persisted Claude settings, leaving
ANTHROPIC_BASE_URL/ANTHROPIC_MODEL still pointing to the bridge; modify this
branch to clear those env vars in the stored settings: load
currentSettings/currentEnv, set ANTHROPIC_BASE_URL = '' (and remove or clear
ANTHROPIC_MODEL if present), update nextSettings.env accordingly, call
ensureDir(CLAUDE_DIR), backupFileIfNeededOnce(CLAUDE_SETTINGS_FILE),
writeJsonAtomic(CLAUDE_SETTINGS_FILE, nextSettings), then set settings.enabled =
false and call writeClaudeLocalBridgeSettings(settings) before returning the
success result so the bridge env is actually removed.
In `@cli/local-bridge.js`:
- Line 266: The proxy currently allows any suffix under /bridge/claude-local/*;
add an explicit guard to only forward POST requests whose upstream path is
exactly /v1/messages. After computing suffix from parsedUrl.pathname, normalize
it to a single-leading-slash path and check that request.method === 'POST' and
normalizedSuffix === '/v1/messages'; if either check fails return a 404 or 405
(and do not forward). Apply the same validation to the other forwarding
branch(s) handling the Claude-local proxy (the blocks around the earlier suffix
use, i.e., the code covering lines ~278-330) so all proxy entry points enforce
method+path equality before proxying upstream.
In `@tests/unit/provider-share-command.test.mjs`:
- Line 197: The test expectations in provider-share-command.test.mjs currently
normalize unquoted URLs; update the expected command strings that include "npm
start -- add ...", "switch ...", and "use ..." so the dynamic baseUrl stays
shell-quoted using the same quoting helper used in code (quoteShellArg). Replace
the plain https://... occurrences in the five failing expectations with the
quoted form (e.g., 'https://api.example.com/v1') — or build the expected string
via quoteShellArg(...) — so the tests assert the generated share commands
include the shell-safe quoted baseUrl.
In `@web-ui/modules/app.methods.claude-config.mjs`:
- Around line 224-233: In toggleClaudeLocalBridge, don’t update UI state or show
a success toast unless the API explicitly indicates success; after calling
api('claude-local-bridge-toggle', { enable }) check for res.error OR res.success
=== false (or absence of res.success) and if so call
this.showMessage(res.message || res.error || 'Toggle failed', 'error') and
return; only then set this.claudeLocalBridgeEnabled and
this.claudeLocalBridgeActive and show the success toast—use the function name
toggleClaudeLocalBridge and the response fields res.success/res.error/res.active
to locate and fix the logic.
- Around line 243-259: The toggleClaudeLocalBridgeExcluded method assumes
this.claudeLocalBridgeExcluded is an array and silently swallows API errors;
update toggleClaudeLocalBridgeExcluded to guard by normalizing
this.claudeLocalBridgeExcluded with Array.isArray (or default to []) before
using indexOf/splice/push, build next from that safe array, and replace the
empty catch with proper error handling (log the error via console or
processLogger and surface user feedback or set an error state instead of
silently ignoring); apply the same normalization and error-handling changes to
the similar exclusion-list method referenced around the other block (lines
261-263) so both methods validate the list and report API failures instead of
no-oping silently.
---
Duplicate comments:
In `@cli.js`:
- Around line 2208-2226: The getProviderKey function currently returns plaintext
secrets by reading config.toml and OPENAI_BRIDGE_SETTINGS_FILE without verifying
the request is trusted; change getProviderKey(params = {}, reqOrHeaders) to
accept the request/headers, perform the existing anti-rebinding/trusted-request
check (the same check used elsewhere in the API gateway) before calling
readConfig() or readOpenaiBridgeSettings(), and if the anti-rebinding check
fails return an error (e.g., { error: 'unauthorized' }) rather than the key.
Ensure you apply the same guard to both the code path that reads
provider.preferred_auth_method and the bridge path that reads
settings.providers[name].apiKey so no secret is read unless the request passes
the anti-rebinding check.
- Around line 2216-2218: The current detection uses String(provider.base_url ||
'').includes('/bridge/openai/') which can false-positive on any URL containing
that substring; update the isTransform check to robustly parse provider.base_url
(guarding for falsy values) and test the pathname specifically (e.g., new
URL(provider.base_url).pathname startsWith('/bridge/openai/')) or compare exact
pathname segments instead of includes, keeping the existing codexmate_bridge
check (provider.codexmate_bridge === 'openai') and ensuring exceptions from new
URL are caught so managed-bridge detection only matches real bridge paths and
doesn't override provider.preferred_auth_method.
🪄 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: 926cee81-4fd6-4ec3-a5fd-f1093fbc818c
📒 Files selected for processing (11)
cli.jscli/local-bridge.jstests/unit/config-tabs-ui.test.mjstests/unit/provider-share-command.test.mjstests/unit/provider-switch-regression.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/panel-config-claude.html
✅ Files skipped from review due to trivial changes (2)
- web-ui/modules/app.methods.startup-claude.mjs
- web-ui/modules/i18n.dict.mjs
📜 Review details
🔇 Additional comments (4)
tests/unit/config-tabs-ui.test.mjs (1)
331-331: Claude share-button assertion is still too permissive.This only validates element presence, not that it remains enabled/interactive.
tests/unit/web-ui-behavior-parity.test.mjs (1)
1273-1273: Parity expectations now codify unquoted URL commands.This mirrors the same shell-quoting regression already noted on share-command tests.
Also applies to: 1315-1315
cli.js (1)
195-196: LGTM!Also applies to: 340-349
web-ui/modules/app.methods.claude-config.mjs (1)
44-49: LGTM!Also applies to: 69-70, 86-92, 209-222, 265-268
Comment on lines
+5603
to
+5608
| function readClaudeLocalProvidersFile() { | ||
| try { | ||
| if (!fs.existsSync(CLAUDE_LOCAL_PROVIDERS_FILE)) return { providers: {} }; | ||
| const raw = JSON.parse(fs.readFileSync(CLAUDE_LOCAL_PROVIDERS_FILE, 'utf-8')); | ||
| return { providers: (raw && typeof raw.providers === 'object') ? raw.providers : {} }; | ||
| } catch (e) { |
Contributor
There was a problem hiding this comment.
Preserve excludedProviders when syncing Claude bridge providers.
readClaudeLocalProvidersFile() drops excludedProviders, then Line 5724 falls back to []. Every provider sync therefore wipes the exclusion list, so the load-balancing opt-out state is lost on the next UI sync.
Suggested fix
function readClaudeLocalProvidersFile() {
try {
- if (!fs.existsSync(CLAUDE_LOCAL_PROVIDERS_FILE)) return { providers: {} };
+ if (!fs.existsSync(CLAUDE_LOCAL_PROVIDERS_FILE)) {
+ return { providers: {}, excludedProviders: [] };
+ }
const raw = JSON.parse(fs.readFileSync(CLAUDE_LOCAL_PROVIDERS_FILE, 'utf-8'));
- return { providers: (raw && typeof raw.providers === 'object') ? raw.providers : {} };
+ return {
+ providers: raw && raw.providers && typeof raw.providers === 'object' && !Array.isArray(raw.providers)
+ ? raw.providers
+ : {},
+ excludedProviders: Array.isArray(raw && raw.excludedProviders)
+ ? raw.excludedProviders.filter((name) => typeof name === 'string' && name.trim())
+ : []
+ };
} catch (e) {
- return { providers: {} };
+ return { providers: {}, excludedProviders: [] };
}
}Also applies to: 5721-5725
🤖 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 `@cli.js` around lines 5603 - 5608, The readClaudeLocalProvidersFile function
currently returns only a providers object and drops the excludedProviders list,
causing syncs to wipe exclusions; modify readClaudeLocalProvidersFile to also
read and return excludedProviders from the CLAUDE_LOCAL_PROVIDERS_FILE
(defaulting to an empty array only if the file value is missing or not an array)
so callers (the Claude bridge provider sync logic) receive and preserve
excludedProviders instead of falling back to [] and losing the opt-out state.
Comment on lines
+5664
to
+5679
| const restoreBaseUrl = settings.lastActiveBaseUrl || ''; | ||
| if (!restoreBaseUrl) { | ||
| settings.enabled = false; | ||
| writeClaudeLocalBridgeSettings(settings); | ||
| return { success: true, enabled: false, notice: '已关闭 Claude 本地负载均衡（无历史配置可恢复）' }; | ||
| } | ||
| const nextEnv = { ...currentEnv, ANTHROPIC_BASE_URL: restoreBaseUrl }; | ||
| if (settings.lastModel) { | ||
| nextEnv.ANTHROPIC_MODEL = settings.lastModel; | ||
| } | ||
| const nextSettings = { ...currentSettings, env: nextEnv }; | ||
| ensureDir(CLAUDE_DIR); | ||
| backupFileIfNeededOnce(CLAUDE_SETTINGS_FILE); | ||
| writeJsonAtomic(CLAUDE_SETTINGS_FILE, nextSettings); | ||
| settings.enabled = false; | ||
| writeClaudeLocalBridgeSettings(settings); |
Contributor
There was a problem hiding this comment.
Clear bridge env vars when disabling without a previous base URL.
If Claude had no ANTHROPIC_BASE_URL before enable, lastActiveBaseUrl stays empty. This branch returns success without touching settings.json, so Claude keeps using /bridge/claude-local/ even though the API now reports the bridge as disabled.
Suggested fix
const restoreBaseUrl = settings.lastActiveBaseUrl || '';
- if (!restoreBaseUrl) {
- settings.enabled = false;
- writeClaudeLocalBridgeSettings(settings);
- return { success: true, enabled: false, notice: '已关闭 Claude 本地负载均衡（无历史配置可恢复）' };
- }
- const nextEnv = { ...currentEnv, ANTHROPIC_BASE_URL: restoreBaseUrl };
- if (settings.lastModel) {
- nextEnv.ANTHROPIC_MODEL = settings.lastModel;
- }
+ const nextEnv = { ...currentEnv };
+ if (restoreBaseUrl) {
+ nextEnv.ANTHROPIC_BASE_URL = restoreBaseUrl;
+ } else {
+ delete nextEnv.ANTHROPIC_BASE_URL;
+ }
+ if (settings.lastModel) {
+ nextEnv.ANTHROPIC_MODEL = settings.lastModel;
+ } else {
+ delete nextEnv.ANTHROPIC_MODEL;
+ }
const nextSettings = { ...currentSettings, env: nextEnv };
ensureDir(CLAUDE_DIR);
backupFileIfNeededOnce(CLAUDE_SETTINGS_FILE);
writeJsonAtomic(CLAUDE_SETTINGS_FILE, nextSettings);
settings.enabled = false;
writeClaudeLocalBridgeSettings(settings);
- return { success: true, enabled: false, restoredBaseUrl: restoreBaseUrl, restoredModel: settings.lastModel };
+ return restoreBaseUrl
+ ? { success: true, enabled: false, restoredBaseUrl: restoreBaseUrl, restoredModel: settings.lastModel }
+ : { success: true, enabled: false, notice: '已关闭 Claude 本地负载均衡（无历史配置可恢复）' };🤖 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 `@cli.js` around lines 5664 - 5679, The branch that handles disabling when
restoreBaseUrl is empty returns early without updating the persisted Claude
settings, leaving ANTHROPIC_BASE_URL/ANTHROPIC_MODEL still pointing to the
bridge; modify this branch to clear those env vars in the stored settings: load
currentSettings/currentEnv, set ANTHROPIC_BASE_URL = '' (and remove or clear
ANTHROPIC_MODEL if present), update nextSettings.env accordingly, call
ensureDir(CLAUDE_DIR), backupFileIfNeededOnce(CLAUDE_SETTINGS_FILE),
writeJsonAtomic(CLAUDE_SETTINGS_FILE, nextSettings), then set settings.enabled =
false and call writeClaudeLocalBridgeSettings(settings) before returning the
success result so the bridge env is actually removed.
| const pool = poolResult.pool; | ||
| const { entry } = pickUpstream(pool); | ||
|
|
||
| const suffix = (parsedUrl.pathname || '').replace(/^\/bridge\/claude-local\/?/, ''); |
Contributor
There was a problem hiding this comment.
Restrict Claude-local proxy to the intended endpoint (/v1/messages) and method.
Current logic proxies arbitrary suffixes under /bridge/claude-local/*. That expands attack surface and bypasses the stated contract of messages-only proxying. Add explicit path/method validation before forwarding upstream.
Suggested guard
- // Proxy Anthropic Messages API requests
+ // Only allow Anthropic Messages API
+ const method = (req.method || 'GET').toUpperCase();
+ if (suffix !== 'v1/messages' && suffix !== 'v1/messages/') {
+ res.writeHead(405, { 'Content-Type': 'application/json; charset=utf-8' });
+ res.end(JSON.stringify({ error: 'Method Not Allowed' }));
+ return;
+ }
+ if (method !== 'POST') {
+ res.writeHead(405, { 'Content-Type': 'application/json; charset=utf-8' });
+ res.end(JSON.stringify({ error: 'Method Not Allowed' }));
+ return;
+ }Also applies to: 278-330
🤖 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 `@cli/local-bridge.js` at line 266, The proxy currently allows any suffix under
/bridge/claude-local/*; add an explicit guard to only forward POST requests
whose upstream path is exactly /v1/messages. After computing suffix from
parsedUrl.pathname, normalize it to a single-leading-slash path and check that
request.method === 'POST' and normalizedSuffix === '/v1/messages'; if either
check fails return a 404 or 405 (and do not forward). Apply the same validation
to the other forwarding branch(s) handling the Claude-local proxy (the blocks
around the earlier suffix use, i.e., the code covering lines ~278-330) so all
proxy entry points enforce method+path equality before proxying upstream.
| assert.strictEqual( | ||
| command, | ||
| "npm start -- add alpha 'https://api.example.com/v1' sk-alpha && npm start -- switch alpha && npm start -- use alpha-share-model" | ||
| "npm start -- add alpha https://api.example.com/v1 sk-alpha && npm start -- switch alpha && npm start -- use alpha-share-model" |
Contributor
There was a problem hiding this comment.
Keep shell quoting for dynamic URL arguments in share commands.
These expectations normalize unquoted URLs, which can break executable commands (and reduce shell-safety) when URLs contain shell-significant characters. Keep baseUrl quoted via quoteShellArg(...) in generated command output and update tests accordingly.
Also applies to: 211-211, 226-226, 242-242, 256-256
🤖 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 `@tests/unit/provider-share-command.test.mjs` at line 197, The test
expectations in provider-share-command.test.mjs currently normalize unquoted
URLs; update the expected command strings that include "npm start -- add ...",
"switch ...", and "use ..." so the dynamic baseUrl stays shell-quoted using the
same quoting helper used in code (quoteShellArg). Replace the plain https://...
occurrences in the five failing expectations with the quoted form (e.g.,
'https://api.example.com/v1') — or build the expected string via
quoteShellArg(...) — so the tests assert the generated share commands include
the shell-safe quoted baseUrl.
Comment on lines
+224
to
+233
| async toggleClaudeLocalBridge(enable) { | ||
| try { | ||
| const res = await api('claude-local-bridge-toggle', { enable }); | ||
| if (res.error) { | ||
| this.showMessage(res.error, 'error'); | ||
| return; | ||
| } | ||
| this.claudeLocalBridgeEnabled = !!enable; | ||
| this.claudeLocalBridgeActive = !!res.active; | ||
| if (enable) { |
Contributor
There was a problem hiding this comment.
Handle unsuccessful toggle responses before updating UI state.
Line 227 only checks res.error. If the API returns { success: false } (without error), the code still updates state and shows a success toast, which can desync UI from backend state.
Suggested fix
async toggleClaudeLocalBridge(enable) {
try {
const res = await api('claude-local-bridge-toggle', { enable });
- if (res.error) {
- this.showMessage(res.error, 'error');
+ if (!res || res.error || res.success === false) {
+ this.showMessage((res && res.error) || '操作失败', 'error');
return;
}
- this.claudeLocalBridgeEnabled = !!enable;
+ this.claudeLocalBridgeEnabled = typeof res.enabled === 'boolean' ? !!res.enabled : !!enable;
this.claudeLocalBridgeActive = !!res.active;
if (enable) {
this.showMessage('Claude 本地负载均衡已启用', 'success');
} else {
this.showMessage('Claude 本地负载均衡已关闭', 'success');Also applies to: 227-237
🤖 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-ui/modules/app.methods.claude-config.mjs` around lines 224 - 233, In
toggleClaudeLocalBridge, don’t update UI state or show a success toast unless
the API explicitly indicates success; after calling
api('claude-local-bridge-toggle', { enable }) check for res.error OR res.success
=== false (or absence of res.success) and if so call
this.showMessage(res.message || res.error || 'Toggle failed', 'error') and
return; only then set this.claudeLocalBridgeEnabled and
this.claudeLocalBridgeActive and show the success toast—use the function name
toggleClaudeLocalBridge and the response fields res.success/res.error/res.active
to locate and fix the logic.
Comment on lines
+243
to
+259
| async toggleClaudeLocalBridgeExcluded(providerName) { | ||
| const name = String(providerName || '').trim(); | ||
| if (!name) return; | ||
| const idx = this.claudeLocalBridgeExcluded.indexOf(name); | ||
| const next = [...this.claudeLocalBridgeExcluded]; | ||
| if (idx >= 0) { | ||
| next.splice(idx, 1); | ||
| } else { | ||
| next.push(name); | ||
| } | ||
| try { | ||
| const res = await api('claude-local-bridge-set-excluded', { names: next }); | ||
| if (res && !res.error) { | ||
| this.claudeLocalBridgeExcluded = next; | ||
| } | ||
| } catch (e) { /* ignore */ } | ||
| }, |
Contributor
There was a problem hiding this comment.
Guard exclusion list access and avoid silent failure on API errors.
Line 246 assumes this.claudeLocalBridgeExcluded is always an array, and Line 258 swallows failures silently. This can produce runtime errors or no-op clicks with no user feedback.
Suggested fix
async toggleClaudeLocalBridgeExcluded(providerName) {
const name = String(providerName || '').trim();
if (!name) return;
- const idx = this.claudeLocalBridgeExcluded.indexOf(name);
- const next = [...this.claudeLocalBridgeExcluded];
+ const current = Array.isArray(this.claudeLocalBridgeExcluded) ? this.claudeLocalBridgeExcluded : [];
+ const idx = current.indexOf(name);
+ const next = [...current];
if (idx >= 0) {
next.splice(idx, 1);
} else {
next.push(name);
}
try {
const res = await api('claude-local-bridge-set-excluded', { names: next });
- if (res && !res.error) {
+ if (res && !res.error && res.success !== false) {
this.claudeLocalBridgeExcluded = next;
+ } else {
+ this.showMessage((res && res.error) || '更新排除列表失败', 'error');
}
- } catch (e) { /* ignore */ }
+ } catch (e) {
+ this.showMessage('更新排除列表失败', 'error');
+ }
},
isClaudeLocalBridgeExcluded(providerName) {
- return this.claudeLocalBridgeExcluded.indexOf(String(providerName || '').trim()) >= 0;
+ const current = Array.isArray(this.claudeLocalBridgeExcluded) ? this.claudeLocalBridgeExcluded : [];
+ return current.indexOf(String(providerName || '').trim()) >= 0;
},Also applies to: 261-263
🤖 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-ui/modules/app.methods.claude-config.mjs` around lines 243 - 259, The
toggleClaudeLocalBridgeExcluded method assumes this.claudeLocalBridgeExcluded is
an array and silently swallows API errors; update
toggleClaudeLocalBridgeExcluded to guard by normalizing
this.claudeLocalBridgeExcluded with Array.isArray (or default to []) before
using indexOf/splice/push, build next from that safe array, and replace the
empty catch with proper error handling (log the error via console or
processLogger and surface user feedback or set an error state instead of
silently ignoring); apply the same normalization and error-handling changes to
the similar exclusion-list method referenced around the other block (lines
261-263) so both methods validate the list and report API failures instead of
no-oping silently.
Replace toggle switch with a 'local' provider card in the card-list, matching the Codex pattern. Clicking the card shows the bridge pool panel below with provider checkboxes and enable/disable button.
ymkiux
changed the title
- Add lib/win-tray.js: PowerShell-based tray icon manager - Blue "C" icon with tooltip showing app name and port - Context menu: Open browser / Exit - Exit writes marker file, Node.js polls and exits gracefully - Integrate into cmdStart server lifecycle - Fix parity test for removed dead data properties - Fix web-run-host test mock for startWinTray binding
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@lib/win-tray.js`:
- Line 18: The tray URL currently hardcodes "localhost" when constructing const
url = `http://localhost:${port}`; change it to use the resolved bind host
variable (e.g. bindHost or resolvedHost) instead of "localhost" so the tray Open
action and tooltip point to the actual server address; update the template to
`http://${bindHost}:${port}` (handle IPv6/bracketed hosts if your code uses
them) and ensure the same variable is used wherever url is referenced in
lib/win-tray.js.
- Around line 45-46: The embedded C# references an undefined variable pid in the
exit handler (exitItem.Click) causing compilation to fail; update the handler in
the CodexMateTrayIcon class to avoid using pid (e.g., write an empty string or a
constant) by replacing System.IO.File.WriteAllText(_exitMarkerPath,
pid.ToString()) with a call that writes an empty value
(System.IO.File.WriteAllText(_exitMarkerPath, "")) so the undefined pid is
removed and the tray compiles and starts normally.
- Line 72: The PowerShell single-quoted arguments in the New-Object template
must have internal apostrophes doubled; update the interpolation for name, url,
and exitMarkerPath in lib/win-tray.js so each value is escaped with
replace(/'/g, "''") (and still preserve the existing backslash-to-backslash
replace for exitMarkerPath) before embedding into the template line that creates
CodexMateTrayIcon (the line using $app = New-Object CodexMateTrayIcon
-ArgumentList ...). Ensure all three interpolated values use this escaping so
names like "CodexMate's App" or paths with "O'Brien" don't break PowerShell
parsing.
🪄 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: 918cd57d-836b-40fb-834c-480992b1b650
📒 Files selected for processing (8)
cli.jslib/win-tray.jstests/unit/web-run-host.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/panel-config-claude.html
✅ Files skipped from review due to trivial changes (1)
- web-ui/modules/i18n.dict.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
- web-ui/partials/index/panel-config-claude.html
- web-ui/modules/app.methods.claude-config.mjs
- tests/unit/web-ui-behavior-parity.test.mjs
- cli.js
Temporarily removed due to detection bugs with upstream providers.
Temporarily removed due to detection bugs with upstream providers.
… better reliability
ymkiux
force-pushed
the
feat/enable-claude-share-command
branch
from
f82cd8b to
6a5b08f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/partials/index/panel-config-codex.html (1)
175-186:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize bridge-pool strings instead of hardcoding Chinese text.
This section bypasses existing i18n and will render Chinese in other locales. Please move these labels to translation keys (
t(...)).Suggested patch
- <span class="bridge-pool-title">轮询池</span> - <span class="bridge-pool-hint">勾选参与负载均衡的提供商</span> + <span class="bridge-pool-title">{{ t('config.bridgePool.title') }}</span> + <span class="bridge-pool-hint">{{ t('config.bridgePool.hint') }}</span> ... - <span>暂无可用上游 provider，请先添加直连 provider</span> + <span>{{ t('config.bridgePool.empty') }}</span> ... - <span class="bridge-pool-item-status" :class="{ active: !isLocalBridgeExcluded(cp.name) }">{{ isLocalBridgeExcluded(cp.name) ? '未启用' : '已启用' }}</span> + <span class="bridge-pool-item-status" :class="{ active: !isLocalBridgeExcluded(cp.name) }">{{ isLocalBridgeExcluded(cp.name) ? t('config.bridgePool.disabled') : t('config.bridgePool.enabled') }}</span>🤖 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-ui/partials/index/panel-config-codex.html` around lines 175 - 186, The template hardcodes Chinese strings for the bridge pool UI; replace them with i18n keys using the t(...) helper so other locales render correctly: wrap "轮询池" and "勾选参与负载均衡的提供商" with t('...') keys, replace the empty-state text "暂无可用上游 provider，请先添加直连 provider" with t('...'), and use t(...) for the status labels "未启用"/"已启用" inside the span that uses isLocalBridgeExcluded(cp.name); keep logic using localBridgeCandidateProviders(), isLocalBridgeExcluded(...) and toggleLocalBridgeExcluded(...) unchanged but reference the new translation keys instead of hardcoded Chinese.
🤖 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 `@cli/update.js`:
- Around line 61-63: The catch block in cli/update.js logs update failures but
leaves the process exit code as success; update the catch handler (the catch
around the update logic) to propagate failure by setting a non-zero exit status
(e.g., set process.exitCode = 1 or call process.exit(1)) after logging the error
so automation detects the failure. Ensure you keep the existing
console.error(`[Update] 更新失败: ${err.message}`) and then set the exit code in the
same catch block.
- Around line 66-83: fetchLatestVersion currently can hang and attempts to parse
any HTTP response body without checking status; update fetchLatestVersion to add
a request timeout and HTTP status handling: when creating the GET request (in
fetchLatestVersion) set a timeout (e.g., req.setTimeout or AbortController) that
rejects with a clear timeout error and aborts/cleans up the request, and before
parsing JSON check res.statusCode and reject with a descriptive error if it's
not 200 (include the statusCode and statusMessage), and ensure all error/timeout
handlers remove listeners and stop further processing to avoid memory leaks.
In `@README.md`:
- Around line 132-134: The <img> tag for the contributor badge is missing an alt
attribute; update the <img> element (the one with
src="https://contrib.rocks/image?repo=SakuraByteCore/codexmate") to include a
descriptive alt value (for example: alt="Contributors to
SakuraByteCore/codexmate") so the image is accessible and satisfies markdownlint
MD045.
In `@README.zh.md`:
- Around line 132-134: The contributor image tag lacks alt text which triggers
markdownlint MD045 and hurts accessibility; update the <img> element in the
README snippet (the <img
src="https://contrib.rocks/image?repo=SakuraByteCore/codexmate" />) to include a
meaningful alt attribute (e.g., alt="Contributors to SakuraByteCore/codexmate"
or similar) so the markup becomes accessible and satisfies the linter.
---
Outside diff comments:
In `@web-ui/partials/index/panel-config-codex.html`:
- Around line 175-186: The template hardcodes Chinese strings for the bridge
pool UI; replace them with i18n keys using the t(...) helper so other locales
render correctly: wrap "轮询池" and "勾选参与负载均衡的提供商" with t('...') keys, replace the
empty-state text "暂无可用上游 provider，请先添加直连 provider" with t('...'), and use t(...)
for the status labels "未启用"/"已启用" inside the span that uses
isLocalBridgeExcluded(cp.name); keep logic using
localBridgeCandidateProviders(), isLocalBridgeExcluded(...) and
toggleLocalBridgeExcluded(...) unchanged but reference the new translation keys
instead of hardcoded Chinese.
🪄 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: 6c716300-84fe-4aa8-9321-21138512d725
📒 Files selected for processing (11)
README.mdREADME.zh.mdcli.jscli/update.jslib/win-tray.jspackage.jsontests/unit/readme-docs-consistency.test.mjstests/unit/run.mjsweb-ui/partials/index/panel-config-claude.htmlweb-ui/partials/index/panel-config-codex.htmlweb-ui/partials/index/panel-dashboard.html
💤 Files with no reviewable changes (1)
- web-ui/partials/index/panel-dashboard.html
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- web-ui/partials/index/panel-config-claude.html
- lib/win-tray.js
📜 Review details
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~32-~32: Using many exclamation marks might seem excessive (in this case: 5 exclamation marks for a text that’s 2746 characters long)
Context: ... to help build the local agent ecosystem! ## What is Codex Mate? Have you ever ...
(EN_EXCESSIVE_EXCLAMATION)
README.zh.md
[uncategorized] ~7-~7: 数词与名词之间一般应存在量词，可能缺少量词。
Context: ...e logo" width="160" /> # Codex Mate **一站式本地 AI 编程智能体管理面板。统一管理 Codex、Claude Code ...
(wa5)
🪛 markdownlint-cli2 (0.22.1)
README.md
[warning] 30-30: Blank line inside blockquote
(MD028, no-blanks-blockquote)
[warning] 133-133: Images should have alternate text (alt text)
(MD045, no-alt-text)
README.zh.md
[warning] 30-30: Blank line inside blockquote
(MD028, no-blanks-blockquote)
[warning] 133-133: Images should have alternate text (alt text)
(MD045, no-alt-text)
🔇 Additional comments (4)
cli.js (4)
2209-2227: Lock down plaintext key reads before exposing them over/api.This helper still returns raw provider secrets without any anti-rebinding/trusted-request gate. Because
/apistill fully trusts loopback requests, a DNS-rebinding page can reachget-provider-keyand exfiltrate local keys once the Web UI is open.
2217-2223: Narrow the transform-provider detection.Treating any
base_urlcontaining/bridge/openai/as managed bridge traffic can still misclassify a normal upstream URL and make the edit flow readsettings.providers[name].apiKeyinstead of the provider’s real secret.
5604-5610: PreserveexcludedProviderswhen reading the Claude bridge providers file.
readClaudeLocalProvidersFile()still dropsexcludedProviders, sosyncClaudeBridgeProviders()falls back to[]and wipes the user’s opt-out state on the next sync.Also applies to: 5722-5726
5665-5681: Clear Claude bridge env vars even when there is no previous base URL to restore.If
lastActiveBaseUrlis empty, this branch still returns before updating~/.claude/settings.json, soANTHROPIC_BASE_URL/ANTHROPIC_MODELcan keep pointing at the local bridge while the API reports the feature as disabled.
Comment on lines
+61
to
+63
| } catch (err) { | ||
| console.error(`[Update] 更新失败: ${err.message}`); | ||
| } |
Contributor
There was a problem hiding this comment.
Propagate update failure via exit code.
On update failure, the command logs but still exits successfully. Set a non-zero exit code so automation can detect failure.
Suggested patch
} catch (err) {
console.error(`[Update] 更新失败: ${err.message}`);
+ process.exitCode = 1;
}
}📝 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
| } catch (err) { | |
| console.error(`[Update] 更新失败: ${err.message}`); | |
| } | |
| } catch (err) { | |
| console.error(`[Update] 更新失败: ${err.message}`); | |
| process.exitCode = 1; | |
| } |
🤖 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 `@cli/update.js` around lines 61 - 63, The catch block in cli/update.js logs
update failures but leaves the process exit code as success; update the catch
handler (the catch around the update logic) to propagate failure by setting a
non-zero exit status (e.g., set process.exitCode = 1 or call process.exit(1))
after logging the error so automation detects the failure. Ensure you keep the
existing console.error(`[Update] 更新失败: ${err.message}`) and then set the exit
code in the same catch block.
Comment on lines
+66
to
+83
| async function fetchLatestVersion() { | ||
| return new Promise((resolve, reject) => { | ||
| const url = 'https://registry.npmjs.org/codexmate/latest'; | ||
| https.get(url, (res) => { | ||
| let data = ''; | ||
| res.on('data', (chunk) => { data += chunk; }); | ||
| res.on('end', () => { | ||
| try { | ||
| const json = JSON.parse(data); | ||
| resolve(json.version || ''); | ||
| } catch (e) { | ||
| reject(new Error('解析 NPM 响应失败')); | ||
| } | ||
| }); | ||
| }).on('error', (err) => { | ||
| reject(err); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add timeout and HTTP status handling in registry fetch.
fetchLatestVersion() can hang indefinitely and may parse non-200 responses as normal payload. Please fail fast with timeout + status checks before JSON parse.
Suggested patch
function fetchLatestVersion() {
return new Promise((resolve, reject) => {
const url = 'https://registry.npmjs.org/codexmate/latest';
- https.get(url, (res) => {
+ const req = https.get(url, (res) => {
+ if (res.statusCode !== 200) {
+ res.resume();
+ reject(new Error(`NPM registry HTTP ${res.statusCode}`));
+ return;
+ }
let data = '';
res.on('data', (chunk) => { data += chunk; });
res.on('end', () => {
try {
const json = JSON.parse(data);
resolve(json.version || '');
} catch (e) {
reject(new Error('解析 NPM 响应失败'));
}
});
- }).on('error', (err) => {
+ });
+ req.setTimeout(8000, () => {
+ req.destroy(new Error('请求 NPM 超时'));
+ });
+ req.on('error', (err) => {
reject(err);
});
});
}🤖 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 `@cli/update.js` around lines 66 - 83, fetchLatestVersion currently can hang
and attempts to parse any HTTP response body without checking status; update
fetchLatestVersion to add a request timeout and HTTP status handling: when
creating the GET request (in fetchLatestVersion) set a timeout (e.g.,
req.setTimeout or AbortController) that rejects with a clear timeout error and
aborts/cleans up the request, and before parsing JSON check res.statusCode and
reject with a descriptive error if it's not 200 (include the statusCode and
statusMessage), and ensure all error/timeout handlers remove listeners and stop
further processing to avoid memory leaks.
Comment on lines
+132
to
+134
| <a href="https://github.com/SakuraByteCore/codexmate/graphs/contributors"> | ||
| <img src="https://contrib.rocks/image?repo=SakuraByteCore/codexmate" /> | ||
| </a> |
Contributor
There was a problem hiding this comment.
Add alt text for contributor image.
The <img> tag is missing alt, which hurts accessibility and triggers markdownlint MD045.
Suggested patch
- <img src="https://contrib.rocks/image?repo=SakuraByteCore/codexmate" />
+ <img src="https://contrib.rocks/image?repo=SakuraByteCore/codexmate" alt="Codex Mate contributors" />📝 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
| <a href="https://github.com/SakuraByteCore/codexmate/graphs/contributors"> | |
| <img src="https://contrib.rocks/image?repo=SakuraByteCore/codexmate" /> | |
| </a> | |
| <a href="https://github.com/SakuraByteCore/codexmate/graphs/contributors"> | |
| <img src="https://contrib.rocks/image?repo=SakuraByteCore/codexmate" alt="Codex Mate contributors" /> | |
| </a> |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 133-133: Images should have alternate text (alt text)
(MD045, no-alt-text)
🤖 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 `@README.md` around lines 132 - 134, The <img> tag for the contributor badge is
missing an alt attribute; update the <img> element (the one with
src="https://contrib.rocks/image?repo=SakuraByteCore/codexmate") to include a
descriptive alt value (for example: alt="Contributors to
SakuraByteCore/codexmate") so the image is accessible and satisfies markdownlint
MD045.
Comment on lines
+132
to
+134
| <a href="https://github.com/SakuraByteCore/codexmate/graphs/contributors"> | ||
| <img src="https://contrib.rocks/image?repo=SakuraByteCore/codexmate" /> | ||
| </a> |
Contributor
There was a problem hiding this comment.
为贡献者图片补充 alt 文本。
当前 <img> 缺少 alt，会触发 markdownlint MD045，也影响可访问性。
建议修改
- <img src="https://contrib.rocks/image?repo=SakuraByteCore/codexmate" />
+ <img src="https://contrib.rocks/image?repo=SakuraByteCore/codexmate" alt="Codex Mate 贡献者列表" />📝 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
| <a href="https://github.com/SakuraByteCore/codexmate/graphs/contributors"> | |
| <img src="https://contrib.rocks/image?repo=SakuraByteCore/codexmate" /> | |
| </a> | |
| <a href="https://github.com/SakuraByteCore/codexmate/graphs/contributors"> | |
| <img src="https://contrib.rocks/image?repo=SakuraByteCore/codexmate" alt="Codex Mate 贡献者列表" /> | |
| </a> |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 133-133: Images should have alternate text (alt text)
(MD045, no-alt-text)
🤖 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 `@README.zh.md` around lines 132 - 134, The contributor image tag lacks alt
text which triggers markdownlint MD045 and hurts accessibility; update the <img>
element in the README snippet (the <img
src="https://contrib.rocks/image?repo=SakuraByteCore/codexmate" />) to include a
meaningful alt attribute (e.g., alt="Contributors to SakuraByteCore/codexmate"
or similar) so the markup becomes accessible and satisfies the linter.
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.
Summary
Tests
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores