feat(web-ui): provider presets, model memory, settings redesign and v0.0.28#156
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds per-provider model memory ( ChangesPer-provider model memory, Codex catalogs, and transform detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
SurviveM
force-pushed
the
feat/provider-management-optimization
branch
from
79abf67 to
92ae559
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/modules/app.methods.providers.mjs`:
- Around line 331-335: The current update only sets codexmate_bridge when
this.editingProvider.useTransform is true, leaving a stale value when transform
is turned off; in the block that prepares updates before calling
incrementallyUpdateProvider (around the const updates = ... and the if
(this.editingProvider && this.editingProvider.useTransform) check), add an else
branch that clears the bridge marker (e.g., set updates.codexmate_bridge to null
or an empty string) so incrementallyUpdateProvider receives an explicit removal
and providersList/UI state is cleared accordingly.
In `@web-ui/modules/app.methods.startup-claude.mjs`:
- Around line 116-118: The async prefetch call in loadAll() can finish out of
order and overwrite a newer upstreamUrlCache; to fix, add a per-instance
prefetch token/version (e.g., this._prefetchVersion) that you increment before
calling prefetchUpstreamUrls(this.providersList, api) and capture into a local
const (prefetchId), then when the promise resolves check that the current
instance token still equals prefetchId before assigning this.upstreamUrlCache =
cache; use the same token check in the .catch() path to avoid swallowing newer
results; reference prefetchUpstreamUrls, providersList, api, upstreamUrlCache,
and loadAll().
In `@web-ui/modules/provider-url-display.mjs`:
- Around line 36-37: The code trusts prototype properties when checking
upstreamCache (e.g. the conditional using upstreamCache &&
upstreamCache[providerName]); change the lookup to use safe own-property checks
such as Object.prototype.hasOwnProperty.call(upstreamCache, providerName) (or
switch upstreamCache to a Map) before returning upstreamCache[providerName];
apply the same fix to the other occurrence in the file (the block around lines
55–67) so providerName like "toString" cannot falsely resolve via the prototype
chain.
In `@web-ui/partials/index/panel-config-codex.html`:
- Line 212: Replace the hardcoded Chinese title on the transform icon span with
an i18n call and add an aria-label: change the span that checks
isTransformProvider(provider) (the element rendering the
"provider-transform-icon") to use t('...') for the tooltip text (e.g.,
t('panel.transform_adapter') or existing key) and set the same translated string
to aria-label so both tooltip and accessibility label come from i18n; ensure you
import/use the template's existing t(...) helper consistent with other titles in
this file.
🪄 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: c99034e3-4004-4439-a0cd-5ce192ca9876
📒 Files selected for processing (8)
web-ui/app.jsweb-ui/modules/app.computed.dashboard.mjsweb-ui/modules/app.methods.providers.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/modules/provider-incremental-update.mjsweb-ui/modules/provider-url-display.mjsweb-ui/partials/index/panel-config-codex.htmlweb-ui/styles/titles-cards.css
📜 Review details
🔇 Additional comments (4)
web-ui/styles/titles-cards.css (1)
414-429: Looks good for the transform icon styling.The new
.provider-transform-iconrules are clean and consistent with the existing card typography/hover system.web-ui/modules/provider-incremental-update.mjs (1)
11-91: Incremental mutation helpers look solid.Add/update/remove flows are clearly separated, and current-provider side effects are handled in one place.
web-ui/app.js (1)
43-43: State addition is appropriate.
upstreamUrlCacheis correctly introduced at app state level for downstream computed/template usage.web-ui/modules/app.computed.dashboard.mjs (1)
45-57: Dashboard computed integration looks good.The new URL-display and transform-detection helpers cleanly support the provider-card UI behavior.
| const updates = { url: validation.url }; | ||
| if (this.editingProvider && this.editingProvider.useTransform) { | ||
| updates.codexmate_bridge = 'openai'; | ||
| } | ||
| incrementallyUpdateProvider(this, validation.name, updates); |
There was a problem hiding this comment.
Clear bridge marker when transform is turned off.
Line 332-334 only writes codexmate_bridge for the true case. Disabling transform leaves stale bridge state in providersList, so UI indicators can stay wrong.
💡 Suggested fix
const updates = { url: validation.url };
- if (this.editingProvider && this.editingProvider.useTransform) {
- updates.codexmate_bridge = 'openai';
- }
+ updates.codexmate_bridge = (this.editingProvider && this.editingProvider.useTransform)
+ ? 'openai'
+ : '';
incrementallyUpdateProvider(this, validation.name, updates);🤖 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 331 - 335, The current
update only sets codexmate_bridge when this.editingProvider.useTransform is
true, leaving a stale value when transform is turned off; in the block that
prepares updates before calling incrementallyUpdateProvider (around the const
updates = ... and the if (this.editingProvider &&
this.editingProvider.useTransform) check), add an else branch that clears the
bridge marker (e.g., set updates.codexmate_bridge to null or an empty string) so
incrementallyUpdateProvider receives an explicit removal and providersList/UI
state is cleared accordingly.
| prefetchUpstreamUrls(this.providersList, api).then(cache => { | ||
| this.upstreamUrlCache = cache; | ||
| }).catch(() => {}); |
There was a problem hiding this comment.
Guard against stale async prefetch overwriting newer cache.
Line 116-118 can race across consecutive loadAll() executions; an older prefetch may finish later and replace a newer upstreamUrlCache.
💡 Suggested fix
- prefetchUpstreamUrls(this.providersList, api).then(cache => {
- this.upstreamUrlCache = cache;
- }).catch(() => {});
+ const prefetchToken = Symbol('upstream-prefetch');
+ this._upstreamUrlPrefetchToken = prefetchToken;
+ prefetchUpstreamUrls(this.providersList, api)
+ .then((cache) => {
+ if (this._upstreamUrlPrefetchToken === prefetchToken) {
+ this.upstreamUrlCache = cache;
+ }
+ })
+ .catch(() => {});📝 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.
| prefetchUpstreamUrls(this.providersList, api).then(cache => { | |
| this.upstreamUrlCache = cache; | |
| }).catch(() => {}); | |
| const prefetchToken = Symbol('upstream-prefetch'); | |
| this._upstreamUrlPrefetchToken = prefetchToken; | |
| prefetchUpstreamUrls(this.providersList, api) | |
| .then((cache) => { | |
| if (this._upstreamUrlPrefetchToken === prefetchToken) { | |
| this.upstreamUrlCache = cache; | |
| } | |
| }) | |
| .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 `@web-ui/modules/app.methods.startup-claude.mjs` around lines 116 - 118, The
async prefetch call in loadAll() can finish out of order and overwrite a newer
upstreamUrlCache; to fix, add a per-instance prefetch token/version (e.g.,
this._prefetchVersion) that you increment before calling
prefetchUpstreamUrls(this.providersList, api) and capture into a local const
(prefetchId), then when the promise resolves check that the current instance
token still equals prefetchId before assigning this.upstreamUrlCache = cache;
use the same token check in the .catch() path to avoid swallowing newer results;
reference prefetchUpstreamUrls, providersList, api, upstreamUrlCache, and
loadAll().
| <div class="card-title"> | ||
| <span>{{ provider.name }}</span> | ||
| <span v-if="provider.readOnly" class="provider-readonly-badge">{{ t('config.badge.system') }}</span> | ||
| <span v-if="isTransformProvider(provider)" class="provider-transform-icon" title="通过内建转换适配">🔌</span> |
There was a problem hiding this comment.
Localize the transform icon tooltip text.
Line 212 hardcodes Chinese text in title, while this template otherwise uses t(...). Please move it to i18n (and ideally mirror to aria-label).
💡 Suggested fix
-<span v-if="isTransformProvider(provider)" class="provider-transform-icon" title="通过内建转换适配">🔌</span>
+<span
+ v-if="isTransformProvider(provider)"
+ class="provider-transform-icon"
+ :title="t('config.provider.transformHint')"
+ :aria-label="t('config.provider.transformHint')"
+>🔌</span>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <span v-if="isTransformProvider(provider)" class="provider-transform-icon" title="通过内建转换适配">🔌</span> | |
| <span | |
| v-if="isTransformProvider(provider)" | |
| class="provider-transform-icon" | |
| :title="t('config.provider.transformHint')" | |
| :aria-label="t('config.provider.transformHint')" | |
| >🔌</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` at line 212, Replace the
hardcoded Chinese title on the transform icon span with an i18n call and add an
aria-label: change the span that checks isTransformProvider(provider) (the
element rendering the "provider-transform-icon") to use t('...') for the tooltip
text (e.g., t('panel.transform_adapter') or existing key) and set the same
translated string to aria-label so both tooltip and accessibility label come
from i18n; ensure you import/use the template's existing t(...) helper
consistent with other titles in this file.
49ae8ff to
90203ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
update_providers.sed (1)
1-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBuild-time sed patcher should not live in the repo.
This is a single-use migration script. The transformation should be applied directly to
web-ui/modules/app.methods.providers.mjsand committed as a normal source change instead of leaving the sed recipe in tree, where it will rot and confuse future readers.🤖 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 `@update_providers.sed` around lines 1 - 7, The sed patch file should be removed and the transformation applied directly to the source: edit the addProvider method in web-ui/modules/app.methods.providers.mjs (the function that currently calls this.showMessage(...); this.closeAddModal(); await this.loadAll();) to instead create a newProvider object with properties name (validation.name), url (validation.url), hasKey (Boolean of this.newProvider.key), readOnly:false, nonEditable:false, and if this.newProvider && this.newProvider.useTransform set newProvider.useTransform = true and newProvider.codexmate_bridge = 'openai'; then call incrementallyAddProvider(this, newProvider) after this.closeAddModal() (replacing the await this.loadAll() call); finally delete the single-use sed patch file (update_providers.sed) from the repo.add_transform_method.js (1)
1-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the patcher; the inlined
isTransformProvideralso duplicates the exported helper.
- Same hardcoded
/storage/emulated/0/...path issue as the otherfix_*.jsscripts — please don't commit these.- The injected method body inlines the exact predicate already exported as
isTransformProviderfromweb-ui/modules/provider-url-display.mjs. If/when this change is applied directly inapp.computed.dashboard.mjs, import that helper instead of re-implementing it; otherwise the bridge-detection logic will drift between two files.♻️ Suggested shape if the change is applied directly
-import { getProviderDisplayUrl } from './provider-url-display.mjs'; +import { getProviderDisplayUrl, isTransformProvider } from './provider-url-display.mjs'; ... displayProviderUrl() { return (provider) => getProviderDisplayUrl(provider, this.upstreamUrlCache || {}); }, + + isTransformProvider() { + return (provider) => isTransformProvider(provider); + },🤖 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 `@add_transform_method.js` around lines 1 - 12, Remove this file-based patcher and the hardcoded filesystem path; do not commit scripts that mutate sources with absolute local paths. If you need the isTransformProvider behavior in app.computed.dashboard.mjs, import the exported helper from web-ui/modules/provider-url-display.mjs (use the module's exported isTransformProvider) and call that inside the class instead of inlining the predicate; delete the duplicated inlined implementation in your addition and update any code that referenced the new method to delegate to the imported isTransformProvider. Ensure any automation that modifies source files uses relative/repo paths or build-time transforms rather than absolute device paths.fix_startup_import.js (1)
1-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't commit one-off, environment-specific patcher scripts.
This file is a throwaway in-place fixer hardcoded to the author's device path (
/storage/emulated/0/yun/project/code-test/1/codexmate/...). It can't run in any other environment, thebrokenImportpattern only matches a transient malformed state that shouldn't exist onmain, and it is not wired into build/CI. Please remove it from the PR (and add a.gitignoreentry for thesefix_*.jshelpers if they are kept locally).🤖 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 `@fix_startup_import.js` around lines 1 - 12, This file fix_startup_import.js is a throwaway, environment-specific patcher (uses hardcoded filePath and one-off replacement logic via fs.readFileSync/fs.writeFileSync, brokenImport/fixedImport) and must be removed from the PR; delete the file from the branch, revert any changes it applied, and if you want to keep local fixer scripts, move them out of the repo and add a .gitignore entry (e.g., ignore fix_*.js) so they are not committed; do not integrate hardcoded filePath or one-off content.replace logic into the codebase or CI.web-ui/modules/app.methods.providers.mjs.backup2 (1)
1-406:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove all backup files before merging.
The repository contains four accidental backup copies:
app.computed.dashboard.mjs.backup,app.methods.providers.mjs.backup,app.methods.providers.mjs.backup2, andapp.methods.startup-claude.mjs.backup. Theapp.methods.providers.mjs.backup2file specifically importsincrementallyAddProvider,incrementallyUpdateProvider, andincrementallyRemoveProvideron line 2 but never uses them—insteadaddProvider,deleteProvider, andupdateProviderstill callawait this.loadAll(). This contradicts the PR's "incremental update" refactoring goal. Delete all.backup*files and add*.backup*to.gitignore.🤖 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.backup2` around lines 1 - 406, Remove all accidental backup files from the repo and add a gitignore entry for the pattern *.backup*; in this specific backup file remove the unused import list (incrementallyAddProvider, incrementallyUpdateProvider, incrementallyRemoveProvider) or, if you intend to keep the incremental-refactor, replace the post-operation await this.loadAll() calls in addProvider, deleteProvider, and updateProvider with calls to incrementallyAddProvider(...), incrementallyRemoveProvider(...), and incrementallyUpdateProvider(...) respectively (passing the same payload/params and handling their responses), then delete the backup file so no unused imports remain.fix_dashboard_final.js (1)
1-23:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove this patcher; it attempts to revert working code to a broken state.
Hardcoded Android path on line 2 — apply changes directly to
web-ui/modules/app.computed.dashboard.mjsinstead of shipping a throwaway script.The patcher injects a broken call signature:
getProviderDisplayUrl(provider, this.upstreamUrlCache || {})However,
provider-url-display.mjsexportsgetProviderDisplayUrl(provider)with only one parameter. The second argument is silently ignored.More critically: the actual
app.computed.dashboard.mjsalready has the correct single-argument call on line 46. This patcher would revert that working code to the broken two-argument version found in the.backupfile. The code is already fixed; this patcher would break it.🤖 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 `@fix_dashboard_final.js` around lines 1 - 23, This patcher script (fix_dashboard_final.js) should be removed because it writes a hardcoded Android path and reintroduces a broken two-argument call to getProviderDisplayUrl; delete the script and any CI/packaging step that runs it, and ensure the real module web-ui/modules/app.computed.dashboard.mjs retains the existing correct single-argument call to getProviderDisplayUrl(provider) inside createDashboardComputed's displayProviderUrl method (do not replace or inject a call like getProviderDisplayUrl(provider, this.upstreamUrlCache || {})); if you must automate fixes, update the automation to edit the actual module path and only enforce the single-argument signature in displayProviderUrl instead of injecting the two-argument variant.web-ui/partials/index/panel-config-codex.html (1)
209-216:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the resolved provider URL in the card subtitle.
Line 212 adds the transform badge, but Line 215 still renders
provider.url. For built-in transform providers, that means the UI can show the 🔌 indicator and still display the URL as unset, so the upstream-URL feature never becomes visible.Minimal fix
- <div class="card-subtitle"> - {{ provider.url || t('config.url.unset') }} - </div> + <div class="card-subtitle"> + {{ displayProviderUrl(provider) || t('config.url.unset') }} + </div>🤖 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 209 - 216, The card subtitle currently shows provider.url which hides the upstream-URL when a transform provider is used; update the subtitle to use the provider's resolved URL (e.g., provider.resolvedUrl) instead of provider.url so the upstream-URL is visible for providers adapted via isTransformProvider(provider): replace the usage of provider.url in the card-subtitle with provider.resolvedUrl || t('config.url.unset') (or call the existing helper that returns the resolved URL) while keeping the transform badge logic intact.
♻️ Duplicate comments (1)
web-ui/modules/app.methods.providers.mjs.backup (1)
1-406:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame issue as
.backup2— please remove this.backupsnapshot.Identical pattern to
app.methods.providers.mjs.backup2: stale manual copy of the live module, imports the incremental-update helpers on line 2 but the CRUD methods still fall back toawait this.loadAll(). Drop the file along with the.backup2sibling.🤖 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.backup` around lines 1 - 406, This file is a stale backup snapshot that re-introduces imports (incrementallyAddProvider, incrementallyUpdateProvider, incrementallyRemoveProvider) but still uses full reloads (await this.loadAll()) instead of the incremental helpers; remove this .backup file (and its .backup2 sibling) from the repository and any references to it so the real module (createProvidersMethods and its methods like addProvider, updateProvider, deleteProvider, openEditModal) is the single source of truth; if there are any accidental imports of this backup elsewhere, update them to import the real module instead and ensure the incremental helper imports remain only in the real module where they are used.
🧹 Nitpick comments (3)
web-ui/modules/app.methods.startup-claude.mjs.backup (1)
1-9: ⚡ Quick winPlease drop this stale
.backupmodule.The opening import block is already half-applied and no longer reflects a usable source snapshot. Keeping broken backup files in the tree increases the chance of future copy/paste regressions as the real module evolves.
🤖 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.startup-claude.mjs.backup` around lines 1 - 9, Delete the stale backup module file web-ui/modules/app.methods.startup-claude.mjs.backup from the repo; this file contains a half-applied import block (e.g., references like prefetchUpstreamUrls, findDuplicateClaudeConfigName, getClaudeModelCatalogForBaseUrl, matchClaudeConfigFromSettings, normalizeClaudeConfig, normalizeClaudeSettingsEnv, normalizeClaudeValue) and should not be kept—remove the file and ensure no build/config references point to this backup so only the real app.methods.startup-claude.mjs remains in use.fix_dashboard_computed.js (1)
2-17: ⚡ Quick winMake this fixer portable or keep it out of the repo.
filePathis hardcoded to a device-specific absolute path, and the uncheckedreplace()calls can silently no-op or duplicate the injected blocks on reruns. If this script still matters, resolve the target relative to the repo and fail fast when the expected text is missing; otherwise it should stay out of the PR.Suggested hardening
+const path = require('path'); const fs = require('fs'); -const filePath = '/storage/emulated/0/yun/project/code-test/1/codexmate/web-ui/modules/app.computed.dashboard.mjs'; +const filePath = path.resolve(__dirname, 'web-ui/modules/app.computed.dashboard.mjs'); let content = fs.readFileSync(filePath, 'utf8'); @@ -content = content.replace(oldHeader, newHeader); +if (!content.includes(oldHeader)) { + throw new Error('Expected dashboard header not found'); +} +content = content.replace(oldHeader, newHeader); @@ -content = content.replace(oldMethod, newMethod); +if (!content.includes(oldMethod)) { + throw new Error('Expected dashboard method block not found'); +} +content = content.replace(oldMethod, newMethod);🤖 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 `@fix_dashboard_computed.js` around lines 2 - 17, The script currently uses a hardcoded absolute filePath and unsafe replace calls that can silently no-op or duplicate injections; change filePath resolution to use a repo-relative path (resolve from __dirname or process.cwd()) and validate the target file exists before proceeding, then read the file and assert the expected markers (oldHeader and oldMethod matching createDashboardComputed and displayProvidersList) are present — if not, exit with an error so the fixer fails fast; also perform idempotent insertion for the import and displayProviderUrl block (check for existing import getProviderDisplayUrl and existing displayProviderUrl method before injecting) to avoid duplicates when running multiple times.web-ui/modules/app.computed.dashboard.mjs.backup (1)
1-46: ⚡ Quick winRemove this checked-in backup snapshot.
This backup has already drifted away from the real module —
displayProvidersList()is split by the injected helper, so it is no longer a trustworthy fallback/reference. Keeping stale.backupsources beside the real implementation makes future edits easy to copy from the wrong place.🤖 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.dashboard.mjs.backup` around lines 1 - 46, Delete the checked-in stale backup snapshot that defines createDashboardComputed and contains the split displayProvidersList/displayProviderUrl helper (the file with the broken duplicate implementations of displayProvidersList and displayProviderUrl), since it has drifted from the real module and can be copied by mistake; remove that backup file from the repository so only the canonical module remains.
🤖 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 `@tmp_patch_add_provider.txt`:
- Around line 1-15: This is a scratch patch fragment that must not be committed
as-is: remove the tmp file and apply the logic into the real provider-add flow
(where provider creation happens) by placing the snippet inside the appropriate
method so it uses existing context (use the same this.showMessage(...) and
this.closeAddModal() calls), create the newProvider object exactly as shown,
preserve the conditional that sets newProvider.useTransform and
newProvider.codexmate_bridge, and call incrementallyAddProvider(this,
newProvider) from that method (ensuring the method that builds providers — which
currently references newProvider and incrementallyAddProvider — is updated
rather than keeping this tmp file).
In `@web-ui/modules/provider-url-display.mjs`:
- Around line 5-16: The getProviderDisplayUrl helper currently ignores an
upstream cache and has a dead branch; change its signature to
getProviderDisplayUrl(provider, upstreamUrlCache = {}) and in the built-in
transform branch (when bridge === 'openai') attempt to resolve and return the
upstream URL from the provided cache using a safe own-property check (e.g.
Object.prototype.hasOwnProperty.call(upstreamUrlCache, provider.id) ?
upstreamUrlCache[provider.id] : provider.url || ''), otherwise fall back to
provider.url; ensure you keep the existing safe trimming of
provider.codexmate_bridge and handle missing provider/id defensively.
---
Outside diff comments:
In `@add_transform_method.js`:
- Around line 1-12: Remove this file-based patcher and the hardcoded filesystem
path; do not commit scripts that mutate sources with absolute local paths. If
you need the isTransformProvider behavior in app.computed.dashboard.mjs, import
the exported helper from web-ui/modules/provider-url-display.mjs (use the
module's exported isTransformProvider) and call that inside the class instead of
inlining the predicate; delete the duplicated inlined implementation in your
addition and update any code that referenced the new method to delegate to the
imported isTransformProvider. Ensure any automation that modifies source files
uses relative/repo paths or build-time transforms rather than absolute device
paths.
In `@fix_dashboard_final.js`:
- Around line 1-23: This patcher script (fix_dashboard_final.js) should be
removed because it writes a hardcoded Android path and reintroduces a broken
two-argument call to getProviderDisplayUrl; delete the script and any
CI/packaging step that runs it, and ensure the real module
web-ui/modules/app.computed.dashboard.mjs retains the existing correct
single-argument call to getProviderDisplayUrl(provider) inside
createDashboardComputed's displayProviderUrl method (do not replace or inject a
call like getProviderDisplayUrl(provider, this.upstreamUrlCache || {})); if you
must automate fixes, update the automation to edit the actual module path and
only enforce the single-argument signature in displayProviderUrl instead of
injecting the two-argument variant.
In `@fix_startup_import.js`:
- Around line 1-12: This file fix_startup_import.js is a throwaway,
environment-specific patcher (uses hardcoded filePath and one-off replacement
logic via fs.readFileSync/fs.writeFileSync, brokenImport/fixedImport) and must
be removed from the PR; delete the file from the branch, revert any changes it
applied, and if you want to keep local fixer scripts, move them out of the repo
and add a .gitignore entry (e.g., ignore fix_*.js) so they are not committed; do
not integrate hardcoded filePath or one-off content.replace logic into the
codebase or CI.
In `@update_providers.sed`:
- Around line 1-7: The sed patch file should be removed and the transformation
applied directly to the source: edit the addProvider method in
web-ui/modules/app.methods.providers.mjs (the function that currently calls
this.showMessage(...); this.closeAddModal(); await this.loadAll();) to instead
create a newProvider object with properties name (validation.name), url
(validation.url), hasKey (Boolean of this.newProvider.key), readOnly:false,
nonEditable:false, and if this.newProvider && this.newProvider.useTransform set
newProvider.useTransform = true and newProvider.codexmate_bridge = 'openai';
then call incrementallyAddProvider(this, newProvider) after this.closeAddModal()
(replacing the await this.loadAll() call); finally delete the single-use sed
patch file (update_providers.sed) from the repo.
In `@web-ui/modules/app.methods.providers.mjs.backup2`:
- Around line 1-406: Remove all accidental backup files from the repo and add a
gitignore entry for the pattern *.backup*; in this specific backup file remove
the unused import list (incrementallyAddProvider, incrementallyUpdateProvider,
incrementallyRemoveProvider) or, if you intend to keep the incremental-refactor,
replace the post-operation await this.loadAll() calls in addProvider,
deleteProvider, and updateProvider with calls to incrementallyAddProvider(...),
incrementallyRemoveProvider(...), and incrementallyUpdateProvider(...)
respectively (passing the same payload/params and handling their responses),
then delete the backup file so no unused imports remain.
In `@web-ui/partials/index/panel-config-codex.html`:
- Around line 209-216: The card subtitle currently shows provider.url which
hides the upstream-URL when a transform provider is used; update the subtitle to
use the provider's resolved URL (e.g., provider.resolvedUrl) instead of
provider.url so the upstream-URL is visible for providers adapted via
isTransformProvider(provider): replace the usage of provider.url in the
card-subtitle with provider.resolvedUrl || t('config.url.unset') (or call the
existing helper that returns the resolved URL) while keeping the transform badge
logic intact.
---
Duplicate comments:
In `@web-ui/modules/app.methods.providers.mjs.backup`:
- Around line 1-406: This file is a stale backup snapshot that re-introduces
imports (incrementallyAddProvider, incrementallyUpdateProvider,
incrementallyRemoveProvider) but still uses full reloads (await this.loadAll())
instead of the incremental helpers; remove this .backup file (and its .backup2
sibling) from the repository and any references to it so the real module
(createProvidersMethods and its methods like addProvider, updateProvider,
deleteProvider, openEditModal) is the single source of truth; if there are any
accidental imports of this backup elsewhere, update them to import the real
module instead and ensure the incremental helper imports remain only in the real
module where they are used.
---
Nitpick comments:
In `@fix_dashboard_computed.js`:
- Around line 2-17: The script currently uses a hardcoded absolute filePath and
unsafe replace calls that can silently no-op or duplicate injections; change
filePath resolution to use a repo-relative path (resolve from __dirname or
process.cwd()) and validate the target file exists before proceeding, then read
the file and assert the expected markers (oldHeader and oldMethod matching
createDashboardComputed and displayProvidersList) are present — if not, exit
with an error so the fixer fails fast; also perform idempotent insertion for the
import and displayProviderUrl block (check for existing import
getProviderDisplayUrl and existing displayProviderUrl method before injecting)
to avoid duplicates when running multiple times.
In `@web-ui/modules/app.computed.dashboard.mjs.backup`:
- Around line 1-46: Delete the checked-in stale backup snapshot that defines
createDashboardComputed and contains the split
displayProvidersList/displayProviderUrl helper (the file with the broken
duplicate implementations of displayProvidersList and displayProviderUrl), since
it has drifted from the real module and can be copied by mistake; remove that
backup file from the repository so only the canonical module remains.
In `@web-ui/modules/app.methods.startup-claude.mjs.backup`:
- Around line 1-9: Delete the stale backup module file
web-ui/modules/app.methods.startup-claude.mjs.backup from the repo; this file
contains a half-applied import block (e.g., references like
prefetchUpstreamUrls, findDuplicateClaudeConfigName,
getClaudeModelCatalogForBaseUrl, matchClaudeConfigFromSettings,
normalizeClaudeConfig, normalizeClaudeSettingsEnv, normalizeClaudeValue) and
should not be kept—remove the file and ensure no build/config references point
to this backup so only the real app.methods.startup-claude.mjs remains in use.
🪄 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: e0e21b43-36de-4f32-864f-28a5beed62d4
📒 Files selected for processing (15)
add_transform_method.jsfix_dashboard_complete.jsfix_dashboard_computed.jsfix_dashboard_final.jsfix_startup_import.jstmp_patch_add_provider.txtupdate_providers.sedweb-ui/modules/app.computed.dashboard.mjsweb-ui/modules/app.computed.dashboard.mjs.backupweb-ui/modules/app.methods.providers.mjs.backupweb-ui/modules/app.methods.providers.mjs.backup2web-ui/modules/app.methods.startup-claude.mjs.backupweb-ui/modules/provider-url-display.mjsweb-ui/partials/index/panel-config-codex.htmlweb-ui/styles/titles-cards.css
✅ Files skipped from review due to trivial changes (2)
- fix_dashboard_complete.js
- web-ui/styles/titles-cards.css
SurviveM
force-pushed
the
feat/provider-management-optimization
branch
from
d33b6f2 to
bf5823d
Compare
ymkiux
changed the title
SurviveM
force-pushed
the
feat/provider-management-optimization
branch
from
bf5823d to
2dd74e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/web-ui-behavior-parity.test.mjs`:
- Around line 338-340: Remove the two computed-only keys from the data keys
drift allowlist: delete 'displayProviderUrl' and 'isTransformProvider' from the
array that currently includes '_taskOrchestrationPollTimer',
'displayProviderUrl', 'isTransformProvider' so they remain only in the computed
keys allowlist (where they are already declared); this ensures computed getters
displayProviderUrl and isTransformProvider are not treated as data keys.
In `@web-ui/styles/titles-cards.css`:
- Around line 414-429: The global rule hiding the transform indicator
(.provider-transform-icon { display: none; }) is overriding the visible styles
and preventing the icon from rendering; remove or change that global selector to
be context-scoped so the base .provider-transform-icon rules (display:
inline-flex, opacity, hover) can take effect—update the hiding rule to target a
specific parent modifier (e.g., .some-parent--hidden .provider-transform-icon)
or a context class so only that context hides the icon while leaving the default
.provider-transform-icon styles intact.
🪄 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: cd42bd91-6c89-4f27-aa86-ffece1cea449
📒 Files selected for processing (5)
tests/unit/web-ui-behavior-parity.test.mjsweb-ui/modules/app.computed.dashboard.mjsweb-ui/modules/provider-url-display.mjsweb-ui/partials/index/panel-config-codex.htmlweb-ui/styles/titles-cards.css
🚧 Files skipped from review as they are similar to previous changes (3)
- web-ui/modules/app.computed.dashboard.mjs
- web-ui/modules/provider-url-display.mjs
- web-ui/partials/index/panel-config-codex.html
| '_taskOrchestrationPollTimer', | ||
| 'displayProviderUrl', | ||
| 'isTransformProvider' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how displayProviderUrl and isTransformProvider are defined
# Search for these properties in data() functions
echo "=== Searching for properties in data() functions ==="
rg -n -A 3 -B 3 "data\s*\(\s*\)" --type=js --type=mjs -g 'web-ui/**' | rg -C 5 "(displayProviderUrl|isTransformProvider)"
# Search for these properties in computed definitions
echo -e "\n=== Searching for properties in computed definitions ==="
rg -n -A 3 -B 3 "computed\s*:" --type=js --type=mjs -g 'web-ui/**' | rg -C 5 "(displayProviderUrl|isTransformProvider)"
# Search for their definitions in dashboard computed module
echo -e "\n=== Searching in dashboard computed module ==="
fd -e js -e mjs "app.computed.dashboard" web-ui/ --exec cat {}Repository: SakuraByteCore/codexmate
Length of output: 8696
Remove displayProviderUrl and isTransformProvider from the data keys drift allowlist at lines 338-340.
These properties are implemented exclusively as computed properties in the dashboard computed module (they return functions wrapping utility functions from provider-url-display.mjs). They should not appear in the data keys drift allowlist—only in the computed keys allowlist (lines 634-636), where they are correctly listed.
Verification details
The dashboard computed module shows both properties implemented as computed getters:
displayProviderUrl() {
return (provider) => getProviderDisplayUrl(provider);
},
isTransformProvider() {
return (provider) => checkIsTransformProvider(provider);
},Search for these properties in data() definitions returned no results.
🤖 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/web-ui-behavior-parity.test.mjs` around lines 338 - 340, Remove
the two computed-only keys from the data keys drift allowlist: delete
'displayProviderUrl' and 'isTransformProvider' from the array that currently
includes '_taskOrchestrationPollTimer', 'displayProviderUrl',
'isTransformProvider' so they remain only in the computed keys allowlist (where
they are already declared); this ensures computed getters displayProviderUrl and
isTransformProvider are not treated as data keys.
| .provider-transform-icon { | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| margin-left: 6px; | ||
| font-size: 14px; | ||
| line-height: 1; | ||
| opacity: 0.6; | ||
| flex-shrink: 0; | ||
| cursor: help; | ||
| transition: opacity 0.2s ease; | ||
| } | ||
|
|
||
| .provider-transform-icon:hover { | ||
| opacity: 1; | ||
| } |
There was a problem hiding this comment.
Transform indicator is unintentionally suppressed by a later CSS override
Line 457 sets .provider-transform-icon { display: none; }, which overrides Line 414-425 and prevents the 🔌 indicator from ever rendering. This conflicts with the feature goal and results in missing UI state.
Suggested fix
.provider-transform-icon {
- display: none;
+ display: inline-flex;
}If hiding is needed in specific contexts, scope that rule to a parent modifier instead of the global selector.
Also applies to: 457-459
🤖 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/styles/titles-cards.css` around lines 414 - 429, The global rule
hiding the transform indicator (.provider-transform-icon { display: none; }) is
overriding the visible styles and preventing the icon from rendering; remove or
change that global selector to be context-scoped so the base
.provider-transform-icon rules (display: inline-flex, opacity, hover) can take
effect—update the hiding rule to target a specific parent modifier (e.g.,
.some-parent--hidden .provider-transform-icon) or a context class so only that
context hides the icon while leaving the default .provider-transform-icon styles
intact.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web-ui/partials/index/panel-config-codex.html (1)
212-212:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize and label the transform indicator.
Line 212 still hardcodes the tooltip text in Chinese and leaves the new indicator without an
aria-label, so the icon stays untranslated in non-zh UIs and is not announced clearly by assistive tech.💡 Minimal fix
-<div class="card-icon">{{ provider.name.charAt(0).toUpperCase() }}<span v-if="isTransformProvider(provider)" class="card-icon-dot" title="通过内建转换适配"></span></div> +<div class="card-icon">{{ provider.name.charAt(0).toUpperCase() }}<span v-if="isTransformProvider(provider)" class="card-icon-dot" :title="t('config.provider.transformHint')" :aria-label="t('config.provider.transformHint')"></span></div>🤖 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` at line 212, The transform indicator currently uses a hardcoded Chinese title and lacks an accessible label; update the element rendered in the provider card (the span with class "card-icon-dot" inside the provider card div and the isTransformProvider(provider) check) to use the app's localization function (e.g., $t or equivalent) for the tooltip text and add an aria-label attribute with the same localized string so assistive tech announces it; ensure the title attribute is replaced with the localized string and aria-label is present when isTransformProvider(provider) returns true.
🤖 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/modules/app.methods.codex-config.mjs`:
- Around line 188-195: The bug is that currentProvider is assigned before
calling activeProviderModel(name), causing activeProviderModel to read state for
the old provider and produce a misleading dictModel; fix by calling
activeProviderModel(name) (or otherwise resolving the target model) first, store
its result in a local (e.g., dictModel), then assign this.currentProvider = name
and only then set this.currentModel = dictModel if present; apply the same
reorder pattern to the other similar blocks (the ones that call
activeProviderModel and then mutate currentProvider/currentModel and/or call
applyCodexConfigDirect) and ensure applyCodexConfigDirect receives the model
resolved for the new provider rather than the previous provider’s model.
---
Duplicate comments:
In `@web-ui/partials/index/panel-config-codex.html`:
- Line 212: The transform indicator currently uses a hardcoded Chinese title and
lacks an accessible label; update the element rendered in the provider card (the
span with class "card-icon-dot" inside the provider card div and the
isTransformProvider(provider) check) to use the app's localization function
(e.g., $t or equivalent) for the tooltip text and add an aria-label attribute
with the same localized string so assistive tech announces it; ensure the title
attribute is replaced with the localized string and aria-label is present when
isTransformProvider(provider) returns true.
🪄 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: 491bb616-56b1-40c5-a90f-be5efc648083
📒 Files selected for processing (14)
cli.jstests/unit/provider-share-command.test.mjstests/unit/providers-validation.test.mjstests/unit/web-run-host.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/logic.codex.mjsweb-ui/modules/app.computed.dashboard.mjsweb-ui/modules/app.methods.codex-config.mjsweb-ui/modules/app.methods.providers.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-codex.html
✅ Files skipped from review due to trivial changes (2)
- web-ui/modules/i18n.dict.mjs
- tests/unit/web-ui-behavior-parity.test.mjs
📜 Review details
🔇 Additional comments (5)
web-ui/logic.codex.mjs (2)
30-42: Good defensive catalog resolution path.
getCodexModelCatalogForProvidersafely handles nullish/invalid providers and returns copied model arrays, which avoids accidental shared-state mutation.
46-54: Template presets are well-structured for the modal workflow.The exported
CODEX_PROVIDER_TEMPLATESshape is consistent and includes practical defaultmodelhints for quick provider setup.tests/unit/web-run-host.test.mjs (1)
1041-1043: Nice test harness alignment with status payload dependencies.Mocking
readCurrentModels()here keeps the health-check/status flow isolated and prevents unrelated failures as the status response evolves.tests/unit/provider-share-command.test.mjs (1)
625-636: Good coverage update for thestatusbranch contract.Adding the
readCurrentModelsstub plus the explicitcurrentModelsassertion makes this test robust against regressions in the API payload shape.cli.js (1)
10020-10020: Looks good: status payload now exposes per-provider model state.Adding
currentModelshere is a safe, additive API update and matches the per-provider model-selection flow.
| this.currentProvider = name; | ||
| // 立即按字典预填，让 UI 不出现空白；远端 /models 后台异步补齐。 | ||
| const dictModel = typeof this.activeProviderModel === 'function' | ||
| ? this.activeProviderModel(name) | ||
| : ''; | ||
| if (dictModel) { | ||
| this.currentModel = dictModel; | ||
| } |
There was a problem hiding this comment.
Resolve the target model before mutating currentProvider.
Line 188 updates this.currentProvider before activeProviderModel(name) runs, so the fallback branch reads the previous provider's this.currentModel as if it belonged to the target provider. That makes dictModel truthy even when no per-provider entry exists, which suppresses both remote fallback blocks and can push applyCodexConfigDirect() with the wrong model for the new provider.
💡 Safer ordering
- this.currentProvider = name;
- // 立即按字典预填，让 UI 不出现空白；远端 /models 后台异步补齐。
- const dictModel = typeof this.activeProviderModel === 'function'
- ? this.activeProviderModel(name)
- : '';
+ const dict = this.currentModels && typeof this.currentModels === 'object'
+ ? this.currentModels
+ : {};
+ const dictModel = typeof dict[name] === 'string' ? dict[name].trim() : '';
+ this.currentProvider = name;
if (dictModel) {
this.currentModel = dictModel;
+ } else {
+ this.currentModel = '';
}Also applies to: 210-214, 227-235
🤖 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.codex-config.mjs` around lines 188 - 195, The bug
is that currentProvider is assigned before calling activeProviderModel(name),
causing activeProviderModel to read state for the old provider and produce a
misleading dictModel; fix by calling activeProviderModel(name) (or otherwise
resolving the target model) first, store its result in a local (e.g.,
dictModel), then assign this.currentProvider = name and only then set
this.currentModel = dictModel if present; apply the same reorder pattern to the
other similar blocks (the ones that call activeProviderModel and then mutate
currentProvider/currentModel and/or call applyCodexConfigDirect) and ensure
applyCodexConfigDirect receives the model resolved for the new provider rather
than the previous provider’s model.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web-ui/partials/index/panel-config-codex.html (1)
236-236:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the transform indicator tooltip.
The hardcoded Chinese text
"通过内建转换适配"in thetitleattribute should be replaced with an i18n call, consistent with the rest of the template. This issue was previously flagged.💡 Suggested fix
-<div class="card-icon">{{ provider.name.charAt(0).toUpperCase() }}<span v-if="isTransformProvider(provider)" class="card-icon-dot" title="通过内建转换适配"></span></div> +<div class="card-icon">{{ provider.name.charAt(0).toUpperCase() }}<span + v-if="isTransformProvider(provider)" + class="card-icon-dot" + :title="t('config.provider.transformHint')" + :aria-label="t('config.provider.transformHint')" +></span></div>Ensure the i18n key exists in
web-ui/modules/i18n.dict.mjs.🤖 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` at line 236, The tooltip text is hardcoded in the title attribute of the transform indicator inside the template (the div rendering provider.name with isTransformProvider(provider) and class "card-icon-dot"); replace the literal Chinese string "通过内建转换适配" with an i18n lookup (use the same i18n helper used elsewhere in this template) so the title uses the localized key, and add the corresponding key/value to web-ui/modules/i18n.dict.mjs so translations are available.
🧹 Nitpick comments (1)
web-ui/logic.codex.mjs (1)
21-21: ⚡ Quick winStrengthen the MiniMax URL pattern.
The regex
/api\.minimax/iwill match any URL containing "api.minimax" as a substring, potentially matching unintended domains. Consider anchoring it with a domain suffix.🔒 Suggested fix
- { match: (u) => /api\.minimax/i.test(u), models: ['MiniMax-M2', 'abab6.5s-chat', 'abab6.5-chat'] }, + { match: (u) => /api\.minimax\./i.test(u), models: ['MiniMax-M2', 'abab6.5s-chat', 'abab6.5-chat'] },This ensures the pattern only matches when "api.minimax" is followed by a dot (domain separator).
🤖 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/logic.codex.mjs` at line 21, The URL matcher object containing match: (u) => /api\.minimax/i.test(u) is too permissive; update the regex used in that match function to require a domain separator after "api.minimax" (e.g., ensure "api.minimax" is followed by a dot) so it only matches real domain hostnames rather than any substring—locate the object with match: (u) => /api\.minimax/i.test(u) and replace the pattern with one that enforces a trailing dot (or equivalent domain boundary) such as requiring a "." after minimax.
🤖 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/logic.codex.mjs`:
- Around line 5-12: DEFAULT_OPENAI_CODEX_CATALOG contains invalid OpenAI model
identifiers causing API errors; update the frozen array entries in
logic.codex.mjs (symbol: DEFAULT_OPENAI_CODEX_CATALOG) to use the correct
current model names: replace gpt-5-codex with a valid codex variant (e.g.,
gpt-5.3-codex or gpt-5.1-codex), replace gpt-5 and gpt-5-mini with current
gpt-5.x names (e.g., gpt-5.5, gpt-5.5-pro, gpt-5.4, or gpt-5.4-mini), remove or
replace gpt-4.1 (no equivalent), change o3-mini to o3, and keep o4-mini; ensure
the final array only lists valid OpenAI model identifiers and freeze it as
before.
---
Duplicate comments:
In `@web-ui/partials/index/panel-config-codex.html`:
- Line 236: The tooltip text is hardcoded in the title attribute of the
transform indicator inside the template (the div rendering provider.name with
isTransformProvider(provider) and class "card-icon-dot"); replace the literal
Chinese string "通过内建转换适配" with an i18n lookup (use the same i18n helper used
elsewhere in this template) so the title uses the localized key, and add the
corresponding key/value to web-ui/modules/i18n.dict.mjs so translations are
available.
---
Nitpick comments:
In `@web-ui/logic.codex.mjs`:
- Line 21: The URL matcher object containing match: (u) =>
/api\.minimax/i.test(u) is too permissive; update the regex used in that match
function to require a domain separator after "api.minimax" (e.g., ensure
"api.minimax" is followed by a dot) so it only matches real domain hostnames
rather than any substring—locate the object with match: (u) =>
/api\.minimax/i.test(u) and replace the pattern with one that enforces a
trailing dot (or equivalent domain boundary) such as requiring a "." after
minimax.
🪄 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: 04ada682-2a37-400e-96e9-48e208eda07e
📒 Files selected for processing (3)
web-ui/logic.codex.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/panel-config-codex.html
✅ Files skipped from review due to trivial changes (1)
- web-ui/modules/i18n.dict.mjs
📜 Review details
🔇 Additional comments (6)
web-ui/logic.codex.mjs (3)
27-29: LGTM!Clean and simple URL normalization that safely handles edge cases.
31-43: LGTM!Well-structured function with proper validation, fallback logic, and defensive array spreading to avoid returning mutable references to frozen constants.
48-56: LGTM!The provider template structure is well-defined and properly frozen. The
useTransformflag clearly indicates transform providers.web-ui/partials/index/panel-config-codex.html (3)
41-63: LGTM!The service preset section is well-structured with proper i18n, conditional rendering, and clear UX flow. The template application logic correctly pre-fills the add-provider modal.
89-89: LGTM!Adding
@keyup.entersupport allows users to commit model changes by pressing Enter, improving keyboard navigation and UX.
243-243: LGTM!Using
displayProviderUrl(provider)centralizes the URL display logic and enables showing upstream URLs for built-in transform providers, as mentioned in the PR objectives.
| const DEFAULT_OPENAI_CODEX_CATALOG = Object.freeze([ | ||
| 'gpt-5-codex', | ||
| 'gpt-5', | ||
| 'gpt-5-mini', | ||
| 'gpt-4.1', | ||
| 'o4-mini', | ||
| 'o3-mini' | ||
| ]); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are the current valid OpenAI model names for code completion and reasoning models as of May 2026?
💡 Result:
Reasoning models (May 10, 2026, valid OpenAI API model IDs) - gpt-5.5 [1] - gpt-5.5-pro [1] - gpt-5.4 [1] - gpt-5.4-mini [1] - gpt-5.4-nano [1] - gpt-5.3-codex (coding/agentic coding family; also supports reasoning effort) [2] - o3 [3] - o4-mini [3] - o1 [3] Code completion / coding (Codex/agentic coding) models (May 10, 2026, valid OpenAI API model IDs) - gpt-5.3-codex [2] - gpt-5.2-codex [3] - gpt-5.1-codex [3] - gpt-5.1-codex-mini [4] - gpt-5.1-codex-max [3] - codex-mini-latest [3] How to verify “current valid” for your account - OpenAI provides a “List models” API (/v1/models); this is the definitive way to confirm which model IDs are available to your specific API project/account. [5][6]
Citations:
- 1: https://platform.openai.com/docs/guides/reasoning
- 2: https://developers.openai.com/api/docs/models/gpt-5.3-codex
- 3: https://developers.openai.com/api/docs/models/all
- 4: https://platform.openai.com/docs/models/gpt-5.1-codex-mini
- 5: https://platform.openai.com/docs/api-reference/models
- 6: https://platform.openai.com/docs/api-reference/models?_clear=true
Multiple invalid OpenAI model identifiers will cause API errors.
The array contains several model names that do not match OpenAI's current API:
gpt-5-codex→ should be one of:gpt-5.3-codex,gpt-5.2-codex,gpt-5.1-codex,gpt-5.1-codex-mini,gpt-5.1-codex-maxgpt-5→ should be one of:gpt-5.5,gpt-5.5-pro,gpt-5.4,gpt-5.4-mini,gpt-5.4-nanogpt-5-mini→ should begpt-5.4-minigpt-4.1→ not available; no equivalent in current OpenAI catalogo3-mini→ should beo3
Only o4-mini is valid. Update the catalog to use correct model identifiers from OpenAI's API.
🤖 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/logic.codex.mjs` around lines 5 - 12, DEFAULT_OPENAI_CODEX_CATALOG
contains invalid OpenAI model identifiers causing API errors; update the frozen
array entries in logic.codex.mjs (symbol: DEFAULT_OPENAI_CODEX_CATALOG) to use
the correct current model names: replace gpt-5-codex with a valid codex variant
(e.g., gpt-5.3-codex or gpt-5.1-codex), replace gpt-5 and gpt-5-mini with
current gpt-5.x names (e.g., gpt-5.5, gpt-5.5-pro, gpt-5.4, or gpt-5.4-mini),
remove or replace gpt-4.1 (no equivalent), change o3-mini to o3, and keep
o4-mini; ensure the final array only lists valid OpenAI model identifiers and
freeze it as before.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 12364-12404: The upstreamUrl is only computed when
codexmate_bridge === 'openai', so legacy providers that only expose a
bridge-style base_url never get an upstreamUrl; update the conditional that
calls resolveOpenaiBridgeUpstream to also detect legacy bridge-form base_url
values (e.g., when p.base_url is a non-empty string that looks like a bridge
URL) and call resolveOpenaiBridgeUpstream in that case. In practice, modify the
branch around resolveOpenaiBridgeUpstream (referencing
OPENAI_BRIDGE_SETTINGS_FILE, resolveOpenaiBridgeUpstream, p.base_url, bridge and
upstreamUrl) to run when bridge === 'openai' OR when p.base_url indicates a
legacy bridge, then preserve the same upstream.error/baseUrl checks and trim
logic to set upstreamUrl.
In `@web-ui/modules/provider-url-display.mjs`:
- Around line 1-9: getProviderDisplayUrl currently only checks codexmate_bridge
while checkIsTransformProvider also falls back to URL-based detection, causing a
mismatch where the UI shows the transform (🔌) but renders the bridge URL;
update getProviderDisplayUrl to use the same detection as
checkIsTransformProvider (either call checkIsTransformProvider(provider) or
duplicate its URL-based fallback logic) and when a provider is detected as a
transform provider prefer provider.upstreamUrl (trimmed) over provider.url,
otherwise return provider.url || ''; ensure you reference the functions
getProviderDisplayUrl and checkIsTransformProvider when making the change.
🪄 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: 54415fa9-6016-40c4-9c6a-739b81804834
📒 Files selected for processing (2)
cli.jsweb-ui/modules/provider-url-display.mjs
📜 Review details
🔇 Additional comments (2)
cli.js (1)
10017-10024: Looks good: status payload now includes provider-scoped model state.This is consistent with preserving per-provider model selections in the Web UI flow.
web-ui/modules/provider-url-display.mjs (1)
11-17: Transform-provider detection helper looks solid.Defensive input handling + trimmed bridge check + URL fallback is a good compatibility path.
| providers: Object.entries(providers).map(([name, p]) => { | ||
| const bridge = typeof p.codexmate_bridge === 'string' ? p.codexmate_bridge.trim() : ''; | ||
| let upstreamUrl = ''; | ||
| if (bridge === 'openai') { | ||
| const upstream = resolveOpenaiBridgeUpstream(OPENAI_BRIDGE_SETTINGS_FILE, name); | ||
| if (upstream && !upstream.error && typeof upstream.baseUrl === 'string') { | ||
| upstreamUrl = upstream.baseUrl.trim(); | ||
| } | ||
| } | ||
| return { | ||
| name, | ||
| url: p.base_url || '', | ||
| upstreamUrl, | ||
| codexmate_bridge: bridge, | ||
| key: maskKey(p.preferred_auth_method || ''), | ||
| hasKey: !!(p.preferred_auth_method && p.preferred_auth_method.trim()), | ||
| models: Array.isArray(p.models) | ||
| ? p.models | ||
| .filter((model) => model && typeof model === 'object' && !Array.isArray(model)) | ||
| .map((model) => ({ | ||
| id: typeof model.id === 'string' ? model.id : '', | ||
| name: typeof model.name === 'string' ? model.name : '', | ||
| cost: model.cost && typeof model.cost === 'object' && !Array.isArray(model.cost) | ||
| ? { | ||
| input: model.cost.input, | ||
| output: model.cost.output, | ||
| cacheRead: model.cost.cacheRead, | ||
| cacheWrite: model.cost.cacheWrite | ||
| } | ||
| : null, | ||
| contextWindow: model.contextWindow, | ||
| maxTokens: model.maxTokens | ||
| })) | ||
| .filter((model) => model.id) | ||
| : [], | ||
| current: name === current, | ||
| readOnly: isBuiltinManagedProvider(name), | ||
| nonDeletable: isNonDeletableProvider(name), | ||
| nonEditable: isNonEditableProvider(name) | ||
| }; | ||
| }) |
There was a problem hiding this comment.
Support legacy bridge detection when computing upstreamUrl.
At Line [12367], upstream resolution only runs when codexmate_bridge === 'openai'. Legacy transform providers that only have bridge-form base_url won’t expose upstreamUrl, so the new UI indicator can disappear for existing users.
🔧 Proposed fix
- providers: Object.entries(providers).map(([name, p]) => {
- const bridge = typeof p.codexmate_bridge === 'string' ? p.codexmate_bridge.trim() : '';
+ providers: Object.entries(providers).map(([name, p]) => {
+ const bridge = typeof p.codexmate_bridge === 'string' ? p.codexmate_bridge.trim() : '';
+ const isOpenaiBridgeProvider = bridge === 'openai'
+ || (typeof p.base_url === 'string' && p.base_url.includes('/bridge/openai/'));
let upstreamUrl = '';
- if (bridge === 'openai') {
+ if (isOpenaiBridgeProvider) {
const upstream = resolveOpenaiBridgeUpstream(OPENAI_BRIDGE_SETTINGS_FILE, name);
if (upstream && !upstream.error && typeof upstream.baseUrl === 'string') {
upstreamUrl = upstream.baseUrl.trim();
}
}
return {
name,
url: p.base_url || '',
upstreamUrl,
- codexmate_bridge: bridge,
+ codexmate_bridge: bridge || (isOpenaiBridgeProvider ? 'openai' : ''),📝 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.
| providers: Object.entries(providers).map(([name, p]) => { | |
| const bridge = typeof p.codexmate_bridge === 'string' ? p.codexmate_bridge.trim() : ''; | |
| let upstreamUrl = ''; | |
| if (bridge === 'openai') { | |
| const upstream = resolveOpenaiBridgeUpstream(OPENAI_BRIDGE_SETTINGS_FILE, name); | |
| if (upstream && !upstream.error && typeof upstream.baseUrl === 'string') { | |
| upstreamUrl = upstream.baseUrl.trim(); | |
| } | |
| } | |
| return { | |
| name, | |
| url: p.base_url || '', | |
| upstreamUrl, | |
| codexmate_bridge: bridge, | |
| key: maskKey(p.preferred_auth_method || ''), | |
| hasKey: !!(p.preferred_auth_method && p.preferred_auth_method.trim()), | |
| models: Array.isArray(p.models) | |
| ? p.models | |
| .filter((model) => model && typeof model === 'object' && !Array.isArray(model)) | |
| .map((model) => ({ | |
| id: typeof model.id === 'string' ? model.id : '', | |
| name: typeof model.name === 'string' ? model.name : '', | |
| cost: model.cost && typeof model.cost === 'object' && !Array.isArray(model.cost) | |
| ? { | |
| input: model.cost.input, | |
| output: model.cost.output, | |
| cacheRead: model.cost.cacheRead, | |
| cacheWrite: model.cost.cacheWrite | |
| } | |
| : null, | |
| contextWindow: model.contextWindow, | |
| maxTokens: model.maxTokens | |
| })) | |
| .filter((model) => model.id) | |
| : [], | |
| current: name === current, | |
| readOnly: isBuiltinManagedProvider(name), | |
| nonDeletable: isNonDeletableProvider(name), | |
| nonEditable: isNonEditableProvider(name) | |
| }; | |
| }) | |
| providers: Object.entries(providers).map(([name, p]) => { | |
| const bridge = typeof p.codexmate_bridge === 'string' ? p.codexmate_bridge.trim() : ''; | |
| const isOpenaiBridgeProvider = bridge === 'openai' | |
| || (typeof p.base_url === 'string' && p.base_url.includes('/bridge/openai/')); | |
| let upstreamUrl = ''; | |
| if (isOpenaiBridgeProvider) { | |
| const upstream = resolveOpenaiBridgeUpstream(OPENAI_BRIDGE_SETTINGS_FILE, name); | |
| if (upstream && !upstream.error && typeof upstream.baseUrl === 'string') { | |
| upstreamUrl = upstream.baseUrl.trim(); | |
| } | |
| } | |
| return { | |
| name, | |
| url: p.base_url || '', | |
| upstreamUrl, | |
| codexmate_bridge: bridge || (isOpenaiBridgeProvider ? 'openai' : ''), | |
| key: maskKey(p.preferred_auth_method || ''), | |
| hasKey: !!(p.preferred_auth_method && p.preferred_auth_method.trim()), | |
| models: Array.isArray(p.models) | |
| ? p.models | |
| .filter((model) => model && typeof model === 'object' && !Array.isArray(model)) | |
| .map((model) => ({ | |
| id: typeof model.id === 'string' ? model.id : '', | |
| name: typeof model.name === 'string' ? model.name : '', | |
| cost: model.cost && typeof model.cost === 'object' && !Array.isArray(model.cost) | |
| ? { | |
| input: model.cost.input, | |
| output: model.cost.output, | |
| cacheRead: model.cost.cacheRead, | |
| cacheWrite: model.cost.cacheWrite | |
| } | |
| : null, | |
| contextWindow: model.contextWindow, | |
| maxTokens: model.maxTokens | |
| })) | |
| .filter((model) => model.id) | |
| : [], | |
| current: name === current, | |
| readOnly: isBuiltinManagedProvider(name), | |
| nonDeletable: isNonDeletableProvider(name), | |
| nonEditable: isNonEditableProvider(name) | |
| }; | |
| }) |
🤖 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 12364 - 12404, The upstreamUrl is only computed when
codexmate_bridge === 'openai', so legacy providers that only expose a
bridge-style base_url never get an upstreamUrl; update the conditional that
calls resolveOpenaiBridgeUpstream to also detect legacy bridge-form base_url
values (e.g., when p.base_url is a non-empty string that looks like a bridge
URL) and call resolveOpenaiBridgeUpstream in that case. In practice, modify the
branch around resolveOpenaiBridgeUpstream (referencing
OPENAI_BRIDGE_SETTINGS_FILE, resolveOpenaiBridgeUpstream, p.base_url, bridge and
upstreamUrl) to run when bridge === 'openai' OR when p.base_url indicates a
legacy bridge, then preserve the same upstream.error/baseUrl checks and trim
logic to set upstreamUrl.
| export function getProviderDisplayUrl(provider) { | ||
| if (!provider) return ''; | ||
| const bridge = typeof provider.codexmate_bridge === 'string' ? provider.codexmate_bridge.trim() : ''; | ||
| if (bridge === 'openai') { | ||
| const upstream = typeof provider.upstreamUrl === 'string' ? provider.upstreamUrl.trim() : ''; | ||
| if (upstream) return upstream; | ||
| } | ||
| return provider.url || ''; | ||
| } |
There was a problem hiding this comment.
Unify transform detection between icon logic and display URL resolution.
checkIsTransformProvider has a URL-based fallback, but getProviderDisplayUrl only checks codexmate_bridge. This can show the 🔌 indicator while still rendering the bridge URL instead of upstreamUrl for fallback-detected transform providers.
💡 Minimal alignment patch
export function getProviderDisplayUrl(provider) {
if (!provider) return '';
- const bridge = typeof provider.codexmate_bridge === 'string' ? provider.codexmate_bridge.trim() : '';
- if (bridge === 'openai') {
+ if (checkIsTransformProvider(provider)) {
const upstream = typeof provider.upstreamUrl === 'string' ? provider.upstreamUrl.trim() : '';
if (upstream) return upstream;
}
return 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/provider-url-display.mjs` around lines 1 - 9,
getProviderDisplayUrl currently only checks codexmate_bridge while
checkIsTransformProvider also falls back to URL-based detection, causing a
mismatch where the UI shows the transform (🔌) but renders the bridge URL;
update getProviderDisplayUrl to use the same detection as
checkIsTransformProvider (either call checkIsTransformProvider(provider) or
duplicate its URL-based fallback logic) and when a provider is detected as a
transform provider prefer provider.upstreamUrl (trimmed) over provider.url,
otherwise return provider.url || ''; ensure you reference the functions
getProviderDisplayUrl and checkIsTransformProvider when making the change.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web-ui/styles/titles-cards.css (1)
427-438:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTransform icon is still globally hidden by the later override.
display: noneat Line 471 overrides the visible style and suppresses the 🔌 indicator everywhere, which contradicts the feature goal. Scope the hiding rule to a specific parent state instead of the base selector.Proposed minimal fix
-.provider-transform-icon { - display: none; -} +/* If needed, hide only in a specific context */ +.some-context-that-hides-transform-icon .provider-transform-icon { + display: none; +}Also applies to: 470-472
🤖 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/styles/titles-cards.css` around lines 427 - 438, The .provider-transform-icon rule is being negated by a later generic display:none override; instead of hiding the icon globally, change that later selector to target a specific parent state/class/attribute (e.g., a collapsed/hidden provider like .provider--collapsed or a data attribute) so the base .provider-transform-icon (display:inline-flex) remains visible by default; locate the later override for .provider-transform-icon and replace its global selector with a scoped selector such as a parent modifier (e.g., .provider--collapsed .provider-transform-icon or .provider[data-hide-transform="true"] .provider-transform-icon) to only hide the icon when the parent is in that state.
🤖 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`:
- Around line 152-153: Do not render raw config.baseUrl in the subtitle; instead
pass it through the shared URL display helper (the app's sanitized/masked URL
formatter) before rendering so tokenized query params/credentials are masked.
Locate the JSX/templating line that outputs config.baseUrl (the element with
class card-subtitle-url) and replace the direct interpolation with a call to the
shared sanitizer/display helper (the URL display helper used elsewhere in the
UI) so the subtitle shows the masked/sanitized URL.
---
Duplicate comments:
In `@web-ui/styles/titles-cards.css`:
- Around line 427-438: The .provider-transform-icon rule is being negated by a
later generic display:none override; instead of hiding the icon globally, change
that later selector to target a specific parent state/class/attribute (e.g., a
collapsed/hidden provider like .provider--collapsed or a data attribute) so the
base .provider-transform-icon (display:inline-flex) remains visible by default;
locate the later override for .provider-transform-icon and replace its global
selector with a scoped selector such as a parent modifier (e.g.,
.provider--collapsed .provider-transform-icon or
.provider[data-hide-transform="true"] .provider-transform-icon) to only hide the
icon when the parent is in that state.
🪄 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: 72d7e1cd-3f7c-40f2-8951-545a00ada54d
📒 Files selected for processing (4)
web-ui/modules/i18n.dict.mjsweb-ui/partials/index/panel-config-claude.htmlweb-ui/partials/index/panel-config-codex.htmlweb-ui/styles/titles-cards.css
🚧 Files skipped from review as they are similar to previous changes (2)
- web-ui/modules/i18n.dict.mjs
- web-ui/partials/index/panel-config-codex.html
📜 Review details
🔇 Additional comments (2)
web-ui/styles/titles-cards.css (1)
246-257: Nice subtitle hierarchy for model vs URL.The split between
.card-subtitle-modeland.card-subtitle-urlimproves scanability without adding visual noise.web-ui/partials/index/panel-config-claude.html (1)
152-153: Good improvement to card metadata visibility.Showing model and endpoint on separate lines makes provider cards much easier to compare quickly.
| <div class="card-subtitle card-subtitle-model">{{ config.model || t('claude.model.unset') }}</div> | ||
| <div class="card-subtitle card-subtitle-url" v-if="config.baseUrl">{{ config.baseUrl }}</div> |
There was a problem hiding this comment.
Avoid rendering raw provider URLs directly in the card subtitle.
At Line 153, printing config.baseUrl as-is can leak credentials/tokenized query params in the UI. Use the shared URL display helper (sanitized/masked) before rendering.
Proposed fix
-<div class="card-subtitle card-subtitle-url" v-if="config.baseUrl">{{ config.baseUrl }}</div>
+<div class="card-subtitle card-subtitle-url" v-if="config.baseUrl">{{ displayProviderUrl(config.baseUrl) }}</div>📝 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.
| <div class="card-subtitle card-subtitle-model">{{ config.model || t('claude.model.unset') }}</div> | |
| <div class="card-subtitle card-subtitle-url" v-if="config.baseUrl">{{ config.baseUrl }}</div> | |
| <div class="card-subtitle card-subtitle-model">{{ config.model || t('claude.model.unset') }}</div> | |
| <div class="card-subtitle card-subtitle-url" v-if="config.baseUrl">{{ displayProviderUrl(config.baseUrl) }}</div> |
🤖 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` around lines 152 - 153, Do
not render raw config.baseUrl in the subtitle; instead pass it through the
shared URL display helper (the app's sanitized/masked URL formatter) before
rendering so tokenized query params/credentials are masked. Locate the
JSX/templating line that outputs config.baseUrl (the element with class
card-subtitle-url) and replace the direct interpolation with a call to the
shared sanitizer/display helper (the URL display helper used elsewhere in the
UI) so the subtitle shows the masked/sanitized URL.
…ume command Co-Authored-By: ymkiux <ymkiux@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf355fb1-8e74-4685-9a13-432fa5976d63
📒 Files selected for processing (3)
web-ui/modules/app.methods.session-actions.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/panel-config-codex.html
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/partials/index/panel-config-codex.html
📜 Review details
🔇 Additional comments (1)
web-ui/modules/i18n.dict.mjs (1)
706-706: LGTM! Clean i18n additions.The new dictionary keys properly support the provider preset UI and model selector features. Both language translations are accurate, follow the existing naming conventions, and maintain structural consistency with the rest of the file.
Also applies to: 742-742, 1764-1764, 1800-1800
| } | ||
| if (source === 'claude') { | ||
| return `claude -r ${arg}`; | ||
| return `claude --dangerously-skip-permissions -r ${arg}`; |
There was a problem hiding this comment.
Do not force --dangerously-skip-permissions in copied Claude resume commands.
At Line 184, this now always disables Claude permission prompts, which weakens safety by default. Please gate this behind an explicit user opt-in (default off), similar to how codex --yolo is handled.
Suggested fix
if (source === 'claude') {
- return `claude --dangerously-skip-permissions -r ${arg}`;
+ if (this.sessionResumeWithDangerousSkipPermissions) {
+ return `claude --dangerously-skip-permissions -r ${arg}`;
+ }
+ return `claude -r ${arg}`;
}
ymkiux
force-pushed
the
feat/provider-management-optimization
branch
from
dbe60c3 to
e777d28
Compare
…sions to resume commands - Remove yolo checkbox, codex resume always uses --yolo - Claude resume always uses --dangerously-skip-permissions - Consolidate settings tabs from 3 (backup/trash/device) to 2 (general/data) Co-Authored-By: ymkiux <ymkiux@users.noreply.github.com>
ymkiux
force-pushed
the
feat/provider-management-optimization
branch
2 times, most recently
from
cbe0357 to
7bbe18e
Compare
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 `@tools/dev/reset-and-dev.js`:
- Around line 5-8: The script builds resetCmd from process.argv[2] and passes it
to execSync, allowing shell injection; fix by validating/sanitizing the CLI arg
or, better, avoid the shell by calling a non-shell API that accepts argv arrays
(e.g., replace building resetCmd and the execSync(resetCmd, ...) call with a
child_process call that takes the command and args array or execFile/
spawnSync), ensure the argument only matches an allowed pattern (whitelist like
/^dev|all|db$/ or similar) before use, and update references to resetCmd and
execSync to use the safe invocation so arbitrary shell metacharacters cannot be
injected.
🪄 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: ba21794a-b683-4dd6-8776-db711bb7bdf0
📒 Files selected for processing (32)
cli.jspackage.jsonplugins/prompt-templates/issue-inject/index.mjsplugins/prompt-templates/overview.mjsplugins/prompt-templates/ownership.mjstests/e2e/test-web-ui-assets.jstests/unit/config-tabs-ui.test.mjstests/unit/provider-share-command.test.mjstests/unit/providers-validation.test.mjstests/unit/session-resume-command.test.mjstests/unit/session-tab-switch-performance.test.mjstests/unit/web-run-host.test.mjstests/unit/web-ui-behavior-parity.test.mjstests/unit/web-ui-source-bundle.test.mjstools/dev/reset-and-dev.jsweb-ui/app.jsweb-ui/logic.codex.mjsweb-ui/modules/app.computed.dashboard.mjsweb-ui/modules/app.methods.codex-config.mjsweb-ui/modules/app.methods.providers.mjsweb-ui/modules/app.methods.session-actions.mjsweb-ui/modules/app.methods.session-browser.mjsweb-ui/modules/app.methods.session-trash.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/modules/i18n.dict.mjsweb-ui/modules/provider-url-display.mjsweb-ui/partials/index/panel-config-claude.htmlweb-ui/partials/index/panel-config-codex.htmlweb-ui/partials/index/panel-sessions.htmlweb-ui/partials/index/panel-settings.htmlweb-ui/session-helpers.mjsweb-ui/styles/titles-cards.css
💤 Files with no reviewable changes (4)
- web-ui/modules/app.methods.session-browser.mjs
- plugins/prompt-templates/ownership.mjs
- plugins/prompt-templates/issue-inject/index.mjs
- web-ui/partials/index/panel-sessions.html
✅ Files skipped from review due to trivial changes (6)
- tests/unit/session-tab-switch-performance.test.mjs
- tests/unit/web-run-host.test.mjs
- web-ui/app.js
- tests/e2e/test-web-ui-assets.js
- tests/unit/provider-share-command.test.mjs
- package.json
🚧 Files skipped from review as they are similar to previous changes (20)
- web-ui/modules/app.methods.session-actions.mjs
- web-ui/partials/index/panel-config-claude.html
- tests/unit/providers-validation.test.mjs
- tests/unit/web-ui-source-bundle.test.mjs
- web-ui/modules/app.methods.startup-claude.mjs
- web-ui/modules/app.methods.providers.mjs
- web-ui/modules/app.computed.dashboard.mjs
- tests/unit/web-ui-behavior-parity.test.mjs
- web-ui/partials/index/panel-config-codex.html
- web-ui/modules/provider-url-display.mjs
- web-ui/modules/app.methods.session-trash.mjs
- cli.js
- tests/unit/session-resume-command.test.mjs
- web-ui/modules/i18n.dict.mjs
- web-ui/styles/titles-cards.css
- web-ui/session-helpers.mjs
- web-ui/logic.codex.mjs
- web-ui/modules/app.methods.codex-config.mjs
- web-ui/partials/index/panel-settings.html
- tests/unit/config-tabs-ui.test.mjs
📜 Review details
🧰 Additional context used
🪛 OpenGrep (1.20.0)
tools/dev/reset-and-dev.js
[ERROR] 8-8: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (5)
plugins/prompt-templates/overview.mjs (3)
34-37: LGTM! Fallback logic handles removed template gracefully.The reduction from 3 to 2 built-in templates is handled safely. The existing logic at lines 56-62 ensures that if a user previously selected
builtin_issue_inject, the system will gracefully fall back tobuiltin_comment_polish.
34-37: Inconsistency: Template removal not mentioned in PR objectives.This change removes the
builtin_issue_injecttemplate from the built-in templates list, which is a user-facing change. However, neither the PR title, description, nor commit messages mention this removal. Consider updating the PR description to document this change.
9-9: ⚡ Quick winRemoval is complete—no dangling references found.
The
buildBuiltinIssueInjectTemplateimport andbuiltin_issue_injecttemplate have been cleanly removed with no remaining references in the codebase.tools/dev/reset-and-dev.js (2)
1-3: LGTM!The shebang and import are correct for a Node.js CLI helper script.
9-9: LGTM!The dev command execution is safe (hardcoded string) and appropriately inherits stdio for terminal output.
| const arg = process.argv[2]; | ||
|
|
||
| const resetCmd = arg ? `npm run reset -- ${arg}` : 'npm run reset'; | ||
| execSync(resetCmd, { stdio: 'inherit' }); |
There was a problem hiding this comment.
Command injection vulnerability: sanitize or validate the CLI argument.
User input from process.argv[2] is directly interpolated into the shell command without validation. This allows command injection if malicious input is provided (e.g., ; malicious-command or $(rm -rf /)).
While this is a dev tool likely used only by trusted developers, it's still a security risk and poor practice.
🔒 Proposed fix using input validation
const arg = process.argv[2];
-const resetCmd = arg ? `npm run reset -- ${arg}` : 'npm run reset';
-execSync(resetCmd, { stdio: 'inherit' });
+if (arg) {
+ // Validate that arg doesn't contain shell metacharacters
+ if (/[;&|`$(){}[\]<>]/.test(arg)) {
+ console.error('Error: Argument contains invalid characters');
+ process.exit(1);
+ }
+ execSync(`npm run reset -- ${arg}`, { stdio: 'inherit' });
+} else {
+ execSync('npm run reset', { stdio: 'inherit' });
+}Alternatively, use spawn with an argument array for safer execution:
-const { execSync } = require('child_process');
+const { spawnSync } = require('child_process');
const arg = process.argv[2];
-const resetCmd = arg ? `npm run reset -- ${arg}` : 'npm run reset';
-execSync(resetCmd, { stdio: 'inherit' });
-execSync('npm run dev', { stdio: 'inherit' });
+const resetArgs = arg ? ['run', 'reset', '--', arg] : ['run', 'reset'];
+spawnSync('npm', resetArgs, { stdio: 'inherit' });
+spawnSync('npm', ['run', 'dev'], { stdio: 'inherit' });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const arg = process.argv[2]; | |
| const resetCmd = arg ? `npm run reset -- ${arg}` : 'npm run reset'; | |
| execSync(resetCmd, { stdio: 'inherit' }); | |
| const arg = process.argv[2]; | |
| if (arg) { | |
| // Validate that arg doesn't contain shell metacharacters | |
| if (/[;&|`$(){}[\]<>]/.test(arg)) { | |
| console.error('Error: Argument contains invalid characters'); | |
| process.exit(1); | |
| } | |
| execSync(`npm run reset -- ${arg}`, { stdio: 'inherit' }); | |
| } else { | |
| execSync('npm run reset', { stdio: 'inherit' }); | |
| } |
🧰 Tools
🪛 OpenGrep (1.20.0)
[ERROR] 8-8: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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 `@tools/dev/reset-and-dev.js` around lines 5 - 8, The script builds resetCmd
from process.argv[2] and passes it to execSync, allowing shell injection; fix by
validating/sanitizing the CLI arg or, better, avoid the shell by calling a
non-shell API that accepts argv arrays (e.g., replace building resetCmd and the
execSync(resetCmd, ...) call with a child_process call that takes the command
and args array or execFile/ spawnSync), ensure the argument only matches an
allowed pattern (whitelist like /^dev|all|db$/ or similar) before use, and
update references to resetCmd and execSync to use the safe invocation so
arbitrary shell metacharacters cannot be injected.
fe3d063 to
7a4c592
Compare
ymkiux
changed the title
ymkiux
changed the title
ymkiux
force-pushed
the
feat/provider-management-optimization
branch
from
b706174 to
903824f
Compare
1 participant
Summary
Provider management overhaul with presets, model memory, settings redesign, dev scripts, README tagline update, and version bump to 0.0.28.
Technical changes
Tests
Summary by CodeRabbit
Release Notes
New Features
Improvements